From f77f36542acc8b3be45fda322c6b3fcfb40a2165 Mon Sep 17 00:00:00 2001 From: TJ Webb Date: Mon, 20 Jul 2026 11:10:08 -0700 Subject: [PATCH 1/3] fix(i18n/audit): eliminate false positives in raw-site classification Two bugs fixed: 1. f-string false positives (_has_string_literal) Previously any ast.JoinedStr was unconditionally classified as 'raw', meaning pure-variable f-strings like f"{result}" or f" {msg}" were counted as un-extracted literals even though they contain no translatable content. Fixed by walking the f-string's values list and only returning True when at least one ast.Constant part has non-whitespace text. Impact: 23 false positives reclassified as 'dynamic' (1302 -> 1279 raw). 2. Single-file path silently returning 0 sites (_iter_py_files) os.walk(file_path) on a regular file yields nothing, so 'python -m code_puppy.i18n.audit path/to/module.py' always reported 0 sites. Fixed by detecting a file path up front and yielding it directly if it ends in .py. Tests added (5 new, 25 total): test_fstring_pure_variable_is_dynamic test_fstring_whitespace_only_literal_is_dynamic test_fstring_with_content_and_variable_is_raw test_fstring_with_only_arrow_is_raw test_single_file_path_is_accepted test_single_file_pure_variable_fstring_not_raw All 25 audit tests pass. ruff clean. --- code_puppy/i18n/audit.py | 26 +++++++++++++++++-- tests/i18n/test_i18n_audit.py | 47 +++++++++++++++++++++++++++++++++++ 2 files changed, 71 insertions(+), 2 deletions(-) diff --git a/code_puppy/i18n/audit.py b/code_puppy/i18n/audit.py index d2ed7b720..d277bd072 100644 --- a/code_puppy/i18n/audit.py +++ b/code_puppy/i18n/audit.py @@ -136,11 +136,23 @@ def _is_translation_call(node: ast.expr) -> bool: def _has_string_literal(node: ast.expr) -> bool: - """True if the expression *contains* a hard-coded string literal.""" + """True if the expression *contains* a hard-coded string literal. + + For f-strings we only return True when at least one constant segment + contains non-whitespace text. Pure-variable f-strings like ``f"{x}"`` + or ``f" {x}"`` carry no translatable literal and are classified as + dynamic instead. + """ if isinstance(node, ast.Constant): return isinstance(node.value, str) if isinstance(node, ast.JoinedStr): # f-string - return True + # An f-string is only "raw" when it has at least one constant part + # with meaningful (non-whitespace) text, e.g. f"Error: {e}". + # Pure-variable forms like f"{var}" or f" {var}" are dynamic. + return any( + isinstance(v, ast.Constant) and isinstance(v.value, str) and v.value.strip() + for v in node.values + ) if isinstance(node, ast.BinOp): # "a" + x, "a" % x return _has_string_literal(node.left) or _has_string_literal(node.right) if isinstance(node, ast.Call): @@ -198,6 +210,16 @@ def audit_source(source: str, path: str) -> List[Site]: def _iter_py_files(root: str) -> Iterable[str]: + """Yield every .py file under ``root``. + + If ``root`` is itself a ``.py`` file it is yielded directly so that + ``python -m code_puppy.i18n.audit path/to/module.py`` works as + expected instead of silently producing an empty report. + """ + if os.path.isfile(root): + if root.endswith(".py"): + yield root + return for dirpath, dirnames, filenames in os.walk(root): dirnames[:] = [d for d in dirnames if d not in _SKIP_DIRS] for name in filenames: diff --git a/tests/i18n/test_i18n_audit.py b/tests/i18n/test_i18n_audit.py index d78b378dd..7539cda3f 100644 --- a/tests/i18n/test_i18n_audit.py +++ b/tests/i18n/test_i18n_audit.py @@ -16,6 +16,26 @@ def test_fstring_is_raw(): assert _kinds('emit_warning(f"hi {name}")') == ["raw"] +def test_fstring_pure_variable_is_dynamic(): + """f"{var}" has no literal content — must not be reported as raw.""" + assert _kinds('emit_info(f"{result}")') == ["dynamic"] + + +def test_fstring_whitespace_only_literal_is_dynamic(): + """f" {var}" has only whitespace in the constant part — not translatable.""" + assert _kinds('emit_info(f" {msg}")') == ["dynamic"] + + +def test_fstring_with_content_and_variable_is_raw(): + """f"Error: {e}" has a meaningful literal prefix — must stay raw.""" + assert _kinds('emit_error(f"Error: {e}")') == ["raw"] + + +def test_fstring_with_only_arrow_is_raw(): + """Non-whitespace punctuation like an arrow counts as a literal.""" + assert _kinds('emit_info(f"-> {item}")') == ["raw"] + + def test_string_concat_is_raw(): assert _kinds('emit_error("bad: " + detail)') == ["raw"] @@ -108,6 +128,33 @@ def test_json_output_is_valid(tmp_path, capsys): assert payload["coverage"] == 50.0 +def test_single_file_path_is_accepted(tmp_path, capsys): + """Passing a .py file directly must produce a non-empty report. + + Previously ``_iter_py_files`` called ``os.walk(file)`` which yields + nothing, so every per-file audit silently returned 0 sites. + """ + import json + + mod = tmp_path / "solo.py" + mod.write_text('emit_info("raw string")\n', encoding="utf-8") + assert audit.main([str(mod), "--json"]) == 0 + payload = json.loads(capsys.readouterr().out) + assert payload["raw"] == 1, "single-file audit must find the raw site" + + +def test_single_file_pure_variable_fstring_not_raw(tmp_path, capsys): + """Single-file audit: f"{var}" must NOT be counted as raw.""" + import json + + mod = tmp_path / "mod.py" + mod.write_text('emit_info(f"{result}")\n', encoding="utf-8") + assert audit.main([str(mod), "--json"]) == 0 + payload = json.loads(capsys.readouterr().out) + assert payload["raw"] == 0 + assert payload["dynamic"] == 1 + + # --- integration smoke ---------------------------------------------------- def test_audits_the_real_package_without_error(): """The tool must stay runnable against the live tree as it evolves. From 27bb1ead096f11832d68436530ad69791867835b Mon Sep 17 00:00:00 2001 From: TJ Webb Date: Mon, 20 Jul 2026 13:03:15 -0700 Subject: [PATCH 2/3] fix(i18n/audit): fix FormattedValue false negative and non-existent path silent zero --- code_puppy/i18n/audit.py | 24 +++++++++++++++++++-- tests/i18n/test_i18n_audit.py | 40 +++++++++++++++++++++++++++++++++++ 2 files changed, 62 insertions(+), 2 deletions(-) diff --git a/code_puppy/i18n/audit.py b/code_puppy/i18n/audit.py index d277bd072..c633d3853 100644 --- a/code_puppy/i18n/audit.py +++ b/code_puppy/i18n/audit.py @@ -149,8 +149,23 @@ def _has_string_literal(node: ast.expr) -> bool: # An f-string is only "raw" when it has at least one constant part # with meaningful (non-whitespace) text, e.g. f"Error: {e}". # Pure-variable forms like f"{var}" or f" {var}" are dynamic. + # + # Also recurse into ``FormattedValue.value`` so a wrapped literal + # like ``f"{'Error: connection refused'}"`` (which parses as + # ``JoinedStr([FormattedValue(Constant('...'))])``) is still + # classified as raw instead of being dropped as dynamic. return any( - isinstance(v, ast.Constant) and isinstance(v.value, str) and v.value.strip() + ( + isinstance(v, ast.Constant) + and isinstance(v.value, str) + and v.value.strip() + ) + or ( + isinstance(v, ast.FormattedValue) + and isinstance(v.value, ast.Constant) + and isinstance(v.value.value, str) + and v.value.value.strip() + ) for v in node.values ) if isinstance(node, ast.BinOp): # "a" + x, "a" % x @@ -228,8 +243,13 @@ def _iter_py_files(root: str) -> Iterable[str]: def audit_tree(root: str) -> Report: - """Audit every Python module under ``root``.""" + """Audit every Python module under ``root``, or ``root`` itself when it is a ``.py`` file.""" report = Report() + if not os.path.isdir(root) and not os.path.isfile(root): + # Silent-zero is worse than useless — it tells CI that a typo'd + # path has "100% coverage". Fail loud instead. + print(f"warning: {root!r} does not exist", file=sys.stderr) + return report for path in sorted(_iter_py_files(root)): try: with open(path, "r", encoding="utf-8") as fh: diff --git a/tests/i18n/test_i18n_audit.py b/tests/i18n/test_i18n_audit.py index 7539cda3f..4db953a98 100644 --- a/tests/i18n/test_i18n_audit.py +++ b/tests/i18n/test_i18n_audit.py @@ -1,5 +1,7 @@ """Tests for the static i18n extraction audit (code_puppy.i18n.audit).""" +import ast + from code_puppy.i18n import audit @@ -31,6 +33,27 @@ def test_fstring_with_content_and_variable_is_raw(): assert _kinds('emit_error(f"Error: {e}")') == ["raw"] +def test_fstring_wrapped_literal_is_raw(): + """f"{'literal'}" wraps a Constant inside a FormattedValue. + + The classifier used to only inspect direct Constant children of + JoinedStr, so this parsed as ``JoinedStr([FormattedValue(Constant)])`` + and was silently classified as dynamic — a false negative. Recurse + into FormattedValue.value so it now shows up as raw. + """ + src = "emit_info(f\"{'Error: connection refused'}\")" + # Direct AST check so we're not relying on the higher-level pipeline. + tree = ast.parse(src, mode="eval") + call = tree.body + assert isinstance(call, ast.Call) + joined = call.args[0] + assert isinstance(joined, ast.JoinedStr) + assert isinstance(joined.values[0], ast.FormattedValue) + # And end-to-end through the classifier. + assert audit._classify(joined) == "raw" + assert _kinds(src) == ["raw"] + + def test_fstring_with_only_arrow_is_raw(): """Non-whitespace punctuation like an arrow counts as a literal.""" assert _kinds('emit_info(f"-> {item}")') == ["raw"] @@ -155,6 +178,23 @@ def test_single_file_pure_variable_fstring_not_raw(tmp_path, capsys): assert payload["dynamic"] == 1 +def test_nonexistent_path_warns_and_returns_empty(tmp_path, capsys): + """A typo'd path must not silently report 100% coverage. + + Previously ``_iter_py_files('/does/not/exist')`` returned ``[]`` and + ``audit_tree`` happily reported ``coverage: 100%, raw: 0`` — the + exact silent-zero bug this PR was created to fix. Now the tool + emits a warning to stderr and returns an empty report cleanly. + """ + missing = tmp_path / "totally-not-real" + assert not missing.exists() + report = audit.audit_tree(str(missing)) + captured = capsys.readouterr() + assert report.sites == [] + assert "does not exist" in captured.err + assert str(missing) in captured.err + + # --- integration smoke ---------------------------------------------------- def test_audits_the_real_package_without_error(): """The tool must stay runnable against the live tree as it evolves. From 44f98c483a36883cf9ddfe1e8d906dcde6c06005 Mon Sep 17 00:00:00 2001 From: TJ Webb Date: Thu, 23 Jul 2026 12:21:56 -0700 Subject: [PATCH 3/3] fix(i18n/audit): fail loud on nonexistent root path --- code_puppy/i18n/audit.py | 11 ++++++----- tests/i18n/test_i18n_audit.py | 27 +++++++++++++++++---------- 2 files changed, 23 insertions(+), 15 deletions(-) diff --git a/code_puppy/i18n/audit.py b/code_puppy/i18n/audit.py index c633d3853..6c8bc06d1 100644 --- a/code_puppy/i18n/audit.py +++ b/code_puppy/i18n/audit.py @@ -244,12 +244,13 @@ def _iter_py_files(root: str) -> Iterable[str]: def audit_tree(root: str) -> Report: """Audit every Python module under ``root``, or ``root`` itself when it is a ``.py`` file.""" - report = Report() if not os.path.isdir(root) and not os.path.isfile(root): - # Silent-zero is worse than useless — it tells CI that a typo'd - # path has "100% coverage". Fail loud instead. - print(f"warning: {root!r} does not exist", file=sys.stderr) - return report + # Silent-zero is worse than useless — an empty Report has + # coverage == 100.0, so a typo'd path would sail past + # ``--fail-under`` and tell CI everything is fine. Fail loud: + # this is a config/programming error, not a data condition. + raise FileNotFoundError(f"audit root does not exist: {root!r}") + report = Report() for path in sorted(_iter_py_files(root)): try: with open(path, "r", encoding="utf-8") as fh: diff --git a/tests/i18n/test_i18n_audit.py b/tests/i18n/test_i18n_audit.py index 4db953a98..d589bf10f 100644 --- a/tests/i18n/test_i18n_audit.py +++ b/tests/i18n/test_i18n_audit.py @@ -178,21 +178,28 @@ def test_single_file_pure_variable_fstring_not_raw(tmp_path, capsys): assert payload["dynamic"] == 1 -def test_nonexistent_path_warns_and_returns_empty(tmp_path, capsys): +def test_nonexistent_path_raises(tmp_path, capsys): """A typo'd path must not silently report 100% coverage. - Previously ``_iter_py_files('/does/not/exist')`` returned ``[]`` and - ``audit_tree`` happily reported ``coverage: 100%, raw: 0`` — the - exact silent-zero bug this PR was created to fix. Now the tool - emits a warning to stderr and returns an empty report cleanly. + Previously ``audit_tree('/does/not/exist')`` returned an empty + ``Report`` whose ``coverage`` property is ``100.0``, so + ``--fail-under`` would happily pass on a typo'd path and tell CI + everything was fine. Fail loud instead: this is a + programming/config error, so ``FileNotFoundError`` propagates all + the way out of ``main()`` and produces a nonzero exit. """ + import pytest + missing = tmp_path / "totally-not-real" assert not missing.exists() - report = audit.audit_tree(str(missing)) - captured = capsys.readouterr() - assert report.sites == [] - assert "does not exist" in captured.err - assert str(missing) in captured.err + + # audit_tree raises directly. + with pytest.raises(FileNotFoundError): + audit.audit_tree(str(missing)) + + # And main() lets it propagate — no accidental swallow, no exit 0. + with pytest.raises(FileNotFoundError): + audit.main([str(missing)]) # --- integration smoke ----------------------------------------------------