Skip to content

Add collaborator permission fallback to response-level integrity functions#3831

Merged
lpcox merged 2 commits intomainfrom
dsyme/collab-permission-response-labeling
Apr 15, 2026
Merged

Add collaborator permission fallback to response-level integrity functions#3831
lpcox merged 2 commits intomainfrom
dsyme/collab-permission-response-labeling

Conversation

@dsyme
Copy link
Copy Markdown
Collaborator

@dsyme dsyme commented Apr 15, 2026

Problem

Issue content authored by an org owner (@dsyme) was unexpectedly filtered by integrity policy in this workflow run. 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)

…tions

The collaborator permission API fallback (from PR #2863) was only applied
in resolve_author_integrity (Phase 1 LabelResource), but not in the
per-item response labeling functions issue_integrity, pr_integrity, and
commit_integrity (Phase 5 LabelResponse).

For reads, Phase 2 skips the coarse-grained block and defers to Phase 5's
per-item labels. So for org owners whose author_association is 'NONE'
(because admin access is inherited through org ownership rather than
direct collaboration), the per-item integrity was too low, causing
content to be incorrectly filtered.

Fix: Extract a shared elevate_via_collaborator_permission helper and
apply it in all four integrity-computing functions:
- resolve_author_integrity (refactored to use shared helper)
- issue_integrity (new: adds collaborator permission fallback)
- pr_integrity (new: adds collaborator permission fallback)
- commit_integrity (new: adds collaborator permission fallback)
Copilot AI review requested due to automatic review settings April 15, 2026 07:39
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Extends the Rust guard’s response-level integrity labeling so org owners/admins with inherited permissions (often surfaced as author_association: "NONE") are not incorrectly downgraded to none integrity and filtered in read responses.

Changes:

  • Extracts a shared elevate_via_collaborator_permission helper for org-repo collaborator-permission fallback.
  • Refactors resolve_author_integrity to use the shared helper.
  • Applies the same fallback to response item integrity functions (issue_integrity, pr_integrity, commit_integrity) for consistent behavior across labeling phases.
Show a summary per file
File Description
guards/github-guard/rust-guard/src/labels/tool_rules.rs Refactors author integrity resolution to call the shared collaborator-permission elevation helper.
guards/github-guard/rust-guard/src/labels/helpers.rs Adds shared elevation helper and invokes it in issue/PR/commit per-item integrity computation.

Copilot's findings

Tip

Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comments suppressed due to low confidence (2)

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

  • commit_integrity is evaluated per-item for commit list responses. With the current elevate_via_collaborator_permission threshold (len < 3), this can lead to many get_collaborator_permission backend calls across a commit list with many distinct authors. Consider narrowing the condition at the call site (e.g., only attempt the fallback when integrity is empty / author_association is NONE) to reduce rate-limit/latency risk.
    if !repo_private {
        let sha = item.get("sha").and_then(|v| v.as_str()).unwrap_or("unknown");
        let short_sha = if sha.len() > 8 { &sha[..8] } else { sha };
        integrity = elevate_via_collaborator_permission(
            author_login, repo_full_name, "commit", &format!("{}@{}", repo_full_name, short_sha),
            integrity, ctx,

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

  • issue_integrity is applied per-item for list/search issue responses, so calling elevate_via_collaborator_permission unconditionally for all non-private issues can trigger many collaborator-permission lookups (and lots of logs) for large result sets. To avoid N backend calls, consider calling the fallback only for the specific low-integrity scenario that needs it (e.g., when the current integrity is empty / corresponds to author_association NONE).
    if !repo_private {
        let number = item.get(field_names::NUMBER).and_then(|v| v.as_u64()).unwrap_or(0);
        integrity = elevate_via_collaborator_permission(
            author_login, repo_full_name, "issue", &format!("{}#{}", repo_full_name, number),
            integrity, ctx,
        );
  • Files reviewed: 2/2 changed files
  • Comments generated: 4

Comment on lines +1294 to +1325
/// Elevate integrity via collaborator permission fallback for org repos.
///
/// When `author_association` gives insufficient integrity (fewer than 3 labels,
/// i.e. below approved/writer level), this function checks the user's effective
/// permission via the GitHub collaborator permission API. This correctly handles
/// org owners/admins whose `author_association` is reported as "NONE" because
/// their access is inherited through org ownership rather than direct collaboration.
///
/// Parameters:
/// - `author_login`: the issue/PR/commit author's GitHub login
/// - `repo_full_name`: "owner/repo" string
/// - `resource_label`: label for logging (e.g. "issue", "pr", "commit")
/// - `resource_id`: number or identifier for logging
/// - `integrity`: current integrity labels to potentially elevate
/// - `ctx`: policy context
///
/// Returns the (potentially elevated) integrity labels.
pub fn elevate_via_collaborator_permission(
author_login: &str,
repo_full_name: &str,
resource_label: &str,
resource_id: &str,
integrity: Vec<String>,
ctx: &PolicyContext,
) -> Vec<String> {
if integrity.len() >= 3 || author_login.is_empty() {
return integrity;
}
let (owner, repo) = match repo_full_name.split_once('/') {
Some((o, r)) if !o.is_empty() && !r.is_empty() => (o, r),
_ => return integrity,
};
Comment on lines +1319 to +1321
if integrity.len() >= 3 || author_login.is_empty() {
return integrity;
}
Comment on lines +1330 to +1343
crate::log_info(&format!(
"[integrity] {}:{}: author_association floor insufficient (len={}), checking collaborator permission for {}",
resource_label, resource_id, integrity.len(), author_login
));
if let Some(collab) = super::backend::get_collaborator_permission(owner, repo, author_login) {
let perm_floor = collaborator_permission_floor(repo_full_name, collab.permission.as_deref(), ctx);
let merged = max_integrity(repo_full_name, integrity, perm_floor, ctx);
crate::log_info(&format!(
"[integrity] {}:{}: collaborator permission={:?} → merged floor len={}",
resource_label, resource_id, collab.permission, merged.len()
));
merged
} else {
crate::log_info(&format!(
Comment on lines +1479 to +1484
if !repo_private {
let number = item.get(field_names::NUMBER).and_then(|v| v.as_u64()).unwrap_or(0);
integrity = elevate_via_collaborator_permission(
author_login, repo_full_name, "pr", &format!("{}#{}", repo_full_name, number),
integrity, ctx,
);
@lpcox
Copy link
Copy Markdown
Collaborator

lpcox commented Apr 15, 2026

@copilot make sure that this solution covers proxy mode, especially when cli-proxy is enabled for the agent

Copy link
Copy Markdown
Contributor

Copilot AI commented Apr 15, 2026

@copilot make sure that this solution covers proxy mode, especially when cli-proxy is enabled for the agent

Yes, the fix covers proxy mode. The elevate_via_collaborator_permission helper lives entirely inside the WASM guard, and the same WASM binary is loaded in both MCP gateway mode and cli-proxy mode.

In proxy mode, the guard's call_backend WASM import routes through the restBackendCaller, which already implements:

  • search_repositories — used by is_repo_private, which piggybacks the org-ownership cache entry that is_repo_org_owned later reads from
  • get_collaborator_permission — directly calls GET /repos/{owner}/{repo}/collaborators/{username}/permission

Both of these were wired up as part of the Phase 1 work (prior to this PR). The Phase 5 response-labeling fallback introduced here therefore works identically in both modes.

Copilot AI requested a review from lpcox April 15, 2026 16:05
1. Replace brittle integrity.len() >= 3 check with semantic
   integrity_rank() >= WRITER_RANK (3). The rank-based check is
   future-proof if label sets ever change.

2. Downgrade all log_info to log_debug in the helper. The function
   runs per-item in list/search responses, so info-level logging
   would be extremely noisy in production.

3. Add 7 unit tests for elevate_via_collaborator_permission covering
   all early-return paths: already writer, already merged, empty
   login, invalid repo format, empty owner/repo, non-org repo
   (cache miss), and reader integrity preservation.

4. Add doc comment noting backend calls are cached per-user, so
   repeated lookups across list items are inexpensive.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@lpcox
Copy link
Copy Markdown
Collaborator

lpcox commented Apr 15, 2026

Review + Fixes

Evaluated the Copilot review feedback and pushed fixes in 5635908:

Addressed

  1. Replaced integrity.len() >= 3 with integrity_rank() >= WRITER_RANK — The length check was a proxy for writer-level. Using the semantic integrity_rank() API is future-proof. Added a WRITER_RANK constant for clarity.

  2. Downgraded log_infolog_debugelevate_via_collaborator_permission runs per-item in list/search responses. log_info would generate very high-volume logs in production.

  3. Added 7 unit tests for elevate_via_collaborator_permission covering all early-return paths: already writer, already merged, empty login, invalid/empty repo format, non-org repo (cache miss), and reader integrity preservation.

Evaluated but not changed

N backend calls concern (suppressed comments): The function already returns early when integrity ≥ writer, so only items with low integrity (NONE/FIRST_TIMER association) proceed to the backend. Both is_repo_org_owned and get_collaborator_permission are cached per-key, so repeated lookups for the same author across list items are O(1) after the first call. Added a doc comment noting this. The caching makes the unconditional call-site pattern acceptable.

Overall assessment

The PR's approach is sound — extracting a shared helper and applying it consistently across pr_integrity, issue_integrity, and commit_integrity is the right pattern. The original problem (org owners with author_association: NONE getting filtered) is correctly addressed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants