jlxq0/matrix-mcp
1
0
Fork 0
Code Issues 14 Pull requests 1 Projects Releases 1 Packages 1 Wiki Activity Actions

feat(channel): let the mount open an attachment, not just name it #113

Merged
jlxq0 merged 2 commits from channel-download into main 2026-08-25 07:11:40 +00:00
Conversation 1 Commits 2 Files changed 5 +74 -1
jlxq0 commented 2026-08-25 06:57:24 +00:00
Owner
Copy link

download_attachment was not missing, it was unexposed. CHANNEL_TOOLS listed
seven tools and it was not one of them, so after #107 a channel session is told
attachment="m.image" filename="..." and has no way to open the file. This adds
it to the mount and tells the model it can fetch.

Closes #111

The two questions the issue asked

Does it decrypt an E2EE file object? Yes, and not from the doc comment.
Media::get_media_content matches MediaSource::Encrypted(file) and runs
AttachmentDecryptor::new(cursor, file) before returning
(matrix-sdk 0.17 src/media.rs:450-478). TimelineEvent::raw() returns
d.event for TimelineEventKind::Decrypted, so the msgtype match sees
plaintext; an UnableToDecrypt event has no msgtype and fails with
"event content is not a room message" rather than handing back ciphertext.

That is true only while the e2e-encryption feature is on: the decrypt block is
inside #[cfg(feature = "e2e-encryption")], and with the feature off the same
call returns the ciphertext, which the tool base64-encodes and reports as the
file. Nothing errors and a test asserting a non-empty body still passes, so it
is now a Known Pitfall in AGENTS.md.

Does the instructions string need to name the tool? Yes. The schema tells
the model the tool exists; the instructions are where it learns that an
attachment= attribute means there is something to fetch. Without that it
reports the filename and stops, which from the far end reads exactly like not
having the tool.

The trap, and the test that can see it

channel_mount_offers_only_the_scoped_tools asserts advertised is a subset of
CHANNEL_TOOLS. That direction cannot see a name added to the const and never
wired to the mount: the advertised list is simply shorter and every entry in it
still checks out. The symptom is the tool being listed and every call failing.

every_channel_tool_is_actually_advertised runs the other direction over the
real router. Negative controls, all run:

Break Old test New test
bogus name in CHANNEL_TOOLS, wired to nothing passes fails
download_attachment removed from CHANNEL_TOOLS passes a_session_told_a_file_arrived_can_open_it fails
instructions no longer name the tool passes a_session_told_a_file_arrived_can_open_it fails

a_session_told_a_file_arrived_can_open_it is const-against-const and says so in
its own comment: it cannot observe whether the fetch works.

Gates

cargo fmt --all --check, cargo clippy --all-targets --all-features --locked -- -D warnings
and cargo test --all-features --locked all green on rustc 1.98.0, which is
current stable and what the runner installs. 208 tests pass.

Note for anyone reproducing: this tree does not lint below clippy 1.98 for
reasons unrelated to this PR — see #112.

Not done here

The acceptance observation is Julian sending one screenshot into
!nJqaJVNKzmgkUjjSLE:kampong.social and the session reporting what is visibly
in it. That needs the deployed server, so it happens after v0.10.3. Neither
test in this PR can stand in for it: a deferred schema can be present while the
fetch fails, and base64 of an encrypted blob is bytes too.

`download_attachment` was not missing, it was unexposed. `CHANNEL_TOOLS` listed seven tools and it was not one of them, so after #107 a channel session is told `attachment="m.image" filename="..."` and has no way to open the file. This adds it to the mount and tells the model it can fetch. Closes #111 ## The two questions the issue asked **Does it decrypt an E2EE `file` object?** Yes, and not from the doc comment. `Media::get_media_content` matches `MediaSource::Encrypted(file)` and runs `AttachmentDecryptor::new(cursor, file)` before returning (matrix-sdk 0.17 `src/media.rs:450-478`). `TimelineEvent::raw()` returns `d.event` for `TimelineEventKind::Decrypted`, so the `msgtype` match sees plaintext; an `UnableToDecrypt` event has no `msgtype` and fails with "event content is not a room message" rather than handing back ciphertext. That is true only while the `e2e-encryption` feature is on: the decrypt block is inside `#[cfg(feature = "e2e-encryption")]`, and with the feature off the same call returns the ciphertext, which the tool base64-encodes and reports as the file. Nothing errors and a test asserting a non-empty body still passes, so it is now a Known Pitfall in `AGENTS.md`. **Does the instructions string need to name the tool?** Yes. The schema tells the model the tool exists; the instructions are where it learns that an `attachment=` attribute means there is something to fetch. Without that it reports the filename and stops, which from the far end reads exactly like not having the tool. ## The trap, and the test that can see it `channel_mount_offers_only_the_scoped_tools` asserts advertised is a subset of `CHANNEL_TOOLS`. That direction cannot see a name added to the const and never wired to the mount: the advertised list is simply shorter and every entry in it still checks out. The symptom is the tool being listed and every call failing. `every_channel_tool_is_actually_advertised` runs the other direction over the real router. Negative controls, all run: | Break | Old test | New test | |---|---|---| | bogus name in `CHANNEL_TOOLS`, wired to nothing | passes | **fails** | | `download_attachment` removed from `CHANNEL_TOOLS` | passes | `a_session_told_a_file_arrived_can_open_it` **fails** | | instructions no longer name the tool | passes | `a_session_told_a_file_arrived_can_open_it` **fails** | `a_session_told_a_file_arrived_can_open_it` is const-against-const and says so in its own comment: it cannot observe whether the fetch works. ## Gates `cargo fmt --all --check`, `cargo clippy --all-targets --all-features --locked -- -D warnings` and `cargo test --all-features --locked` all green on rustc 1.98.0, which is current stable and what the runner installs. 208 tests pass. Note for anyone reproducing: this tree does not lint below clippy 1.98 for reasons unrelated to this PR — see #112. ## Not done here The acceptance observation is Julian sending one screenshot into `!nJqaJVNKzmgkUjjSLE:kampong.social` and the session reporting what is visibly in it. That needs the deployed server, so it happens after `v0.10.3`. Neither test in this PR can stand in for it: a deferred schema can be present while the fetch fails, and base64 of an encrypted blob is bytes too.
feat(channel): let the mount open an attachment, not just name it
All checks were successful
CI / cargo (pull_request) Successful in 1m18s
Details
CI / docker (pull_request) Has been skipped
Details
67c49f9fa1
 
The push already carries attachment="m.image" and filename="...", and the
channel mount offered seven tools with download_attachment not among them.
A session was told a file arrived and had no way to open it: the notice,
never the bytes. A screenshot is how Julian shows a session what he is
looking at, and a filename is not a screenshot.

The tool itself needed nothing. It is fully implemented at src/mcp.rs:4998,
size-capped against the event's declared info.size before any media I/O and
again after, and it decrypts: get_media_content runs AttachmentDecryptor on
a MediaSource::Encrypted, and TimelineEvent::raw() yields the decrypted
inner event, so an undecryptable event fails with "not a room message"
rather than returning ciphertext. That last part is load-bearing and only
true while the e2e-encryption feature is on, which is now in AGENTS.md.

The instructions string names the tool too. Without that a model reports the
filename and stops, which from the far end reads exactly like not having the
tool at all.

The test that matters is every_channel_tool_is_actually_advertised. The
existing check ran advertised ⊆ CHANNEL_TOOLS, which cannot see a name added
to the const and never wired to the mount — the list is simply shorter and
every entry in it still checks out. Confirmed: with a bogus name in the
const, the old test passes and the new one fails.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0176noPigX2cwByDtRcafzyi
fix(mcp): download_attachment says "not joined" without checking
All checks were successful
CI / cargo (pull_request) Successful in 1m22s
Details
CI / docker (pull_request) Has been skipped
Details
4e063decbf
 
Codex, reviewing the mount change, found the comment I wrote next to
CHANNEL_TOOLS was false: it claimed the tool takes a room_id the identity
must be joined to. `client.get_room()` reads the SDK's local state store,
which holds invited, left, knocked and banned rooms, so `Some` means the
identity has heard of the room. `read_thread` and `room_info` both guard on
RoomState::Joined for exactly this reason; this one did not, while emitting
"not joined to {room_id}" as the error for the case it never checked.

Verified rather than taken from the review: matrix-sdk-base 0.17
client.rs:1037 forwards straight to `state_store.room()`, which is
state-agnostic.

This also makes #111's stated negative control true — an event in a room the
identity is not joined to is now refused — where before it held only for a
room the store had never seen.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0176noPigX2cwByDtRcafzyi
jlxq0 commented 2026-08-25 07:00:30 +00:00
Author
Owner
Copy link

Cross-engine review (Codex, read-only) on git diff main...channel-download. Three questions asked, one real finding.

Wrong claim in my own comment, now fixed in 4e063de. I wrote next to CHANNEL_TOOLS that the tool "takes a room_id the identity must be joined to". False. client.get_room() forwards to state_store.room() (matrix-sdk-base 0.17 client.rs:1037), which is state-agnostic and returns invited, left, knocked and banned rooms. download_attachment had no RoomState::Joined guard while read_thread (src/mcp.rs:2589) and room_info (src/mcp.rs:3258) both have one, and it emitted not joined to {room_id} as the error for a case it never checked. Added the guard, matching the sibling idiom. That makes the issue's stated negative control true rather than nearly true.

Test wiring: confirmed. mas_backed_router builds through the production router(...), which nests the channel service at /channel; tool_names_on issues real initialize and tools/list over Router::oneshot. every_channel_tool_is_actually_advertised cannot pass by reading the const.

Instruction-injection surface: none added. The new text is static. Filenames go through build_params → attr_escape; captions go through content_sandbox::evaluate and land inside <matrix:message trust="external">.

Gates re-run after the guard on rustc 1.98.0: fmt, clippy -D warnings, 208 tests, all green.

The joined-room guard is asserted by code inspection and the sibling pattern, not by a test — the three tools that already have it have no test either, and one would need a Client with a populated state store. Worth its own issue if we want it covered.

Cross-engine review (Codex, read-only) on `git diff main...channel-download`. Three questions asked, one real finding. **Wrong claim in my own comment, now fixed in 4e063de.** I wrote next to `CHANNEL_TOOLS` that the tool "takes a `room_id` the identity must be joined to". False. `client.get_room()` forwards to `state_store.room()` (matrix-sdk-base 0.17 `client.rs:1037`), which is state-agnostic and returns invited, left, knocked and banned rooms. `download_attachment` had no `RoomState::Joined` guard while `read_thread` (`src/mcp.rs:2589`) and `room_info` (`src/mcp.rs:3258`) both have one, and it emitted `not joined to {room_id}` as the error for a case it never checked. Added the guard, matching the sibling idiom. That makes the issue's stated negative control true rather than nearly true. **Test wiring: confirmed.** `mas_backed_router` builds through the production `router(...)`, which nests the channel service at `/channel`; `tool_names_on` issues real `initialize` and `tools/list` over `Router::oneshot`. `every_channel_tool_is_actually_advertised` cannot pass by reading the const. **Instruction-injection surface: none added.** The new text is static. Filenames go through `build_params` → `attr_escape`; captions go through `content_sandbox::evaluate` and land inside `<matrix:message trust="external">`. Gates re-run after the guard on rustc 1.98.0: fmt, clippy `-D warnings`, 208 tests, all green. The joined-room guard is asserted by code inspection and the sibling pattern, not by a test — the three tools that already have it have no test either, and one would need a `Client` with a populated state store. Worth its own issue if we want it covered.
jlxq0 merged commit 501f1da32f into main 2026-08-25 07:11:40 +00:00
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels   
blocked
Held on a named, checkable blocker: an issue number, a credential or a measurement. Not "held".

  
bug
Something isn't working

  
documentation
Improvements or additions to documentation

  
duplicate
This issue or pull request already exists

  
enhancement
New feature or request

  
good first issue
Good for newcomers

  
help wanted
Extra attention is needed

  
invalid
This doesn't seem right

  
question
Further information is requested

  
waiting-on-julian
Blocked on Julian. The ask is in the issue.

  
wontfix
This will not be worked on

No labels
blocked
bug
documentation
duplicate
enhancement
good first issue
help wanted
invalid
question
waiting-on-julian
wontfix
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
jlxq0/matrix-mcp!113
No description provided.