ci: make cargo test blocking on every OS (fix the 14 Unix-path tests) - #15
Conversation
The Windows and Linux test legs ran under continue-on-error because 14 tests failed on Windows, all from Unix-path assumptions that had never run off-macOS. A warn-mode leg reports green while failing, so nothing Windows-only - including code that exists ONLY under `#[cfg(target_os = "windows")]`, which no other leg compiles - had a gate that could go red. That enforcement gap surfaced in review three times; this closes it structurally. The 14 break down into four kinds, each fixed at the level it deserved: - clipboard (5): `file_uri_to_path` delegates to `url::to_file_path`, whose accepted shapes are platform-specific by design - a driveless /home/... URI is an error on Windows. Per-platform fixtures; identical assertions on both. - files (2): scan labels were rendered with `display()`, yielding `plans\feat.md` on Windows. Labels are UI-facing logical names (the `path` field carries the OS path), so this one is a production fix: a `slash_label` helper renders them `/`-separated everywhere, and its second call site fixes the same latent bug in the AGENTS.md scan. - history (1): `normalize_path` deliberately lowercases on Windows (NTFS is case-insensitive); the test asserted the Unix rendering. Assert the platform-correct value on each. - community_skills (2) + codex skills (4): fixtures hand-formatted TOML with raw Windows paths - `\U` parses as a unicode escape, the config fails to parse, and the rules silently never applied. Production is immune (it writes through toml_edit, which escapes); the fixtures now use TOML literal strings and native-separator joins, since the rule match is exact string equality against the scanner's path. The community_skills pair also compared rendered strings where PathBuf equality asserts the same slug layout without the separator. With the suite green everywhere (740/740 on this Windows host), the warn-mode step is deleted and `cargo test` blocks on all three OSes. The trigger runs CI on this PR itself, so the ratchet self-validates before it can gate anyone else. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two non-blocking notes from the #13/#14 reviews. The 11-line "why this watcher exists" block ended up attached to mic_watch_policy when that module was inserted in front of it, leaving default_mic_watch - the module it describes - undocumented; moved back down. The policy module's own doc justified its placement by the Windows leg being warn-mode and red, which the previous commit makes untrue; it now states the durable reason (compiled on every leg, not one). And probeAuth now says out loud that an indeterminate verdict deliberately leaves signedOutStreak untouched - it is not evidence of being signed in - so two strikes may span an indeterminate probe between them. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
alonmuroch
left a comment
There was a problem hiding this comment.
Approving. The ratchet is the right call and the fixes underneath it hold up.
I checked the two changes that carry real risk, since the rest is fixtures:
slash_label is genuinely display-only. MemoryFileCandidate.label is consumed in exactly one place — {md.label} at +page.svelte:12044 — while path keeps the real OS path. So rendering plans/feat.md instead of plans\feat.md breaks no lookup, and the second call site in the AGENTS.md scan is the same latent bug, correctly fixed.
normalize_path really does lowercase on Windows (history.rs:73-76), so the #[cfg(windows)] assertion is the platform-correct value rather than a weakened one. The NTFS reasoning in the new comment is right.
The TOML diagnosis is correct — \U in a basic string is a unicode escape — and so is the conclusion that production is immune because toml_edit escapes on write. Good instinct to check that before settling on fixture-only.
Two notes, neither blocking:
- The
community_skillsPathBuf comparisons are now slightly tautological — the expected value is built with the samejoinchain as the subject. They still pin the slug rewrite and the directory layout, which is what those tests are for, so this is fine; just worth knowing they no longer assert anything about rendering. - The TOML literal strings break if a path ever contains a
'. Temp dirs won't, so this is a note for whoever copies the pattern.
On the ratchet itself: making Windows/Linux cargo test blocking is clearly right — #[cfg(target_os = "windows")] code currently has no gate that can go red, which is exactly how a compile error in the mic watcher would have shipped green. The pull_request: trigger validating it on this PR is the correct way to prove it. The cost is that a future Windows-runner flake blocks merges; worth taking.
Merge this before #18 — the @types/node commit there sets up the svelte-check ratchet named at the end of this description, and the two +page.svelte hunks don't overlap.
#1 docs/CHROMIUM.md described the architecture this PR deliberately did not ship. The MCP section, the tool table and the whole Layout table named files that do not exist. Rewritten against the code: the three IPC commands that replaced the server, the real file map, and why each command takes a `Window`. The crash forensics and the pump section are kept — they were the parts worth having. #2 The authorization ledger guarded nothing. `attach`/`is_attached` shipped with tests and a doc calling itself the gate agent tools must consult, while nothing called any of it — worse than no gate, because it reads as a control in review and answers "allowed" at runtime. Deleted. What actually keeps the CDP commands app-only is now real: each takes a `Window`, which the op table classifies as opaque and REFUSES over the remote WS transport. That was the live hole — arbitrary JavaScript in a signed-in Google session, reachable from a socket. #3 cef-fetch fell back to an unpinned CEF on a warning nobody reads inside a gigabyte of clone output, producing exactly the framework/bindings mismatch its own header calls undefined behaviour. Hard-fails now, naming the tag and the crate version it must match. #4 CI compiled none of it. `cargo test --workspace` (the engine suites, including report.rs's — kept outside the feature gate precisely so CI would run them, which it then didn't) and `cargo check -p brains-browser --features chromium`, which is what keeps ~2,000 gated lines from rotting. #5 `page_target_for_url` could resolve to the wrong tab — two Gmail profiles are two page targets on one origin, and `find` took whichever came first, putting one account's mail into a conversation about the other. Targets are now pinned per tab on first resolution and looked up by id after; resolution skips ids another tab already owns. #6 One OS thread per delayed pump request, spawned continuously during load and input. Replaced with the single timer the safety pump already owned, waiting on a condvar with a next-deadline slot. #7 `set_visible(true)` never restored the container's frame, so a view whose panel had not re-measured stayed parked off-screen while nominally visible. The frame is remembered on hide and restored on show — the OS-webview backend has no such asymmetry, and the panel is written not to have to know. #8 `views::open` registered the tab after dispatching the closure whose failure path removes it. Inserted first. #9 Same-site now also requires a USER GESTURE before navigating in place: the panel has no address bar, and a script-initiated hop needs none. `TWO_LABEL_SUFFIXES` gains the user-content hosts, which matter more than the country ones — anyone can take a label under `github.io`, and treating two of them as one site is the actual risk. Test added. #10/#11 The unauthenticated CDP port and the `--use-mock-keychain` + `no_sandbox` pair are now named on a release-blocker list in the doc, with what has to change. #12 The dev-token file was created at the umask and chmod'd afterwards, leaving it world-readable in between. Opened 0600, directory 0700. #15 One `reqwest::Client` for the process rather than one per call; the sentinel parse drops a fragment; "flipped bottom-left" corrected (in AppKit flipped means TOP-left) — the arithmetic was right, the word inverted it; and `init` latches on ATTEMPTED so a failed attempt cannot re-enter CefInitialize. Not done, deliberately: #13 (splitting the CI commit) is the owner's call and the offer stands; #14's unused CDP driving surface is named in the doc's Next section rather than trimmed, since the agent bridge is what lights it up. Gates: fmt 0, clippy 0, workspace tests green, chromium check clean, svelte-check 725/0/0, 1659 frontend tests, all five lints. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
What
The follow-up named in the #12 and #14 reviews: the Windows (and Linux)
cargo testlegs ran undercontinue-on-error, so nothing behind#[cfg(target_os = "windows")]had a gate that could go red — a compile error in the mic watcher would have shown green. This PR fixes the 14 tests that forced warn mode, then deletes the warn-mode step socargo testblocks on all three OSes.Also carries the two non-blocking review notes as a second commit: the misfiled watcher doc block (back on
default_mic_watch), and a written-down statement that an indeterminate probe deliberately doesn't resetsignedOutStreak.The 14 failures, by kind
url::to_file_pathis platform-specific by design (driveless/home/...errors on Windows)display()→plans\feat.mdon Windowsslash_labelhelper renders/everywhere. Its second call site fixes the same latent bug in the AGENTS.md scan, which the hierarchical test caught only after the first site was fixednormalize_pathlowercases on Windows (NTFS case-insensitivity — correct)\Uparses as a unicode escape, config fails to parse, rules silently never applyPathBufs so the slug layout is asserted without the separatorProduction TOML writes were checked before concluding fixture-only:
toggle_codex_skill_with_overrideswrites throughtoml_edit, which escapes — real users can't hit the parse failure the fixtures did.Why the ratchet is safe
pull_request:trigger runs CI on this PR itself, so the newly-blocking legs validate here before they can gate anyone else's merge.What stays in warn mode
prettier, svelte-check, and clippy — unchanged, still red on
mainfor pre-existing reasons (svelte-check needs a one-line@types/nodedevDependency; happy to do that ratchet next).🤖 Generated with Claude Code