Fix Windows test failures and restore workspace: protocol in manifests#167
Open
Tahshsa wants to merge 2 commits intoNarcooo:masterfrom
Open
Fix Windows test failures and restore workspace: protocol in manifests#167Tahshsa wants to merge 2 commits intoNarcooo:masterfrom
Tahshsa wants to merge 2 commits intoNarcooo:masterfrom
Conversation
Fixes two classes of issues that prevented the test suite from passing
on Windows and caused a latent publish bug:
1. Windows path-separator bugs in unit tests. studio.test.ts,
studio-runtime.test.ts, and daemon.test.ts hard-coded POSIX paths
(e.g. "/project/inkos.pid") while the production code uses path.join,
which produces backslashes on Windows. Tests now build expected
paths with path.join so they match whichever separator the current
OS uses. No production code changes.
2. execFileSync("npm", ...) in publish-package.test.ts failed with
ENOENT on Windows because npm ships as npm.cmd. Switched to npm.cmd
plus shell: true on win32.
3. packages/cli/package.json and packages/studio/package.json had been
committed in their prepack-normalized form, pinning
@actalk/inkos-core and @actalk/inkos-studio to "1.1.1" instead of
"workspace:*". This was correctly flagged by the
"keeps source CLI dependencies linked through the workspace
protocol" test, which was failing on every platform. Restored the
workspace protocol so future publishes (via prepack rewrite) cut
clean releases instead of locking siblings to the previous version.
Before: 43 failed / 32 passed on Windows.
After: 79 passed / 0 failed on Windows, typecheck clean across all
three packages.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Guards against Windows-specific regressions like the path-separator and npm.cmd issues fixed in the preceding commit. Prior CI ran only on ubuntu-latest, so the test failures reported in this PR went undetected on main. Co-Authored-By: Claude Opus 4.6 <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.
Summary
Hi! First-time contributor here. I was trying out InkOS on Windows and found the test suite was red (43 failed / 32 passed from a clean clone + build). Digging in, there were three independent bugs — two Windows-portability issues in the tests and one real latent bug in the committed package manifests that would have affected the next release regardless of platform.
After the fixes: 79 passed / 0 failed on Windows, typecheck clean across all three packages.
1. Windows path-separator bugs in tests (6 failures)
studio.test.ts,studio-runtime.test.ts, anddaemon.test.tshard-coded POSIX paths in their assertions and mock implementations:```ts
if (path === "/project/packages/studio/src/api/index.ts") { return; }
```
But the production code (
resolveStudioLaunch,upCommand) usespath.join, which on Windows produces\project\packages\studio\src\api\index.ts. TheaccessMockstring comparison never matched,firstAccessiblePathreturnedundefined, and the command bailed withprocess.exit(1)— surfacing as the confusing "process.exit unexpectedly called with 1" error.Fix: build expected paths with
path.joinso they match whichever separator the host OS uses. No production code changes — the production code is correct; it should use OS-native separators because those paths are passed tofs.accessandspawn.2.
execFileSync(\"npm\", ...)ENOENT on Windows (2 failures)publish-package.test.tsshells out tonpm pack, but on Windowsnpmships asnpm.cmdandexecFileSyncwithout a shell can't launch.cmdfiles. Switched tonpm.cmd+shell: truewhenprocess.platform === \"win32\".3. Committed manifests had
prepack-normalized dependencies (1 failure, all platforms)This one's the real find.
packages/cli/package.jsonandpackages/studio/package.jsonwere committed with concrete versions for their workspace siblings:```json
"@actalk/inkos-core": "1.1.1",
"@actalk/inkos-studio": "1.1.1"
```
instead of the expected
\"workspace:*\". The testpublish-package.test.ts > keeps source CLI dependencies linked through the workspace protocolwas correctly flagging this, but it was failing on every platform — Linux CI just didn't surface it because the rest of the test file couldn't get past the Windows-pack issue... actually it's easier than that: the test was silently red on main. Worth double-checking whether your CI was gating on it.The practical impact: the next
npm publishfrom that state would have pinned siblings to1.1.1, so if you'd cut a1.1.2release the published@actalk/inkos@1.1.2would have pulled in@actalk/inkos-core@1.1.1from the registry instead of the matching 1.1.2 sibling. Bumpy release.Most likely cause: a publish where the
postpackrestore script didn't run (Ctrl-C, pnpm skipping lifecycle hooks, etc.), leaving the dirty state committed. FYIpnpm publishhas a nativeworkspace:*→ concrete transform built in, so you could drop the prepack/postpack script pair entirely if you wanted fewer moving parts.Fix: restored
\"workspace:*\"in both manifests.Test plan
pnpm install && pnpm build && pnpm test— 79 passed / 0 failed on Windows 11, Node 22pnpm typecheck— clean acrosscore,cli,studioThanks for the project — excited to try writing a novel with it once this lands!
🤖 Generated with Claude Code