Release v1.2.0: close all milestone issues (security, perf, features, docs)#81
Conversation
PRICES lacked the claude-sonnet-4 / claude-opus-4 families that the project actually passes in (config/defaults.rb -> claude-sonnet-4-20250514, clients/provider_schemas.rb -> claude-opus-4-7), so price_for fell through to nil and cost was reported unknown for the default Anthropic model. Add both families, preserving longest-prefix matching so dated variants still resolve. Covered by a new test asserting the real default strings.
Tree diagram referenced the dead .skill-bench-history.json; the engine writes .skill-bench-trends.json. Adds a note that .skill-bench-trends.json.bak is the auto-backup. Closes #39.
Adds Contributor Covenant 2.1, bug-report/feature-request issue templates mirroring CONTRIBUTING.md, an issue-template config, and a PR template. Closes #38.
…seam (#21) Documents in the README that allowlisted wrapper binaries (rake/rspec/make/ find/git) equal arbitrary host execution, tied to the fail-closed host model. Adds an optional, default-off command_argument_constraints hook so high-risk allowed commands can be constrained; behavior is unchanged when unconfigured. Closes #21.
…run (#54, #31) Config#mock? distinguishes an explicit {"provider":"mock"} config from a failed/empty load, so ProviderResolver no longer prints a misleading 'Config load failed' warning on a clean offline run; genuine failures still warn. Config.loaded memoizes the parse per absolute path, invalidated by mtime, so skill-bench.json is parsed at most once per run while a rewritten file is still re-read. Closes #54. Closes #31.
skill-bench validate (alias doctor) runs CriteriaValidator over criteria.json, hand-rolled schema-validates skill-bench.json (provider/max_execution_time/config), and reports missing provider API keys via BaseClient#missing_config_keys — all without running an eval or hitting the network. Closes #45.
Adds Services::ResponseCache (SHA-256 over provider/model/system_prompt/messages/ tools/temperature) consulted by Client.call when caching is enabled via --cache or SKILL_BENCH_CACHE. Default off = unchanged behavior. Identical LLM calls — notably compare's twice-run skill-less baseline — hit the cache instead of the network. Null/mock clients are excluded. Closes #42.
Services::HtmlFormatter renders the delta report table, baseline/context iteration timelines, and the tokens/cost header as a self-contained HTML document with all dynamic text CGI-escaped. Wired :html into OutputFormatter.format and the --format help (human, json, junit, html). Closes #44.
JUnitFormatter.format_batch emits one <testcase> per eval for batch runs; SummaryFormatter renders an aggregate JSON gate (passed/failed/total, summed tokens/cost, worst-delta eval); run --all threads --format junit and --summary through BatchResultPrinter while preserving the batch exit code. Adds a top-level composite action.yml so downstream repos can use igmarin/ruby-skill-bench@v1. Closes #46.
PackageVerifier required lib/skill_bench/source_path_resolver.rb, but the file lives at lib/skill_bench/execution/source_path_resolver.rb, so rake package:verify always failed. Points the entry at the real path.
Expands the [1.2.0] CHANGELOG with the full release set (validate/doctor, html format, token/cost, caching, summary/JUnit batch + action.yml, Mistral, init --mock, security note, config-parse perf, mock-warning fix, docs) and documents the new commands, flags, the Mistral provider, token/cost output, and the GitHub Action in the README.
|
Warning Review limit reached
Next review available in: 17 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository UI (base), Organization UI (inherited) Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (18)
📝 WalkthroughWalkthroughThis PR adds nine features to skill-bench: a ChangesCore Feature Implementation
Community Health Files and Docs
Sequence Diagram(s)sequenceDiagram
participant CLI
participant ValidateCommand
participant CriteriaValidator
participant ConfigLoader
participant ProviderRegistry
CLI->>ValidateCommand: call(argv)
ValidateCommand->>CriteriaValidator: validate criteria.json
ValidateCommand->>ConfigLoader: load skill-bench.json
ValidateCommand->>ProviderRegistry: resolve provider client
ProviderRegistry-->>ValidateCommand: missing_config_keys
ValidateCommand-->>CLI: PASS/FAIL report + exit code
sequenceDiagram
participant Client
participant ResponseCache
participant ProviderClient
Client->>Client: cache_eligible?(client_class)
alt eligible and cache hit
Client->>ResponseCache: fetch(key)
ResponseCache-->>Client: cached response
else not eligible or miss
Client->>ProviderClient: call(system_prompt, messages, ...)
ProviderClient-->>Client: response
Client->>ResponseCache: store(key, response)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
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 |
# Conflicts: # test/evaluator/tools/run_command_test.rb
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
.github/PULL_REQUEST_TEMPLATE.md (1)
17-26: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winFix the
yard:coveragecommand syntax.Line 23 lists
bundle exec yard:coverage, but the standard invocation isbundle exec rake yard:coverage(orbundle exec rake yard:coveragedepending on the Rake task name). Withoutrake, most shells will attempt to run a binary namedyard:coverage, which does not exist.- [ ] `bundle exec yard:coverage` passes + [ ] `bundle exec rake yard:coverage` passes🤖 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 @.github/PULL_REQUEST_TEMPLATE.md around lines 17 - 26, The checklist entry for the YARD coverage command uses the wrong invocation syntax, so update the pull request template’s checklist item to use the Rake task form instead of a bare `yard:coverage` command. Make the change in the template text where the existing `bundle exec yard:coverage` appears, keeping it consistent with the other `bundle exec rake ...` entries and preserving the `yard:coverage` task name.lib/skill_bench/config/applier.rb (1)
40-47: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winExpose
command_argument_constraintsinJsonLoaderskill-bench.jsondrops this key today, so the new constraint never reachesApplierfor JSON-based configs. Add it to theslice(...)whitelist inlib/skill_bench/config/json_loader.rb.🤖 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 `@lib/skill_bench/config/applier.rb` around lines 40 - 47, `command_argument_constraints` is being applied in `Applier#apply_scalar_values`, but JSON configs never pass it through because `JsonLoader` filters it out. Update `JsonLoader`’s `slice(...)` whitelist to include `command_argument_constraints` so the value from `skill-bench.json` reaches `Applier` and gets assigned to the store.
🧹 Nitpick comments (9)
lib/skill_bench/cli/init_command.rb (1)
63-67: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
--mockmissing from "provider required" hint.
error_missing_provideronly enumeratesProviderSchemas.names, so users who omit a provider are never told--mockis a valid offline option.💡 Proposed fix
def error_missing_provider - providers = SkillBench::Clients::ProviderSchemas.names.map { |provider_name| "--#{provider_name}" }.join(', ') + providers = (SkillBench::Clients::ProviderSchemas.names.map { |provider_name| "--#{provider_name}" } + ['--mock']).join(', ') warn "Error: provider is required. Use one of: #{providers}" 1 end🤖 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 `@lib/skill_bench/cli/init_command.rb` around lines 63 - 67, The provider-required hint in error_missing_provider omits the offline --mock option because it only uses SkillBench::Clients::ProviderSchemas.names; update the provider list construction in this method to include --mock alongside the schema-based providers so the warning mentions every valid init option.examples/README.md (1)
7-9: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConfusing wording: "scaffold ... by hand".
The tip says running
skill-bench init --mockscaffolds the config "by hand as a fallback", which is contradictory — the command does it automatically; writing the JSON yourself would be the manual fallback.✏️ Proposed fix
-> Tip: run `skill-bench init --mock` to scaffold that offline mock config -> (`{"provider":"mock","max_execution_time":30}`) by hand as a fallback. +> Tip: run `skill-bench init --mock` to scaffold that offline mock config +> (`{"provider":"mock","max_execution_time":30}`), or write it by hand as a fallback.🤖 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 `@examples/README.md` around lines 7 - 9, The README tip in the mock config section uses contradictory wording by saying the CLI command “scaffold[s] ... by hand”; update the sentence in the documentation to clearly separate the automatic path from the manual fallback. Keep the reference to the `skill-bench init --mock` command, and rephrase the fallback so it explicitly refers to writing the JSON config manually instead of implying the command does it “by hand.”test/cli/validate_command_test.rb (1)
26-26: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReaching into
Models::Configinternal ivar from a different layer's test.
Models::Config.instance_variable_set(:@loaded, nil)directly pokes the memoization internals being introduced in the sibling "Config memoization" layer.instance_variable_setsilently no-ops if the ivar name changes (e.g., renamed during refactor), causing memoized config to leak across test runs without any failure signal — a brittle, hard-to-debug coupling.Suggested fix: expose a public reset hook
- Models::Config.instance_variable_set(:`@loaded`, nil) + Models::Config.reset_loaded_cache🤖 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 `@test/cli/validate_command_test.rb` at line 26, The test is tightly coupled to Models::Config’s internal memoization state by setting `@loaded` directly, which is brittle and can break silently if the implementation changes. Add a public reset hook on Models::Config to clear the cached config state, then update validate_command_test to call that helper instead of instance_variable_set so the test uses the supported API.lib/skill_bench/cli/validate_command.rb (1)
162-188: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse the shared env-override logic
provider_client_options/env_settingduplicate the sameSKILL_BENCH_<PROVIDER>_<SETTING>→<PROVIDER>_<SETTING>precedence already used byModels::Provider. Extracting one helper would keep validation and provider resolution in sync if that naming or order changes.🤖 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 `@lib/skill_bench/cli/validate_command.rb` around lines 162 - 188, The env override resolution in missing_provider_keys/provider_client_options/env_setting duplicates the same SKILL_BENCH_<PROVIDER>_<SETTING> then <PROVIDER>_<SETTING> precedence already used by Models::Provider. Refactor ValidateCommand to reuse a shared helper or the existing Models::Provider logic for building provider_client_options so validation stays in sync if the naming or precedence changes, and keep env_setting only as a thin delegate or remove it if no longer needed.test/agent_eval/services/provider_resolver_test.rb (1)
97-107: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPrefer Minitest's built-in
capture_ioover a custom stderr-capturing helper.
Minitest::Assertions#capture_ioalready redirects and restores both$stdout/$stderraround a block and returns the captured strings, making this private helper redundant.♻️ Proposed refactor
- private - - def capture_stderr - original = $stderr - $stderr = StringIO.new - yield - $stderr.string - ensure - $stderr = original - end - def write_mock_configAnd update call sites to use
capture_io { ... }.last(stderr is the second element of the returned array) in place ofcapture_stderr { ... }.🤖 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 `@test/agent_eval/services/provider_resolver_test.rb` around lines 97 - 107, Replace the custom `capture_stderr` helper in `provider_resolver_test.rb` with Minitest’s built-in `capture_io`, since it already handles redirecting and restoring both streams; update every `capture_stderr { ... }` call site to use `capture_io { ... }.last` so the tests still read captured stderr while removing the redundant private method.lib/skill_bench/models/config.rb (1)
27-50: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReflection-based cache reset is fragile; expose a public API instead.
The docstring instructs resetting by setting
@loadedtonildirectly, and bothtest/agent_eval/models/config_test.rbandtest/agent_eval/services/provider_resolver_test.rbdo this viainstance_variable_setat 5 call sites. If@loadedis ever renamed,instance_variable_setsilently no-ops (no error), and tests would start leaking stale cache across runs without any signal. A small public method removes the duplication and the encapsulation break.♻️ Proposed addition
+ # Clears the memoized config cache. Intended for test isolation. + # + # `@return` [void] + def self.reset_cache! + `@loaded` = {} + end + # Returns the configuration for a path, memoizing the parse per run.🤖 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 `@lib/skill_bench/models/config.rb` around lines 27 - 50, The cache reset for SkillBench::Models::Config is being done via direct ivar mutation and reflection, which is brittle and duplicated across tests. Add a small public reset API on SkillBench::Models::Config to clear the `@loaded` cache, update the loaded memoization docs to reference that API instead of setting the ivar directly, and replace all instance_variable_set calls in config and provider_resolver tests with the new method.lib/skill_bench/services/runner_service.rb (1)
177-221: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract shared usage-accumulation helpers to avoid triple duplication.
empty_usage/add_usage/token_counthere are identical to the same-named methods inlib/skill_bench/agent/react_agent/loop_runner.rb(lines 75-100), andtoken_count's symbol/string-key fallback logic is reimplemented again inlib/skill_bench/services/cost_calculator.rb(lines 82-88). Consider extracting a shared module (e.g.SkillBench::Support::TokenUsage) withempty,add, andcountso a future fix to usage-key handling doesn't need to be applied in three places.♻️ Sketch of a shared helper module
module SkillBench module Support module TokenUsage def self.empty { prompt_tokens: 0, completion_tokens: 0, total_tokens: 0 } end def self.add(total, usage) usage ||= {} { prompt_tokens: total[:prompt_tokens] + count(usage, :prompt_tokens), completion_tokens: total[:completion_tokens] + count(usage, :completion_tokens), total_tokens: total[:total_tokens] + count(usage, :total_tokens) } end def self.count(usage, key) (usage[key] || usage[key.to_s] || 0).to_i end end end end🤖 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 `@lib/skill_bench/services/runner_service.rb` around lines 177 - 221, The token-usage helper logic is duplicated across RunnerService, LoopRunner, and CostCalculator, so consolidate it into a shared helper/module (for example, SkillBench::Support::TokenUsage) with the equivalent of empty_usage, add_usage, and token_count. Update RunnerService#aggregate_usage and the other call sites to use the shared methods, preserving the symbol/string-key fallback behavior in token_count so future fixes only need to happen in one place.lib/skill_bench/output_formatter.rb (1)
141-171: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate token/cost rendering logic between formatters.
build_usage_lineformatsTokens: N | Est. Cost: $X.XXXXhere, and per graph evidenceServices::HtmlFormatterindependently builds the same "Tokens"/"Est. Cost" string with its ownKernel.format('$%.4f', cost)call (html_formatter.rb:114). Consider extracting a shared helper (e.g., a smallServices::UsageFormatteror module method) that both the human and HTML formatters call, so cost-formatting/precision stays consistent if it ever changes.🤖 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 `@lib/skill_bench/output_formatter.rb` around lines 141 - 171, The token/cost summary formatting is duplicated between OutputFormatter’s build_usage_line and Services::HtmlFormatter, which risks inconsistent formatting if either changes. Extract the shared “Tokens / Est. Cost” rendering into a common helper (for example, a Services::UsageFormatter method) and have both build_usage_line and the HTML formatter call it so the cost precision and label stay aligned in one place.action.yml (1)
57-59: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider pinning the gem version for reproducible CI gates.
gem install ruby-skill-benchalways installs latest; an unpinned dependency could break consumer pipelines unexpectedly on a future release. Consider adding an optionalversioninput.🤖 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 `@action.yml` around lines 57 - 59, The ruby-skill-bench install step in the workflow is unpinned, so CI may change behavior when new gem releases are published. Update the workflow around the install step to support a versioned install, ideally by adding an optional version input and using it in the install command so consumers can pin ruby-skill-bench to a specific release. Keep the change localized to the action definition where the gem is installed.
🤖 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 `@action.yml`:
- Around line 61-73: Replace the direct GitHub expression interpolation in the
Run skill-bench step with shell variables passed via env:, because the current
use of inputs.* inside the bash script can be script-injected before bash
parsing. Update the run script in action.yml to read those values from
environment variables instead of expanding them inline, and keep the
argument-building logic in the skill-bench step so it uses the safe variables
when constructing run_args and invoking skill-bench.
In `@lib/skill_bench/cli/run_command.rb`:
- Around line 87-91: The run command is passing an unsupported `:summary` option
into the single-eval path, causing `Commands::Run.run` to reject the keyword.
Update `RunCommand#run_single` (and any option handoff it uses) to strip or
ignore `options[:summary]` before calling `Commands::Run.run`, keeping `summary`
only for the batch-run flow where it is actually handled.
- Line 78: BatchResultPrinter.call is still falling through to
OutputFormatter.format_batch for non-junit batch runs, so --all with html/json
does not use the expected structured output. Update
BatchResultPrinter.batch_output to explicitly handle :html and :json (using the
same batch-specific path as :junit, or equivalent), or otherwise restrict the
supported batch formats advertised by RunCommand so they only include formats
actually implemented.
In `@lib/skill_bench/cli/validate_command.rb`:
- Around line 140-169: Avoid instantiating the provider client in
missing_provider_keys, since Clients::BaseClient#initialize can raise on
unrelated config like base_url/endpoint and bypass the intended FAIL result.
Update check_provider_key/missing_provider_keys to inspect required credential
keys from provider metadata or a non-instantiating helper instead of
Clients::ProviderRegistry.for(...).new(...) and send(:missing_config_keys), so
the CLI stays decoupled from provider-private internals.
In `@lib/skill_bench/client.rb`:
- Around line 38-46: `Client.call` is building a cache key in
`Services::ResponseCache.key` without all request-affecting provider config, so
different provider endpoints can collide. Update the cache key generation in
`Client.call` to also include the relevant `ProviderConfig` fields that change
the actual request, such as `base_url`, `request_path`, `endpoint`, `location`,
`project_id`, and `api_version`, alongside the existing `resolved`, `model`,
`system_prompt`, `messages`, `tools`, and `temperature` values.
In `@lib/skill_bench/services/json_formatter.rb`:
- Around line 27-35: The with_usage_fields method in JsonFormatter still allows
an explicit nil tokens value to overwrite the computed fallback, so fix the
merge behavior there by preventing nil :tokens from replacing the derived usage
data or by merging in the opposite order. Update the logic around
result[:tokens], result.dig(:response, :tokens), and EMPTY_USAGE so the output
always preserves a non-nil tokens value, and add a test covering the tokens: nil
case.
In `@lib/skill_bench/services/response_cache.rb`:
- Around line 68-89: ResponseCache#fetch is not thread-safe, so concurrent
Parallel.map callers can race between key? and []= and trigger duplicate
requests. Update the shared cache access in fetch (and any related store
helpers) to serialize the read/write around the per-process store, using a mutex
or equivalent synchronization on the store in ResponseCache so only one thread
computes and stores a missing value per key.
In `@lib/skill_bench/tools/run_command.rb`:
- Around line 96-104: `arguments_permitted?` is failing open because it looks up
`SkillBench::Config.command_argument_constraints` using a String `base_cmd`
while the config may contain symbol keys. Update
`RunCommand.arguments_permitted?` to normalize the lookup key (or normalize keys
in `SkillBench::Config.command_argument_constraints`), so
`constraints[base_cmd]` finds symbol-keyed entries consistently and forbidden
arguments are actually checked. Keep the fix centered on `arguments_permitted?`
and the config accessor used by it.
In `@test/evaluator/tools/run_command_test.rb`:
- Around line 103-134: The new constraint tests only cover the facade path and
miss the JSON-loading/key-type issues; update the tests around RunCommand.call
and SkillBench::Config so they load command_argument_constraints from a real
skill-bench.json via Config::JsonLoader after Config.reset/load_from_file
instead of assigning the hash directly. Add a regression case that uses the
loaded config to verify the constraint is enforced, so the behavior exercised
matches the bug locations in Config::Applier and RunCommand.
---
Outside diff comments:
In @.github/PULL_REQUEST_TEMPLATE.md:
- Around line 17-26: The checklist entry for the YARD coverage command uses the
wrong invocation syntax, so update the pull request template’s checklist item to
use the Rake task form instead of a bare `yard:coverage` command. Make the
change in the template text where the existing `bundle exec yard:coverage`
appears, keeping it consistent with the other `bundle exec rake ...` entries and
preserving the `yard:coverage` task name.
In `@lib/skill_bench/config/applier.rb`:
- Around line 40-47: `command_argument_constraints` is being applied in
`Applier#apply_scalar_values`, but JSON configs never pass it through because
`JsonLoader` filters it out. Update `JsonLoader`’s `slice(...)` whitelist to
include `command_argument_constraints` so the value from `skill-bench.json`
reaches `Applier` and gets assigned to the store.
---
Nitpick comments:
In `@action.yml`:
- Around line 57-59: The ruby-skill-bench install step in the workflow is
unpinned, so CI may change behavior when new gem releases are published. Update
the workflow around the install step to support a versioned install, ideally by
adding an optional version input and using it in the install command so
consumers can pin ruby-skill-bench to a specific release. Keep the change
localized to the action definition where the gem is installed.
In `@examples/README.md`:
- Around line 7-9: The README tip in the mock config section uses contradictory
wording by saying the CLI command “scaffold[s] ... by hand”; update the sentence
in the documentation to clearly separate the automatic path from the manual
fallback. Keep the reference to the `skill-bench init --mock` command, and
rephrase the fallback so it explicitly refers to writing the JSON config
manually instead of implying the command does it “by hand.”
In `@lib/skill_bench/cli/init_command.rb`:
- Around line 63-67: The provider-required hint in error_missing_provider omits
the offline --mock option because it only uses
SkillBench::Clients::ProviderSchemas.names; update the provider list
construction in this method to include --mock alongside the schema-based
providers so the warning mentions every valid init option.
In `@lib/skill_bench/cli/validate_command.rb`:
- Around line 162-188: The env override resolution in
missing_provider_keys/provider_client_options/env_setting duplicates the same
SKILL_BENCH_<PROVIDER>_<SETTING> then <PROVIDER>_<SETTING> precedence already
used by Models::Provider. Refactor ValidateCommand to reuse a shared helper or
the existing Models::Provider logic for building provider_client_options so
validation stays in sync if the naming or precedence changes, and keep
env_setting only as a thin delegate or remove it if no longer needed.
In `@lib/skill_bench/models/config.rb`:
- Around line 27-50: The cache reset for SkillBench::Models::Config is being
done via direct ivar mutation and reflection, which is brittle and duplicated
across tests. Add a small public reset API on SkillBench::Models::Config to
clear the `@loaded` cache, update the loaded memoization docs to reference that
API instead of setting the ivar directly, and replace all instance_variable_set
calls in config and provider_resolver tests with the new method.
In `@lib/skill_bench/output_formatter.rb`:
- Around line 141-171: The token/cost summary formatting is duplicated between
OutputFormatter’s build_usage_line and Services::HtmlFormatter, which risks
inconsistent formatting if either changes. Extract the shared “Tokens / Est.
Cost” rendering into a common helper (for example, a Services::UsageFormatter
method) and have both build_usage_line and the HTML formatter call it so the
cost precision and label stay aligned in one place.
In `@lib/skill_bench/services/runner_service.rb`:
- Around line 177-221: The token-usage helper logic is duplicated across
RunnerService, LoopRunner, and CostCalculator, so consolidate it into a shared
helper/module (for example, SkillBench::Support::TokenUsage) with the equivalent
of empty_usage, add_usage, and token_count. Update RunnerService#aggregate_usage
and the other call sites to use the shared methods, preserving the
symbol/string-key fallback behavior in token_count so future fixes only need to
happen in one place.
In `@test/agent_eval/services/provider_resolver_test.rb`:
- Around line 97-107: Replace the custom `capture_stderr` helper in
`provider_resolver_test.rb` with Minitest’s built-in `capture_io`, since it
already handles redirecting and restoring both streams; update every
`capture_stderr { ... }` call site to use `capture_io { ... }.last` so the tests
still read captured stderr while removing the redundant private method.
In `@test/cli/validate_command_test.rb`:
- Line 26: The test is tightly coupled to Models::Config’s internal memoization
state by setting `@loaded` directly, which is brittle and can break silently if
the implementation changes. Add a public reset hook on Models::Config to clear
the cached config state, then update validate_command_test to call that helper
instead of instance_variable_set so the test uses the supported API.
🪄 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: Repository UI (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 1f1025e8-27c3-4a38-be27-729fd16c0471
📒 Files selected for processing (66)
.github/ISSUE_TEMPLATE/bug_report.md.github/ISSUE_TEMPLATE/config.yml.github/ISSUE_TEMPLATE/feature_request.md.github/PULL_REQUEST_TEMPLATE.md.reek.ymlCHANGELOG.mdCODE_OF_CONDUCT.mdREADME.mdaction.ymldocs/architecture.mdexamples/README.mdlib/skill_bench.rblib/skill_bench/agent/react_agent/loop_runner.rblib/skill_bench/agent/react_agent/step.rblib/skill_bench/cli.rblib/skill_bench/cli/batch_result_printer.rblib/skill_bench/cli/help_printer.rblib/skill_bench/cli/init_command.rblib/skill_bench/cli/result_printer.rblib/skill_bench/cli/run_command.rblib/skill_bench/cli/validate_command.rblib/skill_bench/client.rblib/skill_bench/clients/all.rblib/skill_bench/clients/provider_schemas.rblib/skill_bench/clients/providers/mistral.rblib/skill_bench/commands/init.rblib/skill_bench/config.rblib/skill_bench/config/applier.rblib/skill_bench/config/defaults.rblib/skill_bench/config/facade_writers.rblib/skill_bench/config/store.rblib/skill_bench/models/config.rblib/skill_bench/output_formatter.rblib/skill_bench/package_verifier.rblib/skill_bench/services/agent_spawner_service.rblib/skill_bench/services/compare_option_parser.rblib/skill_bench/services/cost_calculator.rblib/skill_bench/services/html_formatter.rblib/skill_bench/services/json_formatter.rblib/skill_bench/services/junit_formatter.rblib/skill_bench/services/provider_resolver.rblib/skill_bench/services/response_cache.rblib/skill_bench/services/runner_service.rblib/skill_bench/services/summary_formatter.rblib/skill_bench/tools/run_command.rbtest/agent_eval/commands/init_test.rbtest/agent_eval/models/config_test.rbtest/agent_eval/output_formatter_test.rbtest/agent_eval/services/agent_spawner_service_test.rbtest/agent_eval/services/provider_resolver_test.rbtest/agent_eval/services/runner_service_test.rbtest/cli/batch_result_printer_test.rbtest/cli/init_command_test.rbtest/cli/run_command_test.rbtest/cli/validate_command_test.rbtest/client_caching_test.rbtest/evaluator/clients/providers/mistral_test.rbtest/evaluator/react_agent/loop_runner_test.rbtest/evaluator/react_agent/step_test.rbtest/evaluator/tools/run_command_test.rbtest/services/cost_calculator_test.rbtest/services/html_formatter_test.rbtest/services/json_formatter_test.rbtest/services/junit_formatter_test.rbtest/services/response_cache_test.rbtest/services/summary_formatter_test.rb
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
.github/PULL_REQUEST_TEMPLATE.md (1)
17-26: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winFix the
yard:coveragecommand syntax.Line 23 lists
bundle exec yard:coverage, but the standard invocation isbundle exec rake yard:coverage(orbundle exec rake yard:coveragedepending on the Rake task name). Withoutrake, most shells will attempt to run a binary namedyard:coverage, which does not exist.- [ ] `bundle exec yard:coverage` passes + [ ] `bundle exec rake yard:coverage` passes🤖 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 @.github/PULL_REQUEST_TEMPLATE.md around lines 17 - 26, The checklist entry for the YARD coverage command uses the wrong invocation syntax, so update the pull request template’s checklist item to use the Rake task form instead of a bare `yard:coverage` command. Make the change in the template text where the existing `bundle exec yard:coverage` appears, keeping it consistent with the other `bundle exec rake ...` entries and preserving the `yard:coverage` task name.lib/skill_bench/config/applier.rb (1)
40-47: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winExpose
command_argument_constraintsinJsonLoaderskill-bench.jsondrops this key today, so the new constraint never reachesApplierfor JSON-based configs. Add it to theslice(...)whitelist inlib/skill_bench/config/json_loader.rb.🤖 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 `@lib/skill_bench/config/applier.rb` around lines 40 - 47, `command_argument_constraints` is being applied in `Applier#apply_scalar_values`, but JSON configs never pass it through because `JsonLoader` filters it out. Update `JsonLoader`’s `slice(...)` whitelist to include `command_argument_constraints` so the value from `skill-bench.json` reaches `Applier` and gets assigned to the store.
🧹 Nitpick comments (9)
lib/skill_bench/cli/init_command.rb (1)
63-67: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
--mockmissing from "provider required" hint.
error_missing_provideronly enumeratesProviderSchemas.names, so users who omit a provider are never told--mockis a valid offline option.💡 Proposed fix
def error_missing_provider - providers = SkillBench::Clients::ProviderSchemas.names.map { |provider_name| "--#{provider_name}" }.join(', ') + providers = (SkillBench::Clients::ProviderSchemas.names.map { |provider_name| "--#{provider_name}" } + ['--mock']).join(', ') warn "Error: provider is required. Use one of: #{providers}" 1 end🤖 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 `@lib/skill_bench/cli/init_command.rb` around lines 63 - 67, The provider-required hint in error_missing_provider omits the offline --mock option because it only uses SkillBench::Clients::ProviderSchemas.names; update the provider list construction in this method to include --mock alongside the schema-based providers so the warning mentions every valid init option.examples/README.md (1)
7-9: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConfusing wording: "scaffold ... by hand".
The tip says running
skill-bench init --mockscaffolds the config "by hand as a fallback", which is contradictory — the command does it automatically; writing the JSON yourself would be the manual fallback.✏️ Proposed fix
-> Tip: run `skill-bench init --mock` to scaffold that offline mock config -> (`{"provider":"mock","max_execution_time":30}`) by hand as a fallback. +> Tip: run `skill-bench init --mock` to scaffold that offline mock config +> (`{"provider":"mock","max_execution_time":30}`), or write it by hand as a fallback.🤖 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 `@examples/README.md` around lines 7 - 9, The README tip in the mock config section uses contradictory wording by saying the CLI command “scaffold[s] ... by hand”; update the sentence in the documentation to clearly separate the automatic path from the manual fallback. Keep the reference to the `skill-bench init --mock` command, and rephrase the fallback so it explicitly refers to writing the JSON config manually instead of implying the command does it “by hand.”test/cli/validate_command_test.rb (1)
26-26: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReaching into
Models::Configinternal ivar from a different layer's test.
Models::Config.instance_variable_set(:@loaded, nil)directly pokes the memoization internals being introduced in the sibling "Config memoization" layer.instance_variable_setsilently no-ops if the ivar name changes (e.g., renamed during refactor), causing memoized config to leak across test runs without any failure signal — a brittle, hard-to-debug coupling.Suggested fix: expose a public reset hook
- Models::Config.instance_variable_set(:`@loaded`, nil) + Models::Config.reset_loaded_cache🤖 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 `@test/cli/validate_command_test.rb` at line 26, The test is tightly coupled to Models::Config’s internal memoization state by setting `@loaded` directly, which is brittle and can break silently if the implementation changes. Add a public reset hook on Models::Config to clear the cached config state, then update validate_command_test to call that helper instead of instance_variable_set so the test uses the supported API.lib/skill_bench/cli/validate_command.rb (1)
162-188: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse the shared env-override logic
provider_client_options/env_settingduplicate the sameSKILL_BENCH_<PROVIDER>_<SETTING>→<PROVIDER>_<SETTING>precedence already used byModels::Provider. Extracting one helper would keep validation and provider resolution in sync if that naming or order changes.🤖 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 `@lib/skill_bench/cli/validate_command.rb` around lines 162 - 188, The env override resolution in missing_provider_keys/provider_client_options/env_setting duplicates the same SKILL_BENCH_<PROVIDER>_<SETTING> then <PROVIDER>_<SETTING> precedence already used by Models::Provider. Refactor ValidateCommand to reuse a shared helper or the existing Models::Provider logic for building provider_client_options so validation stays in sync if the naming or precedence changes, and keep env_setting only as a thin delegate or remove it if no longer needed.test/agent_eval/services/provider_resolver_test.rb (1)
97-107: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPrefer Minitest's built-in
capture_ioover a custom stderr-capturing helper.
Minitest::Assertions#capture_ioalready redirects and restores both$stdout/$stderraround a block and returns the captured strings, making this private helper redundant.♻️ Proposed refactor
- private - - def capture_stderr - original = $stderr - $stderr = StringIO.new - yield - $stderr.string - ensure - $stderr = original - end - def write_mock_configAnd update call sites to use
capture_io { ... }.last(stderr is the second element of the returned array) in place ofcapture_stderr { ... }.🤖 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 `@test/agent_eval/services/provider_resolver_test.rb` around lines 97 - 107, Replace the custom `capture_stderr` helper in `provider_resolver_test.rb` with Minitest’s built-in `capture_io`, since it already handles redirecting and restoring both streams; update every `capture_stderr { ... }` call site to use `capture_io { ... }.last` so the tests still read captured stderr while removing the redundant private method.lib/skill_bench/models/config.rb (1)
27-50: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReflection-based cache reset is fragile; expose a public API instead.
The docstring instructs resetting by setting
@loadedtonildirectly, and bothtest/agent_eval/models/config_test.rbandtest/agent_eval/services/provider_resolver_test.rbdo this viainstance_variable_setat 5 call sites. If@loadedis ever renamed,instance_variable_setsilently no-ops (no error), and tests would start leaking stale cache across runs without any signal. A small public method removes the duplication and the encapsulation break.♻️ Proposed addition
+ # Clears the memoized config cache. Intended for test isolation. + # + # `@return` [void] + def self.reset_cache! + `@loaded` = {} + end + # Returns the configuration for a path, memoizing the parse per run.🤖 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 `@lib/skill_bench/models/config.rb` around lines 27 - 50, The cache reset for SkillBench::Models::Config is being done via direct ivar mutation and reflection, which is brittle and duplicated across tests. Add a small public reset API on SkillBench::Models::Config to clear the `@loaded` cache, update the loaded memoization docs to reference that API instead of setting the ivar directly, and replace all instance_variable_set calls in config and provider_resolver tests with the new method.lib/skill_bench/services/runner_service.rb (1)
177-221: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract shared usage-accumulation helpers to avoid triple duplication.
empty_usage/add_usage/token_counthere are identical to the same-named methods inlib/skill_bench/agent/react_agent/loop_runner.rb(lines 75-100), andtoken_count's symbol/string-key fallback logic is reimplemented again inlib/skill_bench/services/cost_calculator.rb(lines 82-88). Consider extracting a shared module (e.g.SkillBench::Support::TokenUsage) withempty,add, andcountso a future fix to usage-key handling doesn't need to be applied in three places.♻️ Sketch of a shared helper module
module SkillBench module Support module TokenUsage def self.empty { prompt_tokens: 0, completion_tokens: 0, total_tokens: 0 } end def self.add(total, usage) usage ||= {} { prompt_tokens: total[:prompt_tokens] + count(usage, :prompt_tokens), completion_tokens: total[:completion_tokens] + count(usage, :completion_tokens), total_tokens: total[:total_tokens] + count(usage, :total_tokens) } end def self.count(usage, key) (usage[key] || usage[key.to_s] || 0).to_i end end end end🤖 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 `@lib/skill_bench/services/runner_service.rb` around lines 177 - 221, The token-usage helper logic is duplicated across RunnerService, LoopRunner, and CostCalculator, so consolidate it into a shared helper/module (for example, SkillBench::Support::TokenUsage) with the equivalent of empty_usage, add_usage, and token_count. Update RunnerService#aggregate_usage and the other call sites to use the shared methods, preserving the symbol/string-key fallback behavior in token_count so future fixes only need to happen in one place.lib/skill_bench/output_formatter.rb (1)
141-171: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate token/cost rendering logic between formatters.
build_usage_lineformatsTokens: N | Est. Cost: $X.XXXXhere, and per graph evidenceServices::HtmlFormatterindependently builds the same "Tokens"/"Est. Cost" string with its ownKernel.format('$%.4f', cost)call (html_formatter.rb:114). Consider extracting a shared helper (e.g., a smallServices::UsageFormatteror module method) that both the human and HTML formatters call, so cost-formatting/precision stays consistent if it ever changes.🤖 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 `@lib/skill_bench/output_formatter.rb` around lines 141 - 171, The token/cost summary formatting is duplicated between OutputFormatter’s build_usage_line and Services::HtmlFormatter, which risks inconsistent formatting if either changes. Extract the shared “Tokens / Est. Cost” rendering into a common helper (for example, a Services::UsageFormatter method) and have both build_usage_line and the HTML formatter call it so the cost precision and label stay aligned in one place.action.yml (1)
57-59: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider pinning the gem version for reproducible CI gates.
gem install ruby-skill-benchalways installs latest; an unpinned dependency could break consumer pipelines unexpectedly on a future release. Consider adding an optionalversioninput.🤖 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 `@action.yml` around lines 57 - 59, The ruby-skill-bench install step in the workflow is unpinned, so CI may change behavior when new gem releases are published. Update the workflow around the install step to support a versioned install, ideally by adding an optional version input and using it in the install command so consumers can pin ruby-skill-bench to a specific release. Keep the change localized to the action definition where the gem is installed.
🤖 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 `@action.yml`:
- Around line 61-73: Replace the direct GitHub expression interpolation in the
Run skill-bench step with shell variables passed via env:, because the current
use of inputs.* inside the bash script can be script-injected before bash
parsing. Update the run script in action.yml to read those values from
environment variables instead of expanding them inline, and keep the
argument-building logic in the skill-bench step so it uses the safe variables
when constructing run_args and invoking skill-bench.
In `@lib/skill_bench/cli/run_command.rb`:
- Around line 87-91: The run command is passing an unsupported `:summary` option
into the single-eval path, causing `Commands::Run.run` to reject the keyword.
Update `RunCommand#run_single` (and any option handoff it uses) to strip or
ignore `options[:summary]` before calling `Commands::Run.run`, keeping `summary`
only for the batch-run flow where it is actually handled.
- Line 78: BatchResultPrinter.call is still falling through to
OutputFormatter.format_batch for non-junit batch runs, so --all with html/json
does not use the expected structured output. Update
BatchResultPrinter.batch_output to explicitly handle :html and :json (using the
same batch-specific path as :junit, or equivalent), or otherwise restrict the
supported batch formats advertised by RunCommand so they only include formats
actually implemented.
In `@lib/skill_bench/cli/validate_command.rb`:
- Around line 140-169: Avoid instantiating the provider client in
missing_provider_keys, since Clients::BaseClient#initialize can raise on
unrelated config like base_url/endpoint and bypass the intended FAIL result.
Update check_provider_key/missing_provider_keys to inspect required credential
keys from provider metadata or a non-instantiating helper instead of
Clients::ProviderRegistry.for(...).new(...) and send(:missing_config_keys), so
the CLI stays decoupled from provider-private internals.
In `@lib/skill_bench/client.rb`:
- Around line 38-46: `Client.call` is building a cache key in
`Services::ResponseCache.key` without all request-affecting provider config, so
different provider endpoints can collide. Update the cache key generation in
`Client.call` to also include the relevant `ProviderConfig` fields that change
the actual request, such as `base_url`, `request_path`, `endpoint`, `location`,
`project_id`, and `api_version`, alongside the existing `resolved`, `model`,
`system_prompt`, `messages`, `tools`, and `temperature` values.
In `@lib/skill_bench/services/json_formatter.rb`:
- Around line 27-35: The with_usage_fields method in JsonFormatter still allows
an explicit nil tokens value to overwrite the computed fallback, so fix the
merge behavior there by preventing nil :tokens from replacing the derived usage
data or by merging in the opposite order. Update the logic around
result[:tokens], result.dig(:response, :tokens), and EMPTY_USAGE so the output
always preserves a non-nil tokens value, and add a test covering the tokens: nil
case.
In `@lib/skill_bench/services/response_cache.rb`:
- Around line 68-89: ResponseCache#fetch is not thread-safe, so concurrent
Parallel.map callers can race between key? and []= and trigger duplicate
requests. Update the shared cache access in fetch (and any related store
helpers) to serialize the read/write around the per-process store, using a mutex
or equivalent synchronization on the store in ResponseCache so only one thread
computes and stores a missing value per key.
In `@lib/skill_bench/tools/run_command.rb`:
- Around line 96-104: `arguments_permitted?` is failing open because it looks up
`SkillBench::Config.command_argument_constraints` using a String `base_cmd`
while the config may contain symbol keys. Update
`RunCommand.arguments_permitted?` to normalize the lookup key (or normalize keys
in `SkillBench::Config.command_argument_constraints`), so
`constraints[base_cmd]` finds symbol-keyed entries consistently and forbidden
arguments are actually checked. Keep the fix centered on `arguments_permitted?`
and the config accessor used by it.
In `@test/evaluator/tools/run_command_test.rb`:
- Around line 103-134: The new constraint tests only cover the facade path and
miss the JSON-loading/key-type issues; update the tests around RunCommand.call
and SkillBench::Config so they load command_argument_constraints from a real
skill-bench.json via Config::JsonLoader after Config.reset/load_from_file
instead of assigning the hash directly. Add a regression case that uses the
loaded config to verify the constraint is enforced, so the behavior exercised
matches the bug locations in Config::Applier and RunCommand.
---
Outside diff comments:
In @.github/PULL_REQUEST_TEMPLATE.md:
- Around line 17-26: The checklist entry for the YARD coverage command uses the
wrong invocation syntax, so update the pull request template’s checklist item to
use the Rake task form instead of a bare `yard:coverage` command. Make the
change in the template text where the existing `bundle exec yard:coverage`
appears, keeping it consistent with the other `bundle exec rake ...` entries and
preserving the `yard:coverage` task name.
In `@lib/skill_bench/config/applier.rb`:
- Around line 40-47: `command_argument_constraints` is being applied in
`Applier#apply_scalar_values`, but JSON configs never pass it through because
`JsonLoader` filters it out. Update `JsonLoader`’s `slice(...)` whitelist to
include `command_argument_constraints` so the value from `skill-bench.json`
reaches `Applier` and gets assigned to the store.
---
Nitpick comments:
In `@action.yml`:
- Around line 57-59: The ruby-skill-bench install step in the workflow is
unpinned, so CI may change behavior when new gem releases are published. Update
the workflow around the install step to support a versioned install, ideally by
adding an optional version input and using it in the install command so
consumers can pin ruby-skill-bench to a specific release. Keep the change
localized to the action definition where the gem is installed.
In `@examples/README.md`:
- Around line 7-9: The README tip in the mock config section uses contradictory
wording by saying the CLI command “scaffold[s] ... by hand”; update the sentence
in the documentation to clearly separate the automatic path from the manual
fallback. Keep the reference to the `skill-bench init --mock` command, and
rephrase the fallback so it explicitly refers to writing the JSON config
manually instead of implying the command does it “by hand.”
In `@lib/skill_bench/cli/init_command.rb`:
- Around line 63-67: The provider-required hint in error_missing_provider omits
the offline --mock option because it only uses
SkillBench::Clients::ProviderSchemas.names; update the provider list
construction in this method to include --mock alongside the schema-based
providers so the warning mentions every valid init option.
In `@lib/skill_bench/cli/validate_command.rb`:
- Around line 162-188: The env override resolution in
missing_provider_keys/provider_client_options/env_setting duplicates the same
SKILL_BENCH_<PROVIDER>_<SETTING> then <PROVIDER>_<SETTING> precedence already
used by Models::Provider. Refactor ValidateCommand to reuse a shared helper or
the existing Models::Provider logic for building provider_client_options so
validation stays in sync if the naming or precedence changes, and keep
env_setting only as a thin delegate or remove it if no longer needed.
In `@lib/skill_bench/models/config.rb`:
- Around line 27-50: The cache reset for SkillBench::Models::Config is being
done via direct ivar mutation and reflection, which is brittle and duplicated
across tests. Add a small public reset API on SkillBench::Models::Config to
clear the `@loaded` cache, update the loaded memoization docs to reference that
API instead of setting the ivar directly, and replace all instance_variable_set
calls in config and provider_resolver tests with the new method.
In `@lib/skill_bench/output_formatter.rb`:
- Around line 141-171: The token/cost summary formatting is duplicated between
OutputFormatter’s build_usage_line and Services::HtmlFormatter, which risks
inconsistent formatting if either changes. Extract the shared “Tokens / Est.
Cost” rendering into a common helper (for example, a Services::UsageFormatter
method) and have both build_usage_line and the HTML formatter call it so the
cost precision and label stay aligned in one place.
In `@lib/skill_bench/services/runner_service.rb`:
- Around line 177-221: The token-usage helper logic is duplicated across
RunnerService, LoopRunner, and CostCalculator, so consolidate it into a shared
helper/module (for example, SkillBench::Support::TokenUsage) with the equivalent
of empty_usage, add_usage, and token_count. Update RunnerService#aggregate_usage
and the other call sites to use the shared methods, preserving the
symbol/string-key fallback behavior in token_count so future fixes only need to
happen in one place.
In `@test/agent_eval/services/provider_resolver_test.rb`:
- Around line 97-107: Replace the custom `capture_stderr` helper in
`provider_resolver_test.rb` with Minitest’s built-in `capture_io`, since it
already handles redirecting and restoring both streams; update every
`capture_stderr { ... }` call site to use `capture_io { ... }.last` so the tests
still read captured stderr while removing the redundant private method.
In `@test/cli/validate_command_test.rb`:
- Line 26: The test is tightly coupled to Models::Config’s internal memoization
state by setting `@loaded` directly, which is brittle and can break silently if
the implementation changes. Add a public reset hook on Models::Config to clear
the cached config state, then update validate_command_test to call that helper
instead of instance_variable_set so the test uses the supported API.
🪄 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: Repository UI (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 1f1025e8-27c3-4a38-be27-729fd16c0471
📒 Files selected for processing (66)
.github/ISSUE_TEMPLATE/bug_report.md.github/ISSUE_TEMPLATE/config.yml.github/ISSUE_TEMPLATE/feature_request.md.github/PULL_REQUEST_TEMPLATE.md.reek.ymlCHANGELOG.mdCODE_OF_CONDUCT.mdREADME.mdaction.ymldocs/architecture.mdexamples/README.mdlib/skill_bench.rblib/skill_bench/agent/react_agent/loop_runner.rblib/skill_bench/agent/react_agent/step.rblib/skill_bench/cli.rblib/skill_bench/cli/batch_result_printer.rblib/skill_bench/cli/help_printer.rblib/skill_bench/cli/init_command.rblib/skill_bench/cli/result_printer.rblib/skill_bench/cli/run_command.rblib/skill_bench/cli/validate_command.rblib/skill_bench/client.rblib/skill_bench/clients/all.rblib/skill_bench/clients/provider_schemas.rblib/skill_bench/clients/providers/mistral.rblib/skill_bench/commands/init.rblib/skill_bench/config.rblib/skill_bench/config/applier.rblib/skill_bench/config/defaults.rblib/skill_bench/config/facade_writers.rblib/skill_bench/config/store.rblib/skill_bench/models/config.rblib/skill_bench/output_formatter.rblib/skill_bench/package_verifier.rblib/skill_bench/services/agent_spawner_service.rblib/skill_bench/services/compare_option_parser.rblib/skill_bench/services/cost_calculator.rblib/skill_bench/services/html_formatter.rblib/skill_bench/services/json_formatter.rblib/skill_bench/services/junit_formatter.rblib/skill_bench/services/provider_resolver.rblib/skill_bench/services/response_cache.rblib/skill_bench/services/runner_service.rblib/skill_bench/services/summary_formatter.rblib/skill_bench/tools/run_command.rbtest/agent_eval/commands/init_test.rbtest/agent_eval/models/config_test.rbtest/agent_eval/output_formatter_test.rbtest/agent_eval/services/agent_spawner_service_test.rbtest/agent_eval/services/provider_resolver_test.rbtest/agent_eval/services/runner_service_test.rbtest/cli/batch_result_printer_test.rbtest/cli/init_command_test.rbtest/cli/run_command_test.rbtest/cli/validate_command_test.rbtest/client_caching_test.rbtest/evaluator/clients/providers/mistral_test.rbtest/evaluator/react_agent/loop_runner_test.rbtest/evaluator/react_agent/step_test.rbtest/evaluator/tools/run_command_test.rbtest/services/cost_calculator_test.rbtest/services/html_formatter_test.rbtest/services/json_formatter_test.rbtest/services/junit_formatter_test.rbtest/services/response_cache_test.rbtest/services/summary_formatter_test.rb
🛑 Comments failed to post (9)
action.yml (1)
61-73: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Use
env:instead of direct${{ }}interpolation in therun:script.Quoting
${{ inputs.* }}inside the script does not prevent injection — GitHub substitutes the expression into the script text before bash parses it, so a value like"; rm -rf / #can break out of the quotes and execute arbitrary commands on the runner. This is GitHub's documented "script injection" pattern and the recommended mitigation is to pass values throughenv:and reference them as shell variables.🔒️ Proposed fix
- name: Run skill-bench shell: bash + env: + SB_EVALS_DIR: ${{ inputs.evals-dir }} + SB_FORMAT: ${{ inputs.format }} + SB_SKILL: ${{ inputs.skill }} + SB_ARGS: ${{ inputs.args }} # A non-zero exit here fails the step — that is the CI gate. run: | - run_args=(run --all --evals-dir "${{ inputs.evals-dir }}" --format "${{ inputs.format }}") - if [ -n "${{ inputs.skill }}" ]; then - run_args+=(--skill "${{ inputs.skill }}") + run_args=(run --all --evals-dir "$SB_EVALS_DIR" --format "$SB_FORMAT") + if [ -n "$SB_SKILL" ]; then + run_args+=(--skill "$SB_SKILL") fi - if [ -n "${{ inputs.args }}" ]; then + if [ -n "$SB_ARGS" ]; then # Intentionally unquoted so multiple extra flags split into words. - run_args+=(${{ inputs.args }}) + run_args+=($SB_ARGS) fi skill-bench "${run_args[@]}"📝 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.- name: Run skill-bench shell: bash env: SB_EVALS_DIR: ${{ inputs.evals-dir }} SB_FORMAT: ${{ inputs.format }} SB_SKILL: ${{ inputs.skill }} SB_ARGS: ${{ inputs.args }} # A non-zero exit here fails the step — that is the CI gate. run: | run_args=(run --all --evals-dir "$SB_EVALS_DIR" --format "$SB_FORMAT") if [ -n "$SB_SKILL" ]; then run_args+=(--skill "$SB_SKILL") fi if [ -n "$SB_ARGS" ]; then # Intentionally unquoted so multiple extra flags split into words. run_args+=($SB_ARGS) fi skill-bench "${run_args[@]}"🤖 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 `@action.yml` around lines 61 - 73, Replace the direct GitHub expression interpolation in the Run skill-bench step with shell variables passed via env:, because the current use of inputs.* inside the bash script can be script-injected before bash parsing. Update the run script in action.yml to read those values from environment variables instead of expanding them inline, and keep the argument-building logic in the skill-bench step so it uses the safe variables when constructing run_args and invoking skill-bench.lib/skill_bench/cli/run_command.rb (2)
78-78: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash rg -n "def self.format_batch" lib/skill_bench/output_formatter.rb lib/skill_bench/services/html_formatter.rbRepository: igmarin/ruby-skill-bench
Length of output: 238
🏁 Script executed:
#!/bin/bash set -euo pipefail printf '\n== batch_result_printer ==\n' sed -n '1,220p' lib/skill_bench/services/batch_result_printer.rb printf '\n== output_formatter ==\n' sed -n '1,220p' lib/skill_bench/output_formatter.rb printf '\n== html_formatter ==\n' sed -n '1,240p' lib/skill_bench/services/html_formatter.rb printf '\n== run command ==\n' sed -n '1,220p' lib/skill_bench/cli/run_command.rb printf '\n== commands run ==\n' fd -a 'run.rb' lib/skill_bench && sed -n '1,220p' lib/skill_bench/commands/run.rb printf '\n== compare option parser ==\n' sed -n '1,220p' lib/skill_bench/services/compare_option_parser.rbRepository: igmarin/ruby-skill-bench
Length of output: 282
🏁 Script executed:
#!/bin/bash set -euo pipefail printf '\n== lib/skill_bench tree ==\n' find lib/skill_bench -maxdepth 4 -type f | sort printf '\n== target identifiers ==\n' rg -n "BatchResultPrinter|OutputFormatter|HtmlFormatter|CompareOptionParser|class RunCommand|module Commands|def self.format_batch|summary" lib/skill_benchRepository: igmarin/ruby-skill-bench
Length of output: 17085
🏁 Script executed:
#!/bin/bash set -euo pipefail printf '\n== cli/batch_result_printer.rb ==\n' sed -n '1,220p' lib/skill_bench/cli/batch_result_printer.rb printf '\n== output_formatter.rb ==\n' sed -n '1,240p' lib/skill_bench/output_formatter.rb printf '\n== services/html_formatter.rb ==\n' sed -n '1,260p' lib/skill_bench/services/html_formatter.rb printf '\n== commands/run.rb ==\n' sed -n '1,220p' lib/skill_bench/commands/run.rbRepository: igmarin/ruby-skill-bench
Length of output: 21040
🏁 Script executed:
#!/bin/bash set -euo pipefail sed -n '1,220p' lib/skill_bench/cli/run_command.rbRepository: igmarin/ruby-skill-bench
Length of output: 4161
Batch
--all --format html/jsonstill renders human output.BatchResultPrinter.batch_outputonly handles:junit; every other format falls through toOutputFormatter.format_batch, so the new batch help text advertises modes this path doesn’t implement. Add explicit:html/:jsonhandling here or narrow the documented batch formats.🤖 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 `@lib/skill_bench/cli/run_command.rb` at line 78, BatchResultPrinter.call is still falling through to OutputFormatter.format_batch for non-junit batch runs, so --all with html/json does not use the expected structured output. Update BatchResultPrinter.batch_output to explicitly handle :html and :json (using the same batch-specific path as :junit, or equivalent), or otherwise restrict the supported batch formats advertised by RunCommand so they only include formats actually implemented.
87-91: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash ast-grep run --pattern 'def self.run($$$)' --lang ruby lib/skill_bench/commands/run.rbRepository: igmarin/ruby-skill-bench
Length of output: 717
🏁 Script executed:
#!/bin/bash set -euo pipefail printf '\n## run_command.rb\n' sed -n '1,180p' lib/skill_bench/cli/run_command.rb printf '\n## run.rb\n' sed -n '1,120p' lib/skill_bench/commands/run.rbRepository: igmarin/ruby-skill-bench
Length of output: 5092
Strip
:summarybefore delegating single-eval runs.run_singleforwardsoptions[:summary]toCommands::Run.run, but that method only acceptseval_name,skill_names,pack, andregistry_manifest, soskill-bench run <eval> --summary ...raisesArgumentError: unknown keyword: :summary.🤖 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 `@lib/skill_bench/cli/run_command.rb` around lines 87 - 91, The run command is passing an unsupported `:summary` option into the single-eval path, causing `Commands::Run.run` to reject the keyword. Update `RunCommand#run_single` (and any option handoff it uses) to strip or ignore `options[:summary]` before calling `Commands::Run.run`, keeping `summary` only for the batch-run flow where it is actually handled.lib/skill_bench/cli/validate_command.rb (1)
140-169: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # Inspect provider client constructors/missing_config_keys for eager validation or network calls fd -t f . lib/skill_bench/clients/providers --exec cat -n {} \; rg -n 'def initialize' lib/skill_bench/clients --type=ruby -A 10 rg -n 'missing_config_keys' lib/skill_bench --type=ruby -A 5Repository: igmarin/ruby-skill-bench
Length of output: 36102
🏁 Script executed:
#!/bin/bash set -euo pipefail sed -n '1,220p' lib/skill_bench/clients/base_client.rb | cat -n printf '\n--- provider_config ---\n' sed -n '1,220p' lib/skill_bench/clients/provider_config.rb | cat -n printf '\n--- provider_registry ---\n' sed -n '1,220p' lib/skill_bench/clients/provider_registry.rb | cat -n printf '\n--- validate_command slice ---\n' sed -n '120,190p' lib/skill_bench/cli/validate_command.rb | cat -nRepository: igmarin/ruby-skill-bench
Length of output: 18416
🏁 Script executed:
#!/bin/bash set -euo pipefail printf '%s\n' '--- validate_command top ---' sed -n '1,120p' lib/skill_bench/cli/validate_command.rb | cat -n printf '\n%s\n' '--- search for constructor overrides / network primitives in providers ---' rg -n 'def initialize|Net::HTTP|Faraday|HTTP\.|fetch\(|open\(' lib/skill_bench/clients/providers lib/skill_bench/clients -g '*.rb'Repository: igmarin/ruby-skill-bench
Length of output: 7928
Avoid constructing the provider just to inspect missing keys.
BaseClient#initializeruns local config validation, so a badbase_url/endpointcan escape this path and fall through to the top-levelError:rescue instead of a FAIL row.send(:missing_config_keys)also couples the CLI to provider-private internals.🤖 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 `@lib/skill_bench/cli/validate_command.rb` around lines 140 - 169, Avoid instantiating the provider client in missing_provider_keys, since Clients::BaseClient#initialize can raise on unrelated config like base_url/endpoint and bypass the intended FAIL result. Update check_provider_key/missing_provider_keys to inspect required credential keys from provider metadata or a non-instantiating helper instead of Clients::ProviderRegistry.for(...).new(...) and send(:missing_config_keys), so the CLI stays decoupled from provider-private internals.lib/skill_bench/client.rb (1)
38-46: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
set -e printf '\n== files ==\n' git ls-files | rg '^(lib/skill_bench/client\.rb|lib/.*/response_cache\.rb|lib/skill_bench/.+|spec/.+client.+|spec/.+response_cache.+)$' printf '\n== client outline ==\n' ast-grep outline lib/skill_bench/client.rb --view expanded printf '\n== response cache outline ==\n' fd -i 'response_cache.rb' lib specRepository: igmarin/ruby-skill-bench
Length of output: 6587
🏁 Script executed:
set -e sed -n '1,220p' lib/skill_bench/client.rb printf '\n---\n' fd -i 'response_cache.rb' lib spec -x sh -c 'echo "FILE: $1"; sed -n "1,240p" "$1"' sh {}Repository: igmarin/ruby-skill-bench
Length of output: 7130
🏁 Script executed:
set -e printf 'CLIENT\n' sed -n '1,220p' lib/skill_bench/client.rb printf '\nCACHE\n' fd -i 'response_cache.rb' lib spec -x sh -c 'echo "--- $1 ---"; sed -n "1,260p" "$1"' sh {} printf '\nREFERENCES\n' rg -n "ResponseCache|SKILL_BENCH_CACHE|cache_key|cache\(" lib specRepository: igmarin/ruby-skill-bench
Length of output: 8018
🏁 Script executed:
rg -n "\b(max_tokens|top_p|stop|response_format|seed|temperature|tools|model)\b" lib/skill_bench/clients lib/skill_bench/client.rb lib/skill_bench/servicesRepository: igmarin/ruby-skill-bench
Length of output: 9134
🏁 Script executed:
sed -n '1,220p' lib/skill_bench/clients/base_client.rb printf '\n---\n' sed -n '1,240p' lib/skill_bench/clients/request_builder.rb printf '\n---\n' sed -n '1,240p' lib/skill_bench/clients/provider_schemas.rb printf '\n---\n' sed -n '1,240p' lib/skill_bench/clients/providers/openai.rbRepository: igmarin/ruby-skill-bench
Length of output: 12255
🏁 Script executed:
sed -n '1,220p' lib/skill_bench/clients/provider_config.rb printf '\n---\n' sed -n '1,220p' lib/skill_bench/clients/providers/anthropic.rb printf '\n---\n' sed -n '1,220p' lib/skill_bench/clients/providers/azure_openai.rb printf '\n---\n' sed -n '1,220p' lib/skill_bench/clients/providers/ollama.rb printf '\n---\n' sed -n '1,220p' lib/skill_bench/clients/providers/opencode.rbRepository: igmarin/ruby-skill-bench
Length of output: 15656
🏁 Script executed:
rg -n "def request_body|def base_url|def request_path|`@api_version`|`@location`|`@project_id`|`@endpoint`|`@model`|`@tools`|max_tokens|top_p|stop|response_format|seed" lib/skill_bench/clients/providersRepository: igmarin/ruby-skill-bench
Length of output: 3556
🏁 Script executed:
sed -n '1,240p' lib/skill_bench/clients/provider_config.rb printf '\n---\n' sed -n '1,240p' lib/skill_bench/clients/providers/anthropic.rb printf '\n---\n' sed -n '1,220p' lib/skill_bench/clients/providers/azure_openai.rb printf '\n---\n' sed -n '1,220p' lib/skill_bench/clients/providers/ollama.rb printf '\n---\n' sed -n '1,220p' lib/skill_bench/clients/providers/opencode.rbRepository: igmarin/ruby-skill-bench
Length of output: 15656
Include request-affecting provider config in the cache key.
Client.callcaches only onprovider,model,system_prompt,messages,tools, andtemperature, butProviderConfigalso letsbase_url,request_path,endpoint,location,project_id, andapi_versionvary the actual request. Different endpoints/configs can therefore reuse the same cached response.🤖 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 `@lib/skill_bench/client.rb` around lines 38 - 46, `Client.call` is building a cache key in `Services::ResponseCache.key` without all request-affecting provider config, so different provider endpoints can collide. Update the cache key generation in `Client.call` to also include the relevant `ProviderConfig` fields that change the actual request, such as `base_url`, `request_path`, `endpoint`, `location`, `project_id`, and `api_version`, alongside the existing `resolved`, `model`, `system_prompt`, `messages`, `tools`, and `temperature` values.lib/skill_bench/services/json_formatter.rb (1)
27-35: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash set -euo pipefail printf 'FILES:\n' git ls-files 'lib/skill_bench/services/json_formatter.rb' 'spec' 'test' | sed -n '1,200p' printf '\nOUTLINE json_formatter.rb:\n' ast-grep outline lib/skill_bench/services/json_formatter.rb --view expanded || true printf '\nRELEVANT TEST SEARCH:\n' rg -n "with_usage_fields|tokens: nil|EMPTY_USAGE|pretty_generate|usage" lib spec test -S || true printf '\nFILE CONTENT (json_formatter.rb):\n' nl -ba lib/skill_bench/services/json_formatter.rb | sed -n '1,220p'Repository: igmarin/ruby-skill-bench
Length of output: 27638
🏁 Script executed:
#!/bin/bash set -euo pipefail printf 'json_formatter.rb:\n' sed -n '1,220p' lib/skill_bench/services/json_formatter.rb printf '\njson_formatter tests:\n' sed -n '1,240p' test/services/json_formatter_test.rb printf '\ncall sites that build result hashes with tokens:\n' rg -n "tokens:" lib test -S | sed -n '1,240p'Repository: igmarin/ruby-skill-bench
Length of output: 9771
Merge order can still emit
tokens: nullinlib/skill_bench/services/json_formatter.rb:27-35.defaults.merge(result)lets an explicittokens: niloverride the computed zeroed fallback, so the JSON shape guarantee still breaks for that input. Flip the merge order or ignore nil:tokens, and add a test fortokens: nil.🤖 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 `@lib/skill_bench/services/json_formatter.rb` around lines 27 - 35, The with_usage_fields method in JsonFormatter still allows an explicit nil tokens value to overwrite the computed fallback, so fix the merge behavior there by preventing nil :tokens from replacing the derived usage data or by merging in the opposite order. Update the logic around result[:tokens], result.dig(:response, :tokens), and EMPTY_USAGE so the output always preserves a non-nil tokens value, and add a test covering the tokens: nil case.lib/skill_bench/services/response_cache.rb (1)
68-89: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # Check for thread usage around batch/eval execution that could call Client.call concurrently rg -nP --type=ruby '\bThread\.new\b|\bThread\b' lib/skill_benchRepository: igmarin/ruby-skill-bench
Length of output: 280
🏁 Script executed:
#!/bin/bash set -euo pipefail # Map relevant files first printf '\n== response_cache outline ==\n' ast-grep outline lib/skill_bench/services/response_cache.rb --view expanded || true printf '\n== batch runner outline ==\n' ast-grep outline lib/skill_bench/services/batch_runner_service.rb --view expanded || true printf '\n== search for cache usage ==\n' rg -n "ResponseCache|response_cache|fetch\(" lib/skill_benchRepository: igmarin/ruby-skill-bench
Length of output: 4593
🏁 Script executed:
#!/bin/bash set -euo pipefail # Inspect the batch runner and response cache implementations in manageable slices. for f in lib/skill_bench/services/response_cache.rb lib/skill_bench/services/batch_runner_service.rb; do echo "== $f ==" wc -l "$f" done echo "== response_cache lines 1-220 ==" sed -n '1,220p' lib/skill_bench/services/response_cache.rb | cat -n echo "== batch_runner_service lines 1-260 ==" sed -n '1,260p' lib/skill_bench/services/batch_runner_service.rb | cat -nRepository: igmarin/ruby-skill-bench
Length of output: 10436
🏁 Script executed:
#!/bin/bash set -euo pipefail # Inspect the command runner thread usage that was found, and whether it shares cache state. sed -n '140,230p' lib/skill_bench/tools/run_command.rb | cat -n echo "== any references to BatchRunnerService call sites ==" rg -n "BatchRunnerService\.new|BatchRunnerService\b" lib/skill_benchRepository: igmarin/ruby-skill-bench
Length of output: 5246
🏁 Script executed:
#!/bin/bash set -euo pipefail # Look for actual concurrency in the service layer and cache wiring. printf '\n== thread / parallel primitives ==\n' rg -n --type=ruby '\bThread\.new\b|\bParallel\b|\bConcurrent::|\bmutex\b|\bQueue\b|\bfork\b' lib/skill_bench printf '\n== response cache call sites and ownership ==\n' rg -n --type=ruby 'ResponseCache|response_cache' lib/skill_bench printf '\n== batch runner / eval runner call graph hints ==\n' rg -n --type=ruby 'BatchRunnerService|EvalRunner|RunnerService|run_batch|batch' lib/skill_bench/services lib/skill_benchRepository: igmarin/ruby-skill-bench
Length of output: 10597
🏁 Script executed:
#!/bin/bash set -euo pipefail # Inspect the response cache and the code paths that might share it. echo "== response_cache.rb ==" wc -l lib/skill_bench/services/response_cache.rb sed -n '1,220p' lib/skill_bench/services/response_cache.rb | cat -n echo "== batch_runner_service.rb ==" wc -l lib/skill_bench/services/batch_runner_service.rb sed -n '1,260p' lib/skill_bench/services/batch_runner_service.rb | cat -n echo "== thread/parallel primitives in lib/skill_bench ==" rg -n --type=ruby '\bThread\.new\b|\bParallel\b|\bConcurrent::|\bMutex\b|\bQueue\b|\bfork\b' lib/skill_bench echo "== response cache call sites ==" rg -n --type=ruby 'ResponseCache|response_cache|fetch\(' lib/skill_bench echo "== batch/eval runner references ==" rg -n --type=ruby 'BatchRunnerService|EvalRunner|RunnerService|run_batch|batch' lib/skill_bench/services lib/skill_benchRepository: igmarin/ruby-skill-bench
Length of output: 24542
Serialize the shared response cache
ResponseCacheis process-wide, and batch/runner paths callClientfromParallel.mapthreads. The currentkey?/[]=sequence can race on the same digest, causing duplicate LLM requests and defeating dedup under concurrent runs.🤖 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 `@lib/skill_bench/services/response_cache.rb` around lines 68 - 89, ResponseCache#fetch is not thread-safe, so concurrent Parallel.map callers can race between key? and []= and trigger duplicate requests. Update the shared cache access in fetch (and any related store helpers) to serialize the read/write around the per-process store, using a mutex or equivalent synchronization on the store in ResponseCache so only one thread computes and stores a missing value per key.lib/skill_bench/tools/run_command.rb (1)
96-104: 🔒 Security & Privacy | 🔴 Critical | ⚡ Quick win
arguments_permitted?fails open on symbol-keyed constraints.
constraints[base_cmd]looks up withbase_cmd(always aString, fromcommand.shellsplit). ButSkillBench::Config.command_argument_constraintscan hold symbol keys — either set directly viaconfig.command_argument_constraints = { echo: ['-n'] }in a Rubysetupblock, or (once the JsonLoader gap noted inapplier.rbis fixed) loaded from JSON viaJSON.parse(..., symbolize_names: true). In either caseconstraints[base_cmd]returnsnil,disallowed.nil?is true, and the method returnstrue— silently permitting the very arguments the constraint was meant to block. This is a fail-open bug in a security control, not just an unreachable feature.🐛 Proposed fix
def self.arguments_permitted?(base_cmd, argv) constraints = SkillBench::Config.command_argument_constraints return true if constraints.nil? || constraints.empty? - disallowed = constraints[base_cmd] + disallowed = constraints[base_cmd] || constraints[base_cmd.to_sym] return true if disallowed.nil? || disallowed.empty? argv.drop(1).none? { |arg| disallowed.any? { |bad| arg.include?(bad.to_s) } } endAlternatively, normalize keys once at the
Config.command_argument_constraintsaccessor (store.command_argument_constraints&.transform_keys(&:to_s) || {}) so every consumer gets a consistent key type.📝 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.def self.arguments_permitted?(base_cmd, argv) constraints = SkillBench::Config.command_argument_constraints return true if constraints.nil? || constraints.empty? disallowed = constraints[base_cmd] || constraints[base_cmd.to_sym] return true if disallowed.nil? || disallowed.empty? argv.drop(1).none? { |arg| disallowed.any? { |bad| arg.include?(bad.to_s) } } end🤖 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 `@lib/skill_bench/tools/run_command.rb` around lines 96 - 104, `arguments_permitted?` is failing open because it looks up `SkillBench::Config.command_argument_constraints` using a String `base_cmd` while the config may contain symbol keys. Update `RunCommand.arguments_permitted?` to normalize the lookup key (or normalize keys in `SkillBench::Config.command_argument_constraints`), so `constraints[base_cmd]` finds symbol-keyed entries consistently and forbidden arguments are actually checked. Keep the fix centered on `arguments_permitted?` and the config accessor used by it.test/evaluator/tools/run_command_test.rb (1)
103-134: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Tests only cover the Ruby-facade path, masking the JSON-loading and key-type bugs.
Both new constraint tests set
SkillBench::Config.command_argument_constraints = { 'echo' => ['-n'] }directly via the facade writer, with String keys matchingbase_cmd's type. This never exercises loading constraints from an actualskill-bench.jsonfile throughConfig::JsonLoader, which is the scenario where the missing-slice-key and symbol/string key-mismatch bugs (flagged inlib/skill_bench/config/applier.rbandlib/skill_bench/tools/run_command.rb) actually manifest. Once those are fixed, add a regression test that writes a JSON config withcommand_argument_constraintsand verifiesRunCommand.callhonors it afterConfig.reset/load_from_file.🤖 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 `@test/evaluator/tools/run_command_test.rb` around lines 103 - 134, The new constraint tests only cover the facade path and miss the JSON-loading/key-type issues; update the tests around RunCommand.call and SkillBench::Config so they load command_argument_constraints from a real skill-bench.json via Config::JsonLoader after Config.reset/load_from_file instead of assigning the hash directly. Add a regression case that uses the loaded config to verify the constraint is enforced, so the behavior exercised matches the bug locations in Config::Applier and RunCommand.
…mbol keys JsonLoader's slice whitelist dropped command_argument_constraints, so the #21 arg-constraint feature was dead from skill-bench.json. Adds it to the whitelist, and makes RunCommand.arguments_permitted? look up the command under both String and Symbol keys (JSON loads symbol keys via symbolize_names). Adds a regression test that loads the constraint from a real skill-bench.json.
…ock hint run_single now strips batch-only keys (summary/all/evals_dir) before Commands::Run.run, fixing a keyword error on 'run <eval> --summary'. Batch --format json now emits the JSON summary instead of human text. ValidateCommand rescues provider-client init errors into a FAIL result (decoupled from base_url validation). init's missing-provider hint lists --mock.
Cache key now includes request-affecting provider config (base_url/request_path/ endpoint/location/project_id/api_version) so distinct endpoints don't collide. ResponseCache serializes store access with a mutex (computing off-lock so distinct keys stay concurrent). JsonFormatter no longer lets an explicit tokens: nil overwrite the usage fallback. ParameterLists stops counting kwargs.
action.yml now passes inputs through env vars instead of interpolating GitHub expressions into the bash script. PR template uses 'bundle exec rake yard:coverage'; examples README clarifies the init --mock vs by-hand fallback wording.
1 participant
v1.2.0 — quality program release
Closes the v1.2.0 milestone: every open issue in the issue tab and the
ruby-skill-benchproject board, each delivered with YARD docs and Minitest coverage.Features
validate/doctorpre-flight command —CriteriaValidator+skill-bench.jsonschema check + provider-key check, no eval/network (Closes skill-bench validate/doctor command (CriteriaValidator + config JSON-schema + provider key check) #45)--format html— self-contained HTML report (delta table + iteration timeline), fully escaped (Closes HTML report formatter (:html) #44)--cache/SKILL_BENCH_CACHE, content-addressed;compare's repeated baseline reuses the cache (Closes Response caching/memoization; compare re-runs an identical baseline #42)--summaryJSON gate, per-eval<testcase>JUnit aggregation, and a top-level compositeaction.yml(uses: igmarin/ruby-skill-bench@v1) (Closes CI gate: per-eval testcase aggregation + --summary JSON + official action.yml #46)init --mockscaffolds an offline mock config (Closes Add skill-bench init --mock flag #53)Security
command_argument_constraintshook (Closes Allowlist only checks base token; document/constrain that allowed binaries grant arbitrary exec #21)Performance
skill-bench.jsonparsed at most once per run (mtime-memoized) (Closes Eliminate duplicate skill-bench.json parse across the two config systems #31)Fixed
{"provider":"mock"}no longer prints a misleading "Config load failed" warning (Closes Silence misleading "Config load failed" warning when mock is explicitly chosen #54)package:verifypointed at a stalesource_path_resolver.rbpath (pre-existing release-gate bug)Docs / community
CODE_OF_CONDUCT.md, issue templates, PR template (Closes Add CODE_OF_CONDUCT.md, .github/ISSUE_TEMPLATE/, PULL_REQUEST_TEMPLATE.md #38)docs/architecture.mdtrend filename +.baknote (Closes Update docs/architecture.md tree diagram (trend filename + .bak note) #39)[1.2.0]and README updated with all new commands, flags, the provider, and the ActionVerification
rake test→ 865 runs, 0 failures, 0 errors, 4 skipsrubocop,reek,rake yard:coverage,bundle-audit,rake package:build package:verify— all greenSummary by CodeRabbit
New Features
validate/doctorpre-flight check command.Bug Fixes
Documentation