Repository navigation
refactor(plugins): separate conversation binding approval state - #139984
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 6, 2026 09:32
Contributor
|
Codex review: needs maintainer review before merge. Reviewed September 6, 2026, 5:51 AM ET / 09:51 UTC. ClawSweeper reviewWhat this changesMoves conversation-binding approval storage and shared cache state into an internal module and simplifies lifecycle logging without changing user-visible behavior. Merge readiness✅ Ready for maintainer review This remains a useful, bounded cleanup with no actionable correctness or security findings. The extraction is absent from fetched main and the latest release; the previous review’s empty finding list remains appropriate. Priority: P3 Review scores
Verification
How this fits togetherConversation bindings let approved plugins handle subsequent messages in a channel conversation. This subsystem checks durable approvals scoped to plugin root, channel, and account, then either requests approval or creates the binding. flowchart TD
A[Plugin binding request] --> B[Check existing binding]
B --> C[Approval store and cache]
C --> D{Approval available?}
D -->|Yes| E[Create conversation binding]
D -->|No| F[Ask user for approval]
F -->|Always allow| C
E --> G[Route subsequent messages]
Before mergeNone. Agent review detailsSecurityNone. PR surfaceSource -25. Total -25 across 2 files. View PR surface stats
Review metricsNone. Technical reviewBest possible solution: Keep approval persistence and cache lifecycle together while preserving the existing binding entrypoints, stored representation, and authorization scope. Do we have a high-confidence way to reproduce the issue? Not applicable: this PR reorganizes existing code rather than reporting a runtime defect; source inspection found no introduced failing path. Is this the best way to solve the issue? Yes: extracting the existing state owner is a narrow way to reduce module complexity without introducing parallel persistence or changing the public API. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning medium; reviewed against 2dbbb33887ae. 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)
|
Contributor
Author
|
Merged via squash.
|
github-actions Bot
pushed a commit
to Desicool/openclaw
that referenced
this pull request
Sep 7, 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
What Problem This Solves
Plugin conversation binding combines durable approval state, pending request lifetime, and routing in a grandfathered oversized module. This is the maintainer-requested deletion and suppression-debt follow-up to the #135868 split campaign.
Why This Change Was Made
Separate the existing approval store/cache and shared outage-notice state into one lifecycle state module. Remove redundant approval-scope projections, pass typed identity/conversation objects to the private lifecycle logger, and inline its single-use URI encoder wrapper.
The SQLite queries, transaction boundaries, cache representation, global singleton key, and persist-before-cache-publication ordering are unchanged. This is the first of two small splits; the pending-request lifetime owner still needs extraction before the original module can lose its max-lines suppression.
User Impact
No behavior or public API changes. Production shrinks by 25 lines (164 added / 189 removed); tests and tooling are unchanged. Relocated code is included in both sides of that count. Legacy session-key migration remains because it is present in shipped v2026.9.2; history tracks the recognition through e357203.
Evidence
Existing boundary tests protect root/channel/account approval scope, failed SQLite writes before cache publication, overlapping grants, rows outside the cache, duplicate module instances, and lifecycle drains. No tests or assertions were removed.
Validation: 38 focused tests passed across conversation-binding, its SQLite boundary suite, and global-singleton lifecycle tests. Full
node scripts/check-changed.mjsexited 0, including core/core-test typechecks, lint, storage guards, all three dead-export scans, and zero runtime import cycles. Independent Codex review against the pinned merge base: scoped-clean through P2.git diff --checkpassed.ClawSweeper reviewed exact head
f0adcd63b2aba7e74067e5afe6ead1dc9c867b7c: no actionable correctness/security findings and no before-merge or rank-up requests.