Closes #178#189
Conversation
AudioDevice::from_name detected the type with a case-insensitive check
(name.to_lowercase().ends_with("(input)")) but stripped with an exact-case
trim_end_matches("(input)") on the ORIGINAL name — so "(Input)"/"(OUTPUT)"
passed detection yet left the suffix stuck on the returned name, which would then
never match cpal's exact device.name() (spurious DeviceNotFound). Latent today
(only the crate's lowercase Display impl calls from_name), but the contract is
wrong. Strip by the matched suffix's byte length (ASCII → len == char count in
any casing). Regression test covers (Input)/(INPUT)/(Output).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
with_source_language reduced every non-auto tag to its primary subtag, so the documented special value "auto-translate" (per-utterance detect + translate to English, matched exactly by whisper/engine.rs set_translate) was split to "auto" and silently downgraded to plain auto-detect — the translate-to-English mode was unreachable through the only production setter. Special-case it before the subtag split. Regression test covers "auto-translate" and a cased/spaced variant. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…) rejects stop() used Promise.all, so if either engine's stop() rejected, the two state-reset lines (active = primary; usingFallback = false) were skipped — leaving usingFallback stuck true, so the NEXT session's start() would silently route to the stopped fallback tier even after credit recovered. Switch to Promise.allSettled (await both stops), reset routing UNCONDITIONALLY, then rethrow the first error so the caller still sees the failure. Not reachable with today's engines (neither stop() rejects) but the TranslationEngine contract doesn't forbid it. Regression test: a fallback whose stop() throws still leaves the router routing to primary next session, and the error propagates. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…inel sanitizeTitle had no guard against producing exactly WORKING_TITLE "(recording)". An LLM final title that sanitized to it made finalize() write `<prefix> — (recording).md` — byte-identical to the in-progress working-file grammar — so retention/adopt (which key off the ` — (recording)` suffix, not content) misread the finalized session as a crashed orphan: exempt from the retention sweep, then silently re-titled from its first summary line. Guard in sanitizeTitle (covers finalize AND adopt's rename): a sanitized title equal to the sentinel falls back to FALLBACK_TITLE. Moved WORKING_TITLE into sanitize.ts (re-exported from writer.ts, so retention.ts/index.ts imports are unchanged) so the guard needs no circular import. Regression test: "(recording)" and whitespace variants → FALLBACK_TITLE; a title merely containing the word is untouched. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…round-trip renderBoard joined board items with " · " and parse.ts split them back on it, so a single decision/action/question that itself contained " · " (natural writing; "·" is a common Korean bullet in this EN/KO product) round-tripped as TWO items — silently fracturing the meeting board on the dashboard. Neutralize an item-internal " · " by swapping its U+00B7 middot for a visually-similar U+2027 (‧) before joining, so it can no longer match the U+00B7 separator; a bare "·" is left alone. Regression: a one-decision "Ship v2 · notify design team" stays one item. Golden output unchanged (no fixture item contains the separator). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…w-confidence renderEntryBody appends " (?)" for low-confidence and parse.ts strips a trailing " (?)" from ANY source, so a caption that legitimately ends in " (?)" (STT punctuation / a spoken aside) parsed back as lowConfidence=true with its last four characters deleted — corrupting the dashboard/review text, and a write→parse→amend cycle persisted it. Defuse a source that itself ends with the marker by swapping the trailing ASCII "?" for a look-alike U+FF1F (?) after sanitizeInline, so the suffix is no longer the exact marker; visually near-identical, read back verbatim. Regression: a non-low-confidence "…ok? (?)" stays full text + not flagged; a low-confidence "…what? (?)" keeps its own marker after the appended one is stripped. Golden output unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…mode changes apply_mode set `transitioning = true` and spawned a 250ms timer that unconditionally cleared it — with no generation guard (unlike drag_generation). Two apply_mode calls within 250ms (Alt+Shift+Space key-repeat, rapid Mode-menu clicks) let the FIRST timer clear `transitioning` mid-way through the SECOND transition, so record_geometry went live again and persisted the in-flight animated rect as the remembered per-mode geometry — corrupting the saved window position with no visible error. Add a `transition_generation` epoch (mirroring drag_generation): begin_transition() bumps it and returns the epoch; end_transition_if_current() clears the flag only if its epoch is still current, so a superseded transition's late timer is a no-op. Unit tests cover the stale-timer no-op and the single-transition clear (run under app-macos, like session.rs). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…non-silent Finding A (resampler drops buffered audio on a rate change) is a documented behavior-DECISION, not a defect (Batch-36 amendment): the sub-CHUNK_IN residue was captured at the OLD rate, so it can neither be fed through the freshly-rebuilt new-rate resampler (a rate mismatch would corrupt the output) nor emitted as new-rate pass-through — dropping it is correct. So the "flush the residual" fix-direction is deliberately NOT taken. Instead do the fix-direction's stated minimum: log the dropped sample COUNT (never audio, #23-safe) at debug level so the ~21ms loss at a mid-meeting device switch is observable rather than silent. Behavior unchanged (existing resample tests green); the drop is now documented as the deliberate decision it is. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
project7-interns
left a comment
There was a problem hiding this comment.
Verdict: REQUEST CHANGES
Epic Alignment: PASS
The seven fixes and the documented resampler decision follow #178’s scoped order before #179; the PR has eight focused commits as required.
Checked (evidence)
- Structural gate: live PR body includes filled EPIC Alignment, Self-Verification, and N/A Design Fidelity sections.
- Ticket alignment: live #178 supports the rate-change drop as a documented behavior decision; src/resample.rs:77-91 logs only a count and preserves the intentional old-rate boundary.
- Commit partition: gh api pulls/189/commits → eight focused commits, one for each independent fix/decision.
- Riskiest parts: archive grammar escapes at packages/archive/src/render.ts:45-55 and :108-112 are behaviorally correct and regression-tested, but each introduces an unnecessary one-use helper.
- Kill-list: scanned full diff — hits listed below.
- CI: gh pr checks 189 → color-guard and no-stub-gate pass; app-macos, packages-linux, and release-invariants pending.
Findings
-
[required] Inline the one-call-site board-item escape helper.
- File: packages/archive/src/render.ts:45
- Why it fails: escapeBoardItem has exactly one production call site at :52; it adds a wrapper with no reusable boundary.
- Do instead: inline its split/join substitution in the map expression within renderBoard’s join helper; retain the existing round-trip regression.
-
[required] Inline the one-call-site low-confidence defuser.
- File: packages/archive/src/render.ts:108
- Why it fails: defuseLowConfMarker has exactly one production call site at :119; it is a single-use helper rather than shared behavior.
- Do instead: inline the trailing-marker conditional in renderEntryBody after sanitizeInline; retain both low-confidence regression cases.
Decision
The functional fixes are aligned and well-covered, but these two single-call-site abstractions must be inlined before approval.
RE2 — REQUEST CHANGES (concurring with @re1's one open item; all 7 fixes + the Finding-A decision are correctness-clean)Reviewed all 8 commits against source and ran every suite on Linux. The only change I ask for is the same one @re1 raised (#1010): inline the two one-call-site helpers. Everything else is verified correct. Concur with @re1 (the one blocking item)
Checked (evidence) — all correct
Net: inline the two helpers (per @re1) and re-request — I'll re-confirm the delta. No other changes needed. |
Per RE1 #1010 / RE2 #1012: fold escapeBoardItem (sole caller renderBoard) and defuseLowConfMarker (sole caller renderEntryBody) into their callers, keeping the rationale as inline comments. Behavior identical — the C/D regressions (board middot U+2027, trailing ? U+FF1F) still pass; golden output unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
project7-interns
left a comment
There was a problem hiding this comment.
Verdict: APPROVE
Epic Alignment: PASS
#178’s seven independent fixes and its documented rate-change behavior decision are complete before the overlapping #179 work.
Checked (evidence)
- Re-review range: 37a1ea5..7210fba.
- Prior findings resolved: packages/archive/src/render.ts:47-56 and :105-119 inline the board and low-confidence escape logic; no one-use helpers remain.
- Scope/commit partition: live PR commits show eight focused commits covering the seven fixes plus the resampler decision.
- Riskiest part: rate-change residue handling; src/resample.rs preserves the intentional old-rate drop and logs only its sample count, matching the #178 amendment.
- Kill-list: scanned new range — clean.
- CI: gh pr checks 189 → app-macos, color-guard, no-stub-gate, packages-linux, release-invariants all pass.
Findings
None.
Decision
The focused fixes and regressions resolve #178 without creating a behavior conflict for #179’s required rebase.
RE2 — APPROVE at
|
Closes #178
Small-correctness polish: seven verified independent bugs, each its own commit with focused regression coverage, plus one finding (A) treated as a documented behavior-decision per the Batch-36 amendment.
EPIC Alignment
Part of EPIC #134 (v1.3 audit follow-up), Batch 46 (standing-order … #177 → #178, then #179). #178 runs first; #179 overlaps
resample.rs/render.ts/parse.ts/pipeline.rsand must rebase after this merges (the sequencing RE2 called out). Each fix is independent and isolated to its finding.Self-Verification (per finding, one commit each)
F —
device.rs: case-insensitive suffix strip.from_namedetected the type case-insensitively (to_lowercase().ends_with("(input)")) but stripped with an exact-casetrim_end_matches("(input)")on the original — so(Input)/(OUTPUT)passed detection yet left the suffix stuck on, which would never match cpal's exactdevice.name()(spuriousDeviceNotFound). Strip by the matched suffix's byte length (ASCII → len == char count in any casing). Test:(Input)/(INPUT)/(Output)all strip cleanly.H —
pipeline.rs: preserve"auto-translate".with_source_languagereduced every non-auto tag to its primary subtag, so"auto-translate"(detect + translate to English, matched exactly bywhisper/engine.rsset_translate) split to"auto"and silently downgraded to plain auto-detect. Special-cased before the subtag split. Test:"auto-translate"(and a cased/spaced variant) survives verbatim.G —
fallback-router.ts: reset routing even if astop()rejects.Promise.allskipped the state reset on anystop()rejection, strandingusingFallback = trueso the NEXT session silently routed to the stopped fallback tier.Promise.allSettled→ reset routing UNCONDITIONALLY → rethrow the first error. Test: a fallback whosestop()throws still routes the next session to primary, and the error propagates.E — archive: a finalized title can never be the
(recording)sentinel.sanitizeTitlehad no guard against producingWORKING_TITLE, so an LLM title sanitizing to it made<prefix> — (recording).md— byte-identical to the in-progress grammar — and retention/adopt (which key off the— (recording)suffix) misread the finalized session as a crashed orphan (exempt from the sweep, then re-titled from its first summary line). Guard insanitizeTitle(coversfinalizeANDadopt); movedWORKING_TITLEintosanitize.ts(re-exported fromwriter.ts, soretention.ts/index.tsare unchanged) to avoid a circular import. Test:(recording)(+ whitespace variants) →FALLBACK_TITLE; a title merely containing the word is untouched.C — archive: a board item containing
·no longer fragments.renderBoardjoined items with·andparse.tssplit on it, so a single decision containing·(natural writing;·is a common Korean bullet in this EN/KO product) round-tripped as TWO items. Swap an item-internal·middot (U+00B7) for a look-alike U+2027 (‧) before joining so it can't match the separator; a bare·is untouched. Fix-direction-endorsed ("visually-similar but distinct character"). Test:"Ship v2 · notify design team"stays one decision. Golden output unchanged.D — archive: a caption ending in
(?)isn't misread as low-confidence.renderEntryBodyappends(?)for low-confidence andparse.tsstrips a trailing(?)from ANY source, so a caption legitimately ending in(?)parsed back aslowConfidence=truewith its last four chars deleted. Defuse a source that itself ends with the marker by swapping the trailing ASCII?for a look-alike U+FF1F (?) aftersanitizeInline. Test: a non-low-conf…ok? (?)stays full-text + unflagged; a low-conf…what? (?)keeps its own marker after the appended one is stripped. Golden unchanged.B —
overlay.rs: epoch-guard the transition clear-timer.apply_modesettransitioning = trueand spawned a 250ms timer that cleared it with no generation guard. Twoapply_modecalls within 250ms (Alt+Shift+Space key-repeat, rapid Mode-menu clicks) let the first timer clear the flag mid-way through the second transition, sorecord_geometrypersisted the in-flight animated rect as the saved per-mode geometry. Added atransition_generationepoch (mirroringdrag_generation):begin_transition()bumps it + returns the epoch;end_transition_if_current()clears only if the epoch is still current. Tests: stale-timer no-op + single-transition clear (run underapp-macos, likesession.rs; the pure epoch logic was also validated in a standalone harness).Deviation — finding A (
resample.rs) is a documented behavior-DECISION, not fixedPer the Batch-36 amendment: the
input_bufclear on a rate change is correct, not a defect. The sub-CHUNK_INresidue (≤1023 samples, ~21ms at 48kHz, only at a mid-meeting device/rate switch) was captured at the OLD rate, so it can neither be fed through the freshly-rebuilt new-rate resampler (a rate mismatch would itself corrupt the output — RE2's point) nor emitted as new-rate pass-through. So the "flush the residual" fix-direction is deliberately not taken (it would introduce a real bug). Instead I did the fix-direction's stated minimum — log the dropped sample count (never any audio; #23-safe) at debug level so the loss is observable, not silent — and documented the decision on the newdrop_residuehelper. Behavior is unchanged (existing resample tests green); nothing testable changed.Security invariants
No new dependencies. No caption/audio content logged (A logs only an integer sample count). Each fix is minimal and isolated to its finding — no drive-by refactors (the
WORKING_TITLEmove in E is the minimum needed to place the guard without a circular import).Tests / evidence
Each fix carries a focused regression test where testable (F, H, G, E, C, D, B). Full local run:
livecap-corelib 62 tests (F/H additions; resample/overlay behavior-preserving), TS suites — app 162,@livecap/archive98,@livecap/engine270 — all green;pnpm typecheck(app + packages),pnpm lint,pnpm buildclean; the #176no-stub-gate/color-guardgates pass.livecap-app(overlay) can't build on Linux (macOS-only), so B's tests run in theapp-macosCI job on this PR — watch it green.Design Fidelity
N/A — no UI/webview/CSS surface changed (Rust internals, archive markdown grammar, and the router are the only touched code; the board middot / caption
?look-alikes are near-invisible glyph swaps on a rare edge, not layout).🤖 Generated with Claude Code