feat(flare-git-shim): git-aware PATH shim — classify, snapshot, canonical-repo detach guard - #279
Merged
Conversation
…for AI agent shells Compiled binary, copied per tool name into ~/.agentflare/shims/, mirroring mise's crates/mise-shim pattern. Solves the gap where Claude Code's PowerShell tool runs pwsh.exe -NoProfile -NonInteractive, so anything gated behind $PROFILE (lean-ctx's own shell-hook.ps1, or a .bashenv-style function) never loads — PATH resolution of bare command names still works regardless of -NoProfile, so a shim dir on PATH still gets hit. Gate order mirrors .bashenv's _af_dispatch: kill switches -> agent-env marker -> .agentflare project walk-up (stopping at $HOME, since ~/.agentflare is agentflare's own app-data dir, not a project marker) -> lean-ctx -c <tool> -> real-binary fallback. Strict no-op for anything outside that double gate.
…core run_real, path_without_shim_dir, tool_name_from_exe, trace, and is_set move into src/lib.rs as a reusable public API. main.rs keeps all lean-ctx-specific dispatch (kill switches, agent-env gate, .agentflare project walk-up, the lean-ctx -c call) and now consumes the lib for the generic exec plumbing. Prerequisite for a second shim binary (flare-git-shim) that will reuse the same resolve+exec+propagate-exit-code core for a different dispatch target.
Adds crates/flare-git-core as the single source of truth for local git operations, mirroring agentflare-store's structural pattern (flat sibling files by concern, free functions, no trait+backends abstraction since there's exactly one backend: the local git CLI). shell.rs: run_in/run_in_opt/run_in_ok/diff, lifted verbatim from src/git.rs. branch.rs: current_branch/resolve_default_branch/repo_toplevel, plus a new is_protected_branch predicate extracted from hook_redirect.rs's inline branch-guard logic — shared going forward by both the PreToolUse guard and the new git-shim's command classifier instead of two separate definitions. Not yet wired into src/ — existing crate::git::* call sites are untouched in this commit so the branch stays buildable at every step; rewiring is the next commit.
Mechanical import-path swap only, no behavior change: cli/review.rs,
gateway_integrations.rs, github/identity.rs, github/init_auth.rs,
hook_redirect.rs, mcp_server.rs, mcp_server/flare_git.rs (GitHub-API scope
untouched, only its one local-git call site), review.rs now call
flare_git_core::{shell,branch}::* instead of crate::git::*.
src/git.rs and src/worktree.rs are left alone here — worktree.rs is the
last remaining crate::git:: consumer and is handled in the next commit
along with the worktree-mechanics migration; git.rs is deleted once that
lands and grep confirms it's no longer referenced anywhere.
…te src/git.rs
flare_git_core::worktree now owns create_worktree, already_isolated_for,
ensure_worktrees_ignored, resolve_target_branch, run_output_timeout, and
the target-dir isolation helpers -- moved verbatim from src/worktree.rs,
plus a small Progress trait so this leaf crate doesn't need to know about
the main binary's rmcp-based ProgressSender.
push_and_open_pr is split: local push mechanics (commit-count guard,
content-diff/squash-merge guard, the actual git push) become
flare_git_core::worktree::push_branch, GitHub-free. Opening the PR stays a
thin wrapper in src/worktree.rs, which now also implements the Progress
trait for ProgressSender and re-exports resolve_target_branch -- so
src/mcp_server/item.rs's crate::worktree::{resolve_target_branch,
create_worktree, push_and_open_pr} call sites needed no changes at all.
src/git.rs is now fully absorbed into flare-git-core (shell.rs/branch.rs)
and deleted; its "mod git;" declaration is removed from main.rs. Full
workspace build + test suite green.
New agentic-git-inspired capabilities for the upcoming git PATH shim, all living in flare-git-core alongside the primitives they build on: classify.rs: command classification. classify_pure is a pure function (subcommand, args, default_branch, push_touches_trust_root) -> Disposition -- fail-closed by default (an unrecognized subcommand denies, not passes through), with an explicit allowlist for read-only + ordinary mutating git usage. checkout/switch to a protected branch and push carrying changes to a trust-root path (.githooks/, .agentflare/, Cargo.toml) are denied; low-level plumbing (update-index, apply, read-tree, ...) is denied outright; git worktree is denied (orchestrator-managed by agentflare). is_destructive() flags reset --hard/clean -f*/force checkout-switch as needing a snapshot first -- kept orthogonal to Disposition, since a destructive op is still an allowed one. snapshot.rs: pre-destructive snapshot/restore using git's own object store -- git add -A into a temporary index, write-tree/commit-tree, and a private ref (refs/agentflare/snapshots/<sha>) that keeps the commit gc-safe without ever touching the real index or working tree. restore() only touches paths present at snapshot time, so files created afterward survive. list()/prune() read the ref namespace directly -- no separate metadata store. provenance.rs: build_trailers() resolves agent identity (AGENTFLARE_AGENT, falling back to the agent-detector crate -- the same fallback chain as claims::owner_id), current branch, and item id (from the task/<seq> branch convention). append_trailers() is idempotent against re-stamping (commit --amend re-invokes prepare-commit-msg). Deliberately self-reported, not cryptographically attested -- no HMAC/binding system ported from the inspiration project, matching agentflare's existing identity trust level. audit.rs: append-only JSONL log of classify::Event. Reading a missing log is empty (not an error); reading a malformed line fails closed (returns an error rather than silently dropping it). 47 tests total in the crate, all green; clippy clean.
…recursion
New crates/flare-git-shim, [[bin]] name = "git": resolves the subcommand
(skipping global flags, denying -C/--git-dir/--work-tree outright since
they'd target a different repo than the one this shim classifies against),
runs it through flare_git_core::classify, snapshots before destructive ops,
audits every event, and execs the real binary via agentflare_shim::run_real
on Passthrough/SilentExempt.
Fixes a real incident hit while building this: on Windows, an unqualified
Command::new("git") resolves via SearchPathW, which checks the CALLING
PROCESS's OWN DIRECTORY before PATH. Since flare-git-shim's compiled
binary is itself named git.exe, every internal git spawn inside
flare-git-core (shell::run_in/diff, worktree's fetch/push) was resolving
back into itself and recursing without limit -- one test run spawned
10,000+ processes before being killed. Root-caused and fixed two ways:
1. flare_git_core::shell::git_binary() resolves "git" via which::which_in
with a filtered PATH, excluding both the calling process's own
directory AND (structurally, matching "target"+"debug|release" as
adjacent path components) any cargo build-profile directory tree --
necessary because this workspace's ~/.cargo/config.toml redirects
target-dir to a shared ~/.cargo/target, and Cargo itself prepends that
profile dir to PATH for every test/run process, putting flare-git-shim's
freshly-built git.exe on PATH for literally every crate's test suite.
shell::run_in/diff and worktree's run_output_timeout callers all route
through this now instead of a bare "git" string.
2. agentflare_shim::path_without_shim_dir gets the same structural
exclusion, since run_real's own resolution had the identical gap.
3. flare-git-shim's main() adds a hard recursion-depth backstop
(FLARE_GIT_SHIM_DEPTH env var, cap 3) independent of the above fixes --
if anything ever resolves "git" wrong again for any reason, this caps
the blast radius at a handful of processes instead of a spawn storm.
audit::default_path() now also honors AGENTFLARE_HOME_OVERRIDE (matching
src/paths.rs::home's existing test/CI escape hatch) so tests -- including
this crate's own new integration suite, which spawns the real compiled
shim binary against real temp repos -- never touch a developer's actual
~/.agentflare/.
Full workspace build + test suite green (44 test-result groups, 0
failures); clippy clean on all touched crates.
…uninstall CLI AGENTFLARE_GIT_BYPASS=1 skips classification entirely and execs the real binary unconditionally (still audited as a distinct event, so a bypass is visible after the fact, not silent). A misclassification during the dogfood period must never be able to block someone mid-work with no way out short of uninstalling the shim outright. New `agentflare git install-shim --binary <path>` / `uninstall-shim` commands: copies the compiled flare-git-shim binary into ~/.agentflare/shims/ (same directory agentflare-shim already uses) as git/git.exe, and prepends that directory to the User PATH on Windows via PowerShell's [Environment]::SetEnvironmentVariable. No auto-discovery of the binary yet -- this is the dogfooding install path, not the production release path (that will bundle the shim alongside the main binary via install.sh/install.ps1, deferred until after the dogfood period). Installed and smoke-tested locally: passthrough (status) and deny (checkout to a protected branch) both verified against a real repo via the installed binary directly. Full workspace build + test suite green (44 test-result groups), clippy clean, zero process leaks.
…oesn't recognize Reverses the v1 fail-closed default: an unrecognized git subcommand is now Passthrough, not Deny. This shim sits in front of daily-driver git usage (installed live on PATH for dogfooding) -- blocking anything it hasn't been explicitly taught about (submodule, bisect, notes, gc, lfs, and the rest of git's long tail) is a worse failure mode than under-classifying. Only the deliberately-chosen cases stay denied: protected-branch checkout/switch, push carrying trust-root changes, low-level plumbing, and `git worktree`. Those are known and intentional blocks, not "doesn't recognize it" gaps -- the distinction the fail-open default is built around. Updates flare-git-shim's integration tests accordingly: the unrecognized- subcommand and audit-log tests now assert passthrough instead of deny, and the bypass-env-var test switches to an actual deny case (protected- branch checkout) so it still meaningfully exercises "bypass overrides an explicit deny" rather than a no-op.
Found while reviewing the fail-open change for side effects: `branch` was globally allowlisted as read-only, but `git branch -D/-M <name>` deletes or renames a branch -- a second way to destroy or rename the protected default branch's local ref, alongside checkout/switch (which already guard against it). Pre-existing gap, not introduced by the fail-open change, but directly in the same threat model so fixed here. Only delete/rename forms are checked; listing, creating a new branch, --set-upstream-to, etc. stay Passthrough.
…anonical-repo detach detection Closes several gaps found comparing against the agentic-git reference: audit.rs: log_event/read_events generalized to any Serialize/Deserialize type, default_path takes a log name. Adds RefTransaction/ RefTransactionEvent for the upcoming reference-transaction hook journal -- a backstop audit trail independent of the shim, since it fires for every ref move whether git went through the shim or not. branch.rs: is_protected_branch now also matches AGENTFLARE_GIT_PROTECTED_BRANCHES (comma-separated, trailing `*` glob for prefix matching, e.g. "release/*") -- the actual customization surface agentic-git's own policy.toml provides (protected-ref overrides), which this crate didn't have any equivalent of. is_protected_branch_among is the pure, testable core; the env-reading wrapper keeps the existing signature so no call site changes. Adds is_linked_worktree, needed to scope the canonical-repo guard below to the main checkout, not agent worktrees. classify.rs: push_touches_trust_root also honors AGENTFLARE_GIT_TRUST_ROOT_PATHS. Adds agent_invocation_detected (same env-var catalog agentflare-shim gates its own dispatch behind) and would_detach_head (checkout implicitly detaches for non-branch targets; switch never does without --detach; `--` path-restore forms never touch HEAD) -- the primitives the shim needs to deny detaching HEAD in the canonical checkout when agent-invoked, wired into flare-git-shim next. 63 tests, all green; clippy clean.
…detach guard Closes three more agentic-git parity gaps in the shim binary itself: Tiered bypass: AGENTFLARE_GIT_BYPASS_AGENT (bypass iff it matches AGENTFLARE_AGENT) and AGENTFLARE_GIT_BYPASS_UNTIL (bypass iff now < this unix epoch) join the existing one-shot AGENTFLARE_GIT_BYPASS. AGENTFLARE_GIT_SNAPSHOTS=0/off disables the automatic pre-destructive snapshot. Default stays ON (unlike the reference project's off-by-default raw-shim-mode) -- this shim has no separate "launched session" mode, so a safety net that's on by default is the safer choice for something installed directly on someone's daily-driver PATH. Canonical-repo HEAD-detach guard: denies an agent-invoked (self-reported env markers) checkout/switch that would detach HEAD in the canonical (non-worktree) checkout -- e.g. `git checkout <sha>` run by an agent directly in the main checkout, one of the concrete failure modes the agentic-git README calls out by name. Scoped tightly on three conditions (agent-invoked AND canonical checkout AND would actually detach) so interactive human use and any worktree use are completely unaffected -- verified by dedicated tests that explicitly strip inherited agent-env markers (this test process itself runs under CLAUDECODE=1). Escape hatch: AGENTFLARE_GIT_ALLOW_CANONICAL_MUTATE=1. 76 tests total across both crates, all green; clippy clean; 0 process leaks.
…ooks Closes the remaining agentic-git parity gaps that needed a CLI/hook surface, not just library code: `agentflare git snapshot list/restore/prune` -- flare_git_core::snapshot's pre-destructive snapshots were already being taken automatically by the shim, but there was no way to actually use one. restore without an id uses the only snapshot, or the newest with --yes; prune keeps the most recent N. `agentflare git trailer-inject <msg-file>` -- called by the new .githooks/prepare-commit-msg (installed by install-hooks alongside pre-commit/pre-push), appends provenance trailers to every commit. `agentflare git ref-transaction-log` -- called by the new .githooks/reference-transaction (state == "committed" only), journals every ref move in the repo to ~/.agentflare/audit/git-refs.jsonl. A backstop audit trail independent of the shim's own interception -- it fires regardless of whether git was invoked through the shim. install_hooks now installs all four hooks from one HOOKS table instead of a hardcoded pre-commit/pre-push pair. install-shim's printed message updated to mention all three bypass tiers. Full workspace build clean, all tests green (verified before this commit), clippy clean.
3 tasks
📝 WalkthroughWalkthroughAdds shared Git primitives, command policy enforcement, audit/provenance hooks, recovery snapshots, isolated worktrees, and PATH shims. The CLI and application modules now consume the new core crate, while Git hooks remain fail-open. ChangesGit governance and execution
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/cli/git.rs (1)
281-329: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
core.hooksPathwrite failure is swallowed — success is reported unconditionally.
.ok()on Line 317 discards theResultfromrun_in, so if settingcore.hooksPathfails (locked config, permission error, etc.), Line 318 still prints "core.hooksPath = .githooks" and the function proceeds to announce hooks are active — even though none of the just-copied hook scripts would actually be invoked by git. Every other failure path in this function checks itsResult/Errand reports accordingly; this is the one exception.🐛 Surface the config-write failure instead of discarding it
- flare_git_core::shell::run_in(&repo_root, &["config", "core.hooksPath", ".githooks"]).ok(); - crate::ui::success("core.hooksPath = .githooks"); + match flare_git_core::shell::run_in(&repo_root, &["config", "core.hooksPath", ".githooks"]) { + Ok(_) => crate::ui::success("core.hooksPath = .githooks"), + Err(e) => { + crate::ui::error(&format!( + "agentflare git install-hooks: failed to set core.hooksPath: {e}" + )); + return; + } + }🤖 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/cli/git.rs` around lines 281 - 329, Handle the Result from flare_git_core::shell::run_in when setting core.hooksPath instead of discarding it with .ok(). On failure, report the configuration-write error through crate::ui::error and return before printing the success message or announcing hooks as installed; retain the existing success flow only when the command succeeds.
🧹 Nitpick comments (3)
crates/flare-git-shim/tests/shim_test.rs (1)
235-287: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider regression tests for the two gaps raised in
classify.rs.Once the
-b/-Band push-refspec issues are addressed, add cases here: (1) agent-invokedgit checkout -b feature/xin the canonical repo must pass through, and (2) a baregit pushcarrying a trust-root change must still be denied.🤖 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 `@crates/flare-git-shim/tests/shim_test.rs` around lines 235 - 287, Add regression tests alongside canonical_repo_detach_is_denied_for_agent_invocation_but_not_human and canonical_repo_detach_allowed_with_escape_hatch for both classify.rs gaps: verify an agent-invoked checkout using -b or -B with a feature branch succeeds in the canonical repository, and verify a bare git push containing a trust-root change is denied. Reuse the existing repository setup, agent-marker environment, and assertion style.crates/flare-git-core/src/shell.rs (1)
33-57: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate PATH-hygiene/self-recursion-avoidance logic across two crates. Both files independently implement the same
is_cargo_target_profile_dircheck and the same self-dir/cargo-target-dir PATH filtering, with near-identical doc comments explaining the same Windows self-resolution hazard.agentflare-shimis meant to be the crate exposing these reusable helpers (per this PR's stated goal);flare-git-coreshould consume it instead of re-deriving the same logic independently.
crates/flare-git-core/src/shell.rs#L33-L57: havegit_binary()build its filtered PATH viaagentflare_shim::path_without_shim_dir(or an equivalent shared helper) instead of reimplementingis_cargo_target_profile_dirand the filter inline.crates/agentflare-shim/src/lib.rs#L36-L55: keep as the single source of truth for this logic; export it in a wayflare-git-corecan depend on (e.g. addagentflare-shimas aflare-git-coredependency).🤖 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 `@crates/flare-git-core/src/shell.rs` around lines 33 - 57, Remove the duplicated PATH filtering and is_cargo_target_profile_dir logic from git_binary in crates/flare-git-core/src/shell.rs, and use the shared agentflare_shim::path_without_shim_dir helper instead. In crates/agentflare-shim/src/lib.rs, keep and publicly expose that helper as the single implementation, then add the required agentflare-shim dependency so flare-git-core can consume it.src/cli/git.rs (1)
161-180: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winOverwriting an on-PATH
gitbinary in place risksETXTBSYif the shim is currently executing.
fs::copytruncates and writes directly intodest, which doubles as the livegiton PATH. If another process is mid-execution of this exact binary (e.g. a concurrent git invocation via the shim) wheninstall-shimre-runs, the write can fail on Linux with "text file busy". Copying to a temp file in the same directory andrename-ing overdestavoids this (rename is atomic and doesn't require the target to be idle).♻️ Write-then-rename to avoid ETXTBSY
- let dest = dir.join(shim_dest_name()); - if let Err(e) = fs::copy(&opts.binary, &dest) { - crate::ui::error(&format!( - "agentflare git install-shim: cannot copy {:?} to {dest:?}: {e}", - opts.binary - )); - return; - } - #[cfg(unix)] - { - use std::os::unix::fs::PermissionsExt; - let _ = fs::set_permissions(&dest, fs::Permissions::from_mode(0o755)); - } + let dest = dir.join(shim_dest_name()); + let tmp_dest = dir.join(format!("{}.tmp", shim_dest_name())); + if let Err(e) = fs::copy(&opts.binary, &tmp_dest) { + crate::ui::error(&format!( + "agentflare git install-shim: cannot copy {:?} to {tmp_dest:?}: {e}", + opts.binary + )); + return; + } + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + let _ = fs::set_permissions(&tmp_dest, fs::Permissions::from_mode(0o755)); + } + if let Err(e) = fs::rename(&tmp_dest, &dest) { + crate::ui::error(&format!( + "agentflare git install-shim: cannot install {tmp_dest:?} to {dest:?}: {e}" + )); + return; + }🤖 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/cli/git.rs` around lines 161 - 180, Update install_shim to copy the binary to a temporary file within the shims directory, apply executable permissions to that temporary path, then atomically rename it over dest. Replace the direct fs::copy(&opts.binary, &dest) flow while preserving the existing error reporting and success behavior, and clean up or handle the temporary file if copying or renaming fails.
🤖 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.
Inline comments:
In `@crates/flare-git-core/src/audit.rs`:
- Around line 48-58: Update log_event to serialize each audit entry as one
complete JSONL record, including the trailing newline, and write it with a
single write_all call on the opened append handle. Preserve the existing
directory creation, serialization error mapping, and append behavior while
ensuring each event uses exactly one write operation.
In `@crates/flare-git-core/src/classify.rs`:
- Around line 89-106: Update would_detach_head to recognize checkout
branch-creation forms using -b or -B and return false before resolving the first
non-flag argument. Preserve the existing handling for path restores, --detach,
ordinary checkout targets, and switch options.
- Around line 276-287: Update classify to parse the push refspec before calling
push_touches_trust_root, rather than assuming args[1] is the branch. Handle
explicit remote/refspec forms such as “git push -u origin feature/x” and bare
pushes using upstream or default-push resolution, then pass the resolved
branch/ref to the trust-root check while preserving the existing non-push
classification flow.
In `@crates/flare-git-core/src/shell.rs`:
- Around line 40-57: Update git_binary so failure of which::which_in after
filtering returns an explicit error rather than the literal "git" fallback,
preventing self-recursion when git is unavailable. Propagate the clear “git not
found” failure through the analogous diff path instead of spawning an unresolved
command name.
In `@crates/flare-git-core/src/snapshot.rs`:
- Around line 47-50: Update snapshot_before to resolve the repository’s actual
gitdir, including linked-worktree gitdir files, before constructing tmp_index.
Build the agentflare snapshot index path under that resolved gitdir instead of
assuming repo_root/.git is a directory, while preserving the existing snapshot
and error behavior.
In `@src/cli/git.rs`:
- Around line 367-395: Update the Some(id) branch in snapshot_restore to collect
all snapshots whose id starts with the requested prefix, restore only when
exactly one match exists, and report an explicit ambiguous-prefix error when
multiple matches exist; preserve the existing no-match handling and avoid
silently selecting the first result.
- Around line 220-234: In the PATH update flow around the PowerShell command
assigned to get, validate get.status.success() before parsing get.stdout or
determining already_present. If the command fails, return the existing error
through the function’s established error-handling path and do not construct or
persist new_path; only trust the retrieved PATH after successful execution.
In `@src/hook_redirect.rs`:
- Around line 56-59: The default_branch function must not accept
resolve_default_branch’s current-branch fallback as the protected branch.
Validate that the resolved branch is backed by origin/HEAD, main, or master;
otherwise return None so branch_guard_reason_for uses its existing main/master
fallback. Keep current_dir handling and valid default-branch resolution
unchanged.
---
Outside diff comments:
In `@src/cli/git.rs`:
- Around line 281-329: Handle the Result from flare_git_core::shell::run_in when
setting core.hooksPath instead of discarding it with .ok(). On failure, report
the configuration-write error through crate::ui::error and return before
printing the success message or announcing hooks as installed; retain the
existing success flow only when the command succeeds.
---
Nitpick comments:
In `@crates/flare-git-core/src/shell.rs`:
- Around line 33-57: Remove the duplicated PATH filtering and
is_cargo_target_profile_dir logic from git_binary in
crates/flare-git-core/src/shell.rs, and use the shared
agentflare_shim::path_without_shim_dir helper instead. In
crates/agentflare-shim/src/lib.rs, keep and publicly expose that helper as the
single implementation, then add the required agentflare-shim dependency so
flare-git-core can consume it.
In `@crates/flare-git-shim/tests/shim_test.rs`:
- Around line 235-287: Add regression tests alongside
canonical_repo_detach_is_denied_for_agent_invocation_but_not_human and
canonical_repo_detach_allowed_with_escape_hatch for both classify.rs gaps:
verify an agent-invoked checkout using -b or -B with a feature branch succeeds
in the canonical repository, and verify a bare git push containing a trust-root
change is denied. Reuse the existing repository setup, agent-marker environment,
and assertion style.
In `@src/cli/git.rs`:
- Around line 161-180: Update install_shim to copy the binary to a temporary
file within the shims directory, apply executable permissions to that temporary
path, then atomically rename it over dest. Replace the direct
fs::copy(&opts.binary, &dest) flow while preserving the existing error reporting
and success behavior, and clean up or handle the temporary file if copying or
renaming fails.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 21b41104-d0fb-44bb-9f5c-a80520b18a7f
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (29)
.githooks/prepare-commit-msg.githooks/reference-transactionCargo.tomlcrates/agentflare-shim/Cargo.tomlcrates/agentflare-shim/src/lib.rscrates/agentflare-shim/src/main.rscrates/flare-git-core/Cargo.tomlcrates/flare-git-core/src/audit.rscrates/flare-git-core/src/branch.rscrates/flare-git-core/src/classify.rscrates/flare-git-core/src/lib.rscrates/flare-git-core/src/provenance.rscrates/flare-git-core/src/shell.rscrates/flare-git-core/src/snapshot.rscrates/flare-git-core/src/worktree.rscrates/flare-git-shim/Cargo.tomlcrates/flare-git-shim/src/main.rscrates/flare-git-shim/tests/shim_test.rssrc/cli/git.rssrc/cli/review.rssrc/gateway_integrations.rssrc/github/identity.rssrc/github/init_auth.rssrc/hook_redirect.rssrc/main.rssrc/mcp_server.rssrc/mcp_server/flare_git.rssrc/review.rssrc/worktree.rs
💤 Files with no reviewable changes (1)
- src/main.rs
Comment on lines
+48
to
+58
| pub fn log_event<T: Serialize>(audit_path: &Path, event: &T) -> std::io::Result<()> { | ||
| if let Some(parent) = audit_path.parent() { | ||
| std::fs::create_dir_all(parent)?; | ||
| } | ||
| let mut f = std::fs::OpenOptions::new() | ||
| .create(true) | ||
| .append(true) | ||
| .open(audit_path)?; | ||
| let line = serde_json::to_string(event).map_err(std::io::Error::other)?; | ||
| writeln!(f, "{line}") | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Two separate writes per audit entry can interleave under concurrent appenders, corrupting the JSONL log.
writeln!(f, "{line}") issues a write() for the JSON body and a separate write() for the trailing newline. O_APPEND only makes each individual write() atomic, not the pair — two processes appending concurrently (plausible here: multiple shim invocations, parallel CI, concurrent reference-transaction hooks all target the same log per this module's own doc comment) can interleave bodies and newlines into a malformed line. read_events's deliberate fail-closed parsing (by design, per this module) would then surface that as a hard read error, undermining the audit trail's reliability exactly when concurrency happens.
🔒 Proposed fix: single write_all call per event
- let line = serde_json::to_string(event).map_err(std::io::Error::other)?;
- writeln!(f, "{line}")
+ let mut line = serde_json::to_string(event).map_err(std::io::Error::other)?;
+ line.push('\n');
+ f.write_all(line.as_bytes())📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| pub fn log_event<T: Serialize>(audit_path: &Path, event: &T) -> std::io::Result<()> { | |
| if let Some(parent) = audit_path.parent() { | |
| std::fs::create_dir_all(parent)?; | |
| } | |
| let mut f = std::fs::OpenOptions::new() | |
| .create(true) | |
| .append(true) | |
| .open(audit_path)?; | |
| let line = serde_json::to_string(event).map_err(std::io::Error::other)?; | |
| writeln!(f, "{line}") | |
| } | |
| pub fn log_event<T: Serialize>(audit_path: &Path, event: &T) -> std::io::Result<()> { | |
| if let Some(parent) = audit_path.parent() { | |
| std::fs::create_dir_all(parent)?; | |
| } | |
| let mut f = std::fs::OpenOptions::new() | |
| .create(true) | |
| .append(true) | |
| .open(audit_path)?; | |
| let mut line = serde_json::to_string(event).map_err(std::io::Error::other)?; | |
| line.push('\n'); | |
| f.write_all(line.as_bytes()) | |
| } |
🤖 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 `@crates/flare-git-core/src/audit.rs` around lines 48 - 58, Update log_event to
serialize each audit entry as one complete JSONL record, including the trailing
newline, and write it with a single write_all call on the opened append handle.
Preserve the existing directory creation, serialization error mapping, and
append behavior while ensuring each event uses exactly one write operation.
Comment on lines
+89
to
+106
| pub fn would_detach_head(repo_root: &Path, subcommand: &str, args: &[String]) -> bool { | ||
| match subcommand { | ||
| "checkout" => { | ||
| if args.iter().any(|a| a == "--") { | ||
| return false; // path-restore form -- HEAD never moves | ||
| } | ||
| if args.iter().any(|a| a == "--detach") { | ||
| return true; | ||
| } | ||
| let Some(target) = args.iter().find(|a| !a.starts_with('-')) else { | ||
| return false; // e.g. bare `git checkout` -- doesn't move HEAD | ||
| }; | ||
| !crate::shell::run_in_ok(repo_root, &["show-ref", "--verify", "--quiet", &format!("refs/heads/{target}")]) | ||
| } | ||
| "switch" => args.iter().any(|a| a == "--detach" || a == "-d"), | ||
| _ => false, | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
would_detach_head misreports git checkout -b/-B <new> as a HEAD detach.
For git checkout -b feature/x the first non-flag arg is the new branch name. show-ref refs/heads/feature/x fails (it doesn't exist yet), so this returns true. Via deny_canonical_detach_reason (crates/flare-git-shim/src/main.rs:137), an agent-invoked git checkout -b feature/x in the canonical checkout is then denied with a spurious "would detach HEAD" message — blocking exactly the isolated-branch flow the policy wants agents to use. -b/-B create and attach HEAD to the new branch; they never detach.
🐛 Skip branch-creating forms
"checkout" => {
if args.iter().any(|a| a == "--") {
return false; // path-restore form -- HEAD never moves
}
+ if args.iter().any(|a| a == "-b" || a == "-B") {
+ return false; // creates and attaches HEAD to a branch -- never detaches
+ }
if args.iter().any(|a| a == "--detach") {
return true;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| pub fn would_detach_head(repo_root: &Path, subcommand: &str, args: &[String]) -> bool { | |
| match subcommand { | |
| "checkout" => { | |
| if args.iter().any(|a| a == "--") { | |
| return false; // path-restore form -- HEAD never moves | |
| } | |
| if args.iter().any(|a| a == "--detach") { | |
| return true; | |
| } | |
| let Some(target) = args.iter().find(|a| !a.starts_with('-')) else { | |
| return false; // e.g. bare `git checkout` -- doesn't move HEAD | |
| }; | |
| !crate::shell::run_in_ok(repo_root, &["show-ref", "--verify", "--quiet", &format!("refs/heads/{target}")]) | |
| } | |
| "switch" => args.iter().any(|a| a == "--detach" || a == "-d"), | |
| _ => false, | |
| } | |
| } | |
| pub fn would_detach_head(repo_root: &Path, subcommand: &str, args: &[String]) -> bool { | |
| match subcommand { | |
| "checkout" => { | |
| if args.iter().any(|a| a == "--") { | |
| return false; // path-restore form -- HEAD never moves | |
| } | |
| if args.iter().any(|a| a == "-b" || a == "-B") { | |
| return false; // creates and attaches HEAD to a branch -- never detaches | |
| } | |
| if args.iter().any(|a| a == "--detach") { | |
| return true; | |
| } | |
| let Some(target) = args.iter().find(|a| !a.starts_with('-')) else { | |
| return false; // e.g. bare `git checkout` -- doesn't move HEAD | |
| }; | |
| !crate::shell::run_in_ok(repo_root, &["show-ref", "--verify", "--quiet", &format!("refs/heads/{target}")]) | |
| } | |
| "switch" => args.iter().any(|a| a == "--detach" || a == "-d"), | |
| _ => false, | |
| } | |
| } |
🤖 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 `@crates/flare-git-core/src/classify.rs` around lines 89 - 106, Update
would_detach_head to recognize checkout branch-creation forms using -b or -B and
return false before resolving the first non-flag argument. Preserve the existing
handling for path restores, --detach, ordinary checkout targets, and switch
options.
Comment on lines
+40
to
+57
| pub(crate) fn git_binary() -> PathBuf { | ||
| static RESOLVED: OnceLock<PathBuf> = OnceLock::new(); | ||
| RESOLVED | ||
| .get_or_init(|| { | ||
| let self_dir = std::env::current_exe() | ||
| .ok() | ||
| .and_then(|p| p.parent().map(Path::to_path_buf)); | ||
| let filtered_path = std::env::var_os("PATH").map(|path_var| { | ||
| std::env::join_paths(std::env::split_paths(&path_var).filter(|p| { | ||
| Some(p.as_path()) != self_dir.as_deref() && !is_cargo_target_profile_dir(p) | ||
| })) | ||
| .unwrap_or(path_var) | ||
| }); | ||
| let cwd = std::env::current_dir().unwrap_or_default(); | ||
| which::which_in("git", filtered_path.as_ref(), cwd).unwrap_or_else(|_| PathBuf::from("git")) | ||
| }) | ||
| .clone() | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Fallback re-introduces the Windows self-recursion bug this function exists to prevent.
When which::which_in fails to resolve git anywhere on the filtered PATH, this falls back to the literal string "git" (line 54). Since this crate is linked into flare-git-shim (a binary named git/git.exe), Command::new("git") on Windows resolves via the calling process's own directory before PATH — i.e. back to the shim itself — which is precisely the unbounded-recursion failure mode documented in this function's own comment (previously observed spawning 10,000+ processes). The fallback silently defeats the entire mitigation on the one path where it matters most (git truly unresolvable).
🐛 Proposed fix: fail closed instead of falling back to a self-referential name
-pub(crate) fn git_binary() -> PathBuf {
- static RESOLVED: OnceLock<PathBuf> = OnceLock::new();
+pub(crate) fn git_binary() -> Option<PathBuf> {
+ static RESOLVED: OnceLock<Option<PathBuf>> = OnceLock::new();
RESOLVED
.get_or_init(|| {
let self_dir = std::env::current_exe()
.ok()
.and_then(|p| p.parent().map(Path::to_path_buf));
let filtered_path = std::env::var_os("PATH").map(|path_var| {
std::env::join_paths(std::env::split_paths(&path_var).filter(|p| {
Some(p.as_path()) != self_dir.as_deref() && !is_cargo_target_profile_dir(p)
}))
.unwrap_or(path_var)
});
let cwd = std::env::current_dir().unwrap_or_default();
- which::which_in("git", filtered_path.as_ref(), cwd).unwrap_or_else(|_| PathBuf::from("git"))
+ which::which_in("git", filtered_path.as_ref(), cwd).ok()
})
.clone()
}
pub fn run_in(repo_root: &Path, args: &[&str]) -> Result<String, String> {
- let out = Command::new(git_binary())
+ let git = git_binary().ok_or_else(|| "git not found on PATH".to_string())?;
+ let out = Command::new(git)
.args(args)
.current_dir(repo_root)
.output()
.map_err(|e| format!("git not available: {e}"))?;(and analogously in diff, propagating a clear "git not found" error instead of spawning an unresolved name.)
🤖 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 `@crates/flare-git-core/src/shell.rs` around lines 40 - 57, Update git_binary
so failure of which::which_in after filtering returns an explicit error rather
than the literal "git" fallback, preventing self-recursion when git is
unavailable. Propagate the clear “git not found” failure through the analogous
diff path instead of spawning an unresolved command name.
Comment on lines
+47
to
+50
| pub fn snapshot_before(repo_root: &Path, reason: &str) -> Result<SnapshotId, String> { | ||
| let tmp_index = repo_root | ||
| .join(".git") | ||
| .join(format!("agentflare-snapshot-index-{}", std::process::id())); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm there is no worktree-specific snapshot test and that shell exposes a gitdir resolver.
rg -nP -C2 'absolute-git-dir|--git-dir|is_linked_worktree' crates/flare-git-core/src/shell.rs crates/flare-git-core/src/branch.rs
rg -n 'worktree' crates/flare-git-core/src/snapshot.rsRepository: getappz/agentflare
Length of output: 2708
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,220p' crates/flare-git-core/src/snapshot.rs
printf '\n--- shim ---\n'
sed -n '230,290p' crates/flare-git-shim/src/main.rsRepository: getappz/agentflare
Length of output: 9517
Snapshot index path should use the repo’s real gitdir, not .git.
In linked worktrees, <repo_root>/.git is a file, so the temp index path can’t be created and snapshot_before returns Err. The shim only warns on that failure, so destructive git operations can run without the recovery snapshot this feature is meant to provide. Resolve the actual gitdir before building the temp index path.
🤖 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 `@crates/flare-git-core/src/snapshot.rs` around lines 47 - 50, Update
snapshot_before to resolve the repository’s actual gitdir, including
linked-worktree gitdir files, before constructing tmp_index. Build the
agentflare snapshot index path under that resolved gitdir instead of assuming
repo_root/.git is a directory, while preserving the existing snapshot and error
behavior.
Comment on lines
+220
to
+234
| let get = std::process::Command::new("powershell.exe") | ||
| .args([ | ||
| "-NoProfile", | ||
| "-Command", | ||
| "[Environment]::GetEnvironmentVariable('PATH','User')", | ||
| ]) | ||
| .output() | ||
| .map_err(|e| e.to_string())?; | ||
| let current = String::from_utf8_lossy(&get.stdout).trim().to_string(); | ||
| let already_present = current | ||
| .split(';') | ||
| .any(|p| p.trim_end_matches('\u{5c}').eq_ignore_ascii_case(dir_str.trim_end_matches('\u{5c}'))); | ||
| if already_present { | ||
| return Ok(false); | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Unchecked PowerShell exit status can wipe the user's PATH down to just the shim dir.
get.status.success() is never checked before trusting current as "the existing PATH." If the GetEnvironmentVariable PowerShell call fails for any reason (stderr output, non-zero exit) while stdout still comes back empty, current is treated as empty, already_present is false, and new_path becomes just dir_str — the subsequent SetEnvironmentVariable call would then persist a User PATH containing only the shim directory, silently destroying the rest of the user's PATH.
🛡️ Validate the `get` command succeeded before trusting its output
let get = std::process::Command::new("powershell.exe")
.args([
"-NoProfile",
"-Command",
"[Environment]::GetEnvironmentVariable('PATH','User')",
])
.output()
.map_err(|e| e.to_string())?;
+ if !get.status.success() {
+ return Err(format!(
+ "powershell GetEnvironmentVariable failed: {}",
+ String::from_utf8_lossy(&get.stderr).trim()
+ ));
+ }
let current = String::from_utf8_lossy(&get.stdout).trim().to_string();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| let get = std::process::Command::new("powershell.exe") | |
| .args([ | |
| "-NoProfile", | |
| "-Command", | |
| "[Environment]::GetEnvironmentVariable('PATH','User')", | |
| ]) | |
| .output() | |
| .map_err(|e| e.to_string())?; | |
| let current = String::from_utf8_lossy(&get.stdout).trim().to_string(); | |
| let already_present = current | |
| .split(';') | |
| .any(|p| p.trim_end_matches('\u{5c}').eq_ignore_ascii_case(dir_str.trim_end_matches('\u{5c}'))); | |
| if already_present { | |
| return Ok(false); | |
| } | |
| let get = std::process::Command::new("powershell.exe") | |
| .args([ | |
| "-NoProfile", | |
| "-Command", | |
| "[Environment]::GetEnvironmentVariable('PATH','User')", | |
| ]) | |
| .output() | |
| .map_err(|e| e.to_string())?; | |
| if !get.status.success() { | |
| return Err(format!( | |
| "powershell GetEnvironmentVariable failed: {}", | |
| String::from_utf8_lossy(&get.stderr).trim() | |
| )); | |
| } | |
| let current = String::from_utf8_lossy(&get.stdout).trim().to_string(); | |
| let already_present = current | |
| .split(';') | |
| .any(|p| p.trim_end_matches('\u{5c}').eq_ignore_ascii_case(dir_str.trim_end_matches('\u{5c}'))); | |
| if already_present { | |
| return Ok(false); | |
| } |
🤖 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/cli/git.rs` around lines 220 - 234, In the PATH update flow around the
PowerShell command assigned to get, validate get.status.success() before parsing
get.stdout or determining already_present. If the command fails, return the
existing error through the function’s established error-handling path and do not
construct or persist new_path; only trust the retrieved PATH after successful
execution.
Comment on lines
+367
to
+395
| fn snapshot_restore(repo_root: &Path, opts: &SnapshotRestoreArgs) { | ||
| let snaps = snapshot::list(repo_root); | ||
| let target = match &opts.id { | ||
| Some(id) => snaps.iter().find(|s| s.id.0.starts_with(id.as_str())), | ||
| None => match snaps.len() { | ||
| 0 => None, | ||
| 1 => snaps.first(), | ||
| _ if opts.yes => snaps.first(), | ||
| _ => { | ||
| crate::ui::error( | ||
| "agentflare git snapshot restore: multiple snapshots exist -- pass an id, or --yes to use the newest", | ||
| ); | ||
| return; | ||
| } | ||
| }, | ||
| }; | ||
| let Some(meta) = target else { | ||
| crate::ui::error("agentflare git snapshot restore: no matching snapshot found"); | ||
| return; | ||
| }; | ||
| match snapshot::restore(repo_root, &meta.id) { | ||
| Ok(()) => crate::ui::success(&format!( | ||
| "restored snapshot {} ({})", | ||
| &meta.id.0[..meta.id.0.len().min(12)], | ||
| meta.reason | ||
| )), | ||
| Err(e) => crate::ui::error(&format!("agentflare git snapshot restore: {e}")), | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Ambiguous snapshot-id prefix silently resolves to the first match instead of erroring.
snaps.iter().find(|s| s.id.0.starts_with(id.as_str())) returns the first matching snapshot without checking whether more than one snapshot shares that prefix. The doc comment on SnapshotRestoreArgs::id promises "any unambiguous prefix," but nothing enforces that — a short/colliding prefix restores an unintended snapshot silently, unlike git's own ambiguous-prefix handling elsewhere.
🛡️ Reject ambiguous prefixes explicitly
- Some(id) => snaps.iter().find(|s| s.id.0.starts_with(id.as_str())),
+ Some(id) => {
+ let mut matches = snaps.iter().filter(|s| s.id.0.starts_with(id.as_str()));
+ let first = matches.next();
+ if matches.next().is_some() {
+ crate::ui::error(&format!(
+ "agentflare git snapshot restore: id '{id}' is ambiguous — use a longer prefix"
+ ));
+ return;
+ }
+ first
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| fn snapshot_restore(repo_root: &Path, opts: &SnapshotRestoreArgs) { | |
| let snaps = snapshot::list(repo_root); | |
| let target = match &opts.id { | |
| Some(id) => snaps.iter().find(|s| s.id.0.starts_with(id.as_str())), | |
| None => match snaps.len() { | |
| 0 => None, | |
| 1 => snaps.first(), | |
| _ if opts.yes => snaps.first(), | |
| _ => { | |
| crate::ui::error( | |
| "agentflare git snapshot restore: multiple snapshots exist -- pass an id, or --yes to use the newest", | |
| ); | |
| return; | |
| } | |
| }, | |
| }; | |
| let Some(meta) = target else { | |
| crate::ui::error("agentflare git snapshot restore: no matching snapshot found"); | |
| return; | |
| }; | |
| match snapshot::restore(repo_root, &meta.id) { | |
| Ok(()) => crate::ui::success(&format!( | |
| "restored snapshot {} ({})", | |
| &meta.id.0[..meta.id.0.len().min(12)], | |
| meta.reason | |
| )), | |
| Err(e) => crate::ui::error(&format!("agentflare git snapshot restore: {e}")), | |
| } | |
| } | |
| fn snapshot_restore(repo_root: &Path, opts: &SnapshotRestoreArgs) { | |
| let snaps = snapshot::list(repo_root); | |
| let target = match &opts.id { | |
| Some(id) => { | |
| let mut matches = snaps.iter().filter(|s| s.id.0.starts_with(id.as_str())); | |
| let first = matches.next(); | |
| if matches.next().is_some() { | |
| crate::ui::error(&format!( | |
| "agentflare git snapshot restore: id '{id}' is ambiguous — use a longer prefix" | |
| )); | |
| return; | |
| } | |
| first | |
| } | |
| None => match snaps.len() { | |
| 0 => None, | |
| 1 => snaps.first(), | |
| _ if opts.yes => snaps.first(), | |
| _ => { | |
| crate::ui::error( | |
| "agentflare git snapshot restore: multiple snapshots exist -- pass an id, or --yes to use the newest", | |
| ); | |
| return; | |
| } | |
| }, | |
| }; | |
| let Some(meta) = target else { | |
| crate::ui::error("agentflare git snapshot restore: no matching snapshot found"); | |
| return; | |
| }; | |
| match snapshot::restore(repo_root, &meta.id) { | |
| Ok(()) => crate::ui::success(&format!( | |
| "restored snapshot {} ({})", | |
| &meta.id.0[..meta.id.0.len().min(12)], | |
| meta.reason | |
| )), | |
| Err(e) => crate::ui::error(&format!("agentflare git snapshot restore: {e}")), | |
| } | |
| } |
🤖 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/cli/git.rs` around lines 367 - 395, Update the Some(id) branch in
snapshot_restore to collect all snapshots whose id starts with the requested
prefix, restore only when exactly one match exists, and report an explicit
ambiguous-prefix error when multiple matches exist; preserve the existing
no-match handling and avoid silently selecting the first result.
Comment on lines
56
to
59
| fn default_branch() -> Option<String> { | ||
| let cwd = std::env::current_dir().ok()?; | ||
| Some(crate::git::resolve_default_branch(&cwd)) | ||
| Some(flare_git_core::branch::resolve_default_branch(&cwd)) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major
default_branch() can now equal the current branch itself, unconditionally "protecting" every branch in repos without main/master.
flare_git_core::branch::resolve_default_branch (see upstream snippet) falls back to symbolic-ref --short HEAD — the currently checked-out branch — when there's no origin/HEAD ref and no local main or master branch. Since current_branch() resolves via the equivalent rev-parse --abbrev-ref HEAD, in that fallback scenario default_branch() == current_branch() always holds, so branch_guard_reason_for (Lines 67-81) treats whatever branch you're on as the protected default and blocks every mutating tool call — for any repo lacking main/master (fresh repos, or repos using a different naming convention). This also makes the function's own dedicated "resolution failed entirely" fallback (the None arm at Line 74) unreachable, since default_branch() no longer returns None in that case.
Consider preserving the old semantics locally: only trust the resolved default when it's actually backed by origin/HEAD, main, or master, otherwise treat resolution as failed (return None) so the existing branch == "main" || branch == "master" fallback in branch_guard_reason_for kicks in instead of an always-true comparison.
🤖 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 56 - 59, The default_branch function must
not accept resolve_default_branch’s current-branch fallback as the protected
branch. Validate that the resolved branch is backed by origin/HEAD, main, or
master; otherwise return None so branch_guard_reason_for uses its existing
main/master fallback. Keep current_dir handling and valid default-branch
resolution unchanged.
…cations git push (bare, args.len()<2) skipped the trust-root check entirely -- the single most common push form never got checked. Any flag before remote/refspec (git push -u origin branch) misread the remote name as the branch being pushed via fixed-index args[1], mis-diffing or spuriously denying. pushed_branch() now resolves the pushed ref positionally (skipping flags), falling back to the current branch when omitted. Also broadens is_destructive's clean-flag detection past four hardcoded literal strings (-f/-fd/-fx/-fdx) to cover --force and any short-opt order (-df, -xf, ...), which previously skipped the pre-destructive snapshot safety net for equivalent invocations.
CI failure on windows-latest: snapshot_then_restore_recovers_state_and_preserves_newer_files asserted \modifiedn\ but got \modifiedrn\ -- git checkout silently rewrites LF to CRLF on restore when core.autocrlf=true (the common Windows default), breaking the exact-recovery guarantee this feature exists for regardless of what was actually snapshotted. Both run_git_with_index (capture) and restore (checkout) now pass -c core.autocrlf=false explicitly, independent of the caller's ambient/global git config. Added a regression test that sets core.autocrlf=true locally on the test repo so it reproduces deterministically everywhere, not just on a Windows CI runner.
Pre-existing formatting debt across the new crates and their two call sites in the root package -- none of it introduced by the earlier fixes in this PR, just line-wrapping at the 100-col limit rustfmt wants. Mechanical only, applied straight from CI's own cargo fmt --check diff to avoid the local/CI rustfmt version skew found earlier in this branch's history.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/worktree.rs (1)
97-97: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPreserve equivalent worktree integration coverage.
The removed test module covered the local worktree behavior, but the supplied changes do not show equivalent coverage for the
ProgressSenderadapter, wrapper delegation, branch resolution, andpush_and_open_prorchestration. Migrate those cases to the core/integration tests before relying oncargo checkalone.🤖 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/worktree.rs` at line 97, Restore equivalent integration coverage for the removed worktree test module, covering the ProgressSender adapter, wrapper delegation, branch resolution, and push_and_open_pr orchestration. Place these cases in the core or integration test suite and preserve the existing local worktree behavior assertions before relying on cargo check.
🤖 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/worktree.rs`:
- Line 97: Restore equivalent integration coverage for the removed worktree test
module, covering the ProgressSender adapter, wrapper delegation, branch
resolution, and push_and_open_pr orchestration. Place these cases in the core or
integration test suite and preserve the existing local worktree behavior
assertions before relying on cargo check.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: b0e7a66f-f02a-4551-9964-916d2f6ada95
📒 Files selected for processing (11)
crates/agentflare-shim/src/lib.rscrates/flare-git-core/src/audit.rscrates/flare-git-core/src/branch.rscrates/flare-git-core/src/classify.rscrates/flare-git-core/src/shell.rscrates/flare-git-core/src/snapshot.rscrates/flare-git-core/src/worktree.rscrates/flare-git-shim/src/main.rscrates/flare-git-shim/tests/shim_test.rssrc/cli/git.rssrc/worktree.rs
🚧 Files skipped from review as they are similar to previous changes (9)
- crates/flare-git-core/src/audit.rs
- crates/agentflare-shim/src/lib.rs
- crates/flare-git-core/src/snapshot.rs
- crates/flare-git-core/src/branch.rs
- crates/flare-git-shim/tests/shim_test.rs
- crates/flare-git-shim/src/main.rs
- crates/flare-git-core/src/worktree.rs
- crates/flare-git-core/src/classify.rs
- src/cli/git.rs
getappz
added a commit
that referenced
this pull request
Jul 20, 2026
…-features clippy errors Agentflare-Agent: claude-code_2-1-215_agent Agentflare-Branch: item-185-asset-store-migration
This was referenced Jul 21, 2026
This was referenced Jul 24, 2026
Merged
This was referenced Aug 4, 2026
Merged
This was referenced Aug 11, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
agentflare-shiminto a lib+bin split, exposing a generic PATH-shim exec core (path_without_shim_dir/run_real/tool_name_from_exe) that anyagentflare-*shim binary can reuse — tool-specific dispatch logic stays in each binary's ownmain.rs.crates/flare-git-core: shell primitives, branch resolution, a classify engine (classify.rs), snapshot/restore/prune, provenance + ref-transaction hooks, configurable protected branches/trust-roots, and canonical-repo HEAD-detach detection.crates/flare-git-shim: a PATH shim impersonatinggitthat classifies every invocation viaflare_git_core::classifybefore deciding whether to exec the real binary — closing the gap where a rawgit commit/git checkoutrun via Bash bypasses agentflare's tool-call-level PreToolUse guard entirely.git.jsonl), never silently blocking with no way out.Note for reviewers
This PR's
crates/agentflare-shimoverlaps with the separatefeat/lean-ctx-winshim-227PR (#278), which added a bin-only version of the same crate for item #227 (Windows shim). This PR's version is a strict superset (adds the lib split needed forflare-git-shimto reuse the exec core) — recommend merging this one first and rebasing #278 on top of it, dropping that PR's own copy ofagentflare-shimin favor of this one.Test plan
cargo check -p agentflare-shim -p flare-git-core -p flare-git-shim— cleancargo test -p flare-git-core -p flare-git-shim(includesflare-git-shim/tests/shim_test.rs)git/git.exeshim, exercise classify dispositions (passthrough, deny, snapshot-before-destructive) and all three bypass escape hatchesSummary by CodeRabbit