Conversation
- Update `TaskStep` interface to support optional `tags` array - Create `TaskFilterConfig` interface to configure include/exclude rules - Implement robust `filterTasks` utility to handle names, tags, and dependencies - Integrate `filterTasks` into `TaskRunner` execution flow before graph validation - Provide comprehensive unit and integration tests for filtering logic /openspec-apply add-task-filtering /openspec-archive add-task-filtering 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. |
There was a problem hiding this comment.
Code Review
This pull request introduces task filtering capabilities to the TaskRunner, enabling the execution of specific task subsets based on names or tags. It adds a tags property to TaskStep, a filter configuration to TaskRunnerExecutionConfig, and a filterTasks utility. Feedback focuses on optimizing the filtering logic for performance using Set lookups, ensuring dependency resolution respects exclusion rules, and improving code quality by replacing indexed loops and non-null assertions with for...of loops.
| const { | ||
| includeTags = [], | ||
| excludeTags = [], | ||
| includeNames = [], | ||
| excludeNames = [], | ||
| includeDependencies = false, | ||
| } = config; | ||
|
|
||
| const hasInclusions = includeTags.length > 0 || includeNames.length > 0; | ||
|
|
||
| // 1. Initial Filtering | ||
| const selectedTasks = new Set<string>(); | ||
|
|
||
| for (let i = 0; i < steps.length; i++) { | ||
| const step = steps[i]!; | ||
|
|
||
| // Evaluate exclusions first | ||
| const isExcludedByName = excludeNames.includes(step.name); | ||
| const isExcludedByTag = step.tags?.some(tag => excludeTags.includes(tag)) ?? false; | ||
|
|
||
| if (isExcludedByName || isExcludedByTag) { | ||
| continue; | ||
| } | ||
|
|
||
| if (!hasInclusions) { | ||
| selectedTasks.add(step.name); | ||
| continue; | ||
| } | ||
|
|
||
| // Evaluate inclusions | ||
| const isIncludedByName = includeNames.includes(step.name); | ||
| const isIncludedByTag = step.tags?.some(tag => includeTags.includes(tag)) ?? false; | ||
|
|
||
| if (isIncludedByName || isIncludedByTag) { | ||
| selectedTasks.add(step.name); | ||
| } | ||
| } | ||
|
|
||
| // 2. Resolve Dependencies | ||
| if (includeDependencies) { | ||
| const stepMap = new Map<string, TaskStep<T>>(); | ||
| for (let i = 0; i < steps.length; i++) { | ||
| stepMap.set(steps[i]!.name, steps[i]!); | ||
| } | ||
|
|
||
| const queue = Array.from(selectedTasks); | ||
| let head = 0; | ||
|
|
||
| while (head < queue.length) { | ||
| const currentName = queue[head]!; | ||
| head++; | ||
|
|
||
| const step = stepMap.get(currentName); | ||
| if (!step) continue; | ||
| if (step.dependencies) { | ||
| for (let i = 0; i < step.dependencies.length; i++) { | ||
| const depName = step.dependencies[i]!; | ||
| if (!selectedTasks.has(depName)) { | ||
| selectedTasks.add(depName); | ||
| queue.push(depName); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // 3. Return Filtered Array | ||
| const result: TaskStep<T>[] = []; | ||
| for (let i = 0; i < steps.length; i++) { | ||
| const step = steps[i]!; | ||
| if (selectedTasks.has(step.name)) { | ||
| result.push(step); | ||
| } | ||
| } |
There was a problem hiding this comment.
The filterTasks function can be improved in terms of performance, correctness, and adherence to TypeScript best practices:
- Performance: Use
Setobjects forincludeTags,excludeTags,includeNames, andexcludeNamesto ensure O(1) lookup time. The current implementation usesArray.prototype.includesinside loops, leading to O(N * M) complexity. - Correctness: The dependency resolution phase should respect the exclusion filters. Currently, dependencies are added to the execution set even if they were explicitly excluded by name or tag. By respecting exclusions here, the
TaskRunner's graph validation will correctly identify missing required dependencies. - Maintainability: Use
for...ofloops instead of indexedforloops to avoid the non-null assertion operator (!), adhering to the project's general rules.
const includeTagsSet = new Set(config.includeTags ?? []);
const excludeTagsSet = new Set(config.excludeTags ?? []);
const includeNamesSet = new Set(config.includeNames ?? []);
const excludeNamesSet = new Set(config.excludeNames ?? []);
const { includeDependencies = false } = config;
const hasInclusions = includeTagsSet.size > 0 || includeNamesSet.size > 0;
// 1. Initial Filtering
const selectedTasks = new Set<string>();
for (const step of steps) {
// Evaluate exclusions first
const isExcludedByName = excludeNamesSet.has(step.name);
const isExcludedByTag = step.tags?.some((tag) => excludeTagsSet.has(tag)) ?? false;
if (isExcludedByName || isExcludedByTag) {
continue;
}
if (!hasInclusions) {
selectedTasks.add(step.name);
continue;
}
// Evaluate inclusions
const isIncludedByName = includeNamesSet.has(step.name);
const isIncludedByTag = step.tags?.some((tag) => includeTagsSet.has(tag)) ?? false;
if (isIncludedByName || isIncludedByTag) {
selectedTasks.add(step.name);
}
}
// 2. Resolve Dependencies
if (includeDependencies) {
const stepMap = new Map<string, TaskStep<T>>();
for (const step of steps) {
stepMap.set(step.name, step);
}
const queue = Array.from(selectedTasks);
for (let i = 0; i < queue.length; i++) {
const currentName = queue[i];
if (currentName === undefined) continue;
const step = stepMap.get(currentName);
if (!step || !step.dependencies) continue;
for (const depName of step.dependencies) {
if (!selectedTasks.has(depName)) {
const depStep = stepMap.get(depName);
const isExcludedByName = excludeNamesSet.has(depName);
const isExcludedByTag = depStep?.tags?.some((tag) => excludeTagsSet.has(tag)) ?? false;
if (!isExcludedByName && !isExcludedByTag) {
selectedTasks.add(depName);
queue.push(depName);
}
}
}
}
}
// 3. Return Filtered Array
const result: TaskStep<T>[] = [];
for (const step of steps) {
if (selectedTasks.has(step.name)) {
result.push(step);
}
}References
- When frequent lookups of items by a specific key are required, use a Map (or dictionary/hash table) to store the items, indexed by that key, to ensure O(1) access time.
- In TypeScript, avoid using the non-null assertion operator (!). Instead, use explicit checks or safe fallbacks like the nullish coalescing operator (??) to handle potentially null or undefined values. This improves robustness against future changes that might break assumptions about the value's existence.
| it("should handle exclusions even if includeDependencies pulls them in", () => { | ||
| // Current implementation: Initial filtering applies inclusions and exclusions. | ||
| // If includeDependencies is true, it recursively adds dependencies of *initially selected* tasks, | ||
| // overriding exclusions for those dependencies if they weren't in the initial set. | ||
| // This test verifies the current behavior, if we want strict exclusion we might need to modify filterTasks. | ||
| // But currently, the design is: dependencies of included tasks are included. | ||
| const filtered = filterTasks(tasks, { includeNames: ["taskB"], includeDependencies: true, excludeNames: ["taskA"] }); | ||
| expect(filtered.map(t => t.name).sort()).toEqual(["taskA", "taskB"].sort()); | ||
| }); |
There was a problem hiding this comment.
This test case currently expects that includeDependencies overrides explicit exclusions. However, for consistency and predictability, exclusions should generally take precedence. If a task is explicitly excluded, it should not be pulled back into the execution set. Consider updating the implementation to respect exclusions during dependency resolution and updating this test to verify that behavior.
it("should respect exclusions even if includeDependencies is true", () => {
// If taskB is included and depends on taskA, but taskA is excluded,
// taskA should NOT be pulled in.
const filtered = filterTasks(tasks, { includeNames: ["taskB"], includeDependencies: true, excludeNames: ["taskA"] });
expect(filtered.map(t => t.name)).toEqual(["taskB"]);
});Co-authored-by: thalesraymond <32554150+thalesraymond@users.noreply.github.com>
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |



Implemented the pending specification from
openspec/changes/add-task-filtering/.TaskRunner. Developers can now pass aTaskFilterConfigto optionally include or exclude tasks by name or tag. It also supports recursively resolving dependencies using theincludeDependenciesflag.pnpm installensured dependencies are up-to-date.pnpm buildpassed without errors.pnpm lintpassed with allanytypes correctly resolved tounknown.pnpm testpassed successfully with 100% test coverage across all branches, functions, and lines.includeDependencies: true), ensuring workflow integrity./openspec-archive add-task-filteringPR created automatically by Jules for task 12736052601112169362 started by @thalesraymond