Repository navigation
refactor(channels): reuse setup config patch helpers - #140761
Merged
Merged
Conversation
Contributor
|
🦞👀 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. |
Contributor
|
Codex review: needs maintainer review before merge. Reviewed September 7, 2026, 3:21 AM ET / 07:21 UTC. ClawSweeper reviewWhat this changesFeishu and Synology Chat setup reuse existing configuration helpers, add account-preservation tests, and reduce Synology’s assertion allowance to match removed casts. Merge readiness✅ Ready for maintainer review This remains useful, maintainer-requested consolidation. No introduced correctness defect was found, and the updated validation evidence addresses the earlier Synology import-stall concern. Priority: P3 Review scores
Verification
How this fits togetherChannel setup turns CLI or wizard input into OpenClaw configuration. These helpers place settings at the channel root or selected account while preserving credentials, sibling accounts, and enablement rules. flowchart TD
A[CLI or wizard input] --> B[Plugin setup adapter]
C[Existing channel settings] --> B
B --> D{Root or named account}
D --> E[Shared configuration helpers]
E --> F[Updated channel settings]
F --> G[Channel runtime]
Before mergeNone. Agent review detailsSecurityNone. PR surfaceSource -37, Tests +73, Other 0. Total +36 across 5 files. View PR surface stats
Review metricsNone. Technical reviewBest possible solution: Keep channel-specific account semantics in the plugins while delegating configuration merging to the existing shared helpers. Do we have a high-confidence way to reproduce the issue? Not applicable: this is a behavior-preserving refactor, and source inspection found no introduced failure. The supplied control runs no longer reproduce the earlier import timeout. Is this the best way to solve the issue? Yes: existing helpers cover the required behavior, and the adapters explicitly retain the plugin-specific enablement and account-key semantics without expanding the SDK. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning medium; reviewed against 2d58961f33fd. LabelsLabel changes:
Label justifications:
EvidenceWhat I checked:
Likely related people:
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
HistoryReview history (1 earlier review cycle)
|
Preserve Synology enabling and Feishu exact account keys through existing shared config helpers, without adding SDK exports. Proof attempted: node scripts/run-vitest.mjs extensions/synology-chat/src/channel.integration.test.ts The import hook times out at 180000ms; 31 config/Feishu tests pass. Draft only.
The setup consolidation removes two manual result casts. Shrink its assertion allowance from 5 to 3; no baseline grows. Matched main/candidate Synology controls and 54 focused setup/helper tests passed, and isolated autoreview is clean.
steipete
force-pushed
the
steipete/dedup-channel-setup-0906
branch
from
September 7, 2026 07:14
a981ce8 to
cffbad3
Compare
steipete
marked this pull request as ready for review
September 7, 2026 07:15
Contributor
Author
|
Merged via squash.
|
github-actions Bot
pushed a commit
to Desicool/openclaw
that referenced
this pull request
Sep 8, 2026
* refactor(channels): reuse setup config patch helpers Preserve Synology enabling and Feishu exact account keys through existing shared config helpers, without adding SDK exports. Proof attempted: node scripts/run-vitest.mjs extensions/synology-chat/src/channel.integration.test.ts The import hook times out at 180000ms; 31 config/Feishu tests pass. Draft only. * chore(config): shrink Synology setup assertion allowance The setup consolidation removes two manual result casts. Shrink its assertion allowance from 5 to 3; no baseline grows. Matched main/candidate Synology controls and 54 focused setup/helper tests passed, and isolated autoreview is clean.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What Problem This Solves
Synology Chat and Feishu repeat root/account configuration patching already owned by the shared setup helpers. Keeping separate mutation paths risks inconsistent field clearing, enabling and account placement.
Why This Change Was Made
Synology delegates to
patchScopedAccountConfigwhile preserving explicit enabling and clear-before-patch order. Its CLI/wizard callers already normalize account IDs. Feishu uses the shared root-enable helper and top-level section patching while preserving exact named-account keys, including the empty string, and leaving the channel-enabled flag unchanged for named-account updates.The only follow-up code-accounting change is the maintainer-authorized assertion baseline shrink for Synology setup, 5 → 3, reflecting two deleted casts. No baseline grows and no SDK export, configuration option or persistent-state contract changes. This is a maintainer-requested channel de-duplication.
User Impact
No intended behavior change. Root placement, named-account credentials, sibling accounts, field clearing and enable flags remain the same.
Evidence
Before changing the draft, a clean detached current-main checkout was created outside ancestor installations, with a frozen-lockfile install. The exact command was:
node scripts/run-vitest.mjs extensions/synology-chat/src/channel.integration.test.ts110a636dd16e48af3fa68acc11fe00215bb54e9689dad4f282c3a06dc5738df37e6fafb984b42db3The earlier 180000ms import-hook timeout did not reproduce in any control. No candidate-specific import failure was demonstrated. Its exact historical cause remains unproven; these results do not establish a pre-existing failure on current main. No test, timeout, skip or mock was changed during this attribution run. The final head differs from the tested candidate only by the assertion-baseline shrink. Timings vary with cache and host load and are not performance claims.
node scripts/run-vitest.mjs extensions/feishu/src/setup-surface.test.ts src/channels/plugins/setup-helpers src/channels/plugins/config-helpers: 54 tests passed.node scripts/check-changed.mjs --base 110a636dd16e48af3fa68acc11fe00215bb54e96: passed in the separate physical checkout, including extension production/test types, extension lint, script lint, zero unused exports and zero runtime import cycles.pnpm build: passed in 7m 19.2s in the separate physical checkout, including all 152 public SDK subpaths, 90 external plugins and native loads of 53 built modules.cffbad32c9be6ac48ae4df8e0f77217930f877e7.Known gap: validation uses synthetic configuration and webhook/SDK harnesses, with no live Synology or Feishu account and no external channel send. Local test runs have no hosted run URL.