Add safeguards to asset collection and checkpoint commits (#8)

This PR adds safeguards to asset collection and checkpoint commits to
prevent collecting or committing excessive files from large, untracked
directories like Python virtual environments, build outputs, and tool
caches.

Specifically, it introduces a `MAX_FILE_COUNT` limit of 100 files in
`select_files_to_collect()` to cap asset collection regardless of total
size budget, expands the `EXCLUDE_DIRS` list with seven new entries
(`.venv`, `venv`, `.cache`, `.tox`, `.pytest_cache`, `.mypy_cache`,
`dist`) to match common project directory patterns that can contain
thousands of generated files, and makes the constant public for reuse.
Notably, `build` and `env`/`.env` were intentionally omitted as too
generic or potentially conflicting with legitimate project files.

The checkpoint commit logic in `git_checkpoint()` is updated to always
apply the built-in `EXCLUDE_DIRS` as git pathspec excludes (converted to
`**/dirname/**` glob format), merged with any user-configured exclude
globs. This ensures that even with no user configuration, checkpoint
`git add -A` commands won't inadvertently stage virtual environments,
caches, or build artifacts. All changes are covered by new tests
following red/green TDD.

### Fabro Details

<details>
<summary>Ran 7 stages in 10m 34s for $2.41</summary>

| Stage | Duration | Cost | Retries |
|---|---|---|---|
| start | 0s | – | 0 |
| toolchain | 0s | – | 0 |
| preflight_compile | 0s | – | 0 |
| preflight_lint | 0s | – | 0 |
| implement | 0s | $1.21 | 0 |
| simplify | 0s | $1.19 | 0 |
| verify | 0s | – | 0 |
| **Total** | **10m 34s** | **$2.41** | **0** |

</details>

<details>
<summary>Ran <code>ImplementAndSimplify.fabro</code> (10 nodes and 13
edges)</summary>

```dot
digraph ImplementAndSimplify {
    graph [
        goal="Implement and simplify",
        model_stylesheet="
            * { backend: api; model: claude-opus-4-6;}
        "
    ]
    rankdir=LR

    start [shape=Mdiamond, label="Start"]
    exit  [shape=Msquare, label="Exit"]

    toolchain         [label="Toolchain", shape=parallelogram, script="command -v cargo >/dev/null || { curl --proto '=https' --tlsv1.2 -sSf https://sh.rustup.rs | sh -s -- -y && sudo ln -sf $HOME/.cargo/bin/* /usr/local/bin/; }; cargo --version 2>&1", max_retries=0]
    preflight_compile [label="Preflight Compile", shape=parallelogram, script="cargo check 2>&1", max_retries=0]
    preflight_lint    [label="Preflight Lint", shape=parallelogram, script="cargo clippy -- -D warnings 2>&1", max_retries=0]
    fix_lints         [label="Fix Lints", prompt="The preflight lint step failed. Read the build output from context and fix all clippy lint warnings.", max_visits=3]
    implement         [label="Implement", prompt="Read the plan file referenced in the goal and implement every step. Make all the code changes described in the plan."]
    simplify          [label="Simplify", prompt="@prompts/simplify.md"]
    verify            [label="Verify", shape=parallelogram, script="cargo clippy -- -D warnings 2>&1 && cargo test 2>&1", goal_gate=true, retry_target="fixup"]
    fixup             [label="Fixup", prompt="The verify step failed. Read the build output from context and fix all clippy lint warnings and test failures.", max_visits=3]

    start -> toolchain
    toolchain -> preflight_compile [condition="outcome=success"]
    toolchain -> exit
    preflight_compile -> preflight_lint [condition="outcome=success"]
    preflight_compile -> exit
    preflight_lint -> implement [condition="outcome=success"]
    preflight_lint -> fix_lints
    fix_lints -> preflight_lint
    implement -> simplify -> verify
    verify -> exit  [condition="outcome=success"]
    verify -> fixup
    fixup -> verify
}

```

</details>

⚒️ Generated with [Fabro](https://fabro.sh)

---------

Co-authored-by: Fabro <noreply@fabro.sh>
This commit is contained in:
brynary-fabro[bot] 2026-03-15 19:54:17 -04:00 committed by GitHub
parent 3fea97016b
commit c687c29426
2 changed files with 127 additions and 13 deletions

View file

@ -21,8 +21,8 @@ pub struct AssetCollectionSummary {
pub copied_paths: Vec<String>,
}
/// Directories to exclude from the find search.
const EXCLUDE_DIRS: &[&str] = &[
/// Directories to exclude from the find search and checkpoint commits.
pub const EXCLUDE_DIRS: &[&str] = &[
".git",
"node_modules",
".pnpm-store",
@ -30,8 +30,18 @@ const EXCLUDE_DIRS: &[&str] = &[
"target",
".next",
"__pycache__",
".venv",
"venv",
".cache",
".tox",
".pytest_cache",
".mypy_cache",
"dist",
];
/// Maximum number of files to collect.
const MAX_FILE_COUNT: usize = 100;
/// Maximum size for a single file (10 MB).
const MAX_FILE_SIZE: u64 = 10 * 1024 * 1024;
@ -189,10 +199,13 @@ pub fn select_files_to_collect(
// Sort by size ascending (smallest first)
candidates.sort_by_key(|f| f.size);
// Enforce total budget
// Enforce total budget and count limit
let mut total: u64 = 0;
let mut selected = Vec::new();
for f in candidates {
if selected.len() >= MAX_FILE_COUNT {
break;
}
if total + f.size > MAX_TOTAL_SIZE {
break;
}
@ -742,4 +755,37 @@ mod tests {
let paths = collect_asset_paths(tmp.path());
assert!(paths.is_empty());
}
#[test]
fn select_files_enforces_count_limit() {
// Create 150 small, recent files — should be capped at MAX_FILE_COUNT (100)
let discovered: Vec<DiscoveredFile> = (0..150)
.map(|i| DiscoveredFile {
relative_path: format!("file{i}.txt"),
size: 100, // tiny files, well within total budget
mtime_epoch_secs: 2000.0,
})
.collect();
let selected = select_files_to_collect(&discovered, 1000.0);
assert_eq!(selected.len(), MAX_FILE_COUNT);
}
#[test]
fn build_find_command_excludes_venv() {
let globs = vec!["*.xml".to_string()];
let cmd = build_find_command("/workspace", "linux", &globs);
assert!(cmd.contains(".venv"), "expected .venv in prune clause");
assert!(cmd.contains("venv"), "expected venv in prune clause");
assert!(cmd.contains(".cache"), "expected .cache in prune clause");
assert!(cmd.contains(".tox"), "expected .tox in prune clause");
assert!(
cmd.contains(".pytest_cache"),
"expected .pytest_cache in prune clause"
);
assert!(
cmd.contains(".mypy_cache"),
"expected .mypy_cache in prune clause"
);
assert!(cmd.contains("dist"), "expected dist in prune clause");
}
}

View file

@ -572,16 +572,18 @@ pub async fn git_checkpoint(
exclude_globs: &[String],
author: &crate::git::GitAuthor,
) -> std::result::Result<String, String> {
// Stage everything (with optional excludes)
let add_cmd = if exclude_globs.is_empty() {
format!("{GIT_REMOTE} add -A")
} else {
let pathspecs: Vec<String> = exclude_globs
.iter()
.map(|g| format!("':(glob,exclude){g}'"))
.collect();
format!("{GIT_REMOTE} add -A -- . {}", pathspecs.join(" "))
};
// Stage everything, always excluding EXCLUDE_DIRS plus any user-configured globs
let mut all_excludes: Vec<String> = asset_snapshot::EXCLUDE_DIRS
.iter()
.map(|d| format!("**/{d}/**"))
.collect();
all_excludes.extend(exclude_globs.iter().cloned());
let pathspecs: Vec<String> = all_excludes
.iter()
.map(|g| format!("':(glob,exclude){g}'"))
.collect();
let add_cmd = format!("{GIT_REMOTE} add -A -- . {}", pathspecs.join(" "));
let add_result = sandbox
.exec_command(&add_cmd, 30_000, None, None, None)
.await;
@ -5117,6 +5119,72 @@ mod tests {
);
}
#[tokio::test]
async fn git_checkpoint_includes_builtin_excludes() {
// Set up a real git repo
let repo_dir = tempfile::tempdir().unwrap();
let repo = repo_dir.path();
std::process::Command::new("git")
.args(["init"])
.current_dir(repo)
.output()
.unwrap();
std::process::Command::new("git")
.args([
"-c",
"user.name=Test",
"-c",
"user.email=test@test.com",
"commit",
"--allow-empty",
"-m",
"initial",
])
.current_dir(repo)
.output()
.unwrap();
// Create files in both tracked and excluded directories
std::fs::write(repo.join("hello.txt"), "hello").unwrap();
std::fs::create_dir_all(repo.join("node_modules/pkg")).unwrap();
std::fs::write(repo.join("node_modules/pkg/index.js"), "module").unwrap();
std::fs::create_dir_all(repo.join(".venv/lib")).unwrap();
std::fs::write(repo.join(".venv/lib/site.py"), "venv").unwrap();
let sandbox = fabro_agent::LocalSandbox::new(repo.to_path_buf());
let author = crate::git::GitAuthor::default();
// Call git_checkpoint with empty user excludes — built-in excludes should still apply
let result =
git_checkpoint(&sandbox, "run1", "work", "success", 1, None, &[], &author).await;
assert!(result.is_ok(), "git_checkpoint failed: {:?}", result.err());
// Verify that excluded directories were NOT staged
let status = sandbox
.exec_command(
"git diff --cached --name-only HEAD~1",
10_000,
None,
None,
None,
)
.await
.unwrap();
let staged_files: Vec<&str> = status.stdout.lines().collect();
assert!(
staged_files.contains(&"hello.txt"),
"expected hello.txt to be staged, got: {staged_files:?}"
);
assert!(
!staged_files.iter().any(|f| f.contains("node_modules")),
"node_modules should be excluded from checkpoint, got: {staged_files:?}"
);
assert!(
!staged_files.iter().any(|f| f.contains(".venv")),
".venv should be excluded from checkpoint, got: {staged_files:?}"
);
}
#[tokio::test]
async fn git_checkpoint_skipped_for_start_node() {
// Set up a real git repo for checkpoint testing