Add SkippyBench external eval pack - #865
Conversation
i386
changed the title
dcecac4 to
f7d7ece
Compare
📝 WalkthroughWalkthroughThis PR adds local ChangesStructured Output Support
Estimated code review effort: 4 (Complex) | ~60 minutes Skippy-Bench External Evals and Telemetry
Estimated code review effort: 5 (Critical) | ~110 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
779f392 to
a787530
Compare
a787530 to
beba5a7
Compare
…ternal-evals # Conflicts: # crates/skippy-server/src/frontend.rs # third_party/llama.cpp/patches/0078-Contain-chat-grammar-sampler-exceptions.patch
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (7)
crates/skippy-server/src/frontend.rs (1)
272-311: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRepeated
GuardrailPolicyconstruction acrosscompatibility_for_skippy/for_standalone_mode's enforce arm.
disabled_for_skippy,compatibility_for_skippy, and theEnforcearm offor_standalone_modeeach build a near-identicalSelf { target: OpenAiGuardrailsTarget::Skippy, policy: GuardrailPolicy { ... }.into(), compaction: None }shape. Could be consolidated into a small private helper (e.g.,fn skippy_config(policy: GuardrailPolicy) -> Self) to reduce duplication as more modes are added.🤖 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 `@crates/skippy-server/src/frontend.rs` around lines 272 - 311, There is repeated OpenAiGuardrailsConfig construction in disabled_for_skippy, compatibility_for_skippy, and the Enforce arm of for_standalone_mode; extract the shared Skippy Self shape into a small private helper such as a skippy_config constructor that takes a GuardrailPolicy and returns the configured OpenAiGuardrailsConfig, then have each mode-specific method build only its policy and delegate to that helper.crates/openai-frontend/src/lib.rs (1)
35-36: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueNew crate-root re-exports added.
RetryExhaustionModeandStreamingGuardrailModeare added to the crate-rootpub use guardrails::{...}list. As per coding guidelines,crates/*/src/lib.rs: "Minimize crate-root re-exports. Root re-exports are acceptable as temporary compatibility shims during refactors, but new code should prefer importing from the owning module directly."Since consumers (e.g.,
skippy-server) only need these two types, consider importing them fromopenai_frontend::guardrailsdirectly rather than growing the root re-export surface.🤖 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 `@crates/openai-frontend/src/lib.rs` around lines 35 - 36, The crate root re-export list in openai_frontend has been expanded with RetryExhaustionMode and StreamingGuardrailMode, which should be avoided for new code. Update the consuming code to import these types directly from openai_frontend::guardrails instead of relying on the crate-root pub use in lib.rs, and keep the root re-export surface minimal unless it is needed as a temporary compatibility shim.Source: Coding guidelines
crates/skippy-bench/src/support.rs (1)
52-63: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winDebug-binary detection is a fragile substring heuristic.
ensure_release_skippy_server_binonly rejects paths containing the literal substringtarget/debug/skippy-server(or the Windows variant). This can both:
- False-negative: miss a debug binary launched via a non-canonical relative path (e.g., cwd already inside
target/, so the path is justdebug/skippy-server), silently letting a debug build corrupt benchmark numbers.- False-positive: reject a legitimate release binary that happens to live under a directory whose name coincidentally matches that substring.
Since this guard exists specifically to protect benchmark result integrity (debug builds run much slower / with different codegen), consider checking path components rather than a raw substring match, e.g. via
Path::components()looking for adjacenttarget/debugsegments plus askippy-serverfile stem, ideally after canonicalizing the path.♻️ Proposed refactor using path components instead of substring matching
pub fn ensure_release_skippy_server_bin(path: &Path) -> Result<()> { - let path = path.to_string_lossy(); - let debug_path = path.contains("target/debug/skippy-server") - || path.contains("target\\debug\\skippy-server"); + let is_skippy_server = path + .file_stem() + .and_then(|stem| stem.to_str()) + .is_some_and(|stem| stem == "skippy-server"); + let components: Vec<_> = path.components().collect(); + let debug_path = is_skippy_server + && components.windows(2).any(|window| { + matches!(window[0].as_os_str().to_str(), Some("target")) + && matches!(window[1].as_os_str().to_str(), Some("debug")) + }); if debug_path { bail!( "SkippyBench benchmark-managed skippy-server runs require a release binary; run `just release-build` and use --stage-server-bin target/release/skippy-server" ); } Ok(()) }🤖 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 `@crates/skippy-bench/src/support.rs` around lines 52 - 63, The debug-binary check in ensure_release_skippy_server_bin is using a fragile substring match on the input Path. Replace the raw path.contains logic with a path-component based check (optionally after canonicalizing) so it only rejects actual target/debug/skippy-server binaries and does not miss relative debug paths or falsely reject unrelated release binaries. Use the ensure_release_skippy_server_bin function and its debug_path detection block as the location to update.crates/skippy-bench/README.md (1)
208-220: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDoc doesn't mention that a telemetry fetch failure fails the whole
eval runeven when the harness itself succeeded.This section explains what the telemetry block records, but doesn't call out that
eval runcurrently marksreport.success = false(and the command exits non-zero) whenever the metrics-server report can't be fetched at finalize time — independent of whether the native harness itself passed. Given how expensive some of these harnesses are (SWE-Bench Pro, MCP-Atlas can run for hours), documenting this failure mode explicitly would help operators avoid discarding a valid harness result due to a collector-side blip. See related comment onevals.rs::run_eval.🤖 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 `@crates/skippy-bench/README.md` around lines 208 - 220, Update the eval run telemetry docs in README to explicitly state that a metrics-server fetch failure during finalize causes the overall eval to fail, even if the native harness succeeded. Mention that when SkippyBench cannot fetch `/v1/runs/<run-id>/report.json` in the finalize path, `report.success` is set to false and `eval run` exits non-zero. Keep the note near the existing telemetry block description and reference the `eval run`, `metrics-server`, and `run_eval` behavior so readers can find the failure mode quickly..agents/skills/skippy-bench/SKILL.md (1)
150-156: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSame telemetry/success-coupling caveat applies here as in
evals.rs/chat_corpus.rs.This section documents that
eval run/chat-corpus"fail if the metrics report cannot be exported," which matches current behavior — but doesn't clarify that this failure applies even when the underlying benchmark/harness itself completed successfully (see the related comment onevals.rs::run_eval). Worth a follow-up doc note once/if that behavior is revisited.🤖 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 @.agents/skills/skippy-bench/SKILL.md around lines 150 - 156, Clarify the success/coupling behavior in the SKILL.md telemetry section: the `skippy-bench eval run` and `skippy-bench chat-corpus` paths in this document should state that metrics export failure causes the command to fail even if the benchmark itself completed successfully. Update the wording near the TTFT/FTTT guidance and the `metrics-server` run/export note to make this explicit, and keep the reference consistent with the behavior documented in `evals.rs` and `chat_corpus.rs`.crates/skippy-bench/src/cli.rs (1)
77-84: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
EvalListArgslacks a--packfilter, unlikeEvalSyncArgs/EvalDoctorArgs.
list_evalsalways callsselected_evals(&[], EvalPack::Core)regardless of any pack selection, sinceEvalListArgshas nopackfield. This is harmless today (only one pack,core, exists), but will silently ignore pack scoping once a second pack is added, unlikesync/doctorwhich both expose--pack.🤖 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 `@crates/skippy-bench/src/cli.rs` around lines 77 - 84, EvalListArgs is missing a pack selector, so list_evals cannot honor pack scoping like EvalSyncArgs and EvalDoctorArgs do. Add a pack field to EvalListArgs with the same CLI behavior as the other arg structs, then update list_evals to pass the chosen pack into selected_evals instead of always using EvalPack::Core; use the existing EvalPack handling in EvalSyncArgs and EvalDoctorArgs as the reference.crates/skippy-bench/src/evals.rs (1)
356-395: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFailures before/around the harness invocation skip writing
run.json, losing debug context.If
create_metrics_run(line 357) or the initialspawn()insiderun_command_with_timeout(propagated via?at line 361-367) fails,run_evalreturns early without ever reaching thefs::write(&report_path, ...)call at line 384-386. The already-computedcommand(full shell invocation),run_dir, and pending-telemetry state are lost, making post-mortem debugging of failed benchmark launches harder — especially since these are often run unattended/in CI.Consider writing a partial
run.json(withsuccess: falseand whatever context is available) before propagating these early errors.🤖 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 `@crates/skippy-bench/src/evals.rs` around lines 356 - 395, Failures in run_eval before or during the harness launch currently return early and skip writing run.json, so partial context is lost. Update run_eval to persist a partial report to report_path before propagating errors from create_metrics_run or run_command_with_timeout, marking report.success as false and including whatever command/run_dir/telemetry context is available. Keep the existing final write path intact, but ensure these early failure paths also serialize the report so debugging information is always preserved.
🤖 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 `@crates/mesh-llm-guardrails/src/structured.rs`:
- Around line 55-56: Reject malformed $defs values instead of treating them as
absent. In structured.rs, the validation entry point that calls
validate_supported_schema_inner and the recursive $defs handling inside
validate_supported_schema_inner should first detect whether "$defs" is present;
if it is present and not an object, return a validation error rather than
passing None. Keep the existing object path for valid $defs and preserve the
current $ref validation flow, but ensure a non-object $defs never silently
passes.
In `@crates/openai-frontend/src/guardrails/rescue.rs`:
- Around line 455-459: Direct structured classification in `rescue.rs` is taking
priority over tool-call-shaped JSON, which makes the synthetic structured
tool-call path unreachable. Update the flow in
`classify_direct_structured_payload` and the `InvalidStructuredPayload` handling
so JSON objects shaped like `{"tool_calls":[...]}` are detected and routed
through `raw_tool_calls_from_value` before returning a structured-payload
failure. Preserve the existing direct structured path for true schema-matching
responses, but add an early tool-call-shape check in the rescue/classification
logic to avoid stealing these payloads.
In `@crates/skippy-bench/src/chat_corpus.rs`:
- Around line 113-135: The `run_config` built in `chat_corpus.rs` is using a
`"run_id"` key that gets overwritten by `telemetry_report::create_run_body` when
`metrics_run_id` is injected, so the chat-corpus run id is lost whenever the two
ids differ. Update the `run_config` construction in this call site to use a
distinct field name like `chat_corpus_run_id`, matching the approach used in
`evals.rs::create_metrics_run`, so both ids are preserved in the posted metrics
config.
In `@crates/skippy-bench/src/evals.rs`:
- Around line 1047-1054: The `mcp_atlas_metrics` function is silently
undercounting `request_count` by filtering out CSV parse errors in
`reader.records()`. Update this logic to detect and surface malformed rows
instead of dropping them quietly: either propagate the first parse error or
count/report failed records alongside the valid count, and make sure the final
`EvalMetrics` computation reflects any parse failures rather than only
successful rows.
- Around line 356-382: The telemetry finalization block in evals::run currently
flips report.success to false when collect_telemetry fails, which incorrectly
marks an otherwise successful harness run as failed. Update the error handling
around collect_telemetry so telemetry_unavailable is recorded in
report.telemetry without changing the harness outcome stored in report.success,
or make this failure-to-fail behavior conditional on an explicit flag. Keep the
logic localized to the collect_telemetry/report.telemetry handling in
evals::run.
In `@crates/skippy-bench/src/telemetry_report.rs`:
- Around line 18-19: The telemetry report currently exposes fttt_ms as a
separate metric, but TelemetryReportBuilder is populating it from the same
ttft_values as ttft_ms, so the two fields are always identical. In
telemetry_report.rs, update the TelemetryReport/TelemetryReportBuilder flow so
fttt_ms is only set from a genuinely distinct signal if one exists, otherwise
remove the redundant field and its aggregation from the builder and any report
serialization/consumers.
---
Nitpick comments:
In @.agents/skills/skippy-bench/SKILL.md:
- Around line 150-156: Clarify the success/coupling behavior in the SKILL.md
telemetry section: the `skippy-bench eval run` and `skippy-bench chat-corpus`
paths in this document should state that metrics export failure causes the
command to fail even if the benchmark itself completed successfully. Update the
wording near the TTFT/FTTT guidance and the `metrics-server` run/export note to
make this explicit, and keep the reference consistent with the behavior
documented in `evals.rs` and `chat_corpus.rs`.
In `@crates/openai-frontend/src/lib.rs`:
- Around line 35-36: The crate root re-export list in openai_frontend has been
expanded with RetryExhaustionMode and StreamingGuardrailMode, which should be
avoided for new code. Update the consuming code to import these types directly
from openai_frontend::guardrails instead of relying on the crate-root pub use in
lib.rs, and keep the root re-export surface minimal unless it is needed as a
temporary compatibility shim.
In `@crates/skippy-bench/README.md`:
- Around line 208-220: Update the eval run telemetry docs in README to
explicitly state that a metrics-server fetch failure during finalize causes the
overall eval to fail, even if the native harness succeeded. Mention that when
SkippyBench cannot fetch `/v1/runs/<run-id>/report.json` in the finalize path,
`report.success` is set to false and `eval run` exits non-zero. Keep the note
near the existing telemetry block description and reference the `eval run`,
`metrics-server`, and `run_eval` behavior so readers can find the failure mode
quickly.
In `@crates/skippy-bench/src/cli.rs`:
- Around line 77-84: EvalListArgs is missing a pack selector, so list_evals
cannot honor pack scoping like EvalSyncArgs and EvalDoctorArgs do. Add a pack
field to EvalListArgs with the same CLI behavior as the other arg structs, then
update list_evals to pass the chosen pack into selected_evals instead of always
using EvalPack::Core; use the existing EvalPack handling in EvalSyncArgs and
EvalDoctorArgs as the reference.
In `@crates/skippy-bench/src/evals.rs`:
- Around line 356-395: Failures in run_eval before or during the harness launch
currently return early and skip writing run.json, so partial context is lost.
Update run_eval to persist a partial report to report_path before propagating
errors from create_metrics_run or run_command_with_timeout, marking
report.success as false and including whatever command/run_dir/telemetry context
is available. Keep the existing final write path intact, but ensure these early
failure paths also serialize the report so debugging information is always
preserved.
In `@crates/skippy-bench/src/support.rs`:
- Around line 52-63: The debug-binary check in ensure_release_skippy_server_bin
is using a fragile substring match on the input Path. Replace the raw
path.contains logic with a path-component based check (optionally after
canonicalizing) so it only rejects actual target/debug/skippy-server binaries
and does not miss relative debug paths or falsely reject unrelated release
binaries. Use the ensure_release_skippy_server_bin function and its debug_path
detection block as the location to update.
In `@crates/skippy-server/src/frontend.rs`:
- Around line 272-311: There is repeated OpenAiGuardrailsConfig construction in
disabled_for_skippy, compatibility_for_skippy, and the Enforce arm of
for_standalone_mode; extract the shared Skippy Self shape into a small private
helper such as a skippy_config constructor that takes a GuardrailPolicy and
returns the configured OpenAiGuardrailsConfig, then have each mode-specific
method build only its policy and delegate to that helper.
🪄 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: 04d04082-f520-4c52-925a-7bb1b70b09e1
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (23)
.agents/skills/skippy-bench/SKILL.mdcrates/mesh-llm-guardrails/src/structured.rscrates/openai-frontend/src/guardrails/mod.rscrates/openai-frontend/src/guardrails/rescue.rscrates/openai-frontend/src/guardrails/tests/response_validation.rscrates/openai-frontend/src/lib.rscrates/skippy-bench/Cargo.tomlcrates/skippy-bench/README.mdcrates/skippy-bench/corpora/bench_corpus_sources.jsoncrates/skippy-bench/src/chat_corpus.rscrates/skippy-bench/src/cli.rscrates/skippy-bench/src/distributed.rscrates/skippy-bench/src/evals.rscrates/skippy-bench/src/local_single.rscrates/skippy-bench/src/local_split.rscrates/skippy-bench/src/main.rscrates/skippy-bench/src/support.rscrates/skippy-bench/src/telemetry_report.rscrates/skippy-server/src/cli.rscrates/skippy-server/src/frontend.rscrates/skippy-server/src/frontend/request.rscrates/skippy-server/src/frontend/tests.rsscripts/skippy-ci-smoke.sh
💤 Files with no reviewable changes (2)
- crates/skippy-server/src/frontend/request.rs
- crates/skippy-bench/corpora/bench_corpus_sources.json
| let definitions = schema.get("$defs").and_then(Value::as_object); | ||
| validate_supported_schema_inner(schema, definitions, 0) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Reject non-object $defs instead of ignoring it.
Line 55 and Line 275 currently treat a present-but-malformed $defs value as absent. That lets schemas like "$defs": [] pass validation when no $ref forces lookup, even though $defs must be an object for this supported-schema subset.
Proposed fix
fn validate_supported_schema(schema: &Value) -> Result<(), UnsupportedStructuredSchema> {
- let definitions = schema.get("$defs").and_then(Value::as_object);
+ let definitions = match schema.get("$defs") {
+ Some(Value::Object(definitions)) => Some(definitions),
+ Some(_) => return Err(UnsupportedStructuredSchema),
+ None => None,
+ };
validate_supported_schema_inner(schema, definitions, 0)
}
@@
) -> Result<(), UnsupportedStructuredSchema> {
- let Some(Value::Object(local_definitions)) = object.get("$defs") else {
+ let Some(definitions_value) = object.get("$defs") else {
return Ok(());
};
+ let Value::Object(local_definitions) = definitions_value else {
+ return Err(UnsupportedStructuredSchema);
+ };
for definition in local_definitions.values() {
validate_supported_schema_inner(definition, definitions, depth + 1)?;
}Also applies to: 270-276
🤖 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 `@crates/mesh-llm-guardrails/src/structured.rs` around lines 55 - 56, Reject
malformed $defs values instead of treating them as absent. In structured.rs, the
validation entry point that calls validate_supported_schema_inner and the
recursive $defs handling inside validate_supported_schema_inner should first
detect whether "$defs" is present; if it is present and not an object, return a
validation error rather than passing None. Keep the existing object path for
valid $defs and preserve the current $ref validation flow, but ensure a
non-object $defs never silently passes.
| if let Some(classified) = | ||
| classify_direct_structured_payload(prepared, value, parser_stage, finish_reason) | ||
| { | ||
| return classified; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Don’t let direct structured classification steal tool-call-shaped payloads.
Line 455 runs before raw_tool_calls_from_value, and Line 692 returns InvalidStructuredPayload for any object that fails the response schema. That makes the existing synthetic structured tool-call branch unreachable for JSON content shaped like {"tool_calls":[...]} on structured-output requests without real tools.
Proposed fix
fn classify_direct_structured_payload(
prepared: &PreparedGuardrailRequest,
value: &Value,
parser_stage: GuardrailParserStage,
finish_reason: Option<FinishReason>,
) -> Option<ClassifiedGuardrailResponse> {
if prepared.state.request_contract.has_real_tools() {
return None;
}
let spec = prepared.state.request_contract.structured_output_spec()?;
value.as_object()?;
+ if raw_tool_calls_from_value(value).is_some_and(|tool_calls| !tool_calls.is_empty()) {
+ return None;
+ }
let valid_payload = spec.validate_payload(value).is_ok();Also applies to: 681-692
🤖 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 `@crates/openai-frontend/src/guardrails/rescue.rs` around lines 455 - 459,
Direct structured classification in `rescue.rs` is taking priority over
tool-call-shaped JSON, which makes the synthetic structured tool-call path
unreachable. Update the flow in `classify_direct_structured_payload` and the
`InvalidStructuredPayload` handling so JSON objects shaped like
`{"tool_calls":[...]}` are detected and routed through
`raw_tool_calls_from_value` before returning a structured-payload failure.
Preserve the existing direct structured path for true schema-matching responses,
but add an early tool-call-shape check in the rescue/classification logic to
avoid stealing these payloads.
| let run_id = args | ||
| .run_id | ||
| .clone() | ||
| .unwrap_or_else(generate_chat_corpus_run_id); | ||
| let metrics_run_id = args | ||
| .metrics_run_id | ||
| .clone() | ||
| .unwrap_or_else(|| run_id.clone()); | ||
| let metrics_http = args.metrics_http.trim_end_matches('/').to_string(); | ||
| let metrics_report_output = metrics_report_output_path(&args, &metrics_run_id); | ||
| let run_config = json!({ | ||
| "mode": "skippy-bench-chat-corpus", | ||
| "run_id": &run_id, | ||
| "model": &args.model, | ||
| "base_url": &args.base_url, | ||
| "stream": args.stream, | ||
| "request_count": prompts.len(), | ||
| "prompt_corpus": args.prompt_corpus.as_ref().map(|path| path.display().to_string()), | ||
| "prompt_limit": args.prompt_limit, | ||
| "max_tokens": args.max_tokens, | ||
| "concurrency_depth": args.concurrency_depth, | ||
| }); | ||
| telemetry_report::create_run(&metrics_http, &metrics_run_id, &run_config)?; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
run_config's "run_id" field is silently overwritten by metrics_run_id.
telemetry_report::create_run_body (telemetry_report.rs) always force-inserts "run_id": metrics_run_id into the posted config object, overwriting any existing "run_id" key. Here, run_config is built with "run_id": &run_id (the chat-corpus benchmark's own run id) — when --run-id and --metrics-run-id differ, the value actually stored on the metrics-server for run_id inside the config silently becomes metrics_run_id, and the original chat-corpus run id is lost from that record. This makes it harder to correlate/debug runs where the two ids diverge.
The sibling call site in evals.rs::create_metrics_run avoids this exact collision by naming its own field "eval_run_id" instead of "run_id". Consider the same fix here (e.g., rename this field to "chat_corpus_run_id").
🐛 Proposed fix — avoid the field name collision
let run_config = json!({
"mode": "skippy-bench-chat-corpus",
- "run_id": &run_id,
+ "chat_corpus_run_id": &run_id,
"model": &args.model,📝 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.
| let run_id = args | |
| .run_id | |
| .clone() | |
| .unwrap_or_else(generate_chat_corpus_run_id); | |
| let metrics_run_id = args | |
| .metrics_run_id | |
| .clone() | |
| .unwrap_or_else(|| run_id.clone()); | |
| let metrics_http = args.metrics_http.trim_end_matches('/').to_string(); | |
| let metrics_report_output = metrics_report_output_path(&args, &metrics_run_id); | |
| let run_config = json!({ | |
| "mode": "skippy-bench-chat-corpus", | |
| "run_id": &run_id, | |
| "model": &args.model, | |
| "base_url": &args.base_url, | |
| "stream": args.stream, | |
| "request_count": prompts.len(), | |
| "prompt_corpus": args.prompt_corpus.as_ref().map(|path| path.display().to_string()), | |
| "prompt_limit": args.prompt_limit, | |
| "max_tokens": args.max_tokens, | |
| "concurrency_depth": args.concurrency_depth, | |
| }); | |
| telemetry_report::create_run(&metrics_http, &metrics_run_id, &run_config)?; | |
| let run_config = json!({ | |
| "mode": "skippy-bench-chat-corpus", | |
| "chat_corpus_run_id": &run_id, | |
| "model": &args.model, | |
| "base_url": &args.base_url, | |
| "stream": args.stream, | |
| "request_count": prompts.len(), | |
| "prompt_corpus": args.prompt_corpus.as_ref().map(|path| path.display().to_string()), | |
| "prompt_limit": args.prompt_limit, | |
| "max_tokens": args.max_tokens, | |
| "concurrency_depth": args.concurrency_depth, | |
| }); | |
| telemetry_report::create_run(&metrics_http, &metrics_run_id, &run_config)?; |
🤖 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 `@crates/skippy-bench/src/chat_corpus.rs` around lines 113 - 135, The
`run_config` built in `chat_corpus.rs` is using a `"run_id"` key that gets
overwritten by `telemetry_report::create_run_body` when `metrics_run_id` is
injected, so the chat-corpus run id is lost whenever the two ids differ. Update
the `run_config` construction in this call site to use a distinct field name
like `chat_corpus_run_id`, matching the approach used in
`evals.rs::create_metrics_run`, so both ids are preserved in the posted metrics
config.
| if !args.dry_run { | ||
| create_metrics_run(&args, &run_id, &metrics_run_id)?; | ||
| let started = Instant::now(); | ||
| let stdout_path = run_dir.join("raw").join("stdout.log"); | ||
| let stderr_path = run_dir.join("raw").join("stderr.log"); | ||
| let outcome = run_command_with_timeout( | ||
| &command, | ||
| args.harness_timeout_secs.map(Duration::from_secs), | ||
| &stdout_path, | ||
| &stderr_path, | ||
| ) | ||
| .with_context(|| format!("run {}", definition.id.as_str()))?; | ||
| let duration_ms = started.elapsed().as_secs_f64() * 1000.0; | ||
| report.exit_status = outcome.exit_status; | ||
| report.success = outcome.success; | ||
| report.timed_out = outcome.timed_out; | ||
| report.stdout_path = Some(stdout_path.display().to_string()); | ||
| report.stderr_path = Some(stderr_path.display().to_string()); | ||
| report.metrics = collect_metrics(definition, &run_dir, duration_ms); | ||
| if let Err(error) = collect_telemetry(&metrics_http, &metrics_run_id, &run_dir) | ||
| .map(|telemetry| report.telemetry = telemetry) | ||
| { | ||
| report.telemetry = | ||
| telemetry_report::unavailable(&metrics_http, &metrics_run_id, &error); | ||
| report.success = false; | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the relevant file and inspect the surrounding region.
ast-grep outline crates/skippy-bench/src/evals.rs --view expanded >/tmp/evals_outline.txt
sed -n '320,430p' crates/skippy-bench/src/evals.rs | cat -n
# Find report.success and collect_telemetry usage in the file.
rg -n "collect_telemetry|report\.success|timed_out|run\.json|bail!" crates/skippy-bench/src/evals.rsRepository: Mesh-LLM/mesh-llm
Length of output: 5759
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the telemetry helper and report type to understand expected semantics.
rg -n "fn collect_telemetry|struct .*Report|success:" crates/skippy-bench/src -g '!target'Repository: Mesh-LLM/mesh-llm
Length of output: 1031
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect where run.json is written and whether telemetry failures are treated specially elsewhere.
rg -n "run\.json|serde_json|write.*report|report\.telemetry|telemetry\.status|unavailable\(" crates/skippy-bench/src -g '!target'Repository: Mesh-LLM/mesh-llm
Length of output: 5780
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read a tight window around the telemetry branch and the subsequent reporting logic.
sed -n '360,410p' crates/skippy-bench/src/evals.rs | cat -nRepository: Mesh-LLM/mesh-llm
Length of output: 2445
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect telemetry handling and compare with another caller that records telemetry.
sed -n '900,1015p' crates/skippy-bench/src/evals.rs | cat -n
sed -n '150,205p' crates/skippy-bench/src/chat_corpus.rs | cat -n
sed -n '130,190p' crates/skippy-bench/src/telemetry_report.rs | cat -nRepository: Mesh-LLM/mesh-llm
Length of output: 10034
Don’t fail the eval when telemetry finalization fails. collect_telemetry errors currently set report.success = false, so a successful harness run is treated as failed and the process bails just because the metrics server was unavailable at the end. Keep the harness result in report.success and report telemetry failure separately, or gate this behavior behind an explicit flag.
🤖 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 `@crates/skippy-bench/src/evals.rs` around lines 356 - 382, The telemetry
finalization block in evals::run currently flips report.success to false when
collect_telemetry fails, which incorrectly marks an otherwise successful harness
run as failed. Update the error handling around collect_telemetry so
telemetry_unavailable is recorded in report.telemetry without changing the
harness outcome stored in report.success, or make this failure-to-fail behavior
conditional on an explicit flag. Keep the logic localized to the
collect_telemetry/report.telemetry handling in evals::run.
| fn mcp_atlas_metrics(run_dir: &Path) -> Result<EvalMetrics> { | ||
| let mut reader = csv::Reader::from_path(mcp_atlas_output_path(run_dir))?; | ||
| let data_rows = reader.records().filter(|record| record.is_ok()).count(); | ||
| Ok(EvalMetrics { | ||
| request_count: Some(data_rows as u64), | ||
| ..EvalMetrics::default() | ||
| }) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
MCP-Atlas parse failures are silently dropped from request_count.
reader.records().filter(|record| record.is_ok()).count() silently discards any row that fails to parse as valid CSV, without logging or surfacing how many rows were dropped. If the upstream completion script emits a malformed CSV (e.g., due to an embedded error message), request_count will silently undercount rather than surfacing the discrepancy.
♻️ Suggested fix — surface parse failures
fn mcp_atlas_metrics(run_dir: &Path) -> Result<EvalMetrics> {
let mut reader = csv::Reader::from_path(mcp_atlas_output_path(run_dir))?;
- let data_rows = reader.records().filter(|record| record.is_ok()).count();
+ let (ok_rows, failed_rows): (Vec<_>, Vec<_>) =
+ reader.records().partition(Result::is_ok);
+ if !failed_rows.is_empty() {
+ eprintln!(
+ "warning: mcp-atlas completion CSV had {} unparsable row(s)",
+ failed_rows.len()
+ );
+ }
+ let data_rows = ok_rows.len();
Ok(EvalMetrics {
request_count: Some(data_rows as u64),
..EvalMetrics::default()
})
}📝 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.
| fn mcp_atlas_metrics(run_dir: &Path) -> Result<EvalMetrics> { | |
| let mut reader = csv::Reader::from_path(mcp_atlas_output_path(run_dir))?; | |
| let data_rows = reader.records().filter(|record| record.is_ok()).count(); | |
| Ok(EvalMetrics { | |
| request_count: Some(data_rows as u64), | |
| ..EvalMetrics::default() | |
| }) | |
| } | |
| fn mcp_atlas_metrics(run_dir: &Path) -> Result<EvalMetrics> { | |
| let mut reader = csv::Reader::from_path(mcp_atlas_output_path(run_dir))?; | |
| let (ok_rows, failed_rows): (Vec<_>, Vec<_>) = | |
| reader.records().partition(Result::is_ok); | |
| if !failed_rows.is_empty() { | |
| eprintln!( | |
| "warning: mcp-atlas completion CSV had {} unparsable row(s)", | |
| failed_rows.len() | |
| ); | |
| } | |
| let data_rows = ok_rows.len(); | |
| Ok(EvalMetrics { | |
| request_count: Some(data_rows as u64), | |
| ..EvalMetrics::default() | |
| }) | |
| } |
🤖 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 `@crates/skippy-bench/src/evals.rs` around lines 1047 - 1054, The
`mcp_atlas_metrics` function is silently undercounting `request_count` by
filtering out CSV parse errors in `reader.records()`. Update this logic to
detect and surface malformed rows instead of dropping them quietly: either
propagate the first parse error or count/report failed records alongside the
valid count, and make sure the final `EvalMetrics` computation reflects any
parse failures rather than only successful rows.
| pub ttft_ms: Option<TelemetryAggregate>, | ||
| pub fttt_ms: Option<TelemetryAggregate>, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
fttt_ms is a duplicate of ttft_ms, not a distinct metric.
fttt_ms is populated with the exact same ttft_values used for ttft_ms (aggregate(&ttft_values) for both, Line 238-239). Every consumer (README/SKILL.md) treats "TTFT/FTTT" as two separate correlated metrics, but as implemented they will always be numerically identical, which is misleading in reports and wastes a field. Either compute FTTT from a genuinely distinct signal (e.g., first-byte/network time vs. decode-token time) or drop the redundant field until that logic exists.
🐛 Likely oversight — same source values reused
- ttft_ms: aggregate(&ttft_values),
- fttt_ms: aggregate(&ttft_values),
+ ttft_ms: aggregate(&ttft_values),
+ // TODO: compute a genuinely distinct FTTT signal, or remove this field.
+ fttt_ms: aggregate(&ttft_values),Also applies to: 230-242
🤖 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 `@crates/skippy-bench/src/telemetry_report.rs` around lines 18 - 19, The
telemetry report currently exposes fttt_ms as a separate metric, but
TelemetryReportBuilder is populating it from the same ttft_values as ttft_ms, so
the two fields are always identical. In telemetry_report.rs, update the
TelemetryReport/TelemetryReportBuilder flow so fttt_ms is only set from a
genuinely distinct signal if one exists, otherwise remove the redundant field
and its aggregation from the builder and any report serialization/consumers.
ndizazzo
left a comment
ndizazzo
left a comment
There was a problem hiding this comment.
Additional agent review on top of the code analysis from coderabbit:
Minor Findings
P2 Reproducibility gap: external eval definitions use floating refs like master/main, and run.json does not record the resolved harness commit. For benchmark evidence, this means two “same” runs can use different upstream code. See evals.rs (line 507).
P2 eval sync does not update existing clones: it runs git fetch origin and git checkout , but does not fast-forward/reset the local branch. A stale cache can remain stale while reporting “sync”. See evals.rs (line 781).
P2 --api-key is ignored for speed-bench: EvalRunArgs exposes api_key, and the other adapters pass it through, but speed_bench_command does not. Auth-required endpoints would fail only for this eval. See evals.rs (line 1151).
P3 module shape violates repo policy: the new evals.rs (line 1) is 2,154 lines. It should split into registry/sync/doctor/run plus per-adapter modules before merge.
|
This pull request has not been updated in at least 5 days. It will be closed after 7 days of inactivity to keep the active review queue current. Please update it within 2 days if the changes are still moving forward. |
|
Closing this pull request because it has not been updated in at least 7 days. Please reopen or create a fresh pull request when the changes are ready to continue. |
2 participants
What changed
This PR makes SkippyBench the entry point for the core external agent/coding eval pack while keeping the native upstream harnesses and datasets authoritative. SkippyBench wraps, installs, launches, and collects reports; it does not rebuild miniature benchmark datasets or substitute Skippy-owned task filters.
The branch has been updated with current
origin/mainas of 2026-07-02.SkippyBench eval commands
Adds the
skippy-bench evalcommand family:eval listeval infoeval synceval installeval doctoreval runExternal harness download/install remains explicit through
eval sync/eval install; normal builds do not clone benchmark repos, install Python tools, or pull Docker images.Core benchmark pack
speed-benchspeed_bench.pywith qualitative/all-category traffic against the configured OpenAI endpoint.terminal-benchswe-bench-promcp-atlasOptional future packs remain documented but are not wired in this PR: NL2RepoBench and Toolathlon / Tool-Decathlon.
Metrics and reports
eval runcreates/finalizes a metrics-server run and stores the raw metrics report beside native harness artifacts.chat-corpusnow creates/finalizes a metrics-server run, writes telemetry into its JSON output, and fails if metrics export is unavailable.report.json.--endpoint-concurrencyand validates harness-level request concurrency against the serving endpoint concurrency so benchmark workers do not overload a smaller Skippy lane count.Skippy/OpenAI compatibility
target/release/skippy-serverinstead of debug builds.skippy-server serve-openaiwith metrics-only guardrails by default so weak local model output is recorded as model behavior instead of becoming transport-level HTTP 400 failure.response_format: { type: "json_schema" }through the Skippy runtime feature guard so native eval clients can send their usual requests.$defs/$refsupport in structured-output validation.Skill and docs
crates/skippy-bench/README.mdwith theevalcommand flow, metrics-server requirements, external benchmark table, use-case guidance, and optional later packs..agents/skills/skippy-bench/SKILL.mdso Codex knows:skippy-serverValidation
Ran on the updated-with-main branch:
cargo fmt --all -- --checkjust with-lld cargo check -p skippy-bench -p skippy-server -p openai-frontendjust with-lld cargo clippy -p skippy-bench -p skippy-server -p openai-frontend --all-targets -- -D warningsjust with-lld cargo test -p skippy-bench --binsjust with-lld cargo test -p openai-frontend guardrailsjust with-lld cargo test -p skippy-server --lib frontendNotes:
lab-experiments/benchmark artifacts remain untracked and are not part of this PR.0078-Contain-chat-grammar-sampler-exceptions.patchwas dropped during the main merge because currentmainhas the chat-grammar containment behavior in the squashed patch queue.Summary by CodeRabbit
New Features
Bug Fixes
Documentation