Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds FTS5/BM25 transcript scoring, a Claude Code ChangesTranscript compaction
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant ClaudeCode
participant HookArgs
participant pre_compact
participant score_lines
ClaudeCode->>HookArgs: invoke hook pre-compact
HookArgs->>pre_compact: dispatch resolved agent
pre_compact->>score_lines: score transcript lines by derived query
score_lines-->>pre_compact: return ranked lines
pre_compact-->>ClaudeCode: print scored JSON
sequenceDiagram
participant MCPClient
participant memory_tool
participant handle_compact
participant score_lines
MCPClient->>memory_tool: submit compact action
memory_tool->>handle_compact: pass lines and options
handle_compact->>score_lines: rank lines by query
score_lines-->>handle_compact: return scored lines
handle_compact-->>MCPClient: return keep flags and counts
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/compact.rs`:
- Around line 62-67: Update the SQL constructed in the compact query around
weights.sql_args() so ORDER BY uses a deterministic secondary key, such as
rowid, after score. Preserve score as the primary ordering and ensure
equal-score rows follow the intended insertion-order behavior.
In `@src/hook.rs`:
- Around line 239-247: Update the scoring flow around score_lines to derive an
actual content-relevance query from the active task or recent user intent,
rather than passing parsed.session_id. Ensure the derived query is available
before scoring and preserves meaningful ranking of the transcript entries.
In `@src/memory/mcp.rs`:
- Around line 248-277: Update handle_compact to consume CompactInput.scorer
according to the advertised backend options: use the selected keyword or FTS5
scoring behavior, and return an error for any unsupported value. If keyword
scoring is not available, remove scorer from CompactInput and the MCP schema
instead of silently ignoring it.
- Around line 295-308: Update the by_relevance construction in the score_lines
selection flow to preserve scored’s BM25 relevance order: deduplicate indices
without sorting them by transcript position, then iterate in the original scored
order while filling keep up to keep_count. Retain the existing duplicate
handling and keep[idx] checks.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 2cfc0b4f-6a33-4d8d-a030-faf80f81dec6
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
Cargo.tomlsrc/cli/hook.rssrc/compact.rssrc/hook.rssrc/init.rssrc/main.rssrc/mcp_server.rssrc/memory/mcp.rs
|
Reviewed (first time — this is new work, not something I looked at as a pre-PR worktree). CI is currently red (`fmt` and `clippy` both fail) — needs fixing regardless of the below, but I found two real correctness bugs worth addressing in the same pass: 1. `handle_compact` sorts away the relevance ranking it just computed (src/memory/mcp.rs)`score_lines` returns `scored` already ordered by BM25 relevance (best match first, per `ORDER BY score` in the SQL). The fill loop is supposed to use that order — the comment says so directly: ```rust `by_relevance.sort()` sorts the line indices, i.e. puts them back into transcript order, throwing away the relevance ordering `scored` was built to provide. The subsequent `for idx in by_relevance` loop then fills the keep-quota with whichever matched lines happen to come earliest in the transcript, not the most relevant ones — the opposite of what the comment and the whole point of BM25-scoring says it should do. This only doesn't matter when `scored.len() <= ` the remaining quota (nothing gets dropped either way); it silently misbehaves any time there are more matches than room to keep them, which is exactly the case compaction exists for. Also, the `dedup()` is solving a problem that can't occur here — `scored` is one row per transcript line from a single FTS5 query, so indices can't repeat. The actual "line might be both recent and high-scored" case is already handled correctly by the `if !keep[idx]` check further down. Suggest just dropping the `.sort()`/`.dedup()` and iterating `scored` (or `scored.iter().map(|s| s.index)`) directly in its original relevance order. 2. The PreCompact hook scores lines against the session ID, not a real query (src/hook.rs)```rust `parsed.session_id` is the session UUID from the hook payload — not natural-language text. FTS5-tokenizing a UUID and matching it against transcript line content will essentially never produce a meaningful hit (a UUID won't appear as a word in the conversation). As written, the automatic `PreCompact` hook path will almost always score everything as non-matching and return an empty/near-empty result — i.e. the hook-triggered compaction is effectively a no-op in real use, even though `score_lines` itself and the MCP `memory(action=compact)` path (which takes an explicit, required `query` param) both work correctly in isolation. This needs a real query — e.g. derived from the most recent user turn(s) in the transcript, or whatever signal is meant to drive "what's relevant to keep." 3. (minor) `score_lines` panics instead of failing softEvery SQLite step in `score_lines` (`crates/.../src/compact.rs`, called from both the hook and the MCP action) is `.expect(...)`'d, including once per transcript line in the insert loop, and inserts aren't wrapped in a transaction (autocommit per row). One bad line anywhere in a real transcript kills scoring for the whole call, which is inconsistent with the rest of `hook.rs`'s fail-soft handling (e.g. the transcript-read error path a few lines above just logs and returns). Given this runs on every compaction of potentially large transcripts, worth converting to soft failures and batching the inserts in one transaction. Please fix fmt/clippy and #1/#2 before merge; #3 is a robustness/perf nit, not blocking. |
- handle_compact: stop re-sorting the relevance-scored line indices back into transcript order before filling the keep quota. That sort discarded the BM25 ranking score_lines had just computed, so whenever there were more matches than room to keep, the earliest matches won instead of the most relevant ones -- the opposite of what compaction is for. Now fills from `scored` in its native relevance order. - pre_compact: stop scoring transcript lines against the session UUID. A UUID isn't natural-language text, so FTS5-matching it against transcript content essentially never hit, making the automatic PreCompact hook path a near no-op. Extracted a testable relevance_query() helper that falls back to the transcript's own last non-empty line (the most recent turn) as a stand-in for "what the user is currently focused on". - score_lines: batch the per-line FTS5 inserts into one transaction instead of one implicit commit per row. - Fix clippy (needless_range_loop, useless_vec, dead_code on the not-yet-wired CompactInput.scorer field) and fmt.
- compact::score_lines: add "ix" as a secondary ORDER BY key so equal- score ties resolve deterministically by transcript position instead of being left to SQLite's unspecified tie order. - handle_compact: reject any CompactInput.scorer other than "fts5" instead of silently ignoring it -- the MCP schema advertises fts5|keyword but keyword scoring was never implemented, so a caller selecting it would have gotten FTS5 behavior with no indication anything was wrong.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/memory/mcp.rs (1)
261-267: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRemove
keywordfrom the advertised MCP contract or implement it.
src/mcp_server.rs:355-364still advertisesscorerasfts5|keyword, while this handler rejectskeyword. Schema-generated clients can therefore submit a documented value that always fails.Possible contract fix
-#[schemars(description = "Scorer backend: fts5|keyword (compact)")] +#[schemars(description = "Scorer backend: fts5 (compact)")]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/memory/mcp.rs` around lines 261 - 267, Align the advertised MCP scorer contract with the validation in the handler: update the scorer schema in the MCP server’s tool definition around the scorer advertisement to remove “keyword,” leaving only “fts5” as supported. Keep the existing validation in the memory handler unchanged.
🧹 Nitpick comments (2)
src/memory/mcp.rs (2)
284-284: 🩺 Stability & Availability | 🔵 Trivial | 🏗️ Heavy liftKeep scoring failures inside the MCP error path.
The handler returns
Result, butscore_linesis invoked as infallible code; the scorer’s reported SQLiteexpectcalls can panic on database or FTS5 failures. Makescore_linesfallible and map its error toErrhere rather than aborting the MCP handler.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/memory/mcp.rs` at line 284, Update score_lines and its caller in the MCP handler to return and propagate scoring errors instead of panicking on SQLite or FTS5 failures. In the handler around the scored assignment, map the fallible result into the existing MCP Result error path, preserving normal scoring behavior on success.
316-325: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAvoid the quadratic score lookup.
scored.iter().find(...)rescans the scored lines for every transcript entry. Build an index-to-score map once, then perform constant-time lookups.Proposed refactor
+ let scores: std::collections::HashMap<usize, _> = + scored.iter().map(|s| (s.index, s.score)).collect(); + let output: Vec<serde_json::Value> = entries .iter() .enumerate() .map(|(i, entry)| { - let score = scored.iter().find(|s| s.index == i).map(|s| s.score); + let score = scores.get(&i).copied();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/memory/mcp.rs` around lines 316 - 325, Update the output construction in the entries iterator to build an index-to-score map from scored once before the map operation, then use constant-time lookups for each entry instead of calling scored.iter().find(...). Preserve the existing optional score behavior when no matching index exists.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@src/memory/mcp.rs`:
- Around line 261-267: Align the advertised MCP scorer contract with the
validation in the handler: update the scorer schema in the MCP server’s tool
definition around the scorer advertisement to remove “keyword,” leaving only
“fts5” as supported. Keep the existing validation in the memory handler
unchanged.
---
Nitpick comments:
In `@src/memory/mcp.rs`:
- Line 284: Update score_lines and its caller in the MCP handler to return and
propagate scoring errors instead of panicking on SQLite or FTS5 failures. In the
handler around the scored assignment, map the fallible result into the existing
MCP Result error path, preserving normal scoring behavior on success.
- Around line 316-325: Update the output construction in the entries iterator to
build an index-to-score map from scored once before the map operation, then use
constant-time lookups for each entry instead of calling scored.iter().find(...).
Preserve the existing optional score behavior when no matching index exists.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: fb294c5a-63db-46dd-a44a-190937dc5a73
📒 Files selected for processing (2)
src/compact.rssrc/memory/mcp.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/compact.rs
…P schema CompactInput.scorer only accepts fts5 (handle_compact rejects anything else), but the schemars description still told callers fts5|keyword was valid -- a schema-generated client could submit a documented value that always errors.
# Conflicts: # Cargo.lock
- Backfill items_fts for items that already existed before this migration ran -- the AFTER INSERT/UPDATE/DELETE triggers only keep the index in sync going forward, so without a backfill every item created before this migration would be permanently invisible to item(search) until it happened to be touched again. - Clamp item::search's limit via flare_search_kit::clamped_limit instead of a raw `limit as i64` cast -- usize::MAX (or any caller-supplied value that wraps negative on cast) turns into SQL "LIMIT -1", which SQLite treats as unlimited, defeating the cap. Same class of bug this codebase already guards against in gateway-registry/skill-registry's own search() via the same helper. - Fix the limit field's schema description, which said "omit for no limit" -- true for item(list), but item(search) defaults to 20 (clamped to 1000) when omitted, not unlimited. - Resolve merge conflicts against master's already-fixed compact.rs/ memory/mcp.rs (this branch predates #192/#195 and carried duplicate, pre-fix copies of that code) by taking master's versions.
…ning free text (#624) Item #170's false-positive class hit twice more in one session (items #192, #173): a description that merely mentions "design-spec" (e.g. referencing another item's spec) forces the review-only prompt even for a genuine implementation task, because nothing ever set the structured metadata.task_type signal detect_review_only already knows how to trust. handoff now accepts an optional task_type and merges it into the item's existing metadata (without clobbering other keys) both when targeting an existing item_id and when creating a new one. Agentflare-Agent: claude-code Agentflare-Branch: task/task-type-metadata-review-only-fix Agentflare-Session: e77fc32e-33d0-4884-ab55-fdda48fe45fd Co-authored-by: shiva <shiva@gosysinfo.tech>
1 participant
Auto-opened on
item donefor 019f6440-8014-7fc0-9f79-008aa4cd390e.Summary by CodeRabbit