diff --git a/src/lib/agent/runner/harness/pi/orchestrator-tools.ts b/src/lib/agent/runner/harness/pi/orchestrator-tools.ts index 0b200fb8..42aa778c 100644 --- a/src/lib/agent/runner/harness/pi/orchestrator-tools.ts +++ b/src/lib/agent/runner/harness/pi/orchestrator-tools.ts @@ -20,11 +20,13 @@ import { applyReadHandoffs, ENQUEUE_MODEL_DESCRIPTION, 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 }]; @@ -60,6 +62,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, @@ -114,24 +142,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 a33b32f6..3ee223d9 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 @@ -3,7 +3,11 @@ import * as os from 'os'; import * as path from 'path'; import { z } from 'zod'; 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() }, @@ -14,9 +18,12 @@ import { applyReadHandoffs, buildOrchestratorTools, checkEnqueueGuards, + COMPLETE_SHAPE_KEYS, ENQUEUE_MODEL_DESCRIPTION, + 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'; import { isValidModel, VALID_MODELS, @@ -230,6 +237,76 @@ describe('apply functions', () => { }); }); +/** + * `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(), + ); + }); +}); + /** * The `invalid-model` guard rejects any model outside the allow-list, so an * agent that cannot see the list has to trip the guard to learn it. The 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 9e2e64b3..cb306ef5 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 6ada1369..d6a75a59 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 24b3d999..5f1fa344 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 `enqueue_task` `model` description, shared by both harnesses' schemas. * @@ -288,13 +302,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) { @@ -318,9 +335,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); } @@ -408,6 +433,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, @@ -465,16 +513,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 841c666e..5896ef07 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); }