Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion src/spec/apply.rs
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,9 @@ fn apply_renamed(
),
});
};
if canonical.requirement_index(to).is_some() {
if let Some(target_index) = canonical.requirement_index(to)
&& target_index != source_index
{
return Err(ParseIssue {
line: Some(*line),
message: format!(
Expand Down
50 changes: 49 additions & 1 deletion src/spec/apply/tests.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
use super::*;
use crate::spec::model::Requirement;
use crate::spec::model::{CanonicalSpec, Requirement};

fn requirement(name: &str, scenarios: &[&str]) -> Requirement {
Requirement {
Expand All @@ -10,6 +10,54 @@ fn requirement(name: &str, scenarios: &[&str]) -> Requirement {
}
}

fn empty_result() -> ApplyResult {
ApplyResult {
summary: MergeSummary::default(),
warnings: Vec::new(),
}
}

#[test]
fn added_requirement_with_case_variant_name_is_rejected() {
let mut canonical = CanonicalSpec::new("Cap", "chg");
canonical.add_requirement(requirement("Temporary review state", &[]));
let incoming = requirement("Temporary Review State", &[]);
let mut result = empty_result();

let issue = apply_added(
&mut canonical,
&[DeltaOperation::Added(incoming)],
"Cap",
&mut result,
)
.unwrap_err();

assert!(issue.message.contains("cannot add existing requirement"));
assert_eq!(canonical.requirement_count(), 1);
}

#[test]
fn added_requirement_identical_up_to_case_is_skipped() {
let mut canonical = CanonicalSpec::new("Cap", "chg");
let mut existing = requirement("Temporary review state", &[]);
existing.content = "shared content".to_string();
canonical.add_requirement(existing);
let mut incoming = requirement("Temporary Review State", &[]);
incoming.content = "shared content".to_string();
let mut result = empty_result();

apply_added(
&mut canonical,
&[DeltaOperation::Added(incoming)],
"Cap",
&mut result,
)
.unwrap();

assert_eq!(result.summary.added, 0);
assert_eq!(canonical.requirement_count(), 1);
}

#[test]
fn scenario_preservation_compares_occurrence_multiplicity() {
let current = requirement("Repeated", &["Retry", "Retry"]);
Expand Down
18 changes: 16 additions & 2 deletions src/spec/model.rs
Original file line number Diff line number Diff line change
Expand Up @@ -119,8 +119,10 @@ impl CanonicalSpec {
}

pub(super) fn requirement_index(&self, name: &str) -> Option<usize> {
let identity = requirement_identity(name);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚫 [runeseer] reported by reviewdog 🐶
Highrequirement_index returns the first case-insensitive match, so on a spec that already holds both case variants — the duplicate state this commit exists to stop — a REMOVED, MODIFIED, or RENAMED operation naming the Title Case requirement silently mutates the sentence-case one instead.

  • src/spec/model.rs:123 position() stops at the first identity match and no caller checks for a second.
  • src/spec/apply.rs:111 remove_requirement(element_index) deletes that wrong element with no name reconfirmation.

self.body.iter().position(|element| {
matches!(element, BodyElement::Requirement(requirement) if requirement.name == name)
matches!(element, BodyElement::Requirement(requirement)
if requirement_identity(&requirement.name) == identity)
})
}

Expand All @@ -134,7 +136,7 @@ impl CanonicalSpec {
requirement: &Requirement,
) -> bool {
self.requirement(element_index).is_some_and(|existing| {
existing.name == requirement.name
requirement_identity(&existing.name) == requirement_identity(&requirement.name)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚫 [runeseer] reported by reviewdog 🐶
Highrequirement_matches compares names case-insensitively but still compares content byte-for-byte, and parsed content begins with ### Requirement: <name>, so a genuinely identical case-variant ADD never matches and apply_added returns "cannot add existing requirement" instead of skipping.

  • src/spec/model.rs:141 content equality includes the case-bearing header line, so the loosened name check is a no-op on real data.
  • src/spec/apply/tests.rs:43 the regression test overwrites both contents with "shared content", a value the parser cannot produce, so it passes without exercising the path.

&& existing.content
== normalize_line_endings(&requirement.content, &self.line_ending)
})
Expand Down Expand Up @@ -254,6 +256,18 @@ pub(super) struct ParseIssue {
pub(super) message: String,
}

/// Requirement identity for lookups: case- and whitespace-insensitive,
/// so a Title Case delta finds its sentence-case canonical twin instead
/// of appending a duplicate. Equality checks that decide whether an
/// edit applies keep exact comparison, so a case-only rename still
/// lands.
fn requirement_identity(name: &str) -> String {
name.split_whitespace()
.collect::<Vec<_>>()
.join(" ")
.to_ascii_lowercase()
}

fn normalize_line_endings(content: &str, line_ending: &str) -> String {
content
.replace("\r\n", "\n")
Expand Down
Loading