-
Notifications
You must be signed in to change notification settings - Fork 0
feat(search): add max_snippet_lines to cap returned snippets (semble#198) #80
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -18,6 +18,18 @@ use tokio::sync::Mutex; | |||||||||||||
| use csp::mcp::{find_related_tool, search_tool, IndexCache, SERVER_INSTRUCTIONS}; | ||||||||||||||
| use csp::types::ContentType; | ||||||||||||||
|
|
||||||||||||||
| /// MCP default: signature + first body lines, enough to confirm a location | ||||||||||||||
| /// while spending far fewer tokens than the full chunk (semble#198). | ||||||||||||||
| fn default_max_snippet_lines() -> Option<i64> { | ||||||||||||||
| Some(10) | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| /// Convert the wire value to the `Option<usize>` the handlers take. `None` (the | ||||||||||||||
| /// caller passed JSON `null`) → full content; a negative value clamps to `0`. | ||||||||||||||
| fn resolve_snippet_lines(value: Option<i64>) -> Option<usize> { | ||||||||||||||
| value.map(|n| n.max(0) as usize) | ||||||||||||||
| } | ||||||||||||||
|
Comment on lines
+29
to
+31
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Prompt To Fix With AIThis is a comment left during a code review.
Path: crates/csp/src/bin/csp/mcp_server.rs
Line: 29-31
Comment:
**Duplicated `resolve_snippet_lines` helper**
`resolve_snippet_lines` (and its `default_max_snippet_lines` companion) is defined identically in both `mcp_server.rs` and `main.rs`. Since both binaries already depend on the `csp` library crate (they import from `csp::utils`, `csp::mcp`, etc.), moving this pair to `csp::utils` (or a dedicated `csp::snippet` submodule) and re-exporting it would eliminate the duplication and keep the single source of truth close to `result_to_dict`, which it directly feeds.
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Comment on lines
+27
to
+31
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [MEDIUM] 중복 구현된 resolve_snippet_lines 제거 및 공통 함수 사용 Symptom:
Suggested change
|
||||||||||||||
|
|
||||||||||||||
| /// Parameters for the `search` tool (mirrors the TS MCP tool's args). | ||||||||||||||
| #[derive(Debug, serde::Deserialize, schemars::JsonSchema)] | ||||||||||||||
| pub struct SearchParams { | ||||||||||||||
|
|
@@ -28,6 +40,11 @@ pub struct SearchParams { | |||||||||||||
| pub repo: Option<String>, | ||||||||||||||
| /// Maximum number of results (default 5). | ||||||||||||||
| pub top_k: Option<u32>, | ||||||||||||||
| /// Lines of source per result. Default 10 = signature + first body lines, | ||||||||||||||
| /// enough to confirm the location. 0 = file path and line range only. Pass | ||||||||||||||
| /// `null` for the full chunk when the snippet lacks context. | ||||||||||||||
| #[serde(default = "default_max_snippet_lines")] | ||||||||||||||
| pub max_snippet_lines: Option<i64>, | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| /// Parameters for the `find_related` tool. | ||||||||||||||
|
|
@@ -41,6 +58,10 @@ pub struct FindRelatedParams { | |||||||||||||
| pub repo: Option<String>, | ||||||||||||||
| /// Maximum number of results (default 5). | ||||||||||||||
| pub top_k: Option<u32>, | ||||||||||||||
| /// Lines of source per result. Default 10 = signature + first body lines. | ||||||||||||||
| /// 0 = location only. Pass `null` for the full chunk. | ||||||||||||||
| #[serde(default = "default_max_snippet_lines")] | ||||||||||||||
| pub max_snippet_lines: Option<i64>, | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| /// MCP server holding the session index cache and the default source. | ||||||||||||||
|
|
@@ -82,6 +103,7 @@ impl CspMcpServer { | |||||||||||||
| &p.query, | ||||||||||||||
| p.repo.as_deref(), | ||||||||||||||
| p.top_k.unwrap_or(5) as usize, | ||||||||||||||
| resolve_snippet_lines(p.max_snippet_lines), | ||||||||||||||
| ); | ||||||||||||||
| Ok(CallToolResult::success(vec![Content::text(out)])) | ||||||||||||||
| } | ||||||||||||||
|
|
@@ -102,6 +124,7 @@ impl CspMcpServer { | |||||||||||||
| p.line, | ||||||||||||||
| p.repo.as_deref(), | ||||||||||||||
| p.top_k.unwrap_or(5) as usize, | ||||||||||||||
| resolve_snippet_lines(p.max_snippet_lines), | ||||||||||||||
| ); | ||||||||||||||
| Ok(CallToolResult::success(vec![Content::text(out)])) | ||||||||||||||
| } | ||||||||||||||
|
|
@@ -152,15 +175,30 @@ mod tests { | |||||||||||||
| assert_eq!(minimal.query, "greet"); | ||||||||||||||
| assert!(minimal.repo.is_none()); | ||||||||||||||
| assert!(minimal.top_k.is_none()); | ||||||||||||||
| // Absent max_snippet_lines → the MCP default of 10. | ||||||||||||||
| assert_eq!(minimal.max_snippet_lines, Some(10)); | ||||||||||||||
|
|
||||||||||||||
| let full: SearchParams = serde_json::from_value(serde_json::json!({ | ||||||||||||||
| "query": "greet", | ||||||||||||||
| "repo": "./x", | ||||||||||||||
| "top_k": 3 | ||||||||||||||
| "top_k": 3, | ||||||||||||||
| "max_snippet_lines": 0 | ||||||||||||||
| })) | ||||||||||||||
| .unwrap(); | ||||||||||||||
| assert_eq!(full.repo.as_deref(), Some("./x")); | ||||||||||||||
| assert_eq!(full.top_k, Some(3)); | ||||||||||||||
| assert_eq!(full.max_snippet_lines, Some(0)); | ||||||||||||||
|
|
||||||||||||||
| // Explicit null → None (full chunk), distinct from the absent default. | ||||||||||||||
| let nulled: SearchParams = serde_json::from_value(serde_json::json!({ | ||||||||||||||
| "query": "greet", | ||||||||||||||
| "max_snippet_lines": null | ||||||||||||||
| })) | ||||||||||||||
| .unwrap(); | ||||||||||||||
| assert!(nulled.max_snippet_lines.is_none()); | ||||||||||||||
| assert!(resolve_snippet_lines(nulled.max_snippet_lines).is_none()); | ||||||||||||||
| assert_eq!(resolve_snippet_lines(Some(3)), Some(3)); | ||||||||||||||
| assert_eq!(resolve_snippet_lines(Some(-4)), Some(0)); | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| #[test] | ||||||||||||||
|
|
@@ -208,6 +246,7 @@ mod tests { | |||||||||||||
| query: "greet".to_string(), | ||||||||||||||
| repo: None, | ||||||||||||||
| top_k: Some(5), | ||||||||||||||
| max_snippet_lines: None, | ||||||||||||||
| })) | ||||||||||||||
| .await | ||||||||||||||
| .unwrap(); | ||||||||||||||
|
|
@@ -235,6 +274,7 @@ mod tests { | |||||||||||||
| line: 1, | ||||||||||||||
| repo: None, | ||||||||||||||
| top_k: Some(5), | ||||||||||||||
| max_snippet_lines: None, | ||||||||||||||
| })) | ||||||||||||||
| .await | ||||||||||||||
| .unwrap(); | ||||||||||||||
|
|
||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[MEDIUM] 중복 구현된 resolve_snippet_lines 제거 및 공통 함수 사용
Symptom:
resolve_snippet_lines함수가main.rs에 로컬로 중복 구현되어 있습니다.Source: Fowler — Refactoring (Duplicate Code) / Hunt & Thomas — Pragmatic Programmer (DRY)
Consequence: 동일한 유틸리티 로직이 여러 곳에 중복 존재하여 유지보수성이 저하됩니다.
Remedy: 로컬 구현을 제거하고
csp::utils::resolve_snippet_lines를 가져와 사용하도록 변경합니다.