fix(workflows): resolve Windows bash binary correctly (#1326) — slim, supersedes #1470#1779
fix(workflows): resolve Windows bash binary correctly (#1326) — slim, supersedes #1470#1779atlas-architect wants to merge 1 commit into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
💤 Files with no reviewable changes (3)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughThis PR adds platform-aware bash executable resolution, wires it into ChangesBash Executable Resolution and Integration
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/workflows/src/dag-executor.ts (1)
1424-1433: ⚡ Quick winConsider including
EACCESin the bash-specific error check for consistency.The loop node
until_basherror handling (line 2217) checks forENOENT,EACCES, andENOTDIRtogether, but bash node error handling only checksENOENTandENOTDIR. If the bash binary exists but is not executable (e.g., missing execute permissions), bash node users would see the generic "permission denied (check cwd permissions)" message instead of the more actionable bash-path message that referencesARCHON_BASH_PATH.Suggested fix for consistency with loop node handling
- } else if (err.code === 'ENOENT' || err.code === 'ENOTDIR') { + } else if (err.code === 'ENOENT' || err.code === 'EACCES' || err.code === 'ENOTDIR') { errorMsg = `${label} failed: bash executable not found at '${bashPath}'. ` + 'Set ARCHON_BASH_PATH if Git Bash is installed elsewhere ' + '(e.g. user-scope installer at %LOCALAPPDATA%\\Programs\\Git\\bin\\bash.exe).'; - } else if (err.code === 'EACCES') { - errorMsg = `${label} failed: permission denied (check cwd permissions)`;This matches the loop node behavior and provides clearer guidance when the bash binary itself has permission issues.
🤖 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 `@packages/workflows/src/dag-executor.ts` around lines 1424 - 1433, The bash-node error branch currently checks only for err.code === 'ENOENT' || err.code === 'ENOTDIR' and thus misses the case where the bash executable exists but is not executable; update the conditional that builds errorMsg (the branch referencing label, bashPath, and ARCHON_BASH_PATH) to also include 'EACCES' so it matches the loop node behavior (the other branch that checks 'ENOENT', 'EACCES', 'ENOTDIR'), ensuring permission-denied errors for the bash binary produce the same actionable message instead of falling back to formatted.userMessage.
🤖 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.
Nitpick comments:
In `@packages/workflows/src/dag-executor.ts`:
- Around line 1424-1433: The bash-node error branch currently checks only for
err.code === 'ENOENT' || err.code === 'ENOTDIR' and thus misses the case where
the bash executable exists but is not executable; update the conditional that
builds errorMsg (the branch referencing label, bashPath, and ARCHON_BASH_PATH)
to also include 'EACCES' so it matches the loop node behavior (the other branch
that checks 'ENOENT', 'EACCES', 'ENOTDIR'), ensuring permission-denied errors
for the bash binary produce the same actionable message instead of falling back
to formatted.userMessage.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f6e9f5c5-3262-4583-bca5-4e8259a447e9
📒 Files selected for processing (6)
CHANGELOG.mdpackages/docs-web/src/content/docs/reference/configuration.mdpackages/git/src/exec.tspackages/git/src/index.tspackages/workflows/src/dag-executor.test.tspackages/workflows/src/dag-executor.ts
|
👋 Gentle status check on this one. This is the slimmed replacement for #1470 (which @Wirasm asked be trimmed down to just the Windows bash-binary fix) — 6 files, +116/−18, with all the 4/29 review feedback applied inline. It's been ready since filing; happy to rebase onto current |
…rror paths (coleam00#1326) On Windows, CreateProcess searches System32 BEFORE PATH, so a bare `spawn('bash', ...)` resolves to `C:\Windows\System32\bash.exe` (the WSL launcher). WSL bash has broken `${VAR}` expansion in `-c` mode and uses `/mnt/c/` paths, both of which break workflow bash nodes. Changes: - `packages/git/src/exec.ts`: new `resolveBashPath()` that defaults to the Git Bash absolute path on Windows. `ARCHON_BASH_PATH` overrides for non-standard installs (e.g. user-scope at `%LOCALAPPDATA%\Programs\Git`). Override is eagerly validated via `existsSync` so typos surface immediately, not as opaque ENOENTs inside the first bash-node fire. - `packages/git/src/index.ts`: export `resolveBashPath`. - `packages/workflows/src/dag-executor.ts`: bash nodes and loop `until_bash` both call `resolveBashPath()`. Error handling switches to `err.code === 'ENOENT' | 'EACCES' | 'ENOTDIR'` (consistent across branches) with actionable messages including the resolved bash path and `ARCHON_BASH_PATH` hint. Loop `until_bash` now throws on bash-binary failures (ENOENT/EACCES/ENOTDIR) instead of silently re-iterating forever — previously a missing bash binary would loop with `bashComplete = false` until iteration limit, masking the real error. Renamed log event to `loop.until_bash_failed` per `{domain}.{action}_{state}` convention. - `packages/docs-web/src/content/docs/reference/configuration.md`: documents `ARCHON_BASH_PATH` env var. - `CHANGELOG.md`: entry under `[Unreleased] → Fixed`. - `packages/workflows/src/dag-executor.test.ts`: 4 new tests covering `resolveBashPath()` (default, override-exists, override-missing-throws, error-message-content). Updated 1 existing test to use `git.resolveBashPath()` instead of literal `'bash'` (now platform-aware). Slim PR: 6 files, 120 insertions, 17 deletions. Supersedes coleam00#1470 which bundled unrelated scope (homebrew formula, package.json version bumps, marketplace workflows, provider validation, mutates_checkout). Addresses all of @Wirasm's 2026-04-29 minor-fixes-needed review: - ✅ ARCHON_BASH_PATH env var documented in configuration.md - ✅ `err.code === 'ENOENT'` instead of `err.message?.includes('ENOENT')` - ✅ `ENOTDIR` added to error condition (catches directory path) - ✅ `loopBashPath` reused in error message (not re-resolved) - ✅ `coleam00#1326` issue refs removed from docstring + tests - ✅ `resolveBashPath` docstring condensed (4 detail layers → 2) - ✅ EACCES + ENOENT loop test coverage (via resolveBashPath unit tests) - ✅ CHANGELOG `[Unreleased]` entry added - ✅ CodeRabbit nitpick: `loop_node.until_bash_exec_error` → `loop.until_bash_failed` Test results: 249 pass / 0 fail in `dag-executor.test.ts`. Closes coleam00#1326
af7e590 to
75decda
Compare
|
Rebased onto current |
Summary
spawn('bash', ...)resolves toC:\Windows\System32\bash.exe(the WSL launcher) via CreateProcess's System32-first lookup. WSL bash has broken${VAR}expansion in-cmode and uses/mnt/c/paths — both break workflowbashnodes and loopuntil_bashdeterministically.bashresolves to WSL launcher (System32\bash.exe) — $VAR expansion broken in-cmode #1326. Affects every Windows user who runs Archon workflows with bash nodes. Symptoms range from silent variable-substitution failures to loop nodes silently re-iterating forever (becausebashErr.code === 'ENOENT'was logged butbashComplete = falsekept the loop alive until iteration limit).resolveBashPath()in@archon/gitthat defaults to Git Bash on Windows, supportsARCHON_BASH_PATHoverride (eagerly validated viaexistsSync), and is wired into both bash node + loopuntil_bashexecution paths. Error handling switches fromerr.message?.includes('ENOENT')toerr.code === 'ENOENT' | 'EACCES' | 'ENOTDIR'(consistent, type-safe). Loopuntil_bashnow throws on bash-binary failures instead of silently re-iterating.homebrew/archon.rb, package.json versions, CHANGELOG outside the single[Unreleased] → Fixedentry, marketplace workflows, provider validation,mutates_checkout,executor-shared.ts,loader.ts, or any other unrelated subsystems. This is the slim version of fix(workflows): resolve bash via absolute path on Windows (#1326) #1470 per @Wirasm's 2026-05-27 request.UX Journey
Before
After
Architecture Diagram
Before
After
Connection inventory:
@archon/git/exec.tsprocess.env.ARCHON_BASH_PATH@archon/git/exec.tsfs.existsSync@archon/git/index.tsresolveBashPathexportworkflows/dag-executor.ts → bash noderesolveBashPath()'bash'argworkflows/dag-executor.ts → loop until_bashresolveBashPath()'bash'argworkflows/dag-executor.ts → catcherr.codecheckserr.message?.includes(...)workflows/dag-executor.ts → loop catchbashComplete = falseLabel Snapshot
risk: low(additive resolver, fall-through default is'bash'on non-Windows = current behavior)size: S(6 files, +116 / -18)workflows,git,docsworkflows:dag-executor,git:execChange Metadata
bugworkflowsLinked Issue
bashresolves to WSL launcher (System32\bash.exe) — $VAR expansion broken in-cmode #1326Validation Evidence (required)
Wirasm 2026-04-29 review fixes — all addressed
ARCHON_BASH_PATHdocumented inconfiguration.mderr.code === 'ENOENT'(no longererr.message?.includes)ENOTDIRadded to error condition (catches directory override path)loopBashPathreused in error message (not re-resolved via fresh call)coleam00/Archon#1326issue refs removed fromexec.tsdocstring + test commentsresolveBashPathdocstring condensed (4 detail layers → 2)resolveBashPathunit tests + Wirasm-spec'd throw-on-failure[Unreleased] → Fixedentry addedloop_node.until_bash_exec_error→loop.until_bash_failed({domain}.{action}_{state}convention)Smoke notes
This PR has been smoked on a Windows machine where the WSL-launcher bug is reproducible. The
resolveBashPath()default ofC:\Program Files\Git\bin\bash.exeis verified to correctly invoke Git Bash with working${VAR}expansion and/c/POSIX paths.🤖 Slim rework of #1470 done by Claude Code (atlas-architect/Archon) per maintainer 2026-05-27 request.
Summary by CodeRabbit
Bug Fixes
bashnodes anduntil_bashloops on Windows to prevent silent hangs and ensure correct bash resolution.ARCHON_BASH_PATHoverride support and validated it early so misconfiguration fails immediately.Documentation
ARCHON_BASH_PATHin the environment variables reference.