From dc78ae5fe95d7887ab0c2092c8b3a87c60bec1de Mon Sep 17 00:00:00 2001 From: Lohit Kolluri Date: Sun, 2 Aug 2026 23:44:39 +0530 Subject: [PATCH 1/2] feat: learn from review thread resolve/unresolve MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Handle pull_request_review_thread webhook: resolving a thread on a Codasaurus finding comment dismisses the fingerprint repo-wide (same ACL and learning path as a 👎 reaction); unresolving re-enables it via a new un_dismiss_fingerprint store method. Adds the event to the App manifest default_events. Signed-off-by: Lohit Kolluri --- CHANGELOG.md | 1 + src/api/setup.rs | 1 + src/bot/commands.rs | 1 + src/bot/mod.rs | 134 ++++++++++++++++++++++++++++++++++++++++++ src/bot/threads.rs | 102 ++++++++++++++++++++++++++++++++ src/bot/worker.rs | 2 + src/learning/store.rs | 8 +++ 7 files changed, 249 insertions(+) create mode 100644 src/bot/threads.rs diff --git a/CHANGELOG.md b/CHANGELOG.md index 95c2e9e..796f121 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -42,6 +42,7 @@ Dates are UTC calendar days. Links at the bottom compare tags on GitHub. - Go ecosystem support: `hallucinated_imports` and `phantom_deps` now verify imports and `go.mod` declarations against proxy.golang.org. - New Tier-1 detector `lockfile_drift`: flags dependencies declared in `package.json`, `Cargo.toml`, or `go.mod` that are missing from their lockfile (`package-lock.json`, `Cargo.lock`, `go.sum`). - New Tier-1 detector `license_drift`: flags declared npm / PyPI / crates.io dependencies that carry copyleft-style licenses. +- Resolving a review thread on a Codasaurus finding comment now dismisses that fingerprint repo-wide (same ACL and learning path as a 👎 reaction); unresolving the thread re-enables the finding. - `@codasaurus retry` command re-runs the latest review for a PR. - ARM64 Linux release binary (`aarch64-unknown-linux-gnu`) in the release workflow. - CLI `codasaurus reset-password --email … --password …` for emergency local dashboard recovery (no email flow). diff --git a/src/api/setup.rs b/src/api/setup.rs index 3067999..3754809 100644 --- a/src/api/setup.rs +++ b/src/api/setup.rs @@ -540,6 +540,7 @@ fn build_manifest(public_url: &str) -> serde_json::Value { "pull_request", "issue_comment", "reaction", + "pull_request_review_thread", "installation", "installation_repositories" ] diff --git a/src/bot/commands.rs b/src/bot/commands.rs index 855b056..508809b 100644 --- a/src/bot/commands.rs +++ b/src/bot/commands.rs @@ -1034,6 +1034,7 @@ async fn spawn_review(ctx: WebhookContext, pr_number: i64, timeout_secs: u64) { comment: None, issue: None, reaction: None, + thread: None, sender: None, repositories: None, repositories_added: None, diff --git a/src/bot/mod.rs b/src/bot/mod.rs index 9ea37f5..88b8ed4 100644 --- a/src/bot/mod.rs +++ b/src/bot/mod.rs @@ -27,6 +27,7 @@ mod reactions; pub(crate) mod related_prs; pub(crate) mod repo_context; mod review; +mod threads; pub(crate) use review::{github_api_headers, next_github_link, GITHUB_CLIENT}; pub(crate) mod strictness; pub(crate) mod title_fix; @@ -204,6 +205,8 @@ pub(crate) struct WebhookPayload { issue: Option, /// `reaction` webhook event payload reaction: Option, + /// `pull_request_review_thread` webhook event payload + thread: Option, /// Actor who triggered the event (reactions, etc.) sender: Option, /// Sent in `installation.created` event @@ -366,6 +369,54 @@ fn reactor_can_dismiss(payload: &WebhookPayload) -> bool { reactor.eq_ignore_ascii_case(pr_author) } +/// Who may dismiss findings by resolving a review thread (same trust bar as reactions). +fn thread_resolver_can_dismiss(payload: &WebhookPayload) -> bool { + let assoc = payload + .thread + .as_ref() + .and_then(|t| t.pointer("/comments/0/author_association")) + .and_then(|a| a.as_str()) + .unwrap_or(""); + if matches!(assoc, "OWNER" | "MEMBER" | "COLLABORATOR" | "CONTRIBUTOR") { + return true; + } + + let resolver = payload + .thread + .as_ref() + .and_then(|t| t.pointer("/comments/0/user/login")) + .and_then(|v| v.as_str()) + .or_else(|| { + payload + .sender + .as_ref() + .and_then(|s| s.get("login")) + .and_then(|v| v.as_str()) + }) + .unwrap_or(""); + if resolver.is_empty() { + return false; + } + + let repo_owner = payload + .repo + .as_ref() + .and_then(|r| r.pointer("/owner/login")) + .and_then(|v| v.as_str()) + .unwrap_or(""); + if resolver.eq_ignore_ascii_case(repo_owner) { + return true; + } + + let pr_author = payload + .pull_request + .as_ref() + .and_then(|pr| pr.pointer("/user/login")) + .and_then(|v| v.as_str()) + .unwrap_or(""); + resolver.eq_ignore_ascii_case(pr_author) +} + pub(crate) async fn handle_webhook( headers: axum::http::HeaderMap, body: axum::body::Bytes, @@ -613,6 +664,36 @@ pub(crate) async fn handle_webhook( } }); } + } else if event == "pull_request_review_thread" + && matches!(payload.action.as_str(), "resolved" | "unresolved") + { + let repo_full_name = payload + .repo + .as_ref() + .and_then(|r| r["full_name"].as_str()) + .unwrap_or("") + .to_string(); + if !repo_full_name.is_empty() { + if let Some(thread) = payload.thread.clone() { + let action = payload.action.clone(); + let allowed = thread_resolver_can_dismiss(&payload); + tokio::spawn(async move { + if let Some(pool) = bot_db_pool() { + if let Err(e) = threads::handle_thread_event( + pool, + &action, + &thread, + &repo_full_name, + allowed, + ) + .await + { + tracing::warn!(error = %e, "review thread learning failed"); + } + } + }); + } + } } else if event == "installation" && payload.action == "created" { tokio::spawn(handle_installation_created( payload.installation, @@ -853,6 +934,7 @@ mod author_acl_tests { pull_request: None, installation: None, reaction: None, + thread: None, sender: None, repositories: None, repositories_added: None, @@ -905,4 +987,56 @@ mod author_acl_tests { fn rejects_unrelated_none() { assert!(!author_can_command(&payload("NONE", "eve", "alice", "bob"))); } + + fn thread_payload( + assoc: &str, + resolver: &str, + repo_owner: &str, + pr_author: &str, + ) -> WebhookPayload { + let mut p = payload(assoc, resolver, repo_owner, pr_author); + p.thread = Some(serde_json::json!({ + "node_id": "PRRT_x", + "comments": [{ + "author_association": assoc, + "user": { "login": resolver }, + "body": "**Secrets** · `blocking`\n`fingerprint: abcdef012345`" + }] + })); + p.pull_request = Some(serde_json::json!({ + "user": { "login": pr_author } + })); + p + } + + #[test] + fn thread_owner_association_allows_dismiss() { + assert!(thread_resolver_can_dismiss(&thread_payload( + "OWNER", "alice", "alice", "bob" + ))); + } + + #[test] + fn thread_repo_owner_login_allows_even_if_none() { + assert!(thread_resolver_can_dismiss(&thread_payload( + "NONE", "alice", "alice", "carol" + ))); + } + + #[test] + fn thread_pr_author_allows() { + assert!(thread_resolver_can_dismiss(&thread_payload( + "FIRST_TIME_CONTRIBUTOR", + "bob", + "org", + "bob" + ))); + } + + #[test] + fn thread_unrelated_none_rejected() { + assert!(!thread_resolver_can_dismiss(&thread_payload( + "NONE", "eve", "alice", "bob" + ))); + } } diff --git a/src/bot/threads.rs b/src/bot/threads.rs new file mode 100644 index 0000000..0bad923 --- /dev/null +++ b/src/bot/threads.rs @@ -0,0 +1,102 @@ +use crate::bot::reactions::fingerprint_from_comment_body; +use crate::learning::store::LearningStore; + +pub fn fingerprint_from_thread(thread: &serde_json::Value) -> Option { + let comments = thread.get("comments")?.as_array()?; + for comment in comments { + if let Some(body) = comment.get("body").and_then(|b| b.as_str()) { + if let Some(fp) = fingerprint_from_comment_body(body) { + return Some(fp); + } + } + } + None +} + +pub async fn handle_thread_event( + pool: &crate::db::DbPool, + action: &str, + thread: &serde_json::Value, + repo_full_name: &str, + resolver_allowed: bool, +) -> anyhow::Result { + let Some(fp) = fingerprint_from_thread(thread) else { + tracing::debug!("review thread ignored: no fingerprint in thread comments"); + return Ok(false); + }; + let store = LearningStore::from_pool(pool); + match action { + "resolved" => { + if !resolver_allowed { + tracing::info!( + repo = %repo_full_name, + fingerprint = %fp, + "resolve ignored: resolver lacks command ACL" + ); + return Ok(false); + } + store + .dismiss_fingerprint_for_repo( + &fp, + "resolve", + repo_full_name, + "dismissed via resolved review thread", + Some(repo_full_name), + None, + None, + true, + ) + .await?; + tracing::info!( + repo = %repo_full_name, + fingerprint = %fp, + "learned dismissal from resolved thread" + ); + Ok(true) + } + "unresolved" => { + let removed = store.un_dismiss_fingerprint(&fp).await?; + tracing::info!( + repo = %repo_full_name, + fingerprint = %fp, + removed, + "un-dismissed finding from unresolved thread" + ); + Ok(removed) + } + _ => Ok(false), + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn extracts_fp_from_thread_comment() { + let thread = serde_json::json!({ + "node_id": "PRRT_kwDONx", + "comments": [{ + "body": "**Secrets** · `blocking`\n\n---\n`fingerprint: abcdef012345` · `@codasaurus ignore abcdef012345`", + "author_association": "OWNER" + }] + }); + assert_eq!( + fingerprint_from_thread(&thread).as_deref(), + Some("abcdef012345") + ); + } + + #[test] + fn no_fingerprint_returns_none() { + let thread = serde_json::json!({ + "comments": [{"body": "just a question", "author_association": "MEMBER"}] + }); + assert!(fingerprint_from_thread(&thread).is_none()); + } + + #[test] + fn missing_comments_returns_none() { + assert!(fingerprint_from_thread(&serde_json::json!({"node_id": "x"})).is_none()); + } +} diff --git a/src/bot/worker.rs b/src/bot/worker.rs index 2b4c384..1253819 100644 --- a/src/bot/worker.rs +++ b/src/bot/worker.rs @@ -147,6 +147,7 @@ async fn process_queued_review( comment: None, issue: None, reaction: None, + thread: None, sender: None, repositories: None, repositories_added: None, @@ -223,6 +224,7 @@ pub(crate) async fn run_webhook_review_inline( comment: None, issue: None, reaction: None, + thread: None, sender: None, repositories: None, repositories_added: None, diff --git a/src/learning/store.rs b/src/learning/store.rs index 5846082..7f97983 100644 --- a/src/learning/store.rs +++ b/src/learning/store.rs @@ -145,6 +145,14 @@ impl LearningStore { Ok(()) } + pub async fn un_dismiss_fingerprint(&self, fingerprint: &str) -> Result { + Ok(db_execute!( + &self.pool, + "DELETE FROM dismissed_findings WHERE fingerprint = ?", + fingerprint + )? > 0) + } + pub async fn count_dismissals_for_detector(&self, detector: &str) -> Result { Ok(db_scalar!( &self.pool, From e5ee8d030ba1b907ebab4039daafc438890fae91 Mon Sep 17 00:00:00 2001 From: Lohit Kolluri Date: Sun, 2 Aug 2026 23:53:43 +0530 Subject: [PATCH 2/2] refactor: dedupe dismissal ACL checks Collapse reactor_can_dismiss and thread_resolver_can_dismiss onto a shared actor_can_dismiss core plus repo_owner_login/pr_author_login extractors. Same behavior, ~50 lines less. Signed-off-by: Lohit Kolluri --- src/bot/mod.rs | 101 +++++++++++++++++++++---------------------------- 1 file changed, 43 insertions(+), 58 deletions(-) diff --git a/src/bot/mod.rs b/src/bot/mod.rs index 88b8ed4..d0bac79 100644 --- a/src/bot/mod.rs +++ b/src/bot/mod.rs @@ -313,20 +313,13 @@ fn author_can_command(payload: &WebhookPayload) -> bool { /// Who may dismiss findings via emoji reactions (same trust bar as slash commands). fn reactor_can_dismiss(payload: &WebhookPayload) -> bool { - let reaction_assoc = payload + let assoc = payload .reaction .as_ref() .and_then(|r| r.get("author_association")) .and_then(|a| a.as_str()) .unwrap_or(""); - if matches!( - reaction_assoc, - "OWNER" | "MEMBER" | "COLLABORATOR" | "CONTRIBUTOR" - ) { - return true; - } - - let reactor = payload + let login = payload .reaction .as_ref() .and_then(|r| r.pointer("/user/login")) @@ -337,36 +330,13 @@ fn reactor_can_dismiss(payload: &WebhookPayload) -> bool { .as_ref() .and_then(|s| s.get("login")) .and_then(|v| v.as_str()) - }) - .unwrap_or(""); - if reactor.is_empty() { - return false; - } - - let repo_owner = payload - .repo - .as_ref() - .and_then(|r| r.pointer("/owner/login")) - .and_then(|v| v.as_str()) - .unwrap_or(""); - if reactor.eq_ignore_ascii_case(repo_owner) { - return true; - } - - let pr_author = payload - .issue - .as_ref() - .and_then(|i| i.pointer("/user/login")) - .and_then(|v| v.as_str()) - .or_else(|| { - payload - .pull_request - .as_ref() - .and_then(|pr| pr.pointer("/user/login")) - .and_then(|v| v.as_str()) - }) - .unwrap_or(""); - reactor.eq_ignore_ascii_case(pr_author) + }); + actor_can_dismiss( + assoc, + login, + repo_owner_login(payload), + pr_author_login(payload), + ) } /// Who may dismiss findings by resolving a review thread (same trust bar as reactions). @@ -377,11 +347,7 @@ fn thread_resolver_can_dismiss(payload: &WebhookPayload) -> bool { .and_then(|t| t.pointer("/comments/0/author_association")) .and_then(|a| a.as_str()) .unwrap_or(""); - if matches!(assoc, "OWNER" | "MEMBER" | "COLLABORATOR" | "CONTRIBUTOR") { - return true; - } - - let resolver = payload + let login = payload .thread .as_ref() .and_then(|t| t.pointer("/comments/0/user/login")) @@ -392,29 +358,48 @@ fn thread_resolver_can_dismiss(payload: &WebhookPayload) -> bool { .as_ref() .and_then(|s| s.get("login")) .and_then(|v| v.as_str()) - }) - .unwrap_or(""); - if resolver.is_empty() { - return false; + }); + actor_can_dismiss( + assoc, + login, + repo_owner_login(payload), + pr_author_login(payload), + ) +} + +fn actor_can_dismiss(assoc: &str, login: Option<&str>, repo_owner: &str, pr_author: &str) -> bool { + if matches!(assoc, "OWNER" | "MEMBER" | "COLLABORATOR" | "CONTRIBUTOR") { + return true; } + let Some(login) = login.filter(|l| !l.is_empty()) else { + return false; + }; + login.eq_ignore_ascii_case(repo_owner) || login.eq_ignore_ascii_case(pr_author) +} - let repo_owner = payload +fn repo_owner_login(payload: &WebhookPayload) -> &str { + payload .repo .as_ref() .and_then(|r| r.pointer("/owner/login")) .and_then(|v| v.as_str()) - .unwrap_or(""); - if resolver.eq_ignore_ascii_case(repo_owner) { - return true; - } + .unwrap_or("") +} - let pr_author = payload - .pull_request +fn pr_author_login(payload: &WebhookPayload) -> &str { + payload + .issue .as_ref() - .and_then(|pr| pr.pointer("/user/login")) + .and_then(|i| i.pointer("/user/login")) .and_then(|v| v.as_str()) - .unwrap_or(""); - resolver.eq_ignore_ascii_case(pr_author) + .or_else(|| { + payload + .pull_request + .as_ref() + .and_then(|pr| pr.pointer("/user/login")) + .and_then(|v| v.as_str()) + }) + .unwrap_or("") } pub(crate) async fn handle_webhook(