Skip to content

fix: preserve plugin hook and tool closure state#78075

Open
bryce-d-greybeard wants to merge 1 commit into
openclaw:mainfrom
bryce-d-greybeard:bryce/issue-77822-plugin-tool-closures
Open

fix: preserve plugin hook and tool closure state#78075
bryce-d-greybeard wants to merge 1 commit into
openclaw:mainfrom
bryce-d-greybeard:bryce/issue-77822-plugin-tool-closures

Conversation

@bryce-d-greybeard

@bryce-d-greybeard bryce-d-greybeard commented May 5, 2026

Copy link
Copy Markdown
Contributor

Summary

Fixes plugin tool resolution so cached descriptor wrappers do not bypass an already-live plugin registry that owns both hooks and tools for the same plugin. That keeps hook handlers and tool factories on the same register() closure when a plugin uses closure state across before_prompt_build and a tool like memory_add.

Changes

  • Detect active/pinned-channel registries that already contain a plugin's tools and hooks.
  • Skip cached descriptor wrappers for those stateful hook+tool plugins, forcing the normal live-registry materialization path.
  • Preserve descriptor caching for plain tool-only plugins and cold/no-live-registry paths.
  • Add regression coverage for a warmed descriptor cache being bypassed when a live hook+tool registry is available.

Testing

  • PATH="/tmp/openclaw-pnpm-shim:$PATH" pnpm vitest run src/plugins/tools.optional.test.ts — passed, 59 tests.
  • git diff --check — passed.
  • PATH="/tmp/openclaw-pnpm-shim:$PATH" node scripts/check-changed.mjs — all gates passed through final pairing-account guard; process was SIGKILLed at final exit on this host. Standalone PATH="/tmp/openclaw-pnpm-shim:$PATH" node scripts/check-pairing-account-scope.mjs passed.

Fixes #77822

@openclaw-barnacle openclaw-barnacle Bot added triage: needs-real-behavior-proof Candidate: external PR needs after-fix proof from a real setup. size: S labels May 5, 2026
@clawsweeper

clawsweeper Bot commented May 5, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Thanks for the context here. I swept through the related work, and this is now duplicate or superseded.

Keep open: the linked issue is still open and current main has not implemented a complete closure-state fix. This branch is a useful candidate, but it needs a rebase, real behavior proof, and live-registry-source alignment before merge.

Root-cause cluster
Relationship: fixed_by_candidate
Canonical: #77822
Summary: This PR is the current candidate fix for the canonical mixed hook/tool plugin closure-state split; related mem0 symptoms and registry-retention work overlap but do not replace the canonical issue.

Members:

Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything.

Canonical path: Close this stale PR. The latest review rated it F, the branch still lacks merge-ready proof, and there has been no human follow-up after the durable review.

So I’m closing this here because the remaining work is already tracked in the canonical issue.

Review details

Best possible solution:

Close this stale PR. The latest review rated it F, the branch still lacks merge-ready proof, and there has been no human follow-up after the durable review.

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

Yes, at source level: current main can return cached descriptor-backed tools before the live registry path whose hooks own closure state. I did not run a live mem0 gateway reproduction in this read-only review.

Is this the best way to solve the issue?

No as written. The fix is plausible but too narrow because cached-tool bypass should align with the same live registry sources used by hook dispatch, not only active and channel registries.

Security review:

Security review cleared: The diff is limited to plugin resolver logic and a colocated test, with no dependency, workflow, secret, permission, package, lockfile, or supply-chain surface changed.

AGENTS.md: found and applied where relevant.

What I checked:

  • stale F-rated PR: PR was opened 2026-05-05T21:25:09Z, is older than 60 days, and the latest review rated it F.
  • proof blocker: real behavior proof is mock_only and proof tier is F, so this branch is not merge-ready without contributor follow-up.
  • no human follow-up: live comments and timeline hydrated by apply contain no non-automation activity after the ClawSweeper review.

Likely related people:

  • steipete: PR 76079 introduced the cached plugin tool descriptor resolver surface that this PR changes. (role: descriptor-cache contributor and merger; confidence: high; commits: a9d93daf0656; files: src/plugins/tools.ts, src/plugins/tool-descriptor-cache.ts, src/plugins/tools.optional.test.ts)
  • shakkernerd: The descriptor-cache commit credits this handle as co-author on the implicated resolver path. (role: descriptor-cache co-author; confidence: medium; commits: a9d93daf0656; files: src/plugins/tools.ts, src/plugins/tool-descriptor-cache.ts, src/plugins/tools.optional.test.ts)
  • zanni098: PR 78716 changed cached descriptor-backed tool dispatch in the same resolver and test area. (role: adjacent cached-wrapper contributor; confidence: medium; commits: 3adbbe7c34b5; files: src/plugins/tools.ts, src/plugins/tools.optional.test.ts)
  • obviyus: PR 78716 was merged by this handle and includes a simplification commit in the cached descriptor dispatch path. (role: adjacent reviewer/merger; confidence: medium; commits: 3adbbe7c34b5, e40ce4f34dca; files: src/plugins/tools.ts, src/plugins/tools.optional.test.ts)
  • amknight: PR 92016 added the composed live hook registry source set that a complete closure-state fix should share or mirror. (role: live hook registry contributor; confidence: medium; commits: 23eadfa27783; files: src/plugins/runtime.ts, src/plugins/hook-runner-global-state.ts, src/plugins/hook-runner-global.ts)

Codex review notes: model internal, reasoning high; reviewed against 010b61746379.

This was referenced May 10, 2026
Closed
Open
@RomneyDa

RomneyDa commented May 29, 2026

Copy link
Copy Markdown
Member

Heads up: this PR needs to be updated against current main before the new required Dependency Guard check can pass.

@clawsweeper clawsweeper Bot added rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. P2 Normal backlog priority with limited blast radius. merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. merge-risk: 🚨 session-state 🚨 May lose, corrupt, stale, or mis-associate session, agent, or context state. labels Jun 14, 2026
@clawsweeper clawsweeper Bot mentioned this pull request Jun 20, 2026
Merged
25 tasks
@xuwei-xy

xuwei-xy commented Jun 21, 2026

Copy link
Copy Markdown

The changes look good overall. Consider adding a test case to cover this scenario if possible.

@Last-emo-boy Last-emo-boy mentioned this pull request Jul 15, 2026
Merged
@openclaw-barnacle

openclaw-barnacle Bot commented Jul 20, 2026

Copy link
Copy Markdown

This pull request has been automatically marked as stale due to inactivity.
Please add updates or it will be closed.

@openclaw-barnacle openclaw-barnacle Bot added stale Marked as stale due to inactivity triage: needs-pr-context Candidate: external PR body lacks required problem context or evidence. labels Jul 20, 2026
@clawsweeper

clawsweeper Bot commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

@bryce-d-greybeard thanks for the PR. ClawSweeper is still waiting on real behavior proof before this can move forward.

Useful proof can be a screenshot, short video, terminal output, copied live output, linked artifact, or redacted logs that show the changed behavior after the fix. Please redact private tokens, phone numbers, private endpoints, customer data, and anything else sensitive.

Once proof is added to the PR body or a comment, ClawSweeper or a maintainer can re-check it.

@openclaw-barnacle openclaw-barnacle Bot removed the stale Marked as stale due to inactivity label Jul 21, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. merge-risk: 🚨 session-state 🚨 May lose, corrupt, stale, or mis-associate session, agent, or context state. P2 Normal backlog priority with limited blast radius. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. size: S status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. triage: needs-pr-context Candidate: external PR body lacks required problem context or evidence. triage: needs-real-behavior-proof Candidate: external PR needs after-fix proof from a real setup.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Plugin architecture: register() called multiple times causes closure variables not shared (affects @mem0/openclaw-mem0)

4 participants

@bryce-d-greybeard @RomneyDa @xuwei-xy @Patrick-Erichsen