feat(coding): omp core-tool contract + engines + benchmark arm (issue #7392, slices 1-4) - #7435
serrrfirat wants to merge 7 commits into
Conversation
Add the offline provenance, exact prompt/schema fixtures, rendered read description, golden case inventories, and reusable differential comparison seam for the first slice of nearai#7392.
…esystem Unregistered omp-parity engines (issue nearai#7392 slice 2): hashline edit with run-scoped snapshot tags and CAS writes, omp-compatible read (files+dirs), write, glob, grep, xxHash32 tag parity, plus the harness bin driving 25 engine tests against the pinned fixtures. Fixture correction: invalid_absolute_range renders '.=' per pinned source.
Additive provider_tool_name override on capability declarations/descriptors honored at the model-provider boundary (issue nearai#7392 provider-name resolver; derived spelling stays resolvable), plus a test-support-gated seam registering read/write/edit/glob/grep over the first-party capability path, and a turn-level integration bin proving the exact surface, read->edit->read flow, back-compat alias, and the approval gate.
…benchmark panel TEMPORARY benchmark override (issue nearai#7392): the process-backend builtin package and handler builders now include the five omp capabilities under the exact pinned names, so the /benchmark pinchbench harness (production binary, no env vars) serves the omp surface. Marker comments track the revert-at-cutover point where the old tools are removed and this becomes the only surface. Harness/stock builders stay unchanged (zero golden-payload churn); new host_runtime test pins the production surface; workspace-scoping probes adapted to the omp glob/grep contract.
…rk policy The omp-extended package (TEMPORARY benchmark override) adds builtin.read/write/edit, but the standalone capability policy only granted the old coding ids, so the new tools were filtered from the model surface — the /benchmark arm advertised only glob/grep. Adds the three grants mirroring read_file/write_file.
… core TEMPORARY benchmark override: the omp tools were in the package and granted, but the disclosure bridged mode only advertises CORE_TOOL_NAMES directly, so read/write/edit were hidden behind tool_describe and the /benchmark arm never exercised them.
TEMPORARY benchmark override: remove read_file/write_file/list_dir/apply_patch from the flip package so the model surface is exactly the five omp tools — the smoke showed models keep calling the old names when both surfaces coexist, diluting the A/B.
github-actions
Bot
added
scope: docs
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdded OMP parity coding tools with filesystem engines, pinned schemas and prompts, provider-facing names, runtime registration, approval-aware harness support, and comprehensive contract, engine, and integration tests. ChangesOMP coding runtime
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1⚔️ Resolve merge conflicts 💡
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
🧭 IronLoop Run · ReviewThis comment updates in place as the Run moves through its stages. 🟩 Final result · Completed
Automatic trigger · attempt 1 of 3 · completed in 34m 44s IronLoop completed the review and posted it to GitHub. 🔗 Result |
|
/benchmark claw-swe-bench-lite |
|
🧪 Started |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 24
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/app/ironclaw_composition/src/runtime/capability_host/workspace_scoping_tests.rs (1)
398-422: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftA fresh user's first
glob/grepnow errors in production.Before this PR,
builtin.globandbuiltin.grepreturned empty successful results when the caller's workspace root did not exist yet. They now return a recoverable failure carryingPath not found. Because the omp override is unconditional (crates/app/ironclaw_composition/src/factory.rsLines 1180-1184 callsomp_coding_packagefrom the production factory, andomp.rshas no#[cfg]gate), every new user on a served instance hits this on their first search, not only the/benchmarkarm.Confirm this is the intended shipped behavior. If it is benchmark-only, gate the package selection. If it ships, an empty-result path for a not-yet-created workspace root is the better model-facing contract than an error.
Separately, the two assertions match different message families by substring: the omp glob form is
Path not found: <path>(tests/reborn_omp_coding_engines.rsLine 768) and the grep form isPath not found for line-range selector: ...(Line 891). Pin the exact expected message per tool so a message change cannot pass silently.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/app/ironclaw_composition/src/runtime/capability_host/workspace_scoping_tests.rs` around lines 398 - 422, Resolve the unconditional omp package selection in the production factory so fresh workspaces preserve the intended model-facing behavior: return empty successful glob/grep results for a missing root, or gate the omp selection if it is benchmark-only. Update the workspace-scoping test around invoke_workspace_tool_as and assert_tool_failed_containing to assert each tool’s exact expected error message separately, rather than relying on broad shared substrings.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Cargo.toml`:
- Around line 486-489: Rename the Cargo integration-test target from
reborn_omp_registration to reborn_integration_omp_registration, and update every
matching reference in the repository, including CI scripts and testing
documentation, while preserving the existing test path.
In
`@crates/app/ironclaw_architecture_tests/tests/reborn_dependency_boundaries.rs`:
- Around line 1800-1803: Update the comment above the omp.rs allowlist entry to
remove the incorrect “Test-support-gated” and “compiled out of production
binaries” claims, and describe it instead as an unconditional temporary
production registration seam for the `#7392` benchmark arm that must be removed at
cutover.
In `@crates/contracts/ironclaw_host_api/src/capability.rs`:
- Around line 266-273: Preserve ProviderToolName validation throughout the
resolved descriptor path instead of converting it back to String. In
crates/contracts/ironclaw_host_api/src/capability.rs:266-273, change the field
to Option<ProviderToolName> (or provide equivalent fallible deserialization); in
crates/extensions/ironclaw_extension_registry/src/v2.rs:582-590, store the typed
value in CapabilityDeclV2; at 1211-1226, retain the ProviderToolName::new
result; and at 1426, propagate that typed value.
In
`@crates/extensions/ironclaw_extension_support/src/coding/omp/assets/prompts/grep.md`:
- Line 1: Update the capability claims in grep.md, including the summary and
line-search behavior, to reflect that search_file uses only Rust
regex::RegexBuilder and splits content on newline boundaries for line-by-line
matching. Remove the PCRE2 fallback and cross-line matching claims, or
explicitly document the module deviation consistent with grep.rs.
In
`@crates/extensions/ironclaw_extension_support/src/coding/omp/assets/prompts/read.rendered.md`:
- Line 1: Update the OMP read prompt to remove claims that `path` can read web
URLs or `ssh://host/<path>` resources, keeping only currently supported resource
types. Do not advertise dispatch behavior for unsupported network or SSH access
unless implementing the required typed handlers and security controls.
In
`@crates/extensions/ironclaw_extension_support/src/coding/omp/assets/prompts/write.md`:
- Around line 6-7: Update the write prompt to remove the archive-entry and
SQLite row-operation claims, leaving only capabilities currently implemented by
the plain-file write flow around ctx.filesystem.put(...). Keep the supported
plain-file behavior and any unrelated prompt guidance unchanged.
In `@crates/extensions/ironclaw_extension_support/src/coding/omp/grep.rs`:
- Around line 355-361: Eliminate the redundant file reads in record_snapshot_tag
by having search_file return the normalized decoded text or computed snapshot
tag on FileHits. Update the hit-processing blocks around sections.push and the
other referenced call sites to use that stored value when constructing
header_suffix, while preserving the existing behavior for empty hits.
- Around line 645-648: Update PathSpec and the range validation loop to retain
the resolved canonical virtual path, then change the lookup near the ranges
assignment to match that path by identity instead of comparing spec.clean with
display. Preserve range selection for bare, ./, absolute, and workspace-prefixed
inputs, and add a caller-level test using ./foo.txt:1-2 that verifies only lines
1–2 are searched.
In `@crates/extensions/ironclaw_extension_support/src/coding/omp/mod.rs`:
- Around line 200-204: Gate the `harness` module with `#[cfg(any(test, feature =
"test-support"))]` so it is excluded from production builds, and update the test
binary configuration that consumes it to require the `test-support` feature. Use
exactly the existing `test-support` seam without introducing another feature or
changing the module’s internals.
In `@crates/extensions/ironclaw_extension_support/src/coding/omp/read.rs`:
- Around line 396-398: Apply EXCLUDED_DIRS during recursive directory traversal
in the frontier descent and bucketing logic, ensuring excluded names such as
node_modules and target are never enqueued or rendered. Remove the dead-code
allowance once the constant is used, and preserve the documented parity with the
native walker in the surrounding tree assembly functions.
In `@crates/extensions/ironclaw_extension_support/src/coding/omp/selector.rs`:
- Around line 176-198: Update the line-range parsing logic around raw_end
parsing and the canonical_sep "+" branch: preserve the parse error instead of
converting an overflowing RHS with .ok(), and return Ok(None) for that non-range
input; also replace the unchecked raw_start + count - 1 arithmetic with checked
addition and return Ok(None) when it overflows, while preserving normal
valid-range behavior.
In `@crates/extensions/ironclaw_extension_support/src/coding/omp/state.rs`:
- Around line 199-213: Expose a test-only way to construct OmpSnapshotRegistry
with a custom entry limit, then update bounded_registry_evicts_oldest to use a
small bound that actually triggers the FIFO eviction branch. Assert the oldest
key is absent and the newest key remains recorded, covering consistent
entries/order state, and rename the test to reflect actual eviction behavior.
In `@crates/extensions/ironclaw_extension_support/src/coding/omp/write.rs`:
- Around line 88-98: Hoist the loose-header regex used by strip_write_content
into a module-level LazyLock, matching the existing sibling regex pattern.
Update strip_write_content to reuse that shared compiled regex instead of
constructing it on each call, while preserving the current pattern and matching
behavior.
In `@crates/kernel/ironclaw_host_runtime/src/first_party_tools/mod.rs`:
- Around line 850-857: Update bounded_input_size_with_max to preserve the
serde_json::to_vec serialization error by passing the original error into the
existing error constructor, while retaining a sanitized boundary-facing message.
Do not discard the source error through map_err(|_| input_error()).
In `@crates/kernel/ironclaw_host_runtime/src/first_party_tools/omp.rs`:
- Around line 194-202: Keep the six-ID removal filter in omp_coding_package at
crates/kernel/ironclaw_host_runtime/src/first_party_tools/omp.rs:194-202 and
update its module documentation at Lines 13-20 to no longer claim the legacy
tools remain registered. In
tests/integration/support/harness/profiles/omp_coding.rs:32-48, remove the four
legacy capability grants and outdated comment, then remove their unused imports
at Lines 20-22. In tests/integration/reborn_omp_registration.rs:139-160, assert
that builtin__read_file, builtin__write_file, builtin__list_dir, and
builtin__apply_patch are absent, and update the module documentation at Lines
12-15.
- Around line 339-345: Update omp_error to bound the model-visible
OmpEngineError::message() diagnostic using super::FIRST_PARTY_MAX_OUTPUT_BYTES,
matching the success path’s limit. Truncate safely at a valid UTF-8 character
boundary using the repository’s bounded-output helper or equivalent
character-aware iteration, and pass the bounded text to
dispatch_with_diagnostic.
- Around line 13-20: Update the module-level documentation to accurately
describe the behavior of omp_coding_package: remove the claim that read_file,
write_file, list_dir, and apply_patch remain registered unchanged, and state
that these tools are removed, consistent with the retain filter and the
documentation near omp_coding_package.
- Around line 285-291: Update the OmpEngineContext construction in the dispatch
flow to reject requests whose request.mounts is absent instead of substituting
unwrap_or_default(). Return the existing host dispatch error type with a clear
missing-mount-grant message, while preserving normal context construction when a
MountView is present.
In `@crates/loop/ironclaw_loop_host/src/capability_port.rs`:
- Around line 4905-4954: Add a caller-level test through
HostRuntimeLoopCapabilityPort using a capability descriptor with
provider_tool_name set; verify visible_capabilities advertises the exact
override in its ProviderToolDefinition, then confirm register_provider_tool_call
accepts both the override and preserved derived alias. Exercise the real caller
path rather than only resolve_provider_tool_name, including any required
side-effect gate setup.
In `@tests/CLAUDE.md`:
- Around line 65-66: Update the stale registered flat-bin count in the coverage
documentation from 55 to 56, keeping it consistent with the 56 flat integration
bins reported near the totals summary.
In `@tests/integration/reborn_omp_registration.rs`:
- Around line 256-289: Update the scripted tool call in
omp_derived_spelling_still_resolves to invoke the derived double-underscore
spelling builtin__read instead of the canonical builtin.read identifier, while
preserving the existing assertions and test setup.
In `@tests/integration/support/capability_backend.rs`:
- Around line 265-280: In the RebornCapabilityBackend::OmpCodingTools branch,
validate that keyed_http_responses, web_access_response_bodies,
github_network_statuses, and real_egress_response_bodies are empty before
constructing the host runtime. Return a configuration error for any non-empty
input, alongside the existing shell_mode and park_capability_gate checks, so
unsupported scripted egress configuration fails during construction.
In `@tests/reborn_omp_coding_engines.rs`:
- Around line 55-59: Update the mount-permission coverage in the relevant engine
tests: retain a read-write-only grant and add a REM case that attempts deletion
and asserts the operation is refused. Keep the existing production-equivalent
read_write_list_delete grant for destructive-operation success coverage,
ensuring the tests verify delete permission is actually enforced.
- Around line 262-266: Replace the byte-index slicing in the assertion message
near starts_with and the trailing-output assertion near the later fixture with
character-safe truncation using chars/char_indices, or print the complete text.
Preserve the existing assertion behavior while ensuring multi-byte engine output
cannot panic when constructing failure messages.
---
Outside diff comments:
In
`@crates/app/ironclaw_composition/src/runtime/capability_host/workspace_scoping_tests.rs`:
- Around line 398-422: Resolve the unconditional omp package selection in the
production factory so fresh workspaces preserve the intended model-facing
behavior: return empty successful glob/grep results for a missing root, or gate
the omp selection if it is benchmark-only. Update the workspace-scoping test
around invoke_workspace_tool_as and assert_tool_failed_containing to assert each
tool’s exact expected error message separately, rather than relying on broad
shared substrings.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: eaef3799-0c63-4963-8f67-1732629eceb0
⛔ Files ignored due to path filters (39)
Cargo.lockis excluded by!**/*.lock,!**/Cargo.locktests/fixtures/omp_coding_contract/golden/errors/ast_edit.jsonis excluded by!tests/fixtures/**tests/fixtures/omp_coding_contract/golden/errors/ast_grep.jsonis excluded by!tests/fixtures/**tests/fixtures/omp_coding_contract/golden/errors/edit.jsonis excluded by!tests/fixtures/**tests/fixtures/omp_coding_contract/golden/errors/glob.jsonis excluded by!tests/fixtures/**tests/fixtures/omp_coding_contract/golden/errors/grep.jsonis excluded by!tests/fixtures/**tests/fixtures/omp_coding_contract/golden/errors/read.jsonis excluded by!tests/fixtures/**tests/fixtures/omp_coding_contract/golden/errors/write.jsonis excluded by!tests/fixtures/**tests/fixtures/omp_coding_contract/golden/output/edit.jsonis excluded by!tests/fixtures/**tests/fixtures/omp_coding_contract/golden/output/grep.jsonis excluded by!tests/fixtures/**tests/fixtures/omp_coding_contract/golden/output/read.jsonis excluded by!tests/fixtures/**tests/fixtures/omp_coding_contract/golden/selectors.jsonis excluded by!tests/fixtures/**tests/fixtures/omp_coding_contract/grammars/apply-patch.larkis excluded by!tests/fixtures/**tests/fixtures/omp_coding_contract/grammars/hashline.larkis excluded by!tests/fixtures/**tests/fixtures/omp_coding_contract/licenses/LICENSEis excluded by!tests/fixtures/**tests/fixtures/omp_coding_contract/manifest.jsonis excluded by!tests/fixtures/**tests/fixtures/omp_coding_contract/prompts/apply-patch.mdis excluded by!tests/fixtures/**tests/fixtures/omp_coding_contract/prompts/ast-edit.mdis excluded by!tests/fixtures/**tests/fixtures/omp_coding_contract/prompts/ast-grep.mdis excluded by!tests/fixtures/**tests/fixtures/omp_coding_contract/prompts/glob.mdis excluded by!tests/fixtures/**tests/fixtures/omp_coding_contract/prompts/grep.mdis excluded by!tests/fixtures/**tests/fixtures/omp_coding_contract/prompts/hashline.mdis excluded by!tests/fixtures/**tests/fixtures/omp_coding_contract/prompts/patch.mdis excluded by!tests/fixtures/**tests/fixtures/omp_coding_contract/prompts/read.mdis excluded by!tests/fixtures/**tests/fixtures/omp_coding_contract/prompts/read.rendered.mdis excluded by!tests/fixtures/**tests/fixtures/omp_coding_contract/prompts/replace.mdis excluded by!tests/fixtures/**tests/fixtures/omp_coding_contract/prompts/write.mdis excluded by!tests/fixtures/**tests/fixtures/omp_coding_contract/provenance.jsonis excluded by!tests/fixtures/**tests/fixtures/omp_coding_contract/schemas/ast_edit.jsonis excluded by!tests/fixtures/**tests/fixtures/omp_coding_contract/schemas/ast_grep.jsonis excluded by!tests/fixtures/**tests/fixtures/omp_coding_contract/schemas/edit.jsonis excluded by!tests/fixtures/**tests/fixtures/omp_coding_contract/schemas/glob.jsonis excluded by!tests/fixtures/**tests/fixtures/omp_coding_contract/schemas/grep.jsonis excluded by!tests/fixtures/**tests/fixtures/omp_coding_contract/schemas/read.jsonis excluded by!tests/fixtures/**tests/fixtures/omp_coding_contract/schemas/write.jsonis excluded by!tests/fixtures/**tests/fixtures/omp_coding_contract/sources/match-line-format.tsis excluded by!tests/fixtures/**tests/fixtures/omp_coding_contract/sources/read-path-resolution.tsis excluded by!tests/fixtures/**tests/fixtures/omp_coding_contract/sources/read-selector.tsis excluded by!tests/fixtures/**tests/fixtures/omp_coding_contract/sources/tool-errors.tsis excluded by!tests/fixtures/**
📒 Files selected for processing (81)
Cargo.tomlcrates/app/ironclaw_architecture_tests/tests/reborn_dependency_boundaries.rscrates/app/ironclaw_composition/src/builtin_capability_policy.tomlcrates/app/ironclaw_composition/src/capability_authorization/tests.rscrates/app/ironclaw_composition/src/factory.rscrates/app/ironclaw_composition/src/factory/production_backend_assembly.rscrates/app/ironclaw_composition/src/factory/production_build_assembly.rscrates/app/ironclaw_composition/src/input.rscrates/app/ironclaw_composition/src/product_capability.rscrates/app/ironclaw_composition/src/runtime/capability_host/workspace_scoping_tests.rscrates/app/ironclaw_composition/tests/refreshing_capability_port_test_support.rscrates/contracts/ironclaw_host_api/src/capability.rscrates/extensions/ironclaw_extension_host/src/active.rscrates/extensions/ironclaw_extension_host/src/generic_host.rscrates/extensions/ironclaw_extension_host/src/hosted_mcp_manifest.rscrates/extensions/ironclaw_extension_host/src/mcp.rscrates/extensions/ironclaw_extension_manager/src/admin_configuration_capability.rscrates/extensions/ironclaw_extension_manager/src/extension_lifecycle_capabilities.rscrates/extensions/ironclaw_extension_manager/src/ironhub/capabilities.rscrates/extensions/ironclaw_extension_manager/src/operator_config_capability.rscrates/extensions/ironclaw_extension_manager/src/skill_auto_activate_capability.rscrates/extensions/ironclaw_extension_registry/src/hosted_mcp_discovery.rscrates/extensions/ironclaw_extension_registry/src/package.rscrates/extensions/ironclaw_extension_registry/src/v2.rscrates/extensions/ironclaw_extension_registry/src/v3.rscrates/extensions/ironclaw_extension_support/Cargo.tomlcrates/extensions/ironclaw_extension_support/src/coding/mod.rscrates/extensions/ironclaw_extension_support/src/coding/omp/assets/prompts/glob.mdcrates/extensions/ironclaw_extension_support/src/coding/omp/assets/prompts/grep.mdcrates/extensions/ironclaw_extension_support/src/coding/omp/assets/prompts/hashline.mdcrates/extensions/ironclaw_extension_support/src/coding/omp/assets/prompts/read.rendered.mdcrates/extensions/ironclaw_extension_support/src/coding/omp/assets/prompts/write.mdcrates/extensions/ironclaw_extension_support/src/coding/omp/assets/schemas/edit.jsoncrates/extensions/ironclaw_extension_support/src/coding/omp/assets/schemas/glob.jsoncrates/extensions/ironclaw_extension_support/src/coding/omp/assets/schemas/grep.jsoncrates/extensions/ironclaw_extension_support/src/coding/omp/assets/schemas/read.jsoncrates/extensions/ironclaw_extension_support/src/coding/omp/assets/schemas/write.jsoncrates/extensions/ironclaw_extension_support/src/coding/omp/glob.rscrates/extensions/ironclaw_extension_support/src/coding/omp/grep.rscrates/extensions/ironclaw_extension_support/src/coding/omp/hashline.rscrates/extensions/ironclaw_extension_support/src/coding/omp/mod.rscrates/extensions/ironclaw_extension_support/src/coding/omp/omp_assets.rscrates/extensions/ironclaw_extension_support/src/coding/omp/read.rscrates/extensions/ironclaw_extension_support/src/coding/omp/selector.rscrates/extensions/ironclaw_extension_support/src/coding/omp/state.rscrates/extensions/ironclaw_extension_support/src/coding/omp/write.rscrates/kernel/ironclaw_approvals/src/profile_gate.rscrates/kernel/ironclaw_approvals/tests/approval_resolution_contract.rscrates/kernel/ironclaw_authorization/tests/capability_access_contract.rscrates/kernel/ironclaw_authorization/tests/capability_lease_contract.rscrates/kernel/ironclaw_authorization/tests/runtime_credentials_contract.rscrates/kernel/ironclaw_capabilities/src/registry.rscrates/kernel/ironclaw_host_runtime/src/first_party_tools/mod.rscrates/kernel/ironclaw_host_runtime/src/first_party_tools/omp.rscrates/kernel/ironclaw_host_runtime/src/first_party_tools/schemas.rscrates/kernel/ironclaw_host_runtime/src/lib.rscrates/kernel/ironclaw_host_runtime/src/services/tests.rscrates/kernel/ironclaw_host_runtime/src/services/tests/extension_tool_binder.rscrates/kernel/ironclaw_host_runtime/src/surface.rscrates/kernel/ironclaw_host_runtime/tests/first_party_builtin_tools.rscrates/kernel/ironclaw_host_runtime/tests/first_party_runtime_contract.rscrates/kernel/ironclaw_host_runtime/tests/runtime_policy_planner_contract.rscrates/kernel/ironclaw_runtime_policy/src/planner.rscrates/loop/ironclaw_loop_host/src/capability_port.rscrates/loop/ironclaw_loop_host/src/tool_disclosure.rscrates/loop/ironclaw_loop_host/tests/host_capability_port_composition.rstests/CLAUDE.mdtests/integration/reborn_omp_registration.rstests/integration/support/builder.rstests/integration/support/capability_backend.rstests/integration/support/doubles/recording_delegating_capability_port.rstests/integration/support/group_constructors.rstests/integration/support/harness/mod.rstests/integration/support/harness/options.rstests/integration/support/harness/profiles/mod.rstests/integration/support/harness/profiles/omp_coding.rstests/integration/support/harness_mcp.rstests/reborn_omp_coding_contract_snapshot.rstests/reborn_omp_coding_engines.rstests/support/mod.rstests/support/omp_coding_contract/mod.rs
| [[test]] | ||
| name = "reborn_omp_registration" | ||
| path = "tests/integration/reborn_omp_registration.rs" | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: List existing [[test]] target names whose path is under tests/integration/.
set -euo pipefail
python3 - <<'PY'
import re, pathlib
text = pathlib.Path("Cargo.toml").read_text()
for block in re.findall(r'\[\[test\]\]([^\[]*)', text):
name = re.search(r'name\s*=\s*"([^"]+)"', block)
path = re.search(r'path\s*=\s*"([^"]+)"', block)
if name and path and path.group(1).startswith("tests/integration/"):
print(f"{name.group(1)} <- {path.group(1)}")
PYRepository: nearai/ironclaw
Length of output: 4949
🏁 Script executed:
#!/bin/bash
# Search for the repo testing invariant mentioning reborn_integration_<name>.
set -euo pipefail
rg -n --hidden --glob '!target/**' 'integration-test binary|integration-test binaries|reborn_integration_<name>|reborn_integration_' .Repository: nearai/ironclaw
Length of output: 47202
Rename the integration-test target to match the repo invariant.
.claude/rules/testing.md:30, tests/integration/CLAUDE.md:161, scripts/ci/reborn-coverage-int-tier-tests.sh:9, and docs/internal/testing-playbook.md:267 require integration tests under tests/integration/ be registered as name = "reborn_integration_<name>". reborn_omp_registration violates that invariant; update the target name and any matching references to reborn_integration_omp_registration.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Cargo.toml` around lines 486 - 489, Rename the Cargo integration-test target
from reborn_omp_registration to reborn_integration_omp_registration, and update
every matching reference in the repository, including CI scripts and testing
documentation, while preserving the existing test path.
Source: Coding guidelines
| // Test-support-gated omp registration seam (issue #7392 slice 3): | ||
| // builds the builtin package plus the five omp capabilities with | ||
| // their exact-name overrides; compiled out of production binaries. | ||
| "crates/ironclaw_host_runtime/src/first_party_tools/omp.rs", |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
The allowlist justification claims a gate that does not exist.
The comment states the omp registration seam is "Test-support-gated" and "compiled out of production binaries". crates/kernel/ironclaw_host_runtime/src/first_party_tools/omp.rs carries no #[cfg] gate, its module doc at Lines 22-28 states the surface "is enabled in PRODUCTION builds", and crates/app/ironclaw_composition/src/factory.rs Lines 1180-1184 calls omp_coding_package from the production factory.
This allowlist is a provider-protocol boundary control. A future reviewer reading this entry will conclude the seam cannot reach a shipped binary. Record the real justification: an unconditional, temporary production registration seam for the #7392 benchmark arm, to be removed at cutover.
📝 Proposed fix
- // Test-support-gated omp registration seam (issue `#7392` slice 3):
- // builds the builtin package plus the five omp capabilities with
- // their exact-name overrides; compiled out of production binaries.
+ // ⚠️ TEMPORARY omp registration seam (issue `#7392` slice 3): builds
+ // the builtin package plus the five omp capabilities with their
+ // exact-name overrides. NOT feature-gated — it ships in production
+ // builds for the /benchmark arm and is removed at cutover.
"crates/ironclaw_host_runtime/src/first_party_tools/omp.rs",📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Test-support-gated omp registration seam (issue #7392 slice 3): | |
| // builds the builtin package plus the five omp capabilities with | |
| // their exact-name overrides; compiled out of production binaries. | |
| "crates/ironclaw_host_runtime/src/first_party_tools/omp.rs", | |
| // ⚠️ TEMPORARY omp registration seam (issue `#7392` slice 3): builds | |
| // the builtin package plus the five omp capabilities with their | |
| // exact-name overrides. NOT feature-gated — it ships in production | |
| // builds for the /benchmark arm and is removed at cutover. | |
| "crates/ironclaw_host_runtime/src/first_party_tools/omp.rs", |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/app/ironclaw_architecture_tests/tests/reborn_dependency_boundaries.rs`
around lines 1800 - 1803, Update the comment above the omp.rs allowlist entry to
remove the incorrect “Test-support-gated” and “compiled out of production
binaries” claims, and describe it instead as an unconditional temporary
production registration seam for the `#7392` benchmark arm that must be removed at
cutover.
| /// Optional exact provider-facing tool name (issue #7392 provider-name | ||
| /// resolver). `None` keeps the derived name (`capability_port.rs`'s | ||
| /// `'.' -> "__"` encoding plus collision-digest suffix); when set, the | ||
| /// loop advertises exactly this name while the derived spelling keeps | ||
| /// resolving for back-compat. Additive: `#[serde(default)]` so existing | ||
| /// descriptors/records parse to `None` and serialize unchanged. | ||
| #[serde(default, skip_serializing_if = "Option::is_none")] | ||
| pub provider_tool_name: Option<String>, |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Keep the provider-name type validated across the resolved descriptor path.
The manifest parser validates provider_tool_name, but the resolved models erase ProviderToolName back to String. Direct construction or serde rehydration can bypass the same invariant.
crates/contracts/ironclaw_host_api/src/capability.rs#L266-L273: useOption<ProviderToolName>or add fallible deserialization.crates/extensions/ironclaw_extension_registry/src/v2.rs#L582-L590: store the typed value inCapabilityDeclV2.crates/extensions/ironclaw_extension_registry/src/v2.rs#L1211-L1226: retain the result ofProviderToolName::new.crates/extensions/ironclaw_extension_registry/src/v2.rs#L1426-L1426: propagate the typed value.
As per coding guidelines, use strong types for known domain shapes and keep raw strings at external boundaries; the path invariant also requires specialized Rust domain types for internal values.
📍 Affects 2 files
crates/contracts/ironclaw_host_api/src/capability.rs#L266-L273(this comment)crates/extensions/ironclaw_extension_registry/src/v2.rs#L582-L590crates/extensions/ironclaw_extension_registry/src/v2.rs#L1211-L1226crates/extensions/ironclaw_extension_registry/src/v2.rs#L1426-L1426
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/contracts/ironclaw_host_api/src/capability.rs` around lines 266 - 273,
Preserve ProviderToolName validation throughout the resolved descriptor path
instead of converting it back to String. In
crates/contracts/ironclaw_host_api/src/capability.rs:266-273, change the field
to Option<ProviderToolName> (or provide equivalent fallible deserialization); in
crates/extensions/ironclaw_extension_registry/src/v2.rs:582-590, store the typed
value in CapabilityDeclV2; at 1211-1226, retain the ProviderToolName::new
result; and at 1426, propagate that typed value.
Sources: Coding guidelines, Path instructions
| @@ -0,0 +1,13 @@ | |||
| Searches files and internal URLs with Rust regex plus PCRE2 fallback. | |||
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm no PCRE2 dependency and no cross-line matching path exists.
rg -n 'pcre|fancy[-_]regex' crates/extensions/ironclaw_extension_support/Cargo.toml || echo "no PCRE2-capable engine declared"
rg -nP --type=rust -C3 "split\('\\\\n'\)|multi_line|dot_matches_new_line" crates/extensions/ironclaw_extension_support/src/coding/omp/grep.rsRepository: nearai/ironclaw
Length of output: 496
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== grep.rs outline relevant locations =="
wc -l crates/extensions/ironclaw_extension_support/src/coding/omp/grep.rs crates/extensions/ironclaw_extension_support/src/coding/omp/assets/prompts/grep.md
ast-grep outline crates/extensions/ironclaw_extension_support/src/coding/omp/grep.rs --view compact || true
echo
echo "== grep.rs relevant regex/search sections =="
sed -n '580,680p' crates/extensions/ironclaw_extension_support/src/coding/omp/grep.rs
echo
echo "== prompt/grep.md full =="
cat -n crates/extensions/ironclaw_extension_support/src/coding/omp/assets/prompts/grep.md
echo
echo "== grep.rs crate deps mentioning regex/pcre/fancy =="
rg -n 'fancy[-_]regex|pcre|regex\s*=|regex_builder|RegexBuilder|multi_line|dot_matches_new_line' crates/extensions/ironclaw_extension_support -S
echo
echo "== exact search_file references =="
rg -n --type=rust -C2 "fn search_file|is_match\(line\)|Invalid regex" crates/extensions/ironclaw_extension_support/src/coding/omp/grep.rsRepository: nearai/ironclaw
Length of output: 6541
Align grep.md capability claims with the engine behavior. CLAUDE.md/AGENTS.md/.claude/rules require canonical facts and migration-compatible persisted strings. The repo uses Rust regex::RegexBuilder only, and search_file splits content on '\n' and matches line-by-line. Remove the PCRE2 fallback/cross-line claims from lines 1 and 7, or add module-deviation language here like the module already does in grep.rs.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@crates/extensions/ironclaw_extension_support/src/coding/omp/assets/prompts/grep.md`
at line 1, Update the capability claims in grep.md, including the summary and
line-search behavior, to reflect that search_file uses only Rust
regex::RegexBuilder and splits content on newline boundaries for line-by-line
matching. Remove the PCRE2 fallback and cross-line matching claims, or
explicitly document the module deviation consistent with grep.rs.
| @@ -0,0 +1,27 @@ | |||
| Read files, directories, archives, SQLite, images, documents, internal resources, and web URLs via `path`. | |||
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 6 \
'ssh://|https?://|Url::parse|reqwest|hyper|ironclaw_network|RuntimeHttpEgress|RootFilesystem|private|dns|timeout|limit' \
crates/extensions/ironclaw_extension_support/src/coding/omp || trueRepository: nearai/ironclaw
Length of output: 50371
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== read.prompts.md =="
cat -n crates/extensions/ironclaw_extension_support/src/coding/omp/assets/prompts/read.rendered.md | sed -n '1,200p'
echo
echo "== read.rs outline =="
ast-grep outline crates/extensions/ironclaw_extension_support/src/coding/omp/read.rs --view expanded | sed -n '1,220p'
echo
echo "== read.rs relevant URL/SSH resolution and imports =="
rg -n -C 8 'http|https|ssh|Url::parse|url|scheme|ReadSchema|read_file|read_archive|read_sqlite|read_document|read_resource' crates/extensions/ironclaw_extension_support/src/coding/omp/read.rs | sed -n '1,260p'
echo
echo "== exact repository references to URL/SSH/ironclaw_network in OMP =="
rg -n 'ssh://|https?://|Url::parse|ironclaw_network|RuntimeHttpEgress|private|dns|rebind|allowlist|timeout|limit' crates/extensions/ironclaw_extension_support/src/coding/omp crates/extensions/ironclaw_extension_support/Cargo.toml || true
echo
echo "== module header in omp/mod.rs around scope notes =="
cat -n crates/extensions/ironclaw_extension_support/src/coding/omp/mod.rs | sed -n '1,40p'Repository: nearai/ironclaw
Length of output: 19522
Remove unresolved URL/SSH read claims from the OMP read prompt.
crates/extensions/ironclaw_extension_support/src/coding/omp/mod.rs explicitly lists URLs and SSH as NOT implemented, while read.rendered.md advertises them and dispatch instructions (read.rs) route paths through the mounted RootFilefilesystem. Drop the web URL and ssh://host/<path> claims, or implement typed host-mediated network/runtime handlers with ironclaw_network, private-IP/DNS-rebinding checks, allowlists, timeouts, and bounded output.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@crates/extensions/ironclaw_extension_support/src/coding/omp/assets/prompts/read.rendered.md`
at line 1, Update the OMP read prompt to remove claims that `path` can read web
URLs or `ssh://host/<path>` resources, keeping only currently supported resource
types. Do not advertise dispatch behavior for unsupported network or SSH access
unless implementing the required typed handlers and security controls.
Source: Path instructions
| Totals: **51** group scenarios · **56** flat integration bins (50 in | ||
| `tests/integration/`, 6 in `tests/integration/auth/`) · **41** top-level Rust bins · |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Update the stale flat-bin count.
These lines now report 56 flat integration bins, but Lines 244-245 still say “One of the 55 registered bins”. Change that reference to 56 so the checked-in coverage counts agree.
Proposed correction
-One of the 55 registered bins, `delivery_user_journeys.rs`, holds the explicit
+One of the 56 registered bins, `delivery_user_journeys.rs`, holds the explicit🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/CLAUDE.md` around lines 65 - 66, Update the stale registered flat-bin
count in the coverage documentation from 55 to 56, keeping it consistent with
the 56 flat integration bins reported near the totals summary.
| #[test] | ||
| fn omp_derived_spelling_still_resolves() { | ||
| run_async_test_with_stack("omp_derived_spelling_still_resolves", || async { | ||
| let content = "alpha\nbeta\n"; | ||
| let h = RebornIntegrationHarness::test_default() | ||
| .with_omp_coding_tools() | ||
| .script([ | ||
| RebornScriptedReply::tool_call("builtin.read", json!({ "path": "foo.txt" })), | ||
| RebornScriptedReply::text("read via derived spelling"), | ||
| ]) | ||
| .build() | ||
| .await | ||
| .expect("harness builds"); | ||
| let path = h | ||
| .capability_recorder | ||
| .workspace_file_path("foo.txt") | ||
| .expect("host-runtime harness exposes the workspace root"); | ||
| std::fs::write(&path, content).expect("seed workspace file"); | ||
| h.submit_turn("read the file by its encoded name") | ||
| .await | ||
| .expect("turn completes"); | ||
|
|
||
| let output = output_text( | ||
| &h.tool_result_output("builtin.read") | ||
| .await | ||
| .expect("read result"), | ||
| ); | ||
| assert!( | ||
| output.starts_with("[foo.txt#") | ||
| && output.contains("1:alpha") | ||
| && output.contains("2:beta"), | ||
| "the derived spelling builtin__read resolved to the omp engine: {output}" | ||
| ); | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
This test does not exercise the derived spelling it claims.
The test name, the doc at Lines 253-255, and the assertion message at Line 287 all state that builtin__read resolves. The scripted call at Line 263 uses "builtin.read" — the canonical dotted capability id, not the derived double-underscore provider spelling. The two go through different provider-name resolution paths, so the back-compat claim is untested.
💚 Proposed fix
- RebornScriptedReply::tool_call("builtin.read", json!({ "path": "foo.txt" })),
+ RebornScriptedReply::tool_call("builtin__read", json!({ "path": "foo.txt" })),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #[test] | |
| fn omp_derived_spelling_still_resolves() { | |
| run_async_test_with_stack("omp_derived_spelling_still_resolves", || async { | |
| let content = "alpha\nbeta\n"; | |
| let h = RebornIntegrationHarness::test_default() | |
| .with_omp_coding_tools() | |
| .script([ | |
| RebornScriptedReply::tool_call("builtin.read", json!({ "path": "foo.txt" })), | |
| RebornScriptedReply::text("read via derived spelling"), | |
| ]) | |
| .build() | |
| .await | |
| .expect("harness builds"); | |
| let path = h | |
| .capability_recorder | |
| .workspace_file_path("foo.txt") | |
| .expect("host-runtime harness exposes the workspace root"); | |
| std::fs::write(&path, content).expect("seed workspace file"); | |
| h.submit_turn("read the file by its encoded name") | |
| .await | |
| .expect("turn completes"); | |
| let output = output_text( | |
| &h.tool_result_output("builtin.read") | |
| .await | |
| .expect("read result"), | |
| ); | |
| assert!( | |
| output.starts_with("[foo.txt#") | |
| && output.contains("1:alpha") | |
| && output.contains("2:beta"), | |
| "the derived spelling builtin__read resolved to the omp engine: {output}" | |
| ); | |
| }); | |
| #[test] | |
| fn omp_derived_spelling_still_resolves() { | |
| run_async_test_with_stack("omp_derived_spelling_still_resolves", || async { | |
| let content = "alpha\nbeta\n"; | |
| let h = RebornIntegrationHarness::test_default() | |
| .with_omp_coding_tools() | |
| .script([ | |
| RebornScriptedReply::tool_call("builtin__read", json!({ "path": "foo.txt" })), | |
| RebornScriptedReply::text("read via derived spelling"), | |
| ]) | |
| .build() | |
| .await | |
| .expect("harness builds"); | |
| let path = h | |
| .capability_recorder | |
| .workspace_file_path("foo.txt") | |
| .expect("host-runtime harness exposes the workspace root"); | |
| std::fs::write(&path, content).expect("seed workspace file"); | |
| h.submit_turn("read the file by its encoded name") | |
| .await | |
| .expect("turn completes"); | |
| let output = output_text( | |
| &h.tool_result_output("builtin.read") | |
| .await | |
| .expect("read result"), | |
| ); | |
| assert!( | |
| output.starts_with("[foo.txt#") | |
| && output.contains("1:alpha") | |
| && output.contains("2:beta"), | |
| "the derived spelling builtin__read resolved to the omp engine: {output}" | |
| ); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/integration/reborn_omp_registration.rs` around lines 256 - 289, Update
the scripted tool call in omp_derived_spelling_still_resolves to invoke the
derived double-underscore spelling builtin__read instead of the canonical
builtin.read identifier, while preserving the existing assertions and test
setup.
| RebornCapabilityBackend::OmpCodingTools => { | ||
| if !matches!(shell_mode, ShellMode::Inert) { | ||
| return Err( | ||
| "omp coding harness has no shell capability and does not support \ | ||
| shell mode overrides" | ||
| .into(), | ||
| ); | ||
| } | ||
| if park_capability_gate.is_some() { | ||
| return Err("park_tool_dispatch is only supported by \ | ||
| RebornCapabilityBackend::BuiltinHttpTools" | ||
| .into()); | ||
| } | ||
| let host_runtime = super::harness::profiles::omp_coding::omp_coding_tools().await?; | ||
| GroupCapability::HostRuntime(Arc::new(host_runtime)) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Reject unsupported scripted egress inputs.
OmpCodingTools ignores keyed_http_responses, web_access_response_bodies, github_network_statuses, and real_egress_response_bodies. A test can configure these inputs and then select OMP tools without receiving an error.
Reject non-empty unsupported scripting inputs in this branch. This makes invalid harness configuration fail during construction.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/integration/support/capability_backend.rs` around lines 265 - 280, In
the RebornCapabilityBackend::OmpCodingTools branch, validate that
keyed_http_responses, web_access_response_bodies, github_network_statuses, and
real_egress_response_bodies are empty before constructing the host runtime.
Return a configuration error for any non-empty input, alongside the existing
shell_mode and park_capability_gate checks, so unsupported scripted egress
configuration fails during construction.
Source: Learnings
| let mounts = MountView::new(vec![MountGrant::new( | ||
| MountAlias::new("/workspace").expect("mount alias"), | ||
| VirtualPath::new("/projects/workspace").expect("virtual path"), | ||
| MountPermissions::read_write(), | ||
| )]) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick win
Mount permissions here do not match the production grant.
Fixture::new grants MountPermissions::read_write(). The production harness profile grants MountPermissions::read_write_list_delete() (tests/integration/support/harness/profiles/omp_coding.rs Line 51). The REM and MV cases at Lines 542-569 delete and move files and pass under read_write(), so no test proves the engines consult delete permission at all.
REM is a destructive operation. Add a case with a read-write-only grant asserting REM is refused, so a missing permission check cannot land silently.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/reborn_omp_coding_engines.rs` around lines 55 - 59, Update the
mount-permission coverage in the relevant engine tests: retain a read-write-only
grant and add a REM case that attempts deletion and asserts the operation is
refused. Keep the existing production-equivalent read_write_list_delete grant
for destructive-operation success coverage, ensuring the tests verify delete
permission is actually enforced.
| assert!( | ||
| text.starts_with("[foo.txt#"), | ||
| "{}", | ||
| &text[..text.len().min(60)] | ||
| ); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Byte-index slicing on engine output.
Line 265 uses &text[..text.len().min(60)] and Line 322 uses &text[text.len().saturating_sub(140)..]. Both slice engine output by byte index inside a panic message. The repo rule forbids byte-index slicing on external text; use chars/char_indices. Content is ASCII in these two fixtures today, so neither panics now, but the pattern breaks the moment a case seeds multi-byte content.
♻️ Proposed fix
- assert!(
- text.starts_with("[foo.txt#"),
- "{}",
- &text[..text.len().min(60)]
- );
+ assert!(
+ text.starts_with("[foo.txt#"),
+ "{}",
+ text.chars().take(60).collect::<String>()
+ );Apply the same treatment at Line 322 with a trailing chars().rev().take(140) collect, or simply print the whole text.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| assert!( | |
| text.starts_with("[foo.txt#"), | |
| "{}", | |
| &text[..text.len().min(60)] | |
| ); | |
| assert!( | |
| text.starts_with("[foo.txt#"), | |
| "{}", | |
| text.chars().take(60).collect::<String>() | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/reborn_omp_coding_engines.rs` around lines 262 - 266, Replace the
byte-index slicing in the assertion message near starts_with and the
trailing-output assertion near the later fixture with character-safe truncation
using chars/char_indices, or print the complete text. Preserve the existing
assertion behavior while ensuring multi-byte engine output cannot panic when
constructing failure messages.
Source: Coding guidelines
![ironloopai[bot]](https://avatars.githubusercontent.com/in/4116490?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔍 IronLoop review
Found seven actionable issues in the OMP tool rollout, including two high-severity reliability risks and one newly added integration test that fails.
Findings: 🔴 High 2 · 🟠 Medium 5
🔴 High · Bound cumulative expanded edit ranges
Inline on crates/extensions/ironclaw_extension_support/src/coding/omp/hashline.rs:2235. See the inline comment for details.
🔴 High · Do not downgrade a failed versioned edit to an unconditional write
Inline on crates/extensions/ironclaw_extension_support/src/coding/omp/hashline.rs:4317. See the inline comment for details.
🟠 Medium · Preserve the source line-ending style
Inline on crates/extensions/ironclaw_extension_support/src/coding/omp/hashline.rs:4289. See the inline comment for details.
🟠 Medium · Emit the registered path in read headers
Inline on crates/extensions/ironclaw_extension_support/src/coding/omp/read.rs:1116. See the inline comment for details.
🟠 Medium · Match grep ranges using resolved paths
Inline on crates/extensions/ironclaw_extension_support/src/coding/omp/grep.rs:648. See the inline comment for details.
🟠 Medium · Tag the same snapshot that produced grep results
Inline on crates/extensions/ironclaw_extension_support/src/coding/omp/grep.rs:771. See the inline comment for details.
🟠 Medium · Make the registration test agree with the OMP-only package
Inline on tests/integration/reborn_omp_registration.rs:147. See the inline comment for details.
Validation
- ✅ Extension support tests — The ironclaw_extension_support test suite completed successfully.
- ✅ OMP coding engine integration — All 25 OMP coding-engine integration tests passed.
- ❌ OMP registration integration — 17 tests passed and `omp_surface_advertises_exact_names_schemas_and_descriptions` failed because it expects legacy tools that the submitted package removes.
Review details
- Run:
8ce69e7f-0416-4bb9-85d8-c3800a8377b3 - Workflow: Review
- Attempts: 1
| fn push_delete_range(&mut self, range: &ParsedRange, line_num: u64) { | ||
| for line in range.start.line..=range.end.line { | ||
| self.push_delete(Anchor { line }, line_num); | ||
| } |
There was a problem hiding this comment.
🔍 IronLoop review · Inline finding
🔴 High · Bound cumulative expanded edit ranges
Each individual range is capped, but this loop materializes one delete edit per line with no request-wide budget. A permitted edit input can contain many valid 100k-line CUT or empty-PUT ranges; overlap detection occurs only after all of them have been expanded, allowing an authorized request to consume excessive CPU and memory. Cap cumulative expansion before allocating edits or represent ranges lazily.
| .put( | ||
| &virtual_path, | ||
| Entry::bytes(persisted.clone().into_bytes()), | ||
| CasExpectation::Any, |
There was a problem hiding this comment.
🔍 IronLoop review · Inline finding
🔴 High · Do not downgrade a failed versioned edit to an unconditional write
The edit preparation captures a file version, but byte-only filesystems reject the versioned put and reach this unconditional fallback. A writer that updates the file after the snapshot/tag check and before this call will be silently overwritten. Preserve atomicity or fail/retry with a real atomic check rather than issuing an `Any` write.
| }); | ||
| } | ||
|
|
||
| let persisted = restore_line_endings(&after, detect_line_ending(&after)); |
There was a problem hiding this comment.
🔍 IronLoop review · Inline finding
🟠 Medium · Preserve the source line-ending style
`after` is derived from content already normalized to LF during preparation, so it cannot identify the original newline convention. Editing a CRLF file consequently rewrites its unchanged lines as LF; CRLF in a payload can also be mishandled. Capture the raw file's line-ending style before normalization and use it for normal and move commits.
| let basename = display.rsplit('/').next().unwrap_or(display); | ||
| output_text = format!("{}\n{output_text}", format_hashline_header(basename, &tag)); |
There was a problem hiding this comment.
🔍 IronLoop review · Inline finding
🟠 Medium · Emit the registered path in read headers
The snapshot is registered under the resolved full virtual path, but the emitted header contains only the basename. For a nested file such as `src/foo.rs`, an edit using the advertised `[foo.rs#TAG]` header resolves a different root-level path and cannot find the registered snapshot. Emit the display/canonical relative path instead; the same issue appears in the multi-range path.
| let ranges = specs | ||
| .iter() | ||
| .find(|spec| spec.clean == display) | ||
| .and_then(|spec| spec.ranges.clone()); |
There was a problem hiding this comment.
🔍 IronLoop review · Inline finding
🟠 Medium · Match grep ranges using resolved paths
`spec.clean` retains the caller's spelling while `display` is workspace-relative. Valid selectors such as `./src/foo.rs:1-5` or `/workspace/src/foo.rs:1-5` pass initial validation but fail this comparison, so grep ignores their range and searches the whole file. Compare canonical/resolved paths before applying ranges.
| ctx: &OmpEngineContext, | ||
| virtual_path: &ironclaw_host_api::path::VirtualPath, | ||
| ) -> Option<String> { | ||
| let text = read_file_text(ctx, virtual_path).await.ok()??; |
There was a problem hiding this comment.
🔍 IronLoop review · Inline finding
🟠 Medium · Tag the same snapshot that produced grep results
Grep renders matches from an initial file read, then this helper reads the file again to create the editable tag. If the file changes between reads, output line numbers from version A are paired with a tag for version B; a subsequent edit can pass validation against B and alter the wrong lines. Carry the initial normalized content or tag in `FileHits` instead of rereading.
| "builtin__list_dir", | ||
| "builtin__apply_patch", | ||
| ] { | ||
| assert!( |
There was a problem hiding this comment.
🔍 IronLoop review · Inline finding
🟠 Medium · Make the registration test agree with the OMP-only package
The package removes these legacy coding capabilities, but this new test asserts that they remain advertised. The added registration target currently fails at this assertion, preventing the PR's own integration test from passing. Align the profile and test with the OMP-only surface, or restore the legacy capabilities consistently.
|
Superseded by the same-repo PR (railway preview path); content identical. |
This branch was successfully deployed
1 participant
What this PR is
Benchmark PR for issue #7392: replaces IronClaw's model-visible coding tools with the pinned omp contract (
read,write,edit,glob,grep) — built as unregistered engines first, then a TEMPORARY benchmark flip so the model surface is omp-only for coding tools.This is a benchmark arm, not the production cutover. Every flip point carries a
// TEMPORARY benchmark override (revert at cutover)marker. The atomic cutover (permission migration read_file to read, old-tool removal, docs) is a follow-up once the benchmark panel runs.Contents (4 slices)
How to benchmark
The bench repo (nearai/benchmarks) needs its upstream-tracking fixes merged first — its Cargo.toml still pins pre-WS6 crate names (ironclaw_reborn_composition / ironclaw_reborn_config), so it cannot build any modern ironclaw. Patch available on request (55 lines: package renames + ironclaw_operator dep + runner import fixes + terminal-bench subdir).
Then, same suite + model on two PRs:
/benchmark claw-swe-bench-lite --model <deepseek slug>on a stock-main PR (baseline, v1 tools)/benchmark claw-swe-bench-lite --model <deepseek slug>on this PR (omp tools)Verification
Rollback
Revert the flip commits (slice 4) to return the stock surface; engines stay unregistered. Full revert of the PR restores current main exactly.