From 0e418d368e7e053d973257ab3a019a10d4d1165c Mon Sep 17 00:00:00 2001 From: Nas <36938454+Na5co@users.noreply.github.com> Date: Wed, 23 Sep 2026 20:23:29 +0300 Subject: [PATCH] check: a finding about a file sits on line 1, so a review can carry it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A review can only comment on a line. A `locale` warning — and a `syntax` error whose parser gave no position — had no line, so the reviewer dropped it: the bot reported a pull request that adds an unlisted locale as clean, which is exactly the silence the locale check was added to end. Found by asking the reviewer what it would post before pushing a pull request at the live bot. Co-Authored-By: Claude Opus 5 --- CHANGELOG.md | 2 ++ src/check/formats/syntax.rs | 9 ++++++--- tests/check_syntax.rs | 36 ++++++++++++++++++++++++++++++++++++ 3 files changed, 44 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 367229a..b9d43ba 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,8 @@ All notable changes to polygo. Versions follow [SemVer](https://semver.org); dat ## Unreleased +- A finding about a whole file (`locale`, and a `syntax` error whose parser gave no position) now sits on line 1 instead of nowhere. A review can only comment on a line, so those findings never reached the pull request that introduced them: the bot reported a pull request adding an unlisted locale as clean, which is the failure the `locale` code exists to end. + - `locale` (warning): a locale file sitting next to the ones being checked whose locale is not in `target_locales` — a translator's whole language, silently unchecked. Found the same way as the `syntax` gap: a test fixture added `locales/it.json`, forgot to list `it`, and the reviewer reported a clean pull request. Only real language tags count, so a `README.json` or an Android `values-night` is not mistaken for a locale, and a file that does not parse keeps its `syntax` error instead of being reported twice. - `polygo-bot` runs on Cloud Run, where its always-free tier covers roughly 18,000 pull requests a month: `bot/deploy-cloud-run.sh` takes a project and the App's key and does the rest (APIs, Secret Manager, Cloud Build, deploy), then prints the webhook URL to paste into the App. `POLYGO_BOT_SYNC=1` reviews before answering GitHub, since Cloud Run stops a container's CPU once the response goes out and a background thread would be frozen mid-review. diff --git a/src/check/formats/syntax.rs b/src/check/formats/syntax.rs index 2aec8c6..178863e 100644 --- a/src/check/formats/syntax.rs +++ b/src/check/formats/syntax.rs @@ -18,6 +18,9 @@ pub struct Broken { pub idx: usize, /// Path relative to the checked root, as findings name files. pub file: String, + /// Where to put it. A finding about the file rather than a line in it goes on line 1: + /// a reviewer can only comment on a line, so a finding without one never reaches the + /// pull request that introduced it. pub line: Option, /// The locale the path stands for, or the source locale for a source file. pub locale: String, @@ -112,7 +115,7 @@ pub fn unlisted(cx: &mut crate::check::report::Cx, idx: usize) { cx.emit_at( idx, file.clone(), - None, + Some(1), &file, &locale, Code::Locale, @@ -141,7 +144,7 @@ fn parse_error(path: &Path, format: Format) -> Option<(Option, String)> { }; let e = parsed.err()?; let message = one_line(&format!("{e:#}")); - Some((line_of(&message, &text), message)) + Some((Some(line_of(&message, &text).unwrap_or(1)), message)) } /// Where the fix goes. Parsers say either "at line 584 column 2" or "at byte 34886"; a @@ -244,6 +247,6 @@ mod tests { assert_eq!(line_of("expected ',' or '}' at byte 5", text), Some(3)); assert_eq!(line_of("expected ',' at byte 0", text), Some(1)); assert_eq!(line_of("beyond the end at byte 9999", text), Some(3)); - assert_eq!(line_of("no numbers here", text), None); + assert_eq!(line_of("no numbers here", text), None); // the caller falls back to 1 } } diff --git a/tests/check_syntax.rs b/tests/check_syntax.rs index f653f2d..70a32f1 100644 --- a/tests/check_syntax.rs +++ b/tests/check_syntax.rs @@ -232,3 +232,39 @@ fn an_android_qualifier_directory_is_not_a_locale() { "night and w600dp are qualifiers, not languages: {out}" ); } + +/// A finding about a whole file has to sit on a line, or the reviewer cannot say it: a +/// pull request that adds an unlisted locale would come back clean, which is the very +/// failure the `locale` code exists to end. +#[test] +fn a_file_level_finding_lands_on_line_one_so_a_review_can_carry_it() { + let dir = tempfile::tempdir().unwrap(); + let root = dir.path(); + project(root); + fs::write( + root.join("locales/it.json"), + "{\n \"a\": \"Ciao {{name}}\",\n \"b\": \"Arrivederci\"\n}\n", + ) + .unwrap(); + fs::write( + root.join("polygo.toml"), + "source_locale = \"en\"\ntarget_locales = [\"de\"]\n\n[[files]]\nformat = \"json\"\npath = \"locales/en.json\"\nlocale_path = \"locales/{locale}.json\"\n\n[provider]\nkind = \"mock\"\n", + ) + .unwrap(); + // The diff that adds the file, as GitHub would give it. + fs::write( + root.join("pr.diff"), + "--- a/locales/it.json\n+++ b/locales/it.json\n@@ -0,0 +1,4 @@\n+{\n+ \"a\": \"Ciao {{name}}\",\n+ \"b\": \"Arrivederci\"\n+}\n", + ) + .unwrap(); + let (_, out, _) = check(root, &["--review", "--diff", "pr.diff"]); + let v: serde_json::Value = serde_json::from_str(&out).unwrap(); + let c = &v["comments"][0]; + assert_eq!(c["path"], "locales/it.json", "{out}"); + assert_eq!(c["line"], 1); + assert!( + c["body"].as_str().unwrap().contains("Locale not checked"), + "{}", + c["body"] + ); +}