fix: reject mixed candidate preflight failures - #165
Conversation
|
🦞👀 Pull request received. I will update this pull request when review starts. ClawSweeper review completeClawSweeper finished reviewing this revision. The review result is being finalized. |
|
Codex review: needs real behavior proof before merge. Reviewed September 13, 2026, 12:48 PM ET / 16:48 UTC (Revision 5). ClawSweeper reviewWhat this changesTightens 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 Review scores
Verification
How this fits togetherOCM 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]
Before merge
Agent review detailsSecurityNone. Review metrics
Root-cause clusterRelationship: Members:
Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything. Merge-risk optionsMaintainer options:
Technical reviewBest 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. LabelsLabel justifications:
EvidenceWhat I checked:
Likely related people:
Rank-up movesOptional improvements that raise the rating; they are not merge blockers.
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
HistoryReview history (4 earlier review cycles)
|
…ht-mixed-failures # Conflicts: # tests/upgrade_command_tests.rs
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
checksSkippedcounts 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
doctoror--lintremain 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 upgradethrough a candidate returning both the unsupported lint-selection finding and an invalid-configuration finding. The upgrade now fails, preserves the existingold-localruntime binding, and never invokesupdate finalize.The branch was refreshed onto current
mainatd2efc90f4592953b90435626d9f5576da6711ff4in signed head8c01be13dcb7a4c387232b0881c93441c1dbf331. 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 tosrc/cli/upgrade.rsandtests/upgrade_command_tests.rs.Validation after the current-main refresh:
All commands passed. The check emitted only pre-existing deprecation warnings. Broad validation before this refresh also passed
cargo test --test upgrade_command_testswith 62 tests.Earlier validation at the prior exact head also passed:
With Node 24.16.0 active,
cargo test --lockedpassed the 390 library tests (one ignored), behavior, wrapper, CLI, and validation suites before reaching one failing daemon fixture. The remainingdaemon_defers_a_saved_service_start_until_source_watch_releases_the_envassertion fails identically in isolation on untouchedmainat721ecf9; 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.