Address codex review: drop transcript sample, raise tri-gram floor#3
Merged
Conversation
Two follow-ups to PR #1 raised by an independent codex review: 1. Privacy: drop `sample` from `CohereMLXBackendError.degenerateOutput`. `TranscriptionWorker` persists `String(describing: error)` into the failed `transcript.md` and `metadata.json` artifacts, so the 120-char transcript snippet that used to ride along on the error was landing in on-disk failure metadata. The sample now stays in the unified log at `.private` privacy level for live debugging only. 2. False-positive floor: require the dominant tri-gram to repeat at least 5 times before firing, in addition to the >8% fraction check. At the 30-word minimum, 3 occurrences of an ordinary tri-gram (e.g. "thank you very") cleared the fraction gate at 3/28 ≈ 11% and terminally failed otherwise-usable short transcripts. The observed real-world loop fires 12+ times so the new floor is still well within range. New regression test guards the short-transcript-with-three-repeats case; the existing throw test now also asserts that `String(describing:)` on the error does NOT contain transcript content.
Newarr
force-pushed
the
fix/codex-review-followup
branch
from
3e58a2c to
09257e9
Compare
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
1 participant
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.
Summary
Codex (gpt-5.1-codex) reviewed PR #1 post-merge and flagged two
[P2]issues inDegenerateOutputDetector/CohereMLXBackendError. Both are real; this PR addresses them.Findings and fixes
1. Privacy leak via
sampleVerified:
TranscriptionWorker.swift:226doeslet reason = String(describing: error)then writes it viawriteFailed. The 120-char snippet (which by definition is the first few sentences of a meeting transcript) was landing in the failure artifact.Fix: Drop the
sampleassociated value entirely. Surface it instead viaLog.engine.error(... sample: \(text, privacy: .private))so the unified log can show it for live debugging without leaking to disk.2. Tri-gram threshold too sensitive at the 30-word floor
Verified: at 30 words there are 28 tri-grams, so 3 repeats clears the 8% gate. A short recording with "thank you very" appearing three times would fail terminally.
Fix: Require
n >= 5in addition to the fraction gate. The observed real-world loop has 12+ repeats so it stays caught; ordinary short transcripts no longer false-positive.Tests
testBackendThrowsDegenerateOutputErrorOnLoopedAdapterOutputupdated for the new error shape and now assertsString(describing: error)does NOT contain transcript content.testDetectorDoesNotFireOnShortTranscriptWithFewRepeats— 30-word transcript with 3 occurrences of "thank you very" must returnnil.SCRIBE_RUN_MLX_INTEGRATION=1).Test plan
swift testis cleanmetadata.jsonshows the reason but not the looped text