feat: add Criterion benchmarks for safety layer hot paths by ilblackdragon · Pull Request #836 · nearai/ironclaw

ilblackdragon · 2026-03-10T06:08:19Z

Summary

Add Criterion benchmarks for Sanitizer, Validator, and LeakDetector
Add tool dispatch benchmarks (JSON parsing, schema validation)
Add cargo bench --no-run CI job to ensure benchmarks compile
Add criterion dev-dependency and bench targets to Cargo.toml

Test plan

Run cargo bench locally and verify all benchmarks produce results
Verify CI bench-compile job compiles benchmarks without running them
Each benchmark should complete in <10 seconds

🤖 Generated with Claude Code

Add benchmark suite using Criterion.rs for performance-critical paths: - benches/safety_check.rs: Sanitizer (clean/adversarial), Validator (normal/long/tool params), LeakDetector (clean/secrets/HTTP scan) - benches/tool_dispatch.rs: JSON parsing, schema validation patterns, tool output serialization CI compiles benchmarks on every PR to prevent regressions. Run locally with: cargo bench Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

gemini-code-assist · 2026-03-10T06:08:23Z

Warning

You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again!

Include bench-compile in the run-tests roll-up job's needs array so benchmark compilation failures block PRs. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Copilot

Pull request overview

This PR adds Criterion benchmarking infrastructure, but also bundles several broader refactors and behavior changes across initialization, onboarding, routines, tools, and job execution.

Changes:

Add Criterion benchmarks (safety layer + tool dispatch) and a CI job to compile benches (cargo bench --no-run).
Refactor “module-owned initialization” by extracting setup logic into module factory helpers (DB handles, secrets store creation, orchestrator setup, managed tunnel startup, WASM channel setup, MCP client factory).
Introduce new runtime behaviors: quick onboarding mode, per-job token budgeting, and optional lightweight-routine tool execution (with new routine config knobs), plus various logging level adjustments.

Reviewed changes

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

Show a summary per file

File	Description
tests/support/test_rig.rs	Updates test rig wiring for new `RoutineEngine::new` args and routine config fields.
tests/module_init_integration.rs	Adds integration tests validating module-owned factory wiring (db/handles, secrets store, ExtensionManager).
tests/e2e_routine_heartbeat.rs	Adjusts routine heartbeat e2e tests to provide `ToolRegistry` + `SafetyLayer` to `RoutineEngine`.
src/worker/mod.rs	Adds public `run_worker` / `run_claude_bridge` entrypoints for container subcommands.
src/tunnel/mod.rs	Introduces `start_managed_tunnel` helper that mutates config and returns an active tunnel handle.
src/tracing_fmt.rs	Adds CLI/worker tracing initializers (separate from web tracing layer).
src/tools/wasm/loader.rs	Treat missing tools directory as empty and adds a regression test; mitigates TOCTOU with `read_dir`.
src/tools/registry.rs	Lowers tool registration logs from `info` to `debug`.
src/tools/mcp/mod.rs	Exposes new MCP client factory module + re-exports factory API.
src/tools/mcp/factory.rs	Adds `create_client_from_config` to centralize MCP transport dispatch and auth decisions.
src/tools/builtin/skill_tools.rs	Makes `skill_remove` permanently destructive messaging + changes approval requirement to `Always` with tests.
src/tools/builtin/extension_tools.rs	Makes `tool_remove` permanently destructive messaging + changes approval requirement to `Always` with tests; updates ExtensionManager ctor usage in tests.
src/setup/wizard.rs	Adds “quick” onboarding mode, silent migrations in quick mode, `.env` upsert, and NEAR AI model-fetch config fix + tests.
src/setup/mod.rs	Adds `check_onboard_needed()` helper for first-run detection.
src/setup/README.md	Documents quick mode and updates onboarding step count/flow.
src/settings.rs	Adds `agent.max_tokens_per_job` setting with default.
src/secrets/mod.rs	Adds `create_secrets_store(crypto, handles)` factory returning `Option` based on DB handles.
src/sandbox/proxy/http.rs	Lowers shutdown log level to `debug`.
src/sandbox/manager.rs	Lowers shutdown log level to `debug`.
src/orchestrator/mod.rs	Adds `setup_orchestrator` factory returning `OrchestratorSetup` (job manager, SSE tx, queues, docker status).
src/main.rs	Switches to module factories (orchestrator/tunnel/wasm setup, worker/bridge runners, tracing helpers); adds quick auto-onboard on first run; reduces several logs to `debug`.
src/llm/nearai_chat.rs	Lowers pricing-load log level to `debug`.
src/llm/mod.rs	Lowers various provider init logs to `debug`.
src/history/store.rs	Persists `user_id` in Postgres job upsert; adds ignored integration regression test.
src/extensions/manager.rs	Adds required `McpProcessManager` dependency; uses new MCP factory for activation.
src/db/mod.rs	Introduces `DatabaseHandles` + `connect_with_handles`; updates `connect_from_config` and adds shared `create_secrets_store` DB factory.
src/db/libsql/mod.rs	Adds libsql regression test for persisting `user_id` in jobs.
src/db/libsql/jobs.rs	Persists `user_id` in libsql job upsert.
src/config/routines.rs	Adds lightweight routine tool config (`lightweight_tools_enabled`, `lightweight_max_iterations`) and env parsing with caps.
src/config/embeddings.rs	Lowers embeddings enablement logs to `debug`.
src/config/agent.rs	Adds `AGENT_MAX_TOKENS_PER_JOB` parsing into agent config.
src/cli/tool.rs	Refactors secrets-store initialization to shared CLI helper.
src/cli/mod.rs	Adds `--quick` to onboard command; introduces shared `init_secrets_store()` and restores `run_memory_command()` here.
src/cli/mcp.rs	Refactors secrets-store initialization to shared CLI helper.
src/channels/webhook_server.rs	Lowers shutdown log level to `debug`.
src/channels/web/types.rs	Extends `RoutineRunInfo` with optional `job_id`.
src/channels/web/server.rs	Threads `job_id` through routines run responses; lowers shutdown log level; updates ExtensionManager ctor in tests.
src/channels/web/handlers/routines.rs	Threads `job_id` through routines run responses.
src/channels/web/handlers/jobs.rs	Improves cancel fallback to stop via scheduler first, then persist DB cancellation.
src/channels/wasm/setup.rs	Extracts WASM channel setup + credential injection from `main.rs` into module-owned helper.
src/channels/wasm/mod.rs	Exports new WASM setup helpers.
src/channels/wasm/loader.rs	Treat missing channels directory as empty and adds a regression test; mitigates TOCTOU with `read_dir`.
src/channels/manager.rs	Lowers channel lifecycle logs to `debug`.
src/bootstrap.rs	Adds batched `.env` upsert helper preserving user vars + tests.
src/app.rs	Refactors DB init to use `connect_with_handles` and secrets init via `secrets::create_secrets_store`; switches several logs to `debug`; uses MCP factory.
src/agent/worker.rs	Changes iteration/rate-limit outcomes to `Failed` (not `Stuck`); adds token budget tracking from LLM usage + tests.
src/agent/scheduler.rs	Applies per-job token budget from config/metadata into context manager.
src/agent/routine_engine.rs	Adds lightweight tool loop support gated by new routine config; requires `ToolRegistry` + `SafetyLayer` in engine; adds tests for new config defaults.
src/agent/dispatcher.rs	Updates tests for new agent config field and adds approval requirement regression tests.
src/agent/agent_loop.rs	Wires `ToolRegistry` + `SafetyLayer` into `RoutineEngine`; lowers logs to `debug`.
benches/tool_dispatch.rs	Adds Criterion bench for JSON parsing/validation/serialization patterns in tool dispatch.
benches/safety_check.rs	Adds Criterion benches for Sanitizer, Validator, LeakDetector hot paths.
FEATURE_PARITY.md	Updates parity notes and “last reviewed” date plus feature list adjustments.
Cargo.toml	Adds Criterion dev-dependency and declares bench targets.
CLAUDE.md	Updates repo structure docs and codifies module-owned initialization rule.
.github/workflows/test.yml	Adds `bench-compile` job to compile benchmarks; scopes PR trigger to `main`.
.github/workflows/staging-ci.yml	Removes conditional guard around GitHub App token step.
.github/workflows/code_style.yml	Makes windows clippy run only for PRs targeting `main` and adjusts roll-up logic.
.github/workflows/claude-review.yml	Changes trigger types and adds `allowed_bots` configuration.
.env.example	Documents `AGENT_MAX_TOKENS_PER_JOB`.

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

Copilot · 2026-03-10T06:17:50Z

+criterion = { version = "0.5", features = ["async_tokio"] }
+
+[[bench]]
+name = "safety_check"
+harness = false
+
+[[bench]]
+name = "tool_dispatch"
+harness = false


The PR title/description focus on adding Criterion benchmarks, but this diff also includes substantial runtime behavior changes (quick onboarding, tool approval policy changes, routine tool execution loop, orchestrator/tunnel/module-init refactors, token budgeting, etc.). This makes the PR hard to review and increases merge risk; please either split the non-benchmark changes into separate PRs or update the PR title/description to accurately reflect the full scope.

This PR only modifies .github/workflows/test.yml, Cargo.toml, Cargo.lock, Dockerfile, benches/safety_check.rs, benches/tool_dispatch.rs. Other files are from the staging base.

Copilot · 2026-03-10T06:17:51Z

+            let tool_defs = ctx.tools.tool_definitions().await;
+
+            let request = ToolCompletionRequest::new(messages.clone(), tool_defs)
+                .with_max_tokens(effective_max_tokens)
+                .with_temperature(0.3);
+


execute_lightweight_with_tools passes all registered tool definitions to the LLM (ctx.tools.tool_definitions()), but execute_routine_tool will later reject any tool whose requires_approval() is not Never. Exposing tools that can never run in lightweight routines is likely to waste iterations/tokens and produce confusing failure loops. Consider filtering the tool definitions up-front to only include tools that are actually allowed in lightweight routines (or providing a dedicated lightweight-tool registry/allowlist).

This file is not changed by this PR — it is part of the staging base. Will track separately.

Copilot · 2026-03-10T06:17:51Z

+/// Initialize tracing for simple CLI commands (warn level, no fancy layers).
+pub fn init_cli_tracing() {
+    tracing_subscriber::fmt()
+        .with_env_filter(
+            EnvFilter::try_from_default_env().unwrap_or_else(|_| EnvFilter::new("warn")),
+        )
+        .init();
+}
+
+/// Initialize tracing for worker/bridge processes (info level).
+pub fn init_worker_tracing() {
+    tracing_subscriber::fmt()
+        .with_env_filter(
+            EnvFilter::try_from_default_env().unwrap_or_else(|_| EnvFilter::new("ironclaw=info")),
+        )
+        .init();
+}


init_cli_tracing() / init_worker_tracing() set up a default fmt() subscriber without using the TruncatingStderr writer that this module is built around. If a user sets RUST_LOG=debug, CLI/worker processes can again emit very large events to stderr (the problem this module doc comment describes). Consider wiring these init helpers through tracing_subscriber::registry().with(fmt::layer().with_writer(TruncatingStderr::default()))... (or factoring a shared initializer) so truncation applies consistently.

This file is not changed by this PR — it is part of the staging base. Will track separately.

Copilot

Pull request overview

Copilot reviewed 5 out of 5 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 · 2026-03-10T07:16:41Z

+    group.bench_function("http_request_scan", |b| {
+        b.iter(|| {
+            detector.scan_http_request(
+                "https://api.example.com/data?query=hello",
+                &[
+                    ("Content-Type".to_string(), "application/json".to_string()),
+                    ("Accept".to_string(), "text/html".to_string()),
+                ],
+                Some(b"{\"query\": \"hello world\"}"),


In http_request_scan, the benchmarked closure allocates header Strings and the slice literal on every iteration (to_string() calls). This will inflate the measured time with allocation overhead rather than primarily measuring scan_http_request. Consider constructing the headers/body once outside the iter and passing references (or using iter_batched) to isolate the scanning cost.

Fixed in 219a2ce — header allocations moved outside b.iter().

Copilot · 2026-03-10T07:16:41Z

+        b.iter(|| serde_json::from_str::<serde_json::Value>(black_box(simple_params)))
+    });
+
+    group.bench_function("parse_complex", |b| {
+        b.iter(|| serde_json::from_str::<serde_json::Value>(black_box(complex_params)))


These JSON parsing benchmarks currently ignore the Result from serde_json::from_str. If either JSON string becomes invalid, the benchmark will silently measure error construction instead of parsing. Consider asserting success (e.g., via expect/unwrap) so the benchmark fails loudly when inputs are malformed.

Suggested change

b.iter(|| serde_json::from_str::<serde_json::Value>(black_box(simple_params)))

});

group.bench_function("parse_complex", |b| {

b.iter(|| serde_json::from_str::<serde_json::Value>(black_box(complex_params)))

b.iter(|| serde_json::from_str::<serde_json::Value>(black_box(simple_params)).expect("simple_params must be valid JSON"))

});

group.bench_function("parse_complex", |b| {

b.iter(|| serde_json::from_str::<serde_json::Value>(black_box(complex_params)).expect("complex_params must be valid JSON"))

Fixed in 219a2ce — .unwrap() added to JSON parse results.

Copilot · 2026-03-10T07:16:41Z

 COPY channels-src/ channels-src/
 COPY wit/ wit/
 COPY providers.json providers.json
+COPY benches/ benches/


COPY benches/ benches/ isn’t needed for cargo build --release --bin ironclaw and will invalidate Docker layer caching whenever benchmarks change, increasing image build time. Consider dropping this copy (or moving it behind a build arg / separate stage) unless the image build needs to compile/run benches.

Suggested change

COPY benches/ benches/

Kept with comment — [[bench]] entries require source files for manifest parsing, so removing COPY breaks the Docker build.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

- Move header string allocations outside b.iter() closure in http_request_scan to avoid measuring allocation overhead - Add .unwrap() to serde_json::from_str results in JSON parsing benchmarks to catch invalid JSON instead of silently benchmarking error construction - Add comment explaining why benches/ COPY is needed in Dockerfile ([[bench]] entries require source files for cargo manifest parsing) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Copilot

Pull request overview

Copilot reviewed 5 out of 5 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 · 2026-03-10T07:51:37Z

+        b.iter(|| layer.sanitize_tool_output(black_box("shell"), black_box(clean_tool_output)))
+    });
+
+    group.bench_function("pipeline_adversarial", |b| {
+        b.iter(|| {
+            layer.sanitize_tool_output(black_box("shell"), black_box(adversarial_tool_output))
+        })
+    });
+
+    group.bench_function("pipeline_with_secret", |b| {
+        b.iter(|| layer.sanitize_tool_output(black_box("shell"), black_box(&output_with_secret)))


Most b.iter closures discard the return value from sanitize_tool_output, so the optimizer may be able to eliminate some/all of the work. Feed the result into criterion::black_box (or return it from the closure) to ensure the pipeline actually runs during the benchmark.

Suggested change

b.iter(|| layer.sanitize_tool_output(black_box("shell"), black_box(clean_tool_output)))

});

group.bench_function("pipeline_adversarial", |b| {

b.iter(|| {

layer.sanitize_tool_output(black_box("shell"), black_box(adversarial_tool_output))

})

});

group.bench_function("pipeline_with_secret", |b| {

b.iter(|| layer.sanitize_tool_output(black_box("shell"), black_box(&output_with_secret)))

b.iter(|| black_box(layer.sanitize_tool_output(black_box("shell"), black_box(clean_tool_output))))

});

group.bench_function("pipeline_adversarial", |b| {

b.iter(|| {

black_box(layer.sanitize_tool_output(

black_box("shell"),

black_box(adversarial_tool_output),

))

})

});

group.bench_function("pipeline_with_secret", |b| {

b.iter(|| black_box(layer.sanitize_tool_output(black_box("shell"), black_box(&output_with_secret))))

All b.iter() closures return the value (no trailing ;). Criterion applies black_box() to return values automatically.

Copilot · 2026-03-10T07:51:37Z

+    });
+
+    group.bench_function("pipeline_with_secret", |b| {
+        b.iter(|| layer.sanitize_tool_output(black_box("shell"), black_box(&output_with_secret)))


The pipeline_with_secret benchmark also drops the returned SanitizedOutput. Use black_box on the output (or return it) so LLVM can’t treat the call as dead code.

Suggested change

b.iter(|| layer.sanitize_tool_output(black_box("shell"), black_box(&output_with_secret)))

b.iter(|| black_box(layer.sanitize_tool_output(black_box("shell"), black_box(&output_with_secret))))

Same — closure returns the value, Criterion handles black_box.

Copilot · 2026-03-10T07:51:38Z

+        b.iter(|| layer.wrap_for_llm(black_box("shell"), black_box(clean_tool_output), false))
+    });
+
+    // Benchmark inbound secret scanning
+    group.bench_function("scan_inbound_clean", |b| {
+        b.iter(|| layer.scan_inbound_for_secrets(black_box("Hello, help me code")))
+    });
+
+    group.bench_function("scan_inbound_with_secret", |b| {
+        b.iter(|| layer.scan_inbound_for_secrets(black_box(&output_with_secret)))


These benchmarks discard the results of wrap_for_llm / scan_inbound_for_secrets. To avoid measuring an optimized-away fast path, pass the returned value to black_box (or return it from the closure).

Suggested change

b.iter(|| layer.wrap_for_llm(black_box("shell"), black_box(clean_tool_output), false))

});

// Benchmark inbound secret scanning

group.bench_function("scan_inbound_clean", |b| {

b.iter(|| layer.scan_inbound_for_secrets(black_box("Hello, help me code")))

});

group.bench_function("scan_inbound_with_secret", |b| {

b.iter(|| layer.scan_inbound_for_secrets(black_box(&output_with_secret)))

b.iter(|| black_box(layer.wrap_for_llm(black_box("shell"), black_box(clean_tool_output), false)))

});

// Benchmark inbound secret scanning

group.bench_function("scan_inbound_clean", |b| {

b.iter(|| black_box(layer.scan_inbound_for_secrets(black_box("Hello, help me code"))))

});

group.bench_function("scan_inbound_with_secret", |b| {

b.iter(|| black_box(layer.scan_inbound_for_secrets(black_box(&output_with_secret))))

Same — return values consumed by Criterion.

Copilot · 2026-03-10T07:51:38Z

+    group.bench_function("normal_input", |b| {
+        b.iter(|| validator.validate(black_box(normal_input)))
+    });
+
+    group.bench_function("long_input", |b| {
+        b.iter(|| validator.validate(black_box(&long_input)))
+    });
+
+    group.bench_function("whitespace_heavy", |b| {
+        b.iter(|| validator.validate(black_box(&whitespace_heavy)))
+    });


The validator benchmarks drop the ValidationResult returned by validate, which can allow the optimizer to remove or reduce the work being measured. black_box the result (or return it) to keep the benchmark meaningful.

Same — ValidationResult returned from closure.

Copilot · 2026-03-10T07:51:38Z

+    group.bench_function("clean_content", |b| {
+        b.iter(|| detector.scan(black_box(clean_content)))
+    });
+
+    group.bench_function("content_with_secrets", |b| {
+        b.iter(|| detector.scan(black_box(&content_with_secrets)))
+    });
+
+    group.bench_function("large_clean", |b| {
+        b.iter(|| detector.scan(black_box(&large_clean)))
+    });
+
+    group.bench_function("scan_and_clean", |b| {
+        b.iter(|| detector.scan_and_clean(black_box(clean_content)))
+    });
+
+    let headers = vec![
+        ("Content-Type".to_string(), "application/json".to_string()),
+        ("Accept".to_string(), "text/html".to_string()),
+    ];
+    group.bench_function("http_request_scan", |b| {
+        b.iter(|| {
+            detector.scan_http_request(
+                "https://api.example.com/data?query=hello",
+                black_box(&headers),
+                Some(b"{\"query\": \"hello world\"}"),
+            )
+        })
+    });


Leak detector benchmark iterations discard the results of scan / scan_and_clean / scan_http_request. Since these functions are side-effect free on clean inputs, the compiler may elide work; ensure the return values are consumed via black_box (or returned) to prevent dead-code elimination.

Same — leak detector results returned from closures.

Copilot · 2026-03-10T07:51:38Z

+    let output_with_secret = format!(
+        "Config found:\nAWS_ACCESS_KEY_ID=AKIAIOSFODNN7EXAMPLE\ntoken=ghp_{}",
+        "x".repeat(36)


The benchmark inputs include strings that look like real credentials (AWS access key ID and a GitHub token-formatted ghp_ value with the typical length). These often trigger secret scanners and can block pushes/CI; prefer non-matching placeholders or construct the token pattern in a way that won’t be detected as a real token in source (while still exercising LeakDetector).

Suggested change

let output_with_secret = format!(

"Config found:\nAWS_ACCESS_KEY_ID=AKIAIOSFODNN7EXAMPLE\ntoken=ghp_{}",

"x".repeat(36)

let fake_aws_access_key_id = ["AKIA", "IOSF", "ODNN", "7EXA", "MPLE"].join("");

let fake_ghp_token_prefix = ["gh", "p"].join("");

let fake_ghp_token = format!("{}{}", fake_ghp_token_prefix, "x".repeat(36));

let output_with_secret = format!(

"Config found:\nAWS_ACCESS_KEY_ID={}\ntoken={}",

fake_aws_access_key_id,

fake_ghp_token

Fixed in ae2a7d0 — secret strings built at runtime via format!() to avoid triggering CI scanners.

Copilot · 2026-03-10T07:51:38Z

+    let content_with_secrets = format!(
+        "Output: AKIAIOSFODNN7EXAMPLE and ghp_{} found in config",


This benchmark constructs strings that match common secret formats (AWS access key ID + GitHub ghp_ token length). These often get flagged by secret scanning and can block the PR; use placeholders that don’t match scanners, or assemble the pattern in a way that avoids static detection while still exercising LeakDetector.

Suggested change

let content_with_secrets = format!(

"Output: AKIAIOSFODNN7EXAMPLE and ghp_{} found in config",

// Construct secret-like patterns from smaller fragments to avoid static detection

let aws_access_key_id = ["AKIA", "IOSFODNN7EXAMPLE"].concat();

let github_token_prefix = ["gh", "p_"].concat();

let content_with_secrets = format!(

"Output: {} and {}{} found in config",

aws_access_key_id,

github_token_prefix,

Fixed in ae2a7d0 — same runtime construction in safety_check.rs.

Copilot · 2026-03-10T07:51:39Z

+    group.bench_function("clean_input", |b| {
+        b.iter(|| sanitizer.sanitize(black_box(clean_input)))
+    });
+
+    group.bench_function("adversarial_input", |b| {
+        b.iter(|| sanitizer.sanitize(black_box(adversarial_input)))
+    });
+
+    group.bench_function("detect_only", |b| {
+        b.iter(|| sanitizer.detect(black_box(adversarial_input)))
+    });


The sanitizer benchmarks drop the return values from sanitize/detect, so the compiler may optimize away the work. Use black_box on the returned value (or return it from the closure) to ensure the operations are actually executed.

Same — closure returns value, Criterion handles black_box.

Copilot · 2026-03-10T07:51:39Z

+    });
+
+    group.bench_function("tool_params", |b| {
+        b.iter(|| validator.validate_tool_params(black_box(&params)))


validate_tool_params returns a ValidationResult that is currently ignored. Use black_box on the result (or return it) so the recursive string-walk work can’t be optimized away.

Suggested change

b.iter(|| validator.validate_tool_params(black_box(&params)))

b.iter(|| black_box(validator.validate_tool_params(black_box(&params))))

Same — validate_tool_params result returned from closure.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

zmanian

Overall this is a solid addition. The safety layer is a hot path and having regression benchmarks for it is valuable. A few items to address:

Naming: tool_dispatch.rs is misleading

This file benchmarks the SafetyLayer pipeline and JSON parsing, not tool dispatch (registry lookup, trait dispatch, etc.). Suggest renaming to safety_pipeline.rs or safety_layer.rs to match what it actually measures. The benchmark group name safety_pipeline already reflects the real content.

Unused async_tokio feature on criterion

Neither bench file uses async benchmarks. The features = ["async_tokio"] on the criterion dependency pulls in unnecessary compile-time cost. Drop it:

criterion = { version = "0.5" }

If async benches are planned for the future, add the feature then.

JSON parsing benchmarks are low-signal

bench_json_tool_params benchmarks serde_json::from_str -- that's a third-party library with its own benchmarks. This doesn't exercise any ironclaw code. Consider replacing with a benchmark that actually exercises tool-related ironclaw code (e.g., ToolRegistry::get lookup, parameter schema validation against a tool's declared schema, or validate_tool_params on adversarial inputs).

Missing edge cases in benchmark inputs

The leak detector benchmarks are good but could benefit from a "many false positives" case -- content with strings that look like keys but aren't (e.g., base64-encoded data, hex strings of similar length to API keys). This is where regex-based detection tends to regress in performance.

The validator benchmarks don't test deeply nested JSON for validate_tool_params, which is a realistic concern for complex tool schemas.

CI job: cargo bench --no-run will compile with default features only

The bench-compile CI job runs cargo bench --no-run without specifying features. If the bench files import types that are behind feature gates, this could silently succeed or fail depending on defaults. Consider using --all-features to match the project's test CI convention, or at minimum document the assumption.

Dockerfile change is correct

The COPY benches/ benches/ addition is necessary since [[bench]] entries in Cargo.toml cause manifest parsing to fail if the sources don't exist. Good catch.

Minor: no #[allow(unused)] or conditional compilation concerns

Both bench files compile cleanly against the public API. The imports look correct.

…canners Construct AWS key and GitHub token patterns via format!() concatenation so the literal strings don't appear in source and trigger push protection or secret scanning in CI pipelines. The resulting strings still match LeakDetector patterns for valid benchmarking. [skip-regression-check] Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

zmanian

Re-reviewing after the latest commit (ae2a7d0). The runtime secret string construction is a good fix for the CI scanner issue.

Items from previous review that remain unaddressed:

tool_dispatch.rs naming is still misleading. The file benchmarks SafetyLayer pipeline and JSON parsing -- not tool dispatch. Please rename to safety_pipeline.rs (and update the [[bench]] entry in Cargo.toml). This is a small change and avoids confusion for anyone running cargo bench in the future.
async_tokio feature still on criterion. Neither bench file uses async. This pulls in unnecessary dependencies. Change to:
```
criterion = { version = "0.5" }
```
JSON parsing benchmarks (bench_json_tool_params) are still low-signal. These benchmark serde_json::from_str which is a third-party library -- not ironclaw code. Replace with something that exercises actual ironclaw logic, e.g., Validator::validate_tool_params with deeply nested JSON, or SafetyLayer::sanitize_tool_output with large payloads near the truncation boundary. If you want to keep them as a baseline reference, at least add a comment explaining the rationale.
CI cargo bench --no-run should specify features. The bench files import from ironclaw::safety and ironclaw::config which may behave differently under feature flags. Use --all-features to match the project's clippy/test CI convention, or at minimum --features postgres,libsql to verify both backends don't break the manifest.

New observations on the latest diff:

Copilot black_box comments are resolved. The author's response that Criterion auto-applies black_box to iter closure return values is correct -- this is documented Criterion behavior. No action needed there.
Dockerfile COPY benches/ is necessary and correctly justified. The [[bench]] manifest entries require sources to exist. The added comment explains this well.
The Copilot comments about routine_engine.rs and tracing_fmt.rs are noise -- those files aren't part of this PR's changes. The author correctly flagged this.

Overall: The benchmarks measure the right things (sanitizer, validator, leak detector, safety pipeline) and the test inputs are well-chosen with clean/adversarial/secret-bearing variants. Items 1-4 above are the remaining action items -- 1 and 2 are trivial, 3 and 4 are worth doing before merge.

…marks 1. Rename `tool_dispatch.rs` → `safety_pipeline.rs` to match actual content (SafetyLayer pipeline benchmarks). 2. Drop unused `async_tokio` feature from criterion dependency. 3. Replace serde_json::from_str benchmarks (third-party only) with Validator::validate_tool_params exercising IronClaw's recursive validation on simple, complex, and deeply nested JSON inputs. 4. Add `--all-features` to CI bench-compile to match clippy/test convention and verify both DB backends. Addresses zmanian's review feedback on PR #836. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

ilblackdragon · 2026-03-12T21:00:40Z

@zmanian — All four items addressed in 0fd8bad:

Renamed tool_dispatch.rs → safety_pipeline.rs and updated the [[bench]] entry in Cargo.toml.
Dropped async_tokio feature from criterion — neither bench file uses async.
Replaced JSON parsing benchmarks with Validator::validate_tool_params exercising IronClaw's recursive validation on simple, complex, and deeply nested JSON inputs.
Added --all-features to CI bench-compile to match the project's clippy/test convention.

Copilot

Pull request overview

Copilot reviewed 5 out of 6 changed files in this pull request and generated 1 comment.

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

Copilot · 2026-03-12T21:02:52Z

+    // Deeply nested JSON to stress the recursive validation walk
+    let nested_params: serde_json::Value = serde_json::from_str(
+        r#"{
+        "a": {"b": {"c": {"d": {"e": {"f": {"g": {"h": "deep"}}}},
+        "list": [1, 2, {"nested": true, "values": ["x", "y", "z"]}]}}},
+        "command": "echo",
+        "env": {"KEY1": "val1", "KEY2": "val2", "KEY3": "val3", "KEY4": "val4"}
+    }"#,
+    )
+    .unwrap();


Building complex nested JSON via a raw string + .unwrap() is brittle (easy to introduce mismatched braces/commas) and failures only show up at bench runtime (the CI job here only compiles). Prefer constructing nested_params with serde_json::json!({...}) (or Value builders) so the structure is validated by Rust’s syntax and is easier to edit safely.

zmanian

Good addition. The benchmarks target the right hot paths -- sanitizer, validator, leak detector, and the full SafetyLayer pipeline are all on the critical path for every inbound/outbound message.

Code observations:

Benchmarks are well-structured. Good use of black_box, meaningful input variants (clean vs adversarial vs secret-bearing), and the runtime-built secret strings to avoid tripping CI scanners is a nice touch.
.unwrap() usage in benchmarks is fine per project standards (only forbidden in production code).
Criterion dep is dev-only -- correctly placed under [dev-dependencies] in Cargo.toml. The transitive dep footprint (ciborium, plotters, rayon, etc.) is standard for Criterion 0.5 and acceptable for dev-only.
Dockerfile change is correct -- [[bench]] entries in Cargo.toml require the source files to exist for cargo to parse the manifest, even for non-bench builds.
CI job is compile-only (--no-run) which is the right choice -- avoids flaky timing-dependent failures in CI while still catching compilation regressions.

One note:

Merge conflict: This PR has CONFLICTING mergeable status. Needs a rebase onto the target branch before merge.

LGTM pending rebase.

…chmarks # Conflicts: # Cargo.lock

* feat: add Criterion benchmarks for safety layer hot paths Add benchmark suite using Criterion.rs for performance-critical paths: - benches/safety_check.rs: Sanitizer (clean/adversarial), Validator (normal/long/tool params), LeakDetector (clean/secrets/HTTP scan) - benches/tool_dispatch.rs: JSON parsing, schema validation patterns, tool output serialization CI compiles benchmarks on every PR to prevent regressions. Run locally with: cargo bench Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: add bench-compile to CI roll-up job Include bench-compile in the run-tests roll-up job's needs array so benchmark compilation failures block PRs. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: add black_box to benchmarks, use real SafetyLayer pipeline - Wrap all benchmark inputs in criterion::black_box to prevent compiler optimization from skewing results - Replace generic JSON benchmarks in tool_dispatch.rs with actual SafetyLayer pipeline benchmarks (sanitize_tool_output, wrap_for_llm, scan_inbound_for_secrets) - Keep JSON parsing benchmarks for tool parameter overhead measurement Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: apply cargo fmt to benchmark files Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: copy benches/ in Dockerfile to fix manifest parse error Cargo.toml references [[bench]] targets that must exist for manifest parsing to succeed. Add COPY benches/ to the Docker build stage. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * chore: re-trigger CI after adding skip-regression-check label Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: address PR review comments on criterion benchmarks - Move header string allocations outside b.iter() closure in http_request_scan to avoid measuring allocation overhead - Add .unwrap() to serde_json::from_str results in JSON parsing benchmarks to catch invalid JSON instead of silently benchmarking error construction - Add comment explaining why benches/ COPY is needed in Dockerfile ([[bench]] entries require source files for cargo manifest parsing) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * chore: update Cargo.lock with criterion dependencies Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(bench): build secret-like strings at runtime to avoid CI secret scanners Construct AWS key and GitHub token patterns via format!() concatenation so the literal strings don't appear in source and trigger push protection or secret scanning in CI pipelines. The resulting strings still match LeakDetector patterns for valid benchmarking. [skip-regression-check] Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: rename tool_dispatch bench, drop async_tokio, replace JSON benchmarks 1. Rename `tool_dispatch.rs` → `safety_pipeline.rs` to match actual content (SafetyLayer pipeline benchmarks). 2. Drop unused `async_tokio` feature from criterion dependency. 3. Replace serde_json::from_str benchmarks (third-party only) with Validator::validate_tool_params exercising IronClaw's recursive validation on simple, complex, and deeply nested JSON inputs. 4. Add `--all-features` to CI bench-compile to match clippy/test convention and verify both DB backends. Addresses zmanian's review feedback on PR nearai#836. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>

Copilot AI review requested due to automatic review settings March 10, 2026 06:08

fix: add bench-compile to CI roll-up job

70c80e2

Include bench-compile in the run-tests roll-up job's needs array so benchmark compilation failures block PRs. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Copilot started reviewing on behalf of ilblackdragon March 10, 2026 06:11

View session

Copilot AI reviewed Mar 10, 2026

View reviewed changes

github-actions Bot added size: L 200-499 changed lines risk: medium Business logic, config, or moderate-risk modules and removed size: XL 500+ changed lines risk: high Safety, secrets, auth, or critical infrastructure labels Mar 10, 2026

Copilot started reviewing on behalf of ilblackdragon March 10, 2026 07:12

View session

Copilot AI reviewed Mar 10, 2026

View reviewed changes

ilblackdragon added the skip-regression-check Bypass regression test CI gate (tests exist but not in tests/ dir) label Mar 10, 2026

ilblackdragon and others added 2 commits March 10, 2026 00:18

chore: re-trigger CI after adding skip-regression-check label

446dd31

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Copilot AI review requested due to automatic review settings March 10, 2026 07:47

Copilot started reviewing on behalf of ilblackdragon March 10, 2026 07:47

View session

Copilot AI reviewed Mar 10, 2026

View reviewed changes

chore: update Cargo.lock with criterion dependencies

7f3fe8b

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

zmanian reviewed Mar 10, 2026

View reviewed changes

Copilot AI review requested due to automatic review settings March 12, 2026 21:00

Copilot started reviewing on behalf of ilblackdragon March 12, 2026 21:01

View session

Copilot AI reviewed Mar 12, 2026

View reviewed changes

zmanian previously approved these changes Mar 12, 2026

View reviewed changes

Merge remote-tracking branch 'origin/staging' into feat/criterion-ben…

9722a8c

…chmarks # Conflicts: # Cargo.lock

ilblackdragon dismissed zmanian’s stale review via 9722a8c March 14, 2026 20:47

ilblackdragon merged commit 15ab156 into staging Mar 15, 2026
14 checks passed

ilblackdragon deleted the feat/criterion-benchmarks branch March 15, 2026 03:26

This was referenced Mar 15, 2026

chore: promote staging to staging-promote/c79754df-23099429381 (2026-03-15 04:45 UTC) #1192

Merged

chore: release v0.19.0 #973

Merged

	b.iter(\|\| layer.sanitize_tool_output(black_box("shell"), black_box(&output_with_secret)))
	b.iter(\|\| black_box(layer.sanitize_tool_output(black_box("shell"), black_box(&output_with_secret))))

-    let output_with_secret = format!(
-        "Config found:\nAWS_ACCESS_KEY_ID=AKIAIOSFODNN7EXAMPLE\ntoken=ghp_{}",
-        "x".repeat(36)
+    let fake_aws_access_key_id = ["AKIA", "IOSF", "ODNN", "7EXA", "MPLE"].join("");
+    let fake_ghp_token_prefix = ["gh", "p"].join("");
+    let fake_ghp_token = format!("{}{}", fake_ghp_token_prefix, "x".repeat(36));
+    let output_with_secret = format!(
+        "Config found:\nAWS_ACCESS_KEY_ID={}\ntoken={}",
+        fake_aws_access_key_id,
+        fake_ghp_token

		let content_with_secrets = format!(
		"Output: AKIAIOSFODNN7EXAMPLE and ghp_{} found in config",

-    let content_with_secrets = format!(
-        "Output: AKIAIOSFODNN7EXAMPLE and ghp_{} found in config",
+    // Construct secret-like patterns from smaller fragments to avoid static detection
+    let aws_access_key_id = ["AKIA", "IOSFODNN7EXAMPLE"].concat();
+    let github_token_prefix = ["gh", "p_"].concat();
+    let content_with_secrets = format!(
+        "Output: {} and {}{} found in config",
+        aws_access_key_id,
+        github_token_prefix,

	b.iter(\|\| validator.validate_tool_params(black_box(&params)))
	b.iter(\|\| black_box(validator.validate_tool_params(black_box(&params))))

Conversation

ilblackdragon commented Mar 10, 2026

Summary

Test plan

Uh oh!

gemini-code-assist Bot commented Mar 10, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Mar 10, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 10, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 10, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Mar 10, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 10, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 10, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Mar 10, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 10, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 10, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 10, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 10, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 10, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!