Skip to content

[rust-guard] Rust Guard: Hoist invariant label calls outside per-item loops in response_paths.rs #6193

@github-actions

Description

@github-actions

🦀 Rust Guard Improvement Report

Improvement 1: Hoist invariant label calls outside per-item loops in response_paths.rs

Category: Performance
File(s): guards/github-guard/rust-guard/src/labels/response_paths.rs
Effort: Small (< 15 min)
Risk: Low

Problem

Two collection branches in label_response_paths recompute the same constant labels on every loop iteration AND again in default_labels, wasting allocations:

list_notifications (lines 502–532): private_user_label() is called N+1 times:

  • Once per item inside the loop (line 516)
  • Once more in default_labels (line 526)

private_user_label() has no parameters — it always returns vec!["private:user".to_string()]. The String allocation is repeated for every notification item.

list_gists (lines 534–572): reader_integrity(scope_names::USER, ctx) is called N+1 times:

  • Once per item inside the loop (line 557)
  • Once more in default_labels (line 567)

scope_names::USER and ctx are both loop-invariant. reader_integrity may traverse scope-policy lookups before allocating label strings.

Before

// list_notifications
for (i, item) in limited_items.iter().enumerate() {
    let id = get_str_or(item, "id", "unknown");
    labeled_paths.push(PathLabelEntry {
        path: format!("/{}", i),
        labels: crate::ResourceLabels {
            description: format!("notification:{}", id),
            secrecy: private_user_label().into(),   // ← allocated N times
            integrity: vec![].into(),
        },
    });
}
return Some(PathLabelResult {
    labeled_paths,
    default_labels: Some(crate::ResourceLabels {
        description: "notification".to_string(),
        secrecy: private_user_label().into(),       // ← allocated again
        integrity: vec![].into(),
    }),
    items_path: None,
});

// list_gists (integrity lines)
for (i, item) in limited_items.iter().enumerate() {
    // ...
    labeled_paths.push(PathLabelEntry {
        // ...
        integrity: reader_integrity(scope_names::USER, ctx).into(), // ← N times
    });
}
return Some(PathLabelResult {
    // ...
    default_labels: Some(crate::ResourceLabels {
        // ...
        integrity: reader_integrity(scope_names::USER, ctx).into(), // ← again
    }),
    // ...
});

After

// list_notifications — hoist before loop
let notif_secrecy: crate::SharedLabels = private_user_label().into();

for (i, item) in limited_items.iter().enumerate() {
    let id = get_str_or(item, "id", "unknown");
    labeled_paths.push(PathLabelEntry {
        path: format!("/{}", i),
        labels: crate::ResourceLabels {
            description: format!("notification:{}", id),
            secrecy: notif_secrecy.clone(),
            integrity: vec![].into(),
        },
    });
}
return Some(PathLabelResult {
    labeled_paths,
    default_labels: Some(crate::ResourceLabels {
        description: "notification".to_string(),
        secrecy: notif_secrecy,
        integrity: vec![].into(),
    }),
    items_path: None,
});

// list_gists — hoist integrity before loop
let gist_integrity: crate::SharedLabels = reader_integrity(scope_names::USER, ctx).into();

for (i, item) in limited_items.iter().enumerate() {
    // ...
    labeled_paths.push(PathLabelEntry {
        // ...
        integrity: gist_integrity.clone(),
    });
}
return Some(PathLabelResult {
    // ...
    default_labels: Some(crate::ResourceLabels {
        // ...
        integrity: gist_integrity,
    }),
    // ...
});

Why This Matters

SharedLabels wraps Arc<Vec<String>>, so .clone() is a single atomic reference-count increment — essentially free. The hoisted calls eliminate N redundant Vec<String> allocations (one per item) for every list_notifications and list_gists response. For a typical page of 30–50 items this removes 30–50 heap allocations per request. This mirrors the same improvement already applied to response_items.rs on 2026-05-19.


Improvement 2: Eliminate duplicate extract_number_as_string in the PR match arm of apply_tool_labels

Category: Duplication
File(s): guards/github-guard/rust-guard/src/labels/tool_rules.rs
Effort: Small (< 15 min)
Risk: Low

Problem

Inside the "get_pull_request" | "pull_request_read" | "list_pull_requests" match arm (lines 213–286), the expression:

extract_number_as_string(tool_args, field_names::PULL_NUMBER)
    .or_else(|| extract_number_as_string(tool_args, "pullNumber"))

is computed twice:

  • Lines 217–218 (inside if !owner.is_empty() && !repo.is_empty()) — to populate desc
  • Lines 225–226 (inside if matches!(tool_name, "get_pull_request" | "pull_request_read")) — to invoke the backend

Both reads scan the same tool_args JSON object for the same keys. JSON field lookup is cheap but the duplication is unnecessary and could mask a future divergence (e.g. if the logic for desc and the logic for the backend call ever need to differ).

Before

"get_pull_request" | "pull_request_read" | "list_pull_requests" => {
    if !owner.is_empty() && !repo.is_empty() {
        let pr_num = extract_number_as_string(tool_args, field_names::PULL_NUMBER)
            .or_else(|| extract_number_as_string(tool_args, "pullNumber")); // ← first scan
        if let Some(num) = pr_num {
            desc = format!("pr:{}/{}#{}", owner, repo, num);
        }
    }
    secrecy = apply_repo_visibility_secrecy(&owner, &repo, repo_id, secrecy, ctx);
    if matches!(tool_name, "get_pull_request" | "pull_request_read") {
        let pull_number = extract_number_as_string(tool_args, field_names::PULL_NUMBER)
            .or_else(|| extract_number_as_string(tool_args, "pullNumber")); // ← second scan
        if let Some(number) = pull_number {
            // ... backend call using `number`
        }
    }
}

After

"get_pull_request" | "pull_request_read" | "list_pull_requests" => {
    // Extract once at the top of the arm; both inner blocks reuse the binding.
    let pull_number = extract_number_as_string(tool_args, field_names::PULL_NUMBER)
        .or_else(|| extract_number_as_string(tool_args, "pullNumber"));

    if !owner.is_empty() && !repo.is_empty() {
        if let Some(ref num) = pull_number {
            desc = format!("pr:{}/{}#{}", owner, repo, num);
        }
    }
    secrecy = apply_repo_visibility_secrecy(&owner, &repo, repo_id, secrecy, ctx);
    if matches!(tool_name, "get_pull_request" | "pull_request_read") {
        if let Some(ref number) = pull_number {
            // ... backend call using `number`
        } else {
            integrity = private_writer_integrity(repo_id, repo_private, ctx);
        }
    } else {
        integrity = private_writer_integrity(repo_id, repo_private, ctx);
    }
}

Why This Matters

Eliminates one redundant JSON field lookup per get_pull_request / pull_request_read call. More importantly, it creates a single source of truth for how the PR number is extracted — future changes only need to touch one line instead of two. The ref borrows (ref num, ref number) keep the Option<String> alive through both uses without re-cloning.


Codebase Health Summary

  • Total Rust files: 9
  • Total lines: 14,205
  • Areas analyzed: lib.rs, tools.rs, labels/mod.rs, labels/response_paths.rs, labels/tool_rules.rs, labels/helpers.rs, labels/backend.rs, labels/response_items.rs, labels/constants.rs
  • Areas with no further improvements: tools.rs (well-tested, clean structure)

Generated by Rust Guard Improver • Run: §26220078438

Generated by Rust Guard Improver · ● 1.1M ·

  • expires on May 28, 2026, 10:27 AM UTC

Metadata

Metadata

Assignees

Type

No type
No fields configured for issues without a type.

Projects

No projects

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions