Improvement 1: Extract first_matching_label from duplicated approval/refusal functions
Category: Code Duplication
File(s): guards/github-guard/rust-guard/src/labels/helpers.rs
Effort: Small (< 15 min)
Risk: Low
Problem
first_matching_approval_label (line ~391) and first_matching_refusal_label (line ~411) are near-identical private functions. The only differences are which ctx field they access (approval_labels vs refusal_labels) and the variable name in the closure (al vs rl):
fn first_matching_approval_label<'a>(item: &'a Value, ctx: &PolicyContext) -> Option<&'a str> {
if ctx.approval_labels.is_empty() {
return None;
}
let label_names = extract_github_label_names(item);
label_names.into_iter().find(|name| {
ctx.approval_labels
.iter()
.any(|al| al.eq_ignore_ascii_case(name))
})
}
fn first_matching_refusal_label<'a>(item: &'a Value, ctx: &PolicyContext) -> Option<&'a str> {
if ctx.refusal_labels.is_empty() {
return None;
}
let label_names = extract_github_label_names(item);
label_names.into_iter().find(|name| {
ctx.refusal_labels
.iter()
.any(|rl| rl.eq_ignore_ascii_case(name))
})
}
Suggested Change
Extract a shared private helper first_matching_label that takes the list directly, then delegate both functions to it:
Before
fn first_matching_approval_label<'a>(item: &'a Value, ctx: &PolicyContext) -> Option<&'a str> {
if ctx.approval_labels.is_empty() {
return None;
}
let label_names = extract_github_label_names(item);
label_names.into_iter().find(|name| {
ctx.approval_labels
.iter()
.any(|al| al.eq_ignore_ascii_case(name))
})
}
fn first_matching_refusal_label<'a>(item: &'a Value, ctx: &PolicyContext) -> Option<&'a str> {
if ctx.refusal_labels.is_empty() {
return None;
}
let label_names = extract_github_label_names(item);
label_names.into_iter().find(|name| {
ctx.refusal_labels
.iter()
.any(|rl| rl.eq_ignore_ascii_case(name))
})
}
After
/// Return the first item label that matches any entry in `label_list` (case-insensitive).
/// Returns `None` immediately when `label_list` is empty (feature disabled).
fn first_matching_label<'a>(item: &'a Value, label_list: &[String]) -> Option<&'a str> {
if label_list.is_empty() {
return None;
}
extract_github_label_names(item)
.into_iter()
.find(|name| label_list.iter().any(|l| l.eq_ignore_ascii_case(name)))
}
fn first_matching_approval_label<'a>(item: &'a Value, ctx: &PolicyContext) -> Option<&'a str> {
first_matching_label(item, &ctx.approval_labels)
}
fn first_matching_refusal_label<'a>(item: &'a Value, ctx: &PolicyContext) -> Option<&'a str> {
first_matching_label(item, &ctx.refusal_labels)
}
Why This Matters
- Eliminates 12 lines of copied logic — any future change to matching semantics only needs to happen once.
- Makes the symmetry between approval and refusal label handling visually obvious.
first_matching_label is also reusable if additional label-list fields are added to PolicyContext.
Improvement 2: Add unit tests for label-checking helper functions
Category: Test Coverage
File(s): guards/github-guard/rust-guard/src/labels/helpers.rs
Effort: Small (< 15 min)
Risk: Low
Problem
Four pub functions are exposed in #[cfg(test)] mode and used by apply_post_integrity_adjustments, but have zero direct unit tests:
has_approval_label (~line 385) — wraps first_matching_approval_label
has_refusal_label (~line 406) — wraps first_matching_refusal_label
has_promotion_label (~line 492) — wraps item_has_config_label for promotion_label
has_demotion_label (~line 498) — wraps item_has_config_label for demotion_label
The case-insensitive matching, empty-list early-return, and missing-labels-field behaviors are untested in isolation.
Suggested Change
Add the following test block to the tests module in helpers.rs:
Before
(no tests for these functions)
After
// =========================================================================
// Tests for label-checking helpers
// =========================================================================
#[test]
fn test_has_approval_label_matches_case_insensitively() {
let ctx = PolicyContext {
approval_labels: vec!["approved".to_string()],
..Default::default()
};
let item = serde_json::json!({"labels": [{"name": "APPROVED"}]});
assert!(has_approval_label(&item, &ctx));
}
#[test]
fn test_has_approval_label_no_match_returns_false() {
let ctx = PolicyContext {
approval_labels: vec!["approved".to_string()],
..Default::default()
};
let item = serde_json::json!({"labels": [{"name": "other-label"}]});
assert!(!has_approval_label(&item, &ctx));
}
#[test]
fn test_has_approval_label_empty_list_returns_false() {
let ctx = PolicyContext::default(); // approval_labels is empty
let item = serde_json::json!({"labels": [{"name": "approved"}]});
assert!(!has_approval_label(&item, &ctx));
}
#[test]
fn test_has_approval_label_no_labels_field_returns_false() {
let ctx = PolicyContext {
approval_labels: vec!["approved".to_string()],
..Default::default()
};
let item = serde_json::json!({"number": 1, "title": "no labels field"});
assert!(!has_approval_label(&item, &ctx));
}
#[test]
fn test_has_refusal_label_matches_case_insensitively() {
let ctx = PolicyContext {
refusal_labels: vec!["spam".to_string()],
..Default::default()
};
let item = serde_json::json!({"labels": [{"name": "SPAM"}]});
assert!(has_refusal_label(&item, &ctx));
}
#[test]
fn test_has_refusal_label_empty_list_returns_false() {
let ctx = PolicyContext::default();
let item = serde_json::json!({"labels": [{"name": "spam"}]});
assert!(!has_refusal_label(&item, &ctx));
}
#[test]
fn test_has_promotion_label_matches_case_insensitively() {
let ctx = PolicyContext {
promotion_label: "trusted".to_string(),
..Default::default()
};
let item = serde_json::json!({"labels": [{"name": "TRUSTED"}]});
assert!(has_promotion_label(&item, &ctx));
}
#[test]
fn test_has_promotion_label_empty_string_returns_false() {
let ctx = PolicyContext::default(); // promotion_label is ""
let item = serde_json::json!({"labels": [{"name": "trusted"}]});
assert!(!has_promotion_label(&item, &ctx));
}
#[test]
fn test_has_demotion_label_matches_case_insensitively() {
let ctx = PolicyContext {
demotion_label: "needs-review".to_string(),
..Default::default()
};
let item = serde_json::json!({"labels": [{"name": "NEEDS-REVIEW"}]});
assert!(has_demotion_label(&item, &ctx));
}
#[test]
fn test_has_demotion_label_empty_string_returns_false() {
let ctx = PolicyContext::default(); // demotion_label is ""
let item = serde_json::json!({"labels": [{"name": "needs-review"}]});
assert!(!has_demotion_label(&item, &ctx));
}
Why This Matters
- Pins the case-insensitive matching behavior so it can't silently regress.
- Explicitly documents the "empty list = feature disabled" contract for all four helpers.
- Follows the pattern of other recently-added unit tests (
commit_integrity, is_default_branch_ref, etc.).
Codebase Health Summary
- Total Rust files: 9 (
lib.rs, tools.rs, labels/mod.rs, labels/backend.rs, labels/constants.rs, labels/helpers.rs, labels/response_items.rs, labels/response_paths.rs, labels/tool_rules.rs)
- Total lines: ~17,321
- Areas analyzed: All source files
- Areas with no further improvements:
labels/constants.rs, labels/mod.rs (thin wrappers / re-exports)
Generated by Rust Guard Improver • Run: §28091085981
Generated by Rust Guard Improver · 499.5 AIC · ⊞ 34.1K · ◷
Improvement 1: Extract
first_matching_labelfrom duplicated approval/refusal functionsCategory: Code Duplication
File(s):
guards/github-guard/rust-guard/src/labels/helpers.rsEffort: Small (< 15 min)
Risk: Low
Problem
first_matching_approval_label(line ~391) andfirst_matching_refusal_label(line ~411) are near-identical private functions. The only differences are whichctxfield they access (approval_labelsvsrefusal_labels) and the variable name in the closure (alvsrl):Suggested Change
Extract a shared private helper
first_matching_labelthat takes the list directly, then delegate both functions to it:Before
After
Why This Matters
first_matching_labelis also reusable if additional label-list fields are added toPolicyContext.Improvement 2: Add unit tests for label-checking helper functions
Category: Test Coverage
File(s):
guards/github-guard/rust-guard/src/labels/helpers.rsEffort: Small (< 15 min)
Risk: Low
Problem
Four
pubfunctions are exposed in#[cfg(test)]mode and used byapply_post_integrity_adjustments, but have zero direct unit tests:has_approval_label(~line 385) — wrapsfirst_matching_approval_labelhas_refusal_label(~line 406) — wrapsfirst_matching_refusal_labelhas_promotion_label(~line 492) — wrapsitem_has_config_labelforpromotion_labelhas_demotion_label(~line 498) — wrapsitem_has_config_labelfordemotion_labelThe case-insensitive matching, empty-list early-return, and missing-labels-field behaviors are untested in isolation.
Suggested Change
Add the following test block to the
testsmodule inhelpers.rs:Before
(no tests for these functions)
After
Why This Matters
commit_integrity,is_default_branch_ref, etc.).Codebase Health Summary
lib.rs,tools.rs,labels/mod.rs,labels/backend.rs,labels/constants.rs,labels/helpers.rs,labels/response_items.rs,labels/response_paths.rs,labels/tool_rules.rs)labels/constants.rs,labels/mod.rs(thin wrappers / re-exports)Generated by Rust Guard Improver • Run: §28091085981