Skip to content

fix(native-eval): separate execution validity from diagnostic rewards - #73

Closed
steipete wants to merge 1 commit into
mainfrom
fix/pr64-acceptance-reviewed
Closed

fix(native-eval): separate execution validity from diagnostic rewards#73
steipete wants to merge 1 commit into
mainfrom
fix/pr64-acceptance-reviewed

Conversation

@steipete

Copy link
Copy Markdown
Contributor

Reviewed integration of #64: aggregation no longer retains fully invalid agent-exit runs, recovery no longer infers exit zero without an archived exit status — plus two additional defects found in review (missing exit status in final archives; later execution failures hidden behind earlier agent errors). 134 focused + 471 full-suite tests pass; real shell exports preserved exits 0 and 2; the aggregation CLI retained diagnostic score 0.5 while rejecting the invalid run. Credit to @vincentkoc for the original repair in #64. Fixes #63.

Integrate PR #64 on current main and retain its structured outcomes,
run acceptance, legacy classification, and fail-closed recovery.

Archive the terminal job exit code before packaging so recovery can
prove completion. Keep the first exception for diagnostics while
classifying later verifier or infrastructure failures for acceptance.
Cover real shell exports for exits 0 and 2, post-agent failures,
aggregation validity, and explicit versus unknown recovery status.

Fixes #63

Co-authored-by: Vincent Koc <vincentkoc@ieee.org>
@steipete
steipete requested a review from a team as a code owner August 28, 2026 09:19
@clawsweeper

clawsweeper Bot commented Aug 28, 2026

Copy link
Copy Markdown

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

@clawsweeper clawsweeper Bot added merge-risk: 🚨 other 🚨 Merging this PR has meaningful risk outside the owned taxonomy. P2 Normal priority bug or improvement with limited blast radius. 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 Aug 28, 2026
@clawsweeper

clawsweeper Bot commented Aug 28, 2026

Copy link
Copy Markdown

Codex review: needs real behavior proof before merge. Reviewed August 28, 2026, 5:21 AM ET / 09:21 UTC.

ClawSweeper review

What this changes

The PR adds structured native-evaluation execution outcomes, rejects wholly invalid runs from scoring eligibility, archives terminal exit status, and expands related tests.

Regression provenance

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

Merge readiness

Blocked until stronger real behavior proof is added - 5 items remain

Keep open: an introduced P2 classification-precedence defect can hide a later verifier or infrastructure failure behind OpenClaw exit 70/71, and the external PR lacks inspectable real-behavior proof.

Priority: P2
Reviewed head: 2ca27de7cc87f7eec40155da2e8f72d3163a018e

Review scores

Measure Result What it means
Overall readiness 🦪 silver shellfish (2/6) The patch is focused and well-tested in isolation, but a concrete precedence defect and uninspectable runtime proof prevent merge readiness.
Proof confidence 🦪 silver shellfish (2/6) Needs stronger real behavior proof before merge: The body asserts real shell-export and test results but provides no inspectable after-fix transcript, logs, or artifact; add redacted live output before merge and update the PR body to trigger re-review.
Patch quality 🦐 gold shrimp (3/6) 1 actionable review finding remain.

Verification

Check Result Evidence
Real behavior Needs proof Needs stronger real behavior proof before merge: The body asserts real shell-export and test results but provides no inspectable after-fix transcript, logs, or artifact; add redacted live output before merge and update the PR body to trigger re-review.
Evidence reviewed 4 items Introduced precedence defect: The new classifier returns a harness error solely from a saved OpenClaw exit code 70/71 before checking whether the supplied exception is a later verifier or infrastructure failure.
Failure path and test gap: run_trial intentionally preserves a later outer exception for acceptance, but the new regression test models the preceding agent exit as code 1, not the OpenClaw-specific 70/71 values that trigger the incorrect precedence.
Native-eval provenance: Blame attributes the newly introduced execution_outcome routine to the PR head; earlier native-eval runner and stabilization work is attributed to Vincent Koc.
Findings 1 actionable finding [P2] Classify later failures before OpenClaw exit codes
Security None None.

How this fits together

Native evaluation executes agent tasks, captures verifier rewards and artifacts, then aggregates them into leaderboard eligibility. Fleet recovery uses exported archives and an explicit exit status to determine whether a remote run completed.

flowchart LR
A[Native task plan] --> B[Harness execution]
B --> C[Trial result and verifier reward]
C --> D[Execution outcome classification]
D --> E[Run acceptance decision]
E --> F[Aggregate leaderboard]
B --> G[Remote artifact archive]
G --> H[Fleet recovery status]
Loading

Before merge

  • Add real behavior proof - Needs stronger real behavior proof before merge: The body asserts real shell-export and test results but provides no inspectable after-fix transcript, logs, or artifact; add redacted live output before merge and update the PR body to trigger re-review.
  • Classify later failures before OpenClaw exit codes (P2) - When an agent exits 70/71 and a later verifier or infrastructure failure reaches the outer handler, agent_exit_code remains set and this branch returns harness_error without examining that later exception. This contradicts the stated goal of not letting an agent exit hide a later failure; restrict this special case to NonZeroAgentExitCodeError or classify the later exception first, then add a 70/71 regression case.
  • Resolve merge risk (P1) - A run with OpenClaw exit 70/71 followed by a verifier or infrastructure failure will be labeled as a harness failure, obscuring the later fault even though the PR claims to preserve it.
  • Resolve merge risk (P1) - The PR body reports focused and full-suite results but supplies no inspectable after-fix terminal output, logs, or artifact from a real setup.
  • Complete next step (P2) - A narrow mechanical repair can correct the precedence bug, though the contributor must still supply real-behavior proof before merge.

Findings

  • [P2] Classify later failures before OpenClaw exit codes — scripts/native_eval/runtime.py:1322-1330
Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Patch surface 9 files, +476/-43 The change spans runner, aggregation, fleet recovery, and three test modules.
Production versus test delta production +195/-30; tests +280/-13 The substantial focused test growth is useful, but it misses the 70/71 precedence case.

Root-cause cluster

Relationship: fixed_by_candidate
Canonical: #63
Summary: This PR is a candidate fix for the native-evaluation acceptance defect reported in the canonical 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. Correct later-failure precedence (recommended)
    Classify the final verifier or infrastructure exception before the 70/71 mapping and add focused regressions for both exit codes.
  2. Pause this repair
    Do not merge if preserving the original harness classification is intentional, because that choice conflicts with the stated diagnostic-fidelity goal.
Copy recommended automerge instruction
@clawsweeper automerge

Special instructions:
Classify a later verifier or infrastructure exception before OpenClaw's 70/71 legacy exit mapping, and add focused regression coverage for those exact sequences.

Technical review

Best possible solution:

Classify the final verifier or infrastructure exception before applying the legacy 70/71 harness mapping, cover that exact sequence, and attach redacted live validation evidence before merge.

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

Yes, by source: an agent exit of 70/71 followed by a verifier or infrastructure exception reaches the new classifier with both values, and its early exit-code branch returns harness_error rather than the later failure.

Is this the best way to solve the issue?

No: the legacy 70/71 mapping must apply only when the final exception is the agent-exit error, or after later verifier and infrastructure failures have been classified.

Full review comments:

  • [P2] Classify later failures before OpenClaw exit codes — scripts/native_eval/runtime.py:1322-1330
    When an agent exits 70/71 and a later verifier or infrastructure failure reaches the outer handler, agent_exit_code remains set and this branch returns harness_error without examining that later exception. This contradicts the stated goal of not letting an agent exit hide a later failure; restrict this special case to NonZeroAgentExitCodeError or classify the later exception first, then add a 70/71 regression case.
    Confidence: 0.99

Overall correctness: patch is incorrect
Overall confidence: 0.99

AGENTS.md: not found in the target repository.

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

Labels

Label changes:

  • add P2: The defect affects native benchmark diagnostic correctness and run classification with bounded operational blast radius.
  • add merge-risk: 🚨 other: A green suite can still merge incorrect precedence that misreports the final cause of invalid native runs.
  • add rating: 🦪 silver shellfish: Overall readiness is 🦪 silver shellfish; proof is 🦪 silver shellfish and patch quality is 🦐 gold shrimp.
  • add status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs stronger real behavior proof before merge: The body asserts real shell-export and test results but provides no inspectable after-fix transcript, logs, or artifact; add redacted live output before merge and update the PR body to trigger re-review.

Label justifications:

  • P2: The defect affects native benchmark diagnostic correctness and run classification with bounded operational blast radius.
  • merge-risk: 🚨 other: A green suite can still merge incorrect precedence that misreports the final cause of invalid native runs.
  • rating: 🦪 silver shellfish: Overall readiness is 🦪 silver shellfish; proof is 🦪 silver shellfish and patch quality is 🦐 gold shrimp.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs stronger real behavior proof before merge: The body asserts real shell-export and test results but provides no inspectable after-fix transcript, logs, or artifact; add redacted live output before merge and update the PR body to trigger re-review.

Evidence

Acceptance criteria:

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

What I checked:

  • Introduced precedence defect: The new classifier returns a harness error solely from a saved OpenClaw exit code 70/71 before checking whether the supplied exception is a later verifier or infrastructure failure. (scripts/native_eval/runtime.py:1322, 2ca27de7cc87)
  • Failure path and test gap: run_trial intentionally preserves a later outer exception for acceptance, but the new regression test models the preceding agent exit as code 1, not the OpenClaw-specific 70/71 values that trigger the incorrect precedence. (tests/test_native_eval_runner.py:123, 2ca27de7cc87)
  • Native-eval provenance: Blame attributes the newly introduced execution_outcome routine to the PR head; earlier native-eval runner and stabilization work is attributed to Vincent Koc. (scripts/native_eval/runtime.py:1314, 2ca27de7cc87)
  • Related report: The linked report defines the required distinction: preserve diagnostic rewards while rejecting runs invalidated by harness, infrastructure, or verifier failures. (2ca27de7cc87)

Likely related people:

  • vincentkoc: Introduced the auditable native matrix runner and its early stabilization and eligibility work. (role: native-eval feature introducer; confidence: high; commits: 69f75c6629c4, b9acd9f7a010, 9afe6a4f1746; files: scripts/native_eval/runtime.py, scripts/native_eval/aggregate.py, scripts/native_eval/fleet.py)
  • steipete: Authored the current-main harness bootstrap immediately preceding this PR and the introduced execution-validity implementation. (role: recent native-eval contributor; confidence: high; commits: 643c395f6793, 2ca27de7cc87; files: scripts/native_eval/runtime.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.

  • Classify later verifier or infrastructure failures before the 70/71 compatibility mapping.
  • Add regressions where exit 70 and 71 are followed by verifier and infrastructure failures.
  • Attach redacted terminal output or logs from the actual focused validation; a PR-body update should trigger re-review, or a maintainer can request @clawsweeper re-review.

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.

@steipete

Copy link
Copy Markdown
Contributor Author

Replaced by the rebased branch (see the new PR) — branch protection blocked force-pushing the rebase here. Same reviewed integration, now on current main.

@steipete steipete closed this Aug 28, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge-risk: 🚨 other 🚨 Merging this PR has meaningful risk outside the owned taxonomy. P2 Normal priority bug or improvement with limited blast radius. 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.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Native eval accepts fully invalid harness runs

1 participant