fix(run): resolve bundled binary on Windows Git Bash / MSYS2 / Cygwin#174
Open
keith-mvs wants to merge 3 commits into
Open
fix(run): resolve bundled binary on Windows Git Bash / MSYS2 / Cygwin#174keith-mvs wants to merge 3 commits into
keith-mvs wants to merge 3 commits into
Conversation
The launcher derives the platform from `uname -s` lowercased, which under
Git Bash / MSYS2 / Cygwin is `mingw64_nt-<winbuild>` (e.g.
mingw64_nt-10.0-26200), not `windows`. It then probes `bin/lumen` and
`bin/lumen-${OS}-${ARCH}` with no `.exe` suffix, so it never matches the
shipped `bin/lumen-windows-amd64.exe` and falls through to a GitHub download
for a non-existent `lumen-<ver>-mingw64_nt-...-amd64` asset. The download
404s and the PreToolUse (Grep|Bash) hook exits non-zero on every tool call.
Normalize mingw*/msys*/cygwin* to `windows` and append the `.exe` suffix to
the binary candidates (and the download fallback path) so the bundled Windows
binary resolves with no download attempted.
Tested under Git Bash on Windows 11: `scripts/run version` prints the version
with no download, and `hook pre-tool-use` exits 0.
Signed-off-by: Keith Fleming <248089218+keith-mvs@users.noreply.github.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughNormalize Windows-like ChangesWindows Platform and Executable Path Handling
CI Runtime Pinning
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Updates the scripts/run launcher to correctly detect and execute the bundled Windows binary under Git Bash/MSYS2/Cygwin-style environments.
Changes:
- Normalizes Windows-like
unameoutputs (mingw/msys/cygwin) toOS=windows. - Introduces an executable suffix (
.exe) for Windows-family environments so local binary discovery and download targets match packaged artifacts.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/run (1)
55-56:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winWindows download URL still omits
.exesuffix.Line 55 and Line 82 build
ASSETwithout${EXT}, so Windows still requests a non-existent asset name and fails bootstrap download on first run.🔧 Proposed fix
- ASSET="lumen-${VERSION#v}-${OS}-${ARCH}" + ASSET="lumen-${VERSION#v}-${OS}-${ARCH}${EXT}" URL="https://github.com/${REPO}/releases/download/${VERSION}/${ASSET}" @@ - ASSET="lumen-${VERSION#v}-${OS}-${ARCH}" + ASSET="lumen-${VERSION#v}-${OS}-${ARCH}${EXT}" URL="https://github.com/${REPO}/releases/download/${VERSION}/${ASSET}"Also applies to: 82-83
🤖 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 `@scripts/run` around lines 55 - 56, The ASSET variable is built without the platform-specific extension, so on Windows the download URL (constructed from ASSET and used in URL) omits the .exe and fails; update the ASSET construction (and any other places building ASSET, e.g., the second occurrence around where ASSET is re-set) to append the ${EXT} variable (use ASSET="lumen-${VERSION#v}-${OS}-${ARCH}${EXT}") and ensure the URL uses that ASSET so Windows receives the .exe-suffixed artifact.
🤖 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.
Outside diff comments:
In `@scripts/run`:
- Around line 55-56: The ASSET variable is built without the platform-specific
extension, so on Windows the download URL (constructed from ASSET and used in
URL) omits the .exe and fails; update the ASSET construction (and any other
places building ASSET, e.g., the second occurrence around where ASSET is re-set)
to append the ${EXT} variable (use
ASSET="lumen-${VERSION#v}-${OS}-${ARCH}${EXT}") and ensure the URL uses that
ASSET so Windows receives the .exe-suffixed artifact.
The E2E job's lang suite (TestLang_*) compares committed embedding snapshots in testdata/snapshots/. Those rankings are produced by all-minilm via the Ollama runtime, which the workflow pulled as `ollama/ollama:latest`. Between the last green main run (2026-05-20) and 2026-06-04 the `:latest` tag advanced from 0.24.0 to a newer build, changing the embedding vectors and breaking every snapshot. This is unrelated to the scripts/run change in this PR and would also fail on main if re-run. Pin the service image to 0.24.0 (digest sha256:a6149234667efc71d37766d61c1a16f24c33e4cd7a0bf4125c44a7e47e2419c4) — the last runtime the snapshots were verified green against — so the suite is reproducible. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.github/workflows/ci.yml (1)
82-82: ⚡ Quick winConsider appending the digest to guarantee immutability.
The comment on line 81 documents the specific digest, but the image specification uses only the tag. While Docker tags for official images are generally stable, appending the digest ensures absolute reproducibility even if the tag is republished.
🔒 Proposed enhancement for immutable image reference
- image: ollama/ollama:0.24.0 + image: ollama/ollama:0.24.0@sha256:a6149234667efc71d37766d61c1a16f24c33e4cd7a0bf4125c44a7e47e2419c4🤖 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/ci.yml at line 82, Update the GitHub Actions image reference to an immutable digest by appending the documented sha256 to the existing image name (replace the current "image: ollama/ollama:0.24.0" value with the same image reference including the `@sha256`:<digest> suffix), ensuring the workflow uses the exact digest pinned image; keep the tag if desired but include the `@sha256` to guarantee reproducibility.
🤖 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/ci.yml:
- Line 82: The pinned Ollama image digest is incorrect for the tag used; update
the image reference on the image: ollama/ollama:0.24.0 line to either remove the
digest and rely on the tag or pin to the correct digest for the target
architecture (e.g., use amd64
sha256:7db82c93ff9fae4011a2aa10931b1d2c2a228d508b8736366e3fbaee616b2e8c or arm64
sha256:32f9c3895f3a37cfcf637da5cabfd2d5d600cf3222ab294b366dae316e429589), or pin
the multi-arch manifest digest if you need deterministic images; also add/enable
a vulnerability scan step in the workflow and ensure runtime exposure controls
(do not expose the Ollama API without auth) are enforced rather than relying on
“no known vulnerabilities.”
---
Nitpick comments:
In @.github/workflows/ci.yml:
- Line 82: Update the GitHub Actions image reference to an immutable digest by
appending the documented sha256 to the existing image name (replace the current
"image: ollama/ollama:0.24.0" value with the same image reference including the
`@sha256`:<digest> suffix), ensuring the workflow uses the exact digest pinned
image; keep the tag if desired but include the `@sha256` to guarantee
reproducibility.
🪄 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 Plus
Run ID: 56d69879-512e-460a-bb2c-686b888fdaf3
📒 Files selected for processing (1)
.github/workflows/ci.yml
The annotated digest (sha256:a614...19c4) is the multi-arch manifest-list (index) digest for the 0.24.0 tag -- the value `docker pull` resolves the tag to -- not a per-architecture child manifest (amd64 7db82c93..., arm64 32f9c389...). Word the comment so the digest isn't misread as a per-arch image digest. Comment-only change; the image is still pinned by tag. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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
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.
Problem
On Windows under Git Bash (and MSYS2 / Cygwin),
scripts/runnever finds thebundled binary and tries to download a non-existent release asset. Because the
launcher is wired as a
PreToolUse(Grep|Bash) hook, it exits non-zero onevery tool call, surfacing as a hook error in the agent.
Root cause is platform detection:
uname -sunder Git Bash isMINGW64_NT-<windows-build>, notwindows— andit even embeds the Windows build number (
10.0-26200), so it isn't a stablestring. The candidate loop then probes
bin/lumenandbin/lumen-${OS}-${ARCH}with no
.exe, never matching the shippedbin/lumen-windows-amd64.exe, andfalls through to:
which 404s (
lumen-0.0.41-mingw64_nt-10.0-26200-amd64is not a release asset).Fix
Normalize
mingw*|msys*|cygwin*(and explicitwindows*) towindows, andappend
.exeto the binary candidates and the download fallback path. The newcaseonly triggers on those uname strings, so Linux/macOS resolution isunchanged.
Testing
Git Bash on Windows 11 (
uname -s=MINGW64_NT-10.0-26200):scripts/run version-> prints0.0.41, no "Downloading..." lineecho '{}' | scripts/run hook pre-tool-use lumen-> exit 0🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Chores