diff --git a/.github/workflows/repo-assist.lock.yml b/.github/workflows/repo-assist.lock.yml index b9d1a763c..daf9f6e14 100644 --- a/.github/workflows/repo-assist.lock.yml +++ b/.github/workflows/repo-assist.lock.yml @@ -399,11 +399,11 @@ jobs: - name: Create gh-aw temp directory run: bash ${RUNNER_TEMP}/gh-aw/actions/create_gh_aw_tmp_dir.sh # NOTE: Local container build kept for debugging. Uncomment to test unpublished changes. - # - name: Build local MCPG container (debugging only) - # run: | - # rustup target add wasm32-wasip1 - # cd guards/github-guard/rust-guard && ./build.sh && cd ../../.. - # docker build . -t ghcr.io/github/gh-aw-mcpg:local + - name: Build local MCPG container (debugging only) + run: | + rustup target add wasm32-wasip1 + cd guards/github-guard/rust-guard && ./build.sh && cd ../../.. + docker build . -t ghcr.io/github/gh-aw-mcpg:local - name: Start DIFC proxy for pre-agent gh calls env: GH_TOKEN: ${{ github.token }} @@ -417,7 +417,7 @@ jobs: -e GH_TOKEN \ -e DEBUG='*' \ -v "$PROXY_LOG_DIR:$PROXY_LOG_DIR" \ - ghcr.io/github/gh-aw-mcpg:v0.1.24 proxy \ + ghcr.io/github/gh-aw-mcpg:local proxy \ --policy "$POLICY" \ --listen 0.0.0.0:18443 \ --log-dir "$PROXY_LOG_DIR" \ @@ -514,7 +514,7 @@ jobs: - name: Install AWF binary run: bash ${RUNNER_TEMP}/gh-aw/actions/install_awf_binary.sh v0.24.3 - name: Download container images - run: bash ${RUNNER_TEMP}/gh-aw/actions/download_docker_images.sh ghcr.io/github/gh-aw-firewall/agent:0.24.3 ghcr.io/github/gh-aw-firewall/api-proxy:0.24.3 ghcr.io/github/gh-aw-firewall/squid:0.24.3 ghcr.io/github/gh-aw-mcpg:v0.1.24 ghcr.io/github/github-mcp-server:v0.32.0 node:lts-alpine + run: bash ${RUNNER_TEMP}/gh-aw/actions/download_docker_images.sh ghcr.io/github/gh-aw-firewall/agent:0.24.3 ghcr.io/github/gh-aw-firewall/api-proxy:0.24.3 ghcr.io/github/gh-aw-firewall/squid:0.24.3 ghcr.io/github/github-mcp-server:v0.32.0 node:lts-alpine - name: Write Safe Outputs Config run: | mkdir -p ${RUNNER_TEMP}/gh-aw/safeoutputs @@ -862,7 +862,7 @@ jobs: export DEBUG="*" export GH_AW_ENGINE="copilot" - export MCP_GATEWAY_DOCKER_COMMAND='docker run -i --rm --network host -v /var/run/docker.sock:/var/run/docker.sock -e MCP_GATEWAY_PORT -e MCP_GATEWAY_DOMAIN -e MCP_GATEWAY_API_KEY -e MCP_GATEWAY_PAYLOAD_DIR -e MCP_GATEWAY_PAYLOAD_SIZE_THRESHOLD -e DEBUG -e MCP_GATEWAY_LOG_DIR -e GH_AW_MCP_LOG_DIR -e GH_AW_SAFE_OUTPUTS -e GH_AW_SAFE_OUTPUTS_CONFIG_PATH -e GH_AW_SAFE_OUTPUTS_TOOLS_PATH -e GH_AW_ASSETS_BRANCH -e GH_AW_ASSETS_MAX_SIZE_KB -e GH_AW_ASSETS_ALLOWED_EXTS -e DEFAULT_BRANCH -e GITHUB_MCP_SERVER_TOKEN -e GITHUB_MCP_GUARD_MIN_INTEGRITY -e GITHUB_MCP_GUARD_REPOS -e GITHUB_REPOSITORY -e GITHUB_SERVER_URL -e GITHUB_SHA -e GITHUB_WORKSPACE -e GITHUB_TOKEN -e GITHUB_RUN_ID -e GITHUB_RUN_NUMBER -e GITHUB_RUN_ATTEMPT -e GITHUB_JOB -e GITHUB_ACTION -e GITHUB_EVENT_NAME -e GITHUB_EVENT_PATH -e GITHUB_ACTOR -e GITHUB_ACTOR_ID -e GITHUB_TRIGGERING_ACTOR -e GITHUB_WORKFLOW -e GITHUB_WORKFLOW_REF -e GITHUB_WORKFLOW_SHA -e GITHUB_REF -e GITHUB_REF_NAME -e GITHUB_REF_TYPE -e GITHUB_HEAD_REF -e GITHUB_BASE_REF -e GH_AW_SAFE_OUTPUTS_PORT -e GH_AW_SAFE_OUTPUTS_API_KEY -v /tmp/gh-aw/mcp-payloads:/tmp/gh-aw/mcp-payloads:rw -v /opt:/opt:ro -v /tmp:/tmp:rw -v '"${GITHUB_WORKSPACE}"':'"${GITHUB_WORKSPACE}"':rw ghcr.io/github/gh-aw-mcpg:v0.1.24' + export MCP_GATEWAY_DOCKER_COMMAND='docker run -i --rm --network host -v /var/run/docker.sock:/var/run/docker.sock -e MCP_GATEWAY_PORT -e MCP_GATEWAY_DOMAIN -e MCP_GATEWAY_API_KEY -e MCP_GATEWAY_PAYLOAD_DIR -e MCP_GATEWAY_PAYLOAD_SIZE_THRESHOLD -e DEBUG -e MCP_GATEWAY_LOG_DIR -e GH_AW_MCP_LOG_DIR -e GH_AW_SAFE_OUTPUTS -e GH_AW_SAFE_OUTPUTS_CONFIG_PATH -e GH_AW_SAFE_OUTPUTS_TOOLS_PATH -e GH_AW_ASSETS_BRANCH -e GH_AW_ASSETS_MAX_SIZE_KB -e GH_AW_ASSETS_ALLOWED_EXTS -e DEFAULT_BRANCH -e GITHUB_MCP_SERVER_TOKEN -e GITHUB_MCP_GUARD_MIN_INTEGRITY -e GITHUB_MCP_GUARD_REPOS -e GITHUB_REPOSITORY -e GITHUB_SERVER_URL -e GITHUB_SHA -e GITHUB_WORKSPACE -e GITHUB_TOKEN -e GITHUB_RUN_ID -e GITHUB_RUN_NUMBER -e GITHUB_RUN_ATTEMPT -e GITHUB_JOB -e GITHUB_ACTION -e GITHUB_EVENT_NAME -e GITHUB_EVENT_PATH -e GITHUB_ACTOR -e GITHUB_ACTOR_ID -e GITHUB_TRIGGERING_ACTOR -e GITHUB_WORKFLOW -e GITHUB_WORKFLOW_REF -e GITHUB_WORKFLOW_SHA -e GITHUB_REF -e GITHUB_REF_NAME -e GITHUB_REF_TYPE -e GITHUB_HEAD_REF -e GITHUB_BASE_REF -e GH_AW_SAFE_OUTPUTS_PORT -e GH_AW_SAFE_OUTPUTS_API_KEY -v /tmp/gh-aw/mcp-payloads:/tmp/gh-aw/mcp-payloads:rw -v /opt:/opt:ro -v /tmp:/tmp:rw -v '"${GITHUB_WORKSPACE}"':'"${GITHUB_WORKSPACE}"':rw ghcr.io/github/gh-aw-mcpg:local' mkdir -p /home/runner/.copilot cat << GH_AW_MCP_CONFIG_EOF | bash ${RUNNER_TEMP}/gh-aw/actions/start_mcp_gateway.sh diff --git a/guards/github-guard/rust-guard/src/labels/helpers.rs b/guards/github-guard/rust-guard/src/labels/helpers.rs index 65f4680f2..6dc9b353f 100644 --- a/guards/github-guard/rust-guard/src/labels/helpers.rs +++ b/guards/github-guard/rust-guard/src/labels/helpers.rs @@ -568,22 +568,106 @@ pub fn extract_repo_from_item(item: &Value) -> String { String::new() } -/// Extract items array from response, handling both root array and items field -/// Returns (Option, items_path) where items_path is "" for root array, "/items" for items field -pub fn extract_items_array(response: &Value) -> (Option<&Vec>, &'static str) { +/// Extract items array from response, handling REST, items field, and GraphQL formats. +/// Returns (Option, items_path) where items_path is a JSON Pointer prefix: +/// - "" for root array +/// - "/items" for {items: [...]} +/// - "/data/repository/pullRequests/nodes" for GraphQL nested format +/// - etc. +pub fn extract_items_array(response: &Value) -> (Option<&Vec>, String) { + // REST formats if let Some(arr) = response.as_array() { - (Some(arr), "") - } else if let Some(arr) = response.get("items").and_then(|v| v.as_array()) { - (Some(arr), "/items") - } else if let Some(arr) = response.get("issues").and_then(|v| v.as_array()) { - (Some(arr), "/issues") - } else if let Some(arr) = response.get("pull_requests").and_then(|v| v.as_array()) { - (Some(arr), "/pull_requests") - } else { - (None, "") + return (Some(arr), String::new()); + } + if let Some(arr) = response.get("items").and_then(|v| v.as_array()) { + return (Some(arr), "/items".to_string()); + } + if let Some(arr) = response.get("issues").and_then(|v| v.as_array()) { + return (Some(arr), "/issues".to_string()); + } + if let Some(arr) = response.get("pull_requests").and_then(|v| v.as_array()) { + return (Some(arr), "/pull_requests".to_string()); + } + + // GraphQL format: data.repository..nodes or data.search.nodes + if let Some(data) = response.get("data") { + // data.repository..nodes (issues, pullRequests, discussions, etc.) + if let Some(repo) = data.get("repository") { + for (field, pointer) in GRAPHQL_COLLECTION_FIELDS { + if let Some(arr) = repo.get(*field).and_then(|v| v.get("nodes")).and_then(|v| v.as_array()) { + return (Some(arr), pointer.to_string()); + } + } + } + // data.search.nodes + if let Some(arr) = data.get("search").and_then(|v| v.get("nodes")).and_then(|v| v.as_array()) { + return (Some(arr), "/data/search/nodes".to_string()); + } + // data.search.edges[].node — flatten into nodes + // (not supported as direct reference; caller should use search.nodes form) + } + + (None, String::new()) +} + +/// GraphQL collection fields under data.repository and their JSON Pointer paths. +const GRAPHQL_COLLECTION_FIELDS: &[(&str, &str)] = &[ + ("issues", "/data/repository/issues/nodes"), + ("pullRequests", "/data/repository/pullRequests/nodes"), + ("discussions", "/data/repository/discussions/nodes"), + ("discussionCategories", "/data/repository/discussionCategories/nodes"), +]; + +/// Extract the items array from a GraphQL response. +/// Traverses data.repository..nodes and data.search.nodes paths. +pub fn extract_graphql_nodes(response: &Value) -> Option<&Vec> { + let data = response.get("data")?; + + // data.repository..nodes + if let Some(repo) = data.get("repository") { + for (field, _) in GRAPHQL_COLLECTION_FIELDS { + if let Some(arr) = repo.get(*field).and_then(|v| v.get("nodes")).and_then(|v| v.as_array()) { + return Some(arr); + } + } + } + // data.search.nodes + if let Some(arr) = data.get("search").and_then(|v| v.get("nodes")).and_then(|v| v.as_array()) { + return Some(arr); } + + None } +/// Returns true if the response is a GraphQL wrapper (has a "data" key). +/// Used to prevent treating the entire GraphQL object as a single item. +pub fn is_graphql_wrapper(response: &Value) -> bool { + response.get("data").is_some() +} + +/// Extract a single object from a GraphQL response for singular queries. +/// Traverses data.repository. for fields like "issue", "pullRequest". +pub fn extract_graphql_single_object(response: &Value) -> Option<&Value> { + let data = response.get("data")?; + let repo = data.get("repository")?; + + for field in GRAPHQL_SINGLE_OBJECT_FIELDS { + if let Some(obj) = repo.get(*field) { + if obj.is_object() { + return Some(obj); + } + } + } + None +} + +/// GraphQL singular object fields under data.repository. +const GRAPHQL_SINGLE_OBJECT_FIELDS: &[&str] = &[ + "issue", + "pullRequest", + "discussion", +]; + /// Generate JSON Pointer path for an item index in a collection /// Returns a path like "/items/0" or "/0" depending on the items_path #[inline] @@ -788,6 +872,17 @@ fn extract_author_login(item: &Value) -> &str { get_nested_str(item, "author", "login") } +/// Check whether an item contains an `author_association` (or `authorAssociation`) field. +pub fn has_author_association(item: &Value) -> bool { + item.get("author_association") + .and_then(|v| v.as_str()) + .is_some() + || item + .get("authorAssociation") + .and_then(|v| v.as_str()) + .is_some() +} + /// Extract author_association from an item and return initial integrity floor. /// Trusted first-party GitHub bots and any gateway-configured trusted bots are /// elevated to approved (writer) integrity regardless of their author_association value. @@ -840,6 +935,8 @@ pub fn is_default_branch_commit_context(tool_name: &str, sha_or_ref: &str) -> bo /// - public forked PR => unapproved /// - public direct PR => approved /// - PR with an approval label => at least approved +/// - Backend enrichment: when `author_association` is missing from the item, +/// fetch the individual PR via REST to get the correct association and fork status. pub fn pr_integrity( item: &Value, repo_full_name: &str, @@ -861,12 +958,72 @@ pub fn pr_integrity( let mut integrity = author_association_floor(item, repo_full_name, ctx); // Check if PR is merged (either merged_at field exists or merged boolean is true) - let is_merged = item + let mut is_merged = item .get(field_names::MERGED_AT) .map(|v| !v.is_null()) .or_else(|| item.get(field_names::MERGED).and_then(|v| v.as_bool())) .unwrap_or(false); + // Track whether fork status was enriched from the backend + let mut effective_is_forked = is_forked; + + // Backend enrichment: when author_association is absent from the response + // (e.g. GitHub MCP Server omits it from MinimalPullRequest), fetch the + // individual PR via REST to obtain the correct association, fork status, + // and merge status. + if integrity.is_empty() && !has_author_association(item) && !repo_private { + if let Some(number) = item.get("number").and_then(|v| v.as_u64()) { + let (owner, repo) = repo_full_name.split_once('/').unwrap_or(("", "")); + if !owner.is_empty() && !repo.is_empty() { + let number_str = number.to_string(); + if let Some(facts) = + super::backend::get_pull_request_facts(owner, repo, &number_str) + { + crate::log_debug(&format!( + "[integrity] pr:{}#{} enriched: author_association={:?}, is_forked={:?}, is_merged={}", + repo_full_name, number, facts.author_association, facts.is_forked, facts.is_merged + )); + let enriched_floor = author_association_floor_from_str( + repo_full_name, + facts.author_association.as_deref(), + ctx, + ); + // Elevate trusted bots + let enriched_floor = if let Some(ref login) = facts.author_login { + if is_trusted_first_party_bot(login) + || is_configured_trusted_bot(login, ctx) + { + max_integrity( + repo_full_name, + enriched_floor, + writer_integrity(repo_full_name, ctx), + ctx, + ) + } else { + enriched_floor + } + } else { + enriched_floor + }; + integrity = + max_integrity(repo_full_name, integrity, enriched_floor, ctx); + // Use enriched fork/merge status if missing from item + if effective_is_forked.is_none() { + effective_is_forked = facts.is_forked; + } + if !is_merged && facts.is_merged { + is_merged = true; + } + } else { + crate::log_debug(&format!( + "[integrity] pr:{}#{} enrichment failed (backend returned None)", + repo_full_name, number + )); + } + } + } + } + if repo_private { integrity = max_integrity( repo_full_name, @@ -875,7 +1032,7 @@ pub fn pr_integrity( ctx, ); } else { - integrity = match is_forked { + integrity = match effective_is_forked { Some(true) => max_integrity( repo_full_name, integrity, @@ -927,6 +1084,9 @@ pub fn pr_integrity( /// - private repo issues => approved /// - public repo issues => no integrity /// - Issue with an approval label => at least approved +/// - Backend enrichment: when `author_association` is missing from the item +/// (e.g. GitHub MCP Server GraphQL path omits it), fetch the individual issue +/// via REST to get the correct association value. pub fn issue_integrity( item: &Value, repo_full_name: &str, @@ -945,6 +1105,38 @@ pub fn issue_integrity( } let mut integrity = author_association_floor(item, repo_full_name, ctx); + + // Backend enrichment: when author_association is absent from the response + // (e.g. GitHub MCP Server's list_issues GraphQL path omits it), fetch the + // individual issue via REST to obtain the correct value. This avoids + // incorrectly assigning "none" integrity to members/collaborators. + if integrity.is_empty() && !has_author_association(item) && !repo_private { + if let Some(number) = item.get("number").and_then(|v| v.as_u64()) { + let (owner, repo) = repo_full_name.split_once('/').unwrap_or(("", "")); + if !owner.is_empty() && !repo.is_empty() { + let number_str = number.to_string(); + if let Some(association) = + super::backend::get_issue_author_association(owner, repo, &number_str) + { + crate::log_debug(&format!( + "[integrity] issue:{}#{} enriched author_association='{}'", + repo_full_name, number, association + )); + // Re-check trusted bot status with enriched login + let enriched_floor = + author_association_floor_from_str(repo_full_name, Some(&association), ctx); + integrity = + max_integrity(repo_full_name, integrity, enriched_floor, ctx); + } else { + crate::log_debug(&format!( + "[integrity] issue:{}#{} enrichment failed (backend returned None)", + repo_full_name, number + )); + } + } + } + } + if repo_private { integrity = max_integrity( repo_full_name, diff --git a/guards/github-guard/rust-guard/src/labels/mod.rs b/guards/github-guard/rust-guard/src/labels/mod.rs index b8ebd17ca..ddadb130e 100644 --- a/guards/github-guard/rust-guard/src/labels/mod.rs +++ b/guards/github-guard/rust-guard/src/labels/mod.rs @@ -42,13 +42,14 @@ pub use constants::MEDIUM_BUFFER_SIZE; // re-exported for external modules and tests, not used within mod.rs itself #[allow(unused_imports)] pub use helpers::{ - blocked_integrity, commit_integrity, ensure_integrity_baseline, extract_items_array, + blocked_integrity, commit_integrity, ensure_integrity_baseline, extract_graphql_nodes, + extract_graphql_single_object, extract_items_array, extract_number_as_string, extract_repo_from_item, extract_repo_info, extract_repo_info_from_search_query, get_bool_or, get_nested_str, get_str_or, - is_blocked_user, is_bot, issue_integrity, limit_items_with_log, make_item_path, merged_integrity, - none_integrity, pr_integrity, private_scope_label, private_user_label, project_github_label, - reader_integrity, secret_label, writer_integrity, MinIntegrity, PolicyContext, PolicyScopeEntry, - ScopeKind, + has_author_association, is_blocked_user, is_bot, is_graphql_wrapper, issue_integrity, + limit_items_with_log, make_item_path, merged_integrity, none_integrity, pr_integrity, + private_scope_label, private_user_label, project_github_label, reader_integrity, secret_label, + writer_integrity, MinIntegrity, PolicyContext, PolicyScopeEntry, ScopeKind, }; #[cfg(test)] pub use helpers::has_approval_label; @@ -1706,6 +1707,60 @@ mod tests { ); } + // ========================================================================= + // Tests for has_author_association helper + // ========================================================================= + + #[test] + fn test_has_author_association_rest_field() { + let item = json!({"author_association": "MEMBER", "number": 1}); + assert!(has_author_association(&item)); + } + + #[test] + fn test_has_author_association_graphql_field() { + let item = json!({"authorAssociation": "OWNER", "number": 1}); + assert!(has_author_association(&item)); + } + + #[test] + fn test_has_author_association_missing() { + let item = json!({"user": {"login": "lpcox"}, "number": 2093}); + assert!(!has_author_association(&item)); + } + + #[test] + fn test_has_author_association_null_value() { + let item = json!({"author_association": null}); + assert!(!has_author_association(&item)); + } + + #[test] + fn test_issue_integrity_with_missing_author_association_private_repo() { + // Private repos don't need enrichment — they get writer integrity regardless + let ctx = default_ctx(); + let item = json!({"user": {"login": "lpcox"}, "number": 2093}); + assert_eq!( + issue_integrity(&item, "github/gh-aw-mcpg", true, &ctx), + writer_integrity("github/gh-aw-mcpg", &ctx) + ); + } + + #[test] + fn test_issue_integrity_with_author_association_present_public_repo() { + // When author_association is present, no enrichment needed + let ctx = default_ctx(); + let item = json!({ + "user": {"login": "lpcox"}, + "number": 2093, + "author_association": "MEMBER" + }); + assert_eq!( + issue_integrity(&item, "github/gh-aw-mcpg", false, &ctx), + writer_integrity("github/gh-aw-mcpg", &ctx) + ); + } + // ========================================================================= // Tests for 22 new tools added in feat/guard-tool-coverage // @@ -2709,4 +2764,210 @@ mod tests { assert!(result.is_none(), "pull_request_read single-object response should return None for path labeling"); } + + // ========================================================================= + // GraphQL response format tests + // ========================================================================= + + #[test] + fn test_extract_items_array_graphql_pull_requests() { + let response = json!({ + "data": { + "repository": { + "pullRequests": { + "nodes": [ + {"number": 1, "title": "PR 1"}, + {"number": 2, "title": "PR 2"} + ] + } + } + } + }); + let (items, path) = extract_items_array(&response); + assert!(items.is_some(), "Should extract pullRequests.nodes from GraphQL"); + assert_eq!(items.unwrap().len(), 2); + assert_eq!(path, "/data/repository/pullRequests/nodes"); + } + + #[test] + fn test_extract_items_array_graphql_issues() { + let response = json!({ + "data": { + "repository": { + "issues": { + "nodes": [ + {"number": 10, "title": "Issue 10"} + ] + } + } + } + }); + let (items, path) = extract_items_array(&response); + assert!(items.is_some(), "Should extract issues.nodes from GraphQL"); + assert_eq!(items.unwrap().len(), 1); + assert_eq!(path, "/data/repository/issues/nodes"); + } + + #[test] + fn test_extract_items_array_graphql_search() { + let response = json!({ + "data": { + "search": { + "nodes": [ + {"number": 5, "__typename": "PullRequest"} + ] + } + } + }); + let (items, path) = extract_items_array(&response); + assert!(items.is_some(), "Should extract search.nodes from GraphQL"); + assert_eq!(items.unwrap().len(), 1); + assert_eq!(path, "/data/search/nodes"); + } + + #[test] + fn test_extract_graphql_single_object_issue() { + let response = json!({ + "data": { + "repository": { + "issue": { + "number": 42, + "title": "Bug report", + "author": {"login": "testuser"}, + "authorAssociation": "MEMBER" + } + } + } + }); + let obj = extract_graphql_single_object(&response); + assert!(obj.is_some(), "Should extract single issue from GraphQL"); + assert_eq!(obj.unwrap()["number"], 42); + } + + #[test] + fn test_extract_graphql_single_object_pull_request() { + let response = json!({ + "data": { + "repository": { + "pullRequest": { + "number": 99, + "title": "Feature PR", + "merged": true, + "author": {"login": "dev"}, + "authorAssociation": "MEMBER" + } + } + } + }); + let obj = extract_graphql_single_object(&response); + assert!(obj.is_some(), "Should extract single pullRequest from GraphQL"); + assert_eq!(obj.unwrap()["number"], 99); + } + + #[test] + fn test_is_graphql_wrapper() { + assert!(is_graphql_wrapper(&json!({"data": {"repository": {}}}))); + assert!(!is_graphql_wrapper(&json!({"number": 1, "title": "PR"}))); + assert!(!is_graphql_wrapper(&json!([{"number": 1}]))); + } + + #[test] + fn test_graphql_list_pull_requests_response_labeling() { + let ctx = default_ctx(); + let tool_args = json!({"owner": "testorg", "repo": "testrepo"}); + // GraphQL response format for list_pull_requests + let response = json!({ + "data": { + "repository": { + "pullRequests": { + "nodes": [ + { + "number": 1, + "title": "Merged PR", + "merged": true, + "author": {"login": "dev"}, + "authorAssociation": "MEMBER" + } + ] + } + } + } + }); + + // response_items should label the PR from GraphQL format + let items = label_response_items("list_pull_requests", &tool_args, &response, &ctx); + assert_eq!(items.len(), 1, "Should find 1 PR in GraphQL response"); + assert!( + items[0].labels.integrity.iter().any(|t| t == "approved" || t.starts_with("approved:")), + "Merged MEMBER PR should get approved integrity, got: {:?}", + items[0].labels.integrity + ); + + // response_paths should also work with GraphQL format + let paths = label_response_paths("list_pull_requests", &tool_args, &response, &ctx); + assert!(paths.is_some(), "Should generate path labels for GraphQL PR response"); + let paths = paths.unwrap(); + assert_eq!(paths.labeled_paths.len(), 1); + assert!( + paths.labeled_paths[0].path.contains("pullRequests/nodes/0"), + "Path should reference GraphQL nodes, got: {}", + paths.labeled_paths[0].path + ); + } + + #[test] + fn test_graphql_list_issues_response_labeling() { + let ctx = default_ctx(); + let tool_args = json!({"owner": "testorg", "repo": "testrepo"}); + let response = json!({ + "data": { + "repository": { + "issues": { + "nodes": [ + { + "number": 10, + "title": "Bug", + "author": {"login": "contributor"}, + "authorAssociation": "CONTRIBUTOR" + } + ] + } + } + } + }); + + let items = label_response_items("list_issues", &tool_args, &response, &ctx); + assert_eq!(items.len(), 1, "Should find 1 issue in GraphQL response"); + + let paths = label_response_paths("list_issues", &tool_args, &response, &ctx); + assert!(paths.is_some(), "Should generate path labels for GraphQL issue response"); + let paths = paths.unwrap(); + assert_eq!(paths.labeled_paths.len(), 1); + assert!( + paths.labeled_paths[0].path.contains("issues/nodes/0"), + "Path should reference GraphQL nodes, got: {}", + paths.labeled_paths[0].path + ); + } + + #[test] + fn test_graphql_wrapper_not_treated_as_single_item() { + let ctx = default_ctx(); + let tool_args = json!({"owner": "testorg", "repo": "testrepo"}); + // A GraphQL response with no recognized collection field should NOT be + // treated as a single PR/issue item. + let response = json!({ + "data": { + "viewer": { + "login": "testuser" + } + } + }); + + let items = label_response_items("list_pull_requests", &tool_args, &response, &ctx); + assert_eq!(items.len(), 0, "GraphQL wrapper should not be treated as a single PR"); + + let items = label_response_items("list_issues", &tool_args, &response, &ctx); + assert_eq!(items.len(), 0, "GraphQL wrapper should not be treated as a single issue"); + } } diff --git a/guards/github-guard/rust-guard/src/labels/response_items.rs b/guards/github-guard/rust-guard/src/labels/response_items.rs index ff0e72e3c..b5d90361d 100644 --- a/guards/github-guard/rust-guard/src/labels/response_items.rs +++ b/guards/github-guard/rust-guard/src/labels/response_items.rs @@ -26,6 +26,13 @@ pub fn label_response_items( ) -> Vec { let mut labeled_items = vec![]; + // Skip labeling for error responses (e.g. 404 Not Found). + // Resource-level labels from tool_rules handle these cases. + if response.get("isError").and_then(|v| v.as_bool()) == Some(true) { + crate::log_info("label_response_items: skipping error response (isError=true)"); + return labeled_items; + } + // MCP responses are wrapped in {"content":[{"type":"text","text":"..."}]} // Extract the actual response from content[0].text if needed let actual_response = extract_mcp_response(response); @@ -92,14 +99,32 @@ pub fn label_response_items( // === Pull Requests - label by merged state === "list_pull_requests" | "search_pull_requests" | "pull_request_read" | "get_pull_request" => { - // Handle array, {items: [...]}, or single object response. + // For pull_request_read sub-methods that return non-PR objects (e.g. + // get_check_runs, get_files, get_review_comments, get_reviews, + // get_comments, get_diff, get_status), skip per-item response labeling. + // The resource-level labels from tool_rules (which call + // get_pull_request_facts) provide correct PR-scoped integrity. + let method = tool_args + .get("method") + .and_then(|v| v.as_str()) + .unwrap_or(""); + if tool_name == "pull_request_read" && !method.is_empty() && method != "get" { + // Fall through — use resource-level labels from tool_rules + } else { + // Handle array, {items: [...]}, GraphQL nested, GraphQL single, or REST single object. // Work directly with &[Value] slices to avoid allocating a Vec<&Value>. let single_item_buf; + let graphql_single_buf; let items: &[Value] = if let Some(arr) = actual_response.as_array() { arr.as_slice() } else if let Some(items_arr) = actual_response.get("items").and_then(|v| v.as_array()) { items_arr.as_slice() - } else if actual_response.is_object() { + } else if let Some(nodes) = extract_graphql_nodes(&actual_response) { + nodes.as_slice() + } else if let Some(obj) = extract_graphql_single_object(&actual_response) { + graphql_single_buf = [obj.clone()]; + &graphql_single_buf + } else if actual_response.is_object() && !is_graphql_wrapper(&actual_response) { single_item_buf = [actual_response.clone()]; &single_item_buf } else { @@ -169,11 +194,22 @@ pub fn label_response_items( }); } } + } // end else (non-sub-method) } // === Issues - label by author status === "list_issues" | "search_issues" | "get_issue" | "issue_read" => { - // Handle single issue or array of issues + // For issue_read sub-methods that return non-issue objects (e.g. + // get_comments, get_sub_issues, get_labels), skip per-item labeling. + // Resource-level labels from tool_rules provide correct issue-scoped integrity. + let method = tool_args + .get("method") + .and_then(|v| v.as_str()) + .unwrap_or(""); + if tool_name == "issue_read" && !method.is_empty() && method != "get" { + // Fall through — use resource-level labels from tool_rules + } else { + // Handle single issue, array of issues, GraphQL nested, or GraphQL single object let all_items: Vec<&Value> = if actual_response.is_array() { actual_response .as_array() @@ -181,7 +217,11 @@ pub fn label_response_items( .unwrap_or_default() } else if let Some(items_arr) = actual_response.get("items").and_then(|v| v.as_array()) { items_arr.iter().collect() - } else if actual_response.is_object() { + } else if let Some(nodes) = extract_graphql_nodes(&actual_response) { + nodes.iter().collect() + } else if let Some(obj) = extract_graphql_single_object(&actual_response) { + vec![obj] + } else if actual_response.is_object() && !is_graphql_wrapper(&actual_response) { vec![&actual_response] } else { Vec::new() @@ -234,6 +274,7 @@ pub fn label_response_items( }, }); } + } // end else (non-sub-method) } // === File Contents - repo-scoped secrecy === diff --git a/guards/github-guard/rust-guard/src/labels/response_paths.rs b/guards/github-guard/rust-guard/src/labels/response_paths.rs index e8729c308..123d53c91 100644 --- a/guards/github-guard/rust-guard/src/labels/response_paths.rs +++ b/guards/github-guard/rust-guard/src/labels/response_paths.rs @@ -35,6 +35,13 @@ pub fn label_response_paths( response: &Value, ctx: &PolicyContext, ) -> Option { + // Skip labeling for error responses (e.g. 404 Not Found). + // Resource-level labels from tool_rules handle these cases. + if response.get("isError").and_then(|v| v.as_bool()) == Some(true) { + crate::log_info("label_response_paths: skipping error response (isError=true)"); + return None; + } + // MCP responses are wrapped in {"content":[{"type":"text","text":"..."}]} let actual_response = extract_mcp_response(response); @@ -89,6 +96,16 @@ pub fn label_response_paths( // === Pull Requests - label by merged state === "list_pull_requests" | "search_pull_requests" | "pull_request_read" | "get_pull_request" => { + // Skip per-item labeling for pull_request_read sub-methods that return + // non-PR objects (e.g. get_check_runs, get_files, get_reviews). + // Resource-level labels from tool_rules provide correct PR integrity. + let method = tool_args + .get("method") + .and_then(|v| v.as_str()) + .unwrap_or(""); + if tool_name == "pull_request_read" && !method.is_empty() && method != "get" { + // Fall through — use resource-level labels + } else { let (items, items_path) = extract_items_array(&actual_response); if let Some(items) = items { @@ -148,7 +165,7 @@ pub fn label_response_paths( let pr_number = extract_resource_number(item, "pr", repo_for_labels); let integrity = pr_integrity(item, repo_for_labels, item_repo_private, is_forked, ctx); - let path = make_item_path(items_path, i); + let path = make_item_path(&items_path, i); labeled_paths.push(PathLabelEntry { path, @@ -182,10 +199,20 @@ pub fn label_response_paths( }, }); } + } // end else (non-sub-method) } // === Issues - label by author contributor status === "list_issues" | "search_issues" | "issue_read" | "get_issue" => { + // Skip per-item labeling for issue_read sub-methods (get_comments, + // get_sub_issues, get_labels). Resource-level labels from tool_rules apply. + let method = tool_args + .get("method") + .and_then(|v| v.as_str()) + .unwrap_or(""); + if tool_name == "issue_read" && !method.is_empty() && method != "get" { + // Fall through — use resource-level labels + } else { let (items, items_path) = extract_items_array(&actual_response); if let Some(items) = items { @@ -231,7 +258,7 @@ pub fn label_response_paths( item_repo_private, ctx, ); - let path = make_item_path(items_path, i); + let path = make_item_path(&items_path, i); labeled_paths.push(PathLabelEntry { path, @@ -265,6 +292,7 @@ pub fn label_response_paths( }, }); } + } // end else (non-sub-method) } // === Commits - label by branch === @@ -559,7 +587,7 @@ pub fn label_response_paths( }; labeled_paths.push(PathLabelEntry { - path: make_item_path(items_path, i), + path: make_item_path(&items_path, i), labels: crate::ResourceLabels { description: format!("project-item:{}", item_type.to_lowercase()), secrecy, diff --git a/guards/github-guard/rust-guard/src/labels/tool_rules.rs b/guards/github-guard/rust-guard/src/labels/tool_rules.rs index 043b8a14d..05362face 100644 --- a/guards/github-guard/rust-guard/src/labels/tool_rules.rs +++ b/guards/github-guard/rust-guard/src/labels/tool_rules.rs @@ -119,9 +119,21 @@ pub fn apply_tool_labels( } } - // Search is cross-repo / unknown scope until response labeling + // Search issues: extract repo scope from query when available "search_issues" => { - integrity = vec![]; + let query = tool_args + .get("query") + .and_then(|v| v.as_str()) + .unwrap_or(""); + let (q_owner, q_repo, q_repo_id) = extract_repo_info_from_search_query(query); + if !q_repo_id.is_empty() { + desc = format!("search_issues:{}", q_repo_id); + secrecy = + apply_repo_visibility_secrecy(&q_owner, &q_repo, &q_repo_id, secrecy, ctx); + integrity = private_writer_integrity(&q_repo_id, repo_private, ctx); + } else { + integrity = vec![]; + } } // === Pull Requests === @@ -210,9 +222,21 @@ pub fn apply_tool_labels( } } - // Search is cross-repo / unknown scope until response labeling + // Search pull requests: extract repo scope from query when available "search_pull_requests" => { - integrity = vec![]; + let query = tool_args + .get("query") + .and_then(|v| v.as_str()) + .unwrap_or(""); + let (q_owner, q_repo, q_repo_id) = extract_repo_info_from_search_query(query); + if !q_repo_id.is_empty() { + desc = format!("search_pull_requests:{}", q_repo_id); + secrecy = + apply_repo_visibility_secrecy(&q_owner, &q_repo, &q_repo_id, secrecy, ctx); + integrity = private_writer_integrity(&q_repo_id, repo_private, ctx); + } else { + integrity = vec![]; + } } // === Commits ===