diff --git a/CHANGELOG.md b/CHANGELOG.md index b493ccc860..08bcde9206 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -63,6 +63,21 @@ Added `--priority ` to `update-task`, validated against the same already were (independently settable, recorded in the task audit log as `priority: -> `, no status argument required). +### Fixed — `update-task` had no way to record a blocker discovered after creation + +`blocked_by` was only settable at `create-task` time; `update-task` accepted +only `--assignee`/`--project`, so a task moved to `blocked` status after the +fact (the normal case — a gate usually emerges once work is already +underway) had no structured way to record what was blocking it, beyond an +unstructured event-meta note the tooling can't read. `check-stale-blockers` +was scanning 100+ blocked tasks a night and finding almost none of them +checkable for exactly this reason. Added `--blocked-by ` to +`update-task` (and a matching `blockedBy` option on `updateTask`): additive +(never replaces the existing list), deduped, cycle-checked the same way +`create-task` is (rejecting before any write, so a rejected cycle leaves +zero partial state), and it writes the same symmetric `blocks` edge on the +peer task that `create-task` does. + ### Fixed — `check-stale-blockers` summary had no coverage denominator `resolved_dependency: 0` in `StaleBlockerReport.summary` was indistinguishable diff --git a/src/bus/task.ts b/src/bus/task.ts index 3c6ba2cea9..1f8d63918a 100644 --- a/src/bus/task.ts +++ b/src/bus/task.ts @@ -399,7 +399,9 @@ function findTaskFileByPrefix( } /** - * Update a task's status, and/or reroute it to a new assignee/project/priority. + * Update a task's status, and/or reroute it to a new + * assignee/project/priority, and/or append to its description, and/or append + * blocker edges. * Matches bash update-task.sh behavior for the status-only case, with the * cross-org fallback from findTaskFile so an assignee in one org can drive * the lifecycle of a task filed by an orchestrator in a sibling org. @@ -407,7 +409,8 @@ function findTaskFileByPrefix( * `status` is optional so a caller can reassign/re-project/re-prioritize a task * without restating (and risking accidentally churning) its current status — * create-task is the only place assignee/project/priority are otherwise settable. - * At least one of status/assignee/project/priority/appendDesc must be given. + * At least one of status/assignee/project/priority/appendDesc/blockedBy must + * be given. * * `appendDesc` deliberately APPENDS rather than replaces: a task description * cannot otherwise be corrected after creation, which twice in one hour drove @@ -417,22 +420,39 @@ function findTaskFileByPrefix( * alongside the addition matters specifically when the addition is a * correction: a retraction next to the false claim it retracts is legible in * a way that a silent overwrite is not. + * + * `opts.blockedBy` is ADDITIVE, never a replace: a gate normally emerges + * AFTER creation (that's why a task transitions to `blocked` at all), and + * create-task already owns the initial full list. IDs already present in + * blocked_by are silently deduped rather than re-validated or re-audited — + * only genuinely new edges go through the cycle check and get a symmetric + * `blocks` edge written on the peer, mirroring createTask's own + * validate-before-write / mutate-peers-after-write ordering so a rejected + * cycle never leaves partial state on disk. */ export function updateTask( paths: BusPaths, taskId: string, status?: TaskStatus, - opts: { assignee?: string; project?: string; priority?: Priority; appendDesc?: string } = {}, + opts: { + assignee?: string; + project?: string; + priority?: Priority; + appendDesc?: string; + blockedBy?: string[]; + } = {}, ): void { + const blockedByInput = opts.blockedBy ?? []; if ( status === undefined && opts.assignee === undefined && opts.project === undefined && opts.priority === undefined && - opts.appendDesc === undefined + opts.appendDesc === undefined && + blockedByInput.length === 0 ) { throw new Error( - 'updateTask requires at least one of: status, assignee, project, priority, appendDesc', + 'updateTask requires at least one of: status, assignee, project, priority, appendDesc, blockedBy', ); } if (opts.priority !== undefined) validatePriority(opts.priority); @@ -445,11 +465,27 @@ export function updateTask( let prevStatus: TaskStatus | undefined; let auditAgent: string | undefined; const noteParts: string[] = []; + let newBlockers: string[] = []; try { const content = readFileSync(filePath, 'utf-8'); const task: Task = JSON.parse(content); prevStatus = task.status; auditAgent = task.assigned_to; + + const currentBlockedBy = task.blocked_by ?? []; + newBlockers = blockedByInput.filter(depId => !currentBlockedBy.includes(depId)); + if (newBlockers.length) { + // Cycle check BEFORE any field mutation, same ordering rule createTask + // enforces — a rejected cycle must never leave partial state on disk. + // The virtual task carries the FULL post-update blocked_by set (old + + // new), not just the new edges, so a cycle running back through an + // already-existing blocker is still caught. + const virtualTask = { id: taskId, blocked_by: [...currentBlockedBy, ...newBlockers] }; + detectCycleOrThrow(paths, taskId, newBlockers, virtualTask); + task.blocked_by = [...currentBlockedBy, ...newBlockers]; + noteParts.push(`blocked_by: +[${newBlockers.join(', ')}]`); + } + if (status !== undefined) task.status = status; if (opts.assignee !== undefined && opts.assignee !== task.assigned_to) { noteParts.push(`assignee: ${task.assigned_to} -> ${opts.assignee}`); @@ -474,6 +510,11 @@ export function updateTask( } catch (err) { throw new Error(`Task ${taskId} update failed: ${err}`); } + + // Symmetric edge maintenance — mirrors createTask's own post-write step. + // Cycle-safe now: validation already passed above. + for (const depId of newBlockers) addSymmetricEdge(paths, depId, 'blocks', taskId); + appendTaskAudit(paths, taskId, { event: 'update', agent: auditAgent || 'unknown', diff --git a/src/cli/bus.ts b/src/cli/bus.ts index 85a505de1d..fccfd25553 100644 --- a/src/cli/bus.ts +++ b/src/cli/bus.ts @@ -239,14 +239,17 @@ busCommand busCommand .command('update-task') .argument('', 'Task ID') - .argument('[status]', 'New status (pending, in_progress, completed, blocked, cancelled) — optional when --assignee/--project/--priority/--append-desc is given') + .argument('[status]', 'New status (pending, in_progress, completed, blocked, cancelled) — optional when --assignee/--project/--priority/--append-desc/--blocked-by is given') .option('--assignee ', 'Reroute the task to a different agent') .option('--project ', 'Change the task\'s project') .option('--priority ', 'Change the task\'s priority (urgent, high, normal, low)') .option('--append-desc ', 'Append text to the description with a timestamp — does NOT overwrite the original (a description cannot be edited in place; this keeps a correction visible next to the claim it corrects)') - .action((id: string, status: string | undefined, opts: { assignee?: string; project?: string; priority?: string; appendDesc?: string }) => { - if (status === undefined && opts.assignee === undefined && opts.project === undefined && opts.priority === undefined && opts.appendDesc === undefined) { - console.error('Nothing to update — pass a status, --assignee, --project, --priority, and/or --append-desc'); + .option('--blocked-by ', 'Add one or more blocker task IDs (comma-separated) — ADDS to the existing blocked_by list, never replaces it; cycle-checked the same way create-task is') + .action((id: string, status: string | undefined, opts: { assignee?: string; project?: string; priority?: string; appendDesc?: string; blockedBy?: string }) => { + const parseList = (raw?: string) => (raw ? raw.split(',').map(s => s.trim()).filter(Boolean) : []); + const blockedBy = parseList(opts.blockedBy); + if (status === undefined && opts.assignee === undefined && opts.project === undefined && opts.priority === undefined && opts.appendDesc === undefined && blockedBy.length === 0) { + console.error('Nothing to update — pass a status, --assignee, --project, --priority, --append-desc, and/or --blocked-by'); process.exit(1); } if (status !== undefined) { @@ -283,13 +286,15 @@ busCommand project: opts.project, priority: opts.priority as Priority | undefined, appendDesc: opts.appendDesc, + blockedBy, }); if ( status !== undefined && opts.assignee === undefined && opts.project === undefined && opts.priority === undefined && - opts.appendDesc === undefined + opts.appendDesc === undefined && + blockedBy.length === 0 ) { // Preserve the original status-only message verbatim — scripts/ // dashboards may already parse it. @@ -301,6 +306,7 @@ busCommand opts.project !== undefined ? `project -> ${opts.project}` : null, opts.priority !== undefined ? `priority -> ${opts.priority}` : null, opts.appendDesc !== undefined ? 'description appended' : null, + blockedBy.length > 0 ? `blocked_by +[${blockedBy.join(', ')}]` : null, ].filter(Boolean); console.log(`Updated ${id}: ${changes.join(', ')}`); } diff --git a/tests/integration/bus-task-error-handling-cli.test.ts b/tests/integration/bus-task-error-handling-cli.test.ts index 60282ed43b..7eb558e469 100644 --- a/tests/integration/bus-task-error-handling-cli.test.ts +++ b/tests/integration/bus-task-error-handling-cli.test.ts @@ -203,7 +203,7 @@ describe.skipIf(!existsSync(DIST_CLI))( expect(stdout).toContain("project -> conduit"); }); - it("update-task with neither status nor --assignee/--project/--priority/--append-desc exits 1 with a clean message", async () => { + it("update-task with neither status nor --assignee/--project/--priority/--append-desc/--blocked-by exits 1 with a clean message", async () => { writeTask("task_real_005"); const { stdout, stderr, code } = await runCli([ "bus", @@ -213,7 +213,7 @@ describe.skipIf(!existsSync(DIST_CLI))( expect(code).toBe(1); expect(stderr.trim()).toBe( - "Nothing to update — pass a status, --assignee, --project, --priority, and/or --append-desc", + "Nothing to update — pass a status, --assignee, --project, --priority, --append-desc, and/or --blocked-by", ); expect(stdout).toBe(""); }); diff --git a/tests/unit/bus/task.test.ts b/tests/unit/bus/task.test.ts index 20be352dec..d7af17575a 100644 --- a/tests/unit/bus/task.test.ts +++ b/tests/unit/bus/task.test.ts @@ -128,10 +128,10 @@ describe('Task Management', () => { expect(after >= before).toBe(true); }); - it('throws when neither status, assignee, project, priority, nor appendDesc is given', () => { + it('throws when neither status, assignee, project, priority, appendDesc, nor blockedBy is given', () => { const taskId = createTask(paths, 'paul', 'acme', 'Test task'); expect(() => updateTask(paths, taskId)).toThrow( - 'updateTask requires at least one of: status, assignee, project, priority, appendDesc', + 'updateTask requires at least one of: status, assignee, project, priority, appendDesc, blockedBy', ); }); @@ -861,6 +861,90 @@ describe('Task dependency DAG (blocks / blocked_by)', () => { // Specifically: blocked should no longer be forced after 'free' // (both unblocked now, fall back to created_at ordering). }); + + describe('updateTask --blocked-by (task_1786923653812_76952898 / task_1786773702893_68177835)', () => { + it('adds a post-creation blocker edge + symmetric blocks edge on the peer', () => { + const blocker = createTask(paths, 'alice', 'acme', 'Discovered blocker'); + const taskId = createTask(paths, 'alice', 'acme', 'Blocked after the fact'); + + updateTask(paths, taskId, 'blocked', { blockedBy: [blocker] }); + + expect(readTask(taskId).blocked_by).toEqual([blocker]); + expect(readTask(blocker).blocks).toEqual([taskId]); + }); + + it('is additive: a second call appends rather than replacing the existing list', () => { + const a = createTask(paths, 'alice', 'acme', 'A'); + const b = createTask(paths, 'alice', 'acme', 'B'); + const taskId = createTask(paths, 'alice', 'acme', 'Task', { blockedBy: [a] }); + + updateTask(paths, taskId, undefined, { blockedBy: [b] }); + + expect(readTask(taskId).blocked_by).toEqual([a, b]); + expect(readTask(a).blocks).toEqual([taskId]); + expect(readTask(b).blocks).toEqual([taskId]); + }); + + it('dedupes an id already present in blocked_by rather than erroring or duplicating', () => { + const a = createTask(paths, 'alice', 'acme', 'A'); + const taskId = createTask(paths, 'alice', 'acme', 'Task', { blockedBy: [a] }); + + updateTask(paths, taskId, undefined, { blockedBy: [a] }); + + expect(readTask(taskId).blocked_by).toEqual([a]); + expect(readTask(a).blocks).toEqual([taskId]); + }); + + it('cycle detection: rejects a post-creation edge that would close a loop', () => { + const a = createTask(paths, 'alice', 'acme', 'A'); + // B is blocked_by A. Adding "A blocked_by B" would close the loop A -> B -> A. + const b = createTask(paths, 'alice', 'acme', 'B', { blockedBy: [a] }); + expect(() => updateTask(paths, a, undefined, { blockedBy: [b] })).toThrow(/cycle/i); + }); + + it('rejects a task declaring itself as its own blocker', () => { + const taskId = createTask(paths, 'alice', 'acme', 'Self-referential'); + expect(() => updateTask(paths, taskId, undefined, { blockedBy: [taskId] })).toThrow(/cycle/i); + }); + + it('REGRESSION: a rejected cycle leaves blocked_by and the peer\'s blocks list untouched', () => { + const a = createTask(paths, 'alice', 'acme', 'A'); + const b = createTask(paths, 'alice', 'acme', 'B', { blockedBy: [a] }); + const aBlocksBefore = readTask(a).blocks ?? []; + const bBlockedByBefore = readTask(b).blocked_by ?? []; + + expect(() => updateTask(paths, a, undefined, { blockedBy: [b] })).toThrow(/cycle/i); + + expect(readTask(a).blocked_by ?? []).toEqual([]); + expect(readTask(a).blocks ?? []).toEqual(aBlocksBefore); + expect(readTask(b).blocked_by ?? []).toEqual(bBlockedByBefore); + }); + + it('records the new edge in the task audit log', () => { + const blocker = createTask(paths, 'alice', 'acme', 'Blocker'); + const taskId = createTask(paths, 'alice', 'acme', 'Task'); + + updateTask(paths, taskId, 'blocked', { blockedBy: [blocker] }); + + const log = readTaskAudit(paths, taskId); + const entry = log[log.length - 1]; + expect(entry.note).toContain(`blocked_by: +[${blocker}]`); + }); + + it('the exact motivating scenario: a blocked task with no blocked_by is now recordable, and check-stale-blockers can then evaluate it', () => { + const blocker = createTask(paths, 'alice', 'acme', 'Blocker'); + const taskId = createTask(paths, 'alice', 'acme', 'Orphaned block'); + updateTask(paths, taskId, 'blocked'); + expect(readTask(taskId).blocked_by).toBeUndefined(); + + // The gap this feature closes: attach the blocker after the fact. + updateTask(paths, taskId, undefined, { blockedBy: [blocker] }); + expect(readTask(taskId).blocked_by).toEqual([blocker]); + + const open = checkTaskDependencies(paths, taskId); + expect(open).toEqual([{ id: blocker, status: 'pending' }]); + }); + }); }); describe('compactTasks — semantic compaction of old completed tasks', () => {