Merge pull request #26 from flolibio/fix/mcp-review-findings

fix: address external code review findings (fold into v1.1.0 draft)

Author: wunamesst

CHANGELOG.md

@@ -1,6 +1,6 @@
 # Changelog
 
-## [v1.1.0] - 2026-08-16
+## [v1.1.0] - 2026-08-18
 
 **MCP — the AI log access layer.** tailr is now both a human log viewer and an MCP server: AI agents (Claude Code, Cursor, Codex, OpenCode, any MCP client) can list, search, count, and read server logs directly over `/mcp` — no SSH, no copy-paste. Single binary, zero new runtime dependencies.
 
@@ -21,6 +21,12 @@
 - **`tail_log` byte cap no longer truncates silently** — responses carry `truncated: true` when the 64MB read cap is hit.
 - **Token comparison is constant-time** (hand-rolled, zero new dependencies) for both the Bearer header and the WS query-param fallback.
 - **Disabled `/mcp` returns a deterministic 404** instead of falling into the SPA fallback (which 500s when no frontend dist is built).
+- **`tail_log` dropped empty lines and shifted line numbers** (external review) — the tail reader filtered all empty split segments, so files with blank lines got line numbers drifting from the scanner's. Empty lines now occupy their real line numbers; protocol-level regression test added.
+- **`tail_log` could report wrong line numbers from a partial line count** (external review) — if the line-count scan hit its time budget, the partial total was used as exact. Responses now carry `incomplete: true` in that case and the tool description tells agents to treat line numbers as approximate.
+- **Stats caching (mtime + size keyed)** — completed line/level counts are cached; repeated `tail_log`/`get_log_stats` calls on append-only logs no longer rescan the whole file. Cache hits skip the scan semaphore; file growth invalidates.
+- **Startup warning when MCP is enabled without a token** — the endpoint adds unauthenticated expensive-scan capability on top of the existing no-auth REST default; the combination is now logged loudly.
+- **LevelDetector no longer panics on keywords longer than the 256-byte scan window** (external review) — `limit - len` underflowed (debug panic / release slice OOB). Oversized keywords are now skipped safely.
+- **`list_log_files` signals truncation** — hitting the 5000-entry cap now returns `truncated: true` instead of silently dropping files; `search_logs` description clarifies that multi-page `count_only` results must be summed across pages.
 
 ### Dependencies
 

crates/core/src/query.rs

@@ -8,10 +8,12 @@
 //!
 //! 本模块是纯同步 `fn`：无 HTTP、无 async、无运行时假设，符合 core 边界。
 
+use std::collections::HashMap;
 use std::fmt;
-use std::path::Path;
+use std::path::{Path, PathBuf};
 use std::sync::atomic::AtomicBool;
-use std::time::Duration;
+use std::sync::Mutex;
+use std::time::{Duration, SystemTime};
 
 use tailr_search_engine::{
     file_stats, scan_file, FileStats, LevelDetector, ScanParams, ScanResult, DEFAULT_CONTEXT,
@@ -107,9 +109,18 @@ impl std::error::Error for QueryError {
     }
 }
 
+/// 已完成统计的缓存条目：追加型日志的 (mtime, size) 命中率极高，
+/// 一次全扫的精确行数可服务后续所有 stats/tail 调用。
+struct CachedStats {
+    mtime: SystemTime,
+    size: u64,
+    stats: FileStats,
+}
+
 pub struct QueryService {
     semaphore: tokio::sync::Semaphore,
     caps: QueryCaps,
+    stats_cache: Mutex<HashMap<PathBuf, CachedStats>>,
 }
 
 impl QueryService {
@@ -118,6 +129,7 @@ impl QueryService {
         Self {
             semaphore: tokio::sync::Semaphore::new(permits),
             caps,
+            stats_cache: Mutex::new(HashMap::new()),
         }
     }
 
@@ -180,17 +192,46 @@ impl QueryService {
     }
 
     /// 文件统计。同样占并发闸（全文件扫描同样是重活）。
+    ///
+    /// 缓存策略：以 (mtime, size) 为键缓存**已完成**的统计——追加型日志
+    /// 的连续 tail/stats 调用不再每次全扫；文件增长自动失效。未完成的
+    /// 部分计数不缓存。缓存命中不占并发闸（无扫描发生）。
     pub fn stats(
         &self,
         path: &Path,
         detector: Option<&LevelDetector>,
         cancel: Option<&AtomicBool>,
     ) -> Result<FileStats, QueryError> {
+        let meta = std::fs::metadata(path).map_err(QueryError::Io)?;
+        let (meta_mtime, meta_size) = (meta.modified().map_err(QueryError::Io)?, meta.len());
+
+        if let Ok(cache) = self.stats_cache.lock() {
+            if let Some(hit) = cache.get(path) {
+                if hit.mtime == meta_mtime && hit.size == meta_size {
+                    return Ok(hit.stats.clone());
+                }
+            }
+        }
+
         let _permit = self
             .semaphore
             .try_acquire()
             .map_err(|_| QueryError::Busy)?;
-        file_stats(path, detector, self.caps.max_time_budget, cancel).map_err(QueryError::Io)
+        let stats =
+            file_stats(path, detector, self.caps.max_time_budget, cancel).map_err(QueryError::Io)?;
+        if stats.completed {
+            if let Ok(mut cache) = self.stats_cache.lock() {
+                cache.insert(
+                    path.to_path_buf(),
+                    CachedStats {
+                        mtime: meta_mtime,
+                        size: meta_size,
+                        stats: stats.clone(),
+                    },
+                );
+            }
+        }
+        Ok(stats)
     }
 }
 
@@ -271,6 +312,29 @@ mod tests {
         assert!(matches!(err, QueryError::KeywordTooLong(256)));
     }
 
+    #[test]
+    fn stats_cache_invalidates_on_file_change() {
+        let mut f = tempfile::NamedTempFile::new().unwrap();
+        use std::io::Write;
+        writeln!(f, "one").unwrap();
+        f.flush().unwrap();
+        let svc = QueryService::new(QueryCaps::default());
+
+        let s1 = svc.stats(f.path(), None, None).unwrap();
+        assert_eq!(s1.total_lines, 1);
+
+        // 追加后 (mtime,size) 变化 → 缓存失效 → 重新扫描
+        writeln!(f, "two").unwrap();
+        f.flush().unwrap();
+        std::thread::sleep(std::time::Duration::from_millis(10));
+        let s2 = svc.stats(f.path(), None, None).unwrap();
+        assert_eq!(s2.total_lines, 2);
+
+        // 未变化 → 命中缓存返回同一结果
+        let s3 = svc.stats(f.path(), None, None).unwrap();
+        assert_eq!(s3.total_lines, 2);
+    }
+
     #[test]
     fn search_and_stats_passthrough() {
         let f = write_log("INFO a\nERROR b\n");

crates/search-engine/src/detector.rs

@@ -51,6 +51,11 @@ impl LevelDetector {
                 if line_lower_bytes.len() < keyword_bytes.len() {
                     continue;
                 }
+                // 关键词长于扫描窗口（256B）时 limit - len 下溢：跳过该
+                // 关键词防御，否则 debug panic / release 切片越界 panic。
+                if keyword_bytes.len() > limit {
+                    continue;
+                }
                 for i in 0..=limit - keyword_bytes.len() {
                     if line_lower_bytes[i..i + keyword_bytes.len()]
                         .iter()
@@ -92,6 +97,25 @@ mod tests {
         }
     }
 
+    #[test]
+    fn test_keyword_longer_than_scan_window_does_not_panic() {
+        // >256 字节的关键词 + 足够长的行：曾因 limit - len 下溢 panic。
+        let long_keyword = "k".repeat(300);
+        let line = format!("x{}y", long_keyword);
+        let detector = crate::LevelDetector::from_config(&LogLevelConfig {
+            preset: "custom".to_string(),
+            levels: vec![LevelDef {
+                name: "WEIRD".to_string(),
+                keywords: vec![long_keyword],
+                color_light: "#000000".to_string(),
+                color_dark: "#ffffff".to_string(),
+            }],
+        });
+        // 不 panic、返回 UNKNOWN（超窗关键词被安全跳过）即通过
+        assert_eq!(detector.detect(&line), "UNKNOWN".to_string());
+        assert_eq!(detector.detect_ref(&line), "UNKNOWN");
+    }
+
     #[test]
     fn test_detect_basic() {
         let config = make_config("general", vec![

crates/search-engine/src/scanner.rs

@@ -1,6 +1,6 @@
 //! Cursor 式日志扫描器（MCP search_logs / read 场景的地基）
 //!
-//! 设计要点（见知识库 tailr-MCP-AI日志访问层设计 2026-08-14 决策）：
+//! 设计要点：
 //! - 原语是字节偏移游标，不是行号索引——无预建索引，多 GB 文件零额外内存
 //! - 纯同步计算（`fn`），调用方（Web/MCP 层）自行包 `spawn_blocking`
 //! - 分块扫描（4MB），每块边界检查时间预算与取消标志；预算耗尽返回

crates/server/src/lib.rs

@@ -278,6 +278,16 @@ pub fn app(
     // heavy work is already gated by QueryService's scan semaphore. HTTP-level
     // 429 storms on legitimate agents would be worse than the abuse they'd
     // prevent (see mcp.rs module docs). Optionally disabled via [mcp] enabled=false.
+    // MCP 新增的是「无认证也能触发多 GB 全扫」的能力（REST 默认开放是
+    // 既有立场，但 REST 没有这种昂贵操作）。该组合显式告警。
+    if mcp.enabled && state.token.is_empty() {
+        tracing::warn!(
+            "MCP endpoint is enabled with NO auth token — anyone who can \
+             reach this port can read all configured logs and trigger \
+             full-file scans; set `token` or disable via [mcp] enabled=false"
+        );
+    }
+
     let mcp_router = if mcp.enabled {
         mcp::routes(state.clone())
             .route_layer(middleware::from_fn_with_state(state.clone(), auth_middleware))

crates/server/src/mcp.rs

@@ -1,7 +1,7 @@
 //! MCP (Model Context Protocol) 服务端 — `/mcp` streamable HTTP 端点。
 //!
 //! 让 AI agent（Claude Code / Cursor 等）直接检索服务器日志：搜索、读尾部、
-//! 读任意游标、级别统计。设计决策见知识库《tailr-MCP-AI日志访问层设计》：
+//! 读任意游标、级别统计。关键设计决策（自足摘要，勿引用外部文档）：
 //!
 //! - **token 防护**：所有 tool 的输出都经 QueryService 预算钳制（命中数/
 //!   字节/超时），响应自带 `resumeCursor` 分页语义；tool description 与
@@ -250,20 +250,22 @@ impl McpTools {
     #[tool(description = "List the log files accessible on this server. Returns name/path/size/isDir for each entry. Start here to discover what you can query — the `path` values are the exact identifiers the other tools expect.")]
     async fn list_log_files(&self) -> Result<CallToolResult, McpError> {
         let state = self.state.clone();
-        let entries = tokio::task::spawn_blocking(move || list_files_blocking(&state))
-            .await
-            .map_err(|e| McpError::internal_error(format!("listing task failed: {e}"), None))?;
+        let (entries, truncated) =
+            tokio::task::spawn_blocking(move || list_files_blocking(&state))
+                .await
+                .map_err(|e| McpError::internal_error(format!("listing task failed: {e}"), None))?;
 
         Ok(CallToolResult::success(vec![Content::text(
             json!({
                 "host": self.state.host_name,
                 "files": entries,
+                "truncated": truncated,
             })
             .to_string(),
         )]))
     }
 
-    #[tool(description = "Read the LAST N lines of a log file (default 100, max 5000) with absolute line numbers. Good for a quick look at recent activity; prefer get_log_stats + search_logs for anything bigger. If truncated is true, the tail exceeded the byte cap — request fewer lines.")]
+    #[tool(description = "Read the LAST N lines of a log file (default 100, max 5000) with absolute line numbers. Good for a quick look at recent activity; prefer get_log_stats + search_logs for anything bigger. If truncated is true, the tail exceeded the byte cap — request fewer lines. If incomplete is true, line numbers are approximate (the line count did not finish scanning).")]
     async fn tail_log(
         &self,
         Parameters(args): Parameters<TailLogArgs>,
@@ -305,10 +307,16 @@ impl McpTools {
                 .map_err(query_io)?;
             let file_len = std::fs::metadata(&path_buf).map(|m| m.len()).unwrap_or(0);
             let tail_truncated = file_len.saturating_sub(tail.start_byte) > read as u64;
+            // 只丢弃末尾换行产生的空元素；中间的空行是真实行，
+            // 必须保留并占一个行号——否则与 scanner 的行号体系错位
+            // （空行后所有行号整体漂移）。
+            let mut parts: Vec<&[u8]> = buf.split(|&b| b == b'\n').collect();
+            if parts.last() == Some(&&[][..]) {
+                parts.pop();
+            }
             let mut line_no = start_line;
-            let lines: Vec<serde_json::Value> = buf
-                .split(|&b| b == b'\n')
-                .filter(|l| !l.is_empty())
+            let lines: Vec<serde_json::Value> = parts
+                .into_iter()
                 .map(|l| {
                     let mut end = l.len();
                     if end > 0 && l[end - 1] == b'\r' {
@@ -325,6 +333,9 @@ impl McpTools {
                 "totalLines": total_lines,
                 "sizeBytes": stats.total_bytes,
                 "truncated": tail_truncated,
+                // 行数统计未扫完时 start_line/line 是按部分计数推的，
+                // 只能当近似值——显式透传，别让 AI 当精确数用。
+                "incomplete": !stats.completed,
             }))
         })
         .await
@@ -341,7 +352,7 @@ impl McpTools {
         )]))
     }
 
-    #[tool(description = "Search a log file for lines containing ALL keywords (AND, case-insensitive). For 'how many/how often' questions set count_only=true (fast, counts without content). For inspection, returns matching lines merged into context windows (± context_lines). Workflow: get_log_stats first, then search with 2-3 specific keywords. If more/truncated is true, continue with resumeCursor (never restart); a timeout also returns partial results + resumeCursor — just continue. When reporting results, state the raw matched LINE count first, then any higher-level grouping you derive (e.g. request-pairs) — users usually think in lines.")]
+    #[tool(description = "Search a log file for lines containing ALL keywords (AND, case-insensitive). For 'how many/how often' questions set count_only=true (fast, counts without content). For inspection, returns matching lines merged into context windows (± context_lines). Workflow: get_log_stats first, then search with 2-3 specific keywords. If more/truncated is true, continue with resumeCursor (never restart); a timeout also returns partial results + resumeCursor — just continue. When count_only spans several pages, sum matchedLines across all pages for the total. When reporting results, state the raw matched LINE count first, then any higher-level grouping you derive (e.g. request-pairs) — users usually think in lines.")]
     async fn search_logs(
         &self,
         Parameters(args): Parameters<SearchLogsArgs>,
@@ -437,7 +448,9 @@ impl ServerHandler for McpTools {
 
 /// 同步列文件（spawn_blocking 里跑）：配置的 log_files 平铺 + log_dirs
 /// 递归（深度 ≤ MCP_LIST_DEPTH），只留文本文件，平铺输出（AI 不要树）。
-fn list_files_blocking(state: &AppState) -> Vec<serde_json::Value> {
+/// 返回 (条目, 是否因 MAX_LIST_ENTRIES 截断)——截断必须显式告知，
+/// 否则 AI 会以为列全了。
+fn list_files_blocking(state: &AppState) -> (Vec<serde_json::Value>, bool) {
     let mut out = Vec::new();
 
     for file in &state.log_files {
@@ -459,7 +472,7 @@ fn list_files_blocking(state: &AppState) -> Vec<serde_json::Value> {
         collect_text_files(dir, MCP_LIST_DEPTH, &mut out, &mut total);
     }
 
-    out
+    (out, total >= crate::api::MAX_LIST_ENTRIES)
 }
 
 fn collect_text_files(

crates/server/tests/mcp_integration.rs

@@ -193,6 +193,36 @@ async fn full_protocol_roundtrip() {
     assert_eq!(v["incomplete"], false);
 }
 
+#[tokio::test]
+async fn tail_log_preserves_empty_line_numbering() {
+    // 回归：空行是真实行，必须占行号——曾经被 filter 掉导致行号整体漂移。
+    let dir = fixture_dir();
+    std::fs::write(dir.join("blank.log"), "aaa\n\nccc\n").unwrap();
+    let path = dir.join("blank.log").display().to_string();
+    let url = spawn_app(McpConfig::default(), dir).await;
+    let client = connect(&url, Some(TOKEN)).await.expect("handshake");
+    let args = json!({ "path": path, "lines": 3 }).as_object().cloned().unwrap();
+    let resp = client
+        .peer()
+        .call_tool(rmcp::model::CallToolRequestParam {
+            name: "tail_log".into(),
+            arguments: Some(args),
+        })
+        .await
+        .unwrap();
+    let v = tool_json(&resp);
+    assert_eq!(v["totalLines"], 3);
+    assert_eq!(v["incomplete"], false);
+    let lines = v["lines"].as_array().unwrap();
+    assert_eq!(lines.len(), 3);
+    assert_eq!(lines[0]["line"], 1);
+    assert_eq!(lines[0]["text"], "aaa");
+    assert_eq!(lines[1]["line"], 2);
+    assert_eq!(lines[1]["text"], "");
+    assert_eq!(lines[2]["line"], 3);
+    assert_eq!(lines[2]["text"], "ccc");
+}
+
 #[tokio::test]
 async fn unauthenticated_client_is_rejected_until_token_given() {
     let dir = fixture_dir();