feat(matrix): add subagent hooks for ACP room-binding#51523
Conversation
Add subagent_spawning, subagent_delivery_target, and subagent_ended hooks to the Matrix extension, enabling thread-bound ACP sessions (Claude Code, Codex) on Matrix — analogous to the existing Discord implementation. The Matrix extension already has the full infrastructure: - SessionBindingAdapter (thread-bindings.ts) - In-memory binding store (thread-bindings-shared.ts) - Inbound routing/preflight (monitor/route.ts) This commit adds the missing glue: three hook handlers that connect the ACP session lifecycle to the existing binding infrastructure.
Greptile SummaryThis PR ports the Discord subagent-hooks pattern to the Matrix extension, adding three hook handlers ( Issues found:
Confidence Score: 4/5
Prompt To Fix All With AIThis is a comment left during a code review.
Path: extensions/matrix/src/matrix/subagent-hooks.ts
Line: 115-117
Comment:
**Multi-account fallback only scans "default" account**
When `event.accountId` is absent, the fallback builds `allAccountIds` by calling `listBindingsForAccount("default")` and mapping `.accountId` — which always returns `["default"]` (or `[]`). Bindings for any named account (e.g. `"work"`, `"personal"`) will never be iterated, so their bindings won't be cleaned up on session end, leaving stale in-memory entries.
The comment on line 113 says *"Find and remove all bindings matching the ended subagent session"*, but the implementation only ever searches the `"default"` bucket.
A correct approach would either:
1. Use a store-level helper that returns every binding regardless of account (e.g. an exported `listAllBindings()`), or
2. Rely on the fact that `subagent_ended` always carries a non-empty `accountId` and remove the fallback entirely with a guard.
The test suite also misses this path — `it("unbinds room routing on subagent_ended")` always passes `accountId: "work"`, so the broken fallback branch is never exercised.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: extensions/matrix/src/matrix/subagent-hooks.ts
Line: 1-11
Comment:
**Unused imports**
`resolveBindingKey` and `toSessionBindingRecord` are imported from `./thread-bindings-shared.js` but never called in this file. They should be removed to avoid confusion and keep the import surface clean.
```suggestion
import type { OpenClawPluginApi } from "openclaw/plugin-sdk/core";
import { findMatrixAccountConfig, resolveMatrixBaseConfig } from "./account-config.js";
import {
getMatrixThreadBindingManager,
listBindingsForAccount,
removeBindingRecord,
setBindingRecord,
} from "./thread-bindings-shared.js";
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: extensions/matrix/src/matrix/subagent-hooks.ts
Line: 141-142
Comment:
**Misleading comment contradicts implementation**
The comment says *"Collect bindings across all accounts if no specific account is given"*, but the code immediately falls back to `"default"` — it does **not** collect across all accounts. The same disconnect exists in `subagent_ended` (line 113). Either update the comment to reflect the actual single-account fallback, or implement the described cross-account scan.
```suggestion
const accountId = requesterAccountId || "default";
```
How can I resolve this? If you propose a fix, please make it concise.Last reviewed commit: "feat(matrix): add su..." |
| return; | ||
| } |
There was a problem hiding this comment.
The comment says "Collect bindings across all accounts if no specific account is given", but the code immediately falls back to "default" — it does not collect across all accounts. The same disconnect exists in subagent_ended (line 113). Either update the comment to reflect the actual single-account fallback, or implement the described cross-account scan.
| return; | |
| } | |
| const accountId = requesterAccountId || "default"; |
Prompt To Fix With AI
This is a comment left during a code review.
Path: extensions/matrix/src/matrix/subagent-hooks.ts
Line: 141-142
Comment:
**Misleading comment contradicts implementation**
The comment says *"Collect bindings across all accounts if no specific account is given"*, but the code immediately falls back to `"default"` — it does **not** collect across all accounts. The same disconnect exists in `subagent_ended` (line 113). Either update the comment to reflect the actual single-account fallback, or implement the described cross-account scan.
```suggestion
const accountId = requesterAccountId || "default";
```
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2dd782222c
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| for (const binding of bindings) { | ||
| removeBindingRecord(binding); |
There was a problem hiding this comment.
Persist subagent cleanup instead of removing only in memory
The ended hook bypasses Matrix's normal unbind path and just drops the record from RAM. After any routed message, sessionBindingService.touch() can flush that binding to thread-bindings.json (extensions/matrix/src/matrix/monitor/route.ts:57-59), and the supported unbind flow persists removals before sending farewells (extensions/matrix/src/matrix/thread-bindings.ts:406-413). Here the file is never updated, so a restart reloads the dead binding and the room/thread starts routing back into a session that has already ended.
Useful? React with 👍 / 👎.
- P1: Multi-account fallback — use listAllBindings() instead of only scanning 'default' account in subagent_ended and delivery_target - P1: Persist on subagent_ended — call manager.persist() after removing binding records (mirrors unbind flow in thread-bindings.ts) - P2: Persist on spawn — call manager.persist() after setBindingRecord - P2: Remove unused imports (resolveBindingKey, toSessionBindingRecord) - P2: Fix misleading comments to match actual behavior - Tests: added multi-account fallback test, persist verification
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1bb87c68c1
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3cac943532
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| void import("./src/matrix/subagent-hooks.js") | ||
| .then(({ registerMatrixSubagentHooks }) => { | ||
| try { | ||
| registerMatrixSubagentHooks(api); |
There was a problem hiding this comment.
Register Matrix subagent hooks before returning from plugin load
Because this registration is deferred behind a fire-and-forget import(), there is a startup/reload window where Matrix still has no subagent_spawning hooks. src/plugins/loader.ts:1274-1280 does not wait for plugin registration, and src/agents/subagent-spawn.ts:248-253 immediately rejects thread=true when no hook is present, so the first Matrix sessions_spawn(..., thread:true) after gateway start or plugin reload can fail nondeterministically depending on import timing.
Useful? React with 👍 / 👎.
| const candidates = accountId | ||
| ? listBindingsForAccount(accountId) | ||
| : listAllBindings(); |
There was a problem hiding this comment.
Clean persisted Matrix bindings when the account monitor is offline
This cleanup only searches listBindingsForAccount() / listAllBindings(), so it can remove bindings only while that account's manager is still holding them in memory. When a Matrix monitor stops, extensions/matrix/src/matrix/thread-bindings.ts:372-376 drops the account's records from RAM but leaves thread-bindings.json on disk, and createMatrixThreadBindingManager() reloads that file on the next start (extensions/matrix/src/matrix/thread-bindings.ts:237-239). If the child session is reset or deleted while the Matrix account is offline, subagent_ended never sees the binding and the stale room/thread mapping comes back on restart.
Useful? React with 👍 / 👎.
- P1: subagent_spawning no longer calls setBindingRecord/persist directly. The core invokes the adapter's bind() with placement='child' after we return threadBindingReady: true. The adapter handles record creation, persistence, and child thread creation atomically. - P2: Removed setBindingRecord import (no longer used in hooks). - Tests updated to verify setBindingRecord is NOT called from hooks.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b21468513f
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| for (const binding of matching) { | ||
| if (removeBindingRecord(binding)) { | ||
| affectedAccountIds.add(binding.accountId); | ||
| } | ||
| } | ||
| // Persist for each affected account via its manager. | ||
| for (const acctId of affectedAccountIds) { | ||
| const manager = getMatrixThreadBindingManager(acctId); | ||
| await manager?.persist(); |
There was a problem hiding this comment.
Route ended-session cleanup through Matrix's unbind flow
For spawn-failed, session-reset, and session-delete, callers set sendFarewell: true (src/agents/subagent-spawn.ts:693-700, src/gateway/session-reset-service.ts:95-104), but this handler removes bindings directly and only persists them. Because it never goes through extensions/matrix/src/matrix/thread-bindings.ts:406-413, Matrix never posts the farewell message that the existing unbind path sends, so users are left with a dead room/thread that looks silently active and can keep replying into it.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid enhancement — the current implementation mirrors the cleanup pattern but skips farewell messages. Happy to add this if a maintainer wants it in scope.
|
Matrix ACP thread binding already exists on The main issue is that the hook returns |
|
Thanks for putting this together. I’m going to close this as superseded: Matrix subagent thread binding hooks are already implemented on If there is still a missing Matrix subagent behavior that this PR covers and |
Add
subagent_spawning,subagent_delivery_target, andsubagent_endedhooks to the Matrix extension, enabling thread-bound ACP sessions (Claude Code, Codex) on Matrix — analogous to the existing Discord implementation.The Matrix extension already has the full infrastructure:
thread-bindings.ts)thread-bindings-shared.ts)monitor/route.ts)This commit adds the missing glue: three hook handlers that connect the ACP session lifecycle to the existing binding infrastructure.
Summary
sessions_spawn(runtime: "acp", thread: true)only works on Discord because only Discord registerssubagent_spawninghooks. Matrix users cannot use thread-bound ACP sessions despite the binding infrastructure already existing.subagent_spawning,subagent_delivery_target,subagent_ended) + hook registration inregisterFull()via dynamic import.extensions/matrix/.Change Type
Scope
Linked Issue/PR
User-visible / Behavior Changes
sessions_spawn(runtime: "acp", thread: true)to create thread-bound ACP sessionschannels.matrix.threadBindings.enabled: trueandchannels.matrix.threadBindings.spawnSubagentSessions: trueSecurity Impact
NoNoNoNoNoRepro + Verification
Environment
Steps
channels.matrix.threadBindings.enabled: trueandspawnSubagentSessions: truesessions_spawn(runtime: "acp", thread: true)Expected
Actual
.14release withconversation-runtimein SDK exports). Unit tests cover all hook logic.Evidence
pnpm test:extension matrix— all greenHuman Verification
.14release — installed.13doesn't exposeregisterSessionBindingAdaptervia SDK)AI-Assisted PR 🤖
Review Conversations
Compatibility / Migration
YesNo(new optional config flags, defaults to disabled)NoFailure Recovery
subagent_spawning/subagent_delivery_target/subagent_endedevents match channel=matrix.Risks and Mitigations
thread-bindings-sharedstore. No new state management.