diff --git a/apps/aflow-executor-ai/src/handlers/ai/handlers/decide.test.ts b/apps/aflow-executor-ai/src/handlers/ai/handlers/decide.test.ts new file mode 100644 index 00000000..573cdc50 --- /dev/null +++ b/apps/aflow-executor-ai/src/handlers/ai/handlers/decide.test.ts @@ -0,0 +1,129 @@ +/** + * ai.decision.decide as a step: the questions a request asks reach the model + * as one pass, an item the model declines is never selected, and an `each` + * over an empty list is answered without a model or the credential for one. + */ +import { describe, it, expect, vi, beforeEach } from 'vitest'; +import type { ExecutorContext } from '@aflow/executor-runtime'; +import { + AiDecideOutputSchema, + DECISION_NO_MODEL, + type AiDecideInput, + type TenantId, +} from '@aflow/schemas'; +import { getAIClientForContext } from '../aiClient.js'; +import { handleDecide } from './decide.js'; +import { resolveDecisionModel } from './decisionModel.js'; +import type { HandlerDeps } from './types.js'; + +vi.mock('../aiClient.js', () => ({ getAIClientForContext: vi.fn() })); +vi.mock('./decisionModel.js', () => ({ resolveDecisionModel: vi.fn() })); +vi.mock('@aflow/executor-runtime', async (importOriginal) => ({ + ...(await importOriginal()), + successWithData: vi.fn((_ctx: unknown, data: unknown) => + Promise.resolve({ status: 'SUCCEEDED', data }), + ), +})); + +const decide = vi.fn(); +const getModel = vi.fn(() => ({ decision: { confidence: 'calibrated' } })); + +function fakeCtx(): ExecutorContext { + return { + job: { tenantId: 'tenant-d', stepExecutionId: 'step-d', attempt: 1 }, + tenantId: 'tenant-d' as TenantId, + runId: 'run-d', + log: { warn: () => {}, error: () => {}, info: () => {}, debug: () => {} }, + } as unknown as ExecutorContext; +} + +const deps = { + payloadStore: {} as HandlerDeps['payloadStore'], + handleError: (_ctx: ExecutorContext, label: string, error: unknown) => { + throw new Error(`${label}: ${String(error)}`); + }, + validateToolArgs: () => null, +} satisfies HandlerDeps; + +const FINDING = { + type: 'yes_no' as const, + instructions: 'Names a defect, established by evidence shown', + minConfidence: 0.8, +}; + +async function run(input: AiDecideInput) { + const result = (await handleDecide(fakeCtx(), input, deps)) as unknown as { data: unknown }; + return AiDecideOutputSchema.parse(result.data); +} + +describe('ai.decision.decide — a question of each item', () => { + beforeEach(() => { + vi.clearAllMocks(); + vi.mocked(getAIClientForContext).mockResolvedValue({ decide, getModel } as never); + vi.mocked(resolveDecisionModel).mockResolvedValue('jev'); + }); + + it('asks every item in the one call, and hands back only the items judged true and decided', async () => { + decide.mockResolvedValue({ + model: 'jev-1.13.0', + answers: { + each_0: { type: 'yes_no', probability: 0.97 }, + each_1: { type: 'yes_no', probability: 0.6 }, + }, + usage: { promptTokens: 10, completionTokens: 0, totalTokens: 10 }, + }); + const output = await run({ + state: { verdict: 'request_changes' }, + each: { items: [{ claim: 'leaks' }, { claim: 'maybe' }], question: FINDING }, + }); + expect(decide).toHaveBeenCalledOnce(); + expect(Object.keys(decide.mock.calls[0]![0].questions as object)).toEqual(['each_0', 'each_1']); + expect(output.answers).toEqual({}); + expect(output.each?.selected).toEqual([{ claim: 'leaks' }]); + expect(output.each?.undecidedCount).toBe(1); + }); + + it('never selects an item the model declined, and counts it undecided, beside a named question', async () => { + decide.mockResolvedValue({ + model: 'luna-decisions', + answers: { + route: { type: 'choice', choice: 'fix', confidence: 0.9, probabilities: { fix: 0.9 } }, + each_0: { type: 'refusal' }, + each_1: { type: 'yes_no', probability: 0.95 }, + }, + usage: { promptTokens: 10, completionTokens: 0, totalTokens: 10 }, + }); + const output = await run({ + state: { verdict: 'request_changes' }, + questions: { + route: { + type: 'choice', + options: { fix: 'Fix it.', ship: 'Ship it.' }, + minConfidence: 0.8, + }, + }, + each: { items: [{ claim: 'declined' }, { claim: 'leaks' }], question: FINDING }, + }); + expect(output.answers['route']).toMatchObject({ decidedValue: 'fix', decided: true }); + expect(output.each?.answers[0]).toEqual({ + type: 'refusal', + decidedValue: null, + decided: false, + }); + expect(output.each?.selected).toEqual([{ claim: 'leaks' }]); + expect(output.each?.undecidedCount).toBe(1); + expect(output.calibration).toBe('calibrated'); + }); + + it('asks no model, and needs no credential, for an empty list — and selects nothing', async () => { + const output = await run({ state: {}, each: { items: [], question: FINDING } }); + expect(resolveDecisionModel).not.toHaveBeenCalled(); + expect(getAIClientForContext).not.toHaveBeenCalled(); + expect(decide).not.toHaveBeenCalled(); + expect(output.each).toMatchObject({ selected: [], selectedCount: 0, selectedJson: '[]' }); + expect(output.usage.totalTokens).toBe(0); + // Nothing was asked, so no model answered and no confidence is claimed calibrated. + expect(output.model).toBe(DECISION_NO_MODEL); + expect(output.calibration).toBe('self_reported'); + }); +}); diff --git a/apps/aflow-executor-ai/src/handlers/ai/handlers/decide.ts b/apps/aflow-executor-ai/src/handlers/ai/handlers/decide.ts index 01fb4b44..22278033 100644 --- a/apps/aflow-executor-ai/src/handlers/ai/handlers/decide.ts +++ b/apps/aflow-executor-ai/src/handlers/ai/handlers/decide.ts @@ -3,8 +3,18 @@ */ import type { ExecutorContext, StepResult } from '@aflow/executor-runtime'; import { failureWithError, successWithData } from '@aflow/executor-runtime'; -import type { AiDecideInput, AiDecideOutput, TenantId, StepExecutionId } from '@aflow/schemas'; -import { resolveDecisionAnswers } from '@aflow/ai-client'; +import { + type AiDecideInput, + type AiDecideOutput, + DECISION_NO_MODEL, + type TenantId, + type StepExecutionId, +} from '@aflow/schemas'; +import { + decisionRequestQuestions, + resolveDecisionAnswers, + splitDecisionAnswers, +} from '@aflow/ai-client'; import { getAIClientForContext } from '../aiClient.js'; import { buildUsageBreakdown } from '../helpers.js'; import { resolveDecisionModel } from './decisionModel.js'; @@ -18,6 +28,22 @@ export async function handleDecide( deps: HandlerDeps, ): Promise { try { + const questions = decisionRequestQuestions(params); + // Only an empty `each` asks nothing; the answer is known without a model, + // and so without the credential resolving one would need. + if (Object.keys(questions).length === 0) { + const { answers, each } = splitDecisionAnswers(params, {}); + const output: AiDecideOutput = { + answers, + ...(each !== undefined ? { each } : {}), + model: params.model ?? DECISION_NO_MODEL, + calibration: 'self_reported', + usage: { promptTokens: 0, completionTokens: 0, totalTokens: 0 }, + latencyMs: 0, + }; + return await successWithData(ctx, output); + } + const imageRefs = params.images ?? []; const model = await resolveDecisionModel(ctx, deps, { model: params.model, @@ -45,7 +71,7 @@ export async function handleDecide( const response = await client.decide({ model, state: params.state, - questions: params.questions, + questions, ...(images.length > 0 ? { images } : {}), tenantId: ctx.job.tenantId as TenantId, runId: ctx.runId, @@ -53,8 +79,13 @@ export async function handleDecide( attempt: ctx.job.attempt, }); + const { answers, each } = splitDecisionAnswers( + params, + resolveDecisionAnswers(questions, response.answers), + ); const output: AiDecideOutput = { - answers: resolveDecisionAnswers(params.questions, response.answers), + answers, + ...(each !== undefined ? { each } : {}), model: response.model, calibration: client.getModel(model)?.decision?.confidence ?? 'self_reported', usage: response.usage, diff --git a/apps/aflow-executor-host/src/__tests__/commitCheck.test.ts b/apps/aflow-executor-host/src/__tests__/commitCheck.test.ts index a078ac0f..0b897386 100644 --- a/apps/aflow-executor-host/src/__tests__/commitCheck.test.ts +++ b/apps/aflow-executor-host/src/__tests__/commitCheck.test.ts @@ -589,7 +589,7 @@ describe('host.commit.check — a run lost only to tests running out of time', ( ); } - it('names the tests and the commit a publication runs the checks on again, and clears nothing', async () => { + it('names the tests a publication runs the checks again for, and clears nothing', async () => { const world = await fixture({ branchPrefix: 'aflow/', checks: reportingTimeouts("console.log('FAIL tests'); process.exit(1);"), @@ -598,7 +598,6 @@ describe('host.commit.check — a run lost only to tests running out of time', ( expect(output.passed).toBe(false); expect(output.clearedSha).toBeUndefined(); expect(output.testTimeouts).toEqual(TIMED_OUT); - expect(output.clearedOrRetriableSha).toBe(world.sha); expect(output.summary).toContain('nothing failed but 2 tests running out of time'); expect(output.summary).toContain(TIMED_OUT.join('; ')); expect(output.summary).toContain('a publication runs the checks once more'); @@ -612,7 +611,6 @@ describe('host.commit.check — a run lost only to tests running out of time', ( }); const passed = HostCommitCheckOutputSchema.parse((await check(passing)).captured.output); expect(passed.testTimeouts).toBeUndefined(); - expect(passed.clearedOrRetriableSha).toBe(passing.sha); expect(passed.clearedSha).toBe(passing.sha); const stopped = await fixture({ @@ -623,14 +621,47 @@ describe('host.commit.check — a run lost only to tests running out of time', ( const timedOut = HostCommitCheckOutputSchema.parse((await check(stopped)).captured.output); expect(timedOut).toMatchObject({ passed: false, timedOut: true }); expect(timedOut.testTimeouts).toBeUndefined(); - expect(timedOut.clearedOrRetriableSha).toBeUndefined(); }); it('is not claimed where the checks reported none', async () => { const world = await fixture({ branchPrefix: 'aflow/', checks: FAILING }); const output = HostCommitCheckOutputSchema.parse((await check(world)).captured.output); expect(output.testTimeouts).toBeUndefined(); - expect(output.clearedOrRetriableSha).toBeUndefined(); + }); +}); + +describe('host.commit.check — the step the checks name as failing', () => { + /** A check whose report names `guard` as the failing step, and ends as `exit` says. */ + function reportingGuard(guard: string, exit: string): string[] { + return nodeCheck(`${reportScript(JSON.stringify({ failingGuard: guard }))}\n${exit}`); + } + + it('names the failing step on checks that failed, from a report that carries no listener count', async () => { + const world = await fixture({ + branchPrefix: 'aflow/', + checks: reportingGuard('tsc apps/x/tsconfig.json', 'process.exit(2);'), + }); + const output = HostCommitCheckOutputSchema.parse((await check(world)).captured.output); + expect(output.passed).toBe(false); + expect(output.failingGuard).toBe('tsc apps/x/tsconfig.json'); + // The summary is what a fix is briefed with, so it names the step too. + expect(output.summary).toContain('the check failed at `tsc apps/x/tsconfig.json`.'); + // Unknown, not zero: the run failed before the tests that would count them. + expect(output.skippedListenerTests).toBeUndefined(); + expect(output.receipt).toBeDefined(); + }); + + it('names none on checks that passed, whatever the report says, nor where the checks named none', async () => { + const passing = await fixture({ + branchPrefix: 'aflow/', + checks: reportingGuard('tsc apps/x/tsconfig.json', 'process.exit(0);'), + }); + const passed = HostCommitCheckOutputSchema.parse((await check(passing)).captured.output); + expect(passed.failingGuard).toBeUndefined(); + + const silent = await fixture({ branchPrefix: 'aflow/', checks: FAILING }); + const failed = HostCommitCheckOutputSchema.parse((await check(silent)).captured.output); + expect(failed.failingGuard).toBeUndefined(); }); }); diff --git a/apps/aflow-executor-host/src/__tests__/pullRequestText.test.ts b/apps/aflow-executor-host/src/__tests__/pullRequestText.test.ts index 48464ebd..9d83b24f 100644 --- a/apps/aflow-executor-host/src/__tests__/pullRequestText.test.ts +++ b/apps/aflow-executor-host/src/__tests__/pullRequestText.test.ts @@ -20,6 +20,7 @@ import { type HostCommitDescribeInput, HostCommitDescribeInputSchema, HostCommitDescribeOutputSchema, + HANDED_ON_FINDINGS_MAX_COUNT, PULL_REQUEST_BODY_MAX_CHARS, } from '@aflow/schemas'; @@ -217,6 +218,50 @@ describe('host.commit.describe', () => { ); }); + it("hands back the findings and the decision it recorded, so the run carries the pull request's record", async () => { + const head = await commit('The slice', 'What the slice does.', 'a.txt'); + const finding = { + severity: 'minor', + file: 'a.txt', + line: 1, + claim: 'Says what it was.', + evidence: 'The first line.', + } as const; + const { captured } = await describeRange({ + range: `${start}..${head}`, + review: { + verdict: 'comment', + outcome: 'completed', + reviewSummary: 'Read.', + findings: [finding], + }, + afterReview: 'publish_as_is', + }); + const output = HostCommitDescribeOutputSchema.parse(captured.output); + expect(output.findings).toEqual([ + { severity: 'minor', file: 'a.txt', line: 1, claim: 'Says what it was.', state: 'open' }, + ]); + expect(output.afterReview).toBe('publish_as_is'); + expect(output.findingsLeftOut).toBeUndefined(); + }); + + it('says how many findings the bounded hand-on left out, where it left any', async () => { + const head = await commit('The slice', 'What the slice does.', 'a.txt'); + const findings = Array.from({ length: HANDED_ON_FINDINGS_MAX_COUNT + 2 }, (_, index) => ({ + severity: 'nit' as const, + file: `f${String(index)}.txt`, + claim: 'Says what it was.', + evidence: 'Its name.', + })); + const { captured } = await describeRange({ + range: `${start}..${head}`, + review: { verdict: 'comment', outcome: 'completed', reviewSummary: 'Read.', findings }, + }); + const output = HostCommitDescribeOutputSchema.parse(captured.output); + expect(output.findings).toHaveLength(HANDED_ON_FINDINGS_MAX_COUNT); + expect(output.findingsLeftOut).toBe(2); + }); + it('describes only the range it is given, from its base', async () => { const first = await commit('Earlier work', 'Already on the base.', 'a.txt'); const head = await commit('The slice', 'What the slice does.', 'b.txt'); @@ -263,7 +308,131 @@ describe('composePullRequest', () => { { bindingId: 'hb_app', range, title: 'Their title', summary: 'Their words.' }, commits, ), - ).toEqual({ title: 'Their title', body: 'Their words.' }); + ).toEqual({ title: 'Their title', body: 'Their words.', findings: [] }); + }); + + /** A finding as Local Code Review returns it, its evidence and suggestion beside what the record reads. */ + const REVIEW_FINDING = { + severity: 'major', + file: 'src/cache.ts', + line: 40, + claim: 'The cache is never invalidated after a write.', + evidence: 'put() writes the store and leaves the map entry.', + suggestion: 'Delete the entry in put().', + } as const; + const reviewed = (afterReview?: 'fix_before_merge' | 'publish_as_is' | null) => + HostCommitDescribeInputSchema.parse({ + bindingId: 'hb_app', + range, + findings: FINDINGS, + review: { + verdict: 'request_changes', + outcome: 'completed', + reviewSummary: REVIEW_SUMMARY, + findings: [REVIEW_FINDING], + }, + ...(afterReview !== undefined ? { afterReview } : {}), + }); + + it("hands on the latest review's findings as the open ones, replacing the earlier rounds', with only what the record reads", () => { + const composed = composePullRequest(reviewed('fix_before_merge'), commits); + expect(composed.findings).toEqual([ + { + severity: 'major', + file: 'src/cache.ts', + line: 40, + claim: 'The cache is never invalidated after a write.', + state: 'open', + }, + ]); + // Round after round, what goes on is one review's findings, never every round's. + const next = composePullRequest( + HostCommitDescribeInputSchema.parse({ + bindingId: 'hb_app', + range, + findings: composed.findings, + review: { + verdict: 'request_changes', + outcome: 'completed', + reviewSummary: REVIEW_SUMMARY, + findings: [REVIEW_FINDING], + }, + }), + commits, + ); + expect(next.findings).toHaveLength(1); + }); + + it("says each finding once: the earlier rounds' listed, this review's in its own summary", () => { + const summary = 'One major: src/cache.ts:40 — the cache is never invalidated after a write.'; + const { body } = composePullRequest( + HostCommitDescribeInputSchema.parse({ + bindingId: 'hb_app', + range, + findings: FINDINGS, + review: { + verdict: 'request_changes', + outcome: 'completed', + reviewSummary: summary, + findings: [REVIEW_FINDING], + }, + }), + commits, + ); + for (const finding of FINDINGS) expect(body).toContain(finding.claim); + expect(body).toContain(summary); + expect(body).not.toContain('- **major** `src/cache.ts:40`'); + expect(body).not.toContain(REVIEW_FINDING.evidence); + }); + + it('passes on the open findings it was given where no review of its own finished', () => { + for (const review of [ + undefined, + { verdict: null, outcome: 'failed', reviewSummary: null, findings: null }, + ]) { + const composed = composePullRequest( + HostCommitDescribeInputSchema.parse({ + bindingId: 'hb_app', + range, + findings: FINDINGS, + ...(review !== undefined ? { review } : {}), + }), + commits, + ); + expect(composed.findings).toEqual(FINDINGS.filter((f) => f.state === 'open')); + } + }); + + it('records the decision on those findings for each answer, and undecided where none cleared the bar or none was made', () => { + for (const [answer, recorded] of [ + ['fix_before_merge', 'fix_before_merge'], + ['publish_as_is', 'publish_as_is'], + [null, 'undecided'], + [undefined, 'undecided'], + ] as const) { + const composed = composePullRequest(reviewed(answer), commits); + expect(composed.afterReview, String(answer)).toBe(recorded); + expect(composed.body, String(answer)).toContain(`Decided after the review: \`${recorded}\``); + } + }); + + it('records no decision where the review returned no findings, since none was asked', () => { + for (const review of [ + { verdict: 'approve', outcome: 'completed', reviewSummary: 'Read.', findings: [] }, + { verdict: null, outcome: 'failed', reviewSummary: null, findings: null }, + undefined, + ]) { + const composed = composePullRequest( + HostCommitDescribeInputSchema.parse({ + bindingId: 'hb_app', + range, + ...(review !== undefined ? { review } : {}), + }), + commits, + ); + expect(composed.afterReview).toBeUndefined(); + expect(composed.body).not.toContain('Decided after the review'); + } }); it('says where nothing was reviewed, no checks ran, or no gaps were recorded', () => { diff --git a/apps/aflow-executor-host/src/commitCheck.ts b/apps/aflow-executor-host/src/commitCheck.ts index ec6e62c0..2464f261 100644 --- a/apps/aflow-executor-host/src/commitCheck.ts +++ b/apps/aflow-executor-host/src/commitCheck.ts @@ -17,6 +17,7 @@ import { HOST_CHECK_OUTPUT_HEAD_BYTES, HOST_CHECK_OUTPUT_TAIL_BYTES, HOST_CHECK_TAIL_BYTES, + HOST_CHECK_FAILING_GUARD_MAX_CHARS, HOST_CHECK_TEST_TIMEOUTS_MAX, type HostCommitCheckOutputSchema, type HostSandboxPosture, @@ -162,8 +163,9 @@ export const CHECK_REPORT_ENV = 'AFLOW_CHECK_REPORT'; const CHECK_REPORT_FILE = 'check-report.json'; const CheckReportSchema = z.object({ - skippedListenerTests: z.number().int().nonnegative(), + skippedListenerTests: z.number().int().nonnegative().optional(), testTimeouts: z.array(z.string()).optional(), + failingGuard: z.string().min(1).max(HOST_CHECK_FAILING_GUARD_MAX_CHARS).optional(), }); /** What the check wrote to its report, or nothing where it wrote none a reader can take. */ @@ -194,6 +196,8 @@ export interface FolderCheckRun { readonly skippedListenerTests?: number; /** The tests that ran out of their time, where its report says they alone failed it. */ readonly testTimeouts?: readonly string[]; + /** The step the check names as the one that failed, where its report says. */ + readonly failingGuard?: string; } export interface FolderCheckInput { @@ -270,7 +274,10 @@ export async function runFolderChecks(input: FolderCheckInput): Promise 0 ? { testTimeouts: report.testTimeouts } : {}), @@ -298,7 +305,6 @@ export function skippedCheck(bindingId: string, sha: string): HostCommitCheckOut 'was not checked on this machine. The operator declares them on the machine holding ' + `the folder: \`aflow harness checks ${bindingId} -- [args...]\`.`, clearedSha: sha, - clearedOrRetriableSha: sha, }; } @@ -347,7 +353,8 @@ export function checkOutcome(params: { result.exitCode === null ? `was ended by ${result.signal ?? 'a signal'}` : `exited ${String(result.exitCode)}`; - summary = `${command} ${ending} on ${short} after ${seconds(result.durationMs)}: the check failed.${printed}`; + const at = params.run.failingGuard !== undefined ? ` at \`${params.run.failingGuard}\`` : ''; + summary = `${command} ${ending} on ${short} after ${seconds(result.durationMs)}: the check failed${at}.${printed}`; } return { passed, @@ -365,7 +372,9 @@ export function checkOutcome(params: { ...(testTimeouts.length > 0 ? { testTimeouts: testTimeouts.slice(0, HOST_CHECK_TEST_TIMEOUTS_MAX) } : {}), + ...(!passed && params.run.failingGuard !== undefined + ? { failingGuard: params.run.failingGuard } + : {}), ...(passed ? { clearedSha: params.sha } : {}), - ...(passed || testTimeouts.length > 0 ? { clearedOrRetriableSha: params.sha } : {}), }; } diff --git a/apps/aflow-executor-host/src/handlers/commitHandlers.ts b/apps/aflow-executor-host/src/handlers/commitHandlers.ts index b39ac4bc..fc53c92b 100644 --- a/apps/aflow-executor-host/src/handlers/commitHandlers.ts +++ b/apps/aflow-executor-host/src/handlers/commitHandlers.ts @@ -72,7 +72,10 @@ async function describe(ctx: ExecutorContext, policyPath: string): Promise ({ sha, subject })), + findings, + ...(findingsLeftOut !== undefined ? { findingsLeftOut } : {}), + ...(afterReview !== undefined ? { afterReview } : {}), }; return await successWithData(ctx, output); } diff --git a/apps/aflow-executor-host/src/pullRequestText.ts b/apps/aflow-executor-host/src/pullRequestText.ts index b57ffe12..addcbf77 100644 --- a/apps/aflow-executor-host/src/pullRequestText.ts +++ b/apps/aflow-executor-host/src/pullRequestText.ts @@ -9,12 +9,15 @@ * request needs each of it. */ import { + type AfterReview, HOST_COMMIT_RANGE_PATTERN, type HostCommitDescribeCommit, type HostCommitDescribeInput, PULL_REQUEST_BODY_MAX_CHARS, type PullRequestCheckRun, type PullRequestFinding, + type PullRequestReviewRecord, + pullRequestReviewRecord, } from '@aflow/schemas'; import { messageBody } from './branchCommit.js'; @@ -111,7 +114,22 @@ function findingLine(finding: PullRequestFinding): string { return `- **${finding.severity}** \`${where}\` — ${finding.claim}`; } -function reviewSection(input: HostCommitDescribeInput): string { +const AFTER_REVIEW_LINES: Record = { + fix_before_merge: + "Decided after the review: `fix_before_merge` — this review's findings are to be fixed on this branch before it merges.", + publish_as_is: + "Decided after the review: `publish_as_is` — this review's findings can merge as they stand.", + undecided: + "Decided after the review: `undecided` — no answer cleared the decision's bar, so this review's findings are the reader's to weigh.", +}; + +/** + * The earlier rounds' findings are listed, closed and left open, since no + * text of this round names them. This round's review is told by its summary + * alone, which names every finding it read with its severity and `file:line` + * beside the checks it ran — listing those findings again would say each twice. + */ +function reviewSection(input: HostCommitDescribeInput, record: PullRequestReviewRecord): string { const findings = input.findings ?? []; const parts = ['## Review']; for (const [state, heading] of [ @@ -132,6 +150,7 @@ function reviewSection(input: HostCommitDescribeInput): string { parts.push(`The review of this range returned \`${review.verdict}\`.`); if (review.reviewSummary) parts.push(review.reviewSummary); } + if (record.afterReview !== undefined) parts.push(AFTER_REVIEW_LINES[record.afterReview]); return parts.join(SECTION_BREAK); } @@ -182,12 +201,15 @@ function gapsSection(gaps: readonly string[] | undefined): string { export function composePullRequest( input: HostCommitDescribeInput, commits: readonly HostCommitDescribeCommit[], -): { title: string; body: string } { +): { title: string; body: string } & PullRequestReviewRecord { const title = input.title ?? commits[0]?.subject ?? ''; - if (input.summary !== undefined) return { title, body: input.summary }; - const rest = [reviewSection(input), verificationSection(input), gapsSection(input.gaps)].join( - SECTION_BREAK, - ); + const record = pullRequestReviewRecord(input); + if (input.summary !== undefined) return { title, body: input.summary, ...record }; + const rest = [ + reviewSection(input, record), + verificationSection(input), + gapsSection(input.gaps), + ].join(SECTION_BREAK); const changes = fit( changesSection(commits), PULL_REQUEST_BODY_MAX_CHARS - rest.length - SECTION_BREAK.length, @@ -196,5 +218,6 @@ export function composePullRequest( return { title, body: fit(`${changes}${SECTION_BREAK}${rest}`, PULL_REQUEST_BODY_MAX_CHARS, BODY_CUT_NOTE), + ...record, }; } diff --git a/apps/aflow-orchestrator/src/services/SessionOrchestrator/handlers/inlineOps/__tests__/workflowRunReadOps.test.ts b/apps/aflow-orchestrator/src/services/SessionOrchestrator/handlers/inlineOps/__tests__/workflowRunReadOps.test.ts index 149922d9..86ba02cc 100644 --- a/apps/aflow-orchestrator/src/services/SessionOrchestrator/handlers/inlineOps/__tests__/workflowRunReadOps.test.ts +++ b/apps/aflow-orchestrator/src/services/SessionOrchestrator/handlers/inlineOps/__tests__/workflowRunReadOps.test.ts @@ -911,3 +911,33 @@ describe('workflow.run.detail — one task’s output on request (Plan 313 gap 1 expect(output.taskOutput).toBeUndefined(); }); }); + +describe('workflow.run.fail — a skill stops its run on what decided it', () => { + type Failed = { + status: string; + error: { code: string; message: string; classification: string; retryable: boolean }; + }; + + it('always fails its own task, non-retryable, with the reason it was handed as the message', async () => { + const reason = + 'The checks failed at `tsc apps/x/tsconfig.json`. The last of what they printed: …'; + await handleWorkflowCrudInline(makeArgs('workflow.run.fail', { reason })); + + expect(mockAddStepResult).toHaveBeenCalledOnce(); + const result = mockAddStepResult.mock.calls[0]![1] as Failed; + expect(result.status).toBe('FAILED'); + expect(result.error.code).toBe('RUN_FAILED_BY_SKILL'); + // Never the internal-error message: the reason is what the woken reader acts on. + expect(result.error.message).toBe(reason); + expect(result.error.classification).not.toBe('internal'); + expect(result.error.retryable).toBe(false); + }); + + it('still fails, saying so, when it was handed no reason', async () => { + await handleWorkflowCrudInline(makeArgs('workflow.run.fail', {})); + + const result = mockAddStepResult.mock.calls[0]![1] as Failed; + expect(result.status).toBe('FAILED'); + expect(result.error.message).toContain('without a reason'); + }); +}); diff --git a/apps/aflow-orchestrator/src/services/SessionOrchestrator/handlers/inlineOps/workflowCrud/handleInline.ts b/apps/aflow-orchestrator/src/services/SessionOrchestrator/handlers/inlineOps/workflowCrud/handleInline.ts index 854b52ae..186c1e53 100644 --- a/apps/aflow-orchestrator/src/services/SessionOrchestrator/handlers/inlineOps/workflowCrud/handleInline.ts +++ b/apps/aflow-orchestrator/src/services/SessionOrchestrator/handlers/inlineOps/workflowCrud/handleInline.ts @@ -18,6 +18,7 @@ import type { WorkflowCampaignEndInput, WorkflowCampaignRefreshInput, } from '@aflow/schemas'; +import { WorkflowRunFailInputSchema } from '@aflow/schemas'; import { WorkflowArchivedError } from '@aflow/cybernetic-runtime'; import { getOrchestratorLogger } from '../../../../../lib/orchestratorLogger.js'; import type { InlineHandlerArgs } from '../types.js'; @@ -76,6 +77,21 @@ export async function handleWorkflowCrudInline(args: InlineHandlerArgs): Promise case 'workflow.run.cancel': await handleWorkflowRunCancel(args, input as unknown as WorkflowRunCancelInput, startTime); break; + case 'workflow.run.fail': { + // A skill's decision to stop: the reason is what the reader of the + // failure needs, so it is never replaced by the internal-error message. + const parsed = WorkflowRunFailInputSchema.safeParse(input); + await emitStepError( + args, + 'RUN_FAILED_BY_SKILL', + parsed.success + ? parsed.data.reason + : 'The skill stopped the run here without a reason it could state.', + startTime, + 'validation', + ); + break; + } case 'workflow.run.detail': await handleWorkflowRunDetail(args, input as unknown as WorkflowRunDetailInput, startTime); break; diff --git a/apps/aflow-orchestrator/src/services/SessionOrchestrator/helpers/inlineOperations.ts b/apps/aflow-orchestrator/src/services/SessionOrchestrator/helpers/inlineOperations.ts index 00a147fb..80e95814 100644 --- a/apps/aflow-orchestrator/src/services/SessionOrchestrator/helpers/inlineOperations.ts +++ b/apps/aflow-orchestrator/src/services/SessionOrchestrator/helpers/inlineOperations.ts @@ -65,6 +65,8 @@ export const WORKFLOW_TASK_SAFE_INLINE_OPERATIONS: ReadonlySet = new Set 'workflow.learn', // Waits on the run it starts, no deeper than MAX_WORKFLOW_RUN_DEPTH; the waiter answers the task 'workflow.run.start', + // Always fails its own task, with the reason it was handed + 'workflow.run.fail', 'agent.schedule.snooze', ]); diff --git a/apps/aflow-orchestrator/src/services/cybernetic/harness/__tests__/opTaskOutputContract.test.ts b/apps/aflow-orchestrator/src/services/cybernetic/harness/__tests__/opTaskOutputContract.test.ts index f2865fd8..2f90f0d4 100644 --- a/apps/aflow-orchestrator/src/services/cybernetic/harness/__tests__/opTaskOutputContract.test.ts +++ b/apps/aflow-orchestrator/src/services/cybernetic/harness/__tests__/opTaskOutputContract.test.ts @@ -384,6 +384,36 @@ describe('applyOpTaskOutputContract — projection', () => { expect(failed.failureReason).toContain('Nothing has been pushed'); }); + // A task that starts a run and waits on it succeeds however that run ended, + // so a projection that cannot resolve on its output fails the task naming + // how the run ended — the one account the run's own output cannot give. + for (const outcome of ['failed', 'cancelled', 'paused'] as const) { + it(`names how the run a task waited on ended where its projection cannot resolve: ${outcome}`, async () => { + const instruction = 'The run this task started did not finish; its run says why.'; + const { deps } = makeDeps({ + [RAW_REF]: { runId: 'child-run', outcome, waiterId: 'waiter-1' }, + }); + const failed = await run( + deps, + opTaskDef({ + outputProjection: { + result: { path: 'result.output.value', onMissing: 'error' as const }, + outcome: { path: 'outcome', onMissing: 'error' as const }, + }, + failureInstruction: instruction, + }), + ); + expect(failed.kind).toBe('failed'); + if (failed.kind !== 'failed') return; + expect( + failed.failureReason?.startsWith( + `The run it started, child-run, ended ${outcome}. ${instruction} PROJECTION_FAILED: `, + ), + failed.failureReason, + ).toBe(true); + }); + } + it('fails Local Publish at the scan on a finding, naming where and saying nothing left the machine', async () => { const scan = getSkillCatalogEntry('publish-local-changes')?.bundle.workflow.tasks.find( (t) => t.taskId === 'scan-commit', diff --git a/apps/aflow-orchestrator/src/services/cybernetic/harness/opTaskOutputContract.ts b/apps/aflow-orchestrator/src/services/cybernetic/harness/opTaskOutputContract.ts index 3d0369e0..cd878c80 100644 --- a/apps/aflow-orchestrator/src/services/cybernetic/harness/opTaskOutputContract.ts +++ b/apps/aflow-orchestrator/src/services/cybernetic/harness/opTaskOutputContract.ts @@ -5,7 +5,11 @@ import { storeWorkflowResumeContract, type WorkflowTaskRow, } from '@aflow/cybernetic-runtime'; -import { inferTaskType, POLL_RESERVED_OUTPUT_KEY } from '@aflow/schemas'; +import { + inferTaskType, + POLL_RESERVED_OUTPUT_KEY, + WorkflowRunWakeupEnvelopeSchema, +} from '@aflow/schemas'; import type { SessionId, StepExecutionId, TenantId, Workflow, WorkflowTask } from '@aflow/schemas'; import { getOrchestratorLogger } from '../../../lib/orchestratorLogger.js'; import { @@ -42,6 +46,13 @@ function isRecord(value: unknown): value is Record { function describeOpFailure(value: unknown): string | undefined { if (!isRecord(value)) return undefined; + // A task that started a run and waited on it succeeds however that run + // ended; what it ended as is the account a reader of the task's failure needs. + const wakeup = WorkflowRunWakeupEnvelopeSchema.safeParse(value); + if (wakeup.success) { + return `The run it started, ${wakeup.data.runId}, ended ${wakeup.data.outcome}`; + } + const firstText = (candidate: unknown): string | undefined => { if (!isRecord(candidate)) return undefined; for (const key of ['message', 'error', 'summary']) { diff --git a/docs/dev/driving-work-through-the-loop.md b/docs/dev/driving-work-through-the-loop.md index 2cd750fe..db24abf8 100644 --- a/docs/dev/driving-work-through-the-loop.md +++ b/docs/dev/driving-work-through-the-loop.md @@ -88,7 +88,18 @@ meanwhile is refused with its current state rather than overwritten, and a node For a branch that already has a pull request, name its number for the 422. 2. **Read the review**: `workflow.run.detail` on the child review run gives the verdict and every finding with file and line. `approve` means the publication pushed on its own; - anything else leaves it paused at `approve-push`, and the next round is the fix. + anything else leaves it paused at `approve-push`, as before. Whether the findings want a + fix round the publication decides and records, and never acts on (Plan 315 D22): + `decide-after-review` chooses between `fix_before_merge` and `publish_as_is` over every + finding, and the run's `afterReview` and its pull request record the answer — `undecided` + where none cleared the bar. The fix round is the conversation's: on `fix_before_merge`, + once the publication has ended, commission the fix on the branch (`base: `), + briefed from every finding of the review run the publication names as `reviewRunId`, + whole, with its evidence. Then publish the branch again, passing the run's `findings`, + with those the fix closed marked `closed`. That list is for the pull request's record + only: the latest review's open findings, bounded to fit the next publication's inputs, + with `findingsLeftOut` counting what the bound left out. A publication never starts a + commission or another publication. 3. **The publication runs the folder's checks.** `hb_aflow` declares `node scripts/verify-commit.mjs` (`aflow harness checks hb_aflow -- node scripts/verify-commit.mjs`), and Local Publish runs it after the commit and before the @@ -115,13 +126,16 @@ scripts/verify-commit.mjs`), and Local Publish runs it after the commit and befo touched file by path imports nothing from it, so every `.mjs` or `.ts` test of those workspaces whose source names a touched file's repository path, whole (`scripts/verify-commit.mjs`, not `verify-commit`), in a string literal runs too, counted - and named in the same line as the tests imports reach. A failure fails - the publication with the end of what it printed and nothing pushed: read it on the - `check-commit` task, and commission the fix onto the branch. A run whose only failures - were tests running out of their time names them in `testTimeouts`, and the publication - runs the checks once more itself as `recheck-commit`; no retry from the conversation is - needed, and a failure there is read on that task. The pull request's CI - remains the proof. By hand, from a checkout, it measures `HEAD` against `origin/main`. + and named in the same line as the tests imports reach. The script names the step + that failed in its report, and the check's summary carries it. A run lost only to tests + running out of their time is run again by the publication itself as `recheck-commit`, + with no retry from the conversation, and a failure there is read on that task. Any other + failure fails the publication at `checks-failed` with the end of what the checks printed + and nothing pushed, which is the conversation's to take up: read whether the change or + the machine failed, then commission the fix onto the branch or put the machine right. + No decision is made on a failed check until its measurement has labelled cases (Plan 316 + §7). The pull request's CI remains the proof. By + hand, from a checkout, it measures `HEAD` against `origin/main`. 4. **The operator merges.** A merge lands on the dev stack only when the stream that runs it merges `origin/main` into `live` (below). diff --git a/docs/plans/315-local-first-run-ergonomics.md b/docs/plans/315-local-first-run-ergonomics.md index e8227344..b0ab5219 100644 --- a/docs/plans/315-local-first-run-ergonomics.md +++ b/docs/plans/315-local-first-run-ergonomics.md @@ -58,6 +58,20 @@ The loop delivered by the local operating model works. Reaching it from a first **D21 — The appliance survives sleep.** The machine the loop runs on is a laptop, and laptops sleep: one night it went into idle sleep with a commission in flight, briefs were posted in the dark-wake windows macOS opens while it sleeps, and two Helmsman conversations died (F109). Two things failed, and each has its own answer. The first is the machine sleeping under running work, and the answer is **the hold**: while the host executor has a step running it holds a sleep assertion — a child `caffeinate` on macOS, `systemd-inhibit` on Linux — taken when the first step starts and released when the last one settles, never between two that overlap, and ended with the executor. Whether to hold is the operator's, a machine-wide field of the policy beside the harnesses it protects: `on-ac`, the default, holds while the machine is on power, because a laptop held awake on battery is one an operator closed expecting it to stop; `always` holds on battery too; `never` holds nothing. The second is what the platform made of the sleep. Every executor's heartbeat lapses while its machine sleeps, so on waking every executor is missing until its first tick, and a missing executor failed work terminally on every dispatch path — a synthetic FAILED whose classification was dropped on purpose, a session failed outright by a timer, a workflow task given the raw message as its failure reason. The answer is **the wait**: a step or operation task whose executor is missing is parked on its shard timer and dispatched when a look finds the executor back, the looks growing apart up to the heartbeat's own lifetime, as many as span ten minutes on a machine that stays awake, after which it fails as what it is — a transient, retryable outage, its classification kept. A parked step is visible as waiting from the moment it parks, naming the executor it waits for in the run view and the chat, and one whose executor this orchestrator has never seen since it started fails as `configuration`, not retryable, because that executor was never started rather than asleep. The wait is counted in looks rather than in time because a sleeping machine takes none, so a wait it sleeps through is not spent on waking, and a look that comes a heartbeat's lifetime or more late is that wake, counts for nothing and is followed by one more. The classification was dropped to stop one outage entering every step behind it into the retry budget at once; the budget of looks and the growing gap prevent that now, since nothing enters a retry budget while it waits, a lane that returns costs no retry at all, and one that does not costs each step one failure, once its looks are spent. A waiting step is its own completion path, not a stalled pickup: orphan recovery and the stall watchdog leave it alone while its timer lives, however long that is, and arm a timer that went missing again rather than fail the step. The host lane is no exception, though its executor is the operator's to start: a sleeping machine is exactly the case where the executor comes back. **The cold restart** — a sleep long enough that the stack itself comes back cold, its orchestrator restarted and the conversations it held to be recovered — is not in this decision. Whether it happens and what recovery costs are to be measured first, and it is a slice of its own. +**D22 — Local Publish decides whether a review's findings want a fix round, at a bar measured on the loop's own decisions, and records the answer. Nothing it decides clears a push, and nothing in the publication acts on it.** + +A review with findings woke a Helmsman turn to choose whether a fix round was worth it. That choice is asked many times with the same question, which is what a decision step is for (Plan 316 D2), so the skill asks it through `ai.decision.decide`, with the question written once in `publishLocalChangesDecisions.ts`. The fix round itself is not the publication's: it belongs to the conversation today, and to the shepherd once Plan 321 P1 lands. + +**After a failed check, no decision.** The first build also chose what follows a failed check: a rerun, a fix commissioned and republished, or a stop for the operator. That decision was removed pending labelled `operator_install_mismatch` cases (Plan 316 §7), and its fix round (`fix-checks`, `republish-after-checks`) went with it. A check lost only to tests running out of their time is run again by rule (`testTimeouts`, `recheck-commit`). The check step itself breaking — the executor, the checkout of the commit, the fetch of `origin`'s base — fails at `check-commit` saying the checks could not run, with no fix prescribed. Any other failure fails the run at `checks-failed` (`workflow.run.fail`, a step-only primitive that fails its task with the reason it is handed), with the checks' own account. That wakes the conversation as a failed check did before the decision. With the first run no longer failed by its projection, `host.commit.check` stopped reporting `clearedOrRetriableSha`, which nothing read any more; `failingGuard` is read by the check's own summary, which names the step that failed. The failure says what happened — the checks did not pass, nothing was scanned, reviewed or pushed — and prescribes no fix to the change, since a failure on the machine is not the change's. + +**After a review with findings, recorded.** One `choice` between `fix_before_merge` and `publish_as_is`, over every finding and the verdict, at Jev's 0.80 (`decide-after-review`). The step names no model, so the space's decision model answers it. The bar was measured on Jev, and another decider is measured on the same cases before the bar moves (Plan 316 §7). `describe-pr` records the answer with the findings, on the pull request and as the run's `afterReview`: `fix_before_merge`, `publish_as_is`, or `undecided` where no answer cleared the bar or none could be made. It is absent where the review returned no findings, since none was asked. The pull request lists the earlier rounds' findings as the brief passed them, closed and left open, and tells this review by its own summary, which names each of its findings once. What the run hands on as `findings` (`PullRequestFindingsSchema`) is the latest review's open findings: this review's where it finished, replacing the earlier rounds', or else the open ones it was passed. A round's list replaces the last rather than appending to it, so the list the next publication of the branch takes never grows round on round. It is also held within half of the 32 KB that publication's inputs carry together (`HANDED_ON_FINDINGS_MAX_BYTES`): at most `HANDED_ON_FINDINGS_MAX_COUNT` findings, the most severe first, each cut to `HANDED_ON_FINDING_MAX_BYTES`. The cut is measured in serialised bytes, not characters, since one character can serialise to six. A test holds this for the largest review there can be: fifty findings of the longest claims and paths, in the characters that serialise largest. Where the bound leaves any out, describe-pr says how many as `findingsLeftOut`. The list serves the next pull request's record and nothing else: a fix round is briefed from every finding of the review run, which the run names as `reviewRunId`. The pull request's text keeps every finding whole. No task reads the answer but the composition of the pull request, and no predicate reads it: every answer runs the same tasks, asks the same approval and pushes the same commit. The first build also judged each finding with a `yes_no` before the round was weighed. The measurement took that step out: no decider reached the target without abstaining on most findings. + +**A publication never starts a publication.** The first build ran the fix round inside the publication: a commission, then the fix published as a child publication. That nesting is out, for two reasons. A task-started child that pauses is cancelled: its waiter answers the task with the pause and cancels the child (`waiters.ts`), so a child publication stopping at its own push approval could never be approved. And the chain of task-started runs is bounded (`MAX_WORKFLOW_RUN_DEPTH`), so each nested round used up depth the chain could not spare. Raising the bound to three for two rounds only moved the edge. `MAX_WORKFLOW_RUN_DEPTH` is two again: a publication and the review of its own commit. The skill starts exactly one run, that review, and no skill in the catalog starts a publication; a test over the graph holds both. When `afterReview` is `fix_before_merge`, the conversation — or the shepherd — commissions the fix on the branch (`base: `), and publishes the branch again as a publication of its own, passing the run's `findings`. + +**What no decision touches.** The push gate, the scan and the posture are untouched (Plan 316 D9). A decision reads text that can steer it, so it is never the only gate on anything that leaves the machine, and the tests prove the push and its approval go exactly as they do without it. + +**The bar.** The fix-or-not bar is Jev's 0.80, measured in Plan 316 §7: it agreed on every case it decided, over seventeen, abstaining on 17.6%. The after-check question rests on two cases, none of them labelled `operator_install_mismatch`, and comes back into the skill only once the measurement has such cases. + ## 3. Measurement **First D13 reading** (2026-09-30, Full Circle): asked for a review "without sitting on it", Helmsman chose `wait: 'none'` unprompted, the start returned in 151 ms, the session idled at its prompt, and the review's completion woke it; it reported the verdict with no operator message in between. diff --git a/docs/plans/316-workflows-decide-without-a-language-model.md b/docs/plans/316-workflows-decide-without-a-language-model.md index 5de30b01..fe81683b 100644 --- a/docs/plans/316-workflows-decide-without-a-language-model.md +++ b/docs/plans/316-workflows-decide-without-a-language-model.md @@ -1,6 +1,6 @@ # Plan 316 — Workflows decide without a language model: a typed decision step -**Status:** 🔨 P0–P4 built and proven live; P5 and draft-level editing open · **First backend:** TypeSafe AI's Jev (a "System One" decision model) +**Status:** 🔨 P0–P4 built and proven live; P5 and draft-level editing open · **First backend:** TypeSafe AI's Jev (a "System One" decision model) · **First production consumer:** Publish Local Changes, which decides whether a review's findings want a fix round and records the answer as `afterReview` (Plan 315 D22), at the Jev bar set from §7's measurement of the loop's own decisions ## 1. Problem @@ -42,17 +42,19 @@ Input: - `{ type: 'choice', instructions?, options: Record, minConfidence? }`, 2–255 options - `{ type: 'score', instructions?, levels: (description | null)[], minConfidence? }`, 2–10 levels, index 0 lowest - `{ type: 'yes_no', instructions?, criteria?: { true?, false? }, minConfidence? }` +- `each` — optional `{ items, question }`: one `yes_no` asked of every item of a list (at most 50), for a list whose length is known only when the step runs — a review's findings. Each item travels in its own question's instructions beside the claim, all in the same single pass. `questions` may then be absent; one of the two is required. An empty list asks nothing and needs no model. - `model` — optional catalog reference; the catalog's default decision model when absent. Output: - `answers` — `Record`: - - choice: `{ type, value: label, confidence, probabilities: Record, decided }` - - score: `{ type, value: expected score, confidence, probabilities: Record, decided }` - - yes_no: `{ type, value: p ≥ 0.5, probability: p, confidence: max(p, 1 − p), decided }` + - choice: `{ type, value: label, decidedValue, confidence, probabilities: Record, decided }` + - score: `{ type, value: expected score, decidedValue, confidence, probabilities: Record, decided }` + - yes_no: `{ type, value: p ≥ 0.5, decidedValue, probability: p, confidence: max(p, 1 − p), decided }` +- `each` — where asked: every item's `yes_no` answer in order, `selected` (the items answered true and decided), `selectedCount`, `undecidedCount`, and `selectedJson`, the selected items as text a later step can put in a brief. - `model`, `usage { inputTokens, outputTokens }`, `latencyMs`. -For a `yes_no`, `confidence` is derived as the probability of the more likely outcome, so one threshold reads the same way on every question type. +For a `yes_no`, `confidence` is derived as the probability of the more likely outcome, so one threshold reads the same way on every question type. `decidedValue` is `value` where the answer is `decided` and null where it is not, so a route reads one comparison — `decidedValue == 'billing'` — that can sit beside another condition in a single `anyOf`, which `value` and `decided` read apart cannot. Wire: `POST {base}/v1/systemone` with `Authorization: Bearer `, body `{ state, questions, model }`, where the neutral names map onto the provider's (`options` → `criteria` for a choice, `levels` → `criteria` for a score, `yes_no` → `noul`). The response carries `model`, `answers` and `usage { input_tokens, output_tokens }`; the request id is in `x-typesafe-request-id`. `GET /v1/models` lists the account's models and serves as the credential probe. @@ -110,3 +112,65 @@ The `classifier` rails execute through the operation when a rail opts in. Exit: - A draft whose route names a label the question does not offer is refused with a diagnostic naming the label. - A fork reading `tasks.triage.output.answers.team.valeu` is refused at validation, not skipped at run time. - The operation never appears in an agent's tool list, and does appear in authoring search. + +## 7. Measurement — the loop's own decisions + +**Status: measured** on the operator's stack with all three deciders (the run recorded on plan node 0f442b9a). The results and what each changed in Publish Local Changes are under **Results**, below. + +**What it measures.** The choices Publish Local Changes makes for itself (Plan 315 D22), the per-finding judgement it was first built with, and the CI remedy choice Plan 321 makes by hand, against what was actually decided in this stream. Four questions: + +- **After a failed check**: the check's account → what the next publication of the branch did. A change appended means `fix_commission`; the same head published as it stands means `recheck_on_timeout`. +- **Each review finding**: the finding → whether a later publication carried it `closed` (a defect, fixed) or `open` (left — accepted or dismissed, which the records do not tell apart). +- **After a review with findings**: the findings → `fix_before_merge` where a later change closed any of them, `publish_as_is` where they were carried on all open. +- **A failing CI job of PRs #91–#124**: the last 300 lines of the job's log (Plan 321 §P0's bound) → `rerun` where another attempt of the same run followed on the same commit, `merge_round` for a merge commit next, `resolutions_bump` for a next commit touching only `package.json` and `yarn.lock` under `resolutions`, and `code_fix` otherwise. + +A case nothing followed is left unlabelled, not guessed. `operator_install_mismatch` is never inferred; it comes only from the operator's labels file, which overrides any inference. The report counts how many labels came from the operator and how many were inferred. + +**Deciders.** Every decision model the catalog offers — Jev, and gpt-6-luna-decisions since Plan 319 — each through `AIClient.decide`, the call the decision operation makes, so whichever a space resolves the step to is measured on the same cases. The results below predate the second and were measured on Jev. Then a critique-then-verdict `generate_json` call — a critique, a verdict among the same options and a stated probability that the verdict is right — on gpt-6-luna, and the same call on claude-opus-5-5 as the agent baseline. A language model's stated probability is not calibrated; it is reported as what it is. + +**Recorded per decider and question**: agreement with the operator's decision among the cases it decided; abstention at the bar chosen for it (the lowest bar in the 0.5–0.95 sweep at which agreement reaches the target, 90% by default); agreement over all cases with an abstention counted as not agreeing; errors apart from both; p50 and p95 latency; and cost per decision from the catalog's prices. Jev's whole sweep goes beside the table. + +**Why a script and not the eval plane's dataset (Plan 269).** A golden case there is a run's input and output, labelled by the operator against a skill's suite. These cases are decisions spread across runs and across GitHub — a check's account against the next publication's input, a log against the next commit — and none of them is a run of one skill. So it is a script, as Plan 323 P0 was. + +**Run it**, on the stack that holds the stream, with `AFLOW_API_KEY` set to an API key of the space's tenant and `gh` signed in: + +``` +yarn decisions:measure-loop collect --space --repo aflowai/aflow --prs 91-124 +yarn decisions:measure-loop measure [--labels ] [--target 0.9] +``` + +### Results + +The target is 90% agreement among the cases a decider decides. Each row is the decider at the bar the script chose for it: the lowest in the sweep that reaches the target. _n_ is the labelled cases of that question. Where the operator's run recorded no bar for a decider, the cell says so rather than inferring one. + +| Question | n | Decider | minConfidence | Agreement (decided) | Abstention | +| -------------------------------------------- | --- | -------------------------------- | --------------- | ------------------- | ------------ | +| After a failed check | 2 | Jev | 0.50 | 100% | 0% | +| | | gpt-6-luna (critique first) | not recorded | 100% | 0% | +| | | claude-opus-5-5 (baseline) | not recorded | 100% | 0% | +| Each review finding: a defect, by evidence | 46 | Jev | 0.90 | 100% | 69.6% | +| | | Jev, at 0.80 | 0.80 | 89.3% | 39.1% | +| | | gpt-6-luna (critique first) | 0.80 | 69.6% | 0% | +| | | claude-opus-5-5 (baseline) | 0.90 | 100% | 95.7% | +| After a review: fix before merge or not | 17 | Jev | 0.80 | 100% | 17.6% | +| | | Jev, at 0.50 | 0.50 | 93.8% | 5.9% | +| | | gpt-6-luna (critique first) | 0.90 | 100% | 29.4% | +| | | claude-opus-5-5 (baseline) | 0.70 | 90% | 41.2% | +| A failing CI job of PRs #91–#124: its remedy | 5 | Jev, gpt-6-luna, claude-opus-5-5 | none reaches it | 20% each | not recorded | + +Latency and cost per decision, each a range across the four questions: + +| Decider | p50 latency | Cost per decision | +| --------------------------- | ----------- | ---------------------- | +| Jev | 237–875 ms | $0.00003–$0.0008 | +| gpt-6-luna (critique first) | 2.6–5.6 s | about $0.00025–$0.0015 | +| claude-opus-5-5 (baseline) | 6.2–7.8 s | $0.016–$0.083 | + +**What each result changed** (Plan 315 D22): + +- **Each review finding — taken out of Publish Local Changes.** No decider reaches the target without abstaining on most findings. Jev agrees on every finding it decides only by abstaining on 69.6% of them, and at 0.80 it falls to 89.3%. gpt-6-luna decides every finding and agrees on 69.6%. claude-opus-5-5 abstains on 95.7%. By this section's own rule the step does not belong in the skill. A fix round — the conversation's or the shepherd's (Plan 315 D22) — is briefed from every finding of the review run, as the review reported them. The bounded list a publication hands on as `findings` serves the next pull request's record only. `each` stays on `ai.decision.decide` as an engine capability, with its tests. +- **After a review — kept, Jev at 0.80.** Jev agrees on every case it decides there and abstains on 17.6%, at a few hundred milliseconds and a fraction of a cent per decision. The bar was measured on Jev, and the step names no model: the space's decision model answers it. Another decider meets the bar unmeasured until it is asked the same cases — `yarn decisions:measure-loop` asks every decision model the catalog offers — and the bar moves only on that measurement. The critique-then-verdict baselines needed 0.90 and 0.70 for the same target, which is why one bar is never assumed to carry from one decider to another. claude-opus-5-5, the agent baseline, agrees on 90% while abstaining on 41.2%, at 6–8 seconds and two to three orders of magnitude the cost. The answer is recorded, never acted on: `afterReview` is `fix_before_merge`, `publish_as_is`, or `undecided` below 0.80, on the run and on the pull request with the review's findings. A publication never starts a publication. A task-started child that pauses is cancelled by its waiter, so a child publication stopping at its own approval could never be approved, and the depth of task-started runs is bounded. So the fix round on `fix_before_merge` belongs to the conversation, or to the shepherd once Plan 321 P1 lands, which publishes the branch again. The question was measured with a third option, `ask`, which no label named and which routed exactly as `publish_as_is` did; it was taken out, so the choice is `fix_before_merge` or `publish_as_is`, and an answer too unsure to be either is recorded as `undecided`. +- **After a failed check — removed from Publish Local Changes pending labelled `operator_install_mismatch` cases.** All three deciders agree on both of its two cases, and neither is an `operator_install_mismatch`: that label comes only from the operator's labels file, and the file names none yet. Two cases cannot place a bar — Jev's chosen 0.50 is the bottom of the sweep — and a question measured without one of its three answers has not been measured on the answer that stops for the operator. So the after-check decision is removed until the set holds labelled `operator_install_mismatch` cases. A failed check fails the run, waking Helmsman as it did before the decision, at `checks-failed` with the checks' own account. A check lost only to tests running out of their time is run again by rule. The question stays in `publishLocalChangesDecisions.ts`, and this script still asks it, so it can be measured again as cases accumulate. `ai.decision.decide` keeps its tests and every capability the step used. +- **CI remedy — not a decision task.** Every decider names the remedy actually taken — re-run, code fix, resolutions bump or merge round — for 20% of five failures. Plan 321 P1 decides on `when` rules, with a fix commission as the default for a failure no rule names, and the remedy is measured again once the set holds enough failures to say more. + +Jev's per-finding question is measured from `scripts/loop-decision-set.mjs`, where it now lives beside the CI remedy question, since no skill asks either. The after-review question the skill asks, and the after-check question only this measurement asks for now, are imported from `publishLocalChangesDecisions.ts`. diff --git a/docs/plans/321-the-loop-recovers-from-ci.md b/docs/plans/321-the-loop-recovers-from-ci.md index 1779f783..7561eaf8 100644 --- a/docs/plans/321-the-loop-recovers-from-ci.md +++ b/docs/plans/321-the-loop-recovers-from-ci.md @@ -45,7 +45,7 @@ The four reads are tasks with typed outputs, and the deciding step receives them | `list-prs` | `api.http.call` `listPullRequests` | open pull requests, filtered to the prefix and the marker, minus `hands-off` | | `read-pr` | per pull request: `getPullRequest`, `listCheckRuns`, `listIssueComments` | head sha, checks, the handoff | | `read-in-flight` | `memory.store.get`, `workflow.run.list` | the lease and the non-terminal runs, paused included, whose branch input is the branch | -| `decide` | a pure rule; `ai.generate_json` for a log excerpt only | `fix \| wait \| abandon \| leave`, with the reason | +| `decide` | `when` rules only | `fix \| wait \| abandon \| leave`, with the reason; `fix` where no rule names another | | `read-failure` | `listWorkflowRunJobs`, `getJobLogs` | failing jobs and bounded log excerpts, as a payload | | `lease` | `memory.store.put` | the lease, written before the commission | | `fix` | `host.harness.run` | `base: `, `mergeFrom: origin/`, the assembled task | @@ -54,7 +54,7 @@ The four reads are tasks with typed outputs, and the deciding step receives them | `release` | `memory.store.delete` | the lease, on every terminal path | | `renew` | `agent.schedule.create` | the next schedule when this one nears `maxFirings` | -`decide` is a rule. The four reads of D3 decide `fix`, `wait`, `abandon` and `leave` without a model; a model, `ai.generate_json`, is reached for only to read a log excerpt a rule cannot. +`decide` is a rule. The four reads of D3 decide `fix`, `wait`, `abandon` and `leave` without a model, and a failure no rule names is `fix`: a commission briefed with the log excerpt, which reads what a rule cannot. No model chooses the remedy. Measured on this stream's own CI failures (Plan 316 §7), Jev, gpt-6-luna and claude-opus-5-5 each named the remedy actually taken — re-run, code fix, resolutions bump or merge round — for 1 of 5 (20%), so the remedy is not a decision task in P1; it is measured again once the labelled set holds enough failures to say more. ### 3.2 What is recorded @@ -76,7 +76,7 @@ Every pass leaves the handoff current and one run in the ledger. A `fix` also le 3. **The size of a failing job's log.** _Commands:_ `gh api repos/aflowai/aflow/actions/runs/37460214524/attempts/1/jobs` to name the failing jobs, then `gh api repos/aflowai/aflow/actions/jobs//logs` — GitHub's `GET /repos/{owner}/{repo}/actions/jobs/{job_id}/logs`, which answers with a redirect to a signed URL — for CI run 37460214524 on `aflowai/aflow` (PR #94's first run). _Answer:_ the Test job (112257622723) is 5,955 lines, 923,330 bytes; Vitest's "Failed Tests" block begins at line 5,676, so the last 280 lines carry the failure and its assertion. The Dependency audit job (112257622741) is 421 lines, 32,310 bytes; the advisories first appear at line 232 and the final attempt's block begins at line 387, so the last 35 lines carry every advisory once. An excerpt bounded at the last 300 lines, under 60 KB, carried the failure for both job kinds observed; a bound needs no endpoint binding to hold, and the D5 endpoints are P1's. 4. **Whether the attention list can name a branch.** _Command:_ `workflow.run.list_attention` (pending and consumed items). _Answer:_ no. Items carry `id`, `kind`, `relatedRunId`, `relatedResource` (`workflow_run:`), `payload.terminalStatus`, `priority`, `createdAt`, `consumedAt`, `own` — no branch, no PR, no repo field; learning what a run produced costs a follow-up run-detail read per item. -**P1 — One pass, by hand.** The skill with `read-*`, `decide`, `handoff` and `release`, fired manually against a pull request with a planted failing test; `fix` and `publish` land it. _Exit:_ the planted failure is fixed and pushed within one pass without a hand; a second pass on the same check run is `wait`. +**P1 — One pass, by hand.** The skill with `read-*`, `decide` (on `when` rules, a fix commission the default), `handoff` and `release`, fired manually against a pull request with a planted failing test; `fix` and `publish` land it. _Exit:_ the planted failure is fixed and pushed within one pass without a hand; a second pass on the same check run is `wait`. **P2 — Cold and concurrent.** The schedule, the lease, `hands-off`, the budget and `abandon`. _Exit:_ two firings overlapping on one pull request produce one fix; a branch with a paused publication is `wait`; a labelled pull request is untouched; the fourth failure on one pull request ends in a comment and an Action Center item. diff --git a/package.json b/package.json index 34934fab..5ff113f5 100644 --- a/package.json +++ b/package.json @@ -25,6 +25,7 @@ "db:migrate-tenants": "NODE_OPTIONS='--conditions=ts-source' node scripts/with-stack-env.mjs npx tsx scripts/migrate-tenants.ts", "db:seed": "NODE_OPTIONS='--conditions=ts-source' node scripts/with-stack-env.mjs tsx scripts/seed-flows.ts", "db:seed:cybernetic": "NODE_OPTIONS='--conditions=ts-source' node scripts/with-stack-env.mjs tsx scripts/dev/seed-cybernetic-space.ts", + "decisions:measure-loop": "NODE_OPTIONS='--conditions=ts-source' node scripts/with-stack-env.mjs tsx scripts/decisions-measure-loop.ts", "dev": "node scripts/dev.mjs", "start": "node scripts/start.mjs", "dev:core": "node scripts/dev.mjs --profile core", diff --git a/packages/ai-client/src/decision.test.ts b/packages/ai-client/src/decision.test.ts index 30f3ed19..4d9a58fd 100644 --- a/packages/ai-client/src/decision.test.ts +++ b/packages/ai-client/src/decision.test.ts @@ -1,6 +1,10 @@ import { describe, expect, it } from 'vitest'; import type { DecisionQuestions } from '@aflow/schemas'; -import { resolveDecisionAnswers } from './decision.js'; +import { + decisionRequestQuestions, + resolveDecisionAnswers, + splitDecisionAnswers, +} from './decision.js'; import { AIClientError } from './errors.js'; const questions: DecisionQuestions = { @@ -43,6 +47,7 @@ describe('resolveDecisionAnswers', () => { expect(answers['urgent']).toEqual({ type: 'yes_no', value: false, + decidedValue: false, probability: 0.15, confidence: 0.85, decided: true, @@ -54,6 +59,22 @@ describe('resolveDecisionAnswers', () => { }); }); + it('carries the value a route reads only where the answer cleared its bar, and null where it did not', () => { + const sure = resolve(); + expect(sure['team']?.decidedValue).toBe('billing'); + expect(sure['frustration']?.decidedValue).toBe(0.4); + // A false that cleared the bar is a decided false, never mistaken for none. + expect(sure['urgent']?.decidedValue).toBe(false); + const unsure = resolve({ + team: { type: 'choice', choice: 'technical', confidence: 0.55, probabilities: {} }, + frustration: { type: 'score', score: 0.4, confidence: 0.2, probabilities: {} }, + urgent: { type: 'yes_no', probability: 0.3 }, + }); + expect(unsure['team']).toMatchObject({ value: 'technical', decidedValue: null }); + expect(unsure['frustration']).toMatchObject({ value: 0.4, decidedValue: null }); + expect(unsure['urgent']).toMatchObject({ value: false, decidedValue: null }); + }); + it('decides every answer to a question with no minConfidence', () => { expect(resolve()['refund']).toMatchObject({ value: true, decided: true }); }); @@ -70,10 +91,87 @@ describe('resolveDecisionAnswers', () => { ).toThrow(/yes_no question and got a score answer/); }); - it('passes a refusal on as never decided, whatever the question type', () => { + it("passes a refusal on as never decided, whatever the question type — its decidedValue null, as an unsure answer's is", () => { expect(resolve({ team: { type: 'refusal' } })['team']).toEqual({ type: 'refusal', + decidedValue: null, decided: false, }); }); }); + +describe('a question asked of each item', () => { + const each = { + items: [{ claim: 'leaks a handle' }, { claim: 'a name could be better' }, { claim: 'unsure' }], + question: { + type: 'yes_no' as const, + instructions: 'Names a defect, established by evidence shown', + minConfidence: 0.8, + }, + }; + const named: DecisionQuestions = { + round: { type: 'choice', options: { fix: null, ship: null } }, + }; + + it('asks it once per item, each item beside the claim, alongside the named questions', () => { + const asked = decisionRequestQuestions({ questions: named, each }); + expect(Object.keys(asked)).toEqual(['round', 'each_0', 'each_1', 'each_2']); + expect(asked['each_1']).toEqual({ + type: 'yes_no', + instructions: { + claim: 'Names a defect, established by evidence shown', + about: 'The item below, one of a list the state describes.', + item: { claim: 'a name could be better' }, + }, + minConfidence: 0.8, + }); + }); + + it('selects only the items answered true and decided — never one the model was unsure of', () => { + const input = { questions: named, each }; + const asked = decisionRequestQuestions(input); + const resolved = resolveDecisionAnswers(asked, { + round: { type: 'choice', choice: 'fix', confidence: 0.9, probabilities: {} }, + each_0: { type: 'yes_no', probability: 0.95 }, + each_1: { type: 'yes_no', probability: 0.05 }, + each_2: { type: 'yes_no', probability: 0.7 }, + }); + const { answers, each: out } = splitDecisionAnswers(input, resolved); + expect(Object.keys(answers)).toEqual(['round']); + expect(out?.answers.map((a) => [a.type === 'yes_no' ? a.value : null, a.decided])).toEqual([ + [true, true], + [false, true], + [true, false], + ]); + expect(out?.selected).toEqual([{ claim: 'leaks a handle' }]); + expect(out?.selectedCount).toBe(1); + expect(out?.undecidedCount).toBe(1); + expect(JSON.parse(out?.selectedJson ?? '')).toEqual([{ claim: 'leaks a handle' }]); + }); + + it('never selects an item the model declined, and counts it undecided', () => { + const input = { each: { ...each, items: [{ claim: 'declined' }, { claim: 'leaks' }] } }; + const asked = decisionRequestQuestions(input); + const { each: out } = splitDecisionAnswers( + input, + resolveDecisionAnswers(asked, { + each_0: { type: 'refusal' }, + each_1: { type: 'yes_no', probability: 0.99 }, + }), + ); + expect(out?.answers[0]).toEqual({ type: 'refusal', decidedValue: null, decided: false }); + expect(out?.selected).toEqual([{ claim: 'leaks' }]); + expect(out?.undecidedCount).toBe(1); + }); + + it('reports none selected as an empty list in both forms', () => { + const input = { each: { ...each, items: [{ claim: 'x' }] } }; + const asked = decisionRequestQuestions(input); + const { each: out } = splitDecisionAnswers( + input, + resolveDecisionAnswers(asked, { each_0: { type: 'yes_no', probability: 0.01 } }), + ); + expect(out?.selected).toEqual([]); + expect(out?.selectedJson).toBe('[]'); + }); +}); diff --git a/packages/ai-client/src/decision.ts b/packages/ai-client/src/decision.ts index 5092f9e2..4246f049 100644 --- a/packages/ai-client/src/decision.ts +++ b/packages/ai-client/src/decision.ts @@ -1,4 +1,11 @@ -import type { DecisionAnswer, DecisionQuestions } from '@aflow/schemas'; +import { + decisionEachQuestionName, + type DecisionAnswer, + type DecisionEach, + type DecisionEachAnswer, + type DecisionEachOutput, + type DecisionQuestions, +} from '@aflow/schemas'; import type { ProviderDecisionAnswer } from './types.js'; import { AIClientError } from './errors.js'; @@ -16,7 +23,8 @@ import { AIClientError } from './errors.js'; * values, and a label nothing routes on would silently skip every branch. * * A question the model declined is passed on as a refusal that is never - * decided, so the route a workflow keeps for uncertain answers takes it. + * decided, its `decidedValue` null as an unsure answer's is, so the route a + * workflow keeps for uncertain answers takes it on the same one comparison. */ export function resolveDecisionAnswers( questions: DecisionQuestions, @@ -29,7 +37,7 @@ export function resolveDecisionAnswers( throw mismatch(`no answer was returned for "${name}"`); } if (answer.type === 'refusal') { - resolved[name] = { type: 'refusal', decided: false }; + resolved[name] = { type: 'refusal', decidedValue: null, decided: false }; continue; } if (answer.type !== question.type) { @@ -43,33 +51,40 @@ export function resolveDecisionAnswers( if (question.type !== 'choice' || !Object.hasOwn(question.options, answer.choice)) { throw mismatch(`"${name}" chose "${answer.choice}", which is not one of its options`); } + const decided = clears(answer.confidence); resolved[name] = { type: 'choice', value: answer.choice, + decidedValue: decided ? answer.choice : null, confidence: answer.confidence, probabilities: answer.probabilities, - decided: clears(answer.confidence), + decided, }; break; } case 'score': { + const decided = clears(answer.confidence); resolved[name] = { type: 'score', value: answer.score, + decidedValue: decided ? answer.score : null, confidence: answer.confidence, probabilities: answer.probabilities, - decided: clears(answer.confidence), + decided, }; break; } case 'yes_no': { const confidence = Math.max(answer.probability, 1 - answer.probability); + const value = answer.probability >= 0.5; + const decided = clears(confidence); resolved[name] = { type: 'yes_no', - value: answer.probability >= 0.5, + value, + decidedValue: decided ? value : null, probability: answer.probability, confidence, - decided: clears(confidence), + decided, }; break; } @@ -78,6 +93,71 @@ export function resolveDecisionAnswers( return resolved; } +/** + * Every question a request asks, as one record the provider answers in a + * single pass: the named questions, then `each`'s question once per item. An + * item travels inside its own question's instructions, beside the claim, so + * every answer is about exactly one item while the state stays shared. + */ +export function decisionRequestQuestions(input: { + questions?: DecisionQuestions | undefined; + each?: DecisionEach | undefined; +}): DecisionQuestions { + const questions: DecisionQuestions = { ...(input.questions ?? {}) }; + if (input.each === undefined) return questions; + const { question, items } = input.each; + items.forEach((item, index) => { + questions[decisionEachQuestionName(index)] = { + ...question, + instructions: { + ...(question.instructions !== undefined ? { claim: question.instructions } : {}), + about: 'The item below, one of a list the state describes.', + item, + }, + }; + }); + return questions; +} + +/** + * The resolved answers split back into the named questions' and `each`'s: + * an item is selected only where its answer is true and decided, so an item + * the model was unsure of, or declined, never passes as though it had been + * judged. + */ +export function splitDecisionAnswers( + input: { questions?: DecisionQuestions | undefined; each?: DecisionEach | undefined }, + resolved: Readonly>, +): { answers: Record; each?: DecisionEachOutput } { + const answers: Record = {}; + for (const name of Object.keys(input.questions ?? {})) { + const answer = resolved[name]; + if (answer !== undefined) answers[name] = answer; + } + if (input.each === undefined) return { answers }; + const eachAnswers: DecisionEachAnswer[] = input.each.items.map((_, index) => { + const answer = resolved[decisionEachQuestionName(index)]; + if (answer?.type !== 'yes_no' && answer?.type !== 'refusal') { + throw mismatch(`no yes_no answer was returned for item ${String(index)}`); + } + return answer; + }); + const selected = input.each.items.filter((_, index) => { + const answer = eachAnswers[index]; + return answer?.type === 'yes_no' && answer.value && answer.decided; + }); + return { + answers, + each: { + answers: eachAnswers, + selected, + selectedCount: selected.length, + undecidedCount: eachAnswers.filter((answer) => !answer.decided).length, + selectedJson: JSON.stringify(selected, null, 2), + }, + }; +} + function mismatch(detail: string): AIClientError { return new AIClientError( `The decision model's answer does not fit the question: ${detail}`, diff --git a/packages/ai-client/src/index.ts b/packages/ai-client/src/index.ts index aea27d4e..f2b9f520 100644 --- a/packages/ai-client/src/index.ts +++ b/packages/ai-client/src/index.ts @@ -180,7 +180,11 @@ export type { FireworksConfig } from './providers/fireworks.js'; export { createRunwareAdapter, runwareTaskUuid } from './providers/runware.js'; // Decisions -export { resolveDecisionAnswers } from './decision.js'; +export { + decisionRequestQuestions, + resolveDecisionAnswers, + splitDecisionAnswers, +} from './decision.js'; // Plan 323: the answer floor prompt budgeting and a local runtime's sizing share export { MIN_ANSWER_TOKENS } from './providers/ollamaPromptSize.js'; diff --git a/packages/cybernetic-runtime/src/stagedChange/applyArtifactSeeds.ts b/packages/cybernetic-runtime/src/stagedChange/applyArtifactSeeds.ts index 1daad378..d6c826c5 100644 --- a/packages/cybernetic-runtime/src/stagedChange/applyArtifactSeeds.ts +++ b/packages/cybernetic-runtime/src/stagedChange/applyArtifactSeeds.ts @@ -51,8 +51,8 @@ export interface ArtifactSeedsApplyResult { /** * Inline-encode raw text as a PayloadRef the executor's `loadBlob` (and - * thus `PayloadStore.retrieve → parseInlineRef`) can read back as the - * original string. Mirrors the shape `parseInlineRef` expects: + * thus `PayloadStore.retrieve → decodeInlinePayloadRef`) can read back as the + * original string. Mirrors the shape `decodeInlinePayloadRef` expects: * `inline:` where the value is the raw * source string. `JSON.parse(JSON.stringify('foo')) === 'foo'`, so the * round-trip is lossless and the render path's existing inline branch diff --git a/packages/payload-store/src/__tests__/refCanonicality.test.ts b/packages/payload-store/src/__tests__/refCanonicality.test.ts index 5d41f313..553d319f 100644 --- a/packages/payload-store/src/__tests__/refCanonicality.test.ts +++ b/packages/payload-store/src/__tests__/refCanonicality.test.ts @@ -1,6 +1,10 @@ import { describe, expect, it } from 'vitest'; import { MAX_INLINE_PAYLOAD_BYTES, parsePayloadRef, type PayloadRef } from '@aflow/schemas'; -import { createMemoryPayloadStore } from '../store.js'; +import { + createMemoryPayloadStore, + decodeInlinePayloadRef, + encodeInlinePayloadRef, +} from '../store.js'; /** * Every backend addresses an object through the one shared ref parser, so the @@ -79,4 +83,13 @@ describe('an inline payload ref over the cap', () => { await expect(store.retrieve(small)).resolves.toEqual({ a: 1 }); await expect(store.exists(small)).resolves.toBe(true); }); + + it('decodes, without a store, what encoding wrote — and refuses what no store would read', () => { + const value = { summary: 'checks failed', tail: 'FAIL tsc' }; + expect(decodeInlinePayloadRef(encodeInlinePayloadRef(value))).toEqual(value); + expect(() => decodeInlinePayloadRef(OVERSIZED)).toThrow(/invalid inline payload_ref/i); + expect(() => decodeInlinePayloadRef('inline:@@@not-base64@@@' as PayloadRef)).toThrow( + /invalid inline payload_ref/i, + ); + }); }); diff --git a/packages/payload-store/src/index.ts b/packages/payload-store/src/index.ts index 19dd913e..adca9dc8 100644 --- a/packages/payload-store/src/index.ts +++ b/packages/payload-store/src/index.ts @@ -23,6 +23,7 @@ export { getPayloadStoreConfig, contentAddressForJson, encodeInlinePayloadRef, + decodeInlinePayloadRef, createPayloadStore, createMemoryPayloadStore, createRedisPayloadStore, diff --git a/packages/payload-store/src/store.ts b/packages/payload-store/src/store.ts index 29cc0195..8f236521 100644 --- a/packages/payload-store/src/store.ts +++ b/packages/payload-store/src/store.ts @@ -343,9 +343,10 @@ function assertCanonicalInlineRef(payloadRef: PayloadRef): void { } /** - * Parse an inline payload reference and return the decoded data. + * The value an inline payload reference carries — the counterpart of + * `encodeInlinePayloadRef`, for a reader holding a ref and no store. */ -function parseInlineRef(payloadRef: PayloadRef): unknown { +export function decodeInlinePayloadRef(payloadRef: PayloadRef): unknown { assertCanonicalInlineRef(payloadRef); const base64Data = payloadRef.slice('inline:'.length); const jsonStr = Buffer.from(base64Data, 'base64').toString('utf-8'); @@ -485,7 +486,7 @@ export function createPayloadStore(config: PayloadStoreConfig): PayloadStore { async retrieve(payloadRef) { // Handle inline refs if (isInlineRef(payloadRef)) { - return parseInlineRef(payloadRef); + return decodeInlinePayloadRef(payloadRef); } const path = objectPathForStore(payloadRef, config.bucketName); @@ -678,7 +679,7 @@ export function createMemoryPayloadStore(): PayloadStore { async retrieve(payloadRef) { // Handle inline refs if (isInlineRef(payloadRef)) { - return parseInlineRef(payloadRef); + return decodeInlinePayloadRef(payloadRef); } const data = store.get(keyFor(payloadRef)); @@ -863,7 +864,7 @@ export function createFilePayloadStore(config: FilePayloadStoreConfig): PayloadS async retrieve(payloadRef) { if (isInlineRef(payloadRef)) { - return parseInlineRef(payloadRef); + return decodeInlinePayloadRef(payloadRef); } try { const raw = await readFile(fileStorePath(rootDir, payloadRef), 'utf-8'); @@ -1056,7 +1057,7 @@ export function createRedisPayloadStore( async retrieve(payloadRef) { // Handle inline refs if (isInlineRef(payloadRef)) { - return parseInlineRef(payloadRef); + return decodeInlinePayloadRef(payloadRef); } const key = buildRedisKey(keyPrefix, payloadRef); diff --git a/packages/platform-artifacts/src/index.ts b/packages/platform-artifacts/src/index.ts index e0c065ac..df177b76 100644 --- a/packages/platform-artifacts/src/index.ts +++ b/packages/platform-artifacts/src/index.ts @@ -48,6 +48,15 @@ export type { HelmsmanSurface } from './helmsmanSurface.js'; export { PLATFORM_SKILL_BUNDLES, ALL_PLATFORM_WORKFLOWS } from './skillBundles.js'; export { SKILL_CATALOG, getSkillCatalogEntry, listSkillCatalog } from './skillCatalog.js'; +// The questions Publish Local Changes is measured on, for the measurement that sets their bars +export { + AFTER_CHECK_QUESTION, + AFTER_CHECK_QUESTION_NAME, + AFTER_CHECK_ROUTES, + AFTER_REVIEW_QUESTION, + AFTER_REVIEW_QUESTION_NAME, + AFTER_REVIEW_ROUTES, +} from './skillCatalog/publishLocalChangesDecisions.js'; export { CONNECTOR_CATALOG, diff --git a/packages/platform-artifacts/src/skillBundleCatalog.ts b/packages/platform-artifacts/src/skillBundleCatalog.ts index 54a4c469..d227b578 100644 --- a/packages/platform-artifacts/src/skillBundleCatalog.ts +++ b/packages/platform-artifacts/src/skillBundleCatalog.ts @@ -260,7 +260,7 @@ const LOCAL_CODE_REVIEW_BUNDLE: AuthoredSkillBundle = { const LOCAL_PUBLISH_BUNDLE: AuthoredSkillBundle = { bundleId: 'local-publish' as SkillBundleId, - revision: -3, + revision: 0, name: 'Local Publish', tagline: 'Commit a patch onto a branch of a connected repository, clear the push, and open the pull request.', @@ -269,6 +269,8 @@ const LOCAL_PUBLISH_BUNDLE: AuthoredSkillBundle = { **What it installs**: - The **Publish Local Changes** skill — the patch is committed in a detached worktree, so the working tree is untouched: a fresh branch starts at the commission's base, and the branch a fix was commissioned from is appended to at its head; the checks the folder declares run in a checkout of the commit, and a failure stops the run with nothing scanned or pushed — save one where only tests ran out of their time, which a loaded machine causes and the run checks once more — the push itself is refused without the receipt of their passing on that commit; everything the push would add — the commit and any of the folder's own commits under it that \`origin\` does not have yet — is scanned for secrets, with the commits' headers and messages and the pull request's title and summary, and a finding stops the run with nothing pushed; the run then waits for the operator's approval — unless the folder's push approval says it need not ask — and only after that is that commit pushed to its branch on \`origin\` — refused, with nothing pushed, where \`origin\`'s base no longer holds what the run measured or \`origin\` pushes elsewhere than it fetches — and the pull request opened, or the one the branch already has reported. +**After a review with findings**: the run decides whether they are fixed before the branch merges and records the answer as \`afterReview\` — \`fix_before_merge\`, \`publish_as_is\`, or \`undecided\` where no answer cleared the bar — on the run and on the pull request, with the findings. It acts on none of it: a publication never starts a commission or another publication. When \`afterReview\` is \`fix_before_merge\`, the conversation — or the shepherd, once it runs — commissions the fix round on the branch, briefed from every finding of the review run the publication names as \`reviewRunId\`, and publishes again. No decision clears or holds a push. + **After install**: connect the repository as a folder allowing pushes under a branch prefix, and bind the GitHub connector for the space. Then hand the skill a commission's \`patchRef\`, a branch name under that prefix and a title. To have a publication run the repository's own checks first, declare them on the machine: \`aflow harness checks -- [args...]\`, run from the repository root. A folder that declares none runs none, and the pull request's checks remain the proof. **When it asks before pushing**: the folder's push approval, set on the machine that holds it — \`always\` asks before every push, \`never\` pushes without asking, and \`unless-unreviewed\` has the publication run a Local Code Review of everything the push would add and push without asking only when that review returns \`approve\`. \`unless-unreviewed\` is the default. Every publication scans what the push would carry before it, whatever the posture, which is what lets a review stand in for the approval. Where the scan could not read a file whole — binary, a NUL byte, too large, a line too long, a Git LFS pointer — or a line that looks like a secret carries an \`aflow-scan: allow\` comment, no review runs and the run asks the operator under every posture, \`never\` included, naming each. Change it with \`aflow harness push-approval \`.`, @@ -287,6 +289,7 @@ const LOCAL_PUBLISH_BUNDLE: AuthoredSkillBundle = { 'A folder connected without a publish prefix can be committed to but not pushed from — the push is refused where it runs. Ask for the folder to be reconnected allowing pushes under a prefix before starting a publication.', "Whether the run waits at the approval is the folder's push approval, shown in the machine block. Under `unless-unreviewed`, the default, the run reviews everything its push would add before it gets there, so no review needs starting alongside it. A file the scan could not read or a line marked `aflow-scan: allow` makes it ask under every posture.", "The folder's checks, where it declares them — `checks` in the machine block — run on the commit before anything else, so no build or test needs running beside a publication. A failed check fails the run with the end of what it printed, and nothing has left the machine; one that failed only on tests running out of time is run once more by the publication itself first, so it needs no retry from the conversation.", + "A publication only records what it decided about its review's findings, as `afterReview`. On `fix_before_merge` the fix round is yours once the run has ended: commission the fix on the branch (`base: `) briefed from every finding of the `reviewRunId` run, then publish it again passing the run's `findings`, its fixed ones marked closed — a bounded list for the pull request's record, not the brief. `publish_as_is` or `undecided` leaves the findings to the operator.", "A commission's change is published by its `patchRef`, never its `patch` text: that copy is cut short on a large change, and a run's inputs are capped at 32 KB together. `patch` is for a small diff the operator hands over.", ], }; diff --git a/packages/platform-artifacts/src/skillCatalog/publishLocalChanges.test.ts b/packages/platform-artifacts/src/skillCatalog/publishLocalChanges.test.ts index 8959481c..962aae51 100644 --- a/packages/platform-artifacts/src/skillCatalog/publishLocalChanges.test.ts +++ b/packages/platform-artifacts/src/skillCatalog/publishLocalChanges.test.ts @@ -30,13 +30,21 @@ import { isEvalPlaneOperation, MAX_PARENT_INPUTS_SERIALIZED_BYTES, PAUSE_INSTRUCTION_MAX_CHARS, + HANDED_ON_FINDINGS_MAX_COUNT, PullRequestFindingsSchema, PullRequestGapsSchema, + pullRequestReviewRecord, RUN_PULL_REQUEST_URL_OUTPUT, substituteTemplateBinds, toJsonSchemaSync, } from '@aflow/schemas'; import { PUBLISH_LOCAL_CHANGES } from './publishLocalChanges.js'; +import { + AFTER_REVIEW_QUESTION, + AFTER_REVIEW_ROUTES, + type AfterReviewRoute, +} from './publishLocalChangesDecisions.js'; +import { SKILL_CATALOG } from './index.js'; import { REVIEW_LOCAL_CHANGES } from './reviewLocalChanges.js'; const wf = PUBLISH_LOCAL_CHANGES.bundle.workflow; @@ -133,15 +141,17 @@ describe('Publish Local Changes — the patch becomes a branch, then a pull requ expect(validity.status).toBe('valid'); }); - it('is thirteen tasks — commit, check and its one rerun, scan, what decides the approval, the pull request composed, approve, push, find and rewrite or open the pull request', () => { + it('is fifteen tasks — commit, check, the stop on a failed check, scan, what decides the approval, the review and the decision recorded on its findings, the pull request composed, approve, push, and the pull request found, rewritten or opened', () => { expect(wf.tasks.map((t) => t.taskId)).toEqual([ 'commit', 'check-commit', + 'checks-failed', 'recheck-commit', 'scan-commit', 'read-repository', 'read-push-approval', 'review-commit', + 'decide-after-review', 'describe-pr', 'approve-push', 'push', @@ -149,48 +159,50 @@ describe('Publish Local Changes — the patch becomes a branch, then a pull requ 'update-pr', 'open-pr', ]); - expect(wf.tasks.map((t) => t.type)).toEqual([ - 'operation', - 'operation', - 'operation', - 'operation', - 'operation', - 'operation', - 'operation', - 'operation', - 'human', - 'operation', - 'operation', - 'operation', - 'operation', - ]); - expect(wf.tasks.map((t) => t.dependsOn ?? [])).toEqual([ - [], - ['commit'], - ['commit', 'check-commit'], - ['commit', 'check-commit', 'recheck-commit'], - ['commit'], - ['commit', 'scan-commit'], - ['read-push-approval', 'scan-commit'], - ['commit', 'check-commit', 'recheck-commit', 'scan-commit', 'review-commit'], - ['read-repository', 'scan-commit', 'review-commit'], - ['approve-push', 'scan-commit', 'check-commit', 'recheck-commit', 'describe-pr'], - ['push'], - ['find-pr', 'describe-pr'], - ['find-pr', 'describe-pr'], + expect(wf.tasks.filter((t) => t.type !== 'operation').map((t) => t.taskId)).toEqual([ + 'approve-push', ]); - expect(taskOrThrow('describe-pr').operation).toBe('host.commit.describe'); - expect(taskOrThrow('update-pr').operation).toBe('api.http.call'); - expect(taskOrThrow('commit').operation).toBe('host.file.patch'); - expect(taskOrThrow('check-commit').operation).toBe('host.commit.check'); - expect(taskOrThrow('recheck-commit').operation).toBe('host.commit.check'); - expect(taskOrThrow('scan-commit').operation).toBe('host.commit.scan'); - expect(taskOrThrow('read-repository').operation).toBe('api.http.call'); - expect(taskOrThrow('read-push-approval').operation).toBe('host.binding.inspect'); - expect(taskOrThrow('review-commit').operation).toBe('workflow.run.start'); - expect(taskOrThrow('push').operation).toBe('host.process.exec'); - expect(taskOrThrow('find-pr').operation).toBe('api.http.call'); - expect(taskOrThrow('open-pr').operation).toBe('api.http.call'); + expect(Object.fromEntries(wf.tasks.map((t) => [t.taskId, t.dependsOn ?? []]))).toEqual({ + commit: [], + 'check-commit': ['commit'], + 'checks-failed': ['check-commit'], + 'recheck-commit': ['commit', 'check-commit'], + 'scan-commit': ['commit', 'check-commit', 'recheck-commit'], + 'read-repository': ['commit', 'check-commit', 'recheck-commit'], + 'read-push-approval': ['commit', 'scan-commit'], + 'review-commit': ['read-push-approval', 'scan-commit'], + 'decide-after-review': ['review-commit'], + 'describe-pr': [ + 'commit', + 'check-commit', + 'recheck-commit', + 'scan-commit', + 'review-commit', + 'decide-after-review', + ], + 'approve-push': ['read-repository', 'scan-commit', 'review-commit'], + push: ['approve-push', 'scan-commit', 'check-commit', 'recheck-commit', 'describe-pr'], + 'find-pr': ['push'], + 'update-pr': ['find-pr', 'describe-pr'], + 'open-pr': ['find-pr', 'describe-pr'], + }); + expect(Object.fromEntries(wf.tasks.map((t) => [t.taskId, t.operation]))).toEqual({ + commit: 'host.file.patch', + 'check-commit': 'host.commit.check', + 'checks-failed': 'workflow.run.fail', + 'recheck-commit': 'host.commit.check', + 'scan-commit': 'host.commit.scan', + 'read-repository': 'api.http.call', + 'read-push-approval': 'host.binding.inspect', + 'review-commit': 'workflow.run.start', + 'decide-after-review': 'ai.decision.decide', + 'describe-pr': 'host.commit.describe', + 'approve-push': undefined, + push: 'host.process.exec', + 'find-pr': 'api.http.call', + 'update-pr': 'api.http.call', + 'open-pr': 'api.http.call', + }); }); it('reads the repository on the same GitHub binding the pull request uses, before the approval', () => { @@ -211,8 +223,12 @@ describe('Publish Local Changes — the patch becomes a branch, then a pull requ }); const kinds = Object.values(read.inputBindings ?? {}).map((b) => b.kind); expect(kinds).not.toContain('connection_binding'); + // Only once the checks cleared the commit: a failed check reads nothing more. expect(read.when).toEqual({ - expression: "tasks.commit.output.state == 'applied'", + anyOf: [ + 'tasks.check-commit.output.passed == true', + 'tasks.recheck-commit.output.passed == true', + ], onMissingRef: 'skip', }); }); @@ -739,16 +755,72 @@ type Decision = 'approved' | 'declined'; * The review run's ending as the waiter delivers it to the task: the run's * promoted output only when it completed. */ -function reviewEnding(review: Exclude): Record { +function reviewEnding( + review: Exclude, + findings: readonly Finding[] = [], +): Record { const completed = review !== 'failed' && review !== 'cancelled' && review !== 'paused'; return WorkflowRunWakeupEnvelopeSchema.parse({ runId: 'review-run', outcome: completed ? 'completed' : review, waiterId: 'waiter-1', - ...(completed ? { result: { output: { verdict: review, reviewSummary: 'Read.' } } } : {}), + ...(completed + ? { result: { output: { verdict: review, reviewSummary: summaryOf(findings), findings } } } + : {}), }); } +/** The review's summary, which names every finding inline, as the review skill asks it to. */ +function summaryOf(findings: readonly Finding[]): string { + if (findings.length === 0) return 'Read.'; + return findings + .map( + (f) => + `${f.severity} ${f.file}${f.line === undefined ? '' : `:${String(f.line)}`} — ${f.claim}`, + ) + .join('\n'); +} + +/** A finding as Local Code Review returns it. */ +interface Finding { + severity: 'blocker' | 'major' | 'minor' | 'nit'; + file: string; + line?: number; + claim: string; + evidence: string; +} + +/** + * What a decision model answers, scripted: a choice and its confidence, or + * `unavailable` — no decision model bound, a provider refusing — which fails + * the decision step. + */ +type Choice = + { value: Route; confidence: number } | 'unavailable' | 'refused'; + +/** A choice answered as `ai.decision.decide` returns it, decided by the question's own bar. */ +function choiceOutput( + name: string, + minConfidence: number, + choice: { value: string; confidence: number }, +): Record { + return { + answers: { + [name]: { + type: 'choice', + value: choice.value, + decidedValue: choice.confidence >= minConfidence ? choice.value : null, + confidence: choice.confidence, + probabilities: { [choice.value]: choice.confidence }, + decided: choice.confidence >= minConfidence, + }, + }, + model: 'jev-1.13.0', + usage: { promptTokens: 1, completionTokens: 0, totalTokens: 1 }, + latencyMs: 40, + }; +} + type Scan = 'clean' | 'finding' | 'unscanned' | 'allowed'; /** What `host.commit.scan` returns for each scan, before the task's own projection. */ @@ -835,7 +907,6 @@ function checkResult(check: Check): Record { tail: '', summary: '`hb_app` declares no checks, so none ran.', clearedSha: HEAD, - clearedOrRetriableSha: HEAD, }); case 'passed': return HostCommitCheckOutputSchema.parse({ @@ -847,7 +918,6 @@ function checkResult(check: Check): Record { tail: 'ok prettier (3 files) (1.2 s)\n', summary: '`node scripts/verify-commit.mjs` passed.', clearedSha: HEAD, - clearedOrRetriableSha: HEAD, receipt: CHECK_RECEIPT.passed, }); case 'failed': @@ -872,7 +942,6 @@ function checkResult(check: Check): Record { summary: '`node scripts/verify-commit.mjs` exited 1, and nothing failed but 1 test running out of time.', testTimeouts: [TIMED_OUT_TEST], - clearedOrRetriableSha: HEAD, receipt: CHECK_RECEIPT.failed, }); case 'timed-out': @@ -926,6 +995,10 @@ interface Scenario { inputs?: Record; /** The composed text holds what looks like a secret, so the composition is refused. */ describeRefused?: boolean; + /** The findings a completed review returns; absent is none. */ + findings?: readonly Finding[]; + /** What the decision on a fix round answers; absent is `unavailable`. */ + afterReview?: Choice; } interface Outcome { @@ -946,8 +1019,9 @@ const SLICE_TITLE = 'The slice'; /** * What `host.commit.describe` returns, standing in for the composition the - * host executor's own tests prove: the title given or the slice's, and a body - * naming the range it was composed from, so a test can tell whose it is. + * host executor's own tests prove: the title given or the slice's, a body + * naming the range it was composed from, so a test can tell whose it is, and + * the review record the executor computes, through the same function. */ function describedPullRequest(input: Record): Record { const { title, summary, range } = input; @@ -955,6 +1029,7 @@ function describedPullRequest(input: Record): Record { expect(outcome.ran).toEqual([ 'commit', 'check-commit', - 'read-repository', 'scan-commit', + 'read-repository', 'read-push-approval', 'review-commit', 'describe-pr', @@ -1987,7 +2103,7 @@ describe("Publish Local Changes — the folder's checks run before anything leav expect(outcome.pushed).toBe(true); }); - it('fails at the check, rather than pausing on a resolution that could clear it', () => { + it('records every ending of the first run, so the run itself chooses rerun or stop — and fails at the second', () => { for (const taskId of ['check-commit', 'recheck-commit']) { const check = taskOrThrow(taskId); expect(check.maxAttempts, taskId).toBe(1); @@ -1996,13 +2112,10 @@ describe("Publish Local Changes — the folder's checks run before anything leav } const projects = (taskId: string, ending: Check): boolean => projectTaskOutput(taskOrThrow(taskId).outputProjection ?? {}, checkResult(ending), null).ok; - for (const ending of ['failed', 'timed-out'] as const) { - expect(projects('check-commit', ending), ending).toBe(false); - } - for (const ending of ['passed', 'skipped', 'test-timeouts'] as const) { + for (const ending of ['passed', 'skipped', 'failed', 'timed-out', 'test-timeouts'] as const) { expect(projects('check-commit', ending), ending).toBe(true); } - // The second run is the last: tests running out of time fail it as anything else does. + // The second run is the last: anything but passing fails it, and the run. for (const ending of ['failed', 'timed-out', 'test-timeouts'] as const) { expect(projects('recheck-commit', ending), ending).toBe(false); } @@ -2095,10 +2208,19 @@ describe("Publish Local Changes — the folder's checks run before anything leav }); } - it('says on the second run’s failure that it was the second', () => { + it('says on the second run’s failure that it was the second, and why there was one — never that the first failed only on time', () => { const failure = taskOrThrow('recheck-commit').failureInstruction ?? ''; expect(failure).toContain('Nothing was scanned, reviewed or pushed'); + expect(failure).toContain('because tests ran out of their time on the first'); expect(failure).toContain('this second run failed as well'); + expect(failure).not.toMatch(/failed only/); + }); + + it('is the only route to a second run: no decision sends a run there', () => { + expect(taskOrThrow('recheck-commit').when).toEqual({ + expression: 'tasks.check-commit.output.testTimeouts != null', + onMissingRef: 'skip', + }); }); }); @@ -2151,15 +2273,26 @@ describe("Publish Local Changes — the folder's checks run before anything leav const failed = checkResult('failed'); expect(String(failed['summary'])).toContain(CHECK_TAIL); expect(failed['outputRef']).toBeDefined(); - const failure = taskOrThrow('check-commit').failureInstruction ?? ''; + const outcome = publish({ + pushApproval: 'never', + review: 'approve', + decision: 'approved', + check: 'failed', + }); + expect(outcome.inputs.get('checks-failed')).toEqual({ reason: failed['summary'] }); + const failure = taskOrThrow('checks-failed').failureInstruction ?? ''; expect(failure).toContain('Nothing was scanned, reviewed or pushed'); expect(failure).toContain('`checksTimeoutMs`'); }); it("says in its description that the folder's checks run first, and that the pull request's remain the proof", () => { expect(PUBLISH_LOCAL_CHANGES.description).toContain( - 'run in a checkout of the commit before the scan, and a failure stops the run with nothing scanned, reviewed or pushed', + 'run in a checkout of the commit before the scan, and nothing is scanned, reviewed or pushed until they pass', + ); + expect(PUBLISH_LOCAL_CHANGES.description).toContain( + 'any other failure stops the run with the end of what they printed.', ); + expect(PUBLISH_LOCAL_CHANGES.description).toContain('No decision clears or holds a push.'); expect(PUBLISH_LOCAL_CHANGES.description).toContain('the push needs their passing receipt.'); expect(PUBLISH_LOCAL_CHANGES.description).toContain('A folder that declares none runs none.'); expect(PUBLISH_LOCAL_CHANGES.description).toContain( @@ -2251,6 +2384,7 @@ describe('Publish Local Changes — the pull request describes the branch as a w verdict: 'approve', outcome: 'completed', reviewSummary: 'Read.', + findings: [], }); expect(described.check).toMatchObject({ passed: true, @@ -2284,7 +2418,12 @@ describe('Publish Local Changes — the pull request describes the branch as a w const failed = describedWith( publish({ pushApproval: 'unless-unreviewed', review: 'failed', decision: 'approved' }), ); - expect(failed.review).toEqual({ verdict: null, outcome: 'failed', reviewSummary: null }); + expect(failed.review).toEqual({ + verdict: null, + outcome: 'failed', + reviewSummary: null, + findings: null, + }); }); it("passes a person's own title and summary to the composition, where they stand", () => { @@ -2337,6 +2476,9 @@ describe('Publish Local Changes — the pull request describes the branch as a w expect(outcome.state).toEqual({ prUrl: 'https://github.com/aflowai/aflow/pull/5', prNumber: 5, + findings: [], + findingsLeftOut: null, + afterReview: null, }); const titled = publish({ @@ -2381,3 +2523,360 @@ describe('Publish Local Changes — the pull request describes the branch as a w expect(runInputOrThrow('gaps').schema).toEqual({ ...toJsonSchemaSync(PullRequestGapsSchema) }); }); }); + +// ── A failed check stops the run: no decision is asked of it ─────────────── + +describe('Publish Local Changes — a failed check stops the run, and no decision is asked of it', () => { + /** Everything that only runs on a commit the checks cleared. */ + const CLEARED_ONLY = [ + 'scan-commit', + 'read-repository', + 'read-push-approval', + 'review-commit', + 'describe-pr', + 'approve-push', + 'push', + 'find-pr', + 'open-pr', + ]; + + function checkedWith(check: Check, extra: Partial = {}): Outcome { + return publish({ + pushApproval: 'never', + review: 'approve', + decision: 'approved', + check, + inputs: { owner: 'aflowai', repo: 'aflow' }, + ...extra, + }); + } + + it('asks a decision model nothing over the checks: the only decision reads the review', () => { + const deciders = wf.tasks.filter((t) => t.operation === 'ai.decision.decide'); + expect(deciders.map((t) => t.taskId)).toEqual(['decide-after-review']); + for (const task of wf.tasks) { + expect(JSON.stringify(task), task.taskId).not.toMatch(/decide-after-check|next_step/); + } + }); + + for (const check of ['failed', 'timed-out'] as const) { + it(`${check}: fails the publication at \`checks-failed\` with the checks' account as the reason, and nothing else runs`, () => { + const outcome = checkedWith(check); + expect(outcome.statuses.get('check-commit')).toBe('succeeded'); + expect(outcome.statuses.get('checks-failed')).toBe('failed'); + expect(outcome.inputs.get('checks-failed')).toEqual({ + reason: checkResult(check)['summary'], + }); + for (const id of [...CLEARED_ONLY, 'recheck-commit']) { + expect(outcome.ran, id).not.toContain(id); + } + expect(outcome.pushed).toBe(false); + expect(outcome.state['prUrl']).toBeUndefined(); + }); + } + + for (const check of ['passed', 'skipped', 'test-timeouts'] as const) { + it(`${check}: does not stop the run`, () => { + const outcome = checkedWith(check); + expect(outcome.ran).not.toContain('checks-failed'); + expect(outcome.pushed).toBe(true); + }); + } + + it('says the checks did not pass and what was left untouched, and steers to no remedy their account has not named', () => { + const stop = taskOrThrow('checks-failed'); + expect(stop.optional).not.toBe(true); + const failure = stop.failureInstruction ?? ''; + expect(failure).toContain("The folder's checks did not pass on this run's commit"); + expect(failure).toContain('Nothing was scanned, reviewed or pushed'); + expect(failure).toContain('the change, or the machine the checks ran on'); + // A fault on the machine is not the change's, so no fix to the change is prescribed. + expect(failure).not.toMatch(/[Cc]ommission/); + expect(failure).not.toBe(taskOrThrow('check-commit').failureInstruction); + }); + + it('says, where the check step itself breaks, that the checks could not run — and prescribes no fix to the change', () => { + // A run of the checks that did not pass succeeds and stops at `checks-failed`, so the first + // run's own failure is only ever the step breaking: the executor, the checkout, the fetch. + for (const check of ['failed', 'timed-out', 'test-timeouts'] as const) { + expect( + projectTaskOutput( + taskOrThrow('check-commit').outputProjection ?? {}, + checkResult(check), + null, + ).ok, + check, + ).toBe(true); + } + const failure = taskOrThrow('check-commit').failureInstruction ?? ''; + expect(failure).toContain("The folder's checks could not be run on this run's commit"); + expect(failure).toContain('Nothing was scanned, reviewed or pushed'); + expect(failure).toContain('Nothing says the change is at fault'); + expect(failure).not.toMatch(/[Cc]ommission/); + expect(failure).not.toContain('checks failed'); + }); + + it('projects what the stop, the rerun, the push and the composition read, and nothing else', () => { + expect(Object.keys(taskOrThrow('check-commit').outputProjection ?? {}).sort()).toEqual([ + 'passed', + 'receipt', + 'skipped', + 'summary', + 'tail', + 'testTimeouts', + ]); + }); +}); + +// ── No publication starts a publication, or a commission ────────────────── + +describe('Publish Local Changes — no publication starts a publication', () => { + /** What a task starts as a run of its own, by the catalog entry it names. */ + const startedBy = (task: WorkflowTask): string | undefined => { + if (task.operation !== 'workflow.run.start') return undefined; + const template = task.inputTemplate as { catalogId?: unknown; slug?: unknown } | undefined; + return String(template?.catalogId ?? template?.slug); + }; + + it('starts one run of its own, the review of its commit, and no other skill', () => { + const started = wf.tasks.flatMap((task) => { + const skill = startedBy(task as WorkflowTask); + return skill === undefined ? [] : [[task.taskId, skill]]; + }); + expect(started).toEqual([['review-commit', REVIEW_LOCAL_CHANGES.catalogId]]); + }); + + it('is started by no skill in the catalog, so no publication runs under another run', () => { + for (const skill of SKILL_CATALOG) { + for (const task of skill.bundle.workflow.tasks) { + const template = task.inputTemplate as { catalogId?: unknown; slug?: unknown } | undefined; + if (task.operation !== 'workflow.run.start') continue; + expect( + [template?.catalogId, template?.slug], + `${skill.catalogId} › ${task.taskId}`, + ).not.toContain(PUBLISH_LOCAL_CHANGES.catalogId); + } + } + }); + + it('commits once and pushes once: the fix round is the conversation’s, not a second commit or push of its own', () => { + for (const operation of ['host.file.patch', 'host.process.exec']) { + expect( + wf.tasks.filter((task) => task.operation === operation).map((task) => task.taskId), + operation, + ).toHaveLength(1); + } + }); +}); + +// ── What the run decides about a review's findings, recorded and never acted on + +describe("Publish Local Changes — the decision on a review's findings is recorded, never acted on", () => { + const LEAK: Finding = { + severity: 'major', + file: 'src/cache.ts', + line: 40, + claim: 'The cache is never invalidated after a write.', + evidence: + 'put() writes the store and leaves the map entry; the test reads only before the write.', + }; + const NAME: Finding = { + severity: 'nit', + file: 'src/cache.ts', + claim: 'The name says what it was.', + evidence: 'cacheOld', + }; + const RACE: Finding = { + severity: 'blocker', + file: 'src/lock.ts', + line: 12, + claim: 'Two writers can both take the lock.', + evidence: 'The check and the set are two calls.', + }; + const FINDINGS = [LEAK, NAME, RACE]; + /** An earlier round's finding the brief passed on, closed by a commit of the branch. */ + const EARLIER = { + severity: 'minor', + file: 'src/cache.ts', + line: 12, + claim: 'The size is read twice.', + state: 'closed', + } as const; + /** An earlier round's finding the branch still leaves as the review found it. */ + const EARLIER_OPEN = { + severity: 'nit', + file: 'src/cache.ts', + claim: 'The constant is named for the old default.', + state: 'open', + } as const; + const FIX = { value: 'fix_before_merge', confidence: 0.95 } as const; + + /** Every answer the decision can give, or fail to, and what the run records for it. */ + const ANSWERS: Array<[string, Choice, string]> = [ + ['fix_before_merge', FIX, 'fix_before_merge'], + ['publish_as_is', { value: 'publish_as_is', confidence: 0.95 }, 'publish_as_is'], + [ + 'an answer below the bar', + { value: 'fix_before_merge', confidence: AFTER_REVIEW_QUESTION.minConfidence - 0.01 }, + 'undecided', + ], + ['no decision at all', 'unavailable', 'undecided'], + ['a refusal of the question', 'refused', 'undecided'], + ]; + + function reviewedWith(extra: Partial): Outcome { + return publish({ + pushApproval: 'unless-unreviewed', + review: 'request_changes', + decision: 'approved', + findings: FINDINGS, + inputs: { owner: 'aflowai', repo: 'aflow', findings: [EARLIER, EARLIER_OPEN] }, + ...extra, + }); + } + + it('asks one choice over every finding the review returned, and judges none of them apart', () => { + const outcome = reviewedWith({ afterReview: FIX }); + const asked = outcome.inputs.get('decide-after-review') as { + state: { findings: unknown }; + questions: Record; + each?: unknown; + }; + expect(asked.state.findings).toEqual(FINDINGS); + expect(Object.keys(asked.questions)).toEqual(['after_review']); + expect(asked.each).toBeUndefined(); + expect(AFTER_REVIEW_QUESTION.minConfidence).toBe(0.8); + }); + + for (const [label, answer, recorded] of ANSWERS) { + it(`${label}: records afterReview as \`${recorded}\` on the run and in the pull request's record`, () => { + const outcome = reviewedWith({ afterReview: answer }); + // Only a decision that could not be made fails its step; a refusal is an answer. + expect(outcome.statuses.get('decide-after-review')).toBe( + answer === 'unavailable' ? 'failed' : 'succeeded', + ); + expect(outcome.state['afterReview']).toBe(recorded); + expect(outcome.outputs.get('describe-pr')?.['afterReview']).toBe(recorded); + }); + } + + it('acts on no answer: every answer runs the same tasks, pushes the same way and opens the same pull request', () => { + const runs = ANSWERS.map(([, answer]) => reviewedWith({ afterReview: answer })); + const [first, ...rest] = runs; + for (const outcome of rest) { + expect(outcome.ran).toEqual(first?.ran); + expect(outcome.asked).toBe(first?.asked); + expect(outcome.pushed).toBe(first?.pushed); + expect(outcome.state['prUrl']).toBe(first?.state['prUrl']); + } + // And the same as a review with no findings at all, where nothing is decided. + const undecided = reviewedWith({ findings: [] }); + expect(undecided.ran).toEqual(first?.ran.filter((id) => id !== 'decide-after-review')); + expect(undecided.pushed).toBe(first?.pushed); + }); + + it('is read by the pull request’s composition alone, and by no predicate', () => { + const readers = wf.tasks.filter((task) => + JSON.stringify([task.when, task.inputBindings, task.actionPreview]).includes( + 'decide-after-review', + ), + ); + expect(readers.map((task) => task.taskId)).toEqual(['describe-pr']); + expect(JSON.stringify(taskOrThrow('describe-pr').when)).not.toContain('decide-after-review'); + expect(wf.tasks.some((task) => JSON.stringify(task.when ?? null).includes('afterReview'))).toBe( + false, + ); + }); + + it("hands on this review's findings as the open ones, replacing the earlier rounds' — a round's list never grows", () => { + const outcome = reviewedWith({ afterReview: FIX }); + const described = HostCommitDescribeInputSchema.parse(outcome.inputs.get('describe-pr')); + expect(described.review?.findings?.map((f) => f.claim)).toEqual(FINDINGS.map((f) => f.claim)); + const record = outcome.outputs.get('describe-pr')?.['findings']; + // The most severe first, so a bounded list keeps what matters most. + expect(PullRequestFindingsSchema.parse(record)).toEqual([ + ...[RACE, LEAK, NAME].map(({ severity, file, line, claim }) => ({ + severity, + file, + ...(line === undefined ? {} : { line }), + claim, + state: 'open', + })), + ]); + expect(outcome.state['findings']).toEqual(record); + }); + + it('records no decision where the review returned no findings or did not finish, since none was asked', () => { + for (const review of ['approve', 'failed', 'not-started'] as const) { + const outcome = reviewedWith({ review, findings: review === 'approve' ? [] : FINDINGS }); + expect(outcome.ran, review).not.toContain('decide-after-review'); + expect(outcome.state['afterReview'], review).toBeNull(); + } + }); + + it('names its review run, whose findings a fix round is briefed from, every one whole', () => { + const outcome = reviewedWith({ afterReview: FIX }); + expect(outcome.state['reviewRunId']).toBe('review-run'); + expect(outcome.outputs.get('review-commit')?.['findings']).toEqual(FINDINGS); + }); + + it('says how many findings the bounded hand-on left out, and leaves it null where none', () => { + const many: Finding[] = Array.from({ length: HANDED_ON_FINDINGS_MAX_COUNT + 3 }, (_, i) => ({ + severity: 'minor', + file: `src/f${String(i)}.ts`, + claim: `Finding ${String(i)}.`, + evidence: 'Shown.', + })); + const bounded = reviewedWith({ findings: many, afterReview: FIX }); + expect(bounded.state['findings']).toHaveLength(HANDED_ON_FINDINGS_MAX_COUNT); + expect(bounded.state['findingsLeftOut']).toBe(3); + // The review run itself still holds every one of them. + expect(bounded.outputs.get('review-commit')?.['findings']).toHaveLength(many.length); + expect(reviewedWith({ afterReview: FIX }).state['findingsLeftOut']).toBeNull(); + }); + + it('hands on none where its review finished finding nothing, and the open ones it was given where none finished', () => { + expect(reviewedWith({ review: 'approve', findings: [] }).state['findings']).toEqual([]); + for (const review of ['failed', 'not-started'] as const) { + expect(reviewedWith({ review }).state['findings'], review).toEqual([EARLIER_OPEN]); + } + }); + + it('offers only the two answers that route differently — unsure is the undecided record, not an option', () => { + expect(Object.keys(AFTER_REVIEW_QUESTION.options)).toEqual([ + 'fix_before_merge', + 'publish_as_is', + ]); + expect(AFTER_REVIEW_ROUTES).toEqual(['fix_before_merge', 'publish_as_is']); + }); + + // A decline pushes nothing, whatever the posture and however it is recorded: + // skipped, as the harness does, or as the answer itself, which leaves the + // push to refuse on its own predicate. + for (const declineAs of ['skip', 'answer'] as const) { + it(`a decline recorded as ${declineAs === 'skip' ? 'a skip' : 'an answer'} pushes nothing, whatever the decision`, () => { + const outcome = reviewedWith({ decision: 'declined', declineAs, afterReview: FIX }); + expect(outcome.asked).toBe(true); + expect(outcome.pushed).toBe(false); + expect(outcome.state['prUrl']).toBeUndefined(); + }); + } + + it('leaves the push gate, the scan and the posture exactly as they were', () => { + // 316 D9: a decision reads text it can be steered by, so it is never a gate. + const push = taskOrThrow('push'); + expect(push.when).toEqual({ + anyOf: [ + "tasks.approve-push.output.decision == 'approved'", + "tasks.read-push-approval.output.branchPolicy.pushApproval == 'never'", + "tasks.review-commit.output.verdict == 'approve'", + ], + onMissingRef: 'skip', + }); + const reads = (task: WorkflowTask) => + JSON.stringify([task.when, task.inputBindings, task.dependsOn]); + for (const gate of ['push', 'approve-push', 'read-push-approval', 'scan-commit']) { + expect(reads(taskOrThrow(gate) as WorkflowTask), gate).not.toMatch(/decide-/); + } + }); +}); diff --git a/packages/platform-artifacts/src/skillCatalog/publishLocalChanges.ts b/packages/platform-artifacts/src/skillCatalog/publishLocalChanges.ts index dc5b43cb..e6a19554 100644 --- a/packages/platform-artifacts/src/skillCatalog/publishLocalChanges.ts +++ b/packages/platform-artifacts/src/skillCatalog/publishLocalChanges.ts @@ -9,7 +9,11 @@ import { type SkillCatalogEntry, toJsonSchemaSync, } from '@aflow/schemas'; -import { REVIEW_LOCAL_CHANGES } from './reviewLocalChanges.js'; +import { + AFTER_REVIEW_QUESTION, + AFTER_REVIEW_QUESTION_NAME, +} from './publishLocalChangesDecisions.js'; +import { REVIEW_FINDING_SCHEMA, REVIEW_LOCAL_CHANGES } from './reviewLocalChanges.js'; /** * The push argv, pinned by the skill rather than bound as a whole. Only the @@ -61,6 +65,16 @@ const REVIEW_SKILL = { slug: REVIEW_LOCAL_CHANGES.bundle.workflow.slug, }; +/** + * Where the push may follow the checks: they passed on the first run, or on + * the second, where the first lost only tests to their time. Read by every + * task that only runs on a commit the checks cleared. + */ +const CHECKS_CLEARED = { + anyOf: ['tasks.check-commit.output.passed == true', 'tasks.recheck-commit.output.passed == true'], + onMissingRef: 'skip' as const, +}; + /** The one posture under which the publication reviews its commit to decide whether to ask. */ const REVIEW_GATED_POSTURE: HostPushApproval = 'unless-unreviewed'; @@ -144,6 +158,24 @@ const GAPS_SCHEMA = { ...toJsonSchemaSync(PullRequestGapsSchema) }; const CHECK_FAILED = "The folder's checks failed on this run's commit: their account is above, with the end of what they printed, and the whole of it is stored with this failure. Nothing was scanned, reviewed or pushed, and the commit is still on its branch in the folder. Commission the fix onto this branch — nothing of it has left the machine — and publish again. Where they ran out of the folder's time rather than failed, the folder's `checksTimeoutMs` is the operator's to raise on the machine."; +/** + * The first run's own failure, which is never the checks failing — a run of + * them that did not pass succeeds, and `checks-failed` stops on it — but the + * step that runs them breaking before they reported. Nothing has said the + * change is at fault, so it prescribes no fix to it. + */ +const CHECKS_COULD_NOT_RUN = + "The folder's checks could not be run on this run's commit: the step that runs them stopped before they reported, and its own error is above — the executor, the checkout of the commit, or the fetch of `origin`'s base. Nothing was scanned, reviewed or pushed, and the commit is still on its branch in the folder. Nothing says the change is at fault: put right what the error names, on the machine or in the connected folder, then publish the branch as it stands."; + +/** + * The stop on a first run of the checks that did not pass for any reason but + * tests running out of their time. What they printed may name a fault in the + * change or one in the machine they ran on, and nothing here has told the + * two apart, so it says which happened and leaves the remedy to that account. + */ +const CHECKS_STOPPED = + "The folder's checks did not pass on this run's commit, and the publication stops on that: their account is the reason above, with the end of what they printed. Nothing was scanned, reviewed or pushed, and the commit is still on its branch in the folder. What they printed says what failed — the change, or the machine the checks ran on, such as a missing tool or a service they reach — and so what puts it right. Where they ran out of the folder's time, its `checksTimeoutMs` is the operator's to raise on the machine."; + /** Both runs of the checks take the same commit, measured against the same base. */ const CHECK_INPUT_BINDINGS = { bindingId: { kind: 'run_input' as const, path: 'bindingId' }, @@ -173,29 +205,29 @@ const CHECK_INPUT_TEMPLATE = { const PUBLISH_LOCAL_CHANGES: SkillCatalogEntry = { catalogId: 'publish-local-changes', - version: 24, + version: 31, name: 'Publish Local Changes', tagline: 'Commit a patch onto a branch of a connected repository, then push it and open the pull request — asking the operator first unless the folder says otherwise.', description: `Fits a request to publish an existing patch — a commission's result, or a diff the operator hands over — onto a branch of a connected repository and into a pull request. The working tree is never touched, and nothing leaves the machine until the push is cleared. -**Not for a folder whose machine block shows no publish prefix**: its push is refused. Say so, ask for the folder to be reconnected with one, and stop rather than commit what cannot be published. +**Not for a folder whose machine block shows no publish prefix**: its push is refused. Say so, ask for it to be reconnected with one, and stop. -**What it needs**: the connected folder; the change — for a commission's work, the \`patchRef\` its result reports, passed as \`patchRef\`, or for a diff the operator hands over, that text as \`patch\`, one or the other — or neither, for a branch as it stands; a branch name under the folder's publish prefix; the commit message; the repository owner and name, from the folder's \`origin\` remote; and the base branch the pull request targets. Ask for what is missing; never invent it. +**What it needs**: the connected folder; the change — a commission's \`patchRef\` as its result reports it, or a diff the operator hands over as \`patch\`, one or the other — or neither, for a branch as it stands; a branch name under the folder's publish prefix; the commit message; the repository owner and name, from the folder's \`origin\` remote; and the base branch the pull request targets. Ask for what is missing; never invent it. **The pull request is composed from the whole branch** and rewritten on each push; pass earlier reviews' \`findings\` and the commissions' \`gaps\`, and \`title\` or \`summary\` only for a person's own words. -**Never pass a commission's \`patch\` text.** It is a copy for reading, cut short on a large change, and the run's inputs are capped at ${String(RUN_INPUTS_KB)} KB together. \`patchRef\` names the whole diff at any size. +**Never pass a commission's \`patch\` text.** It is a copy for reading, cut short on a large change; \`patchRef\` names the whole diff at any size. -**When it asks before pushing**: the folder's push approval decides, and the machine block shows it as \`pushApproval\`. \`always\` asks before every push; \`never\` pushes without asking. \`unless-unreviewed\`: the run has Local Code Review read everything the push would add and pushes without asking only when it returns \`approve\` — on any other verdict, or a review that did not finish or could not start, it waits for the operator. The review is the catalog's as installed; an edited copy is refused, and the run asks. A folder that names no posture is \`${HOST_PUSH_APPROVAL_DEFAULT}\`, the default. Whatever the posture, the run scans every line the push would add, the headers and message of every commit it carries, and the pull request's title and summary for secrets before any of this, and a finding stops it with nothing pushed — which is what lets a review stand in for the operator. Only a range the scan cleared can go without asking: where a file was not read whole, or a line that looks like a secret ends in an \`aflow-scan: allow\` comment, no review runs and the run waits for the operator whatever the posture, \`never\` included, naming each and why. The comment never clears a line — whoever wrote the change could have written it; it turns a stop into a question. Once the run asks, the operator's answer decides: a declined push pushes nothing, whatever the posture. +**When it asks before pushing**: the folder's push approval, \`pushApproval\` in the machine block, decides. \`always\` asks before every push; \`never\` pushes without asking. \`unless-unreviewed\`: the run has Local Code Review read everything the push would add and pushes without asking only when it returns \`approve\` — otherwise, a review that did not finish or start included, it waits for the operator. The review is the catalog's as installed; an edited copy is refused, and the run asks. A folder that names no posture is \`${HOST_PUSH_APPROVAL_DEFAULT}\`, the default. Whatever the posture, the run scans every line the push would add, the headers and message of every commit it carries, and the pull request's title and summary for secrets before any of this, and a finding stops it with nothing pushed — which is what lets a review stand in for the operator. Only a range the scan cleared can go without asking: where a file was not read whole, or a line that looks like a secret ends in an \`aflow-scan: allow\` comment, no review runs and the run waits for the operator whatever the posture, \`never\` included. The comment never clears a line — whoever wrote the change could have written it; it turns a stop into a question. Once the run asks, the operator's answer decides: a declined push pushes nothing, whatever the posture. -**Its checks first**: the folder's \`checks\`, shown in the machine block, run in a checkout of the commit before the scan, and a failure stops the run with nothing scanned, reviewed or pushed; the push needs their passing receipt. A folder that declares none runs none. The pull request's own checks remain the proof. +**Its checks first**: the folder's \`checks\`, shown in the machine block, run in a checkout of the commit before the scan, and nothing is scanned, reviewed or pushed until they pass; the push needs their passing receipt. A folder that declares none runs none. The pull request's own checks remain the proof. Test timeouts alone rerun once; any other failure stops the run with the end of what they printed. A decision on review findings is recorded, never acted on. No decision clears or holds a push. **On an existing branch**: a fix commissioned from a branch (\`base: \`) is published onto it by naming it as \`branch\` and passing the commission's \`baseSha\` as \`baseSha\`; a branch is reused only that way, and a fresh change takes a fresh branch. A fix to a branch \`main\` has moved past is commissioned with \`mergeFrom: origin/\` as well and published with the sha the commission reported in \`merge.from\` as \`mergeFrom\`: the branch then carries one merge commit holding the fix. A commission that reported a \`merge\` and no \`patchRef\` changed nothing beyond the merge: it is published with \`mergeFrom\` and neither \`patchRef\` nor \`patch\`. **A branch as it stands**: a commit already made whose push did not land is published with \`branch\`, its head as \`baseSha\`, and no \`patchRef\`, \`patch\`, \`mergeFrom\` or \`commitMessage\`: nothing is committed, and the head is checked, scanned, reviewed and pushed. -**With the result**: report the pull request link, and whether the push was approved by the operator or cleared by the folder's push approval — and, where the run reviewed its commit, the verdict. A branch that already has an open pull request into the base gets no second one: the run reports that one. Where approval was declined, report that nothing was pushed and the commit stays on the machine to publish later. Where the checks failed, nothing left the machine: report the end of what they printed. Where the scan found what looks like a secret, the run failed with nothing pushed: report the files, commit headers and messages, title or summary, lines and rules it names — never ask for or repeat the value — and say the secret has to come out and the change be commissioned again onto a fresh branch, this one holding that commit. A refused push leaves the commit on its branch: report its message and the remedy the run names. Where the push failed, git's own message says why; a branch on \`origin\` that moved on is never overwritten.`, +**With the result**: report the pull request link, and whether the push was approved by the operator or cleared by the folder's push approval — and, where the run reviewed its commit, the verdict. A branch that already has an open pull request into the base gets no second one: the run reports that one. Where approval was declined, report that nothing was pushed and the commit stays on the machine to publish later. Where the checks failed, nothing left the machine: report the end of what they printed. On \`afterReview\` \`fix_before_merge\`, brief the fix from the review run; publish again with \`findings\`. Where the scan found what looks like a secret, the run failed with nothing pushed: report the files, commit headers and messages, title or summary, lines and rules it names — never ask for or repeat the value — and say the secret has to come out and the change go again on a fresh branch, this one holding that commit. A refused push leaves the commit on its branch: report its message and the remedy the run names. Where the push failed, git's own message says why.`, tags: ['coding', 'publish', 'git', 'local', 'developer-tools'], capabilityHints: [ { @@ -300,7 +332,7 @@ const PUBLISH_LOCAL_CHANGES: SkillCatalogEntry = { id: 'findings', required: false, description: - "The findings of the review rounds before this publication — each Local Code Review whose findings a commission on this branch was briefed to fix — as the review named them, each marked `closed` where a commit fixed it or `open` where it was left. This run's own review is added as it returns. Omit it where no review ran before.", + "The findings of the review rounds before this publication — each Local Code Review whose findings a commission on this branch was briefed to fix — as the review named them, each marked `closed` where a commit fixed it or `open` where it was left: what this run's pull request records of the rounds before it. Where this run's own review finishes, its findings replace these in what the run hands on. Omit it where no review ran before.", schema: FINDINGS_SCHEMA, }, { @@ -472,7 +504,7 @@ const PUBLISH_LOCAL_CHANGES: SkillCatalogEntry = { taskId: 'check-commit', name: "Run the folder's checks on the commit", goal: "Run the checks the connected folder declares — the operator's own command, from the repository root — in a detached checkout of this run's commit, measured against where `origin`'s base branch stands, before anything is scanned, reviewed or pushed. A folder that declares none runs none, and the result says so.", - failureInstruction: CHECK_FAILED, + failureInstruction: CHECKS_COULD_NOT_RUN, type: 'operation' as const, operation: 'host.commit.check', dependsOn: ['commit'], @@ -480,20 +512,16 @@ const PUBLISH_LOCAL_CHANGES: SkillCatalogEntry = { expression: "tasks.commit.output.state == 'applied'", onMissingRef: 'skip' as const, }, - // `clearedOrRetriableSha` exists only where the checks passed, none - // are declared, or tests running out of time alone failed them, so - // any other failure fails the projection; with one attempt and no - // output contract that fails the run here, the check's own summary - // — which carries the tail — leading the message. A run lost to - // time alone succeeds with `passed: false` and its `testTimeouts`, - // which is the record `recheck-commit` runs on. + // A run that did not pass succeeds with `passed: false` and its + // account: `recheck-commit` runs the checks again where only tests + // ran out of their time, and `checks-failed` stops the run on + // anything else. Nothing after the checks runs unless they cleared. retryability: 'safe' as const, maxAttempts: 1, inputBindings: CHECK_INPUT_BINDINGS, context: CHECK_CONTEXT, inputTemplate: CHECK_INPUT_TEMPLATE, outputProjection: { - clearedOrRetriableSha: { path: 'clearedOrRetriableSha', onMissing: 'error' as const }, passed: { path: 'passed', onMissing: 'error' as const }, skipped: { path: 'skipped', onMissing: 'null' as const }, testTimeouts: { path: 'testTimeouts', onMissing: 'null' as const }, @@ -504,18 +532,51 @@ const PUBLISH_LOCAL_CHANGES: SkillCatalogEntry = { }, }, + { + taskId: 'checks-failed', + name: 'Stop on the failed checks', + goal: "Fail the publication with the checks' own account where their first run did not pass for any reason but tests running out of their time, so whoever started it is woken with what they printed.", + failureInstruction: CHECKS_STOPPED, + type: 'operation' as const, + operation: 'workflow.run.fail', + dependsOn: ['check-commit'], + when: { + allOf: [ + 'tasks.check-commit.output.passed == false', + 'tasks.check-commit.output.testTimeouts == null', + ], + onMissingRef: 'skip' as const, + }, + retryability: 'safe' as const, + maxAttempts: 1, + inputBindings: { + summary: { kind: 'task_output' as const, taskId: 'check-commit', path: 'summary' }, + }, + context: { + strategy: 'scoped' as const, + contextPolicy: 'auto-optimize' as const, + learnings: 'none' as const, + capabilities: { + operations: ['workflow.run.fail'], + integrations: [], + }, + }, + inputTemplate: { + reason: { $bind: 'summary' }, + }, + }, + { taskId: 'recheck-commit', name: "Run the folder's checks again, after tests ran out of time", - goal: "Run the folder's checks once more on this run's commit, where their first run failed only because tests ran out of their time — `testTimeouts` on the first run names them — which a machine under load does to a passing commit. This run's result is the one the push carries; a failure here fails the publication.", - failureInstruction: `${CHECK_FAILED} Their first run failed only because tests ran out of their time, and this second run failed as well.`, + goal: "Run the folder's checks once more on this run's commit, where nothing failed on their first run but tests running out of their time — `testTimeouts` names them — which a machine under load does to a passing commit. This run's result is the one the push carries; a failure here fails the publication.", + failureInstruction: `${CHECK_FAILED} They were run a second time because tests ran out of their time on the first, and this second run failed as well.`, type: 'operation' as const, operation: 'host.commit.check', dependsOn: ['commit', 'check-commit'], - // The first run succeeds without passing only where tests running - // out of time alone failed it. + // `testTimeouts` is present only where nothing else failed. when: { - expression: 'tasks.check-commit.output.passed == false', + expression: 'tasks.check-commit.output.testTimeouts != null', onMissingRef: 'skip' as const, }, // Its own task rather than a second attempt of the first, which a @@ -545,10 +606,7 @@ const PUBLISH_LOCAL_CHANGES: SkillCatalogEntry = { // After the checks, and their second run where there is one: a // commit they failed goes no further. dependsOn: ['commit', 'check-commit', 'recheck-commit'], - when: { - expression: "tasks.commit.output.state == 'applied'", - onMissingRef: 'skip' as const, - }, + when: CHECKS_CLEARED, // `unflaggedRange` exists only when no rule matched, so a finding // fails the projection; with one attempt and no output contract that // fails the run here instead of parking it on a resolution that @@ -599,11 +657,8 @@ const PUBLISH_LOCAL_CHANGES: SkillCatalogEntry = { "The GitHub credential bound to this space cannot see `owner`/`repo` — GitHub answers 404 for a private repository the token has no access to — and that credential's repository access is the thing to check. Nothing has been pushed, and the commit is still on its branch.", type: 'operation' as const, operation: 'api.http.call', - dependsOn: ['commit'], - when: { - expression: "tasks.commit.output.state == 'applied'", - onMissingRef: 'skip' as const, - }, + dependsOn: ['commit', 'check-commit', 'recheck-commit'], + when: CHECKS_CLEARED, // One attempt and no output contract: a repository the credential // cannot see fails the run here, rather than parking it on a // resolution nobody can supply. @@ -726,13 +781,19 @@ const PUBLISH_LOCAL_CHANGES: SkillCatalogEntry = { outputProjection: { verdict: { path: 'result.output.verdict', onMissing: 'null' as const }, reviewSummary: { path: 'result.output.reviewSummary', onMissing: 'null' as const }, + findings: { path: 'result.output.findings', onMissing: 'null' as const }, outcome: { path: 'outcome', onMissing: 'error' as const }, reviewRunId: { path: 'runId', onMissing: 'error' as const }, }, + // The review run holds every finding whole, which is what a fix + // round is briefed from; the run names it so whoever starts one can read it. + promoteOutputs: [ + { kind: 'output_path' as const, path: 'reviewRunId', toState: 'reviewRunId' }, + ], outputContract: { schema: { type: 'object', - required: ['verdict', 'reviewSummary', 'outcome', 'reviewRunId'], + required: ['verdict', 'reviewSummary', 'findings', 'outcome', 'reviewRunId'], additionalProperties: false, properties: { verdict: { @@ -745,6 +806,12 @@ const PUBLISH_LOCAL_CHANGES: SkillCatalogEntry = { description: "The review's own account — every finding by severity and `file:line`, and the checks it ran — which the pull request carries; null when it did not complete.", }, + findings: { + type: ['array', 'null'], + items: REVIEW_FINDING_SCHEMA, + description: + 'Every finding as the review returned it — severity, file, line, claim, evidence, suggestion — which the decision after the review weighs and the pull request records; null when the review did not complete.', + }, outcome: { type: 'string', description: @@ -756,6 +823,71 @@ const PUBLISH_LOCAL_CHANGES: SkillCatalogEntry = { }, }, + { + taskId: 'decide-after-review', + name: 'Decide whether the findings are fixed before merging', + goal: "Read the review's findings and choose whether this branch takes a fix round before it merges or merges as it is, for the pull request to record. An answer too unsure to stand is recorded as undecided.", + type: 'operation' as const, + operation: 'ai.decision.decide', + dependsOn: ['review-commit'], + // A review with no findings, or one that did not finish, has none to weigh. + when: { + expression: 'tasks.review-commit.output.findings[0] != null', + onMissingRef: 'skip' as const, + }, + // Only `describe-pr` reads the answer, to record it; a decision that + // could not be made is recorded as undecided, and the run goes on. + optional: true, + retryability: 'safe' as const, + maxAttempts: 2, + inputBindings: { + verdict: { kind: 'task_output' as const, taskId: 'review-commit', path: 'verdict' }, + findings: { kind: 'task_output' as const, taskId: 'review-commit', path: 'findings' }, + }, + context: { + strategy: 'scoped' as const, + contextPolicy: 'auto-optimize' as const, + learnings: 'none' as const, + capabilities: { + operations: ['ai.decision.decide'], + integrations: [], + }, + }, + inputTemplate: { + state: { + verdict: { $bind: 'verdict' }, + findings: { $bind: 'findings' }, + }, + questions: { [AFTER_REVIEW_QUESTION_NAME]: AFTER_REVIEW_QUESTION }, + }, + // No model is named: the space's decision model answers. The 0.80 bar + // was measured on Jev alone (Plan 316 §7), and holds for another + // decider only once that decider is measured on the same cases + // (`yarn decisions:measure-loop`); the bar moves only on such a measurement. + // `afterReview` is the answer that cleared the bar, null where none + // did; `chosen` is what it would have chosen, and with `confidence` + // null where the model declined the question — a refusal is an + // answer, never decided, not a failed step. + outputProjection: { + afterReview: { + path: `answers.${AFTER_REVIEW_QUESTION_NAME}.decidedValue`, + onMissing: 'error' as const, + }, + chosen: { + path: `answers.${AFTER_REVIEW_QUESTION_NAME}.value`, + onMissing: 'null' as const, + }, + decided: { + path: `answers.${AFTER_REVIEW_QUESTION_NAME}.decided`, + onMissing: 'error' as const, + }, + confidence: { + path: `answers.${AFTER_REVIEW_QUESTION_NAME}.confidence`, + onMissing: 'null' as const, + }, + }, + }, + { taskId: 'describe-pr', name: 'Compose the pull request from the branch', @@ -764,16 +896,27 @@ const PUBLISH_LOCAL_CHANGES: SkillCatalogEntry = { "The pull request composed from the branch holds what looks like a secret, named above by where it is in the title or the body, line and rule — never by its value — and nothing was pushed: the commit is still on its branch in the folder. Where the line came from `findings` or `gaps`, or from a title or summary passed in, publish this branch as it stands without it. Where it came from the review's summary or the end of what the checks printed, that text is the review's or the checks' own: take the secret out of the code or the environment they read, rotate it if it was real, and publish again. Where it came from a commit's message — a line marked allowed, which the scan let the operator read — that commit has to be rewritten before the branch is published.", type: 'operation' as const, operation: 'host.commit.describe', - dependsOn: ['commit', 'check-commit', 'recheck-commit', 'scan-commit', 'review-commit'], - when: { - expression: "tasks.commit.output.state == 'applied'", - onMissingRef: 'skip' as const, - }, + dependsOn: [ + 'commit', + 'check-commit', + 'recheck-commit', + 'scan-commit', + 'review-commit', + 'decide-after-review', + ], + when: CHECKS_CLEARED, retryability: 'safe' as const, maxAttempts: 1, // Whole outputs, each absent where its task did not run: a review - // that failed or was never started is reported as none finishing. + // that failed or was never started is reported as none finishing, + // and its findings, and the decision on them, are what the pull + // request records of it. inputBindings: { + afterReview: { + kind: 'task_output' as const, + taskId: 'decide-after-review', + path: 'afterReview', + }, bindingId: { kind: 'run_input' as const, path: 'bindingId' }, range: { kind: 'task_output' as const, taskId: 'commit', path: 'commit.pushRange' }, title: { kind: 'run_input' as const, path: 'title' }, @@ -803,11 +946,22 @@ const PUBLISH_LOCAL_CHANGES: SkillCatalogEntry = { check: { $bind: 'check' }, recheck: { $bind: 'recheck' }, gaps: { $bind: 'gaps' }, + afterReview: { $bind: 'afterReview' }, }, outputProjection: { title: { path: 'title', onMissing: 'error' as const }, body: { path: 'body', onMissing: 'error' as const }, + findings: { path: 'findings', onMissing: 'error' as const }, + // Absent where the hand-on left none out. + findingsLeftOut: { path: 'findingsLeftOut', onMissing: 'null' as const }, + // Absent where the review returned no findings, so none was decided. + afterReview: { path: 'afterReview', onMissing: 'null' as const }, }, + promoteOutputs: [ + { kind: 'output_path' as const, path: 'afterReview', toState: 'afterReview' }, + { kind: 'output_path' as const, path: 'findings', toState: 'findings' }, + { kind: 'output_path' as const, path: 'findingsLeftOut', toState: 'findingsLeftOut' }, + ], }, { @@ -1161,11 +1315,47 @@ const PUBLISH_LOCAL_CHANGES: SkillCatalogEntry = { immutable: false, writers: 'alternatives' as const, }, + { + variableId: 'afterReview', + name: 'After the review', + description: + "What the run decided about its review's findings, as the pull request records it: fix_before_merge, publish_as_is, or undecided where no answer cleared the bar or none could be made. Null where the review returned no findings, so none was decided. Nothing in the run acts on it: on fix_before_merge the fix round is the conversation's, or the shepherd's.", + required: false, + sensitive: false, + immutable: false, + }, + { + variableId: 'findings', + name: 'Findings', + description: + "The open findings of the latest review — this run's own review's where it finished, replacing the earlier rounds'; otherwise the open ones it was given — bounded to fit a later publication's inputs. That publication takes them as its `findings`, with each its fix closed marked closed, for its pull request to record. A fix round is briefed from the review run's own findings, never from these.", + required: false, + sensitive: false, + immutable: false, + }, + { + variableId: 'findingsLeftOut', + name: 'Findings left out', + description: + 'How many open findings `findings` left out to stay within its bound, the least severe of them; null where it left none out. The review run keeps every one.', + required: false, + sensitive: false, + immutable: false, + }, + { + variableId: 'reviewRunId', + name: 'Review run', + description: + "The run of this publication's own review, where one started: its output holds every finding whole, with its evidence and suggestion — what a fix round is briefed from.", + required: false, + sensitive: false, + immutable: false, + }, ], output: { primary: 'prUrl', guidance: - 'The run carries prUrl and prNumber once the branch has its pull request — opened by this run, or already open for a branch it appended to — and reporting the link is reporting the result. Absent, the branch was committed and not published: say it stayed on the machine to publish later — unless the run failed at the checks, whose failure names what to fix, or at the scan, where the commit holds what looks like a secret and must not be published.', + "The run carries prUrl and prNumber once the branch has its pull request — opened, or already open — and reporting the link is the result. afterReview records its decision on the review's findings: on fix_before_merge, brief the fix from the reviewRunId run and publish again with findings. Absent prUrl, the branch stayed on the machine to publish later — unless the run failed at the checks, whose account says what failed, or at the scan, where the commit holds what looks like a secret.", }, iteration: { auto: false, maxConsecutiveRuns: 1, stopOnOutcomesMet: true, cooldownMs: 0 }, }, @@ -1206,7 +1396,7 @@ const PUBLISH_LOCAL_CHANGES: SkillCatalogEntry = { priority: 50, }, rationale: - "Thirteen tasks, the approval between local and published halves: the commit lands in a detached worktree. host.commit.check runs the folder's checks on it: a failure fails the run, bar test timeouts alone: those rerun once. host.commit.scan reads all a push would add (from a fetched origin/), commit headers, messages, PR text: a finding fails the run; an unread file or an allowed line asks, unreviewed. Over a cleared range the posture, and under unless-unreviewed a Local Code Review, decide whether to ask; asked, it obeys. The push argv is pinned bar the refspec: no force, tag or submodule. The executor re-measures origin/ as git runs, rescanning where it moved or a restart voids the receipt; declared checks need a passed receipt; an uncleared range goes only on this run's grant. host.commit.describe composes the PR from the whole range, reviews, checks and gaps, scanned, before the push; an open PR gets it by PATCH, not a twin.", + "Fifteen tasks, the approval between local and published halves. The commit lands in a detached worktree; host.commit.check runs the folder's checks. A check lost to test timeouts alone reruns; any other failure is workflow.run.fail with the checks' summary — no decision is asked there until its measurement has labelled cases. host.commit.scan reads all a push adds: a finding fails the run, an unread file or allowed line asks. Over a cleared range the posture, and under unless-unreviewed a Local Code Review, decide whether to ask. Over its findings ai.decision.decide answers fix_before_merge or publish_as_is, which describe-pr records, undecided below the bar; no task acts on it, and no task starts a publication. No decision gates the push. The push argv is pinned bar the refspec; the executor re-measures origin/, needs a passed check receipt, and sends an uncleared range only on this run's grant. host.commit.describe composes the PR before the push; an open PR is PATCHed.", }, }; diff --git a/packages/platform-artifacts/src/skillCatalog/publishLocalChangesDecisions.ts b/packages/platform-artifacts/src/skillCatalog/publishLocalChangesDecisions.ts new file mode 100644 index 00000000..f09ba0e7 --- /dev/null +++ b/packages/platform-artifacts/src/skillCatalog/publishLocalChangesDecisions.ts @@ -0,0 +1,70 @@ +import { AFTER_REVIEW_DECIDED, type DecisionChoiceQuestionSchema } from '@aflow/schemas'; +import type { z } from 'zod'; + +/** + * The judgements Publish Local Changes is measured on — what follows a failed + * check, and whether a review's findings are worth a fix round — as the + * questions a decision model is asked. Defined once, here, so the measurement + * of decision quality on the loop's own history (`yarn decisions:measure-loop`, + * Plan 316 §7) asks exactly what the skill asks. + * + * Only the question after a review is the skill's, and its answer is only + * recorded — on the pull request and as the run's `afterReview`, `undecided` + * below its bar, Jev's, measured over 17 cases — never acted on by the run: + * a fix round is the conversation's or the shepherd's. The question after a + * failed check is asked by the + * measurement alone until it has labelled `operator_install_mismatch` cases + * (Plan 316 §7); a failed check fails the run. + */ +export const AFTER_CHECK_MIN_CONFIDENCE = 0.8; +/** + * Measured on Jev alone. The step asks whichever decision model the space + * resolves, so another decider meets this bar unmeasured until the + * measurement asks it the same cases — the same agreement took the + * critique-then-verdict baselines to 0.90 and 0.70 — and the bar moves only + * on that. + */ +export const AFTER_REVIEW_MIN_CONFIDENCE = 0.8; + +export const AFTER_CHECK_ROUTES = [ + 'recheck_on_timeout', + 'fix_commission', + 'operator_install_mismatch', +] as const; +export type AfterCheckRoute = (typeof AFTER_CHECK_ROUTES)[number]; + +/** The answers the after-review question offers: the ones the pull request records as decided. */ +export const AFTER_REVIEW_ROUTES = AFTER_REVIEW_DECIDED; +export type AfterReviewRoute = (typeof AFTER_REVIEW_ROUTES)[number]; + +/** The question name each decision's answer is read under: `answers.`. */ +export const AFTER_CHECK_QUESTION_NAME = 'next_step'; +export const AFTER_REVIEW_QUESTION_NAME = 'after_review'; + +export const AFTER_CHECK_QUESTION = { + type: 'choice', + instructions: + "The folder's checks did not pass on this publication's commit. From what they printed, what follows: running them again, a fix to the change, or the operator putting the machine right.", + options: { + recheck_on_timeout: + "Nothing failed but time: tests ran out of their time under a loaded machine, or the checks were still running when the folder's time ran out, and nothing printed is an assertion, a type error, a lint finding or a guard refusing the change. Run on the same commit again, they are expected to pass.", + fix_commission: + 'The checks found something wrong in the change itself — a failing assertion, a type error, a lint or format finding, a guard the change trips — which a coding agent can put right in the repository from what they printed.', + operator_install_mismatch: + 'The checks could not run as the folder declares them on this machine — a missing tool or dependency, a version out of step with the lockfile, a sandbox or permission refusal, a service the checks reach that is not there — which only the operator can put right on the machine.', + }, + minConfidence: AFTER_CHECK_MIN_CONFIDENCE, +} as const satisfies z.input; + +export const AFTER_REVIEW_QUESTION = { + type: 'choice', + instructions: + 'These are the findings of the review of a branch about to be published. Whether they are fixed on this branch before it merges, or it merges as it stands with them recorded.', + options: { + fix_before_merge: + 'At least one of them should not merge as it is: a fix round on this branch is worth its cost before the branch merges.', + publish_as_is: + 'They are narrow enough to merge as they stand and be fixed later, if at all; the pull request records them.', + }, + minConfidence: AFTER_REVIEW_MIN_CONFIDENCE, +} as const satisfies z.input; diff --git a/packages/platform-artifacts/src/skillCatalog/reviewLocalChanges.test.ts b/packages/platform-artifacts/src/skillCatalog/reviewLocalChanges.test.ts index 30899091..14f99953 100644 --- a/packages/platform-artifacts/src/skillCatalog/reviewLocalChanges.test.ts +++ b/packages/platform-artifacts/src/skillCatalog/reviewLocalChanges.test.ts @@ -106,12 +106,14 @@ describe('Review Local Changes — the first harness inside a skill', () => { expect(REVIEW_LOCAL_CHANGES.description).toContain('not something to ask the coding agent for'); }); - it('promotes the verdict and the summary only — a promoted array reads as a preview', () => { + it('promotes the verdict, the summary a person reads, and the findings a publication decides on one at a time', () => { const promoted = (task?.promoteOutputs ?? []).map((p) => ('toState' in p ? p.toState : '')); - expect(promoted).toEqual(['verdict', 'reviewSummary']); + expect(promoted).toEqual(['verdict', 'reviewSummary', 'findings']); const declared = (wf.stateVariables ?? []).map((v) => v.variableId); - expect(declared).toEqual(['verdict', 'reviewSummary']); + expect(declared).toEqual(['verdict', 'reviewSummary', 'findings']); for (const id of promoted) expect(declared).toContain(id); + // The summary stays the review as it is reported; the findings are for a run to act on. + expect(wf.output?.primary).toBe('verdict'); }); it('makes the summary self-sufficient, in the schema and in the task prose', () => { diff --git a/packages/platform-artifacts/src/skillCatalog/reviewLocalChanges.ts b/packages/platform-artifacts/src/skillCatalog/reviewLocalChanges.ts index 53bc6689..2d93696b 100644 --- a/packages/platform-artifacts/src/skillCatalog/reviewLocalChanges.ts +++ b/packages/platform-artifacts/src/skillCatalog/reviewLocalChanges.ts @@ -19,6 +19,51 @@ Write the summary as the deliverable: it names every finding inline — severity Edit no file, stage nothing and commit nothing. The result is the whole deliverable.`; +/** A finding as the review returns it: what a publication waiting on it records, and argues from. */ +export const REVIEW_FINDING_SCHEMA = { + type: 'object', + required: ['severity', 'file', 'claim', 'evidence'], + additionalProperties: false, + properties: { + severity: { + type: 'string', + enum: ['blocker', 'major', 'minor', 'nit'], + description: + 'blocker: must not ship. major: wrong or unsafe in a case that will occur. minor: a real defect with narrow effect. nit: a preference.', + }, + file: { + type: 'string', + minLength: 1, + maxLength: 1000, + description: 'Path relative to the repository root.', + }, + line: { + type: 'integer', + minimum: 1, + description: 'Line the finding is about, where it is about one.', + }, + claim: { + type: 'string', + minLength: 1, + maxLength: 4000, + description: 'What is wrong, stated so it can be argued with.', + }, + evidence: { + type: 'string', + minLength: 1, + maxLength: 8000, + description: + 'What in this checkout shows it — the code, the caller, the test that does not cover it.', + }, + suggestion: { + type: 'string', + minLength: 1, + maxLength: 8000, + description: 'What would settle it, where the review can say.', + }, + }, +}; + const RESULT_SCHEMA = { type: 'object', required: ['verdict', 'summary', 'findings'], @@ -41,49 +86,7 @@ const RESULT_SCHEMA = { type: 'array', maxItems: 50, description: 'Everything worth acting on, ordered by severity. Empty when nothing was found.', - items: { - type: 'object', - required: ['severity', 'file', 'claim', 'evidence'], - additionalProperties: false, - properties: { - severity: { - type: 'string', - enum: ['blocker', 'major', 'minor', 'nit'], - description: - 'blocker: must not ship. major: wrong or unsafe in a case that will occur. minor: a real defect with narrow effect. nit: a preference.', - }, - file: { - type: 'string', - minLength: 1, - maxLength: 1000, - description: 'Path relative to the repository root.', - }, - line: { - type: 'integer', - minimum: 1, - description: 'Line the finding is about, where it is about one.', - }, - claim: { - type: 'string', - minLength: 1, - maxLength: 4000, - description: 'What is wrong, stated so it can be argued with.', - }, - evidence: { - type: 'string', - minLength: 1, - maxLength: 8000, - description: - 'What in this checkout shows it — the code, the caller, the test that does not cover it.', - }, - suggestion: { - type: 'string', - minLength: 1, - maxLength: 8000, - description: 'What would settle it, where the review can say.', - }, - }, - }, + items: REVIEW_FINDING_SCHEMA, }, checksRun: { type: 'array', @@ -143,7 +146,7 @@ const BRIEF_MAX_TURNS = 8; const REVIEW_LOCAL_CHANGES: SkillCatalogEntry = { catalogId: 'review-local-changes', - version: 7, + version: 8, name: 'Review Local Changes', tagline: "Review committed changes in a connected repository with the machine's own coding agent.", @@ -257,6 +260,7 @@ const REVIEW_LOCAL_CHANGES: SkillCatalogEntry = { promoteOutputs: [ { kind: 'output_path' as const, path: 'result.verdict', toState: 'verdict' }, { kind: 'output_path' as const, path: 'result.summary', toState: 'reviewSummary' }, + { kind: 'output_path' as const, path: 'result.findings', toState: 'findings' }, ], }, ], @@ -278,6 +282,15 @@ const REVIEW_LOCAL_CHANGES: SkillCatalogEntry = { sensitive: false, immutable: false, }, + { + variableId: 'findings', + name: 'Review findings', + description: + 'Every finding as the review returned it — severity, file, line, claim, evidence and suggestion — for a run that acts on them one at a time, as a publication deciding on a fix round does.', + required: false, + sensitive: false, + immutable: false, + }, ], output: { primary: 'verdict', diff --git a/packages/platform-artifacts/src/storeCatalogHashes.json b/packages/platform-artifacts/src/storeCatalogHashes.json index 2de6b025..4ea3db79 100644 --- a/packages/platform-artifacts/src/storeCatalogHashes.json +++ b/packages/platform-artifacts/src/storeCatalogHashes.json @@ -96,7 +96,7 @@ "contentHash": "5f9151872e93be3bb7ad3b154b9a539677892df0361ad01c2579f227a27b6856" }, "local-code-review": { - "version": 7, + "version": 8, "contentHash": "cd6f27ade9b8f319bd43cafed7e48afffb8412148560638f5181fabb505f8815" }, "local-commission": { @@ -104,8 +104,8 @@ "contentHash": "35998b7e73fb024809520f7738c4f169e18858a800d924baf195f43e6648a90a" }, "local-publish": { - "version": 21, - "contentHash": "4c46d0e167830967d1be26f00f20b0bdb21d36bac7375d1933ba5ff2cc74ac24" + "version": 31, + "contentHash": "ba3f45cc0bfbaafa2c77caa4c6c09256feaa4eb3dde207cd5ceba98b70f4f4ab" }, "newsapi": { "version": 1, diff --git a/packages/schemas/src/operations/__tests__/hostCommitCheck.test.ts b/packages/schemas/src/operations/__tests__/hostCommitCheck.test.ts index 2fa9c831..9db8577a 100644 --- a/packages/schemas/src/operations/__tests__/hostCommitCheck.test.ts +++ b/packages/schemas/src/operations/__tests__/hostCommitCheck.test.ts @@ -89,19 +89,17 @@ describe('what host.commit.check answers', () => { expect(failed.clearedSha).toBeUndefined(); }); - it('names the tests that ran out of time, up to its cap, beside the commit a second run takes', () => { + it('names the tests that ran out of time, up to its cap', () => { const lost = { passed: false, exitCode: 1, durationMs: 1, tail: 'FAIL\n', summary: 'failed on time', - clearedOrRetriableSha: SHA, }; const names = Array.from({ length: HOST_CHECK_TEST_TIMEOUTS_MAX }, (_, i) => `t${String(i)}`); expect(HostCommitCheckOutputSchema.parse({ ...lost, testTimeouts: names })).toMatchObject({ testTimeouts: names, - clearedOrRetriableSha: SHA, }); expect( HostCommitCheckOutputSchema.safeParse({ ...lost, testTimeouts: [...names, 'one more'] }) diff --git a/packages/schemas/src/operations/__tests__/pullRequestReviewRecord.test.ts b/packages/schemas/src/operations/__tests__/pullRequestReviewRecord.test.ts new file mode 100644 index 00000000..9adf1ada --- /dev/null +++ b/packages/schemas/src/operations/__tests__/pullRequestReviewRecord.test.ts @@ -0,0 +1,146 @@ +/** + * Contract: the findings a publication hands on always fit within their share + * of a later publication's run inputs, which takes them back as `findings` + * under the cap on those inputs together — whatever the review returned. + */ +import { Buffer } from 'node:buffer'; + +import { describe, expect, it } from 'vitest'; + +import { + HANDED_ON_FINDING_MAX_BYTES, + HANDED_ON_FINDINGS_MAX_BYTES, + HANDED_ON_FINDINGS_MAX_COUNT, + HostCommitDescribeInputSchema, + PullRequestFindingSchema, + pullRequestReviewRecord, +} from '../hostCommitDescribe.js'; +import { + MAX_PARENT_INPUTS_SERIALIZED_BYTES, + ParentInputsRecordSchema, +} from '../../runtime/parentInstructions.js'; + +const RANGE = `${'a'.repeat(40)}..${'c'.repeat(40)}`; +/** The most findings Local Code Review returns, and the longest file and claim each may carry. */ +const REVIEW_MAX_FINDINGS = 50; +const FILE_MAX_CHARS = 1000; +const CLAIM_MAX_CHARS = 4000; +const SEVERITIES = ['nit', 'minor', 'major', 'blocker'] as const; + +const bytes = (value: unknown) => Buffer.byteLength(JSON.stringify(value), 'utf8'); + +/** + * Text of `length` code units that serialises as large as text can: a control + * character JSON escapes to six bytes, beside three-byte and surrogate-pair + * characters. + */ +function heaviest(length: number): string { + const pattern = '\u0001€😀'; + return pattern.repeat(Math.ceil(length / pattern.length)).slice(0, length); +} + +function largestReview() { + return Array.from({ length: REVIEW_MAX_FINDINGS }, (_, index) => ({ + severity: SEVERITIES[index % SEVERITIES.length] ?? 'nit', + file: heaviest(FILE_MAX_CHARS), + line: Number.MAX_SAFE_INTEGER, + claim: heaviest(CLAIM_MAX_CHARS), + evidence: heaviest(8000), + })); +} + +function recordOf(input: Record) { + return pullRequestReviewRecord( + HostCommitDescribeInputSchema.parse({ bindingId: 'hb_app', range: RANGE, ...input }), + ); +} + +describe('the findings a publication hands on', () => { + it('take half the inputs a later publication carries, shared among the most it hands on', () => { + expect(HANDED_ON_FINDINGS_MAX_BYTES).toBe(MAX_PARENT_INPUTS_SERIALIZED_BYTES / 2); + expect( + 2 + HANDED_ON_FINDINGS_MAX_COUNT * (HANDED_ON_FINDING_MAX_BYTES + 1), + ).toBeLessThanOrEqual(HANDED_ON_FINDINGS_MAX_BYTES); + }); + + it('fit their share from the largest review there can be, and the next publication takes them back', () => { + const { findings, findingsLeftOut } = recordOf({ + review: { + verdict: 'request_changes', + outcome: 'completed', + reviewSummary: 'Read.', + findings: largestReview(), + }, + }); + expect(findings).toHaveLength(HANDED_ON_FINDINGS_MAX_COUNT); + // What the bound dropped is counted, so no reader takes the list for the whole review. + expect(findingsLeftOut).toBe(REVIEW_MAX_FINDINGS - HANDED_ON_FINDINGS_MAX_COUNT); + for (const finding of findings) { + expect(PullRequestFindingSchema.parse(finding)).toEqual(finding); + expect(bytes(finding)).toBeLessThanOrEqual(HANDED_ON_FINDING_MAX_BYTES); + } + expect(bytes(findings)).toBeLessThanOrEqual(HANDED_ON_FINDINGS_MAX_BYTES); + // Beside everything else a publication is started with, at the other half, the cap holds. + const nextInputs = { + findings, + rest: 'x'.repeat(MAX_PARENT_INPUTS_SERIALIZED_BYTES - HANDED_ON_FINDINGS_MAX_BYTES - 32), + }; + expect(ParentInputsRecordSchema.safeParse(nextInputs).success).toBe(true); + }); + + it('fit their share where they are the open ones it was passed, however many', () => { + const passed = largestReview().map(({ evidence: _evidence, ...finding }) => ({ + ...finding, + state: 'open' as const, + })); + const { findings } = recordOf({ findings: passed }); + expect(findings).toHaveLength(HANDED_ON_FINDINGS_MAX_COUNT); + expect(bytes(findings)).toBeLessThanOrEqual(HANDED_ON_FINDINGS_MAX_BYTES); + }); + + it('keep the most severe where some must be left out, each severity in the order the review gave', () => { + const many = Array.from({ length: 20 }, (_, index) => ({ + severity: index < 15 ? ('nit' as const) : ('blocker' as const), + file: `src/f${String(index)}.ts`, + claim: `Finding ${String(index)}.`, + evidence: 'Shown.', + })); + const { findings, findingsLeftOut } = recordOf({ + review: { verdict: 'request_changes', outcome: 'completed', findings: many }, + }); + expect(findingsLeftOut).toBe(many.length - HANDED_ON_FINDINGS_MAX_COUNT); + expect(findings.map((f) => f.file)).toEqual( + [15, 16, 17, 18, 19, 0, 1, 2, 3, 4, 5, 6].map((i) => `src/f${String(i)}.ts`), + ); + }); + + it('go whole where they fit, and cut at the claim where not — marked, and never through a character', () => { + const fits = { + severity: 'major' as const, + file: 'src/a.ts', + line: 3, + claim: 'Leaks.', + evidence: 'x', + }; + const long = { ...fits, file: 'src/b.ts', claim: '😀'.repeat(CLAIM_MAX_CHARS / 2) }; + const { findings, findingsLeftOut } = recordOf({ + review: { verdict: 'request_changes', outcome: 'completed', findings: [fits, long] }, + }); + // A claim cut to fit is no finding left out. + expect(findingsLeftOut).toBeUndefined(); + expect(findings[0]).toEqual({ + severity: 'major', + file: 'src/a.ts', + line: 3, + claim: 'Leaks.', + state: 'open', + }); + const cut = findings[1]; + expect(cut?.file).toBe('src/b.ts'); + expect(cut?.claim.endsWith('…')).toBe(true); + expect(long.claim.startsWith(cut?.claim.slice(0, -1) ?? '∅')).toBe(true); + // A lone high surrogate would serialise escaped, as no character at all. + expect(cut?.claim).not.toMatch(/[\ud800-\udbff](?![\udc00-\udfff])/); + expect(bytes(cut)).toBeLessThanOrEqual(HANDED_ON_FINDING_MAX_BYTES); + }); +}); diff --git a/packages/schemas/src/operations/aiDecision.test.ts b/packages/schemas/src/operations/aiDecision.test.ts index 19320f13..4144a077 100644 --- a/packages/schemas/src/operations/aiDecision.test.ts +++ b/packages/schemas/src/operations/aiDecision.test.ts @@ -49,6 +49,53 @@ describe('AiDecideInputSchema', () => { expect(parse({}).success).toBe(false); }); + it('takes a question of each item alone, named questions alone, or both — never neither', () => { + const each = { items: [{ claim: 'a' }], question: { type: 'yes_no', instructions: 'Defect' } }; + const named = { u: { type: 'yes_no', instructions: 'x' } }; + expect(AiDecideInputSchema.safeParse({ state: 'x', each }).success).toBe(true); + expect(AiDecideInputSchema.safeParse({ state: 'x', each, questions: named }).success).toBe( + true, + ); + expect(AiDecideInputSchema.safeParse({ state: 'x' }).success).toBe(false); + }); + + it('refuses a named question under the name an item is asked by, and an each that cannot be asked', () => { + const each = { items: [{ claim: 'a' }], question: { type: 'yes_no', instructions: 'Defect' } }; + const clash = { each_0: { type: 'yes_no', instructions: 'x' } }; + expect(AiDecideInputSchema.safeParse({ state: 'x', each, questions: clash }).success).toBe( + false, + ); + for (const bad of [ + { ...each, question: { type: 'yes_no' } }, + { ...each, items: Array(51).fill({}) }, + ]) { + expect(AiDecideInputSchema.safeParse({ state: 'x', each: bad }).success).toBe(false); + } + }); + + it('refuses an item that cannot be read as instructions: a number, a boolean or null', () => { + const question = { type: 'yes_no', instructions: 'Defect' }; + for (const item of [3, true, null]) { + const result = AiDecideInputSchema.safeParse({ + state: 'x', + each: { items: [item], question }, + }); + expect(result.success, String(item)).toBe(false); + } + for (const item of ['a claim', { claim: 'a' }, ['a', 'b']]) { + const result = AiDecideInputSchema.safeParse({ + state: 'x', + each: { items: [item], question }, + }); + expect(result.success, JSON.stringify(item)).toBe(true); + } + }); + + it('takes an empty list, which a step reads only when the list it binds came back empty', () => { + const each = { items: [], question: { type: 'yes_no', instructions: 'Defect' } }; + expect(AiDecideInputSchema.safeParse({ state: 'x', each }).success).toBe(true); + }); + it('refuses a minConfidence outside 0..1', () => { expect(parse({ u: { type: 'yes_no', instructions: 'x', minConfidence: 1.5 } }).success).toBe( false, diff --git a/packages/schemas/src/operations/aiDecision.ts b/packages/schemas/src/operations/aiDecision.ts index f813a5ad..b72ec12a 100644 --- a/packages/schemas/src/operations/aiDecision.ts +++ b/packages/schemas/src/operations/aiDecision.ts @@ -124,25 +124,87 @@ export const DecisionQuestionsSchema = z ); export type DecisionQuestions = z.infer; -export const AiDecideInputSchema = z.object({ - state: DecisionEntrySchema.describe( - 'What the questions are about: text, a JSON object or a JSON array. Content in the state can steer the answers, so a decision about untrusted input is never the only gate on something that matters.', - ), - images: z - .array(MediaSourceRefSchema) - .min(1) - .max(DECISION_MAX_IMAGES) - .describe( - 'Images the questions are also about — screenshots, photos, documents — read with the state. Only a decision model that reads images takes them; a step that sends images to one that does not is refused.', - ) - .optional(), - questions: DecisionQuestionsSchema, - model: enumWithCustom(DECISION_MODELS) - .describe( - "Decision model to use. Absent, the space's decision model, or the first decision model whose provider the space holds a key for.", - ) - .optional(), -}); +/** The most items one `each` asks about: a review returns at most this many findings. */ +export const DECISION_MAX_EACH_ITEMS = 50; + +/** + * The name an `each` item's question travels under beside the named questions. + * A named question may not take it, so the two can never answer for each other. + */ +export const DECISION_EACH_QUESTION_PREFIX = 'each_'; +const EACH_QUESTION_NAME = /^each_\d+$/; + +export function decisionEachQuestionName(index: number): string { + return `${DECISION_EACH_QUESTION_PREFIX}${String(index)}`; +} + +export const DecisionEachSchema = z + .object({ + items: z + .array(DecisionEntrySchema) + .max(DECISION_MAX_EACH_ITEMS) + .describe( + "The list the question is asked of, one item at a time — the findings of a review, the lines of an order — each text, a JSON object or a JSON array, as a question's instructions are, since it is read as part of them beside the state. An empty list selects nothing, and with no named questions beside it nothing is asked.", + ), + question: DecisionYesNoQuestionSchema.describe( + 'The yes_no asked of every item, stated about one item. Its minConfidence holds for each answer.', + ), + }) + .superRefine((each, ctx) => { + const q = each.question; + if (q.instructions !== undefined || q.criteria?.true != null || q.criteria?.false != null) { + return; + } + ctx.addIssue({ + code: z.ZodIssueCode.custom, + path: ['question'], + message: + 'A yes_no question states the claim it asks about: give instructions, or criteria for true or false.', + }); + }); +export type DecisionEach = z.infer; + +export const AiDecideInputSchema = z + .object({ + state: DecisionEntrySchema.describe( + 'What the questions are about: text, a JSON object or a JSON array. Content in the state can steer the answers, so a decision about untrusted input is never the only gate on something that matters.', + ), + images: z + .array(MediaSourceRefSchema) + .min(1) + .max(DECISION_MAX_IMAGES) + .describe( + 'Images the questions are also about — screenshots, photos, documents — read with the state. Only a decision model that reads images takes them; a step that sends images to one that does not is refused.', + ) + .optional(), + questions: DecisionQuestionsSchema.optional(), + each: DecisionEachSchema.optional().describe( + 'One yes_no asked of every item of a list, for a list whose length is known only when the step runs. The items it holds true for, confidently, come back as `each.selected` — and as `each.selectedJson`, text a later step can hand on.', + ), + model: enumWithCustom(DECISION_MODELS) + .describe( + "Decision model to use. Absent, the space's decision model, or the first decision model whose provider the space holds a key for.", + ) + .optional(), + }) + .superRefine((input, ctx) => { + if (input.questions === undefined && input.each === undefined) { + ctx.addIssue({ + code: z.ZodIssueCode.custom, + path: ['questions'], + message: 'Ask named questions, a question of each item, or both.', + }); + } + if (input.each === undefined) return; + for (const name of Object.keys(input.questions ?? {})) { + if (!EACH_QUESTION_NAME.test(name)) continue; + ctx.addIssue({ + code: z.ZodIssueCode.custom, + path: ['questions', name], + message: `"${name}" is how an item of \`each\` is asked; name the question otherwise.`, + }); + } + }); export type AiDecideInput = z.infer; const decidedSchema = z @@ -153,9 +215,19 @@ const decidedSchema = z const probabilitiesSchema = z.record(z.string(), z.number().min(0).max(1)); +/** + * One comparison a route can read: `decidedValue == 'billing'` holds only + * for an answer that is billing and cleared the bar, which a predicate that + * can join comparisons only one level deep could otherwise say only as an + * `allOf` — and so never beside another condition in an `anyOf`. + */ +const decidedValueDescription = + 'The value where the answer cleared the question’s minConfidence; null where it did not.'; + export const DecisionChoiceAnswerSchema = z.object({ type: z.literal('choice'), value: z.string().describe('The chosen option label.'), + decidedValue: z.string().nullable().describe(decidedValueDescription), confidence: z.number().min(0).max(1), probabilities: probabilitiesSchema.describe('Option label → probability.'), decided: decidedSchema, @@ -164,6 +236,7 @@ export const DecisionChoiceAnswerSchema = z.object({ export const DecisionScoreAnswerSchema = z.object({ type: z.literal('score'), value: z.number().describe('The expected level index, lowest level 0; may be fractional.'), + decidedValue: z.number().nullable().describe(decidedValueDescription), confidence: z.number().min(0).max(1), probabilities: probabilitiesSchema.describe('Level index → probability.'), decided: decidedSchema, @@ -172,6 +245,7 @@ export const DecisionScoreAnswerSchema = z.object({ export const DecisionYesNoAnswerSchema = z.object({ type: z.literal('yes_no'), value: z.boolean().describe('True when the statement is more likely true than false.'), + decidedValue: z.boolean().nullable().describe(decidedValueDescription), probability: z.number().min(0).max(1).describe('Probability that the statement is true.'), confidence: z .number() @@ -183,6 +257,9 @@ export const DecisionYesNoAnswerSchema = z.object({ export const DecisionRefusalAnswerSchema = z.object({ type: z.literal('refusal'), + decidedValue: z + .null() + .describe(`${decidedValueDescription} A refusal never clears it, so it is always null here.`), decided: z .literal(false) .describe( @@ -198,9 +275,49 @@ export const DecisionAnswerSchema = z.discriminatedUnion('type', [ ]); export type DecisionAnswer = z.infer; +/** An item's answer: the yes_no asked of it, or a refusal where the model declined that item. */ +export const DecisionEachAnswerSchema = z.discriminatedUnion('type', [ + DecisionYesNoAnswerSchema, + DecisionRefusalAnswerSchema, +]); +export type DecisionEachAnswer = z.infer; + +export const DecisionEachOutputSchema = z.object({ + answers: z + .array(DecisionEachAnswerSchema) + .describe( + 'The answer for each item, in the order the items were given. An item the model declined is a refusal, never decided, so never selected.', + ), + selected: z + .array(z.unknown()) + .describe('The items whose answer is true and decided, in their order.'), + selectedCount: z.number().int().nonnegative(), + undecidedCount: z + .number() + .int() + .nonnegative() + .describe('How many items were answered below the question’s minConfidence, or declined.'), + selectedJson: z + .string() + .describe( + '`selected` as indented JSON, for a step that takes text — a brief, a message. `[]` where none was selected.', + ), +}); +export type DecisionEachOutput = z.infer; + +/** The model an answer reports where no model was asked anything. */ +export const DECISION_NO_MODEL = 'none'; + export const AiDecideOutputSchema = z.object({ - answers: z.record(z.string(), DecisionAnswerSchema).describe('Question name → answer.'), - model: z.string(), + answers: z + .record(z.string(), DecisionAnswerSchema) + .describe('Question name → answer. Empty where only `each` was asked.'), + each: DecisionEachOutputSchema.optional().describe('Present where `each` was asked.'), + model: z + .string() + .describe( + `The decision model that answered — or \`${DECISION_NO_MODEL}\` where nothing was asked of one, as for an \`each\` over an empty list with no named question beside it.`, + ), calibration: z .enum(['calibrated', 'self_reported']) .describe( diff --git a/packages/schemas/src/operations/hostCommitCheck.ts b/packages/schemas/src/operations/hostCommitCheck.ts index 08fce053..6d434177 100644 --- a/packages/schemas/src/operations/hostCommitCheck.ts +++ b/packages/schemas/src/operations/hostCommitCheck.ts @@ -40,6 +40,9 @@ export const HOST_CHECK_OUTPUT_TAIL_BYTES = 1024 * 1024; */ export const HOST_CHECK_TEST_TIMEOUTS_MAX = 20; +/** The longest step name a check's report may give as the one that failed. */ +export const HOST_CHECK_FAILING_GUARD_MAX_CHARS = 1000; + export const HostCommitCheckInputSchema = z.object({ bindingId: HostBindingRef, sha: HostCommitShaSchema.describe( @@ -121,13 +124,16 @@ export const HostCommitCheckOutputSchema = z.object({ 'names, that nothing else failed — what a machine under load does to a passing commit. ' + 'At most `HOST_CHECK_TEST_TIMEOUTS_MAX`; the summary counts the rest.', ), - clearedOrRetriableSha: z + failingGuard: z .string() + .min(1) + .max(HOST_CHECK_FAILING_GUARD_MAX_CHARS) .optional() .describe( - 'The commit as a full sha, present where the checks passed, the folder declares none, ' + - 'or only `testTimeouts` failed them. A publication’s first run of the checks reads ' + - 'this, so every other failure fails it and a run lost to load alone is run once more.', + 'The step the checks name as the one that failed — a guard, a type check, a test run — ' + + 'as they reported it in the file `AFLOW_CHECK_REPORT` names. Present only where the ' + + 'checks did not pass and said which step it was. The summary names it, so whoever reads ' + + 'the failure knows which step to run again.', ), receipt: HostCheckReceiptSchema.optional().describe( 'What a push of the commit must carry as `check.receipt`, present wherever the checks ran, ' + diff --git a/packages/schemas/src/operations/hostCommitDescribe.ts b/packages/schemas/src/operations/hostCommitDescribe.ts index 6158d399..85c3b432 100644 --- a/packages/schemas/src/operations/hostCommitDescribe.ts +++ b/packages/schemas/src/operations/hostCommitDescribe.ts @@ -4,6 +4,7 @@ */ import { z } from 'zod'; +import { MAX_PARENT_INPUTS_SERIALIZED_BYTES } from '../runtime/parentInstructions.js'; import { HostBindingRef, HostCommitRangeSchema, HostCommitShaSchema } from './host.js'; /** GitHub's own limit on a pull request's body; a longer one is refused with a 422. */ @@ -43,9 +44,33 @@ export const PullRequestFindingsSchema = z 'The findings of the review rounds before this publication — each Local Code Review ' + 'whose findings a later commission on the branch was briefed to fix — as the reviews ' + "named them, each marked `closed` or `open`. The publication's own review, where one " + - 'runs, is added as it returns.', + 'finishes, is recorded by its summary, and its findings replace these in what the ' + + 'publication hands on.', ); +/** + * A finding as the publication's own review returned it. Only what the pull + * request records is read: the evidence and the suggestion stay on the + * review's run. + */ +const ReviewFindingSchema = PullRequestFindingSchema.omit({ state: true }).strip(); + +/** The answers the decision after a review can clear its bar with. */ +export const AFTER_REVIEW_DECIDED = ['fix_before_merge', 'publish_as_is'] as const; + +/** What a publication records of that decision where it weighed findings and none cleared the bar. */ +export const AFTER_REVIEW_UNDECIDED = 'undecided'; + +export const AfterReviewSchema = z + .enum([...AFTER_REVIEW_DECIDED, AFTER_REVIEW_UNDECIDED]) + .describe( + "What the publication decided about its review's findings: `fix_before_merge`, a fix " + + 'round on the branch before it merges; `publish_as_is`, they can merge as they stand; ' + + "`undecided`, no answer cleared the decision's bar, or none could be made. Recorded, never " + + 'acted on by the publication: a fix round is the conversation’s, or the shepherd’s.', + ); +export type AfterReview = z.infer; + export const PullRequestGapsSchema = z .array(z.string().min(1).max(8000)) .describe( @@ -92,11 +117,21 @@ export const HostCommitDescribeInputSchema = z.object({ verdict: z.string().nullable(), outcome: z.string(), reviewSummary: z.string().nullable().optional(), + findings: z.array(ReviewFindingSchema).nullable().optional(), }) .optional() .describe( "The publication's own review of the range, where one ran: its verdict — null where it " + - 'did not finish — how it ended, and its summary, which names every finding it read.', + 'did not finish — how it ended, its summary, which names every finding it read, and ' + + 'the findings themselves, which the publication hands on as the open ones.', + ), + afterReview: z + .enum(AFTER_REVIEW_DECIDED) + .nullable() + .optional() + .describe( + "The decision on the review's findings as it answered: null where the answer fell below " + + 'its bar, absent where none was made.', ), check: CheckRunSchema.optional().describe("The folder's checks on the commit, as they ran."), recheck: CheckRunSchema.optional().describe( @@ -106,6 +141,125 @@ export const HostCommitDescribeInputSchema = z.object({ }); export type HostCommitDescribeInput = z.infer; +/** + * The share of a publication's run inputs the findings it hands on may take, + * serialised: a later publication of the branch takes them back as its + * `findings`, under the cap on its inputs together, and the other half is left + * to everything else it is started with — the change, the branch, the gaps. + */ +export const HANDED_ON_FINDINGS_MAX_BYTES = MAX_PARENT_INPUTS_SERIALIZED_BYTES / 2; + +/** The most findings a publication hands on: the most severe of them, in the review's order. */ +export const HANDED_ON_FINDINGS_MAX_COUNT = 12; + +/** + * The most one handed-on finding may come to, serialised: the share divided + * among the most findings there can be, less the array's brackets and the + * comma between each — so the list fits its share whatever its findings say. + */ +export const HANDED_ON_FINDING_MAX_BYTES = + Math.floor((HANDED_ON_FINDINGS_MAX_BYTES - 2) / HANDED_ON_FINDINGS_MAX_COUNT) - 1; + +const SEVERITY_RANK: Record = { + blocker: 0, + major: 1, + minor: 2, + nit: 3, +}; + +const TRUNCATED = '…'; +const utf8 = new TextEncoder(); + +/** What `value` comes to as the run-input cap measures it: UTF-8 bytes of its JSON. */ +function serialisedBytes(value: unknown): number { + return utf8.encode(JSON.stringify(value)).length; +} + +/** `text` cut to `length` code units, never through the middle of a surrogate pair. */ +function cut(text: string, length: number): string { + const kept = text.slice(0, length); + const last = kept.charCodeAt(kept.length - 1); + return last >= 0xd800 && last <= 0xdbff ? kept.slice(0, -1) : kept; +} + +/** + * `finding` with `field` cut, marked with an ellipsis, as far as it must be for + * the finding to fit its bytes. Measured, not counted in characters: one + * character can serialise to six bytes, as an escaped control character does. + */ +function shortened(finding: PullRequestFinding, field: 'claim' | 'file'): PullRequestFinding { + let fits = 0; + let over = finding[field].length; + while (fits < over) { + const length = Math.ceil((fits + over) / 2); + const candidate = { ...finding, [field]: `${cut(finding[field], length)}${TRUNCATED}` }; + if (serialisedBytes(candidate) <= HANDED_ON_FINDING_MAX_BYTES) fits = length; + else over = length - 1; + } + return { ...finding, [field]: `${cut(finding[field], fits)}${TRUNCATED}` }; +} + +/** `finding` as it is where it fits its bytes; its claim cut first, then its file, where not. */ +function fitted(finding: PullRequestFinding): PullRequestFinding { + if (serialisedBytes(finding) <= HANDED_ON_FINDING_MAX_BYTES) return finding; + const claimCut = shortened(finding, 'claim'); + return serialisedBytes(claimCut) <= HANDED_ON_FINDING_MAX_BYTES + ? claimCut + : shortened(claimCut, 'file'); +} + +/** + * The findings a publication hands on, held within their share of a later + * publication's inputs: the most severe first, as many as the count allows, + * each cut to its bytes, and how many the count left out, where it left any. + * The pull request's own text keeps every finding whole, and the review run + * keeps every finding as it returned them, which is what a fix round reads. + */ +function handedOn( + findings: readonly PullRequestFinding[], +): Pick { + const kept = [...findings] + .sort((a, b) => SEVERITY_RANK[a.severity] - SEVERITY_RANK[b.severity]) + .slice(0, HANDED_ON_FINDINGS_MAX_COUNT) + .map(fitted); + const leftOut = findings.length - kept.length; + return leftOut > 0 ? { findings: kept, findingsLeftOut: leftOut } : { findings: kept }; +} + +/** What a publication hands on of its review: the findings still open, and the decision on this round's. */ +export interface PullRequestReviewRecord { + findings: PullRequestFinding[]; + /** How many open findings the hand-on left out to stay within its share; absent where none. */ + findingsLeftOut?: number; + afterReview?: AfterReview; +} + +/** + * The review record a publication hands on: the open findings of the latest + * review of the branch. Where this publication's own review finished, its + * findings replace the earlier rounds' — it read the whole range, so it is + * the latest word on every one of them, and nothing has fixed its findings + * yet. Where no review finished, the open findings it was passed go on as + * they are. Either way the list never outgrows one review's, and it is held + * within its share of a later publication's inputs (`handedOn`), so the + * publication taking it back as `findings` is never refused for its size. + * + * The decision on them is recorded exactly where this review returned + * findings — the decision is asked only then — and an answer that is missing + * there, below the bar or never made, is recorded as `undecided`. + */ +export function pullRequestReviewRecord(input: HostCommitDescribeInput): PullRequestReviewRecord { + const reviewFindings = input.review?.findings; + if (reviewFindings === null || reviewFindings === undefined) { + return handedOn((input.findings ?? []).filter((finding) => finding.state === 'open')); + } + const handed = handedOn( + reviewFindings.map((finding) => ({ ...finding, state: 'open' as const })), + ); + if (handed.findings.length === 0) return handed; + return { ...handed, afterReview: input.afterReview ?? AFTER_REVIEW_UNDECIDED }; +} + export const HostCommitDescribeCommitSchema = z.object({ sha: HostCommitShaSchema.describe('The commit, as its full sha.'), subject: z.string().describe("The first line of the commit's message."), @@ -134,5 +288,28 @@ export const HostCommitDescribeOutputSchema = z.object({ commits: z .array(HostCommitDescribeCommitSchema.pick({ sha: true, subject: true })) .describe('Every commit of the range, oldest first.'), + findings: PullRequestFindingsSchema.describe( + "The open findings of the latest review: this publication's own review's where it " + + "finished, replacing the earlier rounds'; otherwise the open ones it was passed. A later " + + 'publication of the branch takes them as `findings`, with those its fix closed marked ' + + '`closed`, for its pull request to record. Never the brief of a fix round, which reads ' + + `every finding from the review run itself: at most ${String(HANDED_ON_FINDINGS_MAX_COUNT)}, ` + + `the most severe first, each cut to fit, so the list stays within ` + + `${String(HANDED_ON_FINDINGS_MAX_BYTES)} bytes of that publication's inputs.`, + ), + findingsLeftOut: z + .number() + .int() + .positive() + .optional() + .describe( + 'How many open findings `findings` left out to stay within its share — the least severe ' + + 'of them; absent where it left none out. The review run and the pull request keep ' + + 'every one.', + ), + afterReview: AfterReviewSchema.optional().describe( + "The decision on this publication's review findings, as the body records it; absent " + + 'where the review returned none.', + ), }); export type HostCommitDescribeOutput = z.infer; diff --git a/packages/schemas/src/operations/workflow/index.ts b/packages/schemas/src/operations/workflow/index.ts index ba685a1f..7f35f5c5 100644 --- a/packages/schemas/src/operations/workflow/index.ts +++ b/packages/schemas/src/operations/workflow/index.ts @@ -16,6 +16,7 @@ export * from './manage.js'; export * from './runStart.js'; export * from './runResume.js'; export * from './runCancelOp.js'; +export * from './runFailOp.js'; export * from './suggestedAction.js'; export * from './runDetail.js'; export * from './runAttention.js'; @@ -58,6 +59,7 @@ import { import { WorkflowRunStartInputSchema, WorkflowRunStartOutputSchema } from './runStart.js'; import { WorkflowRunResumeInputSchema, WorkflowRunResumeOutputSchema } from './runResume.js'; import { WorkflowRunCancelInputSchema, WorkflowRunCancelOutputSchema } from './runCancelOp.js'; +import { WorkflowRunFailInputSchema, WorkflowRunFailOutputSchema } from './runFailOp.js'; import { WorkflowRunDetailInputSchema, WorkflowRunDetailOutputSchema } from './runDetail.js'; import { WorkflowRunListAttentionInputSchema, @@ -96,6 +98,8 @@ export const WorkflowOperationRegistrations = createWorkflowOperationRegistratio WorkflowRunResumeOutputSchema, WorkflowRunCancelInputSchema, WorkflowRunCancelOutputSchema, + WorkflowRunFailInputSchema, + WorkflowRunFailOutputSchema, WorkflowRunDetailInputSchema, WorkflowRunDetailOutputSchema, WorkflowRunListAttentionInputSchema, diff --git a/packages/schemas/src/operations/workflow/registrations.ts b/packages/schemas/src/operations/workflow/registrations.ts index a5678111..29289563 100644 --- a/packages/schemas/src/operations/workflow/registrations.ts +++ b/packages/schemas/src/operations/workflow/registrations.ts @@ -21,6 +21,10 @@ import type { WorkflowRunCancelInputSchema as WorkflowRunCancelInputSchemaT, WorkflowRunCancelOutputSchema as WorkflowRunCancelOutputSchemaT, } from './runCancelOp.js'; +import type { + WorkflowRunFailInputSchema as WorkflowRunFailInputSchemaT, + WorkflowRunFailOutputSchema as WorkflowRunFailOutputSchemaT, +} from './runFailOp.js'; import type { WorkflowRunDetailInputSchema as WorkflowRunDetailInputSchemaT, WorkflowRunDetailOutputSchema as WorkflowRunDetailOutputSchemaT, @@ -71,6 +75,8 @@ interface WorkflowRegistrationSchemas { WorkflowRunResumeOutputSchema: typeof WorkflowRunResumeOutputSchemaT; WorkflowRunCancelInputSchema: typeof WorkflowRunCancelInputSchemaT; WorkflowRunCancelOutputSchema: typeof WorkflowRunCancelOutputSchemaT; + WorkflowRunFailInputSchema: typeof WorkflowRunFailInputSchemaT; + WorkflowRunFailOutputSchema: typeof WorkflowRunFailOutputSchemaT; WorkflowRunDetailInputSchema: typeof WorkflowRunDetailInputSchemaT; WorkflowRunDetailOutputSchema: typeof WorkflowRunDetailOutputSchemaT; WorkflowRunListAttentionInputSchema: typeof WorkflowRunListAttentionInputSchemaT; @@ -113,6 +119,8 @@ export function createWorkflowOperationRegistrations( WorkflowRunResumeOutputSchema, WorkflowRunCancelInputSchema, WorkflowRunCancelOutputSchema, + WorkflowRunFailInputSchema, + WorkflowRunFailOutputSchema, WorkflowRunDetailInputSchema, WorkflowRunDetailOutputSchema, WorkflowRunListAttentionInputSchema, @@ -508,6 +516,42 @@ export function createWorkflowOperationRegistrations( inputZod: WorkflowRunCancelInputSchema, outputZod: WorkflowRunCancelOutputSchema, }, + { + stepType: 'workflow', + group: 'run', + verb: 'fail', + name: 'Fail Workflow Run', + agentTool: false, + actionLabel: 'Stopping the run\u2026', + semanticDescription: + 'Fail the run this task belongs to, with a stated reason, where the skill decided that ' + + 'what an earlier task returned ends it — the branch a decision takes when it cannot go ' + + 'on, written once in the skill. The step always fails, non-retryable, with the reason as ' + + 'its message and the task\u2019s failureInstruction beside it, so the run ends as one whose ' + + 'task failed and wakes whoever started it.', + tags: ['workflow', 'run', 'fail'], + idempotency: 'idempotent', + mutates: false, + usage: { + oneLine: 'End the run as failed, with a reason bound from what decided it.', + whenToUse: [ + 'A decision task routed to a branch that cannot be recovered within the skill', + 'A decision abstained, and the case is one a person or an agent outside the run must take', + ], + whenNotToUse: [ + 'An operation that fails on its own already fails the run — bind nothing to restate it', + 'Stopping another run — use workflow.run.cancel', + ], + pitfalls: [ + 'Gate it with a when: unconditioned, it fails every run that reaches it', + 'The reason is read by whoever is woken; bind it from the output that decided, not a fixed sentence', + ], + minimalExampleInput: { reason: 'The checks failed: tsc apps/x/tsconfig.json.' }, + }, + accessMode: 'read', + inputZod: WorkflowRunFailInputSchema, + outputZod: WorkflowRunFailOutputSchema, + }, { stepType: 'workflow', group: 'run', diff --git a/packages/schemas/src/operations/workflow/runFailOp.ts b/packages/schemas/src/operations/workflow/runFailOp.ts new file mode 100644 index 00000000..22c2f410 --- /dev/null +++ b/packages/schemas/src/operations/workflow/runFailOp.ts @@ -0,0 +1,24 @@ +import { z } from 'zod'; + +// --- workflow.run.fail --- + +/** + * The most a skill's stated reason carries: a check's summary with the end of + * what it printed, which is what a failure is read for. + */ +export const WORKFLOW_RUN_FAIL_REASON_MAX_CHARS = 20_000; + +export const WorkflowRunFailInputSchema = z.object({ + reason: z + .string() + .min(1) + .max(WORKFLOW_RUN_FAIL_REASON_MAX_CHARS) + .describe( + 'Why the run stops here, as whoever reads the failure needs it — usually bound from the output of the task whose result the skill decided to stop on.', + ), +}); +export type WorkflowRunFailInput = z.infer; + +/** Never produced: the step always fails, with `reason` as its message. */ +export const WorkflowRunFailOutputSchema = z.object({}).strict(); +export type WorkflowRunFailOutput = z.infer; diff --git a/scripts/decisions-measure-loop.ts b/scripts/decisions-measure-loop.ts new file mode 100644 index 00000000..e1dc7894 --- /dev/null +++ b/scripts/decisions-measure-loop.ts @@ -0,0 +1,548 @@ +/** + * Plan 316 §7 — how well the loop's own decisions can be made without a + * reasoning model, measured on the loop's own history before Publish Local + * Changes makes them for itself. + * + * yarn decisions:measure-loop collect --space [--repo aflowai/aflow] [--prs 91-124] + * yarn decisions:measure-loop measure [--labels ] [--target 0.9] + * + * `collect` reads every Publish Local Changes and Review Local Changes run of + * the space through the stack's API (the API key in AFLOW_API_KEY, the stack + * at API_URL or http://localhost:3000) and every failing CI job of the pull + * requests named through `gh api`, and writes the labelled set to + * `.debug/decisions/cases.json`: each failed check with what the next + * publication of its branch did, each review finding with whether a later + * publication closed it, each review with whether a fix round followed, each + * failing job's log end with the remedy that followed. A label the records + * cannot give is left out rather than guessed; `--labels` names a file of the + * operator's own (`{ "": "