From 69a8be8c43c3bcb1b2624628d62d3f04779156f9 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sun, 12 Apr 2026 18:49:13 +0000 Subject: [PATCH 1/7] Initial plan From c5d59d62eca8d82f6d66330ff197fd2c4d3d0c67 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sun, 12 Apr 2026 19:04:27 +0000 Subject: [PATCH 2/7] feat: add reaction-based endorsement and disapproval integrity mechanics - Add endorsement_reactions, disapproval_reactions, disapproval_integrity, endorser_min_integrity fields to PolicyContext in Rust guard - Implement has_maintainer_reaction_with_callback() core evaluation function with gateway-mode graceful degradation (log once, skip safely) - Add has_maintainer_endorsement() and has_maintainer_disapproval() functions - Add apply_endorsement_promotion() and apply_disapproval_demotion() functions - Wire endorsement (step 3) and disapproval (step 4, always wins) into issue_integrity() and pr_integrity() after approval-labels promotion - Add cap_integrity() helper for disapproval demotion (min of current and cap) - Add collaborator permission caching in backend.rs (keyed owner/repo:user) - Update AllowOnlyPolicy in lib.rs to deserialize new fields - Update Go config AllowOnlyPolicy and NormalizedGuardPolicy structs - Update JSON marshal/unmarshal with new field support - Update NormalizeGuardPolicy with validation for reaction fields - Update wasm.go buildStrictLabelAgentPayload to allow new allow-only keys - Add comprehensive unit tests (Rust and Go) for all new functionality Agent-Logs-Url: https://github.com/github/gh-aw-mcpg/sessions/1cd99fee-78d0-43bd-bc83-1d39e17d9de3 Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com> --- .../rust-guard/src/labels/backend.rs | 41 ++ .../rust-guard/src/labels/helpers.rs | 496 +++++++++++++++++- .../github-guard/rust-guard/src/labels/mod.rs | 121 +++++ guards/github-guard/rust-guard/src/lib.rs | 12 + internal/config/guard_policy.go | 109 +++- internal/config/guard_policy_test.go | 127 +++++ internal/guard/wasm.go | 44 +- internal/guard/wasm_test.go | 93 ++++ 8 files changed, 1017 insertions(+), 26 deletions(-) diff --git a/guards/github-guard/rust-guard/src/labels/backend.rs b/guards/github-guard/rust-guard/src/labels/backend.rs index 83ebfb9c..77895f5b 100644 --- a/guards/github-guard/rust-guard/src/labels/backend.rs +++ b/guards/github-guard/rust-guard/src/labels/backend.rs @@ -24,6 +24,27 @@ fn repo_owner_type_cache() -> &'static Mutex> { CACHE.get_or_init(|| Mutex::new(HashMap::new())) } +/// Cache for collaborator permission lookups keyed by "owner/repo:username". +/// Caches the raw permission string so it can be reused across multiple items +/// that share the same reactor within a single gateway request. +fn collaborator_permission_cache() -> &'static Mutex>> { + static CACHE: OnceLock>>> = OnceLock::new(); + CACHE.get_or_init(|| Mutex::new(HashMap::new())) +} + +fn get_cached_collaborator_permission(key: &str) -> Option> { + collaborator_permission_cache() + .lock() + .ok() + .and_then(|cache| cache.get(key).cloned()) +} + +fn set_cached_collaborator_permission(key: &str, permission: Option) { + if let Ok(mut cache) = collaborator_permission_cache().lock() { + cache.insert(key.to_string(), permission); + } +} + fn get_cached_repo_visibility(repo_id: &str) -> Option { repo_visibility_cache() .lock() @@ -449,6 +470,9 @@ pub fn get_issue_author_info( /// to GET /repos/{owner}/{repo}/collaborators/{username}/permission. /// Returns the user's effective permission (including inherited org permissions), /// which is more accurate than author_association for org admins. +/// +/// Results are cached per `(owner, repo, username)` to avoid duplicate enrichment +/// calls when the same reactor appears on multiple items in a response collection. pub fn get_collaborator_permission_with_callback( callback: GithubMcpCallback, owner: &str, @@ -463,6 +487,20 @@ pub fn get_collaborator_permission_with_callback( return None; } + let cache_key = format!("{}/{}:{}", owner, repo, username.to_ascii_lowercase()); + + // Return cached permission if available. + if let Some(cached) = get_cached_collaborator_permission(&cache_key) { + crate::log_debug(&format!( + "get_collaborator_permission: cache hit for {}/{} user {} → permission={:?}", + owner, repo, username, cached + )); + return cached.map(|permission| CollaboratorPermission { + permission: Some(permission), + login: Some(username.to_string()), + }); + } + crate::log_debug(&format!( "get_collaborator_permission: fetching permission for {}/{} user {}", owner, repo, username @@ -484,6 +522,7 @@ pub fn get_collaborator_permission_with_callback( "get_collaborator_permission: empty response for {}/{} user {}", owner, repo, username )); + set_cached_collaborator_permission(&cache_key, None); return None; } Err(code) => { @@ -535,6 +574,8 @@ pub fn get_collaborator_permission_with_callback( owner, repo, username, permission, login )); + set_cached_collaborator_permission(&cache_key, permission.clone()); + Some(CollaboratorPermission { permission, login }) } diff --git a/guards/github-guard/rust-guard/src/labels/helpers.rs b/guards/github-guard/rust-guard/src/labels/helpers.rs index 87d10c0c..d75d1926 100644 --- a/guards/github-guard/rust-guard/src/labels/helpers.rs +++ b/guards/github-guard/rust-guard/src/labels/helpers.rs @@ -3,10 +3,16 @@ //! This module contains utility functions used across the labeling system, //! including JSON extraction, integrity determination, and common operations. +use std::sync::atomic::{AtomicBool, Ordering}; + use serde_json::Value; +use super::backend::GithubMcpCallback; use super::constants::{field_names, label_constants}; +/// Ensures the gateway-mode reaction warning is emitted at most once per process lifetime. +static REACTION_GATEWAY_WARNING_EMITTED: AtomicBool = AtomicBool::new(false); + /// Extract a resource number from a JSON item, returning the number as a string. /// Checks the `number` field first, then falls back to extracting the trailing /// number segment from `html_url` or `url` (e.g. `.../issues/123` → `123`). @@ -114,6 +120,23 @@ pub struct PolicyContext { /// their author_association. Analogous to trusted_bots but for regular human users. /// blocked_users takes precedence over trusted_users. pub trusted_users: Vec, + /// GitHub ReactionContent values (e.g. "THUMBS_UP", "HEART") that count as maintainer + /// endorsement. When a maintainer with sufficient integrity reacts with one of these, + /// the item's integrity is promoted to at least approved. Empty = feature disabled. + pub endorsement_reactions: Vec, + /// GitHub ReactionContent values (e.g. "THUMBS_DOWN", "CONFUSED") that count as + /// maintainer disapproval. When a maintainer with sufficient integrity reacts with + /// one of these, the item's integrity is capped at `disapproval_integrity`. + /// Disapproval overrides endorsement. Empty = feature disabled. + pub disapproval_reactions: Vec, + /// The integrity level to cap an item at when a maintainer disapproval reaction is + /// detected. Defaults to "none" when empty. Options: "none", "unapproved", + /// "approved", "merged". + pub disapproval_integrity: String, + /// Minimum integrity level that a reactor must have for their reaction to count as + /// endorsement or disapproval. Defaults to "approved" when empty. Options: + /// "none", "unapproved", "approved", "merged". + pub endorser_min_integrity: String, } fn normalize_scope(scope: &str, ctx: &PolicyContext) -> String { @@ -322,6 +345,248 @@ fn apply_approval_label_promotion( } } +// ============================================================================ +// Reaction-based endorsement and disapproval helpers +// ============================================================================ + +/// Maximum number of reactions to inspect per item. Caps API enrichment calls. +const MAX_REACTIONS_TO_CHECK: usize = 20; + +/// Return the effective `disapproval_integrity` level from context, defaulting to "none". +fn effective_disapproval_integrity<'a>(ctx: &'a PolicyContext) -> &'a str { + let v = ctx.disapproval_integrity.trim(); + if v.is_empty() { "none" } else { v } +} + +/// Return the effective `endorser_min_integrity` level from context, defaulting to "approved". +fn effective_endorser_min_integrity<'a>(ctx: &'a PolicyContext) -> &'a str { + let v = ctx.endorser_min_integrity.trim(); + if v.is_empty() { "approved" } else { v } +} + +/// Convert an integrity level name to its rank (0 = unknown, 1 = none, 2 = reader, 3 = writer, 4 = merged). +fn integrity_level_rank(level: &str) -> u8 { + match level.trim().to_ascii_lowercase().as_str() { + "none" => 1, + "unapproved" => 2, + "approved" => 3, + "merged" => 4, + _ => 3, // unknown → default to approved + } +} + +/// Cap integrity at the given level. Returns `min(current, cap)` using the integrity hierarchy. +fn cap_integrity( + scope: &str, + current: Vec, + cap: Vec, + ctx: &PolicyContext, +) -> Vec { + let current_rank = integrity_rank(scope, ¤t, ctx); + let cap_rank = integrity_rank(scope, &cap, ctx); + integrity_for_rank(scope, current_rank.min(cap_rank), ctx) +} + +/// Build the integrity `Vec` for a given level name over a scope. +fn integrity_for_level(level: &str, scope: &str, ctx: &PolicyContext) -> Vec { + match level.trim().to_ascii_lowercase().as_str() { + "none" => none_integrity(scope, ctx), + "unapproved" => reader_integrity(scope, ctx), + "approved" => writer_integrity(scope, ctx), + "merged" => merged_integrity(scope, ctx), + _ => none_integrity(scope, ctx), // safe default + } +} + +/// Core reaction evaluation helper. +/// +/// Returns `true` if any reaction in `reaction_list` on the item was made by a +/// user whose collaborator permission meets or exceeds `endorser_min_integrity`. +/// +/// - Uses `callback` to invoke `get_collaborator_permission` for each qualifying reactor. +/// - Inspects at most `MAX_REACTIONS_TO_CHECK` reactions to bound API call count. +/// - When `reactions` data is present but contains no per-user nodes (gateway mode), +/// emits a warning at most once per process lifetime and returns `false`. +pub fn has_maintainer_reaction_with_callback( + item: &Value, + repo_full_name: &str, + reaction_list: &[String], + endorser_min: &str, + ctx: &PolicyContext, + callback: GithubMcpCallback, + reaction_kind: &str, // "endorsement" or "disapproval" — used for log messages +) -> bool { + if reaction_list.is_empty() { + return false; + } + + let (owner, repo) = match repo_full_name.split_once('/') { + Some((o, r)) if !o.is_empty() && !r.is_empty() => (o, r), + _ => return false, + }; + + // Try to get per-user reaction nodes. + let nodes = item + .get("reactions") + .and_then(|r| r.get("nodes")) + .and_then(|n| n.as_array()); + + let nodes = match nodes { + Some(n) => n, + None => { + // If a `reactions` field is present but has no `nodes` array, we are in + // gateway mode: reaction counts are available but reactor identity is not. + if item.get("reactions").is_some() { + if !REACTION_GATEWAY_WARNING_EMITTED.swap(true, Ordering::Relaxed) { + crate::log_warn(&format!( + "[integrity] {}: {}-reactions configured but reactor identity unavailable \ + (gateway mode) — ignoring reactions for integrity evaluation", + repo_full_name, reaction_kind + )); + } + } + return false; + } + }; + + let endorser_min_rank = integrity_level_rank(endorser_min); + + for node in nodes.iter().take(MAX_REACTIONS_TO_CHECK) { + let content = match node.get("content").and_then(|v| v.as_str()) { + Some(c) => c, + None => continue, + }; + + // Check if this reaction type is in our configured list (case-insensitive). + if !reaction_list.iter().any(|r| r.eq_ignore_ascii_case(content)) { + continue; + } + + // Retrieve the reactor's login. + let login = match node + .get("user") + .and_then(|u| u.get("login")) + .and_then(|v| v.as_str()) + .filter(|l| !l.is_empty()) + { + Some(l) => l, + None => continue, + }; + + // Fetch reactor's collaborator permission to determine their integrity level. + let perm = super::backend::get_collaborator_permission_with_callback( + callback, owner, repo, login, + ); + let reactor_integrity = collaborator_permission_floor( + repo_full_name, + perm.as_ref().and_then(|p| p.permission.as_deref()), + ctx, + ); + + let reactor_rank = integrity_rank(repo_full_name, &reactor_integrity, ctx); + + if reactor_rank >= endorser_min_rank { + crate::log_debug(&format!( + "[integrity] {}: reactor @{} has permission={:?}, integrity rank {} >= \ + endorser-min-integrity rank {} — counting as {} reaction {}", + repo_full_name, + login, + perm.as_ref().and_then(|p| p.permission.as_deref()), + reactor_rank, + endorser_min_rank, + reaction_kind, + content + )); + return true; + } else { + crate::log_info(&format!( + "[integrity] {}: reactor @{} has integrity rank {}, below \ + endorser-min-integrity rank {} — ignoring {} {}", + repo_full_name, login, reactor_rank, endorser_min_rank, reaction_kind, content + )); + } + } + + false +} + +/// Returns `true` if the item has a qualifying maintainer endorsement reaction. +/// +/// Uses the production backend callback. Respects `PolicyContext.endorsement_reactions` +/// and `PolicyContext.endorser_min_integrity`. +pub fn has_maintainer_endorsement(item: &Value, repo_full_name: &str, ctx: &PolicyContext) -> bool { + has_maintainer_reaction_with_callback( + item, + repo_full_name, + &ctx.endorsement_reactions, + effective_endorser_min_integrity(ctx), + ctx, + crate::invoke_backend, + "endorsement", + ) +} + +/// Returns `true` if the item has a qualifying maintainer disapproval reaction. +/// +/// Uses the production backend callback. Respects `PolicyContext.disapproval_reactions` +/// and `PolicyContext.endorser_min_integrity`. +pub fn has_maintainer_disapproval(item: &Value, repo_full_name: &str, ctx: &PolicyContext) -> bool { + has_maintainer_reaction_with_callback( + item, + repo_full_name, + &ctx.disapproval_reactions, + effective_endorser_min_integrity(ctx), + ctx, + crate::invoke_backend, + "disapproval", + ) +} + +/// Apply endorsement promotion: if a qualified maintainer has reacted with an endorsement +/// reaction, raise integrity to at least writer (approved) level. +fn apply_endorsement_promotion( + item: &Value, + resource_type: &str, + repo_full_name: &str, + integrity: Vec, + ctx: &PolicyContext, +) -> Vec { + if has_maintainer_endorsement(item, repo_full_name, ctx) { + let number = item.get(field_names::NUMBER).and_then(|v| v.as_u64()).unwrap_or(0); + crate::log_info(&format!( + "[integrity] {}:{}#{} promoted to approved (maintainer endorsement reaction)", + resource_type, repo_full_name, number + )); + max_integrity(repo_full_name, integrity, writer_integrity(repo_full_name, ctx), ctx) + } else { + integrity + } +} + +/// Apply disapproval demotion: if a qualified maintainer has reacted with a disapproval +/// reaction, cap the item's integrity at the configured `disapproval_integrity` level. +/// Disapproval overrides endorsement and approval labels (runs last in the chain). +fn apply_disapproval_demotion( + item: &Value, + resource_type: &str, + repo_full_name: &str, + integrity: Vec, + ctx: &PolicyContext, +) -> Vec { + if has_maintainer_disapproval(item, repo_full_name, ctx) { + let number = item.get(field_names::NUMBER).and_then(|v| v.as_u64()).unwrap_or(0); + let demote_level = effective_disapproval_integrity(ctx); + crate::log_info(&format!( + "[integrity] {}:{}#{} demoted to {} (maintainer disapproval reaction)", + resource_type, repo_full_name, number, demote_level + )); + let cap = integrity_for_level(demote_level, repo_full_name, ctx); + cap_integrity(repo_full_name, integrity, cap, ctx) + } else { + integrity + } +} + pub fn ensure_integrity_baseline( scope: &str, integrity: Vec, @@ -1155,7 +1420,14 @@ pub fn pr_integrity( let integrity = ensure_integrity_baseline(repo_full_name, integrity, ctx); // Step 2: Apply approval-labels promotion — raise to at least approved. - apply_approval_label_promotion(item, "pr", repo_full_name, integrity, ctx) + let integrity = apply_approval_label_promotion(item, "pr", repo_full_name, integrity, ctx); + // Step 3: Apply endorsement promotion — raise to at least approved if a qualified + // maintainer reacted with a configured endorsement reaction. + let integrity = apply_endorsement_promotion(item, "pr", repo_full_name, integrity, ctx); + // Step 4: Apply disapproval demotion — cap at configured level if a qualified + // maintainer reacted with a configured disapproval reaction. + // Disapproval runs last so it always wins over all promotion rules. + apply_disapproval_demotion(item, "pr", repo_full_name, integrity, ctx) } /// Determine integrity level for an issue @@ -1232,7 +1504,14 @@ pub fn issue_integrity( let integrity = ensure_integrity_baseline(repo_full_name, integrity, ctx); // Step 2: Apply approval-labels promotion — raise to at least approved. - apply_approval_label_promotion(item, "issue", repo_full_name, integrity, ctx) + let integrity = apply_approval_label_promotion(item, "issue", repo_full_name, integrity, ctx); + // Step 3: Apply endorsement promotion — raise to at least approved if a qualified + // maintainer reacted with a configured endorsement reaction. + let integrity = apply_endorsement_promotion(item, "issue", repo_full_name, integrity, ctx); + // Step 4: Apply disapproval demotion — cap at configured level if a qualified + // maintainer reacted with a configured disapproval reaction. + // Disapproval runs last so it always wins over all promotion rules. + apply_disapproval_demotion(item, "issue", repo_full_name, integrity, ctx) } /// Determine integrity level for a commit. @@ -1340,13 +1619,7 @@ mod tests { use super::*; fn test_ctx() -> PolicyContext { - PolicyContext { - scopes: vec![], - blocked_users: vec![], - trusted_bots: vec![], - trusted_users: vec![], - approval_labels: vec![], - } + PolicyContext::default() } #[test] @@ -1448,4 +1721,209 @@ mod tests { assert_eq!(MinIntegrity::Approved.as_str(), policy_integrity::APPROVED); assert_eq!(MinIntegrity::Merged.as_str(), policy_integrity::MERGED); } + + // ========================================================================= + // Tests for reaction-based endorsement / disapproval helpers + // ========================================================================= + + fn ctx_with_endorsement_reactions(reactions: Vec<&str>) -> PolicyContext { + PolicyContext { + endorsement_reactions: reactions.into_iter().map(|s| s.to_string()).collect(), + ..Default::default() + } + } + + fn ctx_with_disapproval_reactions(reactions: Vec<&str>) -> PolicyContext { + PolicyContext { + disapproval_reactions: reactions.into_iter().map(|s| s.to_string()).collect(), + ..Default::default() + } + } + + /// Mock callback that returns admin permission for any user. + fn admin_permission_callback(_tool: &str, _args: &str, buf: &mut [u8]) -> Result { + let response = r#"{"permission":"admin","user":{"login":"maintainer"}}"#; + let bytes = response.as_bytes(); + let len = bytes.len().min(buf.len()); + buf[..len].copy_from_slice(&bytes[..len]); + Ok(len) + } + + /// Mock callback that returns read (low) permission for any user. + fn read_permission_callback(_tool: &str, _args: &str, buf: &mut [u8]) -> Result { + let response = r#"{"permission":"read","user":{"login":"external"}}"#; + let bytes = response.as_bytes(); + let len = bytes.len().min(buf.len()); + buf[..len].copy_from_slice(&bytes[..len]); + Ok(len) + } + + /// Mock callback that returns an error (simulates unavailable backend). + fn error_callback(_tool: &str, _args: &str, _buf: &mut [u8]) -> Result { + Err(-1) + } + + #[test] + fn test_has_maintainer_reaction_no_reactions_in_ctx() { + let ctx = PolicyContext::default(); + // endorsement_reactions is empty — should always return false + let item = serde_json::json!({ + "number": 1, + "reactions": {"nodes": [{"user": {"login": "alice"}, "content": "THUMBS_UP"}]} + }); + assert!(!has_maintainer_reaction_with_callback( + &item, "owner/repo", &ctx.endorsement_reactions, "approved", &ctx, + admin_permission_callback, "endorsement" + )); + } + + #[test] + fn test_has_maintainer_reaction_with_matching_admin_reactor() { + let ctx = ctx_with_endorsement_reactions(vec!["THUMBS_UP"]); + let item = serde_json::json!({ + "number": 42, + "reactions": {"nodes": [{"user": {"login": "alice"}, "content": "THUMBS_UP"}]} + }); + assert!(has_maintainer_reaction_with_callback( + &item, "owner/repo", &ctx.endorsement_reactions, "approved", &ctx, + admin_permission_callback, "endorsement" + )); + } + + #[test] + fn test_has_maintainer_reaction_reactor_below_threshold() { + let ctx = ctx_with_endorsement_reactions(vec!["THUMBS_UP"]); + let item = serde_json::json!({ + "number": 42, + "reactions": {"nodes": [{"user": {"login": "external"}, "content": "THUMBS_UP"}]} + }); + // read permission → unapproved integrity, below "approved" threshold + assert!(!has_maintainer_reaction_with_callback( + &item, "owner/repo", &ctx.endorsement_reactions, "approved", &ctx, + read_permission_callback, "endorsement" + )); + } + + #[test] + fn test_has_maintainer_reaction_wrong_content() { + let ctx = ctx_with_endorsement_reactions(vec!["HEART"]); + let item = serde_json::json!({ + "number": 42, + "reactions": {"nodes": [{"user": {"login": "alice"}, "content": "THUMBS_UP"}]} + }); + // Reaction content "THUMBS_UP" is not in endorsement list ["HEART"] + assert!(!has_maintainer_reaction_with_callback( + &item, "owner/repo", &ctx.endorsement_reactions, "approved", &ctx, + admin_permission_callback, "endorsement" + )); + } + + #[test] + fn test_has_maintainer_reaction_case_insensitive_content() { + let ctx = ctx_with_endorsement_reactions(vec!["thumbs_up"]); + let item = serde_json::json!({ + "number": 42, + "reactions": {"nodes": [{"user": {"login": "alice"}, "content": "THUMBS_UP"}]} + }); + // Case-insensitive match between "thumbs_up" (config) and "THUMBS_UP" (data) + assert!(has_maintainer_reaction_with_callback( + &item, "owner/repo", &ctx.endorsement_reactions, "approved", &ctx, + admin_permission_callback, "endorsement" + )); + } + + #[test] + fn test_has_maintainer_reaction_no_nodes_gateway_mode() { + // reactions field present but no nodes array (gateway mode — only counts available) + let ctx = ctx_with_endorsement_reactions(vec!["THUMBS_UP"]); + let item = serde_json::json!({ + "number": 42, + "reactions": {"total_count": 3, "thumbs_up": 3, "+1": 3} + }); + // Should return false (graceful degradation) + assert!(!has_maintainer_reaction_with_callback( + &item, "owner/repo", &ctx.endorsement_reactions, "approved", &ctx, + admin_permission_callback, "endorsement" + )); + } + + #[test] + fn test_has_maintainer_reaction_no_reactions_field() { + let ctx = ctx_with_endorsement_reactions(vec!["THUMBS_UP"]); + let item = serde_json::json!({"number": 42, "title": "no reactions"}); + // No reactions field → skip silently + assert!(!has_maintainer_reaction_with_callback( + &item, "owner/repo", &ctx.endorsement_reactions, "approved", &ctx, + admin_permission_callback, "endorsement" + )); + } + + #[test] + fn test_has_maintainer_reaction_empty_nodes() { + let ctx = ctx_with_endorsement_reactions(vec!["THUMBS_UP"]); + let item = serde_json::json!({"number": 42, "reactions": {"nodes": []}}); + assert!(!has_maintainer_reaction_with_callback( + &item, "owner/repo", &ctx.endorsement_reactions, "approved", &ctx, + admin_permission_callback, "endorsement" + )); + } + + #[test] + fn test_has_maintainer_reaction_backend_error_skips() { + let ctx = ctx_with_endorsement_reactions(vec!["THUMBS_UP"]); + let item = serde_json::json!({ + "number": 42, + "reactions": {"nodes": [{"user": {"login": "alice"}, "content": "THUMBS_UP"}]} + }); + // Backend error → can't confirm permission → should not count as endorsement + assert!(!has_maintainer_reaction_with_callback( + &item, "owner/repo", &ctx.endorsement_reactions, "approved", &ctx, + error_callback, "endorsement" + )); + } + + #[test] + fn test_cap_integrity_at_none() { + let ctx = test_ctx(); + let scope = "owner/repo"; + let current = writer_integrity(scope, &ctx); + let cap = none_integrity(scope, &ctx); + let result = cap_integrity(scope, current, cap, &ctx); + assert_eq!(result, none_integrity(scope, &ctx), "capping approved at none should give none"); + } + + #[test] + fn test_cap_integrity_cap_higher_than_current() { + let ctx = test_ctx(); + let scope = "owner/repo"; + let current = reader_integrity(scope, &ctx); + let cap = writer_integrity(scope, &ctx); + // cap > current → should stay at current (min(reader, writer) = reader) + let result = cap_integrity(scope, current.clone(), cap, &ctx); + assert_eq!(result, current, "cap higher than current should not change integrity"); + } + + #[test] + fn test_integrity_for_level_mapping() { + let ctx = test_ctx(); + let scope = "owner/repo"; + assert_eq!(integrity_for_level("none", scope, &ctx), none_integrity(scope, &ctx)); + assert_eq!(integrity_for_level("unapproved", scope, &ctx), reader_integrity(scope, &ctx)); + assert_eq!(integrity_for_level("approved", scope, &ctx), writer_integrity(scope, &ctx)); + assert_eq!(integrity_for_level("merged", scope, &ctx), merged_integrity(scope, &ctx)); + // Unknown should default to none (safe) + assert_eq!(integrity_for_level("unknown", scope, &ctx), none_integrity(scope, &ctx)); + } + + #[test] + fn test_effective_disapproval_integrity_defaults_to_none() { + let ctx = PolicyContext::default(); + assert_eq!(effective_disapproval_integrity(&ctx), "none"); + } + + #[test] + fn test_effective_endorser_min_integrity_defaults_to_approved() { + let ctx = PolicyContext::default(); + assert_eq!(effective_endorser_min_integrity(&ctx), "approved"); + } } diff --git a/guards/github-guard/rust-guard/src/labels/mod.rs b/guards/github-guard/rust-guard/src/labels/mod.rs index 4f01cc82..88d38cc7 100644 --- a/guards/github-guard/rust-guard/src/labels/mod.rs +++ b/guards/github-guard/rust-guard/src/labels/mod.rs @@ -4817,4 +4817,125 @@ mod tests { ); } } + + // ========================================================================= + // Tests for reaction-based endorsement / disapproval wired into integrity + // ========================================================================= + + fn ctx_with_endorsement_reactions(reactions: Vec<&str>) -> PolicyContext { + PolicyContext { + endorsement_reactions: reactions.into_iter().map(|s| s.to_string()).collect(), + ..Default::default() + } + } + + fn ctx_with_disapproval_reactions(reactions: Vec<&str>, demote_to: &str) -> PolicyContext { + PolicyContext { + disapproval_reactions: reactions.into_iter().map(|s| s.to_string()).collect(), + disapproval_integrity: demote_to.to_string(), + ..Default::default() + } + } + + #[test] + fn test_issue_integrity_no_reactions_field_endorsement_configured() { + // Item has no reactions field at all — endorsement_reactions configured but irrelevant. + // Integrity should be unchanged from base rules. + let repo = "github/copilot"; + let ctx = ctx_with_endorsement_reactions(vec!["THUMBS_UP"]); + let issue = json!({ + "user": {"login": "external"}, + "author_association": "NONE", + "number": 10 + }); + // No reactions → base integrity (none for external contributor on public repo) + assert_eq!( + issue_integrity(&issue, repo, false, &ctx), + helpers::none_integrity(repo, &ctx) + ); + } + + #[test] + fn test_issue_integrity_gateway_mode_reactions_only_counts() { + // Item has a reactions object but only count fields — gateway mode. + // Should degrade gracefully (no promotion, no demotion). + let repo = "github/copilot"; + let ctx = ctx_with_endorsement_reactions(vec!["THUMBS_UP"]); + let issue = json!({ + "user": {"login": "external"}, + "author_association": "NONE", + "number": 11, + "reactions": {"total_count": 5, "thumbs_up": 5, "+1": 5} + }); + // Gateway mode → fall through to base integrity (none) + assert_eq!( + issue_integrity(&issue, repo, false, &ctx), + helpers::none_integrity(repo, &ctx) + ); + } + + #[test] + fn test_pr_integrity_no_reactions_field_disapproval_configured() { + // Item has no reactions field — disapproval_reactions configured but irrelevant. + // A merged PR should stay merged. + let repo = "github/copilot"; + let ctx = ctx_with_disapproval_reactions(vec!["THUMBS_DOWN"], "none"); + let merged_pr = json!({ + "user": {"login": "external"}, + "author_association": "NONE", + "merged_at": "2024-01-15T10:00:00Z", + "number": 20 + }); + // No reactions → merged stays merged + assert_eq!( + pr_integrity(&merged_pr, repo, false, Some(false), &ctx), + helpers::merged_integrity(repo, &ctx) + ); + } + + #[test] + fn test_issue_integrity_empty_reaction_nodes_no_change() { + // reactions.nodes is an empty array — no endorsement or disapproval possible. + let repo = "github/copilot"; + let ctx = PolicyContext { + endorsement_reactions: vec!["THUMBS_UP".to_string()], + disapproval_reactions: vec!["THUMBS_DOWN".to_string()], + ..Default::default() + }; + let issue = json!({ + "user": {"login": "external"}, + "author_association": "NONE", + "number": 30, + "reactions": {"nodes": []} + }); + // Empty nodes → no change from base integrity + assert_eq!( + issue_integrity(&issue, repo, false, &ctx), + helpers::none_integrity(repo, &ctx) + ); + } + + #[test] + fn test_blocked_user_not_promoted_by_endorsement() { + // A blocked user's item must not be promoted even when endorsement reactions are present + // and per-user reaction data is available (blocked_users takes highest precedence). + let repo = "github/copilot"; + let ctx = PolicyContext { + blocked_users: vec!["evil-bot".to_string()], + endorsement_reactions: vec!["THUMBS_UP".to_string()], + ..Default::default() + }; + let issue = json!({ + "user": {"login": "evil-bot"}, + "author_association": "NONE", + "number": 99, + "reactions": {"nodes": [{"user": {"login": "maintainer"}, "content": "THUMBS_UP"}]} + }); + // blocked_users takes absolute precedence — even with an endorsement reaction node present. + // (Backend call won't occur in unit tests since invoke_backend returns None.) + assert_eq!( + issue_integrity(&issue, repo, false, &ctx), + helpers::blocked_integrity(repo, &ctx) + ); + } } diff --git a/guards/github-guard/rust-guard/src/lib.rs b/guards/github-guard/rust-guard/src/lib.rs index d8e7e4eb..8e7960f9 100644 --- a/guards/github-guard/rust-guard/src/lib.rs +++ b/guards/github-guard/rust-guard/src/lib.rs @@ -276,6 +276,14 @@ struct AllowOnlyPolicy { approval_labels: Vec, #[serde(rename = "trusted-users", default)] trusted_users: Vec, + #[serde(rename = "endorsement-reactions", default)] + endorsement_reactions: Vec, + #[serde(rename = "disapproval-reactions", default)] + disapproval_reactions: Vec, + #[serde(rename = "disapproval-integrity", default)] + disapproval_integrity: String, + #[serde(rename = "endorser-min-integrity", default)] + endorser_min_integrity: String, } #[derive(Debug, Deserialize)] @@ -502,6 +510,10 @@ pub extern "C" fn label_agent( blocked_users: policy.blocked_users, approval_labels: policy.approval_labels, trusted_users: policy.trusted_users, + endorsement_reactions: policy.endorsement_reactions, + disapproval_reactions: policy.disapproval_reactions, + disapproval_integrity: policy.disapproval_integrity, + endorser_min_integrity: policy.endorser_min_integrity, }; set_runtime_policy_context(ctx.clone()); diff --git a/internal/config/guard_policy.go b/internal/config/guard_policy.go index bc1b2599..57824697 100644 --- a/internal/config/guard_policy.go +++ b/internal/config/guard_policy.go @@ -39,21 +39,29 @@ type WriteSinkPolicy struct { // AllowOnlyPolicy configures scope and minimum required integrity. type AllowOnlyPolicy struct { - Repos interface{} `toml:"Repos" json:"repos"` - MinIntegrity string `toml:"MinIntegrity" json:"min-integrity"` - BlockedUsers []string `toml:"BlockedUsers" json:"blocked-users,omitempty"` - ApprovalLabels []string `toml:"ApprovalLabels" json:"approval-labels,omitempty"` - TrustedUsers []string `toml:"TrustedUsers" json:"trusted-users,omitempty"` + Repos interface{} `toml:"Repos" json:"repos"` + MinIntegrity string `toml:"MinIntegrity" json:"min-integrity"` + BlockedUsers []string `toml:"BlockedUsers" json:"blocked-users,omitempty"` + ApprovalLabels []string `toml:"ApprovalLabels" json:"approval-labels,omitempty"` + TrustedUsers []string `toml:"TrustedUsers" json:"trusted-users,omitempty"` + EndorsementReactions []string `toml:"EndorsementReactions" json:"endorsement-reactions,omitempty"` + DisapprovalReactions []string `toml:"DisapprovalReactions" json:"disapproval-reactions,omitempty"` + DisapprovalIntegrity string `toml:"DisapprovalIntegrity" json:"disapproval-integrity,omitempty"` + EndorserMinIntegrity string `toml:"EndorserMinIntegrity" json:"endorser-min-integrity,omitempty"` } // NormalizedGuardPolicy is a canonical policy representation for caching and observability. type NormalizedGuardPolicy struct { - ScopeKind string `json:"scope_kind"` - ScopeValues []string `json:"scope_values,omitempty"` - MinIntegrity string `json:"min-integrity"` - BlockedUsers []string `json:"blocked-users,omitempty"` - ApprovalLabels []string `json:"approval-labels,omitempty"` - TrustedUsers []string `json:"trusted-users,omitempty"` + ScopeKind string `json:"scope_kind"` + ScopeValues []string `json:"scope_values,omitempty"` + MinIntegrity string `json:"min-integrity"` + BlockedUsers []string `json:"blocked-users,omitempty"` + ApprovalLabels []string `json:"approval-labels,omitempty"` + TrustedUsers []string `json:"trusted-users,omitempty"` + EndorsementReactions []string `json:"endorsement-reactions,omitempty"` + DisapprovalReactions []string `json:"disapproval-reactions,omitempty"` + DisapprovalIntegrity string `json:"disapproval-integrity,omitempty"` + EndorserMinIntegrity string `json:"endorser-min-integrity,omitempty"` } func (p *GuardPolicy) UnmarshalJSON(data []byte) error { @@ -138,6 +146,22 @@ func (p *AllowOnlyPolicy) UnmarshalJSON(data []byte) error { if err := json.Unmarshal(value, &p.TrustedUsers); err != nil { return fmt.Errorf("invalid allow-only.trusted-users: %w", err) } + case "endorsement-reactions": + if err := json.Unmarshal(value, &p.EndorsementReactions); err != nil { + return fmt.Errorf("invalid allow-only.endorsement-reactions: %w", err) + } + case "disapproval-reactions": + if err := json.Unmarshal(value, &p.DisapprovalReactions); err != nil { + return fmt.Errorf("invalid allow-only.disapproval-reactions: %w", err) + } + case "disapproval-integrity": + if err := json.Unmarshal(value, &p.DisapprovalIntegrity); err != nil { + return fmt.Errorf("invalid allow-only.disapproval-integrity: %w", err) + } + case "endorser-min-integrity": + if err := json.Unmarshal(value, &p.EndorserMinIntegrity); err != nil { + return fmt.Errorf("invalid allow-only.endorser-min-integrity: %w", err) + } default: return fmt.Errorf("allow-only contains unsupported field %q", key) } @@ -155,11 +179,15 @@ func (p *AllowOnlyPolicy) UnmarshalJSON(data []byte) error { func (p AllowOnlyPolicy) MarshalJSON() ([]byte, error) { type serializedAllowOnly struct { - Repos interface{} `json:"repos"` - MinIntegrity string `json:"min-integrity"` - BlockedUsers []string `json:"blocked-users,omitempty"` - ApprovalLabels []string `json:"approval-labels,omitempty"` - TrustedUsers []string `json:"trusted-users,omitempty"` + Repos interface{} `json:"repos"` + MinIntegrity string `json:"min-integrity"` + BlockedUsers []string `json:"blocked-users,omitempty"` + ApprovalLabels []string `json:"approval-labels,omitempty"` + TrustedUsers []string `json:"trusted-users,omitempty"` + EndorsementReactions []string `json:"endorsement-reactions,omitempty"` + DisapprovalReactions []string `json:"disapproval-reactions,omitempty"` + DisapprovalIntegrity string `json:"disapproval-integrity,omitempty"` + EndorserMinIntegrity string `json:"endorser-min-integrity,omitempty"` } return json.Marshal(serializedAllowOnly(p)) @@ -355,6 +383,55 @@ func NormalizeGuardPolicy(policy *GuardPolicy) (*NormalizedGuardPolicy, error) { } } + // Validate and normalize endorsement-reactions. + // Dedup uses uppercased keys to match the GraphQL ReactionContent enum. + if len(policy.AllowOnly.EndorsementReactions) > 0 { + seen := make(map[string]struct{}, len(policy.AllowOnly.EndorsementReactions)) + for _, r := range policy.AllowOnly.EndorsementReactions { + r = strings.TrimSpace(r) + if r == "" { + return nil, fmt.Errorf("allow-only.endorsement-reactions entries must not be empty") + } + key := strings.ToUpper(r) + if _, exists := seen[key]; !exists { + seen[key] = struct{}{} + normalized.EndorsementReactions = append(normalized.EndorsementReactions, key) + } + } + } + + // Validate and normalize disapproval-reactions. + if len(policy.AllowOnly.DisapprovalReactions) > 0 { + seen := make(map[string]struct{}, len(policy.AllowOnly.DisapprovalReactions)) + for _, r := range policy.AllowOnly.DisapprovalReactions { + r = strings.TrimSpace(r) + if r == "" { + return nil, fmt.Errorf("allow-only.disapproval-reactions entries must not be empty") + } + key := strings.ToUpper(r) + if _, exists := seen[key]; !exists { + seen[key] = struct{}{} + normalized.DisapprovalReactions = append(normalized.DisapprovalReactions, key) + } + } + } + + // Validate and normalize disapproval-integrity (optional, defaults to "none"). + if v := strings.ToLower(strings.TrimSpace(policy.AllowOnly.DisapprovalIntegrity)); v != "" { + if _, ok := validMinIntegrityValues[v]; !ok { + return nil, fmt.Errorf("allow-only.disapproval-integrity must be one of: none, unapproved, approved, merged") + } + normalized.DisapprovalIntegrity = v + } + + // Validate and normalize endorser-min-integrity (optional, defaults to "approved"). + if v := strings.ToLower(strings.TrimSpace(policy.AllowOnly.EndorserMinIntegrity)); v != "" { + if _, ok := validMinIntegrityValues[v]; !ok { + return nil, fmt.Errorf("allow-only.endorser-min-integrity must be one of: none, unapproved, approved, merged") + } + normalized.EndorserMinIntegrity = v + } + switch scope := policy.AllowOnly.Repos.(type) { case string: scopeValue := strings.ToLower(strings.TrimSpace(scope)) diff --git a/internal/config/guard_policy_test.go b/internal/config/guard_policy_test.go index 71140993..6c22a80c 100644 --- a/internal/config/guard_policy_test.go +++ b/internal/config/guard_policy_test.go @@ -990,3 +990,130 @@ func TestIsScopeTokenChar(t *testing.T) { assert.False(t, isScopeTokenChar(c), "expected isScopeTokenChar(%q) == false", c) } } + +// TestNormalizeGuardPolicyReactionEndorsement tests the new reaction-based endorsement fields. +func TestNormalizeGuardPolicyReactionEndorsement(t *testing.T) { + t.Run("endorsement-reactions propagated and normalized to uppercase", func(t *testing.T) { + policy := &GuardPolicy{AllowOnly: &AllowOnlyPolicy{ + Repos: "public", + MinIntegrity: "approved", + EndorsementReactions: []string{"thumbs_up", "HEART"}, + }} + got, err := NormalizeGuardPolicy(policy) + require.NoError(t, err) + assert.Equal(t, []string{"THUMBS_UP", "HEART"}, got.EndorsementReactions) + }) + + t.Run("disapproval-reactions propagated and normalized to uppercase", func(t *testing.T) { + policy := &GuardPolicy{AllowOnly: &AllowOnlyPolicy{ + Repos: "public", + MinIntegrity: "approved", + DisapprovalReactions: []string{"thumbs_down", "Confused"}, + }} + got, err := NormalizeGuardPolicy(policy) + require.NoError(t, err) + assert.Equal(t, []string{"THUMBS_DOWN", "CONFUSED"}, got.DisapprovalReactions) + }) + + t.Run("disapproval-integrity validated and propagated", func(t *testing.T) { + policy := &GuardPolicy{AllowOnly: &AllowOnlyPolicy{ + Repos: "public", + MinIntegrity: "approved", + DisapprovalReactions: []string{"THUMBS_DOWN"}, + DisapprovalIntegrity: "none", + }} + got, err := NormalizeGuardPolicy(policy) + require.NoError(t, err) + assert.Equal(t, "none", got.DisapprovalIntegrity) + }) + + t.Run("endorser-min-integrity validated and propagated", func(t *testing.T) { + policy := &GuardPolicy{AllowOnly: &AllowOnlyPolicy{ + Repos: "public", + MinIntegrity: "approved", + EndorsementReactions: []string{"THUMBS_UP"}, + EndorserMinIntegrity: "approved", + }} + got, err := NormalizeGuardPolicy(policy) + require.NoError(t, err) + assert.Equal(t, "approved", got.EndorserMinIntegrity) + }) + + t.Run("endorsement-reactions deduplication (case-insensitive)", func(t *testing.T) { + policy := &GuardPolicy{AllowOnly: &AllowOnlyPolicy{ + Repos: "public", + MinIntegrity: "approved", + EndorsementReactions: []string{"THUMBS_UP", "thumbs_up", "THUMBS_UP"}, + }} + got, err := NormalizeGuardPolicy(policy) + require.NoError(t, err) + assert.Len(t, got.EndorsementReactions, 1) + assert.Equal(t, "THUMBS_UP", got.EndorsementReactions[0]) + }) + + t.Run("invalid disapproval-integrity rejected", func(t *testing.T) { + policy := &GuardPolicy{AllowOnly: &AllowOnlyPolicy{ + Repos: "public", + MinIntegrity: "approved", + DisapprovalIntegrity: "invalid-level", + }} + _, err := NormalizeGuardPolicy(policy) + require.Error(t, err) + assert.Contains(t, err.Error(), "disapproval-integrity") + }) + + t.Run("invalid endorser-min-integrity rejected", func(t *testing.T) { + policy := &GuardPolicy{AllowOnly: &AllowOnlyPolicy{ + Repos: "public", + MinIntegrity: "approved", + EndorserMinIntegrity: "unknown", + }} + _, err := NormalizeGuardPolicy(policy) + require.Error(t, err) + assert.Contains(t, err.Error(), "endorser-min-integrity") + }) + + t.Run("empty endorsement-reactions entry rejected", func(t *testing.T) { + policy := &GuardPolicy{AllowOnly: &AllowOnlyPolicy{ + Repos: "public", + MinIntegrity: "approved", + EndorsementReactions: []string{"THUMBS_UP", ""}, + }} + _, err := NormalizeGuardPolicy(policy) + require.Error(t, err) + assert.Contains(t, err.Error(), "endorsement-reactions entries must not be empty") + }) + + t.Run("reaction fields absent → normalized fields empty", func(t *testing.T) { + policy := &GuardPolicy{AllowOnly: &AllowOnlyPolicy{ + Repos: "public", + MinIntegrity: "none", + }} + got, err := NormalizeGuardPolicy(policy) + require.NoError(t, err) + assert.Empty(t, got.EndorsementReactions) + assert.Empty(t, got.DisapprovalReactions) + assert.Empty(t, got.DisapprovalIntegrity) + assert.Empty(t, got.EndorserMinIntegrity) + }) + + t.Run("AllowOnlyPolicy JSON round-trip with reaction fields", func(t *testing.T) { + original := AllowOnlyPolicy{ + Repos: "public", + MinIntegrity: "approved", + EndorsementReactions: []string{"THUMBS_UP", "HEART"}, + DisapprovalReactions: []string{"THUMBS_DOWN", "CONFUSED"}, + DisapprovalIntegrity: "none", + EndorserMinIntegrity: "approved", + } + data, err := json.Marshal(original) + require.NoError(t, err) + + var got AllowOnlyPolicy + require.NoError(t, json.Unmarshal(data, &got)) + assert.Equal(t, original.EndorsementReactions, got.EndorsementReactions) + assert.Equal(t, original.DisapprovalReactions, got.DisapprovalReactions) + assert.Equal(t, original.DisapprovalIntegrity, got.DisapprovalIntegrity) + assert.Equal(t, original.EndorserMinIntegrity, got.EndorserMinIntegrity) + }) +} diff --git a/internal/guard/wasm.go b/internal/guard/wasm.go index bc05b2ed..ea144fa9 100644 --- a/internal/guard/wasm.go +++ b/internal/guard/wasm.go @@ -414,7 +414,8 @@ func buildStrictLabelAgentPayload(policy interface{}) (map[string]interface{}, e // Validate that the allow-only object contains only known keys. for k := range allowOnly { switch k { - case "repos", "min-integrity", "integrity", "blocked-users", "approval-labels", "trusted-users": + case "repos", "min-integrity", "integrity", "blocked-users", "approval-labels", "trusted-users", + "endorsement-reactions", "disapproval-reactions", "disapproval-integrity", "endorser-min-integrity": // valid allow-only keys default: return nil, fmt.Errorf("invalid guard policy transport shape: unexpected allow-only key %q", k) @@ -493,6 +494,47 @@ func buildStrictLabelAgentPayload(policy interface{}) (map[string]interface{}, e } } + // Validate endorsement-reactions and disapproval-reactions if present. + for _, reactionKey := range []string{"endorsement-reactions", "disapproval-reactions"} { + if reactionsRaw, ok := allowOnly[reactionKey]; ok { + arr, ok := reactionsRaw.([]interface{}) + if !ok { + return nil, fmt.Errorf("invalid %s value: expected array of strings", reactionKey) + } + for _, entry := range arr { + if s, ok := entry.(string); !ok || strings.TrimSpace(s) == "" { + return nil, fmt.Errorf("invalid %s value: each entry must be a non-empty string", reactionKey) + } + } + } + } + + // Validate disapproval-integrity if present. + if disIntRaw, ok := allowOnly["disapproval-integrity"]; ok { + disInt, ok := disIntRaw.(string) + if !ok { + return nil, fmt.Errorf("invalid disapproval-integrity value: expected one of none|unapproved|approved|merged") + } + switch strings.ToLower(strings.TrimSpace(disInt)) { + case "none", "unapproved", "approved", "merged": + default: + return nil, fmt.Errorf("invalid disapproval-integrity value: expected one of none|unapproved|approved|merged") + } + } + + // Validate endorser-min-integrity if present. + if endMinRaw, ok := allowOnly["endorser-min-integrity"]; ok { + endMin, ok := endMinRaw.(string) + if !ok { + return nil, fmt.Errorf("invalid endorser-min-integrity value: expected one of none|unapproved|approved|merged") + } + switch strings.ToLower(strings.TrimSpace(endMin)) { + case "none", "unapproved", "approved", "merged": + default: + return nil, fmt.Errorf("invalid endorser-min-integrity value: expected one of none|unapproved|approved|merged") + } + } + return payload, nil } diff --git a/internal/guard/wasm_test.go b/internal/guard/wasm_test.go index d069f42d..66314b2d 100644 --- a/internal/guard/wasm_test.go +++ b/internal/guard/wasm_test.go @@ -690,6 +690,99 @@ func TestBuildStrictLabelAgentPayloadExtended(t *testing.T) { require.Error(t, err) assert.Contains(t, err.Error(), "unexpected allow-only key") }) + + t.Run("valid endorsement-reactions succeeds", func(t *testing.T) { + policy := map[string]interface{}{ + "allow-only": map[string]interface{}{ + "repos": "public", + "min-integrity": "approved", + "endorsement-reactions": []interface{}{"THUMBS_UP", "HEART"}, + }, + } + result, err := buildStrictLabelAgentPayload(policy) + require.NoError(t, err) + allowOnly, _ := result["allow-only"].(map[string]interface{}) + assert.Contains(t, allowOnly, "endorsement-reactions") + }) + + t.Run("valid disapproval-reactions succeeds", func(t *testing.T) { + policy := map[string]interface{}{ + "allow-only": map[string]interface{}{ + "repos": "public", + "min-integrity": "approved", + "disapproval-reactions": []interface{}{"THUMBS_DOWN", "CONFUSED"}, + }, + } + result, err := buildStrictLabelAgentPayload(policy) + require.NoError(t, err) + allowOnly, _ := result["allow-only"].(map[string]interface{}) + assert.Contains(t, allowOnly, "disapproval-reactions") + }) + + t.Run("valid disapproval-integrity succeeds", func(t *testing.T) { + policy := map[string]interface{}{ + "allow-only": map[string]interface{}{ + "repos": "public", + "min-integrity": "approved", + "disapproval-reactions": []interface{}{"THUMBS_DOWN"}, + "disapproval-integrity": "none", + }, + } + _, err := buildStrictLabelAgentPayload(policy) + require.NoError(t, err) + }) + + t.Run("valid endorser-min-integrity succeeds", func(t *testing.T) { + policy := map[string]interface{}{ + "allow-only": map[string]interface{}{ + "repos": "public", + "min-integrity": "approved", + "endorsement-reactions": []interface{}{"THUMBS_UP"}, + "endorser-min-integrity": "approved", + }, + } + _, err := buildStrictLabelAgentPayload(policy) + require.NoError(t, err) + }) + + t.Run("endorsement-reactions with empty string entry returns error", func(t *testing.T) { + policy := map[string]interface{}{ + "allow-only": map[string]interface{}{ + "repos": "public", + "min-integrity": "approved", + "endorsement-reactions": []interface{}{"THUMBS_UP", ""}, + }, + } + _, err := buildStrictLabelAgentPayload(policy) + require.Error(t, err) + assert.Contains(t, err.Error(), "endorsement-reactions") + }) + + t.Run("invalid disapproval-integrity value returns error", func(t *testing.T) { + policy := map[string]interface{}{ + "allow-only": map[string]interface{}{ + "repos": "public", + "min-integrity": "approved", + "disapproval-integrity": "invalid", + }, + } + _, err := buildStrictLabelAgentPayload(policy) + require.Error(t, err) + assert.Contains(t, err.Error(), "disapproval-integrity") + }) + + t.Run("invalid endorser-min-integrity value returns error", func(t *testing.T) { + policy := map[string]interface{}{ + "allow-only": map[string]interface{}{ + "repos": "public", + "min-integrity": "approved", + "endorser-min-integrity": "badvalue", + }, + } + _, err := buildStrictLabelAgentPayload(policy) + require.Error(t, err) + assert.Contains(t, err.Error(), "endorser-min-integrity") + }) } func TestBuildLabelAgentPayload(t *testing.T) { From f524be7851670a4317015fa1a886acb3e29a849b Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sun, 12 Apr 2026 19:09:17 +0000 Subject: [PATCH 3/7] fix: address code review feedback on reaction endorsement implementation - Use separate AtomicBool flags for endorsement vs disapproval gateway warnings so each reaction kind logs its own warning at most once per process lifetime - Add warning log for unrecognised integrity level values in integrity_level_rank() (defaults to 'approved' per spec, now with explicit warning) - Document cache key username lowercasing behavior in get_collaborator_permission_with_callback Agent-Logs-Url: https://github.com/github/gh-aw-mcpg/sessions/1cd99fee-78d0-43bd-bc83-1d39e17d9de3 Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com> --- .../rust-guard/src/labels/backend.rs | 3 +++ .../rust-guard/src/labels/helpers.rs | 23 +++++++++++++++---- 2 files changed, 22 insertions(+), 4 deletions(-) diff --git a/guards/github-guard/rust-guard/src/labels/backend.rs b/guards/github-guard/rust-guard/src/labels/backend.rs index 77895f5b..76c55264 100644 --- a/guards/github-guard/rust-guard/src/labels/backend.rs +++ b/guards/github-guard/rust-guard/src/labels/backend.rs @@ -487,6 +487,9 @@ pub fn get_collaborator_permission_with_callback( return None; } + // Cache key uses lowercase username because GitHub usernames are case-insensitive. + // The original case-sensitive username is preserved in the returned CollaboratorPermission + // struct (via `username.to_string()`) so callers see the canonical display form. let cache_key = format!("{}/{}:{}", owner, repo, username.to_ascii_lowercase()); // Return cached permission if available. diff --git a/guards/github-guard/rust-guard/src/labels/helpers.rs b/guards/github-guard/rust-guard/src/labels/helpers.rs index d75d1926..900ce6c4 100644 --- a/guards/github-guard/rust-guard/src/labels/helpers.rs +++ b/guards/github-guard/rust-guard/src/labels/helpers.rs @@ -10,8 +10,11 @@ use serde_json::Value; use super::backend::GithubMcpCallback; use super::constants::{field_names, label_constants}; -/// Ensures the gateway-mode reaction warning is emitted at most once per process lifetime. -static REACTION_GATEWAY_WARNING_EMITTED: AtomicBool = AtomicBool::new(false); +/// Ensures the endorsement gateway-mode warning is emitted at most once per process lifetime. +static ENDORSEMENT_GATEWAY_WARNING_EMITTED: AtomicBool = AtomicBool::new(false); + +/// Ensures the disapproval gateway-mode warning is emitted at most once per process lifetime. +static DISAPPROVAL_GATEWAY_WARNING_EMITTED: AtomicBool = AtomicBool::new(false); /// Extract a resource number from a JSON item, returning the number as a string. /// Checks the `number` field first, then falls back to extracting the trailing @@ -371,7 +374,13 @@ fn integrity_level_rank(level: &str) -> u8 { "unapproved" => 2, "approved" => 3, "merged" => 4, - _ => 3, // unknown → default to approved + other => { + crate::log_warn(&format!( + "integrity_level_rank: unrecognised level {:?}, defaulting to 'approved'", + other + )); + 3 // unrecognised → safe default is "approved" (matches endorser_min_integrity default) + } } } @@ -437,7 +446,13 @@ pub fn has_maintainer_reaction_with_callback( // If a `reactions` field is present but has no `nodes` array, we are in // gateway mode: reaction counts are available but reactor identity is not. if item.get("reactions").is_some() { - if !REACTION_GATEWAY_WARNING_EMITTED.swap(true, Ordering::Relaxed) { + // Use reaction-kind-specific flags so each kind logs its own warning once. + let already_warned = match reaction_kind { + "endorsement" => ENDORSEMENT_GATEWAY_WARNING_EMITTED.swap(true, Ordering::Relaxed), + "disapproval" => DISAPPROVAL_GATEWAY_WARNING_EMITTED.swap(true, Ordering::Relaxed), + _ => false, + }; + if !already_warned { crate::log_warn(&format!( "[integrity] {}: {}-reactions configured but reactor identity unavailable \ (gateway mode) — ignoring reactions for integrity evaluation", From b99ae97f7cb1524c189373ff753e88dd7a1e7ea6 Mon Sep 17 00:00:00 2001 From: Landon Cox Date: Sun, 12 Apr 2026 12:36:18 -0700 Subject: [PATCH 4/7] Address review: fix cache scope docs, normalize cache keys, fix docstring - Update collaborator permission cache comment to accurately describe it as process-wide static (not per-request). - Normalize owner/repo to lowercase in cache keys since GitHub treats them as case-insensitive, improving cache hit rate. - Fix integrity_level_rank docstring: remove incorrect '0 = unknown' and 'reader/writer' terminology; document actual return values. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../rust-guard/src/labels/backend.rs | 21 ++++++++++++------- .../rust-guard/src/labels/helpers.rs | 4 +++- 2 files changed, 17 insertions(+), 8 deletions(-) diff --git a/guards/github-guard/rust-guard/src/labels/backend.rs b/guards/github-guard/rust-guard/src/labels/backend.rs index 76c55264..c14decfd 100644 --- a/guards/github-guard/rust-guard/src/labels/backend.rs +++ b/guards/github-guard/rust-guard/src/labels/backend.rs @@ -24,9 +24,11 @@ fn repo_owner_type_cache() -> &'static Mutex> { CACHE.get_or_init(|| Mutex::new(HashMap::new())) } -/// Cache for collaborator permission lookups keyed by "owner/repo:username". -/// Caches the raw permission string so it can be reused across multiple items -/// that share the same reactor within a single gateway request. +/// Cache for collaborator permission lookups keyed by "owner/repo:username" (all lowercase). +/// This is a process-wide static cache that persists across requests. Because the WASM guard +/// is instantiated per-request in the gateway, entries accumulate over the process lifetime. +/// All key components (owner, repo, username) are lowercased since GitHub treats them as +/// case-insensitive. fn collaborator_permission_cache() -> &'static Mutex>> { static CACHE: OnceLock>>> = OnceLock::new(); CACHE.get_or_init(|| Mutex::new(HashMap::new())) @@ -487,10 +489,15 @@ pub fn get_collaborator_permission_with_callback( return None; } - // Cache key uses lowercase username because GitHub usernames are case-insensitive. - // The original case-sensitive username is preserved in the returned CollaboratorPermission - // struct (via `username.to_string()`) so callers see the canonical display form. - let cache_key = format!("{}/{}:{}", owner, repo, username.to_ascii_lowercase()); + // Cache key lowercases owner, repo, and username since GitHub treats all three + // as case-insensitive. This ensures "Org/Repo:Alice" and "org/repo:alice" share + // the same cache entry. + let cache_key = format!( + "{}/{}:{}", + owner.to_ascii_lowercase(), + repo.to_ascii_lowercase(), + username.to_ascii_lowercase() + ); // Return cached permission if available. if let Some(cached) = get_cached_collaborator_permission(&cache_key) { diff --git a/guards/github-guard/rust-guard/src/labels/helpers.rs b/guards/github-guard/rust-guard/src/labels/helpers.rs index 900ce6c4..506a3127 100644 --- a/guards/github-guard/rust-guard/src/labels/helpers.rs +++ b/guards/github-guard/rust-guard/src/labels/helpers.rs @@ -367,7 +367,9 @@ fn effective_endorser_min_integrity<'a>(ctx: &'a PolicyContext) -> &'a str { if v.is_empty() { "approved" } else { v } } -/// Convert an integrity level name to its rank (0 = unknown, 1 = none, 2 = reader, 3 = writer, 4 = merged). +/// Convert an integrity level name to its rank for comparison. +/// Returns: 1 = none, 2 = unapproved, 3 = approved, 4 = merged. +/// Unrecognised levels default to rank 3 (approved) with a warning log. fn integrity_level_rank(level: &str) -> u8 { match level.trim().to_ascii_lowercase().as_str() { "none" => 1, From bf5b1a2afba506e91889e5903451631f7bd8d8b7 Mon Sep 17 00:00:00 2001 From: Landon Cox Date: Sun, 12 Apr 2026 12:39:27 -0700 Subject: [PATCH 5/7] Remove unused test helper ctx_with_disapproval_reactions from helpers.rs The function was defined in helpers.rs tests but never called (the equivalent helper in mod.rs tests is the one actually used). Fixes rust-guard-test CI failure: -D dead-code triggered by RUSTFLAGS. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- guards/github-guard/rust-guard/src/labels/helpers.rs | 7 ------- 1 file changed, 7 deletions(-) diff --git a/guards/github-guard/rust-guard/src/labels/helpers.rs b/guards/github-guard/rust-guard/src/labels/helpers.rs index 506a3127..6636dafd 100644 --- a/guards/github-guard/rust-guard/src/labels/helpers.rs +++ b/guards/github-guard/rust-guard/src/labels/helpers.rs @@ -1750,13 +1750,6 @@ mod tests { } } - fn ctx_with_disapproval_reactions(reactions: Vec<&str>) -> PolicyContext { - PolicyContext { - disapproval_reactions: reactions.into_iter().map(|s| s.to_string()).collect(), - ..Default::default() - } - } - /// Mock callback that returns admin permission for any user. fn admin_permission_callback(_tool: &str, _args: &str, buf: &mut [u8]) -> Result { let response = r#"{"permission":"admin","user":{"login":"maintainer"}}"#; From 3ef2a3cf5d3ab52bfd5ab70af5631e329794c278 Mon Sep 17 00:00:00 2001 From: Landon Cox Date: Sun, 12 Apr 2026 12:54:39 -0700 Subject: [PATCH 6/7] review: add precedence test, fix misleading defaults, add validation tests - Add critical test: disapproval overrides endorsement on same item (helpers.rs) verifying the precedence rule via mock callbacks - Add tests for disapproval with admin/read permission thresholds - Fix misleading Go comments claiming defaults are set by normalization (defaults are applied in Rust, not Go) - Add Go tests: empty disapproval-reactions entry rejected, disapproval-reactions deduplication Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../rust-guard/src/labels/helpers.rs | 75 +++++++++++++++++++ internal/config/guard_policy.go | 6 +- internal/config/guard_policy_test.go | 23 ++++++ 3 files changed, 102 insertions(+), 2 deletions(-) diff --git a/guards/github-guard/rust-guard/src/labels/helpers.rs b/guards/github-guard/rust-guard/src/labels/helpers.rs index 6636dafd..a4452dd6 100644 --- a/guards/github-guard/rust-guard/src/labels/helpers.rs +++ b/guards/github-guard/rust-guard/src/labels/helpers.rs @@ -1936,4 +1936,79 @@ mod tests { let ctx = PolicyContext::default(); assert_eq!(effective_endorser_min_integrity(&ctx), "approved"); } + + #[test] + fn test_disapproval_overrides_endorsement_on_same_item() { + // The core precedence rule: when the same item has both an endorsement + // and a disapproval reaction from qualified maintainers, disapproval wins + // because it runs last in the chain. + let repo = "owner/repo"; + let ctx = PolicyContext { + endorsement_reactions: vec!["THUMBS_UP".to_string()], + disapproval_reactions: vec!["THUMBS_DOWN".to_string()], + disapproval_integrity: "none".to_string(), + ..Default::default() + }; + let item = serde_json::json!({ + "number": 42, + "reactions": {"nodes": [ + {"user": {"login": "alice"}, "content": "THUMBS_UP"}, + {"user": {"login": "bob"}, "content": "THUMBS_DOWN"} + ]} + }); + + // Both endorsement and disapproval should match with admin callback + assert!(has_maintainer_reaction_with_callback( + &item, repo, &ctx.endorsement_reactions, "approved", &ctx, + admin_permission_callback, "endorsement" + )); + assert!(has_maintainer_reaction_with_callback( + &item, repo, &ctx.disapproval_reactions, "approved", &ctx, + admin_permission_callback, "disapproval" + )); + + // Simulate the integrity chain: start with none (external contributor), + // apply endorsement (promotes to approved), then apply disapproval (caps to none). + let base = none_integrity(repo, &ctx); + let after_endorsement = max_integrity(repo, base, writer_integrity(repo, &ctx), &ctx); + assert_eq!(after_endorsement, writer_integrity(repo, &ctx), "endorsement should promote to approved"); + + let demote_cap = integrity_for_level("none", repo, &ctx); + let after_disapproval = cap_integrity(repo, after_endorsement, demote_cap, &ctx); + assert_eq!(after_disapproval, none_integrity(repo, &ctx), "disapproval should override endorsement back to none"); + } + + #[test] + fn test_disapproval_reaction_with_admin_callback() { + // Verify has_maintainer_reaction_with_callback works for disapproval reaction kind + let ctx = PolicyContext { + disapproval_reactions: vec!["THUMBS_DOWN".to_string()], + ..Default::default() + }; + let item = serde_json::json!({ + "number": 42, + "reactions": {"nodes": [{"user": {"login": "alice"}, "content": "THUMBS_DOWN"}]} + }); + assert!(has_maintainer_reaction_with_callback( + &item, "owner/repo", &ctx.disapproval_reactions, "approved", &ctx, + admin_permission_callback, "disapproval" + )); + } + + #[test] + fn test_disapproval_reaction_below_threshold() { + // Reactor with read permission should not count for disapproval + let ctx = PolicyContext { + disapproval_reactions: vec!["THUMBS_DOWN".to_string()], + ..Default::default() + }; + let item = serde_json::json!({ + "number": 42, + "reactions": {"nodes": [{"user": {"login": "external"}, "content": "THUMBS_DOWN"}]} + }); + assert!(!has_maintainer_reaction_with_callback( + &item, "owner/repo", &ctx.disapproval_reactions, "approved", &ctx, + read_permission_callback, "disapproval" + )); + } } diff --git a/internal/config/guard_policy.go b/internal/config/guard_policy.go index 57824697..2c8ed19d 100644 --- a/internal/config/guard_policy.go +++ b/internal/config/guard_policy.go @@ -416,7 +416,8 @@ func NormalizeGuardPolicy(policy *GuardPolicy) (*NormalizedGuardPolicy, error) { } } - // Validate and normalize disapproval-integrity (optional, defaults to "none"). + // Validate and normalize disapproval-integrity (optional; empty means feature + // uses Rust-side default of "none" when endorsement/disapproval is evaluated). if v := strings.ToLower(strings.TrimSpace(policy.AllowOnly.DisapprovalIntegrity)); v != "" { if _, ok := validMinIntegrityValues[v]; !ok { return nil, fmt.Errorf("allow-only.disapproval-integrity must be one of: none, unapproved, approved, merged") @@ -424,7 +425,8 @@ func NormalizeGuardPolicy(policy *GuardPolicy) (*NormalizedGuardPolicy, error) { normalized.DisapprovalIntegrity = v } - // Validate and normalize endorser-min-integrity (optional, defaults to "approved"). + // Validate and normalize endorser-min-integrity (optional; empty means feature + // uses Rust-side default of "approved" when evaluating reactor eligibility). if v := strings.ToLower(strings.TrimSpace(policy.AllowOnly.EndorserMinIntegrity)); v != "" { if _, ok := validMinIntegrityValues[v]; !ok { return nil, fmt.Errorf("allow-only.endorser-min-integrity must be one of: none, unapproved, approved, merged") diff --git a/internal/config/guard_policy_test.go b/internal/config/guard_policy_test.go index 6c22a80c..cd8d2632 100644 --- a/internal/config/guard_policy_test.go +++ b/internal/config/guard_policy_test.go @@ -1084,6 +1084,29 @@ func TestNormalizeGuardPolicyReactionEndorsement(t *testing.T) { assert.Contains(t, err.Error(), "endorsement-reactions entries must not be empty") }) + t.Run("empty disapproval-reactions entry rejected", func(t *testing.T) { + policy := &GuardPolicy{AllowOnly: &AllowOnlyPolicy{ + Repos: "public", + MinIntegrity: "approved", + DisapprovalReactions: []string{"THUMBS_DOWN", ""}, + }} + _, err := NormalizeGuardPolicy(policy) + require.Error(t, err) + assert.Contains(t, err.Error(), "disapproval-reactions entries must not be empty") + }) + + t.Run("disapproval-reactions deduplication (case-insensitive)", func(t *testing.T) { + policy := &GuardPolicy{AllowOnly: &AllowOnlyPolicy{ + Repos: "public", + MinIntegrity: "approved", + DisapprovalReactions: []string{"THUMBS_DOWN", "thumbs_down", "THUMBS_DOWN"}, + }} + got, err := NormalizeGuardPolicy(policy) + require.NoError(t, err) + assert.Len(t, got.DisapprovalReactions, 1) + assert.Equal(t, "THUMBS_DOWN", got.DisapprovalReactions[0]) + }) + t.Run("reaction fields absent → normalized fields empty", func(t *testing.T) { policy := &GuardPolicy{AllowOnly: &AllowOnlyPolicy{ Repos: "public", From a2d148c6648988ddc8b234bc65e4d695d3d17646 Mon Sep 17 00:00:00 2001 From: Landon Cox Date: Sun, 12 Apr 2026 13:13:54 -0700 Subject: [PATCH 7/7] fix: use unique login in error_callback test to avoid cache collision The test_has_maintainer_reaction_backend_error_skips test was flaky because it used login 'alice' which could be cached by prior tests using admin_permission_callback. The global permission cache returned the cached 'admin' permission instead of invoking error_callback. Use 'error-test-user' to guarantee a cache miss. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- guards/github-guard/rust-guard/src/labels/helpers.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/guards/github-guard/rust-guard/src/labels/helpers.rs b/guards/github-guard/rust-guard/src/labels/helpers.rs index a4452dd6..2ec0c61e 100644 --- a/guards/github-guard/rust-guard/src/labels/helpers.rs +++ b/guards/github-guard/rust-guard/src/labels/helpers.rs @@ -1881,9 +1881,11 @@ mod tests { #[test] fn test_has_maintainer_reaction_backend_error_skips() { let ctx = ctx_with_endorsement_reactions(vec!["THUMBS_UP"]); + // Use a unique login to avoid hitting the global permission cache populated + // by other tests (e.g. admin_permission_callback caching "alice"). let item = serde_json::json!({ "number": 42, - "reactions": {"nodes": [{"user": {"login": "alice"}, "content": "THUMBS_UP"}]} + "reactions": {"nodes": [{"user": {"login": "error-test-user"}, "content": "THUMBS_UP"}]} }); // Backend error → can't confirm permission → should not count as endorsement assert!(!has_maintainer_reaction_with_callback(