fix(errors): strip terminal colour codes before a failure reaches the user (#1344) (#1353)

* fix(errors): strip terminal colour codes before a failure reaches the user (#1344)

yt-dlp colourizes stderr whenever it thinks a terminal is attached, and the
frozen backend's pipes are enough for it to think so. A restricted-video
failure surfaced as `download: ^[[0;31mERROR:^[[0m [youtube] …`, which reads
as an OmniVoice bug rather than a message from YouTube.

Fixed at build_failure, the choke point every surfaced failure passes
through, so the whole class is covered — ffmpeg, uv, pip, cargo and
anything else that colours its output, not just the reported command.

Order matters and is pinned by a test: strip_ffmpeg_banner anchors on
"ffmpeg version " at the start of a line, so a leading colour code would
hide it and quietly reinstate #1309 for any colour-emitting ffmpeg build.
The pattern covers CSI and OSC (window title / hyperlink) sequences, not
just SGR colour, and never empties a non-empty message.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* fix(errors): escape-only text falls back to the error class, not to bytes

CodeRabbit, both valid:

- strip_ansi kept the original when stripping left nothing visible, so
  build_failure copied raw escape bytes into reason/error/detail — the very
  thing this PR exists to stop. 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 escapes is not, so let it do that.
- the test file bound `from core import failure` at import time. Sibling
  suites reload and purge core.* between tests, so that alias can outlive
  the module the app uses and the file would assert against a stale copy
  while looking green — #1269 exactly. Resolved via a fixture at call time.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>

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
 

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)

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: <urlopen error timed out>"
+    coloured = f"{RED}ERROR:{RESET} [youtube] abc: Unable to download webpage: <urlopen error timed out>"
+    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"