Refactor subagent thread binding into core#88416
Conversation
|
Codex review: needs real behavior proof before merge. Reviewed May 30, 2026, 4:11 PM ET / 20:11 UTC. Summary PR surface: Source +389, Tests +81, Docs +33. Total +503 across 25 files. Reproducibility: yes. for the review findings by source inspection: the account fallback and Discord thread merge paths are deterministic in the PR head. No live channel reproduction was run. Review metrics: 1 noteworthy metric.
Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Proof guidance:
Mantis proof suggestion Risk before merge
Maintainer options:
Next step before merge
Security Review findings
Review detailsBest possible solution: Land this ownership move only after core account resolution uses the channel default contract, bound delivery preserves the child thread target, and redacted live channel proof shows thread-bound spawn and reply routing. Do we have a high-confidence way to reproduce the issue? Yes for the review findings by source inspection: the account fallback and Discord thread merge paths are deterministic in the PR head. No live channel reproduction was run. Is this the best way to solve the issue? No: centralizing thread binding may be the right direction, but this implementation needs the channel account contract and bound-thread delivery fixes plus live proof before it is merge-ready. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model gpt-5.5, reasoning high; reviewed against a9a86f788b08. Label changesLabel changes:
Label justifications:
Evidence reviewedPR surface: Source +389, Tests +81, Docs +33. Total +503 across 25 files. View PR surface stats
What I checked:
Likely related people:
What the crustacean ranks mean
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics. How this review workflow works
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 85a8837d4b
ℹ️ 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 channels = params.cfg.channels as Record<string, { defaultAccount?: unknown } | undefined>; | ||
| return normalizeOptionalString(channels?.[channel]?.defaultAccount) ?? "default"; |
There was a problem hiding this comment.
Use plugin default account resolution
When requester.accountId is omitted, this falls back to channels.<id>.defaultAccount ?? "default", but the channel plugins already resolve omitted accounts differently: Discord/Feishu use createAccountListHelpers(...).resolveDefaultAccountId, and Matrix uses resolveMatrixDefaultOrOnlyAccountId, all of which can select the only configured named account when defaultAccount is absent. In a config like channels.discord.accounts.work with no channels.discord.defaultAccount, the monitor registers the session-binding adapter under work, while this code asks getCapabilities for default and rejects sessions_spawn(mode="session") / thread=true as unavailable. Resolve the account through the plugin/channel config defaultAccountId contract instead of reading only the raw config key.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a8caa6fd50
ℹ️ 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 deliveryOrigin = normalizeDeliveryContext(result.deliveryOrigin); | ||
| const deliveryOrigin = routeToDeliveryFields(routeFromBindingRecord(binding)).deliveryContext; |
There was a problem hiding this comment.
Set Discord delivery thread to the bound child thread
When thread=true is used from an existing Discord thread, this projection returns a delivery origin with to: channel:<new child thread> but no threadId because Discord does not provide a resolveDeliveryTarget for binding records. The later mergeDeliveryContext(bindResult.deliveryOrigin, childSessionOrigin) preserves the requester's old threadId, and Discord outbound routing (extensions/discord/src/outbound-send-context.ts:27) sends to channel:<threadId> whenever it is present. In that scenario the initial child run is delivered back to the requester thread instead of the newly bound subagent thread; include the binding's conversation id as the delivery threadId for Discord (or add an equivalent Discord delivery target resolver) before merging.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7a6acc3730
ℹ️ 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 prepared = prepareSubagentThreadBinding({ | ||
| cfg: params.cfg, | ||
| mode: params.mode, | ||
| requesterSessionKey: params.requesterSessionKey, | ||
| requester: params.requester, | ||
| }); |
There was a problem hiding this comment.
Preserve legacy subagent_spawning fallback
When an installed third-party channel plugin still relies on api.on("subagent_spawning") during the documented deprecation window, this new path prepares core bindings directly and returns an unavailable/error result before ever invoking the registered hook. The same commit adds a compat record with removeAfter: "2026-08-30" for this hook, so deprecated plugins that have not yet migrated to a session-binding adapter lose sessions_spawn(mode="session", thread=true) immediately instead of receiving compatibility behavior; keep the hook runner as a fallback when core binding preparation cannot handle the target.
Useful? React with 👍 / 👎.
| const fallbackConversationRef = resolveConversationRefForThreadBinding({ | ||
| cfg: params.cfg, | ||
| channel: policy.channel, | ||
| accountId: policy.accountId, | ||
| to: params.requester.to, | ||
| threadId: params.requester.threadId, | ||
| }); |
There was a problem hiding this comment.
Reject Feishu group chats without a topic
For Feishu group chat requests with to: "chat:oc_..." and no threadId, this generic fallback now resolves the chat id as a current conversation and the Feishu session-binding adapter accepts it, binding the entire group chat to the subagent. The removed Feishu subagent_spawning handler intentionally returned an error for isChatTarget without a topic, allowing only DMs or topic conversations; preserve that channel-specific restriction here so a group-level sessions_spawn does not hijack all chat messages for the child session.
Useful? React with 👍 / 👎.
Mantis Telegram Desktop ProofSummary: Mantis captured native Telegram Desktop before/after proof for thread-bound subagent spawning.
Motion-trimmed clips: |
Move subagent thread binding ownership into core so session-mode spawns prepare channel bindings before launching the child agent. Deprecate the legacy subagent_spawning SDK hook in code, compatibility metadata, diagnostics, and plugin docs; plugin authors should observe subagent_spawned instead. Verification: - node scripts/run-vitest.mjs src/agents/sessions-spawn-hooks.test.ts src/agents/subagent-spawn.thread-binding.test.ts src/agents/subagent-spawn.workspace.test.ts src/agents/subagent-spawn.mode-session-diagnostics.test.ts - node scripts/run-tsgo.mjs -p tsconfig.core.json --incremental --tsBuildInfoFile .artifacts/tsgo-cache/core.tsbuildinfo - git diff --check - .agents/skills/autoreview/scripts/autoreview --mode local - CI run 26693808952 green, including checks-node-agentic-agents-core and checks-node-agentic-plugin-sdk
Move subagent thread binding ownership into core so session-mode spawns prepare channel bindings before launching the child agent. Deprecate the legacy subagent_spawning SDK hook in code, compatibility metadata, diagnostics, and plugin docs; plugin authors should observe subagent_spawned instead. Verification: - node scripts/run-vitest.mjs src/agents/sessions-spawn-hooks.test.ts src/agents/subagent-spawn.thread-binding.test.ts src/agents/subagent-spawn.workspace.test.ts src/agents/subagent-spawn.mode-session-diagnostics.test.ts - node scripts/run-tsgo.mjs -p tsconfig.core.json --incremental --tsBuildInfoFile .artifacts/tsgo-cache/core.tsbuildinfo - git diff --check - .agents/skills/autoreview/scripts/autoreview --mode local - CI run 26693808952 green, including checks-node-agentic-agents-core and checks-node-agentic-plugin-sdk
Move subagent thread binding ownership into core so session-mode spawns prepare channel bindings before launching the child agent. Deprecate the legacy subagent_spawning SDK hook in code, compatibility metadata, diagnostics, and plugin docs; plugin authors should observe subagent_spawned instead. Verification: - node scripts/run-vitest.mjs src/agents/sessions-spawn-hooks.test.ts src/agents/subagent-spawn.thread-binding.test.ts src/agents/subagent-spawn.workspace.test.ts src/agents/subagent-spawn.mode-session-diagnostics.test.ts - node scripts/run-tsgo.mjs -p tsconfig.core.json --incremental --tsBuildInfoFile .artifacts/tsgo-cache/core.tsbuildinfo - git diff --check - .agents/skills/autoreview/scripts/autoreview --mode local - CI run 26693808952 green, including checks-node-agentic-agents-core and checks-node-agentic-plugin-sdk




Summary
thread: truesubagent binding into core viaSessionBindingServicecapability checks, conversation resolution, bind metadata, and route projection.subagent_spawning; keep the public hook type as deprecated compatibility and keepsubagent_spawnedfor post-launch lifecycle observation.Verification
pnpm docs:listpnpm test src/agents/sessions-spawn-hooks.test.ts src/agents/subagent-spawn.thread-binding.test.ts src/agents/subagent-spawn.workspace.test.ts src/agents/subagent-spawn.mode-session-diagnostics.test.ts src/plugins/contracts/boundary-invariants.test.ts extensions/matrix/index.test.tspnpm test extensions/discord/src/subagent-hooks.test.ts extensions/feishu/src/subagent-hooks.test.ts extensions/matrix/src/matrix/subagent-hooks.test.ts extensions/matrix/index.test.ts src/agents/sessions-spawn-hooks.test.ts src/agents/subagent-spawn.thread-binding.test.ts src/agents/subagent-spawn.workspace.test.ts src/agents/subagent-spawn.mode-session-diagnostics.test.ts src/plugins/contracts/boundary-invariants.test.tspnpm test src/agents/sessions-spawn-hooks.test.ts src/agents/subagent-spawn.thread-binding.test.ts src/agents/subagent-spawn.workspace.test.ts src/agents/subagent-spawn.mode-session-diagnostics.test.ts extensions/feishu/src/subagent-hooks.test.tsnode scripts/run-tsgo.mjs -p test/tsconfig/tsconfig.core.test.json --incremental --tsBuildInfoFile .artifacts/tsgo-cache/core-test.tsbuildinfonode scripts/run-tsgo.mjs -p tsconfig.core.json --incremental --tsBuildInfoFile .artifacts/tsgo-cache/core.tsbuildinfo.agents/skills/autoreview/scripts/autoreview --mode branch --base origin/main(clean; ignored expected no-fallback concern for retiredsubagent_spawningbinding path)pnpm check:changedvia Testboxtbx_01ksx2kt0kbgg68w3wf3z3xrt9, GitHub run26691692808Real behavior proof
Behavior addressed: Native subagent thread binding no longer depends on bundled
subagent_spawninghooks.Real environment tested: Local focused Vitest shards plus Blacksmith Testbox changed gate.
Exact steps or command run after this patch: See Verification above.
Evidence after fix: Focused tests passed, core typechecks passed, autoreview passed, and Testbox
check:changedexited 0.Observed result after fix: Core binds
thread: truesubagent sessions through session-binding adapters and emitssubagent_spawnedafter launch metadata is resolved.What was not tested: Live Discord, Matrix, Telegram, or Feishu channel roundtrips.