From ecec3c55fd9f59c90f2281fa94f675ce62dbce9b Mon Sep 17 00:00:00 2001 From: Tom Owers Date: Wed, 2 Sep 2026 12:59:23 +0100 Subject: [PATCH] fix(orchestrator): record why an agent reported a task 'not needed' MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `SkipReason.AgentNotNeeded` records that the agent decided the skip, not what it decided, and `complete_task` offers `not needed` both for "the step does not apply to this project" and for "you cannot do it". For the one step that stops to ask the user for credentials those are opposite outcomes, and both land in a bucket documented as the first. Add an optional `notNeededReason` to `complete_task` on both harness schemas — a closed set of `not-applicable` / `user-declined` / `blocked` — carry it on the task, and stamp it onto `orchestrator task skipped` as an additive property. `reason` and every other property keep their current meaning. The value is validated against the declared set before it reaches the task: the pi harness passes tool arguments through unvalidated, and free text about this step can name a credential. Generated-By: PostHog Desktop Task-Id: 93b396ed-9a30-4d56-bd97-6bd31c232436 --- .../runner/harness/pi/orchestrator-tools.ts | 49 ++++++----- .../__tests__/queue-tools.test.ts | 79 +++++++++++++++++- .../orchestrator/__tests__/queue.test.ts | 81 +++++++++++++++++++ .../orchestrator/orchestrator-runner.ts | 5 ++ .../sequence/orchestrator/queue-tools.ts | 76 ++++++++++++----- .../runner/sequence/orchestrator/queue.ts | 50 +++++++++++- 6 files changed, 301 insertions(+), 39 deletions(-) diff --git a/src/lib/agent/runner/harness/pi/orchestrator-tools.ts b/src/lib/agent/runner/harness/pi/orchestrator-tools.ts index 50b229dd5..06bdd7e5a 100644 --- a/src/lib/agent/runner/harness/pi/orchestrator-tools.ts +++ b/src/lib/agent/runner/harness/pi/orchestrator-tools.ts @@ -19,11 +19,13 @@ import { applyEnqueue, applyReadHandoffs, HANDOFF_FIELDS, + NOT_NEEDED_REASON_ASK, REMARK_ASK, + type CompleteArgs, type EnqueueArgs, type OrchestratorToolsContext, } from '../../sequence/orchestrator/queue-tools'; -import type { TaskHandoff } from '../../sequence/orchestrator/queue'; +import { NotNeededReason } from '../../sequence/orchestrator/queue'; function text(s: string): { content: [{ type: 'text'; text: string }]; @@ -59,6 +61,32 @@ export const PI_HANDOFF_PARAM_KEYS: readonly string[] = Object.keys( HANDOFF_PARAMS.properties, ); +/** Mirrors the zod `COMPLETE_SHAPE`; ctx-independent, so it lives out here. */ +const COMPLETE_PARAMS = Type.Object({ + status: Type.Union([ + Type.Literal('done'), + Type.Literal('failed'), + Type.Literal('not needed'), + ]), + handoff: HANDOFF_PARAMS, + remark: Type.Optional(Type.String({ description: REMARK_ASK })), + notNeededReason: Type.Optional( + Type.Union( + [ + Type.Literal(NotNeededReason.NotApplicable), + Type.Literal(NotNeededReason.UserDeclined), + Type.Literal(NotNeededReason.Blocked), + ], + { description: NOT_NEEDED_REASON_ASK }, + ), + ), +}); + +/** Exported so the parity test can compare both harnesses' field sets. */ +export const PI_COMPLETE_PARAM_KEYS: readonly string[] = Object.keys( + COMPLETE_PARAMS.properties, +); + /** The three queue tools bound to one agent's orchestrator context. */ export function createPiOrchestratorTools( ctx: OrchestratorToolsContext, @@ -111,24 +139,9 @@ export function createPiOrchestratorTools( "Report the outcome of your task. Always call this exactly once when you finish, with a structured handoff for the next agent. Use status 'not needed' when the task does not apply to this project and you cannot do it (say why in the handoff) — not 'done'.", promptSnippet: 'complete_task(status, handoff) — report your outcome exactly once when done', - parameters: Type.Object({ - status: Type.Union([ - Type.Literal('done'), - Type.Literal('failed'), - Type.Literal('not needed'), - ]), - handoff: HANDOFF_PARAMS, - remark: Type.Optional(Type.String({ description: REMARK_ASK })), - }), + parameters: COMPLETE_PARAMS, execute(_id, args) { - const res = applyComplete( - ctx, - args as { - status: 'done' | 'failed' | 'not needed'; - handoff: TaskHandoff; - remark?: string; - }, - ); + const res = applyComplete(ctx, args as CompleteArgs); if (!res.ok) return Promise.resolve(text(`Error: ${res.message}`)); return Promise.resolve(text('ok')); }, diff --git a/src/lib/agent/runner/sequence/orchestrator/__tests__/queue-tools.test.ts b/src/lib/agent/runner/sequence/orchestrator/__tests__/queue-tools.test.ts index 5be5cf466..07104cf14 100644 --- a/src/lib/agent/runner/sequence/orchestrator/__tests__/queue-tools.test.ts +++ b/src/lib/agent/runner/sequence/orchestrator/__tests__/queue-tools.test.ts @@ -2,7 +2,11 @@ import * as fs from 'fs'; import * as os from 'os'; import * as path from 'path'; import { analytics } from '@utils/analytics'; -import { QueueStore } from '@lib/agent/runner/sequence/orchestrator/queue'; +import { + NotNeededReason, + QueueStore, + SkipReason, +} from '@lib/agent/runner/sequence/orchestrator/queue'; vi.mock('@utils/analytics', () => ({ analytics: { wizardCapture: vi.fn() }, @@ -12,8 +16,11 @@ import { applyEnqueue, applyReadHandoffs, checkEnqueueGuards, + COMPLETE_SHAPE_KEYS, + NOT_NEEDED_REASON_ASK, type OrchestratorToolsContext, } from '@lib/agent/runner/sequence/orchestrator/queue-tools'; +import { PI_COMPLETE_PARAM_KEYS } from '@lib/agent/runner/harness/pi/orchestrator-tools'; function tmpDir(): string { return fs.mkdtempSync(path.join(os.tmpdir(), 'queue-tools-test-')); @@ -222,3 +229,73 @@ describe('apply functions', () => { expect(handoffs[0].did).toBe('installed'); }); }); + +/** + * `complete_task`'s `notNeededReason`. The handoff carries the agent's prose + * and this carries the machine-readable outcome, because the handoff on this + * step reaches live credentials and never reaches telemetry. + */ +describe('complete_task not-needed reasons', () => { + let dir: string; + let store: QueueStore; + let ctx: OrchestratorToolsContext; + const HANDOFF = { goals: 'g', did: 'd', forNextAgent: 'n' }; + + beforeEach(() => { + dir = tmpDir(); + store = new QueueStore(dir, 'run-1'); + ctx = { store, validTypes: VALID }; + }); + + afterEach(() => fs.rmSync(dir, { recursive: true, force: true })); + + function skipWith(notNeededReason?: unknown) { + const t = store.enqueue({ type: 'install' }); + store.start(t.id); + ctx.currentTaskId = t.id; + applyComplete(ctx, { + status: 'not needed', + handoff: HANDOFF, + notNeededReason, + } as never); + return store.get(t.id); + } + + it.each([ + NotNeededReason.NotApplicable, + NotNeededReason.UserDeclined, + NotNeededReason.Blocked, + ])('forwards %s onto the task', (reason) => { + const t = skipWith(reason); + expect(t?.skipReason).toBe(SkipReason.AgentNotNeeded); + expect(t?.notNeededReason).toBe(reason); + }); + + // The pi harness hands tool arguments over unvalidated, and an agent asked + // for a reason readily writes a sentence. A sentence about this step can name + // a database or a key, so it must not become an analytics dimension. + it('drops a value that is not one of the declared reasons', () => { + const t = skipWith('the user cancelled the credential prompt'); + expect(t?.skipReason).toBe(SkipReason.AgentNotNeeded); + expect(t?.notNeededReason).toBeUndefined(); + }); + + it('skips as before when the agent declares no reason', () => { + const t = skipWith(undefined); + expect(t?.skipReason).toBe(SkipReason.AgentNotNeeded); + expect(t?.notNeededReason).toBeUndefined(); + }); + + it('asks for every reason the type declares', () => { + for (const reason of Object.values(NotNeededReason)) { + expect(NOT_NEEDED_REASON_ASK).toContain(reason); + } + }); + + it('offers the field on both harnesses, and pi is the one that runs', () => { + expect(PI_COMPLETE_PARAM_KEYS).toContain('notNeededReason'); + expect(PI_COMPLETE_PARAM_KEYS.slice().sort()).toEqual( + COMPLETE_SHAPE_KEYS.slice().sort(), + ); + }); +}); diff --git a/src/lib/agent/runner/sequence/orchestrator/__tests__/queue.test.ts b/src/lib/agent/runner/sequence/orchestrator/__tests__/queue.test.ts index 9e2e64b3c..cb306ef5b 100644 --- a/src/lib/agent/runner/sequence/orchestrator/__tests__/queue.test.ts +++ b/src/lib/agent/runner/sequence/orchestrator/__tests__/queue.test.ts @@ -2,6 +2,8 @@ import * as fs from 'fs'; import * as os from 'os'; import * as path from 'path'; import { + isNotNeededReason, + NotNeededReason, QueueStore, QUEUE_DIR_NAME, SkipReason, @@ -472,3 +474,82 @@ describe('skip reasons', () => { ); }); }); + +/** + * The second dimension of an agent-reported skip. + * + * `agent-not-needed` names the agent as the decider, not what it decided, and + * `complete_task` offers `not needed` both for "the step does not apply" and + * for "you cannot do it". On the one step that stops to ask for credentials + * those are opposite outcomes, so the agent's own answer is recorded next to + * the reason rather than folded into it. + */ +describe('not-needed reasons', () => { + let dir: string; + let q: QueueStore; + + beforeEach(() => { + dir = tmpDir(); + q = new QueueStore(dir, 'run-1'); + }); + + afterEach(() => fs.rmSync(dir, { recursive: true, force: true })); + + it.each([ + NotNeededReason.NotApplicable, + NotNeededReason.UserDeclined, + NotNeededReason.Blocked, + ])('records %s alongside the skip reason', (reason) => { + const t = q.enqueue({ type: 'warehouse' }); + q.start(t.id); + q.skip(t.id, SkipReason.AgentNotNeeded, undefined, reason); + + expect(q.get(t.id)?.skipReason).toBe(SkipReason.AgentNotNeeded); + expect(q.get(t.id)?.notNeededReason).toBe(reason); + }); + + it('stays unset on a skip that declares none', () => { + const t = q.enqueue({ type: 'warehouse' }); + q.start(t.id); + q.skip(t.id, SkipReason.UserDeclined); + + expect(q.get(t.id)?.skipReason).toBe(SkipReason.UserDeclined); + expect(q.get(t.id)?.notNeededReason).toBeUndefined(); + }); + + it('hands it to the transition listener, so the skip event carries it', () => { + const seen: { reason?: string; notNeeded?: string }[] = []; + const listened = new QueueStore(dir, 'run-1', { + onTransition: (event: TransitionEvent, task: QueuedTask) => { + if (event === 'skip') { + seen.push({ + reason: task.skipReason, + notNeeded: task.notNeededReason, + }); + } + }, + }); + + const t = listened.enqueue({ type: 'warehouse' }); + listened.start(t.id); + listened.skip( + t.id, + SkipReason.AgentNotNeeded, + undefined, + NotNeededReason.UserDeclined, + ); + + expect(seen.at(-1)).toEqual({ + reason: 'agent-not-needed', + notNeeded: 'user-declined', + }); + }); + + it('only names outcomes the type declares', () => { + expect(isNotNeededReason('user-declined')).toBe(true); + expect(isNotNeededReason('the user cancelled the postgres prompt')).toBe( + false, + ); + expect(isNotNeededReason(undefined)).toBe(false); + }); +}); diff --git a/src/lib/agent/runner/sequence/orchestrator/orchestrator-runner.ts b/src/lib/agent/runner/sequence/orchestrator/orchestrator-runner.ts index 6ada13690..d6a75a592 100644 --- a/src/lib/agent/runner/sequence/orchestrator/orchestrator-runner.ts +++ b/src/lib/agent/runner/sequence/orchestrator/orchestrator-runner.ts @@ -476,6 +476,11 @@ export async function runOrchestrator( // did not apply were one number — which is how a regression that // halved the warehouse completion rate stayed invisible for a week. reason: task.skipReason, + // Also additive, and only ever set alongside `agent-not-needed`: + // that reason names the agent as the decider without saying what it + // decided, so a step the user withheld a credential for counted as + // a step that did not apply to the project. + not_needed_reason: task.notNeededReason, }); break; case 'fail': diff --git a/src/lib/agent/runner/sequence/orchestrator/queue-tools.ts b/src/lib/agent/runner/sequence/orchestrator/queue-tools.ts index 76fdc6a4e..e1560b621 100644 --- a/src/lib/agent/runner/sequence/orchestrator/queue-tools.ts +++ b/src/lib/agent/runner/sequence/orchestrator/queue-tools.ts @@ -13,6 +13,8 @@ import { VALID_MODELS, } from '@lib/agent/runner/switchboard/models'; import { + isNotNeededReason, + NotNeededReason, SkipReason, TaskStatus, type QueueStore, @@ -20,6 +22,18 @@ import { type TaskHandoff, } from './queue'; +/** + * The `complete_task` `notNeededReason` description, shared by both harnesses' + * schemas so the ask cannot drift between them. + * + * Named per value rather than left to prose: the distinction the flow needs is + * "the user did not hand over a credential" against "there was nothing here to + * connect", and an agent asked for that in free text writes it into the handoff + * — where {@link applyComplete} deliberately cannot forward it, because handoff + * prose on this step reaches live database passwords. + */ +export const NOT_NEEDED_REASON_ASK = `Required when status is 'not needed': which of these ended the task. '${NotNeededReason.NotApplicable}' — the step genuinely does not apply to this project. '${NotNeededReason.UserDeclined}' — you asked the user and they declined, cancelled, or never answered. '${NotNeededReason.Blocked}' — something outside your and the user's control stopped you (a credential the project does not have, a plan or permission it lacks, an endpoint you could not reach). Ignored for any other status.`; + /** The per-task remark ask, shared by both harnesses' complete_task schemas. */ export const REMARK_ASK = 'What information or guidance would have been useful to have in the integration prompt or documentation for this task — specifically anything that would have prevented tool failures, erroneous edits, or other wasted turns.'; @@ -274,13 +288,16 @@ export function applyEnqueue( export type CompleteResult = { ok: true } | { ok: false; message: string }; +export type CompleteArgs = { + status: 'done' | 'failed' | 'not needed'; + handoff: TaskHandoff; + remark?: string; + notNeededReason?: NotNeededReason; +}; + export function applyComplete( ctx: OrchestratorToolsContext, - args: { - status: 'done' | 'failed' | 'not needed'; - handoff: TaskHandoff; - remark?: string; - }, + args: CompleteArgs, ): CompleteResult { const id = ctx.currentTaskId; if (!id) { @@ -304,9 +321,17 @@ export function applyComplete( } else if (args.status === TaskStatus.Skipped) { // The agent's own words stay in the handoff and out of telemetry. This flow // reaches live database and API credentials, and the repo has no redaction - // pass for handoff prose, so the event carries the reason and the task type - // only — enough to separate an agent no-op from a user decline. - ctx.store.skip(id, SkipReason.AgentNotNeeded, args.handoff); + // pass for handoff prose, so the event carries the task type, the reason, + // and the closed set of `notNeededReason` values — enough to separate an + // agent no-op from a user decline from a blocked step, with no free text. + ctx.store.skip( + id, + SkipReason.AgentNotNeeded, + args.handoff, + isNotNeededReason(args.notNeededReason) + ? args.notNeededReason + : undefined, + ); } else { ctx.store.complete(id, args.handoff); } @@ -394,6 +419,29 @@ const HANDOFF_SHAPE = { /** Exported so the parity test can compare both harnesses' field sets. */ export const HANDOFF_SHAPE_KEYS: readonly string[] = Object.keys(HANDOFF_SHAPE); +/** + * `complete_task`'s own arguments, held level with the pi mirror by the same + * parity test that guards the handoff — a top-level field can go missing on the + * harness that runs just as easily as a nested one. + */ +const COMPLETE_SHAPE = { + status: z.enum(['done', 'failed', 'not needed']), + handoff: z.object(HANDOFF_SHAPE), + remark: z.string().optional().describe(REMARK_ASK), + notNeededReason: z + .enum([ + NotNeededReason.NotApplicable, + NotNeededReason.UserDeclined, + NotNeededReason.Blocked, + ]) + .optional() + .describe(NOT_NEEDED_REASON_ASK), +}; + +/** Exported so the parity test can compare both harnesses' field sets. */ +export const COMPLETE_SHAPE_KEYS: readonly string[] = + Object.keys(COMPLETE_SHAPE); + type SdkTool = ( name: string, description: string, @@ -451,16 +499,8 @@ export function buildOrchestratorTools( const completeTask = tool( 'complete_task', "Report the outcome of your task. Always call this exactly once when you finish, with a structured handoff for the next agent. Use status 'not needed' when the task does not apply to this project and you cannot do it (say why in the handoff) — not 'done'.", - { - status: z.enum(['done', 'failed', 'not needed']), - handoff: z.object(HANDOFF_SHAPE), - remark: z.string().optional().describe(REMARK_ASK), - }, - ((args: { - status: 'done' | 'failed' | 'not needed'; - handoff: TaskHandoff; - remark?: string; - }) => { + COMPLETE_SHAPE, + ((args: CompleteArgs) => { const res = applyComplete(ctx, args); if (!res.ok) return textResult(res.message, true); return textResult('ok'); diff --git a/src/lib/agent/runner/sequence/orchestrator/queue.ts b/src/lib/agent/runner/sequence/orchestrator/queue.ts index 841c666ef..5896ef07e 100644 --- a/src/lib/agent/runner/sequence/orchestrator/queue.ts +++ b/src/lib/agent/runner/sequence/orchestrator/queue.ts @@ -46,12 +46,47 @@ export const SkipReason = { NoticeTimeout: 'notice-timeout', /** The notice could not be shown at all, so consent failed closed. */ NoticeError: 'notice-error', - /** The task agent reported `not needed`: the step did not apply here. */ + /** The task agent reported `not needed`. See {@link NotNeededReason} for why. */ AgentNotNeeded: 'agent-not-needed', } as const; export type SkipReason = (typeof SkipReason)[keyof typeof SkipReason]; +/** + * Why the agent itself reported `not needed`. + * + * {@link SkipReason.AgentNotNeeded} records who decided the skip, not what they + * decided. `complete_task` offers `not needed` both for "the step does not + * apply" and for "you cannot do it", so an agent that never got a credential + * out of the user lands in the same bucket as one that found nothing to + * connect — and the bucket is named after the first meaning. For the one step + * that stops to ask for credentials, that is the difference that matters: a + * step nobody supplied a password for is not a step that did not apply. + * + * A sub-dimension of the skip reason, never a replacement for it, so every + * existing reason value keeps counting exactly what it counted before. + */ +export const NotNeededReason = { + /** Nothing to do: the step genuinely does not apply to this project. */ + NotApplicable: 'not-applicable', + /** The user was asked and declined, or left the prompts unanswered. */ + UserDeclined: 'user-declined', + /** Something outside the run stopped it — a credential, plan, or endpoint. */ + Blocked: 'blocked', +} as const; + +export type NotNeededReason = + (typeof NotNeededReason)[keyof typeof NotNeededReason]; + +/** + * Whether a value names a reason. The pi harness hands tool arguments over + * unvalidated, so an invented value would otherwise reach the analytics + * dimension as free text — the one thing this step must never emit. + */ +export function isNotNeededReason(value: unknown): value is NotNeededReason { + return (Object.values(NotNeededReason) as unknown[]).includes(value); +} + export interface QueuedTask { id: string; type: string; @@ -87,6 +122,8 @@ export interface QueuedTask { error?: { type: string; message: string }; /** Set when `status` is `not needed`: why. The failed-task `error` of a skip. */ skipReason?: SkipReason; + /** Set only for an agent-reported skip, and only when the agent declared it. */ + notNeededReason?: NotNeededReason; } export interface QueueFile { @@ -327,10 +364,19 @@ export class QueueStore { * The reason is required, and sits before the optional handoff for that * reason. A skip carrying no reason is what let a five-minute auto-decline * hide inside the same event as an agent deciding a step did not apply. + * + * `notNeededReason` splits the agent-reported reason one level further; only + * an agent supplies it, and only when it declared one. */ - skip(id: string, reason: SkipReason, handoff?: TaskHandoff): QueuedTask { + skip( + id: string, + reason: SkipReason, + handoff?: TaskHandoff, + notNeededReason?: NotNeededReason, + ): QueuedTask { const t = this.require(id); t.skipReason = reason; + if (notNeededReason) t.notNeededReason = notNeededReason; return this.finish(id, TaskStatus.Skipped, handoff); }