Repository navigation
refactor(plugins): remove the redundant config policy relay - #140904
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. |
steipete
marked this pull request as ready for review
September 7, 2026 06:07
Contributor
|
Codex review: needs maintainer review before merge. Reviewed September 7, 2026, 2:26 AM ET / 06:26 UTC. ClawSweeper reviewWhat this changesThe PR replaces a forwarding plugin-configuration function with a direct re-export, simplifies type imports, and makes its unused default-resolver export private. Merge readiness✅ Ready for maintainer review This remains a useful, bounded cleanup: current main retains the redundant wrapper, and the introduced changes preserve its callable contract. No actionable correctness or security findings remain. Priority: P3 Review scores
Verification
How this fits togetherPlugin configuration normalization converts operator settings into a consistent shape used by manifest discovery, hooks, skills, and agent settings. The policy facade provides this normalization without activating plugin runtime code. flowchart LR
A[Plugin configuration] --> C[Shared configuration normalizer]
B[Optional plugin ID resolver] --> C
C --> D[Policy facade re-export]
D --> E[Manifest discovery]
D --> F[Hook and skill discovery]
D --> G[Agent settings]
Before mergeNone. Agent review detailsSecurityNone. PR surfaceSource -12. Total -12 across 2 files. View PR surface stats
Review metricsNone. Technical reviewBest possible solution: Keep one canonical normalizer behind the existing internal import name while preserving defaults, alias resolution, and activation semantics. Do we have a high-confidence way to reproduce the issue? Not applicable: this is behavior-preserving cleanup, and source inspection confirms the existing forwarding wrapper rather than a reported runtime defect. Is this the best way to solve the issue? Yes: a named re-export removes the redundant implementation while retaining the existing call signature and canonical default resolver. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning medium; reviewed against c78e6aea330f. LabelsLabel 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)
|
steipete
force-pushed
the
steipete/cleanup-plugin-policy-relay
branch
from
September 7, 2026 06:21
75d77b6 to
bf1b889
Compare
Contributor
Author
|
Merged via squash.
|
github-actions Bot
pushed a commit
to Desicool/openclaw
that referenced
this pull request
Sep 8, 2026
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.
Related: #135868, #140815
What Problem This Solves
The private plugin-policy facade repeats a normalizer signature and forwards every call directly to the shared implementation. Two additional aliases repeat imported types. This is the requested owner follow-up to #140815 in the maintainer's deletion campaign.
Why This Change Was Made
Replace the forwarding function with a named re-export of the same canonical normalizer and import the existing types directly. Make the shared default resolver private after the facade, its only external consumer, disappears. Both exported names, the normalizer's call signature/default resolver/results, and the activation-policy implementation stay intact. The four direct consumer families—manifest discovery, hook discovery, skill discovery, and agent settings—continue using the same facade.
The existing wrapper-shadowing guidance explicitly permits a pure re-export when the wrapper adds no behavior. Its earlier rename in
e74be5d41dbonly distinguished the core import; it introduced no policy requiring an additional wrapper. The separate registry timeout helper remains because it floors positive values and has a different accepted domain.User Impact
No user-visible behavior change. Plugin configuration, activation policy, and discovery retain their existing semantics with less duplicate code. No configuration option, schema, storage behavior, dependency, or authorization change.
Evidence
All 245 existing tests across eight suites passed before (65.64s) and after the complete two-file cleanup (39.25s), covering config policy/state, manifest registry, hooks, skills, agent settings, bundle settings, and cached MCP settings.
The first full check passed core and test types, then both Knip scans correctly flagged the now-unused
identityNormalizePluginIdexport. The completed patch makes that existing helper private; it keeps all internal uses and retains the separately consumedNormalizePluginIdtype.The complete selected
check:changedplan passed in 603.94s, including core/test types, all three zero-entry Knip scans, lint, zero runtime import cycles, and architecture/storage guards. All 17 recorded source/test/dependency input hashes remained unchanged.Full
pnpm buildpassed in 544.62s, including public SDK declarations/exports, 53 native control-plane module loads, CLI bootstrap imports, and UI checks. Both full checks and build ran on pinned base038acb2695d298ac671c3b886647025abdbfd22c, verifying all 17 recorded input hashes and the complete changed-path set before and after each command.Independent review of the complete patch is scoped-clean through P2.
Refresh onto
64887802d73bb7f2d0ce499aef979a71807ac5fdwas clean; all 17 source/test/dependency input hashes remained byte-identical, including the lockfile. Exact-head CI supplies integration proof for refreshed main.Fresh author-open PR discovery and complete net diffs for all 16 truncated/missing file inventories found no competing change in this owner.
Initial exact-head CI passed. Final refresh onto
7ab1c97cb7fee1a5bc72da1cf913ea25873c3497is patch-identical. Main added the Team Reports workspace; the offline install lacked cached package metadata, then the normal frozen install passed its supply-chain check. All 15 source/test hashes stayed identical, all 245 tests passed again (83.45s), and final branch review is scoped-clean through P2.ClawSweeper review: no findings, security issues, or before-merge/rank-up actions. Its source/caller review confirms the preserved callable contract and absence of external/default-resolver SDK consumers.
The policy owner shrinks from 53 to 41 lines; the shared normalizer keeps the same line count. Existing behavior tests and all baselines remain intact.