🦀 Rust Guard Improvement Report
Improvement 1: Redundant match arm in author_association_floor_from_str
Category: Dead Code
File(s): guards/github-guard/rust-guard/src/labels/helpers.rs
Effort: Small (< 15 min)
Risk: Low
Problem
Lines 907–908 contain two consecutive match arms that both return vec![]. The "FIRST_TIMER" | "NONE" arm is entirely subsumed by the wildcard _ arm — it is dead code, never distinguished from the catch-all:
// helpers.rs line 904
match normalized.as_str() {
"OWNER" | "MEMBER" | "COLLABORATOR" => writer_integrity(scope, ctx),
"CONTRIBUTOR" | "FIRST_TIME_CONTRIBUTOR" => reader_integrity(scope, ctx),
"FIRST_TIMER" | "NONE" => vec![], // ← identical to the arm below
_ => vec![], // ← wildcard already covers these
}
The doc comment already documents the intended mapping, so the explicit arms add no informational value over a single wildcard.
Suggested Change
Remove the "FIRST_TIMER" | "NONE" arm and consolidate the no-op cases into the wildcard:
Before
match normalized.as_str() {
"OWNER" | "MEMBER" | "COLLABORATOR" => writer_integrity(scope, ctx),
"CONTRIBUTOR" | "FIRST_TIME_CONTRIBUTOR" => reader_integrity(scope, ctx),
"FIRST_TIMER" | "NONE" => vec![],
_ => vec![],
}
After
match normalized.as_str() {
"OWNER" | "MEMBER" | "COLLABORATOR" => writer_integrity(scope, ctx),
"CONTRIBUTOR" | "FIRST_TIME_CONTRIBUTOR" => reader_integrity(scope, ctx),
_ => vec![], // FIRST_TIMER, NONE, or any unrecognised value
}
Why This Matters
Dead match arms are misleading — a reader may wonder whether "FIRST_TIMER" and "NONE" are intended to be treated differently from unknown values. Removing them signals clearly that all remaining cases are equivalent, and the doc comment already documents the intended semantics. Rust's exhaustive match means no behaviour change whatsoever.
Improvement 2: Duplicate integrity-assignment branches in label_resource
Category: Code Duplication
File(s): guards/github-guard/rust-guard/src/lib.rs
Effort: Small (< 15 min)
Risk: Low
Problem
Lines 620–641 contain three separate if !repo_id.is_empty() guards with duplicated assignment bodies inside the is_write_operation block:
is_merge_operation → writer_integrity (lines 622–626)
is_delete_operation → writer_integrity (lines 627–631) ← identical body to case 1
is_update_operation || is_create_operation → reader_integrity (lines 632–638)
- final
else → reader_integrity (lines 639–641) ← identical body to case 3
if tools::is_write_operation(&input.tool_name) {
operation = "write";
if tools::is_merge_operation(&input.tool_name) {
// Writer-level baseline for merge operations
if !repo_id.is_empty() {
integrity = labels::writer_integrity(&repo_id, &ctx);
}
} else if tools::is_delete_operation(&input.tool_name) {
// Writer-level baseline ← same comment, same body
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)
{
// Contributor level
if !repo_id.is_empty() {
integrity = labels::reader_integrity(&repo_id, &ctx);
}
} else if !repo_id.is_empty() { ← same body as above, different guard
integrity = labels::reader_integrity(&repo_id, &ctx);
}
}
Suggested Change
Merge the two writer_integrity branches and collapse the two reader_integrity branches:
Before
if tools::is_write_operation(&input.tool_name) {
operation = "write";
if tools::is_merge_operation(&input.tool_name) {
// Writer-level baseline for merge operations
if !repo_id.is_empty() {
integrity = labels::writer_integrity(&repo_id, &ctx);
}
} else if tools::is_delete_operation(&input.tool_name) {
// Writer-level baseline
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)
{
// Contributor level
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 tools::is_write_operation(&input.tool_name) {
operation = "write";
if tools::is_merge_operation(&input.tool_name)
|| tools::is_delete_operation(&input.tool_name)
{
// Writer-level baseline for merge and delete operations
if !repo_id.is_empty() {
integrity = labels::writer_integrity(&repo_id, &ctx);
}
} else if !repo_id.is_empty() {
// Contributor level for update, create, and other write operations
integrity = labels::reader_integrity(&repo_id, &ctx);
}
}
Why This Matters
The current structure implies that is_update_operation || is_create_operation and the catch-all else case behave differently, when they don't. The refactored version makes the two-tier policy (merge/delete → writer; everything else → reader) immediately obvious, reducing the cognitive load on reviewers and removing the risk that a future editor adds a third handler between the two identical arms without noticing they were duplicates.
Codebase Health Summary
- Total Rust files: 9
- Total lines: 10,927
- Areas analyzed:
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
- Areas with no further improvements: (none yet — first run)
Generated by Rust Guard Improver • Run: §24183235827
Generated by Rust Guard Improver · ● 1.2M · ◷
🦀 Rust Guard Improvement Report
Improvement 1: Redundant match arm in
author_association_floor_from_strCategory: Dead Code
File(s):
guards/github-guard/rust-guard/src/labels/helpers.rsEffort: Small (< 15 min)
Risk: Low
Problem
Lines 907–908 contain two consecutive match arms that both return
vec![]. The"FIRST_TIMER" | "NONE"arm is entirely subsumed by the wildcard_arm — it is dead code, never distinguished from the catch-all:The doc comment already documents the intended mapping, so the explicit arms add no informational value over a single wildcard.
Suggested Change
Remove the
"FIRST_TIMER" | "NONE"arm and consolidate the no-op cases into the wildcard:Before
After
Why This Matters
Dead match arms are misleading — a reader may wonder whether
"FIRST_TIMER"and"NONE"are intended to be treated differently from unknown values. Removing them signals clearly that all remaining cases are equivalent, and the doc comment already documents the intended semantics. Rust's exhaustive match means no behaviour change whatsoever.Improvement 2: Duplicate integrity-assignment branches in
label_resourceCategory: Code Duplication
File(s):
guards/github-guard/rust-guard/src/lib.rsEffort: Small (< 15 min)
Risk: Low
Problem
Lines 620–641 contain three separate
if !repo_id.is_empty()guards with duplicated assignment bodies inside theis_write_operationblock:is_merge_operation→writer_integrity(lines 622–626)is_delete_operation→writer_integrity(lines 627–631) ← identical body to case 1is_update_operation || is_create_operation→reader_integrity(lines 632–638)else→reader_integrity(lines 639–641) ← identical body to case 3Suggested Change
Merge the two
writer_integritybranches and collapse the tworeader_integritybranches:Before
After
Why This Matters
The current structure implies that
is_update_operation || is_create_operationand the catch-allelsecase behave differently, when they don't. The refactored version makes the two-tier policy (merge/delete → writer; everything else → reader) immediately obvious, reducing the cognitive load on reviewers and removing the risk that a future editor adds a third handler between the two identical arms without noticing they were duplicates.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.rsGenerated by Rust Guard Improver • Run: §24183235827