feat(packaging): signed Windows .msi installer#768
Merged
Conversation
📝 WalkthroughWalkthroughChangesThis PR adds Windows MSI installer packaging for the adder application. It introduces a WiX v4 installer definition (adder.wxs), a PowerShell build/sign script (build-msi.ps1), documentation (README.md), and a new test-msi GitHub Actions workflow that builds amd64/arm64 binaries, conditionally signs them via jsign/Google Cloud, and produces/verifies the MSI. The existing publish.yml workflow is updated to quote RELEASE_TAG exports, sign both adder.exe and adder-tray.exe, build and sign the MSI, verify signatures, upload the MSI as a release asset, attest it, and rewrite the Docker manifest-create loop. Sequence Diagram(s)Sequence diagrams are included within the hidden review stack artifact above for the build-msi.ps1, test-msi workflow, and publish.yml layers. Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ 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: 4
🧹 Nitpick comments (1)
.github/workflows/test-msi.yml (1)
30-32: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winDisable checkout credential persistence before running build scripts.
This workflow builds code from the checked-out tree; keeping the GitHub token in git config is unnecessary and increases exposure if this runs for PRs.
Proposed hardening
- uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0 with: fetch-depth: '0' + persist-credentials: false🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/test-msi.yml around lines 30 - 32, The checkout step in the test-msi workflow is persisting GitHub credentials unnecessarily; update the actions/checkout usage to disable credential persistence before any build scripts run. Keep the existing checkout behavior in the workflow, but add the appropriate setting on the checkout step so the token is not written to git config, using the actions/checkout invocation as the place to make the change.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/publish.yml:
- Around line 592-600: The loop in the manifest publishing step is iterating
directly over steps.meta.outputs.tags, but that value is multiline and can break
the shell script when multiple tags are present. Update the publish workflow to
pass the tags through a DOCKER_TAGS env var before the loop, then iterate over
DOCKER_TAGS in the manifest creation block that uses docker manifest create,
docker manifest inspect, and docker manifest push.
- Around line 328-329: The Windows MSI/signing/upload flow is using injected
values directly in PowerShell, so update the steps around the MSI path
construction and release upload to read RELEASE_TAG via runtime env access (for
example in the logic that builds the MSI path) and use Join-Path instead of
string interpolation. Also in the GitHub release upload logic, make sure the
assetName is URL-encoded before appending it to the query string so the upload
URL is safe.
In @.github/workflows/test-msi.yml:
- Around line 3-6: The workflow trigger for the MSI build is too narrow and
prevents PRs/forks from producing unsigned MSI artifacts automatically. Update
the trigger section in the test-msi workflow to include a pull_request event
alongside the existing push/manual dispatch behavior, and keep the existing job
logic unchanged so the MSI artifact generation still runs through the same
workflow path.
In `@packaging/windows/README.md`:
- Around line 7-12: The introductory fenced blocks in the Windows README are
missing a language tag, causing markdownlint and GitHub syntax highlighting
issues. Update both code fences in the README section shown by the examples to
use the same plain-text language identifier, and keep the content unchanged; the
fix should be applied consistently to both introductory snippets so the markdown
parser treats them as labeled code blocks.
---
Nitpick comments:
In @.github/workflows/test-msi.yml:
- Around line 30-32: The checkout step in the test-msi workflow is persisting
GitHub credentials unnecessarily; update the actions/checkout usage to disable
credential persistence before any build scripts run. Keep the existing checkout
behavior in the workflow, but add the appropriate setting on the checkout step
so the token is not written to git config, using the actions/checkout invocation
as the place to make 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: cc7688ce-d927-4623-b07f-d335641614d6
📒 Files selected for processing (5)
.github/workflows/publish.yml.github/workflows/test-msi.ymlpackaging/windows/README.mdpackaging/windows/adder.wxspackaging/windows/build-msi.ps1
Comment on lines
+328
to
+329
| $cleanVersion = "${{ env.RELEASE_TAG }}" -replace '^v', '' | ||
| $msi = "dist\${{ env.APPLICATION_NAME }}-$cleanVersion-windows-${{ matrix.arch }}.msi" |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
wc -l .github/workflows/publish.yml
sed -n '300,340p' .github/workflows/publish.yml
printf '\n---\n'
sed -n '400,470p' .github/workflows/publish.yml
printf '\n---\n'
sed -n '580,610p' .github/workflows/publish.ymlRepository: blinklabs-io/adder
Length of output: 7531
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "RELEASE_TAG|APPLICATION_NAME|create-draft-release|Upload MSI release asset|manifest-create|steps\.meta\.outputs\.tags" .github/workflows/publish.ymlRepository: blinklabs-io/adder
Length of output: 3412
Use runtime env vars for the Windows MSI paths and encode the asset name. $RELEASE_TAG is injected into PowerShell source in the Windows MSI/signing/upload steps; switch to $env:RELEASE_TAG with Join-Path so a crafted tag can’t break the script. Also URL-encode $assetName before adding it to the GitHub release upload query string.
🧰 Tools
🪛 zizmor (1.26.1)
[warning] 328-328: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/publish.yml around lines 328 - 329, The Windows
MSI/signing/upload flow is using injected values directly in PowerShell, so
update the steps around the MSI path construction and release upload to read
RELEASE_TAG via runtime env access (for example in the logic that builds the MSI
path) and use Join-Path instead of string interpolation. Also in the GitHub
release upload logic, make sure the assetName is URL-encoded before appending it
to the query string so the upload URL is safe.
Source: Linters/SAST tools
Comment on lines
+592
to
+600
| for t in ${{ steps.meta.outputs.tags }}; do | ||
| # Extract the underlying manifests from each manifests list and create a new single manifest list. | ||
| # Word-splitting of the two command substitutions below is intentional: | ||
| # each digest line becomes a separate argument to `docker manifest create`. | ||
| # shellcheck disable=SC2046 | ||
| docker manifest create "${t}" \ | ||
| $(docker manifest inspect "${t}-amd64" | jq -r '.manifests[] | .digest' | sed -e "s|^|${t%:*}@|") \ | ||
| $(docker manifest inspect "${t}-arm64" | jq -r '.manifests[] | .digest' | sed -e "s|^|${t%:*}@|") | ||
| docker manifest push "${t}" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the workflow and inspect the relevant region with line numbers.
wf=".github/workflows/publish.yml"
wc -l "$wf"
sed -n '540,620p' "$wf" | cat -n
# Search for the meta step that defines steps.meta.outputs.tags.
rg -n "id:\s*meta|outputs\.tags|docker/metadata-action|steps\.meta\.outputs\.tags" "$wf" -n -A3 -B3
# Show the exact shell context around the manifest loop.
rg -n "manifest-create|docker manifest create|for t in|done <<<|DOCKER_TAGS" "$wf" -n -A10 -B10Repository: blinklabs-io/adder
Length of output: 7906
🏁 Script executed:
#!/bin/bash
set -euo pipefail
bash <<'BASH'
set -euo pipefail
printf 'Case 1: literal newlines in the word list\n'
for t in a
b; do
printf '[%s]\n' "$t"
done
printf '\nCase 2: variable expansion containing newlines\n'
TAGS=$'a\nb'
for t in $TAGS; do
printf '[%s]\n' "$t"
done
printf '\nCase 3: value with spaces/newlines (shell splitting demo)\n'
TAGS=$'a b\nc'
for t in $TAGS; do
printf '[%s]\n' "$t"
done
BASHRepository: blinklabs-io/adder
Length of output: 251
Pass steps.meta.outputs.tags through an env var before the loop .github/workflows/publish.yml:592-600
steps.meta.outputs.tags is multiline, so inlining it in for t in ... can break the shell script as soon as more than one tag is emitted. Iterate over DOCKER_TAGS instead.
🧰 Tools
🪛 zizmor (1.26.1)
[info] 592-592: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/publish.yml around lines 592 - 600, The loop in the
manifest publishing step is iterating directly over steps.meta.outputs.tags, but
that value is multiline and can break the shell script when multiple tags are
present. Update the publish workflow to pass the tags through a DOCKER_TAGS env
var before the loop, then iterate over DOCKER_TAGS in the manifest creation
block that uses docker manifest create, docker manifest inspect, and docker
manifest push.
Source: Linters/SAST tools
Comment on lines
+3
to
+6
| on: | ||
| push: | ||
| branches: ['feat/windows-msi-689'] | ||
| workflow_dispatch: |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Add the PR trigger for MSI artifacts.
This currently runs only on one feature branch or manual dispatch, so PRs/forks will not automatically produce the unsigned MSI artifacts described by the PR objective.
Proposed workflow trigger fix
on:
+ pull_request:
push:
branches: ['feat/windows-msi-689']
workflow_dispatch:📝 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.
Suggested change
| on: | |
| push: | |
| branches: ['feat/windows-msi-689'] | |
| workflow_dispatch: | |
| on: | |
| pull_request: | |
| push: | |
| branches: ['feat/windows-msi-689'] | |
| workflow_dispatch: |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/test-msi.yml around lines 3 - 6, The workflow trigger for
the MSI build is too narrow and prevents PRs/forks from producing unsigned MSI
artifacts automatically. Update the trigger section in the test-msi workflow to
include a pull_request event alongside the existing push/manual dispatch
behavior, and keep the existing job logic unchanged so the MSI artifact
generation still runs through the same workflow path.
Comment on lines
+7
to
+12
| ``` | ||
| packaging/windows/ | ||
| ├── adder.wxs # WiX v4 source (product, both binaries, shortcut, ARP) | ||
| ├── build-msi.ps1 # main build script (build → wix build → jsign) | ||
| └── README.md # this file | ||
| ``` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Label the two introductory code fences.
These blocks are missing a language tag, so markdownlint will keep flagging them and GitHub won’t apply syntax highlighting. Use text for both.
🛠 Proposed fix
-```
+```text
packaging/windows/
├── adder.wxs # WiX v4 source (product, both binaries, shortcut, ARP)
├── build-msi.ps1 # main build script (build → wix build → jsign)
└── README.md # this file
-```
+```-```
+```text
%ProgramFiles%\Adder\adder.exe # CLI binary
%ProgramFiles%\Adder\adder-tray.exe # tray GUI binary
Start Menu\Programs\Adder # shortcut → adder-tray.exe
-```
+```Also applies to: 16-20
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 7-7: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packaging/windows/README.md` around lines 7 - 12, The introductory fenced
blocks in the Windows README are missing a language tag, causing markdownlint
and GitHub syntax highlighting issues. Update both code fences in the README
section shown by the examples to use the same plain-text language identifier,
and keep the content unchanged; the fix should be applied consistently to both
introductory snippets so the markdown parser treats them as labeled code blocks.
Source: Linters/SAST tools
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 5 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name=".github/workflows/publish.yml">
<violation number="1" location=".github/workflows/publish.yml:409">
P2: Windows tag builds with `tray: false` will produce no Windows release artifact even though the matrix contract says that mode should still produce the core binary. The new `&& matrix.tray` gating on the Windows upload/attest flow removes the only publish path for Windows in that configuration; a fallback upload path for `adder.exe` (or zip) would keep behavior consistent.</violation>
</file>
<file name="packaging/windows/build-msi.ps1">
<violation number="1" location="packaging/windows/build-msi.ps1:209">
P2: The keystore password / access token is leaked onto the command line via `--storepass $JsignStorePass`, making it visible in process listings on the build host. Jsign supports `--storepass env:VAR_NAME` to read the credential directly from the environment variable, keeping it off the command line. Since the script already sources `$JsignStorePass` from `$env:JSIGN_STOREPASS`, you can replace the literal password argument with `'--storepass', 'env:JSIGN_STOREPASS'` and let jsign read it directly from the environment. This is especially relevant for non-ephemeral dev machines where command-line arguments persist in process lists.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| - name: Upload release asset (Windows) | ||
| if: startsWith(github.ref, 'refs/tags/') && matrix.os == 'windows' | ||
| - name: Upload MSI release asset (Windows) | ||
| if: startsWith(github.ref, 'refs/tags/') && matrix.os == 'windows' && matrix.tray |
There was a problem hiding this comment.
P2: Windows tag builds with tray: false will produce no Windows release artifact even though the matrix contract says that mode should still produce the core binary. The new && matrix.tray gating on the Windows upload/attest flow removes the only publish path for Windows in that configuration; a fallback upload path for adder.exe (or zip) would keep behavior consistent.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At .github/workflows/publish.yml, line 409:
<comment>Windows tag builds with `tray: false` will produce no Windows release artifact even though the matrix contract says that mode should still produce the core binary. The new `&& matrix.tray` gating on the Windows upload/attest flow removes the only publish path for Windows in that configuration; a fallback upload path for `adder.exe` (or zip) would keep behavior consistent.</comment>
<file context>
@@ -306,20 +405,26 @@ jobs:
- - name: Upload release asset (Windows)
- if: startsWith(github.ref, 'refs/tags/') && matrix.os == 'windows'
+ - name: Upload MSI release asset (Windows)
+ if: startsWith(github.ref, 'refs/tags/') && matrix.os == 'windows' && matrix.tray
shell: pwsh
run: |
</file context>
| # is set, in which case invoke via java -jar. | ||
| $jsignArgs = @( | ||
| '--keystore', $JsignKeystore, | ||
| '--storepass', $JsignStorePass, |
There was a problem hiding this comment.
P2: The keystore password / access token is leaked onto the command line via --storepass $JsignStorePass, making it visible in process listings on the build host. Jsign supports --storepass env:VAR_NAME to read the credential directly from the environment variable, keeping it off the command line. Since the script already sources $JsignStorePass from $env:JSIGN_STOREPASS, you can replace the literal password argument with '--storepass', 'env:JSIGN_STOREPASS' and let jsign read it directly from the environment. This is especially relevant for non-ephemeral dev machines where command-line arguments persist in process lists.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packaging/windows/build-msi.ps1, line 209:
<comment>The keystore password / access token is leaked onto the command line via `--storepass $JsignStorePass`, making it visible in process listings on the build host. Jsign supports `--storepass env:VAR_NAME` to read the credential directly from the environment variable, keeping it off the command line. Since the script already sources `$JsignStorePass` from `$env:JSIGN_STOREPASS`, you can replace the literal password argument with `'--storepass', 'env:JSIGN_STOREPASS'` and let jsign read it directly from the environment. This is especially relevant for non-ephemeral dev machines where command-line arguments persist in process lists.</comment>
<file context>
@@ -0,0 +1,255 @@
+ # is set, in which case invoke via java -jar.
+ $jsignArgs = @(
+ '--keystore', $JsignKeystore,
+ '--storepass', $JsignStorePass,
+ '--tsaurl', $JsignTsaUrl,
+ '--name', 'Adder',
</file context>
verbotenj
force-pushed
the
feat/windows-msi-689
branch
2 times, most recently
from
July 3, 2026 16:44
49b4520 to
c3beb73
Compare
There was a problem hiding this comment.
2 issues found across 11 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packaging/windows/build-msi.ps1">
<violation number="1" location="packaging/windows/build-msi.ps1:265">
P2: A partial prebuilt-binary configuration can silently package a mixed set of binaries (one supplied, one rebuilt) instead of failing fast. This makes CI misconfiguration hard to detect and can unintentionally ship unsigned or mismatched artifacts; treating prebuilt inputs as all-or-nothing would make the release path safer.</violation>
</file>
<file name=".github/workflows/test-msi.yml">
<violation number="1" location=".github/workflows/test-msi.yml:190">
P2: This versioning scheme can eventually fail `wix build` when the workflow run number grows past MSI ProductVersion limits. The third ProductVersion field is capped at 65535, but `github.run_number` is unbounded for the workflow.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
There was a problem hiding this comment.
1 issue found across 3 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="tray/app.go">
<violation number="1" location="tray/app.go:738">
P1: Startup can attempt to re-register/start the service when service-status probing actually failed, which hides the real failure mode and can trigger unintended service changes. This happens because `ServiceStatusCheck()` errors are discarded and the fallback `ServiceNotRegistered` value is treated as authoritative; handling the error before the switch would keep self-heal logic only for confirmed states.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
There was a problem hiding this comment.
4 issues found across 9 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packaging/windows/build-msi.ps1">
<violation number="1" location="packaging/windows/build-msi.ps1:231">
P1: Release integrity can be bypassed because the build trusts downloaded Mesa DLL archives without verifying authenticity/integrity before packaging. Adding a pinned hash check (or equivalent signature verification) before extraction would prevent shipping tampered binaries in a signed MSI.</violation>
</file>
<file name="tray/setup/service_windows.go">
<violation number="1" location="tray/setup/service_windows.go:335">
P2: Restart can race the previous engine because wait timeout is ignored and execution continues as if stop succeeded. Recording timeout/wait errors as stop failures would prevent launching a second instance that can contend for the API port.</violation>
<violation number="2" location="tray/setup/service_windows.go:361">
P1: Service status and stop logic can act on the wrong process when another `adder.exe` is running, because PID selection is based only on image basename. Matching against the full executable path from the recorded command would avoid false "running" state and accidental termination of unrelated processes.</violation>
</file>
<file name="cmd/adder-tray/main.go">
<violation number="1" location="cmd/adder-tray/main.go:60">
P1: `io.MultiWriter(os.Stderr, f)` stops writing to the file if `os.Stderr.Write()` fails — which is exactly what happens on Windows GUI (`-H=windowsgui`) where stderr is discarded. The whole purpose of the log file is to capture diagnostics on Windows GUI where there's no console, but `io.MultiWriter` aborts on the first error and never reaches the file. The order matters: stderr is listed first, so every log write tries stderr first; when that fails (as it does on Windows GUI), the file never receives the write. Swap the order so the file is written first, or wrap `os.Stderr` in a discard-on-error writer, or use a custom `TeeWriter` that always writes to all targets regardless of per-target errors.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
There was a problem hiding this comment.
1 issue found across 7 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name=".github/workflows/publish.yml">
<violation number="1" location=".github/workflows/publish.yml:409">
P2: Windows tag builds with `tray: false` will produce no Windows release artifact even though the matrix contract says that mode should still produce the core binary. The new `&& matrix.tray` gating on the Windows upload/attest flow removes the only publish path for Windows in that configuration; a fallback upload path for `adder.exe` (or zip) would keep behavior consistent.</violation>
</file>
<file name="packaging/windows/adder.wxs">
<violation number="1" location="packaging/windows/adder.wxs:74">
P0: The icon file referenced by `$(var.AdderIco)` does not exist. `build-msi.ps1` defaults `$IconSrc` to `.github\assets\adder.ico`, but that directory contains only `Adder.icns` (a macOS `.icns` format). WiX requires a proper Windows `.ico` file for the `Icon` element's `SourceFile`. Without it, `wix build` will fail with a missing-source error. Add a `.ico` file to the repo (converted from the existing `Adder.icns`) and update the `ICON_SRC` default path if needed.</violation>
</file>
<file name="packaging/windows/build-msi.ps1">
<violation number="1" location="packaging/windows/build-msi.ps1:209">
P2: The keystore password / access token is leaked onto the command line via `--storepass $JsignStorePass`, making it visible in process listings on the build host. Jsign supports `--storepass env:VAR_NAME` to read the credential directly from the environment variable, keeping it off the command line. Since the script already sources `$JsignStorePass` from `$env:JSIGN_STOREPASS`, you can replace the literal password argument with `'--storepass', 'env:JSIGN_STOREPASS'` and let jsign read it directly from the environment. This is especially relevant for non-ephemeral dev machines where command-line arguments persist in process lists.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
verbotenj
force-pushed
the
feat/windows-msi-689
branch
from
July 6, 2026 20:28
91cd032 to
8d8f8dc
Compare
There was a problem hiding this comment.
3 issues found across 9 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name=".github/workflows/publish.yml">
<violation number="1" location=".github/workflows/publish.yml:409">
P2: Windows tag builds with `tray: false` will produce no Windows release artifact even though the matrix contract says that mode should still produce the core binary. The new `&& matrix.tray` gating on the Windows upload/attest flow removes the only publish path for Windows in that configuration; a fallback upload path for `adder.exe` (or zip) would keep behavior consistent.</violation>
</file>
<file name="packaging/windows/adder.wxs">
<violation number="1" location="packaging/windows/adder.wxs:74">
P0: The icon file referenced by `$(var.AdderIco)` does not exist. `build-msi.ps1` defaults `$IconSrc` to `.github\assets\adder.ico`, but that directory contains only `Adder.icns` (a macOS `.icns` format). WiX requires a proper Windows `.ico` file for the `Icon` element's `SourceFile`. Without it, `wix build` will fail with a missing-source error. Add a `.ico` file to the repo (converted from the existing `Adder.icns`) and update the `ICON_SRC` default path if needed.</violation>
</file>
<file name="packaging/windows/build-msi.ps1">
<violation number="1" location="packaging/windows/build-msi.ps1:209">
P2: The keystore password / access token is leaked onto the command line via `--storepass $JsignStorePass`, making it visible in process listings on the build host. Jsign supports `--storepass env:VAR_NAME` to read the credential directly from the environment variable, keeping it off the command line. Since the script already sources `$JsignStorePass` from `$env:JSIGN_STOREPASS`, you can replace the literal password argument with `'--storepass', 'env:JSIGN_STOREPASS'` and let jsign read it directly from the environment. This is especially relevant for non-ephemeral dev machines where command-line arguments persist in process lists.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
verbotenj
force-pushed
the
feat/windows-msi-689
branch
from
July 6, 2026 23:24
35f84e1 to
d95d5d3
Compare
wolf31o2
requested changes
Jul 7, 2026
wolf31o2
left a comment
wolf31o2
left a comment
Member
There was a problem hiding this comment.
Requesting changes: the service registration/restart ordering can persist a new config without restarting an already-running engine, so Apply/Reconfigure may report success while the process keeps using the old configuration.
| "stage", "service-restart", | ||
| "error", err) | ||
| } | ||
| } else if err := r.Service.EnsureRegistered(binPath, engineCfgPath); err != nil { |
Member
There was a problem hiding this comment.
This new ordering breaks config-change restarts. EnsureRegistered writes the desired unit/command file first; then RestartIfConfigChanged compares desired against that already-updated file and sees no diff. If the engine is already running, it falls through to EnsureRunning and does not restart, leaving the old config live.
| if !strings.HasPrefix(path, "schtasks://") { | ||
| existing, _ = os.ReadFile(path) | ||
| } | ||
| existing, _ := os.ReadFile(path) |
Member
There was a problem hiding this comment.
This diff check only works if existing is read before the desired service definition is written. With the current caller ordering, existing has already been replaced by EnsureRegistered, so this path misses real config changes. Consider having RestartIfConfigChanged handle missing registration itself, or return a changed/registered signal from EnsureRegistered and force a restart when it updates the unit.
There was a problem hiding this comment.
2 issues found across 4 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name=".github/workflows/publish.yml">
<violation number="1" location=".github/workflows/publish.yml:409">
P2: Windows tag builds with `tray: false` will produce no Windows release artifact even though the matrix contract says that mode should still produce the core binary. The new `&& matrix.tray` gating on the Windows upload/attest flow removes the only publish path for Windows in that configuration; a fallback upload path for `adder.exe` (or zip) would keep behavior consistent.</violation>
</file>
<file name="packaging/windows/adder.wxs">
<violation number="1" location="packaging/windows/adder.wxs:74">
P0: The icon file referenced by `$(var.AdderIco)` does not exist. `build-msi.ps1` defaults `$IconSrc` to `.github\assets\adder.ico`, but that directory contains only `Adder.icns` (a macOS `.icns` format). WiX requires a proper Windows `.ico` file for the `Icon` element's `SourceFile`. Without it, `wix build` will fail with a missing-source error. Add a `.ico` file to the repo (converted from the existing `Adder.icns`) and update the `ICON_SRC` default path if needed.</violation>
</file>
<file name="packaging/windows/build-msi.ps1">
<violation number="1" location="packaging/windows/build-msi.ps1:209">
P2: The keystore password / access token is leaked onto the command line via `--storepass $JsignStorePass`, making it visible in process listings on the build host. Jsign supports `--storepass env:VAR_NAME` to read the credential directly from the environment variable, keeping it off the command line. Since the script already sources `$JsignStorePass` from `$env:JSIGN_STOREPASS`, you can replace the literal password argument with `'--storepass', 'env:JSIGN_STOREPASS'` and let jsign read it directly from the environment. This is especially relevant for non-ephemeral dev machines where command-line arguments persist in process lists.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
verbotenj
force-pushed
the
feat/windows-msi-689
branch
7 times, most recently
from
July 9, 2026 17:33
60bbb46 to
2638b25
Compare
wolf31o2
requested changes
Jul 9, 2026
wolf31o2
left a comment
wolf31o2
left a comment
Member
There was a problem hiding this comment.
Reposting against the current PR head (2638b2532ef6ddf23d1ee28ff7cb3b2bf611c39a). My prior review was accidentally attached to stale head 8a4142e2e46bc435ee55db6148041a4b6a1a47ec. The startup/AutoStart issue appears fixed in this head, but Windows registration still has one blocker.
| fpPath := serviceStatePath() | ||
| got, _ := os.ReadFile(fpPath) | ||
|
|
||
| if !bytes.Equal(got, want) { |
Member
There was a problem hiding this comment.
This fingerprint only covers the rendered engine command plus config.yaml contents. On Windows, registration also includes the HKCU Run value that launches adder-tray.exe; that value can be missing or stale while this fingerprint still matches. In that case this branch skips registerService and falls through to EnsureRunning, which can start the engine from service-command.txt without repairing login startup. The idempotency check needs to include platform registration state / the Run value target, or Windows needs to force registerService before starting when the Run value is absent or points somewhere else.
wolf31o2
dismissed
their stale review
July 9, 2026 20:40
Superseded by replacement review on current head 2638b25.
Build a signed Windows .msi containing both binaries (adder.exe CLI and adder-tray.exe GUI) installed under %ProgramFiles%\Adder\, with a Start Menu shortcut for the tray and full Add/Remove Programs metadata. - packaging/windows/adder.wxs: WiX v4 source. Fixed UpgradeCode + auto ProductCode, MajorUpgrade downgrade block, per-component KeyPaths, ARP metadata (publisher/contact/help/about/update URLs). Deliberately does NOT register a Scheduled Task; the adder-tray first-run wizard owns service registration. 64-bit components (Bitness="always64"), valid for both x64 and arm64. - packaging/windows/build-msi.ps1: builds both binaries (CGO for the Fyne tray), pins WiX 4.0.5, signs the MSI via jsign, and passes the WiX `-arch` platform per target ($WixArch: amd64->x64, arm64->arm64) so arm64 builds are stamped correctly. - packaging/windows/README.md: env-var contract and verification steps. - .github/workflows/publish.yml: wire the MSI into the release job for the windows matrix rows (sign binaries -> build+sign MSI -> signtool verify -> upload + attest). Also quiet pre-existing actionlint/shellcheck nits (quote $GITHUB_ENV, $() over backticks in the docker manifest job). - .github/workflows/test-msi.yml: PR-only workflow producing installable artifacts. Signing is optional and gated on a check_signing step output (the secrets context is not available in step-level `if:`), so forks still build an UNSIGNED msi. Signed-off-by: Ales Verbic <verbotenj@blinklabs.io>
Wizard window used SetFixedSize with unscrolled step content, so tall steps ballooned the window, jumped size when target chips were added, and a squeezed box layout gave the step-3 Spacer negative height — overlapping the output section onto the target lists with no way to reach Next. Wrap step content in a VScroll (matching RulesEditor), drop SetFixedSize for a constant 1024x800 resizable window, and remove the Spacer. Add a dev-only screenshot harness + make wizard-screenshots. Signed-off-by: Ales Verbic <verbotenj@blinklabs.io>
The setup wizard finished with "Failed to (re)start the adder service:
starting scheduled task: ERROR: The system cannot find the file
specified." on a clean Windows install. SetupRunner.Apply only called
RestartIfConfigChanged, which on first run skips to EnsureRunning ->
startService (schtasks /Run) against a scheduled task that was never
created. EnsureRegistered existed but was never wired into the apply
flow. The same first-run gap affected macOS/Linux (start before the
launchd/systemd unit is installed).
Call EnsureRegistered before RestartIfConfigChanged; registration
failures surface as the existing soft ServiceRestartErr and skip the
restart attempt.
Also reword the notifications step: the System Verification blurb and
the "didn't show" hint were macOS-specific ("macOS requires...",
"System Settings > Notifications"). Make both platform-neutral.
Signed-off-by: Ales Verbic <verbotenj@blinklabs.io>
The Windows MSI smoke-test workflow is not needed in future PRs, so drop it from the repo (and gitignore it). The file is kept on disk for local, on-demand runs only. Signed-off-by: Ales Verbic <verbotenj@blinklabs.io>
- Log file is written before os.Stderr in the tray's io.MultiWriter. MultiWriter stops at the first writer that errors; under -H=windowsgui os.Stderr.Write fails, so stderr-first meant the log file (the whole point on Windows) was never written. - setupTray only self-heals/autostarts on a confirmed service status. A dropped ServiceStatusCheck error previously fell back to ServiceNotRegistered and could spuriously re-register and restart the engine on a transient probe failure. - terminateEngine surfaces a WaitForSingleObject timeout/error as a stop failure so a restart cannot launch a second engine that races the dying one for the API port. Signed-off-by: Ales Verbic <verbotenj@blinklabs.io>
- Verify the downloaded Mesa archive against a pinned SHA-256 before extraction so a tampered/MITM'd payload cannot be baked into the signed MSI. Overridable via MESA_SHA256 (required when bumping MESA_VERSION). - Match engine processes by full image path, not just the adder.exe basename, so an unrelated adder.exe elsewhere is not reported as "running" or terminated. A candidate is skipped only when its path is readable AND confirmed different, so a query failure never loses our own engine. - Render the vote cast by the followed DRep (voteFor .params) instead of the first voting procedure, so a governance event carrying several votes no longer shows a different DRep's vote. Signed-off-by: Ales Verbic <verbotenj@blinklabs.io>
Re-add .github/workflows/test-msi.yml (and drop its gitignore entry) so the MSI smoke-test builds run on push again while Windows testing is in progress. Also address the review findings on this workflow: - quote the --storetype/--keystore/--alias jsign secret args - SHA-pin msys2/setup-msys2 (v2.32.0) - cap the derived MSI ProductVersion at the 65535 third-field limit (github.run_number modulo 65536) Signed-off-by: Ales Verbic <verbotenj@blinklabs.io>
There was a problem hiding this comment.
7 issues found across 49 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="go.mod">
<violation number="1" location="go.mod:11">
P1: Dependency `plutigo` downgraded from v0.1.16 (target branch `main`) to v0.1.15 without stated reason. This was previously updated to v0.1.16 in an earlier PR and this change silently reverts it.</violation>
<violation number="2" location="go.mod:130">
P2: Transitive dependency `golang.org/x/crypto` downgraded from v0.53.0 to v0.52.0. While indirect, this was probably auto-updated by `go mod tidy` when the direct dependency versions changed. Still, this is a regression from the target branch state and should be restored.</violation>
</file>
<file name="output/telegram/telegram_test.go">
<violation number="1" location="output/telegram/telegram_test.go:27">
P3: The `getBaseURL` function is now a thin wrapper that delegates directly to `explorer.BaseURL`. With that delegation, the `telegram_test.go` constants (`mainnetNetworkMagic`, `previewNetworkMagic`, `preprodNetworkMagic`) duplicate the magics already defined (privately) in the `explorer` package. Since the tests only need magics to feed the `getBaseURL` → `explorer.BaseURL` chain, they should use the same literal values that `explorer.BaseURL` switches on. The duplication is small but creates a maintenance hazard: if the explorer or the preprod/preview magics ever change, the test would pass even after the broken change because the test uses its own set of magic constants rather than the actual ones the production code checks. Consider referencing the explorer package's constants if they're exported, or keeping a single source of truth via `explorer_test.go`'s table-driven approach. At HIGH sensitivity, this duplication in a newly introduced test-constants block is worth noting.</violation>
</file>
<file name=".github/workflows/publish.yml">
<violation number="1" location=".github/workflows/publish.yml:112">
P2: Downgrading `actions/setup-go` from v6.5.0 to v6.4.0 introduces a regression risk: v6.5.0 (sha `924ae3a`) includes a critical Go toolchain cache fix and updated hashFiles globbing that v6.4.0 (sha `4a36011`) lacks. Restore v6.5.0 unless there's a specific compatibility reason to stay on the older version.</violation>
<violation number="2" location=".github/workflows/publish.yml:189">
P2: Downgrading `actions/setup-java` from v5.4.0 to v5.3.0 loses the dependency caching improvements and SDKMAN! integration fix in v5.4.0. Restore v5.4.0 unless there's a specific reason to pin the older version.</violation>
<violation number="3" location=".github/workflows/publish.yml:474">
P2: Removed the standalone `Attest binary (Windows)` step that attested `${{ env.APPLICATION_NAME }}.exe`. Only the MSI is now attested for Windows, but the unsigned PE inside the MSI is no longer independently attestable. If downstream consumers need attestation on the raw binary (e.g. for direct `adder.exe` downloads without the MSI), this step should be preserved.</violation>
</file>
<file name=".github/workflows/golangci-lint.yml">
<violation number="1" location=".github/workflows/golangci-lint.yml:19">
P3: This delta downgrades both `actions/setup-go` from v6.5.0 to v6.4.0 and `golangci/golangci-lint-action` from v9.3.0 to v9.2.1 without explanation. The target branch (`main`) and the previously reviewed PR state both used the newer versions. If there's no reason to revert, keep the versions that were already in place to avoid losing the fixes and features in the newer releases (e.g., `no-run-logs-group` option in v9.3.0).</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| github.com/blinklabs-io/gouroboros v0.186.1 | ||
| github.com/blinklabs-io/plutigo v0.1.16 | ||
| github.com/blinklabs-io/gouroboros v0.183.0 | ||
| github.com/blinklabs-io/plutigo v0.1.15 |
There was a problem hiding this comment.
P1: Dependency plutigo downgraded from v0.1.16 (target branch main) to v0.1.15 without stated reason. This was previously updated to v0.1.16 in an earlier PR and this change silently reverts it.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At go.mod, line 11:
<comment>Dependency `plutigo` downgraded from v0.1.16 (target branch `main`) to v0.1.15 without stated reason. This was previously updated to v0.1.16 in an earlier PR and this change silently reverts it.</comment>
<file context>
@@ -7,8 +7,8 @@ require (
- github.com/blinklabs-io/gouroboros v0.186.1
- github.com/blinklabs-io/plutigo v0.1.16
+ github.com/blinklabs-io/gouroboros v0.183.0
+ github.com/blinklabs-io/plutigo v0.1.15
github.com/btcsuite/btcd/btcutil v1.2.0
github.com/gen2brain/beeep v0.11.2
</file context>
Suggested change
| github.com/blinklabs-io/plutigo v0.1.15 | |
| github.com/blinklabs-io/plutigo v0.1.16 |
| go.yaml.in/yaml/v3 v3.0.4 // indirect | ||
| golang.org/x/arch v0.27.0 // indirect | ||
| golang.org/x/crypto v0.53.0 // indirect | ||
| golang.org/x/crypto v0.52.0 // indirect |
There was a problem hiding this comment.
P2: Transitive dependency golang.org/x/crypto downgraded from v0.53.0 to v0.52.0. While indirect, this was probably auto-updated by go mod tidy when the direct dependency versions changed. Still, this is a regression from the target branch state and should be restored.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At go.mod, line 130:
<comment>Transitive dependency `golang.org/x/crypto` downgraded from v0.53.0 to v0.52.0. While indirect, this was probably auto-updated by `go mod tidy` when the direct dependency versions changed. Still, this is a regression from the target branch state and should be restored.</comment>
<file context>
@@ -127,7 +127,7 @@ require (
go.yaml.in/yaml/v3 v3.0.4 // indirect
golang.org/x/arch v0.27.0 // indirect
- golang.org/x/crypto v0.53.0 // indirect
+ golang.org/x/crypto v0.52.0 // indirect
golang.org/x/mod v0.36.0 // indirect
golang.org/x/net v0.55.0 // indirect
</file context>
|
|
||
| // Cardano network magics used by the tests as BlockContext fixtures and to | ||
| // exercise getBaseURL's mapping. | ||
| const ( |
There was a problem hiding this comment.
P3: The getBaseURL function is now a thin wrapper that delegates directly to explorer.BaseURL. With that delegation, the telegram_test.go constants (mainnetNetworkMagic, previewNetworkMagic, preprodNetworkMagic) duplicate the magics already defined (privately) in the explorer package. Since the tests only need magics to feed the getBaseURL → explorer.BaseURL chain, they should use the same literal values that explorer.BaseURL switches on. The duplication is small but creates a maintenance hazard: if the explorer or the preprod/preview magics ever change, the test would pass even after the broken change because the test uses its own set of magic constants rather than the actual ones the production code checks. Consider referencing the explorer package's constants if they're exported, or keeping a single source of truth via explorer_test.go's table-driven approach. At HIGH sensitivity, this duplication in a newly introduced test-constants block is worth noting.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At output/telegram/telegram_test.go, line 27:
<comment>The `getBaseURL` function is now a thin wrapper that delegates directly to `explorer.BaseURL`. With that delegation, the `telegram_test.go` constants (`mainnetNetworkMagic`, `previewNetworkMagic`, `preprodNetworkMagic`) duplicate the magics already defined (privately) in the `explorer` package. Since the tests only need magics to feed the `getBaseURL` → `explorer.BaseURL` chain, they should use the same literal values that `explorer.BaseURL` switches on. The duplication is small but creates a maintenance hazard: if the explorer or the preprod/preview magics ever change, the test would pass even after the broken change because the test uses its own set of magic constants rather than the actual ones the production code checks. Consider referencing the explorer package's constants if they're exported, or keeping a single source of truth via `explorer_test.go`'s table-driven approach. At HIGH sensitivity, this duplication in a newly introduced test-constants block is worth noting.</comment>
<file context>
@@ -22,6 +22,14 @@ import (
+// Cardano network magics used by the tests as BlockContext fixtures and to
+// exercise getBaseURL's mapping.
+const (
+ mainnetNetworkMagic uint32 = 764824073
+ previewNetworkMagic uint32 = 2
</file context>
| steps: | ||
| - uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0 https://github.com/actions/checkout/releases/tag/v7.0.0 | ||
| - uses: actions/setup-go@924ae3a1cded613372ab5595356fb5720e22ba16 # v6.5.0 https://github.com/actions/setup-go/releases/tag/v6.5.0 | ||
| - uses: actions/setup-go@4a3601121dd01d1626a1e23e37211e3254c1c06c # v6.4.0 https://github.com/actions/setup-go/releases/tag/v6.4.0 |
There was a problem hiding this comment.
P3: This delta downgrades both actions/setup-go from v6.5.0 to v6.4.0 and golangci/golangci-lint-action from v9.3.0 to v9.2.1 without explanation. The target branch (main) and the previously reviewed PR state both used the newer versions. If there's no reason to revert, keep the versions that were already in place to avoid losing the fixes and features in the newer releases (e.g., no-run-logs-group option in v9.3.0).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At .github/workflows/golangci-lint.yml, line 19:
<comment>This delta downgrades both `actions/setup-go` from v6.5.0 to v6.4.0 and `golangci/golangci-lint-action` from v9.3.0 to v9.2.1 without explanation. The target branch (`main`) and the previously reviewed PR state both used the newer versions. If there's no reason to revert, keep the versions that were already in place to avoid losing the fixes and features in the newer releases (e.g., `no-run-logs-group` option in v9.3.0).</comment>
<file context>
@@ -16,12 +16,12 @@ jobs:
steps:
- uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0 https://github.com/actions/checkout/releases/tag/v7.0.0
- - uses: actions/setup-go@924ae3a1cded613372ab5595356fb5720e22ba16 # v6.5.0 https://github.com/actions/setup-go/releases/tag/v6.5.0
+ - uses: actions/setup-go@4a3601121dd01d1626a1e23e37211e3254c1c06c # v6.4.0 https://github.com/actions/setup-go/releases/tag/v6.4.0
with:
go-version: 1.26.x
</file context>
Suggested change
| - uses: actions/setup-go@4a3601121dd01d1626a1e23e37211e3254c1c06c # v6.4.0 https://github.com/actions/setup-go/releases/tag/v6.4.0 | |
| - uses: actions/setup-go@924ae3a1cded613372ab5595356fb5720e22ba16 # v6.5.0 https://github.com/actions/setup-go/releases/tag/v6.5.0 |
verbotenj
force-pushed
the
feat/windows-msi-689
branch
2 times, most recently
from
July 12, 2026 17:02
ba0f002 to
e5f0f04
Compare
Drop the branch push trigger; keep workflow_dispatch so the MSI smoke test runs on demand only. Signed-off-by: Ales Verbic <verbotenj@blinklabs.io>
Recent-events transaction links used the block hash instead of the transaction hash: transactionHash lives in the event Context while blockHash lives in the Payload, and the menu builder read only Payload (blockHash first). Every transaction linked to /transactions/<blockHash> (404), and all transactions in one block shared the same link. Links now resolve the hash by event type via eventLinkHash: transactionHash from Context for transaction/governance events, blockHash from Payload for blocks; governance now routes to /transactions/. Recent events from a previous network survived a network switch, so their explorer links resolved on the wrong chain. applyPlan now drops cached events whose network magic differs from the newly-applied network. The tray now starts the engine whenever the service is registered and a config exists (previously gated on AutoStart), so launching the tray always brings monitoring up. Windows: ensure exactly one engine. An MSI upgrade over a running engine renamed the in-use adder.exe to a .rbf rollback file; the old process kept running and escaped image-name matching, so engines accumulated and the tray connected to a stale one. The tray now records the launched engine PID and terminates by PID (robust to rename) and matches engine processes by install directory rather than file name (catches the .rbf), and treats an un-killable but live process as a hard error instead of silently launching another. The installer additionally closes adder-tray.exe and adder.exe (util:CloseApplication) before replacing files, so the .rbf rename never happens. chainsync logs the network name and magic on connect so a config switch is visible in the logs. RestartIfConfigChanged keys on a fingerprint of the service command AND the engine config contents, so a reconfigure that only rewrites config.yaml restarts the engine; an unchanged config does not. On macOS, registerService retries the launchctl bootstrap on the transient 'Bootstrap failed: 5' (EIO) race after bootout. Block header events now carry NetworkMagic (was unset), so tray icon status and explorer network selection are correct. Explorer links (tray, webhook, telegram) moved to adastat.net (/transactions, /blocks) with preprod/preview subdomains. Signed-off-by: Ales Verbic <verbotenj@blinklabs.io>
verbotenj
force-pushed
the
feat/windows-msi-689
branch
9 times, most recently
from
July 13, 2026 01:25
ef11770 to
9b4f013
Compare
Add a Windows tray distribution: MSI packaging, per-user HKCU Run-key autostart of the GUI tray, and a detached windowless engine child managed by recorded PID. Engine start now recovers when a stale PID has been reused by a protected or other-user process instead of deadlocking. Also: shared explorer URL helper, notification rules and filter logic, config-change restart, service registration assertion, and cleanup across the tray, wizard, and output plugins. Signed-off-by: Ales Verbic <verbotenj@blinklabs.io>
verbotenj
force-pushed
the
feat/windows-msi-689
branch
from
July 13, 2026 01:50
9b4f013 to
d317715
Compare
There was a problem hiding this comment.
All reported issues were addressed across 22 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
verbotenj
force-pushed
the
feat/windows-msi-689
branch
from
July 14, 2026 02:40
576eb77 to
990e749
Compare
Signed-off-by: Ales Verbic <verbotenj@blinklabs.io>
verbotenj
force-pushed
the
feat/windows-msi-689
branch
from
July 14, 2026 02:51
990e749 to
ced68db
Compare
wolf31o2
requested changes
Jul 14, 2026
wolf31o2
left a comment
wolf31o2
left a comment
Member
There was a problem hiding this comment.
NO-GO for merge. The updated Windows service lifecycle still has a stale-PID path that can make the tray believe the engine is running when it is not, so startup can leave users permanently reconnecting until they manually intervene.
| } | ||
|
|
||
| if strings.Contains(string(out), "Running") { | ||
| if pid, ok := readEnginePID(); ok && processAlive(pid) { |
Member
There was a problem hiding this comment.
This status check still treats any live PID recorded in engine.pid as the Adder engine. That file can survive a crash/reboot and Windows can later reuse the PID for an unrelated process; in that case this returns ServiceRunning even though adder.exe is not running. Please validate the PID's image/full path against the registered engine command, or clear/ignore the stale PID, before returning ServiceRunning.
| if err := registerService(cfg); err != nil { | ||
| got, _ := os.ReadFile(serviceStatePath()) | ||
|
|
||
| if !bytes.Equal(got, want) { |
Member
There was a problem hiding this comment.
For the matching-fingerprint path, this method falls through to EnsureRunning() below instead of forcing a restart. That makes the Windows ServiceRunning check security-critical: if it accepts a reused foreign PID from engine.pid, startup will skip startService() and the tray will keep retrying against an engine that was never launched.
Only report the Windows engine as running when the recorded PID is alive and its full image path matches the registered command. Otherwise clear the stale PID so unchanged configurations fall through EnsureRunning and relaunch the engine. Cover missing identity, reused same-name foreign PIDs, valid matches, and concurrent PID replacement cleanup with Windows-specific tests. Signed-off-by: Ales Verbic <verbotenj@blinklabs.io>
verbotenj
force-pushed
the
feat/windows-msi-689
branch
from
July 14, 2026 20:34
0463bc9 to
e9241ec
Compare
wolf31o2
approved these changes
Jul 14, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Build a signed Windows .msi containing both binaries (adder.exe CLI and adder-tray.exe GUI) installed under %ProgramFiles%\Adder, with a Start Menu shortcut for the tray and full Add/Remove Programs metadata.
-archplatform per target ($WixArch: amd64->x64, arm64->arm64) so arm64 builds are stamped correctly.if:), so forks still build an UNSIGNED msi.Summary by cubic
Ships a signed Windows MSI with a per‑user autostarting tray and a hardened engine lifecycle. Adds target‑based notifications with simple/advanced rules, scoped DRep/pool alerts, and correct, network‑aware explorer links.
New Features
WiXv4 MSI (x64/arm64) installsadder.exeandadder-tray.exeto %ProgramFiles%\Adder\ with a Start Menu shortcut; built viapackaging/windows/build-msi.ps1, signed withjsign, closes runningadder*.exeon upgrade, and bundles Mesa llvmpipe (amd64, SHA‑256 verified).-H=windowsgui), single‑instance mutex, per‑user HKCU Run autostart; engine launched detached and tracked by PID and full image path/dir; logs to%LOCALAPPDATA%\Adder\Logs\adder-tray.log; panic recovery.docs/adder-tray-filtering.md.internal/explorerwith correct routes/hashes and network selection fromNetworkMagic.Shutdown(ctx)for graceful stop;/eventssupports?replay=; tray requests live‑only on first connect and replays on reconnect; block header events carryNetworkMagic.test-msiworkflow.Bug Fixes
-H=windowsgui.Written for commit e9241ec. Summary will update on new commits.
Summary by CodeRabbit
New Features
Documentation
Bug Fixes