[rust-guard] Rust Guard: consolidate remaining split_once('/') call sites to use split_repo_id()

[github-actions]

🦀 Rust Guard Improvement Report

Improvement 1: Reuse split_repo_id() in remaining ad-hoc split_once('/') call sites

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

Problem

labels/helpers.rs already defines a private helper split_repo_id() (line 260) that splits "owner/repo" and rejects malformed input (empty owner/repo or an extra /):

fn split_repo_id(repo_id: &str) -> Option<(&str, &str)> {
    let (owner, repo) = repo_id.split_once('/')?;
    if owner.is_empty() || repo.is_empty() || repo.contains('/') {
        return None;
    }
    Some((owner, repo))
}

It is already reused at several call sites (e.g. lines 243, 690, 987, 1813, 1939, 2074), but four call sites still perform a raw, unvalidated repo_id.split_once('/') instead, which skips the empty-owner/empty-repo/extra-slash safety checks that split_repo_id provides:

• repo_visibility_secrecy_for_repo_id (line 1064)
• repo_visibility_private_for_repo_id (line 1075)
• extract_repo_info_from_search_query (line 1211)
• the owner-authored-commit writer-floor check inside the commit integrity function (line 2162)

Because split_repo_id returns None for a malformed id like "owner/" or "/repo" or "a/b/c", while raw split_once('/') would happily return ("owner", "") or ("", "repo") or ("a", "b/c"), these four sites can behave slightly differently (and less safely) than the rest of the codebase for malformed repo identifiers.

Suggested Change

Replace the raw .split_once('/') calls at these four sites with split_repo_id(...), matching the pattern already used elsewhere in the file.

Before

// line ~1064
pub(crate) fn repo_visibility_secrecy_for_repo_id(
    repo_id: &str,
    ctx: &PolicyContext,
) -> Vec<String> {
    if let Some((owner, repo)) = repo_id.split_once('/') {
        repo_visibility_secrecy(owner, repo, repo_id, ctx)
    } else {
        policy_private_scope_label("", "", repo_id, ctx)
    }
}

// line ~1075
pub(crate) fn repo_visibility_private_for_repo_id(repo_id: &str) -> Option<bool> {
    let (owner, repo) = repo_id.split_once('/')?;
    super::backend::is_repo_private(owner, repo)
}

// line ~1211 (inside extract_repo_info_from_search_query)
if let Some((owner, repo)) = repo_ref.split_once('/') {
    if !owner.is_empty() && !repo.is_empty() {
        ...
    }
}

// line ~2162
if let Some((owner, _repo)) = repo_full_name.split_once('/') {
    if author_login.eq_ignore_ascii_case(owner) {
        ...
    }
}

After

pub(crate) fn repo_visibility_secrecy_for_repo_id(
    repo_id: &str,
    ctx: &PolicyContext,
) -> Vec<String> {
    if let Some((owner, repo)) = split_repo_id(repo_id) {
        repo_visibility_secrecy(owner, repo, repo_id, ctx)
    } else {
        policy_private_scope_label("", "", repo_id, ctx)
    }
}

pub(crate) fn repo_visibility_private_for_repo_id(repo_id: &str) -> Option<bool> {
    let (owner, repo) = split_repo_id(repo_id)?;
    super::backend::is_repo_private(owner, repo)
}

// inside extract_repo_info_from_search_query
if let Some((owner, repo)) = split_repo_id(repo_ref) {
    // the explicit !owner.is_empty() && !repo.is_empty() check becomes redundant,
    // since split_repo_id already guarantees both are non-empty
    let owner = owner.to_string();
    let repo = repo.to_string();
    let repo_id = format_repo_id(&owner, &repo);
    return (owner, repo, repo_id);
}

if let Some((owner, _repo)) = split_repo_id(repo_full_name) {
    if author_login.eq_ignore_ascii_case(owner) {
        ...
    }
}

Note: split_repo_id additionally rejects ids containing an extra / in the repo segment (e.g. "owner/repo/extra"), which the raw split_once('/') calls do not — switching these sites tightens validation slightly but shouldn't change behavior for any well-formed owner/repo string seen in practice.

Why This Matters

Reduces duplicated validation logic to a single source of truth, and makes all four sites consistently reject malformed repo identifiers instead of silently producing an empty owner or repo string that could propagate into downstream backend calls or policy label decisions.

---

Improvement 2: Replace raw "get_issue" / "get_pull_request" literals in labels/mod.rs tests with tool_names constants

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

Problem

labels/constants.rs already defines a tool_names module (PULL_REQUEST_READ, GET_PULL_REQUEST, ISSUE_READ, GET_ISSUE) specifically to avoid raw string-literal typos for these tool names (see doc comment at constants.rs:141-145). However, labels/mod.rs still has three raw string-literal usages in tests that duplicate these values instead of using the constants:

• mod.rs:643 — "get_issue" in test_issue_desc_number_formatting
• mod.rs:659 — "get_issue" (same test, second case)
• mod.rs:717 — "get_pull_request" in test_apply_tool_labels_pull_request_read_matches_get_pull_request

Suggested Change

Import crate::labels::constants::tool_names (or reuse the existing super::constants import path used elsewhere in the test module) and replace the raw literals.

Before

let (_s1, _i1, desc1) = apply_tool_labels(
    "get_issue",
    &tool_args_str,
    ...
);
...
let (_s2, _i2, desc2) = apply_tool_labels(
    "get_issue",
    &tool_args_i64,
    ...
);
...
let expected = apply_tool_labels(
    "get_pull_request",
    &tool_args,
    ...
);

After

use super::constants::tool_names;
...
let (_s1, _i1, desc1) = apply_tool_labels(
    tool_names::GET_ISSUE,
    &tool_args_str,
    ...
);
...
let (_s2, _i2, desc2) = apply_tool_labels(
    tool_names::GET_ISSUE,
    &tool_args_i64,
    ...
);
...
let expected = apply_tool_labels(
    tool_names::GET_PULL_REQUEST,
    &tool_args,
    ...
);

Why This Matters

Keeps the already-established tool_names constants as the single source of truth for these tool-name strings, preventing future drift or typos between production code and tests, and completing the migration started in the 2026-08-17 change that introduced tool_names.

---

Codebase Health Summary

• Total Rust files: 9 (lib.rs, tools.rs, labels/mod.rs, labels/helpers.rs, labels/backend.rs, labels/tool_rules.rs, labels/response_items.rs, labels/response_paths.rs, labels/constants.rs)
• Total lines: ~20,600
• Areas analyzed: lib.rs (FFI/dispatch), labels/helpers.rs (repo-id parsing, secrecy/integrity helpers), labels/mod.rs (core label application + tests), labels/tool_rules.rs, labels/backend.rs, labels/response_items.rs, labels/response_paths.rs, labels/constants.rs
• Areas with no further improvements found this run: tools.rs, labels/tool_rules.rs (#[allow(clippy::too_many_arguments)] annotations are legitimate given many WASM-boundary policy parameters), lib.rs (clone() usages are already minimal and necessary for Mutex-guarded global state)

---

Generated by Rust Guard Improver • Run: 32352796579

Generated by Rust Guard Improver · auto · 43 AIC · ⊞ 11.3K · ◷

• expires on Aug 27, 2026, 9:17 AM UTC

[github-actions]

🤖 This is an automated response from Repo Assist.

Thanks for the detailed report! I implemented both suggested improvements in a small PR: replaced the four raw repo_id.split_once('/') call sites with the existing validated split_repo_id() helper, and replaced the raw "get_issue"/"get_pull_request" test literals with the tool_names constants. cargo build and cargo test (642 tests) pass, and cargo fmt --check shows no new diffs from this change.

PR: (linked automatically once merged into this workflow run)

Generated by Repo Assist · auto · 99 AIC · ⊞ 18.5K · ◷
_{Comment /repo-assist to run again}

_{Add this agentic workflow to your repo}

To install this agentic workflow, run

gh aw add githubnext/agentics/workflows/repo-assist.md@851905c06e905bf362a9f6cc54f912e3df747d55