fix(agents): prefer target agent's bound Matrix account for subagent spawns#1
Closed
lukeboyett wants to merge 2 commits into
Closed
fix(agents): prefer target agent's bound Matrix account for subagent spawns#1lukeboyett wants to merge 2 commits into
lukeboyett wants to merge 2 commits into
Conversation
…spawns A Matrix-bound parent session spawning a subagent for another agent could seed the child's deliveryContext.accountId with the caller's account before target bindings were consulted, causing child posts to come from the wrong Matrix identity. - src/agents/subagent-spawn.ts: add resolveRequesterOriginForChild(...) that prefers the target agent's bound account via resolveFirstBoundAccountId before falling back to the caller's accountId. - src/routing/bound-account-read.ts: resolveFirstBoundAccountId now accepts an optional peerId and uses two-tier matching — peer-specific bindings win immediately, channel-only bindings are kept as fallback, non-matching peer bindings are skipped. The existing cron delivery-target caller is unaffected (it does not pass peerId, so it gets channel-only matching as before). Regression coverage in openclaw-tools.subagents.sessions-spawn.lifecycle.test.ts: - Matrix room-bound route uses the target agent's bound account over the caller's account. - Peer-specific binding wins over channel-only binding for the same agent. - Non-matching peer falls back to the channel-only binding. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
Owner
Author
|
@codex review |
![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: 5d11d7bb1e
ℹ️ 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".
…nt lookup `resolveFirstBoundAccountId` previously compared `resolved.peerId` as a literal string, so a binding with `match.peer.id: "*"` would never match real peer IDs and would get skipped. That could make child spawn identity selection fall through to a channel-only or requester account instead of the target agent's wildcard-bound account. Align the helper's three-tier precedence with `resolve-route.ts`: 1. Exact peer match wins immediately. 2. Wildcard peer match (`peer.id: "*"`) wins over channel-only. 3. Channel-only binding is the final fallback. Addresses Codex review comment on PR #1. Regression coverage: - Wildcard peer binding matches a caller with any peer and beats a channel-only binding for the same agent. - Exact peer binding still wins over a wildcard binding when both exist. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
Owner
Author
|
@codex review |
|
Codex Review: Didn't find any major issues. 🚀 ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
Owner
Author
|
Review consensus reached. Codex approved after one P1 fix (wildcard peer binding handling). Squashed to a single commit and opened upstream as openclaw#67508. Closing this fork PR as the review record. |
lukeboyett
added a commit
that referenced
this pull request
May 26, 2026
…d fallback against shutdown race Two follow-up findings from PR review on openclaw#86701: 1. Linux gating (clawsweeper [P2], also codex round-1 #1). On non-macOS/non-Windows platforms, Node routes `fs.watch(dir, { recursive: true })` through `internal/fs/recursive_watch`, which walks the tree and attaches one watcher per discovered entry. That defeats the constant-watcher- profile goal of the parent commit's fix without throwing, so the creation-failure fallback never fires. Restrict native attachment to `process.platform === "darwin" || "win32"`; Linux and others go straight to the chokidar fallback, matching pre-PR behavior on those platforms. 2. Shutdown-race guard (codex-connector inline reviews 4359699784 + 4359929223 [P2]). The native watcher `error` handler called `attachMemoryChokidarFallback` unconditionally, including after `close()` had set `this.closed = true` and begun teardown. That left a window where an error fired mid-shutdown could spin up a new chokidar watcher after the normal watcher-close step had already run, leaking watch handles past manager close. Both the error handler and the `attachMemoryChokidarFallback` helper now short-circuit when `this.closed` is true. Tests: - new case in manager.watcher-config.test.ts verifies that `process.platform === "linux"` routes both `memory/` and dir-typed `extraPaths` directly to chokidar (`nativeWatchMock` not called at all) — covers the Linux gate - all 17 tests in manager.watcher-config.test.ts pass Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
osolmaz
pushed a commit
that referenced
this pull request
May 26, 2026
…d fallback against shutdown race Two follow-up findings from PR review on openclaw#86701: 1. Linux gating (clawsweeper [P2], also codex round-1 #1). On non-macOS/non-Windows platforms, Node routes `fs.watch(dir, { recursive: true })` through `internal/fs/recursive_watch`, which walks the tree and attaches one watcher per discovered entry. That defeats the constant-watcher- profile goal of the parent commit's fix without throwing, so the creation-failure fallback never fires. Restrict native attachment to `process.platform === "darwin" || "win32"`; Linux and others go straight to the chokidar fallback, matching pre-PR behavior on those platforms. 2. Shutdown-race guard (codex-connector inline reviews 4359699784 + 4359929223 [P2]). The native watcher `error` handler called `attachMemoryChokidarFallback` unconditionally, including after `close()` had set `this.closed = true` and begun teardown. That left a window where an error fired mid-shutdown could spin up a new chokidar watcher after the normal watcher-close step had already run, leaking watch handles past manager close. Both the error handler and the `attachMemoryChokidarFallback` helper now short-circuit when `this.closed` is true. Tests: - new case in manager.watcher-config.test.ts verifies that `process.platform === "linux"` routes both `memory/` and dir-typed `extraPaths` directly to chokidar (`nativeWatchMock` not called at all) — covers the Linux gate - all 17 tests in manager.watcher-config.test.ts pass Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
osolmaz
pushed a commit
that referenced
this pull request
May 26, 2026
…d fallback against shutdown race Two follow-up findings from PR review on openclaw#86701: 1. Linux gating (clawsweeper [P2], also codex round-1 #1). On non-macOS/non-Windows platforms, Node routes `fs.watch(dir, { recursive: true })` through `internal/fs/recursive_watch`, which walks the tree and attaches one watcher per discovered entry. That defeats the constant-watcher- profile goal of the parent commit's fix without throwing, so the creation-failure fallback never fires. Restrict native attachment to `process.platform === "darwin" || "win32"`; Linux and others go straight to the chokidar fallback, matching pre-PR behavior on those platforms. 2. Shutdown-race guard (codex-connector inline reviews 4359699784 + 4359929223 [P2]). The native watcher `error` handler called `attachMemoryChokidarFallback` unconditionally, including after `close()` had set `this.closed = true` and begun teardown. That left a window where an error fired mid-shutdown could spin up a new chokidar watcher after the normal watcher-close step had already run, leaking watch handles past manager close. Both the error handler and the `attachMemoryChokidarFallback` helper now short-circuit when `this.closed` is true. Tests: - new case in manager.watcher-config.test.ts verifies that `process.platform === "linux"` routes both `memory/` and dir-typed `extraPaths` directly to chokidar (`nativeWatchMock` not called at all) — covers the Linux gate - all 17 tests in manager.watcher-config.test.ts pass Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
osolmaz
pushed a commit
that referenced
this pull request
May 26, 2026
…d fallback against shutdown race Two follow-up findings from PR review on openclaw#86701: 1. Linux gating (clawsweeper [P2], also codex round-1 #1). On non-macOS/non-Windows platforms, Node routes `fs.watch(dir, { recursive: true })` through `internal/fs/recursive_watch`, which walks the tree and attaches one watcher per discovered entry. That defeats the constant-watcher- profile goal of the parent commit's fix without throwing, so the creation-failure fallback never fires. Restrict native attachment to `process.platform === "darwin" || "win32"`; Linux and others go straight to the chokidar fallback, matching pre-PR behavior on those platforms. 2. Shutdown-race guard (codex-connector inline reviews 4359699784 + 4359929223 [P2]). The native watcher `error` handler called `attachMemoryChokidarFallback` unconditionally, including after `close()` had set `this.closed = true` and begun teardown. That left a window where an error fired mid-shutdown could spin up a new chokidar watcher after the normal watcher-close step had already run, leaking watch handles past manager close. Both the error handler and the `attachMemoryChokidarFallback` helper now short-circuit when `this.closed` is true. Tests: - new case in manager.watcher-config.test.ts verifies that `process.platform === "linux"` routes both `memory/` and dir-typed `extraPaths` directly to chokidar (`nativeWatchMock` not called at all) — covers the Linux gate - all 17 tests in manager.watcher-config.test.ts pass Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes Matrix subagent spawns inheriting the caller's
accountIdinstead of the target agent's room-bound account.src/agents/subagent-spawn.ts: addsresolveRequesterOriginForChild(...)that prefers the target agent's bound account viaresolveFirstBoundAccountId({ cfg, channelId, agentId: targetAgentId, peerId: requesterTo })before falling back to the caller'saccountId.src/routing/bound-account-read.ts:resolveFirstBoundAccountIdnow accepts an optionalpeerIdand uses two-tier matching — peer-specific bindings win immediately, channel-only bindings are kept as fallback, non-matching peer bindings are skipped.peerId(e.g.src/cron/isolated-agent/delivery-target.tsis unaffected).Why
Before this fix, a Matrix-bound parent session spawning a child for another agent could seed
deliveryContext.accountIdwith the caller's account before target bindings were consulted. A child intended to post as the target agent's bound Matrix identity could end up posting as the caller.The original
resolveFirstBoundAccountIdalso matched only on channel + agent, so multi-room-bound agents could get the "first binding wins" regardless of which room the spawn was happening in. The two-tier matching fixes that while staying backward-compatible for callers that don't pass a peer.Regression coverage
src/agents/openclaw-tools.subagents.sessions-spawn.lifecycle.test.ts:sessions_spawn uses the target agent's bound account for a Matrix room-bound route— a room-bound target binding beats the caller's account.sessions_spawn prefers peer-specific binding over channel-only binding— two bindings for the same agent (channel-only default, peer-specific for room A); spawning in room A returns the peer-specific account.sessions_spawn falls back to channel-only binding when peer does not match— same two bindings; spawning in a non-matching room returns the channel-only default.Existing caller-fallback coverage is provided by
sessions_spawn announces with requester accountId.Validation
pnpm build✓pnpm check✓ (typecheck, lint, import cycles, boundary checks; also re-run by the pre-commit hook)pnpm test:changed✓ 3777/3777 (360s)AI-assisted
Authored with Claude Code (Claude Opus 4.6, 1M-context). Fully tested locally (typecheck + lint + targeted vitest +
pnpm test:changed+ live Matrix validation). I've reviewed the change and understand what it does.Related / prior art
deliveryContextfrom spawn params at the gateway layer — complementary, different layer.accountIdforwarding insendMessage.@codex review