Conversation
…ojects Every deny classify.rs produces (protected-branch checkout/switch/ delete/rename, trust-root push, plumbing block, worktree) exists to protect agentflare's own orchestration. None of that rationale holds in a repo agentflare doesn't track, and the shim is installed globally on PATH, so without this gate it polices ordinary git use in every unrelated project on the machine too -- confirmed happening via a vent from a standalone appz-dev-site project with no agentflare tracking at all. Reuses agentflare-shim's existing in_scoped_project walk-up (moved from main.rs into lib.rs so flare-git-core can share it) instead of inventing a second project-detection mechanism. Also closes the same gap in flare-git-shim's canonical-repo HEAD-detach guard, which runs before classify() and wasn't covered by the classify()-level fix alone. Also scopes the worktree deny specifically to mutating subcommands -- list and prune --dry-run now pass through regardless of tracking status, since they carry no risk to agentflare's claim ledger. Agentflare-Agent: claude-code_2-1-219_agent Agentflare-Branch: task/338 Agentflare-Item: 338
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe change centralizes ChangesScoped Git policy enforcement
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Git
participant flare_git_core
participant agentflare_shim
participant flare_git_shim
Git->>flare_git_core: classify command
flare_git_core->>agentflare_shim: check repository scope
agentflare_shim-->>flare_git_core: scoped status
flare_git_core-->>Git: Deny or Passthrough
Git->>flare_git_shim: evaluate canonical detach
flare_git_shim->>agentflare_shim: check repository scope
agentflare_shim-->>flare_git_shim: scoped status
flare_git_shim-->>Git: deny reason or allow
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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: 1
🤖 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/agentflare-shim/src/lib.rs`:
- Line 34: Require the PROJECT_MARKER check to recognize only directories by
replacing exists() with is_dir() in the repository-scoping logic. Update all
marker tests in crates/agentflare-shim/src/lib.rs lines 170-200 to create
.agentflare with create_dir_all instead of write, preserving the documented
directory-marker contract.
🪄 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: 81cbb363-2fee-4778-9d6d-8e50aac48824
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
crates/agentflare-shim/src/lib.rscrates/agentflare-shim/src/main.rscrates/flare-git-core/Cargo.tomlcrates/flare-git-core/src/classify.rscrates/flare-git-shim/Cargo.tomlcrates/flare-git-shim/src/main.rs
…cked The Linux CI build caught what my earlier unit-test fixes didn't: crates/flare-git-shim/tests/shim_test.rs spawns the actual compiled shim binary against real temp repos, separate from classify.rs's own unit tests. Its init_repo() helper didn't create a .agentflare marker, so every deny-path test (protected-branch checkout, canonical detach, audit logging) started passing through instead of denying once denies became tracked-repo-gated. Also adds an end-to-end test for the actual bug this whole PR fixes: git worktree add through the real shim binary, in a git repo with no .agentflare marker at all, must succeed. Agentflare-Agent: claude-code_2-1-219_agent Agentflare-Branch: task/338 Agentflare-Item: 338
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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-shim/tests/shim_test.rs`:
- Around line 110-117: Update the worktree setup in this test to allocate a
unique, test-owned temporary parent or path instead of the fixed ../wt location.
Pass that generated path to shim, assert the worktree is created there, and keep
all resulting files and Git metadata within TempDir so repeated and parallel
runs remain isolated.
🪄 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: 509db9b9-acdf-4df5-ba8b-132f91160b1d
📒 Files selected for processing (1)
crates/flare-git-shim/tests/shim_test.rs
…ndary Windows CI caught a real bug the earlier test fixes only masked: in_scoped_project's home-boundary check used plain `==` on PathBuf, which is byte-exact and does NOT account for Windows/macOS case- insensitive filesystems. A real ambient dirs::home_dir() on a Windows CI runner didn't byte-match the walk-up's own ancestor path, so the boundary silently never triggered and the walk kept climbing past home -- exactly the false-positive class this function's own doc comment says it exists to prevent. Fix: use paths_eq (this crate's own existing case/separator-insensitive comparison, already used elsewhere for the identical class of problem) instead of `==`. Added a regression test with a case-differing home to prove it. Also makes classify() delegate to a new classify_with_home so tests can inject a synthetic home boundary instead of depending on the real ambient one to stay deterministic -- and fixes a path-collision bug in my own new flare-git-shim integration test (relative "../wt" collided with leftover state from a prior run; now an isolated absolute path). Agentflare-Agent: claude-code_2-1-219_agent Agentflare-Branch: task/338 Agentflare-Item: 338
Agentflare-Agent: claude-code_2-1-219_agent Agentflare-Branch: task/338 Agentflare-Item: 338
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
classify.rs's denials (protected-branch checkout/switch/delete/rename, trust-root push, plumbing block, worktree) now only apply in a repo agentflare actually tracks (.agentflare/marker present, walked up from repo root). Outside that, every deny downgrades toPassthroughin theclassify()I/O wrapper.agentflare-shim's existingin_scoped_projectwalk-up (moved from itsmain.rsintolib.rsso it's shared) instead of inventing a second project-detection mechanism.flare-git-shim's canonical-repo HEAD-detach guard the same way — that check runs beforeclassify()and wasn't covered by theclassify()-level fix alone.worktree's deny is further scoped to mutating subcommands only:listandprune --dry-runnow pass through regardless of tracking status (no risk to the claim ledger either way).Fixes agentflare items #338 (cross-repo scope bug) and the read-only half of #335, both under epic #337. Confirmed via a real vent:
EnterWorktree/git worktree addwere blocked in a standaloneappz-dev-siteproject with zero agentflare tracking, breaking the genericusing-git-worktreesskill there.Test plan
cargo build --workspace --all-features— cleancargo fmt --check(package-scoped) — cleancargo test --workspace --all-features— 749 passed (1 pre-existing unrelated flaky test incoaching::store, confirmed passes in isolation with--test-threads=1, not touched by this change)cargo clippy -p flare-git-core -p agentflare-shim -p flare-git-shim --all-features -- -D warnings— cleanworktree_list_is_passthrough,worktree_prune_dry_run_is_passthrough,worktree_prune_without_dry_run_is_denied,worktree_add_is_denied_in_an_agentflare_tracked_repo,worktree_add_passes_through_in_an_untracked_repo,protected_branch_checkout_passes_through_in_an_untracked_repo,protected_branch_checkout_is_still_denied_in_a_tracked_repo, plus 4 movedin_scoped_projecttests now inagentflare-shim::libSummary by CodeRabbit