Cut RS-L second fix: title is is_alphanumeric/is_control/bidi, OrgRepo
refuses .git and gains identity()
Soul's F2 finding on the second RS-L pass: a 1-byte title, " ", "\0", "\n",
U+200B and U+FEFF all passed as "non-empty" under the whitespace-plus-hand-
written-denylist check, because that check tested is_whitespace() rather
than the actual grammar rule (F2) or "not a bare placeholder" (F1/F3).
title_text is rewritten to the three-rule grammar Self ruled: at least one
char::is_alphanumeric() character, no char::is_control() character, and no
bidi control character (U+061C, U+200E, U+200F, U+202A-U+202E,
U+2066-U+2069). The hand-written C0/C1 ranges and the zero-width/BOM
denylist are deleted outright, not narrowed -- a character not covered by
one of the three tests is allowed, which is the point: no more enumerated
exception list to keep in sync with Unicode.
Discrepancy from the fix spec: the spec's refused-case list names "U+3164
alone" (HANGUL FILLER). Unicode gives it General_Category Lo, so
char::is_alphanumeric() is true for it on this toolchain (confirmed by a
failing assertion on Yggdrasil before the test was corrected). Reintroducing
a denylist entry for one code point to force that single case would
contradict the "delete the denylist" instruction this same fix batch
carries, so the test now pins U+3164 alone as accepted and documents why
inline.
OrgRepo (F7's GitHub grammar, unchanged in shape) gains two things:
- a repo name ending in ".git" is refused (GitHub strips it, so a document
that carried it would silently alias another stored repo);
- OrgRepo::identity() is the one canonical key, ASCII-lowercased, with a doc
comment saying every comparison and lookup goes through it. Huginn adopts
it at BP-2 (out of scope here).
The dead `!repo.contains('/')` check is deleted: the repo byte alphabet
already excludes '/', so a second slash was always refused by the byte
check, never by that clause.
Consecutive hyphens stay allowed (GitHub's own signup rule is stricter than
its stored-repo grammar; the ruling this leaf implements is silent on
consecutive hyphens, so nothing here narrows it).
OrgRepo's published maxLength moves from 200 to 140 (39 + 1 + 100, the true
grammar maximum); the Title and OrgRepo doc comments now state the rule and
the true maxima directly and name no private function, since the derived
JSON Schema descriptions come from these comments verbatim.
Tests added/changed in question_and_ruling_carry_a_title and
the_org_repo_and_label_doors_are_the_grammar: the full refused/accepted
title matrix from the fix spec (RLO alone and with text, LRM alone, a lone
combining mark, a variation selector alone, NBSP alone, emoji-only,
Persian text with ZWNJ, a ZWJ pair, "a 🚀", 200 accepted / 201 refused
bytes already pinned); OrgRepo's owner=39/repo=100 accepted boundaries
beside the existing 40/101 refused ones, a/b.git and a/.git refused,
a--b/c--d (consecutive hyphens) accepted, and identity() lowercasing.
Hand probes on Yggdrasil (M0 green, restored): H01-owner-max-38,
H02-owner-min-2, H03-repo-max-99, H04-repo-min-2, H05-repo-no-hyphen,
H05b-repo-no-dot, H05c-repo-no-underscore and H05d-owner-no-hyphen all still
die under the rewrite. H06/H06b (2028/2029), H07/H07b/H07c (C0/C1 ranges),
H08 (zero-width-only), H09/H09b (U+2060/U+FEFF) and H10 (is_whitespace vs
is_ascii_whitespace) no longer apply: the code they mutate no longer exists,
since this fix batch deletes exactly that denylist and that whitespace
check.
cargo test -p epiphany-pipeline --lib: 37/37 on Yggdrasil.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This commit is contained in:
Meta Vens
parent
1298dace8c
commit
998f97e84d
1 changed files with 83 additions and 32 deletions
|
|
@ -156,11 +156,13 @@ fn dotted_text(field: &str, value: &str) -> Result<(), PipelineRefusal> {
|
|||
|
||||
/// GitHub's own grammar for `owner/repo`, exactly one `/`: the owner is
|
||||
/// `[A-Za-z0-9-]`, 1 to 39 bytes, with no leading or trailing hyphen; the repo
|
||||
/// is `[A-Za-z0-9._-]`, 1 to 100 bytes, and is never `.` or `..`.
|
||||
/// is `[A-Za-z0-9._-]`, 1 to 100 bytes, is never `.` or `..`, and does not end
|
||||
/// in `.git`. The repo's own byte alphabet excludes `/`, so a second `/`
|
||||
/// inside it is already refused by the byte check; no separate
|
||||
/// `contains('/')` check is needed.
|
||||
fn org_repo_text(field: &str, value: &str) -> Result<(), PipelineRefusal> {
|
||||
let valid = value.split_once('/').is_some_and(|(owner, repo)| {
|
||||
!repo.contains('/')
|
||||
&& (1..=39).contains(&owner.len())
|
||||
(1..=39).contains(&owner.len())
|
||||
&& owner.bytes().all(|byte| byte.is_ascii_alphanumeric() || byte == b'-')
|
||||
&& !owner.starts_with('-')
|
||||
&& !owner.ends_with('-')
|
||||
|
|
@ -168,6 +170,7 @@ fn org_repo_text(field: &str, value: &str) -> Result<(), PipelineRefusal> {
|
|||
&& repo.bytes().all(|byte| byte.is_ascii_alphanumeric() || matches!(byte, b'.' | b'_' | b'-'))
|
||||
&& repo != "."
|
||||
&& repo != ".."
|
||||
&& !repo.ends_with(".git")
|
||||
});
|
||||
if valid {
|
||||
Ok(())
|
||||
|
|
@ -201,19 +204,21 @@ fn within(limit: usize) -> impl Fn(&str, &str) -> Result<(), PipelineRefusal> {
|
|||
move |field: &str, value: &str| bound(field, limit, value.len())
|
||||
}
|
||||
|
||||
/// A title holds at least one non-whitespace character and no control
|
||||
/// characters: C0 (`U+0000`-`U+001F`), C1 (`U+0080`-`U+009F`), the line and
|
||||
/// paragraph separators `U+2028`/`U+2029`, the zero-width characters
|
||||
/// `U+200B`-`U+200D` and `U+2060`, and the byte-order mark `U+FEFF`.
|
||||
/// Surrounding whitespace is otherwise allowed once a visible character is
|
||||
/// present.
|
||||
/// A title holds at least one `char::is_alphanumeric()` character, no
|
||||
/// `char::is_control()` character, and no bidi control character: `U+061C`,
|
||||
/// `U+200E`, `U+200F`, `U+202A`-`U+202E`, `U+2066`-`U+2069`. No denylist of
|
||||
/// individual zero-width or format characters is maintained here; a
|
||||
/// character not covered by one of these three tests is allowed.
|
||||
fn title_text(field: &str, value: &str) -> Result<(), PipelineRefusal> {
|
||||
let visible = value.chars().any(|character| !character.is_whitespace());
|
||||
let control = value.chars().any(|character| {
|
||||
matches!(character, '\u{0000}'..='\u{001F}' | '\u{0080}'..='\u{009F}' | '\u{2028}' | '\u{2029}')
|
||||
|| matches!(character, '\u{200B}'..='\u{200D}' | '\u{2060}' | '\u{FEFF}')
|
||||
let alphanumeric = value.chars().any(|character| character.is_alphanumeric());
|
||||
let control = value.chars().any(|character| character.is_control());
|
||||
let bidi = value.chars().any(|character| {
|
||||
matches!(
|
||||
character,
|
||||
'\u{061C}' | '\u{200E}' | '\u{200F}' | '\u{202A}'..='\u{202E}' | '\u{2066}'..='\u{2069}'
|
||||
)
|
||||
});
|
||||
if !visible || control {
|
||||
if !alphanumeric || control || bidi {
|
||||
return Err(format_error(field, value));
|
||||
}
|
||||
within(200)(field, value)
|
||||
|
|
@ -222,9 +227,9 @@ fn title_text(field: &str, value: &str) -> Result<(), PipelineRefusal> {
|
|||
bounded_text! {
|
||||
/// Text of at most 200 UTF-8 bytes.
|
||||
Short = 200, |field, value| within(200)(field, value);
|
||||
/// A title: 1 to 200 UTF-8 bytes, enforced by admission; JSON Schema
|
||||
/// length counts characters. A title holds at least one non-whitespace
|
||||
/// character and carries no control characters (see `title_text`).
|
||||
/// A title: 1 to 200 UTF-8 bytes. Holds at least one alphanumeric
|
||||
/// character, no control character, and no bidi control character
|
||||
/// (`U+061C`, `U+200E`, `U+200F`, `U+202A`-`U+202E`, `U+2066`-`U+2069`).
|
||||
#[schemars(extend("minLength" = 1))]
|
||||
Title = 200, title_text;
|
||||
/// Text of at most 1,000 UTF-8 bytes.
|
||||
|
|
@ -235,8 +240,12 @@ bounded_text! {
|
|||
Label = 64, label_text;
|
||||
/// A campaign slug or key segment: label parts joined by `.`.
|
||||
Slug = 64, dotted_text;
|
||||
/// A GitHub repository as `Org/Repo`.
|
||||
OrgRepo = 200, org_repo_text;
|
||||
/// A GitHub repository as `Org/Repo`, at most 140 UTF-8 bytes (39 + 1 +
|
||||
/// 100). The owner is 1 to 39 `[A-Za-z0-9-]` bytes with no leading or
|
||||
/// trailing hyphen; the repo is 1 to 100 `[A-Za-z0-9._-]` bytes, is never
|
||||
/// `.` or `..`, and does not end in `.git`. Compare and key repositories
|
||||
/// only through `OrgRepo::identity()`.
|
||||
OrgRepo = 140, org_repo_text;
|
||||
/// A git commit id: 7-40 lowercase hex characters.
|
||||
Sha = 40, |field, value| hex(field, value, 7..=40);
|
||||
/// A full git commit id: exactly 40 lowercase hex characters.
|
||||
|
|
@ -270,6 +279,14 @@ impl OrgRepo {
|
|||
pub fn validate_org_repo(&self) -> Result<(), PipelineRefusal> {
|
||||
Bounded::validate(self, "org_repo")
|
||||
}
|
||||
|
||||
/// The one canonical key for an `OrgRepo`: the value, ASCII-lowercased.
|
||||
/// Every comparison and every lookup keyed by repository identity goes
|
||||
/// through this, so `GameCult/Epiphany` and `gamecult/epiphany` are the
|
||||
/// same repository.
|
||||
pub fn identity(&self) -> String {
|
||||
self.0.to_ascii_lowercase()
|
||||
}
|
||||
}
|
||||
|
||||
impl Label {
|
||||
|
|
@ -2603,6 +2620,14 @@ mod tests {
|
|||
assert_eq!(OrgRepo::from("GameCult/Epiphany").validate_org_repo(), Ok(()));
|
||||
assert_eq!(OrgRepo::from("a/b").validate_org_repo(), Ok(()));
|
||||
assert_eq!(OrgRepo::from("a-b/c.d_e").validate_org_repo(), Ok(()));
|
||||
// Consecutive hyphens stay allowed on both sides; only a leading or
|
||||
// trailing owner hyphen is refused.
|
||||
assert_eq!(OrgRepo::from("a--b/c--d").validate_org_repo(), Ok(()));
|
||||
// S4: a 39-byte owner is accepted, a 100-byte repo is accepted.
|
||||
let owner_at_39 = format!("{}/b", "a".repeat(39));
|
||||
assert_eq!(OrgRepo::from(owner_at_39.as_str()).validate_org_repo(), Ok(()));
|
||||
let repo_at_100 = format!("a/{}", "a".repeat(100));
|
||||
assert_eq!(OrgRepo::from(repo_at_100.as_str()).validate_org_repo(), Ok(()));
|
||||
let repo_refused = |value: &str, why: &str| {
|
||||
let result = OrgRepo::from(value).validate_org_repo();
|
||||
assert!(
|
||||
|
|
@ -2633,6 +2658,17 @@ mod tests {
|
|||
repo_refused("a/", "an empty repo is refused");
|
||||
repo_refused("a.b/c", "a dot is not an owner byte");
|
||||
repo_refused("a_b/c", "an underscore is not an owner byte");
|
||||
// F7 fix batch: a repo name ending in `.git` is refused.
|
||||
repo_refused("a/b.git", "a repo ending in .git is refused");
|
||||
repo_refused("a/.git", "a repo of exactly .git is still a .git suffix");
|
||||
|
||||
// identity(): the one canonical key, ASCII-lowercased. Every
|
||||
// comparison and key derivation goes through it.
|
||||
assert_eq!(
|
||||
OrgRepo::from("GameCult/Epiphany").identity(),
|
||||
OrgRepo::from("gamecult/epiphany").identity()
|
||||
);
|
||||
assert_eq!(OrgRepo::from("GameCult/Epiphany").identity(), "gamecult/epiphany");
|
||||
|
||||
assert_eq!(Label::from("cut-10").validate_label(), Ok(()));
|
||||
let label_refused = |value: &str, why: &str| {
|
||||
|
|
@ -2681,12 +2717,13 @@ mod tests {
|
|||
Err(PipelineRefusal::InvalidFormat { field: "ruling.title".into(), value: String::new() })
|
||||
);
|
||||
|
||||
// F2: a title is a visible placeholder, not merely a non-empty one. A
|
||||
// string of nothing but whitespace, or one carrying a control
|
||||
// character (C0, C1, a line/paragraph separator, a zero-width
|
||||
// character, or the byte-order mark), is refused the same way an
|
||||
// empty title is; surrounding whitespace around a visible character
|
||||
// is still allowed.
|
||||
// F2 second fix batch: a title holds at least one
|
||||
// `char::is_alphanumeric()` character, no `char::is_control()`
|
||||
// character, and no bidi control character. No zero-width denylist:
|
||||
// a zero-width joiner/non-joiner inside real text is allowed, and a
|
||||
// string of nothing but invisible characters is refused because it
|
||||
// has no alphanumeric character, not because those characters are
|
||||
// individually denylisted.
|
||||
let refused = |value: &str| {
|
||||
let mut refused_question = question.clone();
|
||||
refused_question.title = Title(value.into());
|
||||
|
|
@ -2697,13 +2734,18 @@ mod tests {
|
|||
);
|
||||
};
|
||||
refused(" ");
|
||||
refused(" ");
|
||||
refused("\0");
|
||||
refused("\n");
|
||||
refused("a\nb");
|
||||
refused("\u{200b}");
|
||||
refused("\u{feff}");
|
||||
refused("\u{80}");
|
||||
refused("\u{7f}");
|
||||
refused("a\u{7f}");
|
||||
refused("\u{0}");
|
||||
refused("\u{9f}");
|
||||
refused("\u{2028}");
|
||||
refused("\u{202e}"); // RLO alone
|
||||
refused("\u{202e}text"); // RLO plus text
|
||||
refused("\u{200e}"); // LRM alone
|
||||
refused("\u{0301}"); // a lone combining mark
|
||||
refused("\u{fe0f}"); // a variation selector alone
|
||||
refused("\u{a0}"); // NBSP alone
|
||||
refused("\u{1f680}"); // emoji only
|
||||
|
||||
let accepted = |value: &str| {
|
||||
let mut accepted_question = question.clone();
|
||||
|
|
@ -2716,7 +2758,16 @@ mod tests {
|
|||
};
|
||||
accepted("a");
|
||||
accepted("\u{e9}");
|
||||
accepted("\u{200c}\u{647}\u{6cc}"); // Persian text containing ZWNJ
|
||||
accepted("a\u{200d}b");
|
||||
accepted("a \u{1f680}");
|
||||
accepted(" a ");
|
||||
// Discrepancy from the fix spec: HANGUL FILLER (U+3164) is Unicode
|
||||
// General_Category Lo, so `char::is_alphanumeric()` is true for it in
|
||||
// this toolchain; it is not a denylist candidate, since the fix
|
||||
// batch deletes the hand-written exclusion list on purpose. A title
|
||||
// of U+3164 alone is therefore accepted, not refused.
|
||||
accepted("\u{3164}");
|
||||
|
||||
question.title = Title("a".repeat(200));
|
||||
assert_eq!(PipelineDocument::Question(question.clone()).validate(), Ok(()));
|
||||
|
|
|
|||
Reference in a new issue