Skip to content

fix(capture,summary): pad detector cried wolf on a healthy recording; timeout reported as a permissions error - #180

Open
fmasi wants to merge 2 commits into
mainfrom
fix/pad-false-positive
Open

fix(capture,summary): pad detector cried wolf on a healthy recording; timeout reported as a permissions error#180
fmasi wants to merge 2 commits into
mainfrom
fix/pad-false-positive

Conversation

@fmasi

@fmasi fmasi commented Aug 14, 2026

Copy link
Copy Markdown
Owner

DO NOT MERGE until at least one device test call has been made on this build. CI is welcome to run; the merge waits on real-world confirmation. Closes #179, addresses #173.

Both defects were found by device-testing the merged v0.9.0 build on a real 30-minute call (2026-08-11, speaker only, no device changes). Worth separating up front: the capture path behaved correctly — no rate drift, no chipmunk, 48 kHz throughout, and the diagnostics named everything within seconds. Both bugs are in the reporting layer.

1. The pad detector fired on a healthy recording — its first real firing

The user got "Transcription Complete — capture anomalies". The recording was fine:

window zero-sample ratio
0–30 s 97.6%
60–300 s 2.8%
whole file 4.2%

The call audio simply started ~60 s after recording began. The monitor judged at exactly t=30 s: ratio 0.911, padded 27s of 30s — on nothing but leading silence. It cleared the 10% threshold and the 15 s absolute floor added during review specifically to prevent this class of false positive, because leading silence trivially exceeds any absolute floor.

Root cause — a tap property ScreenCaptureKit does not share: the Core Audio tap delivers no buffers at all while the output device is idle. SCK keeps delivering zero-filled buffers (the #86 liveness probe depends on that), which is why nobody anticipated this. Before a call connects there is genuinely nothing to capture, so timelineSilencePad fabricates the whole span.

Starting a recording before joining the call is the ordinary way to use this app, so this fires on the common case — the failure mode that makes a warning worthless (gotcha #62).

Fix — not a bigger threshold, the right distinction: padding before a track has ever delivered a frame is a start offset, not a deficit. Nothing went missing because there was nothing to capture. PadRatioMonitor accumulates nothing until its first dataFrames > 0.

#58 detection is preserved exactly: in a genuine rate drift the device is delivering (a call is connected, so silence still arrives as zero-filled buffers at the wrong rate), so the monitor starts when the frames do. Both cases are pinned — leadingSilenceBeforeTheCallStartsIsNotCorruption and deficitAfterDataStartsStillFires, the latter also asserting leading silence cannot inflate the ratio.

2. A request timeout told the user to check permissions

MeetingSummarizer.runSummary's catch-all assumed any non-SummaryError was a file failure and reported "Couldn't read the transcript or write the summary — check disk space and permissions". URLError isn't a SummaryError, so a -1001 timeout landed there.

That misdirection cost two separate debugging sessions (2026-08-04 and 2026-08-11) chasing a permissions problem that never existed. An error message that misdescribes the cause is worse than a vague one, because it gets acted on.

Now: URLError is caught explicitly (timeout / unreachable / other), and 401-403 becomes SummaryError.authenticationFailed — naming the API key and status, and deliberately omitting the response body, because some servers echo the offending credential back and this text reaches a notification that can be on screen during a shared meeting (the same reason invalidEndpoint is already sanitized). Tests assert both that it says "API key" and that no bearer token or server body can reach it.

The taxonomy now reads: timeout / unreachable / bad key / bad endpoint / server fault / genuine file error — and only the last mentions permissions.

3. The timeout itself (#173)

Measured on this machine, same transcript (500 segments, ~7.7k tokens, 32.6 min):

condition time
Cold — model not loaded 36 s
Warm — model resident 13 s
The real failure 60.5 s → timed out

Idle-cold fits inside even the old 60 s default, so timing, not transcript size, was the risk: auto-summary fires the instant the transcript is written, i.e. exactly when ASR and diarization have finished saturating the ANE/GPU. Cold load plus contention pushed a 36 s job past 60 s.

SummaryConfig.requestTimeoutSeconds now defaults to 600 s and is honoured by both providers' chat requests. A timeout is a ceiling, not a wait — the warm path still returns in 13 s. Prevention (pre-warming) and recovery (retry when the endpoint returns) are tracked separately in #178.

Testing

881 tests across 100 suites. Gotchas #65 and #66.

The device test that matters is the one this PR is waiting on: a call recorded on this build where the audio starts after recording begins — i.e. the exact shape that produced the false positive — confirming no anomaly label appears. A second, stronger check is a Bluetooth/headphones call per the "Rate integrity" checklist, where any label that does appear should be believed.

🤖 Generated with Claude Code

https://claude.ai/code/session_01Bo3kqs5dCMN2mPN2DMkr74

fmasi and others added 2 commits August 11, 2026 12:46
…out reported as a permissions error

Both found by device-testing the merged v0.9.0 build on a real call (2026-08-11).

1. THE PAD DETECTOR CRIED WOLF ON ITS FIRST REAL FIRING

A healthy 30-minute recording was labelled "capture anomalies". Measured on the system track:
97.6% zero samples in the first 30s, 2.8% from 60-300s, 4.2% overall — the call audio simply
started ~60s after recording began. The monitor judged at exactly t=30s: ratio 0.911, 27s padded.
It cleared the 10% threshold AND the 15s absolute floor I added specifically to stop this, because
leading silence trivially exceeds any absolute floor.

The cause is a tap property SCK does not share: the tap delivers NO buffers while the output device
is idle, so before the call connects there is nothing to capture and the padder fabricates the whole
span. Starting the recording before joining is the ordinary way to use this app.

The fix is the right distinction rather than a bigger number: padding before a track has ever
delivered a frame is a START OFFSET, not a deficit. `PadRatioMonitor` now accumulates nothing until
its first `dataFrames > 0`. #58 detection is preserved exactly — in a real rate drift the device IS
delivering (silence still arrives as zero-filled buffers at the wrong rate), so the monitor starts
when the frames do. Both cases are pinned by tests.

2. A REQUEST TIMEOUT TOLD THE USER TO CHECK PERMISSIONS

`runSummary`'s catch-all assumed any non-`SummaryError` was a file failure and reported "check disk
space and permissions". `URLError` isn't a `SummaryError`, so a `-1001` timeout landed there. That
misdirection cost two debugging sessions before the log revealed the real cause both times. URLError
is now caught explicitly with an accurate message.

And the timeout itself (#173): the stock 60s `URLSession` default is the wrong order of magnitude
for a LOCAL model, which accepts instantly then generates for minutes. `SummaryConfig
.requestTimeoutSeconds` now defaults to 600s and is honoured by both providers' chat requests.

Gotchas #65 and #66. 876 tests across 99 suites pass.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Bo3kqs5dCMN2mPN2DMkr74
…dential echo

Follow-on from the misleading-error work. A rejected API key produced
"Summary request failed: HTTP 401: <raw body>" — accurate, unactionable, and it echoed the server's
response body into a user notification. Some servers reflect the offending credential back, and that
text can be on screen during a shared meeting; `invalidEndpoint` is already sanitized for exactly
this reason.

401/403 is now `SummaryError.authenticationFailed`, whose message names the API key and the status
and includes nothing else. Tests pin both halves: that it says "API key", and that no server body or
bearer token can reach it.

Also asserts the three failure kinds (auth / bad endpoint / server error) read differently and that
none of them blames the filesystem — the specific regression that sent two debugging sessions after
a permissions problem that did not exist.

Worth recording why this came up: the 2026-08-11 failure was NOT auth. The `/api/v0/models` probe
returned 200 in 17ms, which proves both reachability and a valid key; the chat request was also
accepted (200) and then produced zero bytes for 60.5s. It was purely generation outrunning the
timeout. But the question was fair precisely because the message gave no way to tell.

881 tests across 100 suites pass.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Bo3kqs5dCMN2mPN2DMkr74
.localizedDescription
#expect(message.contains("model failed to load"))
}
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The suite title says "What the user is TOLD when a summary fails" and the PR narrative is about the wrong message reaching the user — but the URLError paths added to MeetingSummarizer.runSummary (.timedOut, .cannotConnectToHost / .cannotFindHost, and the default fallback) have no tests here.

Those three messages are the literal fix for bug #2. If runSummary's catch order is later reshuffled, or a new SummaryError case accidentally matches before URLError, the old misdirecting message can come back with nothing failing.

Suggested additions — these can be async tests that call runSummary with a stub SummaryProvider that throws the target URLError:

@Test func timeoutYieldsActionableMessage() async {
    let outcome = await MeetingSummarizer.runSummary(
        transcriptPath: URL(fileURLWithPath: "/dev/null"),
        provider: ThrowingProvider(URLError(.timedOut)),
        endpoint: "http://localhost"
    )
    guard case .failed(let msg) = outcome else { Issue.record("expected failed"); return }
    #expect(msg.contains("too long"))
    #expect(!msg.lowercased().contains("disk space"))
    #expect(!msg.lowercased().contains("permissions"))
}

@Test func unreachableHostYieldsActionableMessage() async {
    let outcome = await MeetingSummarizer.runSummary(
        transcriptPath: URL(fileURLWithPath: "/dev/null"),
        provider: ThrowingProvider(URLError(.cannotConnectToHost)),
        endpoint: "http://localhost"
    )
    guard case .failed(let msg) = outcome else { Issue.record("expected failed"); return }
    #expect(msg.lowercased().contains("server") || msg.lowercased().contains("endpoint"))
    #expect(!msg.lowercased().contains("disk space"))
}

(ThrowingProvider is a one-liner conforming to SummaryProvider that just re-throws its stored error — no real network needed.)

if httpResponse.statusCode == 401 || httpResponse.statusCode == 403 {
throw SummaryError.authenticationFailed(status: httpResponse.statusCode)
}
throw SummaryError.requestFailed("HTTP \(httpResponse.statusCode): \(body.prefix(200))")

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: body is decoded on line 116 and then silently discarded for the 401/403 path. It's a local String that never reaches a log or throw in the auth branch, so there's no actual security impact — but it reads as if the credential-containing body is being handled before the guard that prevents it from escaping.

Moving the decode after the auth check makes the intent explicit and avoids the unnecessary allocation on that path:

Suggested change
throw SummaryError.requestFailed("HTTP \(httpResponse.statusCode): \(body.prefix(200))")
// 401/403 has a precise fix and a body that may echo the credential — classify it.
if httpResponse.statusCode == 401 || httpResponse.statusCode == 403 {
throw SummaryError.authenticationFailed(status: httpResponse.statusCode)
}
let body = String(data: data, encoding: .utf8) ?? ""
throw SummaryError.requestFailed("HTTP \(httpResponse.statusCode): \(body.prefix(200))")

@claude

claude Bot commented Aug 14, 2026

Copy link
Copy Markdown

Review summary

Both fixes are correct and well-motivated. The PadRatioMonitor change is the right distinction (start offset vs deficit), the URLError classification is the right taxonomy (timeout / unreachable / bad key / file error), and the new requestTimeoutSeconds default (600 s) matches the measured failure mode exactly. The security reasoning around not echoing the 401/403 body is sound and the tests on SummaryError itself are thorough.

Two things worth addressing before merge:

1. URLError message paths are untested (inline comment on SummaryErrorMessageTests.swift:62)

The SummaryErrorMessageTests suite covers SummaryError values directly but not the MeetingSummarizer.runSummary → URLError → user-facing string path — which is the actual fix for bug #2. The three messages (timedOut, unreachable, default) are tested nowhere. Since the wrong message is the bug, the right message should be a pinned assertion. See the inline for a suggested test shape.

2. request_timeout_seconds not in docs/parameters.md

The new config key joins max_output_tokens, context_length, etc. in SummaryConfig, but the ## Summary table in docs/parameters.md (line 65) doesn't list it. It's the only way a user can tune this without reading the source — especially relevant because the default is a 10× change from URLSession's implicit 60 s.


One nit (inline on OpenAISummaryProvider.swift:116): body is decoded before the 401/403 guard in the OpenAI provider — harmless, since the local String never escapes — but swapping the order makes the intent clearer and avoids a pointless allocation on the auth path. The same decode-before-check pattern exists in LMStudioSummaryProvider (initial request and retry paths) and could be cleaned up consistently, though it's cosmetic.

Everything else looks good: the reset() correctly zeroes hasDeliveredData, the retryRequest path in LMStudioSummaryProvider calls buildRequest and therefore picks up the 600 s timeout, and the deficitAfterDataStartsStillFires arithmetic checks out (ratio ≈ 0.5, well within the 0.45–0.55 assertion).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bug: excessivePadding fires on healthy recordings — leading silence before the call counts as fabricated (fix written, parked on a branch)

1 participant