refactor: encapsulate leaked abstractions into owning modules - #778
Conversation
…o owning modules Move module-specific initialization logic out of main.rs (1222→665 lines, -46%) and app.rs (944→780 lines, -17%) into their respective owning modules as public factory functions. This enforces separation of concerns so that adding a new DB backend, MCP transport, or channel doesn't require editing main.rs/app.rs. Key changes: - Tracing init functions → src/tracing_fmt.rs - DB connection factory (connect_with_handles + DatabaseHandles) → src/db/mod.rs - Secrets store factory (create_secrets_store) → src/secrets/mod.rs - MCP transport dispatch factory (create_client_from_config) → src/tools/mcp/factory.rs - Orchestrator setup (setup_orchestrator + OrchestratorSetup) → src/orchestrator/mod.rs - WASM channel setup (setup_wasm_channels) → src/channels/wasm/setup.rs - Worker entry points (run_worker, run_claude_bridge) → src/worker/mod.rs - Shared CLI secrets init (init_secrets_store) → src/cli/mod.rs - Tunnel startup (start_managed_tunnel) → src/tunnel/mod.rs - Onboard check (check_onboard_needed) → src/setup/mod.rs - ExtensionManager unified MCP: uses create_client_from_config via McpProcessManager, enabling stdio/Unix transports for hot-activated MCP servers - Deduplicated ~130 lines of secrets store init across cli/mcp.rs and cli/tool.rs - CLAUDE.md updated with module-owned initialization guideline [skip-regression-check] Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
github-actions
Bot
added
scope: channel/cli
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly refactors the application's initialization process by moving module-specific setup logic into dedicated factory functions within their respective modules. This change aims to improve code organization, reduce the complexity of core application files, and make it easier to add new components (like database backends or MCP transports) without modifying central entry points. The refactoring also unifies MCP client creation and streamlines secrets store initialization across various CLI commands. Highlights
Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request is a significant and well-executed refactoring that successfully encapsulates module-specific initialization logic into their respective modules, primarily by introducing factory functions. This greatly improves the structure of the codebase, making main.rs and app.rs much cleaner and more focused on orchestration. The deduplication of secrets store initialization is also a welcome improvement.
My review includes a suggestion for further refactoring in one of the newly created setup functions to enhance its readability and maintainability by breaking it down into smaller, more focused helpers. Overall, this is an excellent change that improves the long-term health of the codebase.
| pub async fn setup_wasm_channels( | ||
| config: &Config, | ||
| secrets_store: &Option<Arc<dyn SecretsStore + Send + Sync>>, | ||
| extension_manager: Option<&Arc<ExtensionManager>>, | ||
| database: Option<&Arc<dyn Database>>, | ||
| ) -> Option<WasmChannelSetup> { | ||
| let runtime = match WasmChannelRuntime::new(WasmChannelRuntimeConfig::default()) { | ||
| Ok(r) => Arc::new(r), | ||
| Err(e) => { | ||
| tracing::warn!("Failed to initialize WASM channel runtime: {}", e); | ||
| return None; | ||
| } | ||
| }; | ||
|
|
||
| let pairing_store = Arc::new(PairingStore::new()); | ||
| let settings_store: Option<Arc<dyn crate::db::SettingsStore>> = | ||
| database.map(|db| Arc::clone(db) as Arc<dyn crate::db::SettingsStore>); | ||
| let mut loader = WasmChannelLoader::new( | ||
| Arc::clone(&runtime), | ||
| Arc::clone(&pairing_store), | ||
| settings_store, | ||
| ); | ||
| if let Some(secrets) = secrets_store { | ||
| loader = loader.with_secrets_store(Arc::clone(secrets)); | ||
| } | ||
|
|
||
| let results = match loader | ||
| .load_from_dir(&config.channels.wasm_channels_dir) | ||
| .await | ||
| { | ||
| Ok(r) => r, | ||
| Err(e) => { | ||
| tracing::warn!("Failed to scan WASM channels directory: {}", e); | ||
| return None; | ||
| } | ||
| }; | ||
|
|
||
| let wasm_router = Arc::new(WasmChannelRouter::new()); | ||
| let mut channels: Vec<(String, Box<dyn crate::channels::Channel>)> = Vec::new(); | ||
| let mut channel_names: Vec<String> = Vec::new(); | ||
|
|
||
| for loaded in results.loaded { | ||
| let channel_name = loaded.name().to_string(); | ||
| channel_names.push(channel_name.clone()); | ||
| tracing::info!("Loaded WASM channel: {}", channel_name); | ||
|
|
||
| let secret_name = loaded.webhook_secret_name(); | ||
| let sig_key_secret_name = loaded.signature_key_secret_name(); | ||
| let hmac_secret_name = loaded.hmac_secret_name(); | ||
|
|
||
| let webhook_secret = if let Some(secrets) = secrets_store { | ||
| secrets | ||
| .get_decrypted("default", &secret_name) | ||
| .await | ||
| .ok() | ||
| .map(|s| s.expose().to_string()) | ||
| } else { | ||
| None | ||
| }; | ||
|
|
||
| let secret_header = loaded.webhook_secret_header().map(|s| s.to_string()); | ||
|
|
||
| let webhook_path = format!("/webhook/{}", channel_name); | ||
| let endpoints = vec![RegisteredEndpoint { | ||
| channel_name: channel_name.clone(), | ||
| path: webhook_path, | ||
| methods: vec!["POST".to_string()], | ||
| require_secret: webhook_secret.is_some(), | ||
| }]; | ||
|
|
||
| let channel_arc = Arc::new(loaded.channel); | ||
|
|
||
| { | ||
| let mut config_updates = std::collections::HashMap::new(); | ||
|
|
||
| if let Some(ref tunnel_url) = config.tunnel.public_url { | ||
| config_updates.insert( | ||
| "tunnel_url".to_string(), | ||
| serde_json::Value::String(tunnel_url.clone()), | ||
| ); | ||
| } | ||
|
|
||
| if let Some(ref secret) = webhook_secret { | ||
| config_updates.insert( | ||
| "webhook_secret".to_string(), | ||
| serde_json::Value::String(secret.clone()), | ||
| ); | ||
| } | ||
|
|
||
| // Inject owner_id if configured for this channel. | ||
| if let Some(&owner_id) = config | ||
| .channels | ||
| .wasm_channel_owner_ids | ||
| .get(channel_name.as_str()) | ||
| { | ||
| config_updates.insert("owner_id".to_string(), serde_json::json!(owner_id)); | ||
| } | ||
|
|
||
| if !config_updates.is_empty() { | ||
| channel_arc.update_config(config_updates).await; | ||
| tracing::info!( | ||
| channel = %channel_name, | ||
| has_tunnel = config.tunnel.public_url.is_some(), | ||
| has_webhook_secret = webhook_secret.is_some(), | ||
| "Injected runtime config into channel" | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| tracing::info!( | ||
| channel = %channel_name, | ||
| has_webhook_secret = webhook_secret.is_some(), | ||
| secret_header = ?secret_header, | ||
| "Registering channel with router" | ||
| ); | ||
|
|
||
| wasm_router | ||
| .register( | ||
| Arc::clone(&channel_arc), | ||
| endpoints, | ||
| webhook_secret.clone(), | ||
| secret_header, | ||
| ) | ||
| .await; | ||
|
|
||
| // Register Ed25519 signature key if declared in capabilities | ||
| if let Some(ref sig_key_name) = sig_key_secret_name | ||
| && let Some(secrets) = secrets_store | ||
| && let Ok(key_secret) = secrets.get_decrypted("default", sig_key_name).await | ||
| { | ||
| match wasm_router | ||
| .register_signature_key(&channel_name, key_secret.expose()) | ||
| .await | ||
| { | ||
| Ok(()) => { | ||
| tracing::info!(channel = %channel_name, "Registered Ed25519 signature key") | ||
| } | ||
| Err(e) => { | ||
| tracing::error!(channel = %channel_name, error = %e, "Invalid signature key in secrets store") | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Register HMAC signing secret if declared in capabilities | ||
| if let Some(ref hmac_secret_name) = hmac_secret_name | ||
| && let Some(secrets) = secrets_store | ||
| && let Ok(secret) = secrets.get_decrypted("default", hmac_secret_name).await | ||
| { | ||
| wasm_router | ||
| .register_hmac_secret(&channel_name, secret.expose()) | ||
| .await; | ||
| tracing::info!(channel = %channel_name, "Registered HMAC signing secret"); | ||
| } | ||
|
|
||
| if let Some(secrets) = secrets_store { | ||
| match inject_channel_credentials(&channel_arc, secrets.as_ref(), &channel_name).await { | ||
| Ok(count) => { | ||
| if count > 0 { | ||
| tracing::info!( | ||
| channel = %channel_name, | ||
| credentials_injected = count, | ||
| "Channel credentials injected" | ||
| ); | ||
| } | ||
| } | ||
| Err(e) => { | ||
| tracing::error!( | ||
| channel = %channel_name, | ||
| error = %e, | ||
| "Failed to inject channel credentials" | ||
| ); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| channels.push((channel_name, Box::new(SharedWasmChannel::new(channel_arc)))); | ||
| } | ||
|
|
||
| for (path, err) in &results.errors { | ||
| tracing::warn!("Failed to load WASM channel {}: {}", path.display(), err); | ||
| } | ||
|
|
||
| // Always create webhook routes (even with no channels loaded) so that | ||
| // channels hot-added at runtime can receive webhooks without a restart. | ||
| let webhook_routes = { | ||
| Some(create_wasm_channel_router( | ||
| Arc::clone(&wasm_router), | ||
| extension_manager.map(Arc::clone), | ||
| )) | ||
| }; | ||
|
|
||
| Some(WasmChannelSetup { | ||
| channels, | ||
| channel_names, | ||
| webhook_routes, | ||
| wasm_channel_runtime: runtime, | ||
| pairing_store, | ||
| wasm_channel_router: wasm_router, | ||
| }) | ||
| } |
There was a problem hiding this comment.
The setup_wasm_channels function is quite long and handles multiple responsibilities within its main loop. To improve readability, maintainability, and testability, consider extracting the logic inside the for loaded in results.loaded loop into a separate helper function. This new function, say process_loaded_channel, would encapsulate the setup for a single WASM channel, including secret retrieval, config injection, router registration, and credential injection.
There was a problem hiding this comment.
Already addressed — the loop body was extracted into register_channel() (line 103) in a prior commit per this feedback.
There was a problem hiding this comment.
Pull request overview
This PR refactors the codebase to move module-specific initialization logic out of main.rs and app.rs into owning modules as public factory functions. This follows a new "module-owned initialization" guideline to reduce the coupling between entry-point files and individual modules, making it easier to add new backends, transports, or channels without editing the main entry points.
Changes:
- Introduces new factory functions (
db::connect_with_handles(),secrets::create_secrets_store(),mcp::create_client_from_config(),orchestrator::setup_orchestrator(),tunnel::start_managed_tunnel(), etc.) that encapsulate initialization logic previously scattered inmain.rsandapp.rs. - Adds
McpProcessManagertoExtensionManager, enabling stdio/Unix MCP transport support for hot-activated MCP servers (previously HTTP-only). - Deduplicates ~130 lines of secrets store initialization across CLI subcommands and moves tracing initialization, worker/bridge entry points, and WASM channel setup into their owning modules.
Reviewed changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
src/db/mod.rs |
New DatabaseHandles struct and connect_with_handles() factory |
src/secrets/mod.rs |
New create_secrets_store() factory using DatabaseHandles |
src/tools/mcp/factory.rs |
New file: transport dispatch factory for MCP client creation |
src/tools/mcp/mod.rs |
Exports new factory module |
src/orchestrator/mod.rs |
New OrchestratorSetup struct and setup_orchestrator() factory |
src/tunnel/mod.rs |
New start_managed_tunnel() factory |
src/channels/wasm/setup.rs |
New file: WASM channel setup and credential injection moved from main.rs |
src/channels/wasm/mod.rs |
Exports new setup module |
src/worker/mod.rs |
New run_worker() and run_claude_bridge() entry points |
src/tracing_fmt.rs |
New init_cli_tracing() and init_worker_tracing() functions |
src/setup/mod.rs |
New check_onboard_needed() function |
src/cli/mod.rs |
New shared init_secrets_store() and run_memory_command() |
src/cli/mcp.rs |
Simplified secrets init to delegate to cli::init_secrets_store() |
src/cli/tool.rs |
Simplified secrets init to delegate to cli::init_secrets_store() |
src/extensions/manager.rs |
Added McpProcessManager parameter; uses shared MCP factory |
src/app.rs |
Simplified DB, secrets, and MCP init using new factories |
src/main.rs |
Simplified by delegating to module-owned factories |
src/channels/web/server.rs |
Test updates for new McpProcessManager parameter |
src/tools/builtin/extension_tools.rs |
Test updates for new McpProcessManager parameter |
CLAUDE.md |
Documents module-owned initialization guideline |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| pub async fn connect_with_handles( | ||
| config: &crate::config::DatabaseConfig, | ||
| ) -> Result<(Arc<dyn Database>, DatabaseHandles), DatabaseError> { | ||
| let mut handles = DatabaseHandles::default(); | ||
|
|
||
| match config.backend { | ||
| #[cfg(feature = "libsql")] | ||
| crate::config::DatabaseBackend::LibSql => { | ||
| use secrecy::ExposeSecret as _; | ||
|
|
||
| let default_path = crate::config::default_libsql_path(); | ||
| let db_path = config.libsql_path.as_deref().unwrap_or(&default_path); | ||
|
|
||
| let backend = if let Some(ref url) = config.libsql_url { | ||
| let token = config.libsql_auth_token.as_ref().ok_or_else(|| { | ||
| DatabaseError::Pool( | ||
| "LIBSQL_AUTH_TOKEN required when LIBSQL_URL is set".to_string(), | ||
| ) | ||
| })?; | ||
| libsql::LibSqlBackend::new_remote_replica(db_path, url, token.expose_secret()) | ||
| .await | ||
| .map_err(|e| DatabaseError::Pool(e.to_string()))? | ||
| } else { | ||
| libsql::LibSqlBackend::new_local(db_path) | ||
| .await | ||
| .map_err(|e| DatabaseError::Pool(e.to_string()))? | ||
| }; | ||
| backend.run_migrations().await?; | ||
| tracing::info!("libSQL database connected and migrations applied"); | ||
|
|
||
| handles.libsql_db = Some(backend.shared_db()); | ||
|
|
||
| Ok((Arc::new(backend) as Arc<dyn Database>, handles)) | ||
| } | ||
| #[cfg(feature = "postgres")] | ||
| _ => { | ||
| let pg = postgres::PgBackend::new(config) | ||
| .await | ||
| .map_err(|e| DatabaseError::Pool(e.to_string()))?; | ||
| pg.run_migrations().await?; | ||
| tracing::info!("PostgreSQL database connected and migrations applied"); | ||
|
|
||
| handles.pg_pool = Some(pg.pool()); | ||
|
|
||
| Ok((Arc::new(pg) as Arc<dyn Database>, handles)) | ||
| } | ||
| #[cfg(not(feature = "postgres"))] | ||
| _ => Err(DatabaseError::Pool( | ||
| "No database backend available. Enable 'postgres' or 'libsql' feature.".to_string(), | ||
| )), | ||
| } | ||
| } |
There was a problem hiding this comment.
connect_with_handles duplicates almost all the logic of connect_from_config (~30 lines of backend matching, connection setup, and migration). Now that connect_with_handles exists, connect_from_config could be refactored to delegate to it (e.g., connect_from_config calls connect_with_handles and discards the handles). This would eliminate the duplicated feature-flag branching and backend initialization code, reducing the maintenance burden when a new backend is added.
There was a problem hiding this comment.
Already addressed — connect_from_config now delegates to connect_with_handles (line 54: let (db, _handles) = connect_with_handles(config).await?;).
…-init # Conflicts: # src/cli/mcp.rs # src/cli/tool.rs # src/db/mod.rs # src/main.rs # src/orchestrator/mod.rs
zmanian
left a comment
zmanian
left a comment
There was a problem hiding this comment.
Review
Large refactoring PR that moves initialization logic from `app.rs`/`main.rs` into owning modules. Adds a new CLAUDE.md principle: "Module-owned initialization."
Scope
- DB initialization: Extracted `connect_with_handles()` factory in `db/mod.rs`, replacing ~60 lines of feature-gated DB init in `app.rs`
- MCP client creation: Extracted `create_client_from_config()` factory in `tools/mcp/factory.rs`, replacing ~80 lines of transport dispatch in `app.rs`
- WASM channel setup: Extracted `setup_wasm_channels()` in `channels/wasm/setup.rs`, encapsulating channel loading, credential injection, and webhook route registration
- Secrets store creation: Extracted `create_secrets_store()` factory
- `DatabaseHandles`: New struct replacing scattered `pg_pool`/`libsql_db` fields on `AppBuilder`
- Webhook proxy: Added `/webhook/{*path}` reverse proxy on the gateway for tunnel support
Good
- The "module-owned initialization" principle is sound -- `app.rs` was becoming a dumping ground for feature-flag branching. Moving this to the modules that own the abstractions is cleaner.
- `DatabaseHandles` with `Default` impl is a nice way to carry backend-specific handles without feature-flag fields scattered across the builder.
- The webhook proxy implementation includes path traversal protection (`.." check) and proper error handling.
- Static `PROXY_CLIENT` via `OnceLock` for connection reuse is good.
Concerns
-
Webhook proxy path traversal check: `path.contains("..")` is a basic check. It would miss URL-encoded traversal (`%2e%2e`). Since Axum likely decodes the path before the handler runs, this might be sufficient, but worth verifying.
-
PR size: This is a significant refactor touching app.rs, main.rs, multiple module files, and adding new files. The changes are individually correct but the combination makes it hard to verify nothing was lost in translation. Would benefit from a before/after integration test ensuring all subsystems still wire up correctly.
-
`ExtensionManager::new` now takes `mcp_process_manager`: This is a new parameter added to the constructor. Verify all call sites are updated (including tests).
-
`webhook_proxy_addr: None` in all test GatewayState constructions: Consistent, but if someone adds a test for webhook proxy behavior, they'll need to set this manually.
The refactoring direction is good. Would be more comfortable with smaller, incremental PRs (e.g., DB factory first, then MCP factory, then WASM setup) to reduce risk.
…-init # Conflicts: # CLAUDE.md # src/main.rs
…hannel helper - connect_from_config() now delegates to connect_with_handles() to eliminate duplicated backend-matching logic (Copilot review feedback) - Extract register_channel() helper from setup_wasm_channels() loop body to improve readability (Gemini review feedback) [skip-regression-check] Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Addressed both review comments in 5ae43ca:
|
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 19 out of 19 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /// Initialize a secrets store from environment config. | ||
| /// | ||
| /// Shared helper for CLI subcommands (`mcp auth`, `tool auth`, etc.) that need | ||
| /// access to encrypted secrets without spinning up the full AppBuilder. | ||
| pub async fn init_secrets_store() | ||
| -> anyhow::Result<Arc<dyn crate::secrets::SecretsStore + Send + Sync>> { | ||
| let config = crate::config::Config::from_env().await?; | ||
| let master_key = config.secrets.master_key().ok_or_else(|| { | ||
| anyhow::anyhow!( | ||
| "SECRETS_MASTER_KEY not set. Run 'ironclaw onboard' first or set it in .env" | ||
| ) | ||
| })?; | ||
|
|
||
| let crypto = Arc::new(crate::secrets::SecretsCrypto::new(master_key.clone())?); | ||
|
|
||
| Ok(crate::db::create_secrets_store(&config.database, crypto).await?) | ||
| } |
There was a problem hiding this comment.
This init_secrets_store() function is introduced as a "Shared helper for CLI subcommands (mcp auth, tool auth, etc.)" but it is currently unused. The PR description claims "Deduplicated ~130 lines of secrets store initialization across cli/mcp.rs and cli/tool.rs", but neither file was updated: cli/mcp.rs::get_secrets_store() (line 630) and cli/tool.rs::init_secrets_store() (line 555) still contain their own duplicate implementations. This creates three copies of the same logic instead of the original two.
Either update cli/mcp.rs and cli/tool.rs to call this shared function (completing the claimed deduplication), or remove this dead code until it's ready to be wired up.
There was a problem hiding this comment.
Already addressed — both cli/tool.rs::init_secrets_store() and cli/mcp.rs::get_secrets_store() now delegate to crate::cli::init_secrets_store(). Fixed in a prior commit.
| #[allow(unused_imports)] | ||
| use crate::config::Config; |
There was a problem hiding this comment.
The #[allow(unused_imports)] annotation is incorrect here — Config IS used on line 556 (Config::from_env().await?). This suggests an incomplete refactoring: the local init_secrets_store() (lines 555–566) was likely intended to be replaced by the new shared crate::cli::init_secrets_store(), which would make Config truly unused. Either complete that refactoring or remove the #[allow(unused_imports)].
There was a problem hiding this comment.
Fixed in 3f18fe0 — removed both the #[allow(unused_imports)] and the unused Config import.
Exercises the full factory chain end-to-end to verify nothing was lost when initialization logic was moved from main.rs/app.rs into owning modules: - connect_with_handles returns Database + populated backend handles - connect_from_config delegates correctly (produces working Database) - secrets::create_secrets_store builds working store from DatabaseHandles - db::create_secrets_store standalone factory round-trips secrets - Both secrets factories produce compatible stores (cross-read works) - ExtensionManager constructs with McpProcessManager and is functional - DatabaseHandles default is empty All tests run without external services using libsql in-memory/tempfile. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Both files had inline implementations identical to cli::init_secrets_store(). Replace with delegation to complete the claimed deduplication. [skip-regression-check] Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 21 out of 21 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| #[allow(unused_imports)] | ||
| use crate::config::Config; |
There was a problem hiding this comment.
The Config import is now unused after moving the init_secrets_store logic to crate::cli::init_secrets_store(). Rather than suppressing the warning with #[allow(unused_imports)], the import should be removed entirely.
There was a problem hiding this comment.
Fixed in 3f18fe0 — removed the unused Config import entirely.
| ### Error Handling | ||
| - Use `thiserror` for error types in `error.rs` | ||
| - Never use `.unwrap()` or `.expect()` in production code (tests are fine) | ||
| - Map errors with context: `.map_err(|e| SomeError::Variant { reason: e.to_string() })?` | ||
| - Before committing, grep for `.unwrap()` and `.expect(` in changed files to catch violations mechanically |
There was a problem hiding this comment.
The newly added "Error Handling" subsection (lines 180-183) duplicates rules already stated in the "Code Style" section above (lines 21-23): thiserror, no .unwrap()/.expect(), and map_err with context. Consider removing the duplicate bullet points and keeping only the new guideline on line 184 ("Before committing, grep for…") here, or alternatively, moving all error-handling guidance to a single location.
There was a problem hiding this comment.
Fixed in 3f18fe0 — removed the duplicate Error Handling subsection. All four bullets already exist in the Code Style section (lines 21-23) and .claude/rules/review-discipline.md.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
zmanian
left a comment
zmanian
left a comment
There was a problem hiding this comment.
Re-review after commits 5ae43ca..16085d0.
Previous feedback addressed
-
DB factory deduplication --
connect_from_confignow delegates toconnect_with_handlesand discards the handles. Clean. -
Extract helper from
setup_wasm_channels--register_channel()extracted as a separate async fn. Addresses the readability concern. -
Dead
init_secrets_storein cli/mod.rs -- Bothcli/mcp.rs::get_secrets_store()andcli/tool.rs::init_secrets_store()now delegate to the sharedcrate::cli::init_secrets_store(). Deduplication is complete. -
Integration test --
tests/module_init_integration.rscoversconnect_with_handles,connect_from_config,create_secrets_storeround-trips, cross-factory compatibility,ExtensionManagerconstruction withMcpProcessManager, andDatabaseHandles::default(). Good coverage for the refactoring.
Remaining minor items (non-blocking)
-
#[allow(unused_imports)]onConfigincli/tool.rs-- Now thatinit_secrets_store()delegates to the shared helper,Configis genuinely unused. The#[allow(unused_imports)]should be replaced by removing the import entirely. This is a one-line cleanup. -
Duplicate Error Handling rules in CLAUDE.md -- The inline comment about duplicated
thiserror/unwrap/expectrules between the new "Error Handling" subsection and the existing "Code Style" section was not addressed. Minor docs issue. -
Webhook proxy URL-encoded traversal -- The
path.contains("..")check was noted in the original review as potentially insufficient for%2e%2eencoded paths. Not addressed, but Axum's path extractor decodes percent-encoding before the handler runs, so this is likely safe in practice.
The refactoring is solid. All substantive review feedback has been addressed, and the new integration tests provide confidence that the factory wiring is correct.
…ng section - Remove `#[allow(unused_imports)]` and unused `use crate::config::Config` from cli/tool.rs (no longer needed after delegating to shared `cli::init_secrets_store()`) - Remove duplicate Error Handling subsection from CLAUDE.md Key Patterns (all four bullets already exist in Code Style section and review-discipline.md) Addresses Copilot review comments. [skip-regression-check] Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 21 out of 21 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| pub fn create_secrets_store( | ||
| crypto: std::sync::Arc<SecretsCrypto>, | ||
| handles: &crate::db::DatabaseHandles, | ||
| ) -> Option<std::sync::Arc<dyn SecretsStore + Send + Sync>> { |
There was a problem hiding this comment.
The docstring says "Returns None if no matching backend handle is available," but the integration test at tests/module_init_integration.rs:98-99 calls .expect("create_secrets_store should return Some for libsql"), treating None as an error. Callers in app.rs also silently proceed on None. Consider returning a Result instead of Option to distinguish "no backend configured" (which may be a legitimate error in some call sites) from actual failures, or at minimum ensure the doc and call sites agree on whether None is an error condition.
There was a problem hiding this comment.
Fixed in 951b148 — expanded the docstring to clarify that None is a normal condition (no-db mode), not an error. The .expect() in the integration test is correct because the test specifically sets up a libsql backend and expects it to be available. Keeping Option rather than Result because the no-backend state is not an error — it's a legitimate configuration.
| return Ok(()); | ||
| } | ||
| }; | ||
|
|
There was a problem hiding this comment.
self.handles is set to Some(handles) just a few lines above (line 137) in the same method flow. The fallback to empty_handles here is defensive but obscures the fact that if self.handles is None at this point, it means the database init was skipped (early return on line 131), so this code is unreachable with None. Consider using unwrap_or_default() directly or adding a brief comment explaining why the fallback exists (e.g., for the no-db path where init_database returned early before the secrets store setup path was reached via some other route).
| // In the normal path, `self.handles` was set earlier during DB init. | |
| // If it is `None` here, we're in a no-DB configuration (e.g. early | |
| // return from `init_database`), so we fall back to default/empty | |
| // handles to allow secrets-store setup to proceed without a DB. |
There was a problem hiding this comment.
Fixed in 951b148 — added a comment explaining the fallback covers the no-database path where init_database returned early.
| let job_event_tx: Option<broadcast::Sender<(Uuid, SseEvent)>> = | ||
| if config.sandbox.enabled && docker_status.is_ok() { | ||
| let (tx, _) = broadcast::channel(256); | ||
| Some(tx) | ||
| } else { | ||
| None | ||
| }; | ||
|
|
||
| let container_job_manager: Option<Arc<ContainerJobManager>> = | ||
| if config.sandbox.enabled && docker_status.is_ok() { |
There was a problem hiding this comment.
The condition config.sandbox.enabled && docker_status.is_ok() is checked twice (lines 100 and 108). Since job_event_tx is only used inside the container_job_manager block, consider combining these into a single conditional block where both job_event_tx and container_job_manager are created together to avoid the duplication.
| let job_event_tx: Option<broadcast::Sender<(Uuid, SseEvent)>> = | |
| if config.sandbox.enabled && docker_status.is_ok() { | |
| let (tx, _) = broadcast::channel(256); | |
| Some(tx) | |
| } else { | |
| None | |
| }; | |
| let container_job_manager: Option<Arc<ContainerJobManager>> = | |
| if config.sandbox.enabled && docker_status.is_ok() { | |
| let mut job_event_tx: Option<broadcast::Sender<(Uuid, SseEvent)>> = None; | |
| let container_job_manager: Option<Arc<ContainerJobManager>> = | |
| if config.sandbox.enabled && docker_status.is_ok() { | |
| let (tx, _) = broadcast::channel(256); | |
| job_event_tx = Some(tx.clone()); |
There was a problem hiding this comment.
Fixed in 951b148 — combined both conditions into a single let (job_event_tx, container_job_manager) = if ... { ... } else { (None, None) }; block.
| pub async fn init_secrets_store() | ||
| -> anyhow::Result<Arc<dyn crate::secrets::SecretsStore + Send + Sync>> { |
There was a problem hiding this comment.
The return type arrow -> is on the next line without indentation alignment. While this compiles, it's inconsistent with the rest of the codebase's formatting style where the return type is typically on the same line as the function signature or consistently indented. Consider running cargo fmt to normalize this.
| pub async fn init_secrets_store() | |
| -> anyhow::Result<Arc<dyn crate::secrets::SecretsStore + Send + Sync>> { | |
| pub async fn init_secrets_store() -> anyhow::Result<Arc<dyn crate::secrets::SecretsStore + Send + Sync>> { |
There was a problem hiding this comment.
No change needed — this IS the output of cargo fmt. The return type wraps to the next line because the full signature exceeds the max line width. Running cargo fmt --check passes cleanly.
| use std::sync::Arc; | ||
|
|
||
| use crate::channels::wasm::{ | ||
| LoadedChannel, RegisteredEndpoint, SharedWasmChannel, WasmChannel, WasmChannelLoader, |
There was a problem hiding this comment.
LoadedChannel is imported but only used as a parameter type in the private register_channel function. This is fine, but note that LoadedChannel was not previously part of the public re-exports of the wasm module. If LoadedChannel visibility was pub(crate) or private, this import works because setup.rs is inside the wasm module. No action needed — just confirming the visibility is correct.
There was a problem hiding this comment.
Confirmed — LoadedChannel is pub(crate) and used correctly within the wasm module. No action needed.
| pub fn check_onboard_needed() -> Option<&'static str> { | ||
| let has_db = std::env::var("DATABASE_URL").is_ok() | ||
| || std::env::var("LIBSQL_PATH").is_ok() | ||
| || crate::config::default_libsql_path().exists(); |
There was a problem hiding this comment.
This function reads multiple environment variables. Since it was moved from main.rs into a public API, callers outside the main entry point may invoke it concurrently or in test contexts where env vars are being mutated. The original code in main.rs had the same behavior, but as a public function this is now more exposed. Consider documenting that this function reads environment state and is not thread-safe with respect to env::set_var.
There was a problem hiding this comment.
Fixed in 951b148 — added doc comment noting that the function reads environment variables and is not safe to call concurrently with env::set_var.
- secrets/mod.rs: clarify docstring that None is a normal no-db condition - app.rs: add comment explaining the empty_handles fallback path - orchestrator/mod.rs: combine duplicated sandbox condition into single block - setup/mod.rs: document env var reads and thread-safety caveat [skip-regression-check] Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
zmanian
left a comment
zmanian
left a comment
There was a problem hiding this comment.
Re-review after commits 16085d0..951b148.
The two new commits address all three non-blocking notes from the previous (now-dismissed) approval:
- cli/tool.rs unused import --
#[allow(unused_imports)]and the deadConfigimport removed. Done. - Duplicate Error Handling in CLAUDE.md -- The duplicated subsection removed. Done.
- orchestrator/mod.rs sandbox condition -- The two separate
if config.sandbox.enabled && docker_status.is_ok()blocks combined into a single block returning a tuple. Strictly a readability improvement, behavior unchanged.
Additional changes in 951b148 are docstring improvements (secrets/mod.rs, setup/mod.rs, app.rs) clarifying intent for callers. No new logic, no regressions, no .unwrap() in production code.
LGTM. Ship it.
…#778) * refactor: encapsulate leaked abstractions from main.rs and app.rs into owning modules Move module-specific initialization logic out of main.rs (1222→665 lines, -46%) and app.rs (944→780 lines, -17%) into their respective owning modules as public factory functions. This enforces separation of concerns so that adding a new DB backend, MCP transport, or channel doesn't require editing main.rs/app.rs. Key changes: - Tracing init functions → src/tracing_fmt.rs - DB connection factory (connect_with_handles + DatabaseHandles) → src/db/mod.rs - Secrets store factory (create_secrets_store) → src/secrets/mod.rs - MCP transport dispatch factory (create_client_from_config) → src/tools/mcp/factory.rs - Orchestrator setup (setup_orchestrator + OrchestratorSetup) → src/orchestrator/mod.rs - WASM channel setup (setup_wasm_channels) → src/channels/wasm/setup.rs - Worker entry points (run_worker, run_claude_bridge) → src/worker/mod.rs - Shared CLI secrets init (init_secrets_store) → src/cli/mod.rs - Tunnel startup (start_managed_tunnel) → src/tunnel/mod.rs - Onboard check (check_onboard_needed) → src/setup/mod.rs - ExtensionManager unified MCP: uses create_client_from_config via McpProcessManager, enabling stdio/Unix transports for hot-activated MCP servers - Deduplicated ~130 lines of secrets store init across cli/mcp.rs and cli/tool.rs - CLAUDE.md updated with module-owned initialization guideline [skip-regression-check] Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * refactor: address review feedback — deduplicate db factory, extract channel helper - connect_from_config() now delegates to connect_with_handles() to eliminate duplicated backend-matching logic (Copilot review feedback) - Extract register_channel() helper from setup_wasm_channels() loop body to improve readability (Gemini review feedback) [skip-regression-check] Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * style: fix rustfmt line wrapping in setup_wasm_channels Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * test: add integration test for module-owned initialization factories Exercises the full factory chain end-to-end to verify nothing was lost when initialization logic was moved from main.rs/app.rs into owning modules: - connect_with_handles returns Database + populated backend handles - connect_from_config delegates correctly (produces working Database) - secrets::create_secrets_store builds working store from DatabaseHandles - db::create_secrets_store standalone factory round-trips secrets - Both secrets factories produce compatible stores (cross-read works) - ExtensionManager constructs with McpProcessManager and is functional - DatabaseHandles default is empty All tests run without external services using libsql in-memory/tempfile. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: wire cli/mcp.rs and cli/tool.rs to shared init_secrets_store() Both files had inline implementations identical to cli::init_secrets_store(). Replace with delegation to complete the claimed deduplication. [skip-regression-check] Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * style: fix rustfmt line wrapping in integration test Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(review): remove unused Config import and deduplicate Error Handling section - Remove `#[allow(unused_imports)]` and unused `use crate::config::Config` from cli/tool.rs (no longer needed after delegating to shared `cli::init_secrets_store()`) - Remove duplicate Error Handling subsection from CLAUDE.md Key Patterns (all four bullets already exist in Code Style section and review-discipline.md) Addresses Copilot review comments. [skip-regression-check] Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(review): address remaining Copilot review comments - secrets/mod.rs: clarify docstring that None is a normal no-db condition - app.rs: add comment explaining the empty_handles fallback path - orchestrator/mod.rs: combine duplicated sandbox condition into single block - setup/mod.rs: document env var reads and thread-safety caveat [skip-regression-check] Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> Co-authored-by: Henry Park <henrypark133@gmail.com>
…#778) * refactor: encapsulate leaked abstractions from main.rs and app.rs into owning modules Move module-specific initialization logic out of main.rs (1222→665 lines, -46%) and app.rs (944→780 lines, -17%) into their respective owning modules as public factory functions. This enforces separation of concerns so that adding a new DB backend, MCP transport, or channel doesn't require editing main.rs/app.rs. Key changes: - Tracing init functions → src/tracing_fmt.rs - DB connection factory (connect_with_handles + DatabaseHandles) → src/db/mod.rs - Secrets store factory (create_secrets_store) → src/secrets/mod.rs - MCP transport dispatch factory (create_client_from_config) → src/tools/mcp/factory.rs - Orchestrator setup (setup_orchestrator + OrchestratorSetup) → src/orchestrator/mod.rs - WASM channel setup (setup_wasm_channels) → src/channels/wasm/setup.rs - Worker entry points (run_worker, run_claude_bridge) → src/worker/mod.rs - Shared CLI secrets init (init_secrets_store) → src/cli/mod.rs - Tunnel startup (start_managed_tunnel) → src/tunnel/mod.rs - Onboard check (check_onboard_needed) → src/setup/mod.rs - ExtensionManager unified MCP: uses create_client_from_config via McpProcessManager, enabling stdio/Unix transports for hot-activated MCP servers - Deduplicated ~130 lines of secrets store init across cli/mcp.rs and cli/tool.rs - CLAUDE.md updated with module-owned initialization guideline [skip-regression-check] Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * refactor: address review feedback — deduplicate db factory, extract channel helper - connect_from_config() now delegates to connect_with_handles() to eliminate duplicated backend-matching logic (Copilot review feedback) - Extract register_channel() helper from setup_wasm_channels() loop body to improve readability (Gemini review feedback) [skip-regression-check] Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * style: fix rustfmt line wrapping in setup_wasm_channels Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * test: add integration test for module-owned initialization factories Exercises the full factory chain end-to-end to verify nothing was lost when initialization logic was moved from main.rs/app.rs into owning modules: - connect_with_handles returns Database + populated backend handles - connect_from_config delegates correctly (produces working Database) - secrets::create_secrets_store builds working store from DatabaseHandles - db::create_secrets_store standalone factory round-trips secrets - Both secrets factories produce compatible stores (cross-read works) - ExtensionManager constructs with McpProcessManager and is functional - DatabaseHandles default is empty All tests run without external services using libsql in-memory/tempfile. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: wire cli/mcp.rs and cli/tool.rs to shared init_secrets_store() Both files had inline implementations identical to cli::init_secrets_store(). Replace with delegation to complete the claimed deduplication. [skip-regression-check] Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * style: fix rustfmt line wrapping in integration test Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(review): remove unused Config import and deduplicate Error Handling section - Remove `#[allow(unused_imports)]` and unused `use crate::config::Config` from cli/tool.rs (no longer needed after delegating to shared `cli::init_secrets_store()`) - Remove duplicate Error Handling subsection from CLAUDE.md Key Patterns (all four bullets already exist in Code Style section and review-discipline.md) Addresses Copilot review comments. [skip-regression-check] Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(review): address remaining Copilot review comments - secrets/mod.rs: clarify docstring that None is a normal no-db condition - app.rs: add comment explaining the empty_handles fallback path - orchestrator/mod.rs: combine duplicated sandbox condition into single block - setup/mod.rs: document env var reads and thread-safety caveat [skip-regression-check] Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> Co-authored-by: Henry Park <henrypark133@gmail.com>
…#778) * refactor: encapsulate leaked abstractions from main.rs and app.rs into owning modules Move module-specific initialization logic out of main.rs (1222→665 lines, -46%) and app.rs (944→780 lines, -17%) into their respective owning modules as public factory functions. This enforces separation of concerns so that adding a new DB backend, MCP transport, or channel doesn't require editing main.rs/app.rs. Key changes: - Tracing init functions → src/tracing_fmt.rs - DB connection factory (connect_with_handles + DatabaseHandles) → src/db/mod.rs - Secrets store factory (create_secrets_store) → src/secrets/mod.rs - MCP transport dispatch factory (create_client_from_config) → src/tools/mcp/factory.rs - Orchestrator setup (setup_orchestrator + OrchestratorSetup) → src/orchestrator/mod.rs - WASM channel setup (setup_wasm_channels) → src/channels/wasm/setup.rs - Worker entry points (run_worker, run_claude_bridge) → src/worker/mod.rs - Shared CLI secrets init (init_secrets_store) → src/cli/mod.rs - Tunnel startup (start_managed_tunnel) → src/tunnel/mod.rs - Onboard check (check_onboard_needed) → src/setup/mod.rs - ExtensionManager unified MCP: uses create_client_from_config via McpProcessManager, enabling stdio/Unix transports for hot-activated MCP servers - Deduplicated ~130 lines of secrets store init across cli/mcp.rs and cli/tool.rs - CLAUDE.md updated with module-owned initialization guideline [skip-regression-check] Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * refactor: address review feedback — deduplicate db factory, extract channel helper - connect_from_config() now delegates to connect_with_handles() to eliminate duplicated backend-matching logic (Copilot review feedback) - Extract register_channel() helper from setup_wasm_channels() loop body to improve readability (Gemini review feedback) [skip-regression-check] Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * style: fix rustfmt line wrapping in setup_wasm_channels Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * test: add integration test for module-owned initialization factories Exercises the full factory chain end-to-end to verify nothing was lost when initialization logic was moved from main.rs/app.rs into owning modules: - connect_with_handles returns Database + populated backend handles - connect_from_config delegates correctly (produces working Database) - secrets::create_secrets_store builds working store from DatabaseHandles - db::create_secrets_store standalone factory round-trips secrets - Both secrets factories produce compatible stores (cross-read works) - ExtensionManager constructs with McpProcessManager and is functional - DatabaseHandles default is empty All tests run without external services using libsql in-memory/tempfile. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: wire cli/mcp.rs and cli/tool.rs to shared init_secrets_store() Both files had inline implementations identical to cli::init_secrets_store(). Replace with delegation to complete the claimed deduplication. [skip-regression-check] Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * style: fix rustfmt line wrapping in integration test Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(review): remove unused Config import and deduplicate Error Handling section - Remove `#[allow(unused_imports)]` and unused `use crate::config::Config` from cli/tool.rs (no longer needed after delegating to shared `cli::init_secrets_store()`) - Remove duplicate Error Handling subsection from CLAUDE.md Key Patterns (all four bullets already exist in Code Style section and review-discipline.md) Addresses Copilot review comments. [skip-regression-check] Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(review): address remaining Copilot review comments - secrets/mod.rs: clarify docstring that None is a normal no-db condition - app.rs: add comment explaining the empty_handles fallback path - orchestrator/mod.rs: combine duplicated sandbox condition into single block - setup/mod.rs: document env var reads and thread-safety caveat [skip-regression-check] Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> Co-authored-by: Henry Park <henrypark133@gmail.com>
…#778) * refactor: encapsulate leaked abstractions from main.rs and app.rs into owning modules Move module-specific initialization logic out of main.rs (1222→665 lines, -46%) and app.rs (944→780 lines, -17%) into their respective owning modules as public factory functions. This enforces separation of concerns so that adding a new DB backend, MCP transport, or channel doesn't require editing main.rs/app.rs. Key changes: - Tracing init functions → src/tracing_fmt.rs - DB connection factory (connect_with_handles + DatabaseHandles) → src/db/mod.rs - Secrets store factory (create_secrets_store) → src/secrets/mod.rs - MCP transport dispatch factory (create_client_from_config) → src/tools/mcp/factory.rs - Orchestrator setup (setup_orchestrator + OrchestratorSetup) → src/orchestrator/mod.rs - WASM channel setup (setup_wasm_channels) → src/channels/wasm/setup.rs - Worker entry points (run_worker, run_claude_bridge) → src/worker/mod.rs - Shared CLI secrets init (init_secrets_store) → src/cli/mod.rs - Tunnel startup (start_managed_tunnel) → src/tunnel/mod.rs - Onboard check (check_onboard_needed) → src/setup/mod.rs - ExtensionManager unified MCP: uses create_client_from_config via McpProcessManager, enabling stdio/Unix transports for hot-activated MCP servers - Deduplicated ~130 lines of secrets store init across cli/mcp.rs and cli/tool.rs - CLAUDE.md updated with module-owned initialization guideline [skip-regression-check] Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * refactor: address review feedback — deduplicate db factory, extract channel helper - connect_from_config() now delegates to connect_with_handles() to eliminate duplicated backend-matching logic (Copilot review feedback) - Extract register_channel() helper from setup_wasm_channels() loop body to improve readability (Gemini review feedback) [skip-regression-check] Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * style: fix rustfmt line wrapping in setup_wasm_channels Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * test: add integration test for module-owned initialization factories Exercises the full factory chain end-to-end to verify nothing was lost when initialization logic was moved from main.rs/app.rs into owning modules: - connect_with_handles returns Database + populated backend handles - connect_from_config delegates correctly (produces working Database) - secrets::create_secrets_store builds working store from DatabaseHandles - db::create_secrets_store standalone factory round-trips secrets - Both secrets factories produce compatible stores (cross-read works) - ExtensionManager constructs with McpProcessManager and is functional - DatabaseHandles default is empty All tests run without external services using libsql in-memory/tempfile. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: wire cli/mcp.rs and cli/tool.rs to shared init_secrets_store() Both files had inline implementations identical to cli::init_secrets_store(). Replace with delegation to complete the claimed deduplication. [skip-regression-check] Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * style: fix rustfmt line wrapping in integration test Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(review): remove unused Config import and deduplicate Error Handling section - Remove `#[allow(unused_imports)]` and unused `use crate::config::Config` from cli/tool.rs (no longer needed after delegating to shared `cli::init_secrets_store()`) - Remove duplicate Error Handling subsection from CLAUDE.md Key Patterns (all four bullets already exist in Code Style section and review-discipline.md) Addresses Copilot review comments. [skip-regression-check] Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(review): address remaining Copilot review comments - secrets/mod.rs: clarify docstring that None is a normal no-db condition - app.rs: add comment explaining the empty_handles fallback path - orchestrator/mod.rs: combine duplicated sandbox condition into single block - setup/mod.rs: document env var reads and thread-safety caveat [skip-regression-check] Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> Co-authored-by: Henry Park <henrypark133@gmail.com>
Summary
main.rs(1222→665 lines, -46%) andapp.rs(944→780 lines, -17%) into owning modules as public factory functions, so adding a new DB backend, MCP transport, or channel no longer requires editing entry-point files.db::connect_with_handles(),secrets::create_secrets_store(),mcp::create_client_from_config(),orchestrator::setup_orchestrator(),wasm::setup_wasm_channels(),tunnel::start_managed_tunnel(),cli::init_secrets_store(),setup::check_onboard_needed()ExtensionManagernow uses the sharedcreate_client_from_config()factory viaMcpProcessManager, enabling stdio/Unix transport support for hot-activated MCP servers (previously HTTP-only)cli/mcp.rsandcli/tool.rsFiles changed
src/db/mod.rs,src/secrets/mod.rs,src/orchestrator/mod.rs,src/tunnel/mod.rs,src/setup/mod.rs,src/tracing_fmt.rssrc/tools/mcp/factory.rs,src/channels/wasm/setup.rssrc/worker/mod.rs,src/cli/mod.rssrc/main.rs,src/app.rs,src/cli/mcp.rs,src/cli/tool.rs,src/extensions/manager.rssrc/channels/web/server.rs,src/tools/builtin/extension_tools.rsCLAUDE.md(module-owned init guideline)Test plan
cargo check(default features)cargo check --no-default-features --features libsqlcargo check --all-featurescargo test— all tests pass.unwrap()/.expect()in new production code🤖 Generated with Claude Code