fix(ci) [BRNS-DESK-034, BRNS-DESK-035]: Windows runs the path spec, and the cache caches what cargo writes - #71
Conversation
…nd the cache caches what cargo writes Two CI truth problems, both in the same three workflow files. ## The Windows path fix was never proven on Windows (BRNS-DESK-034, AC4) `ledger-net.test.ts` walks the tree with `join()` and compares the result against forward-slash literals, which makes it the one spec in this repository whose result depends on the path separator. It failed on every entry at once on Windows until #62 normalized the discovered side, and nothing re-proved that afterwards: the matrix #62 added is a cargo matrix, and cargo does not run vitest. The `Frontend` job that does run vitest is ubuntu-only. So the fix for a Windows bug was a claim rather than a test, and AC4 stayed open. The cheapest true closer is a Windows-gated step inside the existing `rust` job, which already installs node 22 and runs `npm ci`. The check costs seconds there; a second Windows frontend job would cost a job. It is that job's LAST step, deliberately. A failing step aborts the leg, so a frontend spec placed ahead of the cargo steps would let one red assertion delete that leg's clippy, its `--workspace` tests and its Windows link check — exactly the coverage the matrix exists to provide, erased by an unrelated failure. `fail-fast: false` guards sibling legs, not sibling steps. Running last costs nothing, because the spec's walker skips `target` and `node_modules` by name. Both flags carry weight. `--passWithNoTests=false` pins the behavior that makes the hardcoded path load-bearing — vitest exits 1 on a filter matching nothing, so a rename cannot silently retire the step. That is vitest's default today, but it is vitest's, and one line in `vitest.config.ts` would flip it for the whole repo. `--no-install` stops `npm exec` from falling back to a registry download of an unpinned vitest if the local binary ever goes missing; a step whose value is determinism should fail loudly instead. Verified locally: the spec passes 26/26, a stale filter exits 1, and `--passWithNoTests=true` exits 0 on that same stale filter, which proves the flag is read rather than ignored. Not `npm test`: that chains `drive:test` and `eval:test` behind vitest and neither has ever run on Windows, so widening to the full frontend gate would be a different change with a real chance of landing red for unrelated reasons. ## The Rust cache pointed at a directory cargo no longer writes (BRNS-DESK-035) `Swatinem/rust-cache` caches `<workspaces>/target` and defaults `workspaces` to `.`. Six sites overrode that with `src-tauri`, so the action cached a path cargo does not write: `./Cargo.toml` is the workspace root and lists the seven engine crates and `src-tauri` as members, so every artifact lands in repo-root `target/`. Removing the input at all six sites is the whole fix. The lesson is NOT that the input was always wrong, and the comments say so plainly, because the true version is the useful one. It entered at e152464 (2026-03-01), when this tree had no root `Cargo.toml` and `src-tauri` carried its own manifest and lockfile — src-tauri genuinely was the cargo root, and `src-tauri/target/` genuinely was where cargo wrote. Commit 2e0e247 (2026-08-06) added the root workspace and deleted `src-tauri/Cargo.lock` in one move, and the input silently became stale. Nothing lints a workspace-root move against the CI inputs naming the old root. That is the failure mode worth remembering, and an earlier draft of these comments asserted the opposite. The key was also wrong, in a way that is easy to miss: the action hashes `<workspaces>/Cargo.lock` only if it exists, and `src-tauri/Cargo.lock` had been deleted, so the lockfile dropped out of the key entirely and a `cargo update` stopped invalidating the entry. That is why the first run after this change re-keys and saves rather than no-opping. `save-if` on the two ci.yml jobs that run on every PR is the other half of the fix, not a separate opinion. These entries stop being a ~741 MB registry and become a real target directory, and a PR-scoped save is readable only by re-runs of that same PR while still counting against the 10 GB repository budget that evicts by last use. Writing only on `main` keeps one warm entry every PR can restore from, instead of per-PR copies nobody reads that evict the shared one and the npm caches beside it. `build-macos` is deliberately excluded: it is label-gated on a pull request and never runs on `main`, so restricting its save would mean it never writes and never restores. `dev-release.yml` gets `key: universal-apple-darwin-dev`. Both release workflows name the job `build` and run it on `macos-15`, and the action builds its key from the job id, the runner OS and arch, and the `key` input — not from the workflow name. With the same key literal, a manual `release.yml` dispatch on `dev` lands in the same ref scope with the same key, and cache entries are immutable: the second writer is silently skipped and one channel serves the other channel's target directory. That was harmless while both held nothing but the registry; it is not harmless now that they hold real builds, since the two channels pin different Recall SDKs and carry different bundle identities. Two expectations are tempered in the comments rather than overstated. A restore returns third-party dependency artifacts only — the action prunes workspace members before saving, so our own eight crates relink every run. And `release.yml` triggers on `v*` tags, where Actions caches are ref-scoped: a tag run restores from its own new ref or from the default branch, which never runs that job id, so its entries are written and in the normal flow never read. Fixing the path there is still right, but warming the release lane needs its own change and its own ticket. `dev-release.yml` is the one lane this genuinely warms, since it runs on pushes to `dev` and restores its own previous save. Verified: `actionlint .github/workflows/*.yml` exits 0, all three files parse, no `workspaces:` input remains, and the vitest spec passes under the exact command string CI will run. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
CI outcome on BRNS-DESK-034 AC4 — the spec now actually runs on Windows. The forward-slash filter resolved against a BRNS-DESK-035 AC5 — the cache now points where cargo writes. From the same job's The last path is the repository root's The same step also prints Pixel eval is red, and it is not from this branch. This branch touches four files: three workflow YAMLs and
Two branches with no UI change and no golden change, both based on So the goldens need re-capturing against Two follow-ups this change makes visible, both deliberately left out. The repository cache is at The action's save step is gated on |
stefan-ssv-labs
left a comment
stefan-ssv-labs
left a comment
There was a problem hiding this comment.
✅ review-pr (self-review, cannot approve own PR): clean — no blockers or criticals. Exact-head CI proves Frontend, both Rust legs (including the Windows path spec and root target cache path), Eval, and Lint resources. Pixel eval remains red, but the same base run reproduces the identical divergence and this diff is workflow/README-only.
2 participants
Closes the last open acceptance criterion on BRNS-DESK-034 and takes
BRNS-DESK-035 whole. Both are CI-truth problems and both live in the same
three workflow files, so they ship together.
BRNS-DESK-034 AC4 — the Windows path fix was never proven on Windows
src/layout/panes/session/__tests__/ledger-net.test.tswalks the tree withjoin()and compares the result against forward-slash literals. That makes itthe one spec in this repository whose result depends on the path separator. It
failed on every entry at once on Windows until #62 normalized the discovered
side — and nothing re-proved it afterwards, because the matrix #62 added is a
cargo matrix and cargo does not run vitest, while the
Frontendjob thatdoes run vitest is ubuntu-only. A fix for a Windows bug that no Windows job
exercises is a claim, not a test.
The closer is a Windows-gated step inside the existing
rustjob, which alreadyinstalls node 22 and runs
npm ci, so the check costs seconds rather than awhole extra job.
Three details are load-bearing:
the cargo steps, one red frontend assertion would delete that leg's clippy,
its
--workspacetests and its Windows link check — precisely the coveragethe matrix exists for.
fail-fast: falseguards sibling legs, not siblingsteps. Running last costs nothing: the spec's walker skips
targetandnode_modulesby name.--passWithNoTests=falsepins the behavior that makes the hardcoded pathmeaningful. vitest exits 1 on a filter matching nothing, so a rename cannot
silently retire the step — but that is vitest's default, and one line in
vitest.config.tswould flip it repo-wide. The flag makes the guarantee local.--no-installstopsnpm execfalling back to a registry download of anunpinned vitest if the local binary ever goes missing.
Not
npm test: it chainsdrive:testandeval:testbehind vitest and neitherhas ever run on Windows.
BRNS-DESK-035 — the Rust cache pointed at a directory cargo no longer writes
Swatinem/rust-cachecaches<workspaces>/targetand defaultsworkspacesto.. Six sites overrode that withsrc-tauri../Cargo.tomlis the workspaceroot and lists the seven engine crates plus
src-taurias members, so cargowrites to repo-root
target/and the action was caching a path nothing wrote.Removing the input at all six sites is the fix.
The comments do not say the input was always wrong, because it wasn't. It
entered at
e152464(2026-03-01), when this tree had no rootCargo.tomlandsrc-tauricarried its own manifest and lockfile — src-tauri genuinely was thecargo root. Commit
2e0e247(2026-08-06) added the root workspace and deletedsrc-tauri/Cargo.lockin one move, and the input silently went stale. Nothinglints a workspace-root move against the CI inputs naming the old root; that is
the lesson the comments record.
The key was wrong too, which is easy to miss: the action hashes
<workspaces>/Cargo.lockonly if it exists.src-tauri/Cargo.lockwas gone, sothe lockfile dropped out of the key and a
cargo updatestopped invalidatingthe entry. This is also why the first run after this change re-keys and saves
rather than silently no-opping.
Two changes beyond the bare
workspacesremovalsave-ifon the two ci.yml jobs that run on every PR. These entries stopbeing a ~741 MB registry and become a real target directory. A PR-scoped save
is readable only by re-runs of that same PR, while still counting against the
10 GB repository budget that evicts by last use — so without this, the fix
plausibly makes CI slower: per-PR copies nobody reads evict the shared entry
and the npm caches beside it. Writing only on
mainkeeps one warm entry everyPR restores from.
build-macosis excluded on purpose: it is label-gated on apull request and never runs on
main, so restricting its save would mean itnever writes and never restores.
key: universal-apple-darwin-devindev-release.yml. Both releaseworkflows name the job
buildand run onmacos-15, and the action's keycomes from job id + runner OS/arch + the
keyinput, not the workflow name.With the same literal, a manual
release.ymldispatch ondevlands in thesame ref scope under the same key — and cache entries are immutable, so the
second writer is silently skipped and one channel serves the other's target
directory. Harmless while both held only the registry; not harmless now that
they hold real builds, since the channels pin different Recall SDKs and carry
different bundle identities.
Expectations deliberately tempered, not oversold
workspace members before saving (
cache-workspace-cratesdefaults false), soour own eight crates relink every run. An earlier draft quoted a precise
minutes-saved figure; it is gone, because a number that specific in a
comment-as-contract invites re-opening a closed ticket when the first
measurement comes in lower.
release.ymltriggers onv*tags, and Actions caches are ref-scoped: a tagrun restores from its own new ref or from the default branch, which never runs
that job id. Its entries are written and, in the normal release flow, never
read. Fixing the path is still correct, but warming the release lane needs
its own change and its own ticket.
dev-release.ymlis the one lane thisgenuinely warms — it runs on pushes to
devand restores its own prior save.Verification
Local, all green:
actionlint .github/workflows/*.yml— exit 0workspaces:input remains at any of the six sitesnpm exec --no-install -- vitest run --passWithNoTests=false src/layout/panes/session/__tests__/ledger-net.test.ts— 26/26--passWithNoTests=true— exits 0, which proves the flag is read rather than ignoredsave-ifconfirmed a real input at the pinned SHA (c1937114), default"true", documented as "iffalse, the cache is only restored"Only CI can prove the rest, and these are the things to read on this PR's own run:
rustjob'sSwatinem/rust-cachestep must log a cache pathending at
<workspace>/target, not<workspace>/src-tauri/target.save-if), so the warmrestore shows up on the first push to
mainafter merge, not here.Rust (windows-latest)must showPath-portability spec (Windows)as itsfinal step, passing; the macOS leg shows it skipped, which is expected.
Build macOSis label-gated (build-macos/build-test) and will not run onan unlabelled PR.
Notes for the reviewer
job as macOS-only running only cargo. The runner column has been stale since
Windows: make dev build, compile and test on Windows #62; this change would have made the checks column stale too. CLAUDE.md points
readers at README first, so leaving it contradicting the workflow was not an
option.
ci.yml's
rustjob. That PR replaces theFormat checkcomment block and theclippy steps; this one touches the cache step above them and appends a step at
the end of the job. The hunks do not overlap, but whichever lands second will
want a look.
release.yml/dev-release.yml(they are@v2while ci.yml pinsc1937114); collapsing therustandevalmacOS entries with ashared-key; and the release-lane cache being unrestorable by construction.Each is a real finding from review and each deserves its own ticket rather
than being folded in here.
Merge method: squash (do not merge-commit or rebase-merge).