From a1ba3a3df2ac7dbda24f9dab2e04197edc64997f Mon Sep 17 00:00:00 2001 From: Max Ghenis Date: Sat, 10 Oct 2026 20:07:51 -0400 Subject: [PATCH 1/2] Classify verify-raw's Wrangler diagnostics on whole phrases, after stripping colour codes The review of #334 found two misclassifications among failing entries. Colour codes inside the missing-object phrase produced fetch_error, and unrelated text that merely mentioned NoSuchKey produced missing_remote. Neither could report verified. _is_missing_object_diagnostic now strips ANSI sequences and collapses whitespace, matches markers only as whole phrases (not inside a longer identifier), and counts a marker only on a line that holds nothing but markers or is an error line. Invariants, each with tests: - Colour codes at any position of the missing-object message still give missing_remote (exhaustive over every insertion point). - A marker inside a longer identifier, or on a line reporting no error, gives fetch_error. - The bare markers and Wrangler's real captured output keep giving missing_remote; no status other than the missing/fetch-error split changes. Co-Authored-By: Claude Opus 5.5 --- .../334-verify-raw-diagnostics.fixed.md | 6 ++ chronicle/artifacts.py | 39 ++++++++++--- tests/test_chronicle_verify_raw.py | 56 +++++++++++++++++++ 3 files changed, 92 insertions(+), 9 deletions(-) create mode 100644 changelog.d/334-verify-raw-diagnostics.fixed.md diff --git a/changelog.d/334-verify-raw-diagnostics.fixed.md b/changelog.d/334-verify-raw-diagnostics.fixed.md new file mode 100644 index 00000000..a7e2f6ab --- /dev/null +++ b/changelog.d/334-verify-raw-diagnostics.fixed.md @@ -0,0 +1,6 @@ +Make `chronicle verify-raw` classify Wrangler diagnostics more carefully. Colour +codes are stripped before matching, so a missing object is still reported as +`missing_remote` when Wrangler colours words inside the message. A marker now +counts only as a whole phrase on a line that holds nothing but markers or is an +error line, so unrelated text that merely mentions `NoSuchKey` stays a +`fetch_error`. Neither case ever reported `verified`. diff --git a/chronicle/artifacts.py b/chronicle/artifacts.py index 4929bae6..9b5b5444 100644 --- a/chronicle/artifacts.py +++ b/chronicle/artifacts.py @@ -10,6 +10,7 @@ from datetime import UTC, datetime from pathlib import Path import posixpath +import re import shlex import sqlite3 import subprocess @@ -968,15 +969,7 @@ def result( return result("fetch_error", str(exc)) values["fetch"] = fetch if not fetch.ok: - missing = any( - marker in fetch.stderr.lower() - for marker in ( - "nosuchkey", - "specified key does not exist", - "object not found", - "object does not exist", - ) - ) + missing = _is_missing_object_diagnostic(fetch.stderr) return result("missing_remote" if missing else "fetch_error", "r2_get_failed") values["sha256_remote"] = remote_digest values["size_bytes_remote"] = remote_size @@ -1011,6 +1004,34 @@ def _hash_stream(stream: BinaryIO) -> tuple[str, int]: return digest.hexdigest(), size_bytes +_ANSI_ESCAPE = re.compile(r"\x1b\[[0-?]*[ -/]*[@-~]") +_MISSING_OBJECT_MARKER = re.compile( + r"(? bool: + """Return whether Wrangler's diagnostics say the object is absent. + + Colour codes are removed first, because Wrangler wraps words in them. A + marker counts only as a whole phrase, and only on a line that holds nothing + but markers (such as ``NoSuchKey: specified key does not exist``) or is an + error line, so unrelated text that merely mentions a marker stays a fetch + error. + """ + for raw_line in _ANSI_ESCAPE.sub("", stderr).lower().splitlines(): + line = " ".join(raw_line.split()) + if _MISSING_OBJECT_MARKER.search(line) is None: + continue + remainder = _MISSING_OBJECT_MARKER.sub("", line).strip(" .:;()[]-") + if not remainder or _ERROR_LINE.search(line): + return True + return False + + def _stream_r2_object( location: ArtifactStorageLocation, *, wrangler_command: str ) -> tuple[str, int, ArtifactCommandResult]: diff --git a/tests/test_chronicle_verify_raw.py b/tests/test_chronicle_verify_raw.py index c7e974b0..9ada9d01 100644 --- a/tests/test_chronicle_verify_raw.py +++ b/tests/test_chronicle_verify_raw.py @@ -355,6 +355,62 @@ def test_verify_raw_missing_object_markers_and_other_errors(tmp_path): assert _verify(root, fake).entries[0].status == "fetch_error" +# Wrangler's real stderr for an absent object, captured from a live read-back. +_REAL_WRANGLER_MISSING = ( + "\x1b[31m\u2718 \x1b[41;31m[\x1b[41;97mERROR\x1b[41;31m]\x1b[0m " + "\x1b[1mThe specified key does not exist.\x1b[0m\n\n\n" + '\U0001fab5 Logs were written to "/tmp/wrangler.log"\n' +) + + +@pytest.mark.parametrize( + ("stderr", "status"), + [ + (_REAL_WRANGLER_MISSING, "missing_remote"), + # Colour codes inside the phrase itself. + ( + "\x1b[31m\u2718 [ERROR]\x1b[0m The specified \x1b[1mkey\x1b[0m does " + "\x1b[4mnot\x1b[0m exist.", + "missing_remote", + ), + ("An error occurred (NoSuchKey) when calling GetObject", "missing_remote"), + (" Object not found. ", "missing_remote"), + ("NoSuchKey: specified key does not exist", "missing_remote"), + # A marker inside a longer identifier is not the marker. + ("error: NoSuchKeyHandler could not start", "fetch_error"), + ("error: nosuchkeys table is locked", "fetch_error"), + # A marker merely mentioned on a line that reports no error. + ("note: see the docs on NoSuchKey\nconnection reset by peer", "fetch_error"), + ("warning: Object not found pages are cached for 60s", "fetch_error"), + ( + "retrying; last reply said the specified key does not exist yet", + "fetch_error", + ), + ], +) +def test_verify_raw_classifies_wrangler_diagnostics(tmp_path, stderr, status): + root = tmp_path / "db" / "data" + _, key = _add_artifact(root) + fake = FakeWrangler(tmp_path / "fake") + fake.set_remote({}, {f"ledger-raw/{key}": stderr}) + + report = _verify(root, fake) + + assert not report.valid + assert report.entries[0].status == status + + +def test_verify_raw_missing_marker_survives_colour_codes_at_any_position(tmp_path): + root = tmp_path / "db" / "data" + _, key = _add_artifact(root) + fake = FakeWrangler(tmp_path / "fake") + phrase = "[ERROR] The specified key does not exist." + for position in range(len(phrase) + 1): + coloured = phrase[:position] + "\x1b[1;31m" + phrase[position:] + "\x1b[0m" + fake.set_remote({}, {f"ledger-raw/{key}": coloured}) + assert _verify(root, fake).entries[0].status == "missing_remote", position + + def test_verify_raw_missing_executable_is_fetch_error(tmp_path): root = tmp_path / "db" / "data" _add_artifact(root) From fc9532add6901e53058e4c2c5fe6077e47632327 Mon Sep 17 00:00:00 2001 From: Max Ghenis Date: Sun, 11 Oct 2026 06:05:30 -0400 Subject: [PATCH 2/2] Qualify the diagnostics changelog and pin the known error-line limit Review r1 of #336: an error line that merely mentions NoSuchKey still gives missing_remote, so the fragment overstated the fix. Say so plainly, pin that case in the parametrised test, and label the captured Wrangler bytes as the hub's observation rather than a retained log. Co-Authored-By: Claude Opus 5.5 --- changelog.d/334-verify-raw-diagnostics.fixed.md | 16 ++++++++++------ tests/test_chronicle_verify_raw.py | 8 +++++++- 2 files changed, 17 insertions(+), 7 deletions(-) diff --git a/changelog.d/334-verify-raw-diagnostics.fixed.md b/changelog.d/334-verify-raw-diagnostics.fixed.md index a7e2f6ab..9fed6109 100644 --- a/changelog.d/334-verify-raw-diagnostics.fixed.md +++ b/changelog.d/334-verify-raw-diagnostics.fixed.md @@ -1,6 +1,10 @@ -Make `chronicle verify-raw` classify Wrangler diagnostics more carefully. Colour -codes are stripped before matching, so a missing object is still reported as -`missing_remote` when Wrangler colours words inside the message. A marker now -counts only as a whole phrase on a line that holds nothing but markers or is an -error line, so unrelated text that merely mentions `NoSuchKey` stays a -`fetch_error`. Neither case ever reported `verified`. +Make `chronicle verify-raw` classify Wrangler diagnostics more carefully. +Colour codes are now stripped before matching, so a missing object is still +reported as `missing_remote` when Wrangler colours words inside the message. A +marker now counts only as a whole phrase, and only on a line that holds nothing +but markers or that reports an error. So a marker inside a longer identifier, +or on a line that reports no error, now gives `fetch_error`. An error line that +merely mentions a marker (for example +`error: could not reach bucket; NoSuchKey handling disabled`) is still +classified as `missing_remote`. Both statuses are failures, and neither change +can produce `verified`. diff --git a/tests/test_chronicle_verify_raw.py b/tests/test_chronicle_verify_raw.py index 9ada9d01..39ea4ea7 100644 --- a/tests/test_chronicle_verify_raw.py +++ b/tests/test_chronicle_verify_raw.py @@ -355,7 +355,8 @@ def test_verify_raw_missing_object_markers_and_other_errors(tmp_path): assert _verify(root, fake).entries[0].status == "fetch_error" -# Wrangler's real stderr for an absent object, captured from a live read-back. +# Wrangler's stderr for an absent object, as the NZ hub observed it in a live +# read-back on 2026-10-10 (bytes reproduced here, not a retained log). _REAL_WRANGLER_MISSING = ( "\x1b[31m\u2718 \x1b[41;31m[\x1b[41;97mERROR\x1b[41;31m]\x1b[0m " "\x1b[1mThe specified key does not exist.\x1b[0m\n\n\n" @@ -386,6 +387,11 @@ def test_verify_raw_missing_object_markers_and_other_errors(tmp_path): "retrying; last reply said the specified key does not exist yet", "fetch_error", ), + # Known limit: an error line that merely mentions a marker still counts. + ( + "error: could not reach bucket; NoSuchKey handling disabled", + "missing_remote", + ), ], ) def test_verify_raw_classifies_wrangler_diagnostics(tmp_path, stderr, status):