Conversation
…sions.rs Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR cleans up the Rust GitHub guard by consolidating MCP response unwrapping into the shared labels::extract_mcp_response helper and tightening permissions.rs by removing dead-code suppression and an obsolete bot-detection shim.
Changes:
- Replaced three local MCP payload-extraction call sites with
super::extract_mcp_responseand removed the duplicate helper. - Removed
#![allow(dead_code)]frompermissions.rs, deleted the deprecatedis_bot_accountwrapper, and updated tests/imports accordingly.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| guards/github-guard/rust-guard/src/permissions.rs | Removes dead-code suppression and the deprecated is_bot_account shim; updates tests to import is_bot directly. |
| guards/github-guard/rust-guard/src/labels/backend.rs | Deduplicates MCP unwrapping by using super::extract_mcp_response and deletes the local unwrapping function. |
Comments suppressed due to low confidence (2)
guards/github-guard/rust-guard/src/labels/backend.rs:389
- super::extract_mcp_response currently emits several crate::log_info() messages on every call. If get_pull_request_facts_with_callback is called in tight loops (e.g., contributor verification), this can be noisy; consider using debug-level logging for extraction or providing a quieter unwrap helper.
let response_str = std::str::from_utf8(&result_buffer[..len]).ok()?;
let response = serde_json::from_str::<Value>(response_str).ok()?;
let pr = super::extract_mcp_response(&response);
let base_full_name = pr
guards/github-guard/rust-guard/src/labels/backend.rs:456
- extract_mcp_response logs at INFO level (top-level keys, wrapper presence, parse success/failure). Using it here may add substantial INFO log volume for issue lookups; consider gating these logs behind debug or adding a non-logging variant for backend helpers.
let response_str = std::str::from_utf8(&result_buffer[..len]).ok()?;
let response = serde_json::from_str::<Value>(response_str).ok()?;
let issue = super::extract_mcp_response(&response);
issue
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| let response_str = std::str::from_utf8(&result_buffer[..len]).ok()?; | ||
| let response = serde_json::from_str::<Value>(response_str).ok()?; | ||
| let pr = extract_mcp_payload_json(&response); | ||
| let pr = super::extract_mcp_response(&response); | ||
|
|
||
| let base_full_name = pr |
There was a problem hiding this comment.
@copilot apply changes based on this feedback downgrade the extraction logs to debug
There was a problem hiding this comment.
Done in 83876b4 — all crate::log_info calls in extract_mcp_response are now crate::log_debug.
Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com>
Closes #[issue number]
Summary
Resolves two improvements identified in the rust-guard codebase:
1. Deduplicate MCP response extraction
Removed the private
extract_mcp_payload_jsonfunction fromlabels/backend.rsand replaced its 3 call sites with the canonicalsuper::extract_mcp_responsefromlabels/mod.rs.extract_mcp_responseincludeshost_logdebug traces that the old function silently dropped2. Remove
#![allow(dead_code)]frompermissions.rs#![allow(dead_code)]suppressor so the compiler now surfaces unused items (visible as warnings)is_bot_accountshim (deprecated since0.2.0) and its associated testis_botimport at the module level; added explicit import in the test moduleThe remaining dead-code warnings in
permissions.rsare intentional scaffolding, now visible to guide future cleanup.Testing
All 57 existing tests pass. No CodeQL alerts.
Security Summary
No security vulnerabilities introduced or discovered.