feat(search): add max_snippet_lines to cap returned snippets (semble#198)#80
feat(search): add max_snippet_lines to cap returned snippets (semble#198)#80amondnet wants to merge 1 commit into
Conversation
…198)
Port semble#198's max_snippet_lines: results can return a preview of each
chunk instead of the full content, so an agent spends fewer tokens
confirming a location before navigating to the file.
Semantics (utils::format_results / result_to_dict):
- None → full chunk content
- 0 → omit `content` (file path + line range only)
- N > 0 → first N lines
Also flattens the wire dict to match upstream after #198: results are now
`{file_path, start_line, end_line, score, content?}` at the top level
(dropping the nested `chunk` wrapper, `location`, and `language`). This
follows the upstream shape csp had faithfully mirrored before #198
reshaped it; the library `SearchResult` is unchanged.
Surface:
- CLI `search` / `find-related`: `--max-snippet-lines N`, default None
(full content — human-facing).
- MCP `search` / `find_related`: `max_snippet_lines` param, default 10
(token-frugal preview — agent-facing). Absent → 10, JSON null → full,
0 → location only (tri-state via serde field default).
Not in scope: savings accounting (semble#206). csp does not yet wire
save_search_stats into the search flow (no file_sizes on CspIndex), so
there is nothing to correct until savings telemetry is wired — tracked
separately.
Refs #75
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 10 |
| Duplication | 2 |
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
No issues found across 6 files
Architecture diagram
sequenceDiagram
participant CLI as CLI User (csp search)
participant MCP as MCP Client (agent)
participant Handler as Search Handler (CLI / MCP)
participant Utils as format_results / result_to_dict
participant Search as IndexCache / Search Engine
Note over CLI,Search: NEW: max_snippet_lines parameter & flat wire shape
CLI->>Handler: csp search --max-snippet-lines N "query" ./repo (default: None)
MCP->>Handler: JSON-RPC search(query, repo, max_snippet_lines=10|null) (default: 10)
Handler->>Search: search(query, repo, top_k)
Search-->>Handler: Vec<SearchResult>
Handler->>Utils: format_results(query, results, max_snippet_lines)
loop per result
Utils->>Utils: Build flat dict: file_path, start_line, end_line, score
alt max_snippet_lines = None
Utils->>Utils: include full content
else max_snippet_lines = 0
Utils->>Utils: omit content
else max_snippet_lines = N
Utils->>Utils: include first N lines as content
end
end
Note over Utils: CHANGED: no nested "chunk"/"location"/"language" fields
Utils-->>Handler: JSON { query, results: [flat dicts] }
Handler-->>CLI: Print JSON (flat)
Handler-->>MCP: JSON-RPC response (flat)
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
|
Greptile SummaryThis PR adds a
Confidence Score: 4/5Safe to merge; the logic is correct and the new parameter is well-tested across all three states. The core truncation logic in result_to_dict has a minor inconsistency: when Some(n) is requested but the chunk content is empty, join produces an empty string and the field is emitted as content: '' rather than being omitted. The resolve_snippet_lines helper is also duplicated verbatim between the two binary modules instead of living in the shared library crate. Both are low-impact and do not affect normal use. crates/csp/src/utils.rs — the Some(n) branch in result_to_dict; crates/csp/src/bin/csp/mcp_server.rs — the duplicated resolve_snippet_lines. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
CLI["CLI: --max-snippet-lines N\nOption i64, default absent = full"]
MCP["MCP: max_snippet_lines\nabsent=Some10, null=None"]
CLI -->|resolve_snippet_lines| R1["Option usize"]
MCP -->|resolve_snippet_lines| R2["Option usize"]
R1 --> FMT["format_results"]
R2 --> FMT
FMT --> RTD["result_to_dict"]
RTD --> NONE{max_snippet_lines}
NONE -->|None| FULL["content: full chunk text"]
NONE -->|Some 0| OMIT["content field omitted"]
NONE -->|Some n| TRUNC["content: first N lines"]
FULL --> OUT["Flat JSON output\nfile_path, start_line, end_line, score, content"]
OMIT --> OUT
TRUNC --> OUT
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
CLI["CLI: --max-snippet-lines N\nOption i64, default absent = full"]
MCP["MCP: max_snippet_lines\nabsent=Some10, null=None"]
CLI -->|resolve_snippet_lines| R1["Option usize"]
MCP -->|resolve_snippet_lines| R2["Option usize"]
R1 --> FMT["format_results"]
R2 --> FMT
FMT --> RTD["result_to_dict"]
RTD --> NONE{max_snippet_lines}
NONE -->|None| FULL["content: full chunk text"]
NONE -->|Some 0| OMIT["content field omitted"]
NONE -->|Some n| TRUNC["content: first N lines"]
FULL --> OUT["Flat JSON output\nfile_path, start_line, end_line, score, content"]
OMIT --> OUT
TRUNC --> OUT
Prompt To Fix All With AIFix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
crates/csp/src/utils.rs:30-33
When `Some(n)` is requested but `content` is an empty string, `lines()` yields nothing, `take(n)` yields nothing, and `join("
")` produces `""` — so the field is emitted as `content: ""` rather than being omitted. This is inconsistent with `Some(0)` which skips the field entirely, and could confuse callers that check `result.content.is_some()` to decide whether to navigate to the file. An explicit guard keeps the three cases consistently distinct.
```suggestion
Some(n) => {
let snippet: Vec<&str> = c.content.lines().take(n).collect();
if !snippet.is_empty() {
entry["content"] = json!(snippet.join("\n"));
}
}
```
### Issue 2 of 2
crates/csp/src/bin/csp/mcp_server.rs:29-31
**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.
Reviews (1): Last reviewed commit: "feat(search): add max_snippet_lines to c..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| Some(n) => { | ||
| let snippet: Vec<&str> = c.content.lines().take(n).collect(); | ||
| entry["content"] = json!(snippet.join("\n")); | ||
| } |
There was a problem hiding this comment.
Some(n) is requested but content is an empty string, lines() yields nothing, take(n) yields nothing, and join(" ") produces "" — so the field is emitted as content: "" rather than being omitted. This is inconsistent with Some(0) which skips the field entirely, and could confuse callers that check result.content.is_some() to decide whether to navigate to the file. An explicit guard keeps the three cases consistently distinct.
| Some(n) => { | |
| let snippet: Vec<&str> = c.content.lines().take(n).collect(); | |
| entry["content"] = json!(snippet.join("\n")); | |
| } | |
| Some(n) => { | |
| let snippet: Vec<&str> = c.content.lines().take(n).collect(); | |
| if !snippet.is_empty() { | |
| entry["content"] = json!(snippet.join("\n")); | |
| } | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/csp/src/utils.rs
Line: 30-33
Comment:
When `Some(n)` is requested but `content` is an empty string, `lines()` yields nothing, `take(n)` yields nothing, and `join("
")` produces `""` — so the field is emitted as `content: ""` rather than being omitted. This is inconsistent with `Some(0)` which skips the field entirely, and could confuse callers that check `result.content.is_some()` to decide whether to navigate to the file. An explicit guard keeps the three cases consistently distinct.
```suggestion
Some(n) => {
let snippet: Vec<&str> = c.content.lines().take(n).collect();
if !snippet.is_empty() {
entry["content"] = json!(snippet.join("\n"));
}
}
```
How can I resolve this? If you propose a fix, please make it concise.
| fn resolve_snippet_lines(value: Option<i64>) -> Option<usize> { | ||
| value.map(|n| n.max(0) as usize) | ||
| } |
There was a problem hiding this comment.
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.
Prompt To Fix With AI
This 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!
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
| /// Serialize a search result to the flat CLI/MCP wire dict — **snake_case** | ||
| /// fields (`file_path`, `start_line`, `end_line`, `score`, optional `content`), | ||
| /// matching semble `utils.format_results` after semble#198. | ||
| /// | ||
| /// `max_snippet_lines` caps the `content` field so agents can spend fewer tokens | ||
| /// confirming a location before navigating to the file: | ||
| /// - `None` → full chunk content | ||
| /// - `Some(0)` → omit `content` entirely (path + line range only) | ||
| /// - `Some(n)` → the first `n` lines of content | ||
| pub fn result_to_dict(result: &SearchResult, max_snippet_lines: Option<usize>) -> Value { |
There was a problem hiding this comment.
[MEDIUM] 중복된 유틸리티 함수 공통화 (resolve_snippet_lines)
Symptom: resolve_snippet_lines 함수가 main.rs와 mcp_server.rs 두 곳에 동일하게 중복 구현되어 있습니다.
Source: Fowler — Refactoring (Duplicate Code) / Hunt & Thomas — Pragmatic Programmer (DRY)
Consequence: 동일한 변환 로직이 여러 곳에 분산되어 있으면, 향후 타입 변경이나 범위 제한 로직 수정 시 일관성이 깨질 위험이 있습니다.
Remedy: 이 함수를 공통 라이브러리 유틸리티인 crates/csp/src/utils.rs에 정의하고, 각 바이너리 모듈에서는 이를 가져와 사용하도록 합니다.
/// Map the CLI/MCP --max-snippet-lines value to the Option<usize> cap. Absent
/// (None) -> full chunk content; a negative value clamps to 0 (no code).
pub fn resolve_snippet_lines(value: Option<i64>) -> Option<usize> {
value.map(|n| n.max(0) as usize)
}
/// Serialize a search result to the flat CLI/MCP wire dict - snake_case
/// fields (file_path, start_line, end_line, score, optional content),
/// matching semble utils.format_results after semble#198.
///
/// max_snippet_lines caps the content field so agents can spend fewer tokens
/// confirming a location before navigating to the file:
/// - None -> full chunk content
/// - Some(0) -> omit content entirely (path + line range only)
/// - Some(n) -> the first n lines of content
pub fn result_to_dict(result: &SearchResult, max_snippet_lines: Option<usize>) -> Value {| /// Map the CLI `--max-snippet-lines` value to the `Option<usize>` cap. Absent | ||
| /// (`None`) → full chunk content; a negative value clamps to `0` (no code). | ||
| fn resolve_snippet_lines(value: Option<i64>) -> Option<usize> { | ||
| value.map(|n| n.max(0) as usize) | ||
| } |
There was a problem hiding this comment.
[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를 가져와 사용하도록 변경합니다.
| /// Map the CLI `--max-snippet-lines` value to the `Option<usize>` cap. Absent | |
| /// (`None`) → full chunk content; a negative value clamps to `0` (no code). | |
| fn resolve_snippet_lines(value: Option<i64>) -> Option<usize> { | |
| value.map(|n| n.max(0) as usize) | |
| } | |
| use csp::utils::resolve_snippet_lines; |
| /// 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) | ||
| } |
There was a problem hiding this comment.
[MEDIUM] 중복 구현된 resolve_snippet_lines 제거 및 공통 함수 사용
Symptom: resolve_snippet_lines 함수가 mcp_server.rs에 로컬로 중복 구현되어 있습니다.
Source: Fowler — Refactoring (Duplicate Code) / Hunt & Thomas — Pragmatic Programmer (DRY)
Consequence: 동일한 유틸리티 로직이 여러 곳에 중복 존재하여 유지보수성이 저하됩니다.
Remedy: 로컬 구현을 제거하고 csp::utils::resolve_snippet_lines를 가져와 사용하도록 변경합니다.
| /// 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) | |
| } | |
| use csp::utils::resolve_snippet_lines; |
1 participant
Addresses #75. Ports upstream semble#198 (
d561953).What & why
semble#198's insight: a search snippet is a locator, not the final content — the agent usually only needs enough to confirm it found the right place, then navigates to the file. Returning full chunks by default wastes tokens.
max_snippet_lineslets results carry a short preview instead.Semantics (
utils::format_results/result_to_dict):None→ full chunk content0→ omitcontent(file path + line range only)N > 0→ firstNlinesSurface:
search/find-related--max-snippet-lines Nsearch/find_relatedmax_snippet_linesMCP default is tri-state via a serde field default: field absent → 10; JSON
null→ full chunk;0→ location only.Wire-shape flatten (faithful to upstream)
Per discussion, this follows upstream faithfully. #198 also flattened the wire dict, so results are now:
{ "file_path": "...", "start_line": 1, "end_line": 9, "score": 0.87, "content": "..." }top-level, dropping the nested
chunkwrapper,location, andlanguage. Context: csp's previous nested shape was itself a faithful mirror of upstream's pre-#198SearchResult.to_dict— upstream reshaped it in #198, so matching it keeps parity. The librarySearchResultstruct is unchanged; only the CLI/MCP JSON envelope changed.Out of scope
save_search_statsinto the search flow at all (CspIndexhas nofile_sizes), so savings telemetry isn't recorded yet and there's nothing for #206 to correct. Wiring savings is a separate pre-existing gap; #206 rides on it. Recommend a dedicated issue.--agent/flags line — deliberately untouched to avoid a merge conflict with docs: correct thecsp --agentlist in CLAUDE.md #79 (which already rewrites that exact line). The--max-snippet-linesflag should be added to that Public API bullet once docs: correct thecsp --agentlist in CLAUDE.md #79 lands.Verification
cargo fmt --all✅ ·cargo clippy --all-targets --all-features -- -D warnings✅cargo test --workspace✅ — 270 lib + 21 CLI, incl. new unit tests for None/N/0 truncation (utils), CLIsearch_output_caps_snippet_lines, MCPsearch_tool_respects_max_snippet_lines_zero, and the tri-state param default (mcp_server).chunk/location);--max-snippet-lines 2→ first 2 lines;--max-snippet-lines 0→ nocontent, location kept.Summary by cubic
Adds a
max_snippet_linescap to search results so they return a short preview instead of full chunks, reducing token usage for agents. Also flattens the results JSON to top-level fields for parity with upstream.New Features
--max-snippet-lines Nonsearchandfind-related. None → full chunk (default), 0 → nocontent, N>0 → first N lines. Negative values clamp to 0.max_snippet_linesparam onsearchandfind_related. Default is 10. Passnullfor full chunk, or0for location-only.Migration
{ file_path, start_line, end_line, score, content? }. The nestedchunk/location/languagefields were removed; update any parsers.null(MCP) or omit the flag (CLI) for full chunks, or0for location-only.Written for commit 3fb210a. Summary will update on new commits.