Skip to content

fix(plugin): restore bug fixes from #681 and #688 lost during #662 merge#779

Merged
chenjw merged 1 commit intomainfrom
fix/restore-plugin-bugfixes-681-688
Mar 19, 2026
Merged

fix(plugin): restore bug fixes from #681 and #688 lost during #662 merge#779
chenjw merged 1 commit intomainfrom
fix/restore-plugin-bugfixes-681-688

Conversation

@qin-ctx
Copy link
Collaborator

@qin-ctx qin-ctx commented Mar 19, 2026

Description

PR #662 renamed the plugin directory from openclaw-memory-plugin to openclaw-plugin and rewrote index.ts, but was based on a stale branch that predated two important bug fixes (#681 and #688). This PR restores those lost fixes, adapted to the current context-engine code.

Related Issue

Restores fixes from:

Type of Change

  • Bug fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation update
  • Refactoring (no functional changes)
  • Performance improvement
  • Test update

Changes Made

Testing

  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • I have tested this on the following platforms:
    • Linux
    • macOS
    • Windows

Checklist

  • My code follows the project's coding style
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • Any dependent changes have been merged and published

Additional Notes

The PendingClientEntry type and localClientPendingPromises map were already present in client.ts (from #681), but unused — index.ts never imported them after #662's rewrite. This PR reconnects the wiring.

🤖 Generated with Claude Code

PR #662 renamed the plugin directory and rewrote index.ts from a stale
branch base, silently dropping two merged bug fixes:

- #681: share pending clientPromise across dual-context registrations
  to prevent before_agent_start hook from hanging forever
- #688: wrap auto-recall search in withTimeout(5s) to prevent indefinite
  agent hang when OpenViking search API is slow or unresponsive
@github-actions
Copy link

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis ✅

688 - PR Code Verified

Compliant requirements:

  • Add AUTO_RECALL_TIMEOUT_MS = 5_000 constant
  • Wrap entire auto-recall block in withTimeout
  • No changes to happy path

Requires further human verification:

  • Update catch block log message to mention "timed out" (the current catch block uses a generic message, but the timeout error will be included in the err string)

681 - PR Code Verified

Compliant requirements:

  • register() checks localClientPendingPromises first, shares existing promise
  • start() claims entry (deletes from map), only first caller spawns process
  • stop() clears map entry

Requires further human verification:

  • Verify localClientPendingPromises is properly exported/imported from client.ts (not shown in diff)
⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Missing Import?

Verify that withTimeout is imported in this file (used on line 461 but not shown in the diff). The original ticket #688 mentions it was already imported from process-manager.ts, but confirm it's present.

await withTimeout(
Variable Declaration Check

Verify that resolveLocalClient and rejectLocalClient are still declared in the outer scope (they're used in start() but their declaration isn't shown in the diff).

resolveLocalClient = pendingEntry!.resolve;
rejectLocalClient = pendingEntry!.reject;

@github-actions
Copy link

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
General
Combine consecutive isSpawner checks

Combine the two consecutive if (isSpawner) blocks into a single block to improve
code readability and avoid redundant checks. This does not change functionality but
makes the code cleaner.

examples/openclaw-plugin/index.ts [598-604]

 const isSpawner = cfg.mode === "local" && !!pendingEntry;
 if (isSpawner) {
   localClientPendingPromises.delete(localCacheKey);
   resolveLocalClient = pendingEntry!.resolve;
   rejectLocalClient = pendingEntry!.reject;
-}
-if (isSpawner) {
Suggestion importance[1-10]: 4

__

Why: The suggestion correctly identifies redundant consecutive if (isSpawner) checks and proposes combining them, which improves readability without changing functionality. This is a minor code style enhancement, so it gets a moderate score.

Low

@chenjw chenjw merged commit e908b6a into main Mar 19, 2026
6 checks passed
@chenjw chenjw deleted the fix/restore-plugin-bugfixes-681-688 branch March 19, 2026 12:34
@github-project-automation github-project-automation bot moved this from Backlog to Done in OpenViking project Mar 19, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

2 participants