Skip to content

Commit ea54e3c

Browse files
authored
Add collaborator permission fallback to response-level integrity functions (#3831)
## Problem Issue content authored by an org owner (@dsyme) was unexpectedly filtered by integrity policy in [this workflow run](https://github.com/githubnext/gh-aw-test/actions/runs/24441119155/job/71406472575). The agent reported the issue content was "filtered by the integrity policy" even though the author is an org owner with admin rights. Related issue: githubnext/gh-aw-test#1904 ## Root Cause PR #2863 added a collaborator permission API fallback to `resolve_author_integrity` in `tool_rules.rs` (Phase 1 — `LabelResource`). However, for **read operations**, Phase 2 (coarse-grained check) is skipped, deferring to Phase 5's per-item labels from `LabelResponse`. The per-item integrity functions — `issue_integrity`, `pr_integrity`, and `commit_integrity` in `helpers.rs` — did **not** have the collaborator permission fallback. For org owners whose `author_association` is reported as `"NONE"` (because admin access is inherited through org ownership rather than direct collaboration), the per-item integrity was computed as `none`, which is below the agent's `min_integrity` threshold, causing the content to be filtered. ## Fix - Extract a shared `elevate_via_collaborator_permission` helper that encapsulates the org-repo check + collaborator permission API fallback logic - Apply it in all four integrity-computing call sites: - `resolve_author_integrity` in `tool_rules.rs` (refactored to use shared helper) - `issue_integrity` in `helpers.rs` (new fallback) - `pr_integrity` in `helpers.rs` (new fallback) - `commit_integrity` in `helpers.rs` (new fallback) This ensures consistent integrity computation across both Phase 1 (resource labeling) and Phase 5 (response labeling). ## Testing - All 291 Rust guard tests pass - `make agent-finished` passes (format, build, lint, all Go tests)
2 parents 6989c26 + 5635908 commit ea54e3c

3 files changed

Lines changed: 196 additions & 30 deletions

File tree

guards/github-guard/rust-guard/src/labels/helpers.rs

Lines changed: 97 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1291,6 +1291,72 @@ pub fn collaborator_permission_floor(
12911291
}
12921292
}
12931293

1294+
/// Elevate integrity via collaborator permission fallback for org repos.
1295+
///
1296+
/// Rank threshold for writer-level integrity (none=1, reader=2, writer=3, merged=4).
1297+
const WRITER_RANK: u8 = 3;
1298+
1299+
/// Attempt to elevate integrity for an author in an org-owned repository
1300+
/// by checking their effective collaborator permission.
1301+
///
1302+
/// When `author_association` gives insufficient integrity (below writer level),
1303+
/// this function checks the user's effective permission via the GitHub
1304+
/// collaborator permission API. This correctly handles org owners/admins whose
1305+
/// `author_association` is reported as "NONE" because their access is inherited
1306+
/// through org ownership rather than direct collaboration.
1307+
///
1308+
/// Backend calls are cached per-user, so repeated lookups for the same author
1309+
/// across list/search items are inexpensive.
1310+
///
1311+
/// Parameters:
1312+
/// - `author_login`: the issue/PR/commit author's GitHub login
1313+
/// - `repo_full_name`: "owner/repo" string
1314+
/// - `resource_label`: label for logging (e.g. "issue", "pr", "commit")
1315+
/// - `resource_id`: number or identifier for logging
1316+
/// - `integrity`: current integrity labels to potentially elevate
1317+
/// - `ctx`: policy context
1318+
///
1319+
/// Returns the (potentially elevated) integrity labels.
1320+
pub fn elevate_via_collaborator_permission(
1321+
author_login: &str,
1322+
repo_full_name: &str,
1323+
resource_label: &str,
1324+
resource_id: &str,
1325+
integrity: Vec<String>,
1326+
ctx: &PolicyContext,
1327+
) -> Vec<String> {
1328+
if integrity_rank(repo_full_name, &integrity, ctx) >= WRITER_RANK || author_login.is_empty() {
1329+
return integrity;
1330+
}
1331+
let (owner, repo) = match repo_full_name.split_once('/') {
1332+
Some((o, r)) if !o.is_empty() && !r.is_empty() => (o, r),
1333+
_ => return integrity,
1334+
};
1335+
let is_org = super::backend::is_repo_org_owned(owner, repo).unwrap_or(false);
1336+
if !is_org {
1337+
return integrity;
1338+
}
1339+
crate::log_debug(&format!(
1340+
"[integrity] {}:{}: author_association floor below writer (rank={}), checking collaborator permission for {}",
1341+
resource_label, resource_id, integrity_rank(repo_full_name, &integrity, ctx), author_login
1342+
));
1343+
if let Some(collab) = super::backend::get_collaborator_permission(owner, repo, author_login) {
1344+
let perm_floor = collaborator_permission_floor(repo_full_name, collab.permission.as_deref(), ctx);
1345+
let merged = max_integrity(repo_full_name, integrity, perm_floor, ctx);
1346+
crate::log_debug(&format!(
1347+
"[integrity] {}:{}: collaborator permission={:?} → merged rank={}",
1348+
resource_label, resource_id, collab.permission, integrity_rank(repo_full_name, &merged, ctx)
1349+
));
1350+
merged
1351+
} else {
1352+
crate::log_debug(&format!(
1353+
"[integrity] {}:{}: collaborator permission lookup returned None for {}, keeping author_association floor",
1354+
resource_label, resource_id, author_login
1355+
));
1356+
integrity
1357+
}
1358+
}
1359+
12941360
/// Check if a branch/ref should be treated as default branch context
12951361
pub fn is_default_branch_ref(branch_ref: &str) -> bool {
12961362
branch_ref.is_empty()
@@ -1417,6 +1483,16 @@ pub fn pr_integrity(
14171483
}
14181484
}
14191485

1486+
// Collaborator permission fallback for org repos (handles org owners/admins
1487+
// whose author_association is "NONE" due to inherited org access).
1488+
if !repo_private {
1489+
let number = item.get(field_names::NUMBER).and_then(|v| v.as_u64()).unwrap_or(0);
1490+
integrity = elevate_via_collaborator_permission(
1491+
author_login, repo_full_name, "pr", &format!("{}#{}", repo_full_name, number),
1492+
integrity, ctx,
1493+
);
1494+
}
1495+
14201496
if repo_private {
14211497
integrity = max_integrity(
14221498
repo_full_name,
@@ -1527,6 +1603,16 @@ pub fn issue_integrity(
15271603
}
15281604
}
15291605

1606+
// Collaborator permission fallback for org repos (handles org owners/admins
1607+
// whose author_association is "NONE" due to inherited org access).
1608+
if !repo_private {
1609+
let number = item.get(field_names::NUMBER).and_then(|v| v.as_u64()).unwrap_or(0);
1610+
integrity = elevate_via_collaborator_permission(
1611+
author_login, repo_full_name, "issue", &format!("{}#{}", repo_full_name, number),
1612+
integrity, ctx,
1613+
);
1614+
}
1615+
15301616
if repo_private {
15311617
integrity = max_integrity(
15321618
repo_full_name,
@@ -1579,6 +1665,17 @@ pub fn commit_integrity(
15791665

15801666
let mut integrity = author_association_floor(item, repo_full_name, ctx);
15811667

1668+
// Collaborator permission fallback for org repos (handles org owners/admins
1669+
// whose author_association is "NONE" due to inherited org access).
1670+
if !repo_private {
1671+
let sha = item.get("sha").and_then(|v| v.as_str()).unwrap_or("unknown");
1672+
let short_sha = if sha.len() > 8 { &sha[..8] } else { sha };
1673+
integrity = elevate_via_collaborator_permission(
1674+
author_login, repo_full_name, "commit", &format!("{}@{}", repo_full_name, short_sha),
1675+
integrity, ctx,
1676+
);
1677+
}
1678+
15821679
if repo_private {
15831680
integrity = max_integrity(
15841681
repo_full_name,

guards/github-guard/rust-guard/src/labels/mod.rs

Lines changed: 93 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5053,4 +5053,97 @@ mod tests {
50535053
helpers::blocked_integrity(repo, &ctx)
50545054
);
50555055
}
5056+
5057+
// === elevate_via_collaborator_permission tests ===
5058+
5059+
#[test]
5060+
fn test_elevate_via_collab_permission_skips_when_already_writer() {
5061+
let ctx = default_ctx();
5062+
let repo = "github/copilot";
5063+
let writer = writer_integrity(repo, &ctx);
5064+
let result = helpers::elevate_via_collaborator_permission(
5065+
"someuser", repo, "issue", "github/copilot#1",
5066+
writer.clone(), &ctx,
5067+
);
5068+
assert_eq!(result, writer, "should return unchanged when already at writer level");
5069+
}
5070+
5071+
#[test]
5072+
fn test_elevate_via_collab_permission_skips_when_merged() {
5073+
let ctx = default_ctx();
5074+
let repo = "github/copilot";
5075+
let merged = merged_integrity(repo, &ctx);
5076+
let result = helpers::elevate_via_collaborator_permission(
5077+
"someuser", repo, "issue", "github/copilot#1",
5078+
merged.clone(), &ctx,
5079+
);
5080+
assert_eq!(result, merged, "should return unchanged when already at merged level");
5081+
}
5082+
5083+
#[test]
5084+
fn test_elevate_via_collab_permission_skips_empty_login() {
5085+
let ctx = default_ctx();
5086+
let repo = "github/copilot";
5087+
let none = none_integrity(repo, &ctx);
5088+
let result = helpers::elevate_via_collaborator_permission(
5089+
"", repo, "issue", "github/copilot#1",
5090+
none.clone(), &ctx,
5091+
);
5092+
assert_eq!(result, none, "should return unchanged when author_login is empty");
5093+
}
5094+
5095+
#[test]
5096+
fn test_elevate_via_collab_permission_skips_invalid_repo() {
5097+
let ctx = default_ctx();
5098+
let none = none_integrity("invalid", &ctx);
5099+
let result = helpers::elevate_via_collaborator_permission(
5100+
"someuser", "invalid", "issue", "invalid#1",
5101+
none.clone(), &ctx,
5102+
);
5103+
assert_eq!(result, none, "should return unchanged for invalid repo format");
5104+
}
5105+
5106+
#[test]
5107+
fn test_elevate_via_collab_permission_skips_empty_owner_or_repo() {
5108+
let ctx = default_ctx();
5109+
let none = none_integrity("/repo", &ctx);
5110+
let result = helpers::elevate_via_collaborator_permission(
5111+
"someuser", "/repo", "issue", "/repo#1",
5112+
none.clone(), &ctx,
5113+
);
5114+
assert_eq!(result, none, "should return unchanged for empty owner");
5115+
5116+
let none2 = none_integrity("owner/", &ctx);
5117+
let result2 = helpers::elevate_via_collaborator_permission(
5118+
"someuser", "owner/", "issue", "owner/#1",
5119+
none2.clone(), &ctx,
5120+
);
5121+
assert_eq!(result2, none2, "should return unchanged for empty repo");
5122+
}
5123+
5124+
#[test]
5125+
fn test_elevate_via_collab_permission_no_elevation_non_org_repo() {
5126+
// In test mode, is_repo_org_owned returns None (no cache) → unwrap_or(false)
5127+
// so the function should return integrity unchanged
5128+
let ctx = default_ctx();
5129+
let repo = "github/copilot";
5130+
let none = none_integrity(repo, &ctx);
5131+
let result = helpers::elevate_via_collaborator_permission(
5132+
"dsyme", repo, "issue", "github/copilot#42",
5133+
none.clone(), &ctx,
5134+
);
5135+
assert_eq!(result, none, "should return unchanged when repo is not org-owned (cache miss → false)");
5136+
}
5137+
5138+
#[test]
5139+
fn test_elevate_via_collab_permission_preserves_reader_integrity() {
5140+
let ctx = default_ctx();
5141+
let repo = "github/copilot";
5142+
let reader = reader_integrity(repo, &ctx);
5143+
let result = helpers::elevate_via_collaborator_permission(
5144+
"contributor", repo, "pr", "github/copilot#10",
5145+
reader.clone(), &ctx,
5146+
);
5147+
assert_eq!(result, reader, "should preserve reader integrity when no org lookup available");
5148+
}
50565149
}

guards/github-guard/rust-guard/src/labels/tool_rules.rs

Lines changed: 6 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,8 @@ use serde_json::Value;
77

88
use super::constants::{field_names, SENSITIVE_FILE_KEYWORDS, SENSITIVE_FILE_PATTERNS};
99
use super::helpers::{
10-
author_association_floor_from_str, collaborator_permission_floor, ensure_integrity_baseline,
10+
author_association_floor_from_str,
11+
elevate_via_collaborator_permission, ensure_integrity_baseline,
1112
extract_number_as_string, extract_repo_info, extract_repo_info_from_search_query,
1213
format_repo_id, is_configured_trusted_bot, is_default_branch_commit_context,
1314
is_default_branch_ref, is_trusted_first_party_bot, is_trusted_user, max_integrity,
@@ -100,35 +101,10 @@ fn resolve_author_integrity(
100101
{
101102
floor = max_integrity(repo_id, floor, writer_integrity(repo_id, ctx), ctx);
102103
}
103-
if floor.len() < 3 {
104-
let is_org = super::backend::is_repo_org_owned(owner, repo).unwrap_or(false);
105-
if is_org {
106-
crate::log_info(&format!(
107-
"{} {}/{}#{}: author_association floor insufficient (len={}), checking collaborator permission for {}",
108-
resource_label, owner, repo, resource_num, floor.len(), login
109-
));
110-
if let Some(collab) =
111-
super::backend::get_collaborator_permission(owner, repo, login)
112-
{
113-
let perm_floor = collaborator_permission_floor(
114-
repo_id,
115-
collab.permission.as_deref(),
116-
ctx,
117-
);
118-
let merged = max_integrity(repo_id, floor, perm_floor, ctx);
119-
crate::log_info(&format!(
120-
"{} {}/{}#{}: collaborator permission={:?} → merged floor len={}",
121-
resource_label, owner, repo, resource_num, collab.permission, merged.len()
122-
));
123-
floor = merged;
124-
} else {
125-
crate::log_info(&format!(
126-
"{} {}/{}#{}: collaborator permission lookup returned None for {}, keeping author_association floor",
127-
resource_label, owner, repo, resource_num, login
128-
));
129-
}
130-
}
131-
}
104+
let resource_id = format!("{}/{}#{}", owner, repo, resource_num);
105+
floor = elevate_via_collaborator_permission(
106+
login, repo_id, resource_label, &resource_id, floor, ctx,
107+
);
132108
}
133109

134110
max_integrity(repo_id, base_integrity, floor, ctx)

0 commit comments

Comments
 (0)