Apply verified codebase-audit findings - #1699
Merged
Merged
Conversation
Applies the first tranche of verified findings from the codebase audit.
User-visible defects:
- Relocating the capture library without copying no longer deletes queued
failed-meeting rows. Audio outside the current roots that still looks like
archived capture audio (a `<stem>_audio` directory, file still present) is
now counted unavailable instead of rejected as tampering, which also
suppresses the destructive queue rewrite. Genuinely tampered paths (/tmp,
`..` traversal, arbitrary home files) are still rejected.
- A short system-audio track no longer kills a meeting whose mic track is
good. The system-side load now degrades like the mic side already did, and
the too-short check routes to the mic-only pipeline instead of throwing a
non-retryable `recordingTooShort`.
- Orphaned-recording recovery no longer archives and unlinks audio owned by a
running or queued transcription. Core gains a `reservedAudioURLsProvider`
seam because the queue lives in the app layer; the app supplies the queued
jobs' audio.
- A successful failed-meeting retry no longer settles the session back onto
the previous failure's `.error`. The success is published before the
occupancy counters decrement, matching every other success path.
- Ghost-speaker remaps now resolve to a fixed point, so a ghost merged onto a
cluster that is later linked to a representative keeps its persistent id
instead of producing an unnameable duplicate speaker block.
Crash traps (all reachable from file content or tool arguments):
- `frontmatterBlock` clamps its slice end; a capture file ending at its
closing `---` fence used to call `index(after: endIndex)` and abort the MCP
server.
- MCP tool arguments convert with `Int(exactly:)`; `{"count": 1e30}` used to
abort the server. `read_meeting`'s limit math no longer overflows.
- `duration:` frontmatter components are bounded above in both parsers, so a
corrupted value cannot overflow the *60/*3600 multiply. The app-side copy
runs over every capture file during the Home scan.
- Speaker-id lookups build with `uniquingKeysWith:` in the three places they
were built from file content, where a repeated id used to trap.
- The QA automation warmup flag is lock-guarded and only trusted when the
wait actually succeeds.
Correctness and hygiene:
- The silent mic placeholder WAV is zeroed explicitly rather than relying on
an undocumented allocator guarantee; it is fed back into the pipeline on
retry.
- Scratch mic placeholders are tracked by task and retired once the archive
repoint is durable, instead of leaking ~160KB per system-only failure.
- `looksLikeCaptureMarkdown` classifies from a bounded 64KB prefix instead of
reading whole transcripts, which the MCP watcher did once per file on every
reconcile tick, before any staleness check could skip it.
- The CLI reads each meeting once instead of three times.
- Deleted `TranscriptIndex.indexSingleFile` and its three orphaned tests: it
had no production callers, and FileWatcher's `onChange` carries no URL so
it could never be reached.
Nothing here was compiled or tested: this environment is Linux with no Swift
toolchain, and the app targets macOS 26 on Apple Silicon. The full
verification set still needs to run locally.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011JPzxZWnJ1cSKxDgUXFgVF
testMalformedDurationFallsBackToZero asserts on the literal source text of
parseDurationSeconds as a behavior contract. Adding the upper-bound check
changed that guard from
!components.contains(where: { $0 < 0 })
to
!components.contains(where: { $0 < 0 || $0 > maxDurationComponent })
so the pinned substring no longer matched and the assertion failed.
Repoint the pin at the new predicate and add the overflow fixture the guard
exists for ("200000000000000000:00"), which traps on the *60 multiply without
it. The other two pins still match unchanged.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011JPzxZWnJ1cSKxDgUXFgVF
…plicate UIDs Three verified audit findings, all self-contained. meeting_capture_stopped_under_controller was tracked but never allowlisted, so AnalyticsReporter.trackEvent dropped it before delivery and the fleet-wide count for "capture died under us, audio preserved for retry" was permanently zero. Its call site builds properties with the same MeetingCaptureHealthTelemetry .snapshotProperties initializer as meeting_capture_health_snapshot, so it takes that event's property set. Added to both the PSV registry and the observability doc, which AnalyticsEventPolicyTests asserts stay in parity. AppLogSink only checked debug.log's size in reset(at:), whose sole caller is init(), so a resident menubar app grew past the 500 KB threshold for a whole session while its sibling sinks rotated mid-write. Track appended bytes in the writer actor, seeded from the file on disk, and re-run LogTailTrimmer past the threshold. The handle closes first because the trimmer rewrites the file. Both Dictionary(uniqueKeysWithValues:) sites in PersistentDictationInputController build a UID map from OS-supplied strings. A duplicate key is a trap, not a throw, so the enclosing do/catch could not contain it and the app would die with the persisted input override still applied to the system. Keep the first match, which is what the first(where:) UID lookups elsewhere in the subsystem already do. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011JPzxZWnJ1cSKxDgUXFgVF
…-meeting Record updateCorrectionSpoken runs per keystroke and mirrored `spoken` into `replacement` only while `replacement` was empty, so it froze at the first character typed: "okay ours" persisted as the substitution "okay ours" -> "o", rewriting that phrase in every later dictation. Editing an existing vocabulary hint was worse — there spoken == replacement and both are non-empty, so the very first keystroke turned "foo" into "foos" -> "foo". Keep mirroring until the user actually makes the two fields differ; a real substitution still keeps its replacement. The detected-meeting Record action ran unconditionally, but startRecording returns true early without publishing a state transition when a meeting is already recording, starting, or stopping. The overlay therefore entered .preparing and never left — "Starting meeting…" with no timer and no stop button — while the candidate was marked accepted so it was never recorded, and both choice events reported a selection that could not start anything. Gate the whole record path on the session-state check the detector already applies in shouldSkipPromptEvaluation, which covers all three symptoms at the one place they branch from. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011JPzxZWnJ1cSKxDgUXFgVF
MeetingCaptureBridge mirrored seven Audio publishers through Combine's RunLoop.main scheduler, which enqueues via RunLoop.perform and so only runs in .default mode. Nothing was delivered while an NSMenu tracked or a modal ran, so the pill's mm:ss timer and both level meters froze behind the pill's own context menu, the discard alert, and NSOpenPanel — MeetingSessionController re-publishes these already-stalled mirrors, so the overlay's own correct DispatchQueue.main hops could not rescue them. It also split the start handshake: finishPendingStartAttemptIfPossible's only trigger is these sinks, while its 12s counterpart is a MainActor Task on the main dispatch queue, serviced in all common modes. Switch all seven to DispatchQueue.main, matching MeetingOverlayController, which already mirrors the same state that way. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011JPzxZWnJ1cSKxDgUXFgVF
Commit fa39443 removed the DictationSessionController caller and 138 lines of tests but left retryPaste itself in place. An exhaustive search across Sources, Tools, Tests, scripts and the build source lists finds no remaining reference to it, and its only helper, restoreRetainedClipboardBeforePasteRetry, was called from nowhere else. Deletes both. The live "transcript stays on the clipboard" path is untouched: discardPasteRetry, restoreRetainedClipboardNow, and paste's own retainClipboardForPasteRetry parameter all survive. DictationPasteRetryTelemetry .performUserRetry also stays — it takes the retry closure as a parameter, so it never depended on this method. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011JPzxZWnJ1cSKxDgUXFgVF
copyItem wrote straight into the final destination, so a throw part-way through a <stem>_audio/ directory left a half-populated directory behind. Both the planner's collision check and the pre-copy recheck are bare existence checks, so a retry classified that residue as skippedExisting, reported success with "Skipped N that already existed at the destination", and applyCaptureLibraryChoice switched the library to the truncated copy. Not data loss on its own — the planner never deletes originals — but a false success signal that becomes loss if the user then clears the old folder. Copy to a hidden sibling and moveItem into place, which is atomic within one directory, and remove the staging entry on any failure. The directory enumerator already passes .skipsHiddenFiles, so an orphaned staging entry cannot be planned into a later migration. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011JPzxZWnJ1cSKxDgUXFgVF
applyMerge documents itself as mirroring SpeakerDatabase.mergeProfilesImpl's merged-name rule so the transcript rewrite uses the exact name the DB mutation is about to produce. It did not: mergeProfilesImpl binds SQL NULL when neither profile has a display name, while this fell back to "Speaker <uuid-prefix>" and wrote that hex fragment of a database UUID into the YAML name: and every body label — the precise thing the invariant comment above it says must not happen. Not exotic. duplicateReason flags two unnamed profiles at cosine >= 0.90 as a suggested duplicate and sortedMergeTargets applies no name filter, so merging two unnamed voices is a normal path through the duplicate-suggestion UI. Drop the fallback and skip the rename when there is no name, leaving each file's existing "Speaker N" placeholder alone while still repointing db_id. Two placeholders in one file then share a db_id until the user names the merged voice, which the same merge already repoints — better than overwriting both with an identifier. Outcome.resolvedDisplayName is already String? and two other paths already pass nil, so nothing downstream changes shape. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011JPzxZWnJ1cSKxDgUXFgVF
…nned non-Swift paths build-deps.sh's Metal stage had two unreachable warn-and-continue branches under set -euo pipefail: find exits 1 when the generated-metal directory is absent and an assignment takes its substitution's status, and pipefail makes the empty-glob ls abort the AIR_COUNT pipeline. Both aborted the whole deps build instead — after the multi-minute SwiftPM build and before the staging swap, with "Compiling MLX Metal shaders..." as the last output. Add `|| true` to each, and to the un-guarded xcrun metal call that aborted before either could be reached. Making them reachable would otherwise let warn-and-continue ship an app with no MLX Metal shaders, since nothing verifies that artifact — neither deps_are_ready nor build.sh's deps check looks for mlx.metallib. So when the Cmlx checkout is present its metallib is now required, failing with a message that says what happened rather than aborting mid-pipe. run-tests.sh kept one full ~184-object directory per distinct APP_SOURCES hash and nothing ever evicted them. Keep the most recently used entries (default 5, override with TRANSCRIPTED_FAST_TESTS_CACHE_ENTRIES), touching an entry on hit so reuse counts as recency. Pruning runs while the cache lock is held so a concurrent run cannot lose its objects mid-compile, only removes paths under CACHE_ROOT, and leaves the dot-prefixed lock directory alone. Root fast tests assert on the literal content of ~15 non-Swift files, but the matrix mapped those paths only to preflight and a `bash -n`, so editing one broke run-tests.sh in CI rather than locally. Add a rule routing the shell/YAML/Python members of that pinned set to run-tests.sh. The docs/** members stay out on purpose — AGENTS.md and the docs-only note below the rules cover them. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011JPzxZWnJ1cSKxDgUXFgVF
…y against drift
Six SentryEventPolicy rows had no emitter anywhere: parakeet.resync_engine_failed,
parakeet.zero_sample_rate, parakeet.audio_format_failed, capture.hotkey_register_failed,
and both onboarding.first_dictation_{start,stop}_failed. The last three were
referenced only by SentryEventPolicyTests looking themselves up, so those lookups
and their assertions go with them.
Two entries the audit grouped with them are deliberately kept.
parakeet.device_change_recovery_timeout looked dead to a `event: "..."` scan but is
emitted through a ternary in ParakeetDeviceRecovery and recorded by
ReliabilityPacketRecorder. parakeet.prewarm_failed does have an emitter; it is only
unreachable because that call site is .warning and the Sentry gate takes .error, so
dropping the row would be a latent behavior change the moment anyone raises the
level or adds a terminal emitter.
The seven unemitted onboarding events in the analytics registry are NOT deleted.
They are the activation funnel: five ops scripts (posthog-activation-funnel.py,
posthog-dashboard-queries.py, posthog-product-dashboard-summary.py,
generate-nightly-digest.py, health-probe.sh) and four docs are built around those
names. They are registered ahead of the UI that will fire them, so removing them
would tear out a documented design, not clean up a leftover.
Instead, close the direction that actually bites: check-analytics-emitters.py
asserts every event name passed to AnalyticsReporter.track exists in the PSV.
AnalyticsEventPolicyTests only compares the doc and the PSV to each other, so
nothing checked either against the emitting code — which is how
meeting_capture_stopped_under_controller stayed unregistered and silently dropped.
Removing that event from the registry now fails this check with the file that
emits it. Registered-but-unemitted is reported for information and never fails,
and the two call sites that dispatch a computed name are listed as uncheckable.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011JPzxZWnJ1cSKxDgUXFgVF
…y pickers The analytics toggle deliberately tracks the opt-out before flipping the preference, so the event is captured while sending is still allowed. But track only enqueues onto the delivery queue, and enqueue re-reads analyticsEnabled() on the far side — so the setEnabled(false) landing microseconds later discarded the capture, and opt-out rate, the one metric that ordering exists to preserve, was unmeasurable. Drain the serial delivery queue before flipping. A sync barrier after an async enqueue on a serial queue guarantees the enqueue ran, so the capture gets its one delivery attempt while still enabled. This does not weaken "opt-out purges the buffer" — the preference change still deletes the retry file and blocks further sends, which is exactly the shape AnalyticsReporterTests already pins (it expects the pre-opt-out event's single request to have happened). Deliberately not fixed by threading a wasEnabled snapshot through enqueue, which would break that invariant. chooseCaptureLibrary and resetCaptureLibraryToDefault also carried the same 15-line tail, differing only in destination, whether a preference URL is written, and the destination kind reported to the migration prompt — and applyCaptureLibraryChoice takes exactly that preference URL, so the final call unifies too. One selectCaptureLibrary(destination:preferenceURL:destinationKind:) now backs both. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011JPzxZWnJ1cSKxDgUXFgVF
SystemAudioCapture and its two extension files have no construction site
anywhere. Audio.swift hardcodes `{ SCKAudioCapture() }` as its
systemAudioCaptureFactory, the class is internal so no embedder can reach it
either, and an exhaustive search across Sources, Tools, Tests, scripts, build.sh,
build-beta.sh and Package.swift finds only prose mentions. build-deps.sh globs
the whole Core tree, so all 791 lines shipped inside libDraftDeps.a, and the
backend has already cost at least one concurrency refactor pass.
Also refreshes the six comments that described it as a live alternate backend,
so the tree stops advertising a swap that cannot happen.
Deliberately left alone, against the audit's suggestion:
- `deliversOwnedAudioBuffers` stays on SystemAudioCaptureEngine. It is not
vestigial — SCKAudioCapture sets it, AudioFileManager branches on it, and four
AudioTests files assert against it.
- The always-true branch in AudioFileManager stays. Collapsing it is a behavior
change in the riskiest file in the repo, its else arm has no test coverage, and
nothing forces the question now that the second backend is gone.
- CoreAudioUtils keeps readProcessList, translatePIDToProcessObjectID and
readAudioTapStreamBasicDescription, which this deletion orphans. They are
small, self-contained, and worth removing under a compiler rather than blind.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011JPzxZWnJ1cSKxDgUXFgVF
TranscriptStorage was a 94-line public protocol with exactly one conformer, an
empty `extension TranscriptSaver: TranscriptStorage {}`, and no use as an
abstraction anywhere: no `any TranscriptStorage`, no generic constraint, no
metatype. Its only test built a fake conformer purely to exercise the protocol's
own default implementation, so that test and the fake go with it. Nothing is
injected through it and nothing selects a storage backend at runtime.
The three TranscriptSaver.saveTranscript overloads stay. The audit suggested
pruning two of them as protocol witnesses, but they form a forwarding chain with
live callers in TranscriptionPipelineRunner and the test suite, and working out
which overload each call site resolves to is a question for a compiler, not a
grep. The indirection this removes is the protocol itself.
Also resyncs Sources/TranscriptedCore/CLAUDE.md with the tree after this and the
process-tap deletion: Audio/ 23 -> 20 files with "process tap" dropped from its
blurb, Protocols/ 7 -> 6, and the line claiming Audio can switch between a legacy
CoreAudio path and the ScreenCaptureKit one now says what is actually true —
SCKAudioCapture is the only conformer.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011JPzxZWnJ1cSKxDgUXFgVF
Deleting `extension TranscriptSaver: TranscriptStorage {}` left the
`@available(macOS 14.0, *)` that decorated it sitting at end of file with
nothing to attach to, so the deps build failed with "expected declaration".
Brace-balance checking passed over it because attributes do not affect brace
counts — the gap in how the deletion was verified. Swept every Swift file
touched on this branch for the same shape; this was the only one.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011JPzxZWnJ1cSKxDgUXFgVF
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
A 14-domain audit of the codebase produced 37 findings that survived adversarial verification (13 more were refuted and dropped). This PR applies 35 of them across 18 commits. Two were deliberately left undone — see Not done, on purpose below.
Verification status has changed since this PR was opened. The session applying these changes runs on Linux with no Swift toolchain, so nothing could be compiled locally. CI has since run the full set on macOS and it is green end to end:
app-build,spm-tests,checks,repo-hygieneand thebuild-and-testgate all pass onea15465.spm-testscovers Coreswift test, the integration smoke, and all four Tools packages;checkscovers the root fast tests and the e2e smoke.Product Impact
meetings/agent artifactsmeeting reliabilityWhat changed
Recordings that were stranded or lost
allowedAudioRootsis built from the current library, so after "switch without copying" every queued row failedisSafeAudioURL, counted asremovedCountwithunavailableCountstill 0, and the queue file was rewritten without it. The recording survived on disk; the row did not./tmp,..traversal and arbitrary home paths are still rejected.recordingTooShorteven with 45 minutes of usable mic audio, and that error kind is both non-retryable and non-recoverable, so the result was a dead failed-queue row.tasksat all, so Core gains areservedAudioURLsProviderseam and defers those candidates to the rescan.<stem>_audio/on a mid-tree failure, which a retry then classified asskippedExistingand reported as success.Crash traps, all reachable from file content or tool arguments
frontmatterBlockclamps its slice end — a file ending at its closing---fence calledindex(after: endIndex), a release-checked precondition.Int(exactly:);{"count": 1e30}is legal JSON and aborted the process before any clamp ran.duration:components are bounded above in both parsers.uniquingKeysWith:where they are built from file content or OS-supplied strings.Wrong output and identity drift
applyMergeclaimed to mirrormergeProfilesImpl, which binds SQL NULL for two unnamed profiles, but fell back to"Speaker <uuid-prefix>"and wrote that into the YAMLname:and every body label.spokenintoreplacementonly whilereplacementwas empty, freezing at the first character typed; editing an existing vocabulary hint turnedfoointofoos -> fooon the first keystroke.UI that showed something false
RunLoop.main, which only services.defaultmode, so the pill's timer and both level meters froze behind any menu or modal.startRecordingreturns true early without publishing a transition when a meeting is already recording, so the overlay entered.preparingand never left, the candidate was marked accepted, and two choice events reported a selection that could not start anything.Observability
meeting_capture_stopped_under_controllerwas tracked but never allowlisted, so it was dropped before delivery and its fleet count was permanently zero.debug.logis trimmed mid-session instead of only at launch.enqueuere-read the preference on the delivery queue after the flip had landed.SentryEventPolicyrows removed.scripts/dev/check-analytics-emitters.pyasserts every tracked event exists in the registry, wired into the test matrix. Verified it catches the exact slip above.Dead code and hygiene
Audio.swifthardcodesSCKAudioCapture().TranscriptStorageprotocol is deleted — one conformer, an empty extension, no existential or generic use.ClipboardRestoringTextPaster.retryPasteandTranscriptIndex.indexSingleFileare deleted.looksLikeCaptureMarkdownclassifies from a bounded prefix instead of reading whole transcripts on every MCP reconcile tick; the CLI reads each meeting once instead of three times.build-deps.sh's two Metal warn-and-continue branches were unreachable underset -euo pipefail; they now work, paired with a requiredmlx.metallibcheck so warn-and-continue cannot silently ship an app without shaders.run-tests.sh.Not done, on purpose
Audio.startMonitoringand its cluster.docs/MEETING_CAPTURE_PROMPTING.mdTier 3 designs pre-arm/pre-roll staging around reusing it. Deleting it forecloses a planned feature — a product call, not a cleanup.SpeakerReviewQueueScannercache remain open. The former changes how aggressively voices merge, which is a product decision.How I checked it
bash build.sh --no-open— via CIapp-buildbash run-tests.sh— via CIchecksbash run-integration-smoke.sh— via CIspm-testsswift testplus all four Tools packages — via CIspm-testsbash run-e2e-smoke.sh— via CIcheckspython3 scripts/dev/check-build-source-lists.py,python3 scripts/dev/test-matrix-checks.py --self-test,python3 scripts/dev/check-analytics-emitters.py,bash -non every touched scripthardware-smokes— skipped by CI as always; needs a self-hosted Apple Silicon runner with mic and System Audio Recording permissionsEvery deleted symbol was re-checked with
rg -wacrossSources,Tools,Tests,scripts,build.sh,build-beta.shandPackage.swiftbefore removal.One compile error did reach CI and was fixed in
ea15465: deleting theTranscriptStorageconformance left its@availableattribute orphaned at end of file. Brace-balance checking passed over it because attributes do not affect brace counts. Every Swift file touched on this branch was then swept for that shape; it was the only one.Risk Review
.agent-review/visuals/evidence — n/a, the UI changes here are behavioral, with no new surfacesNotes
Three changes go slightly beyond the audit's 37 findings, all the same crash class as a finding already in scope and within two lines of it: the two
uniqueKeysWithValuesspeaker-id sites in MCP, the one in the CLI, and the app-sideTranscriptFrontmatterduration overflow.Three of the audit's own recommendations were overridden after checking, each of which would have been a regression:
parakeet.device_change_recovery_timeoutlooked dead to anevent: "..."scan but is emitted through a ternary inParakeetDeviceRecoveryand recorded byReliabilityPacketRecorder.parakeet.prewarm_failedhas a live emitter and is only unreachable because that call site is.warningwhile the Sentry gate takes.error; dropping the row would be a latent behavior change.deliversOwnedAudioBuffersis not vestigial —SCKAudioCapturesets it,AudioFileManagerbranches on it, and fourAudioTestsfiles assert against it.Also kept deliberately: the always-true branch in
AudioFileManager(collapsing it is a behavior change in the riskiest file in the repo, and itselsearm has no test coverage), the threeCoreAudioUtilshelpers the backend deletion orphans, and all threesaveTranscriptoverloads (a forwarding chain with live callers; which overload each call site binds to is a question for a compiler, not a grep).Generated by Claude Code