a2ui-project / a2ui-project/a2ui

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

Abierto
#2,370 4 comentarios 0 reacciones 0 asignados Ver en GitHub
P3 status: first-line-handled
Lenguaje dominante
TypeScript
Estrellas
16.4k
Forks
1.3k
Merge medio
2 d 13 h
PR fusionados (30 d)
134

Descripción

- [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.

Guía de contribución

Abrir la guía de contribución

Evaluación

Este issue todavía no se ha evaluado.

Recibe los nuevos issues en tu correo

Un resumen breve de issues de GitHub para principiantes.