Skip to content

fix(audio): clean scratch files and surface refinement errors - #3687

Merged
kovtcharov-amd merged 4 commits into
amd:mainfrom
kovtcharov:codex/transcription-maintenance-followup
Sep 24, 2026
Merged

kovtcharov-amd merged 4 commits into
amd:mainfrom
kovtcharov:codex/transcription-maintenance-followup

Conversation

@kovtcharov

Copy link
Copy Markdown
Contributor

Failed transcription now cleans up its temporary audio without deleting the source, and speaker refinement reports model failures instead of returning incomplete work as success. This follows the already merged #3597 and also corrects the Lemonade startup guidance.

Compared with the open provider work in #3672: its audio edits change endpoint/auth resolution, while this patch changes cleanup and failure reporting.

Test plan

  • pytest tests/unit/test_audio_tools.py tests/unit/test_lemonade_asr.py -q: 115 passed on current main; one existing live Lemonade test skipped because no server was available.
  • Black and isort checks on the four affected Python files; staged diff check.
  • Independent review of the maintenance patch before publication.
  • Fresh CI, live transcription/refinement and human review.

@github-actions github-actions Bot added documentation Documentation changes audio Audio (ASR/TTS) changes tests Test changes agents labels Sep 11, 2026
@kovtcharov
kovtcharov marked this pull request as ready for review September 17, 2026 20:04
@github-actions

Copy link
Copy Markdown
Contributor

Verdict: Approve with suggestions

This fixes two quiet failures in the transcription path: a temp directory that was created for every decode and never deleted, and speaker-naming steps that swallowed a dead model connection and wrote a transcript that looked complete but wasn't. Both are real user-facing problems and both are now covered by tests that pin the behaviour rather than just the call.

Nothing here blocks the merge. What's left is polish:

  • When refinement fails, the user is told "server down" and nothing else. They don't learn which stage failed, that their raw transcript is still safe on disk, or that re-running is the fix. The transcription path already goes out of its way to say exactly that on failure — refinement should match it.
  • The old temp-file cleanup is now doing nothing. The decoded audio always lives inside the new scratch directory, so removing that directory already removes the file. The extra step is harmless but dead.
  • One new test sits in the wrong test class, and the diarized branch of refinement has no end-to-end failure test — only the text-only branch does.

Also worth knowing: when Lemonade isn't installed at all, the new "not reachable" message now tells the user to run gaia init twice in the same sentence pair.

Real-world evidence

N/A — no evidence bundle was produced for this run, and the gh CLI and shell were unavailable in this environment, so I could not read the PR description to check for evidence the author attached. The verdict rests on static review plus the new unit tests alone.

Transcription needs real media plus a live Lemonade server, so this is reasonably deferred to the strix-halo lane. What would settle it there: one transcribe_media run on a real recording confirming no gaia-media-* directory is left behind in the temp folder afterwards, and one refine_transcript run against a stopped server confirming it now returns an error instead of a transcript with placeholder speakers.

🔍 Technical details

Issues

🟢 Refinement errors lose all context (src/gaia/agents/tools/audio_tools.py:708)

_refine_transcript returns the bare exception string, so a dropped connection surfaces as {"status": "error", "error": "server down"}. The new test asserts exactly that. Compare _transcribe_media:574-583, which deliberately attaches transcript_path and a note because "Minutes of compute already landed on disk. Losing the path here is what makes a user pay for it twice." The same is true here — the raw transcript and its timings are untouched, and the call is safely retryable, but nothing says so. (Losing the naming work itself is pre-existing — _name_turns failures already propagated — so that part isn't on this PR.)

        except Exception as e:
            logger.error("Refinement failed for %s: %s", source, e)
            return {
                "status": "error",
                "error": (
                    f"Speaker identification failed: {e}. The raw transcript at "
                    f"{source} is intact — retry refine_transcript on it once "
                    "the model is reachable again."
                ),
                "source_transcript": str(source),
            }

🟢 Dead WAV cleanup (src/gaia/agents/tools/audio_tools.py:587-595)

to_wav16k_mono now always writes into Path(scratch.name) / "audio.16k.wav" and returns that path, so scratch.cleanup() already removes it. The whole if wav_path is not None: unlink block in finally can be deleted — keep the wav_path variable itself, it's still what's handed to client.transcribe and _diarize_if_possible.

🟢 Test in the wrong class (tests/unit/test_audio_tools.py:59-76)

test_speaker_processing_does_not_hide_infrastructure_failure is a refinement-behaviour test living in TestRegistration, which otherwise only asserts KNOWN_TOOLS wiring and tool registration. TestRefineTranscript is where a reader will look for it.

🟢 Diarized branch has no end-to-end failure test (tests/unit/test_audio_tools.py:398)

test_consolidation_failure_returns_error_without_output covers the text-only path. The acoustic_labels is not None branch (audio_tools.py:681-690) calls _name_known_voices, which this PR also made raise — but nothing asserts that _refine_transcript turns that into an error with no file written. That branch is the normal path when diarization works, so it's the one more users hit. A parametrized variant over _name_known_voices / _consolidate_speakers would cover both.

🟢 Doubled install advice when Lemonade isn't installed (src/gaia/audio/lemonade_asr.py:627-635)

In the not-installed case describe_start_hint().instruction is already "Lemonade Server is not installed. Run gaia init to install it, or set LEMONADE_SERVER_PATH to an existing install." (lemonade_launcher.py:366-371), so the assembled message reads "…not reachable at X (…). Lemonade Server is not installed. Run gaia init to install it, or set LEMONADE_SERVER_PATH… Run gaia init to install it, or set LEMONADE_BASE_URL…" — the same instruction twice. No suggestion block here because the obvious trim (dropping the tail's install clause) would break test_unreachable_server_names_url_and_next_step's assert "gaia init" in message for the installed case; it needs a small rethink of which half owns the install advice.

Strengths

  • The leak is genuinely fixed, not papered over. _default_dest (media.py:266-268) called mkdtemp per decode and only the WAV inside it was ever unlinked — so every transcription left an empty gaia-media-* directory behind. Passing an owned dest and cleaning the directory is the right shape, and test_owned_scratch_directory_is_removed_on_failure parametrizes over both failure points (decode raising, and transcribe raising after decode succeeded) while also asserting the source file is untouched.
  • The fail-loudly change is done completely. Both except Exception: return named swallows are gone, _consolidate_speakers moved inside the try so its failure actually reaches the error return, and the behaviour change is reflected in both docs that describe it (docs/guides/transcription.mdx, docs/sdk/sdks/audio.mdx) rather than one. Checked hub/skills/transcribe-meeting/SKILL.md too — it never claimed the old anonymous-label fallback, so it doesn't contradict.
  • The lemonade-server serve removal is enforced, not just done. The ASR test now asserts the string is absent as well as asserting the new hint is present, so the removed-CLI rule can't quietly regress here.

Kalin Ovtcharov added 2 commits September 18, 2026 23:44
…nstall advice

A failed speaker-naming pass returned only the bare exception ("server down"),
so the user never learned that the raw transcript was intact and the call
could simply be retried. The error now names the stage, the untouched raw
transcript, and the retry, and returns it as `source_transcript`.

When Lemonade isn't installed, the unreachable-server message printed the
`gaia init` install advice twice: once from the start hint and once from its
own tail. The hint now owns start/install advice and the tail only adds the
LEMONADE_BASE_URL alternative.

Also drops the WAV unlink that the scratch-directory cleanup already covers,
moves the speaker-processing test into the refinement tests, and adds a
failure test for the diarized branch alongside the text-only one.
@kovtcharov

Copy link
Copy Markdown
Contributor Author

All five suggestions are in (adc6b7d), and the branch now includes current main (it was 154 commits behind). When speaker identification fails, the user is now told which step failed, that the raw transcript is untouched, and that re-running refine_transcript is the fix. The install advice no longer prints twice when Lemonade is missing.

  • Refinement errors lose context: the error now names the failed step, the raw transcript path and how to retry, and returns source_transcript. Both docs that describe the failure say so too.
  • Dead WAV cleanup: removed. The scratch-directory cleanup already deletes the WAV.
  • Test in the wrong class: moved into the refinement tests.
  • No diarized failure test: the failure test now runs on both the diarized and text-only paths. It checks that no output is written and that the raw transcript and timings are unchanged.
  • Install advice printed twice: the start hint now owns the start and install advice. The message itself only adds the LEMONADE_BASE_URL option.
🔍 Technical details
  • _refine_transcript error: "Speaker identification failed: {e}. The raw transcript at {source} and its timings are unchanged; fix the cause and call refine_transcript on it again." plus source_transcript. The wording doesn't assume a connection error, since RuntimeError("empty reply") also lands here.
  • _transcribe_media: dropped the Path(wav_path).unlink block and the now-unused wav_path = None. Removed test_scratch_wav_is_removed_even_on_failure, which mocked the decoder to write outside the scratch directory, so it only exercised the deleted unlink. test_owned_scratch_directory_is_removed_on_failure covers the real path.
  • LemonadeASRClient._unreachable: "… not reachable at {url} ({error}). {describe_start_hint().instruction} If it runs elsewhere, set LEMONADE_BASE_URL to that server. See {DOCS_URL}". The installed-case test now asserts LEMONADE_BASE_URL rather than gaia init. New test_unreachable_without_lemonade_gives_install_advice_once runs the real describe_start_hint with Lemonade unresolved and asserts gaia init appears exactly once.
  • New test: test_naming_failure_returns_error_without_output[diarized|text-only].
  • pytest tests/unit/test_audio_tools.py tests/unit/test_lemonade_asr.py tests/unit/test_lemonade_launcher.py tests/unit/test_media.py: 172 passed, 7 skipped. Deselected TestRealServer::test_real_transcription_round_trip, which needs a live Lemonade on the default port. black, isort, flake8 and pylint clean.
  • Still open from the review: a live check on real media (no gaia-media-* directory left behind; refinement against a stopped server returns an error). That needs a machine with Lemonade and Whisper.

@kovtcharov-amd

Copy link
Copy Markdown
Collaborator

Merged main in — the Test Gaia Agent failure here was not this branch's doing. It was a check that main itself fixed after this branch forked, so the branch was failing on stale code.

🔍 Technical details

The failing step was Run Skill Framework Tests, specifically tests/unit/test_starter_skills.py::test_starter_skill_tools_required_are_real_tools[coding] — AssertionError: coding declares tools_required that no mixin registers. The coding skill started requiring run_python, added by #3994; #4051 then taught the starter-skill guard to see it. This branch predated both.

The merge was clean. pytest tests/unit/test_starter_skills.py on the merged head → 188 passed, 15 skipped.

The same stale-base failure hit #3687, #3911, #3982 and #3616 identically.

@kovtcharov-amd
kovtcharov-amd added this pull request to the merge queue Sep 24, 2026
Merged via the queue into amd:main with commit 72f6bb6 Sep 24, 2026
39 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

agents audio Audio (ASR/TTS) changes documentation Documentation changes tests Test changes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants