From 4e2090e54dc0cc009600f4a429afca6e91acc412 Mon Sep 17 00:00:00 2001 From: th-kim0823 Date: Thu, 7 May 2026 16:59:40 +0900 Subject: [PATCH] =?UTF-8?q?=F0=9F=8F=97=EF=B8=8F=20refactor(fb-30):=20appl?= =?UTF-8?q?y=20round=201=20review=20nits?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - error_wire.rs: extract `pub const ERROR_V1_ID = "error.v1"` + replace 9 inline literals (parallel to schema.rs::SCHEMA_V1_ID pattern). Re-export via kebab-app::lib.rs. - kebab-mcp/src/lib.rs: extract `KebabHandler::spawn_tool` helper — search + ask arms reduce from ~17 lines each to a one-line dispatch. Future tool 추가 시 boilerplate 안 늘림. - ask.rs: defensive `to_value(&answer)` — silent Null 위험 제거, 실패 시 to_tool_error fallthrough. - HOTFIXES: note AskOpts Default 미도입 limitation. - ARCHITECTURE.md: directory tree 의 kebab-mcp 항목에 `schema` 추가 (4 tool 모두 명시). Round 1 review summary: http://gitea.altair823.xyz/altair823-org/kebab/pulls/108#issuecomment-1855 Co-Authored-By: Claude Opus 4.7 (1M context) --- crates/kebab-app/src/error_wire.rs | 22 +++++++------ crates/kebab-app/src/lib.rs | 2 +- crates/kebab-mcp/src/lib.rs | 50 ++++++++++++++---------------- crates/kebab-mcp/src/tools/ask.rs | 5 ++- docs/ARCHITECTURE.md | 2 +- tasks/HOTFIXES.md | 1 + 6 files changed, 44 insertions(+), 38 deletions(-) diff --git a/crates/kebab-app/src/error_wire.rs b/crates/kebab-app/src/error_wire.rs index 192cf7c..e1d91e1 100644 --- a/crates/kebab-app/src/error_wire.rs +++ b/crates/kebab-app/src/error_wire.rs @@ -11,6 +11,10 @@ use serde_json::{Value, json}; use crate::error_signal::{ConfigInvalid, LlmError, NotIndexed}; +/// Wire schema id for [`ErrorV1`]. Single source of truth — kebab-cli +/// + kebab-mcp use this via `kebab_app::ERROR_V1_ID`. +pub const ERROR_V1_ID: &str = "error.v1"; + #[derive(Debug, Clone, Serialize, Deserialize)] pub struct ErrorV1 { pub schema_version: String, @@ -23,7 +27,7 @@ pub struct ErrorV1 { pub fn classify(err: &anyhow::Error, verbose: bool) -> ErrorV1 { if let Some(s) = err.downcast_ref::() { return ErrorV1 { - schema_version: "error.v1".to_string(), + schema_version: ERROR_V1_ID.to_string(), code: "config_invalid".to_string(), message: s.to_string(), details: json!({ @@ -35,7 +39,7 @@ pub fn classify(err: &anyhow::Error, verbose: bool) -> ErrorV1 { } if let Some(s) = err.downcast_ref::() { return ErrorV1 { - schema_version: "error.v1".to_string(), + schema_version: ERROR_V1_ID.to_string(), code: "not_indexed".to_string(), message: s.to_string(), details: json!({ @@ -50,7 +54,7 @@ pub fn classify(err: &anyhow::Error, verbose: bool) -> ErrorV1 { } if let Some(io) = err.downcast_ref::() { return ErrorV1 { - schema_version: "error.v1".to_string(), + schema_version: ERROR_V1_ID.to_string(), code: "io_error".to_string(), message: io.to_string(), details: json!({"kind": format!("{:?}", io.kind())}), @@ -63,7 +67,7 @@ pub fn classify(err: &anyhow::Error, verbose: bool) -> ErrorV1 { details = json!({"chain": chain}); } ErrorV1 { - schema_version: "error.v1".to_string(), + schema_version: ERROR_V1_ID.to_string(), code: "generic".to_string(), message: err.to_string(), details, @@ -74,7 +78,7 @@ pub fn classify(err: &anyhow::Error, verbose: bool) -> ErrorV1 { fn classify_llm(s: &LlmError) -> ErrorV1 { match s { LlmError::Unreachable { endpoint, source } => ErrorV1 { - schema_version: "error.v1".to_string(), + schema_version: ERROR_V1_ID.to_string(), code: "model_unreachable".to_string(), message: format!("ollama unreachable at {endpoint}"), details: json!({ @@ -84,28 +88,28 @@ fn classify_llm(s: &LlmError) -> ErrorV1 { hint: Some(format!("ensure `ollama serve` is reachable at {endpoint}")), }, LlmError::ModelNotPulled(model) => ErrorV1 { - schema_version: "error.v1".to_string(), + schema_version: ERROR_V1_ID.to_string(), code: "model_not_pulled".to_string(), message: format!("ollama model `{model}` is not pulled"), details: json!({"model": model}), hint: Some(format!("run `ollama pull {model}`")), }, LlmError::Timeout(e) => ErrorV1 { - schema_version: "error.v1".to_string(), + schema_version: ERROR_V1_ID.to_string(), code: "timeout".to_string(), message: format!("ollama timeout: {e}"), details: json!({"source": e.to_string()}), hint: Some("increase timeout or check Ollama load".to_string()), }, LlmError::Stream(body) => ErrorV1 { - schema_version: "error.v1".to_string(), + schema_version: ERROR_V1_ID.to_string(), code: "generic".to_string(), message: format!("ollama HTTP error: {body}"), details: json!({"body": body}), hint: None, }, LlmError::Malformed(line) => ErrorV1 { - schema_version: "error.v1".to_string(), + schema_version: ERROR_V1_ID.to_string(), code: "generic".to_string(), message: format!("malformed response line: {line}"), details: json!({"line": line}), diff --git a/crates/kebab-app/src/lib.rs b/crates/kebab-app/src/lib.rs index 3c2740c..58c4062 100644 --- a/crates/kebab-app/src/lib.rs +++ b/crates/kebab-app/src/lib.rs @@ -66,7 +66,7 @@ pub mod schema; pub use app::App; pub use ingest_progress::{AggregateCounts, IngestEvent, render_skipped_breakdown}; pub use reset::{ResetReport, ResetScope}; -pub use error_wire::{ErrorV1, classify}; +pub use error_wire::{ERROR_V1_ID, ErrorV1, classify}; pub use schema::{Capabilities, Models, SCHEMA_V1_ID, SchemaV1, Stats, WireBlock, schema_with_config}; /// p9-fb-25: sentinel for files without an extension in diff --git a/crates/kebab-mcp/src/lib.rs b/crates/kebab-mcp/src/lib.rs index d50febf..a190673 100644 --- a/crates/kebab-mcp/src/lib.rs +++ b/crates/kebab-mcp/src/lib.rs @@ -67,6 +67,28 @@ impl KebabHandler { pub fn state(&self) -> &KebabAppState { &self.state } + + /// Spawn a tool handler on the blocking pool. Used by tools that + /// transitively touch reqwest::blocking::Client (search, ask) — calling + /// from the async dispatch directly panics inside the runtime. + async fn spawn_tool( + &self, + args: serde_json::Map, + handle: F, + ) -> Result + where + I: serde::de::DeserializeOwned + Send + 'static, + F: FnOnce(KebabAppState, I) -> CallToolResult + Send + 'static, + { + let input: I = match serde_json::from_value(serde_json::Value::Object(args)) { + Ok(i) => i, + Err(e) => return Ok(error::to_tool_error(&anyhow::Error::from(e))), + }; + let state = self.state.clone(); + tokio::task::spawn_blocking(move || handle(state, input)) + .await + .map_err(|e| ErrorData::internal_error(e.to_string(), None)) + } } impl ServerHandler for KebabHandler { @@ -99,41 +121,17 @@ impl ServerHandler for KebabHandler { } "search" => { let args = request.arguments.unwrap_or_default(); - let input: tools::search::SearchInput = - match serde_json::from_value(serde_json::Value::Object(args)) { - Ok(i) => i, - Err(e) => { - return Ok(error::to_tool_error(&anyhow::Error::from(e))); - } - }; - let state = self.state.clone(); - let result = tokio::task::spawn_blocking(move || { + self.spawn_tool(args, |state, input| { tools::search::handle(&state, input) }) .await - .map_err(|e| { - ErrorData::internal_error(e.to_string(), None) - })?; - Ok(result) } "ask" => { let args = request.arguments.unwrap_or_default(); - let input: tools::ask::AskInput = - match serde_json::from_value(serde_json::Value::Object(args)) { - Ok(i) => i, - Err(e) => { - return Ok(error::to_tool_error(&anyhow::Error::from(e))); - } - }; - let state = self.state.clone(); - let result = tokio::task::spawn_blocking(move || { + self.spawn_tool(args, |state, input| { tools::ask::handle(&state, input) }) .await - .map_err(|e| { - ErrorData::internal_error(e.to_string(), None) - })?; - Ok(result) } _other => Err(ErrorData::method_not_found::< rmcp::model::CallToolRequestMethod, diff --git a/crates/kebab-mcp/src/tools/ask.rs b/crates/kebab-mcp/src/tools/ask.rs index 96c74f7..283bf4f 100644 --- a/crates/kebab-mcp/src/tools/ask.rs +++ b/crates/kebab-mcp/src/tools/ask.rs @@ -50,7 +50,10 @@ pub fn handle(state: &KebabAppState, input: AskInput) -> CallToolResult { Ok(answer) => { // `Answer` does not carry `schema_version`; tag inline (idempotent // via entry().or_insert_with in case a future version adds it). - let mut v = serde_json::to_value(&answer).unwrap_or_default(); + let mut v = match serde_json::to_value(&answer) { + Ok(v) => v, + Err(e) => return to_tool_error(&anyhow::anyhow!("answer serialize failed: {e}")), + }; if let serde_json::Value::Object(ref mut map) = v { map.entry("schema_version".to_string()) .or_insert_with(|| serde_json::Value::String("answer.v1".to_string())); diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index 4bb499f..0c12084 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -170,7 +170,7 @@ kebab/ │ ├── kebab-parse-pdf/ # lopdf per-page text extractor (P7-1) │ ├── kebab-app/ # facade (P0 시그니처 + P3-5/P6-4/P7-3 본체) │ ├── kebab-tui/ # Ratatui shell + Library 패널 (P9-1) -│ ├── kebab-mcp/ # stdio MCP server — tools: search, ask, doctor (P9-FB-30) +│ ├── kebab-mcp/ # stdio MCP server — tools: schema, doctor, search, ask (P9-FB-30) │ └── kebab-cli/ # binary (P0 → 핫픽스로 --config flag wiring 강화) ├── migrations/ # SQLite refinery V001/V002/V003 └── fixtures/ # 테스트 fixture 트리 diff --git a/tasks/HOTFIXES.md b/tasks/HOTFIXES.md index 27adfc8..6dc93e2 100644 --- a/tasks/HOTFIXES.md +++ b/tasks/HOTFIXES.md @@ -47,6 +47,7 @@ git history. - Server-scope state caching — 현재 매 tool call 마다 store open. 첫 call 시 `KebabAppState` 에 `OnceLock` 도입 검토 (post-merge 후속 PR). - rmcp SDK API 호환성 — 1.6 채택, 미래 major bump 시 별 task. - Manual `tools/list` + `tools/call` dispatch 채택 — rmcp 1.6 의 `#[tool_router]` 매크로보다 명시적, 디버깅 쉬움. 하지만 새 tool 추가 시 두 곳 (list_tools 의 vec + call_tool 의 match) 동시 갱신 필요. 후속 task 가 5개 이상 tool 추가하면 매크로 도입 재검토. +- `AskOpts` 가 `Default` 미도입 — kebab-cli + kebab-tui + kebab-mcp 의 모든 호출 site 가 9 field 를 명시적으로 초기화. 새 field 추가 시 모든 site 동시 갱신 필요. `impl Default for AskOpts` 또는 builder 패턴 도입은 별 PR. **Amends**: - design §10 (MCP transport subsection 추가).