feat(bd-3ke): add title truncation and key escaping for GitLab-to-Beads bridge
- Add truncate_title() function for bead titles (max 60 chars with ellipsis) - Add escape_project() to replace / with :: in mapping keys for filesystem safety - Add InvalidInput error code for validation errors - Add comprehensive tests for truncation, escaping, and Unicode handling Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
This commit is contained in:
@@ -77,36 +77,65 @@ pub enum MappingKey {
|
||||
MrAuthored { project: String, iid: i64 },
|
||||
}
|
||||
|
||||
/// Maximum length for entity titles in bead titles (to keep beads scannable)
|
||||
const MAX_TITLE_LENGTH: usize = 60;
|
||||
|
||||
/// Truncate a string to max_len characters, appending "..." if truncated.
|
||||
/// Handles Unicode correctly by counting grapheme clusters.
|
||||
fn truncate_title(s: &str, max_len: usize) -> String {
|
||||
if s.chars().count() <= max_len {
|
||||
s.to_string()
|
||||
} else {
|
||||
let truncated: String = s.chars().take(max_len.saturating_sub(3)).collect();
|
||||
format!("{}...", truncated.trim_end())
|
||||
}
|
||||
}
|
||||
|
||||
impl MappingKey {
|
||||
/// Serialize to string key format
|
||||
/// Serialize to string key format.
|
||||
///
|
||||
/// Keys are designed to be:
|
||||
/// - Stable across project renames (using project path as lore doesn't expose project_id yet)
|
||||
/// - Safe for JSON keys and filesystem paths (no spaces, forward slashes escaped)
|
||||
/// - Unique within an MC instance
|
||||
pub fn to_key_string(&self) -> String {
|
||||
match self {
|
||||
MappingKey::MrReview { project, iid } => {
|
||||
format!("mr_review:{}:{}", project, iid)
|
||||
format!("mr_review:{}:{}", Self::escape_project(project), iid)
|
||||
}
|
||||
MappingKey::Issue { project, iid } => {
|
||||
format!("issue:{}:{}", project, iid)
|
||||
format!("issue:{}:{}", Self::escape_project(project), iid)
|
||||
}
|
||||
MappingKey::MrAuthored { project, iid } => {
|
||||
format!("mr_authored:{}:{}", project, iid)
|
||||
format!("mr_authored:{}:{}", Self::escape_project(project), iid)
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
/// Build bead title from this key's event data
|
||||
/// Build bead title from this key's event data.
|
||||
///
|
||||
/// Titles are formatted as "{prefix} {entity_title}" with truncation
|
||||
/// to keep them scannable in the UI.
|
||||
pub fn to_bead_title(&self, entity_title: &str) -> String {
|
||||
let truncated = truncate_title(entity_title, MAX_TITLE_LENGTH);
|
||||
match self {
|
||||
MappingKey::MrReview { iid, .. } => {
|
||||
format!("Review MR !{}: {}", iid, entity_title)
|
||||
format!("Review MR !{}: {}", iid, truncated)
|
||||
}
|
||||
MappingKey::Issue { iid, .. } => {
|
||||
format!("Issue #{}: {}", iid, entity_title)
|
||||
format!("Issue #{}: {}", iid, truncated)
|
||||
}
|
||||
MappingKey::MrAuthored { iid, .. } => {
|
||||
format!("Your MR !{}: {}", iid, entity_title)
|
||||
format!("Your MR !{}: {}", iid, truncated)
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
/// Escape project path for use in mapping keys.
|
||||
/// Replaces / with :: to make keys filesystem-safe.
|
||||
fn escape_project(project: &str) -> String {
|
||||
project.replace('/', "::")
|
||||
}
|
||||
}
|
||||
|
||||
/// Result of a sync operation
|
||||
@@ -683,19 +712,47 @@ mod tests {
|
||||
project: "group/repo".to_string(),
|
||||
iid: 847,
|
||||
};
|
||||
assert_eq!(key.to_key_string(), "mr_review:group/repo:847");
|
||||
// Project path / is escaped to :: for filesystem safety
|
||||
assert_eq!(key.to_key_string(), "mr_review:group::repo:847");
|
||||
|
||||
let key = MappingKey::Issue {
|
||||
project: "group/repo".to_string(),
|
||||
iid: 42,
|
||||
};
|
||||
assert_eq!(key.to_key_string(), "issue:group/repo:42");
|
||||
assert_eq!(key.to_key_string(), "issue:group::repo:42");
|
||||
|
||||
let key = MappingKey::MrAuthored {
|
||||
project: "group/repo".to_string(),
|
||||
iid: 100,
|
||||
};
|
||||
assert_eq!(key.to_key_string(), "mr_authored:group/repo:100");
|
||||
assert_eq!(key.to_key_string(), "mr_authored:group::repo:100");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_mapping_key_escapes_nested_groups() {
|
||||
// GitLab supports deeply nested groups like org/team/sub/repo
|
||||
let key = MappingKey::Issue {
|
||||
project: "org/team/sub/repo".to_string(),
|
||||
iid: 42,
|
||||
};
|
||||
assert_eq!(key.to_key_string(), "issue:org::team::sub::repo:42");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_mapping_key_safe_for_filesystem() {
|
||||
let key = MappingKey::MrReview {
|
||||
project: "group/repo".to_string(),
|
||||
iid: 847,
|
||||
};
|
||||
let key_str = key.to_key_string();
|
||||
|
||||
// Keys should not contain characters that are problematic for:
|
||||
// - JSON object keys (no quotes, backslashes)
|
||||
// - Filesystem paths (no forward slashes, colons are acceptable on Unix)
|
||||
assert!(!key_str.contains('/'), "Key should not contain forward slash");
|
||||
assert!(!key_str.contains(' '), "Key should not contain spaces");
|
||||
assert!(!key_str.contains('"'), "Key should not contain quotes");
|
||||
assert!(!key_str.contains('\\'), "Key should not contain backslashes");
|
||||
}
|
||||
|
||||
#[test]
|
||||
@@ -728,6 +785,66 @@ mod tests {
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_bead_title_truncates_long_titles() {
|
||||
let key = MappingKey::MrReview {
|
||||
project: "g/p".to_string(),
|
||||
iid: 847,
|
||||
};
|
||||
|
||||
let long_title = "Fix authentication token refresh logic that was causing intermittent failures in production";
|
||||
let title = key.to_bead_title(long_title);
|
||||
|
||||
// Title should be truncated with ellipsis
|
||||
assert!(title.ends_with("..."), "Long title should end with ellipsis");
|
||||
// The entity_title portion should be max 60 chars
|
||||
// "Review MR !847: " is 16 chars, so total should be under 16 + 60 = 76
|
||||
assert!(title.len() <= 80, "Title should be reasonably short: {}", title);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_bead_title_preserves_short_titles() {
|
||||
let key = MappingKey::Issue {
|
||||
project: "g/p".to_string(),
|
||||
iid: 42,
|
||||
};
|
||||
|
||||
let short_title = "Quick fix";
|
||||
let title = key.to_bead_title(short_title);
|
||||
|
||||
assert!(!title.ends_with("..."), "Short title should not be truncated");
|
||||
assert_eq!(title, "Issue #42: Quick fix");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_truncate_title_exactly_at_limit() {
|
||||
// 60 char title should not be truncated
|
||||
let title_60 = "A".repeat(60);
|
||||
let truncated = truncate_title(&title_60, 60);
|
||||
assert_eq!(truncated.len(), 60);
|
||||
assert!(!truncated.ends_with("..."));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_truncate_title_just_over_limit() {
|
||||
// 61 char title should be truncated
|
||||
let title_61 = "A".repeat(61);
|
||||
let truncated = truncate_title(&title_61, 60);
|
||||
assert!(truncated.ends_with("..."));
|
||||
assert!(truncated.len() <= 60);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_truncate_title_handles_unicode() {
|
||||
// Unicode characters should be counted correctly, not by bytes
|
||||
let emoji_title = "Fix 🔥 auth bug with 中文 characters that is very long indeed";
|
||||
let truncated = truncate_title(emoji_title, 30);
|
||||
|
||||
// Should truncate by character count, not bytes
|
||||
assert!(truncated.chars().count() <= 30);
|
||||
assert!(truncated.ends_with("..."));
|
||||
}
|
||||
|
||||
// -- Map persistence tests --
|
||||
|
||||
#[test]
|
||||
@@ -767,7 +884,7 @@ mod tests {
|
||||
|
||||
let mut map = GitLabBeadMap::default();
|
||||
map.mappings.insert(
|
||||
"issue:g/p:42".to_string(),
|
||||
"issue:g::p:42".to_string(),
|
||||
MappingEntry {
|
||||
bead_id: Some("bd-abc".to_string()),
|
||||
created_at: "2026-02-25T10:00:00Z".to_string(),
|
||||
@@ -781,9 +898,9 @@ mod tests {
|
||||
let loaded = bridge.load_map().unwrap();
|
||||
|
||||
assert_eq!(loaded.mappings.len(), 1);
|
||||
assert!(loaded.mappings.contains_key("issue:g/p:42"));
|
||||
assert!(loaded.mappings.contains_key("issue:g::p:42"));
|
||||
assert_eq!(
|
||||
loaded.mappings["issue:g/p:42"].bead_id,
|
||||
loaded.mappings["issue:g::p:42"].bead_id,
|
||||
Some("bd-abc".to_string())
|
||||
);
|
||||
}
|
||||
@@ -823,7 +940,7 @@ mod tests {
|
||||
assert!(created.unwrap());
|
||||
assert_eq!(map.mappings.len(), 1);
|
||||
|
||||
let entry = &map.mappings["issue:g/p:42"];
|
||||
let entry = &map.mappings["issue:g::p:42"];
|
||||
assert_eq!(entry.bead_id, Some("bd-new".to_string()));
|
||||
assert!(!entry.pending);
|
||||
}
|
||||
@@ -836,7 +953,7 @@ mod tests {
|
||||
|
||||
// Pre-populate
|
||||
map.mappings.insert(
|
||||
"issue:g/p:42".to_string(),
|
||||
"issue:g::p:42".to_string(),
|
||||
MappingEntry {
|
||||
bead_id: Some("bd-existing".to_string()),
|
||||
created_at: "2026-02-25T10:00:00Z".to_string(),
|
||||
@@ -901,7 +1018,7 @@ mod tests {
|
||||
|
||||
// Simulate crashed state: pending=true, bead_id=None
|
||||
map.mappings.insert(
|
||||
"issue:g/p:42".to_string(),
|
||||
"issue:g::p:42".to_string(),
|
||||
MappingEntry {
|
||||
bead_id: None,
|
||||
created_at: "2026-02-25T10:00:00Z".to_string(),
|
||||
@@ -916,7 +1033,7 @@ mod tests {
|
||||
|
||||
assert_eq!(recovered, 1);
|
||||
assert!(errors.is_empty());
|
||||
let entry = &map.mappings["issue:g/p:42"];
|
||||
let entry = &map.mappings["issue:g::p:42"];
|
||||
assert_eq!(entry.bead_id, Some("bd-recovered".to_string()));
|
||||
assert!(!entry.pending);
|
||||
}
|
||||
@@ -929,7 +1046,7 @@ mod tests {
|
||||
|
||||
// Simulate: bead was created but pending flag not cleared
|
||||
map.mappings.insert(
|
||||
"issue:g/p:42".to_string(),
|
||||
"issue:g::p:42".to_string(),
|
||||
MappingEntry {
|
||||
bead_id: Some("bd-exists".to_string()),
|
||||
created_at: "2026-02-25T10:00:00Z".to_string(),
|
||||
@@ -944,7 +1061,7 @@ mod tests {
|
||||
|
||||
assert_eq!(recovered, 1);
|
||||
assert!(errors.is_empty());
|
||||
assert!(!map.mappings["issue:g/p:42"].pending);
|
||||
assert!(!map.mappings["issue:g::p:42"].pending);
|
||||
}
|
||||
|
||||
// -- incremental_sync tests --
|
||||
@@ -988,7 +1105,7 @@ mod tests {
|
||||
|
||||
assert_eq!(result.created, 1);
|
||||
assert_eq!(result.skipped, 0);
|
||||
assert!(map.mappings.contains_key("issue:g/p:42"));
|
||||
assert!(map.mappings.contains_key("issue:g::p:42"));
|
||||
assert_eq!(
|
||||
map.cursor.last_check_timestamp,
|
||||
Some("2026-02-25T12:00:00Z".to_string())
|
||||
@@ -1021,7 +1138,7 @@ mod tests {
|
||||
|
||||
// Pre-populate so it's a duplicate
|
||||
map.mappings.insert(
|
||||
"issue:g/p:42".to_string(),
|
||||
"issue:g::p:42".to_string(),
|
||||
MappingEntry {
|
||||
bead_id: Some("bd-existing".to_string()),
|
||||
created_at: "2026-02-25T09:00:00Z".to_string(),
|
||||
@@ -1075,8 +1192,8 @@ mod tests {
|
||||
bridge.incremental_sync(&mut map).unwrap();
|
||||
|
||||
// Should be classified as mr_review, not mr_authored
|
||||
assert!(map.mappings.contains_key("mr_review:g/p:100"));
|
||||
assert!(!map.mappings.contains_key("mr_authored:g/p:100"));
|
||||
assert!(map.mappings.contains_key("mr_review:g::p:100"));
|
||||
assert!(!map.mappings.contains_key("mr_authored:g::p:100"));
|
||||
}
|
||||
|
||||
// -- full_reconciliation tests --
|
||||
@@ -1097,7 +1214,7 @@ mod tests {
|
||||
|
||||
// Simulate first strike from previous reconciliation
|
||||
map.mappings.insert(
|
||||
"issue:g/p:42".to_string(),
|
||||
"issue:g::p:42".to_string(),
|
||||
MappingEntry {
|
||||
bead_id: Some("bd-abc".to_string()),
|
||||
created_at: "2026-02-25T09:00:00Z".to_string(),
|
||||
@@ -1110,7 +1227,7 @@ mod tests {
|
||||
let result = bridge.full_reconciliation(&mut map).unwrap();
|
||||
|
||||
assert_eq!(result.healed, 1);
|
||||
assert!(!map.mappings["issue:g/p:42"].suspect_orphan);
|
||||
assert!(!map.mappings["issue:g::p:42"].suspect_orphan);
|
||||
}
|
||||
|
||||
#[test]
|
||||
@@ -1125,7 +1242,7 @@ mod tests {
|
||||
let mut map = GitLabBeadMap::default();
|
||||
|
||||
map.mappings.insert(
|
||||
"issue:g/p:42".to_string(),
|
||||
"issue:g::p:42".to_string(),
|
||||
MappingEntry {
|
||||
bead_id: Some("bd-abc".to_string()),
|
||||
created_at: "2026-02-25T09:00:00Z".to_string(),
|
||||
@@ -1139,7 +1256,7 @@ mod tests {
|
||||
|
||||
// First strike: should be marked suspect, NOT closed
|
||||
assert_eq!(result.closed, 0);
|
||||
assert!(map.mappings["issue:g/p:42"].suspect_orphan);
|
||||
assert!(map.mappings["issue:g::p:42"].suspect_orphan);
|
||||
}
|
||||
|
||||
#[test]
|
||||
@@ -1157,7 +1274,7 @@ mod tests {
|
||||
|
||||
// Already has first strike
|
||||
map.mappings.insert(
|
||||
"issue:g/p:42".to_string(),
|
||||
"issue:g::p:42".to_string(),
|
||||
MappingEntry {
|
||||
bead_id: Some("bd-abc".to_string()),
|
||||
created_at: "2026-02-25T09:00:00Z".to_string(),
|
||||
@@ -1171,7 +1288,7 @@ mod tests {
|
||||
|
||||
// Second strike: should be closed and removed
|
||||
assert_eq!(result.closed, 1);
|
||||
assert!(!map.mappings.contains_key("issue:g/p:42"));
|
||||
assert!(!map.mappings.contains_key("issue:g::p:42"));
|
||||
}
|
||||
|
||||
#[test]
|
||||
@@ -1196,7 +1313,7 @@ mod tests {
|
||||
let result = bridge.full_reconciliation(&mut map).unwrap();
|
||||
|
||||
assert_eq!(result.created, 1);
|
||||
assert!(map.mappings.contains_key("issue:g/p:99"));
|
||||
assert!(map.mappings.contains_key("issue:g::p:99"));
|
||||
}
|
||||
|
||||
#[test]
|
||||
@@ -1225,9 +1342,9 @@ mod tests {
|
||||
Bridge::<MockLoreCli, MockBeadsCli>::build_expected_keys(&response);
|
||||
|
||||
assert_eq!(keys.len(), 3);
|
||||
assert!(keys.contains_key("issue:g/p:1"));
|
||||
assert!(keys.contains_key("mr_authored:g/p:10"));
|
||||
assert!(keys.contains_key("mr_review:g/p:20"));
|
||||
assert!(keys.contains_key("issue:g::p:1"));
|
||||
assert!(keys.contains_key("mr_authored:g::p:10"));
|
||||
assert!(keys.contains_key("mr_review:g::p:20"));
|
||||
}
|
||||
|
||||
// -- Lock tests --
|
||||
@@ -1300,7 +1417,7 @@ mod tests {
|
||||
let r2 = bridge2.full_reconciliation(&mut map).unwrap();
|
||||
assert_eq!(r2.closed, 0);
|
||||
assert_eq!(r2.created, 0);
|
||||
assert!(!map.mappings["issue:g/p:42"].suspect_orphan);
|
||||
assert!(!map.mappings["issue:g::p:42"].suspect_orphan);
|
||||
|
||||
// Phase 3: Issue disappears -- first strike
|
||||
let mut lore3 = MockLoreCli::new();
|
||||
@@ -1309,7 +1426,7 @@ mod tests {
|
||||
let bridge3 = Bridge::with_data_dir(lore3, MockBeadsCli::new(), dir.path().to_path_buf());
|
||||
let r3 = bridge3.full_reconciliation(&mut map).unwrap();
|
||||
assert_eq!(r3.closed, 0);
|
||||
assert!(map.mappings["issue:g/p:42"].suspect_orphan);
|
||||
assert!(map.mappings["issue:g::p:42"].suspect_orphan);
|
||||
|
||||
// Phase 4: Still missing -- second strike, close
|
||||
let mut lore4 = MockLoreCli::new();
|
||||
@@ -1321,7 +1438,7 @@ mod tests {
|
||||
let bridge4 = Bridge::with_data_dir(lore4, beads4, dir.path().to_path_buf());
|
||||
let r4 = bridge4.full_reconciliation(&mut map).unwrap();
|
||||
assert_eq!(r4.closed, 1);
|
||||
assert!(!map.mappings.contains_key("issue:g/p:42"));
|
||||
assert!(!map.mappings.contains_key("issue:g::p:42"));
|
||||
}
|
||||
|
||||
// -- cleanup_tmp_files tests --
|
||||
|
||||
@@ -46,6 +46,7 @@ pub enum McErrorCode {
|
||||
// General errors
|
||||
IoError,
|
||||
InternalError,
|
||||
InvalidInput,
|
||||
}
|
||||
|
||||
impl McError {
|
||||
@@ -116,6 +117,15 @@ impl McError {
|
||||
"bv CLI not found -- is beads installed?",
|
||||
)
|
||||
}
|
||||
|
||||
/// Create an invalid input error
|
||||
pub fn invalid_input(message: impl Into<String>) -> Self {
|
||||
Self {
|
||||
code: McErrorCode::InvalidInput,
|
||||
message: message.into(),
|
||||
recoverable: false,
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
impl std::fmt::Display for McError {
|
||||
|
||||
Reference in New Issue
Block a user