[rust-guard] Rust Guard: extract_items_array should return Option<&'static str> for items path, not "" sentinel

[github-actions]

🦀 Rust Guard Improvement Report

Improvement 1: Replace "" sentinel in extract_items_array with Option<&'static str>

Category: Type Safety
File(s): guards/github-guard/rust-guard/src/labels/helpers.rs, guards/github-guard/rust-guard/src/labels/response_paths.rs
Effort: Medium (15–30 min)
Risk: Low

Problem

extract_items_array returns (Option<&Vec<Value>>, &'static str) where the empty string "" serves as a sentinel for "bare root array" (and also "not found"):

pub fn extract_items_array(response: &Value) -> (Option<&Vec<Value>>, &'static str) {
    if let Some(arr) = response.as_array() {
        return (Some(arr), "");       // "" = bare root array
    }
    if let Some(arr) = response.get("items").and_then(|v| v.as_array()) {
        return (Some(arr), "/items");
    }
    // ...
    (None, "")                        // "" = not found (same sentinel, different meaning!)
}

This leaks into every caller. In response_paths.rs, three call sites manually convert back to Option<&'static str> before storing in PathLabelResult.items_path: Option<&'static str>:

// Repeated 3× in response_paths.rs (lines 215, 315, 637):
items_path: if items_path.is_empty() {
    None
} else {
    Some(items_path)
},

The PathLabelResult struct already uses Option<&'static str> for items_path — the empty-string sentinel is the wrong abstraction all along.

Suggested Change

Change the return type to (Option<&Vec<Value>>, Option<&'static str>), using None for "no items path key" instead of "".

Before

// helpers.rs
pub fn extract_items_array(response: &Value) -> (Option<&Vec<Value>>, &'static str) {
    if let Some(arr) = response.as_array() {
        return (Some(arr), "");
    }
    if let Some(arr) = response.get("items").and_then(|v| v.as_array()) {
        return (Some(arr), "/items");
    }
    if let Some(arr) = response.get("issues").and_then(|v| v.as_array()) {
        return (Some(arr), "/issues");
    }
    if let Some(arr) = response.get("pull_requests").and_then(|v| v.as_array()) {
        return (Some(arr), "/pull_requests");
    }
    if let Some((arr, pointer)) = find_graphql_nodes_with_path(response) {
        return (Some(arr), pointer);
    }
    (None, "")
}

// response_paths.rs (×3 call sites)
let (items, items_path) = extract_items_array(&actual_response);
// ...
make_item_path(&items_path, i)
// ...
items_path: if items_path.is_empty() {
    None
} else {
    Some(items_path)
},

After

// helpers.rs
pub fn extract_items_array(response: &Value) -> (Option<&Vec<Value>>, Option<&'static str>) {
    if let Some(arr) = response.as_array() {
        return (Some(arr), None);          // bare root array — no key
    }
    if let Some(arr) = response.get("items").and_then(|v| v.as_array()) {
        return (Some(arr), Some("/items"));
    }
    if let Some(arr) = response.get("issues").and_then(|v| v.as_array()) {
        return (Some(arr), Some("/issues"));
    }
    if let Some(arr) = response.get("pull_requests").and_then(|v| v.as_array()) {
        return (Some(arr), Some("/pull_requests"));
    }
    if let Some((arr, pointer)) = find_graphql_nodes_with_path(response) {
        return (Some(arr), Some(pointer));
    }
    (None, None)
}

// response_paths.rs (×3 call sites — boilerplate removed)
let (items, items_path) = extract_items_array(&actual_response);
// ...
make_item_path(items_path.unwrap_or(""), i)
// ...
items_path,  // already Option<&'static str> — no conversion needed

Also update the test assertions in helpers.rs and mod.rs (≈8 lines):

// was: assert_eq!(path, "");
assert_eq!(path, None);

// was: assert_eq!(path, "/items");
assert_eq!(path, Some("/items"));

Why This Matters

• Removes a hidden sentinel value ("") with two distinct meanings (bare array vs not-found)
• Eliminates 3 identical conversion blocks in response_paths.rs
• PathLabelResult.items_path is already Option<&'static str> — this makes the types consistent end-to-end
• Compiler enforces correct handling of the "no path" case rather than silently using an empty string

---

Improvement 2: Add list_commits tests to response_paths.rs

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

Problem

response_paths.rs has only 4 tests (754 lines). The list_commits branch, which contains important logic for differentiating default-branch commits (merged-level integrity) vs feature-branch commits (writer-level or empty), has zero test coverage.

The key logic being untested:

let is_default_branch = is_default_branch_ref(sha);  // sha = "" / "main" / "master" / "HEAD"
// ...
integrity: if is_default_branch {
    merged_integrity(&default_repo, ctx)
} else if repo_private {
    writer_integrity(&default_repo, ctx)
} else {
    vec![]
}

Suggested Change

Add two tests in response_paths.rs under the existing mod tests block:

Before

// No list_commits tests exist

After

#[test]
fn list_commits_default_branch_gets_merged_integrity() {
    let tool_args = json!({"owner": "octocat", "repo": "hello-world", "sha": "main"});
    let response = json!([{
        "sha": "abc1234def5678",
        "commit": {"message": "fix: bug"},
        "author": {"login": "octocat"},
        "author_association": "OWNER"
    }]);

    let result = label_response_paths("list_commits", &tool_args, &response, &ctx())
        .expect("list_commits should produce path labels");

    assert_eq!(result.labeled_paths.len(), 1);
    assert_eq!(result.labeled_paths[0].path, "/0");

    // Default branch (main) → merged-level integrity
    let integrity = &result.labeled_paths[0].labels.integrity;
    assert!(
        integrity.iter().any(|l| l.starts_with("merged:")),
        "default-branch commit should have merged-level integrity, got: {:?}",
        integrity
    );
}

#[test]
fn list_commits_feature_branch_public_repo_gets_no_items_path() {
    let tool_args = json!({"owner": "octocat", "repo": "hello-world", "sha": "feature/my-branch"});
    let response = json!([{
        "sha": "deadbeef12345678",
        "commit": {"message": "wip"},
        "author_association": "CONTRIBUTOR"
    }]);

    let result = label_response_paths("list_commits", &tool_args, &response, &ctx())
        .expect("list_commits should produce path labels");

    assert_eq!(result.labeled_paths.len(), 1);
    // Root array → items_path should be None
    assert!(
        result.items_path.is_none(),
        "list_commits root array should have None items_path"
    );
    // Non-default branch of public repo → no merged integrity
    let integrity = &result.labeled_paths[0].labels.integrity;
    assert!(
        !integrity.iter().any(|l| l.starts_with("merged:")),
        "feature-branch commit should NOT have merged-level integrity, got: {:?}",
        integrity
    );
}

Why This Matters

• The sha field drives the is_default_branch → merged-level integrity promotion, which is a security-relevant decision. A regression here (e.g. treating all commits as default-branch) would over-elevate integrity.
• The items_path: None case (root array) is tested indirectly as a side-effect of the second test.
• Both tests are self-contained and require no backend mocking.

---

Codebase Health Summary

• Total Rust files: 10
• Total lines: 14,117
• Areas analyzed: All source files
• Areas with no further improvements: labels/backend.rs (rate-limit extraction, redundant branch, find_org duplication), tools.rs (BLOCKED_TOOLS, is_blocked_tool tests), labels/constants.rs (ORDER_LOW_TO_HIGH_PIPED, scope constants), labels/helpers.rs (strip_query_punctuation, TRUSTED_FIRST_PARTY_BOTS array, DEFAULT_BRANCH_NAMES array)

---

Generated by Rust Guard Improver • Run: §26155959348

References:

• §26155959348

Generated by Rust Guard Improver · ● 2.8M · ◷

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