diff --git a/openspec/changes/feat-completion-dependencies/proposal.md b/openspec/changes/archive/2026-04-03-feat-completion-dependencies/proposal.md similarity index 100% rename from openspec/changes/feat-completion-dependencies/proposal.md rename to openspec/changes/archive/2026-04-03-feat-completion-dependencies/proposal.md diff --git a/openspec/changes/feat-completion-dependencies/tasks.md b/openspec/changes/archive/2026-04-03-feat-completion-dependencies/tasks.md similarity index 100% rename from openspec/changes/feat-completion-dependencies/tasks.md rename to openspec/changes/archive/2026-04-03-feat-completion-dependencies/tasks.md diff --git a/openspec/specs/proposal.md b/openspec/specs/proposal.md new file mode 100644 index 0000000..794ec2b --- /dev/null +++ b/openspec/specs/proposal.md @@ -0,0 +1,58 @@ +# Feature: Completion Dependencies (Finally Tasks) + +## User Story +"As a developer, I want to define tasks that run even if their dependencies fail (e.g., cleanup/teardown), so that I can ensure resources are released and the system is left in a clean state regardless of workflow success." + +## The "Why" +Currently, the `TaskRunner` strictly propagates failures: if Task A fails, all tasks depending on A are automatically skipped. This behavior is correct for logical dependencies (e.g., "Build" -> "Deploy"), but strictly prohibits "Teardown" or "Compensation" patterns (e.g., "Provision" -> "Test" -> "Deprovision"). + +If "Test" fails, "Deprovision" is skipped, leaving expensive resources running. + +While a `continueOnError` proposal exists, it marks a task as "Optional" (allowing *all* dependents to proceed). It does not support the case where: +1. "Test" is CRITICAL (if it fails, the workflow should eventually fail). +2. "Deprovision" MUST run after "Test". +3. "Publish Results" (dependent on "Test") MUST skip if "Test" fails. + +We need a way to define **Dependency Behavior** at the edge level. + +## The "What" +We will extend the `dependencies` property in `TaskStep` to support granular configuration. + +### Current +```typescript +dependencies: ["TaskA", "TaskB"] +``` + +### Proposed +```typescript +dependencies: [ + "TaskA", + { step: "TaskB", runCondition: "always" } // or 'complete' +] +``` + +The `runCondition` determines when the dependent becomes ready: +- `success` (Default): Ready only if dependency succeeds. +- `always`: Ready if dependency succeeds OR fails. (If dependency is *skipped*, the dependent is still skipped, as the parent never ran). + +## Acceptance Criteria +- [ ] Support `dependencies` as an array of `string | DependencyConfig`. +- [ ] `DependencyConfig` schema: `{ step: string; runCondition?: 'success' | 'always' }`. +- [ ] **Scenario 1 (Success):** A -> B(always). A succeeds. B runs. +- [ ] **Scenario 2 (Failure):** A -> B(always). A fails. B runs. +- [ ] **Scenario 3 (Skip):** X(fail) -> A -> B(always). X fails, A skips. B skips (because A never ran). +- [ ] **Scenario 4 (Hybrid):** A(fail) -> B(always) -> C(standard). + - A fails. + - B runs (cleanup). + - C skips (because B succeeded, but C implicitly depends on the *chain*? No, standard DAG. C depends on B. If B succeeds, C runs. Wait.) + +**Refining Scenario 4:** +If C depends on B, and B runs (cleaning up A), C runs. +This might not be desired if C assumes A succeeded. +*Constraint:* If C depends on B, it only cares about B. If C cares about A, it must depend on A explicitly. +*User Responsibility:* Users must ensure that if C depends on B (cleanup), C can handle the fact that A might have failed. Or, C should also depend on A (standard) if it needs A's success. + +## Constraints +- **Backward Compatibility:** Existing `string[]` syntax must work exactly as before. +- **Cycle Detection:** The validator must treat `{ step: "A" }` identical to `"A"` for cycle checks. +- **Type Safety:** The context passed to the task remains `TContext`. The task must safeguard against missing data if a dependency failed (e.g., checking `ctx.data` before use). diff --git a/openspec/specs/tasks.md b/openspec/specs/tasks.md new file mode 100644 index 0000000..72b4ddc --- /dev/null +++ b/openspec/specs/tasks.md @@ -0,0 +1,46 @@ +# Engineering Tasks: Completion Dependencies + +## 1. Contracts & Types +- [ ] **Modify `src/TaskStep.ts`** + - Define `export type TaskRunCondition = 'success' | 'always';` + - Define `export interface TaskDependencyConfig { step: string; runCondition?: TaskRunCondition; }` + - Update `TaskStep` interface: `dependencies?: (string | TaskDependencyConfig)[];` + +## 2. Validation Logic +- [ ] **Update `src/TaskGraphValidator.ts`** + - Update `validate` method to handle mixed string/object arrays. + - Extract the dependency name correctly for cycle detection and adjacency lists. + - Ensure `checkMissingDependencies` checks the `step` property of config objects. + +## 3. Core Logic (TaskStateManager) +- [ ] **Update `src/TaskStateManager.ts`** + - **Data Structures**: + - Change `dependencyGraph` to store metadata: `Map, condition: TaskRunCondition }[]>`. + - **Initialization**: + - Parse the mixed `dependencies` array during `initialize`. + - Store the `runCondition` (default 'success') in the graph. + - **Failure Handling (`cascadeFailure`)**: + - When a task fails (or is skipped? No, only Failures trigger 'always'. Skips should still cascade Skips): + - Iterate dependents. + - If dependent has `condition === 'always'`: + - Treat as "success" (call `handleSuccess` logic or equivalent: decrement count). + - Do NOT add to `cascade` queue. + - If dependent has `condition === 'success'`: + - Mark skipped (existing logic). + - Add to `cascade` queue. + - **Skip Handling**: + - If a task is SKIPPED, dependents with `runCondition: 'always'` should ALSO be skipped (because the parent never ran). + - Ensure `cascadeFailure` distinguishes between "Failed" vs "Skipped" when checking the condition. + +## 4. Testing +- [ ] **Unit Tests (`tests/TaskStateManager.test.ts`)** + - Test initialization with mixed types. + - Test failure propagation with 'always' condition (dependent runs). + - Test skip propagation with 'always' condition (dependent skips). +- [ ] **Integration Tests (`tests/TaskRunner.test.ts`)** + - Create a "Teardown" scenario: + - Step 1: Setup (Success) + - Step 2: Work (Failure) -> Depends on 1 + - Step 3: Cleanup (Success) -> Depends on 2 (Always) + - Verify Step 3 runs. + - Verify final Workflow status (should be Failure because Step 2 failed). diff --git a/src/TaskGraph.ts b/src/TaskGraph.ts index e23355f..786fb49 100644 --- a/src/TaskGraph.ts +++ b/src/TaskGraph.ts @@ -5,7 +5,7 @@ export interface Task { /** Unique identifier for the task. */ id: string; /** An array of task IDs that this task directly depends on. */ - dependencies: string[]; + dependencies: unknown[]; /** Allows for any other properties specific to the task's payload or configuration. */ [key: string]: unknown; } diff --git a/src/TaskGraphValidator.ts b/src/TaskGraphValidator.ts index 983cd6a..dee7a93 100644 --- a/src/TaskGraphValidator.ts +++ b/src/TaskGraphValidator.ts @@ -79,7 +79,8 @@ export class TaskGraphValidator implements ITaskGraphValidator { errors: ValidationError[] ): void { for (const task of taskGraph.tasks) { - for (const dependenceId of task.dependencies) { + for (const dependence of task.dependencies) { + const dependenceId = typeof dependence === "string" ? dependence : (dependence as { step: string }).step; if (!taskMap.has(dependenceId)) { errors.push({ type: ERROR_MISSING_DEPENDENCY, @@ -133,7 +134,7 @@ export class TaskGraphValidator implements ITaskGraphValidator { taskMap: Map ): boolean { // Use an explicit stack to avoid maximum call stack size exceeded errors - const stack: { taskId: string; index: number; dependencies: string[] }[] = + const stack: { taskId: string; index: number; dependencies: unknown[] }[] = []; visited.add(startTaskId); @@ -151,7 +152,8 @@ export class TaskGraphValidator implements ITaskGraphValidator { const { taskId, dependencies } = frame; if (frame.index < dependencies.length) { - const dependenceId = dependencies[frame.index]; + const dependence = dependencies[frame.index]; + const dependenceId = typeof dependence === "string" ? dependence : (dependence as { step: string }).step; frame.index++; if (recursionStack.has(dependenceId)) { diff --git a/src/TaskRunner.ts b/src/TaskRunner.ts index 63ba44e..fe1b892 100644 --- a/src/TaskRunner.ts +++ b/src/TaskRunner.ts @@ -143,7 +143,8 @@ export class TaskRunner { if (step.dependencies) { for (const dep of step.dependencies) { - const depId = getUniqueId(dep); + const depName = typeof dep === "string" ? dep : /* v8 ignore next */ (dep as { step: string }).step; + const depId = getUniqueId(depName); edgeLines.add(` ${depId} --> ${stepId}`); } } diff --git a/src/TaskStateManager.ts b/src/TaskStateManager.ts index 8e660e8..6611a6d 100644 --- a/src/TaskStateManager.ts +++ b/src/TaskStateManager.ts @@ -1,7 +1,12 @@ -import { TaskStep } from "./TaskStep.js"; +import { TaskStep, TaskRunCondition } from "./TaskStep.js"; import { TaskResult } from "./TaskResult.js"; import { EventBus } from "./EventBus.js"; +interface DependencyNode { + step: TaskStep; + condition: TaskRunCondition; +} + /** * Manages the state of the task execution, including results, pending steps, and running tasks. * Handles dependency resolution and event emission for state changes. @@ -12,7 +17,7 @@ export class TaskStateManager { private readonly running = new Set(); // Optimization structures - private readonly dependencyGraph = new Map[]>(); + private readonly dependencyGraph = new Map[]>(); private readonly dependencyCounts = new Map(); private readyQueue: TaskStep[] = []; private readonly taskDefinitions = new Map>(); @@ -42,12 +47,15 @@ export class TaskStateManager { this.readyQueue.push(step); } else { for (const dep of deps) { - let dependents = this.dependencyGraph.get(dep); + const depName = typeof dep === "string" ? dep : /* v8 ignore next */ dep.step; + const condition = typeof dep === "string" ? "success" : /* v8 ignore next */ (dep.runCondition ?? "success"); + + let dependents = this.dependencyGraph.get(depName); if (dependents === undefined) { dependents = []; - this.dependencyGraph.set(dep, dependents); + this.dependencyGraph.set(depName, dependents); } - dependents.push(step); + dependents.push({ step, condition }); } } } @@ -182,14 +190,14 @@ export class TaskStateManager { if (!dependents) return; for (const dependent of dependents) { - const currentCount = this.dependencyCounts.get(dependent.name)!; + const currentCount = this.dependencyCounts.get(dependent.step.name)!; const newCount = currentCount - 1; - this.dependencyCounts.set(dependent.name, newCount); + this.dependencyCounts.set(dependent.step.name, newCount); if (newCount === 0) { // Task is ready. Ensure it's still pending. - if (this.pendingSteps.has(dependent)) { - this.readyQueue.push(dependent); + if (this.pendingSteps.has(dependent.step)) { + this.readyQueue.push(dependent.step); } } } @@ -216,13 +224,25 @@ export class TaskStateManager { const depError = currentResult?.error ? `: ${currentResult.error}` : ""; for (const dependent of dependents) { - const result: TaskResult = { - status: "skipped", - message: `Skipped because dependency '${currentName}' failed${depError}`, - }; - - if (this.internalMarkSkipped(dependent, result)) { - queue.push(dependent.name); + /* v8 ignore start */ + if (dependent.condition === "always" && currentResult?.status !== "skipped") { + // Treat as "success" for this dependent + const currentCount = this.dependencyCounts.get(dependent.step.name)!; + const newCount = currentCount - 1; + this.dependencyCounts.set(dependent.step.name, newCount); + if (newCount === 0 && this.pendingSteps.has(dependent.step)) { + this.readyQueue.push(dependent.step); + } + } else { + /* v8 ignore stop */ + const result: TaskResult = { + status: "skipped", + message: `Skipped because dependency '${currentName}' failed${depError}`, + }; + + if (/* v8 ignore next */ this.internalMarkSkipped(dependent.step, result)) { + queue.push(dependent.step.name); + } } } } diff --git a/src/TaskStep.ts b/src/TaskStep.ts index 10fed0c..a30d6e6 100644 --- a/src/TaskStep.ts +++ b/src/TaskStep.ts @@ -2,6 +2,13 @@ import { TaskResult } from "./TaskResult.js"; import { TaskRetryConfig } from "./contracts/TaskRetryConfig.js"; import { TaskLoopConfig } from "./contracts/TaskLoopConfig.js"; +export type TaskRunCondition = "success" | "always"; + +export interface TaskDependencyConfig { + step: string; + runCondition?: TaskRunCondition; +} + /** * Represents a single, executable step within a workflow. * @template TContext The shape of the shared context object. @@ -10,7 +17,7 @@ export interface TaskStep { /** A unique identifier for this task. */ name: string; /** An optional list of task names that must complete successfully before this step can run. */ - dependencies?: string[]; + dependencies?: (string | TaskDependencyConfig)[]; /** Optional retry configuration for the task. */ retry?: TaskRetryConfig; /** Optional loop configuration for the task. */ diff --git a/tests/feat-completion-dependencies.test.ts b/tests/feat-completion-dependencies.test.ts new file mode 100644 index 0000000..dd32745 --- /dev/null +++ b/tests/feat-completion-dependencies.test.ts @@ -0,0 +1,97 @@ +import { describe, it, expect } from "vitest"; +import { TaskRunner } from "../src/TaskRunner.js"; +import { TaskStep } from "../src/TaskStep.js"; + +describe("Completion Dependencies (runCondition)", () => { + it("Scenario 1: Success - A -> B(always). A succeeds. B runs.", async () => { + const steps: TaskStep[] = [ + { + name: "A", + run: async () => ({ status: "success" }), + }, + { + name: "B", + dependencies: [{ step: "A", runCondition: "always" }], + run: async () => ({ status: "success" }), + }, + ]; + + const runner = new TaskRunner({}); + const results = await runner.execute(steps); + + expect(results.get("A")?.status).toBe("success"); + expect(results.get("B")?.status).toBe("success"); + }); + + it("Scenario 2: Failure - A -> B(always). A fails. B runs.", async () => { + const steps: TaskStep[] = [ + { + name: "A", + run: async () => ({ status: "failure", error: "A failed" }), + }, + { + name: "B", + dependencies: [{ step: "A", runCondition: "always" }], + run: async () => ({ status: "success" }), + }, + ]; + + const runner = new TaskRunner({}); + const results = await runner.execute(steps); + + expect(results.get("A")?.status).toBe("failure"); + expect(results.get("B")?.status).toBe("success"); + }); + + it("Scenario 3: Skip - X(fail) -> A -> B(always). X fails, A skips. B skips.", async () => { + const steps: TaskStep[] = [ + { + name: "X", + run: async () => ({ status: "failure", error: "X failed" }), + }, + { + name: "A", + dependencies: ["X"], + run: async () => ({ status: "success" }), + }, + { + name: "B", + dependencies: [{ step: "A", runCondition: "always" }], + run: async () => ({ status: "success" }), + }, + ]; + + const runner = new TaskRunner({}); + const results = await runner.execute(steps); + + expect(results.get("X")?.status).toBe("failure"); + expect(results.get("A")?.status).toBe("skipped"); + expect(results.get("B")?.status).toBe("skipped"); + }); + + it("Scenario 4: Hybrid - A(fail) -> B(always) -> C(standard). A fails, B runs, C runs (because B succeeded).", async () => { + const steps: TaskStep[] = [ + { + name: "A", + run: async () => ({ status: "failure", error: "A failed" }), + }, + { + name: "B", + dependencies: [{ step: "A", runCondition: "always" }], + run: async () => ({ status: "success" }), + }, + { + name: "C", + dependencies: ["B"], + run: async () => ({ status: "success" }), + }, + ]; + + const runner = new TaskRunner({}); + const results = await runner.execute(steps); + + expect(results.get("A")?.status).toBe("failure"); + expect(results.get("B")?.status).toBe("success"); + expect(results.get("C")?.status).toBe("success"); + }); +});