Log local TTS volume and alert lifecycle diagnostics - #583
Conversation
Deploying mouseterm with
|
| Latest commit: |
34bbd09
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://2771b0cd.mouseterm.pages.dev |
| Branch Preview URL: | https://alert-diagnostics.mouseterm.pages.dev |
dormouse-bot
left a comment
There was a problem hiding this comment.
Reviewing as a draft — flagging anything that looks worth a quick fix. Mark ready for a full review.
The journal/emitter bounds match the spec exactly (100/200 per second, 512 × 8 KiB, 4 MiB / 24 h rotation, 30-day + 64 MiB − one-file headroom), the redaction path is right (toSpokenText caps at 120 code points, well under the validator's 512, so text can't be silently rejected), and the nlink === 0 reopen for the multi-window case is a nice catch. Seven notes inline; the first is the one I'd fix before this lands.
|
Re-reviewed The two I checked hardest: the loss marker now clears this module's own One consequence of tracing on the way out rather than up front, worth knowing before someone reads a journal rather than worth changing: Per-finding verification
Not run locally: the suite ( |
dormouse-bot
left a comment
There was a problem hiding this comment.
Reviewing as a draft — flagging anything that looks worth a quick fix. Mark ready for a full review.
All seven earlier notes are addressed, and the two structural ones came out well: every dispatchCompletion path now falls through to a single traceDecision, so the mirrored predicate ladder is gone, and the loss marker carries a real envelope (journal:<uuid>, seq >= 1, monotonicMs) that isAlertDiagnostic accepts — the new expect(all.every(isAlertDiagnostic)).toBe(true) pins it. alertDiagnosticsEnabled() gating in both AlertManager.trace and setTerminalActivity removes the discarded-snapshot cost on Pocket, the demo, and unsink'd tests. The Rust move, the sidecar require + provenance comment, and the bundler header index all check out; pnpm lint:specs passes with the new vscode.md sentence.
Three notes inline. The first two are on the shutdown/trace reordering — one of them is my earlier suggestion going one line too far, and I would unwind that half.
|
Re-verified The decision snapshot is the one I cared about most, and the new test picks exactly the case the old code got wrong: a deferred Both hosts now bracket teardown, and the asymmetry is the right one: One consequence worth knowing before someone reads a standalone journal rather than worth changing: the sidecar's lifecycle markers carry the writer identity ( Per-finding verification
Ran locally: |
dormouse-bot
left a comment
There was a problem hiding this comment.
Reviewing as a draft — flagging anything that looks worth a quick fix. Mark ready for a full review.
The three earlier notes are all closed out correctly. traceFields is captured before the switch, so manager.completion now carries the decision's inputs on every branch — the new alert-diagnostics.test.ts case is a good pin for it: attention expires on the 100 ms timer during the first advanceTimersByTime(500), which is what arms the exit and makes eligible (not attended) the right expectation, and the decision/publication pair shows pendingNotification: 'OSC 9' → null across flushDeferredNotification. host.stopping at the top of both hosts' shutdown answers the "clean quit or killed host" question without depending on [deactivate] done ever being reached, and journal.recordLifecycle keeps the journal open across teardown. summarize-alert-log.test.mjs passes against the new journal: filter (ran it directly — it has no node_modules dependency); the vitest suites are on CI.
One note inline, on the standalone side of the new markers.
dormouse-bot
left a comment
There was a problem hiding this comment.
Reviewing as a draft — flagging anything that looks worth a quick fix. Mark ready for a full review.
The gate is complete across all three bundles, and I checked each of the three claims in the new spec bullet against the code rather than the tests. No IPC: the renderer's only route to recordAlertDiagnostic is sink inside alertDiagnostic, so both vscode-adapter.ts and tauri-adapter.ts go quiet. No listeners: observeAlertFocus returns the no-op before touching window, and platform/index.ts calls it only after configureAlertDiagnostics, so ordering is right on an enabled build too. No files, no pruning: append is the sole entry to queue, pump runs only from append, and drain is the only caller of mkdir and prune — so nothing reaches disk, including recordLifecycle, which routes through append rather than around it. build-sidecar-proxy.mjs runs bundle: true, so alert-diagnostics-config.ts is inlined into alert-journal.cjs and the sidecar carries its own baked copy, which is what makes "rebuild the host/sidecar bundles" the accurate instruction. pnpm lint:specs is green with the ratcheted budget.
Two notes inline. One more, on text this PR doesn't touch:
docs/specs/vscode.md → Local alert diagnostics still opens "Must append host decisions and renderer diagnostic messages to the same local journal", and docs/specs/standalone.md says alert diagnostics "live in <app_data_dir>/alert-logs/, written by the Node sidecar through alert_diagnostic". Both read as unconditional present tense, and above the fold that means "the code as it is" — but a default build now creates no directory on either host, and on VS Code initAlertJournal returns before createAlertJournal is even called. Each already points at alert.md for behavior, so the fix is the same scoping clause you added to the alert.md bullet one line up: "when diagnostics are enabled". Outside the diff, so no inline suggestion — happy to push it if you'd rather not hand-edit two specs.
| const windowListener = vi.spyOn(window, 'addEventListener'); | ||
| const documentListener = vi.spyOn(document, 'addEventListener'); |
There was a problem hiding this comment.
This case passes with alertDiagnosticsConfig.enabled = true, so it doesn't pin the rule the spec says it pins.
No sink is configured here — the afterEach above leaves configureAlertDiagnostics('off'), so sink is undefined. alertDiagnosticsEnabled() is alertDiagnosticsConfig.enabled && sink !== undefined, and the second term alone already makes it false. Flip the flag to true and this test still goes green while a real enabled build installs all five listeners.
The other two cases don't have this problem: the first configures a sink explicitly, and the third's gate is !alertDiagnosticsConfig.enabled on its own inside append. Configuring a sink first makes this one isolate the flag the same way:
| const windowListener = vi.spyOn(window, 'addEventListener'); | |
| const documentListener = vi.spyOn(document, 'addEventListener'); | |
| configureAlertDiagnostics('renderer', vi.fn()); | |
| const windowListener = vi.spyOn(window, 'addEventListener'); | |
| const documentListener = vi.spyOn(document, 'addEventListener'); |
| } | ||
|
|
||
| export function configureAlertDiagnostics(label: string, write?: (record: AlertDiagnostic) => void): void { | ||
| sink = alertDiagnosticsConfig.enabled ? write : undefined; |
There was a problem hiding this comment.
This second gate is redundant, and it's the one that can strand you.
sink has exactly one consumer — alertDiagnostic, which already returns on !alertDiagnosticsEnabled(), and that reads the flag live. So capturing it here changes nothing about what gets recorded; it only makes the flag order-dependent: set it after configureAlertDiagnostics has run and sink stays undefined forever, with alertDiagnosticsEnabled() reporting false and no error anywhere. Every diagnostics test already has to set the flag in beforeEach above its configureAlertDiagnostics call for that reason, which is a quiet trap for the next one added.
Dropping it also makes a runtime flip work for anyone collecting a sample without a full rebuild, and the gate test still passes — case 1 asserts sink is never called, which the live check in alertDiagnostic delivers on its own.
| sink = alertDiagnosticsConfig.enabled ? write : undefined; | |
| sink = write; |
Local speech currently leaves no usage record, and an alert heard after a long focus gap cannot be traced back to its manager decision, renderer timer, or speech-engine queue. Add local JSONL diagnostics to both desktop hosts and a daily character-volume report for planning ElevenLabs usage.
Diagnostics default off behind the shared
alertDiagnosticsConfig.enabledsource flag inlib/src/lib/alert-diagnostics-config.ts. Set it totrue, rebuild the desktop frontend and host/sidecar bundles, and restart to collect a sample. The disabled path emits no diagnostic IPC, installs no diagnostic focus listeners, and writes or prunes no log files, including direct lifecycle records.docs/alert-diagnostics-removal.mdlists the removal steps.The journal records the final redacted spoken text, character counts, separate initial/requeue/test attempts, and engine outcomes. Alert traces include dismissal and attention, completion suppression, detector output counters, deferral deadlines, timer lateness, renderer snapshots, and focus/visibility. No PTY bytes, raw commands, or notification bodies are recorded. Bounded queues, rotation, retention, and loss markers keep diagnostics best-effort and separate from alert state.
Usage:
node scripts/summarize-alert-log.mjs <alert-logs-directory>. Standalone writes under its app-data directory; VS Code writes under extension global storage and reports the path in the Dormouse output channel. This adds observability only; ElevenLabs delivery/fallback and fixes to alert behavior are follow-up work.Stacked on #582 (
redact-speech).Validation: full
pnpm test; focused logging tests covering hour-long gaps, rearmed animation deferral, delayed speech starts/timers, redaction, retries, disk failure, permissions and rotation; standalone sidecar bundling;pnpm exec cargo check --manifest-path standalone/src-tauri/Cargo.toml.Default-off validation: 53 targeted tests, including normal speech with no diagnostic emission, no focus listeners, and no journal directory; lib and extension TypeScript checks; spec lint; sidecar bundling.