Improvement 1: Extract resolve_integrity_level from duplicated pair
Category: Duplication
File(s): guards/github-guard/rust-guard/src/labels/helpers.rs
Effort: Small (< 15 min)
Risk: Low
Problem
effective_disapproval_integrity (lines 581–596) and effective_endorser_min_integrity (lines 598–614) share a nearly identical 14-line body. The only differences are the field read from ctx, the default MinIntegrity variant, and the caller name used in the warning log.
Before
fn effective_disapproval_integrity(ctx: &PolicyContext) -> &'static str {
let v = ctx.disapproval_integrity.trim();
if v.is_empty() {
policy_integrity::NONE
} else {
MinIntegrity::from_policy_str(v)
.unwrap_or_else(|| {
crate::log_warn(&format!(
"effective_disapproval_integrity: unrecognised level {:?}, defaulting to 'none'",
v
));
MinIntegrity::None
})
.as_str()
}
}
fn effective_endorser_min_integrity(ctx: &PolicyContext) -> &'static str {
let v = ctx.endorser_min_integrity.trim();
if v.is_empty() {
policy_integrity::APPROVED
} else {
MinIntegrity::from_policy_str(v)
.unwrap_or_else(|| {
crate::log_warn(&format!(
"effective_endorser_min_integrity: unrecognised level {:?}, defaulting to 'approved'",
v
));
MinIntegrity::Approved
})
.as_str()
}
}
After
/// Parse a policy integrity level from a config string, returning `default` when empty or
/// unrecognised, and emitting a single log warning on unrecognised values.
fn resolve_integrity_level(value: &str, default: MinIntegrity, caller: &str) -> &'static str {
let v = value.trim();
if v.is_empty() {
default.as_str()
} else {
MinIntegrity::from_policy_str(v)
.unwrap_or_else(|| {
crate::log_warn(&format!(
"{}: unrecognised level {:?}, defaulting to '{}'",
caller, v, default.as_str()
));
default
})
.as_str()
}
}
fn effective_disapproval_integrity(ctx: &PolicyContext) -> &'static str {
resolve_integrity_level(&ctx.disapproval_integrity, MinIntegrity::None, "effective_disapproval_integrity")
}
fn effective_endorser_min_integrity(ctx: &PolicyContext) -> &'static str {
resolve_integrity_level(&ctx.endorser_min_integrity, MinIntegrity::Approved, "effective_endorser_min_integrity")
}
Why This Matters
Removes ~14 lines of duplicated parse-and-warn logic. Any future PolicyContext field that needs the same "empty → default, unrecognised → warn + default" pattern (e.g. a hypothetical min_read_integrity) can reuse resolve_integrity_level with zero boilerplate. The existing unit tests (test_effective_disapproval_integrity_*, test_effective_endorser_min_integrity_*) provide full coverage without modification.
Improvement 2: Test promotion/demotion label flow through issue_integrity / pr_integrity
Category: Test Coverage
File(s): guards/github-guard/rust-guard/src/labels/helpers.rs
Effort: Small (< 15 min)
Risk: Low
Problem
has_promotion_label and has_demotion_label each have unit tests that check the predicate in isolation, but there are no tests that exercise the full apply_post_integrity_adjustments pipeline via issue_integrity or pr_integrity. The built-in promotion_label / demotion_label features could regress silently — for example if apply_promotion_label_promotion or apply_demotion_label_demotion were accidentally removed from the apply_post_integrity_adjustments call chain.
Suggested Change
Add four tests inside the existing #[cfg(test)] mod tests block in helpers.rs, after the current test_pr_integrity_* group:
#[test]
fn test_issue_integrity_promotion_label_promotes_to_writer() {
let ctx = PolicyContext {
promotion_label: "trusted".to_string(),
..Default::default()
};
let item = serde_json::json!({
"number": 100,
"user": { "login": "external" },
"author_association": "NONE",
"labels": [{ "name": "trusted" }]
});
let result = issue_integrity(&item, "owner/repo", false, &ctx);
assert_eq!(
integrity_rank("owner/repo", &result, &ctx), 3,
"promotion_label should raise NONE-association issue to writer integrity"
);
}
#[test]
fn test_issue_integrity_demotion_label_caps_to_none() {
let ctx = PolicyContext {
demotion_label: "untrusted".to_string(),
..Default::default()
};
let item = serde_json::json!({
"number": 101,
"user": { "login": "owner" },
"author_association": "OWNER",
"labels": [{ "name": "untrusted" }]
});
let result = issue_integrity(&item, "owner/repo", false, &ctx);
assert_eq!(
integrity_rank("owner/repo", &result, &ctx), 1,
"demotion_label should cap OWNER-association issue to none integrity"
);
}
#[test]
fn test_pr_integrity_promotion_label_raises_forked_pr_to_writer() {
let ctx = PolicyContext {
promotion_label: "trusted".to_string(),
..Default::default()
};
let item = serde_json::json!({
"number": 102,
"user": { "login": "external" },
"author_association": "NONE",
"labels": [{ "name": "trusted" }]
});
// Forked PR from external contributor normally gets reader (rank 2);
// promotion_label should raise it to writer (rank 3).
let result = pr_integrity(&item, "owner/repo", false, Some(true), &ctx);
assert_eq!(
integrity_rank("owner/repo", &result, &ctx), 3,
"promotion_label should raise forked PR from reader to writer integrity"
);
}
#[test]
fn test_pr_integrity_demotion_label_caps_merged_pr_to_none() {
let ctx = PolicyContext {
demotion_label: "reverted".to_string(),
..Default::default()
};
let item = serde_json::json!({
"number": 103,
"user": { "login": "owner" },
"author_association": "OWNER",
"merged_at": "2024-06-01T12:00:00Z",
"labels": [{ "name": "reverted" }]
});
// Merged PR would normally be rank 4; demotion_label should cap it to none (rank 1).
let result = pr_integrity(&item, "owner/repo", false, Some(false), &ctx);
assert_eq!(
integrity_rank("owner/repo", &result, &ctx), 1,
"demotion_label should cap merged PR to none integrity"
);
}
Why This Matters
These four tests close a gap where an accidental removal of apply_promotion_label_promotion or apply_demotion_label_demotion from apply_post_integrity_adjustments would go undetected. They also serve as clear documentation of the expected precedence (demotion_label beats merged_at, promotion_label beats fork-from-external-contributor).
Codebase Health Summary
- Total Rust files: 9
- Total lines: ~17,920 (including tests)
- Areas analyzed:
lib.rs, labels/helpers.rs, labels/backend.rs, labels/response_items.rs, labels/response_paths.rs, labels/tool_rules.rs, labels/constants.rs, labels/mod.rs, tools.rs
- Areas with no further improvements:
labels/constants.rs, tools.rs (well-structured, minimal duplication)
Generated by Rust Guard Improver • Run: §28581952405
Generated by Rust Guard Improver · 110.5 AIC · ⊞ 7.2K · ◷
Improvement 1: Extract
resolve_integrity_levelfrom duplicated pairCategory: Duplication
File(s):
guards/github-guard/rust-guard/src/labels/helpers.rsEffort: Small (< 15 min)
Risk: Low
Problem
effective_disapproval_integrity(lines 581–596) andeffective_endorser_min_integrity(lines 598–614) share a nearly identical 14-line body. The only differences are the field read fromctx, the defaultMinIntegrityvariant, and the caller name used in the warning log.Before
After
Why This Matters
Removes ~14 lines of duplicated parse-and-warn logic. Any future
PolicyContextfield that needs the same "empty → default, unrecognised → warn + default" pattern (e.g. a hypotheticalmin_read_integrity) can reuseresolve_integrity_levelwith zero boilerplate. The existing unit tests (test_effective_disapproval_integrity_*,test_effective_endorser_min_integrity_*) provide full coverage without modification.Improvement 2: Test promotion/demotion label flow through
issue_integrity/pr_integrityCategory: Test Coverage
File(s):
guards/github-guard/rust-guard/src/labels/helpers.rsEffort: Small (< 15 min)
Risk: Low
Problem
has_promotion_labelandhas_demotion_labeleach have unit tests that check the predicate in isolation, but there are no tests that exercise the fullapply_post_integrity_adjustmentspipeline viaissue_integrityorpr_integrity. The built-inpromotion_label/demotion_labelfeatures could regress silently — for example ifapply_promotion_label_promotionorapply_demotion_label_demotionwere accidentally removed from theapply_post_integrity_adjustmentscall chain.Suggested Change
Add four tests inside the existing
#[cfg(test)] mod testsblock inhelpers.rs, after the currenttest_pr_integrity_*group:Why This Matters
These four tests close a gap where an accidental removal of
apply_promotion_label_promotionorapply_demotion_label_demotionfromapply_post_integrity_adjustmentswould go undetected. They also serve as clear documentation of the expected precedence (demotion_labelbeatsmerged_at,promotion_labelbeats fork-from-external-contributor).Codebase Health Summary
lib.rs,labels/helpers.rs,labels/backend.rs,labels/response_items.rs,labels/response_paths.rs,labels/tool_rules.rs,labels/constants.rs,labels/mod.rs,tools.rslabels/constants.rs,tools.rs(well-structured, minimal duplication)Generated by Rust Guard Improver • Run: §28581952405