🏗️ refactor(fb-30): apply round 1 review nits
- 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<I, F>` 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: #108 (comment) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -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::<ConfigInvalid>() {
|
||||
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::<NotIndexed>() {
|
||||
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::<std::io::Error>() {
|
||||
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}),
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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<I, F>(
|
||||
&self,
|
||||
args: serde_json::Map<String, serde_json::Value>,
|
||||
handle: F,
|
||||
) -> Result<CallToolResult, ErrorData>
|
||||
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,
|
||||
|
||||
@@ -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()));
|
||||
|
||||
@@ -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 트리
|
||||
|
||||
@@ -47,6 +47,7 @@ git history.
|
||||
- Server-scope state caching — 현재 매 tool call 마다 store open. 첫 call 시 `KebabAppState` 에 `OnceLock<SqliteStore>` 도입 검토 (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 추가).
|
||||
|
||||
Reference in New Issue
Block a user