From a9b7332fc1cf546817976096e1186293445d7723 Mon Sep 17 00:00:00 2001 From: Amir SSV Labs Date: Mon, 10 Aug 2026 14:38:46 +0300 Subject: [PATCH 01/16] build(recall): the Windows staging steps can actually run, and ship a pinned Node MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Nothing here had been run on Windows, and both steps died before staging a byte: * `npm` is `npm.cmd`, and since the 2024 argument-injection fix Node will not execFile a batch file at all — EINVAL, on a machine with npm installed perfectly well. * GNU tar (what Git for Windows puts first on PATH) parses `-czf C:\...` as the remote spec `host:path` and aborts. System32's bsdtar does not, so which shell invoked the build decided whether recording packaged. Paths now go relative to an explicit cwd, which both tars read alike. And the runtime the sidecar runs ON is now pinned on Windows too, not copied from `process.execPath`. macOS already downloads 22.17.0; Windows shipped whatever Node ran the build — 22.14 here, 24 from release.yml, something older on the machine whose build could not resolve a verbatim path at all. That is how a path-resolution bug hides in the build machine instead of in the code. win-x64 publishes a bare self-contained node.exe, so there is nothing to unpack. Verified on Windows 11 with GNU tar first on PATH: prepare downloads the pinned 22.17.0 and the win32 SDK, package stages a 37.0 MiB archive with a win32/x64 manifest, agent-windows.exe and the SDK's own node_modules inside. Co-Authored-By: Claude Opus 5 --- scripts/dev/recall/host.mjs | 40 ++++++++++++++++++++++++++ scripts/dev/recall/package-runtime.mjs | 19 ++++++------ scripts/dev/recall/prepare-sidecar.mjs | 32 +++++++++++++++++++-- 3 files changed, 79 insertions(+), 12 deletions(-) create mode 100644 scripts/dev/recall/host.mjs diff --git a/scripts/dev/recall/host.mjs b/scripts/dev/recall/host.mjs new file mode 100644 index 00000000..226f09a0 --- /dev/null +++ b/scripts/dev/recall/host.mjs @@ -0,0 +1,40 @@ +// Running host tools from Node, on every platform this build targets. +// +// Two Windows facts live here so no call site has to rediscover them: +// +// * `npm` IS `npm.cmd`, AND A .cmd NEEDS A SHELL. execFile resolves the +// extension for you on neither count: without it the staging step dies +// with ENOENT, and with it Node refuses outright (`EINVAL`) — since the +// 2024 argument-injection fix, execFile will not run a batch file except +// through a shell. So: the real name, and `shell: true`. +// * GNU TAR READS A DRIVE LETTER AS A HOSTNAME. `tar -czf C:\...` is +// parsed as the remote spec `host:path` and aborts before touching the +// disk. Git for Windows puts that tar first on PATH while System32's +// bsdtar accepts either form — so which shell invoked the build decided +// whether recording packaged. Every path goes relative to an explicit +// cwd instead, which both tars read the same way. +// +// Both are no-ops on macOS and Linux: the name is `npm`, and a relative path +// under `cwd` names the same file an absolute one did. +import { execFileSync } from "node:child_process"; +import { isAbsolute, relative } from "node:path"; + +const windows = process.platform === "win32"; + +/// Run npm in `cwd`. Every argument a caller passes is a literal flag, which +/// is what makes the Windows shell hop harmless — nothing here comes from +/// outside this repo, so there is no argument to be quoted badly. +export function npm(cwd, args) { + execFileSync(windows ? "npm.cmd" : "npm", args, { cwd, stdio: "inherit", shell: windows }); +} + +/// Run `tar` with every absolute path argument rewritten relative to `cwd`. +/// Flags pass through untouched — only an absolute path can carry the drive +/// letter that GNU tar misreads. +export function tar(cwd, args) { + execFileSync( + "tar", + args.map((arg) => (isAbsolute(arg) ? relative(cwd, arg) : arg)), + { cwd, stdio: "inherit" }, + ); +} diff --git a/scripts/dev/recall/package-runtime.mjs b/scripts/dev/recall/package-runtime.mjs index 8545bec0..6e0720db 100644 --- a/scripts/dev/recall/package-runtime.mjs +++ b/scripts/dev/recall/package-runtime.mjs @@ -24,10 +24,10 @@ // how the archive was built. Full lockfile determinism is out of scope — // tracking the SDK's dependencies is the SDK maintainer's responsibility. import { createHash } from "node:crypto"; -import { execFileSync } from "node:child_process"; import { cpSync, existsSync, readFileSync } from "node:fs"; import { copyFile, mkdir, rename, rm, writeFile } from "node:fs/promises"; import { basename, join, resolve } from "node:path"; +import { npm, tar } from "./host.mjs"; const root = resolve(import.meta.dirname, "..", "..", ".."); const sdkDir = join(root, "node_modules", "@recallai", "desktop-sdk"); @@ -64,14 +64,15 @@ const staging = join(stagingDir, ".stage"); await rm(staging, { recursive: true, force: true }); await mkdir(staging, { recursive: true }); cpSync(sdkDir, join(staging, basename(sdkDir)), { recursive: true }); -execFileSync( - "npm", - ["install", "--omit=dev", "--ignore-scripts", "--no-audit", "--no-fund", "--no-package-lock"], - { cwd: join(staging, basename(sdkDir)), stdio: "inherit" }, -); -execFileSync("tar", ["-czf", temporaryArchive, "-C", staging, basename(sdkDir)], { - stdio: "inherit", -}); +npm(join(staging, basename(sdkDir)), [ + "install", + "--omit=dev", + "--ignore-scripts", + "--no-audit", + "--no-fund", + "--no-package-lock", +]); +tar(root, ["-czf", temporaryArchive, "-C", staging, basename(sdkDir)]); await rm(staging, { recursive: true, force: true }); await rm(archivePath, { force: true }); await rename(temporaryArchive, archivePath); diff --git a/scripts/dev/recall/prepare-sidecar.mjs b/scripts/dev/recall/prepare-sidecar.mjs index e5f21ceb..53c554b9 100644 --- a/scripts/dev/recall/prepare-sidecar.mjs +++ b/scripts/dev/recall/prepare-sidecar.mjs @@ -24,6 +24,7 @@ import { Readable } from "node:stream"; import { finished } from "node:stream/promises"; import { execFileSync } from "node:child_process"; import { basename, dirname, join, resolve } from "node:path"; +import { tar } from "./host.mjs"; const root = resolve(import.meta.dirname, "..", "..", ".."); const sdkDir = join(root, "node_modules", "@recallai", "desktop-sdk"); @@ -90,7 +91,7 @@ async function ensureRecallSdk() { const url = `https://recallai-desktop-sdk-releases.s3.us-east-1.amazonaws.com/${commit}/${archiveName}`; console.log(`Preparing the Recall Desktop SDK ${pkg.version} for ${process.platform}…`); await download(url, archive); - execFileSync("tar", ["-xf", archive, "-C", sdkDir], { stdio: "inherit" }); + tar(root, ["-xf", archive, "-C", sdkDir]); await unlink(archive).catch(() => {}); if (!existsSync(executable)) { throw new Error(`the Recall SDK archive did not produce ${basename(executable)}`); @@ -116,16 +117,41 @@ async function officialMacNode() { if (!existsSync(extracted)) { console.log(`Downloading a self-contained Node ${nodeVersion} (${arch}) for the sidecar…`); await download(`https://nodejs.org/dist/v${nodeVersion}/${folder}.tar.gz`, archive); - execFileSync("tar", ["-xzf", archive, "-C", binariesDir], { stdio: "inherit" }); + tar(root, ["-xzf", archive, "-C", binariesDir]); await unlink(archive).catch(() => {}); } return extracted; } +/// The pinned Windows runtime. `nodejs.org/dist/…/win-x64/node.exe` is already +/// the single self-contained executable, so there is nothing to unpack. +/// +/// PINNED FOR THE SAME REASON AS macOS, and the reason is not tidiness: what +/// the sidecar ships decides how it resolves paths. The reference app's +/// Windows recording died because its Node could not resolve a main module +/// from the verbatim path Tauri hands over (`\\?\C:\…`) — and a Node that +/// old shipped only because it happened to be the one that ran the build. +/// Node 22.14 and 24 both resolve it; some older ones do not. Copying +/// `process.execPath` makes the shipped app a function of the build machine, +/// which is how that class of bug stays invisible until a user hits it. +async function officialWindowsNode() { + const pinned = join(binariesDir, `node-v${nodeVersion}-win-x64.exe`); + if (!existsSync(pinned)) { + console.log(`Downloading the pinned Node ${nodeVersion} (win-x64) for the sidecar…`); + await download(`https://nodejs.org/dist/v${nodeVersion}/win-x64/node.exe`, pinned); + } + return pinned; +} + async function ensureNodeSidecar() { await mkdir(binariesDir, { recursive: true }); const suffix = process.platform === "win32" ? ".exe" : ""; - const source = process.platform === "darwin" ? await officialMacNode() : process.execPath; + const source = + process.platform === "darwin" + ? await officialMacNode() + : process.platform === "win32" + ? await officialWindowsNode() + : process.execPath; const names = new Set([`recall-node-${target}${suffix}`]); if (process.platform === "darwin") { // Tauri's universal build resolves the first name; single-arch local From bbc23a8e1bd3497419f7283fb9ab7470cd5ce79f Mon Sep 17 00:00:00 2001 From: Amir SSV Labs Date: Mon, 10 Aug 2026 14:39:02 +0300 Subject: [PATCH 02/16] fix(windows): simplify the verbatim resource dir before a child process sees it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Tauri answers `resource_dir()` with `\?\C:\...` on Windows. Every Rust-side check passes on it, which is why nothing catches this: the path is valid to Rust and to Windows. It reaches Node twice — the sidecar's script argument and RECALL_SDK_PATH — and an older Node parses it as a UNC share, takes `C:` for the host, and lstats it. That is `EISDIR: lstat 'C:'`, the sidecar dying before it runs a line of its own code, and it is what "Record does nothing on Windows" was in the reference app (brains-desktop PR #12, commit 0e06e45). Honest scope: on the Node this now pins (22.17.0) I could NOT reproduce the crash — an A/B against the real binaries loads the sidecar and requires the SDK from a verbatim path fine, on both forms. Node fixed it somewhere between the version that build shipped and this one. So this is one line of insurance that removes a variable rather than the crash-fix it was upstream; the pin in the previous commit is what actually makes the behaviour deterministic. Both are cheap and neither is speculative — the same path also reaches the browser engine and the manifest loaders. Simplified once, at the seam where Tauri hands the path over, rather than at each of the places that eventually spawn a child. A genuine UNC path has no drive letter to misread and is left alone (dunce's rule, not ours). dunce was already in the lockfile via the Tauri tree; this only makes it a direct dep. Co-Authored-By: Claude Opus 5 --- Cargo.lock | 2 + src-tauri/Cargo.toml | 4 ++ src-tauri/src/lib.rs | 6 ++- src-tauri/src/resources.rs | 77 ++++++++++++++++++++++++++++++++++++++ 4 files changed, 88 insertions(+), 1 deletion(-) create mode 100644 src-tauri/src/resources.rs diff --git a/Cargo.lock b/Cargo.lock index 6a658f7a..e5277894 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -397,6 +397,7 @@ dependencies = [ "brains-recording", "brains-storage", "chrono", + "dunce", "log", "once_cell", "serde", @@ -477,6 +478,7 @@ dependencies = [ "tar", "tempfile", "thiserror 2.0.19", + "windows-sys 0.59.0", ] [[package]] diff --git a/src-tauri/Cargo.toml b/src-tauri/Cargo.toml index b90093c1..c51048cb 100644 --- a/src-tauri/Cargo.toml +++ b/src-tauri/Cargo.toml @@ -47,6 +47,10 @@ chrono = { workspace = true } # 32-byte token (src/remote/mod.rs). sha1 = "0.10" base64 = { workspace = true } +# One job: strip the `\\?\` that Tauri puts on `resource_dir()`, before the +# path reaches a Node child that cannot parse it (src/resources.rs). Already in +# the lockfile via the Tauri tree — this only makes it a direct dependency. +dunce = "1" # Permissions granted in capabilities/default.json must have their plugin # present here, or ACL resolution fails at build time. tauri-plugin-single-instance = "2" diff --git a/src-tauri/src/lib.rs b/src-tauri/src/lib.rs index d38593ba..19f834e8 100644 --- a/src-tauri/src/lib.rs +++ b/src-tauri/src/lib.rs @@ -32,6 +32,8 @@ pub mod remote; /// The 3-domain readiness composition: each engine answers about its own /// domain, the index puts the report together. pub mod readiness; +/// The bundle's resource directory, in a form a child process can read. +pub mod resources; /// How an app session is made brains-native: the context prompt every /// interactive run appends, and the capture port its turns land through. pub mod session; @@ -387,7 +389,9 @@ pub fn run() { // Construction order is the dependency order: agents run // through model, model persists through storage. let root = brains_storage::default_root().map_err(|e| e.to_string())?; - let resource_dir = app.path().resource_dir().ok(); + // Simplified, not raw: on Windows Tauri answers with a verbatim + // path that Node's module resolver cannot read (resources.rs). + let resource_dir = resources::simplified(app.path().resource_dir().ok()); let version = app.package_info().version.to_string(); // THE DATA GUARD (D1-D4): versioned backups on upgrade, prior-use diff --git a/src-tauri/src/resources.rs b/src-tauri/src/resources.rs new file mode 100644 index 00000000..7aac2581 --- /dev/null +++ b/src-tauri/src/resources.rs @@ -0,0 +1,77 @@ +// THE BUNDLE'S RESOURCE DIRECTORY, IN A FORM A CHILD PROCESS CAN READ. +// +// Tauri answers `resource_dir()` with a VERBATIM path on Windows — +// `\\?\C:\Program Files\brains\resources`. It is a perfectly valid Win32 +// path: Rust opens it, `is_file()` answers, and every check this app makes +// against it passes. So does CI. +// +// Node does not. Its module resolver parses `\\?\C:\…` as a UNC share, +// takes `C:` for the host, and `lstat`s it: +// +// Error: EISDIR: illegal operation on a directory, lstat 'C:' +// at Object.realpathSync (node:fs) +// at resolveMainPath (node:internal/modules/run_main) +// +// That is the recording sidecar dying before it runs a line of its own code, +// which is what "Record does nothing on Windows" looked like in the reference +// app (brains-desktop PR #12). The path is handed to Node twice — as the +// script argument and as `RECALL_SDK_PATH` — and neither is reachable from a +// Rust-side check, because nothing about the path is wrong on the Rust side. +// +// So it is simplified ONCE, here, at the boundary where Tauri hands it over, +// rather than at each of the places that eventually spawn a child. The engines +// downstream (recording, browser, the manifest loaders) then only ever see a +// path every process can resolve. +// +// A genuine UNC path (`\\server\share\…`) has no drive letter to misread and +// is left exactly as it is — that is `dunce`'s rule, not ours. + +use std::path::PathBuf; + +/// Normalize the resource directory Tauri resolved. A no-op on macOS and +/// Linux, and on Windows UNC paths that were never verbatim. +pub fn simplified(dir: Option) -> Option { + dir.map(|dir| dunce::simplified(&dir).to_path_buf()) +} + +#[cfg(test)] +mod tests { + use super::*; + + /// The shape every platform shares: nothing is invented, nothing is lost. + #[test] + fn a_plain_path_is_returned_unchanged() { + let plain = if cfg!(windows) { + PathBuf::from(r"C:\Program Files\brains\resources") + } else { + PathBuf::from("/Applications/brains.app/Contents/Resources") + }; + assert_eq!(simplified(Some(plain.clone())), Some(plain)); + assert_eq!(simplified(None), None); + } + + /// The bug itself: the prefix Tauri adds and Node cannot read. + #[cfg(windows)] + #[test] + fn a_verbatim_resource_dir_loses_its_prefix_before_reaching_node() { + let verbatim = PathBuf::from(r"\\?\C:\Program Files\brains\resources"); + let simple = simplified(Some(verbatim.clone())).expect("Some in, Some out"); + + assert_eq!(simple, PathBuf::from(r"C:\Program Files\brains\resources")); + // The assertion that actually matters is about what Node's resolver + // sees: no `\\?\`, so no phantom `C:` host to lstat. + assert!( + !simple.to_string_lossy().starts_with(r"\\?\"), + "still verbatim: {simple:?}" + ); + assert_ne!(simple, verbatim, "the input really was verbatim"); + } + + /// A real network path is not a bug to be fixed. + #[cfg(windows)] + #[test] + fn a_genuine_unc_path_is_left_alone() { + let unc = PathBuf::from(r"\\build-server\share\brains\resources"); + assert_eq!(simplified(Some(unc.clone())), Some(unc)); + } +} From 2fc1d779865899707e9b0a1d3e04f7b59ec14b49 Mon Sep 17 00:00:00 2001 From: Amir SSV Labs Date: Mon, 10 Aug 2026 14:58:21 +0300 Subject: [PATCH 03/16] fix(recording): give Windows the process-tree kill it never had MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This file's header calls "the group, not the process" load-bearing: Recall launches a native helper beneath Node, and an orphan of it deadlocks the next app launch. Both halves of that were `#[cfg(unix)]` — process_group(0) at spawn and kill(-pid) in Drop — so on Windows nothing in our code could reach the helper at all. Windows has no process group to signal; the primitive is a Job Object, where membership is inherited by everything a member starts and KILL_ON_JOB_CLOSE has the OS terminate all of them when the last handle closes. WHAT I MEASURED, because the header's warning invites a stronger claim than the evidence supports: against the real SDK on Windows 11, `prepare` does launch a real agent-windows.exe (0 → 1 helpers), and dropping the sidecar does clear it. But with the job object disabled the helper ALSO disappeared, ~195ms later, indistinguishable from with it — Node is killed abruptly (TerminateProcess, no exit handling), so the helper is noticing its parent died on its own. The leak this fix is named for did not reproduce. Kept anyway, and the honest justification is narrower than "fixes a leak": the reason a caller discards a sidecar is a WEDGED native SDK, and a wedged helper is precisely the one that may not notice anything. Self-exit is the SDK's courtesy; the job is the OS's guarantee. It costs one handle. The handle is a FIELD rather than a local so it closes after Drop's explicit kill, and so a force-killed app still takes the tree with it (the OS closes handles for a dead process). Assignment happens immediately after spawn, before the child is asked to do anything: Node starts the helper while handling `prepare`, which cannot happen until spawn returns and a caller writes, so nothing it spawns is born outside the job. A job the OS refuses is logged and the sidecar still runs — no recording is worse than a leaked helper. Two unit tests, both non-vacuous on Windows: the child really is a member (IsProcessInJob — a silent assignment failure would make every claim above false), and closing the handle KILLS a child nobody killed, which is the only way to know the limit flag took effect rather than merely being set. Also fixes a Layout assertion that could not pass on Windows: the sidecar runtime is `recall-node.exe` there, and `ends_with` on a Path is by component. Co-Authored-By: Claude Opus 5 --- src/engines/recording/Cargo.toml | 13 ++ src/engines/recording/src/lib.rs | 13 +- src/engines/recording/src/sidecar.rs | 187 ++++++++++++++++++++++++++- 3 files changed, 211 insertions(+), 2 deletions(-) diff --git a/src/engines/recording/Cargo.toml b/src/engines/recording/Cargo.toml index 7a5ea1ab..8c193df4 100644 --- a/src/engines/recording/Cargo.toml +++ b/src/engines/recording/Cargo.toml @@ -31,6 +31,19 @@ tar = "0.4" # to die, or Recall's native helper outlives it and wedges the next launch. libc = "0.2" +[target.'cfg(windows)'.dependencies] +# The same requirement by the only mechanism Windows offers for it: there are +# no process groups to signal, so the child and everything it starts go into a +# Job Object that dies with its handle (the `job` module in src/sidecar.rs). +windows-sys = { version = "0.59", features = [ + "Win32_Foundation", + "Win32_System_JobObjects", + # JOBOBJECT_EXTENDED_LIMIT_INFORMATION embeds IO_COUNTERS, which lives here. + "Win32_System_Threading", + # CreateJobObjectW takes a SECURITY_ATTRIBUTES pointer (we pass null). + "Win32_Security", +] } + [target.'cfg(target_os = "macos")'.dependencies] # CoreAudio FFI: process-object polling for call detection and device queries. coreaudio-sys = "0.2" diff --git a/src/engines/recording/src/lib.rs b/src/engines/recording/src/lib.rs index 432f9028..5b91d265 100644 --- a/src/engines/recording/src/lib.rs +++ b/src/engines/recording/src/lib.rs @@ -250,6 +250,17 @@ mod tests { layout.runtime_dir().unwrap(), Path::new("/app/Resources/recall/runtime") ); - assert!(layout.node_path().unwrap().ends_with("recall-node")); + // The name carries `.exe` on Windows, and the whole point of the field + // is that it resolves NEXT TO the executable rather than in resources. + let node = layout.node_path().unwrap(); + assert_eq!(node.parent().unwrap(), Path::new("/app/MacOS")); + assert!( + node.ends_with(if cfg!(target_os = "windows") { + "recall-node.exe" + } else { + "recall-node" + }), + "unexpected sidecar runtime name: {node:?}" + ); } } diff --git a/src/engines/recording/src/sidecar.rs b/src/engines/recording/src/sidecar.rs index 91bc85e2..f8fbcf2b 100644 --- a/src/engines/recording/src/sidecar.rs +++ b/src/engines/recording/src/sidecar.rs @@ -11,7 +11,9 @@ // * THE GROUP, NOT THE PROCESS. Recall launches a native helper beneath // Node. Killing Node alone orphans it, and an orphaned helper deadlocks // the next app launch. The child is spawned into its own process group -// and the whole group is killed. +// and the whole group is killed. Windows has no process group to signal, +// so a Job Object stands in for one (the `job` module below) — same +// property, different primitive. // * DROP NEVER CALLS BACK IN. The reason a caller discards a sidecar is // usually that the native SDK is wedged; asking that SDK to shut down // politely from Drop turns a recoverable timeout into a second stall. @@ -68,6 +70,12 @@ pub struct Sidecar { stdin: BufWriter, stdout: BufReader, next_id: u64, + /// The job this sidecar's whole tree lives in. Held for as long as the + /// sidecar is, and never read: closing it on Drop is the entire point, and + /// that is what kills Recall's native helper. + #[cfg(windows)] + #[allow(dead_code)] + job: Option, } impl Sidecar { @@ -91,6 +99,14 @@ impl Sidecar { let mut child = command.spawn().map_err(|e| { Error::sidecar(format!("start the sidecar ({}): {e}", spec.program.display())) })?; + // Windows' stand-in for the process group: assign the child NOW, before + // it has been asked to do anything. Node starts Recall's native helper + // while handling `prepare`, which cannot happen until this function + // returns and a caller writes a request — so nothing it spawns can be + // born outside the job. A job that could not be created is logged and + // the sidecar still runs: no recording is worse than a leaked helper. + #[cfg(windows)] + let job = job::Job::containing(&child); let stdin = child .stdin .take() @@ -114,6 +130,8 @@ impl Sidecar { stdin: BufWriter::new(stdin), stdout: BufReader::new(stdout), next_id: PROTOCOL_VERSION, + #[cfg(windows)] + job, }) } @@ -194,5 +212,172 @@ impl Drop for Sidecar { // Always reap: a zombie sidecar is still a process the next launch // has to reason about. let _ = self.child.wait(); + // On Windows the line above killed Node and NOTHING beneath it. What + // takes the native helper with it is `self.job` being dropped, which + // happens right after this body returns (fields drop last) — that is + // the whole reason the handle is a field and not a local in `spawn`. + } +} + +/// Windows' answer to "kill the group, not the process". +/// +/// There is no signal that reaches a process tree here, and Recall's native +/// helper is a grandchild of this app. A Job Object is the primitive that +/// actually models the tree: every process the member starts is a member too, +/// and `KILL_ON_JOB_CLOSE` means the OS terminates all of them when the last +/// handle to the job closes. That happens when this struct drops — including +/// when the app is force-killed, since the OS closes handles for a dead +/// process. An orphaned helper wedges the NEXT launch, so "the app died" is +/// exactly the case that must not leak one. +#[cfg(windows)] +mod job { + use std::os::windows::io::AsRawHandle; + use std::process::Child; + + use windows_sys::Win32::Foundation::{CloseHandle, HANDLE}; + use windows_sys::Win32::System::JobObjects::{ + AssignProcessToJobObject, CreateJobObjectW, JobObjectExtendedLimitInformation, + SetInformationJobObject, JOBOBJECT_EXTENDED_LIMIT_INFORMATION, + JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE, + }; + + pub struct Job(HANDLE); + + // The handle is owned by this struct and only ever used from Drop or the + // constructor; the underlying job is process-wide OS state, not thread + // state. Send is what lets a Sidecar be moved between threads as before. + unsafe impl Send for Job {} + + impl Job { + /// Put `child` — and therefore everything it goes on to start — into a + /// fresh anonymous job. `None` when the OS refused, which is not fatal: + /// the caller keeps a working sidecar and loses only the guarantee. + pub fn containing(child: &Child) -> Option { + // SAFETY: an anonymous, unnamed job with default security. A null + // return is the documented failure and is checked before use. + let handle = unsafe { CreateJobObjectW(std::ptr::null(), std::ptr::null()) }; + if handle.is_null() { + eprintln!("[recall-sidecar] no job object: the native helper may outlive a crash"); + return None; + } + let job = Self(handle); + + let mut limits: JOBOBJECT_EXTENDED_LIMIT_INFORMATION = unsafe { std::mem::zeroed() }; + limits.BasicLimitInformation.LimitFlags = JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE; + // SAFETY: `limits` is a fully initialised struct of exactly the + // class named, and its size is taken from the type itself. + let limited = unsafe { + SetInformationJobObject( + job.0, + JobObjectExtendedLimitInformation, + std::ptr::addr_of!(limits).cast(), + std::mem::size_of::() as u32, + ) + }; + if limited == 0 { + // Without the flag the job would keep the tree ALIVE after the + // handle closed — worse than no job at all, so drop it. + eprintln!("[recall-sidecar] job object refused kill-on-close; not using it"); + return None; + } + // SAFETY: the child is alive (it was just spawned and has not been + // waited on), so its handle is valid for the length of this call. + let assigned = + unsafe { AssignProcessToJobObject(job.0, child.as_raw_handle() as HANDLE) }; + if assigned == 0 { + eprintln!("[recall-sidecar] could not put the sidecar in a job object"); + return None; + } + Some(job) + } + + /// Is `child` actually a member? Only the tests ask — a caller that + /// holds a `Job` has already been told the assignment succeeded. + #[cfg(test)] + pub fn contains(&self, child: &Child) -> bool { + let mut member = 0; + // SAFETY: both handles are live, `member` is a valid out-param. + let queried = unsafe { + windows_sys::Win32::System::JobObjects::IsProcessInJob( + child.as_raw_handle() as HANDLE, + self.0, + &mut member, + ) + }; + queried != 0 && member != 0 + } + } + + #[cfg(test)] + mod tests { + use super::*; + use std::process::{Command, Stdio}; + use std::time::{Duration, Instant}; + + /// A child that stays alive long enough to be asked about, with its + /// stdio piped the way the real sidecar's is. + fn lingering_child() -> Child { + Command::new("cmd") + .args(["/C", "ping", "-n", "20", "127.0.0.1"]) + .stdin(Stdio::piped()) + .stdout(Stdio::piped()) + .stderr(Stdio::piped()) + .spawn() + .expect("spawn cmd") + } + + /// Half the guarantee: the process really is IN the job. If the + /// assignment silently failed, the tree would be unmanaged and every + /// later claim in this file would be false. + #[test] + fn the_child_is_a_member_of_the_job_it_was_given() { + let mut child = lingering_child(); + let job = Job::containing(&child).expect("a job object"); + assert!(job.contains(&child), "the child was not assigned"); + drop(job); + let _ = child.kill(); + let _ = child.wait(); + } + + /// The other half: closing the handle is a KILL. Nothing here kills the + /// child — if `KILL_ON_JOB_CLOSE` were not set it would run its full 20 + /// pings and this test would time out. + /// + /// Membership is inherited by everything the member starts, which is + /// what extends this to Recall's native helper: it is a grandchild, so + /// it is in the job, so this same close terminates it. + #[test] + fn closing_the_job_kills_a_child_nobody_killed() { + let mut child = lingering_child(); + let job = Job::containing(&child).expect("a job object"); + let running = child.try_wait().expect("try_wait").is_none(); + assert!(running, "the child died on its own; nothing is proven"); + + drop(job); + + let deadline = Instant::now() + Duration::from_secs(5); + loop { + match child.try_wait().expect("try_wait") { + Some(_) => break, + None if Instant::now() < deadline => { + std::thread::sleep(Duration::from_millis(25)); + } + None => { + let _ = child.kill(); + let _ = child.wait(); + panic!("the child outlived its job: kill-on-close is not in effect"); + } + } + } + } + } + + impl Drop for Job { + fn drop(&mut self) { + // The kill: this is the last handle, so the OS terminates every + // process still in the job. + // SAFETY: the handle came from CreateJobObjectW and is closed once. + unsafe { CloseHandle(self.0) }; + } } } From 79cc2fa0fd9496a80409198d3d1494db36403f33 Mon Sep 17 00:00:00 2001 From: Amir SSV Labs Date: Mon, 10 Aug 2026 14:58:42 +0300 Subject: [PATCH 04/16] test(recording): run the real sidecar against the real SDK on Windows MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Recording had never been executed on Windows (the engine README said so), and nothing in the suite could have caught what actually broke it in the reference app. `fake_sidecar.rs` is `#![cfg(unix)]`, so on Windows it does not run at all; `test-sidecar.mjs` drives the sidecar against a fake SDK. A path the app resolves correctly and Node cannot, and a helper that outlives its parent, are invisible to both — only real bytes show them. So: the post-install smoke check the reference app's review argued for, in the cheapest form that still runs real code. It needs no API key (the key buys uploads and transcription; extraction, the sidecar and `prepare` are entirely local) and no microphone (`prepare` warms a capture, it does not start one). Ignored by default because it needs a built bundle's assets, which it takes from two env vars and names in the failure when they are unset. What it proves, on this machine today: the 38 MiB archive verifies and unpacks, the real recall-node starts the real sidecar, `init` and `prepare` reach Recall's native Windows recorder (which reports microphone and system-audio granted), a real agent-windows.exe appears, and none survives the teardown. It also refuses to prove things vacuously: it counts helpers BEFORE and DURING, and if `prepare` never launched one it says the orphan check established nothing rather than reporting a pass. That distinction is the whole reason the measurement in the previous commit could be made at all. Co-Authored-By: Claude Opus 5 --- .../recording/tests/real_sidecar_smoke.rs | 143 ++++++++++++++++++ 1 file changed, 143 insertions(+) create mode 100644 src/engines/recording/tests/real_sidecar_smoke.rs diff --git a/src/engines/recording/tests/real_sidecar_smoke.rs b/src/engines/recording/tests/real_sidecar_smoke.rs new file mode 100644 index 00000000..fbacdfc1 --- /dev/null +++ b/src/engines/recording/tests/real_sidecar_smoke.rs @@ -0,0 +1,143 @@ +//! The REAL sidecar, the REAL runtime archive, on this machine. +//! +//! Everything else about recording is proved against a fake: `fake_sidecar.rs` +//! drives the contract with a shell script (and is `#![cfg(unix)]`, so on +//! Windows it does not run at all), and `test-sidecar.mjs` drives the sidecar +//! against a fake SDK. Neither can catch what actually broke recording on +//! Windows in the reference app: a path the app resolved correctly and Node +//! could not, and a native helper that outlived the process that started it. +//! Both are invisible until real bytes are executed. +//! +//! So this is the post-install smoke check the reference app's review argued +//! for, in the cheapest form that still runs real code. It needs NO API key — +//! the key buys uploads and transcription, while extraction, the sidecar and +//! `prepare` are entirely local — and no microphone: `prepare` warms the +//! capture, it does not start one. +//! +//! IGNORED BY DEFAULT because it needs assets a plain checkout does not have. +//! Point it at a built app (or at `src-tauri/` after `npm run recall:package`): +//! +//! ```text +//! BRAINS_RECALL_BUNDLE=…/target/release/recall \ +//! BRAINS_RECALL_NODE=…/target/release/recall-node.exe \ +//! cargo test -p brains-recording --test real_sidecar_smoke -- --ignored --nocapture +//! ``` + +use std::path::PathBuf; + +use brains_recording::{runtime, Sidecar, Spec}; +use serde_json::json; + +/// The two paths this test cannot invent. Absent → the test says what to set +/// rather than passing vacuously. +fn assets() -> (PathBuf, PathBuf) { + let bundle = std::env::var_os("BRAINS_RECALL_BUNDLE") + .map(PathBuf::from) + .expect("set BRAINS_RECALL_BUNDLE to a bundle's `recall` directory (runtime/ + sidecar/)"); + let node = std::env::var_os("BRAINS_RECALL_NODE") + .map(PathBuf::from) + .expect("set BRAINS_RECALL_NODE to the recall-node executable"); + assert!( + bundle.join("runtime").join("manifest.json").is_file(), + "no runtime manifest under {bundle:?} — run `npm run recall:package -- --require` first" + ); + assert!(node.is_file(), "no sidecar runtime at {node:?}"); + (bundle, node) +} + +/// How many `agent-windows.exe` processes exist right now. Recall's native +/// helper is a GRANDCHILD of this test, so it is the thing a process-only kill +/// leaves behind — and an orphan of it wedges the next launch. +#[cfg(windows)] +fn native_helpers() -> usize { + let out = std::process::Command::new("tasklist") + .args(["/FI", "IMAGENAME eq agent-windows.exe", "/NH"]) + .output() + .expect("run tasklist"); + String::from_utf8_lossy(&out.stdout) + .lines() + .filter(|line| line.to_ascii_lowercase().contains("agent-windows.exe")) + .count() +} + +#[test] +#[ignore = "needs a built bundle's recall assets; see the module docs"] +fn the_real_runtime_unpacks_and_the_real_sidecar_prepares() { + let (bundle, node) = assets(); + let temporary = tempfile::tempdir().expect("tempdir"); + + // 1. The archive: hash-verified, unpacked through staging, atomically named. + let sdk = runtime::ensure(&bundle.join("runtime"), &temporary.path().join("rt"), "smoke") + .expect("the bundled Recall runtime must verify and unpack"); + assert!( + runtime::files_present(&sdk), + "the extracted runtime is missing its executable or index.js: {sdk:?}" + ); + println!("runtime unpacked to {sdk:?}"); + + // 2. The sidecar: the real Node, the real script, the real SDK. This is the + // step that dies on a path the app itself considers perfectly valid. + let script = bundle.join("sidecar").join("index.cjs"); + let spec = Spec::new(&node).arg(&script).env("RECALL_SDK_PATH", &sdk); + + #[cfg(windows)] + let helpers_before = native_helpers(); + + let mut sidecar = Sidecar::spawn(&spec).expect("the sidecar must start"); + println!("sidecar pid {}", sidecar.pid()); + + sidecar + .call("init", json!({ "apiUrl": brains_recording::api::url() })) + .expect("init must reach the native SDK"); + // 3. `prepare` is where the native recorder is actually loaded and the + // whole-desktop capture warmed — the first step that runs Recall's own + // code rather than ours. + sidecar + .call( + "prepare", + json!({ "apiUrl": brains_recording::api::url(), "meetingDetectionTrusted": false }), + ) + .expect("prepare must warm the native recorder"); + println!("prepare ok"); + + // Whether Recall's native helper is running YET decides whether the + // teardown check below is worth anything. Measured while the sidecar is + // still alive, and reported either way. + #[cfg(windows)] + let helpers_during = native_helpers(); + #[cfg(windows)] + println!("native helpers: {helpers_before} before, {helpers_during} after prepare"); + + // 4. The teardown that only real bytes can test: dropping the sidecar must + // take the native helper with it, not just Node. + drop(sidecar); + + #[cfg(windows)] + { + // The kill is asynchronous — the OS terminates job members, it does not + // wait for them. + let dropped_at = std::time::Instant::now(); + let deadline = dropped_at + std::time::Duration::from_secs(5); + while native_helpers() > helpers_before && std::time::Instant::now() < deadline { + std::thread::sleep(std::time::Duration::from_millis(25)); + } + println!("helper count settled after {:?}", dropped_at.elapsed()); + assert_eq!( + native_helpers(), + helpers_before, + "a native Recall helper outlived its sidecar — it will wedge the next launch" + ); + // SAY SO WHEN NOTHING WAS PROVED. If `prepare` never launched a helper, + // the assertion above compared zero to zero and this leg of the test is + // vacuous — the mechanism is still covered by the job-object unit tests + // in `sidecar.rs`, but a real orphan was never at stake here. + if helpers_during > helpers_before { + println!("no orphaned native helper (a real one was running before the drop)"); + } else { + println!( + "NOTE: no native helper ran during prepare, so the orphan check proved nothing. \ + Recall may only launch it at `start`, which needs a key." + ); + } + } +} From 4f3e044ba46f4fb9d5653644efa1d66ded45342e Mon Sep 17 00:00:00 2001 From: Amir SSV Labs Date: Mon, 10 Aug 2026 14:58:42 +0300 Subject: [PATCH 05/16] test(recall): the sidecar contract suite can pass on Windows MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three of the four tests failed there, and none for a Windows reason. Two fixtures fired their events from inside requestPermission(), which the sidecar only calls on darwin — so a test about platform-independent detection logic (trust an ID-only meeting event over whole-desktop capture) was silently macOS-only, and the stderr-not-stdout rule went unchecked. The events now come from prepare, which runs everywhere. The permissions assertion was the one real platform difference: requestPermissions() returns early off darwin, so nothing is requested and nothing is reported. Asserted as the Windows contract now instead of failing — worth knowing that Windows recording consults no permission of its own, though the live smoke test shows the SDK reporting microphone and system-audio granted without being asked. Not cosmetic: the failures left children alive, so the suite ran 120s and produced no output at all before the runner gave up. It is 4.6s green now — which matters, because a gate nobody can run on the platform being changed is how this class of bug got in. Audit ledger updated with what was measured rather than what was expected: both Windows rows move out of N/A, and both record that the failure they name did NOT reproduce here. Co-Authored-By: Claude Opus 5 --- scripts/dev/recall/test-sidecar.mjs | 34 ++++++++++++++++++++++------- scripts/eval/RECORDING-FIX-AUDIT.md | 20 ++++++++++++++--- 2 files changed, 43 insertions(+), 11 deletions(-) diff --git a/scripts/dev/recall/test-sidecar.mjs b/scripts/dev/recall/test-sidecar.mjs index 1f404276..af5e5678 100644 --- a/scripts/dev/recall/test-sidecar.mjs +++ b/scripts/dev/recall/test-sidecar.mjs @@ -100,7 +100,14 @@ test("the sidecar writes mixed PCM callbacks to a valid local WAV", async () => await send(1, "init", { apiUrl: "https://example.test" }); const prepared = await send(2, "prepare", { apiUrl: "https://example.test" }); - assert.deepEqual(prepared.permissions, { microphone: "granted", "system-audio": "granted" }); + // Permissions are a macOS concept HERE: `requestPermissions()` returns early + // off darwin, so nothing is requested and nothing is reported. That is the + // sidecar's actual contract on Windows, asserted rather than assumed — a + // Windows build that suddenly reported a permission map would be news. + assert.deepEqual( + prepared.permissions, + process.platform === "darwin" ? { microphone: "granted", "system-audio": "granted" } : {}, + ); const started = await send(3, "start", { apiUrl: "https://example.test", @@ -147,13 +154,18 @@ test("the sidecar trusts an ID-only meeting detection over whole-desktop capture exports.init = async () => null; exports.requestPermission = async (permission) => { listeners.get("permission-status")?.({ permission, status: "granted" }); - if (permission === "accessibility") { - // No title, no URL, no platform — exactly the event that used to - // be discarded, silently downgrading a live Meet. - listeners.get("meeting-detected")?.({ window: { id: "google-meet-window" } }); - } }; - exports.prepareDesktopAudioRecording = async () => "desktop-audio-fallback"; + exports.prepareDesktopAudioRecording = async () => { + // No title, no URL, no platform — exactly the event that used to be + // discarded, silently downgrading a live Meet. + // + // Fired from HERE, not from requestPermission("accessibility"): the + // sidecar only requests permissions on darwin, so hanging the event + // off that call made this test — which is about platform-independent + // detection logic — silently macOS-only. prepare runs everywhere. + listeners.get("meeting-detected")?.({ window: { id: "google-meet-window" } }); + return "desktop-audio-fallback"; + }; exports.startRecording = async () => null; exports.pauseRecording = async () => null; exports.resumeRecording = async () => { @@ -254,7 +266,13 @@ test("SDK chatter never reaches stdout", async () => { }); await send(3, "stop"); - for (const noise of ["init noise", "permission noise", "prepare noise", "start noise"]) { + // "permission noise" only exists where a permission is requested, which is + // darwin (see requestPermissions in the sidecar). The rule under test — SDK + // chatter goes to stderr, never stdout — is checked on every platform by the + // three calls that do run everywhere. + const noises = ["init noise", "prepare noise", "start noise"]; + if (process.platform === "darwin") noises.push("permission noise"); + for (const noise of noises) { assert.ok(stderr.includes(noise), `"${noise}" is missing from stderr:\n${stderr}`); } await close(); diff --git a/scripts/eval/RECORDING-FIX-AUDIT.md b/scripts/eval/RECORDING-FIX-AUDIT.md index 68459c0a..52453392 100644 --- a/scripts/eval/RECORDING-FIX-AUDIT.md +++ b/scripts/eval/RECORDING-FIX-AUDIT.md @@ -14,7 +14,8 @@ Regression ledger: main's production recording fixes mapped to our w2 implementa | Release warm capture before meeting | sidecar fix (3a225de) | A warmed whole-desktop capture holds its own mic binding. Starting meeting recording with that binding alive leaves two recorders on one device. | PRESENT — sidecar/index.cjs:394-407 checks/releases `preparedWindowId` before meeting capture | None | | Watcher race killing live recording | b73bb0f | Windows: watcher checked `is_recording()` OUTSIDE the SIDECAR lock; a recording could start between check and lock acquisition, then watcher kills it. | N/A — Windows-only fix; macOS call_detector is read-only | None | | Stale Recall runtime stuck | 1497859 | Windows: A stuck stale Recall runtime directory (held by EDR/old sidecar) made prepare fail on every launch with error 4395. | PRESENT — runtime.rs:167-199 `retire_stale_runtime()`: retry delete 3x, rename to `.trash-*`, extract to `.alt-*` | None | -| Verbatim path on Windows | 0e06e45 | Tauri's `resource_dir()` returns verbatim path on Windows (`\\?\C:\...`). Node cannot resolve modules from these. | N/A — Windows-only; not currently targeted | None | +| Verbatim path on Windows | 0e06e45 | Tauri's `resource_dir()` returns verbatim path on Windows (`\\?\C:\...`). Node cannot resolve modules from these. | PRESENT — `src-tauri/src/resources.rs::simplified()`, applied at lib.rs:390 where Tauri hands the path over, so recording, browser and the manifest loaders all get a path a child process can resolve | Fixed w2/windows-recording | +| Process tree, not process (Windows) | — (`#[cfg(unix)]` gap, not an upstream fix) | Windows had NO equivalent of the unix process-group kill: `child.kill()` reaps Node only, so nothing in our code reached Recall's `agent-windows.exe` — the helper the file's own header says an orphan of will wedge the next launch. | PRESENT — `sidecar.rs` `job` module: an anonymous Job Object with `KILL_ON_JOB_CLOSE`, assigned before the child is asked to do anything, held as a field so it closes after the explicit kill. **MEASURED, and the leak did not reproduce:** with the job disabled, the real helper still disappeared ~195ms after Node was terminated (identical to with it) — it self-exits when its parent dies. So this is the wedged case only, which is exactly when a sidecar is discarded | Fixed w2/windows-recording | | Default mic watcher (Windows) | d510348 | Windows: sidecar binds input device at prepare forever; changing default in Sound settings ignored. | N/A — Windows-only; macOS uses CoreAudio listeners | None | | ComGuard documentation | c2ad67b | Windows: Comment said guard didn't uninitialize on S_FALSE but code was correct; documentation fixed to match. | N/A — Windows-only COM handling | None | | VAD / never upload silence | 2053b44 | Feeding dead air to Whisper makes it hallucinate fluent text ("Genghis Khan", Bengali). | SDK-internal — handled by Recall SDK's VAD; our `SILENCE_PEAK` guard (library.rs:28, guard.ts:23) prevents filing silent recordings | None | @@ -30,10 +31,23 @@ Regression ledger: main's production recording fixes mapped to our w2 implementa ## Summary -- **Present**: 14 fix classes, 5 corrected in w2/codex-findings -- **N/A**: 6 fix classes (Windows-only or SDK-internal) +- **Present**: 16 fix classes, 5 corrected in w2/codex-findings, 2 in w2/windows-recording +- **N/A**: 4 fix classes (Windows-only features not ported, or SDK-internal) - **Missing**: 0 — all implemented +Both Windows rows that moved out of N/A were measured against the real SDK on +Windows 11 (`tests/real_sidecar_smoke.rs`), and **neither failure reproduced**: +this Node resolves a verbatim path fine, and the native helper self-exits when +Node dies. They are kept as the cheap removal of two variables, not sold as +crash fixes. What DID reproduce, hard, was the build: neither staging step +could run at all (see `scripts/dev/recall/host.mjs`), and the Windows sidecar +shipped whichever Node built it — which is where the verbatim-path crash +actually lived, in the reference app's build machine rather than in its code. + +The three still marked N/A +(default-mic watcher, the watcher race it introduces, ComGuard) all belong to +the device-monitoring thread this wave does not have on either platform. + ## Fixes Implemented ### Mic Permission Gate on Call Detector From b16ea907b9d29d00850804efa94d81cd0e93bcb0 Mon Sep 17 00:00:00 2001 From: Amir SSV Labs Date: Mon, 10 Aug 2026 15:58:40 +0300 Subject: [PATCH 06/16] =?UTF-8?q?docs(recording):=20Windows=20has=20been?= =?UTF-8?q?=20run=20now=20=E2=80=94=20and=20what=20the=20run=20found?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The engine README said "the code paths are there; none of it has been run." That is no longer true: on Windows 11 with a keyed build, record → WAV → upload → Recall → transcript works, rendered with speaker labels. What the run found is worth more than the fact that it passed. The first two captures were 21 seconds of nothing, and the app could not tell: default input, disconnected, +30 dB boost peak -34.9 dB spread 1.4 dB the same device idle, nobody speaking peak -35.9 dB after switching the default input peak -11.5 dB spread 29.5 dB Twelve seconds of loud speech was indistinguishable from that device's idle hiss — measured against ffmpeg on the same machine — because nothing was plugged into it. Recall bound the system default correctly; the default was deaf. Both takes cleared `hasAudio`, uploaded, and billed a transcript job. So the Windows gap is not capture, it is that nothing probes the DEVICE: no default-mic watcher, no probe feeding `micDeviceMismatch`, and a silence gate that a boosted dead input walks straight through. Level cannot catch that. Variance can — 1.4 dB against 29.5 dB — and `window_peak` is already tracked, so it needs no new platform code. Written down where the next person looks rather than left as tribal knowledge. Co-Authored-By: Claude Opus 5 --- scripts/eval/RECORDING-FIX-AUDIT.md | 18 ++++++++++++++++++ src/engines/recording/README.md | 20 ++++++++++++++++++-- 2 files changed, 36 insertions(+), 2 deletions(-) diff --git a/scripts/eval/RECORDING-FIX-AUDIT.md b/scripts/eval/RECORDING-FIX-AUDIT.md index 52453392..5a06a76c 100644 --- a/scripts/eval/RECORDING-FIX-AUDIT.md +++ b/scripts/eval/RECORDING-FIX-AUDIT.md @@ -44,6 +44,24 @@ could run at all (see `scripts/dev/recall/host.mjs`), and the Windows sidecar shipped whichever Node built it — which is where the verbatim-path crash actually lived, in the reference app's build machine rather than in its code. +**The live run (Windows 11, keyed build, 10 Aug).** record → WAV → upload → +Recall → transcript works, rendered with speaker labels. Measured on the way: + +| capture | peak | RMS | envelope spread | +|---|---|---|---| +| default input, disconnected, +30 dB boost | −34.9 dB | −48.1 dB | **1.4 dB** | +| same device idle, nobody speaking (ffmpeg) | −35.9 dB | −48.3 dB | — | +| after switching the default input | **−11.5 dB** | −29.2 dB | **29.5 dB** | + +The first two rows are the same to within 1 dB — twelve seconds of loud speech +was indistinguishable from that device's idle hiss, because nothing was plugged +into it. Recall bound the system default correctly; the default was deaf. Both +of those takes passed `hasAudio`, uploaded, and billed a transcript job. + +So the Windows gap is not capture — it is that NOTHING PROBES THE DEVICE (see +the engine README's "Not ported" list). Level alone cannot catch a boosted dead +input; variance can, and `window_peak` is already tracked. + The three still marked N/A (default-mic watcher, the watcher race it introduces, ComGuard) all belong to the device-monitoring thread this wave does not have on either platform. diff --git a/src/engines/recording/README.md b/src/engines/recording/README.md index 50fd528d..be268a27 100644 --- a/src/engines/recording/README.md +++ b/src/engines/recording/README.md @@ -241,5 +241,21 @@ intentionally NOT ported in this wave: closed-lid microphone while the call runs on another — `client/guard.ts`'s `micDeviceMismatch` is the check, and the probe that feeds it needs real hardware to develop against. -- **Windows.** The code paths are there (`agent-windows.exe`, `.exe` suffixes); - none of it has been run. +- **Windows.** RUN, on Windows 11 with a keyed build (10 Aug): record → WAV → + upload → Recall → transcript, rendered with speaker labels. What that run + found is in `scripts/eval/RECORDING-FIX-AUDIT.md`; the build could not stage + its assets at all before it (`scripts/dev/recall/host.mjs`). + + Still not ported, and all three are the same missing piece — nothing probes + the input device on Windows: + - No default-microphone watcher. The sidecar binds the system default at + `prepare`, so changing it mid-session is ignored until the app restarts. + - `micDeviceMismatch` has no probe feeding it (`call_mic.rs` is macOS-only), + so a capture bound to the wrong device looks identical to a good one. + - A DEAD input passes the silence gate. A disconnected line-in at +30 dB + boost reads about −35 dBFS — comfortably above `SILENCE_PEAK` — and the + first Windows run recorded 21 seconds of it across two takes, marked both + `hasAudio: true`, uploaded both and paid for two transcript jobs. The tell + is that the level never MOVED: 1.4 dB of envelope spread against 29.5 dB + for real speech on the same machine. `recorder.rs` already tracks + `window_peak`, so variance is measurable without any new probe. From 50e9607a9578ea99683726327802c911753fad4d Mon Sep 17 00:00:00 2001 From: Amir SSV Labs Date: Mon, 10 Aug 2026 17:47:12 +0300 Subject: [PATCH 07/16] fix(recording): AudioStatus crosses the wire in snake_case, so nothing reads it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `AudioStatus` was defined in `call_mic.rs` — a platform module — without `#[serde(rename_all = "camelCase")]`, while every sibling wire type in `status.rs` carries it. So the backend answered `window_peak` / `default_device` / `call_app_device` and the client read `windowPeak` / `defaultDevice` / `callAppDevice`: three `undefined`s, on every platform, since the feature was written. `micDeviceMismatch` could never return true, and `micMismatch` was therefore set nowhere and rendered nowhere. This is not a Windows bug; macOS has been carrying it just as long. No test constructed `AudioStatus`, and `recording_audio_status` was missing from the op-table smoke list, which is why nothing anywhere noticed. The type MOVES rather than just gaining the attribute. Leaving it in a platform module fixes today's instance and leaves the trap armed for the next type defined next to a platform query; `status.rs` is where `rename_all` is the default a reader can rely on. `call_mic::audio_status(peak, window_peak)` stays behind as the platform-aware constructor, so the device lookups do not move anywhere. The refresh stays UNARMED, deliberately. Fixing the casing arms `recording_refresh_audio` for the first time in the field, and that call pauses and resumes a live capture — on macOS, which nobody here has hardware to verify a live pause/resume on. The 2-observation detection and the latch stay; the invoke is replaced by a pill title that NAMES both disagreeing devices, which is the part a user could act on anyway. Arm it after one verified macOS run. Tests: a literal specimen asserting the three keys the client actually reads, plus a sweep over one specimen of every type that crosses the IPC boundary asserting no key survives in snake_case. The sweep is the structural guard — a new wire type that forgets `rename_all` now fails at the seam instead of silently disabling whichever client branch reads it. --- src-tauri/src/ops.rs | 4 + src/engines/recording/client/pill.test.ts | 45 +++++ src/engines/recording/client/pill.ts | 35 +++- src/engines/recording/src/call_mic.rs | 30 ++- src/engines/recording/src/lib.rs | 3 +- src/engines/recording/src/recorder.rs | 2 +- src/engines/recording/src/status.rs | 187 ++++++++++++++++++ .../core/runtime/stores/recording.svelte.ts | 33 +++- 8 files changed, 309 insertions(+), 30 deletions(-) diff --git a/src-tauri/src/ops.rs b/src-tauri/src/ops.rs index 7e8166ce..5a9b4ac2 100644 --- a/src-tauri/src/ops.rs +++ b/src-tauri/src/ops.rs @@ -70,6 +70,10 @@ mod tests { "context_list", "approvals_list", "recording_status", + // Registered but unlisted, and the client's only device-health + // read: the whole mic-mismatch path could go dark without any + // suite here noticing. It did. + "recording_audio_status", "run_respond_permission", "drive_result", ] { diff --git a/src/engines/recording/client/pill.test.ts b/src/engines/recording/client/pill.test.ts index c2568f41..0f3578cf 100644 --- a/src/engines/recording/client/pill.test.ts +++ b/src/engines/recording/client/pill.test.ts @@ -75,6 +75,51 @@ describe("pillView", () => { expect(view.title).toContain("Recording continues"); }); + it("names BOTH devices when the default input and the call app disagree", () => { + // The backend answered this in snake_case until the AudioStatus move, so + // `micDeviceMismatch` never fired and this sentence had no way to appear. + const view = pillView({ + availability: READY, + recording: true, + elapsedSec: 120, + micMismatch: true, + micDefaultDevice: "MacBook Pro Microphone", + micCallAppDevice: "Studio Display Microphone", + }); + expect(view.title).toContain("MacBook Pro Microphone"); + expect(view.title).toContain("Studio Display Microphone"); + // Audio IS being captured, just from the wrong place — the warn treatment + // belongs to silence, and claiming it here would cry wolf. + expect(view.warn).toBe(false); + }); + + it("still explains the mismatch when the platform cannot name the devices", () => { + // Windows has no call-app device source at all, and a name can fail to + // resolve anywhere. A half-filled sentence would read as a bug. + const view = pillView({ + availability: READY, + recording: true, + micMismatch: true, + micDefaultDevice: null, + micCallAppDevice: null, + }); + expect(view.title).toContain("system default microphone"); + expect(view.title).not.toContain('""'); + }); + + it("lets silence outrank a device mismatch — nothing captured beats wrongly captured", () => { + const view = pillView({ + availability: READY, + recording: true, + noAudio: true, + micMismatch: true, + micDefaultDevice: "a", + micCallAppDevice: "b", + }); + expect(view.title).toContain("No audio is reaching brains"); + expect(view.warn).toBe(true); + }); + it("disables the pill while a stop is in flight without losing the clock", () => { const view = pillView({ availability: READY, diff --git a/src/engines/recording/client/pill.ts b/src/engines/recording/client/pill.ts index db58c1cc..105f84df 100644 --- a/src/engines/recording/client/pill.ts +++ b/src/engines/recording/client/pill.ts @@ -57,6 +57,26 @@ export function elapsed(seconds: number): string { return hours > 0 ? `${hours}:${pad(minutes)}:${pad(secs)}` : `${minutes}:${pad(secs)}`; } +/** + * The mismatch sentence, with the device names in it when they are known. + * + * brains records the SYSTEM DEFAULT input; the call app may be on a different + * one. That is not silence — the far end hears you perfectly — so it gets a + * title rather than the warn treatment, and it has to name both devices or a + * person cannot act on it. Switching the default is the repair; brains does + * not do it for you (see `recording.svelte.ts`). + */ +export function micMismatchTitle( + defaultDevice?: string | null, + callAppDevice?: string | null, +): string { + const named = + defaultDevice && callAppDevice + ? ` brains is recording "${defaultDevice}"; the call is using "${callAppDevice}".` + : ""; + return `brains records your system default microphone, and the call app is using a different one.${named} Switch your default input to match. Recording continues.`; +} + /** * The whole pill, from the poll plus the two optimistic flags. * @@ -81,6 +101,12 @@ export function pillView(args: { callPrompt?: boolean; /** The detected call application's name. */ callAppName?: string; + /** The system default input and the call app's input disagree. */ + micMismatch?: boolean; + /** The system default input's name, when it is known. */ + micDefaultDevice?: string | null; + /** The call app's input's name, when it is known. */ + micCallAppDevice?: string | null; }): PillView { const { availability, @@ -92,6 +118,9 @@ export function pillView(args: { error, callPrompt, callAppName, + micMismatch, + micDefaultDevice, + micCallAppDevice, } = args; // Unknown availability (status not loaded yet): disable until we know. @@ -125,9 +154,13 @@ export function pillView(args: { return { state: "recording", label: elapsed(elapsedSec), + // Silence outranks a mismatch: nothing is being captured beats + // something is being captured from the wrong place. title: noAudio ? "No audio is reaching brains — check the microphone permission or input device. Recording continues." - : "Stop recording", + : micMismatch + ? micMismatchTitle(micDefaultDevice, micCallAppDevice) + : "Stop recording", disabled: !!stopping, warn: !!noAudio, }; diff --git a/src/engines/recording/src/call_mic.rs b/src/engines/recording/src/call_mic.rs index 85ee65f4..b40fcefe 100644 --- a/src/engines/recording/src/call_mic.rs +++ b/src/engines/recording/src/call_mic.rs @@ -400,22 +400,18 @@ pub fn restore_input_device_after_crash(data_root: &std::path::Path) { } } -/// Audio status for the UI: peaks plus device comparison. -#[derive(Debug, Clone, serde::Serialize)] -pub struct AudioStatus { - pub peak: i32, - pub window_peak: i32, - pub default_device: Option, - pub call_app_device: Option, -} - -impl AudioStatus { - pub fn new(peak: i32, window_peak: i32) -> Self { - Self { - peak, - window_peak, - default_device: default_input_device_name(), - call_app_device: call_app_input_device_name(), - } +/// Answer the UI's device question: the peaks it hands in, plus the two +/// names only this module knows how to ask the platform for. +/// +/// The returned type lives in `status.rs` with every other wire type, because +/// that is where `rename_all = "camelCase"` is the house default. Defined +/// here, it was serialized in snake_case and the client's mismatch check read +/// three `undefined`s for as long as the feature has existed. +pub fn audio_status(peak: i32, window_peak: i32) -> crate::status::AudioStatus { + crate::status::AudioStatus { + peak, + window_peak, + default_device: default_input_device_name(), + call_app_device: call_app_input_device_name(), } } diff --git a/src/engines/recording/src/lib.rs b/src/engines/recording/src/lib.rs index 5b91d265..d7ef8ba3 100644 --- a/src/engines/recording/src/lib.rs +++ b/src/engines/recording/src/lib.rs @@ -35,11 +35,10 @@ use std::path::{Path, PathBuf}; use std::sync::Arc; pub use call_detector::{CallDetector, CallEvent, CallProcess, CallSnooze, Config as DetectorConfig, DetectorState, MicAuthStatus}; -pub use call_mic::AudioStatus; pub use library::{BrainsSync, ListOptions, ListResult, Recording, RecordingMeeting}; pub use recorder::Recorder; pub use sidecar::{Sidecar, Spec}; -pub use status::{Availability, Started, Status, Stopped}; +pub use status::{AudioStatus, Availability, Started, Status, Stopped}; pub use transcript::Transcript; #[derive(Debug, thiserror::Error)] diff --git a/src/engines/recording/src/recorder.rs b/src/engines/recording/src/recorder.rs index b6461333..b08ba3e8 100644 --- a/src/engines/recording/src/recorder.rs +++ b/src/engines/recording/src/recorder.rs @@ -268,7 +268,7 @@ impl Recorder { /// Live capture health for the UI: peaks plus device comparison. pub fn audio_status(&self) -> AudioStatus { let (peak, window_peak) = self.peaks(); - AudioStatus::new(peak, window_peak) + call_mic::audio_status(peak, window_peak) } /// Rebuild the live audio path after the call app switches microphones. diff --git a/src/engines/recording/src/status.rs b/src/engines/recording/src/status.rs index 2984e770..cf9156a3 100644 --- a/src/engines/recording/src/status.rs +++ b/src/engines/recording/src/status.rs @@ -105,6 +105,26 @@ pub struct Stopped { pub duration_sec: f64, } +/// Live capture health for the UI: the peaks, plus the two device names whose +/// disagreement means the call app and Recall are listening to different +/// microphones. +/// +/// It is constructed by [`crate::call_mic::audio_status`] — that is where the +/// platform queries live — but the TYPE is here, beside every other thing +/// that crosses the wire. It was defined in `call_mic.rs` without +/// `rename_all`, so it shipped in snake_case while the client read +/// `windowPeak` / `defaultDevice` / `callAppDevice`; mismatch detection has +/// been dead since it was written, on macOS too. A platform module is exactly +/// where that rule gets forgotten, so the rule lives where it is the default. +#[derive(Debug, Clone, Serialize)] +#[serde(rename_all = "camelCase")] +pub struct AudioStatus { + pub peak: i32, + pub window_peak: i32, + pub default_device: Option, + pub call_app_device: Option, +} + #[cfg(test)] mod tests { use super::*; @@ -122,4 +142,171 @@ mod tests { assert!(json.contains(r#""hint":"rebuild"#), "{json}"); assert!(Availability::ready("ok").hint.is_empty()); } + + /// The three keys the client actually reads. `recording.svelte.ts` compares + /// `defaultDevice` to `callAppDevice` and judges the window on `windowPeak`; + /// against a snake_case payload all three were `undefined`, so the mismatch + /// branch could never be taken and nothing anywhere reported a problem. + #[test] + fn audio_status_reaches_the_client_under_the_names_it_reads() { + let json = serde_json::to_string(&AudioStatus { + peak: 900, + window_peak: 7, + default_device: Some("MacBook Pro Microphone".into()), + call_app_device: Some("Studio Display Microphone".into()), + }) + .unwrap(); + assert!(json.contains(r#""windowPeak":7"#), "{json}"); + assert!( + json.contains(r#""defaultDevice":"MacBook Pro Microphone""#), + "{json}" + ); + assert!( + json.contains(r#""callAppDevice":"Studio Display Microphone""#), + "{json}" + ); + } + + /// The structural guard, and the reason this type moved rather than just + /// gaining an attribute: one specimen of every type that crosses the IPC + /// boundary, asserting no key survives in snake_case. A new wire type that + /// forgets `rename_all` fails HERE, at the seam, instead of silently + /// disabling whichever client branch reads it. + #[test] + fn no_wire_type_reaches_the_client_in_snake_case() { + fn snake_keys(value: &serde_json::Value, found: &mut Vec) { + match value { + serde_json::Value::Object(fields) => { + for (key, child) in fields { + if key.contains('_') { + found.push(key.clone()); + } + snake_keys(child, found); + } + } + serde_json::Value::Array(items) => { + for item in items { + snake_keys(item, found); + } + } + _ => {} + } + } + + let recording = crate::library::Recording { + path: "/r/call-1-mic.wav".into(), + filename: "call-1-mic.wav".into(), + created_ms: 1, + duration_sec: 1.0, + size_bytes: 32000, + has_audio: true, + source_app: Some("zoom".into()), + }; + let specimens: Vec<(&str, serde_json::Value)> = vec![ + ( + "Availability", + serde_json::to_value(Availability::ready("ok")).unwrap(), + ), + ( + "Status", + serde_json::to_value(Status { + availability: Availability::ready("ok"), + recording: true, + peak: 900, + window_peak: 7, + path: Some("/r/call-1-mic.wav".into()), + recordings_dir: "/r".into(), + error: Some("boom".into()), + }) + .unwrap(), + ), + ( + "Started", + serde_json::to_value(Started { + path: "/r/call-1-mic.wav".into(), + capture_mode: Some("meeting".into()), + provider: Some("zoom".into()), + }) + .unwrap(), + ), + ( + "Stopped", + serde_json::to_value(Stopped { + path: "/r/call-1-mic.wav".into(), + peak: 900, + duration_sec: 12.0, + }) + .unwrap(), + ), + ( + "AudioStatus", + serde_json::to_value(AudioStatus { + peak: 900, + window_peak: 7, + default_device: Some("built-in".into()), + call_app_device: Some("display".into()), + }) + .unwrap(), + ), + ("Recording", serde_json::to_value(&recording).unwrap()), + ( + "IndexEntry", + serde_json::to_value(crate::library::IndexEntry { + duration_sec: 12.0, + has_audio: true, + created_ms: 1, + source_app: Some("zoom".into()), + }) + .unwrap(), + ), + ( + "ListResult", + serde_json::to_value(crate::library::ListResult { + recordings: vec![recording.clone()], + total: 1, + }) + .unwrap(), + ), + ( + "BrainsSync", + serde_json::to_value(crate::library::BrainsSync { + slug: "recordings/call-1".into(), + url: Some("https://example.test/x".into()), + filed_at: "2026-08-10T12:00:00Z".into(), + meeting: Some(crate::library::RecordingMeeting { + title: "Standup".into(), + attendees: vec!["a@example.test".into()], + slug: Some("calendar/standup".into()), + when_local: Some("10:30".into()), + platform: Some("google_meet".into()), + overlap_ms: 600_000, + coupled: true, + }), + }) + .unwrap(), + ), + ( + "Transcript", + serde_json::to_value(crate::transcript::Transcript { + state: crate::transcript::state::READY, + text: Some("hello".into()), + detail: "ready".into(), + recording_id: Some("rec_1".into()), + transcript_id: Some("tr_1".into()), + path: Some("/r/call-1-mic.transcript.txt".into()), + }) + .unwrap(), + ), + ]; + + for (name, value) in &specimens { + let mut found = Vec::new(); + snake_keys(value, &mut found); + assert!( + found.is_empty(), + "{name} crosses the wire with snake_case keys {found:?} — \ + add #[serde(rename_all = \"camelCase\")]" + ); + } + } } diff --git a/src/layout/core/runtime/stores/recording.svelte.ts b/src/layout/core/runtime/stores/recording.svelte.ts index c4f44e22..e7b13a2b 100644 --- a/src/layout/core/runtime/stores/recording.svelte.ts +++ b/src/layout/core/runtime/stores/recording.svelte.ts @@ -131,6 +131,10 @@ export class RecordingStore { done = $state(false); /** The default mic differs from the call app's mic (hot-swap warning). */ micMismatch = $state(false); + /** The two device names behind `micMismatch`, so the pill can NAME them. + * A warning that says "your microphones disagree" without saying which + * ones is the sentence a user cannot act on. */ + micDevices = $state<{ default: string | null; callApp: string | null } | null>(null); #startedAt = 0; /** Last observed audio status for 2-observation mismatch logic. */ @@ -170,6 +174,9 @@ export class RecordingStore { error: this.error, callPrompt: this.callPrompt, callAppName: this.callAppName ?? undefined, + micMismatch: this.micMismatch, + micDefaultDevice: this.micDevices?.default ?? null, + micCallAppDevice: this.micDevices?.callApp ?? null, }); } @@ -865,17 +872,24 @@ export class RecordingStore { } else { this.#mismatchCount = 1; } - // Trigger refresh after 2 consecutive observations + // Two consecutive observations, then warn — and ONLY warn. + // + // `recording_refresh_audio` pauses and resumes a live capture + // (src/engines/recall/sidecar/index.cjs). Until this commit the + // backend answered in snake_case, so `defaultDevice` and + // `callAppDevice` were both `undefined`, `micDeviceMismatch` was + // never true, and that call has therefore never run in the field on + // any platform. Fixing the casing arms it for the first time — on + // macOS, where nobody here has a machine to verify a live + // pause/resume on. So this stops at the sentence and the repair is + // the user's: telling them WHICH two devices disagree is the part + // that was missing anyway. Arm it after one verified macOS run. if (this.#mismatchCount === 2 && !this.micMismatch) { this.micMismatch = true; - try { - await getTransport().invoke("recording_refresh_audio", { - from: status.defaultDevice ?? "", - to: status.callAppDevice ?? "", - }); - } catch { - // Refresh failed — warning is already shown - } + this.micDevices = { + default: status.defaultDevice ?? null, + callApp: status.callAppDevice ?? null, + }; } } else { this.#mismatchCount = 0; @@ -893,6 +907,7 @@ export class RecordingStore { this.elapsedSec = 0; this.noAudio = false; this.micMismatch = false; + this.micDevices = null; this.recordingAppName = null; this.#lastAudioStatus = null; this.#mismatchCount = 0; From a4d033e60cd885d0bcee106def3ac34dcc4efbb7 Mon Sep 17 00:00:00 2001 From: Amir SSV Labs Date: Mon, 10 Aug 2026 17:51:34 +0300 Subject: [PATCH 08/16] =?UTF-8?q?fix(recording):=20two=20readers,=20one=20?= =?UTF-8?q?window=20peak=20=E2=80=94=20whichever=20polls=20second=20sees?= =?UTF-8?q?=20zero?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The window peak is CONSUMED by reading it: the sidecar returns `windowPeak` and zeroes it in the same breath (`sidecar/index.cjs`), which is what makes it the only value that can tell a dead microphone from a loud moment that already happened. Two callers were reading it. `recording_status` and `recording_audio_status` both fire on the same one-second tick, and `audio_status()` issued a second consuming `peak` RPC of its own — so whichever landed second read ~0. A perfectly loud call reported silence on some ticks, and which reader lost was a race. `audio_status()` becomes the echo: it reports the values `peaks()` last stored and issues nothing. `peaks()` is now the single reader, which is also what makes `last_window_peak` a live field — it was written and never read. On the client, `#checkAudioStatus()` moves into the `.then()` chain after `refreshStatus()`. With the echo in place, firing them concurrently would hand a tick the PREVIOUS tick's window; ordering makes it guaranteed rather than merely likely. The fake sidecar now models read-and-reset — its `peak` arm returns the window once and 0 afterwards — and the existing capture-lifecycle test asserts both halves: the second reader on a tick still sees 7, and no extra `peak` RPC was issued. No test is added, so the suite's pinned "4 passed" holds. `record-pill.test.ts` gains the `recording_audio_status` stub it never had. Unstubbed, it threw on every tick and the store's empty catch swallowed it — so this path was untested from both ends at once: the backend shape was wrong (previous commit) and the mounted harness never exercised the reader. Flagged: `fake_sidecar.rs` is `#![cfg(unix)]`, so this fix has no Rust coverage on the platform the bug was found on. The mounted test covers the wiring everywhere. --- src/engines/recording/src/recorder.rs | 18 ++++++- src/engines/recording/tests/fake_sidecar.rs | 47 ++++++++++++++++++- .../core/frame/__tests__/record-pill.test.ts | 10 ++++ .../core/runtime/stores/recording.svelte.ts | 9 ++-- 4 files changed, 78 insertions(+), 6 deletions(-) diff --git a/src/engines/recording/src/recorder.rs b/src/engines/recording/src/recorder.rs index b08ba3e8..4fbb5ac1 100644 --- a/src/engines/recording/src/recorder.rs +++ b/src/engines/recording/src/recorder.rs @@ -266,9 +266,23 @@ impl Recorder { } /// Live capture health for the UI: peaks plus device comparison. + /// + /// This is the ECHO, never the reader. [`peaks`](Self::peaks) is the one + /// place that issues the `peak` RPC, because that RPC CONSUMES the window: + /// the sidecar hands back `windowPeak` and zeroes it in the same breath + /// (`sidecar/index.cjs`, "read and reset"). Both `recording_status` and + /// `recording_audio_status` fire on the same one-second tick, so when this + /// issued its own call, whichever landed second read a window of ~0 — + /// half the ticks reporting silence during a perfectly loud call, and + /// which half decided by a race. + /// + /// So it reads what `peaks()` last stored. That is also what makes + /// `last_window_peak` a live field: it was written and never read. pub fn audio_status(&self) -> AudioStatus { - let (peak, window_peak) = self.peaks(); - call_mic::audio_status(peak, window_peak) + call_mic::audio_status( + self.last_peak.load(Ordering::Relaxed), + self.last_window_peak.load(Ordering::Relaxed), + ) } /// Rebuild the live audio path after the call app switches microphones. diff --git a/src/engines/recording/tests/fake_sidecar.rs b/src/engines/recording/tests/fake_sidecar.rs index 100bc389..31f9feae 100644 --- a/src/engines/recording/tests/fake_sidecar.rs +++ b/src/engines/recording/tests/fake_sidecar.rs @@ -49,7 +49,17 @@ while IFS= read -r line; do printf '{"type":"response","id":%s,"ok":true,"result":{"path":"%s","windowId":"w-1","captureMode":"meeting","provider":"google_meet"}}\n' "$id" "$wav" ;; peak) - printf '{"type":"response","id":%s,"ok":true,"result":{"peak":2400,"windowPeak":7,"dataBytes":32000}}\n' "$id" + # READ AND RESET, like the real one. The sidecar returns windowPeak and + # zeroes it in the same breath (sidecar/index.cjs), so the SECOND reader + # on a tick sees 0 no matter how loud the call is. Modelling that here is + # what makes "two readers, one window" a failing test rather than a race + # nobody can reproduce. + n=0 + [ -f "$FAKE_PEAK_CALLS" ] && n=$(cat "$FAKE_PEAK_CALLS") + n=$((n + 1)) + echo "$n" > "$FAKE_PEAK_CALLS" + if [ "$n" -eq 1 ]; then window=7; else window=0; fi + printf '{"type":"response","id":%s,"ok":true,"result":{"peak":2400,"windowPeak":%s,"dataBytes":32000}}\n' "$id" "$window" ;; stop) wav=$(cat "$FAKE_STARTED_WAV") @@ -66,6 +76,19 @@ struct Fake { _dir: tempfile::TempDir, spec: Spec, helper_pid_file: PathBuf, + peak_calls_file: PathBuf, +} + +impl Fake { + /// How many `peak` RPCs the sidecar has been asked for. The point of + /// counting: the window peak is consumed by the read, so an extra caller + /// is not merely wasteful, it is destructive. + fn peak_calls(&self) -> u32 { + std::fs::read_to_string(&self.peak_calls_file) + .ok() + .and_then(|text| text.trim().parse().ok()) + .unwrap_or(0) + } } fn fake_sidecar() -> Fake { @@ -73,14 +96,17 @@ fn fake_sidecar() -> Fake { let script = dir.path().join("fake-sidecar.sh"); std::fs::write(&script, FAKE).unwrap(); let helper_pid_file = dir.path().join("helper.pid"); + let peak_calls_file = dir.path().join("peak-calls.txt"); let spec = Spec::new("/bin/sh") .arg(&script) .env("FAKE_HELPER_PID", &helper_pid_file) + .env("FAKE_PEAK_CALLS", &peak_calls_file) .env("FAKE_STARTED_WAV", dir.path().join("started-wav.txt")); Fake { _dir: dir, spec, helper_pid_file, + peak_calls_file, } } @@ -187,6 +213,25 @@ fn a_capture_writes_its_metadata_and_reports_what_landed() { assert_eq!(status.peak, 2400); assert_eq!(status.window_peak, 7, "the window peak is read per poll"); + // TWO READERS, ONE WINDOW. `recording_status` and `recording_audio_status` + // both fire on the same one-second tick. While `audio_status` issued its + // own `peak` RPC, the second one to land consumed an already-emptied + // window and reported 0 — so a loud call intermittently looked silent, + // and which reader lost was a race. `audio_status` is now the echo: it + // reports what the poll last stored and issues nothing. + let rpcs_after_status = fake.peak_calls(); + let audio = recorder.audio_status(); + assert_eq!( + audio.window_peak, 7, + "the second reader on a tick must not see the emptied window" + ); + assert_eq!(audio.peak, 2400); + assert_eq!( + fake.peak_calls(), + rpcs_after_status, + "audio_status issued a peak RPC of its own — that consumes the window" + ); + // What the sidecar chose is persisted for a later transcript fetch, and // the recognised call app is remembered for the library. let wav = PathBuf::from(&started.path); diff --git a/src/layout/core/frame/__tests__/record-pill.test.ts b/src/layout/core/frame/__tests__/record-pill.test.ts index b5a284d3..e85007f5 100644 --- a/src/layout/core/frame/__tests__/record-pill.test.ts +++ b/src/layout/core/frame/__tests__/record-pill.test.ts @@ -61,6 +61,16 @@ beforeEach(() => { status = idle(); transport = new FakeTransport() .on("recording_status", () => structuredClone(status)) + // The capture beat polls this every tick alongside `recording_status`. + // Unstubbed it threw on every tick and the store's empty catch swallowed + // it, so this whole path was untested from BOTH ends at once: the backend + // shape was wrong and the mounted harness never exercised the reader. + .on("recording_audio_status", () => ({ + peak: status.peak, + windowPeak: status.windowPeak, + defaultDevice: null, + callAppDevice: null, + })) .on("recordings_list", () => []) .on("recording_prepare", true) .on("recording_start", () => { diff --git a/src/layout/core/runtime/stores/recording.svelte.ts b/src/layout/core/runtime/stores/recording.svelte.ts index e7b13a2b..0241bcac 100644 --- a/src/layout/core/runtime/stores/recording.svelte.ts +++ b/src/layout/core/runtime/stores/recording.svelte.ts @@ -847,9 +847,12 @@ export class RecordingStore { ) { this.noAudio = true; } + // AFTER the status read, never beside it. `recording_audio_status` + // now echoes what `recording_status` last fetched, so firing them + // concurrently would hand this tick the PREVIOUS tick's window peak. + // Ordering it here makes that guaranteed rather than merely likely. + void this.#checkAudioStatus(); }); - // Also poll audio status for mic device mismatch detection - void this.#checkAudioStatus(); }, CAPTURE_POLL_MS); } @@ -875,7 +878,7 @@ export class RecordingStore { // Two consecutive observations, then warn — and ONLY warn. // // `recording_refresh_audio` pauses and resumes a live capture - // (src/engines/recall/sidecar/index.cjs). Until this commit the + // (src/engines/recording/sidecar/index.cjs). Until this commit the // backend answered in snake_case, so `defaultDevice` and // `callAppDevice` were both `undefined`, `micDeviceMismatch` was // never true, and that call has therefore never run in the field on From 3ad819dbb2e4bfef55feea9266642e41f7eef025 Mon Sep 17 00:00:00 2001 From: Amir SSV Labs Date: Mon, 10 Aug 2026 18:07:53 +0300 Subject: [PATCH 09/16] feat(recording): a dead input is one whose level never moves MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `SILENCE_PEAK` catches an input feeding zeros. It cannot catch a disconnected one with gain on it, and that is what cost a call. A disconnected USB line-in at +30 dB boost, recorded over twelve seconds of loud speech: default input, disconnected, +30 dB peak -34.9 dB RMS -48.1 dB spread 1.4 dB same device idle, nobody speaking peak -35.9 dB RMS -48.3 dB — after switching the default input peak -11.5 dB RMS -29.2 dB spread 29.5 dB The first two rows agree to within 1 dB: speech was indistinguishable from that device's own hiss BY LEVEL. The app recorded 21 seconds across two takes, marked both as having audio, uploaded both, billed two transcript jobs that came back empty, and told the user nothing. Level cannot see this. Variance can — speech moves, hiss does not. Measured in two places, split by job, and deliberately NOT in the sidecar (no protocol change, no policy in JS, and its accumulators die with a discarded sidecar): * FINALIZE, in `library.rs` — the durable verdict that gates transcription and filing. It reads the finished file, so it depends on neither the UI's poll nor the sidecar's liveness, and it is the same path the index self-heal already uses, so a fresh capture and a legacy one now agree. They did not before: stop took `has_audio` from the sidecar's CUMULATIVE peak while self-heal probed the file. * LIVE, in `guard.ts` + the store — the ~60 s warning, off the window peaks the store already receives once a second. Judged on the ring, never on `status.peak`: a cumulative max never falls, so one blip at capture start would suppress the warning for the rest of the call — the same trap `guard.ts` already documents for the level warning. The verdict is conjunctive with a level band, never variance alone: flat_input ⟺ windows ≥ 40 ∧ floor > 0 ∧ SILENCE_PEAK < ceiling < FLAT_CEILING_PEAK ∧ spread_db(floor, ceiling) < 6.0 6 dB is 4.3× the measured dead spread and about a fifth of the measured speech spread; there is nothing observed between 1.4 and 29.5 to be careful about. The lower bound hands digital silence back to the gate that already owns it. The upper bound is the loud-steady-tone exemption and is THE ONE CONSTANT WITH NO MEASURED NEGATIVE CASE BEHIND IT — capture hold music at the next bring-up. `probe()` becomes a projection of a new `assess()`; 40 × 8 KiB windows replace 5 × 64 KiB at the same 320 KiB. That also fixes a real bug: five fixed points at 10/30/50/70/90% meant a long call whose speech fell between them read as SILENT. The early exit goes — a spread needs every window. `has_audio` keeps its meaning and `flat_input` is a sibling. A boosted dead input is not silent, and a SILENT badge would send the user hunting for a muted microphone instead of a disconnected one, so the badge is NO INPUT and the pill's warning is "⚠ No input". `IndexEntry.flat_input` carries `#[serde(default)]`, which is load-bearing: `read_index` swallows a parse failure into an EMPTY MAP, so without it every existing user's library would silently vanish rather than error. Both billing doors close: the post-stop auto-fetch and `canFetchTranscript`. Filing takes an explicit `flatInput` argument rather than another overload of a peak that already means three things. --- src-tauri/src/commands/recording.rs | 10 + src/engines/recording/client/guard.test.ts | 60 +++ src/engines/recording/client/guard.ts | 70 ++++ src/engines/recording/client/index.ts | 7 +- src/engines/recording/client/overlay.ts | 9 +- src/engines/recording/client/pill.ts | 28 +- .../recording/client/transcript.test.ts | 22 +- src/engines/recording/client/transcript.ts | 16 +- src/engines/recording/client/types.ts | 8 + src/engines/recording/src/library.rs | 370 +++++++++++++++--- src/engines/recording/src/recorder.rs | 17 +- src/engines/recording/src/status.rs | 7 + src/layout/core/frame/Titlebar.svelte | 2 +- .../runtime/stores/recording-filing.svelte.ts | 12 +- .../core/runtime/stores/recording.svelte.ts | 35 +- .../__tests__/recording-library.test.ts | 2 + 16 files changed, 609 insertions(+), 66 deletions(-) diff --git a/src-tauri/src/commands/recording.rs b/src-tauri/src/commands/recording.rs index 02039077..8945435f 100644 --- a/src-tauri/src/commands/recording.rs +++ b/src-tauri/src/commands/recording.rs @@ -36,6 +36,10 @@ pub struct StopResult { pub path: Option, pub peak: i32, pub duration_sec: f64, + /// The capture was above the silence floor but its level never moved. + /// The client gates the transcript fetch on it — a dead input bills a + /// job that comes back empty. + pub flat_input: bool, pub detail: String, } @@ -144,6 +148,7 @@ pub async fn recording_stop(recorder: State<'_, RecordingState>) -> CmdResult) -> CmdResult Err(CmdError::new(error)), @@ -433,9 +439,13 @@ mod tests { path: Some("/x.wav".into()), peak: 2400, duration_sec: 1.5, + flat_input: true, detail: String::new(), }) .unwrap(); assert!(json.contains(r#""durationSec":1.5"#), "{json}"); + // The client gates the transcript fetch on this, so it has to arrive + // under the name the client reads. + assert!(json.contains(r#""flatInput":true"#), "{json}"); } } diff --git a/src/engines/recording/client/guard.test.ts b/src/engines/recording/client/guard.test.ts index 49bc690d..e7ffb899 100644 --- a/src/engines/recording/client/guard.test.ts +++ b/src/engines/recording/client/guard.test.ts @@ -1,10 +1,15 @@ import { describe, expect, it } from "vitest"; import { + FLAT_CEILING_PEAK, + FLAT_LIVE_WINDOWS, + FLAT_SPREAD_DB, NO_AUDIO_WARN_AFTER_MS, SILENCE_PEAK, + envelopeSpreadDb, isCaptureSilent, micDeviceMismatch, shouldFileRecording, + shouldWarnFlatInput, shouldWarnNoAudio, } from "./guard"; @@ -81,3 +86,58 @@ describe("filing gates", () => { expect(shouldFileRecording({ peak: -1, transcriptTrimmedLen: 0 })).toBe(false); }); }); + +describe("shouldWarnFlatInput", () => { + /** A ring whose window peaks alternate between `floor` and `ceiling`. */ + const ring = (floor: number, ceiling: number, length = FLAT_LIVE_WINDOWS) => + Array.from({ length }, (_, i) => (i % 2 === 0 ? floor : ceiling)); + + it("measures the spread between the quietest and loudest window", () => { + // The disconnected +30 dB line-in: 1.4 dB across twelve seconds of speech. + expect(envelopeSpreadDb([200, 235])).toBeCloseTo(1.4, 1); + // Digital silence has no ratio to take — it belongs to shouldWarnNoAudio. + expect(envelopeSpreadDb([0, 0])).toBe(Infinity); + expect(envelopeSpreadDb([])).toBe(Infinity); + }); + + it("warns on an input that is above the floor and never moves", () => { + expect(shouldWarnFlatInput({ windows: ring(200, 235), alreadyWarned: false })).toBe(true); + }); + + it("never warns on a voice — speech moves far more than the threshold", () => { + const speech = ring(SILENCE_PEAK + 2, 3000); + expect(envelopeSpreadDb(speech)).toBeGreaterThan(FLAT_SPREAD_DB); + expect(shouldWarnFlatInput({ windows: speech, alreadyWarned: false })).toBe(false); + }); + + it("leaves silence to the level warning and a loud steady tone alone", () => { + // Below the floor: shouldWarnNoAudio's case, and its wording is the one + // that names the right repair. + expect(shouldWarnFlatInput({ windows: ring(0, 0), alreadyWarned: false })).toBe(false); + // Hold music: loud and unvarying, and doing nothing wrong. + const tone = ring(FLAT_CEILING_PEAK + 100, FLAT_CEILING_PEAK + 120); + expect(shouldWarnFlatInput({ windows: tone, alreadyWarned: false })).toBe(false); + }); + + it("refuses to answer before it has a minute of evidence", () => { + const brief = ring(200, 235, FLAT_LIVE_WINDOWS - 1); + expect(shouldWarnFlatInput({ windows: brief, alreadyWarned: false })).toBe(false); + }); + + it("latches: a warning already shown is not re-raised", () => { + expect(shouldWarnFlatInput({ windows: ring(200, 235), alreadyWarned: true })).toBe(false); + }); +}); + +describe("filing a dead input", () => { + it("refuses to file it, however many words the recogniser produced", () => { + // The peak alone cannot express this failure — it is well above the + // floor, which is why the flag has to be its own argument. + expect( + shouldFileRecording({ peak: 900, flatInput: true, transcriptTrimmedLen: 4000 }), + ).toBe(false); + expect( + shouldFileRecording({ peak: 900, flatInput: false, transcriptTrimmedLen: 4000 }), + ).toBe(true); + }); +}); diff --git a/src/engines/recording/client/guard.ts b/src/engines/recording/client/guard.ts index 721ab895..ea9b4ac5 100644 --- a/src/engines/recording/client/guard.ts +++ b/src/engines/recording/client/guard.ts @@ -57,6 +57,72 @@ export function shouldWarnNoAudio(args: { return observed < SILENCE_PEAK; } +// ── A DEAD INPUT IS ONE WHOSE LEVEL NEVER MOVES ─────────────────────────── +// +// SILENCE_PEAK catches an input feeding zeros. It cannot catch a disconnected +// one with gain on it: a disconnected USB line-in at +30 dB boost measured +// peak −34.9 dB with an envelope spread of 1.4 dB across twelve seconds of +// loud speech, while the same device idle measured −35.9 dB. Level cannot +// separate those. Variance can — speech moves, hiss does not. +// +// This is the LIVE half, off the window peaks the store already receives once +// a second. The durable verdict is taken from the finished file in +// `src/engines/recording/src/library.rs` — mirror the three constants below; +// change both or neither. + +/** How little the envelope may move before the input is called dead. */ +export const FLAT_SPREAD_DB = 6.0; + +/** Above −20 dBFS a steady signal is hold music or a tone, not a dead input. */ +export const FLAT_CEILING_PEAK = 3277; + +/** One-second windows to see before answering — a minute of evidence. */ +export const FLAT_LIVE_WINDOWS = 60; + +/** + * How far the loudest window sits above the quietest, in dB. + * + * Returns `Infinity` for an empty ring or a floor of zero: digital silence + * belongs to `shouldWarnNoAudio`, and an infinite spread is never < 6. + */ +export function envelopeSpreadDb(windows: readonly number[]): number { + if (windows.length === 0) return Infinity; + let floor = Infinity; + let ceiling = 0; + for (const value of windows) { + if (value < 0) continue; // "unavailable" from an older backend + if (value < floor) floor = value; + if (value > ceiling) ceiling = value; + } + if (!Number.isFinite(floor) || floor <= 0 || ceiling <= 0) return Infinity; + return 20 * Math.log10(ceiling / floor); +} + +/** + * WHILE recording: is the input live but deaf? + * + * Judged on the RING of window peaks, never on `status.peak`. A cumulative + * max never falls, so one blip at capture start would pin the spread wide for + * the rest of the call and suppress this warning forever — the same trap + * `shouldWarnNoAudio` documents for the level warning. + * + * Conjunctive with a level band, never variance alone: below the silence + * floor is `shouldWarnNoAudio`'s case, and above the ceiling is a loud steady + * signal that is doing nothing wrong. + */ +export function shouldWarnFlatInput(args: { + /** Window peaks, newest last. */ + windows: readonly number[]; + alreadyWarned: boolean; +}): boolean { + const { windows, alreadyWarned } = args; + if (alreadyWarned) return false; + if (windows.length < FLAT_LIVE_WINDOWS) return false; + const ceiling = Math.max(...windows); + if (ceiling <= SILENCE_PEAK || ceiling >= FLAT_CEILING_PEAK) return false; + return envelopeSpreadDb(windows) < FLAT_SPREAD_DB; +} + /** * Does the capture device differ from the one the call application is using? * @@ -99,7 +165,11 @@ export function isCaptureSilent(peak: number): boolean { export function shouldFileRecording(args: { peak: number; transcriptTrimmedLen: number; + /** The capture was above the floor but never moved. Not expressible as a + * peak — that is the whole point of the failure. */ + flatInput?: boolean; }): boolean { if (isCaptureSilent(args.peak)) return false; + if (args.flatInput) return false; return args.transcriptTrimmedLen > 0; } diff --git a/src/engines/recording/client/index.ts b/src/engines/recording/client/index.ts index 88fee5d3..4ed64fca 100644 --- a/src/engines/recording/client/index.ts +++ b/src/engines/recording/client/index.ts @@ -36,15 +36,20 @@ export { export type { MeetingCandidate, MeetingMatch, MeetingResult, TimeSpan } from "./meeting"; export { + FLAT_CEILING_PEAK, + FLAT_LIVE_WINDOWS, + FLAT_SPREAD_DB, NO_AUDIO_WARN_AFTER_MS, SILENCE_PEAK, + envelopeSpreadDb, isCaptureSilent, micDeviceMismatch, shouldFileRecording, + shouldWarnFlatInput, shouldWarnNoAudio, } from "./guard"; -export { elapsed, pillView } from "./pill"; +export { elapsed, micMismatchTitle, pillView } from "./pill"; export type { PillState, PillView } from "./pill"; export { diff --git a/src/engines/recording/client/overlay.ts b/src/engines/recording/client/overlay.ts index 5ffd3287..38eacdf5 100644 --- a/src/engines/recording/client/overlay.ts +++ b/src/engines/recording/client/overlay.ts @@ -38,6 +38,8 @@ export interface OverlayInput { callAppName: string | null; /** The live dead-microphone warning. */ noAudio: boolean; + /** The input is live but deaf — a different failure, a different repair. */ + flatInput?: boolean; } export interface OverlayOutput { @@ -62,6 +64,7 @@ export function overlayOutput(input: OverlayInput): OverlayOutput { done, callAppName, noAudio, + flatInput, } = input; // Priority: done → filing → transcribing → stopping → recording → starting → prompt → hidden @@ -84,7 +87,11 @@ export function overlayOutput(input: OverlayInput): OverlayOutput { if (pillDismissed) { return { mode: "hidden", label: "" }; } - const label = noAudio ? "⚠ No audio — check your mic" : (callAppName ?? ""); + const label = noAudio + ? "⚠ No audio — check your mic" + : flatInput + ? "⚠ No input — check your mic is connected" + : (callAppName ?? ""); return { mode: "recording", label }; } if (preparing) { diff --git a/src/engines/recording/client/pill.ts b/src/engines/recording/client/pill.ts index 105f84df..f86fbbc8 100644 --- a/src/engines/recording/client/pill.ts +++ b/src/engines/recording/client/pill.ts @@ -41,6 +41,10 @@ export type PillView = { disabled: boolean; /** Show the live no-audio warning (see `guard.shouldWarnNoAudio`). */ warn: boolean; + /** WHICH warning, when `warn` is set. Silence and a dead input are + * different failures with different repairs, so they get different words; + * the surface renders this rather than a hardcoded sentence. */ + warnLabel?: string; /** The detected call application's name (when in callPrompt state). */ callAppName?: string; /** True when the pill has a dismiss action (callPrompt state). */ @@ -101,6 +105,8 @@ export function pillView(args: { callPrompt?: boolean; /** The detected call application's name. */ callAppName?: string; + /** The input is live but deaf (see `guard.shouldWarnFlatInput`). */ + flatInput?: boolean; /** The system default input and the call app's input disagree. */ micMismatch?: boolean; /** The system default input's name, when it is known. */ @@ -118,6 +124,7 @@ export function pillView(args: { error, callPrompt, callAppName, + flatInput, micMismatch, micDefaultDevice, micCallAppDevice, @@ -151,18 +158,23 @@ export function pillView(args: { } if (recording) { + // Precedence, loudest failure first: nothing captured, then captured from + // a dead input, then captured from the wrong device. Each has its own + // repair, so each gets its own words rather than one generic warning. + const title = noAudio + ? "No audio is reaching brains — check the microphone permission or input device. Recording continues." + : flatInput + ? "brains is recording, but the input level never changes — the microphone is probably disconnected or muted at the device. Check your input device. Recording continues." + : micMismatch + ? micMismatchTitle(micDefaultDevice, micCallAppDevice) + : "Stop recording"; return { state: "recording", label: elapsed(elapsedSec), - // Silence outranks a mismatch: nothing is being captured beats - // something is being captured from the wrong place. - title: noAudio - ? "No audio is reaching brains — check the microphone permission or input device. Recording continues." - : micMismatch - ? micMismatchTitle(micDefaultDevice, micCallAppDevice) - : "Stop recording", + title, disabled: !!stopping, - warn: !!noAudio, + warn: !!noAudio || !!flatInput, + warnLabel: noAudio ? "⚠ No audio" : flatInput ? "⚠ No input" : undefined, }; } if (stopping) { diff --git a/src/engines/recording/client/transcript.test.ts b/src/engines/recording/client/transcript.test.ts index 7f4ac12f..35e602d2 100644 --- a/src/engines/recording/client/transcript.test.ts +++ b/src/engines/recording/client/transcript.test.ts @@ -10,8 +10,12 @@ import { } from "./transcript"; import type { Recording, Transcript } from "./types"; -const heard: Pick = { hasAudio: true }; -const silent: Pick = { hasAudio: false }; +type Judged = Pick; +const heard: Judged = { hasAudio: true, flatInput: false }; +const silent: Judged = { hasAudio: false, flatInput: false }; +/** Above the silence floor and completely dead — the boosted disconnected + * input. `hasAudio` waves it through, which is exactly the hole. */ +const deaf: Judged = { hasAudio: true, flatInput: true }; function status(over: Partial): Transcript { return { @@ -46,6 +50,13 @@ describe("canFetchTranscript", () => { expect(canFetchTranscript({ recording: silent, state: "absent" })).toBe(false); }); + it("refuses a DEAD-INPUT capture, which is not silent and costs the same", () => { + // Two of these were uploaded and billed for a call that recorded nothing: + // the peak was well above the floor, so every level-based gate passed. + expect(canFetchTranscript({ recording: deaf, state: "absent" })).toBe(false); + expect(canFetchTranscript({ recording: deaf, state: null })).toBe(false); + }); + it("refuses what is already happening or cannot happen", () => { expect(canFetchTranscript({ recording: heard, state: "processing" })).toBe(false); expect(canFetchTranscript({ recording: heard, state: "ready" })).toBe(false); @@ -58,6 +69,13 @@ describe("transcriptBadge", () => { expect(transcriptBadge({ recording: silent, state: "ready" })).toBe("SILENT"); }); + it("says NO INPUT for a dead input — a different failure needs a different word", () => { + // "SILENT" would send the user hunting for a muted mic. The device was + // connected and unmuted; it was the wrong device, and disconnected. + expect(transcriptBadge({ recording: deaf, state: "ready" })).toBe("NO INPUT"); + expect(transcriptBadge({ recording: deaf, state: null })).toBe("NO INPUT"); + }); + it("marks a fetched transcript and a running one, and stays quiet otherwise", () => { expect(transcriptBadge({ recording: heard, state: "ready" })).toBe("TRANSCRIPT"); expect(transcriptBadge({ recording: heard, state: "processing" })).toBe("WORKING"); diff --git a/src/engines/recording/client/transcript.ts b/src/engines/recording/client/transcript.ts index a88bd190..bbbfc852 100644 --- a/src/engines/recording/client/transcript.ts +++ b/src/engines/recording/client/transcript.ts @@ -40,10 +40,14 @@ export function isTranscribing(state: TranscriptState | null | undefined): boole * already happening. */ export function canFetchTranscript(args: { - recording: Pick; + recording: Pick; state: TranscriptState | null | undefined; }): boolean { if (!args.recording.hasAudio) return false; + // A flat capture costs exactly what a silent one costs and comes back + // exactly as empty — it just is not silent, so `hasAudio` waves it through. + // The dead-input call billed two jobs this way before anyone noticed. + if (args.recording.flatInput) return false; return args.state == null || args.state === "absent" || args.state === "failed"; } @@ -52,13 +56,17 @@ export function canFetchTranscript(args: { * (+page.svelte:11784-11811), on this build's facts. * * SILENT outranks everything: a capture with no audio is the failure worth - * seeing, and its transcript state is irrelevant. + * seeing, and its transcript state is irrelevant. NO INPUT ranks just under + * it and is deliberately a DIFFERENT word: a boosted dead input is not + * silent, and "SILENT" would send the user looking for a muted microphone + * instead of a disconnected one. */ export function transcriptBadge(args: { - recording: Pick; + recording: Pick; state: TranscriptState | null | undefined; -}): "SILENT" | "TRANSCRIPT" | "WORKING" | null { +}): "SILENT" | "NO INPUT" | "TRANSCRIPT" | "WORKING" | null { if (!args.recording.hasAudio) return "SILENT"; + if (args.recording.flatInput) return "NO INPUT"; if (args.state === "ready") return "TRANSCRIPT"; if (args.state === "processing") return "WORKING"; return null; diff --git a/src/engines/recording/client/types.ts b/src/engines/recording/client/types.ts index 31351a90..7e87e10f 100644 --- a/src/engines/recording/client/types.ts +++ b/src/engines/recording/client/types.ts @@ -52,6 +52,10 @@ export type StopResult = { path: string | null; peak: number; durationSec: number; + /** The capture was above the silence floor but its level never moved — a + * live-but-deaf input. Gates the transcript fetch: a dead input bills a + * job that comes back empty. */ + flatInput: boolean; detail: string; }; @@ -92,6 +96,10 @@ export type Recording = { sizeBytes: number; /** False when the whole capture sat below the silence floor. */ hasAudio: boolean; + /** True when the capture was above the floor but never moved. A SIBLING of + * `hasAudio`, not a replacement — a boosted dead input is not silent, and + * a SILENT badge would point at the wrong repair. */ + flatInput: boolean; /** The call app that held the microphone when this started, if recognised. */ sourceApp: string | null; }; diff --git a/src/engines/recording/src/library.rs b/src/engines/recording/src/library.rs index e73737f5..213b50d3 100644 --- a/src/engines/recording/src/library.rs +++ b/src/engines/recording/src/library.rs @@ -27,6 +27,55 @@ const INDEX_FILENAME: &str = "index.json"; /// ~ -55 dBFS. Mirrored by `client/guard.ts` — change both or neither. pub const SILENCE_PEAK: i32 = 58; +// ── A DEAD INPUT IS ONE WHOSE LEVEL NEVER MOVES ─────────────────────────── +// +// [`SILENCE_PEAK`] catches a microphone feeding zeros. It cannot catch a +// disconnected input with gain on it, and that is the case that cost a call: +// a disconnected USB line-in at +30 dB boost measured +// +// peak -34.9 dB RMS -48.1 dB envelope spread 1.4 dB +// +// while the SAME device idle, with nobody speaking, measured -35.9 / -48.3 dB. +// Twelve seconds of loud speech was indistinguishable from that device's own +// hiss by level. After switching the default input: peak -11.5 dB, spread +// 29.5 dB. Level cannot tell those apart. VARIANCE can — speech moves, hiss +// does not — so the verdict is how far the loudest window sits above the +// quietest, in dB. +// +// Mirrored by `client/guard.ts` — change both or neither. + +/// How little the envelope may move before the input is called dead. 6 dB is +/// 4.3× the measured dead spread and about a fifth of the measured speech +/// spread; there is no observation anywhere between 1.4 and 29.5 dB to be +/// careful about. +pub const FLAT_SPREAD_DB: f64 = 6.0; + +/// Above this ceiling (−20 dBFS) a steady signal is exempt: loud and +/// unvarying is hold music or a tone, not a dead input. +/// +/// THE ONE CONSTANT WITH NO MEASURED NEGATIVE CASE BEHIND IT. Capture a +/// hold-music sample at the next bring-up and re-derive it. +pub const FLAT_CEILING_PEAK: i32 = 3277; + +/// Envelope windows required before the question may be answered at all. At +/// [`ENVELOPE_WINDOW_BYTES`] each, this is ~10 s of sampled audio; a capture +/// too short to have an envelope is never called flat. +pub const FLAT_MIN_WINDOWS: usize = 40; + +/// One envelope window: 250 ms at the sidecar's 32000 B/s (16 kHz mono +/// s16le). 40 of these is the same 320 KiB the old 5 × 64 KiB probe read. +pub const ENVELOPE_WINDOW_BYTES: u64 = 8192; + +/// How far `ceiling` sits above `floor`, in dB. A floor of zero is not a +/// small number — it is digital silence, which [`SILENCE_PEAK`] already owns, +/// so it reports an infinite spread rather than dividing by it. +pub fn spread_db(floor: i32, ceiling: i32) -> f64 { + if floor <= 0 || ceiling <= 0 { + return f64::INFINITY; + } + 20.0 * (f64::from(ceiling) / f64::from(floor)).log10() +} + /// Captures are named `call--mic.wav`; the companion files hang off the /// same stem so a capture is one directory listing, not an index. const CAPTURE_SUFFIX: &str = "-mic.wav"; @@ -42,6 +91,11 @@ pub struct Recording { /// False when the whole capture is below [`SILENCE_PEAK`] — the dead-mic /// case, which must never be filed as a meeting record. pub has_audio: bool, + /// True when the capture is above the silence floor but its level never + /// moved — a live-but-deaf input. A SIBLING of `has_audio`, not a + /// replacement: a boosted dead input is not silent, and badging it SILENT + /// would point the user at the wrong repair. + pub flat_input: bool, /// The call app that was using the microphone when this started, if it /// was recognised at the time. pub source_app: Option, @@ -54,6 +108,13 @@ pub struct Recording { pub struct IndexEntry { pub duration_sec: f64, pub has_audio: bool, + /// LOAD-BEARING `#[serde(default)]`. Every `index.json` already on disk + /// predates this field, and `read_index` turns a parse failure into an + /// EMPTY MAP (see its `unwrap_or_default`) — so without the default, + /// adding this field would present every existing user an empty recording + /// library rather than an error anybody could see. + #[serde(default)] + pub flat_input: bool, pub created_ms: u64, #[serde(skip_serializing_if = "Option::is_none")] pub source_app: Option, @@ -92,6 +153,7 @@ pub fn write_index_entry( filename: &str, duration_sec: f64, has_audio: bool, + flat_input: bool, created_ms: u64, source_app: Option<&str>, ) -> Result<()> { @@ -101,6 +163,7 @@ pub fn write_index_entry( IndexEntry { duration_sec, has_audio, + flat_input, created_ms, source_app: source_app.map(str::to_string), }, @@ -301,49 +364,126 @@ fn wav_header(bytes: &[u8]) -> Option<(u32, usize)> { None } -/// Duration, and whether anything was ever said. The peak is sampled from -/// multiple windows distributed across the WAV — not just the one-third point, -/// which would miss speech that only appeared elsewhere in the call. -pub fn probe(path: &Path, size: u64) -> (f64, bool) { +/// Everything readable about a finished capture in one pass over its envelope. +#[derive(Debug, Clone, Copy, PartialEq)] +pub struct Assessment { + pub duration_sec: f64, + /// Anything above [`SILENCE_PEAK`] was ever heard. + pub has_audio: bool, + /// Loud enough to pass [`has_audio`](Self::has_audio), yet the level never + /// moved: a live-but-deaf input. See the constants above. + pub flat_input: bool, + /// The loudest sample found (|s16|, 0..32767). + pub peak: i32, + /// Envelope windows actually read. Below [`FLAT_MIN_WINDOWS`] the flat + /// question is not answerable and is not answered. + pub windows: usize, +} + +/// Read the finished WAV's envelope: duration, whether anything was said, and +/// whether the level ever moved. +/// +/// THE DURABLE VERDICT. It reads the file, so it depends on nothing else — +/// not the UI's poll, not the sidecar's liveness (the sidecar's accumulators +/// die with it). It is also the same path the index self-heal takes, so a +/// fresh capture and a legacy one get the identical answer, which is not true +/// of the sidecar's cumulative peak. +/// +/// The envelope is 40 windows of 250 ms spread from the first sample to the +/// last, at the same 320 KiB the old 5-window probe read. The old shape also +/// had a real bug: five fixed points at 10/30/50/70/90% meant a long call +/// whose speech fell between them read as SILENT. +pub fn assess(path: &Path, size: u64) -> Assessment { use std::io::{Read, Seek, SeekFrom}; + const NOTHING: Assessment = Assessment { + duration_sec: 0.0, + has_audio: false, + flat_input: false, + peak: 0, + windows: 0, + }; let Ok(mut file) = std::fs::File::open(path) else { - return (0.0, false); + return NOTHING; }; let mut header = vec![0_u8; (4096_u64).min(size) as usize]; if file.read_exact(&mut header).is_err() { - return (0.0, false); + return NOTHING; } let Some((byte_rate, data_offset)) = wav_header(&header) else { - return (0.0, false); + return NOTHING; }; let data_len = size.saturating_sub(data_offset as u64); - let duration = if byte_rate == 0 { + let duration_sec = if byte_rate == 0 { 0.0 } else { data_len as f64 / byte_rate as f64 }; - // Scan 5 distributed windows: 10%, 30%, 50%, 70%, 90% into the data. - // Each window is 64 KiB — large enough to catch typical speech, small - // enough to keep listing cheap. Any window above the silence floor passes. - const WINDOW_SIZE: u64 = 64 * 1024; - const WINDOW_POSITIONS: [u64; 5] = [10, 30, 50, 70, 90]; - let mut peak = 0_i32; - let mut buffer = vec![0_u8; WINDOW_SIZE.min(data_len) as usize]; - for pct in WINDOW_POSITIONS { - let offset = data_offset as u64 + (data_len * pct / 100).saturating_sub(WINDOW_SIZE / 2); + if data_len < 2 { + return Assessment { + duration_sec, + ..NOTHING + }; + } + + // Windows are DISJOINT and span the whole file: first starts at the first + // sample, last ends at the last. There is no early exit — a spread needs + // every window, and the cost is bounded by the window count anyway. + let window_bytes = ENVELOPE_WINDOW_BYTES.min(data_len); + let window_count = FLAT_MIN_WINDOWS.min((data_len / window_bytes).max(1) as usize); + let stride = if window_count > 1 { + (data_len - window_bytes) / (window_count as u64 - 1) + } else { + 0 + }; + + let mut buffer = vec![0_u8; window_bytes as usize]; + let mut envelope: Vec = Vec::with_capacity(window_count); + for index in 0..window_count { + let offset = data_offset as u64 + stride * index as u64; if file.seek(SeekFrom::Start(offset)).is_err() { continue; } - let Ok(count) = file.read(&mut buffer) else { continue }; - for sample in buffer[..count].chunks_exact(2) { - peak = peak.max(i16::from_le_bytes([sample[0], sample[1]]).unsigned_abs() as i32); + let Ok(count) = file.read(&mut buffer) else { + continue; + }; + if count < 2 { + continue; } - // Early exit: any window above the floor is enough to call it not silent. - if peak > SILENCE_PEAK { - return (duration, true); + let mut window_peak = 0_i32; + for sample in buffer[..count].chunks_exact(2) { + window_peak = + window_peak.max(i16::from_le_bytes([sample[0], sample[1]]).unsigned_abs() as i32); } + envelope.push(window_peak); + } + + let peak = envelope.iter().copied().max().unwrap_or(0); + let floor = envelope.iter().copied().min().unwrap_or(0); + let windows = envelope.len(); + // Conjunctive with a level band, never variance alone. The lower bound + // hands digital silence back to the gate that already owns it; the upper + // bound exempts a loud steady tone; `floor > 0` short-circuits an + // all-zero capture before the ratio is ever taken. + let flat_input = windows >= FLAT_MIN_WINDOWS + && floor > 0 + && peak > SILENCE_PEAK + && peak < FLAT_CEILING_PEAK + && spread_db(floor, peak) < FLAT_SPREAD_DB; + + Assessment { + duration_sec, + has_audio: peak > SILENCE_PEAK, + flat_input, + peak, + windows, } - (duration, peak > SILENCE_PEAK) +} + +/// Duration and whether anything was said — the two answers listing needs. +/// A projection of [`assess`], so there is one envelope reader, not two. +pub fn probe(path: &Path, size: u64) -> (f64, bool) { + let assessment = assess(path, size); + (assessment.duration_sec, assessment.has_audio) } /// Pagination parameters for listing recordings. @@ -402,23 +542,37 @@ pub fn list_paged(recordings_dir: &Path, options: ListOptions) -> Result Result Vec { + (0..windows * WINDOW_SAMPLES) + .map(|i| { + if (i / WINDOW_SAMPLES) % 2 == 0 { + floor + } else { + ceiling + } + }) + .collect() + } + + /// Speech: quiet windows between loud ones. Deliberately kept UNDER + /// `FLAT_CEILING_PEAK` so this proves the spread rule rather than falling + /// through the loud-signal exemption. + fn speech_shaped(windows: usize) -> Vec { + flat_band(windows, (SILENCE_PEAK + 2) as i16, 3000) + } + + fn assess_wav(dir: &Path, name: &str, samples: &[i16]) -> Assessment { + let path = dir.join(name); + write_wav(&path, samples); + assess(&path, std::fs::metadata(&path).unwrap().len()) + } + + /// THE CASE THAT COST A CALL. A disconnected USB line-in at +30 dB boost + /// is not silent — peak −34.9 dB, well above the floor — and its envelope + /// moved 1.4 dB across twelve seconds of loud speech. Level cannot see it. + #[test] + fn a_boosted_dead_input_is_flat_and_a_voice_never_is() { + let tmp = tempfile::tempdir().unwrap(); + + // 1.4 dB of spread, exactly what the dead device measured. + let dead = assess_wav(tmp.path(), "call-1-mic.wav", &flat_band(40, 200, 235)); + assert!(dead.has_audio, "a boosted dead input is NOT silent"); + assert!(dead.flat_input, "spread {}", spread_db(200, 235)); + assert!(spread_db(200, 235) < FLAT_SPREAD_DB); + + let speech = assess_wav(tmp.path(), "call-2-mic.wav", &speech_shaped(40)); + assert!(speech.has_audio); + assert!( + !speech.flat_input, + "a voice moves — spread was {}", + spread_db((SILENCE_PEAK + 2) as i32, 3000) + ); + } + + /// The three exemptions, each of which would otherwise be a false alarm. + #[test] + fn silence_shortness_and_a_loud_steady_tone_are_never_called_flat() { + let tmp = tempfile::tempdir().unwrap(); + + // Digital silence belongs to SILENCE_PEAK. Calling it flat would badge + // it NO INPUT and point at the wrong repair. + let silence = vec![0_i16; 40 * WINDOW_SAMPLES]; + let silent = assess_wav(tmp.path(), "call-1-mic.wav", &silence); + assert!(!silent.has_audio); + assert!(!silent.flat_input, "digital silence is silent, not flat"); + + // Too short to have an envelope: refuse to answer rather than guess. + let brief = assess_wav(tmp.path(), "call-2-mic.wav", &flat_band(10, 200, 235)); + assert!(brief.windows < FLAT_MIN_WINDOWS, "{}", brief.windows); + assert!(!brief.flat_input, "a capture too short to judge is not flat"); + + // Hold music / a test tone: loud and unvarying. This is the exemption + // with no measured negative case behind it — see FLAT_CEILING_PEAK. + let steady = vec![9000_i16; 40 * WINDOW_SAMPLES]; + let tone = assess_wav(tmp.path(), "call-3-mic.wav", &steady); + assert!(tone.has_audio); + assert!(tone.peak > FLAT_CEILING_PEAK); + assert!( + !tone.flat_input, + "a loud steady tone is a signal, not a dead input" + ); + } + + /// The monotone-off property, and a real bug the old shape had: five fixed + /// probe points at 10/30/50/70/90% meant a long call whose speech fell + /// between them read as SILENT. The envelope now runs first sample to last. + #[test] + fn speech_only_at_the_very_end_of_a_long_capture_still_reads_as_audio() { + let tmp = tempfile::tempdir().unwrap(); + // 30 s of digital silence with 0.4 s of speech at the very end — past + // the old 90% window, which ended at 28 s. + let mut samples = vec![0_i16; 16000 * 30]; + let tail = samples.len() - 16000 * 2 / 5; + samples[tail..].fill(9000); + + let assessment = assess_wav(tmp.path(), "call-1-mic.wav", &samples); + assert!( + assessment.has_audio, + "speech in the last 0.4 s of a 30 s capture was missed" + ); + assert!(!assessment.flat_input); + } + + /// Adding a field to `IndexEntry` without `#[serde(default)]` would not + /// error — `read_index` swallows a parse failure into an EMPTY MAP, so + /// every existing user's library would simply vanish. + #[test] + fn an_index_written_before_flat_input_existed_still_parses() { + let tmp = tempfile::tempdir().unwrap(); + let dir = tmp.path(); + std::fs::write( + dir.join("index.json"), + r#"{"call-1-mic.wav":{"durationSec":12.0,"hasAudio":true,"createdMs":1700000000000}}"#, + ) + .unwrap(); + + let index = read_index(dir); + assert_eq!(index.len(), 1, "a legacy index parsed as an empty library"); + let entry = &index["call-1-mic.wav"]; + assert!(entry.has_audio); + assert!(!entry.flat_input, "an unknown verdict defaults to 'not flat'"); + } + #[test] fn listing_is_newest_first_and_ignores_everything_that_is_not_a_capture() { let tmp = tempfile::tempdir().unwrap(); @@ -685,8 +965,8 @@ mod tests { let dir = tmp.path(); std::fs::create_dir_all(dir).unwrap(); - write_index_entry(dir, "call-1-mic.wav", 3.5, true, 1700000000000, Some("Zoom")).unwrap(); - write_index_entry(dir, "call-2-mic.wav", 2.0, false, 1700000001000, None).unwrap(); + write_index_entry(dir, "call-1-mic.wav", 3.5, true, false, 1700000000000, Some("Zoom")).unwrap(); + write_index_entry(dir, "call-2-mic.wav", 2.0, false, false, 1700000001000, None).unwrap(); let index = read_index(dir); assert_eq!(index.len(), 2); @@ -730,7 +1010,7 @@ mod tests { let wav = capture_path(dir, "1"); write_wav(&wav, &vec![9000_i16; 16000]); - write_index_entry(dir, "call-1-mic.wav", 99.0, false, 1234567890000, Some("FakeApp")).unwrap(); + write_index_entry(dir, "call-1-mic.wav", 99.0, false, false, 1234567890000, Some("FakeApp")).unwrap(); let result = list_paged(dir, ListOptions::default()).unwrap(); assert_eq!(result.recordings.len(), 1); @@ -749,7 +1029,7 @@ mod tests { let wav = capture_path(dir, "1"); write_wav(&wav, &vec![9000_i16; 16000]); - write_index_entry(dir, "call-1-mic.wav", 3.0, true, 1700000000000, None).unwrap(); + write_index_entry(dir, "call-1-mic.wav", 3.0, true, false, 1700000000000, None).unwrap(); assert_eq!(read_index(dir).len(), 1); diff --git a/src/engines/recording/src/recorder.rs b/src/engines/recording/src/recorder.rs index 4fbb5ac1..03fc9993 100644 --- a/src/engines/recording/src/recorder.rs +++ b/src/engines/recording/src/recorder.rs @@ -457,17 +457,29 @@ impl Recorder { self.last_peak.store(peak, Ordering::Relaxed); let wav_path = Path::new(&path_str); + // Judge the FINISHED FILE, not the sidecar's cumulative peak. + // + // Two reasons. A cumulative max never falls, so one blip in the first + // second of a dead call keeps `has_audio` true for the rest of it — + // and it carries no envelope at all, so it cannot see a boosted dead + // input, which is not silent and never varies. And it is why a fresh + // capture and a self-healed one disagreed: the index self-heal has + // always read the file. + let assessment = library::assess( + wav_path, + std::fs::metadata(wav_path).map(|m| m.len()).unwrap_or(0), + ); if let Some(filename) = wav_path.file_name().map(|n| n.to_string_lossy().into_owned()) { // Use the START time, not the stop time (file modified time). // This is what created_ms should be — when the recording started. let created_ms = self.start_time_ms.load(Ordering::SeqCst); - let has_audio = peak > library::SILENCE_PEAK; let source_app = library::read_source_app(wav_path); let _ = library::write_index_entry( &self.layout.recordings_dir, &filename, duration_sec, - has_audio, + assessment.has_audio, + assessment.flat_input, created_ms, source_app.as_deref(), ); @@ -477,6 +489,7 @@ impl Recorder { path: path_str, peak, duration_sec, + flat_input: assessment.flat_input, }) } diff --git a/src/engines/recording/src/status.rs b/src/engines/recording/src/status.rs index cf9156a3..03fad54c 100644 --- a/src/engines/recording/src/status.rs +++ b/src/engines/recording/src/status.rs @@ -103,6 +103,10 @@ pub struct Stopped { pub path: String, pub peak: i32, pub duration_sec: f64, + /// The capture was above the silence floor but its level never moved — + /// a live-but-deaf input. Gates transcription and filing: a dead input + /// bills a transcript job that comes back empty. + pub flat_input: bool, } /// Live capture health for the UI: the peaks, plus the two device names whose @@ -200,6 +204,7 @@ mod tests { duration_sec: 1.0, size_bytes: 32000, has_audio: true, + flat_input: false, source_app: Some("zoom".into()), }; let specimens: Vec<(&str, serde_json::Value)> = vec![ @@ -235,6 +240,7 @@ mod tests { path: "/r/call-1-mic.wav".into(), peak: 900, duration_sec: 12.0, + flat_input: false, }) .unwrap(), ), @@ -254,6 +260,7 @@ mod tests { serde_json::to_value(crate::library::IndexEntry { duration_sec: 12.0, has_audio: true, + flat_input: false, created_ms: 1, source_app: Some("zoom".into()), }) diff --git a/src/layout/core/frame/Titlebar.svelte b/src/layout/core/frame/Titlebar.svelte index 71972005..8d1d8e57 100644 --- a/src/layout/core/frame/Titlebar.svelte +++ b/src/layout/core/frame/Titlebar.svelte @@ -121,7 +121,7 @@ {pill.label} {#if pill.warn} - ⚠ No audio + {pill.warnLabel ?? "⚠ No audio"} {:else} Stop {/if} diff --git a/src/layout/core/runtime/stores/recording-filing.svelte.ts b/src/layout/core/runtime/stores/recording-filing.svelte.ts index 2157a7fe..79118a64 100644 --- a/src/layout/core/runtime/stores/recording-filing.svelte.ts +++ b/src/layout/core/runtime/stores/recording-filing.svelte.ts @@ -76,7 +76,17 @@ class RecordingFiling { // SILENCE_PEAK, so treat it as peak=0 (silent). If hasAudio is true or // row is null, fall back to peak=-1 (unknown, defer to transcript check). const effectivePeak = row?.hasAudio === false ? 0 : -1; - if (!shouldFileRecording({ peak: effectivePeak, transcriptTrimmedLen: transcript.trim().length })) return; + if ( + !shouldFileRecording({ + peak: effectivePeak, + // A dead input is NOT silent, so it cannot be smuggled through the + // peak — it needs its own argument rather than another overload of a + // number that already means three things. + flatInput: row?.flatInput === true, + transcriptTrimmedLen: transcript.trim().length, + }) + ) + return; this.filing = path; const { [path]: _, ...rest } = this.errors; diff --git a/src/layout/core/runtime/stores/recording.svelte.ts b/src/layout/core/runtime/stores/recording.svelte.ts index 0241bcac..051f1cb5 100644 --- a/src/layout/core/runtime/stores/recording.svelte.ts +++ b/src/layout/core/runtime/stores/recording.svelte.ts @@ -29,11 +29,13 @@ import { canFetchTranscript, + FLAT_LIVE_WINDOWS, isCaptureSilent, isTranscribing, micDeviceMismatch, overlayOutput, pillView, + shouldWarnFlatInput, shouldWarnNoAudio, TRANSCRIPT_POLL_MS, type AudioStatus, @@ -129,6 +131,8 @@ export class RecordingStore { transcribing = $state(false); /** The full flow completed (transcript + filing done). Auto-clears. */ done = $state(false); + /** The input is live but deaf — level above the floor, never moving. */ + flatInput = $state(false); /** The default mic differs from the call app's mic (hot-swap warning). */ micMismatch = $state(false); /** The two device names behind `micMismatch`, so the pill can NAME them. @@ -137,6 +141,9 @@ export class RecordingStore { micDevices = $state<{ default: string | null; callApp: string | null } | null>(null); #startedAt = 0; + /** The last FLAT_LIVE_WINDOWS window peaks — the live envelope. Bounded, + * because a three-hour call must not grow an array all afternoon. */ + #windowPeaks: number[] = []; /** Last observed audio status for 2-observation mismatch logic. */ #lastAudioStatus: AudioStatus | null = null; /** Consecutive observations of the same mismatch (2 needed to trigger). */ @@ -174,6 +181,7 @@ export class RecordingStore { error: this.error, callPrompt: this.callPrompt, callAppName: this.callAppName ?? undefined, + flatInput: this.flatInput, micMismatch: this.micMismatch, micDefaultDevice: this.micDevices?.default ?? null, micCallAppDevice: this.micDevices?.callApp ?? null, @@ -346,6 +354,11 @@ export class RecordingStore { * instead. A capture the peak says was SILENT is never sent — that is * `guard.isCaptureSilent`, and it is the difference between a neutral "no * audio" row and a page of words invented over digital silence. + * + * A FLAT capture is refused for the same reason and costs the same money. + * The dead-input call that prompted this recorded 21 seconds across two + * takes, marked both as having audio, uploaded both, and billed two + * transcript jobs that came back empty — with nothing said to the user. */ async stop(): Promise { this.stopping = true; @@ -356,7 +369,7 @@ export class RecordingStore { this.#stopCapture(); await this.refreshStatus(); await this.refreshRecordings(); - if (result.stopped && result.path && !isCaptureSilent(result.peak)) { + if (result.stopped && result.path && !isCaptureSilent(result.peak) && !result.flatInput) { this.#pendingPath = result.path; this.transcribing = true; void this.fetchTranscript(result.path); @@ -765,6 +778,7 @@ export class RecordingStore { done: this.done, callAppName: effectiveAppName, noAudio: this.noAudio, + flatInput: this.flatInput, }); // Catch to avoid unhandled rejection in tests (no Tauri host). void getTransport().invoke("set_overlay", { mode, label }).catch(() => {}); @@ -829,6 +843,7 @@ export class RecordingStore { if (!this.#startedAt) this.#startedAt = Date.now(); this.#lastAudioStatus = null; this.#mismatchCount = 0; + this.#windowPeaks = []; this.#captureTimer = setInterval(() => { const elapsedMs = Date.now() - this.#startedAt; this.elapsedSec = Math.floor(elapsedMs / 1000); @@ -847,6 +862,22 @@ export class RecordingStore { ) { this.noAudio = true; } + // The live envelope, on the same latch-never-clear rule and for the + // same reason: a spread that widens for one window does not give back + // the minutes that were dead. + const window = this.status?.windowPeak; + if (typeof window === "number" && window >= 0) { + this.#windowPeaks.push(window); + if (this.#windowPeaks.length > FLAT_LIVE_WINDOWS) this.#windowPeaks.shift(); + } + if ( + shouldWarnFlatInput({ + windows: this.#windowPeaks, + alreadyWarned: this.flatInput || this.noAudio, + }) + ) { + this.flatInput = true; + } // AFTER the status read, never beside it. `recording_audio_status` // now echoes what `recording_status` last fetched, so firing them // concurrently would hand this tick the PREVIOUS tick's window peak. @@ -909,6 +940,8 @@ export class RecordingStore { this.#startedAt = 0; this.elapsedSec = 0; this.noAudio = false; + this.flatInput = false; + this.#windowPeaks = []; this.micMismatch = false; this.micDevices = null; this.recordingAppName = null; diff --git a/src/layout/panes/sidebar/__tests__/recording-library.test.ts b/src/layout/panes/sidebar/__tests__/recording-library.test.ts index 66459338..5bb8a8ee 100644 --- a/src/layout/panes/sidebar/__tests__/recording-library.test.ts +++ b/src/layout/panes/sidebar/__tests__/recording-library.test.ts @@ -35,6 +35,7 @@ const HEARD: Recording = { durationSec: 760, sizeBytes: 1024, hasAudio: true, + flatInput: false, sourceApp: "zoom.us", }; @@ -44,6 +45,7 @@ const SILENT: Recording = { filename: "call-1-mic.wav", durationSec: 40, hasAudio: false, + flatInput: false, sourceApp: null, }; From 523ba9f116716d0632e4becba934c99cccb1429a Mon Sep 17 00:00:00 2001 From: Amir SSV Labs Date: Mon, 10 Aug 2026 18:20:14 +0300 Subject: [PATCH 10/16] feat(windows): name the input device MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Recall binds capture to the SYSTEM DEFAULT input. When that default is the wrong device, everything downstream still succeeds — the sidecar runs, the WAV is written, the upload completes, the transcript job is billed — and the only thing missing is the audio. The warning this replaces read, in full: No audio is reaching brains — check the microphone permission or input device. True, and useless. On the bring-up that produced this change the answer was a disconnected USB line-in that had quietly become the system default; naming it would have saved the hour it cost. It now reads: No audio is reaching brains from "Microphone (3- USB Audio Device)" — check that device is connected, unmuted, and permitted. New file rather than an addition to `call_mic.rs`: that file is 418 lines and Windows COM would carry it past the 600-line gate. It also is not the same job — `call_mic` BORROWS and restores the macOS default device; this only asks a question. Zero new crates. `windows` 0.61.3 is already in Cargo.lock via tao/tauri/wry/webview2-com, so the lockfile gains exactly one line. `windows-sys` stays for the Job Object — rewriting proven shipped code is churn. The sequence is the one verified by hand from PowerShell on 10 Aug: CoInitializeEx → MMDeviceEnumerator → GetDefaultAudioEndpoint(eCapture, eConsole) → GetId → OpenPropertyStore(STGM_READ) → PKEY_Device_FriendlyName. eConsole, not eCommunications: eConsole is what the Settings picker writes and what the SDK binds, so eCommunications could name a device that is not the one being recorded — worse than saying nothing. `ComGuard` balances CoInitializeEx INCLUDING when it answered S_FALSE. S_FALSE means COM was already initialized on this thread and it still took a per-thread reference; skipping the uninit leaks one every capture. DEPARTURE FROM THE PLAN, deliberate: the plan called for a `PropVariant` RAII guard calling `PropVariantClear`, on the basis that windows-rs 0.61's PROPVARIANT has no Drop. It does — `windows-0.61.3/src/extensions/Win32/ System/StructuredStorage.rs` implements Drop as PropVariantClear. Adding the guard would have been a DOUBLE FREE. The type's own Drop is left to do it, and the name is read through its `Display` (PropVariantToBSTR) rather than by reaching into the union by hand. Privacy, deliberate: the friendly name is shown and logged; the ENDPOINT ID never leaves the process. It is a stable per-device identifier, v1 refused to log it, and that judgement stands — it is fetched only as a future watcher's comparison key. It can never break recording: `Option`, every step `.ok()?`, and no unwrap/expect/panic in the file. A machine with no capture endpoint — every CI runner — answers None and the UI shows nothing extra. The deciding logic is platform-free so it is covered on every CI leg, not just the one that does not exist yet: `display_name`, `state_word` and `is_suspect` are ordinary functions with ordinary tests. Behind the Windows cfg only what is assertable without knowing the machine: that the query is stable across calls and survives a short-lived thread (which is the S_FALSE balance). Verified on this Windows 11 machine, where it returns "Microphone Array (Intel® Smart Sound Technology for Digital Mic…" — the ® being exactly why truncation is on a char boundary and not a byte one. The device also appears as a chip beside the warning in the titlebar, bounded at 18ch and ellipsised so arbitrary vendor text cannot push the titlebar's other columns around, and only while WARNING — it is dead weight on a healthy call. On the plan's open question of whether that chip moves an `eval:pixels` region: the pixel eval CANNOT answer it on this base. It is already BLOCKED on all four surfaces (goldens self-marked UNUSABLE after the W1 gaps refactor, plus missing fixtures for set_overlay, set_tray_status, set_tray_recording_visible and recordings_list_paged). The chip is gated on `pill.warn`, which requires a live capture the static fixtures do not have, so it cannot appear in those frames regardless. Re-check when the goldens are re-recorded. --- Cargo.lock | 1 + src/engines/recording/Cargo.toml | 18 ++ src/engines/recording/client/index.ts | 2 +- src/engines/recording/client/pill.test.ts | 46 +++ src/engines/recording/client/pill.ts | 37 ++- src/engines/recording/client/types.ts | 4 + src/engines/recording/src/input_device.rs | 266 ++++++++++++++++++ src/engines/recording/src/lib.rs | 1 + src/engines/recording/src/recorder.rs | 1 + src/engines/recording/src/status.rs | 6 + src/layout/core/frame/Titlebar.svelte | 17 ++ .../core/runtime/stores/recording.svelte.ts | 1 + 12 files changed, 397 insertions(+), 3 deletions(-) create mode 100644 src/engines/recording/src/input_device.rs diff --git a/Cargo.lock b/Cargo.lock index e5277894..cb736789 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -478,6 +478,7 @@ dependencies = [ "tar", "tempfile", "thiserror 2.0.19", + "windows", "windows-sys 0.59.0", ] diff --git a/src/engines/recording/Cargo.toml b/src/engines/recording/Cargo.toml index 8c193df4..2e2031c5 100644 --- a/src/engines/recording/Cargo.toml +++ b/src/engines/recording/Cargo.toml @@ -43,6 +43,24 @@ windows-sys = { version = "0.59", features = [ # CreateJobObjectW takes a SECURITY_ATTRIBUTES pointer (we pass null). "Win32_Security", ] } +# WASAPI, to NAME the default input device (src/input_device.rs). The +# ergonomic `windows` crate rather than `windows-sys`, because the COM +# interfaces and PROPVARIANT's own Drop are what keep that code free of +# hand-rolled vtables and manual frees. NO NEW CRATE: 0.61.3 is already in +# Cargo.lock via tao/tauri/wry/webview2-com. `windows-sys` stays for the Job +# Object — rewriting proven shipped code is churn. +windows = { version = "0.61", features = [ + "Win32_Foundation", + "Win32_Media_Audio", + "Win32_System_Com", + # IPropertyStore::GetValue is cfg'd on BOTH of these. + "Win32_System_Com_StructuredStorage", + "Win32_System_Variant", + # PropVariantToBSTR, behind PROPVARIANT's Display. + "Win32_UI_Shell_PropertiesSystem", + # PKEY_Device_FriendlyName itself. + "Win32_Devices_FunctionDiscovery", +] } [target.'cfg(target_os = "macos")'.dependencies] # CoreAudio FFI: process-object polling for call detection and device queries. diff --git a/src/engines/recording/client/index.ts b/src/engines/recording/client/index.ts index 4ed64fca..5629d8b6 100644 --- a/src/engines/recording/client/index.ts +++ b/src/engines/recording/client/index.ts @@ -49,7 +49,7 @@ export { shouldWarnNoAudio, } from "./guard"; -export { elapsed, micMismatchTitle, pillView } from "./pill"; +export { elapsed, flatInputTitle, micMismatchTitle, noAudioTitle, pillView } from "./pill"; export type { PillState, PillView } from "./pill"; export { diff --git a/src/engines/recording/client/pill.test.ts b/src/engines/recording/client/pill.test.ts index 0f3578cf..951a2a5e 100644 --- a/src/engines/recording/client/pill.test.ts +++ b/src/engines/recording/client/pill.test.ts @@ -75,6 +75,52 @@ describe("pillView", () => { expect(view.title).toContain("Recording continues"); }); + it("names the device in the dead-microphone warning — the whole point of it", () => { + // The sentence this replaces was "check the microphone permission or + // input device": true, and no help at all in finding WHICH one. + const named = pillView({ + availability: READY, + recording: true, + noAudio: true, + inputDevice: "Microphone (3- USB Audio Device)", + }); + expect(named.title).toContain('"Microphone (3- USB Audio Device)"'); + expect(named.inputDevice).toBe("Microphone (3- USB Audio Device)"); + expect(named.warnLabel).toBe("⚠ No audio"); + + // Every non-Windows machine, and any machine with no capture endpoint. + const unnamed = pillView({ availability: READY, recording: true, noAudio: true }); + expect(unnamed.title).toContain("check the microphone permission"); + expect(unnamed.inputDevice).toBeUndefined(); + expect(unnamed.title).not.toContain('""'); + }); + + it("gives the dead INPUT its own words and its own label", () => { + const view = pillView({ + availability: READY, + recording: true, + flatInput: true, + inputDevice: "Line In (3- USB Audio Device)", + }); + // Not "no audio" — there IS audio, it just never changes. Different + // failure, different repair. + expect(view.warnLabel).toBe("⚠ No input"); + expect(view.title).toContain("the level never changes"); + expect(view.title).toContain("Line In (3- USB Audio Device)"); + expect(view.warn).toBe(true); + }); + + it("treats a blank device name as no name rather than empty quotes", () => { + const view = pillView({ + availability: READY, + recording: true, + noAudio: true, + inputDevice: " ", + }); + expect(view.inputDevice).toBeUndefined(); + expect(view.title).not.toContain('""'); + }); + it("names BOTH devices when the default input and the call app disagree", () => { // The backend answered this in snake_case until the AudioStatus move, so // `micDeviceMismatch` never fired and this sentence had no way to appear. diff --git a/src/engines/recording/client/pill.ts b/src/engines/recording/client/pill.ts index f86fbbc8..7800d514 100644 --- a/src/engines/recording/client/pill.ts +++ b/src/engines/recording/client/pill.ts @@ -45,6 +45,8 @@ export type PillView = { * different failures with different repairs, so they get different words; * the surface renders this rather than a hardcoded sentence. */ warnLabel?: string; + /** The device to go and fix, shown beside a warning when it is known. */ + inputDevice?: string; /** The detected call application's name (when in callPrompt state). */ callAppName?: string; /** True when the pill has a dismiss action (callPrompt state). */ @@ -61,6 +63,33 @@ export function elapsed(seconds: number): string { return hours > 0 ? `${hours}:${pad(minutes)}:${pad(secs)}` : `${minutes}:${pad(secs)}`; } +/** + * The dead-microphone sentence — WITH the device in it when it is known. + * + * The version this replaces read "No audio is reaching brains — check the + * microphone permission or input device." Every word of that is true and none + * of it tells you which device to look at. On the bring-up that produced this + * change, the answer was a disconnected USB line-in that had quietly become + * the system default; naming it would have saved the hour. + */ +export function noAudioTitle(inputDevice?: string | null): string { + const device = (inputDevice ?? "").trim(); + if (!device) { + return "No audio is reaching brains — check the microphone permission or input device. Recording continues."; + } + return `No audio is reaching brains from "${device}" — check that device is connected, unmuted, and permitted. Recording continues.`; +} + +/** + * The dead-INPUT sentence: a level that never moves. Different failure, + * different repair — see `guard.shouldWarnFlatInput`. + */ +export function flatInputTitle(inputDevice?: string | null): string { + const device = (inputDevice ?? "").trim(); + const named = device ? ` "${device}"` : " your input device"; + return `brains is recording from${named}, but the level never changes — that usually means the microphone is disconnected or muted at the device. Recording continues.`; +} + /** * The mismatch sentence, with the device names in it when they are known. * @@ -107,6 +136,8 @@ export function pillView(args: { callAppName?: string; /** The input is live but deaf (see `guard.shouldWarnFlatInput`). */ flatInput?: boolean; + /** The device brains is recording from, when the platform can name it. */ + inputDevice?: string | null; /** The system default input and the call app's input disagree. */ micMismatch?: boolean; /** The system default input's name, when it is known. */ @@ -125,6 +156,7 @@ export function pillView(args: { callPrompt, callAppName, flatInput, + inputDevice, micMismatch, micDefaultDevice, micCallAppDevice, @@ -162,9 +194,9 @@ export function pillView(args: { // a dead input, then captured from the wrong device. Each has its own // repair, so each gets its own words rather than one generic warning. const title = noAudio - ? "No audio is reaching brains — check the microphone permission or input device. Recording continues." + ? noAudioTitle(inputDevice) : flatInput - ? "brains is recording, but the input level never changes — the microphone is probably disconnected or muted at the device. Check your input device. Recording continues." + ? flatInputTitle(inputDevice) : micMismatch ? micMismatchTitle(micDefaultDevice, micCallAppDevice) : "Stop recording"; @@ -175,6 +207,7 @@ export function pillView(args: { disabled: !!stopping, warn: !!noAudio || !!flatInput, warnLabel: noAudio ? "⚠ No audio" : flatInput ? "⚠ No input" : undefined, + inputDevice: (inputDevice ?? "").trim() || undefined, }; } if (stopping) { diff --git a/src/engines/recording/client/types.ts b/src/engines/recording/client/types.ts index 7e87e10f..1d774e54 100644 --- a/src/engines/recording/client/types.ts +++ b/src/engines/recording/client/types.ts @@ -28,6 +28,10 @@ export type RecordingStatus = { path: string | null; /** Where finished captures live, available even when recording is not. */ recordingsDir: string; + /** The input device brains is recording from, when the platform can name + * it (Windows today). Absent elsewhere and on a machine with no capture + * endpoint — the warnings fall back to their device-less wording. */ + inputDevice?: string | null; /** Terminal error from sidecar loss during capture. Cleared by next start. */ error?: string; }; diff --git a/src/engines/recording/src/input_device.rs b/src/engines/recording/src/input_device.rs new file mode 100644 index 00000000..58e6140b --- /dev/null +++ b/src/engines/recording/src/input_device.rs @@ -0,0 +1,266 @@ +// WHICH microphone is brains actually recording from? +// +// Recall binds capture to the SYSTEM DEFAULT input. When that default is the +// wrong device, everything downstream still succeeds: the sidecar runs, the +// WAV is written, the upload completes, the transcript job is billed. The +// only thing missing is the audio, and until this module there was no way for +// the app to say which device it had been listening to. +// +// The warning it replaces read, in full: "No audio is reaching brains — check +// the microphone permission or input device." True, and useless. Naming the +// device would have saved the hour the dead-input bring-up cost. +// +// THREE RULES HERE: +// +// 1. IT NEVER BREAKS RECORDING. Every step is `.ok()?` into an +// `Option`; there is no `unwrap`, `expect` or `panic!` in this +// file. No default device — every CI runner — answers `None`, and the UI +// simply shows nothing extra. +// 2. THE ENDPOINT ID NEVER LEAVES THE PROCESS. The friendly name is shown, +// written beside the WAV and logged once per capture. The endpoint ID is +// a stable per-device identifier; v1 refused to log it and that judgement +// stands. It exists here only as a future watcher's comparison key. +// 3. THE DECIDING LOGIC IS PLATFORM-FREE. `display_name`, `state_word` and +// `is_suspect` are ordinary functions with ordinary tests that run on +// every CI leg. v1's lesson was that a Windows-cfg test "cannot go red in +// a way anyone sees" — and until the CI leg in the next commit, there was +// no Windows leg here at all. + +/// How long a device name may be before the UI would rather elide it. Long +/// enough for "Microphone (3- USB Audio Device)"; short enough that a +/// pathological name cannot blow out a tooltip. +const MAX_NAME_CHARS: usize = 64; + +/// Clean a raw device name into something showable, or nothing. +/// +/// A blank name is NOT a name: WASAPI happily returns an empty friendly name +/// for some virtual endpoints, and `Some("")` would render as a warning with +/// an empty pair of quotes in it, which reads as a bug. +/// +/// Truncation is on a CHAR boundary. Device names carry manufacturer strings +/// and those are routinely non-ASCII; slicing a `String` by bytes panics, and +/// this module's first rule is that it cannot. +pub fn display_name(raw: Option<&str>) -> Option { + let trimmed = raw?.trim(); + if trimmed.is_empty() { + return None; + } + if trimmed.chars().count() <= MAX_NAME_CHARS { + return Some(trimmed.to_string()); + } + let cut: String = trimmed.chars().take(MAX_NAME_CHARS - 1).collect(); + Some(format!("{cut}…")) +} + +/// The `DEVICE_STATE` bits, in words a person can read. +/// +/// `unplugged` is the interesting one and the reason this exists: a jack with +/// nothing in it still enumerates, still binds, and still feeds a level when +/// the input has gain on it. +pub fn state_word(state: u32) -> &'static str { + match state { + 0x1 => "active", + 0x2 => "disabled", + 0x4 => "not present", + 0x8 => "unplugged", + _ => "unknown", + } +} + +/// Is this the KIND of input that is usually not a microphone at all? +/// +/// A line-in, a "Stereo Mix", or a virtual cable will happily be the system +/// default and will happily record nothing (or, boosted, its own hiss — the +/// case that started all of this). This does not decide anything; it lets the +/// warning add a sentence when the name itself is the clue. +pub fn is_suspect(name: &str) -> bool { + let lowered = name.to_ascii_lowercase(); + [ + "line in", + "line-in", + "stereo mix", + "what u hear", + "virtual", + "vb-audio", + "cable output", + "aux", + ] + .iter() + .any(|needle| lowered.contains(needle)) +} + +/// The name of the input device the system would record from right now, if +/// this platform can say. `None` everywhere except Windows, and `None` on +/// Windows when there is no default capture endpoint. +pub fn default_input_name() -> Option { + #[cfg(windows)] + { + imp::default_input_name() + } + #[cfg(not(windows))] + { + // macOS answers this through `call_mic::default_input_device_name`, + // which predates this module and also does the device BORROWING that + // has no Windows equivalent. Two callers, one platform each; merging + // them would drag CoreAudio's borrow/restore state into a query. + None + } +} + +#[cfg(windows)] +mod imp { + use windows::Win32::Devices::FunctionDiscovery::PKEY_Device_FriendlyName; + use windows::Win32::Media::Audio::{ + eCapture, eConsole, IMMDeviceEnumerator, MMDeviceEnumerator, + }; + use windows::Win32::System::Com::{ + CoCreateInstance, CoInitializeEx, CoUninitialize, CLSCTX_ALL, COINIT_MULTITHREADED, + STGM_READ, + }; + + /// Balances `CoInitializeEx` on drop — INCLUDING when it answered + /// `S_FALSE`. + /// + /// `S_FALSE` means "COM was already initialized on this thread", and it is + /// still a successful initialization that took a reference on the + /// per-thread apartment. Treating it as "someone else owns this, skip the + /// uninit" leaks a reference every call, and this runs once per capture + /// for the life of the process. v1 got this right; it is repeated here + /// because it is the kind of thing a rewrite quietly drops. + struct ComGuard; + + impl ComGuard { + fn enter() -> Option { + // SAFETY: no arguments, no aliasing; the return is inspected + // rather than assumed. + let hr = unsafe { CoInitializeEx(None, COINIT_MULTITHREADED) }; + if hr.is_ok() { + Some(Self) + } else { + None + } + } + } + + impl Drop for ComGuard { + fn drop(&mut self) { + // SAFETY: paired with the CoInitializeEx that produced this guard, + // on the same thread. + unsafe { CoUninitialize() }; + } + } + + pub fn default_input_name() -> Option { + let _com = ComGuard::enter()?; + + // SAFETY: every call below is a plain COM call whose result is + // checked. Nothing is transmuted and no raw pointer outlives the + // expression it is produced in. + unsafe { + let enumerator: IMMDeviceEnumerator = + CoCreateInstance(&MMDeviceEnumerator, None, CLSCTX_ALL).ok()?; + + // eConsole, NOT eCommunications. eConsole is the role the Windows + // Sound settings picker writes and the role the SDK binds; asking + // for eCommunications can name a different device than the one + // actually being recorded, which would make this WORSE than + // saying nothing. + let device = enumerator + .GetDefaultAudioEndpoint(eCapture, eConsole) + .ok()?; + + // The endpoint id is deliberately not returned, logged, or stored + // — see rule 2 at the top. It is fetched because a future default + // -device watcher needs a comparison key that survives a rename, + // and because failing here means the endpoint is already gone. + let _endpoint_id = device.GetId().ok()?; + + let store = device.OpenPropertyStore(STGM_READ).ok()?; + let value = store.GetValue(&PKEY_Device_FriendlyName).ok()?; + // PROPVARIANT owns its buffer and clears it on Drop (windows-rs + // 0.61 implements Drop -> PropVariantClear). Calling + // PropVariantClear here as well would be a double free. + let name = value.to_string(); + + super::display_name(Some(&name)) + } + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn a_blank_or_missing_name_is_no_name_at_all() { + // `Some("")` would render as a warning quoting an empty string, which + // reads as a bug rather than as "unknown". + assert_eq!(display_name(None), None); + assert_eq!(display_name(Some("")), None); + assert_eq!(display_name(Some(" ")), None); + assert_eq!( + display_name(Some(" Microphone (3- USB Audio Device) ")).as_deref(), + Some("Microphone (3- USB Audio Device)") + ); + } + + #[test] + fn a_long_name_is_cut_on_a_char_boundary_not_a_byte_one() { + // Manufacturer strings are routinely non-ASCII, and slicing a String + // by bytes panics — which this module is not allowed to do. + let wide = "é".repeat(200); + let cut = display_name(Some(&wide)).unwrap(); + assert_eq!(cut.chars().count(), MAX_NAME_CHARS); + assert!(cut.ends_with('…')); + + let exact = "a".repeat(MAX_NAME_CHARS); + assert_eq!( + display_name(Some(&exact)).unwrap(), + exact, + "no needless cut" + ); + } + + #[test] + fn device_states_read_as_words_and_an_unknown_bit_is_not_a_panic() { + assert_eq!(state_word(0x1), "active"); + // The state that started this: a jack with nothing in it still + // enumerates, still binds, and still feeds a level when boosted. + assert_eq!(state_word(0x8), "unplugged"); + assert_eq!(state_word(0xdead), "unknown"); + } + + #[test] + fn inputs_that_are_usually_not_microphones_are_flagged() { + assert!(is_suspect("Line In (3- USB Audio Device)")); + assert!(is_suspect("Stereo Mix (Realtek(R) Audio)")); + assert!(is_suspect("CABLE Output (VB-Audio Virtual Cable)")); + assert!(!is_suspect("Microphone Array (Intel® Smart Sound)")); + assert!(!is_suspect("Headset (AirPods Pro)")); + } + + /// Behind the Windows cfg, only what is actually assertable without + /// knowing the machine: the query is stable across calls, and it survives + /// a short-lived thread — which is the S_FALSE balance in `ComGuard` + /// failing loudly if it is ever dropped. + #[cfg(windows)] + #[test] + fn the_query_is_stable_and_survives_a_thread_that_ends() { + let first = default_input_name(); + let second = default_input_name(); + assert_eq!(first, second, "the same default named two different ways"); + + let threaded = std::thread::spawn(default_input_name).join().unwrap(); + assert_eq!( + first, threaded, + "a fresh COM apartment answered differently" + ); + + // Whatever it answered, it obeys display_name's contract. A CI runner + // with no capture endpoint answers None, and that is a pass. + if let Some(name) = first { + assert!(!name.trim().is_empty()); + assert!(name.chars().count() <= MAX_NAME_CHARS); + } + } +} diff --git a/src/engines/recording/src/lib.rs b/src/engines/recording/src/lib.rs index d7ef8ba3..cbff80ae 100644 --- a/src/engines/recording/src/lib.rs +++ b/src/engines/recording/src/lib.rs @@ -24,6 +24,7 @@ pub mod api; pub mod call_detector; pub mod call_mic; +pub mod input_device; pub mod library; pub mod recorder; pub mod runtime; diff --git a/src/engines/recording/src/recorder.rs b/src/engines/recording/src/recorder.rs index 03fc9993..6f54b4f4 100644 --- a/src/engines/recording/src/recorder.rs +++ b/src/engines/recording/src/recorder.rs @@ -322,6 +322,7 @@ impl Recorder { .ok() .and_then(|path| path.as_ref().map(|p| p.to_string_lossy().into_owned())), recordings_dir: self.layout.recordings_dir.to_string_lossy().into_owned(), + input_device: crate::input_device::default_input_name(), error: self.last_error.lock().ok().and_then(|e| e.clone()), } } diff --git a/src/engines/recording/src/status.rs b/src/engines/recording/src/status.rs index 03fad54c..1e619cad 100644 --- a/src/engines/recording/src/status.rs +++ b/src/engines/recording/src/status.rs @@ -76,6 +76,11 @@ pub struct Status { pub path: Option, /// Where finished captures live, whether or not recording is available. pub recordings_dir: String, + /// The input device the system would record from, when the platform can + /// say. `None` off Windows and on a machine with no capture endpoint — + /// the UI simply shows nothing extra. See `input_device.rs`. + #[serde(skip_serializing_if = "Option::is_none")] + pub input_device: Option, /// Terminal error from the last capture (sidecar crash/loss). Cleared /// by the next `start`. #[serde(skip_serializing_if = "Option::is_none")] @@ -221,6 +226,7 @@ mod tests { window_peak: 7, path: Some("/r/call-1-mic.wav".into()), recordings_dir: "/r".into(), + input_device: Some("Microphone (3- USB Audio)".into()), error: Some("boom".into()), }) .unwrap(), diff --git a/src/layout/core/frame/Titlebar.svelte b/src/layout/core/frame/Titlebar.svelte index 8d1d8e57..99fae6a8 100644 --- a/src/layout/core/frame/Titlebar.svelte +++ b/src/layout/core/frame/Titlebar.svelte @@ -122,6 +122,11 @@ {pill.label} {#if pill.warn} {pill.warnLabel ?? "⚠ No audio"} + + {#if pill.inputDevice} + {pill.inputDevice} + {/if} {:else} Stop {/if} @@ -284,6 +289,18 @@ color: var(--warning-ink); } + /* The device behind a warning. Bounded and ellipsised: a device name is + arbitrary vendor text and must never push the titlebar's other columns + around. */ + .device { + max-width: 18ch; + overflow: hidden; + text-overflow: ellipsis; + white-space: nowrap; + color: var(--record-hint-ink); + opacity: 0.85; + } + .spin { width: var(--record-spinner-size); height: var(--record-spinner-size); diff --git a/src/layout/core/runtime/stores/recording.svelte.ts b/src/layout/core/runtime/stores/recording.svelte.ts index 051f1cb5..e213c3bf 100644 --- a/src/layout/core/runtime/stores/recording.svelte.ts +++ b/src/layout/core/runtime/stores/recording.svelte.ts @@ -182,6 +182,7 @@ export class RecordingStore { callPrompt: this.callPrompt, callAppName: this.callAppName ?? undefined, flatInput: this.flatInput, + inputDevice: this.status?.inputDevice ?? null, micMismatch: this.micMismatch, micDefaultDevice: this.micDevices?.default ?? null, micCallAppDevice: this.micDevices?.callApp ?? null, From 34cc66945bb4da66daee0d3f4c98d7cc7adeab3a Mon Sep 17 00:00:00 2001 From: Amir SSV Labs Date: Mon, 10 Aug 2026 18:27:43 +0300 Subject: [PATCH 11/16] ci: a Windows leg that can go red MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Call recording had never been run on Windows, and the reason it could go that long is that nothing in CI ever compiled for it — `ci.yml`'s matrix was `os: [macos-latest]`. That is the root cause of the whole saga, not a footnote to it. Two gaps, both load-bearing: * NO WINDOWS LEG AT ALL. New `windows` job: clippy with `--all-targets` (without it the test-only code is not even compiled), the unit tests, and a build. `defaults.run.shell: bash` because every step here is POSIX and the runner's default is pwsh. * CLIPPY NEVER REACHED THE ENGINES. `-D warnings` only ever covered `brains-desktop`, so no engine crate's warnings gated anything. Added to the macOS leg too — these gaps are why the defects survived. That second one found SIX errors in `brains-recording`, not the two the plan expected, and none are `#[allow]`ed: * `call_detector.rs` — an unused `let config` in a test. Fires on macOS too; it survived because clippy never looked here. * `library.rs` ×3 — `io::Error::new(ErrorKind::Other, e)` → `Error::other`. * `library.rs` ×2 — a manual `is_multiple_of` and an `i32 as i32`, both in the new detector tests. * `sidecar.rs` — `impl Drop for Job` sat AFTER the test module. Moved up beside the rest of `Job`'s behaviour, which reads better anyway. `src-tauri/src/lib.rs` also had `CallDetector`/`DetectorConfig` imported at the top and used only inside the macOS block. They move to a local `use` INSIDE that block: the file is at the 600-line hard ceiling with exactly one line of headroom (`check-file-sizes.mjs`), and a top-level cfg-split costs two and fails the Frontend job. It is now at exactly 600. THE JOB IS DELIBERATELY NARROW, and the scope is the design. `--workspace` is excluded because `agents/local/src/sandbox.rs` asserts `real_path("/tmp") == "/private/tmp"` with no platform gate; `-p brains-desktop --lib` is excluded because it pulls in `brains-storage`, which has its own pre-existing clippy debt. Both are worth fixing and both are their own PR. Landing this job red would get `continue-on-error` bolted onto it within a week — which is the exact failure it exists to prevent. PR #12 was publicly corrected for a green Windows job hiding 14 failing tests. `14-recording.yaml`'s pinned test counts are relaxed to count-agnostic patterns. One of them was ALREADY BROKEN on dev: the two mounted files hold 21 tests and the spec demanded `Tests 1[0-9] passed`. A pinned count makes adding a test a spec failure, which teaches exactly the wrong lesson; the `output_not_matches: FAILED` beside it is what actually guards these. Not done, and commented in ci.yml where the next person will look: widening the fmt gate to `cargo fmt --all --check`. The base carries ~160 fmt-dirty sites across the workspace and the existing src-tauri-only check is already failing on dev. That reformat belongs in its own commit, before the flag. --- .github/workflows/ci.yml | 67 ++++++++++++++++++++++ scripts/eval/specs/14-recording.yaml | 10 +++- src-tauri/src/lib.rs | 3 +- src/engines/recording/src/call_detector.rs | 1 - src/engines/recording/src/library.rs | 10 ++-- src/engines/recording/src/sidecar.rs | 18 +++--- 6 files changed, 90 insertions(+), 19 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 4772c170..651faaf9 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -82,6 +82,13 @@ jobs: with: workspaces: src-tauri + # NOT WIDENED TO `cargo fmt --all --check`, though it should be. The + # engine crates are unchecked today, and widening was scoped into this + # PR — but the base is ~160 fmt-dirty sites across the workspace, and + # this narrower check is ALREADY failing on dev (src-tauri/src/lib.rs + # alone has 11). Widening it here would mean either landing red or + # burying an audio review under a whole-workspace reformat. Do the + # reformat as its own commit, then add --all. - name: Format check run: | if ! cargo fmt --manifest-path src-tauri/Cargo.toml --check; then @@ -109,9 +116,69 @@ jobs: - name: Clippy run: cargo clippy --manifest-path src-tauri/Cargo.toml -- -D warnings + # The engine crates are NOT covered by the step above, which is how the + # recording crate accumulated six clippy errors nobody saw. --all-targets + # or the test-only code is not even compiled. + - name: Clippy (recording engine, tests included) + run: cargo clippy -p brains-recording --all-targets -- -D warnings + - name: Unit tests run: cargo test --manifest-path src-tauri/Cargo.toml + # WHY THIS JOB EXISTS: call recording had never been run on Windows, and the + # reason it could go that long is that nothing here ever compiled for it. + # The bugs that cost a bring-up day were a verbatim resource path, a missing + # process-tree kill, and a wire type in snake_case — the first two are + # Windows-only by construction and the third was invisible everywhere. + # + # It is deliberately NARROW and deliberately BLOCKING. Never + # continue-on-error: PR #12 was publicly corrected for exactly that, a green + # Windows job hiding 14 failing tests. A job that cannot go red is worse + # than no job, because it reads as coverage. + windows: + name: Windows (recording) + runs-on: windows-latest + timeout-minutes: 45 + defaults: + run: + # Every step here is POSIX; the runner's default shell is pwsh. + shell: bash + steps: + - uses: actions/checkout@11d5960a326750d5838078e36cf38b85af677262 # v4.4.0 + + - uses: dtolnay/rust-toolchain@4cda84d5c5c54efe2404f9d843567869ab1699d4 # stable branch + with: + toolchain: stable + components: clippy + + - uses: Swatinem/rust-cache@c19371144df3bb44fab255c43d04cbc2ab54d1c4 # v2.9.1 + with: + workspaces: src-tauri + + # SCOPED TO brains-recording ON PURPOSE, and the scope is the whole + # design of this job. Two things are knowingly excluded: + # + # * --workspace, because src/engines/agents/local/src/sandbox.rs + # asserts real_path("/tmp") == "/private/tmp" with no platform gate, + # so the workspace is red on Windows for reasons that have nothing + # to do with recording. + # * -p brains-desktop --lib, because it pulls brains-storage in and + # that crate has its own pre-existing clippy debt (backups.rs + # sort_by_key, previews.rs needless borrows). + # + # Both are worth fixing and both are their own PR. Widening this job + # before they are fixed would mean landing it red, and a red-on-arrival + # job gets `continue-on-error` bolted on within a week — which is the + # exact failure this job exists to avoid. + - name: Clippy (recording engine, tests included) + run: cargo clippy -p brains-recording --all-targets -- -D warnings + + - name: Unit tests (recording engine) + run: cargo test -p brains-recording + + - name: Build the recording engine + run: cargo build -p brains-recording + eval: name: Eval (token-less) runs-on: macos-latest diff --git a/scripts/eval/specs/14-recording.yaml b/scripts/eval/specs/14-recording.yaml index 0ad80a71..690300a2 100644 --- a/scripts/eval/specs/14-recording.yaml +++ b/scripts/eval/specs/14-recording.yaml @@ -46,14 +46,16 @@ checks: - name: sidecar spawn/handshake/kill-group against a fake binary type: cmd run: cargo test -p brains-recording --test fake_sidecar - output_matches: "4 passed" + # Count-agnostic. A pinned count makes ADDING A TEST a spec failure, which + # teaches the wrong lesson; "FAILED" below is what actually guards this. + output_matches: "[1-9][0-9]* passed" output_not_matches: "FAILED" timeout_ms: 300000 - name: the Node sidecar against a fake Recall SDK type: cmd run: npm run test:recall - output_matches: "pass 4" + output_matches: "pass [1-9][0-9]*" output_not_matches: "fail [1-9]" timeout_ms: 300000 @@ -71,7 +73,9 @@ checks: run: >- npx vitest run src/layout/core/frame/__tests__/record-pill.test.ts src/layout/panes/sidebar/__tests__/recording-library.test.ts - output_matches: "Tests 1[0-9] passed" + # Was "Tests 1[0-9] passed", which had ALREADY BEEN BROKEN on dev: the + # two mounted files hold 21 tests, and 21 does not match 1[0-9]. + output_matches: "Tests [1-9][0-9]* passed" output_not_matches: "failed" timeout_ms: 300000 diff --git a/src-tauri/src/lib.rs b/src-tauri/src/lib.rs index 19f834e8..1dfcf9b8 100644 --- a/src-tauri/src/lib.rs +++ b/src-tauri/src/lib.rs @@ -55,7 +55,7 @@ use brains_native::{BrainsClient, SseSupervisor}; use brains_context::ContextEngine; use brains_local_agents::{ActorRunner, LocalAgents, SchedulerConfig}; use brains_model::{CancelToken, EventSink, SessionRegistry}; -use brains_recording::{CallDetector, CallSnooze, DetectorConfig, MicAuthStatus, Recorder}; +use brains_recording::{CallSnooze, MicAuthStatus, Recorder}; use brains_storage::{EventWriter, GuardOutcome, Settings, Storage}; use tauri::{Emitter, Manager, WindowEvent}; @@ -528,6 +528,7 @@ pub fn run() { // non-macOS gets None and the commands gracefully no-op. #[cfg(target_os = "macos")] let (call_snooze, mic_auth_status) = { + use brains_recording::{CallDetector, DetectorConfig}; let (detector, snooze, mic_auth) = CallDetector::start(DetectorConfig::default()); // Mic auth gate: the poll loop skips external_mic_users() until // mic permission is confirmed. Check AVCaptureDevice status at diff --git a/src/engines/recording/src/call_detector.rs b/src/engines/recording/src/call_detector.rs index f1736145..80909cfb 100644 --- a/src/engines/recording/src/call_detector.rs +++ b/src/engines/recording/src/call_detector.rs @@ -568,7 +568,6 @@ mod tests { #[test] fn cooldown_prevents_prompt() { - let config = Config::default(); let state = DetectorState { in_call: true, snoozed: false, diff --git a/src/engines/recording/src/library.rs b/src/engines/recording/src/library.rs index 213b50d3..dbe908da 100644 --- a/src/engines/recording/src/library.rs +++ b/src/engines/recording/src/library.rs @@ -141,7 +141,7 @@ pub fn write_index(recordings_dir: &Path, index: &Index) -> Result<()> { let path = index_path(recordings_dir); let tmp = recordings_dir.join(".index.json.tmp"); let json = serde_json::to_string_pretty(index).map_err(|e| { - Error::io(&path, std::io::Error::new(std::io::ErrorKind::Other, e)) + Error::io(&path, std::io::Error::other(e)) })?; std::fs::write(&tmp, json).map_err(|e| Error::io(&tmp, e))?; std::fs::rename(&tmp, &path).map_err(|e| Error::io(&path, e)) @@ -271,7 +271,7 @@ pub fn write_brains_sync( meeting, }; let json = serde_json::to_string_pretty(&sync).map_err(|e| { - Error::io(&path, std::io::Error::new(std::io::ErrorKind::Other, e)) + Error::io(&path, std::io::Error::other(e)) })?; std::fs::write(&path, json).map_err(|e| Error::io(&path, e)) } @@ -292,7 +292,7 @@ pub fn update_brains_meeting(wav_path: &Path, meeting: RecordingMeeting) -> Resu })?; sync.meeting = Some(meeting); let json = serde_json::to_string_pretty(&sync).map_err(|e| { - Error::io(&path, std::io::Error::new(std::io::ErrorKind::Other, e)) + Error::io(&path, std::io::Error::other(e)) })?; std::fs::write(&path, json).map_err(|e| Error::io(&path, e)) } @@ -674,7 +674,7 @@ mod tests { fn flat_band(windows: usize, floor: i16, ceiling: i16) -> Vec { (0..windows * WINDOW_SAMPLES) .map(|i| { - if (i / WINDOW_SAMPLES) % 2 == 0 { + if (i / WINDOW_SAMPLES).is_multiple_of(2) { floor } else { ceiling @@ -714,7 +714,7 @@ mod tests { assert!( !speech.flat_input, "a voice moves — spread was {}", - spread_db((SILENCE_PEAK + 2) as i32, 3000) + spread_db(SILENCE_PEAK + 2, 3000) ); } diff --git a/src/engines/recording/src/sidecar.rs b/src/engines/recording/src/sidecar.rs index f8fbcf2b..663f0599 100644 --- a/src/engines/recording/src/sidecar.rs +++ b/src/engines/recording/src/sidecar.rs @@ -308,6 +308,15 @@ mod job { } } + impl Drop for Job { + fn drop(&mut self) { + // The kill: this is the last handle, so the OS terminates every + // process still in the job. + // SAFETY: the handle came from CreateJobObjectW and is closed once. + unsafe { CloseHandle(self.0) }; + } + } + #[cfg(test)] mod tests { use super::*; @@ -371,13 +380,4 @@ mod job { } } } - - impl Drop for Job { - fn drop(&mut self) { - // The kill: this is the last handle, so the OS terminates every - // process still in the job. - // SAFETY: the handle came from CreateJobObjectW and is closed once. - unsafe { CloseHandle(self.0) }; - } - } } From 382fd822ff6067fbe3010b14858e593919d2b89c Mon Sep 17 00:00:00 2001 From: Amir SSV Labs Date: Mon, 10 Aug 2026 18:30:20 +0300 Subject: [PATCH 12/16] docs(recording): what the detector catches, and what it still cannot MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The engine README gains two sections and loses a stale one; the fix audit's "still N/A" paragraph is replaced by what is closed and what is not. The README now carries the measured table the thresholds were derived from, where the verdict is taken (finalize reads the file, live reads a ring — and why neither is in the sidecar), the conjunctive rule in full, and the WASAPI sequence with the two things a rewrite drops: the S_FALSE balance, and the `PropVariantClear` guard that must NOT be ported because windows-rs 0.61's PROPVARIANT already implements Drop. The four limits are stated as limits rather than left implicit: * `FLAT_CEILING_PEAK` was reasoned to, not measured — it is the one constant with no negative case behind it. * No default-mic watcher: a default wrong at capture start is caught, one that changes mid-call is not. The README's own claim that "no race is possible" holds BECAUSE there is no device thread — porting one makes it false. * No call-app device source on Windows, so the mismatch warning cannot fire there at all. The DEFAULT device is named; the CALL APP's is not. * The mid-capture refresh is unarmed on purpose, pending one verified macOS run. The "Windows: still not ported" list from b16ea90 said all three gaps were the same missing piece. Two of the three are now closed, so it says so instead of continuing to describe the app as it was three commits ago. --- scripts/eval/RECORDING-FIX-AUDIT.md | 38 +++++++- src/engines/recording/README.md | 130 +++++++++++++++++++++++++--- 2 files changed, 152 insertions(+), 16 deletions(-) diff --git a/scripts/eval/RECORDING-FIX-AUDIT.md b/scripts/eval/RECORDING-FIX-AUDIT.md index 5a06a76c..efcf9fb1 100644 --- a/scripts/eval/RECORDING-FIX-AUDIT.md +++ b/scripts/eval/RECORDING-FIX-AUDIT.md @@ -62,9 +62,41 @@ So the Windows gap is not capture — it is that NOTHING PROBES THE DEVICE (see the engine README's "Not ported" list). Level alone cannot catch a boosted dead input; variance can, and `window_peak` is already tracked. -The three still marked N/A -(default-mic watcher, the watcher race it introduces, ComGuard) all belong to -the device-monitoring thread this wave does not have on either platform. +### Closed since the paragraph above was written + +- **The dead input itself.** `library.rs::assess` computes an envelope over 40 + windows and calls an input flat when its spread is under 6 dB inside a level + band; `client/guard.ts::shouldWarnFlatInput` does the live half off the + window peaks. Badge `NO INPUT`, pill `⚠ No input`, and BOTH billing doors — + the post-stop auto-fetch and `canFetchTranscript` — now refuse it. The + thresholds and their derivation are in the engine README. +- **Naming the device.** `input_device.rs` reads the WASAPI default capture + endpoint's friendly name, and the no-audio warning names it instead of saying + "check the input device". `ComGuard` came with it — the S_FALSE balance the + `c2ad67b` row is about. v1's `PropVariantClear` guard was deliberately NOT + ported and must not be: windows-rs 0.61's `PROPVARIANT` implements `Drop` as + `PropVariantClear`, so a guard would be a double free. +- **No Windows CI leg.** `ci.yml` gains a blocking `windows` job (clippy + `--all-targets`, tests, build) scoped to `brains-recording`. Clippy also now + reaches the engine crates on the macOS leg, which immediately found six + errors in this crate that nothing had ever gated. + +### Still open + +- **The default-mic watcher** (`d510348`) and **the watcher race it introduces** + (`b73bb0f`). Both belong to the device-monitoring thread that exists on + neither platform. Naming the device delivers most of the value at none of the + risk: a user who changes the default mid-session sees the pill still naming + the old device, rather than a background thread racing a live capture. +- **`micDeviceMismatch` on Windows.** The DEFAULT device is named there now; + the CALL APP's is not (`call_mic.rs` is macOS-only), so the comparison still + cannot fire at all. +- **Arming `recording_refresh_audio`.** It could never have fired on any + platform before the `AudioStatus` casing fix, so arming it would be a + first-ever run in the field — on macOS, unverified. Needs one verified macOS + run first. +- **`FLAT_CEILING_PEAK`.** The loud-steady-tone exemption, reasoned to rather + than measured. Capture hold music at the next bring-up and re-derive it. ## Fixes Implemented diff --git a/src/engines/recording/README.md b/src/engines/recording/README.md index be268a27..22417e0c 100644 --- a/src/engines/recording/README.md +++ b/src/engines/recording/README.md @@ -215,6 +215,103 @@ Stop; the WAV is saved locally, uploaded to Recall, transcribed after the call, and handed to a conversation via `transcriptHandoff`. This is the capture/ transcription loop — a working microphone → a readable transcript. +## The dead-input detector — what it catches, and what it still cannot + +`SILENCE_PEAK` catches an input feeding zeros. It cannot catch a **disconnected +input with gain on it**, and that is the case that cost a bring-up day. A +disconnected USB line-in at +30 dB boost, over twelve seconds of loud speech: + +| capture | peak | RMS | envelope spread | +|---|---|---|---| +| default input, disconnected, +30 dB | −34.9 dB | −48.1 dB | **1.4 dB** | +| the same device idle, nobody speaking | −35.9 dB | −48.3 dB | — | +| after switching the default input | **−11.5 dB** | −29.2 dB | **29.5 dB** | + +The first two rows agree to within 1 dB. Level cannot separate speech from that +device's own hiss; **variance can** — speech moves, hiss does not. So the +verdict is how far the loudest envelope window sits above the quietest, in dB. + +It is measured in **two places, split by job**, and deliberately *not* in the +sidecar — that would mean a protocol change, policy in JS, and accumulators +that die with a discarded sidecar: + +- **Finalize**, `library.rs::assess` — the durable verdict that gates + transcription and filing. It reads the finished file, so it depends on + neither the UI's poll nor the sidecar's liveness, and it is the same path the + index self-heal takes, so a fresh capture and a legacy one agree. +- **Live**, `client/guard.ts::shouldWarnFlatInput` + the store — the ~60 s + warning, off the window peaks the store already receives once a second. + Judged on a **ring**, never on `status.peak`: a cumulative max never falls, + so one blip at capture start would suppress the warning for the whole call. + +The verdict is conjunctive with a level band, never variance alone: + +``` +flat_input ⟺ windows ≥ FLAT_MIN_WINDOWS (40) ∧ floor > 0 + ∧ SILENCE_PEAK < ceiling < FLAT_CEILING_PEAK (3277, −20 dBFS) + ∧ spread_db(floor, ceiling) < FLAT_SPREAD_DB (6.0) +``` + +`FLAT_SPREAD_DB = 6` is 4.3× the measured dead spread and about a fifth of the +measured speech spread; nothing has been observed between 1.4 and 29.5 dB. +The constants are mirrored in `client/guard.ts` — **change both or neither**. + +`has_audio` keeps its meaning and `flat_input` is a **sibling**, because a +boosted dead input is not silent and a `SILENT` badge would send someone +hunting for a muted microphone instead of a disconnected one. The badge is +`NO INPUT` and the pill reads `⚠ No input`. + +### What it still cannot do + +- **`FLAT_CEILING_PEAK` has no measured negative case behind it.** It is the + loud-steady-tone exemption — hold music, a test tone — and it was reasoned + to, not measured. Capture a hold-music sample at the next bring-up and + re-derive it. +- **No default-mic watcher.** A default that is wrong *at capture start* is + caught; one that changes *mid-call* is not, and the pill keeps naming the + device it started with. A watcher means a background device thread and the + TOCTOU race that comes with it — deliberately deferred. +- **No call-app device source on Windows.** `call_app_device` is macOS-only, so + the mic-*mismatch* warning cannot fire on Windows at all. The dead-input + detector and the device name both work there; the comparison does not. +- **The mid-capture refresh is deliberately unarmed.** `recording_refresh_audio` + pauses and resumes a live capture. Until `AudioStatus`'s wire casing was + fixed it could never have fired on any platform, so arming it now would be + its first-ever run in the field — on macOS, unverified. The mismatch surfaces + as a sentence naming both devices instead; arm it after one verified macOS + run. + +## Naming the input device (Windows) + +`input_device.rs` answers "which microphone is brains actually recording from?" +via WASAPI: `GetDefaultAudioEndpoint(eCapture, eConsole)` → +`PKEY_Device_FriendlyName`. `eConsole`, not `eCommunications` — that is the +role the Sound settings picker writes and the role the SDK binds. + +The warning it makes possible is the whole point. It used to read *"No audio is +reaching brains — check the microphone permission or input device."* It now +names the device to go and fix. + +It never breaks recording: `Option`, every step `.ok()?`, and no +`unwrap`/`expect`/`panic!` in the file. A machine with no capture endpoint — +every CI runner — answers `None` and the warnings fall back to their +device-less wording. + +**The endpoint ID never leaves the process.** It is a stable per-device +identifier; the friendly name is what gets shown and logged. + +Two things a rewrite tends to drop, both deliberate: + +- `ComGuard` balances `CoInitializeEx` **including when it returned `S_FALSE`** + (already initialized on this thread — still a per-thread reference that has + to be released). +- **No manual `PropVariantClear`.** windows-rs 0.61's `PROPVARIANT` implements + `Drop` as `PropVariantClear`; adding a guard would be a double free. + +The deciding logic (`display_name`, `state_word`, `is_suspect`) is +platform-free so it is covered on every CI leg — a Windows-cfg test cannot go +red in a way anyone sees. + ## Not ported from the reference These features exist in the reference app (`brains-desktop`) and are @@ -246,16 +343,23 @@ intentionally NOT ported in this wave: found is in `scripts/eval/RECORDING-FIX-AUDIT.md`; the build could not stage its assets at all before it (`scripts/dev/recall/host.mjs`). - Still not ported, and all three are the same missing piece — nothing probes - the input device on Windows: - - No default-microphone watcher. The sidecar binds the system default at - `prepare`, so changing it mid-session is ignored until the app restarts. - - `micDeviceMismatch` has no probe feeding it (`call_mic.rs` is macOS-only), - so a capture bound to the wrong device looks identical to a good one. - - A DEAD input passes the silence gate. A disconnected line-in at +30 dB - boost reads about −35 dBFS — comfortably above `SILENCE_PEAK` — and the - first Windows run recorded 21 seconds of it across two takes, marked both - `hasAudio: true`, uploaded both and paid for two transcript jobs. The tell - is that the level never MOVED: 1.4 dB of envelope spread against 29.5 dB - for real speech on the same machine. `recorder.rs` already tracks - `window_peak`, so variance is measurable without any new probe. + That run found three gaps, all the same missing piece — nothing probed the + input device on Windows. **Two are now closed:** + - ~~A DEAD input passes the silence gate.~~ **Closed.** A disconnected + line-in at +30 dB boost reads about −35 dBFS, comfortably above + `SILENCE_PEAK`, and the first Windows run recorded 21 seconds of it across + two takes, marked both `hasAudio: true`, uploaded both and paid for two + transcript jobs. The tell was that the level never MOVED. See + "The dead-input detector" above. + - ~~Nothing names the device.~~ **Closed.** See "Naming the input device + (Windows)" above. + - **No default-microphone watcher — still open.** The sidecar binds the + system default at `prepare`, so changing it mid-session is ignored until + the app restarts, and the pill keeps naming the device the capture started + with. Note that the README's claim elsewhere that "no race is possible" + holds *because* there is no background device thread; porting a watcher + makes it false, which is why it is its own change. + - **`micDeviceMismatch` still has no probe on Windows.** `call_mic.rs` is + macOS-only, so `call_app_device` is always `None` there and the comparison + cannot fire. The DEFAULT device is now named on Windows; the CALL APP's is + not. From 8e10c8cfcbb795f31bf2a2f8438e9a9eae27baf7 Mon Sep 17 00:00:00 2001 From: Amir SSV Labs Date: Mon, 10 Aug 2026 18:36:32 +0300 Subject: [PATCH 13/16] fix(recording): a NO INPUT row should look like the failure it is MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `transcriptBadge` gained NO INPUT, and the rail rendered it — in the neutral badge style, with an un-struck microphone glyph and a row tooltip offering to fetch the transcript that the flat-input gate had just refused to fetch. A dead input is the same CLASS of failure as silence: nothing usable was captured, no transcript will be fetched, and nothing will be filed. So it gets the same treatment SILENT gets — the struck-through glyph and the warning badge colour — and its own tooltip saying what happened and that no transcript was fetched. Only the WORD stays different, which was the point of having a second badge at all: "SILENT" sends someone hunting for a muted microphone, and this device was neither muted nor silent. --- src/layout/panes/sidebar/RecordingList.svelte | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/src/layout/panes/sidebar/RecordingList.svelte b/src/layout/panes/sidebar/RecordingList.svelte index d23df3c6..f718b4ca 100644 --- a/src/layout/panes/sidebar/RecordingList.svelte +++ b/src/layout/panes/sidebar/RecordingList.svelte @@ -106,11 +106,13 @@ class:active={row.path === recording.detailPath} type="button" onclick={() => recording.open(row.path)} - title={row.hasAudio - ? "Open this recording — replay it, or fetch its transcript" - : "No audio was captured in this recording"} + title={!row.hasAudio + ? "No audio was captured in this recording" + : row.flatInput + ? "The input device produced a level that never changed — probably disconnected or muted. No transcript was fetched." + : "Open this recording — replay it, or fetch its transcript"} > - + - {#if !row.hasAudio}{/if} + {#if !row.hasAudio || row.flatInput}{/if} {row.when} · {row.len} {#if hasUncoupledMeeting} ATTACH? {:else if badge} - {badge} {/if} From 72527d796e31e0d7bb614b2f8bca997850880c32 Mon Sep 17 00:00:00 2001 From: Amir SSV Labs Date: Mon, 10 Aug 2026 19:30:53 +0300 Subject: [PATCH 14/16] fix(recording): satisfy clippy's unnecessary_sort_by on the newest-first sort MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The Windows job went red on its first run, which is exactly what it is for. `list_paged`'s newest-first sort tripped `clippy::unnecessary_sort_by` — a pre-existing line, newly gated because this PR points clippy at the engine crates for the first time. `sort_by_key(Reverse(created_ms))` says the same thing, and the newest-first test is unchanged and still passes. WHY IT DID NOT REPRODUCE LOCALLY: CI takes `dtolnay/rust-toolchain@stable`, which is floating, and stable had moved to 1.97.1 while this machine was on 1.93.0. The lint does not fire on 1.93. Verified after `rustup update stable`, so local and CI now agree — clippy 1.97.1, zero errors on `-p brains-recording --all-targets`. Worth knowing about the new job: a floating toolchain means a future stable can turn it red without anything in this repo changing. That is the cost of not pinning, and it is the right trade for a leg whose whole purpose is to notice things nobody looked at — but the first person it surprises should not have to rediscover why. --- src/engines/recording/src/library.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/engines/recording/src/library.rs b/src/engines/recording/src/library.rs index dbe908da..dc1e8601 100644 --- a/src/engines/recording/src/library.rs +++ b/src/engines/recording/src/library.rs @@ -590,7 +590,7 @@ pub fn list_paged(recordings_dir: &Path, options: ListOptions) -> Result 0 || options.limit.is_some() { From c6081ab75623d4b2b77db3a0ed621872a8ddda2a Mon Sep 17 00:00:00 2001 From: Amir SSV Labs Date: Wed, 12 Aug 2026 15:49:02 +0300 Subject: [PATCH 15/16] docs(resources): say that these regression tests run nowhere in CI MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review catch (@sebastian-ssvlabs): the two `#[cfg(windows)]` tests in `resources.rs` are the regression coverage for the verbatim-path bug in bbc23a8, and they execute NOWHERE in CI. The macOS leg compiles them to nothing; the `windows` leg is scoped to `-p brains-recording` and never builds this crate. That is the same "never ran off-macOS" class this PR is otherwise about, one crate over, and it should not be left for a reader to discover. The gate itself is correct and stays: `dunce::simplified` is the identity function off Windows, so these assertions cannot hold there, and rewriting them platform-free would assert against a reimplementation of dunce instead of the code that ships — a test that passes without proving anything. So the note says what is true, at the tests themselves: they run nowhere in CI, what would make them run (the Windows job compiling this crate, once `browser_host.rs` stops calling macOS-only `ns_window()` ungated), and that today the breakage means even a by-hand `cargo test -p brains-desktop resources` cannot run on Windows. Both were re-checked green on Windows 11 for this commit by cfg-gating that one `ns_window()` call locally and reverting it — which is also the evidence that gating it is the whole of the fix that crate needs. --- src-tauri/src/resources.rs | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/src-tauri/src/resources.rs b/src-tauri/src/resources.rs index 7aac2581..d15dcf95 100644 --- a/src-tauri/src/resources.rs +++ b/src-tauri/src/resources.rs @@ -50,6 +50,26 @@ mod tests { assert_eq!(simplified(None), None); } + // THE TWO TESTS BELOW DO NOT RUN IN CI TODAY. Say so out loud, because + // they are the regression coverage for the bug this module exists to fix, + // and a gated test that never executes is the same "never ran off-macOS" + // failure this work is otherwise about — just one crate over. + // + // The gate itself is CORRECT and must stay: `dunce::simplified` is the + // identity function off Windows, so these assertions cannot hold there. + // Rewriting them to be platform-free would mean asserting against a + // reimplementation of dunce rather than against the code that ships. + // + // What would make them run: the `windows` CI job compiling THIS crate. + // It is scoped to `-p brains-recording` because `brains-desktop` does not + // build for Windows at all — `browser_host.rs` calls the macOS-only + // `ns_window()` with no cfg gate. Widen the job the moment that is fixed. + // + // Note that the same breakage means even `cargo test -p brains-desktop + // resources` cannot be run on Windows today. Both tests were last checked + // green on Windows 11 by cfg-gating that one `ns_window()` call locally — + // which is also the evidence that gating it is all the fix needs. + /// The bug itself: the prefix Tauri adds and Node cannot read. #[cfg(windows)] #[test] From acb525822018c260e31e2a4055b799f212f97724 Mon Sep 17 00:00:00 2001 From: Amir SSV Labs Date: Wed, 12 Aug 2026 21:20:38 +0300 Subject: [PATCH 16/16] chore(recording): drop two helpers nothing calls, and comments that narrate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Hygiene pass over this branch's own diff. DEAD CODE. `input_device::state_word` and `is_suspect` were defined, tested, and called from nowhere — proven by a repo-wide search, the only references being their own definitions and their own tests. They were written so the module would have "platform-free deciding logic" exercised on every CI leg, but two of the three deciding functions decided nothing; only `display_name` has a caller. Tests over uncalled code make coverage look better than it is, which is the opposite of what that seam was for. Removed with their tests, and the module header no longer claims all three. COMMENTS. Three that narrated provenance rather than describing the code: the ComGuard doc's aside about what a rewrite tends to drop, the store's "until this commit the backend answered in snake_case", and the Windows job's citation of PR #12. The requirement, the warning and the never-continue-on-error rule all survive; only the history goes. The measured evidence behind the detector's constants stays exactly where it is — that is the reason those numbers are trustworthy, not narration. Verified: 63 tests (was 65, minus the two that tested the removed helpers), clippy `--all-targets -D warnings` clean, `cargo fmt --check` clean, the recording client suites 87 passed, and `npm run check` / `lint:size` / `lint:imports` / `lint:css-vars` each exit 0. --- .github/workflows/ci.yml | 5 +- src/engines/recording/src/input_device.rs | 66 ++----------------- .../core/runtime/stores/recording.svelte.ts | 16 ++--- 3 files changed, 12 insertions(+), 75 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 21b24601..052ef724 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -140,9 +140,8 @@ jobs: # Windows-only by construction and the third was invisible everywhere. # # It is deliberately NARROW and deliberately BLOCKING. Never - # continue-on-error: PR #12 was publicly corrected for exactly that, a green - # Windows job hiding 14 failing tests. A job that cannot go red is worse - # than no job, because it reads as coverage. + # continue-on-error: a job that cannot go red is worse than no job, because + # it reads as coverage — a green Windows job here once hid 14 failing tests. windows: name: Windows (recording) runs-on: windows-latest diff --git a/src/engines/recording/src/input_device.rs b/src/engines/recording/src/input_device.rs index 58e6140b..39249c66 100644 --- a/src/engines/recording/src/input_device.rs +++ b/src/engines/recording/src/input_device.rs @@ -20,11 +20,9 @@ // written beside the WAV and logged once per capture. The endpoint ID is // a stable per-device identifier; v1 refused to log it and that judgement // stands. It exists here only as a future watcher's comparison key. -// 3. THE DECIDING LOGIC IS PLATFORM-FREE. `display_name`, `state_word` and -// `is_suspect` are ordinary functions with ordinary tests that run on -// every CI leg. v1's lesson was that a Windows-cfg test "cannot go red in -// a way anyone sees" — and until the CI leg in the next commit, there was -// no Windows leg here at all. +// 3. THE DECIDING LOGIC IS PLATFORM-FREE. `display_name` is an ordinary +// function with ordinary tests that run on every CI leg, because a +// Windows-cfg test cannot go red in a way anyone sees. /// How long a device name may be before the UI would rather elide it. Long /// enough for "Microphone (3- USB Audio Device)"; short enough that a @@ -52,43 +50,6 @@ pub fn display_name(raw: Option<&str>) -> Option { Some(format!("{cut}…")) } -/// The `DEVICE_STATE` bits, in words a person can read. -/// -/// `unplugged` is the interesting one and the reason this exists: a jack with -/// nothing in it still enumerates, still binds, and still feeds a level when -/// the input has gain on it. -pub fn state_word(state: u32) -> &'static str { - match state { - 0x1 => "active", - 0x2 => "disabled", - 0x4 => "not present", - 0x8 => "unplugged", - _ => "unknown", - } -} - -/// Is this the KIND of input that is usually not a microphone at all? -/// -/// A line-in, a "Stereo Mix", or a virtual cable will happily be the system -/// default and will happily record nothing (or, boosted, its own hiss — the -/// case that started all of this). This does not decide anything; it lets the -/// warning add a sentence when the name itself is the clue. -pub fn is_suspect(name: &str) -> bool { - let lowered = name.to_ascii_lowercase(); - [ - "line in", - "line-in", - "stereo mix", - "what u hear", - "virtual", - "vb-audio", - "cable output", - "aux", - ] - .iter() - .any(|needle| lowered.contains(needle)) -} - /// The name of the input device the system would record from right now, if /// this platform can say. `None` everywhere except Windows, and `None` on /// Windows when there is no default capture endpoint. @@ -125,8 +86,7 @@ mod imp { /// still a successful initialization that took a reference on the /// per-thread apartment. Treating it as "someone else owns this, skip the /// uninit" leaks a reference every call, and this runs once per capture - /// for the life of the process. v1 got this right; it is repeated here - /// because it is the kind of thing a rewrite quietly drops. + /// for the life of the process. struct ComGuard; impl ComGuard { @@ -221,24 +181,6 @@ mod tests { ); } - #[test] - fn device_states_read_as_words_and_an_unknown_bit_is_not_a_panic() { - assert_eq!(state_word(0x1), "active"); - // The state that started this: a jack with nothing in it still - // enumerates, still binds, and still feeds a level when boosted. - assert_eq!(state_word(0x8), "unplugged"); - assert_eq!(state_word(0xdead), "unknown"); - } - - #[test] - fn inputs_that_are_usually_not_microphones_are_flagged() { - assert!(is_suspect("Line In (3- USB Audio Device)")); - assert!(is_suspect("Stereo Mix (Realtek(R) Audio)")); - assert!(is_suspect("CABLE Output (VB-Audio Virtual Cable)")); - assert!(!is_suspect("Microphone Array (Intel® Smart Sound)")); - assert!(!is_suspect("Headset (AirPods Pro)")); - } - /// Behind the Windows cfg, only what is actually assertable without /// knowing the machine: the query is stable across calls, and it survives /// a short-lived thread — which is the S_FALSE balance in `ComGuard` diff --git a/src/layout/core/runtime/stores/recording.svelte.ts b/src/layout/core/runtime/stores/recording.svelte.ts index e213c3bf..e88c3dcb 100644 --- a/src/layout/core/runtime/stores/recording.svelte.ts +++ b/src/layout/core/runtime/stores/recording.svelte.ts @@ -909,16 +909,12 @@ export class RecordingStore { } // Two consecutive observations, then warn — and ONLY warn. // - // `recording_refresh_audio` pauses and resumes a live capture - // (src/engines/recording/sidecar/index.cjs). Until this commit the - // backend answered in snake_case, so `defaultDevice` and - // `callAppDevice` were both `undefined`, `micDeviceMismatch` was - // never true, and that call has therefore never run in the field on - // any platform. Fixing the casing arms it for the first time — on - // macOS, where nobody here has a machine to verify a live - // pause/resume on. So this stops at the sentence and the repair is - // the user's: telling them WHICH two devices disagree is the part - // that was missing anyway. Arm it after one verified macOS run. + // WARN ONLY. `recording_refresh_audio` pauses and resumes a live + // capture (src/engines/recording/sidecar/index.cjs), and it has never + // run in the field on any platform — so arming it would be its first + // real run, on macOS, unverified. Naming the two devices that + // disagree is the part a user can act on anyway. Arm it after one + // verified macOS run. if (this.#mismatchCount === 2 && !this.micMismatch) { this.micMismatch = true; this.micDevices = {