Conversation
cpfiffer
left a comment
There was a problem hiding this comment.
Thanks for the work on this, @zaus0r -- Matrix support is a big addition. I went through the diff in detail and have a few things that need addressing before we can merge.
Blocking
1. getFormatterHints() not implemented -- TS compile error
ChannelAdapter requires this method (not optional). Every other adapter implements it. This will fail strict TypeScript compilation. Add it with appropriate hints for Matrix formatting capabilities.
2. onCommand signature mismatch -- commands can't route to correct room
matrix.ts:502 -- The callback is declared as (command: string) => Promise<string | null> but the interface requires (command: string, chatId?: string, args?: string). Without chatId, command responses can't route back to the correct Matrix room, and arguments (e.g. /model gpt-4) are silently dropped.
3. supportsEditing() hardcoded true with no config opt-out
All other adapters that support editing (Discord, Slack) make streaming opt-in via streaming?: boolean config (default false). This forces streaming edits unconditionally, and the PR's own docs note Cinny has "known bugs with E2EE message edits." Add the same config toggle pattern.
4. No group mode support
Every other adapter implements group modes (open | listen | mention-only | disabled) via isGroupAllowed, resolveGroupMode, wasMentioned/isListeningMode. The Matrix adapter has no groups config field, no gating, and applies DM access control to group rooms. A bot in any room with >2 members will process every message with no scoping.
5. Duplicate MatrixConfig with conflicting signatures
matrix.ts and config/types.ts both export MatrixConfig with different shapes (required vs optional fields, different dmPolicy types). Follow the existing pattern: define the type once in the adapter and import it in config/types.ts.
Suggestions
- Logging: Uses
console.*throughout instead ofcreateLogger('Matrix'). This bypasses structured logging (pino) and will fail CI's console lint check. - Pairing messages:
sendTextToRoomsends plain text, so**CODE123**renders as literal asterisks. UsesendHtmlTextlike thesendMessagepath does. markdownToHtml: Only handles bold/italic/code/linebreaks. The code comment itself says "For production, consider using a library like 'marked'." Should use a proper library.sendTypingIndicator: Throws on missing client and on network errors. Other adapters swallow typing errors since it's cosmetic.stopTypingIndicator: Not implemented but Matrix supportssetTyping(roomId, false).userHandle: Missing from theonMessagecall -- should be set to the raw Matrix ID.MATRIX_MESSAGE_PREFIX: Referenced in config but absent from.env.example.docs/MATRIX_INTEGRATION_COMPLETE.md: This is an internal dev checklist -- belongs in the PR description, not committed to the repo'sdocs/folder.
Nits
- Tab indentation in
matrix.tsandmatrix-login.ts-- project uses 2-space throughout - Unused
RustSdkCryptoStorageProviderimport inmatrix-login.ts GROUP_ID_HINTS['matrix']in setup.ts references group IDs but group modes aren't implementedchannels addhelp text incli.tsdoesn't listmatrix- Tchap URL detection (
url.includes("tchap.gouv.fr")) is duplicated per-message -- extract to a constructor-time getter
Solid foundation -- the config plumbing, onboard wizard, login helper, and env var wiring are all well done. The blockers are mostly about matching the patterns the other adapters follow. Happy to help if you have questions on any of these.
cpfiffer
left a comment
There was a problem hiding this comment.
Good foundation — E2EE support is valuable and the onboarding wizard integration is well done. A few things need fixing before this can merge.
Blockers
1. getFormatterHints() missing
The ChannelAdapter interface now requires getFormatterHints(): FormatterHints (added after this PR was opened). Merged against current main this will fail to compile. All other adapters implement it:
getFormatterHints(): FormatterHints {
return {
supportsReactions: true,
supportsFiles: true,
formatHint: 'Matrix supports **bold**, *italic*, `code` — HTML formatted',
};
}Also pass formatterHints in the onMessage call so the bot core uses Matrix-appropriate formatting.
2. Duplicate MatrixConfig with conflicting shapes
MatrixConfig is defined in both src/channels/matrix.ts and src/config/types.ts with different field optionality — the adapter requires homeserverUrl and accessToken (non-optional) but config/types.ts marks them optional and adds a required enabled field. The correct pattern is to define the interface once in the adapter file and import it in config/types.ts. See how DiscordConfig, SlackConfig, etc. are handled.
3. No group room access control
The adapter detects group rooms correctly (member count > 2) and passes isGroup/groupName through — that part is good. But there's no per-room gating. dmPolicy is applied to group rooms the same as DMs, so a bot in a public room processes every message from every user. Other adapters implement a groups config field with resolveGroupMode(), isGroupAllowed(), and mention-only filtering. Without this, deploying to a room with many users is effectively open access.
4. onCommand drops chatId and args
The property is typed as (command: string) => Promise<string | null> but the interface expects (command: string, chatId?: string, args?: string) => Promise<string | null>. TypeScript won't error on this (structural subtyping), but commands can't be routed to a specific room and arguments are silently dropped (e.g. /model gpt-4 loses gpt-4).
Major
supportsEditing() hardcoded true — this forces streaming on all Matrix clients. The PR docs note Cinny has known bugs with E2EE message edits. Add a streaming?: boolean config option (default false) like Discord and Slack do.
console.log/console.error/console.warn throughout — the project uses structured logging via createLogger. This will fail CI. Replace with:
import { createLogger } from '../logger.js';
const log = createLogger('Matrix');Pairing code renders as literal asterisks — sendTextToRoom() sends plain text, so **${code}** shows as **ABC123** not bold. Either use sendHtmlText() with <strong> or strip the markdown markers.
sendTypingIndicator throws on network errors — should be fire-and-forget like other adapters:
async sendTypingIndicator(chatId: string): Promise<void> {
if (!this.client) return;
try {
await this.client.setTyping(chatId, true, 5000);
} catch {
// Ignore — typing indicator is cosmetic
}
}Minor
docs/MATRIX_INTEGRATION_COMPLETE.mdis a dev checklist and shouldn't be in the docs folder. Either move to the PR description or delete it.markdownToHtml()— the inline comment already suggests usingmarked. Worth doing since code blocks with newlines won't render correctly with the current regex approach.- Branch needs a rebase onto current
main(several relevant files have changed).
What's working well
- E2EE via
RustSdkCryptoStorageProvideris solid - Onboarding wizard integration is clean
matrix-loginhelper is a nice touchdocs/matrix-setup.mdis thorough- Access control plumbing (pairing/allowlist/open) follows the existing pattern correctly
- Dynamic import pattern for the SDK dependency is correct
Happy to answer questions on any of the above. The group mode implementation is probably the most involved piece — worth looking at src/channels/discord.ts lines ~276–298 for the reference pattern.
|
Triage update: this PR is still blocked and has not changed since the requested-changes review (last commit is from Feb 27). Before we can re-review, please push an update that addresses the blockers called out in review:
Also please rebase on latest |
Summary