Skip to content

fix: reject mixed candidate preflight failures - #165

Open
goutamadwant wants to merge 5 commits into
openclaw:mainfrom
goutamadwant:fix/candidate-preflight-mixed-failures
Open

fix: reject mixed candidate preflight failures#165
goutamadwant wants to merge 5 commits into
openclaw:mainfrom
goutamadwant:fix/candidate-preflight-mixed-failures

Conversation

@goutamadwant

@goutamadwant goutamadwant commented Sep 8, 2026

Copy link
Copy Markdown

What Problem This Solves

Candidate upgrade preflight treated a failure as an unsupported-check compatibility case whenever any matching unsupported finding appeared. A response containing that finding plus a real validation failure could therefore continue to finalization.

Closes #128.

Why This Change Was Made

Unsupported preflight is now accepted only when the complete result represents that single compatibility condition. Structured output must contain exactly one matching finding with non-contradictory summary fields and no stderr. Registry-dependent checksSkipped counts remain accepted when numeric. Text fallback accepts one exact unsupported-command or unsupported-option diagnostic plus benign usage guidance, and fails closed for malformed JSON or additional diagnostics.

User Impact

Upgrades no longer finalize a candidate when its preflight output mixes an unsupported-check diagnostic with a real failure. Legacy candidates that only lack doctor or --lint remain compatible.

Evidence

The focused unit regression failed before the production change because mixed findings were classified as unsupported. After the fix, the unit matrix rejects mixed findings, contradictory fields, malformed JSON, appended fatal text, and stderr failures while preserving sole-unsupported compatibility cases with zero, one, or multiple skipped registry checks.

The integration regression drives ocm upgrade through a candidate returning both the unsupported lint-selection finding and an invalid-configuration finding. The upgrade now fails, preserves the existing old-local runtime binding, and never invokes update finalize.

The branch was refreshed onto current main at d2efc90f4592953b90435626d9f5576da6711ff4 in signed head 8c01be13dcb7a4c387232b0881c93441c1dbf331. The only conflict was an adjacent test insertion; the resolution retained both this regression and the new upstream release-reporting regression. The resulting comparison remains limited to src/cli/upgrade.rs and tests/upgrade_command_tests.rs.

Validation after the current-main refresh:

cargo fmt --check
cargo check --workspace --all-targets --locked
cargo test --locked codex_candidate_probe_only_skips_explicit_unsupported_results
cargo test --locked --test upgrade_command_tests upgrade_rejects_mixed_unsupported_candidate_failure_before_finalization
cargo test --locked --test upgrade_command_tests named_runtime_upgrade_preserves_non_comparable_release_version

All commands passed. The check emitted only pre-existing deprecation warnings. Broad validation before this refresh also passed cargo test --test upgrade_command_tests with 62 tests.

Earlier validation at the prior exact head also passed:

cargo check --workspace --all-targets --locked
cargo test --locked --lib codex_candidate_probe
cargo test --locked --test upgrade_command_tests upgrade_rejects_mixed_unsupported_candidate_failure_before_finalization -- --exact
cargo test --locked --test bin_wrapper_tests

With Node 24.16.0 active, cargo test --locked passed the 390 library tests (one ignored), behavior, wrapper, CLI, and validation suites before reaching one failing daemon fixture. The remaining daemon_defers_a_saved_service_start_until_source_watch_releases_the_env assertion fails identically in isolation on untouched main at 721ecf9; it is not introduced by this two-file change.

Real OpenClaw output for the candidate-failure and retained-binding paths is not available in this environment, so the behavior evidence above is fixture-backed rather than a live installation transcript.

Disclosure: AI was used to understand the codebase and review the fix.

@clawsweeper

clawsweeper Bot commented Sep 8, 2026

Copy link
Copy Markdown

🦞👀
ClawSweeper picked this up.

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

ClawSweeper review complete

ClawSweeper finished reviewing this revision. The review result is being finalized.

View the workflow run.

@clawsweeper clawsweeper Bot added 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. 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 Sep 8, 2026
@clawsweeper

clawsweeper Bot commented Sep 8, 2026

Copy link
Copy Markdown

Codex review: needs real behavior proof before merge. Reviewed September 13, 2026, 12:48 PM ET / 16:48 UTC (Revision 5).

ClawSweeper review

What this changes

Tightens OCM’s candidate upgrade checks to reject mixed or malformed failures and adds regression coverage for retaining the previous runtime without finalizing the candidate.

Merge readiness

Blocked before merge - 3 items remain

The fix remains necessary: current main and v0.2.46 still accept mixed failures. The earlier skipped-count finding is resolved, and no blocking code defect was found, but real upgrade and legacy-compatibility proof remains outstanding.

Priority: P2
Reviewed head: 8c01be13dcb7a4c387232b0881c93441c1dbf331

Review scores

Measure Result What it means
Overall readiness 🦪 silver shellfish (2/6) The focused patch and regression coverage are sound, but fixture-only evidence leaves the required real-behavior gate unmet.
Proof confidence 🦪 silver shellfish (2/6) Needs real behavior proof before merge: OCM’s classifier is exercised through ocm upgrade against scripted OpenClaw executables, with assertions for retained binding and suppressed finalization. The captured body explicitly lacks real OpenClaw upgrade/recovery output, so the prior real-behavior and legacy-compatibility proof request remains unresolved. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Patch quality 🐚 platinum hermit (4/6) No actionable review findings were identified.

Verification

Check Result Evidence
Real behavior Needs proof Needs real behavior proof before merge: OCM’s classifier is exercised through ocm upgrade against scripted OpenClaw executables, with assertions for retained binding and suppressed finalization. The captured body explicitly lacks real OpenClaw upgrade/recovery output, so the prior real-behavior and legacy-compatibility proof request remains unresolved. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Evidence reviewed 9 items Verified introduced scope: The complete local main-to-head diff matches the supplied introduction evidence: only the classifier, its unit tests, and the upgrade integration fixture/test change.
Current main still contains the defect: The main classifier accepts any matching JSON finding or text substring. Additional failures therefore do not prevent the compatibility exception.
Latest release remains affected: GitHub identifies v0.2.46 as the latest release; reading that tag’s classifier confirms the same existential JSON matching and substring fallback.
Findings None None.
Security None None.

How this fits together

OCM runs the candidate OpenClaw runtime’s doctor check after configuration repair and before upgrade finalization. Its interpretation of that command’s exit status and diagnostics determines whether the upgrade continues or enters recovery.

flowchart TD
 A[Requested runtime upgrade] --> B[Repair target configuration]
 B --> C[Run candidate doctor check]
 C --> D{Successful or solely unsupported?}
 D -->|Yes| E[Finalize candidate]
 E --> F[Publish runtime binding]
 D -->|No| G[Fail upgrade and recover previous state]
Loading

Before merge

  • Add real behavior proof - Needs real behavior proof before merge: OCM’s classifier is exercised through ocm upgrade against scripted OpenClaw executables, with assertions for retained binding and suppressed finalization. The captured body explicitly lacks real OpenClaw upgrade/recovery output, so the prior real-behavior and legacy-compatibility proof request remains unresolved. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
  • Resolve merge risk (P1) - Rejecting all stderr alongside JSON and accepting only narrowly matched text could stop otherwise valid legacy upgrades that emit benign warnings or additional help text; actual supported-version output has not established compatibility.
  • Complete next step (P2) - Provide after-fix proof from an isolated real OpenClaw setup showing candidate failure recovery, retained binding, and successful supported and legacy upgrades. Terminal screenshots or recordings are preferred when useful; copied output and logs also count. Redact credentials, IP addresses, private endpoints, and personal details. Update the PR body to trigger re-review; if it does not run, ask a maintainer to comment @clawsweeper re-review.
Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Production and test growth Production net +49 lines; tests net +71 lines Production growth implements complete-result classification, with additional unit and integration coverage for the stated failure.

Root-cause cluster

Relationship: fixed_by_candidate
Canonical: #128
Summary: This PR is the implementation candidate for the explicitly linked mixed-preflight-failure 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. Verify real legacy diagnostics (recommended)
    Provide real supported and legacy upgrade output, preserving benign compatibility diagnostics narrowly if those runs expose a mismatch.

Technical review

Best possible solution:

Validate the complete failed result while preserving genuine legacy compatibility, with real upgrade evidence confirming recovery and successful supported and legacy candidates.

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

Yes, from source: a nonzero candidate response containing an unsupported marker plus a genuine error satisfies main’s classifier and returns success. This review did not execute that path.

Is this the best way to solve the issue?

Yes, the classifier is the narrow repair boundary, and the patch preserves config-repair and finalization ordering; real legacy output is still needed to validate its stricter acceptance rules.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning medium; reviewed against d2efc90f4592.

Labels

Label justifications:

  • P2: This repairs a bounded upgrade-validation defect without evidence of an active outage.
  • merge-risk: 🚨 compatibility: Stricter diagnostic matching may reject legitimate legacy candidates whose actual output differs from the synthetic fixtures.
  • rating: 🦪 silver shellfish: Overall readiness is 🦪 silver shellfish; proof is 🦪 silver shellfish and patch quality is 🐚 platinum hermit.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: OCM’s classifier is exercised through ocm upgrade against scripted OpenClaw executables, with assertions for retained binding and suppressed finalization. The captured body explicitly lacks real OpenClaw upgrade/recovery output, so the prior real-behavior and legacy-compatibility proof request remains unresolved. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Evidence

What I checked:

  • Verified introduced scope: The complete local main-to-head diff matches the supplied introduction evidence: only the classifier, its unit tests, and the upgrade integration fixture/test change. (src/cli/upgrade.rs:5593, 8c01be13dcb7)
  • Current main still contains the defect: The main classifier accepts any matching JSON finding or text substring. Additional failures therefore do not prevent the compatibility exception. (src/cli/upgrade.rs:5593, d2efc90f4592)
  • Latest release remains affected: GitHub identifies v0.2.46 as the latest release; reading that tag’s classifier confirms the same existential JSON matching and substring fallback. (src/cli/upgrade.rs, d2efc90f4592)
  • Production boundary and ordering: OCM executes the selected OpenClaw runtime with doctor --lint --only codex/managed-app-server --json. A successful exit bypasses the classifier; failed output reaches it after config repair and before finalization. Compatibility depends on OpenClaw CLI diagnostics, not the Codex harness. (src/cli/upgrade.rs:3770, 8c01be13dcb7)
  • Regression coverage and previous finding: The revised unit matrix accepts numeric skipped counts including zero and four, resolving the earlier finding. The new integration test supplies mixed failures through a scripted OpenClaw executable and asserts failure, retained old-local binding, and no update finalize invocation. (tests/upgrade_command_tests.rs:6800, 8c01be13dcb7)
  • Captured proof and review continuity: The supplied complete body at sourceRevision 42c07710a6dbc8be8f944b8e462c8c7357e99fe76aed7b250c261812713ac396 reports passing focused checks but explicitly says real OpenClaw failure/recovery output is unavailable. The previous completed review requested real failure recovery and supported/legacy upgrade evidence; that request remains unresolved. No tests were executed during this read-only review. (8c01be13dcb7)

Likely related people:

  • unknown: The claimed source-line change could not be verified from bounded local history. (role: source history unknown; confidence: low)
  • shakkernerd: 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 redacted after-fix terminal output or a recording showing candidate-failure recovery and successful supported and legacy unsupported-functionality upgrades.

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 (4 earlier review cycles)
  • reviewed 2026-09-08T03:28:51.606Z sha c4a6cfb :: needs real behavior proof before merge. :: [P1] Preserve registry-dependent skipped-check counts
  • reviewed 2026-09-08T05:42:02.294Z sha 5415191 :: needs real behavior proof before merge. :: none
  • reviewed 2026-09-11T07:16:41.915Z sha bbd2417 :: needs real behavior proof before merge. :: none
  • reviewed 2026-09-12T02:03:11.578Z sha 2649303 :: needs real behavior proof before merge. :: none

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: 🦪 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.

Candidate preflight treats mixed unsupported-check failures as success

1 participant