refactor(cli): extract service layer for MCP reuse#34
Conversation
Prepares for the MCP server by moving business logic out of command handlers into service/reads and service/writes, with CLI commands as thin wrappers. No user-facing behavior change. Co-authored-by: Cursor <cursoragent@cursor.com>
Unblocks cargo-deny CI after unsoundness advisory on 1.0.102. Co-authored-by: Cursor <cursoragent@cursor.com>
MaximeGaudin
left a comment
There was a problem hiding this comment.
PR Review — #34: refactor(cli): extract service layer for MCP reuse
Recommendation: Request changes
CI: ✅ all green (Linux/macOS/Windows, MSRV, cargo-deny, coverage, format)
1. Intent & fit
Fits — Well-motivated architectural refactoring. Clean separation of concerns, CHANGELOG updated, conventional commits, focused on one logical change.
2. Security · Verdict: clean ✅
- Only dep change: anyhow 1.0.102 → 1.0.103 (existing dep, version bump for RUSTSEC-2026-0190). No new crates.
- No
.github/workflow changes, no malicious patterns, no new filesystem/network access.
3. Code review
Blocker 🚫
reads.rs uses the wrong parse_date_to_ts for messages — timezone regression
service/reads.rs imports parse_date_to_ts from crate::commands::calendar::parsing, which interprets dates as local midnight (.and_local_timezone(Local)):
// calendar/parsing.rs — uses LOCAL timezone
pub(crate) fn parse_date_to_ts(date: &str) -> Option<i64> {
chrono::NaiveDate::parse_from_str(date, "%Y-%m-%d")
.ok()
.and_then(|d| d.and_hms_opt(0, 0, 0))
.and_then(|dt| dt.and_local_timezone(Local).single())
.map(|dt| dt.timestamp())
}But the original commands/messages.rs had its own version that used UTC midnight (.and_utc()):
// original messages.rs — used UTC
fn parse_date_to_ts(date: &str) -> Option<i64> {
chrono::NaiveDate::parse_from_str(date, "%Y-%m-%d")
.ok()
.and_then(|d| d.and_hms_opt(0, 0, 0))
.map(|dt| dt.and_utc().timestamp())
}This changes the behavior of void messages --since/--until — dates are now offset by the user's timezone. For a user in UTC+2, --since 2024-03-15 now means "since 2024-03-14 22:00:00 UTC" instead of "since 2024-03-15 00:00:00 UTC".
Fix: Either add a parse_date_to_ts_utc variant and import it in reads.rs for the messages path, or decide that local-time is the correct interpretation and note the intentional behavior change in CHANGELOG.
Should-fix
-
Duplicated
parse_date_to_tsinwrites.rs:492— defines a private UTC copy instead of sharing from a common location. With the fix above, extract both variants (_utcand_local) into one module. -
cleanup_cached_filessilently swallows all errors (writes.rs:511:let _ = std::fs::remove_file(path)). The original code loggedwarn!for non-NotFound errors. This makes file-cleanup failures invisible.
Nits
archive_bulk_beforeisasyncbut awaits nothing — could be sync.- Reply/send CLI handlers re-parse
at_strafter the service function already consumed it (harmless redundancy for the eprintln timestamp).
4. Test coverage · adequate ✅
Unit tests cover the new public APIs (envelope shapes, resolve_send_target, archive validation). Existing integration tests (read_paths.rs, cli_contract.rs) exercise the full path through the service layer. Adequate for a refactoring PR — though notably, no existing test covers --since/--until filtering, which is how the timezone regression slipped through.
Summary
Solid refactoring with a clear rationale and clean architecture. One blocker: the timezone semantic mismatch in parse_date_to_ts silently regresses void messages --since/--until. Fix that, address the error-swallowing in cleanup_cached_files, and this is merge-ready.
Split parse_date_to_ts into explicit local (calendar) and UTC (messages, archive --before) helpers. Restore debug/warn logging in cleanup_cached_files. Add regression tests for both semantics. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Thanks for the thorough review — agreed on all blocking/should-fix items. Blocker (timezone): Fixed in 5442b71. Should-fix (duplicated helper): Removed the private UTC copy in Should-fix ( Tests: Added regression tests for UTC vs local midnight semantics in Nits (async bulk archive, send/reply CHANGELOG updated under |
Intent
Void is gaining an MCP server so AI agents can read the inbox, search messages, send replies, and perform other void operations over stdio. Those tools need the same business logic the CLI already implements — but today that logic lives inside command handlers, tightly coupled to clap argument parsing and terminal output.
This PR is the foundation: it extracts read and write operations into a shared service layer that both the CLI and the upcoming MCP server can call. The CLI keeps its existing flags and output format; command handlers become thin wrappers that parse args, call the service, and print the result.
No user-facing behavior change in this PR. The goal is a single source of truth for void operations before wiring up MCP tools in follow-up PRs.
What changed
crates/void-cli/src/service/with:reads.rs— inbox, messages, search, contacts, channels, calendar, slack saved, etc.writes.rs— send, reply, forward, archive, mutemod.rs— shared JSON rendering helperrun()functions to delegate to the service layerCHANGELOG.mdunder[Unreleased]anyhowto 1.0.103 (RUSTSEC-2026-0190) to unblockcargo-denyCIArchitecture
Command handlers own how (args, stdout). The service layer owns what (queries, mutations, JSON envelopes).
Deferred to follow-up PRs
void mcpsubcommand and rmcp/schemars dependenciesruntool,triage_inboxpromptTest plan
./scripts/check.shpasses (fmt, clippy, tests)CHANGELOG.mdupdated