From 771769f747d22265969aa6ea6945c3aa02b45456 Mon Sep 17 00:00:00 2001 From: n1ckyb Date: Sat, 8 Aug 2026 05:28:50 +0100 Subject: [PATCH] fix(cli): correct the diff header, and stop warning on every run MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two defects that each produced a CORRECT diff and still made the tool look broken. Neither could fail a diff test, so neither was caught. 1. 'intentumdiff file a.py b.py' labelled both sides 'b.py', so the header said a file had been compared with itself — directly above the right answer. Source.get_content returns one filename because that is what language detection needs, and for a git diff one name genuinely covers both sides. FileSource is the exception and knew both names all along, so it now surfaces them via display_names(). 2. Every run wrote an OSV advisory warning to stderr naming an 'allow vulnerable' override. Nothing was actionable: first-party plugins ship inside the wheel and third-party ones stay blocked either way. It is now debug. The deliberate-override warning stays a warning, since disabling a safety check is worth saying, but fires once per process rather than once per plugin — there are 78 plugins. Also drops 'Scope: working tree' from diffs where no working tree exists; it was a renderer default printed for file, string and patch diffs alike, and is now shown only when a staging status is actually known. Fixes #12 Fixes #13 Co-Authored-By: Claude Opus 5 --- src/intentumdiff/cli/_shared.py | 6 +- src/intentumdiff/differ.py | 9 ++- src/intentumdiff/plugins/loader.py | 24 +++++-- src/intentumdiff/sources/base.py | 15 +++++ src/intentumdiff/sources/file_source.py | 9 +++ tests/unit/test_cli_header_and_stderr.py | 82 ++++++++++++++++++++++++ 6 files changed, 137 insertions(+), 8 deletions(-) create mode 100644 tests/unit/test_cli_header_and_stderr.py diff --git a/src/intentumdiff/cli/_shared.py b/src/intentumdiff/cli/_shared.py index 9589af1..1b90a0d 100644 --- a/src/intentumdiff/cli/_shared.py +++ b/src/intentumdiff/cli/_shared.py @@ -347,14 +347,16 @@ def _render_terminal(diff: SemanticDiff) -> None: ) ) return - scope_label = (diff.staging_status or "working tree").replace("_", " ") header = Table.grid(padding=(0, 2)) header.add_column(style="bold") header.add_column() header.add_row("Old", diff.old_filename or "") header.add_row("New", diff.new_filename or "") header.add_row("Language", diff.language or "unknown") - header.add_row("Scope", scope_label) + # Only a git-backed diff has a staging scope. Defaulting to "working tree" told + # someone comparing two local files that a working tree was involved when none was. + if diff.staging_status: + header.add_row("Scope", diff.staging_status.replace("_", " ")) header.add_row("Changes", str(len(diff.changes))) _console.print( Panel( diff --git a/src/intentumdiff/differ.py b/src/intentumdiff/differ.py index 54e346d..ed1a1b9 100644 --- a/src/intentumdiff/differ.py +++ b/src/intentumdiff/differ.py @@ -491,13 +491,20 @@ def diff(self, source: Source) -> SemanticDiff: profiler = self._new_profiler() with profiler.phase("source_loading"): old_content, new_content, filename, language_hint = source.get_content() - return self._run_pipeline( + diff = self._run_pipeline( old_content, new_content, filename, language_hint, _profiler=profiler, ) + # The pipeline works from the single filename language detection needs. A source + # comparing two differently named files knows both, so restore them here rather + # than threading a second name through every construction site. + names = source.display_names() + if names is not None: + diff = diff.model_copy(update={"old_filename": names[0], "new_filename": names[1]}) + return diff def diff_strings( self, diff --git a/src/intentumdiff/plugins/loader.py b/src/intentumdiff/plugins/loader.py index 85eb39e..966b8aa 100644 --- a/src/intentumdiff/plugins/loader.py +++ b/src/intentumdiff/plugins/loader.py @@ -40,6 +40,9 @@ ) logger = logging.getLogger(__name__) + +# Emitted once per process, not once per plugin load: there are 78 plugins. +_OVERRIDE_WARNED: bool = False _MAX_TELEMETRY_RECORDS = 128 # --------------------------------------------------------------------------- @@ -404,13 +407,24 @@ def _check_osv_cache_or_block( # Stamp is fresh but no cache file: the last fetch failed. override = os.environ.get("INTENTUMDIFF_ALLOW_VULNERABLE_WASMTIME", "").strip() if override in ("1", "true", "yes"): - logger.warning( - "INTENTUMDIFF_ALLOW_VULNERABLE_WASMTIME: loading plugins with " - "unverified OSV status — last advisory fetch failed." - ) + # Worth saying — a safety check has been deliberately disabled — but worth + # saying ONCE. Emitting it per plugin load repeated it up to 78 times a run. + global _OVERRIDE_WARNED + if not _OVERRIDE_WARNED: + _OVERRIDE_WARNED = True + logger.warning( + "INTENTUMDIFF_ALLOW_VULNERABLE_WASMTIME: loading plugins with " + "unverified OSV status — last advisory fetch failed." + ) return if trusted or _is_trusted_wasm_path(wasm_path): - logger.warning( + # DEBUG, not WARNING. This fires on every ordinary run whenever the machine + # is offline or the advisory cache has expired, and there is nothing for the + # user to do about it: first-party plugins ship inside the wheel and + # third-party ones stay blocked either way. Emitting it at warning level put + # alarming text — naming a "allow vulnerable" override — on stderr beside + # correct results, which is precisely how 0.0.1 came to look broken. + logger.debug( "Loading first-party trusted Wasm plugin with unverified OSV " "status because the last advisory fetch failed. Third-party " "plugins remain blocked without INTENTUMDIFF_ALLOW_VULNERABLE_WASMTIME=1." diff --git a/src/intentumdiff/sources/base.py b/src/intentumdiff/sources/base.py index 4c6b1d8..1e354f6 100644 --- a/src/intentumdiff/sources/base.py +++ b/src/intentumdiff/sources/base.py @@ -26,3 +26,18 @@ def get_content(self) -> tuple[str, str, str, str | None]: ``language_hint`` — e.g. "python", "sql". Pass None to auto-detect. """ ... + + def display_names(self) -> tuple[str, str] | None: + """ + Distinct ``(old, new)`` names, when the two sides are named differently. + + ``get_content`` returns ONE filename because that is what language detection + needs, and for most sources it is also the right thing to display: a git diff + compares one path at two revisions, so both sides share a name. + + ``FileSource`` is the exception — it compares two separately named files — and + collapsing both sides onto the new name made the CLI report that it had diffed + a file with itself. Sources that know two names override this; ``None`` means + "one name applies to both", which stays the default. + """ + return None diff --git a/src/intentumdiff/sources/file_source.py b/src/intentumdiff/sources/file_source.py index 4e96f6a..9dea41d 100644 --- a/src/intentumdiff/sources/file_source.py +++ b/src/intentumdiff/sources/file_source.py @@ -46,3 +46,12 @@ def get_content(self) -> tuple[str, str, str, str | None]: old_content = self._old_path.read_text(encoding="utf-8", errors="replace") new_content = self._new_path.read_text(encoding="utf-8", errors="replace") return old_content, new_content, self._filename, self._language_hint + + def display_names(self) -> tuple[str, str] | None: + # This source is the one that genuinely has two names. An explicit ``filename`` + # overrides both, and identical basenames need no distinction. + if self._filename != self._new_path.name: + return None + if self._old_path.name == self._new_path.name: + return None + return self._old_path.name, self._new_path.name diff --git a/tests/unit/test_cli_header_and_stderr.py b/tests/unit/test_cli_header_and_stderr.py new file mode 100644 index 0000000..b1ccfe2 --- /dev/null +++ b/tests/unit/test_cli_header_and_stderr.py @@ -0,0 +1,82 @@ +"""The first thing a user sees, and the noise printed beside it. + +Both defects covered here produced *correct diffs*. The classification was right, the +change count was right, the exit code was 0 — and the tool still looked broken, because +the header said it had compared a file with itself and stderr carried a warning naming a +"allow vulnerable" override. + +That is why these assertions exist separately from the diff tests: a passing diff test +was never going to catch either one. +""" + +from __future__ import annotations + +import logging + +from intentumdiff import SemanticDiffer +from intentumdiff.sources.file_source import FileSource +from intentumdiff.sources.string_source import StringSource + +OLD = "def greet(name):\n return 'hi ' + name\n" +NEW = "def greet(name):\n if not name:\n return None\n return 'hi ' + name\n" + + +def _files(tmp_path): + old = tmp_path / "a.py" + new = tmp_path / "b.py" + old.write_text(OLD, encoding="utf-8") + new.write_text(NEW, encoding="utf-8") + return old, new + + +def test_two_files_keep_their_own_names(tmp_path): + # The reported bug: both sides were labelled with the NEW filename, so the header + # claimed a file had been diffed against itself. + old, new = _files(tmp_path) + diff = SemanticDiffer().diff(FileSource(old, new)) + assert diff.old_filename == "a.py" + assert diff.new_filename == "b.py" + + +def test_an_explicit_display_filename_still_wins(tmp_path): + # FileSource lets a caller override the display name; that must apply to both sides + # rather than being half-overridden by the fix above. + old, new = _files(tmp_path) + diff = SemanticDiffer().diff(FileSource(old, new, filename="renamed.py")) + assert diff.old_filename == diff.new_filename == "renamed.py" + + +def test_identical_basenames_are_left_alone(tmp_path): + # The common case for a git-style comparison: one path, two versions. Nothing should + # be "corrected" here. + a = tmp_path / "one" / "mod.py" + b = tmp_path / "two" / "mod.py" + for p, text in ((a, OLD), (b, NEW)): + p.parent.mkdir(parents=True, exist_ok=True) + p.write_text(text, encoding="utf-8") + diff = SemanticDiffer().diff(FileSource(a, b)) + assert diff.old_filename == diff.new_filename == "mod.py" + + +def test_a_non_git_diff_claims_no_staging_scope(): + # The header used to print "Scope: working tree" unconditionally, which is simply + # untrue for a diff of two in-memory strings. + diff = SemanticDiffer().diff(StringSource(OLD, NEW, "example.py")) + assert diff.staging_status is None + + +def test_a_successful_diff_logs_nothing_at_warning_or_above(caplog, tmp_path, monkeypatch): + # 0.0.1 printed ~69 plugin errors on every run while returning correct results, and + # the exit code called that success. This asserts the quiet the user should get. + # Assert what a USER gets. The suite sets the allow-vulnerable override, whose + # warning is legitimate precisely because it is deliberate. + monkeypatch.delenv("INTENTUMDIFF_ALLOW_VULNERABLE_WASMTIME", raising=False) + old, new = _files(tmp_path) + with caplog.at_level(logging.WARNING): + diff = SemanticDiffer().diff(FileSource(old, new)) + + assert diff.changes, "fixture should produce at least one change" + noisy = [r for r in caplog.records if r.levelno >= logging.WARNING] + assert not noisy, "a successful diff must be silent, got: " + "; ".join( + f"{r.levelname} {r.getMessage()[:120]}" for r in noisy + )