fix(firewall): cache the Windows binary as sfw.exe so the shims can run it - #24
Conversation
f706ec9 to
64040f1
Compare
4ba8fc2 to
fecefa2
Compare
| } | ||
|
|
||
| const pathBinary = path.join(pathCache, FIREWALL_EXEC_NAME) | ||
| const pathBinary = path.join(pathCache, FIREWALL_EXEC_FILE) |
There was a problem hiding this comment.
Blocking: an existing cache entry can point at a binary that isn't there.
Earlier action versions cached socket-firewall-<edition>/1.15.3/<arch>/sfw (no .exe) under the same cache key. find() (L168-170) only checks that the directory and its .complete marker exist, so on a self-hosted Windows runner that keeps its tool cache, the download is skipped. This line then builds ...\sfw.exe, which doesn't exist. The step goes green, and every shim fails afterwards until someone clears the cache.
Fix: if FIREWALL_EXEC_FILE isn't in pathCache after find(), treat it as a cache miss. Please add a unit test where find returns a directory containing only sfw.
There was a problem hiding this comment.
Done in 8c28dcd. findCachedFirewall wraps find and treats an entry without FIREWALL_EXEC_FILE as a miss, so the download runs and cacheFile replaces the stale entry under the same key.
Unit tests cover the three cases: no entry, an entry holding the binary this version runs, and an entry holding only the other name (sfw on Windows, sfw.exe elsewhere, so the miss is exercised on every CI platform rather than only on the Windows legs).
…un it Since the shims landed (bad87d6, Aug 4) every Windows install through this action with the default `shims: true` has failed: the binary is cached as `.../sfw` with no suffix, and the `.cmd` shims run it through cmd.exe, which does not execute a suffix-less file ("is not recognized as an internal or external command"). Bash on a Windows runner does, so `sfw npm install` typed in a workflow started fine; it then resolved `npm` to the shim, and the shim failed. The CI simulation on main showed 0 of 10 installs succeeding on windows-2025 and windows-11-arm against 10 of 10 for the pre-shim v1.3.2 action. Cache the file under FIREWALL_EXEC_FILE, `sfw.exe` on Windows and `sfw` elsewhere, the way PATCH_EXEC_NAME already does, and point the binary path and therefore the shims at it. FIREWALL_EXEC_NAME stays the bare name the release asset names are built from. A tool cache kept between jobs can still hold an entry an earlier version wrote under the same key, holding `sfw` and no `sfw.exe`. `find` only checks the version directory and its `.complete` marker, so it would hand that entry back and the step would go green with a binary path that does not exist. findCachedFirewall treats an entry without FIREWALL_EXEC_FILE as a miss, and the fresh download replaces it.
fecefa2 to
8c28dcd
Compare
This comment was marked as low quality.
This comment was marked as low quality.
Andre Coetzee (Andre153)
left a comment
There was a problem hiding this comment.
Cache check looks good. Ship in the same release as #18.
sfw v1.15.4 restores the "not found in PATH" verdict for a command PowerShell genuinely cannot resolve on Windows (SocketDev/firewall#210); v1.15.3 reported that case as a resolver error. The action installs only the version its checksum table covers, so the fix is not installable through it until this bump. Recompute all twelve checksums from the published v1.15.4 assets and rebuild dist/. Also move findCachedFirewall above firewallDownloadUrls. #18 and #24 each passed lint on their own, but merging both left the export out of the alphabetical order the sort-source-methods rule wants, which fails lint on main and blocks any commit touching this file.
sfw v1.15.4 restores the "not found in PATH" verdict for a command PowerShell genuinely cannot resolve on Windows (SocketDev/firewall#210); v1.15.3 reported that case as a resolver error. The action installs only the version its checksum table covers, so the fix is not installable through it until this bump. Recompute all twelve checksums from the published v1.15.4 assets and rebuild dist/. Also move findCachedFirewall above firewallDownloadUrls. #18 and #24 each passed lint on their own, but merging both left the export out of the alphabetical order the sort-source-methods rule wants, which fails lint on main and blocks any commit touching this file.
Why
Found by the first dispatch of the CI simulation on
main(run 36721798455): every Windows install through the current action failed, deterministically, while the pre-shimv1.3.2action passed.main)main)main)The install log:
The binary is cached as
sfwwith no suffix. The.cmdshims the action writes run it throughcmd.exe, which does not execute a suffix-less file, and neither does PowerShell. Bash on a Windows runner does, which is whysfw npm installtyped in a workflow started fine: it then resolvednpmto the shim, and the shim failed. So with the defaultshims: true, every Windows install viamainhas been broken since the shims landed inbad87d6on Aug 4. Nobody hit it because no release has been tagged since March and current customer pins predate the shims.What
Cache the file under
FIREWALL_EXEC_FILE,sfw.exeon Windows andsfwelsewhere, the wayPATCH_EXEC_NAMEalready does, and point the binary path (and therefore the shims) at it.FIREWALL_EXEC_NAMEstays the bare name the release asset names are built from. One unit test.dist/rebuilt.Stacked on #23 so that dispatching the simulation on this branch produces a report.
Verification
Run 36723011554, the simulation dispatched on this branch, 3 iterations per runner:
Windows goes from 0 of 10 on
mainto 3 of 3 here with only the file-name change.🤖 Generated with Claude Code
Note
Medium Risk
Changes firewall install and cache paths on Windows (high-impact for CI), but scope is narrow and guarded by cache miss logic plus new unit tests.
Overview
Fixes broken Windows installs when package-manager shims are enabled: the action cached the firewall as
sfwwithout an extension, but.cmdshims invoke the binary throughcmd.exe, which requiressfw.exe.Introduces
FIREWALL_EXEC_FILE(sfw.exeon Windows,sfwelsewhere—matching the existing patch binary naming) and uses it for tool-cache storage, the resolved binary path, and shim targets. AddsfindCachedFirewallso a runner tool-cache hit from older action versions (directory present but wrong filename) is treated as a miss and re-downloaded.dist/main.jsis rebuilt; checksum reads switch toreadFilefromnode:fs/promises.Unit tests cover the platform-specific filename and cache validation behavior.
Reviewed by Cursor Bugbot for commit 8c28dcd. Configure here.