commit_check/engine.py (2)

463-484: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Deduplicate ref verification and broaden exception handling.

The local and remote checks are identical except for the ref string, and both only catch subprocess.CalledProcessError, so a missing git executable (FileNotFoundError/OSError) would propagate unhandled instead of falling through to return None like the rest of this method does.

♻️ Proposed refactor to consolidate the duplicated verification logic

-        # Try local branch first
-        try:
-            subprocess.run(
-                ["git", "rev-parse", "--verify", branch_name],
-                stdout=subprocess.DEVNULL,
-                stderr=subprocess.DEVNULL,
-                check=True,
-            )
-            return branch_name
-        except subprocess.CalledProcessError:
-            pass
-
-        # Try remote tracking branch under origin/
-        try:
-            subprocess.run(
-                ["git", "rev-parse", "--verify", f"origin/{branch_name}"],
-                stdout=subprocess.DEVNULL,
-                stderr=subprocess.DEVNULL,
-                check=True,
-            )
-            return branch_name
-        except subprocess.CalledProcessError:
-            pass
-
-        return None
+        def _ref_exists(ref: str) -> bool:
+            try:
+                subprocess.run(
+                    ["git", "rev-parse", "--verify", ref],
+                    stdout=subprocess.DEVNULL,
+                    stderr=subprocess.DEVNULL,
+                    check=True,
+                )
+                return True
+            except (subprocess.CalledProcessError, OSError):
+                return False
+
+        if _ref_exists(branch_name) or _ref_exists(f"origin/{branch_name}"):
+            return branch_name
+
+        return None

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@commit_check/engine.py` around lines 463 - 484, The ref verification in the
branch lookup method is duplicated and only handles
subprocess.CalledProcessError, so missing git or other OS-level failures can
escape instead of falling through cleanly. Refactor the local and origin/ checks
in the branch-resolution logic to share a single verification path keyed by the
ref string, and broaden the exception handling around subprocess.run in this
method to also catch FileNotFoundError/OSError so it still returns None on any
verification failure.

---

455-456: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Move subprocess/re imports to module top-level.

Local import subprocess / import re inside the method is non-idiomatic; both are cheap, stdlib, and unconditionally needed, so hoist them to the top of the file for consistency with the rest of the module.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@commit_check/engine.py` around lines 455 - 456, Move the local subprocess and
re imports out of the method and into the module-level import block in
engine.py; these stdlib imports are used unconditionally, so update the
top-of-file imports and remove the in-method import statements from the code
path around the affected function.

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@commit_check/engine.py`:
- Around line 463-484: The ref verification in the branch lookup method is
duplicated and only handles subprocess.CalledProcessError, so missing git or
other OS-level failures can escape instead of falling through cleanly. Refactor
the local and origin/ checks in the branch-resolution logic to share a single
verification path keyed by the ref string, and broaden the exception handling
around subprocess.run in this method to also catch FileNotFoundError/OSError so
it still returns None on any verification failure.
- Around line 455-456: Move the local subprocess and re imports out of the
method and into the module-level import block in engine.py; these stdlib imports
are used unconditionally, so update the top-of-file imports and remove the
in-method import statements from the code path around the affected function.

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 29e5e2c2-0bbd-47b2-8b8c-439c762ea853

Reviewing files that changed from the base of the PR and between ac1e9a9 and 4735bee.

• commit_check/engine.py
• tests/engine_test.py