Skip to content

Commit 074f8f0

Browse files
authored
Centralize Rust guard issue_read skip logic and mutation constants (#14903)
Both response labelers duplicated the `issue_read` skip condition, including its `get_comments` exception. Granular mutation tools also repeated raw names across operation lists and labeling rules. - **Shared skip rule:** Extract `skip_issue_read_per_item_labeling` and introduce `GET` / `GET_COMMENTS` constants. Preserve per-item labeling for `get`, `get_comments`, and an empty method. - **Mutation constants:** Centralize granular issue mutation names and `unlock_pull_request`, then reuse them in operation lists and DIFC rules. - **Regression coverage:** Cover skip-rule edge cases and canonical constant values. Both response labelers now use: ```rust if skip_issue_read_per_item_labeling(tool_name, method) { // Use resource-level labels. } ``` No labeling or tool-classification behavior change is intended. <!-- START COPILOT CODING AGENT SUFFIX --> - Fixes #14878
2 parents 019edb2 + 84dfc10 commit 074f8f0

8 files changed

Lines changed: 210 additions & 59 deletions

File tree

‎guards/github-guard/rust-guard/src/labels/constants.rs‎

Lines changed: 46 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -37,9 +37,10 @@ pub mod policy_integrity {
3737
#[cfg(test)]
3838
mod tests {
3939
use super::{
40-
desc_default, desc_prefix, field_names, policy_integrity, tool_names, ORG_FIELD_ALIASES,
41-
SENSITIVE_PATH_PREFIXES, UI_GET_ACCESS_SENSITIVE_METHODS, UI_GET_GITHUB_APPROVED_METHODS,
42-
UI_GET_REPO_SCOPED_METHODS, UNKNOWN_LABEL_FALLBACK, URL_FALLBACK_FIELDS,
40+
desc_default, desc_prefix, field_names, method_values, policy_integrity, tool_names,
41+
ORG_FIELD_ALIASES, SENSITIVE_PATH_PREFIXES, UI_GET_ACCESS_SENSITIVE_METHODS,
42+
UI_GET_GITHUB_APPROVED_METHODS, UI_GET_REPO_SCOPED_METHODS, UNKNOWN_LABEL_FALLBACK,
43+
URL_FALLBACK_FIELDS,
4344
};
4445

4546
/// Ensures ORDER_LOW_TO_HIGH_PIPED stays in sync with ORDER_HIGH_TO_LOW.
@@ -108,6 +109,27 @@ mod tests {
108109
);
109110
}
110111

112+
#[test]
113+
fn method_and_issue_mutation_constants_match_canonical_values() {
114+
assert_eq!(method_values::GET, "get");
115+
assert_eq!(method_values::GET_COMMENTS, "get_comments");
116+
assert_eq!(tool_names::CLOSE_ISSUE, "close_issue");
117+
assert_eq!(tool_names::REOPEN_ISSUE, "reopen_issue");
118+
assert_eq!(tool_names::LOCK_ISSUE, "lock_issue");
119+
assert_eq!(tool_names::UNLOCK_ISSUE, "unlock_issue");
120+
assert_eq!(tool_names::UNLOCK_PULL_REQUEST, "unlock_pull_request");
121+
assert_eq!(tool_names::TRANSFER_ISSUE, "transfer_issue");
122+
assert_eq!(tool_names::UNPIN_ISSUE, "unpin_issue");
123+
assert_eq!(tool_names::UPDATE_ISSUE_ASSIGNEES, "update_issue_assignees");
124+
assert_eq!(tool_names::UPDATE_ISSUE_BODY, "update_issue_body");
125+
assert_eq!(tool_names::UPDATE_ISSUE_COMMENT, "update_issue_comment");
126+
assert_eq!(tool_names::UPDATE_ISSUE_LABELS, "update_issue_labels");
127+
assert_eq!(tool_names::UPDATE_ISSUE_MILESTONE, "update_issue_milestone");
128+
assert_eq!(tool_names::UPDATE_ISSUE_STATE, "update_issue_state");
129+
assert_eq!(tool_names::UPDATE_ISSUE_TITLE, "update_issue_title");
130+
assert_eq!(tool_names::UPDATE_ISSUE_TYPE, "update_issue_type");
131+
}
132+
111133
#[test]
112134
fn dispatch_constants_match_canonical_values() {
113135
assert_eq!(tool_names::ACTIONS_GET, "actions_get");
@@ -288,6 +310,12 @@ pub mod field_names {
288310
pub const TYPE: &str = "type";
289311
}
290312

313+
/// Canonical method values for read-style dispatch tools.
314+
pub mod method_values {
315+
pub const GET: &str = "get";
316+
pub const GET_COMMENTS: &str = "get_comments";
317+
}
318+
291319
/// Fallback identifier used in labels when a response item lacks an expected field.
292320
pub const UNKNOWN_LABEL_FALLBACK: &str = "unknown";
293321

@@ -370,6 +398,21 @@ pub mod tool_names {
370398
pub const GET_PULL_REQUEST: &str = "get_pull_request";
371399
pub const ISSUE_READ: &str = "issue_read";
372400
pub const GET_ISSUE: &str = "get_issue";
401+
pub const CLOSE_ISSUE: &str = "close_issue";
402+
pub const REOPEN_ISSUE: &str = "reopen_issue";
403+
pub const LOCK_ISSUE: &str = "lock_issue";
404+
pub const UNLOCK_ISSUE: &str = "unlock_issue";
405+
pub const UNLOCK_PULL_REQUEST: &str = "unlock_pull_request";
406+
pub const TRANSFER_ISSUE: &str = "transfer_issue";
407+
pub const UNPIN_ISSUE: &str = "unpin_issue";
408+
pub const UPDATE_ISSUE_ASSIGNEES: &str = "update_issue_assignees";
409+
pub const UPDATE_ISSUE_BODY: &str = "update_issue_body";
410+
pub const UPDATE_ISSUE_COMMENT: &str = "update_issue_comment";
411+
pub const UPDATE_ISSUE_LABELS: &str = "update_issue_labels";
412+
pub const UPDATE_ISSUE_MILESTONE: &str = "update_issue_milestone";
413+
pub const UPDATE_ISSUE_STATE: &str = "update_issue_state";
414+
pub const UPDATE_ISSUE_TITLE: &str = "update_issue_title";
415+
pub const UPDATE_ISSUE_TYPE: &str = "update_issue_type";
373416
pub const LIST_PULL_REQUESTS: &str = "list_pull_requests";
374417
pub const LIST_PULL_REQUESTS_FF_FIELDS_PARAM: &str = "list_pull_requests_ff_fields_param";
375418
pub const GET_FILE_CONTENTS: &str = "get_file_contents";

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

Lines changed: 33 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@ use serde_json::Value;
1010

1111
use super::backend::GithubMcpCallback;
1212
use super::constants::{
13-
field_names, label_constants, scope_names, tool_names, UNKNOWN_LABEL_FALLBACK,
13+
field_names, label_constants, method_values, scope_names, tool_names, UNKNOWN_LABEL_FALLBACK,
1414
URL_FALLBACK_FIELDS,
1515
};
1616

@@ -1165,7 +1165,15 @@ pub(crate) fn is_non_get_read_sub_method(
11651165
read_tool_name: &str,
11661166
method: &str,
11671167
) -> bool {
1168-
tool_name == read_tool_name && !method.is_empty() && method != "get"
1168+
tool_name == read_tool_name && !method.is_empty() && method != method_values::GET
1169+
}
1170+
1171+
/// `issue_read` sub-methods whose responses are not issue objects, so per-item
1172+
/// labeling is skipped (`get_comments` is handled by the item path).
1173+
#[inline]
1174+
pub(crate) fn skip_issue_read_per_item_labeling(tool_name: &str, method: &str) -> bool {
1175+
is_non_get_read_sub_method(tool_name, tool_names::ISSUE_READ, method)
1176+
&& method != method_values::GET_COMMENTS
11691177
}
11701178

11711179
/// Extract a string field from a JSON value
@@ -4669,7 +4677,26 @@ mod tests {
46694677

46704678
#[cfg(test)]
46714679
mod is_non_get_read_sub_method_tests {
4672-
use super::is_non_get_read_sub_method;
4680+
use super::{
4681+
is_non_get_read_sub_method, method_values, skip_issue_read_per_item_labeling, tool_names,
4682+
};
4683+
4684+
#[test]
4685+
fn issue_read_per_item_labeling_skip_condition() {
4686+
for (tool_name, method, expected) in [
4687+
(tool_names::ISSUE_READ, method_values::GET, false),
4688+
(tool_names::ISSUE_READ, method_values::GET_COMMENTS, false),
4689+
(tool_names::ISSUE_READ, "get_labels", true),
4690+
(tool_names::ISSUE_READ, "", false),
4691+
(tool_names::LIST_ISSUES, "get_labels", false),
4692+
] {
4693+
assert_eq!(
4694+
skip_issue_read_per_item_labeling(tool_name, method),
4695+
expected,
4696+
"tool={tool_name}, method={method}"
4697+
);
4698+
}
4699+
}
46734700

46744701
#[test]
46754702
fn matches_non_get_sub_methods() {
@@ -4690,7 +4717,7 @@ mod is_non_get_read_sub_method_tests {
46904717
assert!(!is_non_get_read_sub_method(
46914718
"pull_request_read",
46924719
"pull_request_read",
4693-
"get"
4720+
method_values::GET
46944721
));
46954722
assert!(!is_non_get_read_sub_method(
46964723
"pull_request_read",
@@ -4704,12 +4731,12 @@ mod is_non_get_read_sub_method_tests {
47044731
assert!(!is_non_get_read_sub_method(
47054732
"issue_read",
47064733
"pull_request_read",
4707-
"get_comments"
4734+
method_values::GET_COMMENTS
47084735
));
47094736
assert!(!is_non_get_read_sub_method(
47104737
"list_issues",
47114738
"issue_read",
4712-
"get_comments"
4739+
method_values::GET_COMMENTS
47134740
));
47144741
}
47154742
}

‎guards/github-guard/rust-guard/src/labels/mod.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4004,7 +4004,7 @@ mod tests {
40044004
"owner": "github",
40054005
"repo": "gh-aw-mcpg",
40064006
"issue_number": 2278,
4007-
"method": "get_comments"
4007+
"method": constants::method_values::GET_COMMENTS
40084008
});
40094009
let response = json!([
40104010
{

‎guards/github-guard/rust-guard/src/labels/response_items.rs‎

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -273,15 +273,13 @@ pub fn label_response_items(
273273
| tool_names::GET_ISSUE
274274
| tool_names::ISSUE_READ => {
275275
// For issue_read sub-methods that return non-issue objects (e.g.
276-
// get_comments, get_sub_issues, get_labels), skip per-item labeling.
276+
// get_sub_issues, get_labels), skip per-item labeling.
277277
// Resource-level labels from tool_rules provide correct issue-scoped integrity.
278278
let method = tool_args
279279
.get(field_names::METHOD)
280280
.and_then(|v| v.as_str())
281281
.unwrap_or("");
282-
if is_non_get_read_sub_method(tool_name, tool_names::ISSUE_READ, method)
283-
&& method != "get_comments"
284-
{
282+
if skip_issue_read_per_item_labeling(tool_name, method) {
285283
// Fall through — use resource-level labels from tool_rules
286284
} else {
287285
let items = extract_items_slice(&actual_response, "issues");

‎guards/github-guard/rust-guard/src/labels/response_paths.rs‎

Lines changed: 3 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -306,15 +306,13 @@ pub fn label_response_paths(
306306
| tool_names::SEARCH_ISSUES_FF_FIELDS_PARAM
307307
| tool_names::ISSUE_READ
308308
| tool_names::GET_ISSUE => {
309-
// Skip per-item labeling for issue_read sub-methods (get_comments,
310-
// get_sub_issues, get_labels). Resource-level labels from tool_rules apply.
309+
// Skip per-item labeling for issue_read sub-methods (get_sub_issues,
310+
// get_labels). Resource-level labels from tool_rules apply.
311311
let method = tool_args
312312
.get(field_names::METHOD)
313313
.and_then(|v| v.as_str())
314314
.unwrap_or("");
315-
if is_non_get_read_sub_method(tool_name, tool_names::ISSUE_READ, method)
316-
&& method != "get_comments"
317-
{
315+
if skip_issue_read_per_item_labeling(tool_name, method) {
318316
// Fall through — use resource-level labels
319317
} else if let Some(repo_item_ctx) = resolve_repo_item_context(
320318
tool_name,

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

Lines changed: 15 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -315,8 +315,8 @@ pub fn apply_tool_labels(
315315
"issue_dependency_write"
316316
| "issue_dependency_write_ff_issue_dependencies"
317317
| "pin_issue"
318-
| "transfer_issue"
319-
| "unpin_issue" => {
318+
| tool_names::TRANSFER_ISSUE
319+
| tool_names::UNPIN_ISSUE => {
320320
if !owner.is_empty() && !repo.is_empty() {
321321
if let Some(issue_num) =
322322
extract_number_as_string(tool_args, field_names::ISSUE_NUMBER)
@@ -750,17 +750,17 @@ pub fn apply_tool_labels(
750750
| tool_names::CREATE_DISCUSSION // gh discussion create — creates a discussion in a repository
751751
| tool_names::EDIT_DISCUSSION // gh discussion edit — edits title/body/labels of a discussion
752752
// Granular issue mutation
753-
| "close_issue"
754-
| "reopen_issue"
755-
| "lock_issue"
756-
| "unlock_issue"
757-
| "update_issue_assignees"
758-
| "update_issue_body"
759-
| "update_issue_labels"
760-
| "update_issue_milestone"
761-
| "update_issue_state"
762-
| "update_issue_title"
763-
| "update_issue_type"
753+
| tool_names::CLOSE_ISSUE
754+
| tool_names::REOPEN_ISSUE
755+
| tool_names::LOCK_ISSUE
756+
| tool_names::UNLOCK_ISSUE
757+
| tool_names::UPDATE_ISSUE_ASSIGNEES
758+
| tool_names::UPDATE_ISSUE_BODY
759+
| tool_names::UPDATE_ISSUE_LABELS
760+
| tool_names::UPDATE_ISSUE_MILESTONE
761+
| tool_names::UPDATE_ISSUE_STATE
762+
| tool_names::UPDATE_ISSUE_TITLE
763+
| tool_names::UPDATE_ISSUE_TYPE
764764
| "set_issue_fields"
765765
// Sub-issues
766766
| "add_sub_issue"
@@ -779,7 +779,7 @@ pub fn apply_tool_labels(
779779
| "mark_pull_request_as_draft"
780780
| "mark_pull_request_as_ready_for_review"
781781
| "lock_pull_request"
782-
| "unlock_pull_request"
782+
| tool_names::UNLOCK_PULL_REQUEST
783783
| "update_pull_request_body"
784784
| "update_pull_request_draft_state"
785785
| "update_pull_request_state"
@@ -820,7 +820,7 @@ pub fn apply_tool_labels(
820820
| "revert_pull_request"
821821
// Pre-emptive: issue deletion, issue comments, repository deletion, releases
822822
| tool_names::DELETE_ISSUE
823-
| "update_issue_comment"
823+
| tool_names::UPDATE_ISSUE_COMMENT
824824
| "delete_issue_comment"
825825
| tool_names::DELETE_REPOSITORY
826826
| tool_names::CREATE_RELEASE

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1157,7 +1157,7 @@ mod tests {
11571157
"owner": "org",
11581158
"repo": "repo",
11591159
"issue_number": "7",
1160-
"method": "get_comments"
1160+
"method": labels::constants::method_values::GET_COMMENTS
11611161
}),
11621162
tool_result: json!([
11631163
{"id": 1, "body": "first"},

0 commit comments

Comments
 (0)