review classifier: plain table cells, no backslash escapes
This commit is contained in:
parent
a9d181b53f
commit
a2dca32bb8
3 changed files with 57 additions and 18 deletions
|
|
@ -398,11 +398,14 @@ followed by the table when there is one. A comment neither approves nor
|
||||||
blocks the merge: nine timeouts and one pass post a comment, not an
|
blocks the merge: nine timeouts and one pass post a comment, not an
|
||||||
approval.
|
approval.
|
||||||
|
|
||||||
The controller flattens every cell to one line, with `|`, backticks, brackets, `<`, `>`,
|
The controller flattens every cell to one line and backslash-escapes the
|
||||||
`&`, `#` and other markup characters escaped, and a zero-width space after
|
characters that could end the cell or start markup, math or raw HTML: `|`, `\`,
|
||||||
each `@`, `#`, `!`, `://` and `www.`, so a model's `reason` can't break the
|
backticks, `[`, `]`, `<`, `>`, `&`, `*`, `~`, `$`, and `_` except between two
|
||||||
table, embed markup, mention anyone, reference an issue or PR, or carry a
|
letters or digits (`ci_metrics.rs` stays as is). Ordinary punctuation such as
|
||||||
link.
|
`(`, `)` and `.` passes through unescaped. Each `@`, `#`, `!`, `://` and `www.`
|
||||||
|
gets a zero-width space after it, with no backslash, so a model's `reason`
|
||||||
|
can't break the table, embed markup, mention anyone, reference an issue or PR,
|
||||||
|
or carry a link. The prompt asks the model for plain text in `reason`.
|
||||||
|
|
||||||
## Runtimes
|
## Runtimes
|
||||||
|
|
||||||
|
|
|
||||||
|
|
@ -842,26 +842,37 @@ fn overall_line(overall: Overall) -> &'static str {
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
/// `text` as one table cell's inline text: on one line, with every character
|
/// `text` as one table cell's inline text: on one line, with a backslash before
|
||||||
/// that could end the cell, start markup, math (`$`) or raw HTML escaped, and a zero-width
|
/// every character that could end the cell, start markup, math (`$`) or raw
|
||||||
/// space after every `@`, `#`, `!`, `://` and `www.`, so it carries no live
|
/// HTML, and a zero-width space after every `@`, `#`, `!`, `://` and `www.`, so
|
||||||
/// mention, issue or PR reference, or link. The forge links issue and PR
|
/// it carries no live mention, issue or PR reference, or link. The forge links
|
||||||
/// numbers and bare URLs itself, so the escapes alone don't stop those.
|
/// issue and PR numbers and bare URLs itself, so the escapes alone don't stop
|
||||||
|
/// those; `#` and `!` get only the zero-width space.
|
||||||
|
///
|
||||||
|
/// `_` is escaped except between two alphanumerics, where it can't open
|
||||||
|
/// emphasis (`ci_metrics.rs`). Other punctuation, such as `(`, `)`, `.` and
|
||||||
|
/// `:`, passes through: it starts no markup inside a table cell, and the `[`
|
||||||
|
/// that would make `(` a link target is escaped.
|
||||||
fn cell(text: &str) -> String {
|
fn cell(text: &str) -> String {
|
||||||
let mut out = String::with_capacity(text.len());
|
let mut out = String::with_capacity(text.len());
|
||||||
for (i, word) in text.split_whitespace().enumerate() {
|
for (i, word) in text.split_whitespace().enumerate() {
|
||||||
if i > 0 {
|
if i > 0 {
|
||||||
out.push(' ');
|
out.push(' ');
|
||||||
}
|
}
|
||||||
for c in word.chars() {
|
let chars: Vec<char> = word.chars().collect();
|
||||||
|
for (j, &c) in chars.iter().enumerate() {
|
||||||
match c {
|
match c {
|
||||||
'\\' | '|' | '`' | '*' | '_' | '~' | '[' | ']' | '(' | ')' | '<' | '>' | '&'
|
'_' if j > 0
|
||||||
| '$' => {
|
&& chars[j - 1].is_alphanumeric()
|
||||||
|
&& chars.get(j + 1).is_some_and(|n| n.is_alphanumeric()) =>
|
||||||
|
{
|
||||||
|
out.push(c);
|
||||||
|
}
|
||||||
|
'\\' | '|' | '`' | '*' | '_' | '~' | '[' | ']' | '<' | '>' | '&' | '$' => {
|
||||||
out.push('\\');
|
out.push('\\');
|
||||||
out.push(c);
|
out.push(c);
|
||||||
}
|
}
|
||||||
'#' | '!' => {
|
'#' | '!' => {
|
||||||
out.push('\\');
|
|
||||||
out.push(c);
|
out.push(c);
|
||||||
out.push('\u{200b}');
|
out.push('\u{200b}');
|
||||||
}
|
}
|
||||||
|
|
@ -1932,7 +1943,7 @@ mod tests {
|
||||||
.count();
|
.count();
|
||||||
assert_eq!(unescaped, 5, "{line}");
|
assert_eq!(unescaped, 5, "{line}");
|
||||||
assert!(
|
assert!(
|
||||||
line.contains("\\!\u{200b}\\[\\]") && line.contains("\\<b\\>"),
|
line.contains("!\u{200b}\\[\\]") && line.contains("\\<b\\>"),
|
||||||
"{line}"
|
"{line}"
|
||||||
);
|
);
|
||||||
}
|
}
|
||||||
|
|
@ -1965,8 +1976,33 @@ mod tests {
|
||||||
let text = cell("see https://evil.example/x, WWW.evil.example, #1, o/r#3 and !4");
|
let text = cell("see https://evil.example/x, WWW.evil.example, #1, o/r#3 and !4");
|
||||||
assert_eq!(
|
assert_eq!(
|
||||||
text,
|
text,
|
||||||
"see https://\u{200b}evil.example/x, WWW.\u{200b}evil.example, \\#\u{200b}1, \
|
"see https://\u{200b}evil.example/x, WWW.\u{200b}evil.example, #\u{200b}1, \
|
||||||
o/r\\#\u{200b}3 and \\!\u{200b}4"
|
o/r#\u{200b}3 and !\u{200b}4"
|
||||||
|
);
|
||||||
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn a_plain_reason_reads_without_backslashes() {
|
||||||
|
assert_eq!(
|
||||||
|
cell("an added #[allow(clippy::foo)] in ci_metrics.rs (see main_loop_v2.rs)"),
|
||||||
|
"an added #\u{200b}\\[allow(clippy::foo)\\] in ci_metrics.rs (see main_loop_v2.rs)"
|
||||||
|
);
|
||||||
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn an_underscore_escapes_unless_it_sits_between_alphanumerics() {
|
||||||
|
// Presence control: the edge underscores are escaped.
|
||||||
|
assert_eq!(
|
||||||
|
cell("_em_ __b__ a_ _b a__b snake_case"),
|
||||||
|
"\\_em\\_ \\_\\_b\\_\\_ a\\_ \\_b a\\_\\_b snake_case"
|
||||||
|
);
|
||||||
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn a_reason_with_markup_characters_keeps_them_inert() {
|
||||||
|
assert_eq!(
|
||||||
|
cell(r"a\b `c` *d* ~e~ [f] <g> &h; i|j $k"),
|
||||||
|
r"a\\b \`c\` \*d\* \~e\~ \[f\] \<g\> \&h; i\|j \$k"
|
||||||
);
|
);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|
|
||||||
|
|
@ -26,7 +26,7 @@ Answer with exactly one JSON object and nothing else, no code fence:
|
||||||
- \"no-match\": the answer is no.
|
- \"no-match\": the answer is no.
|
||||||
- \"not-applicable\": the question does not concern anything this pull request changes.
|
- \"not-applicable\": the question does not concern anything this pull request changes.
|
||||||
- \"certainty\": how sure you are of that verdict, 0 to 100.
|
- \"certainty\": how sure you are of that verdict, 0 to 100.
|
||||||
- \"reason\": one or two sentences saying why.";
|
- \"reason\": one or two sentences saying why, in plain text only: no markdown, no backticks, no backslash escapes.";
|
||||||
|
|
||||||
/// The shared context: the PR's commits with their full messages, then its
|
/// The shared context: the PR's commits with their full messages, then its
|
||||||
/// diff with lockfiles collapsed. Built once per run.
|
/// diff with lockfiles collapsed. Built once per run.
|
||||||
|
|
|
||||||
Loading…
Reference in a new issue