feat: Completion Dependencies (Always RunCondition)#257
feat: Completion Dependencies (Always RunCondition)#257thalesraymond wants to merge 1 commit intomainfrom
Conversation
- Updates `TaskStep.ts` with `TaskRunCondition` and mixed dependencies type - Updates `TaskStateManager.ts` to support 'always' execution condition and execute cleanup tasks regardless of preceding task status - Updates `TaskRunner.ts` to parse mixed dependencies for graph execution and validation - Adds full suite of tests covering the 'always' dependency flow - Maintains 100% test coverage and passes all linting - Archives the specification according to OpenSpec workflow 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. |
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![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 new 'always' run condition for task dependencies, allowing cleanup tasks to execute even if their preceding tasks fail. The implementation updates the task runner and state manager to handle these dependencies and includes comprehensive tests to verify the behavior. I have provided a suggestion to improve the condition logic in TaskStateManager.ts to ensure that 'always' tasks also trigger when a parent task is cancelled, rather than just when it fails.
| // If the dependent runs 'always' and the parent task *actually failed* (not skipped), we can unblock it | ||
| if (dependent.condition === "always" && currentResult?.status === "failure") { | ||
| this.decrementDependencyCount(dependent.step); | ||
| continue; | ||
| } |
There was a problem hiding this comment.
The current logic for always dependencies only considers the failure status to unblock the dependent task. This means if a parent task is cancelled, any always dependent task will be incorrectly skipped instead of being executed for cleanup.
According to the feature's goal of ensuring cleanup tasks run, always should trigger on any non-successful completion of the parent task, except when the parent was skipped.
I suggest modifying the condition to unblock always dependents for any terminal status that isn't skipped, which would correctly include both failure and cancelled.
| // If the dependent runs 'always' and the parent task *actually failed* (not skipped), we can unblock it | |
| if (dependent.condition === "always" && currentResult?.status === "failure") { | |
| this.decrementDependencyCount(dependent.step); | |
| continue; | |
| } | |
| // If the dependent runs 'always' and the parent task *actually ran* (i.e., was not skipped), we can unblock it | |
| if (dependent.condition === "always" && currentResult && currentResult.status !== "skipped") { | |
| this.decrementDependencyCount(dependent.step); | |
| continue; | |
| } |
1 participant
Implement the feature to support completion dependencies by adding granular execution controls (runConditions). Dependencies can now be assigned an 'always' condition allowing teardowns and cleanup tasks to execute successfully even if their parent steps result in failure.
PR created automatically by Jules for task 14820992859916586404 started by @thalesraymond