Skip to content

Commit 52fbc5a

Browse files
committed
fix(ai): route every report-supplied cell through one markdown encoder
The comparison table interpolated report-provided text straight into pipe-delimited rows. `fail_reason` is free-form — the suite writes whatever `classify` produced — and `ai_duel compare` will read any report file it is handed, so a `|` in one of those fields adds a column to that row and a newline ends the row early. The table stops being rectangular exactly when a matchup is already failing, which is the moment the diagnostics are actually read, and the nightly `cat`s that table into a GitHub issue body. `md_cell` escapes `|` to `\|` (the documented markdown escape, which renders correctly and keeps raw CI logs readable) and folds newlines to spaces. It is applied to every cell that originates in a report rather than in this file: `matchup_id`, `exercises`, and the reason continuation row. Uniformly, and not only where a pipe looks reachable today — deciding per field means re-deciding each time a field is added, and one of those decisions will eventually be wrong. This one is uncomfortable to write, because the invariant already had a test. `markdown_rows_are_rectangular` asserted precisely the property that was broken, and passed — every one of its fixtures was pipe-free, so it verified the renderer against input that could not trigger the hazard. It was not a weak test of the right thing; it was a confident test of the wrong thing, and it made the table look guarded for as long as it existed. `report_supplied_pipes_cannot_add_columns` is the reachability arm it never had: a matchup id containing a pipe and a `fail_reason` containing two, asserted to leave the header, separator, data and reason rows all the same width. The measurement in that test had to be fixed before it could prove anything. It first counted `line.split('|')`, which cannot distinguish `\|` from `|` — so it reported the escaped output as broken, and would equally have reported a broken encoder as fine had the counts happened to line up. It now counts separators the way a markdown parser does, skipping any pipe preceded by a backslash. A test that cannot tell the fix from the defect is not evidence either way. Evidence. Four mutants, tree restored byte-identical after each, all four killing `report_supplied_pipes_cannot_add_columns`: making `md_cell` the identity; reverting the reason call site alone; reverting the matchup-id call site alone; and keeping newline folding while dropping pipe escaping. The two single-call-site mutants are the sweep check — a partial revert fails, so the fix is not pinned at only one of the places it is applied. Disclosed gap: the `exercises` cell is encoded for uniformity but no test can kill that call site. It renders `format!("{f:?}")` of a fieldless enum, so no fixture can put a pipe in it. Reverting that one site alone leaves the suite green. It is defense against a future `FeatureKind` that carries data, not a defect being fixed, and it is stated here rather than folded into the mutant count. Assisted-by: ClaudeCode:claude-opus-5
1 parent 2ae09a7 commit 52fbc5a

1 file changed

Lines changed: 65 additions & 3 deletions

File tree

crates/phase-ai/src/duel_suite/compare.rs

Lines changed: 65 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -688,8 +688,8 @@ fn render_markdown(report: &CompareReport) -> String {
688688
(None, None) => "—".to_string(),
689689
};
690690
let cells = [
691-
row.matchup_id.clone(),
692-
exercises.join(", "),
691+
md_cell(&row.matchup_id),
692+
md_cell(&exercises.join(", ")),
693693
baseline_cell,
694694
current_cell,
695695
row.flipped_w_to_l.to_string(),
@@ -711,7 +711,8 @@ fn render_markdown(report: &CompareReport) -> String {
711711
// Reason spans one labelled cell plus blanks for the rest, so the row stays
712712
// rectangular no matter how many columns the table has.
713713
out.push_str(&format!(
714-
"| ↳ _{reason}_ |{}\n",
714+
"| ↳ _{}_ |{}\n",
715+
md_cell(reason),
715716
" |".repeat(COLUMNS.len() - 1)
716717
));
717718
}
@@ -720,6 +721,21 @@ fn render_markdown(report: &CompareReport) -> String {
720721
out
721722
}
722723

724+
/// Encode a report-provided string for one markdown table cell.
725+
///
726+
/// Every cell below that originates in a report rather than in this file goes through here.
727+
/// Report fields are free-form and arrive from JSON — `fail_reason` is whatever the suite
728+
/// wrote, and `ai_duel compare` will read whatever file it is handed — so a `|` silently adds
729+
/// a column to that row and a newline ends the row early. Either way the table stops being
730+
/// rectangular exactly when a matchup is already failing, which is the moment the diagnostics
731+
/// are actually read.
732+
///
733+
/// Applied uniformly rather than only to the fields that look risky today: deciding per field
734+
/// means re-deciding every time a field is added, and one of those decisions will be wrong.
735+
fn md_cell(text: &str) -> String {
736+
text.replace('|', "\\|").replace(['\n', '\r'], " ")
737+
}
738+
723739
/// Render the comparison table to stdout + emit a summary line.
724740
pub fn print_markdown(report: &CompareReport) {
725741
println!();
@@ -1825,6 +1841,52 @@ mod tests {
18251841
/// same cell count, or the table renders broken in the nightly drift issue that
18261842
/// `.github/workflows/ai-gate.yml` posts. Exercises all four row shapes at once: a paired row
18271843
/// that warns (so its reason continuation is emitted), a New row, and a Removed row.
1844+
/// Reachability arm for `md_cell`. `markdown_rows_are_rectangular` asserted the property
1845+
/// this test is named for, but every one of its fixtures was pipe-free — so it passed for
1846+
/// a reason unrelated to the hazard and gave false confidence about exactly the invariant
1847+
/// it claimed. A `fail_reason` is free-form text from a report, and `ai_duel compare`
1848+
/// reads whatever file it is handed, so a pipe is reachable input, not a hypothetical.
1849+
#[test]
1850+
fn report_supplied_pipes_cannot_add_columns() {
1851+
let mut baseline = mk_report(vec![mk_result("m|id", 5, 10, SuiteStatus::Pass)]);
1852+
let mut current = mk_report(vec![mk_result("m|id", 5, 10, SuiteStatus::Fail)]);
1853+
baseline.results[0].matchup_id = "m|id".into();
1854+
current.results[0].matchup_id = "m|id".into();
1855+
current.results[0].fail_reason =
1856+
Some("imbalance: p0=0.10 | CI [0.02, 0.40] | excludes 0.50".into());
1857+
1858+
let report = compare(&baseline, &current, &CompareOptions).unwrap();
1859+
let rendered = render_markdown(&report);
1860+
1861+
// PREMISE: the row really did render a reason carrying pipes, so this is not vacuous.
1862+
assert!(
1863+
rendered.contains("excludes 0.50"),
1864+
"reason must be rendered:\n{rendered}"
1865+
);
1866+
assert_eq!(report.rows[0].status, CompareStatus::Fail);
1867+
1868+
// Every row — header, separator, data, and the reason continuation — must have the
1869+
// same cell count. An unescaped pipe shows up here as a longer row.
1870+
// Count SEPARATORS the way a markdown parser does — a `|` preceded by a backslash is
1871+
// cell content, not a boundary. Splitting on the raw character cannot tell the escape
1872+
// from the hazard, so it would report this test green against a broken encoder.
1873+
let separators = |line: &str| {
1874+
line.char_indices()
1875+
.filter(|(i, c)| *c == '|' && (*i == 0 || !line[..*i].ends_with('\\')))
1876+
.count()
1877+
};
1878+
let widths: Vec<usize> = rendered
1879+
.lines()
1880+
.filter(|l| l.starts_with('|'))
1881+
.map(separators)
1882+
.collect();
1883+
assert!(widths.len() >= 4, "expected a reason row too: {widths:?}");
1884+
assert!(
1885+
widths.iter().all(|w| *w == widths[0]),
1886+
"pipes changed the column count: {widths:?}\n{rendered}"
1887+
);
1888+
}
1889+
18281890
#[test]
18291891
fn markdown_rows_are_rectangular() {
18301892
let paired_before: &[(u64, Option<u8>, u32)] = &[(1, Some(0), 10), (2, Some(1), 10)];

0 commit comments

Comments
 (0)