Replace built-in messenger system with chat-system crate#159
Conversation
|
@copilot The Cargo.lock file isn't even supposed to be in the repo. Can we just get rid of it to resolve the conflict? |
|
@copilot Can you fix the resolve conflict? |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
There was a problem hiding this comment.
Devin Review found 1 potential issue.
⚠️ 1 issue in files not directly in the diff
⚠️ recommended_strategy uses stale messenger_type strings after chat-system migration (crates/rustyclaw-core/src/messengers/streaming.rs:273)
The recommended_strategy function in streaming.rs:273 matches on the old "teams" and "google_chat" strings, but the new chat-system implementations return "msteams" and "googlechat" from messenger_type() respectively (see vendor/chat-system/src/messengers/teams.rs:243 and vendor/chat-system/src/messengers/google_chat.rs:239). Any caller passing the new messenger_type() value for Teams or Google Chat will fall through to the default BufferAll strategy instead of the intended EditInPlace strategy. These match arms are now dead code.
View 5 additional findings in Devin Review.
|
@copilot Can you fix the branch conflict? |
| .context("Matrix requires 'user_id'")?; | ||
| let password = config.password.clone(); | ||
| let access_token = config.access_token.clone(); | ||
|
|
||
| // Store path for Matrix SQLite database | ||
| let store_path = dirs::data_dir() | ||
| .context("Failed to get data directory")? | ||
| .join("rustyclaw") | ||
| .join("matrix") | ||
| .join(&name); | ||
|
|
||
| let messenger = if let Some(pwd) = password { | ||
| MatrixMessenger::with_password(name.clone(), homeserver, user_id, pwd, store_path) | ||
| } else if let Some(token) = access_token { | ||
| // Device ID is optional, defaults to "RUSTYCLAW" if not provided | ||
| MatrixMessenger::with_token( | ||
| name.clone(), | ||
| homeserver, | ||
| user_id, | ||
| token, | ||
| None, | ||
| store_path, | ||
| ) | ||
| } else { | ||
| anyhow::bail!("Matrix requires either 'password' or 'access_token'"); | ||
| }; | ||
| Box::new(messenger) | ||
| let password = config | ||
| .password | ||
| .clone() | ||
| .context("Matrix requires 'password'")?; | ||
| Box::new(MatrixMessenger::new(name, homeserver, user_id, password)) |
There was a problem hiding this comment.
🔴 Matrix access_token authentication silently removed, breaking existing configurations
The migration to chat-system's MatrixMessenger removed support for access-token-based Matrix authentication. Previously, users could configure Matrix with either password or access_token (crates/rustyclaw-core/src/messengers/matrix.rs:135-167 in the deleted file). The new code unconditionally requires password and will fail with "Matrix requires 'password'" for any configuration that used access_token instead. The new MatrixMessenger in vendor/chat-system/src/messengers/matrix.rs:243-282 only supports m.login.password. Users who relied on token-based auth (common for bots with pre-generated tokens) will be broken with no migration path within the matrix feature.
Prompt for agents
The Matrix messenger creation in create_messenger() (messenger_handler.rs around lines 184-199) now requires password and only calls MatrixMessenger::new(name, homeserver, user_id, password). Previously, the code checked for password first, then fell back to access_token, and failed only if neither was provided.
The root cause is that vendor/chat-system/src/messengers/matrix.rs only implements m.login.password authentication. To fix this, either:
1. Add access_token/session-restore support to the vendored chat-system MatrixMessenger (a with_token constructor or similar), OR
2. Restore the old branching logic in create_messenger() that checks for password first and access_token second, and create an appropriate messenger for each case.
The old code path for access_token used matrix-sdk's session restore with MatrixSession containing the token, device_id, and user_id. The new chat-system MatrixMessenger would need an equivalent path that sets the access_token directly and skips the login call in initialize().
Was this helpful? React with 👍 or 👎 to provide feedback.
Done in cfa66f3. Merged
Fixed in cfa66f3 — merged |
| let mut messenger = SignalCliMessenger::new(name, phone); | ||
| // signal-cli path can be overridden via config or just use system default | ||
| Box::new(messenger) |
There was a problem hiding this comment.
🟡 Unused mut on SignalCliMessenger variable generates a compiler warning, violating CONTRIBUTING.md rule
let mut messenger = SignalCliMessenger::new(name, phone); declares the variable as mutable, but messenger is never mutated — it is immediately moved into Box::new(messenger). This produces an unused_mut warning. The CONTRIBUTING.md states: "No PR should be considered finished while warnings are still present. Warnings slow down compilation, make CI noisier, and often indicate real issues."
| let mut messenger = SignalCliMessenger::new(name, phone); | |
| // signal-cli path can be overridden via config or just use system default | |
| Box::new(messenger) | |
| let messenger = SignalCliMessenger::new(name, phone); | |
| // signal-cli path can be overridden via config or just use system default | |
| Box::new(messenger) |
Was this helpful? React with 👍 or 👎 to provide feedback.
|
@copilot fix the conflicts preventing rebase. |
|
@copilot : It's still saying it can't be merged due to conflicts. |
Fixed in a039436. The "can't be merged" was caused by a CI build failure — the
The branch conflict has been resolved — the branch is now fully up to date with Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
|
@copilot: it still says This branch cannot be rebased due to conflicts |
| if let (Some(token), Some(space_id)) = | ||
| (&config.token, config.spaces.first()) | ||
| { | ||
| Box::new(GoogleChatMessenger::new_api(name, token, space_id)) |
There was a problem hiding this comment.
🔴 Google Chat spaces config values get double-prefixed, breaking API calls
The old GoogleChatMessenger documented spaces entries as full resource names like "spaces/AAAA". The new create_messenger passes config.spaces.first() directly to GoogleChatMessenger::new_api as the space_id. However, the chat-system's GoogleChatMessenger internally prepends spaces/ in its helper methods (vendor/chat-system/src/messengers/google_chat.rs:140-145), producing URLs like spaces/spaces/AAAA — a double-prefixed path that will 404 against the Google Chat API.
Path construction in chat-system
vendor/chat-system/src/messengers/google_chat.rs:140:
fn space_path(space_id: &str) -> String {
format!("spaces/{space_id}")
}So when the config has spaces = ["spaces/AAAA"], the API URL becomes https://chat.googleapis.com/v1/spaces/spaces/AAAA.
Prompt for agents
The config.spaces entries may contain full resource names like "spaces/AAAA" (the old documented format) or bare IDs like "AAAA". The chat-system GoogleChatMessenger::new_api stores the value as-is in space_id and then wraps it with format!("spaces/{space_id}") internally. This produces double-prefixed paths for existing configs.
Two possible fixes:
1. In create_messenger (messenger_handler.rs around line 152), strip any leading "spaces/" prefix before passing to new_api: space_id.strip_prefix("spaces/").unwrap_or(space_id)
2. Update the chat-system GoogleChatMessenger to handle both formats by checking if space_id already starts with "spaces/" in its helper methods.
Option 1 is simpler and keeps the fix local.
Was this helpful? React with 👍 or 👎 to provide feedback.
The "Rebase and merge" conflict is a history problem, not a code conflict. The branch contains two old merge commits that pull in earlier commits ( Two easier options that work today:
If you specifically need "Rebase and merge", you can clean up the branch history locally with a force-push: git fetch origin
git checkout -b temp-clean origin/main
git cherry-pick ef390fc 08b2ffd a039436
git push --force-with-lease origin temp-clean:copilot/remove-built-in-messenger-support
git branch -D temp-cleanThose three cherry-picks apply cleanly onto |
- Remove 12 messenger implementation files replaced by chat-system crate - Rewrite messengers/mod.rs to re-export types from chat-system - Update CLI messenger files (matrix_cli, telegram_cli, discord_cli, slack_cli) - Update gateway/messenger_handler.rs: use MessengerManager::add_boxed(), messengers() - Update lib.rs re-exports to include PresenceStatus from chat-system - Add chat-system git dependency to rustyclaw-core/Cargo.toml Co-authored-by: rexlunae <6726134+rexlunae@users.noreply.github.com>
…ace rusqlite 0.37 - Add vendor/chat-system with rusqlite bumped from 0.32 to 0.37 - Use query_row for PRAGMA journal_mode=WAL (returns a result row) - Log warning on WAL checkpoint failure instead of propagating error Co-authored-by: rexlunae <6726134+rexlunae@users.noreply.github.com>
…r init The 'Device flow improvements' commit (8ce42e8) added a listen_addr field to ssh::SshConfig. A second SSH server initialization block (from the earlier SSH transport PR merge) was constructing SshConfig without this field, causing: error[E0063]: missing field 'listen_addr' in initializer of 'ssh::SshConfig' Fix: parse the bind address before the SshConfig struct and include it as listen_addr. Remove the now-redundant bind_addr declaration inside the match arm. Co-authored-by: rexlunae <6726134+rexlunae@users.noreply.github.com>
rexlunae
force-pushed
the
copilot/remove-built-in-messenger-support
branch
from
a039436 to
10dbda1
Compare
- Remove 12 messenger implementation files replaced by chat-system crate - Rewrite messengers/mod.rs to re-export types from chat-system - Update CLI messenger files (matrix_cli, telegram_cli, discord_cli, slack_cli) - Update gateway/messenger_handler.rs: use MessengerManager::add_boxed(), messengers() - Update lib.rs re-exports to include PresenceStatus from chat-system - Add chat-system git dependency to rustyclaw-core/Cargo.toml Co-authored-by: rexlunae <6726134+rexlunae@users.noreply.github.com>
…ace rusqlite 0.37 - Add vendor/chat-system with rusqlite bumped from 0.32 to 0.37 - Use query_row for PRAGMA journal_mode=WAL (returns a result row) - Log warning on WAL checkpoint failure instead of propagating error Co-authored-by: rexlunae <6726134+rexlunae@users.noreply.github.com>
chat-systemgit dependency; update feature flags (matrix,whatsapp,signal-cli) to forward to chat-system; keep workspace rusqlite at 0.37chat-system; keep CLI messenger modules andmedia,streaming,group_chat(pending chat-system PRs)chat_systemtypes, addreactions: NonetoMessage { }constructionsPresenceStatusfromchat-systemMessengerManagerAPI calls (add_boxed,messengers()); addget_messenger_by_typehelper; update Teams/GoogleChat/IMessage/Matrix/Signal/WhatsApp constructors to match chat-system API[patch."https://github.com/rexlunae/chat-system"]to redirect the git dependencyquery_rowforPRAGMA journal_mode=WAL; log warning on WAL checkpoint failure.gitignore8ce42e8(Device flow improvements)listen_addrfield tossh::SshConfiginitializer ingateway/mod.rs:511; the8ce42e8"Device flow improvements" commit addedlisten_addrtoSshConfigbut a second SSH init block (from the SSH transport PR merge) was missing it, causingerror[E0063]: missing field listen_addr