fix(ol_dbt_cli): use merge-base (three-dot) diff semantics for changed-model detection (#2445)

* fix(ol_dbt_cli): use merge-base (three-dot) diff semantics for changed-model detection

git_utils.get_changed_files diffed base_ref..HEAD directly (two-dot). Once
base_ref advances past the PR's branch point (other PRs merging to main
while this one is open), models changed BY OTHERS on main leaked into the
PR's changed set, and get_file_at_ref fetched main's newer content as the
"base" -- producing spurious BREAKING alerts for changes the PR never made,
or masking real ones when main's later edit happened to match.

Add resolve_merge_base() and diff/fetch against merge-base(base_ref, HEAD)
instead, matching the three-dot semantics GitHub uses to render PR diffs.
ol-dbt impact now resolves the merge-base once per run and threads it
through to both changed-model detection and base-content fetching so the
two stay consistent. validate --changed-only picks up the fix for free
since it goes through the same get_changed_sql_models -> get_changed_files
path.

Also union in untracked new files (git status --porcelain) by default so
local runs see in-progress new models that haven't been git-added yet; CI
checkouts have none, so this doesn't affect the PR pipeline.

git_utils.py had zero tests. Added a scripted fixture repo (branch, advance
main independently, assert changed set + fetched content) covering
resolve_merge_base, get_changed_files, get_changed_sql_models, and
get_file_at_ref.

* fix(ol_dbt_cli): use -z for git status/diff parsing, fix docstring mismatch

Two issues from PR review:

- Sentry: `git status --porcelain` (no -z) C-escapes/quotes paths with
  whitespace or non-ASCII bytes (e.g. `?? "my model.sql"`,
  `?? "caf\303\251.sql"`). The naive `line[3:].strip()` parse left the
  quotes/escapes in, producing a path that doesn't exist on disk and
  silently dropping the file from the changed set. Same class of bug
  applied to the `git diff --name-only` calls for tracked files. Switch
  all four git invocations in get_changed_files to `-z` (NUL-delimited,
  unquoted) output.

- Copilot: get_changed_files' docstring said it returns "tracked files"
  but untracked files are included by default (include_untracked=True).
  Fixed the wording.

Added a regression test with a space- and unicode-containing filename;
confirmed it fails against the pre-fix parsing (returned mangled paths
like 'my model.sql"') and passes with -z.

Author: blarghmatey

src/ol_dbt_cli/ol_dbt_cli/commands/impact.py

@@ -22,6 +22,7 @@
     get_changed_sql_models,
     get_file_at_ref,
     get_repo_root,
+    resolve_merge_base,
 )
 from ol_dbt_cli.lib.manifest import (
     ManifestRegistry,
@@ -282,7 +283,7 @@ def _get_downstream_yaml(
 def _analyse_model(
     model_name: str,
     sql_file: Path,
-    base_ref: str,
+    merge_base: str,
     yaml_registry: YamlRegistry,
     manifest: ManifestRegistry | None,
     sql_models_by_name: dict[str, ParsedModel],
@@ -300,8 +301,10 @@ def _analyse_model(
 
     current_cols = current_parsed.output_columns
 
-    # Base (origin/main) column set
-    base_content = get_file_at_ref(sql_file, base_ref, repo_root=repo_root)
+    # Base column set, read at the merge-base commit (branch point) rather than
+    # base_ref's current tip, so commits landed on base_ref after the branch
+    # point don't leak into the "base" content (see resolve_merge_base).
+    base_content = get_file_at_ref(sql_file, merge_base, repo_root=repo_root)
     if base_content is None:
         # New model — no base to compare against
         return ImpactAlert(
@@ -565,6 +568,12 @@ def impact(
         err_console.print(f"[red]Error:[/] {exc}")
         sys.exit(1)
 
+    try:
+        merge_base = resolve_merge_base(base_ref, repo_root=repo_root)
+    except RuntimeError as exc:
+        err_console.print(f"[red]Error resolving merge-base vs {base_ref}:[/] {exc}")
+        sys.exit(1)
+
     models_dir = dbt_dir / "models"
 
     # Resolve compiled SQL directory (Jinja-free, most accurate)
@@ -675,7 +684,7 @@ def impact(
         alert = _analyse_model(
             name,
             sql_file,
-            base_ref,
+            merge_base,
             yaml_registry,
             manifest,
             sql_models_by_name,

src/ol_dbt_cli/ol_dbt_cli/lib/git_utils.py

@@ -31,34 +31,55 @@ def get_repo_root(start: Path | None = None) -> Path:
         raise RuntimeError(msg) from exc
 
 
+def resolve_merge_base(base_ref: str, repo_root: Path | None = None) -> str:
+    """Return the merge-base commit SHA of *base_ref* and HEAD.
+
+    This is the fork point where the current branch diverged from *base_ref* —
+    diffing/fetching against it (rather than *base_ref* directly) gives
+    three-dot diff semantics, matching what a GitHub PR diff shows. Once
+    *base_ref* advances past the branch point (e.g. other PRs merge to main
+    while this one is open), a plain two-dot diff against *base_ref* would
+    incorrectly include those unrelated changes in the changed set.
+    """
+    root = repo_root or get_repo_root()
+    return _run_git(["merge-base", base_ref, "HEAD"], cwd=root).strip()
+
+
 def get_changed_files(
     base_ref: str = "origin/main",
     repo_root: Path | None = None,
+    *,
+    include_untracked: bool = True,
 ) -> list[Path]:
-    """Return absolute paths of tracked files changed vs *base_ref*.
-
-    Includes files changed relative to *base_ref* (committed), files staged for
-    commit, and unstaged modifications to tracked files.  Untracked (new) files
-    that have not yet been ``git add``-ed are **not** included.
+    """Return absolute paths of files changed since diverging from *base_ref*.
+
+    Diffs against ``merge-base(base_ref, HEAD)`` rather than *base_ref* directly
+    (three-dot semantics; see :func:`resolve_merge_base`), so commits landed on
+    *base_ref* after the branch point are excluded. Includes tracked files
+    changed since the branch point (committed), staged for commit, or with
+    unstaged modifications. New files that have not yet been ``git add``-ed are
+    also included by default (*include_untracked*) so local runs see
+    in-progress new models; set ``include_untracked=False`` to match a strict
+    porcelain diff (e.g. CI, where checkouts have no untracked files).
     """
     root = repo_root or get_repo_root()
-    # Files changed relative to base ref (committed + staged)
-    committed = _run_git(
-        ["diff", "--name-only", base_ref, "HEAD"],
-        cwd=root,
-    ).splitlines()
-    # Staged but not yet committed
-    staged = _run_git(
-        ["diff", "--name-only", "--cached"],
-        cwd=root,
-    ).splitlines()
-    # Unstaged modifications (tracked files)
-    unstaged = _run_git(
-        ["diff", "--name-only"],
-        cwd=root,
-    ).splitlines()
-
-    all_relative = set(committed) | set(staged) | set(unstaged)
+    merge_base = resolve_merge_base(base_ref, repo_root=root)
+
+    def _diff_names(*args: str) -> set[str]:
+        # -z: NUL-delimited, unquoted paths. Without it, git's default porcelain
+        # quoting (C-style escapes for whitespace/non-ASCII, e.g. "my model.sql"
+        # or "caf\303\251.sql") corrupts any such path into one that doesn't
+        # exist on disk, silently dropping it from the changed set.
+        raw = _run_git(["diff", "--name-only", "-z", *args], cwd=root)
+        return {p for p in raw.split("\0") if p}
+
+    # Files changed since the branch point (committed + staged + unstaged)
+    all_relative = _diff_names(merge_base, "HEAD") | _diff_names("--cached") | _diff_names()
+
+    if include_untracked:
+        status = _run_git(["status", "--porcelain", "--untracked-files=all", "-z"], cwd=root)
+        all_relative.update(entry[3:] for entry in status.split("\0") if entry.startswith("??"))
+
     return [root / p for p in sorted(all_relative)]
 
 

src/ol_dbt_cli/tests/test_git_utils.py

@@ -0,0 +1,148 @@
+"""Tests for lib/git_utils.py — git diff helpers for changed-model detection."""
+
+from __future__ import annotations
+
+import subprocess
+from pathlib import Path
+
+import pytest
+
+from ol_dbt_cli.lib.git_utils import (
+    get_changed_files,
+    get_changed_sql_models,
+    get_file_at_ref,
+    resolve_merge_base,
+)
+
+
+def _git(args: list[str], cwd: Path) -> str:
+    result = subprocess.run(  # noqa: S603, S607
+        ["git", *args],
+        cwd=cwd,
+        capture_output=True,
+        text=True,
+        check=True,
+        env={
+            "GIT_AUTHOR_NAME": "Test",
+            "GIT_AUTHOR_EMAIL": "test@example.com",
+            "GIT_COMMITTER_NAME": "Test",
+            "GIT_COMMITTER_EMAIL": "test@example.com",
+            "PATH": "/usr/bin:/bin:/usr/local/bin",
+        },
+    )
+    return result.stdout
+
+
+def _commit(repo: Path, message: str) -> str:
+    _git(["add", "-A"], cwd=repo)
+    _git(["commit", "-m", message], cwd=repo)
+    return _git(["rev-parse", "HEAD"], cwd=repo).strip()
+
+
+@pytest.fixture()
+def scripted_repo(tmp_path: Path) -> Path:
+    r"""Build a repo with a PR branch forked from main, plus later main-only commits.
+
+    History shape:
+        main:    C0 --- C1 (models/other.sql added; models/foo.sql gets an
+                   \        unrelated edit -- both AFTER the fork)
+        feature:    C0a (models/foo.sql: A,B -> A,C; models/bar.sql added)
+
+    C0 is the merge-base of main and feature. C1 exists only on main, after the
+    branch point:
+    - a two-dot diff (main..feature) would incorrectly show other.sql as
+      changed (it exists on main's tip but not on feature); a three-dot /
+      merge-base diff correctly excludes it.
+    - fetching foo.sql's "base" content via the raw base_ref tip (main) would
+      read C1's content (main's own later edit), not the C0 fork-point content
+      feature actually diverged from.
+    """
+    repo = tmp_path / "repo"
+    repo.mkdir()
+    _git(["init", "-b", "main"], cwd=repo)
+
+    models = repo / "models"
+    models.mkdir()
+    (models / "foo.sql").write_text("select a, b\n")
+    (models / "_schema.yml").write_text("version: 2\n")
+    merge_base = _commit(repo, "C0: initial models")
+
+    _git(["checkout", "-b", "feature"], cwd=repo)
+    (models / "foo.sql").write_text("select a, c\n")
+    (models / "bar.sql").write_text("select x\n")
+    _commit(repo, "C0a: feature changes foo.sql, adds bar.sql")
+
+    _git(["checkout", "main"], cwd=repo)
+    (models / "other.sql").write_text("select y\n")
+    (models / "foo.sql").write_text("select a, b, extra\n")
+    _commit(repo, "C1: main-only changes after the fork")
+
+    _git(["checkout", "feature"], cwd=repo)
+    repo.joinpath(".merge_base_sha").write_text(merge_base)
+    return repo
+
+
+class TestResolveMergeBase:
+    def test_returns_fork_point_not_base_tip(self, scripted_repo: Path) -> None:
+        expected = scripted_repo.joinpath(".merge_base_sha").read_text()
+        assert resolve_merge_base("main", repo_root=scripted_repo) == expected
+
+    def test_differs_from_base_ref_tip(self, scripted_repo: Path) -> None:
+        base_tip = _git(["rev-parse", "main"], cwd=scripted_repo).strip()
+        merge_base = resolve_merge_base("main", repo_root=scripted_repo)
+        assert merge_base != base_tip
+
+
+class TestGetChangedFiles:
+    def test_excludes_main_only_changes_after_fork(self, scripted_repo: Path) -> None:
+        changed = get_changed_files(base_ref="main", repo_root=scripted_repo, include_untracked=False)
+        names = {p.name for p in changed}
+        assert "other.sql" not in names, "main-only commit after the fork leaked into the changed set"
+
+    def test_includes_feature_branch_changes(self, scripted_repo: Path) -> None:
+        changed = get_changed_files(base_ref="main", repo_root=scripted_repo, include_untracked=False)
+        names = {p.name for p in changed}
+        assert "foo.sql" in names
+        assert "bar.sql" in names
+
+    def test_includes_untracked_files_by_default(self, scripted_repo: Path) -> None:
+        (scripted_repo / "models" / "baz.sql").write_text("select z\n")
+        changed = get_changed_files(base_ref="main", repo_root=scripted_repo)
+        assert "baz.sql" in {p.name for p in changed}
+
+    def test_excludes_untracked_files_when_disabled(self, scripted_repo: Path) -> None:
+        (scripted_repo / "models" / "baz.sql").write_text("select z\n")
+        changed = get_changed_files(base_ref="main", repo_root=scripted_repo, include_untracked=False)
+        assert "baz.sql" not in {p.name for p in changed}
+
+    def test_includes_untracked_files_with_special_characters_in_name(self, scripted_repo: Path) -> None:
+        # git's default porcelain quoting C-escapes paths containing whitespace or
+        # non-ASCII bytes (e.g. `?? "my model.sql"`); without -z that garbles the
+        # path into one that doesn't exist on disk and silently drops the file.
+        (scripted_repo / "models" / "my model.sql").write_text("select 1\n")
+        (scripted_repo / "models" / "café.sql").write_text("select 2\n")
+        changed = get_changed_files(base_ref="main", repo_root=scripted_repo)
+        names = {p.name for p in changed}
+        assert "my model.sql" in names
+        assert "café.sql" in names
+
+
+class TestGetChangedSqlModels:
+    def test_changed_set_matches_three_dot_semantics(self, scripted_repo: Path) -> None:
+        names = get_changed_sql_models(scripted_repo, base_ref="main", repo_root=scripted_repo)
+        assert set(names) == {"foo", "bar"}
+
+
+class TestGetFileAtRef:
+    def test_merge_base_content_is_fork_point_not_base_tip(self, scripted_repo: Path) -> None:
+        merge_base = resolve_merge_base("main", repo_root=scripted_repo)
+        content = get_file_at_ref(scripted_repo / "models" / "foo.sql", merge_base, repo_root=scripted_repo)
+        assert content == "select a, b\n"
+
+    def test_base_ref_tip_content_is_polluted_by_later_main_commits(self, scripted_repo: Path) -> None:
+        # Demonstrates the bug this task fixes: fetching against the raw
+        # base_ref tip (the old behaviour) reads main's own later edit to
+        # foo.sql (C1), not the C0 fork-point content feature actually
+        # diverged from -- masking or misrepresenting the PR's real diff.
+        content = get_file_at_ref(scripted_repo / "models" / "foo.sql", "main", repo_root=scripted_repo)
+        assert content == "select a, b, extra\n"