feat(trg): support fixed mcp mocks with expectation guards - #153
Conversation
A harness offering a control and trg exercising it were folded into one value, so a reader of the support table could not tell which cells describe trg's actual invocations from which just describe the harness's --help. The same collapse let an argument builder ask any cell for a flag, so a control could sit in the table looking wired up while no runner ever read it. Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
Real mcp servers make eval runs nondeterministic and unsafe to replay in ci, so fixed mocks let cases assert on tool inputs without a live server. A runner that cannot honor --strict-mcp-config must skip a mocked case rather than silently run it against whatever mcp servers happen to be configured on the host. Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
PR SummaryMedium Risk Overview Eval suites declare mocks as frontmatter markdown under Cache keys gain optional Reviewed by Cursor Bugbot for commit 274d77e. Bugbot is set up for automated code reviews on this repo. Configure here. |
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (18)
📝 WalkthroughWalkthroughChangesThe evaluation harness now supports fixed MCP mocks with suite and case overrides, expectation checks, response substitutions, call logging, and a hidden stdio mock server. Runs pass materialized MCP configuration to supported runners, record violations, grade them as failures, and include mock content in cache identity. MCP mock execution
Estimated code review effort: 4 (Complex) | ~45 minutes Change: Feature Sequence Diagram(s)sequenceDiagram
participant EvalCase
participant EvalRun
participant ClaudeCode
participant MockServer
participant RunRecord
EvalRun->>EvalCase: resolve_mock_set
EvalRun->>MockServer: materialize MCP config
EvalRun->>ClaudeCode: invoke with MCP config and guard flag
ClaudeCode->>MockServer: tools/call
MockServer-->>EvalRun: response and call log
EvalRun->>RunRecord: store artifacts and mock violations
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
an expectation that cannot survive the cache key's own serialization means the guard never runs, so a violation is invisible from the very first run. a cache hit that forgets what the source run found turns a suite that already failed into one that reads as clean on reuse. Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
A retried attempt is discarded whole, so the calls it made of the mocks have to go with it. Otherwise a run that did everything asked of it is failed for a call it never made. Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
The harness resolves these paths from the workspace rather than from where the run was started, so a relative out-dir sent it looking in the wrong place and it would not start at all. Clearing the call log between attempts now empties it instead of removing it, because a run that called nothing still owes the reader an artifact saying so. Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
A file reference was documented as resolving next to the mock that writes it and resolved from the mocks root instead, so a payload placed where the docs say could not be found at all and one name could only ever mean one file for the whole suite. Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
A regex compiled at match time cannot be told apart from one that simply did not match, so a typo in a mock's `expect` reads as a skill that called the tool wrongly. The same function pinned every other path in the generated mcp config to a root and left the command it spawns to whatever `current_exe` happened to hand back, which the harness then resolves against the run's workspace rather than where trg was invoked. Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com> # Conflicts: # crates/trg/docs/reference/ai-skills-eval.md # crates/trg/schemas/report.json.schema.json # crates/trg/src/agentskills/cache.rs # crates/trg/src/agentskills/ci.rs # crates/trg/src/agentskills/grading.rs # crates/trg/src/agentskills/report.rs # crates/trg/src/agentskills/runner/capabilities.rs # crates/trg/src/agentskills/runner/claude_code.rs # crates/trg/src/agentskills/runner/mod.rs # crates/trg/src/commands/ai/skills/eval/mod.rs # crates/trg/src/commands/ai/skills/eval/run.rs
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 2 potential issues.
Reviewed by Cursor Bugbot for commit 274d77e. Configure here.
| eprintln!("Run {}: failed to resolve mcp mocks: {}", run.id, e); | ||
| run.status = "failed".to_string(); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Mock load failures get graded
High Severity
A mock that fails to resolve or materialize sets status to failed with no failure_kind and returns before the workspace exists. started() only treats known not-started kinds as unexecuted, so grading still scores the run and reads missing outputs as assertion failures. An authoring error in a mock file then looks like the skill failed the case.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 274d77e. Configure here.
| line.push('\n'); | ||
| let mut file = fs::OpenOptions::new().create(true).append(true).open(path)?; | ||
| file.write_all(line.as_bytes()) | ||
| } |
There was a problem hiding this comment.
Call log writes can interleave
Medium Severity
Every mocked server process appends to the same mock-calls.jsonl with no lock. Interleaved lines are dropped by read_mock_violations, which ignores parse failures. Concurrent tools/call traffic, especially with large inputs, can lose expect mismatches and let a run pass grading.
Additional Locations (2)
Reviewed by Cursor Bugbot for commit 274d77e. Configure here.
1 participant
Stacked on #147.
Summary by CodeRabbit