fix: router warnings never reached the user without --explain (#136) - #141
fix: router warnings never reached the user without --explain (#136)#141kiki830621 wants to merge 7 commits into
Conversation
Every warning the router produced was folded into the `explanation` prose block, which the CLI prints only under --explain. The entire output of a run that silently substituted a different backend was "Wrote txt transcript to …". Warnings now print to stderr on the default path. The split is the point: `reason` explains a choice and is legitimately opt-in, but a warning is what the reader needs in order to trust the file that was just written. Both arrays already existed on ASRRecommendation; only the CLI conflated them, so this carries `warnings` through TranscribeOutcome rather than asking the caller to parse prose. Also unhidden: the --backend substitution notice (the #121 case), the cold-start memory-downgrade warnings, and the unverified-model notice. --explain output is unchanged and the warnings are not duplicated when it is passed. diagnose and recommend were already unaffected. Refs #136
CI caught what a developer machine could not: `a clean run carries no warnings` failed remotely while passing locally, because these tests used the default WhisperKitLanguageDetector — which downloads and runs a real model. Locally the model is cached and detection succeeds; in CI it fails, and the fallback emits a warning on every `--language auto` run. The failing test is the guard half of the pair added with this change, whose whole job is to assert the default path stays quiet. It did exactly that, just against an environment difference rather than a production defect: the warning itself is correct and stays, since language-agnostic ranking is what let an English-only backend win zh audio (#105). Stubbing the detector restores the hermetic contract this file already claims. Side effect: the three CLI suites drop from ~5.8s to ~0.02s, which is how much real model work was hiding in them. Refs #136
Verify Report — PR #141Enginemanual fan-out (4 lens Agents + sequenced Devil's Advocate, model: opus, file-based output) + Codex ( Backend note: the canonical Diff-freshness gate (#228): PASS — frozen AggregateFAIL — 2 blocking, 6 follow-up. Both blocking findings survived a dedicated adversarial pass (see DA-9 for the attacks that were tried against them and failed). Requirements coverage — issue #136 acceptance criteria
Suggested-direction items: reasons/warnings split PARTIALLY (F1); Findings (merged, deduped; severity = max across sources)
Adversarial pass — what the Devil's Advocate changedRecorded because a verdict that only ever accumulates findings is not adversarially tested. Partial overturn of a 4-source consensus. Four sources listed the quality-floor bypass notice ( Both published remedies for F2 are defective, in opposite directions. The pure-function seam ( Two security remediations do not deliver what they claim. The proposed Attacks that failed (so the surviving verdict is load-bearing): a second already-correct "unverified" notice (none exists — one Scope checkClean. The diff touches 4 files, +115/−4, all within issue #136's stated surface. The second commit ( The stub was also checked for coverage loss and is a net improvement: No downstream breakage found, each call site verified by reading rather than inference: MCP and GUI both append NextF1 and F2 gate the merge. F1 admits a documentation-only remedy (correct the record) or a code remedy (reclassify the #50 notice, stripping its embedded Verify was run at |
…nts it (#136) Verification found the fix had separated the CLI's rendering while leaving the classification underneath it half-done, and had said otherwise in the permanent record. The unverified-model notice this issue names explicitly was still reasons.append, so it stayed --explain-only while the CHANGELOG listed it among the notices that had been surfaced. The tell was in the string: it began with the literal token "warning: " while living in the reasons array, which is the same conflation #136 exists to end, one layer below the one it fixed. It is now a warning, without the inline prefix -- the CLI adds its own, so a straight move would have rendered "warning: warning: '...' is unverified". It was not merely mis-placed, it was PROTECTED: RouterTests' "Locking an unverified backend warns about unestablished quality" asserted on rec.reason. Name saying one thing, assertion certifying the other. No compiler and no test would have caught the misclassification, because a green test vouched for it. The quality-floor bypass notice deliberately stays in reasons, and an earlier draft of the CHANGELOG wrongly listed it as surfaced too. openspec/specs/asr-routing/spec.md is normative that a locked backend "bypasses the floor with a quality warning in the reasons", with a governing scenario; moving it would be a spec change, not a bug fix. A test now pins each placement and the asymmetry is stated at both sites so it does not read as an oversight. Nothing tested the CLI's printing path at all -- deleting the branch outright left the entire suite green, which is the failure mode this issue's third acceptance line names, reproduced one layer up. The rendering moved into BestASRKit as TranscribeDiagnostics, split so that all three properties are assertable: which lines (branch), how they read (prefix), and WHERE they go. The last is why `destination` is a named constant rather than an inlined stderr -- a pure test of the rendered strings stays green when they are sent to stdout, which for anyone piping a transcript is #136 again. Note on how NOT to test that: redirecting the process's real fd 2 around the call is the more direct proof and is unsafe inside a test bundle. After close(2) the next open() anywhere in the process receives fd 2, and restoring it closes that resource out from under whoever opened it -- including the harness's own result channel, which hangs the run. Measured the hard way. A closed stderr also turned a successful run into a fatal signal. FileHandle.write(_:) raises an uncatchable ObjC exception on write failure, so 2>&- produced SIGABRT (exit 134) and an early-exiting reader such as 2> >(head -n1) produced SIGPIPE (141) -- transcript already on disk, "Wrote ..." already printed. Pre-existing misuse, promoted from --explain-only to the default path by this issue, and reachable from the skill templates in plugins/bestasr/ that gate on $?. Now fputs. Warnings are emitted BEFORE the success line. stderr is unbuffered and stdout is at best line-buffered, so warning-first is the only order that holds under both a terminal and a pipe; the reverse announced the file as written before the reason to distrust it appeared, and made the samples in the CHANGELOG true only when stdout was redirected. Non-vacuous, measured by reverting each guard independently: the original #136 bug (warnings only under --explain) breaks 4 assertions where it previously broke 0; the F1 misclassification breaks 4; sending diagnostics to stdout breaks 2; dropping the prefix breaks 2. 456 tests / 89 suites green. Refs #136
Round-2 verification found the previous commit had made the rendering testable and left the WIRING uncovered -- and #136 was a wiring bug. The reported behaviour is a call-site branch, and re-adding it to the fixed code restored that behaviour verbatim with 456/456 green: if explain { TranscribeDiagnostics.emit(for: result, explain: explain) } Under --explain that renders the explanation to stderr; on the default path it renders nothing. Byte-for-byte the bug the issue reports, inside the commit that claimed to have made it untestable-to-restore. Deleting the call, overriding its stream, and moving it after the success line were all equally green. The previous commit's own non-vacuity claim was wrong for the same reason: it reported that reintroducing the bug broke 4 assertions. What it actually mutated was the new library function's branch. Measured now across three reversions -- the literal call-site restoration breaks 0 (before this commit), the library-layer one breaks 2, the full plumbing revert breaks 5. Never 4. The sentence that row was making -- "before this round, reintroducing the exact bug broke nothing" -- was still true of the round that wrote it. Both statements the CLI used to run now live behind TranscribeDiagnostics.report(_:explain:out:err:), so their ORDER is a property of tested code rather than of two adjacent lines nobody asserts on, and the success line gets its first assertion of any kind. A source-level lock pins the one line that invokes it: measured to refuse all four call-site regressions and to tolerate comments, reflows and renames. Executing the command instead was tried and abandoned. Transcribe.run() calls CommandCore.live() unconditionally and there is no injection seam; $HOME is not one either, because NSHomeDirectory() ignores it on Darwin -- a subprocess test aimed at a fake home silently loads the developer's real ~/.bestasr/engines.json and can spawn a real model load. A subprocess CLI test on this path is non-hermetic by construction today. runMapped's error line still used the raising API, so a FAILING run under 2>&- still aborted; it is fputs now too. Two limits stated rather than implied: fputs fixes the uncatchable-exception class, not SIGPIPE, which is a signal from the underlying write -- and since diagnostics now precede the success line, an early-exiting reader takes the process down sooner than before. And ignoring the write result trades a loud failure for a silent one: under 2>&- warnings are discarded and the run exits 0. Record corrections, all of which claimed more than shipped: - The CHANGELOG said the destination was "asserted by capturing the process's real fd 1 and fd 2". Nothing captures either; that sentence survived from the dup2 approach that was abandoned after it hung the test harness. The type's own doc comment contradicted itself 24 lines apart for the same reason. - "--explain output is unchanged" is not byte-true and cannot be: reasons render as " - " and warnings as " ! ", so reclassifying the #50 notice necessarily moves it between markers. Read byte-literally, A2 would forbid the reclassification the issue's own Suggested direction asks for. The reading that makes #136 coherent -- no loss, no duplication -- is stated instead, and is satisfied. - RouterTests' comment still said the "reasons" must carry the notice, fourteen lines above the assertion corrected to read warnings. That is the defect this PR's thesis names, left sitting above the line that fixed it. - Moving the notice migrates it between fields of the recommend JSON, which cli/spec.md describes normatively and BestASRMCPCore returns as an MCP tool result. A repo sweep sees no consumer; the consumers are agents past the MCP boundary, where a sweep cannot look. Now disclosed. 459 tests / 91 suites green. Refs #136
The remaining items from verification, none of which changes the verdict on the wiring lock but each of which was a claim outrunning the code. fputs takes a NUL-terminated C string, so an embedded U+0000 truncated a warning AND swallowed its terminator, gluing the next warning onto the same physical line. Measured: warnings ["abc\0def", "second"] arrived as one line ending mid-word. Unreachable from argv, but TranscribeOutcome is public and the FileHandle path it replaced had no such limit. Both write sites now put raw UTF-8 bytes through fwrite, and a test pins both halves (content preserved, two lines not one). Ordering had no assertion at all. Captured separately it is invisible -- two files have no relative order -- so the claim that warnings precede the success line was untestable as written. Both streams now go to one file in the test, which is the 2>&1 case the claim is actually about; reordering report() turns it red. The test named "a failed write does not turn a successful run into a fatal signal" proved much less than that. It uses a read-only stream, which is the EBADF class only; SIGPIPE is a signal from the underlying write and no stdio choice suppresses it. Renamed to what it covers, with the limit stated in the body rather than implied by the name. openspec/specs/cli/spec.md enumerated `reason` for the recommend payload and never mentioned `warnings`, so moving the #50 notice took it off the only field the spec names -- on a surface BestASRMCPCore returns verbatim as an MCP tool result, where the consumers are agents a repo sweep cannot see. The spec now enumerates both arrays and says notices move between them, so a consumer needing every notice reads both. Also: the CHANGELOG sample was still hand-wrapped across two lines while the code emits one, and its "now fputs" sentence survived the change to fwrite. And the emit doc comment still opened "fputs rather than FileHandle" after the code stopped using fputs -- prose drifting from code inside the commit that fixed prose drifting from code. 461 tests / 91 suites green. Non-vacuous, measured with a real rebuild (a --skip-build mutation run is a no-op for Swift sources and reads as 0 for every guard): reordering report() breaks 1, reverting fwrite to fputs breaks 2. Refs #136
Verify Report — PR #141, Round 2Re-verify after the round-1 blocking findings were addressed. This report describes Engine4 lens Agents (opus) + sequenced Devil's Advocate + Codex ( AggregateFAIL at F1 is genuinely closed. F2 is not, and the reason is sharper than "insufficient coverage": the fix moved the tested boundary to a place that excludes the thing #136 actually was. Round-1 findings — dispositionEvery R1 finding was checked for silent claiming. None was claimed fixed without being fixed; the two non-fixes that are absent from the PR's "Not in scope" list (F5, F9) are omissions from a disclosure list, not false claims.
Issue #136's acceptance: A1 MET, A2 MET (see the adjudication below — the claim about it was wrong, not the code), A3 still PARTIALLY — which is the blocking item. The adjudication: what F2 actually isAll six verifiers reached "F2 is open". That unanimity is not the interesting part; the reasoning that separated them is. Three sources ran the wrong mutation. logic, regression and Codex each demonstrated the gap by deleting the CLI's call ( The Devil's Advocate ran the right one. #136's reported behaviour is a call-site branch. Re-adding it to the fixed code: if explain { TranscribeDiagnostics.emit(for: result, explain: explain) }Under The DA also put the strongest case for closure on the record before rejecting it, which is worth preserving: no architecture has a fully-tested wiring seam short of executing the entry point; round 1's mutation deleted ten lines of logic where this one deletes one line of wiring; and the rendering that line invokes is now genuinely pinned. That is real progress. It collapses on one fact — #136 was never a rendering bug. The refactor drew the tested boundary to exclude call-site wiring, which is precisely and only where the bug lived. Two of the three published remedies are wrong, in opposite directions. This is the part that changes what someone does:
A working remedy exists and was measured: a source-level wiring lock, 22 lines, 0.003 s, no process spawn and no new production API. Measured against seven tree states — it fails on the guarded call, the deleted call, the stream override and the reordering; it passes on an inserted comment, a reflowed call and a renamed variable. Its honest weakness is that it pins text rather than behaviour: it cannot prove a byte reached fd 2 (the constant and the stream-pair test do that), and a genuine restructuring of The DA also corrected a framing it was handed: PR #140 did not establish a source-text assertion pattern — those tests execute the real script and byte-pin its stdout. What #140 established is Findings (merged; ~30 filed items dedupe to these)
A2, adjudicatedCodex rated the The byte-literal reading of A2 cannot be the intended one, because under it issue #136 contradicts itself. The issue's own Suggested direction asks for exactly the reclassification that changes The reading that makes #136 coherent — no loss, no duplication under Negative space — attacks that failed
What changed in response (
|
Verify Report — PR #141, Round 3Third verification after round 2's blocking finding was addressed. This report describes Engine6 verifiers: 3 lens Agents (opus, isolated clones) + Devil's Advocate (sequenced) + Codex (gpt-5.x, cross-model) + coordinator. Manual fan-out, file-based output. The cross-model leg is back — the round-2 quota exhaustion ( The security lens was not dispatched separately. That was a coordinator judgment about a diff whose surface is one CLI call site, one small library type and a spec line — and it was partly wrong: the logic lens surfaced a security-class finding anyway (R3-9, a forgeable Gates, all re-checked at AggregateFAIL at Round 2's blocker was answered at the layer it named. The lock does catch outright deletion, and The adjudication: where F2 actually standsRound 1 left the behaviour layer untested — Six mutations restore a user-visible regression with 461/461 green. Every one was applied to a real build (
R3-1 — the stream defaults. The chain the record describes, with the broken link marked:
Sharper still: the lock does not merely fail to catch this — it mandates it. Its needle requires the CLI not to override the stream, which is exactly what forces production through the untested default. And On round 2's adjudication. Round 2 rejected the logic lens's remedy ("drop the default, pass the stream explicitly") on the grounds that it "eliminates one mutation by deleting the construct, not by covering it." That was correct at R3-2 — the wiring lock is two substring searches. It strips One further pass, not a guard: emitting Honest boundary on the fix. The DA implemented Codex's proposed remedy and measured that it closes R3-1 but leaves R3-2 completely untouched —
Findings (merged, deduplicated)
Negative space — attacks that failedWhat separates a verified PASS from an unexamined one.
Reviewers overturned
The pattern, third occurrenceEach round's remediation has introduced at least one new claim that measurement refutes, and each time the claim has been about the mechanism that round added:
The correction habit is real — R2-D, R2-E, R2-G, R2-H, R2-I, R2-M all genuinely closed, each verified against the artifact rather than the commit message. What is not improving is the record's reach: the PR body has not been touched in two rounds, and it is the document a reviewer opens first. What would close round 4
Only items 1 and 2 are code. If they are fixed and the two mutation batteries re-run and reported, round 4 should not need a full ensemble. |
Round 3 measured six regressions that restored a user-visible failure with
461/461 green. Two blocking findings, two axes, both now closed and measured.
1. `report`'s stream defaults were read only by production.
The CLI passes neither `out:` nor `err:`; every test passed both. So the
two values that decide where a byte lands were executed by nothing, and
the wiring lock — by requiring the call to carry no override — guaranteed
production went through them. Changing `err:`'s default alone, call site
untouched, put every warning on stdout at 461/461 green, with
`destination == stderr` green, the stream-pair test green, and the lock
green. The CHANGELOG named those two tests as proving a byte reaches fd 2;
one asserts a constant the call site no longer names, the other overrides
both streams.
`bestasr-diagnostics-probe` builds a TranscribeOutcome from argv and calls
`report` passing no streams. A test spawns it with separate pipes on fd 1
and fd 2. No model, no $HOME dependence — which descriptor a byte reaches
does not depend on either. Measured: `err:` flip breaks 4 assertions,
`out:` flip breaks 3.
2. The wiring lock was two substring searches.
Measured green against it: `if (explain) {`, `guard explain else { return }`,
a hoisted `let shouldReport = explain`, the call wrapped in `/* … */`
(which left transcribe printing nothing at all — a block comment deletes
the call while leaving the searched-for text behind), and a `print("Wrote
…")` ahead of it. Measured red: renaming the local `result`, a pure
refactor. Both error modes were backwards, and the entry claimed the
opposite in both directions.
The lock is positional now: walk to the closing paren of the transcribe
call, assert the diagnostics call follows immediately, assert nothing
follows it. Names no variable and no argument label. Measured: all eight
regressions red at 1 assertion each; rename and reflow green.
Also, because they were introduced or exposed by this PR rather than
inherited:
- The NUL fix had landed on the unreachable path. `emit`'s embedded NUL needs
a library caller to construct it; the `error:` channel this PR moved onto
`fputs` embeds an external adapter's stderr verbatim into
TranscriptionError, and adapters are third-party programs (#51). An adapter
emitting `printf 'boom\0DETAIL' >&2` truncated the error and glued the next
one to it, measured. Both channels now share `ConsoleLine`; reverting it to
`fputs` breaks 4 assertions across 2 tests.
- "`--explain` output is unchanged" was false and had survived a round that
claimed to have replaced it. Rendered: the #50 notice went from
` - warning: '…'` to ` ! '…'` — marker and text. No loss and no
duplication, which is the property that matters, is true and now said
instead.
- "warning-first is the only order that holds under both a terminal and a
pipe" was false: `report` flushes both streams, so both orders are stable
on a pty, a pipe and a file. It is a presentation choice, as the type's own
doc comment already said — the diff contradicted itself.
- The CHANGELOG stated in the present tense that the spec does not mention
`warnings` while the same commit had added it.
- The type doc still said `emit` is what the CLI calls. It is `report`; after
the seam landed, `emit` has no production caller at all.
- The recommend spec: scenario synced with its requirement, `profile` and
`language` added (shipped since 471218a, never specified), the consumer
SHALL re-aimed at the CLI, and `measured` corrected from "null otherwise"
to absent — JSONEncoder omits nil optionals, so `"measured" in obj` and
`obj["measured"] is None` disagree. Found by tightening the contract test
to assert shape rather than key presence.
466 tests / 93 suites.
Verify Report — PR #141, Round 4Fourth verification, after round 3's two blocking findings were addressed. This report describes Engine6 verifiers, full: 4 lens Agents (opus, isolated clones) + Devil's Advocate (sequenced) + Codex (gpt-5.6-sol, xhigh, cross-model, diff-only) + coordinator. The security lens was dispatched this round. Round 3's report recorded that skipping it was partly wrong — the logic lens had surfaced a security-class finding anyway. Dispatching it was the right call and it returned PASS on its own axis, with a disciplined attacker model (it explicitly declined to inflate findings that require the attacker to already run code as the user). My round-3 report said a full ensemble should not be needed if the two mutation batteries were re-run and reported. That assumption did not survive the diff: the fix added a new public type, a new executable target, spec changes and a heavy prose rewrite. The full engine was justified, and five of six verifiers found blocking material. Gates, re-checked at AggregateFAIL at Round 3's findings were genuinely addressed, and this must be said before the rest: R3-1 is fully closed (flipping The adjudication: a coverage regression, and a fifth layerThe fix removed a protection while claiming to strengthen one. Round 3's lock matched the literal call text
The first row is #136's title condition: every warning lands on stdout, stderr is empty, and a user running The whole argument axis lost coverage, not just the streams. And two shipped documents assert in the present tense that it did not: Then the fifth layer. The probe's entire value comes from calling
Result: 466/466 green, #136 fully restored. And the Devil's Advocate's own remedy patch — argument-label pin, prefix anchor, lexer — also stays green (G2). Only after adding a pin on the fixture's call shape does it go red (H1).
And the technique has a ceiling. The DA closed On abandoning the text pinRound 2 rejected extracting the command body into a drivable library type as "a larger and more debatable change than the gap it closes". The DA re-costed both halves and recommends — with measurements — not extracting now: apply four pins (one file, 250 lines, 467/467 green, zero false positives), and file the extraction as a separate issue. Its reasoning is that extraction shrinks the residue rather than eliminating it — after extraction a guard can still be added in front of the one-line delegation, so a text pin is still required, just over one line instead of a multi-statement body. I accept that recommendation. Findings (merged, deduplicated)
Negative space — attacks that failed
Reviewers overturned — including me
The pattern, fourth occurrence — and a new variantRounds 1–3 each left the next layer down untested. Round 4 did something different and worse in one respect: it removed an existing protection as a side effect of adding a new one, and described the result as strictly stronger. The rename fix was real; the argument-axis loss went unnoticed because the mutation battery only tested what the old lock failed to catch, never re-testing what it had caught. That is the transferable lesson: when you replace a guard, re-run the mutations the old guard passed, not just the ones it failed. Nothing in four rounds of this PR's record does that, and it is the one check that would have caught R4-1 before it shipped. What would close round 5The DA produced a measured, ready-to-apply patch:
Only items 1–5 are blocking, all five have measured fixes, and the sweep shows they compose without false positives. |
Round 4 found that round 4's own lock rewrite removed a protection while claiming to strengthen one, plus four more holes. All five blocking findings are closed and measured; the battery now has a second half that would have caught the regression before it shipped. The regression, first, because it is the useful part. Round 3's lock matched the whole call text, so it implicitly forbade any extra argument. Round 4 replaced it with a positional anchor to stop a rename of the local `result` turning it red. The rename tolerance was real. What went with it was not noticed, because the mutation battery only re-ran the mutations the OLD lock had FAILED to catch — never the ones it had caught. Measured: report(result, explain: explain, err: stdout) 466/466 green at 2bd7156 RED, 1 issue, at 9637ed2 Same for `out:/err:` swapped, `explain: false` (which makes --explain a no-op) and `explain: true` (which drops the `warning:` token the skill templates key on). The whole argument axis, not just the streams. Lesson, and the reason section B of the battery now exists: when you replace a guard, re-run what the old guard passed, not only what it failed. Closed, each measured on a real build: - The call site's arguments. The anchor now pins labels — no `out:`/`err:`, and `explain:explain` forwarded rather than decided. Names no local, so the rename that round 4 set out to tolerate stays green. Asserting the label alone was not enough: `explain: false` contains `explain:`. - The probe's own call shape. Its entire value is that it passes no streams, and nothing asserted that. Two innocuous edits — fixture passes streams, `err:` default flips — restored #136 at 466/466 green. Same label pin now applies to the fixture. - A `guard` hoisted ABOVE the transcribe call. The anchor only looked at what followed. It now extends backwards to `runMapped {` and requires exactly one binding between. Matched by shape, not by name. - The decoy. `range(of:)` takes the file's first match, and block comments were deliberately left in place, so a `/* historical note */` showing the old shape — or a single string literal — plus deleting the real call left the suite green with `transcribe` printing nothing. A ~45-line token-hiding lexer closes it, and incidentally fixes a false failure: a comment merely sitting between the two statements used to turn the lock red, while its own doc said such edits stay green. - ConsoleLine's wiring. The writer was pinned; nothing pinned that `runMapped` still called it, so reverting those two lines to `fputs` was green and silently restored the NUL truncation. Both branches are now counted. - The probe test's assertions were `contains`-only and ignored terminationStatus. A leak that only occurs on the default path — raw unprefixed warnings copied to stdout — was green, because the one test that compares exactly passes both streams explicitly and never executes the defaults. Exact equality now, plus the exit status. Also, since leaving a known-false claim is what generates the next round: - `fflush` was the premise of this round's ordering argument and was pinned by nothing; deleting it was green. A test now reads a fully-buffered stream while it is still open. - The spec required `language`, which `JSONEncoder` omits when nil — the same mechanism the same paragraph correctly diagnosed for `measured`, and the tightened contract test could not see it because every fixture injects a detector that always resolves. `language` is now conditional, with a test that drives the nil branch. - `cli/spec.md` said `warnings` carries what bears on trusting the output, which read as a rule would place the quality-floor bypass notice there — and `asr-routing/spec.md` mandates it in `reasons`. The split is now described as editorial, and `Router.swift`'s comment no longer says no spec governs the placement, because as of the previous commit one does. - Three residues of claims already retracted elsewhere: `--explain output is unchanged` (third location), the buffering rationale in `report`'s doc — which the previous commit message cited as the one that had it right — and two `Package.swift` mechanism claims measurement refutes (`swift run` does surface a non-product executable target; `nm` finds 23 probe symbols in the test bundle). - ConsoleLine's scope is stated: five other sites still use the raising API and still abort under `2>&-`, one on every transcribe run. Filed separately. 28 mutations, zero deviations. 470 tests / 93 suites.
1 participant
Fixes the reported bug in #136 and four rounds of verification findings against the fix itself.
The bug
Every warning the router produced was folded into the
explanationprose block, whichbestasr transcribeprinted only under--explain. So the entire output of a run that silently substituted a different backend was:Warnings now print to stderr on the default path:
reasonexplains a choice and is legitimately opt-in. A warning is what the reader needs in order to trust the file that was just written. Both arrays already existed onASRRecommendation; only the CLI conflated them.Also unhidden by the same change: the
--backend X is unavailable; selecting automaticallysubstitution notice (the #121 case), the cold-start memory-downgrade warnings, and the #105 declared-language gate.What four rounds of verification found
Each round found that the previous round's fix left the next layer untested — and round 4 found something different: that round 4's own fix had removed a protection while claiming to strengthen one.
lines(for:explain:)emit+destinationreport+ a source-level lockbestasr-diagnostics-probe, a real processThe regression, because it is the useful part
Round 3's lock matched the whole call text, so it implicitly forbade any extra argument. Round 4 replaced it with a positional anchor to stop a rename of the local
resultturning it red. The rename tolerance was real. The loss was not noticed, because the mutation battery only re-ran the mutations the old lock had failed to catch — never the ones it had caught:Same for
out:/err:swapped,explain: false(which makes--explaina no-op) andexplain: true(which drops the literalwarning:token the skill templates key on). The whole argument axis, not just the streams.Non-vacuity — 28 mutations, measured
Each applied to a real build (
--skip-buildis a silent no-op for Swift sources), full suite, then reverted.A — the five round-4 blocking findings
err: stdoutout: stderr, err: stdoutexplain: falseexplain: trueerr:default flipped (two edits)guard explain else { return }above the transcribe callrunMappedreverted tofputsexit(70)after correct outputB — everything an earlier guard caught, re-checked
err:default →stdout;out:default →stderr;destination→stdout;if explain {;if (explain) {;guard explain elsebetween the statements; hoistedlet shouldReport;/* … */wrapping the call; the call deleted;printbefore it; a statement after it;ConsoleLine→fputs; the two statements insidereportreordered; the #50 reclassification reverted — all 14 RED.C — benign refactors
rename
result→outcome; a line comment plus a four-line reflow; a block comment sitting between the two statements — all GREEN. The last was a false failure until this round.28 expected, 0 deviations.
What the lock now does
It runs the source through a token-hiding lexer (line comments, block comments, string literals — a lexer, not a parser), extends backwards to
runMapped {requiring exactly one binding between it and the transcribe call, requires the diagnostics call immediately after that call and nothing after it, and pins the argument labels: noout:/err:, andexplain:explainforwarded rather than decided. It names no local, so renames stay green. The probe fixture gets the same label pin, because the probe only measures the defaults for as long as it passes none.What no text pin can do
Stated plainly, because three rounds of this PR's record implied otherwise: a text pin's coverage is exactly the lexical block it anchors to. Hoisting the same
guardone level further out — intorun(), aboverunMapped— is green under every version of this lock, including the current one, and no finite set of anchors changes that. Proving the line executes means running the executable, andTranscribe.run()callsCommandCore.live()unconditionally whileNSHomeDirectory()ignores$HOMEon Darwin, so a subprocess test aimed at a fake home reads the developer's real~/.bestasr/engines.json.Extraction is filed as #156. It would shrink the residue to a single delegation line, not remove it — which is why it is a separate refactor rather than part of this fix.
Other fixes
emit's embedded NUL needs a library caller to construct it; theerror:channel embeds an external adapter's stderr verbatim intoTranscriptionError.message, and adapters are third-party programs registered from~/.bestasr/engines.json(通用 external-process engine 協定 — 長尾 ASR 家族的掛載機制 (follow-up from #35) #51). Measured end-to-end:printf 'boom\0DETAIL' >&2truncated the error and glued the next one to it. Both channels shareConsoleLine, and both branches are counted by a test.fflushis pinned. It is the premise of this round's ordering argument and nothing asserted it — deleting it was green, because the ordering test passes oneFILE*as both streams and every other test reads afterfclose. A test now reads a fully-buffered stream while it is still open.--explainoutput is not "unchanged". The 中文 high-value ASR 家族評估 — FluidAudio 已內建 Paraformer/SenseVoice（零新依賴，優先於 MLX-Swift Qwen3-ASR）(follow-up from #35) #50 notice went from- warning: '…'to! '…'— marker and text. What holds, and is what the issue's second acceptance line is about, is that nothing is lost and nothing is duplicated.ConsoleLineflushes every write, so both orders are stable on a pty, a pipe and a file — measured.recommendspec.profileadded;measuredcorrected from "null otherwise" to absent (JSONEncoderomits nil optionals, so"measured" in objandobj["measured"] is Nonedisagree);languagelikewise made conditional after it turned out to have the same shape — with a test that drives the nil branch, which no fixture did before. Thereason/warningssplit is described as editorial rather than as a classification rule, because read as a rule it would place the quality-floor bypass notice inwarningswhileasr-routing/spec.mdmandates it inreasons.External surface
Moving the #50 notice from
reasontowarningsmigrates it between fields of therecommendJSON, whichBestASRMCPCore/Server.swiftreturns verbatim as an MCP tool result. No field was added or removed and the payload still carries the notice, but its location changed and that is observable: a client keying onreasonspecifically stops seeing it. A repo-wide sweep finds no such consumer — but the real consumers are agents on the far side of the MCP boundary, where a sweep cannot look. Very likely harmless, not provably harmless.Scope
--quietopt-out. The issue floated one parenthetically; there is no verbosity surface to be consistent with. The honest consequence: with a cold WhisperKit cache the auto-detect path warns on every run, and the only way to silence it is2>/dev/null, which also discardserror:.diagnosealready printed warnings unconditionally;recommendalready emits them in its JSON.benchmarknever callsRouter.recommendat all.bestasr-diagnostics-probeis not a package product.release-app.shandrelease-mcp.shbuild per-product;install.shcopies two named binaries; CI publishes nothing. Verified four ways. (swift run bestasr-diagnostics-probedoes work —swift runtakes target names — but that is a developer convenience, not a distribution path.)Not fixed here — follow-ups filed
--languagecontrol characters can forge a top-levelwarning:line. Severity raised by this PR: before, that string appeared only indented inside--explain.bestasr transcribeskill templates pass--explain, so the repo's flagship consumer never sees the surface this PR added.2>&-, one of them on every transcribe run.ConsoleLine's documentation now states this scope rather than implying the class is closed.470 tests / 93 suites green.
Refs #136