Redesign routine create requests for LLMs#1147
Conversation
github-actions
bot
added
scope: tool/builtin
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 significantly refactors the internal representation and external API for creating routines, particularly for LLMs. The primary goal was to introduce a more structured and maintainable schema for routine requests, moving from a flat parameter list to a logically grouped one. This change improves clarity and extensibility for future routine enhancements, while carefully preserving compatibility with existing routine definitions through intelligent parsing and aliasing. Highlights
Changelog
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. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![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 introduces a significant and well-executed redesign of the routine_create tool, moving from a flat parameter structure to a more organized, grouped schema under request, execution, delivery, and advanced keys. The implementation thoughtfully maintains backward compatibility by supporting legacy flat fields as aliases, which is crucial for a smooth transition. The code is cleaner, more maintainable, and the addition of comprehensive tests for both the new schema and legacy compatibility is excellent. I have one minor suggestion to improve the clarity of a tool description.
There was a problem hiding this comment.
Pull request overview
This PR refactors the routine_create/event_emit built-in tool parameter schemas to support a grouped/nested request shape (while keeping legacy aliases for discovery), and adds new E2E coverage + fixtures demonstrating grouped routine creation and grouped system-event emission.
Changes:
- Refactor
routine_createschema + parsing to prefer a grouped{ request, execution, delivery, advanced }structure and add a separate discovery schema that retains legacy aliases. - Refactor
event_emitschema and parsing to preferevent_source, with a discovery-onlysourcealias. - Add new LLM trace fixtures and E2E tests covering grouped routine creation and firing grouped system-event routines.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
src/tools/builtin/routine.rs |
Major refactor: new grouped schemas, normalization/parsing helpers, and discovery schemas for legacy alias compatibility. |
src/tools/schema_validator.rs |
Reuses the centralized event_emit parameter schema function in schema validator tests. |
tests/e2e_builtin_tool_coverage.rs |
Adds E2E tests for grouped routine_create and grouped system-event + event_emit behavior. |
tests/fixtures/llm_traces/tools/routine_create_grouped.json |
New trace fixture for grouped cron routine creation with delivery/advanced settings. |
tests/fixtures/llm_traces/tools/routine_system_event_emit_grouped.json |
New trace fixture for grouped system-event routine creation plus emitting a matching event. |
skills/ironclaw-workflow-orchestrator/references/workflow-routines.md |
Updates reference routine JSON examples to the new grouped shape and canonical event_source. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Addressed the valid review points in
Local verification passed:
|
There was a problem hiding this comment.
Pull request overview
This PR updates the routine_create and event_emit built-in tools to support a new “grouped” request shape (nested request / execution / delivery / advanced objects) while keeping backward compatibility via parsing aliases and richer discovery schemas. It also adds E2E trace fixtures/tests and updates workflow documentation to the new format.
Changes:
- Refactor
routine_createto accept grouped trigger/execution/delivery/advanced blocks, with legacy flat-field aliases supported in parsing and exposed viadiscovery_schema(). - Add
event_emitschema + parsing support for asourcealias in discovery while keepingevent_sourcecanonical in the compact parameters schema. - Add new E2E coverage and fixtures for grouped routine creation and grouped system-event emission; update workflow routine docs accordingly.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
src/tools/builtin/routine.rs |
Implements grouped schemas, parsing/normalization, and discovery schemas for routine_create and event_emit, plus expanded unit tests. |
src/tools/schema_validator.rs |
Reuses the centralized event_emit_parameters_schema() instead of inlining the schema. |
tests/e2e_builtin_tool_coverage.rs |
Adds E2E tests validating grouped routine_create flows and grouped system_event + event_emit behavior. |
tests/fixtures/llm_traces/tools/routine_create_grouped.json |
New trace fixture for grouped cron routine creation + routine listing. |
tests/fixtures/llm_traces/tools/routine_system_event_emit_grouped.json |
New trace fixture for grouped system-event routine creation + emitting a matching event. |
skills/ironclaw-workflow-orchestrator/references/workflow-routines.md |
Updates reference JSON examples to the grouped routine shape and canonical event_source for event_emit. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
zmanian
left a comment
zmanian
left a comment
There was a problem hiding this comment.
LGTM. Clean redesign that genuinely improves LLM usability.
The grouped { request, execution, delivery, advanced } structure makes the discriminated-union nature of trigger types explicit. Backward compatibility via dual-schema approach (compact for validation, discovery with legacy aliases) is well-designed. CI is green, test coverage is solid (10 unit + 2 E2E).
Minor notes (non-blocking):
- Test helper wrappers (
expect_ok,expect_some, etc.) add unnecessary indirection --#[cfg(test)]code is already exempt from the no-unwrap rule - Compact schema test only checks a subset of legacy aliases -- worth expanding for completeness
|
Addressed the two non-blocking review notes in 3a1f47b: removed the test-only helper wrappers and expanded the compact/discovery schema coverage to the full legacy alias set. |
zmanian
left a comment
zmanian
left a comment
There was a problem hiding this comment.
Review: Redesign routine create requests for LLMs
The redesign concept (grouping flat parameters into request, execution, advanced objects) is reasonable for LLM ergonomics, but CI is currently failing.
Blocking
1. CI failures
- "Code Style (fmt + clippy)" -- failing
- "No panics in production code" -- failing
These must be resolved before review of the code changes is meaningful. Please fix and push.
Preliminary observations (will review in depth after CI passes)
- The grouped schema (
kind,source,event_typeunderrequest;modeunderexecution;cooldown_secsunderadvanced) is a cleaner LLM interface than flat parameters - 1522 additions is large -- consider whether the skill reference doc changes can be a separate commit
- The
zmanianreview was dismissed, suggesting previous feedback exists -- please confirm prior items were addressed
Title check: accurate -- this is a schema redesign for LLM consumption.
There was a problem hiding this comment.
Pull request overview
Updates the routines tool API to support a new “grouped” request shape (nested request/execution/delivery/advanced objects) while keeping backward compatibility via discovery-schema aliases, and adds E2E fixtures/tests and documentation to cover the new shape.
Changes:
- Refactors
routine_createandevent_emitschemas to a grouped shape, adds parsing/normalization helpers, and exposes legacy fields as discovery-schema aliases. - Adds new LLM trace fixtures and E2E coverage for grouped
routine_createand groupedsystem_event+event_emitflows. - Updates schema validation harness and workflow orchestrator reference docs to the new field names/shape.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/fixtures/llm_traces/tools/routine_system_event_emit_grouped.json | New fixture covering grouped system-event routine creation + emitting a matching event. |
| tests/fixtures/llm_traces/tools/routine_create_grouped.json | New fixture covering grouped cron routine creation with execution/delivery/advanced blocks. |
| tests/e2e_builtin_tool_coverage.rs | Adds new E2E tests that exercise grouped routine creation and grouped system-event emission. |
| src/tools/schema_validator.rs | Uses the centralized event_emit schema function in the schema sample list to avoid drift. |
| src/tools/builtin/routine.rs | Main refactor: grouped schemas + normalization/parsing, plus discovery schemas that retain legacy aliases. |
| skills/ironclaw-workflow-orchestrator/references/workflow-routines.md | Updates example routine configs and event_emit example to the new grouped/canonical fields. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
The PR were already approved. The additional changes only entail some minor changes which were addressed. |
zmanian
left a comment
zmanian
left a comment
There was a problem hiding this comment.
Review: APPROVE
Well-designed redesign. Previous CI failures resolved, all prior review feedback addressed.
What's good
- Strong type modeling via
NormalizedTriggerRequest,NormalizedExecutionMode,NormalizedExecutionRequest, etc. -- follows project preference for strong types over strings - Dual-schema approach: compact
parameters_schema()for validation,discovery_schema()with legacy aliases for backward compat -- clean and avoids breaking existing callers - Helper functions (
string_field,bool_field, etc.) abstract the grouped-then-alias lookup, reducing duplication - 10 unit tests covering all trigger types, legacy flat shape, mixed grouped+legacy, schema assertions, negative cases + 2 E2E trace fixtures
event_emitalso updated to dual-schema pattern withsourcealias- Schema validator now uses centralized
event_emit_parameters_schema()instead of inline duplicate - No DB changes, no
.unwrap()in production code
Suggestion (non-blocking)
~70+ // safety: test-only assertion in #[cfg(test)] module comments in the test module are excessive. The #[cfg(test)] context already communicates this. A single block comment at the top of mod tests would be cleaner.
CI all green.
There was a problem hiding this comment.
Pull request overview
This PR updates the built-in routine tools to support a new “grouped” request shape (e.g., request, execution, delivery, advanced) while adding fixtures, E2E coverage, schema validation updates, and documentation examples aligned to the new shape.
Changes:
- Refactors
routine_createandevent_emittool schemas/parsing to support grouped parameters (with compatibility aliases via discovery schema). - Adds new LLM trace fixtures and E2E tests covering grouped routine creation and grouped system-event emit + routine firing.
- Updates workflow orchestrator reference docs to use the grouped routine JSON format.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
src/tools/builtin/routine.rs |
Implements grouped schemas/parsers, adds discovery schemas + extensive tests, refactors tool execution to use normalized requests. |
src/tools/schema_validator.rs |
Uses the shared event_emit_parameters_schema() to keep schema validation consistent with the tool implementation. |
tests/e2e_builtin_tool_coverage.rs |
Adds E2E tests validating grouped routine creation and system-event emission behavior. |
tests/fixtures/llm_traces/tools/routine_create_grouped.json |
Adds replay fixture for grouped routine_create + routine_list. |
tests/fixtures/llm_traces/tools/routine_system_event_emit_grouped.json |
Adds replay fixture for grouped system-event routine creation + event_emit. |
skills/ironclaw-workflow-orchestrator/references/workflow-routines.md |
Updates routine JSON examples to the grouped schema; adjusts event_emit example field naming. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/tools/builtin/routine.rs
Outdated
| }, | ||
| "user": { | ||
| "type": "string", | ||
| "description": "Default user or target for notifications and routine job message calls. Defaults to 'default'." |
There was a problem hiding this comment.
Fixed in 71a490d. delivery.user no longer defaults to the legacy default sentinel. The schema and summary now describe omission as falling back to the owner last-seen notification target.
src/tools/builtin/routine.rs
Outdated
| user: string_field(params, "delivery", "user", &["notify_user"]) | ||
| .unwrap_or_else(|| "default".to_string()), |
There was a problem hiding this comment.
Fixed in 71a490d. NormalizedDeliveryRequest.user is now Option, parse_routine_delivery leaves omission as None, and the deprecated default sentinel is no longer reintroduced.
src/tools/builtin/routine.rs
Outdated
| .and_then(|v| v.as_str()) | ||
| .map(String::from), | ||
| channel: normalized.delivery.channel.clone(), | ||
| user: Some(normalized.delivery.user.clone()), |
There was a problem hiding this comment.
Fixed in 71a490d. NotifyConfig.user now passes through the optional parsed delivery target directly, so omitted delivery.user stays None and resolves via owner-scope fallback instead of persisting a placeholder value.
| "expected lightweight execution mode", | ||
| ); | ||
| assert_eq!(parsed.cooldown_secs, 300); | ||
| assert_eq!(parsed.delivery.user.as_str(), "default"); | ||
| } |
There was a problem hiding this comment.
Fixed in 71a490d. The parser now represents omitted delivery.user as None, and the regression test was updated to assert the owner-target fallback behavior instead of the legacy default sentinel.
github-actions
bot
added
scope: agent
There was a problem hiding this comment.
Pull request overview
This PR enhances tool discovery and routine tooling by introducing curated tool_info summaries, adding grouped request shapes for routine_create / event_emit, and updating tests/fixtures/docs to validate and demonstrate the new behavior.
Changes:
- Add
tool_info(detail: "summary")support with a structuredToolDiscoverySummaryand update traces/tests accordingly. - Extend
routine_createandevent_emitwith grouped request schemas plus richer discovery schemas (including legacy alias compatibility) and new coverage fixtures/tests. - Refactor lightweight routine prompt construction to include delivery/tooling guidance and add unit tests for the prompt builder.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/fixtures/llm_traces/tools/tool_info_discovery.json | Updates tool_info trace to cover the new summary detail level (3 calls instead of 2). |
| tests/fixtures/llm_traces/tools/routine_system_event_emit_grouped.json | Adds a new grouped routine_create + event_emit trace fixture. |
| tests/fixtures/llm_traces/tools/routine_create_grouped.json | Adds a new grouped routine_create trace fixture. |
| tests/e2e_builtin_tool_coverage.rs | Adds E2E coverage for grouped routine_create + grouped system_event emit and extends tool_info coverage to summary detail. |
| src/tools/tool.rs | Introduces ToolDiscoverySummary and augments tool schema descriptions with tool_info discovery hints. |
| src/tools/schema_validator.rs | Reuses event_emit_parameters_schema() instead of duplicating the schema inline in tests. |
| src/tools/registry.rs | Centralizes tool definition construction via tool.schema() and adds a regression test covering schema-based definitions. |
| src/tools/builtin/tool_info.rs | Implements the new detail parameter (names/summary/schema) and fallback summary behavior with tests. |
| src/tools/builtin/routine.rs | Implements grouped routine_create/event_emit schemas, parsing/normalization, discovery summaries, and adds extensive unit tests. |
| src/agent/routine_engine.rs | Extracts lightweight prompt building into a helper and adds tests for delivery/tools guidance behavior. |
| skills/ironclaw-workflow-orchestrator/references/workflow-routines.md | Updates documentation examples to the new grouped request/execution/advanced blocks and canonical event_source. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let has_discovery_hint = | ||
| self.discovery_summary().is_some() || self.discovery_schema() != parameters; |
There was a problem hiding this comment.
Good call on the cost here. I kept the broader discovery_schema() != parameters_schema() check in this PR because narrowing it to discovery_summary().is_some() would silently drop the tool_info hint for tools that expose a richer discovery schema without a custom summary. That said, the performance concern is valid. The better follow-up is a cheap has_discovery_hints() / has_richer_discovery_schema() hook (or cached registration-time flag) so we can preserve the current behavior without materializing and deep-comparing large JSON values on the hot path. I am treating that as follow-up work rather than changing semantics in this PR.
henrypark133
left a comment
henrypark133
left a comment
There was a problem hiding this comment.
Clearing a stale pending review so the resolved thread replies can be posted cleanly.
Summary
routine_create/event_emitaround grouped canonical request shapes while keeping legacy flat aliases available in parsing and discoverytool_info(detail: "summary" | "schema"), richerroutine_creatediscovery output, and routing live tool definitions throughtool.schema()"default"sentinel fordelivery.user; omission now stays unset and falls back to the owner’s last-seen notification targetTesting
cargo fmt --allpython3 scripts/check_no_panics.py --base "$(git merge-base origin/staging HEAD)" --head HEADcargo test routine_create_ --libcargo test tool_info_ --libcargo test routine_ --libcargo test test_tool_definitions_use_tool_schema --libcargo test --test e2e_builtin_tool_coverage tool_info_discovery -- --nocapturecargo clippy --all-targets --all-features -- -D warnings