Skip to content

Commit d64e882

Browse files
authored
refactor(rust-guard): replace reaction_kind: &str with ReactionKind enum; collapse issue_dependency_read double guard (#8279)
Two type-safety and duplication improvements to the Rust Guard. ### `ReactionKind` enum (`helpers.rs`) `has_maintainer_reaction_with_callback` previously accepted `reaction_kind: &str` constrained only by a comment, leaving a dead `_ => false` arm that would silently swallow any typo at a future call site. **Before:** ```rust let already_warned = match reaction_kind { "endorsement" => ENDORSEMENT_GATEWAY_WARNING_EMITTED.swap(true, Ordering::Relaxed), "disapproval" => DISAPPROVAL_GATEWAY_WARNING_EMITTED.swap(true, Ordering::Relaxed), _ => false, // dead arm }; ``` **After:** ```rust enum ReactionKind { Endorsement, Disapproval } impl ReactionKind { fn as_str(self) -> &'static str { ... } fn warning_emitted(self) -> &'static AtomicBool { ... } } // In has_maintainer_reaction_with_callback: let already_warned = reaction_kind.warning_emitted().swap(true, Ordering::Relaxed); ``` All 18 call sites updated (`has_maintainer_endorsement`, `has_maintainer_disapproval`, and 16 test sites). ### Collapse double guard in `issue_dependency_read` (`tool_rules.rs`) The arm checked `!owner.is_empty() && !repo.is_empty()` and called `extract_number_as_string` twice. The second block is now nested inside the first, removing the redundant guard and allocation.
2 parents 9260fe8 + 507e803 commit d64e882

2 files changed

Lines changed: 62 additions & 41 deletions

File tree

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

Lines changed: 61 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,29 @@ static ENDORSEMENT_GATEWAY_WARNING_EMITTED: AtomicBool = AtomicBool::new(false);
1717
/// Ensures the disapproval gateway-mode warning is emitted at most once per process lifetime.
1818
static DISAPPROVAL_GATEWAY_WARNING_EMITTED: AtomicBool = AtomicBool::new(false);
1919

20+
/// Identifies which type of maintainer reaction is being evaluated.
21+
#[derive(Clone, Copy)]
22+
pub(crate) enum ReactionKind {
23+
Endorsement,
24+
Disapproval,
25+
}
26+
27+
impl ReactionKind {
28+
fn as_str(self) -> &'static str {
29+
match self {
30+
ReactionKind::Endorsement => "endorsement",
31+
ReactionKind::Disapproval => "disapproval",
32+
}
33+
}
34+
35+
fn warning_emitted(self) -> &'static AtomicBool {
36+
match self {
37+
ReactionKind::Endorsement => &ENDORSEMENT_GATEWAY_WARNING_EMITTED,
38+
ReactionKind::Disapproval => &DISAPPROVAL_GATEWAY_WARNING_EMITTED,
39+
}
40+
}
41+
}
42+
2043
/// Extract a resource number from a JSON item, returning the number as a string.
2144
/// Checks the `number` field first, then falls back to extracting the trailing
2245
/// number segment from `html_url` or `url` (e.g. `.../issues/123` → `123`).
@@ -649,7 +672,7 @@ pub fn has_maintainer_reaction_with_callback(
649672
endorser_min: &str,
650673
ctx: &PolicyContext,
651674
callback: GithubMcpCallback,
652-
reaction_kind: &str, // "endorsement" or "disapproval" — used for log messages
675+
reaction_kind: ReactionKind,
653676
) -> bool {
654677
if reaction_list.is_empty() {
655678
return false;
@@ -673,20 +696,14 @@ pub fn has_maintainer_reaction_with_callback(
673696
// gateway mode: reaction counts are available but reactor identity is not.
674697
if item.get("reactions").is_some() {
675698
// Use reaction-kind-specific flags so each kind logs its own warning once.
676-
let already_warned = match reaction_kind {
677-
"endorsement" => {
678-
ENDORSEMENT_GATEWAY_WARNING_EMITTED.swap(true, Ordering::Relaxed)
679-
}
680-
"disapproval" => {
681-
DISAPPROVAL_GATEWAY_WARNING_EMITTED.swap(true, Ordering::Relaxed)
682-
}
683-
_ => false,
684-
};
699+
let already_warned =
700+
reaction_kind.warning_emitted().swap(true, Ordering::Relaxed);
685701
if !already_warned {
686702
crate::log_warn(&format!(
687703
"[integrity] {}: {}-reactions configured but reactor identity unavailable \
688704
(gateway mode) — ignoring reactions for integrity evaluation",
689-
repo_full_name, reaction_kind
705+
repo_full_name,
706+
reaction_kind.as_str()
690707
));
691708
}
692709
}
@@ -739,7 +756,12 @@ pub fn has_maintainer_reaction_with_callback(
739756
crate::log_debug(&format!(
740757
"[integrity] {}: skipping stale {} reaction {} from @{} \
741758
(item updatedAt={} > reaction createdAt={})",
742-
repo_full_name, reaction_kind, content, login, item_updated, reaction_created
759+
repo_full_name,
760+
reaction_kind.as_str(),
761+
content,
762+
login,
763+
item_updated,
764+
reaction_created
743765
));
744766
continue;
745767
}
@@ -765,15 +787,20 @@ pub fn has_maintainer_reaction_with_callback(
765787
perm.as_ref().and_then(|p| p.permission.as_deref()),
766788
reactor_rank,
767789
endorser_min_rank,
768-
reaction_kind,
790+
reaction_kind.as_str(),
769791
content
770792
));
771793
return true;
772794
} else {
773795
crate::log_info(&format!(
774796
"[integrity] {}: reactor @{} has integrity rank {}, below \
775797
endorser-min-integrity rank {} — ignoring {} {}",
776-
repo_full_name, login, reactor_rank, endorser_min_rank, reaction_kind, content
798+
repo_full_name,
799+
login,
800+
reactor_rank,
801+
endorser_min_rank,
802+
reaction_kind.as_str(),
803+
content
777804
));
778805
}
779806
}
@@ -793,7 +820,7 @@ pub fn has_maintainer_endorsement(item: &Value, repo_full_name: &str, ctx: &Poli
793820
effective_endorser_min_integrity(ctx),
794821
ctx,
795822
crate::invoke_backend,
796-
"endorsement",
823+
ReactionKind::Endorsement,
797824
)
798825
}
799826

@@ -809,7 +836,7 @@ pub fn has_maintainer_disapproval(item: &Value, repo_full_name: &str, ctx: &Poli
809836
effective_endorser_min_integrity(ctx),
810837
ctx,
811838
crate::invoke_backend,
812-
"disapproval",
839+
ReactionKind::Disapproval,
813840
)
814841
}
815842

@@ -2632,7 +2659,7 @@ mod tests {
26322659
"approved",
26332660
&ctx,
26342661
admin_permission_callback,
2635-
"endorsement"
2662+
ReactionKind::Endorsement
26362663
));
26372664
}
26382665

@@ -2650,7 +2677,7 @@ mod tests {
26502677
"approved",
26512678
&ctx,
26522679
admin_permission_callback,
2653-
"endorsement"
2680+
ReactionKind::Endorsement
26542681
));
26552682
}
26562683

@@ -2669,7 +2696,7 @@ mod tests {
26692696
"approved",
26702697
&ctx,
26712698
read_permission_callback,
2672-
"endorsement"
2699+
ReactionKind::Endorsement
26732700
));
26742701
}
26752702

@@ -2688,7 +2715,7 @@ mod tests {
26882715
"approved",
26892716
&ctx,
26902717
admin_permission_callback,
2691-
"endorsement"
2718+
ReactionKind::Endorsement
26922719
));
26932720
}
26942721

@@ -2707,7 +2734,7 @@ mod tests {
27072734
"approved",
27082735
&ctx,
27092736
admin_permission_callback,
2710-
"endorsement"
2737+
ReactionKind::Endorsement
27112738
));
27122739
}
27132740

@@ -2727,7 +2754,7 @@ mod tests {
27272754
"approved",
27282755
&ctx,
27292756
admin_permission_callback,
2730-
"endorsement"
2757+
ReactionKind::Endorsement
27312758
));
27322759
}
27332760

@@ -2743,7 +2770,7 @@ mod tests {
27432770
"approved",
27442771
&ctx,
27452772
admin_permission_callback,
2746-
"endorsement"
2773+
ReactionKind::Endorsement
27472774
));
27482775
}
27492776

@@ -2758,7 +2785,7 @@ mod tests {
27582785
"approved",
27592786
&ctx,
27602787
admin_permission_callback,
2761-
"endorsement"
2788+
ReactionKind::Endorsement
27622789
));
27632790
}
27642791

@@ -2779,7 +2806,7 @@ mod tests {
27792806
"approved",
27802807
&ctx,
27812808
error_callback,
2782-
"endorsement"
2809+
ReactionKind::Endorsement
27832810
));
27842811
}
27852812

@@ -2802,7 +2829,7 @@ mod tests {
28022829
"approved",
28032830
&ctx,
28042831
admin_permission_callback,
2805-
"endorsement"
2832+
ReactionKind::Endorsement
28062833
));
28072834
}
28082835

@@ -2825,7 +2852,7 @@ mod tests {
28252852
"approved",
28262853
&ctx,
28272854
admin_permission_callback,
2828-
"endorsement"
2855+
ReactionKind::Endorsement
28292856
));
28302857
}
28312858

@@ -2848,7 +2875,7 @@ mod tests {
28482875
"approved",
28492876
&ctx,
28502877
admin_permission_callback,
2851-
"endorsement"
2878+
ReactionKind::Endorsement
28522879
));
28532880
}
28542881

@@ -2878,7 +2905,7 @@ mod tests {
28782905
"approved",
28792906
&ctx,
28802907
admin_permission_callback,
2881-
"endorsement"
2908+
ReactionKind::Endorsement
28822909
));
28832910
}
28842911

@@ -2899,7 +2926,7 @@ mod tests {
28992926
"approved",
29002927
&ctx,
29012928
admin_permission_callback,
2902-
"endorsement"
2929+
ReactionKind::Endorsement
29032930
));
29042931
}
29052932

@@ -3106,7 +3133,7 @@ mod tests {
31063133
"approved",
31073134
&ctx,
31083135
admin_permission_callback,
3109-
"endorsement"
3136+
ReactionKind::Endorsement
31103137
));
31113138
assert!(has_maintainer_reaction_with_callback(
31123139
&item,
@@ -3115,7 +3142,7 @@ mod tests {
31153142
"approved",
31163143
&ctx,
31173144
admin_permission_callback,
3118-
"disapproval"
3145+
ReactionKind::Disapproval
31193146
));
31203147

31213148
// Simulate the integrity chain: start with none (external contributor),
@@ -3155,7 +3182,7 @@ mod tests {
31553182
"approved",
31563183
&ctx,
31573184
admin_permission_callback,
3158-
"disapproval"
3185+
ReactionKind::Disapproval
31593186
));
31603187
}
31613188

@@ -3177,7 +3204,7 @@ mod tests {
31773204
"approved",
31783205
&ctx,
31793206
read_permission_callback,
3180-
"disapproval"
3207+
ReactionKind::Disapproval
31813208
));
31823209
}
31833210

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

Lines changed: 1 addition & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -173,20 +173,14 @@ pub fn apply_tool_labels(
173173
}
174174

175175
"issue_dependency_read" => {
176-
if !owner.is_empty() && !repo.is_empty() {
177-
if let Some(issue_num) =
178-
extract_number_as_string(tool_args, field_names::ISSUE_NUMBER)
179-
{
180-
desc = format!("issue:{}/{}#{}", owner, repo, issue_num);
181-
}
182-
}
183176
secrecy = apply_repo_visibility_secrecy(&owner, &repo, repo_id, secrecy, ctx);
184177
integrity = private_writer_integrity(repo_id, repo_private, ctx);
185178

186179
if !owner.is_empty() && !repo.is_empty() {
187180
if let Some(issue_num) =
188181
extract_number_as_string(tool_args, field_names::ISSUE_NUMBER)
189182
{
183+
desc = format!("issue:{}/{}#{}", owner, repo, &issue_num);
190184
if let Some(info) =
191185
super::backend::get_issue_author_info(&owner, &repo, &issue_num)
192186
{

0 commit comments

Comments
 (0)