Skip to content

fix(native-eval): reject invalid harness runs - #64

Draft
vincentkoc wants to merge 1 commit into
mainfrom
fix/native-eval-run-acceptance
Draft

fix(native-eval): reject invalid harness runs#64
vincentkoc wants to merge 1 commit into
mainfrom
fix/native-eval-run-acceptance

Conversation

@vincentkoc

Copy link
Copy Markdown
Member

What does this PR do?

Records structured native-eval execution outcomes and rejects runs where every
trial failed at the harness, infrastructure, or verifier layer.

Why?

Verifier rewards are useful diagnostics, but they are not proof that the
benchmark harness executed correctly. Fully invalid runs were being presented
as completed score data, and fleet recovery could infer success without an
archived exit status.

Fixes #63

Changes

  • classify clean, agent, harness, infrastructure, and verifier outcomes
  • preserve per-trial rewards while recording run-level execution acceptance
  • exclude rejected runs from score-eligible aggregates
  • infer historical OpenClaw exit 70/71 outcomes correctly
  • remove fleet success inference based only on result count and checkpoint text

Tests

  • python -m pytest -q passes on Blacksmith Testbox
  • python -m ruff check clawbench app.py scripts tests passes on Blacksmith Testbox

Focused proof: 117 tests passed; Ruff clean. Autoreview reported no actionable
findings.

@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. 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 30, 2026
@clawsweeper

clawsweeper Bot commented Jul 30, 2026

Copy link
Copy Markdown

Codex review: found issues before merge. Reviewed July 30, 2026, 4:38 AM ET / 08:38 UTC.

ClawSweeper review

What this changes

The PR records native-eval execution outcomes, excludes fully invalid harness runs from score-eligible aggregates, and requires an archived exit status when fleet recovery completes an exported run.

Merge readiness

Blocked by patch quality or review findings - 6 items remain

Keep this PR open. It addresses the paired native-eval bug, but its removal of legacy fleet recovery makes previously exported full-coverage runs fail whenever their archived exit status was not retained; the current head is unchanged from the prior review cycle, so that compatibility blocker remains.

Priority: P1
Reviewed head: 9f4490e8e4370cbc752404a1becce42d5a6a69f8
Owner decision: Required. See Decision needed.

Review scores

Measure Result What it means
Overall readiness 🦪 silver shellfish (2/6) The invalid-run classification work is focused, but the unresolved P1 upgrade regression in fleet recovery prevents a merge-ready rating.
Proof confidence 🌊 off-meta tidepool Not applicable: This is a MEMBER-authored PR, so the external-contributor real-behavior-proof gate does not apply; the PR body reports test and lint results, which remain supplemental validation rather than live recovery proof.
Patch quality 🦪 silver shellfish (2/6) 1 actionable review finding remain.

Verification

Check Result Evidence
Real behavior Not applicable Not applicable: This is a MEMBER-authored PR, so the external-contributor real-behavior-proof gate does not apply; the PR body reports test and lint results, which remain supplemental validation rather than live recovery proof.
Evidence reviewed 4 items Current PR diff removes compatibility fallback: The PR deletes recovery that marked a verified, full-coverage exported run successful when a final checkpoint existed but no archived exit status was available; the revised test now expects that same legacy state to fail.
Existing regression test demonstrates upgrade-visible change: The changed fleet test switches a legacy full-coverage export from completed with inferred exit code 0 to failed with an unknown-exit error, without a migration or recovery path for existing archives.
Prior review blocker remains at current head: The supplied prior ClawSweeper review identified the same legacy-recovery compatibility concern at this exact head SHA; no later commit is present in the provided PR context.
Findings 1 actionable finding [P1] Preserve or migrate legacy recovery before removing the fallback
Security None None.

How this fits together

Native eval runs agent trials, stores result artifacts and manifests, and aggregates accepted runs into benchmark scores. Fleet recovery later validates exported artifacts and determines whether a remote campaign can be marked completed or must be retried.

flowchart LR
  A[Native eval trial] --> B[Execution outcome record]
  B --> C[Run acceptance decision]
  C --> D[Score aggregation]
  E[Exported run artifacts] --> F[Fleet recovery]
  G[Archived exit status] --> F
  F --> H[Completed or retry state]
  C --> D
Loading

Decision needed

Question Recommendation
Should historical full-coverage exports without archived exit status retain a supported recovery path, or should this release intentionally require operators to migrate or rerun them? Add a legacy migration path: Keep strict exit-status enforcement for new runs while classifying old archives without exit status as explicitly legacy/unknown and providing a supported retry or migration path.

Why: The defect fix correctly avoids treating artifact count as proof of successful execution, but choosing the upgrade behavior for already-exported campaigns is a maintainer compatibility policy decision.

Before merge

  • Preserve or migrate legacy recovery before removing the fallback (P1) - The deleted branch changes already-exported runs with verified full coverage and a final checkpoint—but no archived exit status—from completed to failed. This is the prior review blocker at the unchanged current head; retain a narrow legacy migration/retry path or document and prove the intentional upgrade behavior before merge.
  • Resolve merge risk (P1) - Existing exported campaigns with verified result coverage and a final checkpoint, but no archived exit status, now transition to failed instead of completed; operators may need an undocumented retry or migration path after upgrade.
  • Complete next step (P2) - A maintainer must choose the compatibility contract for legacy exported runs before a mechanical repair can be safely scoped.
  • Improve patch quality - Add a version-aware migration or retry path for pre-existing archives without an archived exit status.
  • Improve patch quality - Demonstrate fresh-run and historical-export recovery behavior with a redacted terminal transcript or equivalent live output.

Findings

  • [P1] Preserve or migrate legacy recovery before removing the fallback — scripts/native_eval/fleet.py:1272-1277
Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Patch surface 7 files; 279 added, 35 removed The change spans trial recording, aggregation, fleet recovery, and regression coverage rather than a single scoring check.
Recovery fallback 1 legacy success inference removed This behavior change directly affects upgrade handling for exported campaigns lacking archived exit status.

Root-cause cluster

Relationship: fixed_by_candidate
Canonical: #63
Summary: This PR is the candidate implementation for the canonical report about fully invalid native-eval harness runs.

Members:

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

Merge-risk options

Maintainer options:

  1. Preserve recoverability for legacy exports (recommended)
    Add a version-aware legacy state or migration that avoids reclassifying pre-existing full-coverage exports as hard failures solely because historic exit status was never archived.
  2. Make the breaking recovery policy explicit
    If legacy exports must fail, document the required operator retry path and provide proof that both fresh and upgraded campaign states behave as intended.
  3. Pause the recovery change
    Leave the fallback removal out of this PR until maintainers choose and validate the historical-archive contract.

Technical review

Best possible solution:

Keep strict archived-exit requirements for newly created runs, while adding a versioned legacy recovery or migration path that preserves historical exports as explicitly unknown/retriable rather than silently converting them into failures.

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

Yes in source, though it was not executed in this read-only review: create an exported OpenClaw run with full result coverage and a final checkpoint but no archived exit status, then invoke fleet recovery. The changed regression test describes that path and demonstrates the before/after state transition.

Is this the best way to solve the issue?

No, not yet. Rejecting inferred success is the right bug fix, but replacing it with an unconditional legacy failure needs a versioned migration, retry path, or explicit maintainer-approved upgrade policy.

Full review comments:

  • [P1] Preserve or migrate legacy recovery before removing the fallback — scripts/native_eval/fleet.py:1272-1277
    The deleted branch changes already-exported runs with verified full coverage and a final checkpoint—but no archived exit status—from completed to failed. This is the prior review blocker at the unchanged current head; retain a narrow legacy migration/retry path or document and prove the intentional upgrade behavior before merge.
    Confidence: 0.96

Overall correctness: patch is incorrect
Overall confidence: 0.91

AGENTS.md: unclear because the file could not be read completely.

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

Labels

Label justifications:

  • P1: The patch can make existing exported native-eval campaigns fail during recovery, affecting an active benchmark workflow after upgrade.
  • merge-risk: 🚨 compatibility: Removing the historical recovery fallback changes the outcome for valid-looking legacy archives that do not contain an archived exit code.
  • rating: 🦪 silver shellfish: Overall readiness is 🦪 silver shellfish; proof is 🌊 off-meta tidepool and patch quality is 🦪 silver shellfish.
  • status: ⏳ waiting on author: ClawSweeper has contributor-facing work open and is waiting for author action. Not applicable: This is a MEMBER-authored PR, so the external-contributor real-behavior-proof gate does not apply; the PR body reports test and lint results, which remain supplemental validation rather than live recovery proof.

Evidence

What I checked:

  • Current PR diff removes compatibility fallback: The PR deletes recovery that marked a verified, full-coverage exported run successful when a final checkpoint existed but no archived exit status was available; the revised test now expects that same legacy state to fail. (scripts/native_eval/fleet.py:1272, 9f4490e8e437)
  • Existing regression test demonstrates upgrade-visible change: The changed fleet test switches a legacy full-coverage export from completed with inferred exit code 0 to failed with an unknown-exit error, without a migration or recovery path for existing archives. (tests/test_native_eval_fleet.py:1273, 9f4490e8e437)
  • Prior review blocker remains at current head: The supplied prior ClawSweeper review identified the same legacy-recovery compatibility concern at this exact head SHA; no later commit is present in the provided PR context. (scripts/native_eval/fleet.py:1272, 9f4490e8e437)
  • Paired issue establishes the intended bug boundary: The linked issue requests rejection of invalid runs and explicit archived exit status, so the new acceptance model is aligned with the reported defect; the unresolved question is upgrade handling for historical exports. (scripts/native_eval/aggregate.py:283, 9f4490e8e437)

Likely related people:

  • vincentkoc: Authored the paired native-eval report and the current implementation that changes execution acceptance and fleet recovery behavior; repository history could not be inspected in this read-only runner. (role: recent native-eval contributor; confidence: low; commits: 9f4490e8e437; files: scripts/native_eval/fleet.py, scripts/native_eval/aggregate.py, scripts/native_eval/runtime.py)

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 (1 earlier review cycle)
  • reviewed 2026-07-30T03:42:48.905Z sha 9f4490e :: found issues before merge. :: [P1] Preserve or migrate legacy recovery before removing the fallback

@clawsweeper clawsweeper Bot added status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. P1 Urgent regression or broken agent/channel workflow affecting real users now. and removed status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. P2 Normal priority bug or improvement with limited blast radius. labels Jul 30, 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: 🦪 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.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Native eval accepts fully invalid harness runs

1 participant