Skip to content

Commit 0f464bf

Browse files
authored
rust-guard: remove dead params from issue_integrity + fix heap-allocating string comparison (#2152)
Two small cleanup items in the Rust guard: `issue_integrity` carried two never-read parameters (`_owner`, `_repo`) that forced callers to perform unnecessary string splits, and `tool_rules.rs` was heap-allocating a `String` on every `actions_get` call just to do an equality check. ## Changes - **`labels/helpers.rs`** — drop `_owner: &str` and `_repo: &str` from `issue_integrity` signature - **`labels/response_items.rs`** — remove `repo_owner` split (existed solely to fill the dead arg) - **`labels/response_paths.rs`** — remove `owner` split + stale `// Extract owner from repo for owner check` comment - **`labels/mod.rs`** — update all 10 test call sites to match new signature - **`labels/tool_rules.rs`** — replace heap-allocating comparison with zero-allocation `.as_str()` idiom: ```rust // Before — allocates on every actions_get call tool_args.get("method") == Some(&Value::String("download_workflow_run_artifact".to_string())) // After — allocation-free tool_args.get("method").and_then(|v| v.as_str()) == Some("download_workflow_run_artifact") ``` > [!WARNING] > > <details> > <summary>Firewall rules blocked me from connecting to one or more addresses (expand for details)</summary> > > #### I tried to connect to the following addresses, but was blocked by firewall rules: > > - `example.com` > - Triggering command: `/tmp/go-build1926788072/b332/launcher.test /tmp/go-build1926788072/b332/launcher.test -test.testlogfile=/tmp/go-build1926788072/b332/testlog.txt -test.paniconexit0 -test.timeout=10m0s /tmp/go-build1926788072/b253/vet.cfg 64/src/runtime/cgo erive-f8a9da973ea849b8.serde_dernet/http x86_64/as erive-f8a9da973e/opt/hostedtoolcache/go/1.25.8/x64/pkg/tool/linux_amd64/vet 378038/b011/syma-unsafeptr=false erive-f8a9da973e-unreachable=false _main.o eriv�� ache/go/1.25.8/x64/src/runtime/cgo1.25.8 CLzi/BTFciEBdHbS8DwRuCLzi x_amd64/vet erive-f8a9da973e/opt/hostedtoolcache/go/1.25.8/x64/pkg/tool/linux_amd64/vet erive-f8a9da973e-atomic -Wl,-Bstatic x_amd64/vet` (dns block) > - `invalid-host-that-does-not-exist-12345.com` > - Triggering command: `/tmp/go-build1926788072/b317/config.test /tmp/go-build1926788072/b317/config.test -test.testlogfile=/tmp/go-build1926788072/b317/testlog.txt -test.paniconexit0 -test.timeout=10m0s 2c9e�� 64/src/runtime/cgo -gnu/lib/libstd-267b04dbd87607fbgithub.com/tetratelabs/wazero/internal/platform x_amd64/compile -gnu/lib/libobje/opt/hostedtoolcache/go/1.25.8/x64/pkg/tool/linux_amd64/vet 378038/b009/ -gnu/lib/libaddr-unreachable=false x_amd64/compile -gnu�� ; then \ $GOPATH/bin/golangci-lint run --timeout=5m || echo &#34;��� Warning: golangci-lint failed /opt/hostedtoolcache/go/1.25.8/x64/pkg/tool/linux_amd64/vet -gnu/lib/libstd_detect-3c01aa300-nolocalimports x_amd64/vet -gnu/lib/librust/opt/hostedtoolcache/go/1.25.8/x64/pkg/tool/linux_amd64/vet ry 64-REDACTED-linux-unreachable=false x_amd64/vet` (dns block) > - `nonexistent.local` > - Triggering command: `/tmp/go-build1926788072/b332/launcher.test /tmp/go-build1926788072/b332/launcher.test -test.testlogfile=/tmp/go-build1926788072/b332/testlog.txt -test.paniconexit0 -test.timeout=10m0s /tmp/go-build1926788072/b253/vet.cfg 64/src/runtime/cgo erive-f8a9da973ea849b8.serde_dernet/http x86_64/as erive-f8a9da973e/opt/hostedtoolcache/go/1.25.8/x64/pkg/tool/linux_amd64/vet 378038/b011/syma-unsafeptr=false erive-f8a9da973e-unreachable=false _main.o eriv�� ache/go/1.25.8/x64/src/runtime/cgo1.25.8 CLzi/BTFciEBdHbS8DwRuCLzi x_amd64/vet erive-f8a9da973e/opt/hostedtoolcache/go/1.25.8/x64/pkg/tool/linux_amd64/vet erive-f8a9da973e-atomic -Wl,-Bstatic x_amd64/vet` (dns block) > - `slow.example.com` > - Triggering command: `/tmp/go-build1926788072/b332/launcher.test /tmp/go-build1926788072/b332/launcher.test -test.testlogfile=/tmp/go-build1926788072/b332/testlog.txt -test.paniconexit0 -test.timeout=10m0s /tmp/go-build1926788072/b253/vet.cfg 64/src/runtime/cgo erive-f8a9da973ea849b8.serde_dernet/http x86_64/as erive-f8a9da973e/opt/hostedtoolcache/go/1.25.8/x64/pkg/tool/linux_amd64/vet 378038/b011/syma-unsafeptr=false erive-f8a9da973e-unreachable=false _main.o eriv�� ache/go/1.25.8/x64/src/runtime/cgo1.25.8 CLzi/BTFciEBdHbS8DwRuCLzi x_amd64/vet erive-f8a9da973e/opt/hostedtoolcache/go/1.25.8/x64/pkg/tool/linux_amd64/vet erive-f8a9da973e-atomic -Wl,-Bstatic x_amd64/vet` (dns block) > - `this-host-does-not-exist-12345.com` > - Triggering command: `/tmp/go-build1926788072/b341/mcp.test /tmp/go-build1926788072/b341/mcp.test -test.testlogfile=/tmp/go-build1926788072/b341/testlog.txt -test.paniconexit0 -test.timeout=10m0s o_.o�� 64/src/net a/dsa.go x_amd64/vet -p internal/runtime--version -lang=go1.25 x_amd64/vet -o ew@v1.1.1/spew/bypass.go ew@v1.1.1/spew/common.go x_amd64/vet -p 378038/b107/_cgo-qE -lang=go1.25 x_amd64/vet` (dns block) > > If you need me to access, download, or install something from one of these locations, you can either: > > - Configure [Actions setup steps](https://gh.io/copilot/actions-setup-steps) to set up my environment, which run before the firewall is enabled > - Add the appropriate URLs or hosts to the custom allowlist in this repository's [Copilot coding agent settings](https://github.com/github/gh-aw-mcpg/settings/copilot/coding_agent) (admins only) > > </details> <!-- START COPILOT CODING AGENT TIPS --> --- 📱 Kick off Copilot coding agent tasks wherever you are with [GitHub Mobile](https://gh.io/cca-mobile-docs), available on iOS and Android.
2 parents c9ac184 + 5b00551 commit 0f464bf

5 files changed

Lines changed: 14 additions & 32 deletions

File tree

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

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -812,8 +812,6 @@ pub fn pr_integrity(
812812
pub fn issue_integrity(
813813
item: &Value,
814814
repo_full_name: &str,
815-
_owner: &str,
816-
_repo: &str,
817815
repo_private: bool,
818816
ctx: &PolicyContext,
819817
) -> Vec<String> {

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

Lines changed: 10 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -729,15 +729,13 @@ mod tests {
729729
fn test_issue_integrity() {
730730
let ctx = default_ctx();
731731
let repo = "github/copilot";
732-
let owner = "github";
733-
let repo_name = "copilot";
734732

735733
// Private repo issues get approved integrity
736734
let bot_issue = json!({
737735
"user": {"login": "dependabot[bot]"}
738736
});
739737
assert_eq!(
740-
issue_integrity(&bot_issue, repo, owner, repo_name, true, &ctx),
738+
issue_integrity(&bot_issue, repo, true, &ctx),
741739
writer_integrity(repo, &ctx)
742740
);
743741

@@ -746,7 +744,7 @@ mod tests {
746744
"user": {"login": "github"}
747745
});
748746
assert_eq!(
749-
issue_integrity(&owner_issue, repo, owner, repo_name, false, &ctx),
747+
issue_integrity(&owner_issue, repo, false, &ctx),
750748
none_integrity(repo, &ctx)
751749
);
752750

@@ -755,28 +753,21 @@ mod tests {
755753
"user": {"login": "someone"}
756754
});
757755
assert_eq!(
758-
issue_integrity(&issue, "", "", "", false, &ctx),
756+
issue_integrity(&issue, "", false, &ctx),
759757
none_integrity("", &ctx)
760758
);
761759

762760
// Public issue with OWNER association retains approved floor
763761
let owner_assoc_issue = json!({"author_association": "OWNER"});
764762
assert_eq!(
765-
issue_integrity(&owner_assoc_issue, repo, owner, repo_name, false, &ctx),
763+
issue_integrity(&owner_assoc_issue, repo, false, &ctx),
766764
writer_integrity(repo, &ctx)
767765
);
768766

769767
// Public issue with CONTRIBUTOR association gets unapproved floor
770768
let contributor_assoc_issue = json!({"author_association": "CONTRIBUTOR"});
771769
assert_eq!(
772-
issue_integrity(
773-
&contributor_assoc_issue,
774-
repo,
775-
owner,
776-
repo_name,
777-
false,
778-
&ctx
779-
),
770+
issue_integrity(&contributor_assoc_issue, repo, false, &ctx),
780771
reader_integrity(repo, &ctx)
781772
);
782773
}
@@ -857,8 +848,6 @@ mod tests {
857848
fn test_trusted_bot_issue_integrity_public_repo() {
858849
let ctx = default_ctx();
859850
let repo = "github/copilot";
860-
let owner = "github";
861-
let repo_name = "copilot";
862851

863852
// Trusted bot issue on public repo gets approved (writer) integrity
864853
// even though author_association is NONE
@@ -867,7 +856,7 @@ mod tests {
867856
"author_association": "NONE"
868857
});
869858
assert_eq!(
870-
issue_integrity(&dependabot_issue, repo, owner, repo_name, false, &ctx),
859+
issue_integrity(&dependabot_issue, repo, false, &ctx),
871860
writer_integrity(repo, &ctx)
872861
);
873862

@@ -876,15 +865,15 @@ mod tests {
876865
"author_association": "NONE"
877866
});
878867
assert_eq!(
879-
issue_integrity(&actions_issue, repo, owner, repo_name, false, &ctx),
868+
issue_integrity(&actions_issue, repo, false, &ctx),
880869
writer_integrity(repo, &ctx)
881870
);
882871

883872
let merge_queue_issue = json!({
884873
"user": {"login": "github-merge-queue[bot]"}
885874
});
886875
assert_eq!(
887-
issue_integrity(&merge_queue_issue, repo, owner, repo_name, false, &ctx),
876+
issue_integrity(&merge_queue_issue, repo, false, &ctx),
888877
writer_integrity(repo, &ctx)
889878
);
890879

@@ -893,7 +882,7 @@ mod tests {
893882
"author_association": "NONE"
894883
});
895884
assert_eq!(
896-
issue_integrity(&copilot_issue, repo, owner, repo_name, false, &ctx),
885+
issue_integrity(&copilot_issue, repo, false, &ctx),
897886
writer_integrity(repo, &ctx)
898887
);
899888

@@ -903,7 +892,7 @@ mod tests {
903892
"author_association": "NONE"
904893
});
905894
assert_eq!(
906-
issue_integrity(&renovate_issue, repo, owner, repo_name, false, &ctx),
895+
issue_integrity(&renovate_issue, repo, false, &ctx),
907896
none_integrity(repo, &ctx)
908897
);
909898
}

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

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -205,13 +205,10 @@ pub fn label_response_items(
205205

206206
let repo_private = repo_visibility_private_for_repo_id(&repo_full_name)
207207
.unwrap_or(default_repo_private);
208-
let repo_owner = repo_full_name.split('/').next().unwrap_or("");
209208
let number = item.get("number").and_then(|v| v.as_i64()).unwrap_or(0);
210209
let integrity = issue_integrity(
211210
item,
212211
&repo_full_name,
213-
repo_owner,
214-
&arg_repo,
215212
repo_private,
216213
ctx,
217214
);

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

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -221,17 +221,13 @@ pub fn label_response_paths(
221221
&item_repo
222222
};
223223

224-
// Extract owner from repo for owner check
225-
let owner = repo_for_labels.split('/').next().unwrap_or("");
226224
let item_repo_private = repo_visibility_private_for_repo_id(repo_for_labels)
227225
.unwrap_or(default_repo_private);
228226

229227
let issue_number = item.get("number").and_then(|v| v.as_u64()).unwrap_or(0);
230228
let integrity = issue_integrity(
231229
item,
232230
repo_for_labels,
233-
owner,
234-
&arg_repo,
235231
item_repo_private,
236232
ctx,
237233
);

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

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -277,8 +277,10 @@ pub fn apply_tool_labels(
277277

278278
// Additional secrecy checks for workflow files
279279
if tool_name == "actions_get"
280-
&& tool_args.get("method")
281-
== Some(&Value::String("download_workflow_run_artifact".to_string()))
280+
&& tool_args
281+
.get("method")
282+
.and_then(|v| v.as_str())
283+
== Some("download_workflow_run_artifact")
282284
{
283285
// Artifacts may contain secrets
284286
secrecy = secret_label();

0 commit comments

Comments
 (0)