From 131d2047597ccede747f7c44d71e34e5489f9a0e Mon Sep 17 00:00:00 2001 From: CheckPickerUpper <194722943+CheckPickerUpper@users.noreply.github.com> Date: Fri, 28 Aug 2026 14:01:32 -0400 Subject: [PATCH] fix: keep repository aliases valid in workspaces --- src/reportal_commands/repo_remove.rs | 6 +- src/reportal_commands/workspace_members.rs | 64 ++++++--- src/reportal_config/reportal_config_root.rs | 139 ++++++++++++++++++-- src/reportal_config/workspace_entry.rs | 16 --- 4 files changed, 182 insertions(+), 43 deletions(-) diff --git a/src/reportal_commands/repo_remove.rs b/src/reportal_commands/repo_remove.rs index ee39871..62cca11 100644 --- a/src/reportal_commands/repo_remove.rs +++ b/src/reportal_commands/repo_remove.rs @@ -24,8 +24,10 @@ use crate::terminal_style; /// and save paths surface. pub fn run_remove(repository_alias: &str) -> Result<(), ReportalError> { let mut loaded_config = ReportalConfig::load_or_initialize()?; + let canonical_repository_name = loaded_config.resolve_repo_canonical_name(repository_alias)?; - let containing_workspaces = loaded_config.workspaces_containing_repo(repository_alias); + let containing_workspaces = + loaded_config.workspaces_containing_repo(&canonical_repository_name); if !containing_workspaces.is_empty() { let joined_workspace_names = containing_workspaces .iter() @@ -38,7 +40,7 @@ pub fn run_remove(repository_alias: &str) -> Result<(), ReportalError> { }); } - let removed_entry = loaded_config.remove_repo(repository_alias)?; + let removed_entry = loaded_config.remove_repo(&canonical_repository_name)?; loaded_config.save_to_disk()?; terminal_style::print_success(&format!( "Removed '{}' ({})", diff --git a/src/reportal_commands/workspace_members.rs b/src/reportal_commands/workspace_members.rs index 03813ff..c499920 100644 --- a/src/reportal_commands/workspace_members.rs +++ b/src/reportal_commands/workspace_members.rs @@ -27,12 +27,16 @@ pub fn run_workspace_add_repo( member_edit: &WorkspaceArgumentsMemberEditParts, ) -> Result<(), ReportalError> { let mut loaded_config = ReportalConfig::load_or_initialize()?; - loaded_config.get_repo(member_edit.repository_alias())?; + let canonical_repository_name = + loaded_config.resolve_repo_canonical_name(member_edit.repository_alias())?; let canonical_workspace_name = loaded_config.resolve_workspace_canonical_name(member_edit.workspace_name())?; - let target_workspace = loaded_config.get_workspace_mut(&canonical_workspace_name)?; - if target_workspace.contains_repo(member_edit.repository_alias()) { + let already_member = loaded_config + .workspaces_containing_repo(&canonical_repository_name) + .iter() + .any(|(workspace_name, _workspace)| workspace_name.as_str() == canonical_workspace_name); + if already_member { return Err(ReportalError::ValidationFailure { field: "workspace member".to_owned(), reason: format!( @@ -41,10 +45,10 @@ pub fn run_workspace_add_repo( ), }); } + + let target_workspace = loaded_config.get_workspace_mut(&canonical_workspace_name)?; let mut updated_members = target_workspace.members().to_vec(); - updated_members.push(WorkspaceMember::RegisteredRepo( - member_edit.repository_alias().to_owned(), - )); + updated_members.push(WorkspaceMember::RegisteredRepo(canonical_repository_name)); target_workspace.set_members(updated_members); loaded_config.save_to_disk()?; @@ -83,10 +87,20 @@ pub fn run_workspace_remove_repo( member_edit: &WorkspaceArgumentsMemberEditParts, ) -> Result<(), ReportalError> { let mut loaded_config = ReportalConfig::load_or_initialize()?; + let canonical_repository_name = + loaded_config.resolve_repo_canonical_name(member_edit.repository_alias())?; let canonical_workspace_name = loaded_config.resolve_workspace_canonical_name(member_edit.workspace_name())?; - let target_workspace = loaded_config.get_workspace_mut(&canonical_workspace_name)?; - if !target_workspace.contains_repo(member_edit.repository_alias()) { + + let current_members = loaded_config + .get_workspace(&canonical_workspace_name)? + .members() + .to_vec(); + let workspace_contains_repo = loaded_config + .workspaces_containing_repo(&canonical_repository_name) + .iter() + .any(|(workspace_name, _workspace)| workspace_name.as_str() == canonical_workspace_name); + if !workspace_contains_repo { return Err(ReportalError::ValidationFailure { field: "workspace member".to_owned(), reason: format!( @@ -95,16 +109,11 @@ pub fn run_workspace_remove_repo( ), }); } - let remaining_members: Vec = target_workspace - .members() - .iter() - .filter(|member| match member.registered_repo_alias() { - WorkspaceMemberAliasLookup::Matches(existing_alias) => { - existing_alias != member_edit.repository_alias() - } - WorkspaceMemberAliasLookup::NotARepoReference => true, + let remaining_members: Vec = current_members + .into_iter() + .filter(|member| { + !workspace_member_targets_repo(&loaded_config, member, &canonical_repository_name) }) - .cloned() .collect(); if remaining_members.is_empty() { return Err(ReportalError::ValidationFailure { @@ -115,6 +124,7 @@ pub fn run_workspace_remove_repo( ), }); } + let target_workspace = loaded_config.get_workspace_mut(&canonical_workspace_name)?; target_workspace.set_members(remaining_members); loaded_config.save_to_disk()?; @@ -130,3 +140,23 @@ pub fn run_workspace_remove_repo( )); Ok(()) } + +/// Returns whether a workspace member refers to a given canonical +/// repository key, resolving both canonical names and declared +/// repository aliases. Invalid references are retained by removal +/// operations rather than being silently discarded. +fn workspace_member_targets_repo( + loaded_config: &ReportalConfig, + member: &WorkspaceMember, + canonical_repository_name: &str, +) -> bool { + match member.registered_repo_alias() { + WorkspaceMemberAliasLookup::Matches(member_alias) => { + match loaded_config.resolve_repo_canonical_name(member_alias) { + Ok(member_canonical_name) => member_canonical_name == canonical_repository_name, + Err(_not_found) => false, + } + } + WorkspaceMemberAliasLookup::NotARepoReference => false, + } +} diff --git a/src/reportal_config/reportal_config_root.rs b/src/reportal_config/reportal_config_root.rs index b506121..d1efa13 100644 --- a/src/reportal_config/reportal_config_root.rs +++ b/src/reportal_config/reportal_config_root.rs @@ -240,7 +240,7 @@ impl ReportalConfig { pub fn validate_workspace_references(&self) -> Result<(), ReportalError> { for (workspace_name, workspace) in &self.workspaces { for member_alias in workspace.repo_aliases() { - if !self.repos.contains_key(member_alias) { + if resolve_canonical_key(&self.repos, member_alias).is_none() { return Err(ReportalError::WorkspaceHasDanglingRepo { workspace_name: workspace_name.to_owned(), missing_alias: member_alias.to_owned(), @@ -375,6 +375,28 @@ impl ReportalConfig { }) } + /// Resolves a user-supplied repository name or alias to its + /// canonical config key. + /// + /// Commands that persist workspace membership or inspect the + /// repo reverse index use this boundary so every spelling of a + /// registered repo refers to the same repository entry. + /// + /// # Errors + /// + /// Returns [`ReportalError::RepoNotFound`] if neither the + /// canonical key nor any declared repository alias matches. + pub fn resolve_repo_canonical_name( + &self, + alias_or_canonical: &str, + ) -> Result { + resolve_canonical_key(&self.repos, alias_or_canonical) + .map(str::to_owned) + .ok_or_else(|| ReportalError::RepoNotFound { + alias: alias_or_canonical.to_owned(), + }) + } + /// Registers a new repo from a validated builder result. /// /// Returns `RepoAlreadyExists` if the alias is already taken. @@ -392,13 +414,16 @@ impl ReportalConfig { Ok(()) } - /// Removes a repo by alias. Returns `RepoNotFound` if missing. + /// Removes a repo by canonical key or declared alias. Returns + /// `RepoNotFound` if missing. pub fn remove_repo(&mut self, alias: &str) -> Result { - self.repos - .remove(alias) - .ok_or_else(|| ReportalError::RepoNotFound { + let canonical_key = self.resolve_repo_canonical_name(alias)?; + match self.repos.remove(&canonical_key) { + Some(removed_entry) => Ok(removed_entry), + None => Err(ReportalError::RepoNotFound { alias: alias.to_owned(), - }) + }), + } } /// Returns all registered workspaces with their names, for iteration. @@ -497,7 +522,7 @@ impl ReportalConfig { return Err(ReportalError::WorkspaceAlreadyExists { workspace_name }); } for member_alias in workspace_entry.repo_aliases() { - if !self.repos.contains_key(member_alias) { + if resolve_canonical_key(&self.repos, member_alias).is_none() { return Err(ReportalError::WorkspaceHasDanglingRepo { workspace_name, missing_alias: member_alias.to_owned(), @@ -538,12 +563,37 @@ impl ReportalConfig { &self, repository_alias: &str, ) -> Vec<(&String, &WorkspaceEntry)> { + let Some(canonical_repository_name) = resolve_canonical_key(&self.repos, repository_alias) + else { + return Vec::new(); + }; self.workspaces .iter() - .filter(|(_, workspace)| workspace.contains_repo(repository_alias)) + .filter(|(_, workspace)| { + self.workspace_contains_repo_canonical(workspace, canonical_repository_name) + }) .collect() } + /// Checks whether any registered-repo member of a workspace + /// resolves to the requested canonical repository key. + fn workspace_contains_repo_canonical( + &self, + workspace: &WorkspaceEntry, + canonical_repository_name: &str, + ) -> bool { + for member_alias in workspace.repo_aliases() { + let Some(member_canonical_name) = resolve_canonical_key(&self.repos, member_alias) + else { + continue; + }; + if member_canonical_name == canonical_repository_name { + return true; + } + } + false + } + /// Returns the configured default AI tool name, if set. pub fn default_ai_tool(&self) -> &str { &self.settings.default_ai_tool @@ -704,6 +754,79 @@ mod tests { Ok(()) } + #[test] + fn workspace_reference_resolves_registered_repository_alias() -> Result<(), ReportalError> { + let mut config = ReportalConfig::create_default(); + config + .repos + .insert("venoble-app".to_owned(), make_repo(vec!["vna".to_owned()])); + config.workspaces.insert( + "backend".to_owned(), + make_workspace(vec!["vna".to_owned()], vec![])?, + ); + + let outcome = config.validate_workspace_references(); + assert!( + outcome.is_ok(), + "a workspace member may use a registered repository alias, got {outcome:?}", + ); + Ok(()) + } + + #[test] + fn add_workspace_accepts_registered_repository_alias() -> Result<(), ReportalError> { + let mut config = ReportalConfig::create_default(); + config + .repos + .insert("venoble-app".to_owned(), make_repo(vec!["vna".to_owned()])); + let workspace_registration = ( + "backend".to_owned(), + make_workspace(vec!["vna".to_owned()], vec![])?, + ); + + let outcome = config.add_workspace(workspace_registration); + assert!( + outcome.is_ok(), + "workspace creation by repository alias must succeed, got {outcome:?}", + ); + Ok(()) + } + + #[test] + fn reverse_index_finds_workspace_member_written_with_repository_alias( + ) -> Result<(), ReportalError> { + let mut config = ReportalConfig::create_default(); + config + .repos + .insert("venoble-app".to_owned(), make_repo(vec!["vna".to_owned()])); + config.workspaces.insert( + "backend".to_owned(), + make_workspace(vec!["vna".to_owned()], vec![])?, + ); + + let containing_by_canonical = config.workspaces_containing_repo("venoble-app"); + let containing_by_alias = config.workspaces_containing_repo("vna"); + assert_eq!(containing_by_canonical.len(), 1); + assert_eq!(containing_by_alias.len(), 1); + Ok(()) + } + + #[test] + fn remove_repo_accepts_declared_repository_alias() -> Result<(), ReportalError> { + let mut config = ReportalConfig::create_default(); + config + .repos + .insert("venoble-app".to_owned(), make_repo(vec!["vna".to_owned()])); + + let removed_entry = config.remove_repo("vna")?; + assert_eq!(removed_entry.raw_path(), "C:/fake"); + assert!(matches!( + config.get_repo("venoble-app"), + Err(ReportalError::RepoNotFound { .. }) + )); + Ok(()) + } + #[test] fn alias_collision_workspace_alias_equals_peer_workspace_canonical_name( ) -> Result<(), ReportalError> { diff --git a/src/reportal_config/workspace_entry.rs b/src/reportal_config/workspace_entry.rs index a090c42..d37f379 100644 --- a/src/reportal_config/workspace_entry.rs +++ b/src/reportal_config/workspace_entry.rs @@ -155,22 +155,6 @@ impl WorkspaceEntry { self.path = new_directory_path; } - /// Whether the given repo alias is a registered-repo member of - /// this workspace. - /// - /// Used by the reverse-index lookup `workspaces_containing_repo` - /// and the `rep remove` guard. Inline-path members never match - /// because they carry no repo alias by construction. - #[must_use] - pub fn contains_repo(&self, repository_alias: &str) -> bool { - self.repos.iter().any(|member| { - matches!( - member.registered_repo_alias(), - WorkspaceMemberAliasLookup::Matches(alias) if alias == repository_alias - ) - }) - } - /// @why Exposes the workspace's prompt-badge / window-title /// label so `rep prompt` and `rep workspace rebuild` can render /// the same identity without re-reading the raw field.