Skip to content

fix(native-eval): benchmark genuine OpenClaw code mode - #62

Open
vincentkoc wants to merge 5 commits into
mainfrom
fix/openclaw-tool-search-explicit-off
Open

fix(native-eval): benchmark genuine OpenClaw code mode#62
vincentkoc wants to merge 5 commits into
mainfrom
fix/openclaw-tool-search-explicit-off

Conversation

@vincentkoc

@vincentkoc vincentkoc commented Jul 29, 2026

Copy link
Copy Markdown
Member

What does this PR do?

Makes the OpenClaw native-eval arms represent genuine direct, directory, and
Code Mode execution, then exports and validates their public trajectory bundles
before a run can be scored.

Why?

Fixes #61.

The earlier code arm selected the legacy tool_search_code bridge rather than
OpenClaw Code Mode. The harness also masked setup failures, forced thinking off,
terminated the runtime before lifecycle cleanup settled, and reconstructed
delegated traces from private session files. Those failures made the released
direct/code comparison invalid and disproportionately erased Code Mode's nested
tool calls.

Changes

  • Map direct, directory, and code to explicit, mutually exclusive
    OpenClaw tool surfaces.
  • Propagate the requested reasoning effort through planning, dispatch,
    OpenClaw defaults, subagents, CLI execution, and manifests.
  • Fail setup immediately and let openclaw agent exit naturally.
  • Export root and child trajectories through
    openclaw sessions export-trajectory.
  • Capture child runs at the canonical pre-cleanup terminal hook, including
    nested and repeated runs, without blocking the Gateway.
  • Validate audit initialization, trace/session identity, event counts, terminal
    status, complete Code Mode snapshots, and the exact provider-visible
    exec/wait surface.
  • Reconstruct Code Mode nested calls and complete session-tree usage from the
    public export bundles.
  • Bound child-export stabilization and reject missing or failed evidence rather
    than publishing partial traces.

Tests

  • Blacksmith Testbox full suite: 454 passed, 5 skipped
  • Blacksmith Testbox focused runner suite after final fixes: 61 passed
  • Ruff clean
  • Python compile, git diff --check, generated shell syntax for all three
    modes, and generated audit-plugin node --check
  • Fresh Codex autoreview: clean
  • Fresh matched direct/Code Mode ShellBench batch

The July 29, 2026 released code result remains legacy Tool Search bridge
evidence only. It is not evidence about genuine OpenClaw Code Mode.

@clawsweeper clawsweeper Bot added rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. P2 Normal priority bug or improvement with limited blast radius. merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. labels Jul 29, 2026
@clawsweeper

clawsweeper Bot commented Jul 29, 2026

Copy link
Copy Markdown

Codex review: blocked before merge. Reviewed September 5, 2026, 9:02 AM ET / 13:02 UTC.

ClawSweeper review

What this changes

The PR configures explicit OpenClaw tool modes, propagates reasoning settings, and replaces private-session collection with validated public trajectory exports.

Regression provenance

Possible regression — suspected (reviewed change). No predecessor PR is attributed.

Merge readiness

Blocked before merge - 6 items remain

Keep open: the central work remains necessary, but two introduced reconstruction defects compromise delegated traces. The earlier matrix-expansion finding is withdrawn because the linked issue explicitly requires direct tools when no mode is selected.

Priority: P2
Reviewed head: f5b31cb1260b631b811bf0ede9f4eba51a94b98c

Review scores

Measure Result What it means
Overall readiness 🦐 gold shrimp (3/6) The implementation is focused and extensively tested, but delegated public-bundle reconstruction contains two blocking defects.
Proof confidence 🌊 off-meta tidepool Not applicable: The MEMBER-authored PR is exempt from the ordinary contributor-proof gate. Its captured body reports Testbox and static checks but leaves the matched direct/Code Mode batch unchecked; no after-fix runtime observation of the new exporter and child hooks is supplied.
Patch quality 🦐 gold shrimp (3/6) 2 actionable review findings remain.

Verification

Check Result Evidence
Real behavior Not applicable Not applicable: The MEMBER-authored PR is exempt from the ordinary contributor-proof gate. Its captured body reports Testbox and static checks but leaves the matched direct/Code Mode batch unchecked; no after-fix runtime observation of the new exporter and child hooks is supplied.
Evidence reviewed 9 items Current main still needs the central change: Comparing the pinned base with fetched main shows reasoning propagation changes, but no explicit tool-mode configuration or public-export reconstruction. Current CHANGELOG.md describes the earlier private-session implementation; no fixing release is established.
Prior planner finding reassessed: The supplied body of #61 explicitly requires omitted mode to disable Tool Search. The PR tests direct-by-default behavior and dispatch of an explicitly selected code mode. Automatically tripling the default matrix is not an established requirement.
Snapshot identities collide across sessions: Every exported snapshot receives positional runtime-message IDs. The existing fork-prefix remover compares IDs across parent and child records without comparing message contents, so unrelated child messages can be discarded.
Findings 2 actionable findings [P1] Preserve message identity when deduplicating forked snapshots
[P2] Reconcile repeated exports of the same child session
Security None None.

How this fits together

ShellBench’s native evaluation runner launches agent harnesses against benchmark tasks and collects their execution traces. The OpenClaw adapter converts root and delegated-session exports into trajectories used for research metrics and score eligibility.

flowchart TD
  A[Benchmark plan] --> B[OpenClaw configuration]
  B --> C[Root and delegated runs]
  C --> D[Public trajectory exports]
  D --> E[Validate and reconstruct traces]
  E --> F[Research metrics]
  E --> G[Score eligibility]
Loading

Before merge

  • Preserve message identity when deduplicating forked snapshots (P1) - Each snapshot restarts IDs at runtime-message-1, but _openclaw_child_session_records removes a forked child's prefix by comparing those IDs against the parent's records. With a preserved parentSession header, distinct child messages at overlapping positions are therefore discarded; a shorter child can lose its entire trace, while a longer child loses tools and usage from its prefix. Compare actual inherited messages or preserve stable message identity. This interaction was missed in the earlier review of the same head.
  • Reconcile repeated exports of the same child session (P2) - The new audit plugin exports each (runId, sessionKey) into a separate directory. When a child session runs again, this reader marks its second manifest ambiguous solely because the session key repeats; _openclaw_session_tree then emits ambiguous-session-entry and makes the trajectory unavailable even when both exports succeeded. Reconcile valid per-run bundles without losing or double-counting their messages and usage. This was missed in the earlier review of the same head.
  • Resolve merge risk (P1) - Collection now requires public export commands, lifecycle hooks, and snapshot fields whose compatibility with the pinned OpenClaw build remains unverified.
  • Resolve merge risk (P1) - The persisted archive layout changes from private session files to public bundles; fresh-run and retained-archive compatibility need validation before merge.
  • Resolve merge risk (P1) - The supplied GitHub context reports merge conflicts. No pinned test merge is available to assess the resolved integration with current-main reasoning changes.
  • Complete next step (P2) - Repair forked-snapshot identity and repeated-child export handling, resolve merge conflicts, and verify fresh-export and retained-archive compatibility against the pinned OpenClaw build.

Findings

  • [P1] Preserve message identity when deduplicating forked snapshots — scripts/native_eval/harness_trajectories.py:857-862
  • [P2] Reconcile repeated exports of the same child session — scripts/native_eval/harness_trajectories.py:1473-1476
Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Production and test LOC Production +699/-477 (net +222); tests +1116/-475 (net +641) The stated justification for production growth is public-export validation and lifecycle handling; delegated bundle integration remains the critical coverage gap.

Root-cause cluster

Relationship: fixed_by_candidate
Canonical: #61
Summary: This PR explicitly proposes to fix the open, same-author tool-mode control issue; neither item should close before the implementation lands.

Members:

Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything.

Merge-risk options

Maintainer options:

  1. Repair delegated reconstruction and validate archive compatibility (recommended)
    Preserve forked messages and repeated child runs, then verify the public-export path and retained archives against the pinned runtime.
Copy recommended automerge instruction
@clawsweeper automerge

Special instructions:
Preserve direct-by-default behavior. Repair snapshot identity handling so fork-prefix deduplication compares actual inherited messages, and reconcile repeated child exports using run/session identity without dropping or double-counting events or usage. Add focused public-bundle regressions, preserve current-main reasoning behavior during conflict resolution, and verify fresh-export and retained-archive compatibility without silently changing the OpenClaw pin.

Technical review

Best possible solution:

Preserve direct-by-default behavior while reconstructing complete, deduplicated child traces and usage, with fresh and retained archives validated against the pinned OpenClaw build.

Do we have a high-confidence way to reproduce the issue?

Yes, from source: a forked child snapshot shares positional IDs with its parent, and two exports for one child session trigger the ambiguity guard. These paths were traced without executing a reproduction.

Is this the best way to solve the issue?

No as written: public exports are an appropriate boundary, but reconstruction must preserve actual message identity and distinguish repeated runs before it can reliably replace private-session collection.

Full review comments:

  • [P1] Preserve message identity when deduplicating forked snapshots — scripts/native_eval/harness_trajectories.py:857-862
    Each snapshot restarts IDs at runtime-message-1, but _openclaw_child_session_records removes a forked child's prefix by comparing those IDs against the parent's records. With a preserved parentSession header, distinct child messages at overlapping positions are therefore discarded; a shorter child can lose its entire trace, while a longer child loses tools and usage from its prefix. Compare actual inherited messages or preserve stable message identity. This interaction was missed in the earlier review of the same head.
    Confidence: 0.97
    Late finding: first raised on code an earlier review cycle already covered.
  • [P2] Reconcile repeated exports of the same child session — scripts/native_eval/harness_trajectories.py:1473-1476
    The new audit plugin exports each (runId, sessionKey) into a separate directory. When a child session runs again, this reader marks its second manifest ambiguous solely because the session key repeats; _openclaw_session_tree then emits ambiguous-session-entry and makes the trajectory unavailable even when both exports succeeded. Reconcile valid per-run bundles without losing or double-counting their messages and usage. This was missed in the earlier review of the same head.
    Confidence: 0.97
    Late finding: first raised on code an earlier review cycle already covered.

Overall correctness: patch is incorrect
Overall confidence: 0.94

AGENTS.md: not found in the target repository.

Codex review notes: model internal, reasoning high; reviewed against c1a79f731541.

Labels

Label justifications:

  • P2: This is a bounded benchmark-correctness repair without evidence of an urgent production outage.
  • merge-risk: 🚨 compatibility: The adapter changes its required runtime export contract and persisted archive layout while retaining the existing OpenClaw version pin.
  • merge-risk: 🚨 other: The introduced reconstruction defects can omit delegated work or invalidate otherwise complete benchmark runs.
  • rating: 🦐 gold shrimp: Overall readiness is 🦐 gold shrimp; proof is 🌊 off-meta tidepool and patch quality is 🦐 gold shrimp.
  • status: ⏳ waiting on author: ClawSweeper has contributor-facing work open and is waiting for author action. Not applicable: The MEMBER-authored PR is exempt from the ordinary contributor-proof gate. Its captured body reports Testbox and static checks but leaves the matched direct/Code Mode batch unchecked; no after-fix runtime observation of the new exporter and child hooks is supplied.

Evidence

Acceptance criteria:

  • [P1] python -m pytest -q tests/test_native_eval_runner.py tests/test_native_eval_fleet.py.
  • [P1] python -m pytest -q.
  • [P1] python -m ruff check scripts/native_eval/harness_trajectories.py scripts/native_eval/harnesses.py tests/test_native_eval_runner.py tests/test_native_eval_fleet.py.
  • [P1] git diff --check.

What I checked:

  • Current main still needs the central change: Comparing the pinned base with fetched main shows reasoning propagation changes, but no explicit tool-mode configuration or public-export reconstruction. Current CHANGELOG.md describes the earlier private-session implementation; no fixing release is established. (scripts/native_eval/harnesses.py, c1a79f731541)
  • Prior planner finding reassessed: The supplied body of fix(native-eval): make OpenClaw Tool Search controls explicit #61 explicitly requires omitted mode to disable Tool Search. The PR tests direct-by-default behavior and dispatch of an explicitly selected code mode. Automatically tripling the default matrix is not an established requirement. (tests/test_native_eval_fleet.py:606, f5b31cb1260b)
  • Snapshot identities collide across sessions: Every exported snapshot receives positional runtime-message IDs. The existing fork-prefix remover compares IDs across parent and child records without comparing message contents, so unrelated child messages can be discarded. (scripts/native_eval/harness_trajectories.py:860, f5b31cb1260b)
  • Repeated child exports become ambiguous: The plugin produces separate directories for each runId/sessionKey pair, but the new manifest reader marks a second bundle with the same sessionKey ambiguous. Session-tree validation then rejects that child and reports incomplete evidence. (scripts/native_eval/harness_trajectories.py:1474, f5b31cb1260b)
  • Established delegated-trace contract: Existing tests require forked child tools and usage to survive reconstruction and reused child tasks to appear exactly once. The new public-bundle test exercises one root session, leaving those combined export scenarios uncovered. (tests/test_native_eval_runner.py:2815, f5b31cb1260b)
  • Late-finding continuity: The previous completed review used this exact head. The relevant files have no differences from that revision, so both newly identified defects were previously visible and are marked late findings. (scripts/native_eval/harness_trajectories.py, f5b31cb1260b)

Likely related people:

  • vincentkoc: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)
  • Peter Steinberger: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)

Rank-up moves

Optional improvements that raise the rating; they are not merge blockers.

  • Add public-bundle regressions covering forked snapshots and repeated child runs, preserving every distinct tool call and its usage exactly once.
  • Validate fresh exports and retained-archive compatibility against the pinned OpenClaw build.
  • Resolve merge conflicts while preserving current-main reasoning propagation.

Rating scale

Score Internal tier Crab rank Meaning
6/6 S 🦀 challenger crab Exceptional readiness
5/6 A 🦞 diamond lobster Very strong readiness
4/6 B 🐚 platinum hermit Good normal PR; ordinary maintainer review
3/6 C 🦐 gold shrimp Useful, but confidence is limited
2/6 D 🦪 silver shellfish Proof or implementation needs work
1/6 F 🧂 unranked krab Not merge-ready
N/A NA 🌊 off-meta tidepool Rating does not apply

Overall follows the weaker of proof and patch quality.
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

Workflow

  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

History

Review history (74 earlier review cycles; latest 8 shown)
  • reviewed 2026-08-09T23:12:37.716Z sha f5b31cb :: needs changes before merge. :: [P1] Generate planned runs for every OpenClaw tool mode
  • reviewed 2026-08-11T23:11:15.062Z sha f5b31cb :: needs changes before merge. :: [P1] Expand normal plans across OpenClaw tool modes
  • reviewed 2026-08-12T01:18:05.700Z sha f5b31cb :: needs changes before merge. :: [P1] Generate a plan entry for every OpenClaw tool mode
  • reviewed 2026-08-12T06:19:16.517Z sha f5b31cb :: needs changes before merge. :: [P1] Expand normal OpenClaw plans across tool modes
  • reviewed 2026-08-15T11:06:50.762Z sha f5b31cb :: needs changes before merge. :: [P1] Generate planned runs for every OpenClaw tool mode
  • reviewed 2026-08-15T18:44:55.752Z sha f5b31cb :: needs changes before merge. :: [P1] Generate a planned run for each OpenClaw tool mode
  • reviewed 2026-08-22T23:08:20.889Z sha f5b31cb :: needs changes before merge. :: [P1] Generate planned runs for every OpenClaw tool mode
  • reviewed 2026-08-29T16:59:47.922Z sha f5b31cb :: needs changes before merge. :: [P1] Generate entries for every OpenClaw tool mode

@vincentkoc
vincentkoc marked this pull request as ready for review July 29, 2026 20:56
@vincentkoc
vincentkoc requested a review from a team as a code owner July 29, 2026 20:56
@vincentkoc vincentkoc changed the title fix(native-eval): make OpenClaw Tool Search controls explicit fix(native-eval): benchmark genuine OpenClaw code mode Jul 30, 2026
@clawsweeper clawsweeper Bot added rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. and removed rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. labels Jul 30, 2026
@clawsweeper clawsweeper Bot added rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. and removed rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. labels Jul 30, 2026
@clawsweeper clawsweeper Bot added rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. and removed rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. labels Jul 30, 2026
@clawsweeper clawsweeper Bot added rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. merge-risk: 🚨 other 🚨 Merging this PR has meaningful risk outside the owned taxonomy. and removed status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. labels Aug 3, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. merge-risk: 🚨 other 🚨 Merging this PR has meaningful risk outside the owned taxonomy. P2 Normal priority bug or improvement with limited blast radius. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(native-eval): make OpenClaw Tool Search controls explicit

1 participant