fix(ol_dbt_cli): impact reports BREAKING for deleted models still ref()'d (#2446)

* 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.

* fix(ol_dbt_cli): impact reports BREAKING for deleted models still ref()'d

impact.py's per-model loop looked up each changed stem in sql_file_map
(built from files currently on disk) and silently `continue`d when a
deleted model's stem wasn't found -- so deleting int__combined__users
while three marts still ref() it produced "0 breaking", exit 0. Renames
hit the same skip on the old stem. `ol-dbt impact --model <typo>` also
silently reported success instead of erroring, unlike validate's
--model handling.

Add git_utils.get_deleted_sql_models(), which diffs --diff-filter=D
against the merge-base to map deleted model stems to their last path.
Add _analyse_deleted_model(), which reads the model's column set at the
merge-base and flags BREAKING for every model whose refs still name it
(scanned across all currently-parsed SQL, so it doesn't depend on a
fresh manifest reflecting a state dbt would refuse to compile). Wire it
into the main loop in place of the silent skip, and error loudly (exit
1) when --model doesn't match any on-disk or just-deleted model.

impact.py had zero end-to-end tests. Added a scripted-repo test class
covering: deletion with a surviving consumer (BREAKING, exit 1),
deletion with no remaining consumers (not BREAKING), and an unknown
--model value (exit 1). All three fail against the pre-fix code.

Author: blarghmatey

src/ol_dbt_cli/ol_dbt_cli/commands/impact.py

@@ -20,6 +20,7 @@
 
 from ol_dbt_cli.lib.git_utils import (
     get_changed_sql_models,
+    get_deleted_sql_models,
     get_file_at_ref,
     get_repo_root,
     resolve_merge_base,
@@ -366,6 +367,59 @@ def _analyse_model(
     )
 
 
+def _analyse_deleted_model(
+    model_name: str,
+    path_at_base: Path,
+    merge_base: str,
+    manifest: ManifestRegistry | None,
+    sql_models_by_name: dict[str, ParsedModel],
+    repo_root: Path,
+) -> ImpactAlert:
+    """Return an :class:`ImpactAlert` for *model_name*, deleted since *merge_base*.
+
+    The relation no longer exists, so any surviving ``ref()`` to it is broken
+    outright — not just for specific columns. Consumers are found via textual
+    ``ref()`` scanning of every currently-parsed model (works even without a
+    manifest, and even though a *fresh* manifest can never itself contain a
+    dangling ref to a deleted model — dbt parse would already have failed on
+    it) plus, if a stale manifest still has the deleted node registered, its
+    recorded children.
+    """
+    base_content = get_file_at_ref(path_at_base, merge_base, repo_root=repo_root)
+    base_cols: set[str] = set()
+    if base_content is not None:
+        base_cols = parse_model_sql_at_content(model_name, base_content).output_columns
+
+    consumers = {name for name, parsed in sql_models_by_name.items() if model_name in parsed.refs}
+
+    manifest_available = False
+    if manifest is not None:
+        manifest_model = manifest.get_model(model_name)
+        if manifest_model is not None:
+            manifest_available = True
+            consumers |= {c.name for c in manifest.get_children(manifest_model.unique_id)}
+
+    downstream = [
+        DownstreamImpact(model_name=c, affected_columns=sorted(base_cols), depth=1) for c in sorted(consumers)
+    ]
+
+    if downstream:
+        level = AlertLevel.BREAKING
+        msg = f"Model '{model_name}' was deleted but is still ref()'d by {len(downstream)} downstream model(s)."
+    else:
+        level = AlertLevel.INFO
+        msg = f"Model '{model_name}' was deleted — no remaining ref() to it detected."
+
+    return ImpactAlert(
+        level=level,
+        changed_model=model_name,
+        column_changes=[ColumnChange(column=c, change_type="removed") for c in sorted(base_cols)],
+        downstream=downstream,
+        message=msg,
+        manifest_available=manifest_available,
+    )
+
+
 # ---------------------------------------------------------------------------
 # Output rendering
 # ---------------------------------------------------------------------------
@@ -618,8 +672,17 @@ def impact(
         except Exception as exc:  # noqa: BLE001
             parse_errors[name] = str(exc)
 
+    try:
+        deleted_models = get_deleted_sql_models(dbt_dir, base_ref=base_ref, repo_root=repo_root)
+    except RuntimeError as exc:
+        err_console.print(f"[red]Error resolving git diff:[/] {exc}")
+        sys.exit(1)
+
     # Determine which models to analyse
     if model:
+        if model not in sql_file_map and model not in deleted_models:
+            err_console.print(f"[red]Error:[/] --model value '{model}' did not match any model file.")
+            sys.exit(1)
         target_names = [model]
     else:
         try:
@@ -680,6 +743,11 @@ def impact(
     for name in target_names:
         sql_file = sql_file_map.get(name)
         if sql_file is None:
+            path_at_base = deleted_models.get(name)
+            if path_at_base is not None:
+                alerts.append(
+                    _analyse_deleted_model(name, path_at_base, merge_base, manifest, sql_models_by_name, repo_root)
+                )
             continue
         alert = _analyse_model(
             name,

src/ol_dbt_cli/ol_dbt_cli/lib/git_utils.py

@@ -99,6 +99,33 @@ def get_changed_sql_models(
     return names
 
 
+def get_deleted_sql_models(
+    dbt_dir: Path,
+    base_ref: str = "origin/main",
+    repo_root: Path | None = None,
+) -> dict[str, Path]:
+    """Return ``{model_name: path_at_merge_base}`` for .sql files deleted since *base_ref*.
+
+    Scoped to *dbt_dir*/models/. The returned paths no longer exist on disk —
+    pass them to :func:`get_file_at_ref` with the merge-base ref (see
+    :func:`resolve_merge_base`) to read their last content.
+    """
+    root = repo_root or get_repo_root(dbt_dir)
+    models_dir = dbt_dir / "models"
+    merge_base = resolve_merge_base(base_ref, repo_root=root)
+    output = _run_git(["diff", "--name-status", "--diff-filter=D", merge_base, "HEAD"], cwd=root)
+    deleted: dict[str, Path] = {}
+    for line in output.splitlines():
+        parts = line.split("\t")
+        if len(parts) != 2:  # noqa: PLR2004
+            continue
+        _status, rel_path = parts
+        path = root / rel_path
+        if path.suffix == ".sql" and _is_under(path, models_dir):
+            deleted[path.stem] = path
+    return deleted
+
+
 def get_changed_yaml_models(
     dbt_dir: Path,
     base_ref: str = "origin/main",

src/ol_dbt_cli/tests/test_impact.py

@@ -2,8 +2,11 @@
 
 from __future__ import annotations
 
+import subprocess
 from pathlib import Path
 
+import pytest
+
 from ol_dbt_cli.commands.impact import _diff_columns
 
 
@@ -481,3 +484,95 @@ def test_downstream_manifest_skips_child_when_sql_confirms_no_consumption(self)
         # SQL confirmed no consumption (output_columns doesn't include removed_col
         # and no SQL file for qualified-ref analysis) — child should NOT be flagged
         assert not any(r.model_name == "child" and r.affected_columns for r in results)
+
+
+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) -> None:
+    _git(["add", "-A"], cwd=repo)
+    _git(["commit", "-m", message], cwd=repo)
+
+
+@pytest.fixture()
+def dbt_repo(tmp_path: Path) -> Path:
+    """Build a scripted git repo containing a two-model dbt project, ready to have one deleted."""
+    repo = tmp_path / "repo"
+    repo.mkdir()
+    _git(["init", "-b", "main"], cwd=repo)
+
+    dbt_dir = repo / "src" / "ol_dbt"
+    models = dbt_dir / "models"
+    models.mkdir(parents=True)
+    (dbt_dir / "dbt_project.yml").write_text("name: open_learning\n")
+    (models / "int__combined__users.sql").write_text("select 1 as user_id, 'x' as email\n")
+    (models / "marts__reporting__thing.sql").write_text("select user_id from {{ ref('int__combined__users') }}\n")
+    _commit(repo, "initial models")
+    _git(["checkout", "-b", "feature"], cwd=repo)
+    return repo
+
+
+class TestImpactCommandDeletedModels:
+    """End-to-end tests for the `impact` command's handling of deleted models."""
+
+    def test_deleted_model_still_ref_d_is_breaking(self, dbt_repo: Path, capsys: pytest.CaptureFixture[str]) -> None:
+        """Deleting a model still ref()'d elsewhere must alert BREAKING and exit 1."""
+        from ol_dbt_cli.commands.impact import impact
+
+        dbt_dir = dbt_repo / "src" / "ol_dbt"
+        _git(["rm", "models/int__combined__users.sql"], cwd=dbt_dir)
+        _commit(dbt_repo, "delete int__combined__users")
+
+        with pytest.raises(SystemExit) as exc_info:
+            impact(dbt_dir_path=str(dbt_dir), base_ref="main", output_format="json")
+        assert exc_info.value.code == 1
+
+        import json
+
+        alerts = json.loads(capsys.readouterr().out)
+        deleted_alert = next(a for a in alerts if a["changed_model"] == "int__combined__users")
+        assert deleted_alert["level"] == "BREAKING"
+        assert any(d["model"] == "marts__reporting__thing" for d in deleted_alert["downstream"])
+
+    def test_deleted_model_with_no_remaining_refs_is_not_breaking(
+        self, dbt_repo: Path, capsys: pytest.CaptureFixture[str]
+    ) -> None:
+        """Deleting a model with no surviving ref() must not alert BREAKING."""
+        from ol_dbt_cli.commands.impact import impact
+
+        dbt_dir = dbt_repo / "src" / "ol_dbt"
+        # Remove the consumer too, so nothing references the deleted model anymore.
+        _git(["rm", "models/int__combined__users.sql", "models/marts__reporting__thing.sql"], cwd=dbt_dir)
+        _commit(dbt_repo, "delete both models")
+
+        impact(dbt_dir_path=str(dbt_dir), base_ref="main", output_format="json")
+
+        import json
+
+        alerts = json.loads(capsys.readouterr().out)
+        deleted_alert = next(a for a in alerts if a["changed_model"] == "int__combined__users")
+        assert deleted_alert["level"] != "BREAKING"
+
+    def test_unknown_model_flag_errors_loudly(self, dbt_repo: Path) -> None:
+        """`--model <typo>` must error and exit non-zero, not silently report success."""
+        from ol_dbt_cli.commands.impact import impact
+
+        dbt_dir = dbt_repo / "src" / "ol_dbt"
+        with pytest.raises(SystemExit) as exc_info:
+            impact(dbt_dir_path=str(dbt_dir), model="does_not_exist_typo", base_ref="main", output_format="json")
+        assert exc_info.value.code == 1