Skip to content

fix(openclaw): reject malformed health responses - #348

Merged
loveRhythm1990 merged 1 commit into
matrixorigin:mainfrom
MatrixAdventurer:fix/openclaw-health-response-20261021
Oct 21, 2026
Merged

loveRhythm1990 merged 1 commit into
matrixorigin:mainfrom
MatrixAdventurer:fix/openclaw-health-response-20261021

Conversation

@MatrixAdventurer

Copy link
Copy Markdown
Contributor

由 LW 授权的 ChatGPT Business 智能体提交。

What type of PR is this?

  • fix (bug fix)
  • test (regression coverage)

Which issue(s) this PR fixes

New narrowly scoped OpenClaw readiness-reporting defect. Repository issue/PR searches for malformed health responses, missing status, and false-positive health found no existing fix; no automatic issue closure.

What this PR does / why we need it

MemoriaHttpTransport.healthCheck() currently converts any successful HTTP response that is not the documented /health/instance object—or that omits status—into status: "ok". A wrong proxy target, incompatible server, or malformed upstream response can therefore be reported as healthy.

  • Require a non-empty string status in successful health responses.
  • Trim the status and fail closed with a clear error when the response violates the API contract.
  • Preserve valid status, instance_id, and db values.
  • Add regression coverage for missing, blank, and non-object status responses.

This does not change handling of real HTTP failures or the server's documented ok / degraded semantics.

Scope

2 files, +15/-1, based on upstream main 0695ad61eec9104478f05f5824b75bdbeeca5616.
Head dd6ba90661a9893b46557cacb98efc2558361a81; remote blobs exactly match the locally tested files.
No upstream dev branch exists, so this PR targets main.

Verification

Node 24.19.0 / npm 11.9.0, working directory plugins/openclaw:

  • Identical regression test on unchanged baseline: 3 failed / 46 passed, proving missing, blank, and non-object responses were accepted.
  • Fixed focused suite: 49/49 passed.
  • Full npm test: 206/206 passed across 12 files.
  • npm run test:pack: passed, including bundle build, package validation, and all 6 packed files.
  • git diff --check: passed.
  • Independent review agent: Approve; independently repeated baseline red, focused/full green, packaging, and diff checks.

Tests exercise the real transport request/response path with the existing fetch fixture. No live Memoria Cloud or MatrixOne integration test is claimed.

Remaining gates

  • Upstream CI is pending.
  • No force-push, merge, deployment, or package publication.

由 LW 授权的 ChatGPT Business 智能体提交。
@loveRhythm1990

Copy link
Copy Markdown
Collaborator

Code review: LGTM on dd6ba90661a9893b46557cacb98efc2558361a81, with CI gates still outstanding.

The reported defect is real: successful HTTP responses with a missing/non-string status were converted to ok, and blank statuses were accepted. Requiring a non-empty string status correctly rejects these malformed responses. I checked /health/instance: the server emits a string status, and the existing HTTP error path remains unchanged. Valid status values and optional instance/database fields are preserved.

Locally, all 206 OpenClaw tests and package build/verification passed. The same focused suite against the baseline transport produced 3 failures and 46 passes, reproducing the malformed-response cases. Coverage: 2/2 changed files reviewed (100%), 0 skipped; no actionable code issues found.

At the time of review, OpenClaw CI, Rust unit tests, and Check & Clippy have passed, but DB Tests are still running, so full CI success is not yet established. Separately, Mergify rejects the existing queue_rules[0].autoqueue configuration; this PR does not change that file. Please resolve the queue configuration and confirm the remaining DB checks pass. No local database integration run is claimed.

@loveRhythm1990

Copy link
Copy Markdown
Collaborator

Additional validation on dd6ba90661a9893b46557cacb98efc2558361a81: 15 synthetic checks through the registered memory_health tool and native loopback HTTP passed.

These cover null/array/object responses, non-string and blank statuses, plain text, empty HTTP 204 responses, HTTP 403/503 failures, valid trimmed statuses, preservation of db: false and instance_id, and the service startup warning path for malformed responses. Non-empty statuses remain visible as returned; the change does not coerce an unknown/degraded status into ok.

No additional code issues found. Coverage remains 2/2 changed files, 0 skipped. DB Tests are still running at this check, and the previously noted Mergify configuration failure remains outstanding.

Copy link
Copy Markdown
Contributor Author

CI follow-up for dd6ba90661a9893b46557cacb98efc2558361a81: the remaining Test workflow has completed successfully. Check & Clippy, Unit Tests, DB Tests, OpenClaw tests/package verification, and PR title validation are now all green.

The Mergify queue_rules[0].autoqueue validation error is a pre-existing repository-wide configuration issue outside this two-file health-response patch; this PR does not modify .mergify.yml or rely on auto-merge. No merge or deployment was performed.

由 LW 授权的 ChatGPT Business 智能体提交。

@aptend aptend left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Independent deep review of exact head dd6ba90661a9893b46557cacb98efc2558361a81, base/merge-base 0695ad61eec9104478f05f5824b75bdbeeca5616. Read both changed files, all historical review/discussion (no inline threads), the actual server endpoint, HTTP parsing/error/timeout owner and client/tool/startup consumers. No blocking finding.

Successful malformed/missing/non-string/blank statuses now fail closed instead of becoming ok; valid statuses are trimmed and preserved, not coerced from unknown/degraded into ok. Arrays/null/plain text/empty responses cannot supply a valid record status. Optional instance_id and boolean db remain type-checked, including false. Existing HTTP failure handling, timeout cleanup and /health/instance server semantics remain unchanged. Rejection propagates through client.health and memory_health; startup records a warning rather than a false connected result. No new resource/async ownership is introduced.

Verified exact-head OpenClaw CI 39531927038, job 118999593240, raw logs on Node22.23.3: HTTP suite49/49, full suite206/206 across12files, build/package validation and6 packed files all PASS. git diff --check PASS. Reused qualified CI; no local npm install/live cloud or MatrixOne test, and no claim that another reviewer's15 additional synthetic checks were executed by me. No product edits/probes.

@loveRhythm1990
loveRhythm1990 merged commit 919b2a0 into matrixorigin:main Oct 21, 2026
5 of 6 checks passed
loveRhythm1990 added a commit that referenced this pull request Oct 21, 2026
## What type of PR is this?

- [x] build / ci (build or CI changes)

## What this PR does / why we need it

**Behavior change: this proposal restores valid configuration, but does
not restore automatic enqueueing with the currently reported product
settings. Maintainers would use `@mergifyio queue`. The enqueueing
policy needs a maintainer decision before merge.**

Remove rejected `queue_rules[].autoqueue` and migrate
`commit_message_template` to `commit_message_format`. Keep one approval,
no `do-not-merge`, and `base=main` as queue-level conditions. Squash
merging, timeout, concurrency, and GitHub branch protection remain
unchanged. Approver attribution becomes standard `Approved-by` trailers.

Related: #285; configuration failures on #347, #348, and #350.

## Enqueueing decision

- **Automatic:** enable Merge Protections in the dashboard, then
explicitly configure a conditional list (not unconditional `true`):

  ```yaml
  merge_protections_settings:
    auto_merge_conditions:
      - "#approved-reviews-by>=1"
      - label!=do-not-merge
      - base=main
  ```

This is an alternative, not included in the patch. Once `autoqueue` is
removed, do not rely on the bot to migrate that field automatically.
- **Manual:** accept this proposal and use `@mergifyio queue` for
eligible PRs.

The live configuration check confirms Workflow Automation is disabled,
so the retained `pull_request_rules` are inactive; this is now
documented inline. The Merge Protections prerequisite comes from the
bot's #285 notice; dashboard settings have not been independently
verified or changed.

## Validation

- Official schema validation, parsed-YAML gate checks, and `git diff
--check` passed.
- Live `Configuration changed` passed on `999ccb4`; the latest commit
only adds comments and parses identically. Latest-head CI is pending.
- No tracked references to the old approver-attribution syntax outside
`.mergify.yml` were found.
- `Mergify Merge Queue` still evaluates main's old configuration until
merge, so its `autoqueue` error is separate from proposed-file
validation.

Only `.mergify.yml` changes. No merge or product-setting changes
performed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants