fix: post-merge review sweep — 8 fixes across security, perf, and correctness - #1550
Conversation
…rectness 1. Fix code fence detection in extract_suggestions() (issue #1180) - rfind("```") couldn't handle odd fence counts (unclosed blocks) - Now counts all fence positions and checks parity 2. Cache routine parameters_schema() with OnceLock (issue #1361) - routine_create_parameters_schema() and event_emit_parameters_schema() were regenerating JSON on every LLM call 3. Replace O(n) LRU eviction with lru crate (issue #1430) - Embedding cache now uses lru::LruCache for O(1) eviction - Removes manual HashMap + last_accessed tracking 4. Fix WASM router secret_validated semantics (issue #1281) - Now reflects whether any auth (secret/Ed25519/HMAC) was performed - Previously only checked if a secret was configured 5. Sanitize channel/user in routine prompt interpolation (issue #1364) - Defense-in-depth: strip newlines, replace backticks, truncate to 128 chars before injecting into LLM prompt 6. Remove duplicate 401 retry in github_copilot.rs (PR #1512 review) - Internal retry conflicted with outer RetryProvider causing nested retries; now invalidates token and lets RetryProvider handle retry 7. Fix token error classification in github_copilot.rs (PR #1512 review) - AccessDenied/Expired errors now map to AuthFailed (non-retryable) - Transient errors remain RequestFailed (retryable) 8. Fix parse_extra_headers() hardcoded env var name (PR #1512 review) - Error messages now report the actual env var being parsed instead of always saying LLM_EXTRA_HEADERS Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request performs a comprehensive post-merge review sweep, integrating several critical fixes and improvements across security, performance, and correctness. It addresses 8 residual issues identified from past PRs and automated reviews, ranging from refining code fence detection and optimizing caching mechanisms to enhancing authentication logic and sanitizing user inputs for LLM prompts. The changes aim to bolster the system's robustness and efficiency by resolving known vulnerabilities and performance bottlenecks. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request delivers a valuable collection of fixes across security, performance, and correctness. The changes are well-implemented and address several residual issues effectively. Key improvements include a more robust suggestion extraction logic, caching for schema generation, and a significant performance enhancement to the embedding cache by switching to lru::LruCache. The refactoring of the GitHub Copilot provider to better handle auth errors and retries is also a notable improvement. I have one minor suggestion to improve the performance of the new sanitization function.
| fn sanitize_prompt_field(value: &str) -> String { | ||
| const MAX_LEN: usize = 128; | ||
| let cleaned: String = value | ||
| .chars() | ||
| .filter(|c| *c != '\n' && *c != '\r') | ||
| .take(MAX_LEN) | ||
| .collect(); | ||
| cleaned.replace('`', "'") |
There was a problem hiding this comment.
This implementation is correct, but it can be made slightly more efficient. It currently creates an intermediate String via collect(), and then performs another pass over that string with replace(). You can combine these operations into a single pass using map() to handle the backtick replacement during the initial character iteration. This avoids the extra allocation and iteration.
| fn sanitize_prompt_field(value: &str) -> String { | |
| const MAX_LEN: usize = 128; | |
| let cleaned: String = value | |
| .chars() | |
| .filter(|c| *c != '\n' && *c != '\r') | |
| .take(MAX_LEN) | |
| .collect(); | |
| cleaned.replace('`', "'") | |
| fn sanitize_prompt_field(value: &str) -> String { | |
| const MAX_LEN: usize = 128; | |
| value | |
| .chars() | |
| .filter(|&c| c != '\n' && c != '\r') | |
| .take(MAX_LEN) | |
| .map(|c| if c == '`' { '\'' } else { c }) | |
| .collect() | |
| } |
References
- To improve performance, avoid unnecessary heap allocations. When processing string parts, use iterators directly instead of collecting them into an intermediate data structure like a
VecorString.
There was a problem hiding this comment.
Fixed in 8e40f7b — switched to single-pass with .map() as suggested. No intermediate allocation.
There was a problem hiding this comment.
Pull request overview
Post-merge cleanup PR that finishes/strengthens several previously discussed fixes across correctness (suggestion extraction), performance (schema + embedding caches), and security/defense-in-depth (prompt interpolation + auth handling).
Changes:
- Fix
<suggestions>extraction to ignore fenced code blocks even with unclosed/odd fence counts; add regression test. - Cache routine/event parameter schemas via
OnceLockto avoid regenerating JSON schema per call. - Replace manual embedding-cache LRU eviction with
lru::LruCache; adjust GitHub Copilot provider auth error classification and remove nested 401 retry logic; improve extra-header parsing error reporting.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/workspace/embedding_cache.rs | Swap manual HashMap+timestamps LRU to lru::LruCache implementation. |
| src/tools/builtin/routine.rs | Cache expensive parameter schema generation with OnceLock. |
| src/llm/github_copilot.rs | Refine token-exchange error classification; remove inline 401 retry and rely on outer RetryProvider. |
| src/config/llm.rs | Improve extra-header parsing errors by reporting the correct env var name; adjust tests. |
| src/channels/wasm/router.rs | Adjust secret_validated semantics to reflect multiple auth mechanisms. |
| src/agent/routine_engine.rs | Sanitize notify channel/user fields before interpolating into lightweight routine prompts. |
| src/agent/dispatcher.rs | Improve code-fence detection for suggestions extraction; add regression test. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // at capacity (O(1)). If another caller already cached this key, | ||
| // push overwrites it — correct and idempotent. | ||
| { | ||
| let mut guard = self.cache.lock().unwrap_or_else(|e| e.into_inner()); |
There was a problem hiding this comment.
embed() currently always clones and inserts the fetched embedding into the LRU, even if another concurrent caller already populated the same key while the lock was released. That reintroduces extra clone+write work in the thundering-herd case. Re-check the cache under the lock (and just touch/promote the existing entry) before cloning/inserting.
| // at capacity (O(1)). If another caller already cached this key, | |
| // push overwrites it — correct and idempotent. | |
| { | |
| let mut guard = self.cache.lock().unwrap_or_else(|e| e.into_inner()); | |
| // at capacity (O(1)). | |
| { | |
| let mut guard = self.cache.lock().unwrap_or_else(|e| e.into_inner()); | |
| // Re-check cache under the lock in case another concurrent caller | |
| // has already populated this key while the lock was released. | |
| if let Some(existing) = guard.get(&key) { | |
| tracing::trace!("embedding cache filled by concurrent caller"); | |
| return Ok(existing.clone()); | |
| } | |
| // Still not present; insert our freshly fetched embedding. |
There was a problem hiding this comment.
Fixed in 8e40f7b — embed() now re-checks cache under lock before cloning. If another concurrent caller already populated the key, we skip the clone and just log a trace.
| // Cache new embeddings (clone only the cacheable subset), then move | ||
| // originals into results. LruCache::push handles eviction at capacity. | ||
| { | ||
| let mut guard = self.cache.lock().unwrap_or_else(|e| e.into_inner()); | ||
| let cacheable = miss_indices.len().min(self.config.max_entries); | ||
| let skip = miss_indices.len() - cacheable; | ||
| let need_to_evict = (guard.len() + cacheable).saturating_sub(self.config.max_entries); | ||
| if need_to_evict > 0 { | ||
| Self::evict_k_oldest(&mut guard, need_to_evict); | ||
| } | ||
| let now = Instant::now(); | ||
| for (&orig_idx, emb) in miss_indices[skip..].iter().zip(&new_embeddings[skip..]) { | ||
| guard.insert( | ||
| keys[orig_idx], | ||
| CacheEntry { | ||
| embedding: emb.clone(), | ||
| last_accessed: now, | ||
| }, | ||
| ); | ||
| for (&orig_idx, emb) in miss_indices.iter().zip(&new_embeddings) { | ||
| guard.push(keys[orig_idx], emb.clone()); | ||
| } |
There was a problem hiding this comment.
In embed_batch, the code now clones every newly fetched embedding into the cache via push, even when miss_indices.len() greatly exceeds the cache capacity (those entries will be immediately evicted). This can create large, avoidable clone/CPU overhead for big batches. Consider limiting caching to at most the cache capacity (or a bounded subset) and update the comment that says “clone only the cacheable subset”, which is no longer true.
There was a problem hiding this comment.
Fixed in 8e40f7b — embed_batch() now limits caching to the last cap entries via guard.cap().get(), skipping overflow entries that would be immediately evicted.
| // Call the WASM channel. | ||
| // If we reach this point, any required secret/signature validation has | ||
| // already passed (the guards above return 401 on failure). So | ||
| // `secret_validated` is true whenever a secret was configured and checked. | ||
| let secret_validated = state.router.requires_secret(channel_name).await | ||
| || state.router.get_signature_key(channel_name).await.is_some() | ||
| || state.router.get_hmac_secret(channel_name).await.is_some(); |
There was a problem hiding this comment.
secret_validated is now computed by re-calling requires_secret() / get_signature_key() / get_hmac_secret() even though those values were already fetched earlier for the auth guards. This adds extra awaits and makes the comment misleading (“secret was configured and checked” now includes signature/HMAC too). Prefer threading booleans from the earlier checks (e.g., did_authenticate) and adjust the comment/variable name to reflect “any auth validated”.
There was a problem hiding this comment.
Fixed in 8e40f7b — now threads a did_authenticate bool through the auth guards (secret/Ed25519/HMAC) instead of re-calling async methods. Each guard sets it to true on success.
| return Err(LlmError::AuthFailed { | ||
| return Err(LlmError::RequestFailed { | ||
| provider: "github_copilot".to_string(), | ||
| reason: format!("HTTP 401: {}", crate::agent::truncate_for_preview(&response_text, 256)), |
There was a problem hiding this comment.
On HTTP 401, this returns LlmError::RequestFailed with a reason that includes a truncated copy of the response body. RequestFailed’s {reason} is part of the user-facing error string, so this may leak arbitrary backend text. Consider keeping the user-facing reason generic for 401 (status + short message) and only logging the body at TRACE/DEBUG if needed.
| reason: format!("HTTP 401: {}", crate::agent::truncate_for_preview(&response_text, 256)), | |
| // Avoid including backend-provided response body in user-facing error. | |
| reason: "HTTP 401 Unauthorized".to_string(), |
There was a problem hiding this comment.
Fixed in 8e40f7b — 401 error now uses generic "HTTP 401 Unauthorized" message. The response body is already logged at WARN level above (truncated to 256 chars), so it's still available for debugging.
- sanitize_prompt_field: single-pass with map() instead of collect+replace - embed(): re-check cache under lock before cloning (thundering herd) - embed_batch(): limit caching to cache capacity, skip overflow entries - router: thread did_authenticate bool instead of re-calling async methods - github_copilot 401: use generic error message, avoid leaking response body - cargo fmt: fix two formatting violations caught by CI Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
[skip-regression-check] Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
zmanian
left a comment
There was a problem hiding this comment.
Code Review: Post-merge review sweep (8 fixes)
Verdict: APPROVE
All 8 fixes are correct and well-scoped (+114/-170, net reduction).
Per-fix assessment
| # | Fix | Verdict |
|---|---|---|
| 1 | Code fence parity detection in extract_suggestions() |
Sound. Regression test added. |
| 2 | OnceLock schema caching for routine/event params |
Correct, idiomatic. |
| 3 | lru::LruCache replacing manual O(n) eviction |
Sound. Net -73 lines. |
| 4 | WASM router did_authenticate reflects actual auth |
Correct, important for non-secret auth channels. |
| 5 | Prompt field sanitization (defense-in-depth) | Good. Single-pass implementation is efficient. |
| 6 | Remove duplicate inline 401 retry conflicting with RetryProvider |
Highest-risk fix, done correctly. Clean separation of concerns. |
| 7 | Token error classification (auth non-retryable, transient retryable) | Correct. Aligns with retry/circuit-breaker design. |
| 8 | parse_extra_headers reports correct env var name |
Clean. |
Suggestions (non-blocking)
-
Missing regression tests for fixes 4-7. Per
review-discipline.md: "Every bug fix must include a test that would have caught the bug." Consider adding:- Fix 4: Test channel using Ed25519/HMAC auth verifying
secret_validated=true - Fix 5: Unit test for
sanitize_prompt_field(newlines, backticks, truncation) - Fix 6/7: Test that
AccessDeniedmaps toAuthFailed(non-retryable)
- Fix 4: Test channel using Ed25519/HMAC auth verifying
-
sanitize_prompt_fieldcould strip all control characters (c.is_control()) instead of just\n/\r. -
.expect()onNonZeroUsize::new(max_entries)inembedding_cache.rs-- safe due to.max(1)above, but strict reading of project rules says no.expect()in production.
Code reviewFound 6 issues:
|
…rectness (nearai#1550) * fix: post-merge review sweep — 8 fixes across security, perf, and correctness 1. Fix code fence detection in extract_suggestions() (issue nearai#1180) - rfind("```") couldn't handle odd fence counts (unclosed blocks) - Now counts all fence positions and checks parity 2. Cache routine parameters_schema() with OnceLock (issue nearai#1361) - routine_create_parameters_schema() and event_emit_parameters_schema() were regenerating JSON on every LLM call 3. Replace O(n) LRU eviction with lru crate (issue nearai#1430) - Embedding cache now uses lru::LruCache for O(1) eviction - Removes manual HashMap + last_accessed tracking 4. Fix WASM router secret_validated semantics (issue nearai#1281) - Now reflects whether any auth (secret/Ed25519/HMAC) was performed - Previously only checked if a secret was configured 5. Sanitize channel/user in routine prompt interpolation (issue nearai#1364) - Defense-in-depth: strip newlines, replace backticks, truncate to 128 chars before injecting into LLM prompt 6. Remove duplicate 401 retry in github_copilot.rs (PR nearai#1512 review) - Internal retry conflicted with outer RetryProvider causing nested retries; now invalidates token and lets RetryProvider handle retry 7. Fix token error classification in github_copilot.rs (PR nearai#1512 review) - AccessDenied/Expired errors now map to AuthFailed (non-retryable) - Transient errors remain RequestFailed (retryable) 8. Fix parse_extra_headers() hardcoded env var name (PR nearai#1512 review) - Error messages now report the actual env var being parsed instead of always saying LLM_EXTRA_HEADERS Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: address PR review comments and fix formatting - sanitize_prompt_field: single-pass with map() instead of collect+replace - embed(): re-check cache under lock before cloning (thundering herd) - embed_batch(): limit caching to cache capacity, skip overflow entries - router: thread did_authenticate bool instead of re-calling async methods - github_copilot 401: use generic error message, avoid leaking response body - cargo fmt: fix two formatting violations caught by CI Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * chore: trigger CI re-run with updated refs [skip-regression-check] Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…rectness (nearai#1550) * fix: post-merge review sweep — 8 fixes across security, perf, and correctness 1. Fix code fence detection in extract_suggestions() (issue nearai#1180) - rfind("```") couldn't handle odd fence counts (unclosed blocks) - Now counts all fence positions and checks parity 2. Cache routine parameters_schema() with OnceLock (issue nearai#1361) - routine_create_parameters_schema() and event_emit_parameters_schema() were regenerating JSON on every LLM call 3. Replace O(n) LRU eviction with lru crate (issue nearai#1430) - Embedding cache now uses lru::LruCache for O(1) eviction - Removes manual HashMap + last_accessed tracking 4. Fix WASM router secret_validated semantics (issue nearai#1281) - Now reflects whether any auth (secret/Ed25519/HMAC) was performed - Previously only checked if a secret was configured 5. Sanitize channel/user in routine prompt interpolation (issue nearai#1364) - Defense-in-depth: strip newlines, replace backticks, truncate to 128 chars before injecting into LLM prompt 6. Remove duplicate 401 retry in github_copilot.rs (PR nearai#1512 review) - Internal retry conflicted with outer RetryProvider causing nested retries; now invalidates token and lets RetryProvider handle retry 7. Fix token error classification in github_copilot.rs (PR nearai#1512 review) - AccessDenied/Expired errors now map to AuthFailed (non-retryable) - Transient errors remain RequestFailed (retryable) 8. Fix parse_extra_headers() hardcoded env var name (PR nearai#1512 review) - Error messages now report the actual env var being parsed instead of always saying LLM_EXTRA_HEADERS Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: address PR review comments and fix formatting - sanitize_prompt_field: single-pass with map() instead of collect+replace - embed(): re-check cache under lock before cloning (thundering herd) - embed_batch(): limit caching to cache capacity, skip overflow entries - router: thread did_authenticate bool instead of re-calling async methods - github_copilot 401: use generic error message, avoid leaking response body - cargo fmt: fix two formatting violations caught by CI Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * chore: trigger CI re-run with updated refs [skip-regression-check] Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Summary
Reviewed the past week's merged PRs and closed issues, verified which fixes landed correctly, and addresses 8 residual issues that were either incompletely fixed, closed as false positives but still had code quality gaps, or flagged by automated reviewers on PR #1512.
Issues addressed
extract_suggestions():rfind("```")couldn't handle odd fence counts (unclosed code blocks). Now counts all fence positions and uses parity to determine inside/outside.Tool::schema()in `src/tools/b #1361 (partial) —routine_create_parameters_schema()andevent_emit_parameters_schema()were regenerating the full JSON schema on every LLM call. Now cached withOnceLocklike their*_discovery_schema()counterparts.lru::LruCachefor O(1) insertion, lookup, and eviction.secret_validatedin WASM router now reflects whether any authentication was performed (secret, Ed25519, HMAC), not just whether a secret is configured.PR #1512 review comments addressed
send_request()had a 50-line inline 401 retry that conflicted with the outerRetryProvider, causing nested retries with exponential backoff. Removed; now invalidates token and returnsRequestFailed(retryable) soRetryProviderhandles it.RequestFailed. NowAccessDenied/Expiredmap toAuthFailed(non-retryable), transient errors remain retryable.parse_extra_headers()hardcoded key (LOW): Error messages always saidLLM_EXTRA_HEADERSregardless of which env var was being parsed. Now accepts the env var name for accurate reporting.Verified as properly fixed (no action needed)
llm/retry.rsvalidate_tool_schema()#975 — Unbounded recursion: bounded to depth 16FullJobWatcherTest plan
cargo clippy --all --benches --tests --examples --all-features— zero warningstest_extract_suggestions_inside_unclosed_code_fencelru::LruCacheswap🤖 Generated with Claude Code