fix(eval-runner): require trusted adapter configuration - #104
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (3)
WalkthroughAdapter resolution is centralized in shared helpers. Sibling adapter files are no longer discovered. Explicit paths and ChangesAdapter trust boundary
Estimated code review effort: 3 (Moderate) | ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
Greptile SummaryThis PR removes the implicit
Confidence Score: 5/5Safe to merge — the core trust-boundary fix is correct and well-tested. The security-relevant change is implemented correctly in the single canonical location in adapter.py and is guarded by regression tests. Both runners delegate cleanly to the shared module. The only new issues are a naming collision between the adapter module and the adapter local variable in each runner's main(), and a slightly misleading error message for an empty invocation list — neither affects current runtime behaviour. Files Needing Attention: The
|
| Filename | Overview |
|---|---|
| skills/bmad-eval-runner/scripts/adapter.py | New centralised adapter module — sibling-directory lookup removed, expanduser() applied to both explicit and env paths, and load_adapter now rejects an empty invocation list. Core security fix is correct. |
| skills/bmad-eval-runner/scripts/run_evals.py | Delegates adapter logic to the new module. Local variable `adapter: dict |
| skills/bmad-eval-runner/scripts/run_triggers.py | Same delegation pattern as run_evals.py, same adapter local-variable shadowing of the newly-imported module in main(). |
| skills/bmad-eval-runner/scripts/tests/test_adapter_resolution.py | Good regression coverage for the trust boundary. Both FINDERS proxy to the same adapter.find_adapter, so the loop verifies the shared impl twice rather than two independent code paths (see existing thread). |
| skills/bmad-eval-runner/SKILL.md | Step 5 updated to document the new trust-boundary rule — no sibling adapter lookup, explicit config only. |
| skills/bmad-eval-runner/references/platform-adapter.md | Discovery rule 3 (sibling adapter.json) removed; new paragraph explains the trust boundary and why the sibling lookup was eliminated. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
CLI["--adapter path"] -->|explicit Path| FA
ENV["BMAD_EVAL_ADAPTER env var"] -->|env_path| FA
BUNDLE["Sibling adapter.json (untrusted bundle)"] -->|IGNORED| FA
FA["adapter.find_adapter()"]
FA -->|Path found| LA["adapter.load_adapter() validate dict + non-empty invocation"]
FA -->|None| SKIP["Degrade to staging-only all cases skipped"]
LA -->|valid cfg| BA["adapter.build_argv() expand placeholders"]
LA -->|ValueError| SKIP
BA --> SUB["subprocess.run(argv, env=...)"]
ENV2["adapter.build_case_env() PATH + HOME + CLAUDE_CONFIG_DIR + passthrough"] --> SUB
Reviews (2): Last reviewed commit: "test(eval-runner): address adapter revie..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/adapter.py`:
- Around line 41-42: Update the invocation validation in the adapter
configuration handling to reject an empty list as well as missing or non-list
values. Ensure the existing ValueError path for the “invocation” configuration
treats a zero-length argv list as invalid, preventing it from reaching
subprocess execution.
In `@skills/bmad-eval-runner/scripts/tests/test_adapter_resolution.py`:
- Around line 25-64: Update test_sibling_adapter_is_ignored and
test_explicit_adapter_is_used_even_beside_bundle to create both adapter.json and
.bmad-eval-adapter.json sibling files. In
test_explicit_adapter_is_used_even_beside_bundle, configure BMAD_EVAL_ADAPTER
with a different valid adapter path while asserting every FINDERS implementation
still returns the explicit path.
🪄 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: bd946feb-aa20-44db-b422-844a33c46961
📒 Files selected for processing (6)
skills/bmad-eval-runner/SKILL.mdskills/bmad-eval-runner/references/platform-adapter.mdskills/bmad-eval-runner/scripts/adapter.pyskills/bmad-eval-runner/scripts/run_evals.pyskills/bmad-eval-runner/scripts/run_triggers.pyskills/bmad-eval-runner/scripts/tests/test_adapter_resolution.py
|
Addressed all four unresolved review requests in commit
Validation:
The docstring-coverage warning remains informational; no repository enforcement gate for it is configured. |
1 participant
What
Remove implicit adapter discovery from untrusted evaluation bundles.
Why
adapter.jsonbeside a cases or queries file controlled the executable argv and optional host-environment forwarding passed tosubprocess.run. A developer running a third-party bundle could therefore execute arbitrary local commands as the evaluator user. The same root cause affected both eval runners.Fixes #103
How
--adapteror operator-controlledBMAD_EVAL_ADAPTERconfiguration.Testing
python3 -m pytest -q skills/bmad-eval-runner/scripts/tests— 15 passed.git diff --checkpasses.npm testis not defined in this package.Summary by CodeRabbit
Bug Fixes
BMAD_EVAL_ADAPTERenvironment setting.Tests