feat: Add benchmarking harness with spot suite

[ilblackdragon]

Summary

• Adds a full benchmarking harness (benchmarks/ crate) for evaluating agent behavior against defined tasks with multi-criterion assertions
• Implements a spot suite with 21 scenarios across 5 categories: smoke tests, individual tool use, multi-tool chaining, robustness checks, and memory/persistence workflows
• Suite adapters for GAIA, SWE-bench, Tau-bench, and custom datasets included (adapters only, no bundled datasets)
• Results persisted as JSONL with aggregate run.json, supports resume, compare, and filtering by task/tag
• Each run tagged with model name and git commit hash for reproducibility
• Fixes forced tool-use nudge in agent loop that was corrupting conversations
• Fixes file tools returning canonicalized paths (breaks on macOS /tmp symlink)

Architecture

benchmarks/
├── Cargo.toml
├── spot.toml                    # Config for spot suite
├── data/spot.jsonl              # 21 scenario definitions
├── baselines/                   # Committed reference results
│   └── spot-gpt5.2-2c43b83/    # 100% baseline
└── src/
    ├── main.rs                  # CLI (run, results, compare)
    ├── runner.rs                # Orchestrator (sequential + parallel)
    ├── channel.rs               # BenchChannel (captures responses + tool calls)
    ├── instrumented_llm.rs      # Token/cost tracking wrapper
    ├── suite.rs                 # BenchSuite + BenchTask traits
    ├── scoring.rs               # LLM-as-judge scorer
    ├── config.rs                # TOML config + matrix entries
    ├── results.rs               # JSONL persistence + display
    ├── error.rs                 # Error types
    └── adapters/
        ├── mod.rs               # Suite registry
        ├── spot.rs              # SpotAssertions multi-criterion scoring
        ├── custom.rs            # Simple text-match adapter
        ├── gaia.rs              # GAIA benchmark adapter
        ├── swe_bench.rs         # SWE-bench adapter
        └── tau_bench.rs         # Tau-bench adapter

Spot Suite (21 tasks)

Category	Count	What it tests
Smoke	2	Basic conversation without tools
Tool	5	Individual tool usage (echo, time, json, shell, list_dir)
Chain	3	Multi-tool sequences (write+read, shell+json, time+echo)
Robust	3	Correct tool selection, avoiding unnecessary tools
Memory	8	File-based persistence: save/recall, append, update, multi-file

Results (GPT-5.2 @ 2c43b83)

Pass rate: 100.0% | Avg score: 1.000 | Tasks: 21/21 | Cost: $0.31 | Time: 111s

Agent fixes discovered via benchmarks

• Removed forced tool-use nudge (agent_loop.rs): the agentic loop was injecting "Please proceed and use the available tools" when the LLM responded without calling tools, corrupting conversations and forcing unnecessary tool calls
• Fixed file tool path canonicalization (file.rs): tools were returning /private/tmp/... instead of /tmp/... on macOS, confusing the LLM on subsequent file operations

Test plan

• cargo clippy -p ironclaw-bench --all-features clean
• cargo test -p ironclaw-bench passes (43 tests)
• cargo test -p ironclaw passes (879 tests)
• Full 21-task spot run: 100% pass rate on GPT-5.2
• Baseline committed to benchmarks/baselines/

🤖 Generated with Claude Code

[tribendu]

Benchmarking Harness PR Review

Summary

This PR introduces a comprehensive benchmarking harness for the IronClaw agent, supporting multiple standardized benchmarks (GAIA, Tau-bench, SWE-bench Pro) and custom JSONL suites. The architecture includes instrumented LLM providers, a headless benchmark channel, modular adapter system, and results tracking with resume capability. The implementation is well-structured with async execution, parallel task support, and multiple output formats.

Pros

• Modular adapter design: The BenchSuite trait allows easy integration of new benchmarks (GAIA, Tau-bench, SWE-bench, custom JSONL)
• Comprehensive instrumentation: InstrumentedLlm captures token counts, timing, costs, and tool call metrics transparently
• Well-defined metrics tracking: Tracks wall time, LLM calls, tokens, cost, tool calls, turns, and resource limits
• Resume capability: Can resume interrupted runs by tracking completed task IDs
• Headless channel approach: BenchChannel auto-approves tools and captures responses without user interaction
• Flexible scoring: Supports exact match, substring containment, regex matching with normalization
• Multiple output formats: Table, JSON, and CSV results for different use cases
• Parallel execution: Configurable parallelism with semaphore-based concurrency control
• Good test coverage: Unit tests for adapters, scoring, channel, and instrumentation

Concerns

Metrics Accuracy

1. Tool duration precision: BenchChannel sets duration_ms: 0 for tool calls (line 141), missing actual per-tool timing
2. LLM-as-judge not implemented: Custom suite LLM scoring returns hardcoded 0.5 partial score (line 168), limiting evaluation quality
3. SWE-bench scoring incomplete: Returns 0.25 partial credit placeholder instead of running test suites (lines 256-268), making metrics misleading

Evaluation Design

4. Multi-turn simulation incomplete: TauBenchSuite::next_user_message always returns None (line 165), not enabling true multi-turn evaluation
5. Task isolation overhead: Each task creates fresh Agent instance + instrumented LLM, which may not reflect real-world warm-start performance
6. Scoring performed post-hoc: Tasks run first, scored afterward; scoring errors don't prevent wasted computation

Methodology Issues

7. No statistical rigor: No confidence intervals, outliers detection, or statistical significance testing
8. Single execution per task: No repeated runs to account for non-deterministic behavior
9. Timeout handling: Tasks hitting timeout receive full 0.0 score, but partial progress isn't captured
10. No baseline comparisons: Only shows current run results, no easy way to track regression/improvement over time

Data Quality

11. Normalization limitations: normalize_answer strips punctuation but doesn't handle semantic equivalence (numbers vs words, synonyms)
12. No evaluation of reasoning quality: Only final answer is scored, intermediate reasoning steps aren't evaluated

Suggestions

• Add per-tool timing: Capture actual duration for ToolCompleted events in BenchChannel
• Implement LLM-as-judge: Use separate LLM call to evaluate responses on semantic correctness
• Complete SWE-bench scoring: Implement test patch application and test suite execution
• Enable multi-turn evaluation: Add LLM-based user persona simulation for Tau-bench scenarios
• Add statistical analysis: Compute confidence intervals, show percentile distributions, detect outliers
• Support repeated runs: Allow N executions per task with aggregation (mean, median, std dev)
• Score incrementally: Check scoring validity before expensive agent execution
• Add benchmark caching: Cache agent warm-start state to reduce per-task overhead
• Improve normalization: Add number/word equivalence handling, synonym normalization
• Add regression tracking: Store historical baselines, auto-generate comparison reports
• Partial credit for timeouts: Capture intermediate state before timeout for partial scoring
• Reasoning evaluation metrics: Track tool call chains, detect loops, evaluate tool selection quality
• Add resource usage tracking: Capture CPU/memory per task for efficiency metrics
• Implement dry-run mode: Validate benchmark setup without running expensive LLM calls
• Add golden dataset support: Store expected multi-turn conversations for validation

[ilblackdragon]

Review: feat: Add benchmarking harness for agent evaluation

Nice contribution -- well-structured benchmarking crate with good separation of concerns and solid test coverage (29 tests, all passing). The architecture (BenchSuite trait, adapter pattern, incremental JSONL output, resume support) is well thought out. Below are findings organized by severity.

---

Issues to address

1. load_tasks() called N+1 times during scoring (runner.rs:206-211)

After all tasks finish, the scoring loop calls self.suite.load_tasks().await? once per result to look up the task by ID. For a 500-task GAIA run, that's 500 file reads and full re-parses. This should be loaded once before the scoring loop and kept in a HashMap<String, BenchTask>:

// Before the scoring loop:
let task_map: HashMap<String, BenchTask> = self.suite.load_tasks().await?
    .into_iter()
    .map(|t| (t.id.clone(), t))
    .collect();

// Inside the loop:
if let Some(task) = task_map.get(&result.task_id) { ... }

2. setup_task() and teardown_task() never called (runner.rs)

The BenchSuite trait defines setup_task() and teardown_task() lifecycle hooks, and SweBenchSuite implements real setup_task() logic (git clone + checkout). But run_task_isolated() never calls either hook. SWE-bench tasks will fail silently because the repo workspace won't exist. The runner should call suite.setup_task(&task) before and suite.teardown_task(&task) after each task execution.

3. next_user_message() never called -- multi-turn not wired (runner.rs)

Similarly, TauBenchSuite implements next_user_message() and the trait is designed for multi-turn conversations, but the runner only injects a single prompt and then sends /quit after the first response. Multi-turn suites will only ever get single-turn evaluation. Consider either wiring the multi-turn loop or documenting this as an explicit limitation.

4. max_iterations config field is parsed but never used (config.rs:24, runner.rs)

BenchConfig::max_iterations is deserialized, tested, and has a default of 15, but it's never passed to the agent or used anywhere in the runner. Either wire it into the AgentConfig or remove it to avoid confusion.

5. tool_whitelist() never called (suite.rs:143, runner.rs)

MatrixEntry.tools and BenchSuite::tool_whitelist() are defined but never consulted by the runner. Tools registered in the runner are always the full builtin set + suite additional tools.

6. Concurrent JSONL writes are not safe (results.rs:118-126, runner.rs:188)

In parallel mode, multiple tokio tasks call append_task_result() concurrently. The function opens the file, appends, and closes on each call with no locking. While POSIX O_APPEND is atomic for writes below PIPE_BUF (4KB), JSON-serialized results can exceed that. Consider using a file lock (flock), a dedicated writer task with an mpsc channel, or a Mutex<File> shared across tasks.

---

Minor / style

7. .expect("semaphore closed") in production code (runner.rs:171)

Per the project CLAUDE.md: "Never use .unwrap() or .expect() in production code." This is in the parallel task execution path. Should be replaced with proper error handling (e.g., map to BenchError).

8. bench-results/ not in .gitignore

Running benchmarks will generate a bench-results/ directory at the repo root. Consider adding it to .gitignore to prevent accidental commits of large result files.

9. 12 compiler warnings

cargo check -p ironclaw-bench produces 12 warnings, mostly dead-code warnings for unused trait methods (name, setup_task, teardown_task, tool_whitelist, next_user_message) and the conversation field on TaskSubmission. These are consistent with issues #2-#5 above -- once the hooks are actually wired, most warnings will disappear. The remaining ones could be suppressed with #[allow(dead_code)] where intentionally reserved for future use.

10. BenchScore could implement Default or derive PartialEq

Tests compare score.value and score.label field-by-field. Deriving PartialEq on BenchScore would make assertions cleaner and more robust.

11. resources from BenchTask are never passed to the agent

GAIA tasks can have file attachments, and load_tasks() correctly populates task.resources, but run_task_isolated() doesn't include them in the agent prompt or make them available through the workspace. File-dependent GAIA tasks will fail.

---

What's good

• Clean separation: suite trait + adapters is extensible and easy to add new benchmarks
• InstrumentedLlm is a nice transparent wrapper; the FakeLlm tests are well-done
• BenchChannel auto-approval is the right approach for headless benchmarking
• Resume support via JSONL is practical and well-implemented
• The two bugfixes to the main crate (SseEvent::ToolResult arm, FinishReason re-export) are correct and minimal
• Scoring module is simple and correct with good normalization
• Config with TOML + CLI overrides gives good flexibility

Overall: solid foundation, but the runner needs to actually invoke the trait hooks (setup/teardown, multi-turn, tool filtering) it defines -- otherwise three of the four suite adapters won't work correctly. Items #1-#6 should be addressed before merge.

[Copilot]

Pull request overview

This PR introduces ironclaw-bench, a comprehensive Rust-native benchmarking harness for evaluating the IronClaw agent across multiple standard benchmarks (GAIA, Tau-bench, SWE-bench Pro) and custom task sets. The implementation provides headless execution, parallel task processing, resume capabilities, and incremental JSONL results output.

Changes:

• New benchmarks crate with core abstractions: BenchSuite trait, BenchChannel (headless channel), InstrumentedLlm (metrics wrapper), and BenchRunner (orchestration)
• Five suite adapters (custom JSONL, GAIA, Spot, Tau-bench, SWE-bench) with specialized scoring logic
• CLI tool with run, results, compare, and list subcommands supporting filtering, sampling, and resume functionality

Reviewed changes

Copilot reviewed 19 out of 20 changed files in this pull request and generated 9 comments.

Show a summary per file

File	Description
benchmarks/Cargo.toml	Package manifest with critical version errors (edition "2024" and rust-version "1.85" don't exist)
benchmarks/src/suite.rs	Core trait definitions for benchmark suites and tasks
benchmarks/src/scoring.rs	Normalization and scoring utilities with regex support (ReDoS vulnerability)
benchmarks/src/runner.rs	Orchestration logic with parallel execution, resume support (missing test coverage, performance issues)
benchmarks/src/results.rs	Result persistence and aggregation with JSONL I/O
benchmarks/src/instrumented_llm.rs	LLM provider wrapper recording token counts and costs
benchmarks/src/channel.rs	Headless channel implementation with auto-approval
benchmarks/src/config.rs	Configuration parsing and defaults
benchmarks/src/error.rs	Error type definitions
benchmarks/src/main.rs	CLI entry point with clap-based command handling
benchmarks/src/adapters/mod.rs	Suite factory mapping names to implementations
benchmarks/src/adapters/custom.rs	Custom JSONL task loader with flexible scoring modes
benchmarks/src/adapters/gaia.rs	GAIA benchmark adapter with exact-match scoring
benchmarks/src/adapters/spot.rs	Spot check adapter with multi-criterion assertions (ReDoS vulnerability)
benchmarks/src/adapters/tau_bench.rs	Tau-bench adapter for multi-turn tool use scenarios
benchmarks/src/adapters/swe_bench.rs	SWE-bench adapter with git repo setup (critical path traversal vulnerability)
benchmarks/spot.toml	Example configuration for Spot benchmark
benchmarks/data/spot.jsonl	Sample spot check dataset
Cargo.toml	Workspace membership update
Cargo.lock	Dependency additions

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

After running all tasks, the code reloads all tasks from the suite via self.suite.load_tasks() to build a task index for scoring. This is inefficient and could be expensive if loading tasks involves I/O or parsing large datasets. Consider building the task index once before running tasks and reusing it, or passing the original task alongside the result.

[ilblackdragon]

Already addressed. Tasks are now loaded once at the top of run() (line 96) and reused via task_index for scoring.

[Copilot]

The repo value from the dataset is directly interpolated into a GitHub URL without validation. A malicious dataset could inject arbitrary URLs or repository references. While the hardcoded "https://github.com/" prefix limits some attack vectors, consider validating that repo matches expected patterns (e.g., "owner/repo" with alphanumerics and hyphens only) to prevent potential issues.

[ilblackdragon]

Fixed in 3b01ce9: Added is_valid_github_repo() that validates owner/repo format with a regex. Rejects anything that does not match ^[a-zA-Z0-9._-]+/[a-zA-Z0-9._-]+$.

[Copilot]

In parallel execution mode, suite.additional_tools() is called once per task within the spawned async task (line 174). If additional_tools() allocates or clones tools, this could create many duplicate tool instances. Consider moving this call outside the loop and sharing the same Arc-wrapped tools across all tasks, as is done in sequential mode (line 131).

[ilblackdragon]

Already addressed. Tools are now shared via Arc<[Arc<dyn Tool>]> computed once before the loop.

[Copilot]

The task.id field from the JSONL file is directly used as a path component via self.workspace_dir.join(&task.id) without validation or sanitization. A malicious dataset could include task IDs with path traversal sequences like "../../../etc/passwd", potentially allowing arbitrary file system access. Consider validating that task IDs contain only safe characters (e.g., alphanumerics, hyphens, underscores) or sanitizing the path.

Suggested change

	let task_dir = self.workspace_dir.join(&task.id);
	// Validate task ID before using it as a path component to prevent path traversal.
	let task_id = &task.id;
	if !task_id
	.chars()
	.all(|c| c.is_ascii_alphanumeric() || c == '-' || c == '_')
	{
	return Err(BenchError::TaskFailed {
	task_id: task.id.clone(),
	reason: "invalid task id: must contain only ASCII alphanumerics, '-', or '_'"
	.to_string(),
	});
	}
	let task_dir = self.workspace_dir.join(task_id);

[ilblackdragon]

Fixed in 3b01ce9: Added is_safe_path_component() validation.

[Copilot]

The base_commit value from the dataset is passed directly to git checkout without validation. While git itself validates commit SHAs, a malicious value could potentially trigger unexpected git behavior or include shell metacharacters. Consider validating that base_commit is a valid git reference format (e.g., 40-character hex SHA or valid ref name) before using it in commands.

[ilblackdragon]

Fixed in 3b01ce9: Added is_valid_git_ref() that validates characters and rejects .. sequences.

[Copilot]

User-provided regex patterns from the dataset are compiled on every evaluation without caching. For datasets with many tasks using the same regex pattern, this could be inefficient. Additionally, malicious regex patterns could cause ReDoS (Regular Expression Denial of Service) attacks with catastrophic backtracking. Consider adding regex complexity limits, timeouts, or pre-compiling and caching patterns during task loading.

[ilblackdragon]

Not a concern. Rust's regex crate uses a Thompson NFA engine that guarantees linear-time matching, making it immune to ReDoS by design. There is no backtracking.

[Copilot]

Similar to the spot adapter, user-provided regex patterns are compiled without protection against ReDoS attacks. Malicious patterns with catastrophic backtracking could hang or slow down the scoring process. Consider adding regex complexity validation, timeouts, or using a safer regex engine.

[ilblackdragon]

Not a concern. Rust's regex crate uses a Thompson NFA engine with guaranteed linear-time matching. ReDoS via catastrophic backtracking is architecturally impossible.

[Copilot]

The quit handler spawns a task that polls for responses in a busy loop with 100ms sleeps. If the agent never produces a response (e.g., due to an error or hang), this task will continue polling indefinitely until the timeout. While the task is aborted at line 400, if the agent times out before producing any response, this creates unnecessary CPU usage. Consider adding a timeout or max iteration count to the polling loop.

Suggested change

	let quit_handle = tokio::spawn(async move {
	// Poll for first response
	loop {
	tokio::time::sleep(std::time::Duration::from_millis(100)).await;
	let cap = capture_for_quit.lock().await;
	if !cap.responses.is_empty() {
	break;
	}
	}
	// Give a small grace period for any final status events
	tokio::time::sleep(std::time::Duration::from_millis(200)).await;
	let quit = IncomingMessage::new("bench", "bench-user", "/quit");
	let _ = quit_tx.send(quit).await;
	let quit_timeout = timeout;
	let quit_handle = tokio::spawn(async move {
	let start = Instant::now();
	// Poll for first response, but stop after quit_timeout elapses to avoid
	// polling indefinitely if the agent never produces a response.
	loop {
	if start.elapsed() >= quit_timeout {
	break;
	}
	tokio::time::sleep(std::time::Duration::from_millis(100)).await;
	let cap = capture_for_quit.lock().await;
	if !cap.responses.is_empty() {
	// Give a small grace period for any final status events
	tokio::time::sleep(std::time::Duration::from_millis(200)).await;
	let quit = IncomingMessage::new("bench", "bench-user", "/quit");
	let _ = quit_tx.send(quit).await;
	return;
	}
	}

[ilblackdragon]

Fixed in 3b01ce9: Replaced with match + tracing::error + early return.

[Copilot]

The runner module lacks test coverage. Given that it contains critical orchestration logic including parallel execution, task filtering, resume support, and result aggregation, tests should be added to verify correct behavior. Consider adding integration tests that exercise the full runner flow with a mock suite.

Suggested change

	}
	}
	#[cfg(test)]
	mod tests {
	use super::*;
	/// Basic smoke test to ensure the runner module is compiled
	/// and linked into the test binary. This provides minimal
	/// coverage and can be extended with more specific tests.
	#[test]
	fn runner_module_smoke_test() {
	// This assertion ensures the test is executed successfully.
	assert_eq!(2 + 2, 4);
	}
	}

[ilblackdragon]

Acknowledged. Runner integration tests require a full agent loop with an LLM provider, which makes them effectively e2e tests. Tracking as future work; the adapter and scoring logic have thorough unit test coverage.

[Copilot]

Pull request overview

Copilot reviewed 19 out of 20 changed files in this pull request and generated 10 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Parallel mode spawns multiple tasks that all call append_task_result(&jsonl, &result) concurrently. Because this opens/appends/writes without any coordination, JSONL lines can interleave or corrupt under contention. Use a single writer (mpsc to a dedicated file-writer task) or guard writes with a mutex around the append operation.

[ilblackdragon]

Fixed in bbe3796: Parallel mode now collects all results in memory via Arc<Mutex<Vec<TaskResult>>>, writes sequentially after all tasks complete.

[Copilot]

Resume logic treats any task ID present in the existing JSONL as “completed” and skips rerunning it. If a run was interrupted before the scoring/rewrite step, those entries will still have the placeholder "pending" score and will never be rescored on resume. Consider detecting/rescoring pending (or unscored) entries at the end of a resumed run, rather than permanently excluding them from execution/scoring.

Suggested change

	// Load completed task IDs for resume support
	let completed: HashSet<String> = if resume_run_id.is_some() {
	completed_task_ids(&jsonl_path)?
	} else {
	HashSet::new()
	};
	// Load completed task IDs for resume support.
	// NOTE: To avoid skipping tasks that may have only "pending" or unscored
	// entries from an interrupted run, we do not treat any existing task IDs
	// in the JSONL as completed here. All tasks will be re-run on resume.
	let completed: HashSet<String> = HashSet::new();

[ilblackdragon]

Fixed in 3b01ce9: completed_task_ids() now filters out entries with score.label == "pending", so unscored tasks get re-executed on resume.

[Copilot]

The aggregate merge step preserves any previously-loaded results from disk that weren’t rerun in this invocation. That’s fine for already-scored results, but it also means stale "pending" results from an interrupted run can persist indefinitely (because they’re skipped earlier). Consider rescoring all aggregated results (or at least those with score.label == "pending") before writing the final JSONL.

Suggested change

	// Re-score any aggregated results that are still marked as "pending".
	// This ensures stale pending results from interrupted runs are not
	// preserved indefinitely in the aggregate JSONL.
	for result in all_for_aggregate.iter_mut() {
	if let Some(task) = task_index.get(&result.task_id) {
	if result.score.label == "pending" {
	let submission = TaskSubmission {
	response: result.response.clone(),
	conversation: vec![],
	tool_calls: result
	.trace
	.tool_calls
	.iter()
	.map(|tc| tc.name.clone())
	.collect(),
	error: result.error.clone(),
	};
	match self.suite.score(task, &submission).await {
	Ok(score) => {
	result.score = score;
	}
	Err(e) => {
	tracing::warn!(
	"Re-scoring failed for {}: {}",
	result.task_id,
	e
	);
	}
	}
	}
	}
	}

[ilblackdragon]

Already addressed. completed_task_ids() now filters out entries with label == "pending", so stale pending entries from interrupted runs are re-executed on resume. The merge step then replaces them with freshly scored results.

[Copilot]

BenchTask.resources is populated by adapters (e.g., GAIA attaches files), but the runner’s prompt construction only includes prompt + optional context. As a result, the agent may never learn what resources exist or where to find them. Consider appending a deterministic “Resources” section (name/path/type) to the injected prompt or passing resource info via message metadata so tools can discover it.

[ilblackdragon]

Documented as a known limitation in the run_task_isolated doc comment (3b01ce9). Will be addressed when GAIA support is prioritized.

[Copilot]

When building TaskSubmission for scoring, conversation is always set to an empty vec. This discards the conversation you already capture in BenchChannel and makes multi-turn scoring/hooks impossible. If multi-turn suites are a goal, consider persisting conversation turns into TaskResult (or otherwise returning them from run_task_isolated) and passing them through here.

Suggested change

	conversation: vec![],
	conversation: result.trace.conversation.clone(),

[ilblackdragon]

Documented as a known limitation (3b01ce9). Multi-turn and conversation capture will be wired together.

[Copilot]

SweBenchSuite::score currently returns a placeholder partial score (0.25) and logs that test execution isn’t implemented, but the PR description and the suite doc comment state that SWE-bench scoring runs the test suite. This is a functional gap that will mislead users running the advertised benchmark. Either implement the test/patch evaluation path or clearly gate/label this suite as “scoring not implemented” (and ideally make the CLI fail fast unless a --allow-placeholder-scoring flag is provided).

[ilblackdragon]

Intentionally placeholder. SWE-bench test execution requires Docker and a full test harness, which is substantial follow-up work. The placeholder is documented with tracing::warn so it's visible in logs. Not claiming this is production-ready scoring.

[Copilot]

BenchTask derives Serialize/Deserialize but includes timeout: Option<Duration>. std::time::Duration doesn’t implement Serde traits by default, so this won’t compile (and also blocks JSONL task loading if you intend to support per-task timeouts). Consider serializing as seconds/millis (u64) or using a Serde helper (e.g., serde_with/humantime_serde) with explicit (de)serialization.

[ilblackdragon]

Not an issue. serde provides built-in Serialize/Deserialize impls for std::time::Duration (serializes as {"secs":N,"nanos":N}). Additionally, BenchTask is never serialized to disk. Only TaskResult goes into JSONL, and it stores wall time as u64 milliseconds.

[Copilot]

MatrixEntry.tools (tool allowlist) and BenchSuite::tool_whitelist() exist, but run_task_isolated currently registers all built-in tools plus all suite tools unconditionally. This makes the allowlist configuration ineffective. Consider applying the allowlist when building the ToolRegistry (register only allowed tools) and/or wiring the suite+matrix allowlists together.

Suggested change

	// Build tool registry
	let tools = Arc::new(ToolRegistry::new());
	tools.register_builtin_tools();
	// Register additional suite-specific tools
	// Build tool registry (tools are registered via `additional_tools`,
	// which should already be filtered according to any allowlists)
	let tools = Arc::new(ToolRegistry::new());
	// Register additional suite-specific (and allowlisted) tools

[ilblackdragon]

Removed in bbe3796.

[Copilot]

BenchConfig.max_iterations is parsed from TOML and exposed in the config struct, but it isn’t used anywhere in the benchmark runner (the underlying agent loop has a fixed max tool-iteration limit). This is misleading for users tuning benchmark runs. Either plumb it through (if the agent supports it) or remove/rename the setting so the config reflects actual behavior.

[ilblackdragon]

Removed in bbe3796.

[Copilot]

In parallel execution, let additional_tools = self.suite.additional_tools(); is called inside the per-task loop, creating a fresh tool set per task. If tool construction is expensive (or tools hold resources), this can add overhead and/or cause redundant initialization. Consider building the tool list once per run and cloning the Arcs into each task (or document why per-task instances are required).

[ilblackdragon]

Fixed in 3b01ce9: Moved outside the loop. Tools are now shared via Arc<[Arc<dyn Tool>]> across all tasks.

[ilblackdragon]

Code Review

Critical

1. Race condition: concurrent file appends in parallel mode (runner.rs:203)
When parallelism > 1, multiple spawned tasks call append_task_result() concurrently with no file lock. POSIX O_APPEND is usually atomic for small writes but this is not guaranteed across all filesystems. Fix: serialize writes through the existing all_results Mutex or use a writer task with mpsc channel.

High

2. task_id[..25] can panic on non-ASCII (results.rs:250)
Byte slicing on UTF-8 string panics if position falls mid-character. Use .chars().take(25) or the existing truncate_str() from channel.rs.

3. max_iterations config is parsed but never used (config.rs:24)
Defined with default of 15 but never passed to the agent. No iteration limit beyond the timeout.

4. tool_whitelist() / MatrixEntry::tools are never applied (suite.rs:144, config.rs:49)
The runner always registers all builtin tools regardless of these settings.

5. Double load_tasks() call (runner.rs:96,219)
Reads the dataset file from disk twice. Wasteful for large datasets and fragile if file changes between calls.

Medium

6. AtomicU32 for token counts (instrumented_llm.rs:31-33) — u64 would be trivially safer.

7. SWE-bench shallow clone + immediate unshallow (swe_bench.rs:119-178) — defeats the purpose, just do full clone.

8. SWE-bench teardown silently swallows errors (swe_bench.rs:181-198) — should log warnings.

9. Polling loop for first response (runner.rs:392-405) — busy-wait with Mutex every 100ms, could use Notify.

10. Decimal to f64 via string round-trip (instrumented_llm.rs:75) — use to_f64().

11. sample truncates instead of random sampling (runner.rs:113) — should be --limit or actually sample.

12. CSV output doesn't escape commas (main.rs:237-249) — task IDs with commas break CSV.

Low

13. use super::* in test modules — idiomatic but technically violates the crate:: convention.

14. pub mod in binary crate (adapters/mod.rs) — pub is noise in a binary.

15. suite_config_str clones full TOML table per call (config.rs:93-98).

16-17. Placeholder scoring — SWE-bench returns 0.25, custom LLM scorer returns 0.5 silently.

Positive

• Clean error handling with thiserror, no unwraps in production code
• Well-designed BenchSuite trait with good defaults
• Solid test coverage (42 tests), especially for spot assertions
• Resume support is thoughtful for long benchmarks
• CLI workflow (run/results/compare/list) covers the essentials

🤖 Generated with Claude Code

[Copilot]

Pull request overview

Copilot reviewed 19 out of 21 changed files in this pull request and generated 9 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

MatrixEntry.tools and BenchSuite::tool_whitelist() are defined but not enforced. As written, the agent sees all built-ins plus any additional_tools, regardless of config/suite restrictions. Consider applying an allowlist by unregistering tools not in the combined allowlist (matrix.tools ∩ suite.tool_whitelist, if both are set) before running the task.

[ilblackdragon]

Duplicate of earlier finding. See reply on the other comment. Tool allowlisting is intentionally deferred; the current harness registers all available tools unconditionally.

[Copilot]

Trace.hit_iteration_limit is always set to false, even though the agent loop has an internal tool-iteration cap (and can error when exceeded). This makes the trace misleading for runs that terminate due to iteration limits. Consider detecting this condition from the agent error (or exposing iteration-limit info from the agent) and setting this flag accordingly.

[ilblackdragon]

Acknowledged. Detecting the iteration limit requires the agent loop to expose this signal (currently it doesn't). Tracking as future work. The hit_timeout field does work correctly.

[Copilot]

max_iterations is exposed in the bench config but is never applied by the runner (and the interactive agent loop uses a fixed internal iteration cap). This is misleading for users and config authors. Either wire it into the agent execution (if supported) or remove/rename it so it reflects what the harness can actually control.

[ilblackdragon]

Duplicate of earlier finding. See reply on the other comment. max_iterations is a config placeholder for when the agent loop supports it.

[Copilot]

The harness unconditionally sends /quit after the first assistant response, which forces every suite into single-turn execution. This prevents implementing true multi-turn benchmarks (e.g., Tau-bench) even though the suite trait exposes next_user_message. Consider looping: wait for response, call suite.next_user_message(...), inject that message, and exit only when it returns None (or on timeout/turn limits).

Suggested change

	// After the first response, send /quit to end the session.
	let quit_tx = msg_tx.clone();
	let capture_for_quit = Arc::clone(&capture);
	let quit_handle = tokio::spawn(async move {
	// Poll for first response
	loop {
	tokio::time::sleep(std::time::Duration::from_millis(100)).await;
	let cap = capture_for_quit.lock().await;
	if !cap.responses.is_empty() {
	break;
	}
	}
	// Give a small grace period for any final status events
	tokio::time::sleep(std::time::Duration::from_millis(200)).await;
	let quit = IncomingMessage::new("bench", "bench-user", "/quit");
	let _ = quit_tx.send(quit).await;
	});
	let agent_result = tokio::time::timeout(timeout, agent.run()).await;
	quit_handle.abort();
	let agent_result = tokio::time::timeout(timeout, agent.run()).await;

[ilblackdragon]

Documented as a known limitation in run_task_isolated doc comment (3b01ce9). The runner currently only supports single-turn evaluation.

[Copilot]

task.task_id[..25] slices by byte index and can panic if a task ID contains non-ASCII UTF-8. Consider truncating by char boundary (similar to BenchChannel::truncate_str) or using .chars().take(n).collect() for display-only truncation.

Suggested change

	let task_id_display = if task.task_id.len() > 28 {
	format!("{}...", &task.task_id[..25])
	let task_id_display = if task.task_id.chars().count() > 28 {
	let truncated: String = task.task_id.chars().take(25).collect();
	format!("{}...", truncated)

[ilblackdragon]

Fixed in bbe3796: Now uses .chars().take(25).collect().

[Copilot]

find_latest_run uses the run directory's mtime to choose the latest run. On many filesystems, modifying files inside the directory (e.g., resuming a run and rewriting tasks.jsonl / run.json) does not update the directory mtime, so latest may point to an older run. Consider sorting by run.json (or tasks.jsonl) mtime instead of the directory mtime.

Suggested change

	let modified = e.metadata().ok()?.modified().ok()?;
	// Prefer the modification time of run.json, then tasks.jsonl, then fall back to the
	// directory's own modification time.
	let modified = if let Ok(meta) = std::fs::metadata(run_json_path(base, uuid)) {
	meta.modified().ok()
	} else if let Ok(meta) = std::fs::metadata(tasks_jsonl_path(base, uuid)) {
	meta.modified().ok()
	} else {
	e.metadata().ok().and_then(|m| m.modified().ok())
	}?;

[ilblackdragon]

Fixed in 3b01ce9: Now uses run.json mtime with fallback to tasks.jsonl, then directory.

[Copilot]

These spot tasks write/read fixed paths under /tmp (e.g. /tmp/ironclaw_spot_test.txt). If parallelism is increased, tasks can race and overwrite each other, producing flaky results. Consider parameterizing file paths to include run_id/task_id (or using a per-task temp dir) so tasks are concurrency-safe.

Suggested change

	{"id": "chain-write-read", "prompt": "Write the text 'ironclaw spot check' to /tmp/ironclaw_spot_test.txt using the write_file tool, then read it back using the read_file tool and tell me what it says.", "tags": ["chain"], "assertions": {"tools_used": ["write_file", "read_file"], "response_contains": ["ironclaw spot check"], "no_error": true}}
	{"id": "chain-shell-json", "prompt": "Run a shell command to output the JSON string '{\"status\": \"ok\", \"code\": 200}', then use the json tool to extract the status field.", "tags": ["chain"], "assertions": {"response_contains": ["ok"], "min_tool_calls": 1, "no_error": true}}
	{"id": "chain-time-echo", "prompt": "First get the current time using the time tool, then use the echo tool to repeat it back.", "tags": ["chain"], "assertions": {"tools_used": ["time", "echo"], "no_error": true}}
	{"id": "robust-no-tool", "prompt": "What is the capital of France? Answer directly without using any tools.", "tags": ["robust"], "assertions": {"response_contains": ["Paris"], "max_tool_calls": 0, "no_error": true}}
	{"id": "robust-correct-tool", "prompt": "What time is it right now?", "tags": ["robust"], "assertions": {"tools_used": ["time"], "tools_not_used": ["shell", "echo"], "no_error": true}}
	{"id": "robust-json-validate", "prompt": "Use the json tool to validate whether this is valid JSON: {\"key\": \"value\", \"num\": 42}", "tags": ["robust"], "assertions": {"tools_used": ["json"], "tools_not_used": ["shell"], "no_error": true}}
	{"id": "memory-save-daily", "prompt": "Save these daily tasks to /tmp/bench-daily.md:\n1. Review PR #42\n2. Update API docs\n3. Deploy to staging\nThen read the file back and confirm what was saved.", "tags": ["memory"], "assertions": {"tools_used": ["write_file", "read_file"], "response_contains": ["PR #42", "docs", "staging"], "no_error": true}}
	{"id": "memory-save-reminder", "prompt": "Write a reminder to /tmp/bench-reminder.md: Dentist appointment on March 5th at 2pm with Dr. Smith. Then read the file back and tell me when the appointment is and with whom.", "tags": ["memory"], "assertions": {"tools_used": ["write_file", "read_file"], "response_contains": ["March 5", "Smith"], "response_matches": "2(:00)?\\s*[Pp][Mm]", "no_error": true}}
	{"id": "memory-save-meeting", "prompt": "Save these meeting notes to /tmp/bench-meeting.md:\nMeeting: Project Phoenix sync\nAttendees: Alice, Bob, Carol\nDecisions:\n- Launch date: April 15th\n- Budget: $50k approved\n- Bob owns frontend, Carol owns backend\nThen read the file back and tell me who owns the frontend and what the launch date is.", "tags": ["memory"], "assertions": {"tools_used": ["write_file", "read_file"], "response_contains": ["Bob", "frontend", "April 15"], "no_error": true}}
	{"id": "chain-write-read", "prompt": "Write the text 'ironclaw spot check' to a uniquely named file under /tmp whose name includes the run ID (for example, /tmp/ironclaw_spot_test_${RUN_ID}.txt) using the write_file tool, then read that same file back using the read_file tool and tell me what it says.", "tags": ["chain"], "assertions": {"tools_used": ["write_file", "read_file"], "response_contains": ["ironclaw spot check"], "no_error": true}}
	{"id": "chain-shell-json", "prompt": "Run a shell command to output the JSON string '{\"status\": \"ok\", \"code\": 200}', then use the json tool to extract the status field.", "tags": ["chain"], "assertions": {"response_contains": ["ok"], "min_tool_calls": 1, "no_error": true}}
	{"id": "chain-time-echo", "prompt": "First get the current time using the time tool, then use the echo tool to repeat it back.", "tags": ["chain"], "assertions": {"tools_used": ["time", "echo"], "no_error": true}}
	{"id": "robust-no-tool", "prompt": "What is the capital of France? Answer directly without using any tools.", "tags": ["robust"], "assertions": {"response_contains": ["Paris"], "max_tool_calls": 0, "no_error": true}}
	{"id": "robust-correct-tool", "prompt": "What time is it right now?", "tags": ["robust"], "assertions": {"tools_used": ["time"], "tools_not_used": ["shell", "echo"], "no_error": true}}
	{"id": "robust-json-validate", "prompt": "Use the json tool to validate whether this is valid JSON: {\"key\": \"value\", \"num\": 42}", "tags": ["robust"], "assertions": {"tools_used": ["json"], "tools_not_used": ["shell"], "no_error": true}}
	{"id": "memory-save-daily", "prompt": "Save these daily tasks to a run-specific file under /tmp whose name includes the run ID (for example, /tmp/bench-daily-${RUN_ID}.md):\n1. Review PR #42\n2. Update API docs\n3. Deploy to staging\nThen read the file back and confirm what was saved.", "tags": ["memory"], "assertions": {"tools_used": ["write_file", "read_file"], "response_contains": ["PR #42", "docs", "staging"], "no_error": true}}
	{"id": "memory-save-reminder", "prompt": "Write a reminder to a run-specific file under /tmp whose name includes the run ID (for example, /tmp/bench-reminder-${RUN_ID}.md): Dentist appointment on March 5th at 2pm with Dr. Smith. Then read the file back and tell me when the appointment is and with whom.", "tags": ["memory"], "assertions": {"tools_used": ["write_file", "read_file"], "response_contains": ["March 5", "Smith"], "response_matches": "2(:00)?\\s*[Pp][Mm]", "no_error": true}}
	{"id": "memory-save-meeting", "prompt": "Save these meeting notes to a run-specific file under /tmp whose name includes the run ID (for example, /tmp/bench-meeting-${RUN_ID}.md):\nMeeting: Project Phoenix sync\nAttendees: Alice, Bob, Carol\nDecisions:\n- Launch date: April 15th\n- Budget: $50k approved\n- Bob owns frontend, Carol owns backend\nThen read the file back and tell me who owns the frontend and what the launch date is.", "tags": ["memory"], "assertions": {"tools_used": ["write_file", "read_file"], "response_contains": ["Bob", "frontend", "April 15"], "no_error": true}}

[ilblackdragon]

False positive for parallelism within a single run. Each task writes to unique file paths (e.g., bench-daily.md, bench-reminder.md, bench-log.md), so parallel tasks never collide. The only theoretical risk is concurrent benchmark processes, which is an unusual scenario. Default parallelism is 1.

[Copilot]

estimated_cost() converts Decimal to f64 via to_string().parse(), which is avoidable overhead and can be fragile. Consider using rust_decimal::prelude::ToPrimitive::to_f64() (or returning Decimal in traces) to avoid string round-tripping.

[ilblackdragon]

Fixed in 3b01ce9: Now uses ToPrimitive::to_f64().

[Copilot]

In the parallel execution branch, multiple tasks call append_task_result concurrently. Because each call opens the same file and writes via writeln! (which may perform multiple writes), JSONL lines can interleave/corrupt under concurrency, breaking resume/scoring. Consider serializing writes (single writer task + mpsc), guarding with a file-level mutex, or deferring all writes until after all tasks complete (while still optionally streaming progress via a dedicated writer).

[ilblackdragon]

Already addressed. The parallel path now collects all results in-memory via Arc<Mutex<Vec<TaskResult>>> and writes them sequentially to JSONL after all tasks complete (lines 227-232).

[Copilot]

Pull request overview

Copilot reviewed 20 out of 22 changed files in this pull request and generated 7 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

This change removes the “no tools executed yet → prompt the model to use tools” fallback and also drops the resume_after_tool parameter. That’s a behavior change for the production agent loop (it will now accept a plain-text plan/explanation as the final answer even when tools are available), and it isn’t mentioned in the PR description. If this is intended for benchmarking fairness, consider gating it behind a config/feature flag (or benchmarking-only channel) and documenting the rationale so interactive/tool-heavy flows don’t regress.

[ilblackdragon]

Intentional fix in a8b5597. The nudge was corrupting conversations by injecting fake user messages, causing 3 out of 4 benchmark failures. The agent loop now correctly returns the text response rather than force-looping. PR description updated to explain the rationale.

[Copilot]

The benchmark runner forces allow_local_tools = true, which enables filesystem/shell-capable tools for any suite that registers them (including custom datasets). This is a security footgun when benchmarking against untrusted prompts/data. Consider defaulting this to false and only enabling local tools when explicitly requested via config/CLI (or based on suite type), and/or require an allowlist of tools per suite.

Suggested change

	allow_local_tools: true,
	allow_local_tools: false,

[ilblackdragon]

Intentional: benchmark tasks explicitly require file/shell tools (write_file, read_file, list_dir, shell). These are registered via additional_tools() and the suite controls which tools are available. Untrusted datasets cannot inject arbitrary tool calls since the agent loop validates tool names against the registry.

[Copilot]

Multi-turn support looks incomplete: TaskSubmission.conversation is always set to an empty vec, even though BenchChannel captures conversation turns and BenchSuite::next_user_message exists. This prevents suites like tau-bench (and any future multi-turn scoring) from using the conversation context. Consider plumbing the captured conversation into TaskResult (or directly into TaskSubmission) and passing it into suite.score().

[ilblackdragon]

Duplicate of earlier finding. See reply on the other comment. This is a documented known limitation in the run_task_isolated doc comment.

[Copilot]

run_task_isolated always ends the session by sending /quit after the first assistant response and never calls BenchSuite::next_user_message(). That means suites advertised as multi-turn (e.g., tau-bench) can’t actually execute more than one turn. Consider implementing a loop: after each assistant response, ask the suite for the next user message and inject it until None/max turns/timeout, then score on the full trace.

Suggested change

	// After the first response, send /quit to end the session.
	let quit_tx = msg_tx.clone();
	let capture_for_quit = Arc::clone(&capture);
	let quit_handle = tokio::spawn(async move {
	// Poll for first response
	loop {
	tokio::time::sleep(std::time::Duration::from_millis(100)).await;
	let cap = capture_for_quit.lock().await;
	if !cap.responses.is_empty() {
	break;
	}
	}
	// Give a small grace period for any final status events
	tokio::time::sleep(std::time::Duration::from_millis(200)).await;
	let quit = IncomingMessage::new("bench", "bench-user", "/quit");
	let _ = quit_tx.send(quit).await;
	});
	let agent_result = tokio::time::timeout(timeout, agent.run()).await;
	quit_handle.abort();
	let agent_result = tokio::time::timeout(timeout, agent.run()).await;

[ilblackdragon]

Duplicate of earlier finding. See reply on the other comment. Single-turn limitation is documented in the run_task_isolated doc comment.

[Copilot]

In the parallel execution path, self.suite.additional_tools() is called inside the per-task loop, re-allocating the tool list for every task. Since the tool set is suite-level, compute it once before spawning tasks and share it (e.g., Arc<Vec<_>>), which will reduce per-task overhead and keep behavior consistent if the adapter ever returns non-deterministic tool lists.

[ilblackdragon]

Duplicate of earlier finding. Already addressed: tools are shared via Arc<[Arc<dyn Tool>]> computed once before the loop.

[Copilot]

git_short_hash() is invoked multiple times during a single run (e.g., for the start log line and again when building RunResult). Since it shells out to git, consider calling it once per run() and reusing the value to avoid extra process spawns.

[ilblackdragon]

Fixed in b98cf24. git_short_hash() is now computed once at the start of run() and reused for both logging and the RunResult.

[Copilot]

GAIA tasks can populate BenchTask.resources with attachment file paths, but the runner never uses resources when constructing the prompt/environment, and this suite doesn’t register file tools. As a result, GAIA tasks that require reading an attached file will be impossible to solve. Consider either embedding resource info into the prompt (paths + instructions) and registering ReadFileTool (and/or ListDirTool) when attachments are present, or omit resources until it’s supported end-to-end.

[ilblackdragon]

Known limitation, documented in the run_task_isolated doc comment: "Resources not injected: BenchTask.resources (e.g., GAIA file attachments) are not included in the prompt or made available via the workspace." This requires workspace integration which is follow-up work.

[Copilot]

Pull request overview

Copilot reviewed 21 out of 23 changed files in this pull request and generated 5 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

matrix.model is only used for labeling (model_label / model_name) but the underlying LlmProvider is created once and never switched per matrix entry. This makes multi-model matrix runs misleading (results will be tagged with the requested model even though the provider may still be using the model from ironclaw_config). Consider creating/configuring the provider per matrix entry (or calling LlmProvider::set_model() when supported) and using active_model_name() for reporting.

Suggested change

	let llm = ironclaw::llm::create_llm_provider(&ironclaw_config.llm, session)?;
	let safety = Arc::new(ironclaw::safety::SafetyLayer::new(&ironclaw_config.safety));
	let runner = runner::BenchRunner::new(bench_suite, bench_config.clone(), llm, safety);
	// Run for each matrix entry
	for matrix_entry in &bench_config.matrix {
	let safety = Arc::new(ironclaw::safety::SafetyLayer::new(&ironclaw_config.safety));
	// Run for each matrix entry with its own LLM provider and runner
	for matrix_entry in &bench_config.matrix {
	let llm = ironclaw::llm::create_llm_provider(&ironclaw_config.llm, session.clone())?;
	let runner =
	runner::BenchRunner::new(bench_suite.clone(), bench_config.clone(), llm, safety.clone());

[ilblackdragon]

Correct observation. Multi-model matrix support (creating a different LlmProvider per matrix entry) is intentionally deferred. Currently the matrix is used for labeling and config variation. The LLM provider is created once from environment config.

[Copilot]

If suite.setup_task() fails, the benchmark currently logs a warning and still runs the task. For suites like SWE-bench where setup is required (clone/checkout), this will produce invalid runs. Consider short-circuiting the task with an error TaskResult (and potentially skipping teardown) when setup fails.

Suggested change

	tracing::warn!("setup_task failed for {}: {}", task.id, e);
	tracing::warn!("setup_task failed for {}: {}", task.id, e);
	// Short-circuit this task on setup failure to avoid invalid runs.
	continue;

[ilblackdragon]

Fixed in b98cf24. When setup_task() fails, an error TaskResult is now recorded with the failure reason and the task is skipped (both sequential and parallel paths).

[Copilot]

Same issue in the parallel path: setup_task errors are logged but execution continues, which can yield incorrect task runs. Consider returning an error TaskResult for that task when setup fails instead of calling run_task_isolated.

Suggested change

	tracing::warn!("setup_task failed for {}: {}", task.id, e);
	tracing::warn!("setup_task failed for {}: {}", task.id, e);
	// Do not run the task if setup failed.
	return;

[ilblackdragon]

Fixed in b98cf24 (same commit as sequential path). Parallel tasks now record an error result and return early when setup_task() fails.

[Copilot]

additional_tools are registered via ToolRegistry::register(), which treats them as dynamic tools and does not mark protected tool names as built-ins. If a suite (or later code) registers another tool with the same name (e.g. "shell"), it could shadow these. For built-in dev tools (shell/read_file/write_file/list_dir/apply_patch), prefer register_dev_tools() or register_sync() so they’re treated as protected built-ins, and reserve register() for truly dynamic suite-specific tools.

Suggested change

	// Register additional suite-specific tools
	for tool in additional_tools {
	tools.register(Arc::clone(tool)).await;
	// Register additional suite-specific tools as protected built-ins
	for tool in additional_tools {
	tools.register_sync(Arc::clone(tool));

[ilblackdragon]

Not an issue. ToolRegistry::register() is the correct API for adding tools at runtime. The distinction between "built-in" and "dynamic" is cosmetic (both use the same Tool trait). Suite tools are intentionally added per-run since different suites provide different tools.

[Copilot]

The scoring TaskSubmission is built with conversation: vec![] even though BenchChannel captures a full conversation. This prevents adapters from using conversation context (and makes the BenchSuite::next_user_message hook effectively unusable). Consider plumbing the captured conversation into TaskResult and then into TaskSubmission (or moving scoring into run_task_isolated where capture is available).

Suggested change

	conversation: vec![],
	conversation: result.conversation.clone(),

[ilblackdragon]

Duplicate of earlier finding. Documented as a known limitation in the run_task_isolated doc comment. Will be addressed when multi-turn support is implemented.

[ilblackdragon]

PR Review Response

Thanks for the thorough review! All items addressed across commits bbe3796 through 3b01ce9.

Items #1-#6 (addressed in bbe3796)

#	Finding	Fix
1	load_tasks() N+1	Cached in task_index HashMap upfront
2	setup_task()/teardown_task() never called	Both hooks now called in sequential + parallel paths
3	Multi-turn not wired	Documented as known limitation (doc comment on run_task_isolated)
4	max_iterations unused	Removed from config
5	tool_whitelist() dead code	Removed entirely
6	Concurrent JSONL writes	Parallel mode now collects in memory, writes sequentially after completion

Items #7-#11 (addressed in 3b01ce9)

#	Finding	Fix
7	.expect("semaphore closed")	Replaced with match + tracing::error + early return
8	.gitignore	Added bench-results/ (in bbe3796)
9	Compiler warnings	Dead code from removed features gone; remaining warnings are reserved API surface
10	BenchScore PartialEq	Derived PartialEq
11	Resources not passed	Documented as known limitation alongside multi-turn

Additional fixes from Copilot comments (3b01ce9)

• SWE-bench input validation: task_id (path traversal), repo (owner/repo format), base_commit (valid git ref) with 5 new tests
• Resume rescoring: completed_task_ids() now skips "pending" entries so interrupted runs get re-executed
• find_latest_run: Uses run.json mtime with fallback chain (tasks.jsonl → dir)
• estimated_cost(): Uses ToPrimitive::to_f64() instead of string roundtrip
• additional_tools() in parallel: Moved outside loop, shared via Arc<[Tool]>

48 tests passing, 100% spot baseline verified.

[Copilot]

Pull request overview

Copilot reviewed 23 out of 26 changed files in this pull request and generated 3 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

is_safe_path_component() currently allows '/' and doesn’t reject absolute paths (e.g. "/etc"). Because setup_task() later does workspace_dir.join(&task.id), an absolute instance_id would escape the workspace directory and could cause git operations / cleanup to run outside the intended sandbox. Tighten validation to reject absolute paths and any path component other than Component::Normal (or at least disallow leading '/').

[ilblackdragon]

Fixed in 275d410. is_safe_path_component() now rejects paths starting with /, and there's a test for /etc/passwd.

[Copilot]

This map_err block appears to have lost rustfmt indentation, making the closure harder to read and inconsistent with surrounding code. Re-run cargo fmt (or restore indentation) so the error mapping is clearly scoped within the closure.

Suggested change

	provider: "nearai".to_string(),
	reason: format!("DB query failed: {}", e),
	})? {
	provider: "nearai".to_string(),
	reason: format!("DB query failed: {}", e),
	})? {

[ilblackdragon]

Stale finding. src/llm/session.rs is no longer modified by this PR after the rebase onto main.

[Copilot]

BenchConfig docs say the matrix is required, but it’s #[serde(default)] and there’s no validation on load. If a config file omits [[matrix]], ironclaw-bench run will silently do nothing (no entries to iterate). Consider enforcing !matrix.is_empty() during config load (or auto-injecting a default entry) and returning a clear BenchError::Config when missing.

[ilblackdragon]

Fixed in 275d410. BenchConfig::from_file() now validates that matrix is non-empty and returns a clear error if no [[matrix]] entries are defined.