fix(dev): the dev launch survives cmd.exe, and stages the sidecar it declares - #70
fix(dev): the dev launch survives cmd.exe, and stages the sidecar it declares#70Amir-SSVLabs wants to merge 3 commits into
Conversation
…declares
Two things stood between a Windows checkout and a running app, both in the
launch path rather than the app.
`dev:app` was a line of shell: `sh -c 'set -a; BRAINS_ENABLE_APPS=$(…);
[ -f .env ] && . ./.env; set +a; tauri dev …'`. npm hands a script to
`cmd.exe` on Windows, which does not parse those single quotes — the
`. ./.env` fell out of the quoting as a bare command and the launch died
with "'.' is not recognized as an internal or external command" before a
single file compiled. The assembly moves to `scripts/dev/app.mjs`, in the
one language both platforms have: the same app scan into
BRAINS_ENABLE_APPS, the same `.env` read after it so naming a variable
there still wins, the same dev config. The CLI is invoked as
`node …/@tauri-apps/cli/tauri.js` rather than through `node_modules/.bin`,
for the reason recall/host.mjs already documents about `.cmd` shims.
And `beforeDevCommand` never staged the declared externalBin —
`recall:prepare` runs from `beforeBuildCommand` only. `tauri_build::build()`
hard-fails on a missing one ("resource path
`binaries\recall-node-x86_64-pc-windows-msvc.exe` doesn't exist"), so a
fresh checkout could build a release and not run a dev build. The launcher
now runs prepare-sidecar (real binary when the Recall SDK is installed,
skips cleanly when not) and then stub-bundle-artifacts (no-op when the real
one landed, an empty file otherwise) — the same two steps CI already runs.
The prepare is non-fatal: a flaky download must not be why nobody can
launch.
Verified on Windows from the cold state — sidecar removed, `npm run
dev:app` through cmd.exe, app up.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
nir-ssvlabs
left a comment
There was a problem hiding this comment.
The port is faithful where it matters: findPacks(root).join(",") is byte-for-byte what installed-apps.mjs printed (it is that CLI's only statement), .env still overlays the scan so naming the variable there still wins, and env is actually threaded to the tauri dev call rather than assembled and dropped. Two things to fix, neither blocking.
🟡 Medium · issue (non-blocking): @Amir-SSVLabs readEnvFile loses shell parity on a trailing comment, which corrupts the value silently. The function claims to read "the KEY=value subset that set -a; . ./.env actually reads", but I ran both against the same file: for INLINE=http://localhost:3000 # local override, the shell sets http://localhost:3000 and this sets http://localhost:3000 # local override. A commented .env line is common enough that someone will hit it, and the app will fail on a garbage value rather than on anything that names the cause. Fix inline. Worth noting the shape of it: the one # branch the function does have (line 57) can never fire — a line starting with # fails the identifier regex before that check runs — so the file has dead comment handling and lacks the live case.
🟡 Medium · issue (non-blocking): scripts/dev/installed-apps.mjs:5 still opens "This is what npm run dev:app sets", which this PR makes false — dev:app now calls findPacks directly and that script's only remaining caller is scripts/cef-dev.mjs:94. You correctly kept the file and correctly updated CLAUDE.md and scripts/dev/README.md; this is the third place the same sentence lived. Suggest: "Print the ids of every INSTALLED app … Used by scripts/cef-dev.mjs; the dev:app launcher calls the same findPacks directly." It matters because the next reader of that header will think editing it changes what dev:app does.
Also, all pre-existing and none of it yours to fix here:
- The red
Pixel evalis inherited, not caused. Your #54 (merged 40 min before this run) changedGmailTab.svelte,ChatColumn.svelteandComposerPills.svelteand re-recorded zero goldens; 5 of 7 surfaces now diverge, worst ongmail-thread(0.389%).ci.ymltriggers only onpull_requestandpush: main, so merging intodevnever re-ran it — it went in invisibly and now reddens every PR built ondev. Nothing in this diff can move a pixel. Re-recording the goldens is its own commit, and it unblocks more than this PR. README.md:404pointsBRAINS_ENABLE_APPSat`scripts/dev/installed-removable apps.mjs`— a path with a space in it that has never existed. Already ondev; now doubly stale sincedev:appdoesn't use that script at all. One line, cheap to fold in while you're in the neighbourhood.- No
BRNS-DESK-id on the title. About half this repo's recent PRs carry one, so read this as a note rather than a process complaint.
Checked: the old shell line step-by-step against the new launcher (scan → .env overlay → sidecar → tauri dev, plus the new arg pass-through), findPacks vs the CLI it replaces, .env parsing against real sh by execution, both sidecar scripts' env needs, the import.meta.dirname floor (already the pattern in that directory, and engines is >=20 — pre-existing, not introduced here), and the Pixel eval attribution. Not read: cef-dev.mjs's duplicate assembly, which you disclosed as a follow-up.
Merge: ✅ into dev. The red check is #54's golden drift, not this PR — don't hold this on it, and don't let it merge unnoticed either.
`set -a; . ./.env` treats a WHITESPACE-PRECEDED `#` as the start of a
comment on an unquoted value, and keeps everything inside quotes. The
parser kept the lot, so `FOO=http://host:3000 # note` set FOO to the URL
plus the note — a plausible wrong string that fails later, as a bad URL,
rather than as a parse error.
Verified against `sh` over one fixture, seven cases, all identical now:
a bare `#` inside a word survives (`a#b`), a spaced one does not
(`a #b` → `a`), and both quote styles keep their contents verbatim.
The `startsWith("#")` guard went with it: a comment line cannot satisfy
the `[A-Za-z_]` identifier match, so it never ran, and it implied comment
handling that was not there.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
The red Pixel eval is inherited from This branch changes four files — Same five surfaces, same numbers, as #54 reported before it merged: 0.015% is the ✕ dismiss control on the deepest scope chip, and Since #54 merged, the goldens in @nir-ssvlabs — could you re-capture on macOS? It can't come from a Windows machine: Everything else on this PR is green: Frontend, Lint resources, Eval (token-less), Rust (macos-latest) and Rust (windows-latest) — the last one being the first clean Windows Rust run on a branch that is actually about Windows. |
…ke it
Two review nits, both about the two `run()` calls disagreeing with the
`tauri dev` call twelve lines below them.
They inherited `process.env` while the app got the merged environment, so
the comment above them ("`.env` is read after, so naming the variable there
still wins") was true of the app and not of them. Inert today — neither
script reads a variable — and exactly the kind of thing that stops being
inert without anyone noticing. Both now take the same `env` the app does.
And a failure in the stub step threw uncaught, so a fresh machine with no
`rustc` on PATH got a Node stack trace about `execFileSync` rather than the
one-line report this file uses everywhere else. `run()` now reports every
failure the same way and exits with the child's own status when the step
was load-bearing. Reproduced with rustc off PATH: the launch stops at the
stub step with `dev launch: …stub-bundle-artifacts.mjs did not finish`
and exit 1.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two things stood between a Windows checkout and a running app. Both are in the launch path, neither is in the app.
1.
dev:appdid not survivecmd.exeThe script was a line of shell:
npm hands a script to
cmd.exeon Windows, which does not parse those single quotes. The. ./.envfell out of the quoting as a bare command and every Windows dev launch died before a single file compiled:The assembly moves to
scripts/dev/app.mjs, in the one language both platforms have. Same three steps, same order, same semantics: scansrc/apps/*/app.jsonintoBRAINS_ENABLE_APPS, read.envafter it so naming a variable there still wins, launch with the dev config. The CLI is invoked asnode …/@tauri-apps/cli/tauri.jsrather than throughnode_modules/.bin, for the reasonrecall/host.mjsalready documents about.cmdshims needing a shell.2.
beforeDevCommandnever staged the externalBin it declaresrecall:prepareruns frombeforeBuildCommandonly. Buttauri_build::build()hard-fails on a declaredexternalBinthat is not on disk, on dev as much as on build:So a fresh checkout could build a release and not run a dev build. The launcher now runs
prepare-sidecar.mjs(the real binary when the Recall SDK is installed; skips cleanly when it isn't) and thenstub-bundle-artifacts.mjs(a no-op when the real one landed, an empty file otherwise) — the same two steps CI already runs for its own jobs. The prepare step is non-fatal: a flaky S3 download must not be the reason nobody can launch.This one is not Windows-specific in principle. It is just that everyone on macOS already has a staged sidecar, so nobody hits it.
Verification
Tested on Windows from the cold state — sidecar deleted,
npm run dev:apprun through npm so it goes viacmd.exeexactly as a fresh checkout would:then a running
brains.exe. Before this change that state failed twice over, once per bug above.npm run check740 files / 0 errors.drive:testandeval:testpass. All five lints green.Left out on purpose
scripts/cef-dev.mjscarries its own copy of the same env assembly and spawnsnode_modules/.bin/tauri, which would break identically on Windows — but it hard-exits on non-darwin, so it is macOS-only by construction. Refactoring a script I cannot run here did not belong in this PR; it is a follow-up.scripts/dev/installed-apps.mjsstays:cef-dev.mjsstill shells out to it, so it is not orphaned.🤖 Generated with Claude Code