Skip to content

fix(native-eval): rebootstrap replacement leases - #58

Open
vincentkoc wants to merge 1 commit into
mainfrom
fix/native-fleet-rebootstrap
Open

fix(native-eval): rebootstrap replacement leases#58
vincentkoc wants to merge 1 commit into
mainfrom
fix/native-fleet-rebootstrap

Conversation

@vincentkoc

@vincentkoc vincentkoc commented Jul 29, 2026

Copy link
Copy Markdown
Member

What does this PR do?

Rehydrates replacement native-eval leases before dispatch.

Why?

The controller previously treated an old bootstrap timestamp as proof that a
new lease contained the runner. Recovery could then fail before the harness
started, producing a false benchmark failure.

Fixes #55

Changes

  • record the exact bootstrapped_lease_id
  • reuse bootstrap state only when it matches the active lease
  • add a regression test for recovery with an old timestamp and replacement lease

Live proof

Recovered a legacy OpenClaw run whose manifest had an old bootstrap timestamp
but no bootstrapped_lease_id.

  • rebound the run to its active replacement lease
  • performed the full native-runner bootstrap on that lease
  • persisted the exact active lease as bootstrapped_lease_id
  • dispatched the OpenClaw harness successfully
  • exported a verified final artifact with 4/4 completed results
  • run exit code: 0

This is the exact recovery state the regression test covers; the controller no
longer trusts timestamp-only bootstrap state.

Tests

  • 40 focused fleet tests pass
  • Python 3.11 and Python 3.12 CI pass
  • Ruff passes on the touched files
  • fresh Codex autoreview reports no actionable findings
  • live replacement-lease recovery completes a four-task OpenClaw run

@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. 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: needs maintainer review before merge. Reviewed July 30, 2026, 4:38 AM ET / 08:38 UTC.

ClawSweeper review

What this changes

The PR records which Crabbox lease received native-runner bootstrap and rehydrates the active replacement lease before dispatch when that identity does not match.

Merge readiness

⚠️ Ready for maintainer review - 1 item remains

This focused recovery fix remains necessary because the supplied current-main SHA does not include the open PR branch. The patch aligns with the linked replacement-lease failure, adds targeted regression coverage, and should stay open for normal maintainer review because its author is a repository member.

Priority: P2
Reviewed head: 8949dc8328e9b86a0cf1f80bcf3a976c67c4837a

Review scores

Measure Result What it means
Overall readiness 🐚 platinum hermit (4/6) A narrow, well-targeted repair with direct regression coverage and no actionable correctness findings; maintainer confirmation of the legacy recovery behavior remains the relevant merge check.
Proof confidence 🌊 off-meta tidepool Not applicable: The author is a repository member, so the external-contributor real-behavior-proof gate does not apply; the PR body nevertheless reports a successful four-task live recovery run.
Patch quality 🐚 platinum hermit (4/6) No actionable review findings were identified.

Verification

Check Result Evidence
Real behavior Not applicable Not applicable: The author is a repository member, so the external-contributor real-behavior-proof gate does not apply; the PR body nevertheless reports a successful four-task live recovery run.
Evidence reviewed 5 items Replacement-lease guard: The proposed controller change replaces timestamp-only reuse with a comparison between persisted bootstrapped_lease_id and the active lease ID, then hydrates when they differ.
State is persisted after hydration: The hydration path now saves the exact lease ID together with the existing bootstrap timestamp, making later resume decisions lease-specific.
Regression scenario: The added test constructs a recovery-required run with an old bootstrap timestamp and no lease ID, then verifies rehydration, lease-ID persistence, and successful dispatch.
Findings None None.
Security None None.

How this fits together

The native-eval fleet controller reads a persisted run manifest, binds the run to a remote Crabbox lease, prepares the remote runner, and dispatches the benchmark harness. Bootstrap state must correspond to the active lease so recovery does not dispatch onto a replacement machine that lacks the runner.

flowchart LR
  A[Persisted run manifest] --> B[Recovery lease selection]
  B --> C[Compare bootstrap lease identity]
  C -->|Different or absent| D[Hydrate active remote lease]
  C -->|Same lease| E[Reuse bootstrap state]
  D --> F[Dispatch native benchmark runner]
  E --> F
  F --> G[Verified artifacts and completion state]
Loading

Before merge

  • Resolve merge risk (P1) - Legacy manifests that contain a bootstrap timestamp but no lease ID will perform one additional hydration on their first resumed dispatch; maintainers should accept that small recovery-time cost as the compatibility-safe migration behavior.
Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Focused patch 2 files affected; 32 added, 1 removed The implementation changes one recovery decision and adds one direct regression test, keeping the fix narrowly scoped.
Validation surface 1 controller path, 1 recovery regression test The test exercises the exact persisted-state condition that previously skipped remote hydration.

Merge-risk options

Maintainer options:

  1. Accept the legacy recovery migration (recommended)
    Merge with the documented one-time rehydration for manifests that have a timestamp but no recorded bootstrap lease ID, preserving safe recovery on replacement machines.
  2. Pause for explicit migration policy
    Hold the PR if maintainers want a different treatment for legacy manifests, such as an explicit operator notice or alternate migration contract.

Technical review

Best possible solution:

Merge the lease-identity check after a maintainer confirms that one-time rehydration of ambiguous legacy recovery manifests is the intended compatibility behavior.

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

Yes, from source: a recovery-required manifest with an old bootstrapped_at_utc value and no bootstrapped_lease_id follows the prior timestamp-only reuse path, while the supplied regression test models that exact state. I did not execute the test in this read-only review.

Is this the best way to solve the issue?

Yes: tying bootstrap reuse to the active lease identity is the narrowest maintainable repair because the remote runner is lease-local and a timestamp cannot establish that a replacement machine was prepared.

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

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

Labels

Label justifications:

  • P2: This is a bounded native-eval recovery failure that can produce false benchmark failures but does not indicate a security, data-loss, or core-runtime emergency.
  • merge-risk: 🚨 compatibility: The patch intentionally changes how legacy manifests without a bootstrap lease ID resume, adding a one-time rehydration operation before dispatch.
  • rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🌊 off-meta tidepool and patch quality is 🐚 platinum hermit.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Not applicable: The author is a repository member, so the external-contributor real-behavior-proof gate does not apply; the PR body nevertheless reports a successful four-task live recovery run.

Evidence

What I checked:

  • Replacement-lease guard: The proposed controller change replaces timestamp-only reuse with a comparison between persisted bootstrapped_lease_id and the active lease ID, then hydrates when they differ. (scripts/native_eval/fleet.py:566, 8949dc8328e9)
  • State is persisted after hydration: The hydration path now saves the exact lease ID together with the existing bootstrap timestamp, making later resume decisions lease-specific. (scripts/native_eval/fleet.py:898, 8949dc8328e9)
  • Regression scenario: The added test constructs a recovery-required run with an old bootstrap timestamp and no lease ID, then verifies rehydration, lease-ID persistence, and successful dispatch. (tests/test_native_eval_fleet.py:1276, 8949dc8328e9)
  • Canonical report relationship: The linked report describes the same failure mode: recovery moves to a replacement lease while an old bootstrap timestamp causes the runner hydration step to be skipped.
  • Prior review continuity: The previous three ClawSweeper review cycles reported no discrete patch findings; the supplied current PR state remains cleanly mergeable and its Python and CodeQL checks are successful. (8949dc8328e9)

Likely related people:

  • vincentkoc: Authored both the linked failure report and this narrowly matching recovery patch, including the regression scenario and stated live recovery validation; repository history could not be independently inspected in the read-only shell session. (role: native-eval recovery contributor; confidence: low; commits: 8949dc8328e9; files: scripts/native_eval/fleet.py, tests/test_native_eval_fleet.py)

Rank-up moves

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

  • Confirm that a one-time hydration for legacy manifests lacking bootstrapped_lease_id is the intended upgrade behavior before merging.

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 (3 earlier review cycles)
  • reviewed 2026-07-29T16:24:16.302Z sha 8949dc8 :: needs maintainer review before merge. :: none
  • reviewed 2026-07-29T17:33:21.029Z sha 8949dc8 :: needs maintainer review before merge. :: none
  • reviewed 2026-07-29T21:18:33.256Z sha 8949dc8 :: needs maintainer review before merge. :: none

@vincentkoc
vincentkoc marked this pull request as ready for review July 29, 2026 17:29
@vincentkoc
vincentkoc requested a review from a team as a code owner July 29, 2026 17:29
@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 29, 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. P2 Normal priority bug or improvement with limited blast radius. 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.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Rebootstrap native evals when recovery replaces a lease

1 participant