Conversation
Adds a proposal for "Task Loop" (Polling) support to the `openspec/changes` directory. - `proposal.md`: Defines the need for polling vs retrying. - `tasks.md`: Outlines engineering tasks. - `spec.md`: Defines acceptance criteria for looping behavior. - `.jules/nexus.md`: Adds strategic insight on Retry vs Loop. Co-authored-by: thalesraymond <32554150+thalesraymond@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
Summary of ChangesHello @thalesraymond, 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 proposes and outlines the implementation of a new 'Task Loop' feature within the Nexus system. The primary goal is to provide a dedicated and robust mechanism for tasks to poll for asynchronous external conditions, such as waiting for a server to be ready or a report to be generated. This change addresses the current practice of misusing retry logic or implementing manual, less visible loops, thereby improving the clarity, reliability, and maintainability of workflows that depend on external state 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. 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 proposes a new task looping/polling feature, which is a great addition to address a common use case where users need to wait for asynchronous states. The proposal is well-structured, separating the concept of looping from retrying. My review includes a few suggestions to enhance the design for more robustness and clarity. Specifically, I've recommended adding a backoff strategy to the loop configuration, clarifying the behavior on loop timeout, and defining the interaction between the new looping strategy and the existing retry strategy. These changes should help ensure the feature is both powerful and predictable for users.
| - [ ] 1.1 Define `TaskLoopConfig` interface in `src/contracts/TaskLoopConfig.ts` | ||
| - [ ] 1.2 Update `TaskStep` interface in `src/TaskStep.ts` to include `loop?: TaskLoopConfig` | ||
| - [ ] 1.3 Create `LoopingExecutionStrategy` in `src/strategies/LoopingExecutionStrategy.ts` implementing `IExecutionStrategy` | ||
| - [ ] 1.4 Update `TaskRunnerBuilder` (if necessary) or ensure strategy composition works |
There was a problem hiding this comment.
This is a good implementation plan. However, it's missing a crucial point: defining the interaction between the new LoopingExecutionStrategy and the existing RetryingExecutionStrategy. The order in which these strategies are composed will significantly affect behavior when a task has both loop and retry configurations.
For example:
Retry(Loop(Task)): Retries the entire loop if it fails (e.g., times out).Loop(Retry(Task)): Retries individual polling attempts within the loop.
The second option (Loop(Retry(Task))) is likely the more intuitive and useful behavior for most use cases. This should be explicitly decided, documented in the spec, and tested. I suggest adding a task to address this:
- [ ] 1.x Define and document the interaction between `LoopingExecutionStrategy` and `RetryingExecutionStrategy`.|
|
||
| ## What Changes | ||
| - **TaskStep Interface**: Add optional `loop` configuration. | ||
| - **Loop Configuration**: Define `interval` (delay between checks), `maxIterations` (timeout safety), and `until` (completion predicate). |
There was a problem hiding this comment.
The proposed Loop Configuration is a great start. To make it more robust, consider adding support for a backoff strategy, similar to the existing RetryingExecutionStrategy. Polling a service that is starting up or under heavy load could benefit from an exponential or jittered backoff to avoid overwhelming it.
The configuration could be extended like this:
- **Loop Configuration**: Define `interval` (delay), `backoff` ('fixed' | 'exponential'), `maxIterations`, and `until` (predicate).This would provide more flexibility and align the looping feature with the capabilities of the retry mechanism.
| #### Scenario: Loop timeout (max iterations) | ||
| - **GIVEN** a task with `loop` configuration | ||
| - **WHEN** the `maxIterations` limit is reached without the condition becoming true | ||
| - **THEN** it SHALL return the last result (or a specific failure status). |
There was a problem hiding this comment.
The specification for loop timeout is a bit ambiguous with "(or a specific failure status)". It would be beneficial to define the exact behavior. For example, does the loop itself introduce a new status like loop_timed_out, or does it simply return the last TaskResult from the final attempt?
A more explicit requirement would be:
"THEN it SHALL fail the task with a TaskResult indicating status failed and a message specifying that the loop timed out after reaching maxIterations."
This clarity will ensure consistent implementation and predictable behavior for users.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
1 participant
Nexus Proposal for Task Loop / Polling support.
Identified a gap where users misuse Retries for waiting on async states.
Proposed a new
LoopingExecutionStrategyandloopconfig onTaskStep.PR created automatically by Jules for task 2761546751575442487 started by @thalesraymond