feat(cli): implement backend connections for CLI commands#148
Conversation
There was a problem hiding this comment.
Pull request overview
This PR implements backend connections for 14 CLI command stubs that previously returned hardcoded placeholder data. It introduces a CliContext struct that initializes and provides access to backend services (SessionManager, AgentRegistry, SimplePluginRegistry, and ToolRegistry), and converts the command dispatch to async to support calling backend APIs.
Changes:
- Introduced
CliContextstruct with SessionManager (file-based persistence) and in-memory registries for agents, plugins, and tools - Converted command dispatch to async with conditional context initialization
- Implemented real backend integrations for session, agent, plugin, and tool commands
Reviewed changes
Copilot reviewed 17 out of 18 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/mofa-cli/src/main.rs | Added async runtime, CliContext initialization, and updated command routing to async |
| crates/mofa-cli/src/context.rs | New file defining CliContext with backend service initialization |
| crates/mofa-cli/src/commands/agent/start.rs | Implemented agent starting with config loading and registry registration |
| crates/mofa-cli/src/commands/agent/stop.rs | Implemented graceful agent shutdown and registry unregistration |
| crates/mofa-cli/src/commands/agent/restart.rs | Implemented agent restart by calling stop then start |
| crates/mofa-cli/src/commands/agent/list.rs | Implemented agent listing from registry with filtering |
| crates/mofa-cli/src/commands/agent/status.rs | Implemented agent status retrieval from registry metadata |
| crates/mofa-cli/src/commands/plugin/list.rs | Implemented plugin listing from registry |
| crates/mofa-cli/src/commands/plugin/info.rs | Implemented plugin info retrieval from registry |
| crates/mofa-cli/src/commands/plugin/uninstall.rs | Implemented plugin uninstallation with confirmation |
| crates/mofa-cli/src/commands/tool/list.rs | Implemented tool listing from registry with source information |
| crates/mofa-cli/src/commands/tool/info.rs | Implemented tool info retrieval with metadata display |
| crates/mofa-cli/src/commands/session/list.rs | Implemented session listing with filtering and metadata |
| crates/mofa-cli/src/commands/session/show.rs | Implemented session detail display in multiple formats |
| crates/mofa-cli/src/commands/session/export.rs | Implemented session export to JSON/YAML files |
| crates/mofa-cli/src/commands/session/delete.rs | Implemented session deletion with confirmation prompt |
| crates/mofa-cli/Cargo.toml | Added dependencies for mofa-runtime, mofa-foundation, async-trait, and chrono |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let sessions_dir = data_dir.join("sessions"); | ||
| let session_manager = SessionManager::with_jsonl(&sessions_dir) |
There was a problem hiding this comment.
There's a path construction issue here. The context passes data_dir.join("sessions") as the workspace path, but JsonlSessionStorage::new in the SessionManager implementation also appends "sessions" to the workspace path (see mofa-foundation/src/agent/session.rs:137). This results in the actual sessions directory being ~/.local/share/mofa/sessions/sessions instead of the intended ~/.local/share/mofa/sessions. Either remove the "sessions" suffix here and pass just data_dir, or ensure that JsonlSessionStorage::new doesn't append "sessions" again.
| let sessions_dir = data_dir.join("sessions"); | |
| let session_manager = SessionManager::with_jsonl(&sessions_dir) | |
| let session_manager = SessionManager::with_jsonl(&data_dir) |
| ctx: &CliContext, | ||
| agent_id: &str, | ||
| config_path: Option<&std::path::Path>, | ||
| daemon: bool, |
There was a problem hiding this comment.
The daemon parameter is accepted but not implemented in the function body. It is only used for printing a message on line 17, but the actual daemon mode functionality (backgrounding the process, proper daemon setup, etc.) is not implemented. Consider either implementing daemon mode or removing this parameter until the feature is ready.
| println!(" Agent config stored for: {}", agent_config.name.cyan()); | ||
| } else { | ||
| // Try to create via factory | ||
| let type_id = factory_types.first().unwrap(); |
There was a problem hiding this comment.
Using unwrap() here is technically safe since factory_types.is_empty() was already checked in the if condition, but it's not idiomatic. Consider using pattern matching or a more explicit approach to avoid unwrap in production code. For example, you could use let type_id = &factory_types[0]; or restructure to avoid the unwrap entirely.
| let type_id = factory_types.first().unwrap(); | |
| let type_id = &factory_types[0]; |
| cli::AgentCommands::List { running, all } => { | ||
| commands::agent::list::run(running, all)?; | ||
| Some(Commands::Agent(agent_cmd)) => { | ||
| let ctx = ctx.as_ref().unwrap(); |
There was a problem hiding this comment.
Using unwrap() on ctx.as_ref() is technically safe here because the needs_context check on line 45 ensures that ctx is Some for all Agent commands. However, this pattern is not idiomatic and could be fragile if the logic changes in the future. Consider refactoring to avoid unwrap, perhaps by handling the context requirement more explicitly or using pattern matching that makes the safety more obvious to readers and the compiler.
| let ctx = ctx.as_ref().unwrap(); | |
| let Some(ctx) = ctx.as_ref() else { | |
| eprintln!( | |
| "Internal error: context is missing for Agent command. This should never happen." | |
| ); | |
| std::process::exit(1); | |
| }; |
| let mut sessions = Vec::new(); | ||
| for key in &keys { | ||
| let session = ctx.session_manager.get_or_create(key).await; | ||
|
|
||
| // Filter by agent_id if provided (check metadata or key prefix) | ||
| if let Some(agent) = agent_id { | ||
| let matches = session | ||
| .metadata | ||
| .get("agent_id") | ||
| .and_then(|v| v.as_str()) | ||
| .map(|v| v == agent) | ||
| .unwrap_or_else(|| session.key.contains(agent)); | ||
| if !matches { | ||
| continue; | ||
| } | ||
| } |
There was a problem hiding this comment.
Performance concern: This code loads all sessions from storage before filtering by agent_id. For large numbers of sessions, this could be inefficient as it requires deserializing every session. While the current Session structure doesn't allow filtering without loading the full session data, consider whether this approach will scale well. You might want to add a note or TODO about potential optimization strategies (e.g., maintaining an index of session metadata).
| pub async fn run(ctx: &CliContext, _available: bool, _enabled: bool) -> anyhow::Result<()> { | ||
| println!("{} Listing tools", "→".green()); |
There was a problem hiding this comment.
The available and enabled parameters are accepted via the CLI but not implemented in this function. The old placeholder code had filtering logic based on these flags. Consider either implementing the filtering functionality (if the ToolRegistry supports these concepts) or documenting why these parameters are currently ignored. Users may expect the --enabled flag to filter tools, which would be a regression from the previous behavior.
| pub async fn run(ctx: &CliContext, _available: bool, _enabled: bool) -> anyhow::Result<()> { | |
| println!("{} Listing tools", "→".green()); | |
| pub async fn run(ctx: &CliContext, available: bool, enabled: bool) -> anyhow::Result<()> { | |
| println!("{} Listing tools", "→".green()); | |
| if available || enabled { | |
| println!(" Note: --available/--enabled filtering is not yet implemented; listing all tools."); | |
| } |
|
|
||
| /// Execute the `mofa plugin list` command | ||
| pub fn run(installed_only: bool, available: bool) -> anyhow::Result<()> { | ||
| pub async fn run(ctx: &CliContext, _installed_only: bool, _available: bool) -> anyhow::Result<()> { |
There was a problem hiding this comment.
The installed_only and available parameters are accepted via the CLI but not implemented in this function. The old placeholder code had filtering logic based on the installed flag. Consider either implementing the filtering functionality (if the PluginRegistry supports tracking installation state) or documenting why these parameters are currently ignored. Users may expect the --installed flag to filter plugins, which would be a regression from the previous behavior.
| agent_registry: AgentRegistry::new(), | ||
| plugin_registry: Arc::new(SimplePluginRegistry::new()), | ||
| tool_registry: ToolRegistry::new(), |
There was a problem hiding this comment.
The agent_registry, plugin_registry, and tool_registry are all in-memory and will not persist across CLI invocations. This means that agents started with mofa agent start will not be visible when running mofa agent list in a separate CLI invocation, and similarly for plugins and tools. This significantly limits the usefulness of these commands. Consider either: 1) implementing persistence for these registries (similar to session_manager), 2) clearly documenting this limitation in user-facing documentation, or 3) rethinking the agent lifecycle management approach for the CLI context.
Closes #146
The CLI had 14 command stubs that returned hardcoded example data instead of talking to the actual backend. This PR replaces all of them with real integrations.
Added a CliContext struct that initializes SessionManager, AgentRegistry, SimplePluginRegistry, and ToolRegistry. Made the command dispatch async so everything can call the backend APIs properly.
Session commands now use SessionManager with file based persistence under ~/.local/share/mofa/sessions. The delete command also has a proper confirmation prompt now.
Agent commands use the AgentRegistry from mofa-runtime for lifecycle management. Start loads config via ConfigLoader, stop does graceful shutdown, restart calls stop then start, and list/status query the registry.
Plugin and tool commands query their respective registries and display real metadata instead of fake placeholder data.
All 16 CLI tests pass and the build compiles cleanly.