diff --git a/docs/REFERENCE.md b/docs/REFERENCE.md index 4e760b5..23d44eb 100644 --- a/docs/REFERENCE.md +++ b/docs/REFERENCE.md @@ -1487,9 +1487,15 @@ on a merge, and the subject GitHub composes for `gh pr merge --squash` or `glab mr merge -m`/`--squash-message` or GitLab's `squash_commit_message` and `merge_commit_message` fields, is judged as its first line, a subject, and the rest, prose. An `--input` JSON document is prose in every value, `title` -included. A pull-request, -issue, release or gist body, a comment, and -anything handed to `uphold guard --text` or the harness hook is prose a reader +included. The harness hook reads a tool call's arguments by the same keys: a +string under `title`, `commit_title` or `subject` is a subject, and one under +`squash_commit_message` or `merge_commit_message` is a message, so an MCP +merge's commit title gets the check `gh pr merge --subject` gets. The keys are read in +every tool the hook's matcher sends it, not only the forge tools, so a matcher +wider than the documented one puts a `title`, `subject` or `commit_title` of +any other tool under the lookalike check as well, and a finding names the key. A pull-request, +issue, release or gist body, a comment, anything handed to +`uphold guard --text`, and every other string of a tool call is prose a reader reads, and the rule asks it only for characters that draw nothing — so `gh issue edit --body-file` over a body that carries a degree sign someone typed months ago runs. **An edit is judged on what it adds.** `gh issue edit` and `gh pr edit` resubmit the whole body, so @@ -2158,7 +2164,7 @@ in front of its own lookup is a loop. |---|---| | `-F`/`--field` | the value after `key=`, with `@file` read from the file and `@-` from stdin | | `-f`/`--raw-field` | the value after `key=` as written, `@` included, because that is what is sent | -| either, by key | a **title** where the key is `title` or `commit_title`; a **message** (first line a title, the rest prose) where it is `squash_commit_message` or `merge_commit_message`; prose otherwise | +| either, by key | a **title** where the key is `title`, `commit_title` or `subject`; a **message** (first line a title, the rest prose) where it is `squash_commit_message` or `merge_commit_message`; prose otherwise | | `--input FILE` | the whole file — each **string value** where it is JSON, the raw text where it is not; every value is prose, a `title` key included | | the endpoint path | `repos/OWNER/REPO/…`, or GitLab's `projects/OWNER%2FREPO`, as the **destination** | @@ -2529,6 +2535,16 @@ list of field names per tool: a server decides what to call the field holding a body, a release note or a branch name, and a table of those names is a table missing the one a new server just added — silently, and in the green direction. +A key decides only which check a string gets. A string whose nearest key is +`title`, `commit_title` or `subject` is judged as a subject, and one under +`squash_commit_message` or `merge_commit_message` as a subject on its first line +and prose below it — the closed list the shim reads `gh api` and `glab api` +fields by, one list and not a copy. It fails the other way from a table of +fields to read: a title key it lacks leaves that string prose, which is how +every string here was judged before the list existed, so a missing key costs one +field its lookalike pass and never costs the seam a string. Every checker other +than the guards still reads the call's strings joined. + ### Adding a harness The shapes are data, one row per harness: a pointer to what the call is called, diff --git a/src/hook.rs b/src/hook.rs index 17b9b93..6b826b6 100644 --- a/src/hook.rs +++ b/src/hook.rs @@ -51,6 +51,7 @@ use serde_json::Value; use crate::error::{Exit, Fatal, Result}; use crate::guard; +use crate::shim::{API_MESSAGE_KEYS, API_TITLE_KEYS, Subject, message_subjects}; use crate::text::{self, Verdict}; /// What one harness calls the three things every harness has. @@ -90,37 +91,70 @@ pub(crate) fn known() -> String { .join("|") } -/// Every string anywhere under `value`. +/// Every string anywhere under `value`, as prose or as a headline. /// -/// Deliberately not a list of field names per harness and per tool. A server -/// decides what to call the field holding a pull-request body, a release note -/// or a branch name, and a table of those names is a table that is missing the -/// one a new server just added -- silently, and in the green direction. Reading -/// them all costs a second pass over text that is already in memory; missing -/// one costs the whole seam. +/// Deliberately not a list of field names per harness and per tool to READ. A +/// server decides what to call the field holding a pull-request body, a release +/// note or a branch name, and a table of those names is a table that is missing +/// the one a new server just added -- silently, and in the green direction. +/// Reading them all costs a second pass over text that is already in memory; +/// missing one costs the whole seam. Every string is read, whatever its key. +/// +/// What a key does decide is which CHECK a string gets, and only towards the +/// stricter one. A string whose nearest key is one the shim already treats as +/// a title in an `api` field -- [`API_TITLE_KEYS`] -- is a headline, and one +/// under a whole-message key -- [`API_MESSAGE_KEYS`] -- is a headline for its +/// first line and prose for the rest. That list fails the other way from a +/// table of fields to read: a title key it lacks leaves that string prose, +/// which is what every string here was judged as before, so a missing key costs +/// the lookalike pass on one field and never the seam. It is the shim's list, +/// not a copy of it, so a key added for `gh api` is a key here too. /// /// The order is whatever the JSON reader hands back, which for an object is by /// key rather than by the order somebody wrote the fields in. Nothing here /// depends on it: the strings are joined and searched, and a rule that matched /// only when two fields happened to be adjacent would be a rule about the /// harness's serializer. -fn strings(value: &Value, into: &mut Vec) { +fn strings(value: &Value, key: Option<&str>, into: &mut Collected) { match value { - Value::String(text) => into.push(text.clone()), + Value::String(text) => match key { + Some(key) if API_TITLE_KEYS.contains(&key) => { + into.headlines.push((key.to_owned(), text.clone())); + } + Some(key) if API_MESSAGE_KEYS.contains(&key) => { + for subject in message_subjects(text.clone()) { + if subject.kind == Subject::HEADLINE { + into.headlines.push((key.to_owned(), subject.value)); + } else { + into.prose.push(subject.value); + } + } + } + _ => into.prose.push(text.clone()), + }, Value::Array(items) => { for item in items { - strings(item, into); + strings(item, key, into); } } Value::Object(fields) => { - for field in fields.values() { - strings(field, into); + for (name, field) in fields { + strings(field, Some(name), into); } } _ => {} } } +/// A tool call's strings, sorted by the check each gets. A headline keeps the +/// key it was found under, so a finding names `commit_title` or `subject` +/// rather than a generic "title" the call never said. +#[derive(Default)] +struct Collected { + prose: Vec, + headlines: Vec<(String, String)>, +} + /// Put the report inside the harness's refusal document. /// /// Through `serde_json` rather than by formatting a string, because a report @@ -235,17 +269,25 @@ pub(crate) fn run(harness: &str, found: Option<&(PathBuf, PathBuf)>) -> Result = collected + .headlines + .into_iter() + .map(|(key, headline)| text::Headline { + label: format!("{label} {key}"), + text: headline, + }) + .collect(); let mut report = String::new(); - for verdict in text::judged(text::Seam::Hook, &root, &policy, label, &text)? { + for verdict in text::judged(text::Seam::Hook, &root, &policy, label, &text, &headlines)? { match verdict { // The message as well as the finding. At the shim and at `--text` // the message is what tells the author what to do instead; here the @@ -305,10 +347,33 @@ mod tests { r#"{"title":"a","nested":{"body":"b"},"list":["c"],"count":1,"flag":true}"#, ) .unwrap(); - let mut found = Vec::new(); - strings(&event, &mut found); - found.sort(); - assert_eq!(found, vec!["a", "b", "c"]); + let mut found = Collected::default(); + strings(&event, None, &mut found); + found.prose.sort(); + assert_eq!(found.prose, vec!["b", "c"]); + assert_eq!(found.headlines, vec![("title".to_owned(), "a".to_owned())]); + } + + /// A title key is read at any depth, and a message key splits its value + /// into the subject line and the body below it. + #[test] + fn a_title_key_marks_a_headline_at_any_depth() { + let event: Value = serde_json::from_str( + r#"{"merge":{"commit_title":"t","subject":["s"]},"squash_commit_message":"m\n\nbody"}"#, + ) + .unwrap(); + let mut found = Collected::default(); + strings(&event, None, &mut found); + found.headlines.sort(); + assert_eq!( + found.headlines, + vec![ + ("commit_title".to_owned(), "t".to_owned()), + ("squash_commit_message".to_owned(), "m".to_owned()), + ("subject".to_owned(), "s".to_owned()), + ] + ); + assert_eq!(found.prose, vec!["\n\nbody"]); } /// The report is what the offending text is quoted in, so the one thing the diff --git a/src/main.rs b/src/main.rs index 3d83f97..ef49e25 100644 --- a/src/main.rs +++ b/src/main.rs @@ -768,7 +768,7 @@ fn guard_command(arguments: &[OsString]) -> Result { // reports in: a finding reached through `--text` and one reached // through a guard are the same verdict on the same text. let refusals: Vec = - text::judged(text::Seam::Guard, &root, &policy, &source, &text)? + text::judged(text::Seam::Guard, &root, &policy, &source, &text, &[])? .into_iter() .map(|verdict| match verdict { text::Verdict::Guard(refusal) => refusal, diff --git a/src/shim.rs b/src/shim.rs index 191ba16..b099071 100644 --- a/src/shim.rs +++ b/src/shim.rs @@ -280,7 +280,7 @@ fn read_cluster(argument: &str, arity: impl Fn(&str) -> Option) -> Cluster /// A whole commit message as the subjects it is: its first line a title, /// because that line is the commit subject, and the rest a body. -fn message_subjects(message: String) -> Vec { +pub(crate) fn message_subjects(message: String) -> Vec { let Some((first, rest)) = message.split_once('\n') else { return vec![Subject { kind: Subject::HEADLINE, @@ -555,17 +555,26 @@ const API_FIELD_FLAGS: &[&str] = &["-f", "--raw-field", "-F", "--field"]; /// The typed field spellings, whose `@file` is read. const API_TYPED_FIELD_FLAGS: &[&str] = &["-F", "--field"]; -/// The `api` field keys whose value is a headline rather than prose: a pull -/// request's, merge request's or issue's `title` on either forge, and the -/// `commit_title` a GitHub merge writes as the commit subject. The same subject kind `--title` and `--subject` collect, -/// so the same text gets the same check whichever door it came through. -const API_TITLE_KEYS: &[&str] = &["title", "commit_title"]; +/// The field keys whose value is a headline rather than prose: a pull +/// request's, merge request's or issue's `title` on either forge, the +/// `commit_title` a GitHub merge writes as the commit subject, and the +/// `subject` that `--subject` spells. The same subject kind `--title` and +/// `--subject` collect, so the same text gets the same check whichever door it +/// came through -- an `api` field here, and a tool call's argument at the +/// harness hook, which reads this same list. +/// +/// A closed list, and safe as one in a way a table of fields to READ is not: a +/// key missing from it leaves its value judged as prose, which is what every +/// string was judged as before the list existed. It can add a check and never +/// remove one. +pub(crate) const API_TITLE_KEYS: &[&str] = &["title", "commit_title", "subject"]; -/// The `api` field keys whose value is a whole commit message: GitLab's +/// The field keys whose value is a whole commit message: GitLab's /// `squash_commit_message` and `merge_commit_message` on a merge request's /// merge. The first line is the subject and is judged as a title, the rest as -/// a body, the way `glab mr merge --squash-message` is. -const API_MESSAGE_KEYS: &[&str] = &["squash_commit_message", "merge_commit_message"]; +/// a body, the way `glab mr merge --squash-message` is. Read by the harness +/// hook too, for the same reason as [`API_TITLE_KEYS`]. +pub(crate) const API_MESSAGE_KEYS: &[&str] = &["squash_commit_message", "merge_commit_message"]; /// Whether an `api` option takes the word after it. fn api_takes_value(flag: &str) -> bool { diff --git a/src/text.rs b/src/text.rs index 6147b12..61fe388 100644 --- a/src/text.rs +++ b/src/text.rs @@ -290,21 +290,44 @@ pub(crate) fn over_kinds( Ok(verdicts) } +/// One headline a seam was handed beside its prose: a title, or a commit +/// message's first line, and what the report calls it. +pub(crate) struct Headline { + pub label: String, + pub text: String, +} + /// Everything a piece of published text is judged by, at one seam. /// /// The single assembly. `label` is what the subject is called in a report -- /// the tool name at the hook, the source at `--text` -- and is reported rather -/// than matched on. +/// than matched on. `text` is the prose; `headlines` are the subjects the seam +/// could tell apart from it, which only the harness hook can, by a tool call's +/// argument names. Every checker but the guards reads the two joined, as it +/// always has; the guards are handed each headline as one, so the lookalike +/// pass a title gets at the shim is the pass it gets here. pub(crate) fn judged( seam: Seam, root: &std::path::Path, policy: &Policy, label: &str, text: &str, + headlines: &[Headline], ) -> Result> { + let joined; + let whole = if headlines.is_empty() { + text + } else { + joined = std::iter::once(text) + .chain(headlines.iter().map(|headline| headline.text.as_str())) + .filter(|part| !part.is_empty()) + .collect::>() + .join("\n"); + joined.as_str() + }; over_kinds(seam, |kind| { Ok(match kind { - Judged::Literals => failures_in(root, policy, text)? + Judged::Literals => failures_in(root, policy, whole)? .into_iter() .map(Verdict::Rule) .collect(), @@ -314,27 +337,50 @@ pub(crate) fn judged( // `public-target` scope about. The shim, which does have one, // passes its own memo. Judged::Guards => { - // Not a headline: what these seams are handed is a body, or a - // tool call's strings joined, and neither says which part of - // it is a subject. - let published = crate::guard::Published { - label, - text, - added: None, - headline: false, - }; - crate::guard::over_text(root, policy, None, &published, &mut |_| Ok(true))? - .into_iter() - .map(Verdict::Guard) - .collect() + // The prose is not a headline: what these seams are handed is a + // body, or a tool call's strings joined, and neither says which + // part of it is a subject. The headlines are, each on its own, + // because a subject is judged line by line as one. + let mut verdicts = Vec::new(); + if !text.is_empty() || headlines.is_empty() { + let published = crate::guard::Published { + label, + text, + added: None, + headline: false, + }; + verdicts.extend(crate::guard::over_text( + root, + policy, + None, + &published, + &mut |_| Ok(true), + )?); + } + for headline in headlines { + let published = crate::guard::Published { + label: &headline.label, + text: &headline.text, + added: None, + headline: true, + }; + verdicts.extend(crate::guard::over_text( + root, + policy, + None, + &published, + &mut |_| Ok(true), + )?); + } + verdicts.into_iter().map(Verdict::Guard).collect() } - Judged::Prose => crate::prose::over_text(policy, seam, text)? + Judged::Prose => crate::prose::over_text(policy, seam, whole)? .into_iter() .map(Verdict::Rule) .collect(), // Only the hook consults these here, and only the rules whose // `seams` names it -- see `Seam::runs_by_default`. - Judged::Patterns => patterns_over(policy, seam, label, text)? + Judged::Patterns => patterns_over(policy, seam, label, whole)? .into_iter() .map(Verdict::Rule) .collect(), @@ -414,7 +460,7 @@ fn patterns_over(policy: &Policy, seam: Seam, label: &str, text: &str) -> Result pub(crate) fn check(found: Option<&(PathBuf, PathBuf)>, source: &str) -> Result { let text = read(source)?; let (root, policy) = load_for(found)?; - let verdicts = judged(Seam::Scan, &root, &policy, source, &text)?; + let verdicts = judged(Seam::Scan, &root, &policy, source, &text, &[])?; for verdict in &verdicts { match verdict { diff --git a/tests/hook_cli.rs b/tests/hook_cli.rs index 758cd95..9f51e56 100644 --- a/tests/hook_cli.rs +++ b/tests/hook_cli.rs @@ -613,3 +613,145 @@ fn a_regexp_rule_that_does_not_name_the_hook_is_not_asked_there() { assert_eq!(code(&output), 0, "{}", stderr(&output)); assert_eq!(stdout(&output), "", "{}", stdout(&output)); } + +// -- a title argument is judged as a title --------------------------------- + +/// The invisible-character and lookalike guard, declared the way a repository +/// declares it. +const UNICODE_POLICY: &str = "\ +[rule.prevent-unusual-unicode] +builtin = \"prevent-unusual-unicode\" +git.hooks = [\"commit-msg\"] +"; + +/// A Latin word with a Cyrillic small a (U+0430) in it: a lookalike in a +/// subject, and an ordinary letter in prose. +const DISGUISED: &str = "Fix the p\u{0430}rser"; + +fn unicode_call(root_name: &str, tool: &str, input: &Value) -> Output { + let root = workspace(root_name, Some(UNICODE_POLICY)); + let event = serde_json::json!({"tool_name": tool, "tool_input": input}).to_string(); + hook(&root, "claude-code", &event) +} + +fn refused_as_lookalike(output: &Output) { + assert_eq!(code(output), 0, "{}", stderr(output)); + let reason = reason(output); + assert!(reason.contains("prevent-unusual-unicode"), "{reason}"); + assert!(reason.contains("CYRILLIC SMALL LETTER A"), "{reason}"); +} + +/// A merge's `commit_title` is the commit subject the forge writes, and the +/// shim judges `gh pr merge --subject` and `gh api -f commit_title=` as one. The +/// same title handed to an MCP server gets the same lookalike pass. +#[test] +fn a_commit_title_argument_is_judged_as_a_subject() { + let output = unicode_call( + "hook-commit-title", + "mcp__github__merge_pull_request", + &serde_json::json!({"pullNumber": 7, "commit_title": DISGUISED}), + ); + refused_as_lookalike(&output); +} + +/// A finding names the key the subject was found under, so the reader is +/// pointed at `commit_title` or `squash_commit_message` and not at a "title" +/// the call never carried. +#[test] +fn a_finding_names_the_key_it_was_found_under() { + for (name, key) in [ + ("hook-key-commit-title", "commit_title"), + ("hook-key-subject", "subject"), + ("hook-key-squash-message", "squash_commit_message"), + ] { + let output = unicode_call( + name, + "mcp__forge__merge", + &serde_json::json!({ key: DISGUISED }), + ); + refused_as_lookalike(&output); + let reason = reason(&output); + assert!( + reason.contains(&format!("mcp__forge__merge {key}")), + "{key}: {reason}" + ); + } +} + +/// A pull request's `title` is the subject a squash merge makes of it. +#[test] +fn a_title_argument_is_judged_as_a_subject() { + let output = unicode_call( + "hook-title", + "mcp__github__create_pull_request", + &serde_json::json!({"title": DISGUISED, "body": "An ordinary body."}), + ); + refused_as_lookalike(&output); +} + +/// A whole-message argument is a subject on its first line and prose below. +#[test] +fn a_merge_message_argument_is_a_subject_then_a_body() { + let refused = unicode_call( + "hook-merge-message-subject", + "mcp__gitlab__merge_merge_request", + &serde_json::json!({"squash_commit_message": format!("{DISGUISED}\n\nAn ordinary body.")}), + ); + refused_as_lookalike(&refused); + + let passed = unicode_call( + "hook-merge-message-body", + "mcp__gitlab__merge_merge_request", + &serde_json::json!({"squash_commit_message": format!("Fix the parser\n\n{DISGUISED}")}), + ); + assert_eq!(code(&passed), 0, "{}", stderr(&passed)); + assert_eq!(stdout(&passed), "", "{}", stdout(&passed)); +} + +/// The same word in a body is prose, where a lookalike letter is not a hazard. +#[test] +fn the_same_word_in_a_body_is_prose() { + let output = unicode_call( + "hook-body-prose", + "mcp__github__create_pull_request", + &serde_json::json!({"title": "Fix the parser", "body": DISGUISED}), + ); + assert_eq!(code(&output), 0, "{}", stderr(&output)); + assert_eq!(stdout(&output), "", "{}", stdout(&output)); +} + +/// A key not on the list stays prose: the list can add a check, never remove +/// one, and a key it lacks is judged the way every string was before it. +#[test] +fn an_unknown_key_stays_prose() { + let output = unicode_call( + "hook-unknown-key-prose", + "mcp__forge__publish", + &serde_json::json!({"headline_text": DISGUISED}), + ); + assert_eq!(code(&output), 0, "{}", stderr(&output)); + assert_eq!(stdout(&output), "", "{}", stdout(&output)); +} + +/// A character that draws nothing is refused under any key, title or not. +#[test] +fn an_invisible_character_is_refused_under_any_key() { + for (name, key) in [ + ("hook-invisible-body", "body"), + ("hook-invisible-title", "title"), + ("hook-invisible-unknown", "headline_text"), + ] { + let output = unicode_call( + name, + "mcp__github__create_pull_request", + &serde_json::json!({ key: "Fix the\u{202E}parser" }), + ); + assert_eq!(code(&output), 0, "{}", stderr(&output)); + let reason = reason(&output); + assert!( + reason.contains("prevent-unusual-unicode"), + "{key}: {reason}" + ); + assert!(reason.contains("U+202E"), "{key}: {reason}"); + } +} diff --git a/tests/shim_cli.rs b/tests/shim_cli.rs index bc9cb92..1e0bf1b 100644 --- a/tests/shim_cli.rs +++ b/tests/shim_cli.rs @@ -2653,6 +2653,9 @@ fn a_title_set_through_the_api_is_judged_as_the_title_it_is() { )); let title = format!("title={LOOKALIKE}"); let commit_title = format!("commit_title={LOOKALIKE}"); + // `subject` is the key `--subject` spells, and the list the harness hook + // reads a tool call's arguments by is this one. + let subject = format!("subject={LOOKALIKE}"); for form in [ vec![ "gh", @@ -2672,6 +2675,15 @@ fn a_title_set_through_the_api_is_judged_as_the_title_it_is() { "-f", &commit_title, ], + vec![ + "gh", + "api", + "-X", + "PUT", + "repos/o/r/pulls/1/merge", + "-f", + &subject, + ], ] { let output = shim(&root, &form); assert_eq!(code(&output), 1, "{form:?}: {}", stderr(&output));