diff --git a/CHANGELOG.md b/CHANGELOG.md index 594ecac8..551f09df 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -44,6 +44,7 @@ The bundled TTS model package (`pyproject.toml`) is versioned independently. - Linux AppImage: a permanently blank window on Mesa 26.1+ hosts (Arch/CachyOS and other rolling distros) — the bundled WebKit ran against a newer system Mesa than it was built for, and no environment variable could help because the failure precedes every rendering flag; the launcher now lets a newer system WebKitGTK take precedence — thanks @rvasilev and @HannaLovvold! (#1258, #1244) - Linux AppImage: `OMNIVOICE_PREFER_SYSTEM_WEBKIT=1` forces your own WebKitGTK for hosts where its version can't be read automatically (no `pkg-config`), and `=0` forces the bundled one (#1258) - Dubbing: the transcription overlay said "Transcribing with Whisper…" whatever ASR engine was actually running — it now names the stage, in all 21 languages — thanks @paoloantinori! (#1352) +- Error messages no longer arrive with terminal colour codes spliced into the sentence (`download: ^[[0;31mERROR:^[[0m …`) — every surfaced failure is cleaned now, whichever tool produced it. (#1344) ### Docs diff --git a/backend/core/failure.py b/backend/core/failure.py index 2e98117c..80fe4679 100644 --- a/backend/core/failure.py +++ b/backend/core/failure.py @@ -470,6 +470,40 @@ def strip_ffmpeg_banner(text: Optional[str]) -> str: return out or str(text).strip() +# Terminal escape sequences from a captured stderr (#1344). Command-line tools +# colourize their output whenever they think a terminal is attached, and the +# frozen backend's pipes are enough for several of them to decide that: yt-dlp +# reported "download: ^[[0;31mERROR:^[[0m [youtube] …" verbatim into the UI, +# where the escapes render as literal mojibake in the middle of the sentence. +# +# Stripping matters for more than looks: this text is also the input to the +# pattern matching below. ``classify()``'s patterns happen to match past the +# escape and survive it, but ``strip_ffmpeg_banner`` is anchored to the start +# of a line — a leading colour code hides "ffmpeg version " from it and +# quietly reinstates #1309 for any ffmpeg build that colours its output. Hence +# the strip runs FIRST, at the choke point every failure passes through. +# +# CSI (colour, cursor movement) plus OSC (window title / hyperlink), which +# yt-dlp and ffmpeg both emit and which carries its own terminator. +_ANSI_ESCAPE = re.compile( + r"\x1B(?:\][^\x07\x1B]*(?:\x07|\x1B\\)|[@-Z\\-_]|\[[0-?]*[ -/]*[@-~])" +) + + +def strip_ansi(text: Optional[str]) -> str: + """Remove terminal escape sequences from a captured stderr/stdout. + + Returns the text unchanged when there are none, and empty when the input + was nothing BUT escapes. Emptying it is correct rather than lossy: + ``build_failure`` already falls back to the exception class name for an + empty ``reason``, and a class name is a real answer where a run of escape + bytes copied into ``reason``/``error``/``detail`` is not (CodeRabbit). + """ + if not text: + return text or "" + return _ANSI_ESCAPE.sub("", str(text)) + + def sanitize(text: Optional[str]) -> str: """Redact secrets and strip the home path from a string. @@ -713,10 +747,13 @@ def build_failure( error_class = "Error" raw = str(exc_or_msg).strip() or "Unknown failure" - # Strip ffmpeg's stderr banner before anything reads the text — both the + # Normalise the captured text before anything reads it — both the # user-facing reason and classify() below, which would otherwise be - # matching against a build configuration string (#1309). - raw = strip_ffmpeg_banner(raw) + # matching against a build configuration string (#1309) or against + # terminal colour codes (#1344). + # ANSI first: the banner matcher anchors on "ffmpeg version " at the start + # of a line, which a leading colour code would push out of reach. + raw = strip_ffmpeg_banner(strip_ansi(raw)) reason = sanitize(raw) or error_class docs_topic = classify(raw) # HF_MIRROR_UNREACHABLE's hint is dynamic (it names the configured mirror) diff --git a/tests/test_failure_ansi_stripping.py b/tests/test_failure_ansi_stripping.py new file mode 100644 index 00000000..0ef38d98 --- /dev/null +++ b/tests/test_failure_ansi_stripping.py @@ -0,0 +1,138 @@ +"""#1344 — terminal colour codes must never reach the user, or the classifier. + +yt-dlp colourizes its stderr whenever it believes a terminal is attached, and +the frozen backend's pipes are enough for it to believe that. A restricted-video +failure arrived in the UI as:: + + download: ^[[0;31mERROR:^[[0m [youtube] jsWTu0rJcmE: Video unavailable… + +The visible defect is that the escapes render as literal mojibake in the middle +of the sentence, which reads as a bug in OmniVoice rather than a message from +YouTube. + +The less visible one is that the same text is the input to the pattern matching +that picks the docs topic, the hint, and the ffmpeg-banner strip. Most of those +patterns match on substrings well past the escape and survive it; the anchored +one does not — ``strip_ffmpeg_banner`` requires "ffmpeg version " at the start +of a line, and a leading colour code pushes it out of reach, quietly +reinstating #1309 for any ffmpeg build that colours its output. So the strip +has to happen BEFORE that matcher, not merely before display. + +The fix lives in ``build_failure`` — the choke point every surfaced failure +passes through — so it covers the whole class (yt-dlp, ffmpeg, uv, pip, cargo, +anything else that colours stderr), not just the reported command. +""" +import importlib +import os +import sys + +import pytest + +os.environ.setdefault("OMNIVOICE_MODEL", "test") +os.environ.setdefault("OMNIVOICE_DISABLE_FILE_LOG", "1") + +sys.path.insert(0, os.path.join(os.path.dirname(__file__), "..", "backend")) + + +@pytest.fixture +def failure(): + """Resolve ``core.failure`` at call time, never at import time. + + A module-level ``from core import failure`` binds whichever object exists + during collection, and sibling suites reload/purge ``core.*`` between + tests — so the alias can outlive the module the app is actually using and + this file would then assert against a stale copy while looking green. That + is #1269 exactly, and it is a repo-wide rule for ``tests/**`` rather than a + quirk of this file (CodeRabbit). + """ + return importlib.import_module("core.failure") + +RED = "\x1b[0;31m" +RESET = "\x1b[0m" + +# The reported string, verbatim apart from the escapes being real here. +_YTDLP = ( + f"{RED}ERROR:{RESET} [youtube] jsWTu0rJcmE: Video unavailable. This video " + "is restricted. Please check the Google Workspace administrator and/or the " + "network administrator restrictions." +) + + +def test_strip_ansi_removes_colour_codes(failure): + assert failure.strip_ansi(_YTDLP) == ( + "ERROR: [youtube] jsWTu0rJcmE: Video unavailable. This video is " + "restricted. Please check the Google Workspace administrator and/or " + "the network administrator restrictions." + ) + + +def test_strip_ansi_handles_cursor_and_osc_sequences(failure): + """Not only SGR colour: progress output moves the cursor, and OSC sequences + (window title / hyperlinks) carry their own terminator, so a naive + ``\\x1b\\[...m`` pattern leaves half of one behind.""" + assert failure.strip_ansi("a\x1b[2K\x1b[1Gb") == "ab" + assert failure.strip_ansi("x\x1b]0;some title\x07y") == "xy" + assert failure.strip_ansi("x\x1b]8;;https://example.com\x1b\\y") == "xy" + + +def test_strip_ansi_passes_clean_text_through(failure): + assert failure.strip_ansi("plain text") == "plain text" + assert failure.strip_ansi("") == "" + assert failure.strip_ansi(None) == "" + + +def test_escape_only_input_becomes_empty(failure): + """A message made only of escapes is not a message, so it must not survive + into the UI as a run of escape bytes.""" + assert failure.strip_ansi(RED + RESET) == "" + + +def test_escape_only_failure_falls_back_to_the_error_class(failure): + """...and ``build_failure`` still honours its non-empty ``reason`` + guarantee, by naming the exception class instead. A class name is a real + answer; escape bytes copied into reason/error/detail are not.""" + fields = failure.build_failure(ValueError(RED + RESET), stage="download") + assert fields["reason"] == "ValueError" + assert fields["error"] == "ValueError" + for key in ("reason", "error", "detail"): + assert "\x1b" not in fields[key], key + + +def test_build_failure_reason_is_free_of_escapes(failure): + fields = failure.build_failure(_YTDLP, stage="download") + for key in ("reason", "error", "detail"): + assert "\x1b" not in fields[key], f"{key} still carries terminal escapes" + assert fields["reason"].startswith("ERROR: [youtube]") + + +def test_classification_survives_colourized_output(failure): + """Invariance guard, not a repair: today's ``classify`` patterns all match + on substrings past the escape, so this passes with or without the strip. + It is here so that stays true — a future pattern anchored at the start of + the message would otherwise give a user whose tool emits colour different + guidance from one whose tool does not, with nothing to catch it.""" + plain = "ERROR: [youtube] abc: Unable to download webpage: " + coloured = f"{RED}ERROR:{RESET} [youtube] abc: Unable to download webpage: " + assert failure.classify(coloured) == failure.classify(plain) + + plain_fields = failure.build_failure(plain, stage="download") + coloured_fields = failure.build_failure(coloured, stage="download") + for key in ("docs_topic", "hint", "docs_url"): + assert coloured_fields[key] == plain_fields[key], ( + f"colourized stderr changed {key}: a user with a colour-emitting " + f"tool gets different guidance than one without" + ) + + +def test_ffmpeg_banner_is_still_stripped_when_colourized(failure): + """Order matters: ``strip_ffmpeg_banner`` anchors on 'ffmpeg version ' at + the start of a line, so a leading colour code would push it out of reach and + quietly reinstate #1309 for any colour-emitting ffmpeg build.""" + raw = ( + f"{RED}ffmpeg version N-125781-gacf6b520c1-20260727{RESET} Copyright (c) 2000-2026\n" + " built with gcc 14\n" + " configuration: --enable-gpl\n" + "Output file does not contain any stream\n" + ) + fields = failure.build_failure(raw, stage="export") + assert fields["reason"] == "Output file does not contain any stream"