fix: address valid review comments from PR #1359#1380
Conversation
- Cache discovery_schema() with OnceLock for routine tools (fixes #1361, #1371) - Early-return on empty event cache before allocating Vec (fixes #1369) - Extract batch concurrent count query helper to reduce duplication - Fix ROUTINE_OK sentinel substring matching - Migrate crate::safety import to ironclaw_safety per project convention Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
github-actions
bot
added
scope: agent
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 refines the routine engine and built-in tools by addressing several review comments from a previous PR. The changes focus on improving performance through schema caching and early-return optimizations, enhancing correctness by fixing a sentinel matching logic, and improving code maintainability by deduplicating common database query patterns and updating a dependency import. 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.
Pull request overview
This PR applies targeted performance and correctness fixes to the routine tooling and routine engine, addressing review feedback from PR #1359 (schema caching, trigger fast-paths, deduped batch queries, and stricter ROUTINE_OK sentinel handling).
Changes:
- Cache complex tool discovery schemas for
routine_createandevent_emitusingOnceLockto avoid rebuilding JSON on every call. - Add fast-path early returns for empty matcher caches and extract a shared
batch_concurrent_counts()helper to dedupe batch-query logic. - Tighten ROUTINE_OK sentinel detection to an exact match on trimmed content and update the related test; migrate
SafetyLayerimport toironclaw_safety.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
src/tools/builtin/routine.rs |
Adds OnceLock caching for discovery schemas to reduce repeated schema construction cost. |
src/agent/routine_engine.rs |
Adds matcher fast-paths, dedupes batch concurrent-count query logic, updates ROUTINE_OK detection, and adjusts SafetyLayer import. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| async fn batch_concurrent_counts(&self, routine_ids: &[Uuid]) -> Option<HashMap<Uuid, i64>> { | ||
| match self | ||
| .store | ||
| .count_running_routine_runs_batch(routine_ids) | ||
| .await | ||
| { | ||
| Ok(counts) => Some(counts), | ||
| Err(e) => { | ||
| tracing::error!("Failed to batch-load concurrent counts: {}", e); | ||
| None | ||
| } |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the routine engine by introducing early return optimizations in event trigger checks, extracting batch concurrent count logic into a dedicated helper function, and updating the SafetyLayer dependency to an external crate. It also improves the precision of the ROUTINE_OK sentinel detection to require an exact match, with corresponding test updates. Additionally, schema generation for routine_create_discovery_schema and event_emit_discovery_schema is now cached using OnceLock for performance.
Summary
Addresses valid review comments from PR #1359 with four targeted fixes:
discovery_schema()withOnceLockforRoutineCreateToolandRoutineSystemEventEmitTool— avoids rebuilding complex JSON schemas on every call (fixes [CRITICAL] **N+1 schema generation on every LLM call** —Tool::schema()in `src/tools/b #1361, [MEDIUM] **Discovery schema regeneration on every tool_info call** —tool_info.rs:134#1371)check_event_triggers()andemit_system_event()— skipsVecallocation and batch DB query when there are no relevant matchers (fixes [HIGH] **Large Vec allocation on every trigger check** — Lines 145-151 and 235-241 al #1369)batch_concurrent_counts()helper — deduplicates identical batch concurrent-count query patterns from both event-matching methodsROUTINE_OKsentinel substring matching — the previous.contains("ROUTINE_OK")check was a superset of the==check and could match substrings likeNOT_ROUTINE_OK; now uses exact match only on trimmed content, matching the prompt's instruction to reply "EXACTLY with: ROUTINE_OK"Also migrates
crate::safety::SafetyLayerimport toironclaw_safety::SafetyLayerper project convention (CLAUDE.md).Test plan
cargo fmtpassescargo clippy --all --benches --tests --examples --all-featurespasses with zero warningscargo testpasses (2 pre-existing failures intest_cancel_job_completedandtest_execute_empty_tool_name_returns_not_foundare unrelated)test_routine_sentinel_detection_exact_matchtest covers new exact-match behavior andNOT_ROUTINE_OKfalse positive caseroutine_create_discovery_schema_keeps_legacy_aliases,event_emit_discovery_schema_keeps_source_alias) validate cached schemas return correct contentCloses #1361, #1369, #1371
Related: #1359
🤖 Generated with Claude Code