Conversation
Adds `loop` configuration to `TaskStep` to support conditional re-execution of a task until a condition is met. Introduces `TaskLoopConfig` interface. Implements `LoopingExecutionStrategy` that conditionally wraps `RetryingExecutionStrategy` inside `TaskRunnerBuilder`. Adds complete test coverage for polling delays, max iterations limits, and cancellation handling. Marks the related specification as implemented and archives the OpenSpec proposal. Co-authored-by: google-labs-jules[bot] <161369871+google-labs-jules[bot]@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, 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 introduces a new task looping mechanism, enabling tasks to be executed repeatedly until a specified condition is met. This feature is implemented via a dedicated 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
|
![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 task looping feature, allowing tasks to be polled until a condition is met. The implementation adds a LoopingExecutionStrategy which is correctly integrated into the TaskRunnerBuilder to wrap other strategies. The changes are well-tested. My feedback includes suggestions to improve the developer experience by changing a default value, enhance maintainability by removing duplicated code, and increase test reliability by using fake timers.
| return this.innerStrategy.execute(step, context, signal); | ||
| } | ||
|
|
||
| const maxIterations = config.maxIterations ?? 1; |
There was a problem hiding this comment.
The default value for maxIterations is set to 1. This can lead to unexpected behavior where a task with a loop config only runs once and then fails if the until condition isn't met. This forces users to always specify maxIterations to get any looping behavior. A more intuitive default, like Infinity, would allow for indefinite polling until the condition is met or the task is cancelled.
| const maxIterations = config.maxIterations ?? 1; | |
| const maxIterations = config.maxIterations ?? Infinity; |
| private sleep(ms: number, signal?: AbortSignal): Promise<void> { | ||
| if (ms <= 0) { | ||
| return Promise.resolve(); | ||
| } | ||
|
|
||
| return new Promise((resolve, reject) => { | ||
| if (signal?.aborted) { | ||
| reject(new Error("AbortError")); | ||
| return; | ||
| } | ||
|
|
||
| const timer = setTimeout(() => { | ||
| cleanup(); | ||
| resolve(); | ||
| }, ms); | ||
|
|
||
| const onAbort = () => { | ||
| clearTimeout(timer); | ||
| cleanup(); | ||
| reject(new Error("AbortError")); | ||
| }; | ||
|
|
||
| const cleanup = () => { | ||
| signal?.removeEventListener("abort", onAbort); | ||
| }; | ||
|
|
||
| signal?.addEventListener("abort", onAbort); | ||
| }); | ||
| } |
There was a problem hiding this comment.
This sleep method, which correctly handles AbortSignal, appears to be identical to the one in RetryingExecutionStrategy.ts. To adhere to the Don't Repeat Yourself (DRY) principle and improve maintainability, consider extracting this function into a shared utility file (e.g., src/utils/sleep.ts) and importing it in both strategies.
References
- To improve maintainability, define common utilities or constants in a shared file and import them where needed, rather than duplicating code. This principle extends beyond just strings to shared functions like
sleep.
| const start = Date.now(); | ||
| await strategy.execute(step, {}); | ||
| const end = Date.now(); | ||
|
|
||
| expect(mockInnerStrategy.execute).toHaveBeenCalledTimes(2); | ||
| // Should have waited at least 10ms once | ||
| expect(end - start).toBeGreaterThanOrEqual(5); |
There was a problem hiding this comment.
This test relies on Date.now() and real-time delays to verify the interval functionality. This can lead to flaky and slow tests, and the assertion toBeGreaterThanOrEqual(5) is not very precise for a 10ms interval. It's highly recommended to use vi.useFakeTimers() to control the passage of time deterministically. This will make the test faster, more reliable, and allow for more precise assertions about timing. The test should handle cancellation during sleep would also benefit from this approach.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
1 participant
Implement LoopingExecutionStrategy feature per the feat-task-loop proposal in OpenSpec workflow. Ensure tasks can be conditionally polled without implementing imperative loops within their
runcontext. TheTaskLoopConfignow supports intervals, maxIterations, and anuntilpredicate. Integrated cleanly intoTaskRunnerBuilderand verified with comprehensive unit tests for intervals, limits, failure states, and cancellations. All linters and tests pass natively. Proposal is marked complete and moved to archive.PR created automatically by Jules for task 7473620260144476195 started by @thalesraymond