# Code Review: 96 Commits (2026-03-03 to 2026-03-05)

Comprehensive review of 8 workstreams: LSP/symbol fixes, tool restructure
(32->23), prompt rewrite, semantic memories, GitHub tool consolidation (44->5),
codescout rename, buffer ref resolution, and run_command improvements.

---

## Must-Fix

### MF-1: `extract_title` can panic on multi-byte UTF-8 at byte 80

**Location:** `src/tools/memory.rs:218-230`

**Problem:** `content[..end]` indexes at byte position 80, which can split a
multi-byte character and panic. This is the same class of bug as BUG-024
(`truncate_compact`), which was just fixed in the same 2-day window.

```rust
let end = first_sentence_end.min(80).min(content.len());
let mut title = content[..end].to_string();  // panics if byte 80 is mid-char
```

**Fix:** Use `floor_char_boundary` (already implemented in `src/tools/mod.rs:185`)
or `content.char_indices()` to find a safe boundary. Consider extracting a shared
`safe_truncate(s, max_bytes) -> &str` utility since this pattern recurs in
`truncate_compact`, `extract_title`, and potentially future code.

---

### MF-2: `search_memories` bucket filter post-filters KNN results

**Location:** `src/embed/index.rs:340-346`

**Problem:** When a bucket filter is provided, `WHERE m.bucket = ?3` runs *after*
the KNN subquery has already returned its top-k results. If you have 100 memories
across 4 buckets and request `limit=5, bucket="code"`, the KNN subquery returns
the 5 nearest neighbors regardless of bucket, then the outer WHERE filters. You
could get 0-5 results, missing relevant memories ranked 6th-10th overall.

```sql
SELECT m.id, m.bucket, m.title, m.content, m.created_at,
       COALESCE(knn.distance, 1.0) AS distance
FROM memories m JOIN (...knn...) knn ON m.id = knn.rowid
WHERE m.bucket = ?3 ORDER BY distance ASC
```

**Fix:** Either (a) over-fetch in the inner KNN query (e.g. `limit * 4`) then
filter and re-limit in the outer query, or (b) document this as a known
limitation in the tool description. Option (a) is preferred since the memory
table is expected to be small.

---

### MF-3: `ensure_vec_memories` silently no-ops on fresh databases

**Location:** `src/embed/index.rs:242-245`

**Problem:** When `embedding_dims` is absent in the meta table (fresh database,
no index built yet), `ensure_vec_memories` returns `Ok(())` without creating the
`vec_memories` table. Then `remember` -> `insert_memory` crashes with "no such
table: vec_memories".

The cross-embed path is safe (wrapped in non-fatal error handling), but the
direct `remember` action propagates the error to the user.

```rust
None => return Ok(()),  // silently skips table creation
```

**Fix:** When `embedding_dims` is absent, either (a) return a `RecoverableError`
with a hint like "run `index_project` first to enable semantic memory", or
(b) store the memory in the `memories` table without an embedding (skip
`vec_memories` insert) so the data is at least persisted for later embedding.

---

## Should-Fix

### SF-1: `GithubRepo::create` and `fork` pass `--json` without field names

**Location:** `src/tools/github.rs` (create ~L1101, fork nearby)

**Problem:** The `--json` flag for `gh repo create` expects a comma-separated
list of fields (e.g. `"name,url,description"`). Passing `--json` alone may cause
`gh` to error or return an empty response.

**Fix:** Add a default field list: `--json name,url,description,visibility` for
create, and similar for fork.

---

### SF-2: `push_files` vs `create_or_update` encoding inconsistency

**Location:** `src/tools/github.rs` (GithubFile tool)

**Problem:** `create_or_update`'s input schema describes content as
"base64-encoded file content" but `push_files` passes content as plaintext via
the Trees API (`"content": f["content"]`). The schema description for `push_files`
says just `"{path, content} array"` without specifying encoding. LLMs will get
confused about which encoding to use.

**Fix:** Align the descriptions. Either both should accept plaintext (and
`create_or_update` should base64-encode internally), or the descriptions should
clearly state the encoding for each method.

---

### SF-3: Tmpfile path is predictable (symlink risk on multi-user systems)

**Location:** `src/tools/workflow.rs:897`

**Problem:** The unfiltered-output tmpfile path
`/tmp/codescout-unfiltered-{nanos:016x}` uses nanosecond timestamp as the sole
source of uniqueness. On multi-user systems, a local attacker could pre-create a
symlink at the predicted path to redirect tee output.

**Fix:** Use `tempfile::NamedTempFile` (already in the dependency tree via other
crates, or add it). The `TmpfileGuard` RAII pattern would wrap the
`NamedTempFile`'s path instead of a manually constructed path.

---

### SF-4: Tmpfile path not shell-escaped in tee injection

**Location:** `src/tools/workflow.rs:898-903`

**Problem:** The tmpfile path is interpolated directly into a shell command string
via `format!("{} | tee {} | {}", ...)`. The current path format
(`/tmp/codescout-unfiltered-{hex}`) contains no shell-special characters, so this
is safe today. But if the format ever changes to include user-controlled
components, it becomes a shell injection vector.

**Fix:** Apply `shell_escape::escape()` to the tmpfile path, or at minimum add a
comment documenting the invariant (path contains only `/`, alphanumeric, `-`).
If SF-3 is fixed with `tempfile::NamedTempFile`, the generated path will use
random alphanumeric chars, which is also safe — but the shell-escape would be
defensive regardless.

---

### SF-5: Partial line range silently ignored in `read_file`

**Location:** `src/tools/file.rs` (~L330)

**Problem:** `read_file(path, start_line=10)` without `end_line` sets
`has_partial_range = true`, which skips the buffering path. But it then falls
through to the exploring-mode cap (first 200 lines) with no error or hint that
`start_line` was ignored.

**Fix:** Return a `RecoverableError` when only one of `start_line`/`end_line` is
provided, with a hint like "both start_line and end_line are required".

---

## Suggestions

### SG-1: Remove dead code from tool restructure

**Location:** `src/tools/memory.rs`, `src/tools/ast.rs`, `src/tools/semantic.rs`,
`src/tools/git.rs`, `src/tools/library.rs`, `src/tools/usage.rs`

**Problem:** The following structs are still `pub` and fully implemented but no
longer registered in `server.rs`:
- `WriteMemory`, `ReadMemory`, `ListMemories`, `DeleteMemory`
- `ListFunctions`, `ListDocs`
- `IndexStatus`
- `GitBlame`
- `IndexLibrary`
- `GetUsageStats`

The integration test at `tests/integration.rs:174` still imports old API structs.
Keeping them `pub` is misleading since they're unreachable via MCP.

**Fix:** Either remove the old structs entirely (migrating any tests to use the
new consolidated APIs), or mark them `#[deprecated]` / `pub(crate)`. Schedule
for the next cleanup sprint.

---

### SG-2: Re-running `validate_symbol_range` for error message recovery

**Location:** `src/tools/symbol.rs:833`

**Problem:** After the document_symbols fallback fails, the code re-runs
`validate_symbol_range` purely to reconstruct the error message. If the file
changes on disk between the first and second call, the function could succeed
and execution falls through to a generic `anyhow::bail!`. This is a TOCTOU gap.

**Fix:** Capture the original error from the first `validate_symbol_range` call
and propagate it directly, avoiding the redundant call.

---

### SG-3: Extract `require_number` helper in GitHub tools

**Location:** `src/tools/github.rs` (throughout)

**Problem:** The pattern
`number.as_deref().ok_or_else(|| RecoverableError::with_hint("number required", ...))`
is repeated ~25 times across the file.

**Fix:** Extract a helper:
```rust
fn require_number<'a>(number: &'a Option<String>, method: &str) -> Result<&'a str> {
    number.as_deref().ok_or_else(|| RecoverableError::with_hint(
        "number required",
        format!("{method} requires the 'number' parameter"),
    ).into())
}
```

---

### SG-4: `delete_memory`/`forget` silently succeeds on nonexistent IDs

**Location:** `src/embed/index.rs:352-356`

**Problem:** `delete_memory` executes two DELETEs without checking
`changes()`. Passing a nonexistent `id` to `forget` returns `json!("ok")`
silently.

**Fix:** Check `conn.changes()` after the DELETE and return a `RecoverableError`
like "memory with id {id} not found" if zero rows were affected.

---

### SG-5: CWD test uses process-global `set_current_dir`

**Location:** `src/util/path_security.rs:679-691`

**Problem:** The `validate_write_path` test calls `std::env::set_current_dir()`,
which affects the entire process. If tests run in parallel (Rust default), another
test calling `current_dir()` during this test's window gets the wrong CWD. A
panic between set and restore would leave it changed.

**Fix:** Add `#[serial]` from the `serial_test` crate, or restructure the test to
avoid changing the process CWD (e.g. pass the CWD as a parameter to the
validation function).

---

### SG-6: `detect_terminal_filter` undocumented `|&` behavior

**Location:** `src/tools/command_summary.rs`

**Problem:** Bash's `|&` (stderr pipe) would result in `&grep` as the filter
name, which doesn't match the filter list and correctly falls through to `None`.
This is safe behavior but undocumented and untested.

**Fix:** Add a test case: `assert!(detect_terminal_filter("cmd |& grep foo").is_none())`
with a comment explaining why `|&` is intentionally not supported.

---

### SG-7: Drift temp table not cleaned up on error path

**Location:** `src/embed/drift.rs:146`

**Problem:** `DROP TABLE IF EXISTS _drift_old` runs after the query loop. If the
loop fails and `?` propagates, the temp table leaks. Since it's `CREATE TEMP
TABLE`, it's cleaned on connection close, but for long-lived connections it
persists.

**Fix:** Use a `scopeguard` or `defer!` pattern to ensure cleanup runs on all
exit paths.

---

### SG-8: `ProjectStatus` drift items use `json!({})` instead of ordered maps

**Location:** `src/tools/config.rs` (~L183)

**Problem:** Inconsistent with the field-ordering effort applied to
`symbol_to_json`, `format_search_result_item`, and `BlameLine` (which all use
explicit `Map::new()` with ordered inserts).

**Fix:** Convert to the `Map::new()` pattern for consistency.

---

### SG-9: `command_summary.rs` not listed in CLAUDE.md project structure

**Location:** `CLAUDE.md`

**Problem:** The new file `src/tools/command_summary.rs` (smart command output
summarization) is not listed in the project structure section.

**Fix:** Add entry under `src/tools/`:
```
│   ├── command_summary.rs  #   Smart output summarization, terminal filter detection
```

---

### SG-10: `as_u64_lenient` should be a shared utility

**Location:** `src/tools/file.rs:12-15`

**Problem:** This function handles the MCP client quirk of sending integers as
strings. Currently only used in `file.rs`, but other tools will likely encounter
the same issue.

**Fix:** Move to `src/tools/mod.rs` or `src/util/` when a second use site
appears. No action needed immediately — just noting for awareness.

---

## Cross-Cutting Observations

### UTF-8 boundary handling is a recurring risk

Three separate boundary issues surfaced in this window:
- `truncate_compact` (BUG-024, fixed)
- `extract_title` (MF-1, not yet fixed)
- `pipe_pos` slice in tee injection (safe because `|` is ASCII, but undocumented)

**Recommendation:** Create a shared `safe_truncate(s: &str, max_bytes: usize) -> &str`
utility using `floor_char_boundary` and use it everywhere byte-level truncation
occurs.

### Re-buffering loop is an architectural tension

The `call_content` auto-buffering (anything over `TOOL_OUTPUT_BUFFER_THRESHOLD`)
conflicts with tools that need large responses to stay inline. Fixes are scattered
across:
- `read_file`: proactive file buffering before `call_content` sees it
- `run_command`: `buffer_only` byte budget
- `onboarding`: custom `call_content` override

**Recommendation:** Document this pattern in `docs/PROGRESSIVE_DISCOVERABILITY.md`
as a "tools producing large output" section, explaining when to pre-buffer vs.
override `call_content`.

### Dead tool structs create dual API surface

The 10 unregistered tool structs (SG-1) and their tests create maintenance burden.
Tests use both old (`WriteMemory.call(...)`) and new (`Memory.call(json!({"action": "write"}))`)
APIs.

**Recommendation:** Schedule a cleanup sprint to migrate tests to new APIs and
either remove or `pub(crate)` the old structs.
