fix(plugins): serialize binding approval saves by Alix-007 · Pull Request #88945 · openclaw/openclaw

Alix-007 · 2026-06-01T06:01:43Z

Summary

What problem does this PR solve?

Serializes plugin-binding approval persistence so overlapping allow-always saves cannot write stale approval state after a newer save updates the in-memory cache.
Keeps the existing in-memory cache behavior immediate for callers, while queueing disk writes behind the previous approval save.
Adds a regression test that holds the first write open, starts a second approval save, and verifies the second write waits and the final file contains both scopes.

Why does this matter now?

#64065 remains open because Race C can lose persistent plugin-binding approvals on restart when concurrent approval saves race on ~/.openclaw/plugin-binding-approvals.json.

What is the intended outcome?

Concurrent persistent plugin-binding approvals finish with disk state matching the latest in-memory approval file.

What is intentionally out of scope?

Race A and Race B from #64065 are left unchanged because the issue review already classifies them as mitigated by existing behavior.

What does success look like?

Two overlapping allow-always approvals can both resolve successfully and the persisted approval file still contains the expected approvals after both writes complete.

What should reviewers focus on?

The saveApprovals queueing behavior and the out-of-order write regression coverage in conversation-binding.test.ts.

Linked context

Which issue does this close?

Closes #64065

Which issues, PRs, or discussions are related?

Related #81402

Was this requested by a maintainer or owner?

The ClawSweeper review on #64065 requested a focused Race C repair with regression coverage.

Real behavior proof (required for external PRs)

Behavior or issue addressed: overlapping plugin-binding allow-always approval saves should not leave plugin-binding-approvals.json missing a newer approval scope.
Real environment tested: local OpenClaw source checkout on Linux with real requestPluginConversationBinding / resolvePluginConversationBindingApproval modules, real session-binding adapters, and a real temporary HOME approval file.
Exact steps or command run after this patch: node --import tsx/esm --input-type=module script that registered Discord adapters for default and work, requested two plugin bindings, resolved both with allow-always via Promise.all, then read .openclaw/plugin-binding-approvals.json from the temporary HOME.
Evidence after fix (terminal capture):

[plugins/binding] plugin binding requested plugin=codex root=/plugins/codex-a channel=discord account=default conversation=proof-channel-1
[plugins/binding] plugin binding requested plugin=codex root=/plugins/codex-a channel=discord account=work conversation=proof-channel-2
[plugins/binding] plugin binding approved plugin=codex root=/plugins/codex-a decision=allow-always channel=discord account=default conversation=proof-channel-1
[plugins/binding] plugin binding approved plugin=codex root=/plugins/codex-a decision=allow-always channel=discord account=work conversation=proof-channel-2
{
  "results": [
    "approved",
    "approved"
  ],
  "approvalsPath": "/media/vdc/0668001470/tmp/openclaw-approval-proof-kiz6XW/.openclaw/plugin-binding-approvals.json",
  "approvals": [
    {
      "pluginRoot": "/plugins/codex-a",
      "channel": "discord",
      "accountId": "default"
    },
    {
      "pluginRoot": "/plugins/codex-a",
      "channel": "discord",
      "accountId": "work"
    }
  ],
  "approvalCount": 2
}

Observed result after fix: both approval resolutions returned approved, and the real approval file contained both default and work approval scopes after the overlapping save path completed.
What was not tested: the broad database-first runtime-state PR path in refactor: move runtime state to SQLite #81402 and the already-mitigated Race A/Race B surfaces.
Proof limitations or environment constraints: local crabbox failed its basic binary sanity check, so the remote-wrapper form of check:changed could not start from this machine; I ran the local changed-check child path directly as supplemental validation.
Before evidence (optional but encouraged): [Bug]: Three async interleaving races in cron, command-queue, and plugin-binding subsystems #64065 documents the pre-fix interleaving where a first stale writeJson can complete after a newer in-memory approval state.

Tests and validation

Which commands did you run?

node scripts/run-vitest.mjs src/plugins/conversation-binding.test.ts
pnpm exec oxfmt --check --threads=1 src/plugins/conversation-binding.ts src/plugins/conversation-binding.test.ts CHANGELOG.md
pnpm exec oxlint src/plugins/conversation-binding.ts src/plugins/conversation-binding.test.ts
git diff --check
OPENCLAW_CHECK_CHANGED_REMOTE_CHILD=1 OPENCLAW_CHANGED_LANES_RAW_SYNC=1 CI=1 PNPM_CONFIG_VERIFY_DEPS_BEFORE_RUN=false pnpm check:changed

What regression coverage was added or updated?

Added serializes overlapping always-allow approval writes, which blocks the first approval write, starts a second approval, verifies the second approval is still waiting, then confirms the persisted approval file contains both approval scopes after the queue drains.

What failed before this fix, if known?

Before this fix, competing saveApprovals calls could await independent writeJson calls, allowing an older file snapshot to persist after newer in-memory approval state was already visible.

If no test was added, why not?

N/A.

Risk checklist

Did user-visible behavior change? (Yes/No)

No.

Did config, environment, or migration behavior change? (Yes/No)

No.

Did security, auth, secrets, network, or tool execution behavior change? (Yes/No)

No.

What is the highest-risk area?

Persistent plugin approval saving now waits behind an earlier approval save, so a slow or failed earlier disk write can delay later approval persistence.

How is that risk mitigated?

Each caller still receives its own write error if its save fails, the chain catches prior failures before accepting later writes, and the cache remains updated synchronously before disk persistence just like before.

Current review state

What is the next action?

Maintainer review and CI.

What is still waiting on author, maintainer, CI, or external proof?

Nothing known from the author side.

Which bot or reviewer comments were addressed?

Addressed the ClawSweeper #64065 recommendation to land a narrow Race C repair with regression coverage.

clawsweeper · 2026-06-01T06:04:12Z

Codex review: needs maintainer review before merge. Reviewed June 14, 2026, 1:02 PM ET / 17:02 UTC.

Summary
The PR serializes plugin-binding approval persistence through a process-local save chain and adds regression coverage for overlapping allow-always approval saves.

PR surface: Source +9, Tests +118. Total +127 across 2 files.

Reproducibility: yes. at source level: current main updates the approvals cache and then awaits an unqueued JSON write from the allow-always approval path, and the linked issue supplies an interleaving reproduction. I did not run the repro in this read-only review.

Review metrics: 1 noteworthy metric.

Persistent approval save path: 1 shared save path serialized. This is the only runtime behavior change and it directly targets the state-loss path maintainers need to evaluate before merge.

Merge readiness
Overall: 🐚 platinum hermit
Proof: 🦞 diamond lobster
Patch quality: 🐚 platinum hermit
Result: ready for maintainer review.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Rank-up moves:

none.

Next step before merge

[P2] No repair lane is needed; the remaining action is maintainer review, normal CI gating, and landing if checks stay green.

Security
Cleared: The diff changes only the existing plugin approval persistence path and its test, with no new dependency, workflow, permission, secret, or broader execution surface.

Review details

Best possible solution:

Land this narrow shared save-chain fix for the shipped JSON approval file, while keeping the broader SQLite runtime-state migration in the separate database-first PR.

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

Yes at source level: current main updates the approvals cache and then awaits an unqueued JSON write from the allow-always approval path, and the linked issue supplies an interleaving reproduction. I did not run the repro in this read-only review.

Is this the best way to solve the issue?

Yes: the shared resolver is used by Discord, Telegram, and Slack approval callbacks, so serializing the common persistence path is narrower and cleaner than channel-specific fixes. The broader SQLite migration remains a separate refactor, not a prerequisite for this bug fix.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against 7e12a3326dd2.

Label changes

Label changes:

add proof: sufficient: Contributor real behavior proof is sufficient. The PR body includes after-fix terminal output from a local checkout using real binding modules and a temporary HOME approval file, showing both overlapping approvals persisted.

Label justifications:

P2: The PR addresses a bounded persistence race that can lose plugin-binding approvals after restart without being a broad runtime outage.
rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🦞 diamond lobster and patch quality is 🐚 platinum hermit.
status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (terminal): The PR body includes after-fix terminal output from a local checkout using real binding modules and a temporary HOME approval file, showing both overlapping approvals persisted.
proof: sufficient: Contributor real behavior proof is sufficient. The PR body includes after-fix terminal output from a local checkout using real binding modules and a temporary HOME approval file, showing both overlapping approvals persisted.

Evidence reviewed

PR surface:

Source +9, Tests +118. Total +127 across 2 files.

View PR surface stats

Area	Files	Added	Removed	Net
Source	1	13	4	+9
Tests	1	118	0	+118
Docs	0	0	0	0
Config	0	0	0	0
Generated	0	0	0	0
Other	0	0	0	0
Total	2	131	4	+127

What I checked:

Repository policy read: Root AGENTS.md and scoped src/plugins/AGENTS.md were read fully; their review-depth, plugin-boundary, lifecycle-cache, and persistence-state guidance shaped the review. (AGENTS.md:1, 7e12a3326dd2)
Current main race surface: Current main updates the in-memory approvals cache, marks it loaded, and then awaits an independent JSON write, so overlapping save snapshots can persist out of order. (src/plugins/conversation-binding.ts:377, 7e12a3326dd2)
Reachable approval path: The allow-always approval resolver awaits addPersistentApproval before binding, making the save race reachable from real plugin-binding approval callbacks. (src/plugins/conversation-binding.ts:882, 7e12a3326dd2)
Shared caller surface: Discord, Telegram, and Slack approval callbacks all call resolvePluginConversationBindingApproval, so fixing the shared plugin-binding runtime is the right owner boundary. (extensions/telegram/src/bot-handlers.runtime.ts:2265, 7e12a3326dd2)
PR implementation: At PR head, PluginBindingGlobalState owns approvalsSaveChain, initializes it on the global singleton, chains each write after the previous save, and resets it in the test helper. (src/plugins/conversation-binding.ts:123, 475abba8d482)
Regression coverage: The added test blocks the first write, starts a second allow-always approval, verifies the second write is queued, then confirms the persisted approval file contains both account scopes. (src/plugins/conversation-binding.test.ts:526, 475abba8d482)

Likely related people:

steipete: GitHub path history shows this person introduced the conversation-binding hook surface and repeatedly maintained src/plugins/conversation-binding.ts and its tests, including related database-first runtime-state work. (role: feature owner and recent area contributor; confidence: high; commits: 5d0887574b44, f91de52f0d23, 27dde7a4d69b; files: src/plugins/conversation-binding.ts, src/plugins/conversation-binding.test.ts)
vincentkoc: GitHub path history and local blame show adjacent plugin/test cleanup and current-main validation touches near this surface, making this person a plausible secondary reviewer for plugin persistence behavior. (role: adjacent plugin-area contributor; confidence: medium; commits: 0131343db8c6, f3f42e6bbfd2, 37e3e895b0; files: src/plugins/conversation-binding.ts, src/plugins/conversation-binding.test.ts)

What the crustacean ranks mean

🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
🌊 off-meta tidepool: rating does not apply to this item.

Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

How this review workflow works

ClawSweeper keeps one durable marker-backed review comment per issue or PR.
Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
Maintainers can also comment @clawsweeper review to request a fresh review only.
Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

Alix-007 · 2026-06-15T22:59:28Z

Thanks for merging this one too, @vincentkoc! Appreciate you landing the binding-approval serialization fix. 🙏

fix(plugins): serialize binding approval saves

114f98c

openclaw-barnacle Bot added size: S proof: supplied External PR includes structured after-fix real behavior proof. labels Jun 1, 2026

vincentkoc self-assigned this Jun 1, 2026

clawsweeper Bot mentioned this pull request Jun 5, 2026

[Bug]: Three async interleaving races in cron, command-queue, and plugin-binding subsystems #64065

Closed

Merge branch 'main' into issue-64065-approval-save-serialization

475abba

openclaw-barnacle Bot removed the proof: sufficient ClawSweeper judged the real behavior proof convincing. label Jun 14, 2026

clawsweeper Bot added the proof: sufficient ClawSweeper judged the real behavior proof convincing. label Jun 14, 2026

vincentkoc merged commit 6cd6e3f into openclaw:main Jun 15, 2026
173 checks passed

github-actions Bot mentioned this pull request Jun 15, 2026

📡 Upstream Digest — 2026-06-15 19:38 UTC curtismercier/openclaw-mods#1084

Open

clawsweeper Bot mentioned this pull request Jun 15, 2026

Feature request: Coalesce interleaved text blocks into single outbound message #15022

Open

github-actions Bot pushed a commit to Desicool/openclaw that referenced this pull request Jun 16, 2026

fix(plugins): serialize binding approval saves (openclaw#88945)

b44810a

crh-code pushed a commit to crh-code/openclaw that referenced this pull request Jun 18, 2026

fix(plugins): serialize binding approval saves (openclaw#88945)

115352c

clawsweeper Bot mentioned this pull request Jun 21, 2026

fix(plugins): guard binding resolved callbacks #89782

Closed

steipete mentioned this pull request Jul 1, 2026

refactor: move runtime state to SQLite #81402

Closed

badgerbees pushed a commit to badgerbees/openclaw that referenced this pull request Jul 8, 2026

fix(plugins): serialize binding approval saves (openclaw#88945)

e835d29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

fix(plugins): serialize binding approval saves#88945

fix(plugins): serialize binding approval saves#88945
vincentkoc merged 2 commits into
openclaw:mainfrom
Alix-007:issue-64065-approval-save-serialization

Alix-007 commented Jun 1, 2026

Uh oh!

clawsweeper Bot commented Jun 1, 2026 •

edited

Loading

Uh oh!

Uh oh!

Alix-007 commented Jun 15, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Uh oh!

Conversation

Alix-007 commented Jun 1, 2026

Summary

Linked context

Real behavior proof (required for external PRs)

Tests and validation

Risk checklist

Current review state

Uh oh!

clawsweeper Bot commented Jun 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Alix-007 commented Jun 15, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

clawsweeper Bot commented Jun 1, 2026 •

edited

Loading