Repository navigation
refactor(eval): make framework selection single-source - #3066
Conversation
Validate the run_trial.py framework argument and install it before Harbor, Pier, or the relay import. Stop injecting or reserving MAKA_EVAL_FRAMEWORK from TypeScript so argv is the only authority. Fixes apache#3036 Generated-by: Grok
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan includes up to 3 reviews per rolling hour; 0 remain after this review. 📝 WalkthroughSummaryThis PR makes the validated
DesignThis PR extends the existing The solution is the smallest coherent change. The module adds limited state to bridge argument parsing and later imports. The tests verify this import-order boundary and the selection behavior. The PR does not change frameworks, workflows, pinned versions, result semantics, or artifact semantics. Tests and validationThe PR adds coverage for:
Reported focused results:
TypeScript typechecking and Biome validation also passed. Full workspace linting, formatting, and typechecking were not run. Required-check status remains unverified without direct CI evidence. Simplification opportunitiesNo deletion is required without weakening behavior or regression coverage. The lifecycle test replaces the weaker source-text scan with an observable child-environment test. The framework, import-order, invalid-input, and relay tests provide distinct coverage. Review-relevant risksThe diff changes the trial launch environment by removing No security, licensing, release, or governance effect was identified in the current diff. The person performing the merge must review the final diff. A maintainer makes the final determination. WalkthroughThe evaluation runner now provides the single framework selection authority. Harbor and Pier relay loading uses the installed selection. The TypeScript harness no longer injects or reserves ChangesFramework selection flow
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to Framework selection now comes only from the validated run_trial.py argument, preventing conflicting configuration between components; no actionable merge-blocking risk remains after normal checks and review. Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/eval/src/__tests__/lifecycle-boundaries.test.ts (1)
1058-1061: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest the launch environment instead of source text.
Line 1059 reads an implementation file. Line 1060 fails for an unrelated comment and does not verify the spawned environment. Replace this test with a harness-boundary test that verifies
MAKA_EVAL_FRAMEWORKis absent from the launched process environment.As per path instructions, flag tests that “assert implementation details” or “do not protect observable behavior.”
Source: Path instructions
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 3f07857a-1877-4dcf-a6ed-e71caa1aef4c
📒 Files selected for processing (11)
packages/eval/harbor/eval_framework.pypackages/eval/harbor/relay_agent.pypackages/eval/harbor/run_trial.pypackages/eval/harbor/test_eval_framework.pypackages/eval/harbor/test_relay_artifacts.pypackages/eval/harbor/test_relay_contract.pypackages/eval/harbor/test_relay_lifecycle.pypackages/eval/harbor/test_run_trial_policy.pypackages/eval/package.jsonpackages/eval/src/__tests__/lifecycle-boundaries.test.tspackages/eval/src/harness-executor.ts
💤 Files with no reviewable changes (1)
- packages/eval/src/harness-executor.ts
Replace the source-text scan with a harness launch that records the child environment after the parent sets the unused selector. Generated-by: Grok
|
Replaced the source-text scan in a1c376c. The new lifecycle test launches a Harbor trial while the parent has
Generated-by: Grok |
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Reviewed against #3036.
The smaller design I wanted (pass the framework into a relay factory) is blocked by Harbor: the trial loads relay_agent:RelayAgent by import path, and the base class is chosen at import time. A process-scoped install is forced by that, not invented here. Reading sys.argv[1] from the relay would be the same kind of global, with the argv layout as a hidden contract. eval_framework.py is the explicit version of that.
install() runs before any harbor / pier / relay import on both run_trial() and main(). Production code no longer reads or injects MAKA_EVAL_FRAMEWORK. The launch test that replaced the source-text scan is the right check.
Non-blocking: test_install_selects_harbor_and_pier treats re-install in one process as part of the contract; only the test helper load_relay does that. The two function-local from eval_framework import install lines in run_trial.py can move to module scope. test_relay_contract.py sets MAKA_EVAL_FRAMEWORK and does not restore it.
Approve.
AI-assisted review: Grok 4.6 drafted the first pass. A second pass on opencode-go/deepseek-v4-flash:max ran the focused Python suites, mutated relay_agent.py back to env selection to confirm those tests fail, and checked that an unset or invalid framework cannot reach the relay import. I read the issue and diff, grepped the PR branch for MAKA_EVAL_FRAMEWORK, and checked how Harbor loads the relay. Unverified by me: I did not rerun those Python suites myself.
|
Fast path: framework selection already lived on the validated I reviewed the final diff and am merging on that basis. Human contributor of record for this review: Astro-Han. |
2 participants
Summary
Harbor or Pier was selected twice: once as the validated
run_trial.pyargv, and again viaMAKA_EVAL_FRAMEWORKforrelay_agent.py. Those contracts could disagree.run_trial.pynow validates argv and installs the selection before any framework or relay import. The relay reads that installed value. TypeScript no longer injects or reservesMAKA_EVAL_FRAMEWORK.Fixes #3036
Verification
python3.13test_eval_framework.py3/3,test_run_trial_policy.py4/4,test_relay_contract.py20/20 (harbor and pier load; env is ignored),test_relay_artifacts.py1/1,test_relay_lifecycle.py12 (2 skip GNUsetsid)npm --workspace @maka/eval run typecheck— passlifecycle-boundariesincluding the new no-MAKA_EVAL_FRAMEWORKlock — passbiome checkon the touched TypeScript — cleanChecklist
Does this PR entail a change in behavior?
run_trial.pyargument