Skip to content

Commit b0342d8

Browse files
authored
Reduce duplication in write-op integrity classification and MinIntegrity conversion (#3534)
`label_resource` had four `if/else if` branches where the first two (merge/delete) were identical and the last two (update/create + catch-all) were identical. `MinIntegrity` lacked a string conversion method, forcing callers to maintain parallel match arms. - **Collapse integrity branches** in `lib.rs`: merge/delete → `writer_integrity`, everything else → `reader_integrity`. Single `!repo_id.is_empty()` guard wrapping a conditional. - **Add `MinIntegrity::as_str()`** in `helpers.rs`: canonical `&'static str` conversion using `policy_integrity` constants. Replaces the 5-line match block in `label_agent` with `integrity_floor.as_str().to_string()`. - **Unit test** for `as_str()` covering all four variants. ```rust // Before: 4 branches, 2 pairs of duplicates if tools::is_merge_operation(&input.tool_name) { if !repo_id.is_empty() { integrity = labels::writer_integrity(&repo_id, &ctx); } } else if tools::is_delete_operation(&input.tool_name) { if !repo_id.is_empty() { integrity = labels::writer_integrity(&repo_id, &ctx); } } else if tools::is_update_operation(&input.tool_name) || tools::is_create_operation(&input.tool_name) { if !repo_id.is_empty() { integrity = labels::reader_integrity(&repo_id, &ctx); } } else if !repo_id.is_empty() { integrity = labels::reader_integrity(&repo_id, &ctx); } // After if !repo_id.is_empty() { integrity = if tools::is_merge_operation(&input.tool_name) || tools::is_delete_operation(&input.tool_name) { labels::writer_integrity(&repo_id, &ctx) } else { labels::reader_integrity(&repo_id, &ctx) }; } ``` > [!WARNING] > > <details> > <summary>Firewall rules blocked me from connecting to one or more addresses (expand for details)</summary> > > #### I tried to connect to the following addresses, but was blocked by firewall rules: > > - `example.com` > - Triggering command: `/tmp/go-build1833741381/b514/launcher.test /tmp/go-build1833741381/b514/launcher.test -test.testlogfile=/tmp/go-build1833741381/b514/testlog.txt -test.paniconexit0 -test.timeout=10m0s -W .cfg olang.org/grpc@v-ifaceassert x_amd64/vet . --gdwarf2 --64 x_amd64/vet .cfg�� 2245169/b376/_pkg_.a -I x_amd64/vet --gdwarf-5 g/grpc/internal//usr/bin/runc -o x_amd64/vet` (dns block) > - `invalid-host-that-does-not-exist-12345.com` > - Triggering command: `/tmp/go-build1833741381/b496/config.test /tmp/go-build1833741381/b496/config.test -test.testlogfile=/tmp/go-build1833741381/b496/testlog.txt -test.paniconexit0 -test.timeout=10m0s /tmp/go-build1833741381/b394/vet.cfg @v1.1.3/cpu/arm/-errorsas -trimpath x_amd64/vet -p nal/encoding/tex-atomic -lang=go1.25 x_amd64/vet -I g_.a -I x_amd64/vet --gdwarf-5 64 -o x_amd64/vet` (dns block) > - `nonexistent.local` > - Triggering command: `/tmp/go-build1833741381/b514/launcher.test /tmp/go-build1833741381/b514/launcher.test -test.testlogfile=/tmp/go-build1833741381/b514/testlog.txt -test.paniconexit0 -test.timeout=10m0s -W .cfg olang.org/grpc@v-ifaceassert x_amd64/vet . --gdwarf2 --64 x_amd64/vet .cfg�� 2245169/b376/_pkg_.a -I x_amd64/vet --gdwarf-5 g/grpc/internal//usr/bin/runc -o x_amd64/vet` (dns block) > - `slow.example.com` > - Triggering command: `/tmp/go-build1833741381/b514/launcher.test /tmp/go-build1833741381/b514/launcher.test -test.testlogfile=/tmp/go-build1833741381/b514/testlog.txt -test.paniconexit0 -test.timeout=10m0s -W .cfg olang.org/grpc@v-ifaceassert x_amd64/vet . --gdwarf2 --64 x_amd64/vet .cfg�� 2245169/b376/_pkg_.a -I x_amd64/vet --gdwarf-5 g/grpc/internal//usr/bin/runc -o x_amd64/vet` (dns block) > - `this-host-does-not-exist-12345.com` > - Triggering command: `/tmp/go-build1833741381/b523/mcp.test /tmp/go-build1833741381/b523/mcp.test -test.testlogfile=/tmp/go-build1833741381/b523/testlog.txt -test.paniconexit0 -test.timeout=10m0s 2245�� .cfg ache/go/1.25.8/x64/src/database/sql/driver/driver.go x_amd64/vet --gdwarf-5 --64 -o x_amd64/vet .cfg�� /tmp/go-build4122245169/b223/_pk-errorsas uxII/pTQM0yfdla9bshGWuxII x_amd64/vet -p 2245169/b468/ -lang=go1.24 x_amd64/vet` (dns block) > > If you need me to access, download, or install something from one of these locations, you can either: > > - Configure [Actions setup steps](https://gh.io/copilot/actions-setup-steps) to set up my environment, which run before the firewall is enabled > - Add the appropriate URLs or hosts to the custom allowlist in this repository's [Copilot coding agent settings](https://github.com/github/gh-aw-mcpg/settings/copilot/coding_agent) (admins only) > > </details>
2 parents d4f63f8 + 81463c9 commit b0342d8

File tree

3 files changed

+31
-35
lines changed

3 files changed

+31
-35
lines changed

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

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -83,6 +83,19 @@ pub enum MinIntegrity {
8383
Merged,
8484
}
8585

86+
impl MinIntegrity {
87+
/// Returns the canonical policy-facing string for this integrity level.
88+
pub fn as_str(self) -> &'static str {
89+
use super::constants::policy_integrity;
90+
match self {
91+
MinIntegrity::None => policy_integrity::NONE,
92+
MinIntegrity::Unapproved => policy_integrity::UNAPPROVED,
93+
MinIntegrity::Approved => policy_integrity::APPROVED,
94+
MinIntegrity::Merged => policy_integrity::MERGED,
95+
}
96+
}
97+
}
98+
8699
#[derive(Debug, Clone, Default)]
87100
pub struct PolicyContext {
88101
pub scopes: Vec<PolicyScopeEntry>,
@@ -1427,4 +1440,13 @@ mod tests {
14271440
let assoc_result = author_association_floor_from_str("owner/repo", Some("CONTRIBUTOR"), &ctx);
14281441
assert_eq!(perm_result, assoc_result, "read permission and CONTRIBUTOR association should produce same integrity");
14291442
}
1443+
1444+
#[test]
1445+
fn test_min_integrity_as_str() {
1446+
use super::super::constants::policy_integrity;
1447+
assert_eq!(MinIntegrity::None.as_str(), policy_integrity::NONE);
1448+
assert_eq!(MinIntegrity::Unapproved.as_str(), policy_integrity::UNAPPROVED);
1449+
assert_eq!(MinIntegrity::Approved.as_str(), policy_integrity::APPROVED);
1450+
assert_eq!(MinIntegrity::Merged.as_str(), policy_integrity::MERGED);
1451+
}
14301452
}

guards/github-guard/rust-guard/src/lib.rs

Lines changed: 9 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -531,12 +531,7 @@ pub extern "C" fn label_agent(
531531

532532
let normalized_policy = NormalizedPolicy {
533533
scope_kind: normalized_scope_kind(&scopes),
534-
min_integrity: match integrity_floor {
535-
MinIntegrity::None => policy_integrity::NONE.to_string(),
536-
MinIntegrity::Unapproved => policy_integrity::UNAPPROVED.to_string(),
537-
MinIntegrity::Approved => policy_integrity::APPROVED.to_string(),
538-
MinIntegrity::Merged => policy_integrity::MERGED.to_string(),
539-
},
534+
min_integrity: integrity_floor.as_str().to_string(),
540535
};
541536

542537
let output = LabelAgentOutput {
@@ -619,25 +614,14 @@ pub extern "C" fn label_resource(
619614
// Classify operation with scoped integrity tags
620615
if tools::is_write_operation(&input.tool_name) {
621616
operation = "write";
622-
if tools::is_merge_operation(&input.tool_name) {
623-
// Writer-level baseline for merge operations
624-
if !repo_id.is_empty() {
625-
integrity = labels::writer_integrity(&repo_id, &ctx);
626-
}
627-
} else if tools::is_delete_operation(&input.tool_name) {
628-
// Writer-level baseline
629-
if !repo_id.is_empty() {
630-
integrity = labels::writer_integrity(&repo_id, &ctx);
631-
}
632-
} else if tools::is_update_operation(&input.tool_name)
633-
|| tools::is_create_operation(&input.tool_name)
634-
{
635-
// Contributor level
636-
if !repo_id.is_empty() {
637-
integrity = labels::reader_integrity(&repo_id, &ctx);
638-
}
639-
} else if !repo_id.is_empty() {
640-
integrity = labels::reader_integrity(&repo_id, &ctx);
617+
if !repo_id.is_empty() {
618+
integrity = if tools::is_merge_operation(&input.tool_name)
619+
|| tools::is_delete_operation(&input.tool_name)
620+
{
621+
labels::writer_integrity(&repo_id, &ctx)
622+
} else {
623+
labels::reader_integrity(&repo_id, &ctx)
624+
};
641625
}
642626
}
643627

guards/github-guard/rust-guard/src/tools.rs

Lines changed: 0 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -89,16 +89,6 @@ pub fn is_delete_operation(tool_name: &str) -> bool {
8989
tool_name.starts_with("delete_")
9090
}
9191

92-
/// Check if a tool is an update operation
93-
pub fn is_update_operation(tool_name: &str) -> bool {
94-
tool_name.starts_with("update_")
95-
}
96-
97-
/// Check if a tool is a create operation
98-
pub fn is_create_operation(tool_name: &str) -> bool {
99-
tool_name.starts_with("create_")
100-
}
101-
10292
/// Check if a tool is a lock operation
10393
pub fn is_lock_operation(tool_name: &str) -> bool {
10494
tool_name.starts_with("lock_")

0 commit comments

Comments
 (0)