Fix SkippyBench eval reproducibility - #992
Merged
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughSkippyBench evaluation handling is reorganized into registry, synchronization, diagnostics, adapter, and run modules. Supported benchmarks now generate dedicated commands, collect evaluation-specific metrics and artifacts, redact secret environment values, and record the resolved harness commit in ChangesSkippyBench evaluation workflow
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant EvalCLI
participant run_eval
participant Adapter
participant Harness
participant RunReport
EvalCLI->>run_eval: submit evaluation arguments
run_eval->>Adapter: build benchmark command
Adapter->>Harness: execute evaluation harness
Harness-->>run_eval: return artifacts and status
run_eval->>RunReport: resolve commit and collect metrics
RunReport-->>EvalCLI: write run.json
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
Contributor
|
This pull request is currently a draft. Reviews will not take place until the PR is marked as ready for review. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/skippy-bench/src/evals.rs (1)
138-221: 🔒 Security & Privacy | 🔴 Critical | ⚡ Quick winRedact
OPENAI_API_KEYincrates/skippy-bench/src/evals/adapters/terminal_bench.rs
OPENAI_API_KEYstill goes through.env(...), so it will be included incommand.display()and persisted inrun.json. Switch it to.secret_env(...).
mcp_atlas_commandandswe_bench_pro_commandalso write the raw key intorun_dir/raw/*.sh, so those artifacts retain secrets on disk too.🤖 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 138 - 221, Update the command construction in terminal_bench.rs so OPENAI_API_KEY is passed through secret_env rather than env, ensuring CommandSpec::display redacts it. Apply the same secret handling in mcp_atlas_command and swe_bench_pro_command, and prevent those commands from writing the raw key into run_dir/raw/*.sh while preserving their existing execution behavior.
🧹 Nitpick comments (2)
crates/skippy-bench/src/evals/run.rs (2)
3-102: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
run_evalis close to the function-size guideline; consider phase-splitting.At ~100 lines this sits right at clippy's default
too_many_linesthreshold: too-many-lines-threshold = 100 # default: 100 (make explicit). The function mixes validation, dry-run branching, command execution, and report assembly. Splitting into e.g.build_initial_report(...)andexecute_and_finalize(...)helpers would keep it comfortably under the limit and match the guideline's "split before it reaches the threshold" intent.🤖 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/run.rs` around lines 3 - 102, Split run_eval before the 100-line threshold by extracting initial RunReport construction into a build_initial_report helper and non-dry-run execution, metrics, telemetry, and report updates into an execute_and_finalize helper. Keep run_eval responsible for validation, setup, command creation, helper orchestration, report serialization, and final success handling.Source: Coding guidelines
183-193: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMetrics-parsing failures are silently swallowed.
collect_metrics(Line 185-188) defaults to an emptyEvalMetricsvia.unwrap_or_default()on any parse error, andmcp_atlas_metrics(Line 345) drops malformed CSV rows via.filter(|record| record.is_ok())— both without any diagnostic. A harness output-format regression would silently produce all-empty/undercounted metrics with no signal in stdout orrun.jsonexplaining why.Also applies to: 343-350
🤖 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/run.rs` around lines 183 - 193, Update collect_metrics to report errors from each evaluator-specific metrics parser instead of silently replacing failures with EvalMetrics::default(), while preserving duration and client-rate population. In mcp_atlas_metrics, stop filtering malformed CSV records without diagnostics; surface or explicitly log each parse failure so undercounted metrics are observable.
🤖 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/skippy-bench/src/evals/adapters/speed_bench.rs`:
- Line 34: Update the cache_root construction in the Speed-Bench adapter to
derive its path from the configured root parameter instead of env::temp_dir().
Keep the existing cache subdirectory name and ensure the --cache-root value
controls the adapter’s cache location consistently with the other adapters.
- Around line 3-21: Update AUTH_LAUNCHER’s authorized_request to inject the
Authorization header only when the request URL targets the benchmark --url host;
leave headers unchanged for all other hosts. Avoid applying credentials through
the globally patched request hook beyond that endpoint, while preserving
existing caller-provided Authorization headers.
In `@crates/skippy-bench/src/evals/adapters/swe_bench_pro.rs`:
- Around line 14-289: Extract the large shell script template from
write_swe_bench_pro_run_script into a separate template file and load it with
include_str!, leaving this function responsible only for setup, value
preparation, formatting, and writing. Apply the same extraction to the
corresponding oversized script template in write_mcp_atlas_run_script in
crates/skippy-bench/src/evals/adapters/mcp_atlas.rs (lines 18-187), preserving
all placeholders and generated script behavior.
In `@crates/skippy-bench/src/evals/adapters/terminal_bench.rs`:
- Around line 25-26: Standardize API-key handling across the eval adapters: in
crates/skippy-bench/src/evals/adapters/terminal_bench.rs:25-26, replace the
recorded OPENAI_API_KEY env assignment with CommandSpec::secret_env; in
crates/skippy-bench/src/evals/adapters/mcp_atlas.rs:34-52, remove direct
args.api_key interpolation from the generated script and its
LLM_API_KEY/OPENAI_API_KEY exports, instead registering it via secret_env and
reading the process environment; in
crates/skippy-bench/src/evals/adapters/swe_bench_pro.rs:54-77, apply the same
secret_env and environment lookup pattern, avoiding plaintext embedding and CLI
propagation at the referenced model-API-key sites.
In `@crates/skippy-bench/src/evals/run.rs`:
- Around line 82-88: Update the collect_telemetry error branch in the eval run
flow so it only records telemetry_report::unavailable and does not set
report.success to false. Preserve report.success as determined by
outcome.success, while continuing to represent telemetry collection failures
separately.
---
Outside diff comments:
In `@crates/skippy-bench/src/evals.rs`:
- Around line 138-221: Update the command construction in terminal_bench.rs so
OPENAI_API_KEY is passed through secret_env rather than env, ensuring
CommandSpec::display redacts it. Apply the same secret handling in
mcp_atlas_command and swe_bench_pro_command, and prevent those commands from
writing the raw key into run_dir/raw/*.sh while preserving their existing
execution behavior.
---
Nitpick comments:
In `@crates/skippy-bench/src/evals/run.rs`:
- Around line 3-102: Split run_eval before the 100-line threshold by extracting
initial RunReport construction into a build_initial_report helper and
non-dry-run execution, metrics, telemetry, and report updates into an
execute_and_finalize helper. Keep run_eval responsible for validation, setup,
command creation, helper orchestration, report serialization, and final success
handling.
- Around line 183-193: Update collect_metrics to report errors from each
evaluator-specific metrics parser instead of silently replacing failures with
EvalMetrics::default(), while preserving duration and client-rate population. In
mcp_atlas_metrics, stop filtering malformed CSV records without diagnostics;
surface or explicitly log each parse failure so undercounted metrics are
observable.
🪄 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: e0c7f538-7cb5-4405-b22a-15d86021b6a7
📒 Files selected for processing (12)
.agents/skills/skippy-bench/SKILL.mdcrates/skippy-bench/README.mdcrates/skippy-bench/src/evals.rscrates/skippy-bench/src/evals/adapters/mcp_atlas.rscrates/skippy-bench/src/evals/adapters/mod.rscrates/skippy-bench/src/evals/adapters/speed_bench.rscrates/skippy-bench/src/evals/adapters/swe_bench_pro.rscrates/skippy-bench/src/evals/adapters/terminal_bench.rscrates/skippy-bench/src/evals/doctor.rscrates/skippy-bench/src/evals/registry.rscrates/skippy-bench/src/evals/run.rscrates/skippy-bench/src/evals/sync.rs
Contributor
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 `@crates/skippy-bench/src/evals/adapters/templates/mcp_atlas_run.sh`:
- Around line 85-113: Update the service startup logic around port_ready, the
Docker agent environment, and mcp_completion.main so an open port is not treated
as sufficient evidence that the expected service is reusable. Track ownership of
processes started by this script and fail when ports 1984 or 3000 are occupied
by unowned services, or add identity/configuration checks that verify the
running services use this evaluation’s harness, base URL, and API key before
reuse.
In `@crates/skippy-bench/src/evals/adapters/templates/swe_bench_pro_run.sh`:
- Around line 87-104: Update the Docker patching logic around
SWEREX_PIP_INDEX_URL and the generated pip install command so the URL is safely
quoted and credentials are not embedded in the Docker RUN command or generated
source/image history. Pass authenticated index credentials through the supported
Docker build-secret mechanism, while retaining only the non-secret index URL in
the command; update the docker module replacement strings accordingly.
🪄 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: cbcf9c38-1698-4556-bf69-02f5b5c68a0a
📒 Files selected for processing (10)
.agents/skills/skippy-bench/SKILL.mdcrates/skippy-bench/README.mdcrates/skippy-bench/src/evals.rscrates/skippy-bench/src/evals/adapters/mcp_atlas.rscrates/skippy-bench/src/evals/adapters/speed_bench.rscrates/skippy-bench/src/evals/adapters/swe_bench_pro.rscrates/skippy-bench/src/evals/adapters/templates/mcp_atlas_run.shcrates/skippy-bench/src/evals/adapters/templates/swe_bench_pro_run.shcrates/skippy-bench/src/evals/adapters/terminal_bench.rscrates/skippy-bench/src/evals/run.rs
🚧 Files skipped from review as they are similar to previous changes (6)
- .agents/skills/skippy-bench/SKILL.md
- crates/skippy-bench/src/evals/adapters/terminal_bench.rs
- crates/skippy-bench/README.md
- crates/skippy-bench/src/evals/adapters/speed_bench.rs
- crates/skippy-bench/src/evals/run.rs
- crates/skippy-bench/src/evals.rs
Comment on lines
+85
to
+113
| if ! port_ready 1984; then | ||
| docker rm -f skippy-bench-mcp-atlas-agent-env >/dev/null 2>&1 || true | ||
| docker run --rm \ | ||
| --name skippy-bench-mcp-atlas-agent-env \ | ||
| -p 1984:1984 \ | ||
| --env-file .env \ | ||
| agent-environment:latest \ | ||
| > "$RAW_DIR/mcp-agent-env.log" 2>&1 & | ||
| agent_started=1 | ||
| fi | ||
| wait_url "MCP-Atlas agent environment" \ | ||
| "http://localhost:1984/enabled-servers" \ | ||
| "$RAW_DIR/mcp-agent-env.log" | ||
|
|
||
| if ! port_ready 3000; then | ||
| ( | ||
| cd "$COMPLETION_DIR" | ||
| LLM_BASE_URL="$BASE_URL" \ | ||
| LLM_API_KEY="$API_KEY" \ | ||
| OPENAI_BASE_URL="$BASE_URL" \ | ||
| OPENAI_API_KEY="$API_KEY" \ | ||
| uv run python -m mcp_completion.main | ||
| ) > "$RAW_DIR/mcp-completion.log" 2>&1 & | ||
| completion_pid="$!" | ||
| completion_started=1 | ||
| fi | ||
| wait_url "MCP-Atlas completion service" \ | ||
| "http://localhost:3000/docs" \ | ||
| "$RAW_DIR/mcp-completion.log" |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Do not silently reuse services merely because their ports are open.
An old or concurrent service can pass these probes while using a different harness, base URL, or API key. Fail when an unowned port is occupied, or verify service identity and configuration before reuse; otherwise evaluation results are not reproducible.
🤖 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/adapters/templates/mcp_atlas_run.sh` around
lines 85 - 113, Update the service startup logic around port_ready, the Docker
agent environment, and mcp_completion.main so an open port is not treated as
sufficient evidence that the expected service is reusable. Track ownership of
processes started by this script and fail when ports 1984 or 3000 are occupied
by unowned services, or add identity/configuration checks that verify the
running services use this evaluation’s harness, base URL, and API key before
reuse.
Comment on lines
+87
to
+104
| if [[ "$DEPLOYMENT_TYPE" == "docker" && -n "$SWEREX_PIP_INDEX_URL" ]]; then | ||
| .venv/bin/python - "$SWEREX_PIP_INDEX_URL" <<'PY' | ||
| import sys | ||
| from pathlib import Path | ||
|
|
||
| import swerex.deployment.docker as docker | ||
|
|
||
| path = Path(docker.__file__) | ||
| text = path.read_text() | ||
| old = 'f"RUN /root/python3.11/bin/pip3 install --no-cache-dir {{PACKAGE_NAME}}\\n\\n"' | ||
| new = ( | ||
| f'f"RUN /root/python3.11/bin/pip3 install --index-url {{sys.argv[1]}} ' | ||
| '--no-cache-dir {{PACKAGE_NAME}}\\n\\n"' | ||
| ) | ||
| if old in text: | ||
| path.write_text(text.replace(old, new)) | ||
| elif new not in text: | ||
| raise RuntimeError(f"could not patch SWE-ReX Docker pip index in {{path}}") |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Do not embed the package-index URL directly into a Docker RUN command.
SWEREX_PIP_INDEX_URL is inserted unquoted, allowing shell metacharacters to alter the build command. Authenticated URLs may also persist credentials in generated source or image history. Pass index credentials through a build secret and safely quote the non-secret URL.
🤖 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/adapters/templates/swe_bench_pro_run.sh` around
lines 87 - 104, Update the Docker patching logic around SWEREX_PIP_INDEX_URL and
the generated pip install command so the URL is safely quoted and credentials
are not embedded in the Docker RUN command or generated source/image history.
Pass authenticated index credentials through the supported Docker build-secret
mechanism, while retaining only the non-secret index URL in the command; update
the docker module replacement strings accordingly.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changed
harness_commitin everyrun.json--api-keyto SPEED-Bench requests through a small bearer-auth launcher while keeping the upstream harness unchanged and redacting the token from recorded commandsevals.rsinto registry, sync, doctor, run, and per-adapter modulesWhy
This follows up on the four findings in the post-merge review of #865. Floating upstream refs were not auditable, cached clones could remain stale after
eval sync, SPEED-Bench ignored endpoint credentials, and the new eval implementation exceeded the repository's source-file size policy.Validation
cargo fmt --all --checkcargo check -p skippy-benchcargo clippy -p skippy-bench --all-targets -- -D warningscargo test -p skippy-bench --bins(55 passed)git diff --checkharness_commitoutput and API-key redactionSummary by CodeRabbit
harness_commit).