Default new lightweight routines to tools-enabled#1573
Conversation
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 modifies the behavior of lightweight routines to default to tools-enabled, enhancing their capabilities by allowing them to use the owner's live autonomous tool scope. It also ensures backward compatibility for existing routines and provides clear documentation on how to opt-out of the new default behavior. Comprehensive end-to-end tests have been added to validate the changes. 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 changes the default behavior for new lightweight routines to have tools enabled. The implementation correctly introduces a default_use_tools parameter to parse_routine_execution and sets it to true for new routine creation, while maintaining backward compatibility for existing routines loaded from the database. The associated documentation, JSON schemas, and tool discovery summaries have been updated to reflect this new default and explain how to opt-out. The changes are well-tested with new unit and end-to-end tests covering both the new default and the explicit opt-out path. The code quality is high, and I have no issues to report.
There was a problem hiding this comment.
Pull request overview
Defaults newly created lightweight routines to use_tools=true so they can run with the owner’s autonomous tool scope by default, while keeping explicit opt-out and legacy behavior working.
Changes:
- Changed routine creation parsing so lightweight routines default to
use_tools=trueunless explicitly disabled. - Updated routine tool discovery/schema text to describe the new default and the
execution.use_tools=falseopt-out. - Added/updated unit + e2e tests and fixtures to cover default-on and explicit no-tools creation paths.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| tests/fixtures/llm_traces/tools/routine_manual_create_no_tools.json | Adds an e2e LLM trace fixture for creating a manual lightweight routine with tools explicitly disabled. |
| tests/e2e_builtin_tool_coverage.rs | Splits/expands the manual routine e2e test to assert the new default (use_tools=true) and the explicit opt-out path. |
| src/tools/builtin/routine.rs | Updates discovery text and changes lightweight execution parsing default to use_tools=true; adds unit coverage for default vs explicit disable. |
| src/agent/routine_engine.rs | Updates inline documentation to reflect autonomous tool-scope behavior for tool-enabled lightweight routines. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| "name": "manual-triage-no-tools", | ||
| "trigger_type": "manual", | ||
| "prompt": "Summarize the latest bug reports when this routine is fired.", | ||
| "use_tools": false |
There was a problem hiding this comment.
This trace uses the legacy top-level use_tools argument. Since the updated routine discovery text instructs callers to opt out via execution.use_tools=false, it would be better for this e2e fixture to use the canonical grouped field (e.g., execution: { use_tools: false }) so the trace matches current guidance and exercises the preferred parameter shape.
| "use_tools": false | |
| "execution": { | |
| "use_tools": false | |
| } |
There was a problem hiding this comment.
Fixed in f5062b6. I switched the fixture to the canonical grouped shape with execution.use_tools=false, and reran CARGO_TARGET_DIR=/tmp/ironclaw-lightweight-routines-default-tools-target cargo test routine_manual_create to confirm the opt-out e2e still passes.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -300,7 +300,7 @@ fn lightweight_execution_variant() -> Value { | |||
| }, | |||
| "use_tools": { | |||
| "type": "boolean", | |||
There was a problem hiding this comment.
Same as above: lightweight_execution_variant() documents use_tools defaulting to true, but the schema omits an explicit default: true. Setting the default in the schema would better match runtime parsing and improve discoverability.
| "type": "boolean", | |
| "type": "boolean", | |
| "default": true, |
There was a problem hiding this comment.
Fixed in 161f3e9. I added explicit "default": true metadata for the lightweight use_tools schema so discovery matches the runtime default.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
zmanian
left a comment
There was a problem hiding this comment.
Code Review: Default lightweight routines to tools-enabled
Verdict: APPROVE
Changes default for newly created lightweight routines from use_tools=false to use_tools=true (with max_tool_rounds=3).
Backward Compatibility: SAFE
Existing routines stored in the database are unaffected -- RoutineAction::from_db() defaults use_tools to false when the field is absent. Only new routine creation is affected.
Security
When use_tools=true, tools are filtered through autonomous_allowed_tool_names() respecting the owner's autonomous scope. Loop capped at min(max_tool_rounds, config.lightweight_max_iterations, 5). Reasonable guardrails.
Suggestions (non-blocking)
-
Unrelated cleanup: The
codex_test_helpers.rschange (removing redundant#![cfg(test)]) doesn't belong in this PR. Harmless but worth noting. -
max_tool_rounds=3whenuse_tools=false: Storingmax_tool_rounds=3on a tools-disabled routine is slightly misleading. Consider defaulting to0when tools are disabled, or documenting that the value is ignored. -
CLI test gating:
cli_create_defaults_lightweight_routines_to_tools_enabledonly runs under#[cfg(feature = "libsql")]. Ensure CI covers this.
Good test coverage at unit and e2e levels. Clean implementation.
|
Thanks for the review. On the non-blocking notes:
|
* Default new lightweight routines to tools-enabled * Fix fmt and clippy on lightweight routine PR * Use grouped execution field in routine no-tools fixture * Align CLI routine defaults with tools-enabled lightweight mode
* Default new lightweight routines to tools-enabled * Fix fmt and clippy on lightweight routine PR * Use grouped execution field in routine no-tools fixture * Align CLI routine defaults with tools-enabled lightweight mode
Summary
use_tools=truewhile keeping legacy stored routines backward compatibleuse_tools=falseopt-outTesting