From a8fcb40928dea93d5940f159901a321d1ec40469 Mon Sep 17 00:00:00 2001 From: Gabriel Dugny Date: Fri, 18 Sep 2026 10:41:10 +0200 Subject: [PATCH] fix: locate pre-commit config edits with exact source marks (#64) `update_pre_commit_repo_versions` found the line to rewrite from strictyaml's `end_line` plus a hand-computed document offset. `end_line` counts logical nodes rather than physical lines, so it drifts by `lines - 1` past any multi-line flow sequence: the rewrite landed on the wrong line, the `.replace()` matched nothing, and the write aborted with "No changes to write, this should not happen". Take positions from a ruamel round-trip parse instead, whose `lc` marks come from the lexer and are absolute. That also covers a second case the offset never handled, a file starting with blank lines and no `---`, which the `pre-commit-config-start-empty-lines.yaml` fixture has reproduced unnoticed since it was added: `document_start_offset` returned 0 while `end_line` was still short by one. The offset is now unnecessary and is removed. Edits are collected and applied rightmost-first, because replacing one scalar shifts the columns of every scalar after it on that line, and each is checked to have landed so a positional miss fails loudly instead of writing a half-updated file. The round-trip parser comes from strictyaml's vendored copy, falling back to a standalone ruamel.yaml, so no new dependency and no reliance on strictyaml's private attributes. Co-Authored-By: Claude Opus 5 (1M context) --- src/sync_pre_commit_lock/pre_commit_config.py | 88 +++++++++------ ...t-config-flow-multiline-deps.expected.yaml | 30 +++++ ...pre-commit-config-flow-multiline-deps.yaml | 30 +++++ tests/test_pre_commit_config_file.py | 105 +++++++++++++++--- 4 files changed, 201 insertions(+), 52 deletions(-) create mode 100644 tests/fixtures/sample_pre_commit_config/pre-commit-config-flow-multiline-deps.expected.yaml create mode 100644 tests/fixtures/sample_pre_commit_config/pre-commit-config-flow-multiline-deps.yaml diff --git a/src/sync_pre_commit_lock/pre_commit_config.py b/src/sync_pre_commit_lock/pre_commit_config.py index f26c816..702e8a8 100644 --- a/src/sync_pre_commit_lock/pre_commit_config.py +++ b/src/sync_pre_commit_lock/pre_commit_config.py @@ -1,8 +1,8 @@ from __future__ import annotations -import difflib from dataclasses import dataclass, field from functools import cached_property +from importlib.metadata import version from typing import TYPE_CHECKING, Any import strictyaml as yaml @@ -43,6 +43,27 @@ ) +def _round_trip_load(raw: str) -> Any: + """Parse ``raw`` with a ruamel round-trip loader, whose nodes carry ``lc`` source marks. + + strictyaml vendors ruamel, so this normally costs no extra dependency. A strictyaml + that stops vendoring it still works if ruamel.yaml is installed on its own. + """ + try: + from strictyaml.ruamel import YAML # type: ignore[import-untyped] + except ImportError: + try: + from ruamel.yaml import YAML # type: ignore[import-not-found] + except ImportError as exc: + msg = ( + "Updating .pre-commit-config.yaml needs a ruamel round-trip parser, which" + f" strictyaml {version('strictyaml')} does not vendor. Install ruamel.yaml," + " or pin strictyaml<2." + ) + raise RuntimeError(msg) from exc + return YAML().load(raw) + + @dataclass(frozen=True) class PreCommitHook: id: str @@ -136,29 +157,23 @@ def repos_normalized(self) -> set[PreCommitRepo]: } @cached_property - def document_start_offset(self) -> int: - """Return the line number where the YAML document starts.""" - lines = self.raw_file_contents.split("\n") - for i, line in enumerate(lines): - # Trim leading/trailing whitespaces - line = line.rstrip() - # Skip if line is a comment or empty/whitespace - if line.startswith("#") or line == "": - continue - # If line is '---', return line number + 1 - if line == "---": - return i + 1 - return 0 + def repo_marks(self) -> Any: + """Exact source positions for each repo entry, from a ruamel round-trip parse. + + strictyaml's own ``end_line`` counts logical nodes, so it drifts past any + multi-line flow sequence (see #64). ruamel's ``lc`` marks come from the lexer + and stay exact, document separator and comments included. + """ + return _round_trip_load(self.raw_file_contents)["repos"] def update_pre_commit_repo_versions(self, new_versions: dict[PreCommitRepo, PreCommitRepo]) -> None: """Fix the pre-commit hooks to match the lockfile. Preserve comments and formatting as much as possible.""" if len(new_versions) == 0: return - original_lines = self.original_file_lines - updated_lines = original_lines[:] + edits: list[tuple[int, int, str, str]] = [] - for repo_rev in self.yaml["repos"]: + for repo_rev, marks in zip(self.yaml["repos"], self.repo_marks): if "rev" not in repo_rev: continue @@ -174,31 +189,36 @@ def update_pre_commit_repo_versions(self, new_versions: dict[PreCommitRepo, PreC if not (updated_repo := new_versions.get(normalized_repo)): continue - rev_line_number: int = rev.end_line + self.document_start_offset - rev_line_idx: int = rev_line_number - 1 - original_rev_line: str = updated_lines[rev_line_idx] - updated_lines[rev_line_idx] = original_rev_line.replace(str(rev), updated_repo.rev) + if str(rev) != updated_repo.rev: + _, _, rev_line, rev_col = marks.lc.data["rev"] + edits.append((rev_line, rev_col, str(rev), updated_repo.rev)) - for src_hook, old_hook, new_hook in zip(hooks, normalized_repo.hooks, updated_repo.hooks): + for hook_marks, old_hook, new_hook in zip( + marks.get("hooks", ()), normalized_repo.hooks, updated_repo.hooks + ): if new_hook == old_hook: continue - for src_dep, old_dep, new_dep in zip( - src_hook.get("additional_dependencies", []), - old_hook.additional_dependencies, - new_hook.additional_dependencies, + for i, (old_dep, new_dep) in enumerate( + zip(old_hook.additional_dependencies, new_hook.additional_dependencies) ): if old_dep == new_dep: continue - dep_line_number: int = src_dep.end_line + self.document_start_offset - dep_line_idx: int = dep_line_number - 1 - original_dep_line: str = updated_lines[dep_line_idx] - updated_lines[dep_line_idx] = original_dep_line.replace(str(src_dep), new_dep) - - changes = difflib.ndiff(original_lines, updated_lines) - change_count = sum(1 for change in changes if change[0] in ["+", "-"]) + dep_line, dep_col = hook_marks["additional_dependencies"].lc.data[i] + edits.append((dep_line, dep_col, old_dep, new_dep)) - if change_count == 0: + if not edits: msg = "No changes to write, this should not happen" raise RuntimeError(msg) + + updated_lines = self.original_file_lines[:] + # Rightmost edit first: replacing an earlier scalar on the same line would shift + # every column after it, and those columns were measured against the original text. + for line_idx, col, old, new in sorted(edits, reverse=True): + line = updated_lines[line_idx] + if old not in line[col:]: + msg = f"Expected {old!r} at line {line_idx + 1}, column {col + 1}, found {line[col:].rstrip()!r}" + raise RuntimeError(msg) + updated_lines[line_idx] = line[:col] + line[col:].replace(old, new, 1) + with self.pre_commit_config_file_path.open("w") as stream: stream.writelines(updated_lines) diff --git a/tests/fixtures/sample_pre_commit_config/pre-commit-config-flow-multiline-deps.expected.yaml b/tests/fixtures/sample_pre_commit_config/pre-commit-config-flow-multiline-deps.expected.yaml new file mode 100644 index 0000000..7a4f709 --- /dev/null +++ b/tests/fixtures/sample_pre_commit_config/pre-commit-config-flow-multiline-deps.expected.yaml @@ -0,0 +1,30 @@ + +# Many unused lines before document separator + +--- +default_language_version: + python: python3.11 + +repos: + + # This multi-line flow sequence is what strictyaml miscounts, shifting every + # line reported below it (see #64). + - repo: https://github.com/python-jsonschema/check-jsonschema + rev: "0.36.1" + hooks: + - id: check-jsonschema + args: [ + "--no-cache", + "--schemafile", + "https://example.invalid/schema.json", + ] + + - repo: https://github.com/pre-commit/mirrors-mypy + # Some comment + rev: v1.5.0 + hooks: + - id: mypy + additional_dependencies: [ + types-PyYAML==1.2.4, + types-requests==3.4.5, + ] diff --git a/tests/fixtures/sample_pre_commit_config/pre-commit-config-flow-multiline-deps.yaml b/tests/fixtures/sample_pre_commit_config/pre-commit-config-flow-multiline-deps.yaml new file mode 100644 index 0000000..4ce2685 --- /dev/null +++ b/tests/fixtures/sample_pre_commit_config/pre-commit-config-flow-multiline-deps.yaml @@ -0,0 +1,30 @@ + +# Many unused lines before document separator + +--- +default_language_version: + python: python3.11 + +repos: + + # This multi-line flow sequence is what strictyaml miscounts, shifting every + # line reported below it (see #64). + - repo: https://github.com/python-jsonschema/check-jsonschema + rev: "0.36.1" + hooks: + - id: check-jsonschema + args: [ + "--no-cache", + "--schemafile", + "https://example.invalid/schema.json", + ] + + - repo: https://github.com/pre-commit/mirrors-mypy + # Some comment + rev: v1.0.0 + hooks: + - id: mypy + additional_dependencies: [ + types-PyYAML==1.2.4, + types-requests, + ] diff --git a/tests/test_pre_commit_config_file.py b/tests/test_pre_commit_config_file.py index 50fd877..2c6527f 100644 --- a/tests/test_pre_commit_config_file.py +++ b/tests/test_pre_commit_config_file.py @@ -1,3 +1,4 @@ +import builtins from pathlib import Path from unittest.mock import MagicMock, mock_open @@ -5,7 +6,12 @@ import yaml from strictyaml.exceptions import YAMLValidationError -from sync_pre_commit_lock.pre_commit_config import PreCommitHook, PreCommitHookConfig, PreCommitRepo +from sync_pre_commit_lock.pre_commit_config import ( + PreCommitHook, + PreCommitHookConfig, + PreCommitRepo, + _round_trip_load, +) def test_pre_commit_hook_config_initialization() -> None: @@ -53,21 +59,6 @@ def test_repos_property() -> None: FIXTURES = Path(__file__).parent / "fixtures" / "sample_pre_commit_config" -@pytest.mark.parametrize( - ("path", "offset"), - [ - (FIXTURES / "pre-commit-config-document-separator.yaml", 4), - (FIXTURES / "pre-commit-config-start-empty-lines.yaml", 0), - (FIXTURES / "pre-commit-config-with-local.yaml", 2), - (FIXTURES / "pre-commit-config.yaml", 1), - (FIXTURES / "sample-django-stubs.yaml", 0), - ], -) -def test_files_offset(path: Path, offset: int) -> None: - config = PreCommitHookConfig.from_yaml_file(path) - assert config.document_start_offset == offset - - def test_update_versions() -> None: config = PreCommitHookConfig.from_yaml_file(FIXTURES / "pre-commit-config-document-separator.yaml") config.pre_commit_config_file_path = MagicMock() @@ -87,13 +78,91 @@ def test_update_versions() -> None: assert config.pre_commit_config_file_path.open.call_count == 1 -@pytest.mark.parametrize("base", ["only-deps", "with-deps", "with-one-liner-deps", "without-new-deps"]) +@pytest.mark.parametrize( + "name", + [ + "pre-commit-config.yaml", + "pre-commit-config-document-separator.yaml", + "pre-commit-config-start-empty-lines.yaml", + "pre-commit-config-with-local.yaml", + "sample-django-stubs.yaml", + ], +) +def test_update_versions_rewrites_only_the_rev(name: str) -> None: + """Whatever precedes the rev -- blank lines, comments, a `---` separator, a local repo + with no rev -- the bump must land on that rev and change nothing else (#64). + """ + path = FIXTURES / name + config = PreCommitHookConfig.from_yaml_file(path) + mock_file = config.pre_commit_config_file_path = MagicMock() + mock_file.open = mock_open() + + initial_repo = config.repos[0] + config.update_pre_commit_repo_versions( + {initial_repo: PreCommitRepo(initial_repo.repo, "99.99.99", initial_repo.hooks)} + ) + + written = "".join(mock_file.open().writelines.call_args[0][0]) + assert "99.99.99" in written + assert written.replace("99.99.99", initial_repo.rev) == path.read_text() + + +def test_round_trip_load_explains_itself_without_a_ruamel_parser(monkeypatch: pytest.MonkeyPatch) -> None: + """strictyaml vendors ruamel today; if a future one stops, say what to install.""" + real_import = builtins.__import__ + + def no_ruamel(name: str, *args: object, **kwargs: object) -> object: + if name in {"strictyaml.ruamel", "ruamel.yaml"}: + raise ImportError(name) + return real_import(name, *args, **kwargs) # type: ignore[arg-type] + + monkeypatch.setattr(builtins, "__import__", no_ruamel) + + with pytest.raises(RuntimeError, match="Install ruamel.yaml"): + _round_trip_load("repos: []") + + +def test_update_deps_sharing_a_line_when_the_first_one_shrinks() -> None: + """A shorter replacement shifts every column after it on that line, so edits must be + applied rightmost-first or the later dependency is silently left untouched. + """ + file_content = """\ +repos: + - repo: https://github.com/pre-commit/mirrors-mypy + rev: v1.0.0 + hooks: + - id: mypy + additional_dependencies: [types-PyYAML==6.0.12.20240311, types-requests==2.31.0] +""" + mock_path = MagicMock(spec=Path) + mock_path.open = mock_open(read_data=file_content) + config = PreCommitHookConfig.from_yaml_file(mock_path) + + initial_repo = config.repos[0] + mock_path.open = mock_open() + config.update_pre_commit_repo_versions( + { + initial_repo: PreCommitRepo( + initial_repo.repo, + initial_repo.rev, + [PreCommitHook("mypy", ["types-PyYAML==6.0.12", "types-requests==2.32.0"])], + ) + } + ) + + written = "".join(mock_path.open().writelines.call_args[0][0]) + assert "additional_dependencies: [types-PyYAML==6.0.12, types-requests==2.32.0]" in written + + +@pytest.mark.parametrize( + "base", ["only-deps", "with-deps", "with-one-liner-deps", "without-new-deps", "flow-multiline-deps"] +) def test_update_additional_dependencies_versions(base: str) -> None: config = PreCommitHookConfig.from_yaml_file(FIXTURES / f"pre-commit-config-{base}.yaml") mock_file = config.pre_commit_config_file_path = MagicMock() mock_file.open = mock_open() - initial_repo = config.repos[0] + initial_repo = next(repo for repo in config.repos if repo.repo.endswith("/mirrors-mypy")) updated_repo = PreCommitRepo( "https://github.com/pre-commit/mirrors-mypy", "v1.5.0",