fix(matrix): preserve ACP thread binding targets#64343
Conversation
Greptile SummaryThis PR fixes Matrix ACP thread-bound session spawns incorrectly using a lowercased Confidence Score: 5/5Safe to merge — fix is correct, targeted, and covered by new seam tests; only finding is a P2 CHANGELOG placement style issue. The implementation correctly prioritises the canonical No files require special attention. Prompt To Fix All With AIThis is a comment left during a code review.
Path: CHANGELOG.md
Line: 13
Comment:
**Changelog entry should be appended at the end of the section**
Per the repo's CLAUDE.md guideline ("append new entries to the end of the target section; do not insert new entries at the top of a section"), this entry should appear after the last existing `### Fixes` bullet rather than at the top of the block. Move the Matrix/ACP line to after the final existing fix entry (e.g. after the QQBot/streaming line).
**Context Used:** CLAUDE.md ([source](https://app.greptile.com/review/custom-context?memory=fd949e91-5c3a-4ab5-90a1-cbe184fd6ce8))
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "fix: preserve ACP thread binding targets" | Re-trigger Greptile |
There was a problem hiding this comment.
Pull request overview
Fixes ACP thread-bound session spawn routing for Matrix by preserving canonical delivery targets (including mixed-case room IDs) and retaining parent conversation context when binding threads, using the channel plugin seam instead of a lossy groupId fallback.
Changes:
- Resolve thread-binding conversation refs via
messaging.resolveInboundConversation()while keepingtoauthoritative and capturingparentConversationIdwhen available. - Extend ACP thread-binding flow to persist/pass
parentConversationIdthrough bind + delivery-plan matching. - Add seam tests covering mixed-case Matrix room IDs (top-level + existing thread) and a regression guard for LINE precedence; refactor Telegram test setup helper.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/agents/acp-spawn.ts | Preserves canonical to in plugin conversation resolution and propagates parentConversationId for ACP thread bindings. |
| src/agents/acp-spawn.test.ts | Adds regression tests for Matrix casing/thread parent preservation; keeps LINE/Telegram behavior covered and reduces setup duplication. |
| CHANGELOG.md | Documents the Matrix/ACP thread-binding spawn fix in Unreleased fixes. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5c85e46838
ℹ️ 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".
5c85e46 to
a37b470
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e198c9263e
ℹ️ 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".
0eb980e to
a7312ad
Compare
a7312ad to
def7dcd
Compare
|
Merged via squash.
Thanks @gumadeiras! |
Merged via squash. Prepared head SHA: def7dcd Co-authored-by: gumadeiras <5599352+gumadeiras@users.noreply.github.com> Co-authored-by: gumadeiras <5599352+gumadeiras@users.noreply.github.com> Reviewed-by: @gumadeiras
Merged via squash. Prepared head SHA: def7dcd Co-authored-by: gumadeiras <5599352+gumadeiras@users.noreply.github.com> Co-authored-by: gumadeiras <5599352+gumadeiras@users.noreply.github.com> Reviewed-by: @gumadeiras
Merged via squash. Prepared head SHA: def7dcd Co-authored-by: gumadeiras <5599352+gumadeiras@users.noreply.github.com> Co-authored-by: gumadeiras <5599352+gumadeiras@users.noreply.github.com> Reviewed-by: @gumadeiras
Merged via squash. Prepared head SHA: def7dcd Co-authored-by: gumadeiras <5599352+gumadeiras@users.noreply.github.com> Co-authored-by: gumadeiras <5599352+gumadeiras@users.noreply.github.com> Reviewed-by: @gumadeiras
Merged via squash. Prepared head SHA: def7dcd Co-authored-by: gumadeiras <5599352+gumadeiras@users.noreply.github.com> Co-authored-by: gumadeiras <5599352+gumadeiras@users.noreply.github.com> Reviewed-by: @gumadeiras
Summary
groupIdas the transport target, which lowercased mixed-case Matrix room ids and dropped parent conversation context for thread binds.src/agents/acp-spawn.tsnow resolves full conversation refs through the plugin seam, keeps canonicaltoauthoritative, preservesparentConversationIdfor thread binds, and keeps channel-specific fallback behavior intact.groupIdsemantics across policy/config code were not redesigned here.Change Type (select all)
Scope (select all touched areas)
Linked Issue/PR
Root Cause (if applicable)
toand fallbackgroupIdinto the same value before callingresolveInboundConversation(), so case-sensitive Matrix room ids were lowercased and parent conversation refs for thread binds were lost.groupIdis still overloaded in core for policy/session metadata, so the ACP seam needed to prefer canonical routing data without changing broadergroupIdbehavior for other channels.Regression Test Plan (if applicable)
src/agents/acp-spawn.test.tsthread=truepreserves canonical Matrix room casing from both top-level rooms and existing Matrix threads while leaving LINE and Telegram behavior unchanged.User-visible / Behavior Changes
groupId, including spawns launched from existing Matrix threads.Diagram (if applicable)
Security Impact (required)
Yes, explain risk + mitigation:Repro + Verification
Environment
channels.matrix.threadBindings.enabled=trueSteps
groupId.sessions_spawn({ runtime: "acp", thread: true })from the room or from an existing Matrix thread.Expected
Actual
Evidence
Human Verification (required)
pnpm test src/agents/acp-spawn.test.tspnpm check