Fix config.patch explicit array replacement by joshavant · Pull Request #91551 · openclaw/openclaw

joshavant · 2026-06-09T01:45:42Z

Summary

Add an explicit replacePaths contract for destructive config.patch array replacement.
Reject unconfirmed config.patch payloads that would remove existing array entries, including nested arrays under id-keyed config arrays.
Update Gateway agent tool preflight, Matrix QA, iOS skill editing, protocol schema, docs, and focused regression coverage.

Refs #90540

Verification

.agents/skills/autoreview/scripts/autoreview --mode local
- autoreview clean: no accepted/actionable findings reported
AWS Crabbox live Gateway/WebSocket/disk proof: run_c11f1f03de06, provider aws, lease cbx_baaa409355ec, machine c7a.8xlarge, exit 0
- Regression repro: old truncated config.patch attempt from 82 bindings to 2 was rejected; disk remained at 82 and hash stayed unchanged.
- Fix proof: same replacement with replacePaths: ["bindings"] succeeded and disk moved to 2 entries intentionally.
AWS Crabbox focused regression lanes passed in the same run:
- src/gateway/server.config-patch.test.ts
- src/agents/tools/gateway-tool.test.ts
- src/agents/openclaw-gateway-tool.test.ts
- extensions/qa-matrix/src/runners/contract/runtime.test.ts
- extensions/qa-matrix/src/runners/contract/scenarios.test.ts
- packages/gateway-protocol/src
Local: pnpm test src/gateway/server.config-patch.test.ts
Local: pnpm test src/agents/tools/gateway-tool.test.ts src/agents/openclaw-gateway-tool.test.ts
Local: pnpm test src/config/merge-patch.test.ts extensions/qa-matrix/src/runners/contract/runtime.test.ts extensions/qa-matrix/src/runners/contract/scenarios.test.ts packages/gateway-protocol/src
Local: pnpm format:docs
Local: git diff --check
Local: swift build in apps/shared/OpenClawKit

Known proof gaps:

Broader local swift test in apps/shared/OpenClawKit still fails existing unrelated TalkConfigContractTests.selectionFixtures fixture expectations.
Local iOS xcodebuild compiled the touched Design files, then failed existing unrelated apps/ios/Sources/Onboarding/QRScannerView.swift / AppleReviewDemoMode resolution.

Real behavior proof

Behavior addressed: config.patch could silently remove existing array entries after a truncated config.get round-trip.

Real environment tested: AWS Crabbox Linux host via Gateway WebSocket RPCs and on-disk config verification.

Exact steps or command run after this patch: pnpm crabbox:run -- --provider aws --idle-timeout 90m --ttl 240m --timing-json --script-stdin with a temporary Gateway RPC proof plus focused regression tests.

Evidence after fix: Crabbox run_c11f1f03de06 printed E2E_REGRESSION_REPRO_SUMMARY showing rejected destructive patch and unchanged disk, then E2E_FIX_PROOF_SUMMARY showing explicit replacePaths replacement was accepted.

Observed result after fix: Unconfirmed destructive array patch from 82 bindings to 2 was rejected; confirmed replacement with replacePaths: ["bindings"] succeeded.

What was not tested: Full Control UI browser flow; this bug’s mutation contract was proven at Gateway RPC and disk-write level.

clawsweeper · 2026-06-09T01:47:34Z

Codex review: needs maintainer review before merge. Reviewed June 8, 2026, 10:27 PM ET / 02:27 UTC.

Summary
Adds a config.patch replacePaths contract that rejects destructive array replacement unless explicitly confirmed, and updates Gateway, agent tool preflight, protocol schema, iOS/QA callers, docs, snapshots, and regression tests.

PR surface: Source +275, Tests +493, Docs +11, Other +14. Total +793 across 28 files.

Reproducibility: Do we have a high-confidence way to reproduce the issue? Yes from source and the linked report: current main lets config.patch write a shorter non-id array over the existing config after a config.get round-trip; I did not execute the repro because this review is read-only.

Review metrics: 1 noteworthy metric.

Config patch contract: 1 protocol parameter added, 1 fail-closed write guard added. The new replacePaths surface changes Gateway API behavior for intentional array removal, so maintainers should review upgrade impact before merge.

Merge readiness
Overall: 🐚 platinum hermit
Proof: 🦞 diamond lobster
Patch quality: 🐚 platinum hermit
Result: ready for maintainer review.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Rank-up moves:

[P1] Maintainer should explicitly confirm the fail-closed replacePaths compatibility tradeoff before merge.

Risk before merge

[P1] Existing Gateway API clients that intentionally remove array entries with config.patch will now fail with INVALID_REQUEST until they send replacePaths or use config.apply; that fail-closed upgrade looks justified by the data-loss bug but needs maintainer acceptance.

Maintainer options:

Confirm the fail-closed upgrade (recommended)
Merge after a maintainer explicitly accepts that intentional config.patch array removals must add replacePaths and that release notes or PR body context cover the new requirement.
Choose a different protocol shape
Pause if maintainers prefer a tagged config.get, diff-based array operation, or another explicit-intent API instead of replacePaths.
Preserve legacy behavior by default
If compatibility is more important than fail-closed safety, require a patch that keeps legacy array replacement as default and introduces strict rejection through an opt-in path with tests.

Next step before merge

[P2] Protected maintainer label plus the intentional Gateway API compatibility change require maintainer review; no narrow automation repair is identified.

Security
Cleared: No concrete supply-chain or secret-handling regression was found; the patch tightens a config write boundary, with compatibility risk handled separately.

Review details

Best possible solution:

Land a maintainer-approved fail-closed config.patch contract with clear upgrade/release-note context: unconfirmed destructive array writes reject, and intentional array replacement uses replacePaths or full config.apply.

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

Do we have a high-confidence way to reproduce the issue? Yes from source and the linked report: current main lets config.patch write a shorter non-id array over the existing config after a config.get round-trip; I did not execute the repro because this review is read-only.

Is this the best way to solve the issue?

Is this the best way to solve the issue? Yes, if maintainers accept the compatibility tradeoff: Gateway-side intent enforcement is the right owner boundary for preventing destructive RPC writes, and the PR updates the protocol/tool/docs/test callers that need the new contract.

AGENTS.md: found and applied where relevant.

Codex review notes: model gpt-5.5, reasoning high; reviewed against 82afc4678ace.

Label changes

Label changes:

add P1: The PR addresses a reported config.patch data-loss path that can silently remove routing bindings and misroute messages for real deployments.
add merge-risk: 🚨 compatibility: The diff intentionally makes previously accepted destructive config.patch array writes fail unless clients add replacePaths or use config.apply.
add proof: sufficient: Contributor real behavior proof is sufficient. The PR body includes after-fix AWS Crabbox live Gateway/WebSocket/disk proof showing the rejected unconfirmed destructive patch and the accepted explicit replacement.
add rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🦞 diamond lobster and patch quality is 🐚 platinum hermit.
add status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (live_output): The PR body includes after-fix AWS Crabbox live Gateway/WebSocket/disk proof showing the rejected unconfirmed destructive patch and the accepted explicit replacement.
remove rating: 🌊 off-meta tidepool: Current PR rating is rating: 🐚 platinum hermit, so this older rating label is no longer current.

Label justifications:

P1: The PR addresses a reported config.patch data-loss path that can silently remove routing bindings and misroute messages for real deployments.
merge-risk: 🚨 compatibility: The diff intentionally makes previously accepted destructive config.patch array writes fail unless clients add replacePaths or use config.apply.
rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🦞 diamond lobster and patch quality is 🐚 platinum hermit.
status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (live_output): The PR body includes after-fix AWS Crabbox live Gateway/WebSocket/disk proof showing the rejected unconfirmed destructive patch and the accepted explicit replacement.
proof: sufficient: Contributor real behavior proof is sufficient. The PR body includes after-fix AWS Crabbox live Gateway/WebSocket/disk proof showing the rejected unconfirmed destructive patch and the accepted explicit replacement.

Evidence reviewed

PR surface:

Source +275, Tests +493, Docs +11, Other +14. Total +793 across 28 files.

View PR surface stats

Area	Files	Added	Removed	Net
Source	9	294	19	+275
Tests	14	527	34	+493
Docs	2	13	2	+11
Config	0	0	0	0
Generated	0	0	0	0
Other	3	16	2	+14
Total	28	850	57	+793

What I checked:

Current main config.patch behavior: Current main parses config.patch raw, applies applyMergePatch to the current snapshot, restores redacted values, then diffs and writes the validated config; there is no destructive-array intent gate on this path. (src/gateway/server-methods/config.ts:581, 82afc4678ace)
Current main array merge semantics: Current main only merges id-keyed arrays when mergeObjectArraysById succeeds; otherwise arrays fall through to replacement semantics, matching the linked data-loss report for non-id arrays such as bindings. (src/config/merge-patch.ts:88, 82afc4678ace)
PR destructive-array guard: The PR head adds replacePaths normalization and a Gateway-side destructive-array detector that rejects unconfirmed removals before validation/write. (src/gateway/server-methods/config.ts:139, c82e172ab5f0)
Protocol and tool contract: The PR head adds optional replacePaths to the Gateway protocol schema and the agent-facing gateway tool normalizes and forwards it for config.patch only. (packages/gateway-protocol/src/schema/config.ts:56, c82e172ab5f0)
Regression coverage: The PR head covers rejection of unconfirmed array shrink/replacement, explicit replacement with replacePaths, nested id-keyed array paths, and parent deletion cases. (src/gateway/server.config-patch.test.ts:461, c82e172ab5f0)
Real behavior proof in PR body: The PR body reports AWS Crabbox live Gateway/WebSocket/disk proof run_c11f1f03de06: an unconfirmed destructive patch from 82 bindings to 2 was rejected without disk hash change, and the same replacement with replacePaths: ["bindings"] succeeded intentionally. (c82e172ab5f0)

Likely related people:

Vincent Koc: Blame and local history show the current config.patch handler, merge-patch helper, Gateway protocol schema, iOS config patch model, and QA helpers all coming from commit cff8154; shallow history limits older provenance. (role: current implementation owner in local history; confidence: medium; commits: cff8154954eb; files: src/gateway/server-methods/config.ts, src/config/merge-patch.ts, packages/gateway-protocol/src/schema/config.ts)

What the crustacean ranks mean

🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
🌊 off-meta tidepool: rating does not apply to this item.

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.

How this review workflow works

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.

* fix config patch explicit array replacement * fix generated config patch protocol model * fix config patch test helper typing * fix shared auth patch replacement tests * update config patch prompt snapshots * harden qa lab config patch replace paths

fix config patch explicit array replacement

7e362e0

openclaw-barnacle Bot added docs Improvements or additions to documentation app: ios App: ios app: web-ui App: web-ui gateway Gateway runtime agents Agent runtime and tooling size: L maintainer Maintainer-authored PR labels Jun 9, 2026

joshavant added 3 commits June 8, 2026 20:47

fix generated config patch protocol model

ad7cb89

fix config patch test helper typing

c9cc8c2

fix shared auth patch replacement tests

c2c2ecd

joshavant requested a review from a team as a code owner June 9, 2026 01:58

update config patch prompt snapshots

d3db468

clawsweeper Bot added the rating: 🌊 off-meta tidepool PR readiness rating does not apply to this item. label Jun 9, 2026

harden qa lab config patch replace paths

c82e172

openclaw-barnacle Bot added the extensions: qa-lab label Jun 9, 2026

joshavant merged commit 9f48254 into main Jun 9, 2026
206 of 217 checks passed

joshavant deleted the fix/config-patch-explicit-array-replace branch June 9, 2026 02:48

joshavant mentioned this pull request Jun 9, 2026

config.patch persists a redaction-truncated array from config.get → silent data loss of bindings[] #90540

Closed

github-actions Bot mentioned this pull request Jun 9, 2026

📡 Upstream Digest — 2026-06-09 07:39 UTC curtismercier/openclaw-mods#1043

Open

yetval mentioned this pull request Jun 10, 2026

fix(config): stop config.patch replacePaths index suffix from widening array consent #91966

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix config.patch explicit array replacement#91551

Fix config.patch explicit array replacement#91551
joshavant merged 6 commits into
mainfrom
fix/config-patch-explicit-array-replace

joshavant commented Jun 9, 2026

Uh oh!

clawsweeper Bot commented Jun 9, 2026 •

edited

Loading

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

joshavant commented Jun 9, 2026

Summary

Verification

Real behavior proof

Uh oh!

clawsweeper Bot commented Jun 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

clawsweeper Bot commented Jun 9, 2026 •

edited

Loading