a2ui-project / a2ui-project/a2ui

[BUG]: archive_run writes outside the repository when no repo root is detected

Aperta
#2,370 4 commenti 0 reazioni 0 assegnatari Vedi su GitHub
P3 status: first-line-handled
Lingua principale
TypeScript
Stelle
16.4k
Fork
1.3k
Merge medio
2g 13h
PR unite (30g)
134

Descrizione

- [x] I have searched the existing issues to make sure this bug has not already been reported.

## Describe the Bug

When `archive_run` cannot detect a repository root, it guesses a location by counting parent directories and writes there:

```python
base = skill_dir.parents[1] if len(skill_dir.parents) > 1 else skill_dir
history_base = base / "history"
```

`eval/iterative_format_optimizer/skills/inference-format-optimizer/scripts/utils/archiver.py:109`

Both branches write outside the repository. The two `TestArchiver` tests exercise this path, and what happens next depends only on how deep the platform's temp directory is.

The tests build their working directory with `tempfile.mkdtemp()` and patch `Path` so `skill_dir` becomes the parent of that temp directory. On Linux that is `/tmp`, which has a single parent, so the `else` branch applies and the archive lands in `/tmp/history/atom/run_NNN_...`. That is outside the directory the test later removes with `shutil.rmtree(temp_dir)`, so the tests pass while leaving files behind and run IDs accumulating across runs.

On macOS `mkdtemp()` returns something like `/var/folders/xq//T/tmpXXXX`, so `parents[1]` resolves to `/private/var/folders/xq` after `.resolve()` and the same code raises:

```
FileNotFoundError: [Errno 2] No such file or directory: '/private/var/folders/xq/history/atom'
...
PermissionError: [Errno 13] Permission denied: '/private/var/folders/xq/history'
```

So this is one cross-platform bug that happens to be silent on CI.

## Steps to Reproduce

On macOS:

1. `cd eval`
2. `uv run python -m pytest iterative_format_optimizer/skills/inference-format-optimizer/tests/test_utils.py::TestArchiver`
3. `test_archive_run` and `test_archive_run_git_patch_failure` fail, so 3 of 5 pass

On Linux the tests pass. `ls /tmp/history` afterwards shows the archives they left outside the temp directory.

## Expected Behavior

`archive_run` does not write outside the repository when it cannot find a repository root, and the tests archive inside their own temp directory on every platform.

## Environment Details

- **OS**: macOS 26.6 (build 25G72, arm64), compared against Linux CI
- **Browser/Platform**: Python 3.12
- **SDK/Package Name & Version**: a2ui-eval 0.1.0 on `main` at 1133490a
- **Protocol Version**: n/a
- **Agent Framework & LLM Model**: n/a, reproduces without model calls

## Additional Context

`sync_history.py`, which `archive_run` already calls, handles the same case without guessing. Line 81 does:

```python
workspace_root = detected_root or skill_dir
```

Anchoring the fallback the same way keeps both functions writing to the same place. Today, when detection fails, `archive_run` archives under `parents[1]/history` while the `sync_worktree_history()` call a few lines later pre-syncs under `skill_dir/eval/iterative_format_optimizer/history`, which defeats the run-ID collision avoidance the pre-sync is there for.

A third problem gets in the way first. Passing `custom_history_dir` raises before any of this matters:

```
UnboundLocalError: cannot access local variable 'detected_root' where it is not associated with a value
```

`detected_root` is only bound inside the `else` branch at line 99, but line 150 reads it whenever `log_dir` is unset or missing, which is how both tests call `archive_run`. So `custom_history_dir` cannot currently be used to redirect the tests.

I plan to send a PR with three changes:

1. Anchor the no-detected-root fallback at `skill_dir`, matching `sync_history.py`, instead of counting parents.
2. Bind `detected_root` before the `custom_history_dir` branch so that argument works.
3. Have the two tests pass an explicit `custom_history_dir`, which depends on 2.

If you would rather `archive_run` raise than fall back at all, say so and I will do that instead. I leaned against it because `SKILL.md` documents invoking these scripts directly, so a copy of the skill outside a checkout is plausible and a raise would stop it working.

One smaller thing in those tests. `@patch("utils.archiver.Path")` also sets `mock_path.resolve.return_value.parent.parent`, but the code calls `Path(__file__).resolve()`, so that attribute is never read and only `mock_path.return_value` has any effect.

Guida per i contributori

Apri la guida per i contributori

Valutazione

Questa issue non è ancora stata valutata.

Ricevi le nuove issue nella tua casella

Un breve riepilogo di issue GitHub adatte ai principianti.