Redesign routine create requests for LLMs#1147
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 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
|
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
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
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.
Summary
routine_createaround a groupedrequest.kindcanonical schema for LLMsTesting