fix(eval-runner): harden fixture isolation with parent-held digests - #101
fix(eval-runner): harden fixture isolation with parent-held digests#101Haven2026 wants to merge 3 commits into
Conversation
Split fixture containment into fixture_integrity, re-validate evidence with an adapter-unreachable digest, exclusive run dirs, and fail-closed CLI exits. chmod is no longer treated as the immutability boundary.
WalkthroughThe eval runner adds trusted fixture resolution, staging verification, immutable evidence snapshots, manifest validation, and fixture digests. It rejects unsafe paths and mutations, records explicit integrity failures, prevents run-directory collisions, and documents trusted evidence usage for grading. ChangesFixture integrity pipeline
Estimated code review effort: 4 (Complex) | ~45 minutes Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
skills/bmad-eval-runner/scripts/run_evals.py (1)
588-603: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
resolve_fixturesruns outside the try/except it's meant to be caught by — a bad fixture path crashes the whole run.
resolve_fixtures(...)is invoked in the main thread as an argument inside thepool.submit(...)dict comprehension (lines 589-593), not inside the callable executed by the worker. AnyFixtureErrorit raises (absolute path,..traversal, symlink escape, missing fixture, duplicate destination) propagates straight out ofmain(), never reaching the newexcept FixtureError/except Exceptionhandlers at lines 600-606. The result: one case with an unsafe"files"entry aborts the entire run — noexecution-summary.jsonis written and every other case's already-computed result is discarded — instead of surfacing a per-casefixture-integrity-erroras the rest of this PR is designed to guarantee. No test intest_fixture_integrity.pyexercises this path (all adversarial-path tests callresolve_fixturesdirectly rather than throughmain()).🐛 Proposed fix — resolve fixtures inside the submitted callable
+ def _run_job(c, case_dir, adapter, timeout, config_name, config_skill): + fixtures = resolve_fixtures(c.get("files", []), project_root, + cases_file.parent) + return run_case(c, case_dir, run_dir, adapter, timeout, config_name, + config_skill, fixtures) + with ThreadPoolExecutor(max_workers=max(1, args.workers)) as pool: fut_to_case = { - pool.submit(run_case, c, case_dir, run_dir, adapter, - int(c.get("timeout", args.timeout)), config_name, - config_skill, - resolve_fixtures(c.get("files", []), project_root, - cases_file.parent)): c + pool.submit(_run_job, c, case_dir, adapter, + int(c.get("timeout", args.timeout)), config_name, + config_skill): c for config_name, c, case_dir, config_skill in jobs } for fut in as_completed(fut_to_case):🤖 Prompt for 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. In `@skills/bmad-eval-runner/scripts/run_evals.py` around lines 588 - 603, Move the resolve_fixtures call out of the pool.submit argument construction and into the submitted worker flow, such as run_case or a wrapper around it, so FixtureError is raised while processing each future. Preserve the existing per-case FixtureError handling around fut.result() and ensure an invalid fixture produces a fixture-integrity-error result without aborting other cases or preventing execution-summary.json generation.
🧹 Nitpick comments (1)
skills/bmad-eval-runner/scripts/tests/test_fixture_integrity.py (1)
112-147: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd CLI-level coverage for an unsafe
filesentry in the cases file.Every adversarial-path test here calls
resolve_fixturesdirectly or mutates the source only after a successful resolve. None exercisesmain()with a case whose"files"entry is itself unsafe (absolute/../missing) — exactly the scenario affected by theresolve_fixtures/pool.submitordering issue flagged inrun_evals.py. Adding a test like the following would pin the expectedfixture-integrity-errorstatus through the full CLI path once that's fixed:def test_cli_fails_when_case_files_contain_traversal(tmp_path): project = tmp_path / "project" project.mkdir() cases = project / "cases.json" cases.write_text(json.dumps({"cases": [{ "id": "traversal", "input": "test", "rubric": [], "files": ["../outside.txt"], }]}), encoding="utf-8") skill = tmp_path / "skill" skill.mkdir() (skill / "SKILL.md").write_text("# test", encoding="utf-8") output = tmp_path / "output" exit_code = run_evals.main([ "--cases", str(cases), "--skill-path", str(skill), "--project-root", str(project), "--output-dir", str(output), "--workers", "1", "--quiet", ]) assert exit_code == 1 summary_path = next(output.glob("*/execution-summary.json")) summary = json.loads(summary_path.read_text(encoding="utf-8")) assert summary["results"][0]["status"] == "fixture-integrity-error"🤖 Prompt for 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. In `@skills/bmad-eval-runner/scripts/tests/test_fixture_integrity.py` around lines 112 - 147, Add CLI-level coverage in the test suite for an unsafe case files entry by adding a test alongside test_cli_fails_when_adapter_mutates_fixture that invokes run_evals.main with a traversal path such as "../outside.txt" and no adapter. Assert the CLI exits with 1 and the generated execution summary reports fixture-integrity-error for the case.
🤖 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 `@skills/bmad-eval-runner/scripts/fixture_integrity.py`:
- Around line 48-55: Update _contained to catch resolution failures from
path.resolve(strict=True), including broken-symlink FileNotFoundError, and
re-raise them as FixtureError while preserving the existing trusted-root escape
handling and label/path context.
---
Outside diff comments:
In `@skills/bmad-eval-runner/scripts/run_evals.py`:
- Around line 588-603: Move the resolve_fixtures call out of the pool.submit
argument construction and into the submitted worker flow, such as run_case or a
wrapper around it, so FixtureError is raised while processing each future.
Preserve the existing per-case FixtureError handling around fut.result() and
ensure an invalid fixture produces a fixture-integrity-error result without
aborting other cases or preventing execution-summary.json generation.
---
Nitpick comments:
In `@skills/bmad-eval-runner/scripts/tests/test_fixture_integrity.py`:
- Around line 112-147: Add CLI-level coverage in the test suite for an unsafe
case files entry by adding a test alongside
test_cli_fails_when_adapter_mutates_fixture that invokes run_evals.main with a
traversal path such as "../outside.txt" and no adapter. Assert the CLI exits
with 1 and the generated execution summary reports fixture-integrity-error for
the case.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 8747278e-7cf3-4892-8be7-bbc003e8977a
📒 Files selected for processing (5)
skills/bmad-eval-runner/SKILL.mdskills/bmad-eval-runner/references/eval-format.mdskills/bmad-eval-runner/scripts/fixture_integrity.pyskills/bmad-eval-runner/scripts/run_evals.pyskills/bmad-eval-runner/scripts/tests/test_fixture_integrity.py
| def _contained(path: Path, root: Path, label: str) -> Path: | ||
| root = root.resolve() | ||
| resolved = path.resolve(strict=True) | ||
| try: | ||
| resolved.relative_to(root) | ||
| except ValueError as exc: | ||
| raise FixtureError(f"{label} escapes trusted root: {path}") from exc | ||
| return resolved |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Broken-symlink source raises a bare OSError, not FixtureError.
path.resolve(strict=True) is unguarded, so a source that is a broken/dangling symlink raises FileNotFoundError instead of FixtureError. Callers that key off except FixtureError to classify fixture problems (see run_evals.py) will instead see this bubble up as a generic exception, losing the explicit fixture-integrity-error status the PR is meant to guarantee for every path/symlink hazard.
🛡️ Proposed fix
def _contained(path: Path, root: Path, label: str) -> Path:
root = root.resolve()
- resolved = path.resolve(strict=True)
+ try:
+ resolved = path.resolve(strict=True)
+ except OSError as exc:
+ raise FixtureError(f"{label} cannot be resolved: {path}") from exc
try:
resolved.relative_to(root)
except ValueError as exc:
raise FixtureError(f"{label} escapes trusted root: {path}") from exc
return resolved📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def _contained(path: Path, root: Path, label: str) -> Path: | |
| root = root.resolve() | |
| resolved = path.resolve(strict=True) | |
| try: | |
| resolved.relative_to(root) | |
| except ValueError as exc: | |
| raise FixtureError(f"{label} escapes trusted root: {path}") from exc | |
| return resolved | |
| def _contained(path: Path, root: Path, label: str) -> Path: | |
| root = root.resolve() | |
| try: | |
| resolved = path.resolve(strict=True) | |
| except OSError as exc: | |
| raise FixtureError(f"{label} cannot be resolved: {path}") from exc | |
| try: | |
| resolved.relative_to(root) | |
| except ValueError as exc: | |
| raise FixtureError(f"{label} escapes trusted root: {path}") from exc | |
| return resolved |
🤖 Prompt for 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.
In `@skills/bmad-eval-runner/scripts/fixture_integrity.py` around lines 48 - 55,
Update _contained to catch resolution failures from path.resolve(strict=True),
including broken-symlink FileNotFoundError, and re-raise them as FixtureError
while preserving the existing trusted-root escape handling and label/path
context.
1 participant
Summary
Hardens
bmad-eval-runnerfixture staging so eval evidence cannot be trusted after path escape, symlink escape, or same-UID mutation.Changes
fixture_integrity.py..sources; contain sources to trusted roots; block destination symlink escapefixture-evidence/only post-adapter; reject pre-existing evidence pathsartifact_digest, notchmod 0400(same UID can still rewrite)Test plan
python -m pytest skills/bmad-eval-runner/scripts/tests/ -q→ 23 passedContext
Addresses fixture isolation gaps tracked downstream as haven-studio/cexing#5361 (Product Experience Audit graduation blocker).
Summary by CodeRabbit
New Features
Bug Fixes
Tests