chore: apply event sweep result for openclaw-openclaw#79549

[skip ci]
This commit is contained in:
clawsweeper 2026-05-08 23:30:07 +00:00
parent bd1f571c24
commit 8858c46bb8

View File

@ -1,5 +1,5 @@
---
review_comment_synced_at: 2026-05-08T22:48:31.789Z
review_comment_synced_at: 2026-05-08T23:30:07.091Z
number: 79549
repository: openclaw/openclaw
type: pull_request
@ -7,13 +7,13 @@ title: "fix(config): preserve absent keys on partial writes; require unsetPaths
url: https://github.com/openclaw/openclaw/pull/79549
state_at_review: open
item_created_at: 2026-05-08T22:39:18Z
item_updated_at: 2026-05-08T22:39:33Z
item_updated_at: 2026-05-08T23:22:40Z
author: efpiva
author_association: CONTRIBUTOR
labels: ["size: M","triage: needs-real-behavior-proof"]
reviewed_at: 2026-05-08T22:47:53.563Z
main_sha: 3256316122f8f04867df0bd21fb948d3896ecee9
pull_head_sha: 3eb489cc66c274b7719143e2496135f55f22e636
reviewed_at: 2026-05-08T23:29:29.311Z
main_sha: 5d87f7b62d370d9feeb4315c0526f9f843589f79
pull_head_sha: 7035cd5b098a06e02f13ff3fee4420bdf074235a
latest_release: v2026.5.7
latest_release_sha: eeef4864494f859838fec1586bedbab1f8fa5702
fixed_release: unknown
@ -31,19 +31,19 @@ review_model: gpt-5.5
review_reasoning_effort: high
review_sandbox: danger-full-access
review_service_tier: default
review_prompt_chars: 71643
review_prompt_chars: 79970
review_static_prompt_chars: 33412
review_context_chars: 36968
review_context_chars: 45295
review_schema_chars: 14081
review_additional_prompt_chars: 0
review_context_elapsed_ms: 3761
review_codex_elapsed_ms: 320872
review_context_elapsed_ms: 3973
review_codex_elapsed_ms: 213884
review_mode: propose
review_status: complete
local_checkout_access: verified
item_snapshot_hash: ad1d4c2b18ba9b8af901bb4141f87f6820df9d88d5007a5b09c1127160beae2e
item_snapshot_hash: f1af17fb4295ea561da4a8df5ceb57a05a72b016a6531ba12ced261ee6de49d4
close_comment_sha256: none
review_comment_sha256: 671622ab6bcc8b6fbe5ac4abe695228b366897004da24d754df8993ef1d2ccf2
review_comment_sha256: 5209e5969f605353d144237b8b6c181f5eb144c797998ac2677fcc2a03af4290
review_comment_id: 4410396270
review_comment_url: https://github.com/openclaw/openclaw/pull/79549#issuecomment-4410396270
decision: keep_open
@ -52,21 +52,21 @@ confidence: high
action_taken: kept_open
work_candidate: manual_review
work_confidence: high
work_priority: medium
work_priority: high
work_status: manual_review
work_reason_sha256: b8d284bcc4885f32ad320bc1f45a7c86ec8d89aa17fe412d5c4dfe32915279ac
work_reason_sha256: 54b559e5dc6b957cf603781b76bcea8bb5b7f40d2c0f0592d79edff290c83c1b
work_prompt_sha256: none
work_cluster_refs: ["https://github.com/openclaw/openclaw/pull/79549","https://github.com/openclaw/openclaw/issues/55219"]
work_validation: ["pnpm check:test-types","pnpm test src/config/io.delivery-mode-persistence.test.ts","pnpm test src/config/io.write-config.test.ts src/config/io.write-prepare.test.ts","node scripts/run-oxlint.mjs src/config/io.write-prepare.ts src/config/io.ts"]
work_likely_files: ["src/config/io.write-config.test.ts","src/config/io.delivery-mode-persistence.test.ts","src/config/io.write-prepare.ts","src/config/io.ts"]
work_cluster_refs: []
work_validation: []
work_likely_files: []
item_category: bug
reproduction_status: source_reproducible
reproduction_confidence: high
requires_new_feature: false
requires_new_config_option: false
requires_product_decision: false
real_behavior_proof_status: insufficient
real_behavior_proof_evidence_kind: terminal
real_behavior_proof_status: missing
real_behavior_proof_evidence_kind: none
real_behavior_proof_needs_contributor_action: true
---
@ -84,9 +84,9 @@ Labels: size: M, triage: needs-real-behavior-proof
Created at: May 8, 2026, 22:39 UTC
Updated at: May 8, 2026, 22:39 UTC
Updated at: May 8, 2026, 23:22 UTC
Reviewed against: [3256316122f8](https://github.com/openclaw/openclaw/commit/3256316122f8f04867df0bd21fb948d3896ecee9)
Reviewed against: [5d87f7b62d37](https://github.com/openclaw/openclaw/commit/5d87f7b62d370d9feeb4315c0526f9f843589f79)
Codex review: model gpt-5.5, reasoning high
@ -104,55 +104,57 @@ Action taken: kept_open
## Summary
Keep open: the PR targets a source-provable config persistence bug, but it is not mergeable yet because the added regression test currently fails type checking and the external after-fix real behavior proof is still insufficient.
Keep open: current main still has the partial config write bug and this PR is a useful targeted fix, but the latest diff is not merge-ready because preservation-by-default also preserves gateway auth credentials that the CLI deletes implicitly, and the required after-fix real behavior proof is still missing.
## What This Changes
The PR changes `src/config/io.write-prepare.ts` and `src/config/io.ts` so partial config writes preserve omitted keys and deletions require explicit `unsetPaths`, with focused config regression tests.
The PR changes config merge-patch generation to preserve absent keys by default, applies explicit `unsetPaths` deletions in config write paths, and adds config persistence regression tests.
## Best Possible Solution
Repair the test type casts, get the config/typecheck CI lane green, and have the contributor add redacted after-fix terminal output or a terminal screenshot/log proving the real restart/config persistence path before merge.
Keep the preservation-by-default design, but thread every intentional deletion path, including CLI gateway auth credential cleanup, into `unsetPaths` and require redacted real docker/restart or equivalent CLI proof before merge.
## Reproduction Assessment
Yes from source inspection: current main generates `null` delete markers for keys omitted from a partial target and then applies those markers to source config. I did not run the new tests because this review was constrained to read-only inspection.
Yes from source inspection: current main generates `null` delete markers for keys omitted from a partial target and then applies those markers to the persisted source config. I did not run tests because this review was constrained to read-only inspection.
## Solution Assessment
Yes in direction, but not merge-ready: preserving absent keys by default with `unsetPaths` for intentional deletion is the narrow maintainable fix for the reported write model. The implementation still needs the added test type error fixed and real behavior proof supplied.
No as currently patched: preserving absent keys with explicit `unsetPaths` is the right direction, but the diff misses existing implicit deletion paths such as gateway auth credential cleanup. The safer fix is to add those cleanup paths to `unsetPaths` and cover them with a write-path regression test.
## Review Findings
Overall correctness: patch is incorrect
Overall confidence: 0.88
Overall confidence: 0.9
Full review comments:
- **[P2] Use a non-null channels type for Discord casts:** `src/config/io.write-config.test.ts:1484-1495`
- body: The added regression test indexes `OpenClawConfig["channels"]["discord"]`, but `channels` is optional, so `check-test-types` fails with `Property 'discord' does not exist on type 'ChannelsConfig | undefined'`. Use `NonNullable<OpenClawConfig["channels"]>["discord"]` or a small local record type for these test casts.
- confidence: 0.98
- **[P1] Pass auth cleanup deletions through unsetPaths:** `src/config/io.write-prepare.ts:31`
- body: With absent keys now preserved, deletions that happen outside explicit `delete` operations must also be sent as `unsetPaths`. `runConfigOperations` prunes inactive `gateway.auth.token`/`password` from the cloned config when switching auth modes, but it never appends those paths to `unsetPaths`; the writer will restore them from the source snapshot, so `openclaw config set gateway.auth.mode trusted-proxy` can leave `gateway.auth.token` on disk and fail startup despite logging that it was removed.
- confidence: 0.91
## Security Review
Status: cleared
Status: needs_attention
Summary: The diff only changes config write logic and tests; it adds no dependencies, workflows, install scripts, publishing metadata, or new secret-handling surface.
Summary: The diff introduces a concrete auth-config persistence regression: inactive gateway auth secrets can remain on disk after the CLI reports that they were removed.
Concerns:
- none
- **[medium] Inactive gateway auth secrets can persist:** `src/config/io.write-prepare.ts:31`
- body: The new merge-patch preservation contract ignores absent keys unless they are included in `unsetPaths`, but gateway auth mode cleanup deletes old token/password fields without forwarding those paths. This can preserve stale shared auth credentials and, for trusted-proxy mode, trigger the existing startup rejection for a configured token.
- confidence: 0.89
## Real Behavior Proof
Status: insufficient
Status: missing
Evidence kind: terminal
Evidence kind: none
Needs contributor action: true
Summary: The PR body has terminal-style expected output and test results, but not a distinct after-fix real docker/restart log; add redacted terminal output, screenshot, recording, or logs, then update the PR body or ask for `@clawsweeper re-review`.
Summary: The PR body contains expected output and test claims, but no distinct after-fix real docker/restart or equivalent CLI log; the latest `Real behavior proof` check is failing.
## Work Candidate
@ -160,91 +162,81 @@ Candidate: manual_review
Confidence: high
Priority: medium
Priority: high
Status: manual_review
Reason: Manual/contributor action is required because the head has a narrow typecheck blocker and the missing real behavior proof cannot be supplied by an automated repair worker.
Reason: Contributor action is required to supply real behavior proof, and maintainer review should confirm the auth cleanup regression is fixed before merge.
Cluster refs:
- https://github.com/openclaw/openclaw/pull/79549
- https://github.com/openclaw/openclaw/issues/55219
- none
Likely files:
- src/config/io.write-config.test.ts
- src/config/io.delivery-mode-persistence.test.ts
- src/config/io.write-prepare.ts
- src/config/io.ts
- none
Validation:
- pnpm check:test-types
- pnpm test src/config/io.delivery-mode-persistence.test.ts
- pnpm test src/config/io.write-config.test.ts src/config/io.write-prepare.test.ts
- node scripts/run-oxlint.mjs src/config/io.write-prepare.ts src/config/io.ts
- none
## Evidence
- **Current main emits deletes for absent target keys:** On current `origin/main`, `createMergePatch` still writes `patch[key] = null` when a key is present in the runtime/base object but absent from the target object.
- file: [src/config/io.write-prepare.ts:27](https://github.com/openclaw/openclaw/blob/3256316122f8f04867df0bd21fb948d3896ecee9/src/config/io.write-prepare.ts#L27)
- command: `git show origin/main:src/config/io.write-prepare.ts | nl -ba | sed -n '1,360p'`
- sha: [3256316122f8](https://github.com/openclaw/openclaw/commit/3256316122f8f04867df0bd21fb948d3896ecee9)
- **Null merge-patch entries delete config fields:** `applyMergePatch` treats `null` values as delete markers, so the current patch generation can remove omitted source fields during partial writes.
- file: [src/config/merge-patch.ts:77](https://github.com/openclaw/openclaw/blob/3256316122f8f04867df0bd21fb948d3896ecee9/src/config/merge-patch.ts#L77)
- command: `git show origin/main:src/config/merge-patch.ts | nl -ba | sed -n '1,130p'`
- sha: [3256316122f8](https://github.com/openclaw/openclaw/commit/3256316122f8f04867df0bd21fb948d3896ecee9)
- **Current write path applies that patch to persisted source config:** Both the internal writer and runtime-snapshot wrapper route writes through `createMergePatch` and `applyMergePatch`, including the `hadBothSnapshots` branch.
- file: [src/config/io.ts:2417](https://github.com/openclaw/openclaw/blob/3256316122f8f04867df0bd21fb948d3896ecee9/src/config/io.ts#L2417)
- command: `git show origin/main:src/config/io.ts | nl -ba | sed -n '1981,2030p'; git show origin/main:src/config/io.ts | nl -ba | sed -n '2405,2427p'`
- sha: [3256316122f8](https://github.com/openclaw/openclaw/commit/3256316122f8f04867df0bd21fb948d3896ecee9)
- **PR changes the preservation/deletion contract:** The PR skips absent-in-target keys in `createMergePatch`, exports `deletePathValue`, and applies explicit `unsetPaths` inside `resolvePersistCandidateForWrite`.
- file: [src/config/io.write-prepare.ts:27](https://github.com/openclaw/openclaw/blob/3eb489cc66c274b7719143e2496135f55f22e636/src/config/io.write-prepare.ts#L27)
- command: `curl -L --silent --show-error https://raw.githubusercontent.com/openclaw/openclaw/3eb489cc66c274b7719143e2496135f55f22e636/src/config/io.write-prepare.ts | nl -ba | sed -n '17,325p'`
- sha: [3eb489cc66c2](https://github.com/openclaw/openclaw/commit/3eb489cc66c274b7719143e2496135f55f22e636)
- **PR adds targeted regression coverage:** The new test file covers the `acp.stream.deliveryMode` loss path for merge-patch primitives and a full file round trip after a simulated restart; `io.write-config.test.ts` also covers explicit `unsetPaths` in the `hadBothSnapshots` branch.
- file: [src/config/io.delivery-mode-persistence.test.ts:122](https://github.com/openclaw/openclaw/blob/3eb489cc66c274b7719143e2496135f55f22e636/src/config/io.delivery-mode-persistence.test.ts#L122)
- command: `curl -L --silent --show-error https://raw.githubusercontent.com/openclaw/openclaw/3eb489cc66c274b7719143e2496135f55f22e636/src/config/io.delivery-mode-persistence.test.ts | nl -ba | sed -n '50,260p'`
- sha: [3eb489cc66c2](https://github.com/openclaw/openclaw/commit/3eb489cc66c274b7719143e2496135f55f22e636)
- **CI typecheck failure is introduced in the PR test:** The `check-test-types` annotation reports `Property 'discord' does not exist on type 'ChannelsConfig | undefined'` at the added casts on lines 1484 and 1495.
- file: [src/config/io.write-config.test.ts:1484](https://github.com/openclaw/openclaw/blob/3eb489cc66c274b7719143e2496135f55f22e636/src/config/io.write-config.test.ts#L1484)
- command: `curl -L --silent --show-error 'https://api.github.com/repos/openclaw/openclaw/check-runs/75105857630/annotations?per_page=100' | sed -n '1,260p'`
- sha: [3eb489cc66c2](https://github.com/openclaw/openclaw/commit/3eb489cc66c274b7719143e2496135f55f22e636)
- **Real behavior proof is not yet sufficient:** The PR has the `triage: needs-real-behavior-proof` label and the `Real behavior proof` check failed; the body includes terminal-style expected before/after snippets but not a distinct captured after-fix docker restart run.
- command: `curl -L --silent --show-error https://api.github.com/repos/openclaw/openclaw/pulls/79549; curl -L --silent --show-error 'https://api.github.com/repos/openclaw/openclaw/commits/3eb489cc66c274b7719143e2496135f55f22e636/check-runs?per_page=100&page=2' | jq -r '.check_runs[] | [.name,.status,.conclusion] | @tsv'`
- sha: [3eb489cc66c2](https://github.com/openclaw/openclaw/commit/3eb489cc66c274b7719143e2496135f55f22e636)
- **Related historical report:** The earlier broad config-clobbering report at https://github.com/openclaw/openclaw/issues/55219 was closed as implemented for runtime-default source preservation, but this PR covers a narrower remaining absent-key partial-write path.
- command: `curl -L --silent --show-error https://api.github.com/repos/openclaw/openclaw/issues/55219; curl -L --silent --show-error https://api.github.com/repos/openclaw/openclaw/issues/55219/comments?per_page=20`
- **Current main emits delete markers for omitted keys:** `createMergePatch` still sets `patch[key] = null` when a base key is absent from the target object, which can delete unrelated persisted config during partial writes.
- file: [src/config/io.write-prepare.ts:27](https://github.com/openclaw/openclaw/blob/5d87f7b62d370d9feeb4315c0526f9f843589f79/src/config/io.write-prepare.ts#L27)
- command: `nl -ba src/config/io.write-prepare.ts | sed -n '1,360p'`
- sha: [5d87f7b62d37](https://github.com/openclaw/openclaw/commit/5d87f7b62d370d9feeb4315c0526f9f843589f79)
- **Current main applies null merge patches as deletion:** `applyMergePatch` deletes object properties when the patch value is `null`, confirming the write-path root cause from source inspection.
- file: [src/config/merge-patch.ts:77](https://github.com/openclaw/openclaw/blob/5d87f7b62d370d9feeb4315c0526f9f843589f79/src/config/merge-patch.ts#L77)
- command: `nl -ba src/config/merge-patch.ts | sed -n '1,140p'`
- sha: [5d87f7b62d37](https://github.com/openclaw/openclaw/commit/5d87f7b62d370d9feeb4315c0526f9f843589f79)
- **Current main routes config writes through that merge patch:** `writeConfigFile` builds a patch from the runtime snapshot and applies it to the runtime source snapshot when both snapshots exist.
- file: [src/config/io.ts:2417](https://github.com/openclaw/openclaw/blob/5d87f7b62d370d9feeb4315c0526f9f843589f79/src/config/io.ts#L2417)
- command: `nl -ba src/config/io.ts | sed -n '2320,2465p'`
- sha: [5d87f7b62d37](https://github.com/openclaw/openclaw/commit/5d87f7b62d370d9feeb4315c0526f9f843589f79)
- **PR flips absent-key semantics:** At PR head, absent target keys are skipped in `createMergePatch`, so callers must pass `unsetPaths` for intentional deletion.
- file: [src/config/io.write-prepare.ts:31](https://github.com/openclaw/openclaw/blob/7035cd5b098a06e02f13ff3fee4420bdf074235a/src/config/io.write-prepare.ts#L31)
- command: `curl -fsSL https://api.github.com/repos/openclaw/openclaw/contents/src/config/io.write-prepare.ts?ref=7035cd5b098a06e02f13ff3fee4420bdf074235a | jq -r '.content' | base64 -d | nl -ba | sed -n '17,34p'`
- sha: [7035cd5b098a](https://github.com/openclaw/openclaw/commit/7035cd5b098a06e02f13ff3fee4420bdf074235a)
- **Implicit gateway auth cleanup is not forwarded as unsetPaths:** `pruneInactiveGatewayAuthCredentials` deletes `gateway.auth.token` or `gateway.auth.password` from the cloned config and only returns display strings; `runConfigOperations` writes only explicit operation deletes in `unsetPaths`, so these implicit deletions are lost under the PR's new merge-patch contract.
- file: [src/cli/config-cli.ts:1417](https://github.com/openclaw/openclaw/blob/5d87f7b62d370d9feeb4315c0526f9f843589f79/src/cli/config-cli.ts#L1417)
- command: `nl -ba src/cli/config-cli.ts | sed -n '1360,1725p'`
- sha: [5d87f7b62d37](https://github.com/openclaw/openclaw/commit/5d87f7b62d370d9feeb4315c0526f9f843589f79)
- **Trusted-proxy mode rejects a preserved token:** Gateway startup rejects `trusted-proxy` auth when a shared token remains configured, so preserving a token that the CLI said it removed can break the auth mode switch after restart.
- file: [src/gateway/auth.ts:256](https://github.com/openclaw/openclaw/blob/5d87f7b62d370d9feeb4315c0526f9f843589f79/src/gateway/auth.ts#L256)
- command: `nl -ba src/gateway/auth.ts | sed -n '220,270p'`
- sha: [5d87f7b62d37](https://github.com/openclaw/openclaw/commit/5d87f7b62d370d9feeb4315c0526f9f843589f79)
- **CI shows the non-proof merge gate is still failing:** The latest head has 77 successful checks and one failing check named `Real behavior proof`; `check-test-types` is now successful after the contributor's follow-up commit.
- command: `curl -fsSL 'https://api.github.com/repos/openclaw/openclaw/commits/7035cd5b098a06e02f13ff3fee4420bdf074235a/check-runs?per_page=100' | jq -r '.check_runs[] | select((.conclusion != "success" and .conclusion != "skipped") or .status != "completed") | [.name,.status,(.conclusion//""),.html_url] | @tsv'`
- sha: [7035cd5b098a](https://github.com/openclaw/openclaw/commit/7035cd5b098a06e02f13ff3fee4420bdf074235a)
## Likely Related People
- **steipete:** recent maintainer and config write owner
- reason: GitHub path history shows recent `src/config/io.write-prepare.ts` config-write fixes by this maintainer, and local blame in the current filtered checkout points the central helper/writer lines at the same maintainer-owned history.
- reason: Recent GitHub path history shows multiple config write, patch, and preservation fixes in the central config files touched by this PR.
- confidence: high
- commits: c238a51f59d5622e6ac1e77b588acdd77810b6b4, 579334f9f8a02d264c2a4d421ab94e37fb8b6244, aa78d9eab9546716b4811e1b8f6f4344eae71b68
- files: src/config/io.write-prepare.ts, src/config/io.ts
- commits: c238a51f59d5, 579334f9f8a0, f70a46b7031f, df51878b0b47, 8b8bba962160, 48a01798b036
- files: src/config/io.write-prepare.ts, src/config/io.ts, src/cli/config-cli.ts
- **shakkernerd:** adjacent unset-path and config migration maintainer
- reason: Recent path history includes shipped plugin install migration and managed unset work adjacent to this PR's explicit deletion contract.
- confidence: medium
- commits: 194c26bcd2f1, b018272fa1ca, df7348e58656
- files: src/config/io.write-prepare.ts, src/config/io.ts, src/cli/config-cli.ts
- **joshp123:** adjacent config persistence maintainer
- reason: Recent merged config work changed automatic writes and Nix mutator guards in `src/config/io.ts`, adjacent to the persistence path touched here.
- reason: Recent merged config work changed automatic writes and Nix mutator guards in `src/config/io.ts`, adjacent to the persistence path changed here.
- confidence: medium
- commits: d4b46600269c6c5e916472630e980789dadbbf4b
- commits: d4b46600269c
- files: src/config/io.ts, src/config/mutate.ts
- **shakkernerd:** adjacent unset-path/config migration maintainer
- reason: Recent path history for `src/config/io.write-prepare.ts` includes shipped plugin-install migration work involving managed config unsets, adjacent to this PR's `unsetPaths` contract.
- **vincentkoc:** recent config CLI maintainer
- reason: Recent config CLI history includes unset behavior for array paths and related config recovery work, which overlaps the deletion side of this PR.
- confidence: medium
- commits: 194c26bcd2f1282f62f4e801a05ef2a1904d5211, b018272fa1caa392c4ca9ab71fb4e454d703346d
- files: src/config/io.write-prepare.ts, src/config/io.ts
- **Vignesh Natarajan:** unset-path behavior contributor
- reason: Local history shows prior CLI/config work to keep explicitly unset keys removed, which is directly related to the deletion side of this PR's contract.
- confidence: medium
- commits: 73b4330d4c5023d358a0be19cedf06f236fbb31c
- files: src/config/io.write-prepare.ts, src/cli/config-cli.ts
- commits: fb73f2161e01, 3c5169254362
- files: src/cli/config-cli.ts, src/config/io.ts
## Risks / Open Questions
- `check-test-types` currently fails on the added `OpenClawConfig["channels"]["discord"]` casts, so the head is not mergeable as-is.
- The contributor still needs redacted after-fix real behavior proof for the actual config persistence/restart path, not only unit results or expected-output comments.
- This changes a primitive config merge helper, so maintainers should wait for the full touched config/changed CI surface to pass after the type fix.
- Gateway auth mode changes that rely on implicit credential cleanup can silently keep stale `gateway.auth.token` or `gateway.auth.password` on disk under the PR's new preservation semantics.
- The contributor still needs redacted after-fix real behavior proof for the actual config persistence and restart path.
## Close Comment
@ -252,19 +244,19 @@ _No close comment posted._
## GitHub Snapshot
- comments: 0
- timeline events: 5
- comments: 2
- timeline events: 8
- related items: 2
- PR files: 5
- PR commits: 3
- PR commits: 4
## Review Telemetry
- prompt chars: 71643
- prompt chars: 79970
- static prompt chars: 33412
- context chars: 36968
- context chars: 45295
- schema chars: 14081
- additional prompt chars: 0
- context collection ms: 3761
- Codex review ms: 320872
- context collection ms: 3973
- Codex review ms: 213884