Rethinking the release pipeline and automation scripts#2065
Conversation
…l, and PowerShell download script Agent-Logs-Url: https://github.com/Belphemur/SoundSwitch/sessions/37ff5235-77dc-44c8-942a-1ddc84b44440 Co-authored-by: Belphemur <197810+Belphemur@users.noreply.github.com>
Agent-Logs-Url: https://github.com/Belphemur/SoundSwitch/sessions/37ff5235-77dc-44c8-942a-1ddc84b44440 Co-authored-by: Belphemur <197810+Belphemur@users.noreply.github.com>
|
@copilot we should have the framework version auto detected |
…roj TargetFramework Agent-Logs-Url: https://github.com/Belphemur/SoundSwitch/sessions/993a5459-0111-4668-8fcf-339ee4abe14a Co-authored-by: Belphemur <197810+Belphemur@users.noreply.github.com>
Done in |
|
@coderabbitai full review |
There was a problem hiding this comment.
Pull request overview
Updates the release/build automation by replacing the prior Markdown-to-HTML npm dependency with a Python-based converter, and restructuring CI artifacts to produce a consistent Final\ package that can be zipped and re-used for signing/installer creation.
Changes:
- Add
tools/markdown_to_html.pyand wire it into local (Make.bat) and CI workflows to generateReadme.html,Changelog.html, andTerms.html. - Add
tools/Download-Release.ps1to fetch and extract the latest release/beta artifact intoFinal\for local signing + installer creation. - Rework CI packaging to publish into
Final\, upload it as an artifact, and (for releases) zip and upload it to the GitHub Release.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| tools/markdown_to_html.py | New Python Markdown→standalone HTML converter used by build/release pipeline. |
| tools/README.md | Documents the new/updated tooling scripts. |
| tools/Download-Release.ps1 | New script to download and extract latest release/beta artifact for local packaging/signing. |
| Make.bat | Switch HTML generation to Python tool; add “skip build if Final already populated” path and signing step. |
| .github/workflows/test-installer-build.yml | Generate real HTML assets in CI using Python/markdown instead of dummy HTML. |
| .github/workflows/reusable-build.yml | Publish outputs into Final\, generate HTML docs, bundle assets, and upload Final\* as artifact. |
| .github/workflows/release.yml | Enable publish mode in reusable build; download artifact, zip it, and upload to GitHub Release. |
| .github/workflows/copilot-setup-steps.yml | Adds a dedicated Copilot setup workflow (checkout, .NET restore, Python markdown install). |
| if exist "%finalDir%\SoundSwitch.exe" ( | ||
| echo. | ||
| echo Final\ directory already contains binaries. |
There was a problem hiding this comment.
The early-exit when %finalDir%\SoundSwitch.exe exists skips generating/copying installer assets (e.g., Readme.html / Changelog.html). If Final\ is only partially populated, the installer will still be built but will reference missing files (see Installer/setup.iss [Run] entries for Readme.html/Changelog.html). Consider either (1) checking for the required HTML files as well before skipping, or (2) still running the HTML generation/copy steps when binaries already exist.
| if exist "%finalDir%\SoundSwitch.exe" ( | |
| echo. | |
| echo Final\ directory already contains binaries. | |
| if exist "%finalDir%\SoundSwitch.exe" if exist "%finalDir%\Readme.html" if exist "%finalDir%\Changelog.html" ( | |
| echo. | |
| echo Final\ directory already contains binaries and installer assets. |
✅ Actions performedFull review triggered. |
|
Important Review skippedBot user detected. To trigger a single review, invoke the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR introduces Python-based Markdown-to-HTML conversion tooling, new GitHub Actions workflows for automated setup and release processes, and a PowerShell script for downloading releases. It updates existing build workflows and the local build script ( Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Note Docstrings generation - SUCCESS |
Docstrings generation was requested by @Belphemur. * #2065 (comment) The following files were modified: * `tools/markdown_to_html.py`
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
tools/markdown_to_html.py (1)
85-89: Consider escaping thetitleparameter to prevent HTML injection.The
titleis inserted directly into the HTML template without escaping. While the title is derived from filenames in the current usage, future callers could pass untrusted input.🛡️ Proposed fix to escape the title
+import html + def convert(source: str, title: str = "") -> str: """Return a full HTML document from *source* Markdown text.""" md = markdown.Markdown(extensions=_EXTENSIONS, output_format="html") body = md.convert(source) - return _HTML_TEMPLATE.format(title=title, body=body) + return _HTML_TEMPLATE.format(title=html.escape(title), body=body)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/markdown_to_html.py` around lines 85 - 89, The convert function inserts title into _HTML_TEMPLATE without escaping, risking HTML injection; update convert to escape the title before formatting by applying an HTML-escaping routine (e.g., html.escape) to the title parameter and then pass the escaped value to _HTML_TEMPLATE.format(title=escaped_title, body=body); keep all other behavior the same and reference the convert function and _HTML_TEMPLATE when making the change.Make.bat (1)
17-20: Static analysis flag: Potential command injection via%buildPlatform%.The
buildPlatformvariable is set from user input (%~1) and used directly indotnet publishcommands. While this is a local build script and exploitation requires malicious local access, sanitizing the input would be more defensive.🛡️ Proposed input validation
set buildPlatform=Release if "%~1" neq "" ( - set buildPlatform=%~1 + rem Validate build configuration + if /i "%~1"=="Release" ( + set buildPlatform=Release + ) else if /i "%~1"=="Debug" ( + set buildPlatform=Debug + ) else ( + echo ERROR: Invalid build configuration "%~1". Use Release or Debug. + exit /b 1 + ) )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Make.bat` around lines 17 - 20, The Make.bat uses unvalidated user input (%~1) to set buildPlatform and later pass it to dotnet publish; sanitize this by whitelisting allowed values (e.g., "Release" and "Debug") in the Make.bat script: when %~1 is provided, compare it against the allowed set and only assign to buildPlatform if it matches, otherwise fallback to the default "Release" and optionally print a warning; ensure all uses of buildPlatform (dotnet publish invocations) then use this validated variable..github/workflows/reusable-build.yml (1)
184-185: Minor: Consider using forward slashes for cross-platform consistency.While this runs on Windows and backslashes work, using
Final/*would be more conventional in YAML and works on both platforms.📝 Suggested change
with: name: ${{ inputs.publish-artifact-name }} - path: Final\* + path: Final/*🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/reusable-build.yml around lines 184 - 185, Replace the Windows-style backslash path in the workflow artifact upload step: locate the stanza containing "name: ${{ inputs.publish-artifact-name }}" and the "path: Final\*" entry and change the path to use a forward slash ("Final/*") so the YAML is cross-platform and conventional; update any matching inputs or references if they assume the backslash form.tools/Download-Release.ps1 (1)
146-150: Consider adding a confirmation or backup before deleting the output directory.The script unconditionally removes the entire
OutputDirif it exists. This could lead to accidental data loss if the user specifies an incorrect path. Consider adding a confirmation prompt or at minimum documenting this behavior prominently.📝 Proposed enhancement to warn before deletion
# Prepare output directory $OutputDir = [System.IO.Path]::GetFullPath($OutputDir) if (Test-Path $OutputDir) { - Write-Host "Cleaning existing output directory: $OutputDir" - Remove-Item $OutputDir -Recurse -Force + Write-Warning "Output directory exists and will be deleted: $OutputDir" + $existingFiles = (Get-ChildItem $OutputDir -Recurse -File).Count + if ($existingFiles -gt 0) { + Write-Host " ($existingFiles existing files will be removed)" + } + Remove-Item $OutputDir -Recurse -Force }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/Download-Release.ps1` around lines 146 - 150, The script currently unconditionally deletes the OutputDir using Test-Path and Remove-Item which risks accidental data loss; update the block around Test-Path/Remove-Item to either prompt the user for confirmation (e.g., ask to proceed, with a --yes/--force override) or create a timestamped backup (e.g., move the existing OutputDir to a backup folder) before removal, and ensure New-Item still creates the directory afterward; reference the OutputDir variable and the Remove-Item/New-Item operations when making the change so behavior is explicit and reversible.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Make.bat`:
- Around line 74-78: The else fallback in the Make.bat script writes dummy
Changelog.html and Readme.html to %finalDir% but omits Terms.html; update the
else block to also output a dummy Terms.html into %finalDir% (same HTML
structure/message style as the other two) so downstream consumers always find
%finalDir%\Terms.html.
---
Nitpick comments:
In @.github/workflows/reusable-build.yml:
- Around line 184-185: Replace the Windows-style backslash path in the workflow
artifact upload step: locate the stanza containing "name: ${{
inputs.publish-artifact-name }}" and the "path: Final\*" entry and change the
path to use a forward slash ("Final/*") so the YAML is cross-platform and
conventional; update any matching inputs or references if they assume the
backslash form.
In `@Make.bat`:
- Around line 17-20: The Make.bat uses unvalidated user input (%~1) to set
buildPlatform and later pass it to dotnet publish; sanitize this by whitelisting
allowed values (e.g., "Release" and "Debug") in the Make.bat script: when %~1 is
provided, compare it against the allowed set and only assign to buildPlatform if
it matches, otherwise fallback to the default "Release" and optionally print a
warning; ensure all uses of buildPlatform (dotnet publish invocations) then use
this validated variable.
In `@tools/Download-Release.ps1`:
- Around line 146-150: The script currently unconditionally deletes the
OutputDir using Test-Path and Remove-Item which risks accidental data loss;
update the block around Test-Path/Remove-Item to either prompt the user for
confirmation (e.g., ask to proceed, with a --yes/--force override) or create a
timestamped backup (e.g., move the existing OutputDir to a backup folder) before
removal, and ensure New-Item still creates the directory afterward; reference
the OutputDir variable and the Remove-Item/New-Item operations when making the
change so behavior is explicit and reversible.
In `@tools/markdown_to_html.py`:
- Around line 85-89: The convert function inserts title into _HTML_TEMPLATE
without escaping, risking HTML injection; update convert to escape the title
before formatting by applying an HTML-escaping routine (e.g., html.escape) to
the title parameter and then pass the escaped value to
_HTML_TEMPLATE.format(title=escaped_title, body=body); keep all other behavior
the same and reference the convert function and _HTML_TEMPLATE when making the
change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b6de786d-13ca-4076-aee4-461f9bff2ea1
📒 Files selected for processing (8)
.github/workflows/copilot-setup-steps.yml.github/workflows/release.yml.github/workflows/reusable-build.yml.github/workflows/test-installer-build.ymlMake.battools/Download-Release.ps1tools/README.mdtools/markdown_to_html.py
| ) else ( | ||
| echo WARNING: Python not found, generating dummy HTML files | ||
| echo ^<html^>^<body^>^<h1^>Dummy Changelog, Python with markdown is required^</h1^>^</body^>^</html^> > %finalDir%\Changelog.html | ||
| echo ^<html^>^<body^>^<h1^>Dummy README, Python with markdown is required^</h1^>^</body^>^</html^> > %finalDir%\Readme.html | ||
| ) |
There was a problem hiding this comment.
Dummy HTML fallback is missing Terms.html.
When Python is not available, the fallback generates dummy Changelog.html and Readme.html but not Terms.html. This inconsistency could cause issues if downstream processes expect Terms.html to exist.
🐛 Proposed fix to add missing Terms.html
) else (
echo WARNING: Python not found, generating dummy HTML files
echo ^<html^>^<body^>^<h1^>Dummy Changelog, Python with markdown is required^</h1^>^</body^>^</html^> > %finalDir%\Changelog.html
echo ^<html^>^<body^>^<h1^>Dummy README, Python with markdown is required^</h1^>^</body^>^</html^> > %finalDir%\Readme.html
+ echo ^<html^>^<body^>^<h1^>Dummy Terms, Python with markdown is required^</h1^>^</body^>^</html^> > %finalDir%\Terms.html
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ) else ( | |
| echo WARNING: Python not found, generating dummy HTML files | |
| echo ^<html^>^<body^>^<h1^>Dummy Changelog, Python with markdown is required^</h1^>^</body^>^</html^> > %finalDir%\Changelog.html | |
| echo ^<html^>^<body^>^<h1^>Dummy README, Python with markdown is required^</h1^>^</body^>^</html^> > %finalDir%\Readme.html | |
| ) | |
| ) else ( | |
| echo WARNING: Python not found, generating dummy HTML files | |
| echo ^<html^>^<body^>^<h1^>Dummy Changelog, Python with markdown is required^</h1^>^</body^>^</html^> > %finalDir%\Changelog.html | |
| echo ^<html^>^<body^>^<h1^>Dummy README, Python with markdown is required^</h1^>^</body^>^</html^> > %finalDir%\Readme.html | |
| echo ^<html^>^<body^>^<h1^>Dummy Terms, Python with markdown is required^</h1^>^</body^>^</html^> > %finalDir%\Terms.html | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Make.bat` around lines 74 - 78, The else fallback in the Make.bat script
writes dummy Changelog.html and Readme.html to %finalDir% but omits Terms.html;
update the else block to also output a dummy Terms.html into %finalDir% (same
HTML structure/message style as the other two) so downstream consumers always
find %finalDir%\Terms.html.
📝 Add docstrings to `copilot/rethink-release-pipeline`
|
@copilot fix the most valuable comments on your pr. Also we should have a new poweshell script to use winget to install properly innosetup, sign tool of Microsoft and any other tools we need to do this build. |
…r.ps1 and Install-BuildTools.ps1, fix LASTEXITCODE check, add permissions, auto-detect framework Agent-Logs-Url: https://github.com/Belphemur/SoundSwitch/sessions/38e34ac6-1952-4ee7-93d6-5c39c48a0142 Co-authored-by: Belphemur <197810+Belphemur@users.noreply.github.com>
Agent-Logs-Url: https://github.com/Belphemur/SoundSwitch/sessions/38e34ac6-1952-4ee7-93d6-5c39c48a0142 Co-authored-by: Belphemur <197810+Belphemur@users.noreply.github.com>
Addressed in Make.bat reverted — no changes to the original script. New PowerShell scripts created:
Reviewer fixes applied:
|
|
@copilot use python 3.14 |
|
@copilot apply changes based on the comments in this thread we're on windows 11 so don't care about powershell lower than 7. We should document that properly. |
… failure, document PowerShell 7+ Agent-Logs-Url: https://github.com/Belphemur/SoundSwitch/sessions/68467dab-1377-4c87-b176-8ab9b8660de3 Co-authored-by: Belphemur <197810+Belphemur@users.noreply.github.com>
Agent-Logs-Url: https://github.com/Belphemur/SoundSwitch/sessions/68467dab-1377-4c87-b176-8ab9b8660de3 Co-authored-by: Belphemur <197810+Belphemur@users.noreply.github.com>
Applied in Review comments addressed:
CI failure fixed:
PowerShell 7+ documented:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
tools/Build-Installer.ps1 (1)
222-238: Installer signing happens after the success message.The "Installer built successfully!" message (line 220) is printed before the installer is signed (lines 226-230). If signing fails, the user already saw a success message, which could be confusing.
♻️ Move success message after signing
-Write-Host "Installer built successfully!" -ForegroundColor Green - $installerDir = Join-Path $finalDir 'Installer' if (Test-Path $installerDir) { $installers = Get-ChildItem $installerDir -Filter '*Installer.exe' if ($installers) { # ── Step 6: Sign the installer ─────────────────────────────────────── if ($canSign) { Write-Host "`n=== Signing installer ===" -ForegroundColor White $installerPaths = $installers | ForEach-Object { $_.FullName } & $signScript -Path $installerPaths -CertificateName $CertificateName } + Write-Host "`n=================================" -ForegroundColor White + Write-Host "Installer built successfully!" -ForegroundColor Green Write-Host "`nInstaller(s):" foreach ($ins in $installers) { Write-Host " $($ins.FullName)" -ForegroundColor Cyan } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/Build-Installer.ps1` around lines 222 - 238, The "Installer built successfully!" success message is emitted before signing occurs; move that message so it is printed only after the signing block completes (and only if signing succeeded). Locate the installer handling code using $installerDir, $installers, $canSign and the call to & $signScript -Path $installerPaths -CertificateName $CertificateName, and relocate the success Write-Host to after the signing and installer listing; also ensure that signing failures return a non-zero exit or emit an error via Write-Error before the success message is shown.tools/Sign-Binary.ps1 (1)
48-61: Pipeline input won't work as expected without a process block.The parameter declaration accepts pipeline input (
ValueFromPipeline), but the script body uses a simpleforeachloop instead ofbegin/process/endblocks. This means piped input likeGet-ChildItem *.exe | .\Sign-Binary.ps1will only process the last item.♻️ Fix for proper pipeline support
[CmdletBinding()] param( [Parameter(Mandatory, ValueFromPipeline, ValueFromPipelineByPropertyName)] [Alias('FullName')] [string[]]$Path, [string]$CertificateName = 'OpenSource Developer, Antoine Aflalo', [string]$TimestampUrl = 'http://time.certum.pl', [int]$MaxRetries = 3, [int]$RetryDelaySec = 5 ) +begin { Set-StrictMode -Version Latest $ErrorActionPreference = 'Stop' # ── Locate signtool ───────────────────────────────────────────────────────── $signtool = Get-Command 'signtool.exe' -ErrorAction SilentlyContinue if (-not $signtool) { throw "signtool.exe not found on PATH. Run tools\Install-BuildTools.ps1 first." } Write-Host "Using signtool: $($signtool.Source)" -ForegroundColor DarkGray + + $allPaths = [System.Collections.Generic.List[string]]::new() +} + +process { + foreach ($p in $Path) { + $allPaths.Add($p) + } +} + +end { # ── Sign each file ────────────────────────────────────────────────────────── - foreach ($file in $Path) { + foreach ($file in $allPaths) { # ... rest of signing logic unchanged ... } +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/Sign-Binary.ps1` around lines 48 - 61, The script currently declares $Path with ValueFromPipeline but processes inputs via a top-level foreach which breaks pipeline semantics; refactor the script into an advanced function or add a Begin/Process/End block so each piped object is handled in the Process block (use the declared parameter $Path and iterate over incoming pipeline items there), preserve existing variables like $CertificateName, $TimestampUrl, $MaxRetries and $RetryDelaySec, and move the signing logic from the current foreach into Process so piped input (e.g., Get-ChildItem *.exe | .\Sign-Binary.ps1) is processed item-by-item rather than only the final item..github/workflows/reusable-build.yml (1)
175-182: Inconsistent error handling for asset copying.
soundSwitched.pngandSoundSwitch.CLI\README.mduse-ErrorAction SilentlyContinue(lines 179-180), whileLICENSE.txtandTerms.txt(lines 181-182) will throw if missing. IfLICENSE.txtorTerms.txtare ever missing, the workflow will fail without a clear error message.Consider making the error handling consistent - either all assets should fail explicitly, or document which are optional:
♻️ Suggested improvement for clarity
# Copy additional assets - Copy-Item 'img\soundSwitched.png' $outputDir -Force -ErrorAction SilentlyContinue - Copy-Item 'SoundSwitch.CLI\README.md' $outputDir -Force -ErrorAction SilentlyContinue - Copy-Item 'LICENSE.txt' $outputDir -Force - Copy-Item 'Terms.txt' $outputDir -Force + # Optional assets (may not exist in all builds) + Copy-Item 'img\soundSwitched.png' $outputDir -Force -ErrorAction SilentlyContinue + Copy-Item 'SoundSwitch.CLI\README.md' $outputDir -Force -ErrorAction SilentlyContinue + + # Required assets (fail if missing) + if (-not (Test-Path 'LICENSE.txt')) { throw "LICENSE.txt not found" } + if (-not (Test-Path 'Terms.txt')) { throw "Terms.txt not found" } + Copy-Item 'LICENSE.txt' $outputDir -Force + Copy-Item 'Terms.txt' $outputDir -Force🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/reusable-build.yml around lines 175 - 182, The Copy-Item calls are using mixed error handling: 'img\soundSwitched.png' and 'SoundSwitch.CLI\README.md' use -ErrorAction SilentlyContinue while 'LICENSE.txt' and 'Terms.txt' will throw if missing; make this consistent by treating LICENSE.txt and Terms.txt as required and change their Copy-Item calls to use -ErrorAction Stop, then wrap those copies in a try/catch that calls Write-Error (or processLogger equivalent) with a clear message including the filename and $outputDir so the workflow fails with an explicit, descriptive error; alternatively, if they are optional, change their Copy-Item calls to use -ErrorAction SilentlyContinue and emit a warning via Write-Warning when the file is not found.tools/Download-Release.ps1 (1)
55-64: Consider warning when no GitHub token is provided.Without a token, GitHub's API has strict rate limits (60 requests/hour for unauthenticated). While this script makes minimal API calls, users running it repeatedly during development may hit limits.
♻️ Optional: Add rate limit warning
function Get-GitHubHeaders { $headers = @{ 'Accept' = 'application/vnd.github+json' 'User-Agent' = 'SoundSwitch-Download/1.0' } if ($Token) { $headers['Authorization'] = "Bearer $Token" } + else { + Write-Host " Note: No GitHub token provided. API rate limits may apply." -ForegroundColor DarkGray + } return $headers }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/Download-Release.ps1` around lines 55 - 64, Get-GitHubHeaders currently builds headers but silently proceeds when $Token is missing; add a user-visible warning in Get-GitHubHeaders that informs users about unauthenticated GitHub API rate limits (60 requests/hour) when $Token is null or empty and suggest supplying a token or retrying later; implement this by checking if -not $Token inside Get-GitHubHeaders and emitting a Write-Warning (or Write-Host) message that explains the rate-limit and recommended action before returning the headers.tools/Install-BuildTools.ps1 (1)
123-126: Hardcoded NuGet package version will require manual updates.The
Microsoft.Windows.SDK.BuildToolsversion is hardcoded to10.0.26100.7705. This will become outdated as new SDK versions are released and may eventually be unavailable.Consider fetching the latest version dynamically or documenting this as a maintenance item:
♻️ Option: Fetch latest version dynamically
# Download the NuGet package and extract signtool $nugetPkg = 'microsoft.windows.sdk.buildtools' - $nugetVer = '10.0.26100.7705' - $nupkgUrl = "https://api.nuget.org/v3-flatcontainer/$nugetPkg/$nugetVer/$nugetPkg.$nugetVer.nupkg" + # Fetch latest version from NuGet + $indexUrl = "https://api.nuget.org/v3-flatcontainer/$nugetPkg/index.json" + $versions = (Invoke-RestMethod -Uri $indexUrl).versions + $nugetVer = $versions | Select-Object -Last 1 + Write-Host " Using SDK BuildTools version: $nugetVer" -ForegroundColor DarkGray + $nupkgUrl = "https://api.nuget.org/v3-flatcontainer/$nugetPkg/$nugetVer/$nugetPkg.$nugetVer.nupkg"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/Install-BuildTools.ps1` around lines 123 - 126, The script currently hardcodes $nugetVer for microsoft.windows.sdk.buildtools which will become stale; update Install-BuildTools.ps1 to determine the package version dynamically (or accept it as an optional parameter) by querying the NuGet flat container index for $nugetPkg (e.g., call the package index URL for $nugetPkg and pick the latest version from the returned versions array), set $nugetVer to that latest value before constructing $nupkgUrl, and include a safe fallback to the existing hardcoded version if the network query fails; reference the variables $nugetPkg, $nugetVer and $nupkgUrl when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Installer/setup.iss`:
- Around line 38-42: Re-enable signing of the uninstaller by restoring
SignedUninstaller=yes or by adding an explicit post-build signing step for the
generated unins000.exe; either set the Inno Setup SignTool directive to invoke
your existing Sign-Binary.ps1 (so Inno signs the uninstaller during build) or
update Build-Installer.ps1 to call Sign-Binary.ps1 on the produced unins000.exe
after Inno runs; reference the SignedUninstaller setting, the SignTool
directive, Sign-Binary.ps1 and Build-Installer.ps1 to implement the fix.
---
Nitpick comments:
In @.github/workflows/reusable-build.yml:
- Around line 175-182: The Copy-Item calls are using mixed error handling:
'img\soundSwitched.png' and 'SoundSwitch.CLI\README.md' use -ErrorAction
SilentlyContinue while 'LICENSE.txt' and 'Terms.txt' will throw if missing; make
this consistent by treating LICENSE.txt and Terms.txt as required and change
their Copy-Item calls to use -ErrorAction Stop, then wrap those copies in a
try/catch that calls Write-Error (or processLogger equivalent) with a clear
message including the filename and $outputDir so the workflow fails with an
explicit, descriptive error; alternatively, if they are optional, change their
Copy-Item calls to use -ErrorAction SilentlyContinue and emit a warning via
Write-Warning when the file is not found.
In `@tools/Build-Installer.ps1`:
- Around line 222-238: The "Installer built successfully!" success message is
emitted before signing occurs; move that message so it is printed only after the
signing block completes (and only if signing succeeded). Locate the installer
handling code using $installerDir, $installers, $canSign and the call to &
$signScript -Path $installerPaths -CertificateName $CertificateName, and
relocate the success Write-Host to after the signing and installer listing; also
ensure that signing failures return a non-zero exit or emit an error via
Write-Error before the success message is shown.
In `@tools/Download-Release.ps1`:
- Around line 55-64: Get-GitHubHeaders currently builds headers but silently
proceeds when $Token is missing; add a user-visible warning in Get-GitHubHeaders
that informs users about unauthenticated GitHub API rate limits (60
requests/hour) when $Token is null or empty and suggest supplying a token or
retrying later; implement this by checking if -not $Token inside
Get-GitHubHeaders and emitting a Write-Warning (or Write-Host) message that
explains the rate-limit and recommended action before returning the headers.
In `@tools/Install-BuildTools.ps1`:
- Around line 123-126: The script currently hardcodes $nugetVer for
microsoft.windows.sdk.buildtools which will become stale; update
Install-BuildTools.ps1 to determine the package version dynamically (or accept
it as an optional parameter) by querying the NuGet flat container index for
$nugetPkg (e.g., call the package index URL for $nugetPkg and pick the latest
version from the returned versions array), set $nugetVer to that latest value
before constructing $nupkgUrl, and include a safe fallback to the existing
hardcoded version if the network query fails; reference the variables $nugetPkg,
$nugetVer and $nupkgUrl when making the change.
In `@tools/Sign-Binary.ps1`:
- Around line 48-61: The script currently declares $Path with ValueFromPipeline
but processes inputs via a top-level foreach which breaks pipeline semantics;
refactor the script into an advanced function or add a Begin/Process/End block
so each piped object is handled in the Process block (use the declared parameter
$Path and iterate over incoming pipeline items there), preserve existing
variables like $CertificateName, $TimestampUrl, $MaxRetries and $RetryDelaySec,
and move the signing logic from the current foreach into Process so piped input
(e.g., Get-ChildItem *.exe | .\Sign-Binary.ps1) is processed item-by-item rather
than only the final item.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 27f2f310-f538-4cd7-b576-94397332f59e
📒 Files selected for processing (10)
.github/workflows/release.yml.github/workflows/reusable-build.yml.github/workflows/test-installer-build.ymlInstaller/setup.isstools/Build-Installer.ps1tools/Download-Release.ps1tools/Install-BuildTools.ps1tools/README.mdtools/Sign-Binary.ps1tools/markdown_to_html.py
🚧 Files skipped from review as they are similar to previous changes (3)
- .github/workflows/release.yml
- .github/workflows/test-installer-build.yml
- tools/markdown_to_html.py
| //SignTool=SoundSwitch | ||
| SignTool=Certum | ||
| //SignTool=Certum | ||
| // Code signing is now handled by tools\Sign-Binary.ps1 (called from Build-Installer.ps1) | ||
|
|
||
| SignedUninstaller=yes | ||
| //SignedUninstaller=yes |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if there's documentation about this signing change or if SignedUninstaller was intentionally disabled
rg -n -i "uninstaller|SignedUninstaller" --type=mdRepository: Belphemur/SoundSwitch
Length of output: 585
🏁 Script executed:
# Find the Sign-Binary.ps1 and Build-Installer.ps1 scripts to understand the signing flow
find . -name "Sign-Binary.ps1" -o -name "Build-Installer.ps1" | head -20Repository: Belphemur/SoundSwitch
Length of output: 116
🏁 Script executed:
# Look at the Installer/setup.iss file more broadly to understand the signing context
rg -n "SignTool\|SignedUninstaller\|Sign-Binary" Installer/setup.iss -B2 -A2Repository: Belphemur/SoundSwitch
Length of output: 47
🏁 Script executed:
# Check if there's any documentation or issues discussing this signing approach change
rg -l "Sign-Binary\|signed.*installer\|unsigned.*uninstaller" --type=md --type=txt --type=rstRepository: Belphemur/SoundSwitch
Length of output: 47
🏁 Script executed:
# List the tools directory to confirm the scripts
ls -la tools/ | grep -i signRepository: Belphemur/SoundSwitch
Length of output: 133
🏁 Script executed:
# Read the Sign-Binary.ps1 script to understand the signing approach
cat tools/Sign-Binary.ps1Repository: Belphemur/SoundSwitch
Length of output: 3843
🏁 Script executed:
# Read the Build-Installer.ps1 script
cat tools/Build-Installer.ps1Repository: Belphemur/SoundSwitch
Length of output: 8422
🏁 Script executed:
# Find and read the setup.iss file
find . -name "setup.iss" -type fRepository: Belphemur/SoundSwitch
Length of output: 86
🏁 Script executed:
# Read the setup.iss file, particularly around lines 38-42 and related sections
cat -n Installer/setup.iss | head -100Repository: Belphemur/SoundSwitch
Length of output: 5469
🏁 Script executed:
# Also check if there are any git commits or changes related to this SignTool change
git log --oneline -20 -- Installer/setup.iss 2>/dev/null || echo "Git history not available"Repository: Belphemur/SoundSwitch
Length of output: 142
🏁 Script executed:
# Check if there's a Make-Installer.bat that might handle signing
cat Installer/Make-Installer.batRepository: Belphemur/SoundSwitch
Length of output: 1654
🏁 Script executed:
# Search for any recent changes or discussions about uninstaller signing
git log --oneline --all -50 2>/dev/null | grep -i "sign\|uninstall" || echo "No recent commits found with sign/uninstall"Repository: Belphemur/SoundSwitch
Length of output: 108
🏁 Script executed:
# Check if there are any scripts in Installer/scripts that might be relevant
ls -la Installer/scripts/ 2>/dev/null || echo "Scripts directory may not exist"Repository: Belphemur/SoundSwitch
Length of output: 801
🏁 Script executed:
# Check the uninstall_utils.iss to see if there's any signing-related logic
cat -n Installer/scripts/uninstall_utils.issRepository: Belphemur/SoundSwitch
Length of output: 2030
🏁 Script executed:
# Search for any mention of uninstaller signing or signing approach in issues/discussions
rg -i "uninstaller.*sign|sign.*uninstaller" --type=md --type=txtRepository: Belphemur/SoundSwitch
Length of output: 47
🏁 Script executed:
# Check if SignedUninstaller or related directives appear anywhere else in the codebase
rg -r "SignedUninstaller\|SignTool" --type=issRepository: Belphemur/SoundSwitch
Length of output: 461
🏁 Script executed:
# Check if there's any documentation about the uninstaller handling or signing strategy
rg -i "uninstaller\|unsigned" README.md CHANGELOG.md 2>/dev/null | head -20Repository: Belphemur/SoundSwitch
Length of output: 47
🏁 Script executed:
# Let me verify the exact flow - check if Setup-Uninstaller is created and if it gets signed
rg "uninstall" tools/Build-Installer.ps1 -A2 -B2Repository: Belphemur/SoundSwitch
Length of output: 47
🏁 Script executed:
# Inno Setup creates an uninstaller executable - let's see if there's any post-build signing of it
rg -i "uninstall.*exe\|uninstall.*sign" tools/ -rRepository: Belphemur/SoundSwitch
Length of output: 478
Uninstaller will remain unsigned with this approach.
Disabling SignedUninstaller=yes means the uninstaller executable (unins000.exe) created by Inno Setup will not be signed. While the installer itself is signed externally via Sign-Binary.ps1, the uninstaller is not re-signed afterward. When users run the uninstaller from Control Panel, they may encounter "Unknown Publisher" security warnings.
The coding guideline requires: "Use digital signature for the installer and application executables configured with SignTool directive in the setup script." The current approach signs the installer and main executables externally, but omits the uninstaller.
Recommended fix: Either keep SignedUninstaller=yes enabled and configure Inno Setup to call Sign-Binary.ps1 via its SignTool directive, or add an explicit signing step in Build-Installer.ps1 after Inno Setup creates the uninstaller executable.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Installer/setup.iss` around lines 38 - 42, Re-enable signing of the
uninstaller by restoring SignedUninstaller=yes or by adding an explicit
post-build signing step for the generated unins000.exe; either set the Inno
Setup SignTool directive to invoke your existing Sign-Binary.ps1 (so Inno signs
the uninstaller during build) or update Build-Installer.ps1 to call
Sign-Binary.ps1 on the produced unins000.exe after Inno runs; reference the
SignedUninstaller setting, the SignTool directive, Sign-Binary.ps1 and
Build-Installer.ps1 to implement the fix.
|
@copilot the GitHub action is still failing at setting up innosetup |
…in test-installer-build Agent-Logs-Url: https://github.com/Belphemur/SoundSwitch/sessions/579d1335-9120-426f-8029-0141c6fe5636 Co-authored-by: Belphemur <197810+Belphemur@users.noreply.github.com>
Fixed in |
|
@copilot for the sign tool we should use a different time stamping server than the certum one. It also support sha512 for the time stamping. As per https://knowledge.digicert.com/general-information/rfc3161-compliant-time-stamp-authority-server |
…amp digest Agent-Logs-Url: https://github.com/Belphemur/SoundSwitch/sessions/9e28f5c0-38a2-4741-92e0-0883a9f32d80 Co-authored-by: Belphemur <197810+Belphemur@users.noreply.github.com>
Agent-Logs-Url: https://github.com/Belphemur/SoundSwitch/sessions/9e28f5c0-38a2-4741-92e0-0883a9f32d80 Co-authored-by: Belphemur <197810+Belphemur@users.noreply.github.com>
Updated in
Also replaced the winget exit code handling in |
|
@copilot this is weird we already have the build ci/CD working but the test installer isn't able to build the full project, make required fixes to the cli/ipc package |
… and build CLI in normal CI Agent-Logs-Url: https://github.com/Belphemur/SoundSwitch/sessions/4fc3f679-b5ff-4d35-be9d-97e23364067b Co-authored-by: Belphemur <197810+Belphemur@users.noreply.github.com>
Agent-Logs-Url: https://github.com/Belphemur/SoundSwitch/sessions/4fc3f679-b5ff-4d35-be9d-97e23364067b Co-authored-by: Belphemur <197810+Belphemur@users.noreply.github.com>
Fixed in |
Rethinks the release pipeline and local build automation by replacing npm-based tooling with Python and PowerShell scripts, restructuring CI artifacts, and adding developer environment setup tooling.
Summary
CI/CD pipeline improvements
dotnet build SoundSwitch.sln) instead of only the main project, ensuring CLI/IPC breakage is caught early$LASTEXITCODEchecks afterdotnet publishto fail deterministically on errorspermissions: contents: writeto the release workflow for artifact uploadsSoundSwitch.csprojTargetFrameworkacross all workflows (reusable-build, test-installer-build, copilot-setup-steps) instead of hardcodingFinal\, upload as artifact, and zip/upload to GitHub Releaseswinget install, avoiding winget exit code issues on pre-installed runnersCLI compatibility fix
ExecuteAsyncfrompublictoprotected overridein all SoundSwitch.CLI command classes (MuteCommand,ProfileCommand,SettingsCommand,SwitchCommand) to match the Spectre.Console 0.55.0 API breaking changeNew PowerShell build tooling
tools/Install-BuildTools.ps1— uses winget to install Inno Setup 6, Certum SimplySign Desktop (cloud certificate provider for code signing), and Python 3.14 with the markdown package. Downloadssigntool.exefrom the lightweightMicrosoft.Windows.SDK.BuildToolsNuGet package (~22 MB) instead of the full multi-GB Windows SDK. Run once on a fresh Windows 11 machine.tools/Build-Installer.ps1— full PowerShell script for building the SoundSwitch installer locally. Supports building from source or from a downloaded release artifact (-DownloadRelease). Handles HTML documentation generation, asset bundling, code signing (viaSign-Binary.ps1), and Inno Setup compilation. Signs both application binaries and the final installer when signtool is available.tools/Sign-Binary.ps1— dedicated code-signing script usingsigntoolwith SHA-256 file digest and SHA-512 RFC 3161 timestamping via DigiCert (http://timestamp.digicert.com). No SHA-1. Certificate subject name is configurable via-CertificateName(defaults to"OpenSource Developer, Antoine Aflalo"). Includes retry logic for transient failures. Called automatically byBuild-Installer.ps1.tools/Download-Release.ps1— downloads the latest release or beta build artifact from GitHub intoFinal\for local signing and installer creation.Code signing moved out of Inno Setup
SignTool=Certum,SignedUninstaller=yes, andsignonceflags inInstaller/setup.iss— Inno Setup no longer handles signingBuild-Installer.ps1callingSign-Binary.ps1for both the application executables (before installer compilation) and the final installer executable (after Inno Setup builds it)Python-based documentation generation
tools/markdown_to_html.py— converts Markdown files to standalone HTML documents, replacing the previously usedmarkdown-htmlnpm package. Used in CI workflows and the new Build-Installer script.No changes to Make.bat — the legacy batch script is preserved as-is; the new PowerShell scripts provide the modern alternative.
Documentation
tools/README.mdwith descriptions of all new scripts and utilities