diff --git a/cr_checker/README.md b/cr_checker/README.md index 6292d844..0bb52cd5 100644 --- a/cr_checker/README.md +++ b/cr_checker/README.md @@ -55,7 +55,7 @@ python cr_checker.py -t [options] - **--offset**: Force this many characters (plus any trailing blank lines) at the start of the file to be treated as a recognized preamble, overriding auto-detection. Character-based, not byte-based. Rarely needed: a leading shebang is detected and preserved automatically; use this only for other preamble kinds the tool doesn't (yet) recognize. - **-f**, **--fix**: Setting script into fix mode where copyright header will be added to the files if it's missing from same. - **--remove-offset**: Number of characters to remove before appending proper copyright header (works only with `--fix` option). -- **--force**: With `--fix`, also rewrite headers whose similarity to the template is below the auto-fix threshold (normally left untouched and only reported, since they may be a genuinely different license text). Never affects a duplicate-header file, which always requires manual review. Ignored without `--fix`. +- **--force**: With `--fix`, also rewrite headers whose similarity to the template is below the auto-fix threshold (normally left untouched and only reported, since they may be a genuinely different license text). Never affects a duplicate-header file or one with a genuine SPDX license mismatch, both of which always require manual review. Ignored without `--fix`. - **--modified-only**: Only check files that differ from `HEAD` (staged and/or unstaged), e.g. for a fast, incremental pre-commit run. Takes precedence over `inputs`. - **inputs**: Directories or files to parse, or a parameter file prefixed with @ that lists files or directories. Optional -- when omitted, the whole repository (per `git ls-files`) is checked. @@ -132,8 +132,16 @@ bazel run //:copyright.fix -- --force ``` `--force` never touches a file with a *duplicate* copyright header -- -that always requires manual review, regardless of similarity. **Always -review the resulting diff afterwards**, since a low similarity score can +that always requires manual review, regardless of similarity. It also never +touches a header with a genuine *SPDX license mismatch* (e.g. an existing +`MIT` header where the template expects `Apache-2.0`) -- that's detected +separately from the overall similarity score (a short header can score +high similarity overall while still naming a different license), and is +always left for manual review since silently overwriting someone else's +license declaration is a legal/compliance decision, not a formatting fix. +The comparison is lenient about spacing, dots and hyphens (`Apache-2.0` == +`Apache 2.0` == `apache2.0`), so only an actual license difference trips it. +**Always review the resulting diff afterwards**, since a low similarity score can also mean the existing text is a genuinely different, unrelated license. `--offset=` is only needed to force-treat a preamble kind the tool diff --git a/cr_checker/tests/test_cr_checker.py b/cr_checker/tests/test_cr_checker.py index cb47d2fc..55fe157b 100644 --- a/cr_checker/tests/test_cr_checker.py +++ b/cr_checker/tests/test_cr_checker.py @@ -493,7 +493,8 @@ def test_classify_scores_formatting_drift_highly(tmp_path): def test_classify_scores_unrelated_license_low(tmp_path): """A header for a genuinely different license must score well below the - auto-fix threshold, so `--fix` never silently overwrites it.""" + auto-fix threshold, AND be caught by the SPDX mismatch guard -- either + would prevent `--fix` from silently overwriting it.""" cr_checker = load_cr_checker_module() header_template = load_template("rs") config_file = write_config(tmp_path, "Author") @@ -507,10 +508,24 @@ def test_classify_scores_unrelated_license_low(tmp_path): layout = cr_checker.locate_header(unrelated_header) status, similarity = cr_checker.classify(layout, header_template, config_file) - assert status is cr_checker.Status.WRONG_FORMAT + assert status is cr_checker.Status.LICENSE_MISMATCH assert similarity < cr_checker.HEADER_SIMILARITY_THRESHOLD +def test_spdx_mismatch_ignores_spacing_dots_and_hyphens(tmp_path): + """A harmless formatting variant of the SAME identifier (spacing/dots/ + hyphens differ but the license doesn't) must NOT trip the mismatch guard.""" + cr_checker = load_cr_checker_module() + header_template = load_template("py") + config_file = write_config(tmp_path, "Author") + variant_header = "# Copyright (c) 2024 Author\n#\n# SPDX-License-Identifier: apache 2.0\n" + + layout = cr_checker.locate_header(variant_header) + status, _ = cr_checker.classify(layout, header_template, config_file) + + assert status is not cr_checker.Status.LICENSE_MISMATCH + + # --- duplicate_similarity (diagnostics for DUPLICATE status) --- @@ -799,16 +814,17 @@ def test_process_files_check_mode_ignores_remove_offset(tmp_path): def test_process_files_fix_without_force_leaves_low_similarity_header_untouched(tmp_path): """Baseline: a WRONG_FORMAT header that scores below - `HEADER_SIMILARITY_THRESHOLD` (a genuinely different license, not just a - formatting drift) must be left alone by `--fix` without `--force`.""" + `HEADER_SIMILARITY_THRESHOLD` (unrecognizable boilerplate, though it + carries the same SPDX identifier so it isn't a LICENSE_MISMATCH) must be + left alone by `--fix` without `--force`.""" cr_checker = load_cr_checker_module() header_template = load_template("py") config = write_config(tmp_path, "Author") unrelated_header = ( - "# Copyright (c) 2020 Some Other Corp. All rights reserved.\n" - "# Licensed under the MIT License; see the LICENSE file for details.\n" + "# Copyright presence only, then completely different padding text follows\n" + "# padding padding padding padding padding padding padding\n" "#\n" - "# SPDX-License-Identifier: MIT\n" + "# SPDX-License-Identifier: Apache-2.0\n" ) test_file = tmp_path / "file.py" test_file.write_text(unrelated_header + "print('hi')\n", encoding="utf-8") @@ -828,16 +844,18 @@ def test_process_files_fix_without_force_leaves_low_similarity_header_untouched( def test_process_files_fix_force_rewrites_low_similarity_header(tmp_path): - """With `force=True`, the same low-similarity header IS rewritten -- - `--force` is an explicit, opt-in override of the similarity guard.""" + """With `force=True`, the same low-similarity (but same-license) header IS + rewritten -- `--force` is an explicit, opt-in override of the similarity + guard, though never of the separate SPDX mismatch guard (see + `test_process_files_fix_force_does_not_touch_license_mismatch`).""" cr_checker = load_cr_checker_module() header_template = load_template("py") config = write_config(tmp_path, "Author") unrelated_header = ( - "# Copyright (c) 2020 Some Other Corp. All rights reserved.\n" - "# Licensed under the MIT License; see the LICENSE file for details.\n" + "# Copyright presence only, then completely different padding text follows\n" + "# padding padding padding padding padding padding padding\n" "#\n" - "# SPDX-License-Identifier: MIT\n" + "# SPDX-License-Identifier: Apache-2.0\n" ) test_file = tmp_path / "file.py" test_file.write_text(unrelated_header + "print('hi')\n", encoding="utf-8") @@ -855,10 +873,44 @@ def test_process_files_fix_force_rewrites_low_similarity_header(tmp_path): expected_header = header_template.format(year=datetime.now().year, author="Author") fixed = test_file.read_text(encoding="utf-8") assert results["fixed"] == 1 - assert "Some Other Corp" not in fixed + assert "padding" not in fixed assert fixed == expected_header + "\n" + "print('hi')\n" +def test_process_files_fix_force_does_not_touch_license_mismatch(tmp_path): + """`force` only bypasses the *similarity* guard for WRONG_FORMAT / + MISPLACED_AND_WRONG_FORMAT; LICENSE_MISMATCH is not in `FIXABLE_STATUSES` + at all and must still be left for manual review even with `force=True`, + since silently overwriting a genuinely different license's SPDX + identifier is a legal/compliance-significant action, not cosmetic + drift.""" + cr_checker = load_cr_checker_module() + header_template = load_template("py") + config = write_config(tmp_path, "Author") + unrelated_header = ( + "# Copyright (c) 2020 Some Other Corp. All rights reserved.\n" + "# Licensed under the MIT License; see the LICENSE file for details.\n" + "#\n" + "# SPDX-License-Identifier: MIT\n" + ) + test_file = tmp_path / "file.py" + test_file.write_text(unrelated_header + "print('hi')\n", encoding="utf-8") + + results = cr_checker.process_files( + [test_file], + {"py": header_template}, + True, + config=config, + use_mmap=False, + encoding="utf-8", + force=True, + ) + + assert results["fixed"] == 0 + assert results["license_mismatch"] == 1 + assert test_file.read_text(encoding="utf-8") == unrelated_header + "print('hi')\n" + + def test_process_files_fix_force_does_not_touch_duplicate(tmp_path): """`force` only bypasses the *similarity* guard for WRONG_FORMAT / MISPLACED_AND_WRONG_FORMAT; DUPLICATE is not in `FIXABLE_STATUSES` at @@ -913,7 +965,7 @@ def test_process_files_check_mode_ignores_force(tmp_path): ) assert results["fixed"] == 0 - assert results["wrong_format"] == 1 + assert results["license_mismatch"] == 1 assert test_file.read_text(encoding="utf-8") == unrelated_header + "print('hi')\n" diff --git a/cr_checker/tests/test_cr_checker_integration.py b/cr_checker/tests/test_cr_checker_integration.py index fdcba49f..3b3d22fc 100644 --- a/cr_checker/tests/test_cr_checker_integration.py +++ b/cr_checker/tests/test_cr_checker_integration.py @@ -369,7 +369,14 @@ def test_real_tool_source_file_is_compliant(): results = cr_checker.process_files([TOOL_MODULE_PATH], templates, fix=False) - assert results == {"missing": 0, "misplaced": 0, "wrong_format": 0, "duplicate": 0, "fixed": 0} + assert results == { + "missing": 0, + "misplaced": 0, + "wrong_format": 0, + "duplicate": 0, + "license_mismatch": 0, + "fixed": 0, + } def test_real_build_file_is_compliant(): @@ -380,7 +387,14 @@ def test_real_build_file_is_compliant(): results = cr_checker.process_files([build_file], templates, fix=False) - assert results == {"missing": 0, "misplaced": 0, "wrong_format": 0, "duplicate": 0, "fixed": 0} + assert results == { + "missing": 0, + "misplaced": 0, + "wrong_format": 0, + "duplicate": 0, + "license_mismatch": 0, + "fixed": 0, + } def test_real_exclusion_file_skips_real_templates_ini(): @@ -399,7 +413,14 @@ def test_real_exclusion_file_skips_real_templates_ini(): exclusion=exclusion, ) - assert results == {"missing": 0, "misplaced": 0, "wrong_format": 0, "duplicate": 0, "fixed": 0} + assert results == { + "missing": 0, + "misplaced": 0, + "wrong_format": 0, + "duplicate": 0, + "license_mismatch": 0, + "fixed": 0, + } def test_real_config_author_is_used_when_fixing(tmp_path): diff --git a/cr_checker/tool/cr_checker.py b/cr_checker/tool/cr_checker.py index 78feb0f8..5a24c5c6 100755 --- a/cr_checker/tool/cr_checker.py +++ b/cr_checker/tool/cr_checker.py @@ -61,6 +61,11 @@ rf"Copyright.{{0,{COPYRIGHT_BLOCK_MAX_GAP}}}?SPDX-License-Identifier\s*:[^\n]*\n?", re.IGNORECASE | re.DOTALL, ) +SPDX_IDENTIFIER_PATTERN = re.compile(r"SPDX-License-Identifier\s*:\s*([^\n]*)", re.IGNORECASE) +# Chars ignored when comparing two SPDX identifiers, so harmless formatting +# variants ("Apache-2.0" vs "Apache 2.0" vs "apache2.0") aren't mistaken for a +# genuine license difference ("MIT" vs "Apache-2.0"). +SPDX_IGNORED_CHARS_PATTERN = re.compile(r"[\s.\-]+") LOGGER = logging.getLogger() @@ -591,13 +596,15 @@ class Status(enum.Enum): WRONG_FORMAT = "wrong_format" MISPLACED_AND_WRONG_FORMAT = "misplaced_and_wrong_format" DUPLICATE = "duplicate" + LICENSE_MISMATCH = "license_mismatch" # Statuses `--fix` is allowed to act on automatically. WRONG_FORMAT and # MISPLACED_AND_WRONG_FORMAT are additionally gated on the similarity # threshold at the call site (see `_process_file_fix`) -- a low score means # the text is probably an unrelated license, which must never be silently -# rewritten. +# rewritten. LICENSE_MISMATCH, like DUPLICATE, is never in this set -- +# unlike the similarity gate, `--force` cannot bypass it either. FIXABLE_STATUSES = { Status.MISSING, Status.MISPLACED, @@ -701,6 +708,34 @@ def _strip_old_wrapper_suffix(remainder, template): return remainder[len(first_line) :].lstrip("\n") +def _extract_spdx_id(text): + """Returns the raw ``SPDX-License-Identifier`` value in `text` (e.g. + ``"Apache-2.0"``), or None if it doesn't contain one.""" + match = SPDX_IDENTIFIER_PATTERN.search(text) + return match.group(1).strip() if match else None + + +def _normalize_spdx_id(spdx_id): + """Folds an SPDX identifier down for lenient comparison (see + `SPDX_IGNORED_CHARS_PATTERN`): case-insensitive, ignoring spaces, dots + and hyphens.""" + return SPDX_IGNORED_CHARS_PATTERN.sub("", spdx_id).casefold() + + +def _spdx_mismatch(existing_text, template): + """True if `existing_text` and `template` each carry an SPDX identifier + and they genuinely differ (e.g. ``MIT`` vs ``Apache-2.0``) -- as opposed + to just a formatting variant of the same one (see `_normalize_spdx_id`). + False if either side has no SPDX identifier at all, since that's not + this check's concern (missing/wrong-format handling already covers it). + """ + existing = _extract_spdx_id(existing_text) + expected = _extract_spdx_id(template) + if not existing or not expected: + return False + return _normalize_spdx_id(existing) != _normalize_spdx_id(expected) + + def classify(layout, template, config): """Classifies a file's header state from its `HeaderLayout`. @@ -726,6 +761,14 @@ def classify(layout, template, config): the file (e.g. an example header, or literally this module's own ``COPYRIGHT_BLOCK_PATTERN`` regex source) isn't mistaken for a duplicate of a real header near the top. + + A wrong-format header whose own SPDX identifier genuinely differs + from the template's (see `_spdx_mismatch`) is classified + LICENSE_MISMATCH instead of WRONG_FORMAT/MISPLACED_AND_WRONG_FORMAT, + regardless of how similar the rest of the boilerplate text looks -- + a whole-block similarity score alone can't be trusted to catch this, + since a short header is mostly shared boilerplate around the one + line that actually matters. """ if not layout.blocks: return Status.MISSING, None @@ -754,6 +797,8 @@ def classify(layout, template, config): correct_format = bool(template_regex.match(block.text)) misplaced = bool(layout.leading_junk) and not _is_old_wrapper_prefix(layout.leading_junk, template) + if not correct_format and _spdx_mismatch(block.text, template): + return Status.LICENSE_MISMATCH, similarity if correct_format and not misplaced: return Status.COMPLIANT, similarity if correct_format and misplaced: @@ -1038,18 +1083,28 @@ def _tally(status, results): results["missing"] += 1 elif status is Status.DUPLICATE: results["duplicate"] += 1 + elif status is Status.LICENSE_MISMATCH: + results["license_mismatch"] += 1 elif status is Status.MISPLACED: results["misplaced"] += 1 elif status in (Status.WRONG_FORMAT, Status.MISPLACED_AND_WRONG_FORMAT): results["wrong_format"] += 1 -def _log_status(item, status, layout, similarity): +def _log_status(item, status, layout, similarity, template): """Logs a diagnostic message for a classified `status`.""" if status is Status.COMPLIANT: LOGGER.debug("File %s has copyright.", item) elif status is Status.MISSING: LOGGER.error("Missing copyright header in: %s, use --fix to introduce it", item) + elif status is Status.LICENSE_MISMATCH: + LOGGER.error( + "License mismatch in: %s (found %r, expected %r) -- this looks like a genuinely " + "different license and is never auto-fixed, not even with --force; resolve manually", + item, + _extract_spdx_id(layout.blocks[0].text), + _extract_spdx_id(template), + ) elif status is Status.MISPLACED: LOGGER.error( "Copyright header in %s is correctly formatted but preceded by other content " @@ -1099,7 +1154,7 @@ def _process_file_check(item, template, encoding, offset, use_mmap, config, resu layout = locate_header(text, manual_prefix_offset=offset) status, similarity = classify(layout, template, config) - _log_status(item, status, layout, similarity) + _log_status(item, status, layout, similarity, template) _tally(status, results) @@ -1120,7 +1175,7 @@ def _process_file_fix(item, template, encoding, offset, remove_offset, config, r layout = locate_header(text, manual_prefix_offset=offset) status, similarity = classify(layout, template, config) - _log_status(item, status, layout, similarity) + _log_status(item, status, layout, similarity, template) _tally(status, results) if status not in FIXABLE_STATUSES: @@ -1183,11 +1238,12 @@ def process_files( WRONG_FORMAT/MISPLACED_AND_WRONG_FORMAT, rewriting the header regardless of how different it looks from the template. Only used in ``--fix`` mode. Never applies to - DUPLICATE, which is always left for manual review. + DUPLICATE or LICENSE_MISMATCH, which are always left for + manual review. Returns: dict: Counters for ``missing``, ``misplaced``, ``wrong_format``, - ``duplicate`` and ``fixed``. + ``duplicate``, ``license_mismatch`` and ``fixed``. Note: A wrong-format or misplaced-and-wrong-format header is only @@ -1199,10 +1255,20 @@ def process_files( untouched and only reported, since it may be a genuinely different license text that must never be silently overwritten. Duplicate headers are never auto-fixed, regardless of similarity or ``force``. + Nor is a header whose own SPDX identifier genuinely differs from the + template's (``LICENSE_MISMATCH``) -- similarity alone can't be + trusted there, and ``force`` does not override it either. """ if exclusion is None: exclusion = [] - results = {"missing": 0, "misplaced": 0, "wrong_format": 0, "duplicate": 0, "fixed": 0} + results = { + "missing": 0, + "misplaced": 0, + "wrong_format": 0, + "duplicate": 0, + "license_mismatch": 0, + "fixed": 0, + } for item in files: key = extension_key(item) @@ -1330,7 +1396,8 @@ def parse_arguments(argv): help="With --fix, also rewrite WRONG_FORMAT/MISPLACED_AND_WRONG_FORMAT headers whose " "similarity to the template is below HEADER_SIMILARITY_THRESHOLD (normally left " "untouched since they may be a genuinely different license text). Never affects " - "DUPLICATE, which always requires manual review. Ignored without --fix.", + "DUPLICATE or LICENSE_MISMATCH (a header whose own SPDX identifier genuinely differs " + "from the template's), which always require manual review. Ignored without --fix.", ) parser.add_argument( @@ -1453,8 +1520,11 @@ def main(argv=None): total_misplaced = results["misplaced"] total_wrong_format = results["wrong_format"] total_duplicates = results["duplicate"] + total_license_mismatches = results["license_mismatch"] total_fixes = results["fixed"] - total_violations = total_missing + total_misplaced + total_wrong_format + total_duplicates + total_violations = ( + total_missing + total_misplaced + total_wrong_format + total_duplicates + total_license_mismatches + ) LOGGER.info("=" * 64) LOGGER.info("Process completed.") @@ -1482,6 +1552,12 @@ def main(argv=None): total_duplicates, COLORS["ENDC"], ) + LOGGER.info( + "Total files with a license mismatch: %s%d%s", + COLORS["RED"] if total_license_mismatches > 0 else COLORS["GREEN"], + total_license_mismatches, + COLORS["ENDC"], + ) if not exclusion_valid: LOGGER.info("The exclusion file contains paths that do not exist.") if args.fix: