fix: isOwnedByAgent derived ownership + dual-track registration rollback (#448)#688
fix: isOwnedByAgent derived ownership + dual-track registration rollback (#448)#688jlin53882 wants to merge 1 commit 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 |
jlin53882
force-pushed
the
fix/issue-448-dual-track
branch
from
516b520 to
f80525b
Compare
jlin53882
force-pushed
the
fix/issue-448-dual-track
branch
2 times, most recently
from
e6429a5 to
577c76d
Compare
…upport Dual-track system alongside existing WeakSet guard: - _registeredApisMap (Map<Api, boolean>): explicit claim before init, rollback on failure - _getRegisteredApisForTest(): export Map for test inspection - register(): claim Map entry + WeakSet BEFORE _initPluginState() - register() catch: delete Map entry on init failure (rollback) - resetRegistration(): clear both WeakSet (new instance) and Map (clear()) Phase 2 singleton guard preserved (WeakSet + singleton guard unchanged). Closes CortexReach#448
jlin53882
force-pushed
the
fix/issue-448-dual-track
branch
from
577c76d to
81b99c7
Compare
|
⏸ 等 PR #522 合併後再處理本 PR。兩PR改同一檔（reflection-store.ts），需依序合併避免conflict。 |
2 participants
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