Skip to content

Commit 91a2aa2

Browse files
authored
perf(rust-guard): eliminate redundant clones in extract_mcp_response and first_matching_scope (#5103)
Two hot-path allocations in the WASM guard's response labeling pipeline: `extract_mcp_response` unconditionally deep-clones the full JSON payload on the common (already-unwrapped) path, and `first_matching_scope` clones a `PolicyScopeEntry` (3× `Option<String>`) on every call to `policy_private_scope_label`. ## `extract_mcp_response` → `Cow<'_, Value>` (`labels/mod.rs`) Common path now borrows; only the rare MCP-wrapped case allocates: ```rust // Before pub(crate) fn extract_mcp_response(response: &Value) -> Value { // ... response.clone() // full payload deep-copy every call } // After pub(crate) fn extract_mcp_response(response: &Value) -> Cow<'_, Value> { // ... Cow::Borrowed(response) // zero allocation on common path // wrapped path: Cow::Owned(parsed) } ``` Call site updates in `response_items.rs` where `Value` (not `Cow`) is required: `actual_response.clone()` → `actual_response.as_ref().clone()` and `&actual_response` → `actual_response.as_ref()`. All other call sites auto-deref through `Deref<Target = Value>` unchanged. ## `first_matching_scope` → `Option<&'a PolicyScopeEntry>` (`labels/helpers.rs`) Removes `.cloned()` to eliminate 3 `String` allocations per lookup. `policy_private_scope_label` (the only call site) is unaffected — `ScopeKind` is `Copy` and field access works identically through a reference. ```rust // Before fn first_matching_scope(owner: &str, repo: &str, ctx: &PolicyContext) -> Option<PolicyScopeEntry> { ctx.scopes.iter().find(|scope| { /* ... */ }).cloned() } // After fn first_matching_scope<'a>(owner: &str, repo: &str, ctx: &'a PolicyContext) -> Option<&'a PolicyScopeEntry> { ctx.scopes.iter().find(|scope| { /* ... */ }) } ``` > [!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-build3904760875/b513/launcher.test /tmp/go-build3904760875/b513/launcher.test -test.testlogfile=/tmp/go-build3904760875/b513/testlog.txt -test.paniconexit0 -test.timeout=10m0s go_.�� ache/go/1.25.9/x-errorsas etut/NpEHhtvPW_A-ifaceassert x_amd64/vet --gdwarf-5 --64 -o x_amd64/vet` (dns block) > - Triggering command: `/tmp/go-build100690393/b509/launcher.test /tmp/go-build100690393/b509/launcher.test -test.testlogfile=/tmp/go-build100690393/b509/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true -bool 72e203ec55626255a1c2f6351011f81be538b3bb6463e1a9d5c ker/cli-plugins/docker-compose 72e203ec55626255bash` (dns block) > - `invalid-host-that-does-not-exist-12345.com` > - Triggering command: `/tmp/go-build3904760875/b495/config.test /tmp/go-build3904760875/b495/config.test -test.testlogfile=/tmp/go-build3904760875/b495/testlog.txt -test.paniconexit0 -test.timeout=10m0s /tmp/go-build3904760875/b396/vet.cfg 1154246/b196/_pkg_.a -trimpath x_amd64/vet -p go.opentelemetry-atomic -lang=go1.25 x_amd64/vet go_.�� .cfg knP4/DXqMOEsmzZ1-ifaceassert x_amd64/vet --gdwarf-5 --64 -o x_amd64/vet` (dns block) > - Triggering command: `/tmp/go-build100690393/b491/config.test /tmp/go-build100690393/b491/config.test -test.testlogfile=/tmp/go-build100690393/b491/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true 4760875/b504/_pkg_.a stmain.go docker-compose by/32b5d288a6552/usr/lib/open-iscsi/net-interface-handler -ifaceassert -nilfunc docker-compose n-me�� b6b9402ad1484ea3 -buildtags &#34;CURL_CA_BUNDLE=/-id by/181260d18b4dc/usr/bin/networkctl -ifaceassert -nilfunc iginal` (dns block) > - `nonexistent.local` > - Triggering command: `/tmp/go-build3904760875/b513/launcher.test /tmp/go-build3904760875/b513/launcher.test -test.testlogfile=/tmp/go-build3904760875/b513/testlog.txt -test.paniconexit0 -test.timeout=10m0s go_.�� ache/go/1.25.9/x-errorsas etut/NpEHhtvPW_A-ifaceassert x_amd64/vet --gdwarf-5 --64 -o x_amd64/vet` (dns block) > - Triggering command: `/tmp/go-build100690393/b509/launcher.test /tmp/go-build100690393/b509/launcher.test -test.testlogfile=/tmp/go-build100690393/b509/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true -bool 72e203ec55626255a1c2f6351011f81be538b3bb6463e1a9d5c ker/cli-plugins/docker-compose 72e203ec55626255bash` (dns block) > - `slow.example.com` > - Triggering command: `/tmp/go-build3904760875/b513/launcher.test /tmp/go-build3904760875/b513/launcher.test -test.testlogfile=/tmp/go-build3904760875/b513/testlog.txt -test.paniconexit0 -test.timeout=10m0s go_.�� ache/go/1.25.9/x-errorsas etut/NpEHhtvPW_A-ifaceassert x_amd64/vet --gdwarf-5 --64 -o x_amd64/vet` (dns block) > - Triggering command: `/tmp/go-build100690393/b509/launcher.test /tmp/go-build100690393/b509/launcher.test -test.testlogfile=/tmp/go-build100690393/b509/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true -bool 72e203ec55626255a1c2f6351011f81be538b3bb6463e1a9d5c ker/cli-plugins/docker-compose 72e203ec55626255bash` (dns block) > - `this-host-does-not-exist-12345.com` > - Triggering command: `/tmp/go-build3904760875/b522/mcp.test /tmp/go-build3904760875/b522/mcp.test -test.testlogfile=/tmp/go-build3904760875/b522/testlog.txt -test.paniconexit0 -test.timeout=10m0s -o 64/src/net -trimpath x_amd64/vet -p net/http/httptes--version -lang=go1.25 x_amd64/vet .cfg�� /opt/hostedtoolcache/go/1.25.9/xgo1.25.9 1154246/b166/ x_amd64/vet --gdwarf-5 --64` (dns block) > - Triggering command: `/tmp/go-build100690393/b518/mcp.test /tmp/go-build100690393/b518/mcp.test -test.testlogfile=/tmp/go-build100690393/b518/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true -bool y /usr/local/bin/bash ntime.v2.task/mobash -ifaceassert 2d7/log.json bash /usr�� 6788dd938a942ab2a742ecfee664e61b/run/containerd/io.containerd.runtime.v2.task/moby/1166b6fa98e14docker -tests /usr/sbin/bash 2d7 /tmp/go-build410/usr/bin/runc 2d7/init.pid bash` (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>
2 parents ce59011 + 3a196e1 commit 91a2aa2

3 files changed

Lines changed: 8 additions & 7 deletions

File tree

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

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -229,15 +229,14 @@ fn repo_matches_scope(
229229
}
230230
}
231231

232-
fn first_matching_scope(owner: &str, repo: &str, ctx: &PolicyContext) -> Option<PolicyScopeEntry> {
232+
fn first_matching_scope<'a>(owner: &str, repo: &str, ctx: &'a PolicyContext) -> Option<&'a PolicyScopeEntry> {
233233
ctx.scopes
234234
.iter()
235235
.find(|scope| {
236236
let scoped_owner = scope.scope_owner.as_deref().unwrap_or("");
237237
let scoped_repo = scope.scope_repo.as_deref().unwrap_or("");
238238
repo_matches_scope(scope.scope_kind, owner, repo, scoped_owner, scoped_repo)
239239
})
240-
.cloned()
241240
}
242241

243242
fn format_integrity_label(prefix: &str, scope: &str, base: &str) -> String {

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

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,8 @@
2424
//! - `unapproved:<repo>` - Lower trust external contribution level
2525
//! - (none) - Untrusted/external content
2626
27+
use std::borrow::Cow;
28+
2729
use serde_json::Value;
2830

2931
// Sub-modules
@@ -93,7 +95,7 @@ pub fn label_response_paths(
9395
/// Extract the actual response from MCP wrapper format
9496
/// MCP responses are wrapped in {"content":[{"type":"text","text":"..."}]}
9597
/// where the text field contains stringified JSON
96-
pub(crate) fn extract_mcp_response(response: &Value) -> Value {
98+
pub(crate) fn extract_mcp_response(response: &Value) -> Cow<'_, Value> {
9799
// Log the top-level keys to understand the structure
98100
if let Some(obj) = response.as_object() {
99101
let keys: Vec<&str> = obj.keys().map(|s| s.as_str()).collect();
@@ -128,7 +130,7 @@ pub(crate) fn extract_mcp_response(response: &Value) -> Value {
128130
// Try to parse the text as JSON
129131
if let Ok(parsed) = serde_json::from_str::<Value>(text) {
130132
crate::log_debug("extract_mcp_response: parsed content[0].text as JSON");
131-
return parsed;
133+
return Cow::Owned(parsed);
132134
} else {
133135
crate::log_debug("extract_mcp_response: failed to parse text as JSON");
134136
}
@@ -143,7 +145,7 @@ pub(crate) fn extract_mcp_response(response: &Value) -> Value {
143145
// If we can't extract from MCP wrapper, return the original response
144146
// (it might already be unwrapped or in a different format)
145147
crate::log_debug("extract_mcp_response: using response as-is");
146-
response.clone()
148+
Cow::Borrowed(response)
147149
}
148150

149151
// ============================================================================

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -128,7 +128,7 @@ pub fn label_response_items(
128128
graphql_single_buf = [obj.clone()];
129129
&graphql_single_buf
130130
} else if actual_response.is_object() && !is_graphql_wrapper(&actual_response) && !is_search_result_wrapper(&actual_response) && !is_mcp_text_wrapper(&actual_response) {
131-
single_item_buf = [actual_response.clone()];
131+
single_item_buf = [actual_response.as_ref().clone()];
132132
&single_item_buf
133133
} else {
134134
&[]
@@ -240,7 +240,7 @@ pub fn label_response_items(
240240
} else if let Some(obj) = extract_graphql_single_object(&actual_response) {
241241
vec![obj]
242242
} else if actual_response.is_object() && !is_graphql_wrapper(&actual_response) && !is_search_result_wrapper(&actual_response) && !is_mcp_text_wrapper(&actual_response) {
243-
vec![&actual_response]
243+
vec![actual_response.as_ref()]
244244
} else {
245245
Vec::new()
246246
};

0 commit comments

Comments
 (0)