🦀 Rust Guard Improvement Report

Improvement 1: Sort BLOCKED_TOOLS and use binary_search in is_blocked_tool

Category: Dead Code + API Surface
File(s): guards/github-guard/rust-guard/src/tools.rs
Effort: Small (< 15 min)
Risk: Low

Problem

BLOCKED_TOOLS is the only slice in tools.rs that is not sorted and whose lookup function uses .contains() (O(n) linear scan). WRITE_OPERATIONS and READ_WRITE_OPERATIONS both have:

• Sorted order enforced by a _are_sorted test
• binary_search in their predicate functions

BLOCKED_TOOLS is inconsistent on all three counts. Additionally, the 15-line doc comment on is_blocked_tool duplicates — verbatim — the inline comments already present on each entry in the BLOCKED_TOOLS const, creating a maintenance burden with no new information.

Suggested Change

1. Sort BLOCKED_TOOLS alphabetically (matches WRITE_OPERATIONS / READ_WRITE_OPERATIONS convention)
2. Switch is_blocked_tool to binary_search
3. Trim the redundant multi-paragraph doc to a concise one-liner
4. Add a blocked_tools_are_sorted test (mirrors the existing write_operations_are_sorted test)

Before

/// Check if a tool is unconditionally blocked (always denied regardless of agent integrity).
///
/// Blocked tools are listed here when the operation is considered too dangerous
/// to ever permit via an agent, even if the agent would otherwise satisfy the
/// integrity requirements for a normal write operation.
///
/// Current entries:
/// - `transfer_repository`: repository ownership transfer is irreversible and
///   must never be performed by an automated agent.
/// - `archive_repository`: archives a repository ...
/// - `unarchive_repository`: re-enables contributions ...
/// - `rename_repository`: renames a repository ...
/// - `create_agent_task`: creates a Copilot coding-agent job ...
pub const BLOCKED_TOOLS: &[&str] = &[
    "transfer_repository",
    "archive_repository",
    "unarchive_repository",
    "rename_repository",
    "create_agent_task",
];

pub fn is_blocked_tool(tool_name: &str) -> bool {
    BLOCKED_TOOLS.contains(&tool_name)
}

After

/// Tools that are unconditionally blocked regardless of agent integrity.
///
/// Keep sorted for `binary_search` correctness (see `blocked_tools_are_sorted` test).
/// Entries here should also appear in `WRITE_OPERATIONS` or `READ_WRITE_OPERATIONS`.
pub const BLOCKED_TOOLS: &[&str] = &[
    "archive_repository",   // repo settings change; unsupported
    "create_agent_task",    // unsupported agent-task creation
    "rename_repository",    // breaks clone URLs and integrations
    "transfer_repository",  // irreversible ownership transfer
    "unarchive_repository", // symmetric to archive_repository
];

/// Returns `true` if `tool_name` is in [`BLOCKED_TOOLS`] — denied regardless
/// of agent integrity level.
pub fn is_blocked_tool(tool_name: &str) -> bool {
    BLOCKED_TOOLS.binary_search(&tool_name).is_ok()
}

// In the tests module:
#[test]
fn blocked_tools_are_sorted() {
    let mut sorted = BLOCKED_TOOLS.to_vec();
    sorted.sort_unstable();
    assert_eq!(
        BLOCKED_TOOLS,
        sorted.as_slice(),
        "BLOCKED_TOOLS must be kept in sorted order for binary_search correctness"
    );
}

Why This Matters

• Consistency: all three "blocked/write/read-write" slices now follow the same sorted + binary_search + sorted-test pattern, making the convention self-evident to new contributors
• Doc hygiene: the removed doc walls duplicate // inline comments already on each const entry; a single pointer-sentence is sufficient
• Regression safety: the new test catches any future unsorted addition before it silently breaks lookups

Improvement 2: Promote CollaboratorPermission.login to #[cfg(test)]

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

Problem

CollaboratorPermission.login is annotated with #[cfg_attr(not(test), allow(dead_code))] (line 133), which silences the lint but keeps the field in production builds. The field is only ever read in test assertions (lines 1206, 1220, 1314). In non-test code, the field is populated but never consumed: one call site allocates Some(username.to_string()) on the cache-hit path (line 497) and one derives login from JSON and moves it into the struct on the API-call path (line 575).

Suppressing the lint is a half-measure. Gating the field with #[cfg(test)] removes it entirely from the WASM binary.

Suggested Change

Gate the field definition and its two struct-literal assignments with #[cfg(test)].

Before

#[derive(Debug, Clone)]
pub struct CollaboratorPermission {
    pub permission: Option<String>,
    #[cfg_attr(not(test), allow(dead_code))]
    pub login: Option<String>,
}

// — cache hit path (line ~495) —
return cached.map(|permission| CollaboratorPermission {
    permission: Some(permission),
    login: Some(username.to_string()),   // allocation; result never read
});

// — API call path (line ~575) —
Some(CollaboratorPermission { permission, login })  // login derived from JSON; never read

After

#[derive(Debug, Clone)]
pub struct CollaboratorPermission {
    pub permission: Option<String>,
    #[cfg(test)]
    pub login: Option<String>,
}

// — cache hit path —
return cached.map(|permission| CollaboratorPermission {
    permission: Some(permission),
    #[cfg(test)]
    login: Some(username.to_string()),
});

// — API call path —
// `login` local variable still computed for the log_info message above;
// just omit it from the struct literal in non-test builds:
Some(CollaboratorPermission {
    permission,
    #[cfg(test)]
    login,
})

No test changes are needed — perm.login.as_deref() assertions continue to compile and run under cargo test.

Why This Matters

• True dead-code removal: #[cfg_attr(..., allow(dead_code))] only hides the warning; #[cfg(test)] actually strips the field and its assignments from the WASM binary, saving one String heap allocation on every collaborator-permission lookup in production
• Signal clarity: future readers immediately see the field is test-only, not "might be used later"
• Pattern consistency: other test-only items in the codebase (secret_label, has_approval_label, etc.) already use #[cfg(test)]; this field should too

Codebase Health Summary

• Total Rust files: 10
• Total lines: ~14,964
• Areas analyzed: tools.rs, labels/backend.rs, labels/mod.rs, labels/constants.rs, labels/helpers.rs, lib.rs
• Areas with no further improvements: None — all modules still have open opportunities

Generated by Rust Guard Improver • Run: §26814371796

Generated by Rust Guard Improver · sonnet46 2.1M · ◷

• expires on Jun 9, 2026, 10:45 AM UTC