Skip to content

fix(openclaw): preserve structured branch identities - #346

Merged
loveRhythm1990 merged 1 commit into
matrixorigin:mainfrom
MatrixAdventurer:fix/openclaw-branch-identity-main-20261020
Oct 21, 2026
Merged

loveRhythm1990 merged 1 commit into
matrixorigin:mainfrom
MatrixAdventurer:fix/openclaw-branch-identity-main-20261020

Conversation

@MatrixAdventurer

@MatrixAdventurer MatrixAdventurer commented Oct 20, 2026 •

Copy link
Copy Markdown
Contributor

What type of PR is this?

  • fix (bug fix)

Which issue(s) this PR fixes

Newly reproduced OpenClaw client defect; no existing issue is closed by this PR. Checked open PRs and relevant issue/PR searches before implementation.

What this PR does / why we need it

The branch-list HTTP response already contains authoritative name and boolean active fields, but the transport formatted them as display text and the client reparsed that text. This loses branch identity:

  • An inactive branch named release ← active becomes active release.
  • Embedded newlines produce invented entries; leading/trailing whitespace is removed.
  • Subsequent checkout uses the parsed, incorrect selector instead of the original branch name.

API branch registration preserves original display names (only the physical table suffix is sanitized), so these are legitimate names, not merely malformed mock responses.

Return the existing human-readable content envelope plus structuredContent for memory_branches, and consume structured names/state directly in the client. Preserve older text-only response compatibility. Empty structured lists and explicit false flags remain authoritative. No server, authentication, mutation, storage or other tool behavior is changed.

Scope: 3 files, +105/-4 (including regression tests). Base: 0695ad61eec9104478f05f5824b75bdbeeca5616; tested tree: 3387b879c2b5cb34e2286502b6200a9882b4e5c2.

Reproduction and validation

Native fetch tests use an ephemeral loopback HTTP server and the real MemoriaClient/transport, not mocked fetch:

  • Identical 12-case regression suite on unchanged base: 7 failed / 5 passed. Six failures expose behavioral differences; the seventh is the new structuredContent contract assertion.
  • Fixed branch: 215 tests / 13 files passed (cd plugins/openclaw && npm test).
  • npm run test:pack: build, package validation and all 6 packed files passed.
  • git diff --cached --check: passed.
  • Cases cover arrays/items, empty list, explicit false, legacy text, Unicode/emoji/CRLF, whitespace, activity-marker text, and exact subsequent HTTP checkout selectors.

Independent review agent: Approve, no blocking findings, on the exact current-base staged tree above. It independently reran all 215 tests, package validation, and diff check.

Local runtime: Node 24.19.0. Additional tsc --noEmit check is not green: base and fixed trees produce the same 42 errors, byte-identical output, starting with unavailable optional openclaw/plugin-sdk types and existing cascading type errors. This check is not part of the existing OpenClaw CI workflow.

Remote CI verification (2026-10-20)

All workflows passed on head ca2a3a20bc5c61750a5b6075ce642a79676d0e14:

Rust/MatrixOne validation was performed by remote CI, not locally. Marking ready for maintainer review; no automatic merge or deployment.

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

Keep exact branch names and explicit active state from the HTTP API while preserving the legacy text envelope.

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

Copy link
Copy Markdown
Collaborator

LGTM on ca2a3a20bc5c61750a5b6075ce642a79676d0e14.

The reported defect is real: formatting structured branch records as display text and reparsing them can alter names, invent entries from embedded newlines, and override an explicit false active flag. I checked the server's branch registration and listing paths: the registry preserves the original display name, and the API supplies authoritative names and boolean active flags. Reading those fields directly fixes the client-side loss while retaining text-only compatibility.

Locally, all 215 OpenClaw tests and package build/verification passed. The identical new regression suite against the baseline implementation produced 7 failures and 5 passes (six behavioral failures plus the new structured-content assertion). The tests also verify exact HTTP checkout selectors through the client. Current OpenClaw and Rust/MatrixOne CI is green, including the explicit restore/cleanup regressions.

Coverage: 3/3 changed files reviewed (100%), 0 skipped. No actionable issues found in the changes. No local database integration run is claimed.

@loveRhythm1990

Copy link
Copy Markdown
Collaborator

Non-blocking follow-up (medium severity, pre-existing): public branch mutation tools still trim branch selectors.

The structured-list change is correct, but the public-tool round trip is not fully fixed. In plugins/openclaw/openclaw/index.ts, readString() returns raw.trim() (line 173), and both memory_checkout (line 1371) and memory_branch_delete (line 1394) use it for name.

I reproduced this by registering the actual plugin with a synthetic host and using native fetch against a loopback HTTP fixture:

  • Listing can preserve a branch named " release " after this PR.
  • memory_checkout({name: " release "}) sends /v1/branches/release/checkout, instead of /v1/branches/%20release%20/checkout.
  • memory_branch_delete({name: " release "}) likewise sends /v1/branches/release.

That can select/delete a different existing branch or fail to find the intended branch. The added public-tool checks produced four failures for surrounding spaces/newline/tab across checkout/delete; four controls with embedded newlines/activity-marker text passed. The surrounding-space mutation failure also reproduces against the unchanged baseline, so this is not introduced by this PR and does not invalidate the narrow client-side fix.

Please preserve raw identifier values at the tool boundary, using trimming only to validate emptiness, and add regression coverage that invokes the registered tools rather than calling MemoriaClient.branchCheckout directly. This can be tracked separately, but the PR's exact-selector guarantee should be understood as applying to the client layer until that follow-up is fixed.

Review coverage remains 3/3 changed files, 0 skipped; the finding above is in unchanged surrounding code.

@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.

Deep-reviewed all three changed files at exact head ca2a3a2 against base/merge-base 0695ad6, including all discussion and the server/registry contracts. No new blocking issue found.

The client now consumes original structured names and boolean activity directly, including authoritative empty lists and explicit false, while preserving legacy text compatibility and the human-readable envelope. Traced the exact name into HTTP checkout encoding, malformed-record guards, existing request error/timeout/auth behavior and test-server cleanup. The twelve native-HTTP regression cases exercise the changed boundary with independent selectors/results.

Verified and reused exact-head Node 22 CI run 39385984087 / job 118505821087: all 215 tests in 13 files pass, including all 12 branch-identity cases; build/package verification and all six packed files pass. No duplicate local execution or database validation is claimed.

Scope caveat retained from the existing follow-up: public tool wrappers in unchanged index.ts still trim checkout/delete selectors. This PR correctly fixes listing and the MemoriaClient layer; it does not complete that separate whitespace-selector mutation fix.

@loveRhythm1990
loveRhythm1990 merged commit 659a777 into matrixorigin:main Oct 21, 2026
7 checks passed

Copy link
Copy Markdown
Contributor Author

Follow-up to the public-tool selector finding: #349 preserves exact non-empty branch names for registered memory_checkout and memory_branch_delete, while retaining whitespace-only rejection and every other parameter's existing trim behavior. Baseline reproduction was 4 failed / 4 passed; the fix passes the focused 8/8 cases, full suite, packaging, and independent review.

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

loveRhythm1990 pushed a commit that referenced this pull request Oct 21, 2026
由 LW 授权的 ChatGPT Business 智能体提交。

## What type of PR is this?
- [x] fix (bug fix)
- [x] test (regression coverage)

## Which issue(s) this PR fixes
Follow-up to the pre-existing defect identified in the review of #346:
#346 (comment)

Repository issue/PR searches for OpenClaw branch-selector whitespace and
`memory_checkout` trimming found no existing fix PR; no automatic issue
closure.

## What this PR does / why we need it
The server preserves branch display names exactly and resolves
checkout/delete by exact string equality. The OpenClaw public tool
wrappers currently use the general `readString()` behavior, which trims
selectors before forwarding them. A listed branch named `" release "` is
therefore checked out or deleted as `"release"`, potentially targeting a
different branch.

- Add an opt-in `preserveWhitespace` mode that still rejects
empty/whitespace-only values.
- Use it only for registered `memory_checkout` and
`memory_branch_delete` selectors.
- Leave every other string parameter's existing trimming behavior
unchanged.
- Verify exact URL encoding through registered public tools and a native
loopback HTTP server.

## Scope
**2 files, +78/-5**, based on upstream `main`
`0695ad61eec9104478f05f5824b75bdbeeca5616`.
Head `41046902369709c0999c686fd7cdd73f3136e65e`; 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`:
- Byte-identical regression test on unchanged baseline: **4 failed / 4
passed**.
- Fixed focused suite: **8/8 passed**.
- Full `npm test`: **211/211 passed** across 13 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, lockfile comparison, and diff
checks.

The regression covers checkout/delete with surrounding spaces,
tab/newline selectors, ordinary controls, and whitespace-only rejection.
It traverses the registered OpenClaw tool, client, transport, WHATWG URL
handling, and native loopback HTTP server. No live Memoria/MatrixOne
integration test is claimed.

## Remaining gates
- Upstream CI is pending.
- No force-push, merge, deployment, or package publication.
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