Skip to content

Commit 58dda41

Browse files
author
Test User
committed
Fix element relocation change detection with semantic relation comparison
When elements are relocated to different files, their identifiers change but they remain the same logical element (same name/Element ID). This was causing false positives in change detection when parent elements had automatic reverse relations pointing to relocated children. Implementation: - Add normalize_relation_for_comparison() to compare relations by element name instead of identifier, preventing false change detection when children relocate - Add line_number field to Element struct for better error reporting - Implement global uniqueness validation for element names across all files - Update duplicate element error format to consistently show both locations Tests: - Add comprehensive test-change-impact-element-relocation covering: * Pure relocation (no content changes) * Relocated + content changed * Relocated + relation added * Relocated parent + new relation to relocated+changed child - Update test fixtures to comply with global uniqueness requirement - Add .reqvireignore files to prevent test artifacts from being detected Specifications: - Update Change Impact Detection Test verification with new acceptance criteria - Update requirements to reflect semantic relation comparison behavior
1 parent 53701aa commit 58dda41

File tree

29 files changed

+523
-142
lines changed

29 files changed

+523
-142
lines changed

cli/src/cli.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1090,6 +1090,7 @@ fn process_shell_command(graph_registry: &mut GraphRegistry, command: &str) -> R
10901090
element_id,
10911091
file_path,
10921092
section,
1093+
1, // REPL-added elements default to line 1
10931094
None,
10941095
);
10951096

core/src/change_impact.rs

Lines changed: 195 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -53,6 +53,14 @@ pub struct RemovedElement {
5353
pub removed_relations: Vec<RelationSummary>,
5454
}
5555

56+
/// Report for an element that has been relocated (name exists in both registries but identifier changed).
57+
#[derive(Debug, Serialize)]
58+
pub struct RelocatedElement {
59+
pub name: String,
60+
pub old_identifier: String,
61+
pub new_identifier: String,
62+
}
63+
5664
#[derive(Debug, Clone, Serialize)]
5765
pub struct InvalidatedVerification {
5866
pub element_id: String,
@@ -86,6 +94,7 @@ pub struct ChangeImpactReport {
8694
pub added: Vec<AddedElement>,
8795
pub removed: Vec<RemovedElement>,
8896
pub changed: Vec<ChangedElement>,
97+
pub relocated: Vec<RelocatedElement>,
8998
pub invalidated_verifications: Vec<InvalidatedVerification>,
9099
#[serde(skip)]
91100
pub all_added_element_ids: HashSet<String>,
@@ -97,6 +106,7 @@ impl ChangeImpactReport {
97106
added: Vec::new(),
98107
removed: Vec::new(),
99108
changed: Vec::new(),
109+
relocated: Vec::new(),
100110
invalidated_verifications: Vec::new(),
101111
all_added_element_ids: HashSet::new(),
102112
}
@@ -195,6 +205,13 @@ impl ChangeImpactReport {
195205
"change_impact_tree": impact_tree
196206
})
197207
}).collect();
208+
let relocated: Vec<_> = self.relocated.iter().map(|elem| {
209+
json!({
210+
"name": elem.name,
211+
"old_location": elem.old_identifier,
212+
"new_location": elem.new_identifier
213+
})
214+
}).collect();
198215
let invalidated_verifications: Vec<_> = self.invalidated_verifications.iter().map(|invalidated_ver| {
199216
let target_url = format!("{}/blob/{}/{}", base_url, git_commit, invalidated_ver.element_id);
200217
json!({
@@ -207,16 +224,38 @@ impl ChangeImpactReport {
207224
"added": added,
208225
"removed": removed,
209226
"changed": changed,
227+
"relocated": relocated,
210228
"invalidated_verifications": invalidated_verifications
211229
})
212230
}
213231
/// Outputs the report as text with GitHub links included.
214232
pub fn to_text(&self, base_url: &str, git_commit: &str, previous_git_commit: &str) -> String {
215233
let mut output = String::new();
216234
output.push_str("## Change Impact Report\n\n");
217-
235+
218236
// Use all_added_element_ids which contains IDs from before filtering
219237
let new_element_ids = &self.all_added_element_ids;
238+
239+
// Relocated Elements section
240+
if !self.relocated.is_empty() {
241+
output.push_str("### Relocated Elements\n\n");
242+
for elem in &self.relocated {
243+
output.push_str(&format!(
244+
"* [{}]({})\n",
245+
elem.name, elem.new_identifier
246+
));
247+
output.push_str(&format!(
248+
" * Old location: {}\n",
249+
elem.old_identifier
250+
));
251+
output.push_str(&format!(
252+
" * New location: {}\n",
253+
elem.new_identifier
254+
));
255+
}
256+
output.push_str("\n---\n\n");
257+
}
258+
220259
// Removed Elements section
221260
if !self.removed.is_empty() {
222261
output.push_str("### Removed Elements\n\n");
@@ -288,8 +327,8 @@ impl ChangeImpactReport {
288327
}
289328
output.push_str("\n");
290329
}
291-
292-
if self.removed.is_empty() && self.added.is_empty() && self.changed.is_empty() {
330+
331+
if self.removed.is_empty() && self.added.is_empty() && self.changed.is_empty() && self.relocated.is_empty() {
293332
output.push_str("\nNothing to report...\n");
294333
}
295334
output
@@ -427,6 +466,7 @@ pub fn build_change_impact_tree(
427466
&element_id,
428467
"unknown",
429468
"Placeholder",
469+
0, // Placeholder elements don't have real line numbers
430470
None,
431471
);
432472
placeholder.content = "Element referenced but not found in registry".to_string();
@@ -574,37 +614,79 @@ fn collect_tree_ids_recursively(
574614

575615

576616

617+
/// Helper to normalize a relation by resolving its target identifier to an element name
618+
/// Returns (relation_type_name, element_name) tuple for semantic comparison
619+
/// Falls back to identifier if element cannot be resolved
620+
fn normalize_relation_for_comparison(rel: &Relation, registry: &graph_registry::GraphRegistry) -> (String, String) {
621+
let relation_type = rel.relation_type.name.to_string();
622+
623+
// Try to resolve the identifier to an element name
624+
if let relation::LinkType::Identifier(identifier) = &rel.target.link {
625+
if let Some(target_elem) = registry.get_element(identifier) {
626+
return (relation_type, target_elem.name.clone());
627+
}
628+
}
629+
630+
// Fall back to identifier for external links or unresolved identifiers
631+
(relation_type, rel.target.link.as_str().to_string())
632+
}
633+
577634
pub fn compute_change_impact(
578635
current: &graph_registry::GraphRegistry,
579636
reference: &graph_registry::GraphRegistry,
580637
) -> Result<ChangeImpactReport, ReqvireError> {
581638
let mut report = ChangeImpactReport::new();
582639
let current_ids: HashSet<String> = current.get_all_elements().iter().map(|e| e.identifier.clone()).collect();
583640
let reference_ids: HashSet<String> = reference.get_all_elements().iter().map(|e| e.identifier.clone()).collect();
584-
641+
585642
// Process elements present in both registries.
586643
for id in current_ids.intersection(&reference_ids) {
587644
let cur_elem = current.get_element(id).unwrap();
588645
let ref_elem = reference.get_element(id).unwrap();
589646
let content_changed = cur_elem.hash_impact_content != ref_elem.hash_impact_content;
590-
647+
591648
// Only track changes to relations that propagate impact according to specifications
592-
let cur_relations: HashSet<_> = cur_elem.relations.iter()
649+
let cur_relations_raw: Vec<_> = cur_elem.relations.iter()
593650
.filter(|r| relation::IMPACT_PROPAGATION_RELATIONS.contains(&r.relation_type.name))
594-
.cloned().collect();
595-
let ref_relations: HashSet<_> = ref_elem.relations.iter()
651+
.collect();
652+
let ref_relations_raw: Vec<_> = ref_elem.relations.iter()
596653
.filter(|r| relation::IMPACT_PROPAGATION_RELATIONS.contains(&r.relation_type.name))
597-
.cloned().collect();
598-
let added_relations: Vec<_> = cur_relations
599-
.difference(&ref_relations)
654+
.collect();
655+
656+
// Normalize relations for semantic comparison (by element name, not identifier)
657+
let cur_relations_normalized: HashSet<_> = cur_relations_raw.iter()
658+
.map(|r| normalize_relation_for_comparison(r, current))
659+
.collect();
660+
let ref_relations_normalized: HashSet<_> = ref_relations_raw.iter()
661+
.map(|r| normalize_relation_for_comparison(r, reference))
662+
.collect();
663+
664+
// Find truly added/removed relations based on semantic comparison
665+
let added_relation_keys: HashSet<_> = cur_relations_normalized
666+
.difference(&ref_relations_normalized)
600667
.cloned()
601-
.map(|rel: Relation| convert_relation_to_summary(&rel))
602668
.collect();
603-
let removed_relations: Vec<_> = ref_relations
604-
.difference(&cur_relations)
669+
let removed_relation_keys: HashSet<_> = ref_relations_normalized
670+
.difference(&cur_relations_normalized)
605671
.cloned()
606-
.map(|rel: Relation| convert_relation_to_summary(&rel))
607672
.collect();
673+
674+
// Map back to actual Relation objects for reporting
675+
let added_relations: Vec<_> = cur_relations_raw.iter()
676+
.filter(|r| {
677+
let normalized = normalize_relation_for_comparison(r, current);
678+
added_relation_keys.contains(&normalized)
679+
})
680+
.map(|r| convert_relation_to_summary(r))
681+
.collect();
682+
let removed_relations: Vec<_> = ref_relations_raw.iter()
683+
.filter(|r| {
684+
let normalized = normalize_relation_for_comparison(r, reference);
685+
removed_relation_keys.contains(&normalized)
686+
})
687+
.map(|r| convert_relation_to_summary(r))
688+
.collect();
689+
608690
let has_changed = content_changed || !added_relations.is_empty() || !removed_relations.is_empty();
609691
if has_changed {
610692
// Debug: print element relations
@@ -629,9 +711,98 @@ pub fn compute_change_impact(
629711
});
630712
}
631713
}
632-
// Process added elements (present only in current registry).
714+
// Detect relocated elements (same name, different identifier)
715+
let mut relocated_element_names = HashSet::new();
716+
let current_by_name: std::collections::HashMap<String, &element::Element> =
717+
current.get_all_elements().iter().map(|e| (e.name.clone(), *e)).collect();
718+
let reference_by_name: std::collections::HashMap<String, &element::Element> =
719+
reference.get_all_elements().iter().map(|e| (e.name.clone(), *e)).collect();
720+
721+
for (name, ref_elem) in &reference_by_name {
722+
if let Some(cur_elem) = current_by_name.get(name) {
723+
// Same name exists in both registries
724+
if ref_elem.identifier != cur_elem.identifier {
725+
// Different identifiers = relocation
726+
report.relocated.push(RelocatedElement {
727+
name: name.clone(),
728+
old_identifier: ref_elem.identifier.clone(),
729+
new_identifier: cur_elem.identifier.clone(),
730+
});
731+
relocated_element_names.insert(name.clone());
732+
733+
// Check if relocated element also has content or relation changes
734+
let content_changed = cur_elem.hash_impact_content != ref_elem.hash_impact_content;
735+
736+
// Use semantic relation comparison (by element name, not identifier)
737+
let cur_relations_raw: Vec<_> = cur_elem.relations.iter()
738+
.filter(|r| relation::IMPACT_PROPAGATION_RELATIONS.contains(&r.relation_type.name))
739+
.collect();
740+
let ref_relations_raw: Vec<_> = ref_elem.relations.iter()
741+
.filter(|r| relation::IMPACT_PROPAGATION_RELATIONS.contains(&r.relation_type.name))
742+
.collect();
743+
744+
let cur_relations_normalized: HashSet<_> = cur_relations_raw.iter()
745+
.map(|r| normalize_relation_for_comparison(r, current))
746+
.collect();
747+
let ref_relations_normalized: HashSet<_> = ref_relations_raw.iter()
748+
.map(|r| normalize_relation_for_comparison(r, reference))
749+
.collect();
750+
751+
let added_relation_keys: HashSet<_> = cur_relations_normalized
752+
.difference(&ref_relations_normalized)
753+
.cloned()
754+
.collect();
755+
let removed_relation_keys: HashSet<_> = ref_relations_normalized
756+
.difference(&cur_relations_normalized)
757+
.cloned()
758+
.collect();
759+
760+
let added_relations: Vec<_> = cur_relations_raw.iter()
761+
.filter(|r| {
762+
let normalized = normalize_relation_for_comparison(r, current);
763+
added_relation_keys.contains(&normalized)
764+
})
765+
.map(|r| convert_relation_to_summary(r))
766+
.collect();
767+
let removed_relations: Vec<_> = ref_relations_raw.iter()
768+
.filter(|r| {
769+
let normalized = normalize_relation_for_comparison(r, reference);
770+
removed_relation_keys.contains(&normalized)
771+
})
772+
.map(|r| convert_relation_to_summary(r))
773+
.collect();
774+
775+
let has_changed = content_changed || !added_relations.is_empty() || !removed_relations.is_empty();
776+
if has_changed {
777+
let mut visited = BTreeSet::new();
778+
visited.insert(cur_elem.identifier.clone());
779+
let change_impact_tree = build_change_impact_tree(current, cur_elem.identifier.to_string(), &mut visited, None);
780+
781+
report.changed.push(ChangedElement {
782+
element_id: cur_elem.identifier.clone(),
783+
name: cur_elem.name.clone(),
784+
old_content: ref_elem.content.clone(),
785+
new_content: cur_elem.content.clone(),
786+
content_changed,
787+
added_relations,
788+
removed_relations,
789+
change_impact_tree,
790+
});
791+
}
792+
}
793+
}
794+
}
795+
796+
// Sort relocated elements by name for deterministic output
797+
report.relocated.sort_by(|a, b| a.name.cmp(&b.name));
798+
799+
// Process added elements (present only in current registry, excluding relocated).
633800
for id in current_ids.difference(&reference_ids) {
634801
let cur_elem = current.get_element(id).unwrap();
802+
// Skip if this element was relocated (not truly added)
803+
if relocated_element_names.contains(&cur_elem.name) {
804+
continue;
805+
}
635806
let added_relations: Vec<_> = cur_elem
636807
.relations
637808
.iter()
@@ -650,9 +821,13 @@ pub fn compute_change_impact(
650821
change_impact_tree,
651822
});
652823
}
653-
// Process removed elements (present only in reference registry).
824+
// Process removed elements (present only in reference registry, excluding relocated).
654825
for id in reference_ids.difference(&current_ids) {
655826
let ref_elem = reference.get_element(id).unwrap();
827+
// Skip if this element was relocated (not truly removed)
828+
if relocated_element_names.contains(&ref_elem.name) {
829+
continue;
830+
}
656831
let removed_relations: Vec<_> = ref_elem
657832
.relations
658833
.iter()
@@ -722,6 +897,7 @@ mod tests {
722897
identifier,
723898
"test.md",
724899
"TestSection",
900+
1,
725901
Some(crate::element::ElementType::Requirement(crate::element::RequirementType::System))
726902
);
727903
element.add_content(content);
@@ -897,6 +1073,7 @@ mod tests {
8971073
"verify.md#parent-verification",
8981074
"verify.md",
8991075
"Verifications",
1076+
1,
9001077
Some(crate::element::ElementType::Verification(crate::element::VerificationType::Test))
9011078
);
9021079
verification.add_content("Verification content");
@@ -964,6 +1141,7 @@ mod tests {
9641141
"verify.md#new-verification",
9651142
"verify.md",
9661143
"Verifications",
1144+
1,
9671145
Some(crate::element::ElementType::Verification(crate::element::VerificationType::Test))
9681146
);
9691147
verification.add_content("Verification content");

core/src/element.rs

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -114,6 +114,7 @@ pub struct Element {
114114
pub relations: Vec<Relation>,
115115
pub identifier: String,
116116
pub file_path: String,
117+
pub line_number: usize,
117118
pub element_type: ElementType,
118119
pub metadata: HashMap<String, String>,
119120
//
@@ -129,7 +130,7 @@ pub struct Element {
129130

130131

131132
impl Element {
132-
pub fn new(name: &str, identifier: &str, file_path: &str, section: &str, element_type: Option<ElementType>) -> Self {
133+
pub fn new(name: &str, identifier: &str, file_path: &str, section: &str, line_number: usize, element_type: Option<ElementType>) -> Self {
133134
Self {
134135
name: name.to_string(),
135136
content: "".to_string(),
@@ -138,6 +139,7 @@ impl Element {
138139
relations: vec![],
139140
identifier: identifier.to_string(),
140141
file_path: file_path.to_string(),
142+
line_number,
141143
element_type: element_type.unwrap_or(ElementType::Requirement(RequirementType::System)),
142144
metadata: HashMap::new(),
143145
changed_since_commit: false,

0 commit comments

Comments
 (0)