From be38e000e7158ca87a6150a3c67b8b27f72ac735 Mon Sep 17 00:00:00 2001 From: Tom Owers Date: Sun, 6 Sep 2026 13:58:02 +0200 Subject: [PATCH] fix(orchestrator): don't report a declined task as a started task MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The offer for a runner-seeded task is answered at seed time, but the answer was applied inside `runTask` — which the executor calls only after it marks the task running and fires `orchestrator task started`. A declined task therefore reported a start no agent ever ran, and then a skip, so the declines landed on both sides of every rate measured against starts. Apply the answers just before the drain instead. Everything upstream is untouched: the planner still sees the task in the graph, the deferral pass and the sink-closure check run against the same queue, and the skip still carries the reason that caused it. Only the drain sees the task already terminal, so it never starts it. Generated-By: PostHog Desktop Task-Id: 39495583-4ca3-4878-a7f1-c925bd1941ff --- .../__tests__/seeded-decline-skip.test.ts | 126 ++++++++++++++++++ .../orchestrator/orchestrator-runner.ts | 85 +++++++----- 2 files changed, 180 insertions(+), 31 deletions(-) create mode 100644 src/lib/agent/runner/sequence/orchestrator/__tests__/seeded-decline-skip.test.ts 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 {