Skip to content
Merged
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
32 changes: 16 additions & 16 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ default-members = [
resolver = "2"

[workspace.package]
version = "0.1.1090"
version = "0.1.1091"
edition = "2024"
rust-version = "1.88"
license = "Apache-2.0"
Expand Down
27 changes: 15 additions & 12 deletions crates/cli-sub-agent/src/review_cmd_prose_findings.rs
Original file line number Diff line number Diff line change
Expand Up @@ -333,11 +333,6 @@ fn parse_severity_prefixed_finding(
allow_description_only: bool,
) -> Option<ParsedProseFinding> {
let (label, rest) = body.split_once(':')?;
// Reject compound priority specs like "P1/P2/P3" — these are acceptance-criteria
// descriptions, not single severity labels.
if label.contains('/') {
return None;
}
let severity = severity_from_label(label).or_else(|| leading_severity_from_title(label))?;
let rest = rest.trim();
if severity_prefixed_rest_is_zero_count(rest) {
Expand Down Expand Up @@ -393,16 +388,24 @@ fn strip_unordered_list_prefix(line: &str) -> &str {
}

fn leading_severity_from_title(title: &str) -> Option<Severity> {
// Reject compound specs like "P1/P2/P3" — the first alphanumeric run
// (e.g., "P1") is part of a compound, not a standalone severity label.
if title.contains('/') {
let mut segments = title.split('/');
let severity = leading_severity_from_title_segment(segments.next()?)?;

// Reject compound specs like "P1/P2/P3" and
// "High-severity/Medium-severity", while allowing descriptive segments
// such as "High correctness / sandbox violation".
if segments.any(|segment| leading_severity_from_title_segment(segment).is_some()) {
return None;
}
let first_word = title
.trim_start()

Some(severity)
}
Comment on lines 390 to +402

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

high

The current implementation of leading_severity_from_title splits the title by / and rejects the entire title if any subsequent segment starts with a word that matches a severity label (via leading_severity_from_title_segment).

This introduces a bug where perfectly valid descriptive titles containing slashes and common words like low, medium, high, or info in subsequent segments (e.g., "High performance / low overhead", "Critical security / low risk of exploit", "Medium priority / low complexity") are incorrectly classified as compound severities and silently rejected.

Solution

Instead of checking if any subsequent segment merely starts with a severity label, we should check if any subsequent segment is solely a severity label (optionally with common noise words like severity or priority and markdown formatting). If a segment contains other descriptive words (like overhead or complexity), it is a descriptive segment and should not trigger rejection.

fn leading_severity_from_title(title: &str) -> Option<Severity> {
    let mut segments = title.split('/');
    let severity = leading_severity_from_title_segment(segments.next()?)?;

    // Reject compound specs like "P1/P2/P3" and
    // "High-severity/Medium-severity", while allowing descriptive segments
    // such as "High correctness / sandbox violation" or "High performance / low overhead".
    let is_pure_severity_segment = |segment: &str| -> bool {
        let mut words = segment
            .split(|ch: char| !ch.is_ascii_alphanumeric())
            .filter(|word| !word.is_empty());
        let mut has_severity = false;
        for word in words {
            if severity_from_label(word).is_some() {
                has_severity = true;
            } else if word.eq_ignore_ascii_case("severity") || word.eq_ignore_ascii_case("priority") {
                // Allowed noise words in compound severity specs
            } else {
                return false;
            }
        }
        has_severity
    };

    if segments.any(is_pure_severity_segment) {
        return None;
    }

    Some(severity)
}


fn leading_severity_from_title_segment(segment: &str) -> Option<Severity> {
segment
.split(|ch: char| !ch.is_ascii_alphanumeric())
.find(|word| !word.is_empty())?;
severity_from_label(first_word)
.find(|word| !word.is_empty())
.and_then(severity_from_label)
}

fn severity_prefixed_description(label: &str, rest: &str) -> String {
Expand Down
51 changes: 51 additions & 0 deletions crates/cli-sub-agent/src/review_cmd_prose_findings_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -262,3 +262,54 @@ fn issue_2637_compound_priority_not_blocking_signal() {
"P1/P2/P3: code paths now classify status into bounded cause labels."
));
}

#[test]
fn issue_2652_title_leading_severities_allow_slashes_in_descriptions() {
for (title, expected_severity) in [
("Critical security / isolation failure", Severity::Critical),
("High correctness / sandbox violation", Severity::High),
("Medium regression / test gap", Severity::Medium),
("Low docs / help mismatch", Severity::Low),
("Info docs / help mismatch", Severity::Low),
("P0 security / isolation failure", Severity::Critical),
("P1 correctness / lock race", Severity::High),
("P2 regression / test gap", Severity::Medium),
("P3 docs / help mismatch", Severity::Low),
] {
let text = format!("## Findings\n1. {title}: active problem remains\n");
let findings = extract_review_findings_from_prose(&text);
assert_eq!(findings.len(), 1, "title should parse: {title}");
assert_eq!(findings[0].severity, expected_severity, "title: {title}");
}
}

#[test]
fn issue_2652_decorated_title_leading_severities_still_parse() {
for (title, expected_severity) in [
("**High correctness / sandbox violation**", Severity::High),
("`P1` correctness / lock race", Severity::High),
("_Medium_ regression / test gap", Severity::Medium),
] {
let text = format!("## Findings\n1. {title}: active problem remains\n");
let findings = extract_review_findings_from_prose(&text);
assert_eq!(findings.len(), 1, "decorated title should parse: {title}");
assert_eq!(findings[0].severity, expected_severity, "title: {title}");
}
}

#[test]
fn issue_2652_compound_severity_prefix_stays_rejected() {
for title in [
"High/Medium",
"P1/P2/P3",
"High-severity/Medium-severity",
"**High** / **Medium**",
] {
let text = format!("## Findings\n{title}: acceptance criteria summary\n");
let findings = extract_review_findings_from_prose(&text);
assert!(
findings.is_empty(),
"compound severity prefix should not parse: {title}"
);
}
}
4 changes: 2 additions & 2 deletions weave.lock
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
[versions]
csa = "0.1.1090"
weave = "0.1.1090"
csa = "0.1.1091"
weave = "0.1.1091"
last_migrated_at = "2026-03-08T12:08:01.820964091Z"

[migrations]
Expand Down
Loading