diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index e88ba48c..052ef724 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -79,6 +79,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 @@ -103,6 +110,12 @@ 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 # THE WHOLE WORKSPACE, not just the shell crate. `--manifest-path` at a workspace # member selects that package alone, so every engine's tests — `brains-browser`'s @@ -120,6 +133,59 @@ jobs: # it type-checks the code against the pinned bindings, which is the part that rots. run: cargo check -p brains-browser --features chromium + # 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: 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 + 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/Cargo.lock b/Cargo.lock index 84a220b5..cad1c53a 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -473,6 +473,7 @@ dependencies = [ "brains-recording", "brains-storage", "chrono", + "dunce", "log", "once_cell", "serde", @@ -555,6 +556,8 @@ dependencies = [ "tar", "tempfile", "thiserror 2.0.19", + "windows", + "windows-sys 0.59.0", ] [[package]] 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 70ad732f..2f247a6f 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 { existsSync, readFileSync } from "node:fs"; import { copyFile, mkdir, rename, rm, writeFile } from "node:fs/promises"; import { basename, dirname, 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"); @@ -66,15 +66,20 @@ await rm(temporaryArchive, { force: true }); // framework rewrites the symlinks its signature depends on, and Apple rejects // what comes out — measured 2026-08-12. if (!existsSync(join(sdkDir, "node_modules"))) { - execFileSync( - "npm", - ["install", "--omit=dev", "--ignore-scripts", "--no-audit", "--no-fund", "--no-package-lock"], - { cwd: sdkDir, stdio: "inherit" }, - ); + // `npm` is `npm.cmd` on Windows and a .cmd needs a shell — see host.mjs. + npm(sdkDir, [ + "install", + "--omit=dev", + "--ignore-scripts", + "--no-audit", + "--no-fund", + "--no-package-lock", + ]); } -execFileSync("tar", ["-czf", temporaryArchive, "-C", dirname(sdkDir), basename(sdkDir)], { - stdio: "inherit", -}); +// Through host.mjs: GNU tar (first on PATH under Git for Windows) reads the +// `C:` in an absolute path as a hostname and aborts, so every path goes +// relative to an explicit cwd. A no-op on macOS and Linux. +tar(root, ["-czf", temporaryArchive, "-C", dirname(sdkDir), basename(sdkDir)]); 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 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/specs/14-recording.yaml b/scripts/eval/specs/14-recording.yaml index 16880a0d..04548d21 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 diff --git a/src-tauri/Cargo.toml b/src-tauri/Cargo.toml index f0c2ac8a..3eabfaa6 100644 --- a/src-tauri/Cargo.toml +++ b/src-tauri/Cargo.toml @@ -76,6 +76,10 @@ chrono = { workspace = true } url = { workspace = true } 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/commands/recording.rs b/src-tauri/src/commands/recording.rs index d3496cca..472ad3e1 100644 --- a/src-tauri/src/commands/recording.rs +++ b/src-tauri/src/commands/recording.rs @@ -34,6 +34,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, } @@ -140,6 +144,7 @@ pub async fn recording_stop(recorder: State<'_, RecordingState>) -> CmdResult) -> CmdResult Err(CmdError::new(error)), @@ -436,9 +442,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-tauri/src/lib.rs b/src-tauri/src/lib.rs index fde7566d..2ccff3e3 100644 --- a/src-tauri/src/lib.rs +++ b/src-tauri/src/lib.rs @@ -25,6 +25,8 @@ pub mod overlay_native; pub mod readiness; /// The embedded, token-gated localhost server — the OTHER transport. pub mod remote; +/// 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; @@ -47,7 +49,7 @@ use brains_context::ContextEngine; use brains_local_agents::{ActorRunner, LocalAgents, SchedulerConfig}; use brains_model::{CancelToken, EventSink, SessionRegistry}; use brains_native::{BrainsClient, SseSupervisor}; -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}; @@ -440,7 +442,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 @@ -587,6 +591,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-tauri/src/ops.rs b/src-tauri/src/ops.rs index 3fe818dc..407c565e 100644 --- a/src-tauri/src/ops.rs +++ b/src-tauri/src/ops.rs @@ -82,6 +82,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-tauri/src/resources.rs b/src-tauri/src/resources.rs new file mode 100644 index 00000000..d15dcf95 --- /dev/null +++ b/src-tauri/src/resources.rs @@ -0,0 +1,97 @@ +// 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 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] + 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)); + } +} diff --git a/src/engines/recording/Cargo.toml b/src/engines/recording/Cargo.toml index 7a5ea1ab..2e2031c5 100644 --- a/src/engines/recording/Cargo.toml +++ b/src/engines/recording/Cargo.toml @@ -31,6 +31,37 @@ 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", +] } +# 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. coreaudio-sys = "0.2" diff --git a/src/engines/recording/README.md b/src/engines/recording/README.md index 1e47cb75..8979f41e 100644 --- a/src/engines/recording/README.md +++ b/src/engines/recording/README.md @@ -216,6 +216,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 @@ -242,5 +339,28 @@ 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`). + + 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. 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..5629d8b6 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, flatInputTitle, micMismatchTitle, noAudioTitle, 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.test.ts b/src/engines/recording/client/pill.test.ts index c2568f41..951a2a5e 100644 --- a/src/engines/recording/client/pill.test.ts +++ b/src/engines/recording/client/pill.test.ts @@ -75,6 +75,97 @@ 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. + 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..7800d514 100644 --- a/src/engines/recording/client/pill.ts +++ b/src/engines/recording/client/pill.ts @@ -41,6 +41,12 @@ 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 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). */ @@ -57,6 +63,53 @@ 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. + * + * 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 +134,16 @@ 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 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. */ + micDefaultDevice?: string | null; + /** The call app's input's name, when it is known. */ + micCallAppDevice?: string | null; }): PillView { const { availability, @@ -92,6 +155,11 @@ export function pillView(args: { error, callPrompt, callAppName, + flatInput, + inputDevice, + micMismatch, + micDefaultDevice, + micCallAppDevice, } = args; // Unknown availability (status not loaded yet): disable until we know. @@ -122,14 +190,24 @@ 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 + ? noAudioTitle(inputDevice) + : flatInput + ? flatInputTitle(inputDevice) + : micMismatch + ? micMismatchTitle(micDefaultDevice, micCallAppDevice) + : "Stop recording"; return { state: "recording", label: elapsed(elapsedSec), - title: noAudio - ? "No audio is reaching brains — check the microphone permission or input device. Recording continues." - : "Stop recording", + title, disabled: !!stopping, - warn: !!noAudio, + warn: !!noAudio || !!flatInput, + warnLabel: noAudio ? "⚠ No audio" : flatInput ? "⚠ No input" : undefined, + inputDevice: (inputDevice ?? "").trim() || 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..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; }; @@ -52,6 +56,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 +100,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/call_detector.rs b/src/engines/recording/src/call_detector.rs index 23f9d058..a719b7b0 100644 --- a/src/engines/recording/src/call_detector.rs +++ b/src/engines/recording/src/call_detector.rs @@ -571,7 +571,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/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/input_device.rs b/src/engines/recording/src/input_device.rs new file mode 100644 index 00000000..39249c66 --- /dev/null +++ b/src/engines/recording/src/input_device.rs @@ -0,0 +1,208 @@ +// 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` 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 +/// 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 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. + 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" + ); + } + + /// 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 ae5c552f..43662a3c 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; @@ -38,11 +39,10 @@ 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)] @@ -259,6 +259,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/library.rs b/src/engines/recording/src/library.rs index 02c45437..df48c728 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, @@ -91,6 +152,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<()> { @@ -100,6 +162,7 @@ pub fn write_index_entry( IndexEntry { duration_sec, has_audio, + flat_input, created_ms, source_app: source_app.map(str::to_string), }, @@ -301,51 +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); + 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. @@ -407,28 +545,36 @@ pub fn list_paged(recordings_dir: &Path, options: ListOptions) -> Result Result Vec { + (0..windows * WINDOW_SAMPLES) + .map(|i| { + if (i / WINDOW_SAMPLES).is_multiple_of(2) { + 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, 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(); @@ -715,11 +993,21 @@ mod tests { "call-1-mic.wav", 3.5, true, + false, 1700000000000, Some("Zoom"), ) .unwrap(); - write_index_entry(dir, "call-2-mic.wav", 2.0, false, 1700000001000, None).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); @@ -768,6 +1056,7 @@ mod tests { "call-1-mic.wav", 99.0, false, + false, 1234567890000, Some("FakeApp"), ) @@ -796,7 +1085,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 1c255d93..04dffdab 100644 --- a/src/engines/recording/src/recorder.rs +++ b/src/engines/recording/src/recorder.rs @@ -288,9 +288,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(); - AudioStatus::new(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. @@ -330,6 +344,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()), } } @@ -466,6 +481,18 @@ 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()) @@ -473,13 +500,13 @@ impl Recorder { // 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(), ); @@ -489,6 +516,7 @@ impl Recorder { path: path_str, peak, duration_sec, + flat_input: assessment.flat_input, }) } diff --git a/src/engines/recording/src/sidecar.rs b/src/engines/recording/src/sidecar.rs index bde0ed74..314262b5 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 { @@ -94,6 +102,14 @@ impl Sidecar { 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() @@ -120,6 +136,8 @@ impl Sidecar { stdin: BufWriter::new(stdin), stdout: BufReader::new(stdout), next_id: PROTOCOL_VERSION, + #[cfg(windows)] + job, }) } @@ -203,5 +221,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 + } + } + + 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::*; + 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"); + } + } + } + } } } diff --git a/src/engines/recording/src/status.rs b/src/engines/recording/src/status.rs index 2984e770..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")] @@ -103,6 +108,30 @@ 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 +/// 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)] @@ -122,4 +151,175 @@ 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, + flat_input: false, + 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(), + input_device: Some("Microphone (3- USB Audio)".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, + flat_input: false, + }) + .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, + flat_input: false, + 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/engines/recording/tests/fake_sidecar.rs b/src/engines/recording/tests/fake_sidecar.rs index caff5733..449d1f37 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, } } @@ -192,6 +218,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/engines/recording/tests/real_sidecar_smoke.rs b/src/engines/recording/tests/real_sidecar_smoke.rs new file mode 100644 index 00000000..004c3e77 --- /dev/null +++ b/src/engines/recording/tests/real_sidecar_smoke.rs @@ -0,0 +1,147 @@ +//! 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." + ); + } + } +} diff --git a/src/layout/core/frame/Titlebar.svelte b/src/layout/core/frame/Titlebar.svelte index 71972005..99fae6a8 100644 --- a/src/layout/core/frame/Titlebar.svelte +++ b/src/layout/core/frame/Titlebar.svelte @@ -121,7 +121,12 @@ {pill.label} {#if pill.warn} - ⚠ No audio + {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/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-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 c4f44e22..e88c3dcb 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,10 +131,19 @@ 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. + * 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; + /** 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). */ @@ -170,6 +181,11 @@ export class RecordingStore { error: this.error, 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, }); } @@ -339,6 +355,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; @@ -349,7 +370,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); @@ -758,6 +779,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(() => {}); @@ -822,6 +844,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); @@ -840,9 +863,28 @@ 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. + // 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); } @@ -865,17 +907,20 @@ export class RecordingStore { } else { this.#mismatchCount = 1; } - // Trigger refresh after 2 consecutive observations + // Two consecutive observations, then warn — and ONLY warn. + // + // 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; - 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; @@ -892,7 +937,10 @@ 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; this.#lastAudioStatus = null; this.#mismatchCount = 0; diff --git a/src/layout/panes/sidebar/RecordingList.svelte b/src/layout/panes/sidebar/RecordingList.svelte index 74d46e37..c8f83594 100644 --- a/src/layout/panes/sidebar/RecordingList.svelte +++ b/src/layout/panes/sidebar/RecordingList.svelte @@ -95,11 +95,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} 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, };