fix(ego-browser): make the repo build, test, and commit on Windows - #148
fix(ego-browser): make the repo build, test, and commit on Windows#148hoklims wants to merge 5 commits into
Conversation
Two path bugs that a POSIX-only checkout never surfaced.
relativeSitePath() compared the resolved tool path against `${siteRoot}/`.
path.resolve() returns backslash-separated paths on Windows, so the guard
never matched and every declared site skill tool was rejected:
runNodeSiteTool() and loadBrowserToolSource() both threw "path must stay
inside the site skill directory". Using path.sep keeps the check
byte-identical on macOS and Linux.
resolvePath() expanded "~" by passing path.slice(1) to resolve(). That
leaves a leading separator, which resolve() treats as an absolute path and
drops the home base entirely -- "~/Downloads/a.png" became "/Downloads/a.png"
on POSIX and "C:\Downloads\a.png" on Windows. This one was never
platform-specific.
Both paths are covered by regression tests that fail without the fix.
npm runs package scripts through cmd.exe on Windows, which does not understand POSIX shell syntax. Three scripts were unusable there: - `prepare` was inline `if [ "$CI" = "true" ]; ...`, so `npm ci` exited 1 before installing anything. Moved to scripts/prepare.mjs, which keeps the same behaviour (including the same non-zero exit when lefthook cannot find a git repository). - `clean` used `rm -rf`, which only resolves when Git for Windows coreutils happen to be on PATH. Moved to scripts/clean.mjs. - `validate:learnings` / `validate:site-skills` used an inline `EGO_BROWSER_AGENT_WORKSPACE=... node ...` prefix, which cmd.exe parses as a command name. The default now lives in scripts/validate-site-skills.ts, anchored on the script's own location rather than the cwd, and an externally provided value still wins. No behaviour change on macOS, Linux, or the ubuntu CI runners.
…latforms The real-browser suite drives the `ego-browser` command that ships inside the ego lite app, which is macOS-only today. It was wired as an unconditional pre-commit gate, so no commit touching the package could be made on Windows or Linux. - run-real-browser-e2e.mjs now reports why it skips instead of failing on an opaque ENOENT. EGO_BROWSER_REAL_E2E_FORCE=1 still runs it anywhere. - The lefthook e2e gate skips off macOS. - runner.mjs spawned the bare name "npm", which resolves to npm.cmd on Windows and cannot be launched without a shell. It now calls the build script through the current Node binary. - Single-quoted the lefthook echo strings: lefthook on Windows delegates to Git Bash but mangles double-quoted literals containing spaces, which made the very first hook fail to parse and blocked every commit.
Without a .gitattributes, Git for Windows checks the tree out as CRLF (core.autocrlf defaults to true). Prettier expects LF, so `prettier --check` failed on files the contributor never touched, both in the pre-commit gate and in the changed-file style job. `* text=auto eol=lf` also keeps CRLF out of scripts/install.sh. The blobs are already LF, so this changes no content. The CI test job now runs on ubuntu-latest and windows-latest so the platform-specific breakage above cannot come back unnoticed. CONTRIBUTING notes which commands are cross-platform and which one is not.
There was a problem hiding this comment.
Pull request overview
This PR makes the ego-browser harness/toolchain buildable, testable, and committable on Windows (without changing the macOS-only ego lite app), and fixes two real path-handling bugs in runtime code.
Changes:
- Fix path confinement and
~expansion bugs in runtime code, with regression tests. - Replace POSIX-shell npm scripts with Node-based scripts to work under Windows
cmd.exe, and improve real-browser e2e skip messaging. - Add Windows CI coverage and enforce LF checkouts to prevent Prettier failures on Git for Windows.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| package/ego-browser/src/learning/index.ts | Use path.sep for the site-tool confinement check so Windows-resolved paths are accepted. |
| package/ego-browser/src/learning/index.test.mjs | Add regression tests covering cross-platform tool path resolution and confinement. |
| package/ego-browser/src/env.ts | Fix ~ expansion so ~/... resolves under the home directory (not filesystem root). |
| package/ego-browser/src/env.test.mjs | Add regression tests for resolvePath tilde expansion behavior. |
| package/ego-browser/scripts/validate-site-skills.ts | Move workspace defaulting into the script (anchored to script location) for Windows compatibility. |
| package/ego-browser/scripts/run-real-browser-e2e.mjs | Skip real-browser e2e with a clear message on non-macOS unless forced. |
| package/ego-browser/scripts/real-browser-e2e/runner.mjs | Avoid spawning bare npm (Windows npm.cmd issue) by invoking build via process.execPath. |
| package/ego-browser/scripts/prepare.mjs | Replace POSIX prepare shell with a Node script to install lefthook cross-platform. |
| package/ego-browser/scripts/clean.mjs | Replace rm -rf clean with a Node implementation for Windows. |
| package/ego-browser/package.json | Wire new Node-based scripts and simplify validate aliases for Windows. |
| lefthook.yml | Skip macOS-only e2e hook off macOS and adjust echo quoting for Windows/Git Bash behavior. |
| CONTRIBUTING.md | Document Windows support for the toolchain and clarify e2e skip behavior off macOS. |
| .github/workflows/ci.yml | Run tests on both ubuntu-latest and windows-latest to prevent regressions. |
| .gitattributes | Enforce LF line endings in working trees to prevent Prettier failures on Windows checkouts. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Git for Windows defaults to core.symlinks=false, so the two skill symlinks land as 24-byte text files and agent skill discovery finds a file instead of the skill directory. Documented the one-time fix rather than changing the repo layout, since enabling symlinks restores them with no diff.
Side observation: the symlink fix makes the skill discoverable on a platform where it cannot runDeliberately not part of this PR — it is a product call, not a portability fix — but it surfaced while verifying the last commit, so it seems worth reporting.
From there,
So on Windows the agent is routed to ego-browser, is instructed not to check, and only finds out by failing: The recovery path is a dead end too:
Two small options, whichever fits your intent better:
Happy to open a separate PR for either, or to leave it entirely — you own that call. Keeping it out of this branch so the Windows toolchain change stays reviewable on its own. |
|
Independent verification on a second Windows machine — Windows 11 Home 10.0.26200, Node v24.14.0, npm 11.9.0, Git for Windows with default settings. Baseline first, to confirm the problem this PR fixes: on current On this branch (8062088), stock npm with no workarounds: Two notes:
FWIW #226 (heredoc-free CLI input) approaches the invocation side of Windows readiness and composes with this — no shared files except CI config context. |
windows-host.yml failed on Windows for the reason its own comment predicted: ego-browser's `prepare` is a POSIX shell one-liner and npm runs package scripts through cmd.exe there. `--ignore-scripts` does not cover it, because npm still prepares a `file:` dependency. Pointing npm at the Git Bash the runner already ships makes the existing script work, which is what ci.yml's Windows jobs already do. That was citrolabs#148. The other failure said `timed out waiting for space` and nothing else, for 30 seconds, and the reason is a hole in the harness rather than a fact about the code: the agent script runs in another process, and when it dies the space simply never appears. The wait throws first, so the child's own error is never read -- the log had a timeout where the actual error was. waitForSpaceOrFailure races the two, so a child that fails reports what it failed with. This does not fix that test on Windows; it makes the next run able to say what is wrong, which the last one could not. Deliberately not guessed at: the underlying failure. The timing points at the 30s before the wait rather than the wait itself, and the lock this branch added is new on Windows -- but no evidence in hand names it, so nothing here pretends to. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Summary
The repo's own toolchain could not run on Windows:
npm ciexited 1 before installing anything, so a Windows contributor was blocked at step 1 of CONTRIBUTING §4 and never reached the test suite. Fixing that surfaced two genuine path bugs insrc/, one of which is not Windows-specific at all.This does not make the ego lite browser itself run on Windows — that app is macOS-only and lives outside this repo. It makes this repo buildable, testable, and committable on Windows, which is a prerequisite for the Windows support already on the roadmap.
Every change is a no-op on macOS, Linux, and the existing ubuntu CI runners.
Related issue
No existing issue. The README lists Windows as roadmap work; this only covers the harness/toolchain side of it.
Changes
src/learning/index.ts—relativeSitePath()compared the resolved path against`${siteRoot}/`.path.resolve()returns backslashes on Windows, so the containment guard never matched and every declared site skill tool was rejected:runNodeSiteTool()andloadBrowserToolSource()both threwpath must stay inside the site skill directory. Now usespath.sep, which is/on POSIX, so the check is byte-identical there. Escaping the site directory is still refused (covered by test).src/env.ts—resolvePath()expanded~viapath.slice(1), leaving a leading separator thatresolve()treats as absolute, discarding the home base.~/Downloads/a.pngresolved to/Downloads/a.pngon POSIX andC:\Downloads\a.pngon Windows. This one was never platform-specific.package.json—prepare(inlineif [ "$CI" = "true" ]),clean(rm -rf) andvalidate:*(inlineVAR=value cmdprefix) are POSIX shell; npm runs scripts throughcmd.exeon Windows. Moved toscripts/prepare.mjs,scripts/clean.mjs, and a default resolved insidescripts/validate-site-skills.ts(anchored on the script's own location, not the cwd; an externally providedEGO_BROWSER_AGENT_WORKSPACEstill wins)..gitattributes(new) — without it, Git for Windows checks the tree out as CRLF andprettier --checkfails on files the contributor never touched, in both the pre-commit gate and the changed-file style job. Existing blobs are already LF, so no file content changes.lefthook.yml— thee2e-testgate ran the macOS-only suite unconditionally, so no commit touching the package was possible elsewhere; it now skips off macOS. Echo strings are single-quoted because lefthook on Windows delegates to Git Bash but mangles double-quoted literals containing spaces, which made the first hook fail to parse and blocked every commit.run-real-browser-e2e.mjs/runner.mjs— the suite reports why it skips instead of dying on an opaqueENOENT(EGO_BROWSER_REAL_E2E_FORCE=1still forces it), and the build step no longer spawns the bare namenpm, which resolves tonpm.cmdon Windows and cannot be launched without a shell..github/workflows/ci.yml— the test job runs onubuntu-latestandwindows-latestso this cannot regress silently.Two pre-existing Prettier deviations (in
src/env.tsandscripts/validate-site-skills.ts) were formatted because the changed-file style job would otherwise fail on files this PR touches.Verification
Run on Windows 11 (Node 22.21.1) — the full CONTRIBUTING §4 sequence, which previously stopped at step 1:
No regression on Linux, same tree in
node:22(Docker):scripts/prepare.mjswas also compared against the previous inline shell in the failure case (no git repository): both exit 1 with the same lefthook message, so the contract is unchanged.The two
src/fixes are covered by regression tests that fail without the fix (verified by reverting each fix against the new tests):resolvePath expands ~ against the home directory— gotE:\Downloads\a.png, expected<home>\Downloads\a.pngsite tool paths resolve on every platform and stay confined—loadBrowserToolSourcethrewpath must stay inside the site skill directoryI could not run the macOS path myself. The real-browser e2e suite is unchanged in behaviour there (
process.platform === "darwin"anduname -sboth keep the existing route), but it is worth a maintainer runningnpm run e2eon macOS before merge.Impact
Behaviour changes, both fixing previously broken paths:
runSiteTool/runSiteBrowserToolnow work on Windows (they always did on macOS), andEGO_BROWSER_AGENT_WORKSPACE=~/...now expands to the home directory instead of the filesystem root. Thevalidate:*scripts change precedence slightly: an externally setEGO_BROWSER_AGENT_WORKSPACEnow wins over the built-in default, where the old inline prefix always overrode it. Nothing in CI sets that variable.Checklist
devfor normal changes; onlydevmay targetmain).feat,fix,docs,chore,ci, orrefactor).