fix(ci): count deletions and honor the Terraform working directory

Deleted paths were excluded from the isolation diff, so a mixed change could pass. The checker now treats working-directory as the Terraform prefix.

.github/workflows/ci-terraform.yaml

@@ -19,7 +19,8 @@ name: CI — Terraform
 # unchanged. A trailing slash is a directory prefix. Any other line is an
 # exact file. pull_request classifies the merge-base of the base SHA to HEAD.
 # merge_group classifies each first-parent commit against its parent, so a
-# Terraform-only PR stacked with an app-only PR still passes.
+# Terraform-only PR stacked with an app-only PR still passes. The classified
+# diff includes deletions. Terraform paths are those under working-directory.
 
 on:
   workflow_call:
@@ -85,6 +86,7 @@ jobs:
         working-directory: ${{ github.workspace }}
         env:
           APP_PATHS: ${{ inputs.app-paths }}
+          TERRAFORM_DIR: ${{ inputs.working-directory }}
           EVENT_NAME: ${{ github.event_name }}
           PR_BASE_SHA: ${{ github.event.pull_request.base.sha }}
           MERGE_GROUP_BASE_SHA: ${{ github.event.merge_group.base_sha }}
@@ -101,7 +103,7 @@ jobs:
                 exit 1
               fi
               merge_base="$(git merge-base "${PR_BASE_SHA}" HEAD)"
-              git diff --name-only --diff-filter=ACMR "${merge_base}" HEAD | classify
+              git diff --name-only --diff-filter=ACMRD "${merge_base}" HEAD | classify
               ;;
             merge_group)
               if [[ -z "${MERGE_GROUP_BASE_SHA}" ]]; then
@@ -110,7 +112,7 @@ jobs:
               fi
               while IFS= read -r sha; do
                 [[ -z "${sha}" ]] && continue
-                git diff --name-only --diff-filter=ACMR "${sha}^" "${sha}" | classify
+                git diff --name-only --diff-filter=ACMRD "${sha}^" "${sha}" | classify
               done < <(git rev-list --reverse --first-parent "${MERGE_GROUP_BASE_SHA}..HEAD")
               ;;
             *)

scripts/check_app_terraform_isolation.py

@@ -5,6 +5,9 @@ APP_PATHS is newline-separated. A trailing slash is a directory prefix.
 Any other entry is an exact file. Paths that are not listed are neutral, so
 workflows, docs, and tests may travel with either side. An empty APP_PATHS
 skips the check.
+
+TERRAFORM_DIR is the Terraform working directory (default terraform). A path
+is Terraform when it equals that directory or sits under it.
 """
 
 from __future__ import annotations
@@ -28,9 +31,15 @@ def parse_app_rules(raw: str) -> tuple[frozenset[str], frozenset[str]]:
     return frozenset(prefixes), frozenset(exact)
 
 
-def is_terraform_path(path: str) -> bool:
+def terraform_prefix(terraform_dir: str) -> str:
+    prefix = terraform_dir.replace("\\", "/").strip().strip("/")
+    return prefix or "terraform"
+
+
+def is_terraform_path(path: str, terraform_dir: str = "terraform") -> bool:
     normalized = path.replace("\\", "/")
-    return normalized == "terraform" or normalized.startswith("terraform/")
+    prefix = terraform_prefix(terraform_dir)
+    return normalized == prefix or normalized.startswith(f"{prefix}/")
 
 
 def is_app_path(path: str, prefixes: frozenset[str], exact: frozenset[str]) -> bool:
@@ -44,12 +53,16 @@ def is_app_path(path: str, prefixes: frozenset[str], exact: frozenset[str]) -> b
 
 
 def isolation_violation(
-    paths: list[str], app_paths: str
+    paths: list[str],
+    app_paths: str,
+    terraform_dir: str = "terraform",
 ) -> tuple[list[str], list[str]] | None:
     prefixes, exact = parse_app_rules(app_paths)
     if not prefixes and not exact:
         return None
-    terraform_files = sorted({path for path in paths if is_terraform_path(path)})
+    terraform_files = sorted(
+        {path for path in paths if is_terraform_path(path, terraform_dir)}
+    )
     app_files = sorted({path for path in paths if is_app_path(path, prefixes, exact)})
     if terraform_files and app_files:
         return terraform_files, app_files
@@ -57,10 +70,12 @@ def isolation_violation(
 
 
 def first_isolation_violation(
-    file_sets: list[list[str]], app_paths: str
+    file_sets: list[list[str]],
+    app_paths: str,
+    terraform_dir: str = "terraform",
 ) -> tuple[list[str], list[str]] | None:
     for paths in file_sets:
-        violation = isolation_violation(paths, app_paths)
+        violation = isolation_violation(paths, app_paths, terraform_dir)
         if violation is not None:
             return violation
     return None
@@ -77,12 +92,18 @@ def main() -> int:
     paths = list(args.paths)
     if not paths and not sys.stdin.isatty():
         paths = [line.strip() for line in sys.stdin if line.strip()]
-    violation = isolation_violation(paths, os.environ.get("APP_PATHS", ""))
+    terraform_dir = terraform_prefix(os.environ.get("TERRAFORM_DIR", "terraform"))
+    violation = isolation_violation(
+        paths, os.environ.get("APP_PATHS", ""), terraform_dir
+    )
     if violation is None:
         print("PASS: application and Terraform changes are isolated")
         return 0
     terraform_files, app_files = violation
-    print("FAIL: do not mix deployable application files with terraform/", file=sys.stderr)
+    print(
+        f"FAIL: do not mix deployable application files with {terraform_dir}/",
+        file=sys.stderr,
+    )
     print("terraform:", file=sys.stderr)
     for path in terraform_files:
         print(f"  {path}", file=sys.stderr)

scripts/test_check_app_terraform_isolation.py

@@ -3,10 +3,19 @@
 
 from __future__ import annotations
 
+import os
+import re
+import subprocess
+import tempfile
 import unittest
+from pathlib import Path
 
 from check_app_terraform_isolation import first_isolation_violation, isolation_violation
 
+WORKFLOW = (
+    Path(__file__).resolve().parents[1] / ".github" / "workflows" / "ci-terraform.yaml"
+)
+
 APP_PATHS = "src/\npackage.json\npackage-lock.json\n"
 
 
@@ -76,6 +85,143 @@ class IsolationTests(unittest.TestCase):
         )
         self.assertIsNotNone(violation)
 
+    def test_non_default_terraform_dir_mixed_fails(self) -> None:
+        violation = isolation_violation(
+            ["infra/main.tf", "src/app.js"],
+            "src/\n",
+            terraform_dir="infra",
+        )
+        self.assertIsNotNone(violation)
+        terraform_files, app_files = violation or ([], [])
+        self.assertEqual(terraform_files, ["infra/main.tf"])
+        self.assertEqual(app_files, ["src/app.js"])
+
+    def test_default_dir_leaves_other_prefixes_neutral(self) -> None:
+        self.assertIsNone(
+            isolation_violation(["infra/main.tf", "src/app.js"], "src/\n")
+        )
+
+    def test_terraform_prefix_does_not_match_a_longer_directory(self) -> None:
+        self.assertIsNone(
+            isolation_violation(
+                ["infrastructure/main.tf", "src/app.js"],
+                "src/\n",
+                terraform_dir="infra",
+            )
+        )
+
+    def test_terraform_dir_trailing_slash(self) -> None:
+        violation = isolation_violation(
+            ["infra/main.tf", "src/app.js"],
+            "src/\n",
+            terraform_dir="infra/",
+        )
+        self.assertIsNotNone(violation)
+
+    def test_blank_terraform_dir_defaults_to_terraform(self) -> None:
+        violation = isolation_violation(
+            ["terraform/lambda.tf", "src/app.js"],
+            "src/\n",
+            terraform_dir="  ",
+        )
+        self.assertIsNotNone(violation)
+
+
+class WorkflowDiffTests(unittest.TestCase):
+    def test_classified_diffs_include_deletions(self) -> None:
+        self.assertEqual(_workflow_diff_filters(), ["ACMRD", "ACMRD"])
+
+    def test_workflow_passes_working_directory(self) -> None:
+        self.assertIn(
+            "TERRAFORM_DIR: ${{ inputs.working-directory }}",
+            WORKFLOW.read_text(),
+        )
+
+    def test_deleted_app_file_is_classified(self) -> None:
+        diff_filter = _workflow_diff_filters()[0]
+        with tempfile.TemporaryDirectory() as tmp:
+            repo = Path(tmp)
+            base = _commit_base(repo)
+            (repo / "terraform" / "lambda.tf").write_text("changed\n")
+            (repo / "src" / "processPaymentCsv.js").unlink()
+            _git(repo, "add", "-A")
+            _git(repo, "commit", "-m", "decommission handler")
+            omitted = _changed_paths(repo, base, "HEAD", "ACMR")
+            included = _changed_paths(repo, base, "HEAD", diff_filter)
+        self.assertNotIn("src/processPaymentCsv.js", omitted)
+        self.assertIn("src/processPaymentCsv.js", included)
+        self.assertIn("terraform/lambda.tf", included)
+        self.assertIsNotNone(isolation_violation(included, APP_PATHS))
+
+    def test_deleted_terraform_file_is_classified(self) -> None:
+        diff_filter = _workflow_diff_filters()[0]
+        with tempfile.TemporaryDirectory() as tmp:
+            repo = Path(tmp)
+            base = _commit_base(repo)
+            (repo / "terraform" / "lambda.tf").unlink()
+            (repo / "src" / "processPaymentCsv.js").write_text("changed\n")
+            _git(repo, "add", "-A")
+            _git(repo, "commit", "-m", "remove lambda")
+            omitted = _changed_paths(repo, base, "HEAD", "ACMR")
+            included = _changed_paths(repo, base, "HEAD", diff_filter)
+        self.assertNotIn("terraform/lambda.tf", omitted)
+        violation = isolation_violation(included, APP_PATHS)
+        self.assertIsNotNone(violation)
+        terraform_files, app_files = violation or ([], [])
+        self.assertEqual(terraform_files, ["terraform/lambda.tf"])
+        self.assertEqual(app_files, ["src/processPaymentCsv.js"])
+
+
+def _workflow_diff_filters() -> list[str]:
+    return re.findall(r"--diff-filter=([A-Z]+)", WORKFLOW.read_text())
+
+
+def _git(repo: Path, *args: str) -> str:
+    env = os.environ.copy()
+    env["GIT_CONFIG_GLOBAL"] = os.devnull
+    env["GIT_CONFIG_NOSYSTEM"] = "1"
+    completed = subprocess.run(
+        [
+            "git",
+            "-c",
+            "commit.gpgsign=false",
+            "-c",
+            "user.name=test",
+            "-c",
+            "user.email=test@example.com",
+            *args,
+        ],
+        cwd=repo,
+        check=True,
+        capture_output=True,
+        text=True,
+        env=env,
+    )
+    return completed.stdout
+
+
+def _commit_base(repo: Path) -> str:
+    _git(repo, "init", "-b", "main")
+    (repo / "terraform").mkdir()
+    (repo / "src").mkdir()
+    (repo / "terraform" / "lambda.tf").write_text("resource\n")
+    (repo / "src" / "processPaymentCsv.js").write_text("export {}\n")
+    _git(repo, "add", ".")
+    _git(repo, "commit", "-m", "base")
+    return _git(repo, "rev-parse", "HEAD").strip()
+
+
+def _changed_paths(repo: Path, base: str, head: str, diff_filter: str) -> list[str]:
+    output = _git(
+        repo,
+        "diff",
+        "--name-only",
+        f"--diff-filter={diff_filter}",
+        base,
+        head,
+    )
+    return [line for line in output.splitlines() if line]
+
 
 if __name__ == "__main__":
     unittest.main()