fix(hook): close branch-guard bypasses for bare filenames, new dirs, missing path fields

[getappz]

Summary

• Follow-up to fix(hook): resolve PreToolUse branch guard against target file's repo, not host cwd #283: CodeRabbit flagged a critical finding on that PR (inline review comment) that was squash-merged without a fix.
• redirect_decision's target-repo resolution called repo_toplevel() directly on file_path.parent(), which fails (ENOENT) and silently skips the branch guard for a bare filename (parent ""), a new file under a not-yet-created directory, or any mutating tool call with no top-level file_path/path (e.g. MultiEdit) — no cwd fallback existed for that last case despite fix(hook): resolve PreToolUse branch guard against target file's repo, not host cwd #283's stated intent.
• Fix: walk Path::ancestors() to the first directory that actually exists before resolving its repo toplevel, and fall back to cwd when the path can't be extracted at all. Genuinely out-of-repo targets still skip the guard as before.

Test plan

• cargo build — clean
• cargo fmt --check — clean
• cargo test hook_redirect — 21/21 pass, including 4 new regression tests (bare filename, new nested dir, missing path field, still-skips-outside-repo)
• cargo clippy --workspace --all-features -- -D warnings — pre-existing unrelated dead-code failure in daemon_autostart.rs confirmed present on master before this change (same class as tracked items handoff: assign items + attach versioned assets instead of raw artifacts #169/chore: add CODEOWNERS #239)

Summary by CodeRabbit

• Bug Fixes
  • Improved branch protection for file changes across nested and newly created directories.
  • Restored protection for bare filenames and tool inputs without an explicit file path.
  • Targets outside a Git repository remain unguarded as expected.
• Tests
  • Added coverage for repository detection, working-directory fallbacks, and paths that do not yet exist.

[coderabbitai]

Caution

Review failed

An error occurred during the review process. Please try again later.

📝 Walkthrough

Walkthrough

redirect_decision now resolves mutating-tool repositories by walking target path ancestors, falls back to the host cwd when needed, and adds synchronized regression tests for nested, bare, missing-field, and outside-repository inputs.

Changes

Mutating-tool repository resolution

Layer / File(s)	Summary
Path-based repository guard src/hook_redirect.rs	Mutating-tool paths are resolved through existing ancestors, branch guards use the discovered repository, and targets outside git repositories remain unguarded.
Repository resolution regression coverage src/hook_redirect.rs	CWD-safe test helpers and temporary repositories support regression tests for bare filenames, missing directories, missing path fields, and external targets.

Estimated code review effort: 4 (Complex) | ~45 minutes

Possibly related PRs

• getappz/agentflare#170: Both changes modify redirect_decision in src/hook_redirect.rs.
• getappz/agentflare#281: Both changes affect mutating-tool classification and branch guarding.
• getappz/agentflare#283: Both changes derive mutating-tool guards from target-file repository paths.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly summarizes the main fix: closing branch-guard bypasses for filename, new-directory, and missing-path cases.
Description check	✅ Passed	The description covers Summary and Test plan well, but it omits the template's Notes for reviewers section.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/hook-redirect-branch-guard-bypass

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

🧹 Nitpick comments (1)

src/hook_redirect.rs (1)

163-175: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win

Redundant repo_toplevel subprocess calls beyond the first existing ancestor.

git rev-parse --show-toplevel already walks up from its start directory to find .git, so once it's called on the first existing ancestor and returns None, every further (higher) existing ancestor is guaranteed to also return None — they're strict super-ancestors already covered by that first call's internal walk. As written, find_map will keep re-invoking repo_toplevel (a new git subprocess spawn) for every subsequent existing ancestor instead of stopping after the first. For a target path with several existing directory levels outside any repo, this spawns one subprocess per level instead of one, adding latency that competes with GATING_TIMEOUT — ironically risking a fail-open skip of the very guard this PR hardens.

♻️ Only call repo_toplevel once, on the first existing ancestor

-            let target_repo = target_path.and_then(|p| {
-                p.ancestors().skip(1).find_map(|ancestor| {
-                    let check = if ancestor == Path::new("") {
-                        Path::new(".")
-                    } else {
-                        ancestor
-                    };
-                    check
-                        .exists()
-                        .then(|| flare_git_core::branch::repo_toplevel(check))
-                        .flatten()
-                })
-            });
+            let target_repo = target_path.and_then(|p| {
+                let first_existing = p.ancestors().skip(1).find(|ancestor| {
+                    let check = if *ancestor == Path::new("") {
+                        Path::new(".")
+                    } else {
+                        *ancestor
+                    };
+                    check.exists()
+                })?;
+                let check = if first_existing == Path::new("") {
+                    Path::new(".")
+                } else {
+                    first_existing
+                };
+                flare_git_core::branch::repo_toplevel(check)
+            });

🤖 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 `@src/hook_redirect.rs` around lines 163 - 175, Update the target_repo
resolution around target_path so it selects only the first existing ancestor
before calling flare_git_core::branch::repo_toplevel. Replace the repeated
find_map subprocess calls with logic that stops at that ancestor and invokes
repo_toplevel exactly once, while preserving the Path::new("") to "."
normalization and the existing Option result behavior.

🤖 Prompt for all review comments with 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.

Nitpick comments:
In `@src/hook_redirect.rs`:
- Around line 163-175: Update the target_repo resolution around target_path so
it selects only the first existing ancestor before calling
flare_git_core::branch::repo_toplevel. Replace the repeated find_map subprocess
calls with logic that stops at that ancestor and invokes repo_toplevel exactly
once, while preserving the Path::new("") to "." normalization and the existing
Option result behavior.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 2eb81b52-84c6-45e0-bbde-aeecc158e4cf

📥 Commits

Reviewing files that changed from the base of the PR and between f7aefcc and 54a80c8.

📒 Files selected for processing (1)

• src/hook_redirect.rs