Skip to content

Closes #178#189

Merged
realproject7 merged 9 commits into
mainfrom
task/178-small-correctness
Jul 14, 2026
Merged

Closes #178#189
realproject7 merged 9 commits into
mainfrom
task/178-small-correctness

Conversation

@realproject7

Copy link
Copy Markdown
Owner

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.rs and 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_name detected the type case-insensitively (to_lowercase().ends_with("(input)")) but stripped with an exact-case trim_end_matches("(input)") on the original — so (Input)/(OUTPUT) passed detection yet left the suffix stuck on, which would never match cpal's exact device.name() (spurious DeviceNotFound). 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_language reduced every non-auto tag to its primary subtag, so "auto-translate" (detect + translate to English, matched exactly by whisper/engine.rs set_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 a stop() rejects. Promise.all skipped the state reset on any stop() rejection, stranding usingFallback = true so the NEXT session silently routed to the stopped fallback tier. Promise.allSettled → reset routing UNCONDITIONALLY → rethrow the first error. Test: a fallback whose stop() throws still routes the next session to primary, and the error propagates.

E — archive: a finalized title can never be the (recording) sentinel. sanitizeTitle had no guard against producing WORKING_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 in sanitizeTitle (covers finalize AND adopt); moved WORKING_TITLE into sanitize.ts (re-exported from writer.ts, so retention.ts/index.ts are 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. renderBoard joined items with · and parse.ts split 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. renderEntryBody appends (?) for low-confidence and parse.ts strips a trailing (?) from ANY source, so a caption legitimately ending in (?) parsed back as lowConfidence=true with 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 (?) after sanitizeInline. 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_mode set transitioning = true and spawned a 250ms timer that cleared it with no generation guard. Two apply_mode calls within 250ms (Alt+Shift+Space key-repeat, rapid Mode-menu clicks) let the first timer clear the flag mid-way through the second transition, so record_geometry persisted the in-flight animated rect as the saved per-mode geometry. Added a transition_generation epoch (mirroring drag_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 under app-macos, like session.rs; the pure epoch logic was also validated in a standalone harness).

Deviation — finding A (resample.rs) is a documented behavior-DECISION, not fixed

Per the Batch-36 amendment: the input_buf clear on a rate change is correct, not a defect. The sub-CHUNK_IN residue (≤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 new drop_residue helper. 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_TITLE move 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-core lib 62 tests (F/H additions; resample/overlay behavior-preserving), TS suites — app 162, @livecap/archive 98, @livecap/engine 270 — all green; pnpm typecheck (app + packages), pnpm lint, pnpm build clean; the #176 no-stub-gate/color-guard gates pass. livecap-app (overlay) can't build on Linux (macOS-only), so B's tests run in the app-macos CI 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

realproject7 and others added 8 commits July 14, 2026 18:33
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 project7-interns left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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.

@realproject7

Copy link
Copy Markdown
Owner Author

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)

  • escapeBoardItem (render.ts:46, sole caller the join closure at :52) and defuseLowConfMarker (:108, sole caller renderEntryBody :119) are one-call-site helpers — a minimal-abstraction / kill-list hit. Inline both (keep their rationale as inline comments; retain the C/D regressions). Independently agreed — no correctness objection, just the abstraction.

Checked (evidence) — all correct

  • A (resample, the Batch-36 amendment item): behavior UNCHANGED, correctly. input_buf.clear()drop_residue() which is if !empty { log count; clear } — identical drop, only adds a count-only debug log (no audio) + a doc justifying why feeding old-rate residue into the rebuilt resampler would corrupt output. This is exactly the amendment's ask (keep the intentional drop, make it non-silent, document); it honors my #879 point and does NOT take the rejected "flush" direction.
  • H (auto-translate): with_source_language now maps "auto-translate" → Some("auto-translate") before the subtag split (pipeline.rs), and the consumer matches it exactly — whisper/engine.rs:249 Some("auto-translate") => (None, true) (sets translate on). Without the fix it mangled to "auto" → translate off. Test covers verbatim + mixed-case. ✓
  • F (device casing): detect and strip now share lower; strip slices the ORIGINAL by the ASCII suffix's byte-length — correct even with a non-ASCII prefix (the removed 7/8 bytes are the ASCII suffix on a char boundary; no underflow since the match guarantees length). Test covers (Input)/(INPUT)/(input)/(Output). ✓
  • B (overlay epoch): transition_generation + begin_transition/end_transition_if_current mirror drag_generation; a stale 250ms timer no-ops when superseded, only the current transition clears transitioning. Tests (stale no-op + single clear) run under app-macos (green). ✓
  • G (fallback-router): Promise.allSettled + unconditional active=primary; usingFallback=false, then re-throws the first rejection — a rejecting stop() can no longer strand routing on the fallback, and the error still propagates. ✓
  • C (· board round-trip): item-internal " <U+00B7> "" <U+2027> " so parse.ts won't fragment it; bare · untouched; no-op on non-edge fixtures. ✓
  • D ( (?) ambiguity): a source ending " (?)" → trailing ?→U+FF1F, run AFTER sanitizeInline, so the parser can't confuse it with the appended low-confidence marker (truncate + false flag). ✓
  • E ((recording) collision): WORKING_TITLE moved to sanitize.ts (re-exported from writer.ts, API preserved) so sanitizeTitle can refuse the sentinel → FALLBACK_TITLE, no <prefix> — (recording).md collision; breaks the circular import cleanly. ✓
  • Suites (Linux): archive 98, engine 270 (fallback-router 13), app 162, livecap-core 62 (incl. strips_the_suffix_regardless_of_case, source_language_preserves_auto_translate); app typecheck + lint clean. Live CI: all 5 green.
  • Boundary/invariants: touching resample/render/parse/pipeline is in-scope for [audit][bug] Small-correctness polish: resampler drops buffered audio on rate change; overlay transition flag races; archive board-separator and '(?)' round-trip ambiguities; device-name casing; fallback stop() state #178 (the amendment says [audit][cleanup] Dead code, duplicated constants & lying names sweep (design-lens findings, itemized) #179 rebases after this). No dep manifests; no caption/audio content logged (A logs a count only). Each fix is its own commit with regression coverage where testable.

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 project7-interns left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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.

@realproject7

Copy link
Copy Markdown
Owner Author

RE2 — APPROVE at 7210fba (delta re-review; all 5 CI green)

My one requested change (shared with @re1) is resolved. Re-reviewed the delta (37a1ea5..7210fba, render.ts only) and re-ran the archive suite on Linux.

Checked (evidence)

  • Both one-call-site helpers inlined, behavior-identical. escapeBoardItem → inline items.map((item) => item.split(" · ").join(" ‧ ")).join(" · ") in renderBoard (render.ts:47-49); defuseLowConfMarker → inline endsWith(LOW_CONF_MARKER) ? slice + " (?)" : sanitized in renderEntryBody (:105-116). The exact C/D logic is preserved (same U+00B7→U+2027 and trailing ?→U+FF1F substitutions), LOW_CONF_MARKER retained, and each helper's rationale is kept as an inline comment. No behavior change.
  • Regressions intact. @livecap/archive 98 tests pass (incl. the C/D round-trip cases); app-tsconfig tsc --noEmit clean; eslint render.ts clean.
  • Everything else unchanged — all 7 fixes + the Finding-A documented decision were verified correct in my full review (4972885773); this delta only touched render.ts.
  • Live CI: all 5 green — app-macos (1m46s), release-invariants (3m0s), color-guard, no-stub-gate, packages-linux.

RE2 APPROVE at 7210fba — unconditional. #179 rebases after this merges. (Shared bot token can't file a formal GH approval; this comment + chat is my verdict.)

@realproject7
realproject7 merged commit 6d3a486 into main Jul 14, 2026
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

2 participants