[PR #555] fix: move is_allowed_repository check to handle_checkpoint for correct repo resolution #563

New issue

Closed

opened 2026-03-02 04:13:59 +03:00 by kerem · 0 comments

kerem commented

2026-03-02 04:13:59 +03:00

Owner

Original Pull Request: https://github.com/git-ai-project/git-ai/pull/555

State: closed
Merged: Yes

fix: move `is_allowed_repository` into `handle_checkpoint` after repo resolution

Summary

Fixes #227. When agent hooks (e.g. Cursor) trigger checkpoints, the working directory is the agent's own directory (e.g. ~/.cursor), not the actual repository. The previous is_allowed_repository check in handle_git_ai ran against find_repository_in_path(cwd) which returns None in this case — bypassing the excluded_repositories check entirely and allowing the checkpoint to proceed.

This PR moves the is_allowed_repository check from handle_git_ai into handle_checkpoint, where it runs after agent presets resolve the actual repository working directory. This is the same number of calls (not an additional one), just in the right place:

Single-repo path: checks the resolved repo right after find_repository_in_path and exits if excluded
Multi-repo path: checks each repo in the loop, skipping excluded ones with continue

Note: the two code-path checks are in mutually exclusive branches (repo_result.is_ok() vs repo_result.is_err() / file-based detection), so only one ever fires per invocation.

Updates since last revision

Consolidated: removed the old early check in handle_git_ai entirely; the single-repo check now lives right after find_repository_in_path inside handle_checkpoint
Fixed clippy lint (collapsed nested if let + condition) and cargo fmt
Added 9 regression tests for is_allowed_repository_with_remotes covering:
- Excluded repo matched by glob
- Allowed repo not matched by exclusion glob
- Allowlist match / deny
- Exclusion takes precedence over allowlist
- No remotes with only excludes (allowed) vs with allowlist (denied)
- Empty remotes treated as no match
- Multiple remotes where one is excluded
Made is_allowed_repository_with_remotes pub(crate) for direct test access
Added TmpRepo::add_remote helper to test harness

Review & Testing Checklist for Human

Two check locations: the single-repo check (line ~537) and multi-repo loop check (line ~626) are in mutually exclusive branches — verify this is acceptable vs. a single consolidated location
Manual test: configure excluded_repositories for a repo, trigger a Cursor hook checkpoint, verify the checkpoint is now skipped
Regression test: verify normal checkpoints (cwd inside repo) still work — the check now happens slightly later, after preset execution but before any checkpoint data is written
Test coverage: the new tests use is_allowed_repository_with_remotes directly with mock remotes rather than going through the full Repository::remotes_with_urls() path — verify this is sufficient or if integration tests are needed
In the multi-repo path, repos_processed and the final log message don't account for skipped repos — confirm the output is acceptable when some repos are excluded

Notes

Tests use is_allowed_repository_with_remotes directly to avoid git remote resolution complexity in unit tests
Agent presets (Cursor, Claude, etc.) still execute before the check, but they only resolve metadata/working directory — no checkpoint data is written until after the allowed check
Devin run | Requested by @svarlamov

**Original Pull Request:** https://github.com/git-ai-project/git-ai/pull/555 **State:** closed **Merged:** Yes --- # fix: move `is_allowed_repository` into `handle_checkpoint` after repo resolution ## Summary Fixes #227. When agent hooks (e.g. Cursor) trigger checkpoints, the working directory is the agent's own directory (e.g. `~/.cursor`), not the actual repository. The previous `is_allowed_repository` check in `handle_git_ai` ran against `find_repository_in_path(cwd)` which returns `None` in this case — bypassing the `excluded_repositories` check entirely and allowing the checkpoint to proceed. This PR **moves** the `is_allowed_repository` check from `handle_git_ai` into `handle_checkpoint`, where it runs **after** agent presets resolve the actual repository working directory. This is the same number of calls (not an additional one), just in the right place: - **Single-repo path**: checks the resolved repo right after `find_repository_in_path` and exits if excluded - **Multi-repo path**: checks each repo in the loop, skipping excluded ones with `continue` Note: the two code-path checks are in mutually exclusive branches (`repo_result.is_ok()` vs `repo_result.is_err()` / file-based detection), so only one ever fires per invocation. ## Updates since last revision - Consolidated: removed the old early check in `handle_git_ai` entirely; the single-repo check now lives right after `find_repository_in_path` inside `handle_checkpoint` - Fixed clippy lint (collapsed nested `if let` + condition) and `cargo fmt` - Added **9 regression tests** for `is_allowed_repository_with_remotes` covering: - Excluded repo matched by glob - Allowed repo not matched by exclusion glob - Allowlist match / deny - Exclusion takes precedence over allowlist - No remotes with only excludes (allowed) vs with allowlist (denied) - Empty remotes treated as no match - Multiple remotes where one is excluded - Made `is_allowed_repository_with_remotes` `pub(crate)` for direct test access - Added `TmpRepo::add_remote` helper to test harness ## Review & Testing Checklist for Human - [ ] **Two check locations**: the single-repo check (line ~537) and multi-repo loop check (line ~626) are in mutually exclusive branches — verify this is acceptable vs. a single consolidated location - [ ] **Manual test**: configure `excluded_repositories` for a repo, trigger a Cursor hook checkpoint, verify the checkpoint is now skipped - [ ] **Regression test**: verify normal checkpoints (cwd inside repo) still work — the check now happens slightly later, after preset execution but before any checkpoint data is written - [ ] **Test coverage**: the new tests use `is_allowed_repository_with_remotes` directly with mock remotes rather than going through the full `Repository::remotes_with_urls()` path — verify this is sufficient or if integration tests are needed - [ ] In the multi-repo path, `repos_processed` and the final log message don't account for skipped repos — confirm the output is acceptable when some repos are excluded ### Notes - Tests use `is_allowed_repository_with_remotes` directly to avoid git remote resolution complexity in unit tests - Agent presets (Cursor, Claude, etc.) still execute before the check, but they only resolve metadata/working directory — no checkpoint data is written until after the allowed check - [Devin run](https://app.devin.ai/sessions/6e93003ca4c14559b4b183949bb95a0c) | Requested by @svarlamov