diff --git a/src/lib/agent/runner/sequence/orchestrator/__tests__/seeded-decline-skip.test.ts b/src/lib/agent/runner/sequence/orchestrator/__tests__/seeded-decline-skip.test.ts new file mode 100644 index 00000000..d2379b32 --- /dev/null +++ b/src/lib/agent/runner/sequence/orchestrator/__tests__/seeded-decline-skip.test.ts @@ -0,0 +1,126 @@ +/** + * Declines are applied before the drain, not inside it. + * + * The executor marks a task running — firing `orchestrator task started` — and + * only then hands it to `runTask`. A decline applied on the far side of that + * reported a start for a task no agent ever ran, so the declines landed on both + * sides of every rate measured against starts. + */ +import * as fs from 'fs'; +import * as os from 'os'; +import * as path from 'path'; + +vi.mock('@ui', () => ({ + getUI: () => ({ showTaskNotice: vi.fn(), cancelTaskNotice: vi.fn() }), +})); +vi.mock('@utils/analytics', () => ({ + analytics: { + wizardCapture: vi.fn(), + setTag: vi.fn(), + capture: vi.fn(), + captureException: vi.fn(), + }, +})); + +import { + skipDeclinedSeededTasks, + type SeededConsent, +} from '@lib/agent/runner/sequence/orchestrator/orchestrator-runner'; +import { + QueueStore, + SkipReason, + TaskStatus, + type TransitionEvent, + type QueuedTask, +} from '@lib/agent/runner/sequence/orchestrator/queue'; + +const KEPT: SeededConsent = { keep: true, timedOut: false, errored: false }; +const DECLINED: SeededConsent = { + keep: false, + timedOut: false, + errored: false, +}; +const TIMED_OUT: SeededConsent = { + keep: false, + timedOut: true, + errored: false, +}; + +const labelFor = (t: { type: string; label?: string }) => t.label ?? t.type; + +describe('skipDeclinedSeededTasks', () => { + let dir: string; + let store: QueueStore; + let events: TransitionEvent[]; + + beforeEach(() => { + dir = fs.mkdtempSync(path.join(os.tmpdir(), 'seeded-decline-')); + events = []; + store = new QueueStore(dir, 'run-1', { + onTransition: (event: TransitionEvent) => events.push(event), + }); + }); + + afterEach(() => fs.rmSync(dir, { recursive: true, force: true })); + + const seed = (): QueuedTask => + store.enqueue({ + type: 'warehouse', + label: 'Connect your data sources', + inputs: {}, + dependsOn: [], + enqueuedBy: 'orchestrator', + }); + + it('skips a declined task without ever starting it', () => { + const task = seed(); + + const skipped = skipDeclinedSeededTasks( + store, + new Map([[task.id, DECLINED]]), + labelFor, + ); + + expect(skipped).toBe(1); + expect(events).not.toContain('start'); + expect(events.filter((e) => e === 'skip')).toHaveLength(1); + expect(store.get(task.id)?.status).toBe(TaskStatus.Skipped); + expect(store.get(task.id)?.skipReason).toBe(SkipReason.UserDeclined); + }); + + it('carries the reason the answer came about', () => { + const task = seed(); + + skipDeclinedSeededTasks(store, new Map([[task.id, TIMED_OUT]]), labelFor); + + expect(store.get(task.id)?.skipReason).toBe(SkipReason.NoticeTimeout); + // A step nobody answered for is reported as never set up, not as refused. + expect(store.readHandoff(task.id)?.did).toContain('never accepted'); + }); + + it('hands the report the task label and the user-declined wording', () => { + const task = seed(); + + skipDeclinedSeededTasks(store, new Map([[task.id, DECLINED]]), labelFor); + + const handoff = store.readHandoff(task.id); + expect(handoff?.goals).toBe('Connect your data sources'); + expect(handoff?.forNextAgent).toContain('declined'); + }); + + it('leaves an accepted task pending for the drain', () => { + const task = seed(); + + expect( + skipDeclinedSeededTasks(store, new Map([[task.id, KEPT]]), labelFor), + ).toBe(0); + expect(store.get(task.id)?.status).toBe(TaskStatus.Pending); + expect(events).not.toContain('skip'); + }); + + it('ignores an answer whose task is no longer in the queue', () => { + expect( + skipDeclinedSeededTasks(store, new Map([['gone', DECLINED]]), labelFor), + ).toBe(0); + }); +}); diff --git a/src/lib/agent/runner/sequence/orchestrator/orchestrator-runner.ts b/src/lib/agent/runner/sequence/orchestrator/orchestrator-runner.ts index d6a75a59..26aa24a2 100644 --- a/src/lib/agent/runner/sequence/orchestrator/orchestrator-runner.ts +++ b/src/lib/agent/runner/sequence/orchestrator/orchestrator-runner.ts @@ -260,6 +260,53 @@ export function consentSkipReason(consent: SeededConsent): SkipReason { return consent.timedOut ? SkipReason.NoticeTimeout : SkipReason.UserDeclined; } +/** + * Apply the seed-time answers to the queue, before the drain starts. + * + * A declined task is skipped rather than dropped, so the graph the planner saw + * is the graph that ran — the sink already depends on this task, and + * `nextRunnable` treats a skipped dependency as satisfied, so the report still + * runs and can say the step was declined. The decline also stays in the funnel, + * arriving as a `skipped` event carrying the reason that caused it rather than + * as a task that silently never existed. + * + * It happens here rather than inside `runTask` because the executor marks a + * task running — and fires `orchestrator task started` — before it hands the + * task over. A decline applied on the far side of that emitted a start for a + * task no agent ever ran, so every rate measured against starts counted the + * declines twice: once in the denominator as a task that began, and again in + * the numerator as a task that was skipped. Nothing upstream of the drain reads + * task status, so applying the answers here changes only what the drain sees. + * + * Returns how many tasks were skipped, so the caller can redraw once. + */ +export function skipDeclinedSeededTasks( + store: Pick, + consentByTaskId: ReadonlyMap, + labelFor: (task: { type: string; label?: string }) => string, +): number { + let skipped = 0; + for (const [taskId, consent] of consentByTaskId) { + if (consent.keep) continue; + const task = store.get(taskId); + if (!task) continue; + const reason = consentSkipReason(consent); + logToFile(`[orchestrator] runner-seeded ${task.type} skipped: ${reason}`); + const declinedByUser = reason === SkipReason.UserDeclined; + store.skip(taskId, reason, { + goals: labelFor(task), + did: declinedByUser + ? 'Nothing — the user chose to skip this step when offered it.' + : 'Nothing — the step was offered at the start of the run and never accepted.', + forNextAgent: declinedByUser + ? 'This step was offered and declined, so it did no work. Report it as skipped at the user’s request, not as failed.' + : 'This step was offered and never accepted, so it did no work. Report it as not set up, and point the user at how to do it later.', + }); + skipped += 1; + } + return skipped; +} + /** * Ask for one seeded task's consent, and record how the answer came about. * @@ -910,37 +957,6 @@ export async function runOrchestrator( const runTask: RunTask = async (task) => { renderQueue(); - // A task that stops for the user is offered, not imposed. The offer was - // made at seed time; this applies the answer, now that the drain has - // reached the task. - // - // A declined task is skipped here rather than dropped at seed time, for two - // reasons. The graph the planner saw is then the graph that ran — the sink - // already depends on this task, and `nextRunnable` treats a skipped - // dependency as satisfied, so the report still runs and can say the step was - // declined. And the decline stays in the funnel: it arrives as a `skipped` - // event carrying the reason that caused it, rather than as a task that - // silently never existed. `orchestrator task skipped` is only readable that - // way because it now carries `reason`; without it, declines and timeouts and - // agent no-ops were one indistinguishable number. - const consent = seededConsent.get(task.id); - if (consent && !consent.keep) { - const reason = consentSkipReason(consent); - logToFile(`[orchestrator] runner-seeded ${task.type} skipped: ${reason}`); - const declinedByUser = reason === SkipReason.UserDeclined; - store.skip(task.id, reason, { - goals: labelFor(task), - did: declinedByUser - ? 'Nothing — the user chose to skip this step when offered it.' - : 'Nothing — the step was offered at the start of the run and never accepted.', - forNextAgent: declinedByUser - ? 'This step was offered and declined, so it did no work. Report it as skipped at the user’s request, not as failed.' - : 'This step was offered and never accepted, so it did no work. Report it as not set up, and point the user at how to do it later.', - }); - renderQueue(); - return; - } - try { const resolved = resolveTask(registry, task, store); // Task instructions are one-run scaffolding, not durable skills, so they @@ -1036,6 +1052,13 @@ export async function runOrchestrator( renderQueue(); } }; + // A task that stops for the user is offered, not imposed, and the answer was + // taken at seed time. Apply it before the drain begins, so the drain only + // ever starts tasks that are going to run. + if (skipDeclinedSeededTasks(store, seededConsent, labelFor) > 0) { + renderQueue(); + } + try { await drainQueue(store, runTask); } finally {