feat: add agent and mcp config file path flag to eval cmd #49
Conversation
WalkthroughAdds two new CLI flags ( Changes
Sequence Diagram(s)sequenceDiagram
participant CLI
participant Loader
participant Override
participant Runner
CLI->>Loader: parse flags & load eval config
Loader-->>CLI: eval config
CLI->>Override: if --mcp-config-file set -> resolve path
CLI->>Override: if --agent-file set -> ensure Agent, set Type=file, resolve path
Override-->>CLI: apply resolved paths into eval config (or return error)
CLI->>Runner: create runner with modified config
Runner-->>CLI: runner started / error
note over Override: Path resolution may return errors (handled before runner creation)
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. 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: 0
🧹 Nitpick comments (1)
pkg/cli/run.go (1)
39-62: Override logic works correctly.The implementation properly resolves relative paths to absolute paths and applies overrides only when flags are provided. Error handling is appropriate.
Consider this minor refactor for clarity in the
overrideFilehelper:overrideFile := func(specFile *string, fileName string) error { if fileName != "" { if !filepath.IsAbs(fileName) { absPath, err := filepath.Abs(fileName) if err != nil { return err } - fileName = absPath + *specFile = absPath + } else { + *specFile = fileName } - *specFile = fileName } return nil }Note: Relative paths are resolved relative to the current working directory, not the eval config file location. This is standard
filepath.Abs()behavior but may be unexpected for users who assume paths are relative to the config file.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pkg/cli/run.go(3 hunks)
🔇 Additional comments (2)
pkg/cli/run.go (2)
22-23: LGTM!Variable declarations for the new CLI flags are clear and follow the established pattern.
100-101: LGTM!Flag declarations are clear, well-documented, and align perfectly with the PR objectives.
jrangelramos
force-pushed
the
eval-path-flags
branch
from
c90760d to
e1eb6b2
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
pkg/cli/run.go (1)
39-49: Consider simplifying the path assignment logic.The local
fileNamevariable is reassigned before being used, which works but could be clearer.Apply this diff to make the logic more direct:
overrideFile := func(specFile *string, fileName string) error { if !filepath.IsAbs(fileName) { absPath, err := filepath.Abs(fileName) if err != nil { return err } - fileName = absPath + *specFile = absPath + } else { + *specFile = fileName } - *specFile = fileName return nil }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pkg/cli/run.go(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
pkg/cli/run.go (2)
pkg/openaiagent/agent.go (1)
Agent(13-15)pkg/eval/config.go (1)
AgentRef(41-53)
🔇 Additional comments (4)
pkg/cli/run.go (4)
22-23: LGTM!The flag variable declarations are straightforward and consistent with the existing flag patterns in the function.
58-68: LGTM! Type override is working as intended.The agent override logic correctly:
- Ensures the Agent reference exists before setting fields
- Sets
Typeto"file"unconditionally, which aligns with the PR objective of overriding the config value when the flag is provided- Preserves other fields like
Modelif they were previously set
106-107: LGTM!The flag bindings are correct and the help text clearly communicates that these flags override values from the eval config.
50-56: File existence validation is properly handled downstream.The MCP config file override logic correctly resolves the path via the
overrideFilefunction. File existence validation occurs later inParseConfigFile(pkg/mcpproxy/config.go:62), which usesos.ReadFile()and returns a clear error if the file is missing or unreadable. This is the appropriate place for validation since the file is read at that point, providing accurate error handling.
|
Hey @jrangelramos this looks good to me overall, however with the addition of "builtin" agents in #38, how would you picture overriding to a built in agent? Would I need to create a agent.yaml referencing the built in agent, or would there be some way with the flag to specify that I want a "builtin.claude-code" or similar agent? IMO, we should have a way to support the second if it doesn't overcomplicate the UX of this command too much. WDYT? |
2 participants
Currently mcp config file and agent file are specified on the evals spec file. For the cases you want to test your evals against different agents or launching your MCP with different parameters (or envs) you need to create multiple evals files.
Proposed Change
Add 2 new optional flags to the
evalscommand, which will allow inform the agent and mcp config file from the command line. In case the values are speficied on the eval.yaml file, the values from command line will override.Summary by CodeRabbit
--mcp-config-fileand--agent-fileto override MCP config and specify an agent file when running evaluations.✏️ Tip: You can customize this high-level summary in your review settings.