fix: isOwnedByAgent derived ownership + dual-track registration rollback (#448)#688
fix: isOwnedByAgent derived ownership + dual-track registration rollback (#448)#688jlin53882 wants to merge 4 commits intoCortexReach:masterfrom
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b20ea9c64a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const owner = typeof metadata.agentId === "string" ? metadata.agentId.trim() : ""; | ||
| if (owner === "main") return []; |
There was a problem hiding this comment.
The new guard if (owner === "main") return []; drops every derived slice from legacy memory-reflection rows, even when agentId is main. In loadAgentReflectionSlicesFromEntries, those rows have already passed isOwnedByAgent, so this extra filter effectively erases main's own historical derived reflections and breaks backward compatibility for existing combined rows after upgrade.
Useful? React with 👍 / 👎.
| // - Accumulated session Maps being lost on re-registration | ||
| // ======================================================================== | ||
| if (!_singletonState) { _singletonState = _initPluginState(api); } | ||
| _registeredApisMap.set(api, true); // claim before init (early track) |
There was a problem hiding this comment.
Storing each api in _registeredApisMap on the success path creates a permanent strong reference for every distinct registration call; entries are only removed on init failure or manual resetRegistration(). Because register() is called repeatedly with new API instances over process lifetime, this map can grow unbounded and defeats the GC-safe behavior that _registeredApis (WeakSet) previously provided.
Useful? React with 👍 / 👎.
Update — CI Manifest + Test CoverageAll dual-track tests have been added to
42 tests total, all pass on upstream/master. Note on test strategy: these are mock-based unit tests (testing the logic in isolation, not actual Latest commit: |
CI 分析：失敗為 upstream 既有问题，與本 PR 無關觀察三個失敗 job 在本 PR 與 master HEAD 都失敗，模式完全一致：
原因CI annotation 只顯示 建議建議維護者確認 master 上 Analysis by AI assistant — 2026-04-25 |
1 participant
Summary
Fixes #448: derived ownership leak in
isOwnedByAgent+ dual-track registration system with rollback support.Changes
1. Bug Fix:
isOwnedByAgentderived ownership leak (src/reflection-store.ts)itemKind) continue usingowner === "main"fallback (相容)itemKind === "derived"rows: no main fallback, blank owner returnsfalse(防止洩漏)test/isOwnedByAgent.test.mjs2. Dual-track Registration System (
index.ts)_registeredApisMap: Map<OpenClawPluginApi, boolean>alongside existing WeakSet_getRegisteredApisForTest(): Map<...>export for test inspectionregister(): early claim Map entry before_initPluginState(), rollback on failureresetRegistration(): clears both WeakSet (new instance) and Map (clear())3. Unit Tests (31 tests, all passing)
test/register-idempotency.test.mjs: idempotency, singleton guard behaviortest/register-rollback.test.mjs: init failure → Map rollback + WeakSet guardtest/register-dual-track-sync.test.mjs: WeakSet/Map synchronizationtest/reset-registration.test.mjs: reset clears both tracks4. Changelog (
CHANGELOG-v1.1.0.md)Verification
Architecture Decision
Option B (dual-track): Upstream Phase 2 singleton preserved. Added Map for explicit rollback tracking and test inspection. WeakSet handles GC safety; Map provides manual clearability.
Closes #448