Skip to content

fix: apply native reasoning effort to harnesses - #54

Open
vincentkoc wants to merge 2 commits into
mainfrom
fix/native-reasoning-effort-parity
Open

fix: apply native reasoning effort to harnesses#54
vincentkoc wants to merge 2 commits into
mainfrom
fix/native-reasoning-effort-parity

Conversation

@vincentkoc

@vincentkoc vincentkoc commented Jul 29, 2026

Copy link
Copy Markdown
Member

What does this PR do?

Propagates the native eval plan's reasoning effort into OpenClaw, Hermes,
Codex, and Claude Code client controls.

Why?

Fixes #53. Without this, a run can be labeled high while OpenClaw executes
with thinking disabled, invalidating cross-harness quality and efficiency
comparisons.

Changes

  • carry reasoning_effort in RunSpec and the run manifest
  • map effort to each native harness's supported control
  • translate ShellBench's canonical xhigh to Claude Code's max
  • preserve existing harness defaults when effort is unspecified
  • test low, medium, high, and xhigh across all four harnesses

Tests

  • 114 focused native runner, proxy, and fleet tests pass
  • focused Ruff check passes
  • fresh Codex autoreview reports no actionable findings
  • all GitHub CI and CodeQL checks pass

@clawsweeper clawsweeper Bot added rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. P1 Urgent regression or broken agent/channel workflow affecting real users now. 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: needs changes before merge. Reviewed August 22, 2026, 5:56 AM ET / 09:56 UTC.

ClawSweeper review

What this changes

The PR propagates planned reasoning effort into OpenClaw, Hermes, Codex, and Claude Code harness commands, run specifications, and run manifests.

Merge readiness

⚠️ Ready for maintainer review - 3 items remain

Keep this member-authored PR open: it still contains the previously reported P1 precedence split, which can make the harness command and manifest report a CLI effort different from the already-started proxy's effort.

Priority: P1
Reviewed head: 887c2597c1e6ef7ad369cd9a72c800b8fb52ffeb

Review scores

Measure Result What it means
Overall readiness 🦐 gold shrimp (3/6) The patch has focused coverage and a bounded repair path, but its remaining precedence defect blocks trustworthy benchmark output.
Proof confidence 🌊 off-meta tidepool Not applicable: The PR author is a repository member, so the external-contributor real-behavior-proof gate does not apply.
Patch quality 🦐 gold shrimp (3/6) 1 actionable review finding remain.

Verification

Check Result Evidence
Real behavior Not applicable Not applicable: The PR author is a repository member, so the external-contributor real-behavior-proof gate does not apply.
Evidence reviewed 5 items Precedence split introduced by this PR: The new resolver gives --reasoning-effort precedence over SHELLBENCH_REASONING_EFFORT, while the proxy is configured from the environment separately.
Proxy starts before runner argument parsing: The remote launcher writes the LiteLLM configuration and starts the proxy before it invokes run_job; write_proxy_config reads only SHELLBENCH_REASONING_EFFORT.
Fleet already has a canonical planned value: Fleet dispatch exports the plan entry's reasoning effort as SHELLBENCH_REASONING_EFFORT, so the repair should preserve that single value through proxy setup and harness construction.
Findings 1 actionable finding [P1] Unify effort before starting the proxy
Security None None.

Live Verification

Command: python -m scripts.native_eval.run_job --help

Result: FAIL (failed) — execution before step 1 run: sh -lc pnpm install --ignore-scripts --frozen-lockfile failed: ! Corepack is about to download https://registry.npmjs.org/pnpm/-/pnpm-11.22.0.tgz

sh -lc pnpm install --ignore-scripts --frozen-lockfile failed: ! Corepack is about to download https://registry.npmjs.org/pnpm/-/pnpm-11.22.0.tgz

Assertions:

  • FAIL expect_output: --reasoning-effort

How this fits together

ShellBench turns a native evaluation plan into a fleet-dispatched run, starts a LiteLLM provider proxy, then launches the selected agent harness. The proxy, harness command, and run manifest must use one reasoning-effort value for benchmark results to be comparable.

flowchart LR
  A[Evaluation plan] --> B[Fleet dispatch]
  B --> C[Provider environment]
  C --> D[LiteLLM proxy]
  B --> E[Run specification]
  E --> F[Native harness command]
  E --> G[Run manifest]
  D --> H[Comparable benchmark result]
  F --> H
  G --> H
Loading

Before merge

  • Unify effort before starting the proxy (P1) - The new CLI value wins in build_run_spec, but remote_run.sh has already generated and started LiteLLM from SHELLBENCH_REASONING_EFFORT. Thus low in the environment plus --reasoning-effort high records and sends high to the harness while provider requests remain configured for low. This is the prior blocker and the relevant code is unchanged from the previous reviewed head; resolve or reject the conflict before proxy setup and add an end-to-end regression.
  • Resolve merge risk (P1) - Merging permits a conflicting environment and CLI effort to produce provider requests at one effort while the native harness command and manifest claim another, invalidating affected benchmark comparisons.
  • Complete next step (P2) - A bounded mechanical repair can establish one effort value before proxy setup and add a regression for conflicting CLI and environment inputs; this member-authored PR remains open for maintainer review.

Findings

  • [P1] Unify effort before starting the proxy — scripts/native_eval/run_job.py:361-363
Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Changed surface 4 files; production +29, tests +92 The patch spans native harness adapters, run construction, manifest metadata, and focused command-generation tests.

Root-cause cluster

Relationship: fixed_by_candidate
Canonical: #53
Summary: This PR is the candidate implementation for the native-harness reasoning-effort mismatch tracked by the linked issue.

Members:

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

Merge-risk options

Maintainer options:

  1. Unify effort before proxy startup (recommended)
    Resolve the CLI and environment inputs before LiteLLM configuration, or reject conflicts, then prove proxy, command, and manifest agree.
Copy recommended automerge instruction
@clawsweeper automerge

Special instructions:
Resolve or reject conflicting SHELLBENCH_REASONING_EFFORT and --reasoning-effort values before proxy setup, and add regression coverage that proves proxy, harness command, and manifest agree.

Technical review

Best possible solution:

Keep the native harness mappings, but establish one canonical reasoning-effort value before LiteLLM configuration and use that same value for proxy, harness, and manifest output.

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

Yes. With SHELLBENCH_REASONING_EFFORT=low and --reasoning-effort high, source shows the proxy is configured first from low while the subsequent run specification, harness command, and manifest use high.

Is this the best way to solve the issue?

No. The harness mappings are appropriate, but the added precedence path must be unified with proxy startup or fail on a conflict before this is a safe benchmark fix.

Full review comments:

  • [P1] Unify effort before starting the proxy — scripts/native_eval/run_job.py:361-363
    The new CLI value wins in build_run_spec, but remote_run.sh has already generated and started LiteLLM from SHELLBENCH_REASONING_EFFORT. Thus low in the environment plus --reasoning-effort high records and sends high to the harness while provider requests remain configured for low. This is the prior blocker and the relevant code is unchanged from the previous reviewed head; resolve or reject the conflict before proxy setup and add an end-to-end regression.
    Confidence: 0.98

Overall correctness: patch is incorrect
Overall confidence: 0.98

AGENTS.md: not found in the target repository.

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

Labels

Label changes:

  • add rating: 🦐 gold shrimp: Overall readiness is 🦐 gold shrimp; proof is 🌊 off-meta tidepool and patch quality is 🦐 gold shrimp.
  • remove rating: 🦪 silver shellfish: Current PR rating is rating: 🦐 gold shrimp, so this older rating label is no longer current.

Label justifications:

  • P1: A reasoning-effort disagreement defeats the comparability of native-harness benchmark results.
  • merge-risk: 🚨 compatibility: The newly accepted CLI override can diverge from the existing environment-based proxy configuration.
  • 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 PR author is a repository member, so the external-contributor real-behavior-proof gate does not apply.

Evidence

Acceptance criteria:

  • [P1] python -m pytest tests/test_native_eval_runner.py tests/test_native_eval_proxy.py tests/test_native_eval_fleet.py -q.

What I checked:

  • Precedence split introduced by this PR: The new resolver gives --reasoning-effort precedence over SHELLBENCH_REASONING_EFFORT, while the proxy is configured from the environment separately. (scripts/native_eval/run_job.py:361, 887c2597c1e6)
  • Proxy starts before runner argument parsing: The remote launcher writes the LiteLLM configuration and starts the proxy before it invokes run_job; write_proxy_config reads only SHELLBENCH_REASONING_EFFORT. (scripts/native_eval/remote_run.sh:79, 884dd1bb5511)
  • Fleet already has a canonical planned value: Fleet dispatch exports the plan entry's reasoning effort as SHELLBENCH_REASONING_EFFORT, so the repair should preserve that single value through proxy setup and harness construction. (scripts/native_eval/fleet.py:976, 884dd1bb5511)
  • Prior blocker remains unchanged: The relevant runner file has no changes since the prior reviewed head, so the previous P1 finding still applies rather than being a new late finding. (scripts/native_eval/run_job.py:361, 887c2597c1e6)
  • Feature-history ownership: Current-main native evaluation history attributes the original matrix runner, subsequent stabilization, and the latest OpenClaw trace maintenance to the same contributor. (scripts/native_eval/harnesses.py:617, 69f75c6629c4)

Likely related people:

  • Vincent Koc: Current-main history credits the native matrix runner, stability work, and latest OpenClaw trace work to this contributor; the same paths are central to this PR. (role: introduced and recently maintained native evaluation; confidence: high; commits: 69f75c6629c4, b9acd9f7a010, 884dd1bb5511; files: scripts/native_eval/harnesses.py, scripts/native_eval/run_job.py, scripts/native_eval/remote_run.sh)

Rank-up moves

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

  • Resolve or reject conflicting environment and CLI effort before proxy setup, then add a regression that observes matching proxy, harness, and manifest values.

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 (72 earlier review cycles; latest 8 shown)
  • reviewed 2026-08-09T07:15:43.345Z sha 887c259 :: needs changes before merge. :: [P1] Unify proxy and harness effort precedence
  • reviewed 2026-08-09T10:57:33.895Z sha 887c259 :: needs changes before merge. :: [P1] Remove the proxy-bypassing CLI override
  • reviewed 2026-08-09T12:41:37.715Z sha 887c259 :: needs changes before merge. :: [P1] Resolve effort before proxy startup
  • reviewed 2026-08-09T15:19:15.497Z sha 887c259 :: needs changes before merge. :: [P1] Resolve effort before starting the proxy
  • reviewed 2026-08-09T19:46:21.916Z sha 887c259 :: needs changes before merge. :: [P1] Resolve effort before starting the proxy
  • reviewed 2026-08-09T21:12:39.234Z sha 887c259 :: needs changes before merge. :: [P1] Resolve effort before starting the proxy
  • reviewed 2026-08-12T06:18:17.786Z sha 887c259 :: needs changes before merge. :: [P1] Resolve effort before starting the proxy
  • reviewed 2026-08-15T04:02:30.065Z sha 887c259 :: needs changes before merge. :: [P1] Resolve effort before starting the proxy

@clawsweeper clawsweeper Bot added rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. and removed rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. labels Jul 29, 2026
@vincentkoc
vincentkoc marked this pull request as ready for review July 29, 2026 16:23
@vincentkoc
vincentkoc requested a review from a team as a code owner July 29, 2026 16:23
@clawsweeper clawsweeper Bot added rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. 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. and removed rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. 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: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. labels Jul 29, 2026
@clawsweeper clawsweeper Bot added 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: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. merge-risk: 🚨 availability 🚨 Merging this PR could cause crashes, hangs, restart loops, stalls, or process outages. rating: 🦞 diamond lobster Very strong PR readiness with only minor maintainer review expected. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. and removed rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. rating: 🦞 diamond lobster Very strong PR readiness with only minor maintainer review expected. merge-risk: 🚨 availability 🚨 Merging this PR could cause crashes, hangs, restart loops, stalls, or process outages. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. labels Aug 2, 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. P1 Urgent regression or broken agent/channel workflow affecting real users now. 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.

Native harnesses do not apply planned reasoning effort consistently

1 participant