Skip to content

Commit dbc0123

Browse files
lpcoxCopilot
andcommitted
fix: move sub-issue and PR review tools to READ_WRITE_OPERATIONS
Address review feedback: granular sub-issue management tools and PR review tools are decomposed versions of composites already classified as read-write (sub_issue_write, pull_request_review_write). Keeping them as pure write would skip read-side secrecy checks and LabelResponse(), allowing private repo content to bypass enforcement. Changes: - Move add_sub_issue, remove_sub_issue, reprioritize_sub_issue from WRITE_OPERATIONS to READ_WRITE_OPERATIONS - Move 7 PR review tools (add_pull_request_review_comment, etc.) from WRITE_OPERATIONS to READ_WRITE_OPERATIONS - Update tests to check is_read_write_operation() consistently - Add apply_tool_labels tests for all 3 new tool groups (issue update, sub-issue management, PR review) asserting writer integrity Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent 7e8fc46 commit dbc0123

2 files changed

Lines changed: 126 additions & 19 deletions

File tree

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

Lines changed: 96 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4735,6 +4735,102 @@ mod tests {
47354735
assert_eq!(integrity, writer_integrity(repo_id, &ctx), "sub_issue_write should have writer integrity");
47364736
}
47374737

4738+
#[test]
4739+
fn test_apply_tool_labels_granular_issue_update_writer_integrity() {
4740+
let ctx = default_ctx();
4741+
let repo_id = "github/copilot";
4742+
let tool_args = json!({
4743+
"owner": "github",
4744+
"repo": "copilot",
4745+
"issue_number": 1
4746+
});
4747+
4748+
for tool in &[
4749+
"update_issue_assignees",
4750+
"update_issue_body",
4751+
"update_issue_labels",
4752+
"update_issue_milestone",
4753+
"update_issue_state",
4754+
"update_issue_title",
4755+
"update_issue_type",
4756+
] {
4757+
let (secrecy, integrity, _desc) = apply_tool_labels(
4758+
tool,
4759+
&tool_args,
4760+
repo_id,
4761+
vec![],
4762+
vec![],
4763+
String::new(),
4764+
&ctx,
4765+
);
4766+
4767+
// Repo-scoped secrecy (public repo in default_ctx → empty)
4768+
assert_eq!(secrecy, vec![] as Vec<String>, "{tool} secrecy mismatch");
4769+
assert_eq!(integrity, writer_integrity(repo_id, &ctx), "{tool} should have writer integrity");
4770+
}
4771+
}
4772+
4773+
#[test]
4774+
fn test_apply_tool_labels_granular_sub_issue_tools_writer_integrity() {
4775+
let ctx = default_ctx();
4776+
let repo_id = "github/copilot";
4777+
let tool_args = json!({
4778+
"owner": "github",
4779+
"repo": "copilot",
4780+
"issue_number": 1,
4781+
"sub_issue_id": 2
4782+
});
4783+
4784+
for tool in &["add_sub_issue", "remove_sub_issue", "reprioritize_sub_issue"] {
4785+
let (secrecy, integrity, _desc) = apply_tool_labels(
4786+
tool,
4787+
&tool_args,
4788+
repo_id,
4789+
vec![],
4790+
vec![],
4791+
String::new(),
4792+
&ctx,
4793+
);
4794+
4795+
assert_eq!(secrecy, vec![] as Vec<String>, "{tool} secrecy mismatch");
4796+
assert_eq!(integrity, writer_integrity(repo_id, &ctx), "{tool} should have writer integrity");
4797+
}
4798+
}
4799+
4800+
#[test]
4801+
fn test_apply_tool_labels_granular_pr_review_tools_writer_integrity() {
4802+
let ctx = default_ctx();
4803+
let repo_id = "github/copilot";
4804+
let tool_args = json!({
4805+
"owner": "github",
4806+
"repo": "copilot",
4807+
"pullNumber": 42
4808+
});
4809+
4810+
for tool in &[
4811+
"add_pull_request_review_comment",
4812+
"create_pull_request_review",
4813+
"delete_pending_pull_request_review",
4814+
"request_pull_request_reviewers",
4815+
"resolve_review_thread",
4816+
"submit_pending_pull_request_review",
4817+
"unresolve_review_thread",
4818+
] {
4819+
let (secrecy, integrity, _desc) = apply_tool_labels(
4820+
tool,
4821+
&tool_args,
4822+
repo_id,
4823+
vec![],
4824+
vec![],
4825+
String::new(),
4826+
&ctx,
4827+
);
4828+
4829+
assert_eq!(secrecy, vec![] as Vec<String>, "{tool} secrecy mismatch");
4830+
assert_eq!(integrity, writer_integrity(repo_id, &ctx), "{tool} should have writer integrity");
4831+
}
4832+
}
4833+
47384834
#[test]
47394835
fn test_apply_tool_labels_fork_repository_writer_integrity() {
47404836
let ctx = default_ctx();

guards/github-guard/rust-guard/src/tools.rs

Lines changed: 30 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -74,19 +74,6 @@ pub const WRITE_OPERATIONS: &[&str] = &[
7474
// Pre-emptive: gist deletion (gh gist delete)
7575
"delete_gist", // DELETE /gists/{gist_id}
7676

77-
// Sub-issue management tools (alongside sub_issue_write composite)
78-
"add_sub_issue", // POST /repos/.../issues/{number}/sub_issues
79-
"remove_sub_issue", // DELETE/POST — remove sub-issue link
80-
"reprioritize_sub_issue", // PATCH — reorder sub-issues
81-
82-
// PR review tools (alongside pull_request_review_write composite)
83-
"add_pull_request_review_comment", // POST /repos/.../pulls/{number}/comments
84-
"create_pull_request_review", // POST /repos/.../pulls/{number}/reviews
85-
"delete_pending_pull_request_review", // DELETE /repos/.../pulls/{number}/reviews/{id}
86-
"request_pull_request_reviewers", // POST /repos/.../pulls/{number}/requested_reviewers
87-
"resolve_review_thread", // PUT /graphql — resolveReviewThread
88-
"submit_pending_pull_request_review", // POST /repos/.../pulls/{number}/reviews/{id}/events
89-
"unresolve_review_thread", // PUT /graphql — unresolveReviewThread
9077
];
9178

9279
/// Read-write operations that both read and modify data
@@ -113,6 +100,20 @@ pub const READ_WRITE_OPERATIONS: &[&str] = &[
113100
"update_issue_title", // PATCH — modifies issue title
114101
"update_issue_type", // PATCH — modifies issue type
115102

103+
// Sub-issue management tools (alongside sub_issue_write composite)
104+
"add_sub_issue", // POST /repos/.../issues/{number}/sub_issues
105+
"remove_sub_issue", // DELETE/POST — remove sub-issue link
106+
"reprioritize_sub_issue", // PATCH — reorder sub-issues
107+
108+
// PR review tools (alongside pull_request_review_write composite)
109+
"add_pull_request_review_comment", // POST /repos/.../pulls/{number}/comments
110+
"create_pull_request_review", // POST /repos/.../pulls/{number}/reviews
111+
"delete_pending_pull_request_review", // DELETE /repos/.../pulls/{number}/reviews/{id}
112+
"request_pull_request_reviewers", // POST /repos/.../pulls/{number}/requested_reviewers
113+
"resolve_review_thread", // PUT /graphql — resolveReviewThread
114+
"submit_pending_pull_request_review", // POST /repos/.../pulls/{number}/reviews/{id}/events
115+
"unresolve_review_thread", // PUT /graphql — unresolveReviewThread
116+
116117
// Granular PR update tools (alongside update_pull_request composite)
117118
"update_pull_request_body", // PATCH — modifies PR body
118119
"update_pull_request_draft_state", // PATCH — converts to/from draft
@@ -362,18 +363,23 @@ mod tests {
362363
}
363364

364365
#[test]
365-
fn test_sub_issue_management_tools_are_write_operations() {
366+
fn test_sub_issue_management_tools_are_read_write_operations() {
366367
for op in &["add_sub_issue", "remove_sub_issue", "reprioritize_sub_issue"] {
367368
assert!(
368-
is_write_operation(op),
369-
"{} must be classified as a write operation",
369+
is_read_write_operation(op),
370+
"{} must be classified as a read-write operation",
371+
op
372+
);
373+
assert!(
374+
!is_write_operation(op),
375+
"{} should not be in WRITE_OPERATIONS (it is in READ_WRITE_OPERATIONS)",
370376
op
371377
);
372378
}
373379
}
374380

375381
#[test]
376-
fn test_pr_review_tools_are_write_operations() {
382+
fn test_pr_review_tools_are_read_write_operations() {
377383
for op in &[
378384
"add_pull_request_review_comment",
379385
"create_pull_request_review",
@@ -384,8 +390,13 @@ mod tests {
384390
"unresolve_review_thread",
385391
] {
386392
assert!(
387-
is_write_operation(op),
388-
"{} must be classified as a write operation",
393+
is_read_write_operation(op),
394+
"{} must be classified as a read-write operation",
395+
op
396+
);
397+
assert!(
398+
!is_write_operation(op),
399+
"{} should not be in WRITE_OPERATIONS (it is in READ_WRITE_OPERATIONS)",
389400
op
390401
);
391402
}

0 commit comments

Comments
 (0)