From ead6b741ce45db01b52d1bb7f703715f564ae103 Mon Sep 17 00:00:00 2001 From: HackingGate Date: Fri, 9 Oct 2026 23:37:57 +0900 Subject: [PATCH 1/2] The harness hook judges a title argument as a title, by the keys the shim reads api fields by A tool call's strings were joined into one prose text, so an MCP merge's commit_title or a create-pull-request title got the invisible-character check alone, where the same text through gh pr merge --subject or gh api -f commit_title= also got the lookalike pass. The hook now notes the nearest key of each string. One under title, commit_title or subject is handed to the guards as a headline; one under squash_commit_message or merge_commit_message is a headline on its first line and prose below, as at the shim. Every string is still read whatever its key, and every checker other than the guards still reads them joined. The key list is the shim's API_TITLE_KEYS and API_MESSAGE_KEYS, shared rather than copied, with subject added. It is a closed list of which check a string gets, not a table of fields to read: a key it lacks leaves that string prose, which is how every string was judged before, so it can add a check and never remove one. Claude-Session: https://claude.ai/code/session_01QJkWXa2HxJ6q9TMNAGSNv4 --- docs/REFERENCE.md | 21 +++++++-- src/hook.rs | 96 ++++++++++++++++++++++++++++--------- src/main.rs | 2 +- src/shim.rs | 27 +++++++---- src/text.rs | 82 +++++++++++++++++++++++++------- tests/hook_cli.rs | 118 ++++++++++++++++++++++++++++++++++++++++++++++ tests/shim_cli.rs | 12 +++++ 7 files changed, 305 insertions(+), 53 deletions(-) diff --git a/docs/REFERENCE.md b/docs/REFERENCE.md index 4e760b5..49b7f6f 100644 --- a/docs/REFERENCE.md +++ b/docs/REFERENCE.md @@ -1487,9 +1487,12 @@ 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. 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 +2161,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 +2532,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..4ba798f 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,66 @@ 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(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(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. +#[derive(Default)] +struct Collected { + prose: Vec, + headlines: Vec, +} + /// Put the report inside the harness's refusal document. /// /// Through `serde_json` rather than by formatting a string, because a report @@ -235,17 +265,25 @@ pub(crate) fn run(harness: &str, found: Option<&(PathBuf, PathBuf)>) -> Result = collected + .headlines + .into_iter() + .map(|headline| text::Headline { + label: format!("{label} title"), + 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 +343,26 @@ 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!["a"]); + } + + /// 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!["m", "s", "t"]); + 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..fd29f50 100644 --- a/tests/hook_cli.rs +++ b/tests/hook_cli.rs @@ -613,3 +613,121 @@ 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 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)); From 11af71926c36e5f45c16fa3a1baffe588a6b83ab Mon Sep 17 00:00:00 2001 From: HackingGate Date: Fri, 9 Oct 2026 23:49:50 +0900 Subject: [PATCH 2/2] A hook finding names the key its subject was found under A headline the harness hook judged was labelled " title" whatever key carried it, so a lookalike in commit_title, subject or squash_commit_message was reported under a field the call never had. Each headline now keeps its key, and the finding says " commit_title". The reference adds that the keys are read in every tool the hook's matcher sends it, 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. Claude-Session: https://claude.ai/code/session_01QJkWXa2HxJ6q9TMNAGSNv4 --- docs/REFERENCE.md | 5 ++++- src/hook.rs | 27 +++++++++++++++++++-------- tests/hook_cli.rs | 24 ++++++++++++++++++++++++ 3 files changed, 47 insertions(+), 9 deletions(-) diff --git a/docs/REFERENCE.md b/docs/REFERENCE.md index 49b7f6f..23d44eb 100644 --- a/docs/REFERENCE.md +++ b/docs/REFERENCE.md @@ -1490,7 +1490,10 @@ the rest, prose. An `--input` JSON document is prose in every value, `title` 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. A pull-request, +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 diff --git a/src/hook.rs b/src/hook.rs index 4ba798f..6b826b6 100644 --- a/src/hook.rs +++ b/src/hook.rs @@ -118,11 +118,13 @@ pub(crate) fn known() -> String { fn strings(value: &Value, key: Option<&str>, into: &mut Collected) { match value { Value::String(text) => match key { - Some(key) if API_TITLE_KEYS.contains(&key) => into.headlines.push(text.clone()), + 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(subject.value); + into.headlines.push((key.to_owned(), subject.value)); } else { into.prose.push(subject.value); } @@ -144,11 +146,13 @@ fn strings(value: &Value, key: Option<&str>, into: &mut Collected) { } } -/// A tool call's strings, sorted by the check each gets. +/// 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, + headlines: Vec<(String, String)>, } /// Put the report inside the harness's refusal document. @@ -275,8 +279,8 @@ pub(crate) fn run(harness: &str, found: Option<&(PathBuf, PathBuf)>) -> Result = collected .headlines .into_iter() - .map(|headline| text::Headline { - label: format!("{label} title"), + .map(|(key, headline)| text::Headline { + label: format!("{label} {key}"), text: headline, }) .collect(); @@ -347,7 +351,7 @@ mod tests { strings(&event, None, &mut found); found.prose.sort(); assert_eq!(found.prose, vec!["b", "c"]); - assert_eq!(found.headlines, vec!["a"]); + 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 @@ -361,7 +365,14 @@ mod tests { let mut found = Collected::default(); strings(&event, None, &mut found); found.headlines.sort(); - assert_eq!(found.headlines, vec!["m", "s", "t"]); + 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"]); } diff --git a/tests/hook_cli.rs b/tests/hook_cli.rs index fd29f50..9f51e56 100644 --- a/tests/hook_cli.rs +++ b/tests/hook_cli.rs @@ -654,6 +654,30 @@ fn a_commit_title_argument_is_judged_as_a_subject() { 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() {