From be88f30875c4b79cf3462085c79d31d1d8d22979 Mon Sep 17 00:00:00 2001 From: che cheng Date: Sat, 1 Aug 2026 17:01:40 +0800 Subject: [PATCH 1/2] fix: validate metric and escape baseline values before they reach the CI log (#117) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit baseline-compare.py printed corpus / language / metric verbatim, and that stdout IS the log a human reads to approve a release. metric was the one rendered field validated NOWHERE — the worklist stage covers corpus and language, and unlike golden/tolerance/error_rate it never passes through float(), so a hostile string arrived intact. Two layers, because they fail differently: - metric must now be cer|wer. A bounded vocabulary makes a bad value a loud gate error instead of a decorative label. - every rendered value is escaped at the RENDER boundary. Validation lives in the gate's worklist stage, but this script is a separate entry point that re-validates nothing it is handed — so a field added to these messages later is covered without the author having to remember. Verified independently: the reproduced payload 'cer\x1b[32m\rFAKE ALL-PASS' needs no timing window, only a committed baseline. Both layers are locked by mutation — deleting safe() reddens 3 assertions, deleting the metric check reddens 2. Escaped, not stripped: \x1b stays visible so the offending bytes remain diagnosable. Refs #117 --- CHANGELOG.md | 13 +++++ .../RegressionBaselineTests.swift | 34 +++++++++++++ scripts/lib/baseline-compare.py | 51 +++++++++++++++++-- 3 files changed, 93 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 945b19b..45fa4b3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -38,6 +38,19 @@ All notable changes to bestASR are documented here. The format follows ### Fixed +- **Baseline values no longer reach the CI log unescaped (#117)**: + `scripts/lib/baseline-compare.py` interpolated `corpus` / `language` / + `metric` straight into the pass/fail lines it prints, and that stdout is the + log a human reads to decide whether a release is safe. `metric` was validated + **nowhere** — the worklist stage checks corpus and language, and unlike + `golden` / `tolerance` / `error_rate` it never passes through `float()` — so + a committed baseline carrying `"cer\x1b[32m\rFAKE ALL-PASS"` repainted the + line into a forged green verdict, with no timing window needed. `metric` now + has to be `cer` or `wer`, and every rendered value is escaped at the render + boundary so a field added to these messages later is covered without the + author having to remember. Escaped rather than stripped — the offending bytes + stay diagnosable. + - **Baseline `language` field validated before the worklist TSV (#112)**: `scripts/regression-gate.sh` whitelisted the `corpus` field but wrote `language` into the same line-oriented TSV unchecked, so an embedded newline diff --git a/Tests/BestASRKitTests/RegressionBaselineTests.swift b/Tests/BestASRKitTests/RegressionBaselineTests.swift index ee90cbe..18fa0d7 100644 --- a/Tests/BestASRKitTests/RegressionBaselineTests.swift +++ b/Tests/BestASRKitTests/RegressionBaselineTests.swift @@ -159,4 +159,38 @@ struct RegressionBaselineTests { ]) #expect(r.exit == 0) } + + // MARK: - baseline values reaching the CI log (#117) + + @Test func `an unknown metric is a gate error, not a label nobody checks`() throws { + // metric was the one rendered field validated NOWHERE: the worklist + // stage checks corpus and language, and unlike golden/tolerance it + // never passes through float(). A bounded vocabulary makes a bad value + // loud instead of decorative. + var hostile = entry + hostile["metric"] = "cer\u{1B}[32m\rFAKE ALL-PASS" + let r = try runCompare( + baseline: [hostile], + measured: [["corpus": "c1", "metric": "cer", "error_rate": 0.10]]) + #expect(r.exit != 0) + #expect(r.output.contains("unknown metric")) + } + + @Test func `control characters never reach the log, even on the passing path`() throws { + // Defense in depth at the RENDER boundary: language passes the worklist + // whitelist in the gate, but this script is a separate entry point that + // re-validates nothing. The payload repaints a CI line — CR returns the + // cursor and the SGR turns it green — so a human reading the log to + // approve a release sees a forged verdict. Escaped, it is inert AND + // still diagnosable. + var hostile = entry + hostile["language"] = "zh\u{1B}[32m\rFAKE ALL-PASS" + let r = try runCompare( + baseline: [hostile], + measured: [["corpus": "c1", "metric": "cer", "error_rate": 0.10]]) + #expect(r.exit == 0, "a hostile label must not change the verdict") + #expect(!r.output.contains("\u{1B}"), "ANSI escape reached the log") + #expect(!r.output.contains("\r"), "carriage return reached the log") + #expect(r.output.contains("\\x1b"), "the offending bytes should stay visible") + } } diff --git a/scripts/lib/baseline-compare.py b/scripts/lib/baseline-compare.py index 55c0d51..ec0f47c 100755 --- a/scripts/lib/baseline-compare.py +++ b/scripts/lib/baseline-compare.py @@ -14,17 +14,58 @@ import json import sys +# The accuracy metrics the gate knows how to judge. Same vocabulary the bench +# validator enforces for `metric_kind`, kept as a set here so an unknown value +# is a loud gate error rather than a label nobody notices (#117). +METRICS = {"cer", "wer"} + +# Every C0 control, DEL, and every C1 control, mapped to a visible escape. +# Baseline values reach stdout verbatim, and this stdout IS the CI log a human +# reads to decide whether a release is safe. A `metric` carrying CR + an ANSI +# SGR sequence can repaint the line — the reproduced payload +# "cer\x1b[32m\rFAKE ALL-PASS" renders as a green pass over the real verdict +# (#117). Escaping rather than deleting keeps the offending bytes diagnosable. +_CONTROL_CHARS = {c: f"\\x{c:02x}" for c in range(0x20)} +_CONTROL_CHARS[0x7F] = "\\x7f" +_CONTROL_CHARS.update({c: f"\\x{c:02x}" for c in range(0x80, 0xA0)}) + + +def safe(value) -> str: + """Render a baseline-supplied value inert for a one-line log message. + + Defense in depth, deliberately at the RENDER boundary: validation lives in + the gate's worklist stage, but this script is a separate entry point that + re-validates nothing it is handed. Any field added to these messages later + is covered without the author having to remember (#117). + """ + return str(value).translate(_CONTROL_CHARS) + def main() -> int: data = json.load(sys.stdin) failures = 0 + # `metric` is the one rendered field with no validation ANYWHERE — the + # worklist stage checks corpus and language, nothing checks this. Unlike + # golden/tolerance/error_rate it never passes through float(), so a hostile + # string reaches the log intact (#117). + for e in data.get("baseline", []): + metric = e.get("metric") + if metric not in METRICS: + print(f"✗ GATE ERROR: baseline corpus '{safe(e.get('corpus'))}' has " + f"unknown metric {safe(metric)!r} — expected one of " + f"{'|'.join(sorted(METRICS))}") + failures += 1 + if failures: + print(f"\n✗ regression gate: {failures} failure(s).") + return 1 + # Duplicate corpus names would silently collapse (last-wins) in the dicts # below — surface them as gate errors instead (#34 verify). for side in ("baseline", "measured"): names = [e["corpus"] for e in data.get(side, [])] for dup in sorted({n for n in names if names.count(n) > 1}): - print(f"✗ GATE ERROR: duplicate corpus '{dup}' in {side} " + print(f"✗ GATE ERROR: duplicate corpus '{safe(dup)}' in {side} " f"— entries would silently shadow each other") failures += 1 if failures: @@ -37,7 +78,7 @@ def main() -> int: for corpus, m in measured.items(): b = baseline.get(corpus) if b is None: - print(f"✗ GATE ERROR: measured corpus '{corpus}' has no baseline entry " + print(f"✗ GATE ERROR: measured corpus '{safe(corpus)}' has no baseline entry " f"— add it to benchmarks/baseline.json (never silently pass)") failures += 1 continue @@ -45,17 +86,17 @@ def main() -> int: actual = float(m["error_rate"]) diff = actual - golden if diff > tol: - print(f"✗ REGRESSION {corpus} [{b['language']}] {b['metric']}: " + print(f"✗ REGRESSION {safe(corpus)} [{safe(b['language'])}] {safe(b['metric'])}: " f"golden {golden:.4f} → measured {actual:.4f} " f"(+{diff:.4f} > tolerance {tol:.4f})") failures += 1 else: - print(f"✓ {corpus} [{b['language']}] {b['metric']}: " + print(f"✓ {safe(corpus)} [{safe(b['language'])}] {safe(b['metric'])}: " f"golden {golden:.4f} → measured {actual:.4f} ({diff:+.4f})") for corpus in baseline: if corpus not in measured: - print(f"✗ GATE ERROR: baseline corpus '{corpus}' was never measured " + print(f"✗ GATE ERROR: baseline corpus '{safe(corpus)}' was never measured " f"— gate cannot verify it (run fetch-corpora / check registration)") failures += 1 From 03c7801f47d2428f5e032523d4885e248f77019b Mon Sep 17 00:00:00 2001 From: che cheng Date: Sat, 1 Aug 2026 17:22:47 +0800 Subject: [PATCH 2/2] docs: narrow four claims this PR could not back (#117) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Verify found the PR asserting more than the code does — four times, in a change whose whole subject is a log that must not mislead. No behaviour changes; the 14/14 byte-identical certification stands. - safe()'s docstring said a later-added field is 'covered without the author having to remember'. It is not: the wrapping is explicit at 11 sites and a new f-string is unprotected until someone wraps it. The docstring now states that, plus the scope it actually has (C0/DEL/C1, so no ANSI sequence and no cursor return) and the scope it does NOT (U+2028, bidi overrides — no control byte needed; unreachable here only because corpus and language are whitelisted upstream). - 'keeps the offending bytes diagnosable' overstated it: backslash is not escaped, so a real ESC renders identically to a benign literal '\x1b'. Now 'stays visible'. - The CHANGELOG's 'unlike golden/tolerance/error_rate it never passes through float()' implied float() was validating those. It is a conversion — it accepts NaN and Infinity, and the gate then reports exit 0 on a real regression. Filed as #134 rather than fixed here: the suggested three-line fix was tested and does not close the class (a JSON string "NaN" never reaches parse_constant). - The headline claimed baseline values no longer reach the CI log unescaped, but regression-gate.sh still echoes model unescaped. Scoped to 'the compare stage'. Not fixed here because PR #126 deletes the exact heredoc involved and already rejects ESC/CR via MODEL_RE — a fix here would collide. Also records the risk no leg had named: METRICS here, MetricKind in Swift and metric_kind in the bench validator are three copies with no coupling test, and because the check returns early, one legitimate future metric would suppress the verdict for every corpus. Refs #117 --- CHANGELOG.md | 21 ++++++++++++-------- scripts/lib/baseline-compare.py | 34 +++++++++++++++++++++++---------- 2 files changed, 37 insertions(+), 18 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 45fa4b3..cee161a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -38,18 +38,23 @@ All notable changes to bestASR are documented here. The format follows ### Fixed -- **Baseline values no longer reach the CI log unescaped (#117)**: +- **The compare stage no longer renders baseline values unescaped (#117)**: `scripts/lib/baseline-compare.py` interpolated `corpus` / `language` / `metric` straight into the pass/fail lines it prints, and that stdout is the log a human reads to decide whether a release is safe. `metric` was validated - **nowhere** — the worklist stage checks corpus and language, and unlike - `golden` / `tolerance` / `error_rate` it never passes through `float()` — so - a committed baseline carrying `"cer\x1b[32m\rFAKE ALL-PASS"` repainted the + **nowhere**: the worklist stage checks corpus and language, and `metric` is + the only rendered field that is neither charset-checked nor numeric, so it + reaches the log as-is. (`float()` on the numbers is a *conversion*, not a + validation — it accepts `NaN` and `Infinity`; see #134.) A committed baseline + carrying `"cer\x1b[32m\rFAKE ALL-PASS"` repainted the line into a forged green verdict, with no timing window needed. `metric` now - has to be `cer` or `wer`, and every rendered value is escaped at the render - boundary so a field added to these messages later is covered without the - author having to remember. Escaped rather than stripped — the offending bytes - stay diagnosable. + has to be `cer` or `wer`, and every rendered value is wrapped in an escaper + that neutralizes C0/DEL/C1 — enough that no value can emit an ANSI sequence or + return the cursor. Escaped rather than stripped, so the value stays visible. + Two honest limits: the wrapping is explicit at each interpolation, so a new + f-string is not covered until someone wraps it; and Unicode format characters + (U+2028, bidi overrides) are out of scope — they need no control byte, and are + unreachable here only because corpus and language are whitelisted upstream. - **Baseline `language` field validated before the worklist TSV (#112)**: `scripts/regression-gate.sh` whitelisted the `corpus` field but wrote diff --git a/scripts/lib/baseline-compare.py b/scripts/lib/baseline-compare.py index ec0f47c..c9aadd4 100755 --- a/scripts/lib/baseline-compare.py +++ b/scripts/lib/baseline-compare.py @@ -14,9 +14,14 @@ import json import sys -# The accuracy metrics the gate knows how to judge. Same vocabulary the bench -# validator enforces for `metric_kind`, kept as a set here so an unknown value -# is a loud gate error rather than a label nobody notices (#117). +# The accuracy metrics the gate knows how to judge. Same vocabulary as Swift's +# `MetricKind` and the bench validator's `metric_kind`, so an unknown value is a +# loud gate error rather than a label nobody notices (#117). +# +# The three definitions are duplicated with NO coupling test. Adding a third +# metric on the Swift side without adding it here does not just reject that +# corpus — the check below returns early, so one legitimate entry would suppress +# the verdict for every other corpus in the run. METRICS = {"cer", "wer"} # Every C0 control, DEL, and every C1 control, mapped to a visible escape. @@ -24,19 +29,28 @@ # reads to decide whether a release is safe. A `metric` carrying CR + an ANSI # SGR sequence can repaint the line — the reproduced payload # "cer\x1b[32m\rFAKE ALL-PASS" renders as a green pass over the real verdict -# (#117). Escaping rather than deleting keeps the offending bytes diagnosable. +# (#117). Escaping rather than deleting keeps the value visible, so a reader +# can still see roughly what was there. _CONTROL_CHARS = {c: f"\\x{c:02x}" for c in range(0x20)} _CONTROL_CHARS[0x7F] = "\\x7f" _CONTROL_CHARS.update({c: f"\\x{c:02x}" for c in range(0x80, 0xA0)}) def safe(value) -> str: - """Render a baseline-supplied value inert for a one-line log message. - - Defense in depth, deliberately at the RENDER boundary: validation lives in - the gate's worklist stage, but this script is a separate entry point that - re-validates nothing it is handed. Any field added to these messages later - is covered without the author having to remember (#117). + """Neutralize ANSI/terminal control codes in a baseline-supplied value. + + Defense in depth at the render boundary: validation lives in the gate's + worklist stage, but this script is a separate entry point that re-validates + nothing it is handed. + + SCOPE, so callers do not over-trust it: this escapes C0, DEL and C1 — + enough that a value cannot emit an ANSI sequence or return the cursor. It + does NOT cover Unicode format characters (U+2028/U+2029, bidi overrides), + which can reorder or split a line in a Unicode-aware viewer without using + any control byte; those are unreachable through the gate today because + corpus and language are `fullmatch`ed upstream and metric is bounded above. + And it must be called EXPLICITLY at each interpolation — a new f-string + added below is not covered until someone wraps it (#117). """ return str(value).translate(_CONTROL_CHARS)