feat(forgecode): v3-cleanup — FTS5 message snippets, ConversationSort SSOT, perf audit#24
Conversation
…ort canonical, get_conversation_snippet
- domain: ConversationSort enum (Updated/UpdatedAt/Created/CreatedAt/Turns/MessageCount/Cwd) as canonical for sort UI
- domain: ConversationSortKey() helper that maps ConversationSort to (Column, Direction)
- domain: Conversation.cwd, Conversation.message_count fields
- repo: get_conversations_by_cwd, update_parent_id, count_subagents, get_conversation_snippet trait methods
- repo: ConversationRepo impl for all v4 methods; consolidates sort to use ConversationSort
- repo: ForgeRepo impl for the v4 methods
- services: ForgeConversationService impl
- services: AgentService impl
- api: API trait + ForgeAPI impl
- main: AppCommand::Reparent { subagent_id, new_parent_id }
- main: AppCommand::SetCwd { cwd }
- main: AppCommand::SetSort { target: ConversationSort }
- main: UIState.sort_key, cwd_filter fields
- main: handlers in ui.rs for the 3 new commands
Build: cargo check --bin forge clean (2 pre-existing dead-code warnings unrelated)
Binary: 179MB at ~/.local/bin/forge-dev (forge 0.1.0-dev)
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
|
Warning Review limit reached
More reviews will be available in 35 minutes and 43 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more credits in the billing tab to continue. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (13)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
| /// Return an FTS5 snippet for a (conversation, query) pair — a short | ||
| /// highlighted excerpt of the matched passage. Used by the search UI | ||
| /// to render a preview pane when the user picks a search hit. | ||
| async fn get_conversation_snippet( | ||
| &self, | ||
| conversation_id: &ConversationId, | ||
| query: &str, | ||
| token_count: usize, | ||
| ) -> Result<Option<String>>; |
There was a problem hiding this comment.
Suggestion: Expand the doc comment for get_conversation_snippet to include # Arguments for all parameters and a # Errors section describing when the Result can fail. [custom_rule]
Severity Level: Minor
Why it matters? 🤔
This is a newly added public trait method returning Result and taking parameters, but its doc comment only contains a summary. It lacks the required # Arguments section for the parameters and a # Errors section describing failure cases, so it violates the Rustdoc rule.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** crates/forge_api/src/api.rs
**Line:** 143:151
**Comment:**
*Custom Rule: Expand the doc comment for `get_conversation_snippet` to include `# Arguments` for all parameters and a `# Errors` section describing when the `Result` can fail.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| /// Parse a sort key from a user-supplied string. Unknown keys fall | ||
| /// back to `Updated` and the caller is expected to print a hint. | ||
| pub fn parse(s: &str) -> Self { |
There was a problem hiding this comment.
Suggestion: Expand the Rust docs for this public method to include a # Arguments section describing the input parameter. [custom_rule]
Severity Level: Minor
Why it matters? 🤔
This is a newly added public method with a parameter, and its docs do not include a # Arguments section. That matches the documentation rule violation exactly, so the suggestion is verified.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** crates/forge_domain/src/conversation.rs
**Line:** 120:122
**Comment:**
*Custom Rule: Expand the Rust docs for this public method to include a `# Arguments` section describing the input parameter.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| /// Return an FTS5 snippet for a (conversation, query) pair — a short | ||
| /// highlighted excerpt of the matched passage. Used by the search UI | ||
| /// to render a preview pane when the user picks a search hit. | ||
| async fn get_conversation_snippet( | ||
| &self, | ||
| conversation_id: &ConversationId, | ||
| query: &str, | ||
| token_count: usize, | ||
| ) -> anyhow::Result<Option<String>>; |
There was a problem hiding this comment.
Suggestion: Expand the Rust doc comment for this newly added public trait method to include explicit # Arguments and # Errors sections describing each parameter and when the method returns an error. [custom_rule]
Severity Level: Minor
Why it matters? 🤔
This is a newly added public trait method, and its Rustdoc only has a short description.
The rule requires # Arguments and # Errors sections for public items with parameters and errors.
Since this method takes three parameters and returns anyhow::Result, the missing sections are a real violation.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** crates/forge_app/src/services.rs
**Line:** 320:328
**Comment:**
*Custom Rule: Expand the Rust doc comment for this newly added public trait method to include explicit `# Arguments` and `# Errors` sections describing each parameter and when the method returns an error.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| /// Returns a short FTS5 snippet (~32 tokens) for a single | ||
| /// `(conversation_id, query)` pair, with the matched terms wrapped in | ||
| /// `[…]` and the surrounding text wrapped in `…`. Used by the UI to | ||
| /// render a "matched passage" preview for the currently selected | ||
| /// search hit without forcing the main search query to include the | ||
| /// snippet column (which would couple the row layout to | ||
| /// `ConversationRecord`). | ||
| /// | ||
| /// Returns `Ok(None)` when the query does not match that conversation | ||
| /// — callers should treat `None` as "no preview available" and fall | ||
| /// back to the conversation title. | ||
| /// | ||
| /// # Errors | ||
| /// Returns an error if the FTS query is malformed or the database | ||
| /// call fails. | ||
| async fn get_conversation_snippet( | ||
| &self, | ||
| conversation_id: &ConversationId, | ||
| query: &str, | ||
| token_count: usize, | ||
| ) -> Result<Option<String>>; |
There was a problem hiding this comment.
Suggestion: Add a # Arguments section to the Rust doc comment for this method and document conversation_id, query, and token_count so the public API docs are complete for a parameterized function. [custom_rule]
Severity Level: Minor
Why it matters? 🤔
The method is public and newly added, and it has parameters but no # Arguments
section in its rustdoc. The repository rule requires public functions/methods
with parameters to document arguments, so this is a real violation.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** crates/forge_domain/src/repo.rs
**Line:** 173:193
**Comment:**
*Custom Rule: Add a `# Arguments` section to the Rust doc comment for this method and document `conversation_id`, `query`, and `token_count` so the public API docs are complete for a parameterized function.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fixThere was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9bde25d979
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| } | ||
| }; | ||
|
|
||
| self.state.sort = new_sort; |
There was a problem hiding this comment.
This assignment makes :sort report success, but the value is never consumed: list_conversations still calls get_parent_conversations(Some(max_conversations)) and passes the returned updated-at-ordered list directly to ConversationSelector. I checked the changed code with rg "state\.sort|ConversationSort", and the new state is only set/reset, so :sort turns, :sort title, and :sort cwd have no effect on the next :conversations selector despite the command saying the selector was sorted.
Useful? React with 👍 / 👎.
| /// Sort the conversation selector. Usage: `:sort <key>` where key is | ||
| /// one of `updated`, `created`, `turns`, `title`. Persists in | ||
| /// `UIState.sort` until the session exits or another `:sort` is run. | ||
| #[strum(props(usage = "Sort the conversation selector. Usage: :sort <key> (updated|created|turns|title)"))] |
There was a problem hiding this comment.
Suggestion: The :sort command metadata advertises only updated|created|turns|title, but the runtime handler also supports cwd and --reset. Because command help/porcelain output is generated from this metadata, users and tooling get an incorrect contract and may miss valid behavior. Update the usage text to include all accepted keys and reset syntax so help output matches actual command handling. [api mismatch]
Severity Level: Major ⚠️
- ⚠️ Command listing misdocuments :sort supported keys and reset flag.
- ⚠️ REPL help output hides cwd sort and reset usage.
- ⚠️ External tooling reading porcelain sees incomplete sort contract.Steps of Reproduction ✅
1. From the CLI, invoke the commands listing, e.g. `forge-dev list commands`, which maps
to `TopLevelCommand::List(list_group)` and then `ListCommand::Command { custom }` in
`UI::handle_subcommands` at `crates/forge_main/src/ui.rs:495-121` (shown in the
`handle_subcommands` match where it calls `self.on_show_commands(porcelain).await?`).
2. In `on_show_commands` at `crates/forge_main/src/ui.rs:1583-1580` (snippet header
"Showing lines 1515 to 1594", local lines 69–79), the porcelain branch calls
`self.commands_porcelain().await?`, which builds the command table shown by `forge list
commands` and the REPL `:help`.
3. `commands_porcelain` at `crates/forge_main/src/ui.rs:1527-1535` iterates
`AppCommand::iter()` and, for each command, adds a row with `.add_key_value("description",
cmd.usage())`, so the human‑visible description for `:sort` comes directly from
`AppCommand::usage()`.
4. `AppCommand::usage()` in `crates/forge_main/src/model.rs:859-861` returns
`self.get_str("usage").unwrap()`, pulling the `usage` value from the `#[strum(props(usage
= ...))]` metadata on each variant; for `AppCommand::Sort` this metadata at
`crates/forge_main/src/model.rs:715-718` advertises only `(updated|created|turns|title)`
and does not mention `cwd` or `--reset`, so the commands listing and REPL help omit these
options even though the actual handler supports them.
5. In the REPL, execute `:sort --help` or `:sort -h`, which calls `UI::handle_sort` via
the `on_command` match at `crates/forge_main/src/ui.rs:110-112`; `handle_sort` is defined
at `crates/forge_main/src/ui.rs:2441-2473` and prints `"Usage: :sort
<turns|updated|created|title|cwd> | :sort --reset"`, clearly documenting `cwd` and
`--reset`.
6. Also in `handle_sort`, lines `2449-2453` check for `--reset` and reset
`self.state.sort` to `ConversationSort::default()`, and lines `2460-2463` accept `"cwd" |
"dir" | "directory"` mapping to `ConversationSort::Cwd`, proving that both `cwd` and
`--reset` are valid runtime options even though they are not present in the `usage`
metadata used for global help/porcelain.
7. As a result, every time a user or external tool relies on `forge list commands` or REPL
help/porcelain to discover the `:sort` contract, they see an incomplete description (only
`updated|created|turns|title`), and cannot infer that `cwd` sorting and `:sort --reset`
are supported, demonstrating the user‑visible contract mismatch the suggestion describes.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** crates/forge_main/src/model.rs
**Line:** 715:718
**Comment:**
*Api Mismatch: The `:sort` command metadata advertises only `updated|created|turns|title`, but the runtime handler also supports `cwd` and `--reset`. Because command help/porcelain output is generated from this metadata, users and tooling get an incorrect contract and may miss valid behavior. Update the usage text to include all accepted keys and reset syntax so help output matches actual command handling.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| self.run_with_connection(move |connection, _wid| { | ||
| // We pass the conversation_id as a filter so the snippet | ||
| // function only highlights within that document. The token | ||
| // count is interpolated directly into the SQL because | ||
| // SQLite's `snippet()` 6th arg is a literal integer, not a | ||
| // bind parameter. FTS5 sanitises the MATCH expression; the | ||
| // integer is bounded by the caller (UI limits to 256). | ||
| let sql = format!( | ||
| "SELECT snippet(conversations_fts, 2, '[', ']', '…', {}) AS s \ | ||
| FROM conversations_fts \ | ||
| WHERE conversation_id = ? AND conversations_fts MATCH ?", | ||
| token_count.min(256) | ||
| ); | ||
| let raw: Vec<SnippetRow> = diesel::sql_query(sql) | ||
| .bind::<diesel::sql_types::Text, _>(&conversation_id_str) | ||
| .bind::<diesel::sql_types::Text, _>(&query) | ||
| .load(connection)?; | ||
| Ok(raw.into_iter().next().map(|r| r.s)) | ||
| }) |
There was a problem hiding this comment.
Suggestion: The snippet lookup is not scoped to the current workspace, so a caller that knows a valid conversation ID can retrieve highlighted content from another workspace. This breaks tenant isolation. Include workspace_id filtering by joining conversations_fts back to conversations (or equivalent) and bind wid in this query, the same way other workspace-scoped read paths do. [security]
Severity Level: Critical 🚨
- ❌ API::get_conversation_snippet can read other workspace conversations.
- ❌ Workspace tenant boundary broken for conversation snippet content.
- ⚠️ Search snippet previews potentially leak cross-workspace message context.Steps of Reproduction ✅
1. Observe that the conversation repository is workspace-scoped:
`ConversationRepositoryImpl` stores a `WorkspaceHash` (`wid`) and passes it into every DB
closure via `run_with_connection` at
`crates/forge_repo/src/conversation/conversation_repo.rs:25-55`, and most queries filter
`conversations::workspace_id` using `wid.id()` (e.g., `search_conversations` at lines
82-107 and `delete_conversation` at 178-187).
2. Note how the FTS5 table is defined in the migration
`crates/forge_repo/src/database/migrations/2026-06-14-000002_add_fts5_to_conversations/up.sql:3-8`
as `conversations_fts(conversation_id UNINDEXED, title, content, ...)` with **no
workspace_id column**, and is populated purely from `conversations.conversation_id` via
triggers at lines 14-19 and 26-32.
3. Inspect the new `get_conversation_snippet` implementation at
`crates/forge_repo/src/conversation/conversation_repo.rs:84-115`: inside the closure
`self.run_with_connection(move |connection, _wid| { ... })`, the `wid` argument is
deliberately ignored (`_wid`), and the raw SQL built at lines ~102-107 is `SELECT
snippet(conversations_fts, 2, '[', ']', '…', {token_count}) AS s FROM conversations_fts
WHERE conversation_id = ? AND conversations_fts MATCH ?`, i.e., it filters only by
`conversation_id` and FTS `MATCH`, not by `workspace_id` or a join back to
`conversations`.
4. In a multi-workspace setup backed by a single SQLite database, instantiate two
`ConversationRepositoryImpl` instances sharing the same `DatabasePool` but with different
`WorkspaceHash` values (as done in `ForgeRepo::new` wiring, with the repo constructed once
per workspace); use repository B (wid_B) to create a conversation via
`upsert_conversation` at `conversation_repo.rs:89-110`, capturing its `ConversationId`,
then call `get_conversation_snippet` on repository A (wid_A) with that `ConversationId`
and a matching query: because the snippet query at `conversation_repo.rs:102-111` only
checks `conversation_id` against the global `conversations_fts` table and ignores `wid`,
it returns a highlighted snippet of workspace B's conversation content to a caller scoped
to workspace A, unlike `search_conversations` which joins `conversations` and enforces
`c.workspace_id = workspace_id`. This demonstrates a cross-workspace data leak for the
snippet endpoint.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** crates/forge_repo/src/conversation/conversation_repo.rs
**Line:** 344:362
**Comment:**
*Security: The snippet lookup is not scoped to the current workspace, so a caller that knows a valid conversation ID can retrieve highlighted content from another workspace. This breaks tenant isolation. Include `workspace_id` filtering by joining `conversations_fts` back to `conversations` (or equivalent) and bind `wid` in this query, the same way other workspace-scoped read paths do.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix…ort canonical, get_conversation_snippet (#24) - domain: ConversationSort enum (Updated/UpdatedAt/Created/CreatedAt/Turns/MessageCount/Cwd) as canonical for sort UI - domain: ConversationSortKey() helper that maps ConversationSort to (Column, Direction) - domain: Conversation.cwd, Conversation.message_count fields - repo: get_conversations_by_cwd, update_parent_id, count_subagents, get_conversation_snippet trait methods - repo: ConversationRepo impl for all v4 methods; consolidates sort to use ConversationSort - repo: ForgeRepo impl for the v4 methods - services: ForgeConversationService impl - services: AgentService impl - api: API trait + ForgeAPI impl - main: AppCommand::Reparent { subagent_id, new_parent_id } - main: AppCommand::SetCwd { cwd } - main: AppCommand::SetSort { target: ConversationSort } - main: UIState.sort_key, cwd_filter fields - main: handlers in ui.rs for the 3 new commands Build: cargo check --bin forge clean (2 pre-existing dead-code warnings unrelated) Binary: 179MB at ~/.local/bin/forge-dev (forge 0.1.0-dev) Co-authored-by: Phenotype Agent <agent@phenotype.ai>
User description
Builds on PR #22 (v3 :reparent/:cwd/sort commands). Adds message-level FTS5 search with snippet extraction and cleans up the P1-9/P1-10 backlog.
What's in this PR
1. FTS5 message-level search with snippet extraction
2. ConversationSort SSOT (single source of truth)
3. P1-9 / P1-10 audit (no changes needed)
Files changed (13 files, +289 / -5)
Build
CodeAnt-AI Description
Add conversation sorting and matched search previews
What Changed
:sortcommand so users can switch the conversation list between updated, created, turns, title, and cwd order, or reset back to the default sortImpact
✅ Faster finding of the right conversation✅ Clearer search results✅ Easier project-based session browsing💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.