Conversation
sgress454
requested review from
georgekarrv,
getvictor,
lucasmrod,
lukeheath and
sharon-fdm
as code owners
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.
Tip: disable this comment in your organization's Code Review settings.
WalkthroughAdds CI and tooling to build, sign, and publish Windows MSI installers for fleetctl. Introduces a manual workflow 🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
.github/workflows/build-fleetctl-msi.yml (1)
79-95: Fail fast when release upload is requested from a branch.With
test_mode=false, the workflow still builds and signs both MSIs on non-tag refs, then errors only inUpload to release. An early guard would avoid burning runner time and touching signing infrastructure for an invocation that can never succeed.Also applies to: 201-232
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/build-fleetctl-msi.yml around lines 79 - 95, Add a fast-fail guard right after IS_TEST_MODE is computed: when test_mode is false (IS_TEST_MODE="false") verify the invocation is on a tag by checking the ref (GITHUB_REF starts with "refs/tags/"); if not, print a clear error like "Release upload requested but not running on a tag ref; aborting" and exit 1 to avoid building/signing. Apply the same check to the duplicate block referenced (lines 201-232) so any path that would attempt upload is prevented early when TAG_NAME/ref conditions are invalid.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/build-fleetctl-msi.yml:
- Around line 64-77: The tag-handling sets VERSION by stripping only "fleet-v"
so tags like "fleet-4.72.0" become "fleet-4.72.0" and later fall back to
MSI_VERSION=0.0.0; fix by normalizing both "fleet-" and an optional leading "v":
when REF_NAME matches fleet-* set TAG_NAME="$REF_NAME", set
VERSION="${REF_NAME#fleet-}" and then remove an optional leading v from VERSION
(e.g., VERSION="${VERSION#v}"), so subsequent MSI_VERSION detection
(MSI_VERSION="${VERSION#v}" or the existing numeric check) will correctly yield
"4.72.0" for both "fleet-4.72.0" and "fleet-v4.72.0".
In @.github/workflows/goreleaser-fleet.yaml:
- Around line 180-194: The upload steps and MSI matrix should gate
per-architecture instead of using the aggregate
steps.check_windows_binaries.outputs.exists flag; update the two upload steps
(the ones named "Upload unsigned Windows fleetctl amd64" and "Upload unsigned
Windows fleetctl arm64") to use per-arch conditionals that check the
corresponding outputs (e.g.
steps.check_windows_binaries.outputs.windows_amd64_exists == 'true' and
steps.check_windows_binaries.outputs.windows_arm64_exists == 'true') and only
reference the matching path outputs (windows_amd64_path / windows_arm64_path)
when present, and change the MSI matrix generation to derive architectures from
the discovered-arches output emitted by check_windows_binaries (emit a JSON or
comma-separated arches output and build the matrix from that) so missing arches
are not scheduled.
---
Nitpick comments:
In @.github/workflows/build-fleetctl-msi.yml:
- Around line 79-95: Add a fast-fail guard right after IS_TEST_MODE is computed:
when test_mode is false (IS_TEST_MODE="false") verify the invocation is on a tag
by checking the ref (GITHUB_REF starts with "refs/tags/"); if not, print a clear
error like "Release upload requested but not running on a tag ref; aborting" and
exit 1 to avoid building/signing. Apply the same check to the duplicate block
referenced (lines 201-232) so any path that would attempt upload is prevented
early when TAG_NAME/ref conditions are invalid.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3d0a061c-cda1-4415-ad44-72c992b04e80
📒 Files selected for processing (5)
.github/workflows/build-fleetctl-msi.yml.github/workflows/goreleaser-fleet.yamlchanges/42972-add-fleetctl-msi-to-releasetools/build-fleetctl-msi/fleetctl.wxstools/build-fleetctl-msi/main.sh
There was a problem hiding this comment.
Pull request overview
Adds a Windows .msi packaging path for fleetctl so signed installers can be built and attached to Fleet GitHub releases.
Changes:
- Introduces a WiX template + Bash build script to compile and code-sign a
fleetctlMSI. - Extends the release workflow to build/sign MSIs for Windows amd64/arm64 and upload them to the GitHub release.
- Adds a separate workflow for manually building/testing the MSI.
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
tools/build-fleetctl-msi/main.sh |
Builds and signs fleetctl.exe and the resulting MSI; optional release upload logic. |
tools/build-fleetctl-msi/fleetctl.wxs |
WiX definition to install fleetctl.exe and add install dir to system PATH. |
changes/42972-add-fleetctl-msi-to-release |
Release note entry for the new Windows MSI deliverable. |
.github/workflows/goreleaser-fleet.yaml |
Adds post-goreleaser jobs to build/sign MSIs on Windows and upload to the release. |
.github/workflows/build-fleetctl-msi.yml |
Adds a manual workflow to build/sign MSIs for testing purposes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| subject-path: dist/fleetctl_v${{ steps.extract_version.outputs.msi_version }}_windows_${{ matrix.arch }}.msi | ||
| push-to-registry: false | ||
|
|
||
| upload-release: |
There was a problem hiding this comment.
Guessing this will get nuked closer to release of this tool so we don't have multiple paths to getting a fleetctl release out?
There was a problem hiding this comment.
I think we keep this, so that if there's any issue getting the MSI uploaded in the release workflow, we can run this after the fact to fix it.
There was a problem hiding this comment.
Got it. Seems like we should do access control at the GHA level for the test mode flag, if not the whole action (guessing the whole action would need to be the scope).
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
.github/workflows/build-fleetctl-msi.yml (1)
74-76:⚠️ Potential issue | 🟡 MinorTighten MSI version regex to avoid false positives.
Line 74 accepts prefixes only, so values like
1.2.3-rc1pass here but fail later intools/build-fleetctl-msi/main.shstrict validation. Anchor the pattern to match the full value.Suggested fix
- if [[ "$VERSION" =~ ^[0-9]+\.[0-9]+\.[0-9]+ ]]; then + if [[ "$VERSION" =~ ^[0-9]+\.[0-9]+\.[0-9]+(\.[0-9]+)?$ ]]; then MSI_VERSION="$VERSION" else MSI_VERSION="0.0.0" fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/build-fleetctl-msi.yml around lines 74 - 76, The if condition that sets MSI_VERSION from VERSION currently uses a regex that only anchors the start and allows trailing characters (so values like "1.2.3-rc1" incorrectly match); update the test that checks VERSION (the [[ "$VERSION" =~ ^[0-9]+\.[0-9]+\.[0-9]+ ]] expression) to require a full-match by adding an end anchor (e.g., ^[0-9]+\.[0-9]+\.[0-9]+$) so MSI_VERSION is only set for strictly valid semver strings and remains consistent with the strict validation in tools/build-fleetctl-msi/main.sh.
🧹 Nitpick comments (1)
.github/workflows/build-fleetctl-msi.yml (1)
35-39: Scope write permissions to the publish path only.This manual workflow keeps
contents: writefor all runs, including default test-mode artifact builds. Consider splitting publish into a separate gated job (or protected environment) so non-publish runs use read-only permissions.Also applies to: 177-184
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/build-fleetctl-msi.yml around lines 35 - 39, The workflow currently grants global contents: write and attestations/id-token write in the top-level permissions block; change this so the default top-level permissions are least-privilege (set contents: read, id-token: write/attestations: write only if needed) and move contents: write into the specific publish job that uploads release assets (add a job-level permissions override on the publish job where you set contents: write). Alternatively gate the publish job behind a protected environment or workflow_dispatch condition so only that job gets write access; apply the same change to the other permissions block referenced (lines 177-184) where publish/upload occurs. Ensure you update the job names that perform upload/release to include the job-level permissions override rather than keeping write at the workflow root.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In @.github/workflows/build-fleetctl-msi.yml:
- Around line 74-76: The if condition that sets MSI_VERSION from VERSION
currently uses a regex that only anchors the start and allows trailing
characters (so values like "1.2.3-rc1" incorrectly match); update the test that
checks VERSION (the [[ "$VERSION" =~ ^[0-9]+\.[0-9]+\.[0-9]+ ]] expression) to
require a full-match by adding an end anchor (e.g., ^[0-9]+\.[0-9]+\.[0-9]+$) so
MSI_VERSION is only set for strictly valid semver strings and remains consistent
with the strict validation in tools/build-fleetctl-msi/main.sh.
---
Nitpick comments:
In @.github/workflows/build-fleetctl-msi.yml:
- Around line 35-39: The workflow currently grants global contents: write and
attestations/id-token write in the top-level permissions block; change this so
the default top-level permissions are least-privilege (set contents: read,
id-token: write/attestations: write only if needed) and move contents: write
into the specific publish job that uploads release assets (add a job-level
permissions override on the publish job where you set contents: write).
Alternatively gate the publish job behind a protected environment or
workflow_dispatch condition so only that job gets write access; apply the same
change to the other permissions block referenced (lines 177-184) where
publish/upload occurs. Ensure you update the job names that perform
upload/release to include the job-level permissions override rather than keeping
write at the workflow root.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 25aa42d0-4e05-454b-af55-c11aa10391f7
📒 Files selected for processing (2)
.github/workflows/build-fleetctl-msi.ymltools/build-fleetctl-msi/fleetctl.wxs
✅ Files skipped from review due to trivial changes (1)
- tools/build-fleetctl-msi/fleetctl.wxs
| curl -fSL -o wix314-binaries.zip \ | ||
| https://github.com/wixtoolset/wix3/releases/download/wix3141rtm/wix314-binaries.zip | ||
| mkdir -p "$RUNNER_TEMP/wix" | ||
| unzip -q wix314-binaries.zip -d "$RUNNER_TEMP/wix" | ||
| echo "$RUNNER_TEMP/wix" >> $GITHUB_PATH |
There was a problem hiding this comment.
Verify integrity before executing downloaded tooling.
These steps download binaries and immediately execute/install them in a code-signing workflow. Please add integrity verification (pinned SHA-256 and/or Authenticode signature checks) before unzip/msiexec.
Suggested hardening (example)
- name: Install WiX 3.14.1
run: |
- curl -fSL -o wix314-binaries.zip \
- https://github.com/wixtoolset/wix3/releases/download/wix3141rtm/wix314-binaries.zip
+ WIX_URL="https://github.com/wixtoolset/wix3/releases/download/wix3141rtm/wix314-binaries.zip"
+ WIX_SHA256="<PIN_OFFICIAL_SHA256>"
+ curl -fSL -o wix314-binaries.zip "$WIX_URL"
+ echo "${WIX_SHA256} *wix314-binaries.zip" | sha256sum -c -
mkdir -p "$RUNNER_TEMP/wix"
unzip -q wix314-binaries.zip -d "$RUNNER_TEMP/wix"
echo "$RUNNER_TEMP/wix" >> $GITHUB_PATH - name: Install DigiCert KeyLocker KSP
run: |
curl https://one.digicert.com/signingmanager/api-ui/v1/releases/Keylockertools-windows-x64.msi/download -H "x-api-key:%SM_API_KEY%" --fail-with-body -o Keylockertools-windows-x64.msi
+ powershell -NoProfile -Command ^
+ "$sig = Get-AuthenticodeSignature 'Keylockertools-windows-x64.msi'; if ($sig.Status -ne 'Valid') { throw 'Invalid MSI signature' }"
msiexec /i Keylockertools-windows-x64.msi /quiet /qn
smksp_registrar.exe listAlso applies to: 165-166
Related issue: For #42972
Checklist for submitter
If some of the following don't apply, delete the relevant line.
changes/,orbit/changes/oree/fleetd-chrome/changes.See Changes files for more information.
Testing
It's very difficult to e2e test the goreleaser workflow without doing an actual release (we could do a prerelease and then delete it, but it's not ideal). But the benefit of having the separate packager workflow is that we can run it at any time with test mode turned off to push a package to a release. So if the gorelease workflow doesn't work right out of the gate, we can use the separate workflow to get the same effect.
Summary by CodeRabbit