From 403f9344ddff97b76a362981a4f8d16049ca7360 Mon Sep 17 00:00:00 2001 From: BryanFRD Date: Sat, 31 Oct 2026 11:07:24 +0100 Subject: [PATCH] fix(preview): only update preview comments written by the token's own identity Signed-off-by: BryanFRD --- src/forge/gitea/api_tests.rs | 40 +++++++++++++---- src/forge/github/api_tests.rs | 84 ++++++++++++++++++++++++++++++----- src/forge/gitlab.rs | 46 +++++++++++++++---- src/forge/gitlab/api_tests.rs | 45 ++++++++++++++++++- src/forge/rest.rs | 75 ++++++++++++++++++++++++++++--- 5 files changed, 254 insertions(+), 36 deletions(-) diff --git a/src/forge/gitea/api_tests.rs b/src/forge/gitea/api_tests.rs index 6adcefb9..1011af20 100644 --- a/src/forge/gitea/api_tests.rs +++ b/src/forge/gitea/api_tests.rs @@ -111,22 +111,44 @@ fn publish_release_patches_draft_false() { } #[test] -fn find_comment_matches_the_marker_on_the_issue_thread() { - let server = FakeServer::start(vec![Reply::json( - 200, - json!([ - { "id": 1, "body": "hello" }, - { "id": 2, "body": " preview" }, - ]), - )]); +fn find_comment_only_matches_the_tokens_own_comments() { + let server = FakeServer::start(vec![ + Reply::json( + 200, + json!([ + { "id": 1, "body": "hello", "user": { "login": "me" } }, + { "id": 2, "body": " preview", "user": { "login": "me" } }, + { "id": 3, "body": " quoted", "user": { "login": "someone" } }, + ]), + ), + Reply::json(200, json!({ "login": "me" })), + ]); let found = forge(&server).find_comment(8, "").unwrap(); assert_eq!(found, Some(2)); + let requests = server.requests(); assert_eq!( - server.only_request().path, + requests[0].path, "/repos/owner/repo/issues/8/comments?limit=50&page=1" ); + assert_eq!(requests[1].path, "/user"); +} + +#[test] +fn find_comment_matches_nothing_when_the_token_cannot_read_its_user() { + let server = FakeServer::start(vec![ + Reply::json( + 200, + json!([{ "id": 2, "body": "", "user": { "login": "me", "type": "Bot" } }]), + ), + Reply::json(403, json!({})), + ]); + + assert_eq!( + forge(&server).find_comment(8, "").unwrap(), + None + ); } #[test] diff --git a/src/forge/github/api_tests.rs b/src/forge/github/api_tests.rs index 3130a194..5f485546 100644 --- a/src/forge/github/api_tests.rs +++ b/src/forge/github/api_tests.rs @@ -284,24 +284,86 @@ fn a_transport_failure_on_auto_merge_uses_the_request_code() { assert_eq!(code(&err).as_deref(), Some("E3008")); } +fn marked_comment(id: u64, login: &str, kind: &str) -> serde_json::Value { + json!({ + "id": id, + "body": "\npreview", + "user": { "login": login, "type": kind }, + }) +} + #[test] -fn find_comment_returns_the_first_comment_carrying_the_marker() { - let server = FakeServer::start(vec![Reply::json( - 200, - json!([ - { "id": 1, "body": "looks good" }, - { "id": 2, "body": "\npreview" }, - { "id": 3, "body": " again" }, - ]), - )]); +fn find_comment_skips_a_foreign_comment_and_prefers_the_most_recent_own_one() { + let server = FakeServer::start(vec![ + Reply::json( + 200, + json!([ + { "id": 1, "body": "looks good", "user": { "login": "me", "type": "User" } }, + marked_comment(2, "me", "User"), + marked_comment(3, "me", "User"), + marked_comment(4, "someone", "User"), + ]), + ), + Reply::json(200, json!({ "login": "me" })), + ]); let found = forge(&server).find_comment(5, "").unwrap(); - assert_eq!(found, Some(2)); + assert_eq!(found, Some(3)); + let requests = server.requests(); assert_eq!( - server.only_request().path, + requests[0].path, "/repos/owner/repo/issues/5/comments?per_page=100&page=1" ); + assert_eq!(requests[1].path, "/user"); +} + +#[test] +fn find_comment_is_none_when_only_someone_else_carries_the_marker() { + let server = FakeServer::start(vec![ + Reply::json(200, json!([marked_comment(4, "someone", "User")])), + Reply::json(200, json!({ "login": "me" })), + ]); + + assert_eq!( + forge(&server).find_comment(5, "").unwrap(), + None + ); +} + +#[test] +fn find_comment_with_an_integration_token_only_matches_bots() { + let server = FakeServer::start(vec![ + Reply::json( + 200, + json!([ + marked_comment(2, "github-actions[bot]", "Bot"), + marked_comment(4, "someone", "User"), + ]), + ), + Reply::json( + 403, + json!({ "message": "Resource not accessible by integration" }), + ), + ]); + + let found = forge(&server).find_comment(5, "").unwrap(); + + assert_eq!(found, Some(2)); +} + +#[test] +fn find_comment_propagates_a_server_error_from_the_identity_lookup() { + let server = FakeServer::start(vec![ + Reply::json(200, json!([marked_comment(2, "me", "User")])), + Reply::json(500, json!({})), + ]); + + let err = forge(&server) + .find_comment(5, "") + .unwrap_err(); + + assert!(format!("{err:#}").contains("token's user"), "{err:#}"); } #[test] diff --git a/src/forge/gitlab.rs b/src/forge/gitlab.rs index db1f37df..83e158b0 100644 --- a/src/forge/gitlab.rs +++ b/src/forge/gitlab.rs @@ -64,6 +64,29 @@ impl GitLabForge { what, ) } + + fn token_username(&self) -> Result> { + let response = self + .agent + .get(format!("{}/user", self.api_base)) + .header(self.token_kind.header(), &self.token) + .header("User-Agent", "ferrflow") + .call(); + match response { + Ok(mut response) => { + let user: serde_json::Value = response + .body_mut() + .read_json() + .context("Failed to parse the token's user")?; + let username = user["username"] + .as_str() + .context("The token's user has no username")?; + Ok(Some(username.to_string())) + } + Err(ureq::Error::StatusCode(401 | 403)) => Ok(None), + Err(e) => Err(e).context("Failed to read the token's user"), + } + } } impl Forge for GitLabForge { @@ -227,15 +250,22 @@ impl Forge for GitLabForge { mr_id ); let notes = self.paginated_json_array(&base_url, "MR notes")?; - for note in notes { - if let Some(body) = note["body"].as_str() - && body.contains(marker) - && let Some(id) = note["id"].as_u64() - { - return Ok(Some(id)); - } + let mut marked = notes + .iter() + .filter(|note| note["body"].as_str().is_some_and(|b| b.contains(marker))) + .peekable(); + if marked.peek().is_none() { + return Ok(None); } - Ok(None) + let Some(username) = self.token_username()? else { + tracing::warn!( + "Warning: the token cannot read its own user, posting a new note instead of updating one" + ); + return Ok(None); + }; + Ok(marked + .find(|note| note["author"]["username"].as_str() == Some(username.as_str())) + .and_then(|note| note["id"].as_u64())) } fn create_comment(&self, mr_id: u64, body: &str) -> Result<()> { diff --git a/src/forge/gitlab/api_tests.rs b/src/forge/gitlab/api_tests.rs index 53fbe977..41d6ee5c 100644 --- a/src/forge/gitlab/api_tests.rs +++ b/src/forge/gitlab/api_tests.rs @@ -197,7 +197,11 @@ fn find_comment_pages_through_notes_until_the_marker() { .collect(); let server = FakeServer::start(vec![ Reply::json(200, json!(first_page)), - Reply::json(200, json!([{ "id": 500, "body": "x y" }])), + Reply::json( + 200, + json!([{ "id": 500, "body": "x y", "author": { "username": "me" } }]), + ), + Reply::json(200, json!({ "username": "me" })), ]); let found = forge(&server) @@ -212,6 +216,45 @@ fn find_comment_pages_through_notes_until_the_marker() { ); } +#[test] +fn find_comment_skips_notes_from_someone_else() { + let server = FakeServer::start(vec![ + Reply::json( + 200, + json!([ + { "id": 9, "body": "", "author": { "username": "someone" } }, + { "id": 8, "body": "", "author": { "username": "me" } }, + { "id": 7, "body": "", "author": { "username": "me" } }, + ]), + ), + Reply::json(200, json!({ "username": "me" })), + ]); + + let found = forge(&server) + .find_comment(15, "") + .unwrap(); + + assert_eq!(found, Some(8)); +} + +#[test] +fn find_comment_matches_nothing_when_the_token_cannot_read_its_user() { + let server = FakeServer::start(vec![ + Reply::json( + 200, + json!([{ "id": 8, "body": "", "author": { "username": "me" } }]), + ), + Reply::json(401, json!({})), + ]); + + assert_eq!( + forge(&server) + .find_comment(15, "") + .unwrap(), + None + ); +} + #[test] fn a_malformed_notes_page_is_an_error() { let server = FakeServer::start(vec![Reply::raw(200, "")]); diff --git a/src/forge/rest.rs b/src/forge/rest.rs index e09debe5..82101f22 100644 --- a/src/forge/rest.rs +++ b/src/forge/rest.rs @@ -6,9 +6,33 @@ use super::ReleaseResult; const MAX_PAGES: u32 = 100; +#[derive(Clone, Copy)] +enum Unidentified { + AnyBot, + Nobody, +} + +enum CommentOwner { + Login(String), + Unidentified(Unidentified), +} + +impl CommentOwner { + fn wrote(&self, comment: &serde_json::Value) -> bool { + let author = &comment["user"]; + match self { + Self::Login(login) => author["login"].as_str() == Some(login), + Self::Unidentified(Unidentified::AnyBot) => author["type"].as_str() == Some("Bot"), + Self::Unidentified(Unidentified::Nobody) => false, + } + } +} + pub(super) struct RestClient<'a> { agent: &'a ureq::Agent, repo_url: String, + user_url: String, + unidentified: Unidentified, auth: (&'static str, String), extra_headers: &'static [(&'static str, &'static str)], page_size_param: &'static str, @@ -20,6 +44,8 @@ impl<'a> RestClient<'a> { Self { agent, repo_url: format!("{api_base}/repos/{slug}"), + user_url: format!("{api_base}/user"), + unidentified: Unidentified::AnyBot, auth: ("Authorization", format!("Bearer {token}")), extra_headers: &[ ("Accept", "application/vnd.github+json"), @@ -34,6 +60,8 @@ impl<'a> RestClient<'a> { Self { agent, repo_url: format!("{api_base}/repos/{slug}"), + user_url: format!("{api_base}/user"), + unidentified: Unidentified::Nobody, auth: ("Authorization", format!("token {token}")), extra_headers: &[], page_size_param: "limit", @@ -119,13 +147,46 @@ impl<'a> RestClient<'a> { pub fn find_comment(&self, pr_id: u64, marker: &str, what: &str) -> Result> { let comments = self.paginated_json_array(&format!("{}/issues/{pr_id}/comments", self.repo_url), what)?; - Ok(comments.iter().find_map(|comment| { - comment["body"] - .as_str() - .is_some_and(|body| body.contains(marker)) - .then(|| comment["id"].as_u64()) - .flatten() - })) + let mut marked = comments + .iter() + .rev() + .filter(|comment| { + comment["body"] + .as_str() + .is_some_and(|body| body.contains(marker)) + }) + .peekable(); + if marked.peek().is_none() { + return Ok(None); + } + let owner = self.comment_owner()?; + Ok(marked + .find(|comment| owner.wrote(comment)) + .and_then(|comment| comment["id"].as_u64())) + } + + fn comment_owner(&self) -> Result { + match self.get(&self.user_url).call() { + Ok(mut response) => { + let user: serde_json::Value = response + .body_mut() + .read_json() + .context("Failed to parse the token's user")?; + let login = user["login"] + .as_str() + .context("The token's user has no login")?; + Ok(CommentOwner::Login(login.to_string())) + } + Err(ureq::Error::StatusCode(401 | 403)) => { + if matches!(self.unidentified, Unidentified::Nobody) { + tracing::warn!( + "Warning: the token cannot read its own user, posting a new comment instead of updating one" + ); + } + Ok(CommentOwner::Unidentified(self.unidentified)) + } + Err(e) => Err(e).context("Failed to read the token's user"), + } } pub fn create_comment(&self, pr_id: u64, body: &str) -> Result<()> {