-
Notifications
You must be signed in to change notification settings - Fork 0
feat(forgecode): v3 — :reparent/:cwd commands, sort UI, cwd+message_count migration #22
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
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 |
|---|---|---|
|
|
@@ -122,6 +122,24 @@ pub trait API: Sync + Send { | |
| /// disk footprint. Safe to call at any time; safe to call repeatedly. | ||
| async fn optimize_fts_index(&self) -> Result<()>; | ||
|
|
||
| /// Re-binds a subagent conversation to a different parent. Pass `None` | ||
| /// for `new_parent_id` to detach (promotes the subagent to a top-level | ||
| /// session). Atomic single-row update; does not recurse into descendants. | ||
| async fn update_parent_id( | ||
| &self, | ||
| conversation_id: &ConversationId, | ||
| new_parent_id: Option<&ConversationId>, | ||
| ) -> Result<()>; | ||
|
|
||
| /// Retrieves conversations whose `cwd` column matches the given path | ||
| /// exactly. Used by the session viewer to filter by current working | ||
| /// directory (per-project scoping). | ||
| async fn get_conversations_by_cwd( | ||
| &self, | ||
| cwd: &str, | ||
| limit: Option<usize>, | ||
| ) -> Result<Option<Vec<Conversation>>>; | ||
|
Comment on lines
+134
to
+141
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. Suggestion: Update this new public method's docs to include Severity Level: Minor Why it matters? 🤔This is also a newly added public method with parameters and a (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:** 134:141
**Comment:**
*Custom Rule: Update this new public method's docs to include `# Arguments` for `cwd` and `limit`, and `# Errors` to describe failure conditions.
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 |
||
|
|
||
| /// Renames a conversation by setting its title | ||
| /// | ||
| /// # Arguments | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -242,6 +242,24 @@ impl< | |
| self.services.optimize_fts_index().await | ||
| } | ||
|
|
||
| async fn update_parent_id( | ||
| &self, | ||
| conversation_id: &ConversationId, | ||
| new_parent_id: Option<&ConversationId>, | ||
| ) -> Result<()> { | ||
| self.services | ||
| .update_parent_id(conversation_id, new_parent_id) | ||
| .await | ||
| } | ||
|
Comment on lines
+245
to
+253
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. Suggestion: This new reparent API path blindly forwards success from lower layers and does not enforce the "conversation must exist" contract, so invalid IDs can be reported as successful operations when the DB update affects zero rows. Ensure the repository/service returns a not-found error on zero affected rows and propagate it here. [logic error] Severity Level: Major
|
||
|
|
||
| async fn get_conversations_by_cwd( | ||
| &self, | ||
| cwd: &str, | ||
| limit: Option<usize>, | ||
| ) -> Result<Option<Vec<Conversation>>> { | ||
| self.services.get_conversations_by_cwd(cwd, limit).await | ||
| } | ||
|
|
||
| async fn rename_conversation( | ||
| &self, | ||
| conversation_id: &ConversationId, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -298,6 +298,24 @@ pub trait ConversationService: Send + Sync { | |
| /// back into a single segment, reducing query-time shadow-walk cost and | ||
| /// disk footprint. Safe to call at any time; safe to call repeatedly. | ||
| async fn optimize_fts_index(&self) -> anyhow::Result<()>; | ||
|
|
||
| /// Re-binds a subagent conversation to a different parent. Pass `None` | ||
| /// for `new_parent_id` to detach (promotes the subagent to a top-level | ||
| /// session). Atomic single-row update; does not recurse into descendants. | ||
| async fn update_parent_id( | ||
| &self, | ||
| conversation_id: &ConversationId, | ||
| new_parent_id: Option<&ConversationId>, | ||
| ) -> anyhow::Result<()>; | ||
|
Comment on lines
+302
to
+309
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. Suggestion: Expand the method documentation to include both Severity Level: Minor Why it matters? 🤔This is a newly added public trait method with parameters and a fallible (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:** 302:309
**Comment:**
*Custom Rule: Expand the method documentation to include both `# Arguments` and `# Errors` sections since this public API takes parameters and returns a fallible result.
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 |
||
|
|
||
| /// Retrieves conversations whose `cwd` column matches the given path | ||
| /// exactly. Used by the session viewer to filter by current working | ||
| /// directory (per-project scoping). | ||
| async fn get_conversations_by_cwd( | ||
| &self, | ||
| cwd: &str, | ||
| limit: Option<usize>, | ||
| ) -> anyhow::Result<Option<Vec<Conversation>>>; | ||
|
Comment on lines
+311
to
+318
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. Suggestion: Update the documentation to include Severity Level: Minor Why it matters? 🤔This is also a newly added public method with parameters and a fallible (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:** 311:318
**Comment:**
*Custom Rule: Update the documentation to include `# Arguments` and `# Errors` sections so this new public method meets the required Rustdoc format for parameterized fallible APIs.
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 |
||
| } | ||
|
|
||
| #[async_trait::async_trait] | ||
|
|
@@ -723,6 +741,26 @@ impl<I: Services> ConversationService for I { | |
| async fn optimize_fts_index(&self) -> anyhow::Result<()> { | ||
| self.conversation_service().optimize_fts_index().await | ||
| } | ||
|
|
||
| async fn update_parent_id( | ||
| &self, | ||
| conversation_id: &ConversationId, | ||
| new_parent_id: Option<&ConversationId>, | ||
| ) -> anyhow::Result<()> { | ||
| self.conversation_service() | ||
| .update_parent_id(conversation_id, new_parent_id) | ||
| .await | ||
| } | ||
|
|
||
| async fn get_conversations_by_cwd( | ||
| &self, | ||
| cwd: &str, | ||
| limit: Option<usize>, | ||
| ) -> anyhow::Result<Option<Vec<Conversation>>> { | ||
| self.conversation_service() | ||
| .get_conversations_by_cwd(cwd, limit) | ||
| .await | ||
| } | ||
| } | ||
| #[async_trait::async_trait] | ||
| impl<I: Services> ProviderService for I { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -48,6 +48,18 @@ pub struct Conversation { | |
| pub metadata: MetaData, | ||
| pub parent_id: Option<ConversationId>, | ||
| pub source: Option<String>, | ||
| /// Working directory of the agent when the conversation was created. | ||
| /// Used for grouping / filtering in the session selector and for FTS5 | ||
| /// search so a user can find sessions by cwd fragment (e.g. "forgecode"). | ||
| #[serde(default, skip_serializing_if = "Option::is_none")] | ||
| pub cwd: Option<String>, | ||
| /// Number of message entries in `context.messages` at the time of the | ||
| /// last write. Used to display a turn count in the session selector | ||
| /// and as a stable secondary sort key when the user picks "by turns". | ||
| /// Kept as a column (not a derived getter) so the selector does not | ||
| /// have to deserialize the full Context blob for every row. | ||
| #[serde(default, skip_serializing_if = "Option::is_none")] | ||
| pub message_count: Option<i32>, | ||
|
Comment on lines
+54
to
+62
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. Suggestion: Since these newly added optional configuration fields are part of a setter-driven domain struct, update the struct-level setter derive configuration to include Severity Level: Minor Why it matters? 🤔The Conversation struct is a domain data type with (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:** 54:62
**Comment:**
*Custom Rule: Since these newly added optional configuration fields are part of a setter-driven domain struct, update the struct-level setter derive configuration to include `strip_option` (alongside `into`) so callers can set plain values without wrapping in `Some(...)`.
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 |
||
| } | ||
|
|
||
| #[derive(Debug, Setters, Serialize, Deserialize, Clone)] | ||
|
|
@@ -75,6 +87,8 @@ impl Conversation { | |
| context: None, | ||
| parent_id: None, | ||
| source: None, | ||
| cwd: None, | ||
|
Comment on lines
89
to
+90
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.
New conversations still default Useful? React with 👍 / 👎. |
||
| message_count: None, | ||
| } | ||
| } | ||
| /// Creates a new conversation with a new conversation ID. | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -14,6 +14,10 @@ pub struct UIState { | |
| pub goal: Option<String>, | ||
| pub loop_enabled: bool, | ||
| pub last_activity: Instant, | ||
| /// CWD filter for the conversation selector. When set, the selector | ||
| /// scopes its results to conversations whose `cwd` column matches. | ||
| /// This is the "filter by project directory" UX. | ||
| pub cwd_filter: Option<String>, | ||
|
Comment on lines
+17
to
+20
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. Suggestion: The new cwd filter state is introduced but never read anywhere in the conversation listing flow, so setting Severity Level: Major
|
||
| } | ||
|
Comment on lines
+17
to
21
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. |
||
|
|
||
| impl Default for UIState { | ||
|
|
@@ -24,12 +28,20 @@ impl Default for UIState { | |
| goal: None, | ||
| loop_enabled: false, | ||
| last_activity: Instant::now(), | ||
| cwd_filter: None, | ||
| } | ||
| } | ||
| } | ||
|
|
||
| impl UIState { | ||
| pub fn new(env: Environment) -> Self { | ||
| Self { cwd: env.cwd, conversation_id: Default::default(), goal: None, loop_enabled: false, last_activity: Instant::now() } | ||
| Self { | ||
| cwd: env.cwd, | ||
| conversation_id: Default::default(), | ||
| goal: None, | ||
| loop_enabled: false, | ||
| last_activity: Instant::now(), | ||
| cwd_filter: None, | ||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2328,6 +2328,101 @@ impl<A: API + ConsoleWriter + 'static, F: Fn(ForgeConfig) -> A + Send + Sync> UI | |
| Ok(()) | ||
| } | ||
|
|
||
| /// Re-binds the current (subagent) conversation to a different parent. | ||
| /// Usage: | ||
| /// - `:reparent <parent_id>` → attach to the given parent | ||
| /// - `:reparent --detach` → promote this session to top-level | ||
| /// - `:reparent` → no-arg; shows usage hint | ||
| async fn handle_reparent(&mut self, target: Vec<String>) -> anyhow::Result<()> { | ||
| let conversation_id = match self.state.conversation_id { | ||
| Some(id) => id, | ||
| None => { | ||
| self.writeln_title(TitleFormat::error( | ||
| "No active session. Start a conversation first.", | ||
| ))?; | ||
| return Ok(()); | ||
| } | ||
| }; | ||
|
|
||
| if target.is_empty() { | ||
| self.writeln_title(TitleFormat::info( | ||
| "Usage: :reparent <parent_id> | :reparent --detach", | ||
| ))?; | ||
| return Ok(()); | ||
| } | ||
|
|
||
| // `:reparent --detach` → detach (None) | ||
| // `:reparent <anything else>` → parse as a ConversationId | ||
| let new_parent_id = if target.iter().any(|t| t == "--detach") { | ||
| None | ||
| } else { | ||
| let raw = target.join(" ").trim().to_string(); | ||
| match ConversationId::parse(&raw) { | ||
| Ok(id) => Some(id), | ||
| Err(err) => { | ||
| self.writeln_title(TitleFormat::error(format!( | ||
| "Invalid parent ID {raw:?}: {err}" | ||
| )))?; | ||
| return Ok(()); | ||
| } | ||
| } | ||
| }; | ||
|
Comment on lines
+2356
to
+2369
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. There is no check to prevent a conversation from being reparented to itself. If let new_parent_id = if target.iter().any(|t| t == "--detach") {
None
} else {
let raw = target.join(" ").trim().to_string();
match ConversationId::parse(&raw) {
Ok(id) => {
if id == conversation_id {
self.writeln_title(TitleFormat::error(
"A session cannot be parented to itself.",
))?;
return Ok(());
}
Some(id)
}
Err(err) => {
self.writeln_title(TitleFormat::error(format!(
"Invalid parent ID {raw:?}: {err}"
)))?;
return Ok(());
}
}
}; |
||
|
|
||
| self.api | ||
| .update_parent_id(&conversation_id, new_parent_id.as_ref()) | ||
| .await?; | ||
|
|
||
| let msg = match new_parent_id { | ||
| Some(pid) => format!( | ||
| "Re-parented current session to {}.", | ||
| pid.into_string().bold() | ||
| ), | ||
| None => "Detached current session — promoted to top-level.".to_string(), | ||
| }; | ||
| self.writeln_title(TitleFormat::info(msg))?; | ||
| Ok(()) | ||
| } | ||
|
|
||
| /// Filters the conversation list by working directory. Usage: | ||
| /// - `:cwd <path>` → exact-match cwd filter | ||
| /// - `:cwd --current` → use the current shell working directory | ||
| /// - `:cwd --clear` → clear the cwd filter | ||
| async fn handle_cwd(&mut self, target: Vec<String>) -> anyhow::Result<()> { | ||
|
Comment on lines
+2386
to
+2390
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. While |
||
| if target.is_empty() || target.iter().any(|t| t == "--help" || t == "-h") { | ||
| self.writeln_title(TitleFormat::info( | ||
| "Usage: :cwd <path> | :cwd --current | :cwd --clear", | ||
| ))?; | ||
| return Ok(()); | ||
| } | ||
|
|
||
| if target.iter().any(|t| t == "--clear") { | ||
| self.state.cwd_filter = None; | ||
| self.writeln_title(TitleFormat::info("Cleared cwd filter."))?; | ||
| return Ok(()); | ||
| } | ||
|
|
||
| let cwd = if target.iter().any(|t| t == "--current") { | ||
| match std::env::current_dir() { | ||
| Ok(p) => p.to_string_lossy().to_string(), | ||
| Err(err) => { | ||
| self.writeln_title(TitleFormat::error(format!( | ||
| "Failed to read current dir: {err}" | ||
| )))?; | ||
| return Ok(()); | ||
| } | ||
| } | ||
| } else { | ||
| target.join(" ").trim().to_string() | ||
| }; | ||
|
Comment on lines
+2414
to
+2416
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. When a relative path is passed to } else {
let raw_path = target.join(" ").trim().to_string();
let path = std::path::Path::new(&raw_path);
if path.is_relative() {
self.state.cwd.join(path).to_string_lossy().to_string()
} else {
raw_path
}
}; |
||
|
|
||
| self.state.cwd_filter = Some(cwd.clone()); | ||
|
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.
This stores a cwd filter and reports success, but the selector paths still call Useful? React with 👍 / 👎. |
||
| self.writeln_title(TitleFormat::info(format!( | ||
| "Cwd filter set to {}", | ||
| cwd.bold() | ||
| )))?; | ||
| Ok(()) | ||
| } | ||
|
|
||
| fn user_initiated_conversations(conversations: Vec<Conversation>) -> Vec<Conversation> { | ||
| let related_ids: HashSet<ConversationId> = conversations | ||
| .iter() | ||
|
|
@@ -2382,6 +2477,12 @@ impl<A: API + ConsoleWriter + 'static, F: Fn(ForgeConfig) -> A + Send + Sync> UI | |
| AppCommand::Parent => { | ||
| self.handle_parent().await?; | ||
| } | ||
| AppCommand::Reparent { target } => { | ||
| self.handle_reparent(target).await?; | ||
| } | ||
| AppCommand::Cwd { target } => { | ||
| self.handle_cwd(target).await?; | ||
| } | ||
| AppCommand::Search { query } => { | ||
| self.handle_search(query).await?; | ||
| } | ||
|
|
||
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.
Suggestion: Expand the method documentation to include explicit
# Argumentsand# Errorssections for this newly added public API method. [custom_rule]Severity Level: Minor⚠️
Why it matters? 🤔
This is a newly added public trait method with parameters and a fallible return type, so the custom rule requires Rustdoc
# Argumentsand# Errorssections. The existing code only has a brief description and does not include those required sections, so the violation is real.(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖