Codex preflight: scan all paths in rename/copy --name-status lines

R8 review on PR #421:

P1: `_scan_prompt_artifacts()` only inspected the substring after the
first tab in each --name-status line, treating it as a single path. For
rename (`R*`) and copy (`C*`) lines the format is multi-column:

  R100\told_path\tnew_path
  C100\told_path\tnew_path

So `R100\tconfig/.env\tconfig/renamed.txt` was parsed as one pseudo-path
"config/.env\tconfig/renamed.txt", whose os.path.basename is just
"renamed.txt" — completely missing the sensitive old path. The diff
body still contains the renamed file's prior content, so the abort gate
should have fired but didn't.

Fix: split each line on \t into status + N paths, scan EACH path
independently for sensitive basenames. Empty paths skipped.

Tests added (5 new, all assert the abort/detection that R8 said was
unpinned):
  - test_changed_files_rename_old_sensitive_new_safe
    (R100\tconfig/.env\tconfig/renamed.txt → detect)
  - test_changed_files_copy_old_sensitive_new_safe
    (C100\tconfig/.env\tconfig/copy.txt → detect)
  - test_changed_files_rename_new_path_also_scanned
    (R100\tconfig/old.txt\tconfig/.env → detect)
  - test_delta_changed_files_rename_also_scanned
    (same pattern via delta_changed_files_text)
  - test_aborts_on_renamed_sensitive_file_in_changed_files
    (non-dry-run integration: rename triggers main()-gate abort)

Tests: 252 pass (5 new).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Author: igerber

.claude/scripts/openai_review.py

@@ -1425,25 +1425,37 @@ def _scan_prompt_artifacts(
     if delta_diff_text and SECRET_CONTENT_PATTERN.search(delta_diff_text):
         findings.append("[delta diff body]")
 
-    # Filename scan over changed-files lists. Each line is a name-status
-    # entry like "M\tfoo.py" or "D\tconfig/.env"; extract the path column.
+    # Filename scan over changed-files lists. Each line is a `git diff
+    # --name-status` entry:
+    #   M\tfoo.py
+    #   D\tconfig/.env
+    #   R100\told/.env\tnew/file.txt    (rename — TWO paths)
+    #   C100\told/.env\tnew/copy.txt    (copy — TWO paths)
+    # We must scan EACH path independently so an old-sensitive→new-safe
+    # rename still aborts (the diff body still contains the old file's
+    # content, and the rename target may incorporate it).
     def _scan_changed_files(text: str, label: str) -> None:
         for line in text.splitlines():
             line = line.strip()
             if not line:
                 continue
-            parts = line.split("\t", 1) if "\t" in line else line.split(None, 1)
-            path = parts[1] if len(parts) > 1 else parts[0]
-            basename_lc = os.path.basename(path).lower()
-            if any(
-                basename_lc.endswith(suffix)
-                for suffix in SENSITIVE_FILE_SAFE_SUFFIXES
-            ):
-                continue
-            for pat in SENSITIVE_FILE_PATTERNS:
-                if fnmatch.fnmatch(basename_lc, pat):
-                    findings.append(f"[{label}: {path}]")
-                    break
+            fields = line.split("\t")
+            # First field is the status code (M/A/D/R*/C*/T/...); the
+            # remaining fields are 1+ paths.
+            paths = fields[1:] if len(fields) > 1 else fields[:1]
+            for path in paths:
+                if not path:
+                    continue
+                basename_lc = os.path.basename(path).lower()
+                if any(
+                    basename_lc.endswith(suffix)
+                    for suffix in SENSITIVE_FILE_SAFE_SUFFIXES
+                ):
+                    continue
+                for pat in SENSITIVE_FILE_PATTERNS:
+                    if fnmatch.fnmatch(basename_lc, pat):
+                        findings.append(f"[{label}: {path}]")
+                        break
 
     _scan_changed_files(changed_files_text or "", "changed-files")
     if delta_changed_files_text:

tests/test_openai_review.py

@@ -2424,6 +2424,58 @@ def test_changed_files_skips_safe_variants(self, review_mod):
         )
         assert result == []
 
+    def test_changed_files_rename_old_sensitive_new_safe(self, review_mod):
+        """`git diff --name-status` rename lines have TWO paths:
+        `R100\\told\\tnew`. If the OLD path is sensitive (e.g. config/.env
+        renamed to config/renamed.txt), the diff body still contains the
+        old file's contents — must abort. Earlier parser only inspected
+        the combined post-first-tab substring, missing this case."""
+        result = review_mod._scan_prompt_artifacts(
+            diff_text="",
+            previous_review=None,
+            delta_diff_text=None,
+            changed_files_text="R100\tconfig/.env\tconfig/renamed.txt",
+            delta_changed_files_text=None,
+        )
+        assert any(".env" in r for r in result), (
+            f"Expected sensitive OLD path to be detected; got: {result}"
+        )
+
+    def test_changed_files_copy_old_sensitive_new_safe(self, review_mod):
+        """`C100\\told\\tnew` copy lines have the same structure as rename
+        lines — both paths must be scanned."""
+        result = review_mod._scan_prompt_artifacts(
+            diff_text="",
+            previous_review=None,
+            delta_diff_text=None,
+            changed_files_text="C100\tconfig/.env\tconfig/copy.txt",
+            delta_changed_files_text=None,
+        )
+        assert any(".env" in r for r in result)
+
+    def test_changed_files_rename_new_path_also_scanned(self, review_mod):
+        """If the NEW path of a rename is sensitive (someone renamed
+        config.txt → .env), that also triggers."""
+        result = review_mod._scan_prompt_artifacts(
+            diff_text="",
+            previous_review=None,
+            delta_diff_text=None,
+            changed_files_text="R100\tconfig/old.txt\tconfig/.env",
+            delta_changed_files_text=None,
+        )
+        assert any(".env" in r for r in result)
+
+    def test_delta_changed_files_rename_also_scanned(self, review_mod):
+        """Same logic applies to delta_changed_files_text."""
+        result = review_mod._scan_prompt_artifacts(
+            diff_text="",
+            previous_review=None,
+            delta_diff_text=None,
+            changed_files_text="",
+            delta_changed_files_text="R100\tconfig/.env\tconfig/safe.txt",
+        )
+        assert any(".env" in r for r in result)
+
     def test_clean_inputs_return_empty(self, review_mod):
         result = review_mod._scan_prompt_artifacts(
             diff_text="--- a/foo\n+++ b/foo\n@@ -1 +1 @@\n-x\n+y\n",
@@ -2865,6 +2917,24 @@ def test_allow_secrets_overrides_prompt_artifact_gate(
         assert "ABORT" not in captured.err
         assert gate_env["codex_calls"]["called"]
 
+    def test_aborts_on_renamed_sensitive_file_in_changed_files(
+        self, gate_env, review_mod, capsys
+    ):
+        """`R100\\told_sensitive\\tnew_safe` rename: old path was a .env,
+        new is a safe rename. The diff body still contains the old
+        file's contents → must abort."""
+        (gate_env["tmp_path"] / "README.md").write_text("# clean repo")
+        gate_env["files"].write_text(
+            "R100\tconfig/.env\tconfig/renamed.txt\n"
+        )
+        with pytest.raises(SystemExit) as exc:
+            self._run(gate_env, review_mod)
+        assert exc.value.code == 1
+        captured = capsys.readouterr()
+        assert "ABORT" in captured.err
+        assert ".env" in captured.err
+        assert not gate_env["codex_calls"]["called"]
+
 
 class TestExtractResponseText:
     def test_prefers_output_text_field(self, review_mod):