diff --git a/.gitignore b/.gitignore index d1da045..55714e5 100644 --- a/.gitignore +++ b/.gitignore @@ -7,3 +7,4 @@ memory-bank/ .idea/ TODO.md plans/ +.grok diff --git a/.pi/extensions/pi-workflow-engine/src/advisory-challenge.ts b/.pi/extensions/pi-workflow-engine/src/advisory-challenge.ts new file mode 100644 index 0000000..f883174 --- /dev/null +++ b/.pi/extensions/pi-workflow-engine/src/advisory-challenge.ts @@ -0,0 +1,75 @@ +import { Type, type Static } from "typebox"; +import { collectAdvisoryStage, type AdvisoryStageCoverage } from "./advisory-evidence.ts"; +import { DEFAULT_ADVISORY_TOOLS, DEFAULT_ADVISORY_TOOL_HINTS, type AdvisoryVerified } from "./workflow-advisory-utils.ts"; +import type { WorkflowApi } from "./types.ts"; + +const ChallengeSchema = Type.Object({ + outcome: Type.Union([Type.Literal("counterexample"), Type.Literal("alternative-explanation"), Type.Literal("supports-original"), Type.Literal("no-counterexample")]), + evidence: Type.Array(Type.String()), + experiment: Type.String({ description: "Smallest distinguishing experiment, with observed result if actually run." }), +}); +const AdjudicationSchema = Type.Object({ + outcome: Type.Union([Type.Literal("upheld"), Type.Literal("refuted"), Type.Literal("unresolved")]), + evidence: Type.Array(Type.String()), + reason: Type.String(), +}); +export type ChallengeRecord = + | { status: "complete"; challenge: Static; adjudication: Static } + | { status: "failed"; challenge?: Static }; +export interface AdvisoryChallengeOptions { + maxChallenges: number; + shouldChallenge?: (finding: AdvisoryVerified) => boolean; +} + +/** Opt in with --challenge or --challenge=N (hard bound of 10). */ +export function parseChallengeArgs(args: string): { args: string; options: AdvisoryChallengeOptions } { + let maxChallenges = 0; + const remaining = args.replace(/(?:^|\s)--challenge(?:=([^\s]*))?(?=\s|$)/g, (_match, limit: string | undefined) => { + if (limit !== undefined && !/^\d+$/.test(limit)) { + throw new Error(`Invalid --challenge value "${limit}". Use --challenge or --challenge=N with a non-negative integer; zero disables challenges.`); + } + maxChallenges = Math.min(10, limit === undefined ? 3 : Number(limit)); + return " "; + }); + return { args: remaining.trim(), options: { maxChallenges } }; +} + +export function needsChallenge(finding: AdvisoryVerified): boolean { + return finding.verdict === "PLAUSIBLE" || finding.verdict === "NOT_SUBSTANTIATED" || finding.evidence.length === 0 || + /\b(security|concurren\w*|race|persist\w*|cancel\w*|retr(?:y|ies)|data loss|corrupt\w*|high impact)\b/i.test(`${finding.category} ${finding.summary} ${finding.impact}`); +} + +/** A bounded recipe using existing agent/parallel calls, not a new runtime primitive. */ +export async function challengeFindings( + api: Pick, + findings: T[], + context: string, + options: AdvisoryChallengeOptions, + coverage: AdvisoryStageCoverage[], +): Promise { + const limit = Number.isFinite(options.maxChallenges) ? Math.max(0, Math.min(10, Math.trunc(options.maxChallenges))) : 0; + if (limit === 0) return findings; + const selected = findings.filter((finding) => finding.verdict !== "REFUTED" && (options.shouldChallenge ?? needsChallenge)(finding)).slice(0, limit); + const replacements = new Map(); + await collectAdvisoryStage(api, "Challenge", selected.map((finding) => ({ id: finding.candidateId, run: async () => { + // Preserve the candidate as unresolved if either independent stage fails. + replacements.set(finding.candidateId, { ...finding, verdict: "NOT_SUBSTANTIATED", challenge: { status: "failed" } }); + const challenge = await api.agent( + `Assume this finding is a false positive. Try to DISPROVE it. Find the strongest concrete counterexample or alternative root cause. Inspect callers, invariants, tests and control flow. For a repair, seek an input, race or error path that still fails. State the smallest experiment distinguishing explanations. Do not edit files or claim tests you did not run. No counterexample found is not proof.\n\nExact review context:\n${context}\n\nCandidate and verifier evidence:\n${JSON.stringify(finding)}`, + { label: `challenge:${finding.candidateId}`, phase: "Challenge", profile: "medium", tools: DEFAULT_ADVISORY_TOOLS, toolHints: DEFAULT_ADVISORY_TOOL_HINTS, schema: ChallengeSchema }, + ); + replacements.set(finding.candidateId, { ...finding, verdict: "NOT_SUBSTANTIATED", challenge: { status: "failed", challenge } }); + const adjudication = await api.agent( + `Adjudicate the original finding, independent verifier evidence and falsification attempt below. Preserve unresolved conflict; do not force consensus. A missing counterexample alone cannot upgrade a plausible claim. Cite concrete evidence and observed test results; never invent experiments.\nContext:\n${context}\nOriginal and verifier:\n${JSON.stringify(finding)}\nChallenger:\n${JSON.stringify(challenge)}`, + { label: `adjudicate:${finding.candidateId}`, phase: "Challenge", profile: "medium", tools: [], schema: AdjudicationSchema }, + ); + replacements.set(finding.candidateId, { + ...finding, + verdict: adjudication.outcome === "refuted" ? "REFUTED" : adjudication.outcome === "unresolved" ? "NOT_SUBSTANTIATED" : finding.verdict, + evidence: [...finding.evidence, ...challenge.evidence, ...(challenge.experiment ? [challenge.experiment] : []), ...adjudication.evidence, adjudication.reason], + challenge: { status: "complete", challenge, adjudication }, + }); + return finding.candidateId; + } })), coverage); + return findings.map((finding) => replacements.get(finding.candidateId) ?? finding); +} diff --git a/.pi/extensions/pi-workflow-engine/src/advisory-evidence.ts b/.pi/extensions/pi-workflow-engine/src/advisory-evidence.ts new file mode 100644 index 0000000..cfd514d --- /dev/null +++ b/.pi/extensions/pi-workflow-engine/src/advisory-evidence.ts @@ -0,0 +1,90 @@ +import { createHash } from "node:crypto"; +import { Type, type Static } from "typebox"; +import { AdvisorySeveritySchema, type IdentifiedAdvisoryCandidate, type AdvisoryCandidate, type AdvisoryReport } from "./advisory-schema.ts"; +import type { WorkflowApi } from "./types.ts"; + +export interface AdvisoryStageCoverage { + stage: string; + expected: number; + completed: number; + failed: number; + failures: { branch: string; reason: string; candidate?: AdvisoryCandidate }[]; +} + +/** Settled results are required here: a missing branch is not a negative finding. */ +export async function collectAdvisoryStage( + api: Pick, + stage: string, + branches: { id: string; candidate?: AdvisoryCandidate; run(): Promise }[], + coverage: AdvisoryStageCoverage[], +): Promise { + const results = await api.parallel(branches.map((branch) => branch.run), { settled: true }); + const failures = results.flatMap((result, index) => result.ok ? [] : [{ branch: branches[index]!.id, reason: result.error.message, ...(branches[index]!.candidate ? { candidate: branches[index]!.candidate } : {}) }]); + coverage.push({ stage, expected: branches.length, completed: results.length - failures.length, failed: failures.length, failures }); + return results.flatMap((result) => result.ok ? [result.value] : []); +} + +export function identifyCandidates(candidates: readonly AdvisoryCandidate[], lens: string): IdentifiedAdvisoryCandidate[] { + return candidates.map((candidate, index) => { + const candidateId = createHash("sha256").update(JSON.stringify([lens, index, candidate])).digest("hex").slice(0, 20); + // Identities are assigned by the workflow, never accepted from a finder. + return { ...candidate, candidateId, sourceCandidateIds: [candidateId] }; + }); +} + +function normalizedSummary(summary: string): string { + return summary.toLowerCase().replace(/\s+/g, " ").trim().replace(/[.!?]+$/, ""); +} + +function candidateDedupKey(candidate: AdvisoryCandidate): string { + const location = candidate.locations[0]; + return JSON.stringify([candidate.category, location?.file.replace(/^\.\//, "").replace(/^[ab]\//, ""), + location?.symbol ?? location?.line ?? null, normalizedSummary(candidate.summary)]); +} + +/** Collapse only equivalent claims at the same anchor; keep every discovery ID and location. */ +export function dedupeCandidates(candidates: readonly T[]): T[] { + const seen = new Map(); + for (const candidate of candidates) { + const key = candidateDedupKey(candidate); + const previous = seen.get(key); + if (!previous) seen.set(key, { ...candidate }); + else { + previous.sourceCandidateIds = [...new Set([...previous.sourceCandidateIds, ...candidate.sourceCandidateIds])]; + previous.locations = uniqueLocations([...previous.locations, ...candidate.locations]); + previous.impact = [...new Set([previous.impact, candidate.impact])].join("\n"); + previous.discoveryEvidence = [...new Set([...(previous.discoveryEvidence ?? []), ...(candidate.discoveryEvidence ?? [])])]; + } + } + return [...seen.values()]; +} + +export function uniqueLocations(locations: AdvisoryCandidate["locations"]): AdvisoryCandidate["locations"] { + return [...new Map(locations.map((location) => [JSON.stringify(location), location])).values()]; +} + +// Synthesis selects evidence records. It has no fields with which to replace their evidence. +export const AdvisorySynthesisSchema = Type.Object({ + summary: Type.String(), + findings: Type.Array(Type.Object({ + sourceCandidateIds: Type.Array(Type.String(), { minItems: 1 }), + severity: AdvisorySeveritySchema, + recommendation: Type.String(), + })), + nextSteps: Type.Array(Type.String()), +}); +export type AdvisorySynthesis = Static; +export const SYNTHESIS_ID_INSTRUCTIONS = "Select or merge findings only by sourceCandidateIds shown below. Return severity and an advisory recommendation per selection. Never select an ID absent from these verified records. "; + +export function withAdvisoryCoverage(report: T, coverage: AdvisoryStageCoverage[]) { + const incomplete = coverage.some((stage) => stage.failed > 0); + const gaps = coverage.flatMap((stage) => stage.failures.map((failure) => `${stage.stage}/${failure.branch}: ${failure.reason}`)); + return { + ...report, + status: incomplete ? "incomplete" as const : "complete" as const, + summary: incomplete ? `Incomplete review: ${gaps.length} branch(es) failed. ${report.findings.length} finding(s) available; no clean conclusion is possible.` : report.summary, + nextSteps: incomplete ? ["Inspect failed branches and rerun missing work.", ...(report.findings.length > 0 ? report.nextSteps : [])] : report.nextSteps, + coverage, + gaps, + }; +} diff --git a/.pi/extensions/pi-workflow-engine/src/advisory-schema.ts b/.pi/extensions/pi-workflow-engine/src/advisory-schema.ts index 69541c0..a5d52b1 100644 --- a/.pi/extensions/pi-workflow-engine/src/advisory-schema.ts +++ b/.pi/extensions/pi-workflow-engine/src/advisory-schema.ts @@ -9,7 +9,7 @@ export const AdvisoryConfidenceSchema = Type.Union([Type.Literal("low"), Type.Li description: "Confidence that the finding is actionable.", }); -export const AdvisoryVerifierVerdictSchema = Type.Union([Type.Literal("CONFIRMED"), Type.Literal("PLAUSIBLE"), Type.Literal("REFUTED")], { +export const AdvisoryVerifierVerdictSchema = Type.Union([Type.Literal("CONFIRMED"), Type.Literal("PLAUSIBLE"), Type.Literal("REFUTED"), Type.Literal("NOT_SUBSTANTIATED")], { description: "Verifier judgment for a candidate finding.", }); @@ -20,6 +20,8 @@ export const AdvisoryLocationSchema = Type.Object({ }); export const AdvisoryCandidateSchema = Type.Object({ + discoveryEvidence: Type.Optional(Type.Array(Type.String())), + reviewAnchor: Type.Optional(AdvisoryLocationSchema), summary: Type.String({ description: "One-line candidate finding or hypothesis." }), category: Type.String({ description: "Workflow-specific category such as bug, duplication, root-cause, or io." }), locations: Type.Array(AdvisoryLocationSchema, { description: "Relevant code or configuration locations." }), @@ -38,6 +40,9 @@ export const AdvisoryVerdictSchema = Type.Object({ }); export const AdvisoryFindingSchema = Type.Object({ + verdict: Type.Optional(AdvisoryVerifierVerdictSchema), + sourceCandidateIds: Type.Optional(Type.Array(Type.String())), + reviewAnchor: Type.Optional(AdvisoryLocationSchema), summary: Type.String({ description: "One-line final finding." }), category: Type.String({ description: "Workflow-specific category for grouping and display." }), severity: AdvisorySeveritySchema, @@ -49,6 +54,12 @@ export const AdvisoryFindingSchema = Type.Object({ }); export const AdvisoryReportSchema = Type.Object({ + status: Type.Optional(Type.Union([Type.Literal("complete"), Type.Literal("incomplete")])), + gaps: Type.Optional(Type.Array(Type.String())), + coverage: Type.Optional(Type.Array(Type.Object({ + stage: Type.String(), expected: Type.Number(), completed: Type.Number(), failed: Type.Number(), + failures: Type.Array(Type.Object({ branch: Type.String(), reason: Type.String(), candidate: Type.Optional(AdvisoryCandidateSchema) })), + }))), summary: Type.String({ description: "Short overall advisory summary." }), findings: Type.Array(AdvisoryFindingSchema, { description: "Verified and ranked advisory findings." }), nextSteps: Type.Array(Type.String({ description: "Concrete follow-up commands, inspections, or decisions." })), @@ -61,6 +72,7 @@ export const AdvisoryReportWithStatsSchema = Type.Object({ export type AdvisoryLocation = Static; export type AdvisoryCandidate = Static; +export type IdentifiedAdvisoryCandidate = AdvisoryCandidate & { candidateId: string; sourceCandidateIds: string[] }; export type AdvisoryVerdict = Static; export type AdvisoryFinding = Static; export type AdvisoryReport = Static; diff --git a/.pi/extensions/pi-workflow-engine/src/agent-workspace.ts b/.pi/extensions/pi-workflow-engine/src/agent-workspace.ts index f3b6325..4cc1028 100644 --- a/.pi/extensions/pi-workflow-engine/src/agent-workspace.ts +++ b/.pi/extensions/pi-workflow-engine/src/agent-workspace.ts @@ -21,7 +21,7 @@ export type AgentWorkspace = SharedAgentWorkspace | IsolatedAgentWorkspace; export interface AgentWorkspaceContext { readonly cwd: string; - readonly worktrees: Pick; + readonly worktrees: Pick; readonly signal: AbortSignal | undefined; readonly progress: Pick; } @@ -31,6 +31,7 @@ export async function createAgentWorkspace( opts: AgentExecutionOptions, label: string, ): Promise { + if (opts.candidatePatch && opts.isolation !== "worktree") throw new Error("Candidate evaluation requires worktree isolation"); if (opts.isolation !== "worktree") { return { kind: "shared", @@ -53,6 +54,19 @@ export async function createAgentWorkspace( const added = await rc.worktrees.add(rc.signal, opts.worktreeBaseline); if ("error" in added) throw new Error(`Failed to create isolated worktree: ${added.error}`); const worktreePath = added.path; + if (opts.candidatePatch) { + try { + if (opts.candidatePatch.baselineOid !== added.baselineOid) throw new Error("Candidate baseline differs from evaluator baseline"); + if (opts.candidatePatch.patch.trim()) { + const applied = await rc.worktrees.applyPatch(worktreePath, opts.candidatePatch.patch, rc.signal); + if (!applied.ok) throw new Error(`Candidate patch could not be applied: ${applied.error ?? applied.stderr}`); + } + } catch (error) { + const removed = await rc.worktrees.remove(worktreePath); + if (!removed.ok) rc.progress.log(`${label}: evaluator setup cleanup failed: ${removed.error ?? removed.stderr}`); + throw error; + } + } rc.progress.log(`${label}: using isolated worktree ${worktreePath}`); return { @@ -62,7 +76,7 @@ export async function createAgentWorkspace( async wrapResult(result) { const patch = await rc.worktrees.capturePatch(worktreePath, added.baselineOid, rc.signal); if ("error" in patch) throw new Error(`Failed to capture isolated worktree patch: ${patch.error}`); - return { result, patch: patch.patch, changed: patch.changed }; + return { result, patch: patch.patch, changed: patch.changed, baselineOid: added.baselineOid }; }, async dispose() { const removed = await rc.worktrees.remove(worktreePath); diff --git a/.pi/extensions/pi-workflow-engine/src/concurrency.ts b/.pi/extensions/pi-workflow-engine/src/concurrency.ts index a03bd90..5c911e0 100644 --- a/.pi/extensions/pi-workflow-engine/src/concurrency.ts +++ b/.pi/extensions/pi-workflow-engine/src/concurrency.ts @@ -14,22 +14,28 @@ export class Semaphore { private active = 0; private readonly waiters: Array<() => void> = []; - constructor(private readonly max: number) {} + constructor(private readonly max: number) { + if (!Number.isInteger(max) || max < 1) throw new RangeError("Semaphore capacity must be a positive integer"); + } async run(fn: () => Promise, options: { onQueueWaitMs?: (durationMs: number) => void; signal?: AbortSignal } = {}): Promise { throwIfAborted(options.signal); const queuedAt = performance.now(); if (this.active >= this.max) { await this.waitForSlot(options.signal); + } else { + this.active++; } - options.onQueueWaitMs?.(performance.now() - queuedAt); - throwIfAborted(options.signal); - this.active++; try { + options.onQueueWaitMs?.(performance.now() - queuedAt); + throwIfAborted(options.signal); return await fn(); } finally { - this.active--; - this.waiters.shift()?.(); + // Transfer ownership directly. A selected waiter owns this reservation + // even before its continuation runs, including if it is then cancelled. + const next = this.waiters.shift(); + if (next) next(); + else this.active--; } } diff --git a/.pi/extensions/pi-workflow-engine/src/journal.ts b/.pi/extensions/pi-workflow-engine/src/journal.ts index 97af638..9bc1a14 100644 --- a/.pi/extensions/pi-workflow-engine/src/journal.ts +++ b/.pi/extensions/pi-workflow-engine/src/journal.ts @@ -115,6 +115,7 @@ function captureAgentCallHash( skills: opts.skills, schema: opts.schema, isolation: opts.isolation, + candidatePatch: opts.candidatePatch ? { baselineOid: opts.candidatePatch.baselineOid, hash: createHash("sha256").update(opts.candidatePatch.patch).digest("hex") } : undefined, worktreeBaseline: worktreeBaseline ? { ref: worktreeBaseline.ref, diff --git a/.pi/extensions/pi-workflow-engine/src/review/code-review-orchestration.ts b/.pi/extensions/pi-workflow-engine/src/review/code-review-orchestration.ts index 81e0929..cc3b6a0 100644 --- a/.pi/extensions/pi-workflow-engine/src/review/code-review-orchestration.ts +++ b/.pi/extensions/pi-workflow-engine/src/review/code-review-orchestration.ts @@ -1,6 +1,3 @@ -import type { AdvisoryCandidate } from "../advisory-schema.ts"; -import { normalizePath, primaryLocation } from "../workflow-advisory-utils.ts"; - const DIFF_EMBED_CAP = 60_000; export function buildCodeReviewScopeBlock(input: { @@ -25,25 +22,3 @@ export function buildCodeReviewScopeBlock(input: { (input.target ? `\n## User instructions (verbatim)\n${input.target}\n` : "") ); } - -export function dedupeCodeReviewCandidates( - groups: readonly { readonly angle: Context; readonly candidates: readonly AdvisoryCandidate[] }[], -): Array<{ readonly angle: Context; readonly candidate: AdvisoryCandidate }> { - const seen = new Set(); - return groups.flatMap(({ angle, candidates }) => - candidates - .filter((candidate) => { - const key = codeReviewDedupKey(candidate); - if (seen.has(key)) return false; - seen.add(key); - return true; - }) - .map((candidate) => ({ angle, candidate })), - ); -} - -function codeReviewDedupKey(candidate: AdvisoryCandidate): string { - const location = primaryLocation(candidate); - const lineKey = location.line != null ? Math.round(location.line / 5) * 5 : candidate.summary.slice(0, 40).toLowerCase(); - return `${normalizePath(location.file)}:${lineKey}`; -} diff --git a/.pi/extensions/pi-workflow-engine/src/review/patch-validation.ts b/.pi/extensions/pi-workflow-engine/src/review/patch-validation.ts new file mode 100644 index 0000000..40cfa82 --- /dev/null +++ b/.pi/extensions/pi-workflow-engine/src/review/patch-validation.ts @@ -0,0 +1,120 @@ +import { createHash } from "node:crypto"; +import { Type, type Static } from "typebox"; +import { throwIfAborted } from "../cancellation.ts"; +import { runBoundedProcess, type BoundedProcessResult } from "../process-runner.ts"; +import { WorktreeRegistry, type WorktreeBaseline } from "../worktree.ts"; +import { unknownErrorMessage } from "../unknown-error.ts"; +import { fingerprintReviewWorktreeBaseline } from "./review-snapshot.ts"; + +export const PatchEvaluationSchema = Type.Object({ + outcome: Type.Union([Type.Literal("accepted"), Type.Literal("rejected"), Type.Literal("blocked")]), + reason: Type.String(), + checks: Type.Array(Type.Object({ + file: Type.String({ description: "Executable for a focused validation command, not a shell command string." }), + args: Type.Array(Type.String()), + required: Type.Boolean(), + regression: Type.Optional(Type.Object({ + baselinePatch: Type.String({ description: "Test-only patch to apply to the original baseline to reproduce the bug; exclude the repair." }), + expectedFailure: Type.String({ description: "Specific failure text proving the intended defect, not a missing dependency or setup error." }), + })), + }), { maxItems: 6 }), +}); +export type PatchEvaluation = Static; +export type PatchCandidateStatus = "verified" | "rejected" | "blocked" | "no-patch"; +export interface PatchValidation { + status: PatchCandidateStatus; + baselineFingerprint: string; + baselineOid?: string; + patchHash: string; + checks: { file: string; args: string[]; required: boolean; stage: "candidate" | "baseline"; result: BoundedProcessResult }[]; + evaluation?: PatchEvaluation; + reason: string; + cleanupError?: string; +} +export function initialPatchValidation(baseline: WorktreeBaseline, baselineOid: string | undefined, patch: string): PatchValidation { + return { status: patch.trim() ? "blocked" : "no-patch", baselineFingerprint: fingerprintReviewWorktreeBaseline(baseline), ...(baselineOid ? { baselineOid } : {}), + patchHash: createHash("sha256").update(patch).digest("hex"), checks: [], reason: patch.trim() ? "Independent evaluation has not completed." : "The implementer produced no patch; this does not establish that no change is needed." }; +} + +interface PatchValidationOptions { + cwd: string; baseline: WorktreeBaseline; expectedFingerprint: string; baselineOid: string; patch: string; + evaluation: PatchEvaluation; signal?: AbortSignal; +} + +/** Reconstruct the exact reviewed baseline and retain evidence even if cleanup fails. */ +export async function validateCandidatePatch(options: PatchValidationOptions): Promise { + const validation = initialPatchValidation(options.baseline, options.baselineOid, options.patch); + validation.evaluation = options.evaluation; + if (validation.baselineFingerprint !== options.expectedFingerprint) return { ...validation, status: "rejected", reason: "Stale reviewed baseline identity." }; + if (!options.patch.trim()) return validation; + const worktrees = new WorktreeRegistry(options.cwd); + let result = validation; + try { + result = await evaluateCandidateInWorktrees(worktrees, options, validation); + } catch (error) { + throwIfAborted(options.signal); + result = { ...validation, status: "blocked", reason: unknownErrorMessage(error) }; + } finally { + try { + await worktrees.removeAll(); + } catch (error) { + const cleanupError = unknownErrorMessage(error); + result = { + ...result, + status: result.status === "verified" ? "blocked" : result.status, + cleanupError, + reason: `${result.reason}\nCleanup failed: ${cleanupError}`, + }; + } + throwIfAborted(options.signal); + } + return result; +} + +async function evaluateCandidateInWorktrees( + worktrees: WorktreeRegistry, options: PatchValidationOptions, validation: PatchValidation, +): Promise { + const candidate = await worktrees.add(options.signal, options.baseline); + if ("error" in candidate) return { ...validation, reason: candidate.error }; + if (candidate.baselineOid !== options.baselineOid) return { ...validation, status: "rejected", reason: "Candidate was produced from a different baseline." }; + const applied = await worktrees.applyPatch(candidate.path, options.patch, options.signal); + if (!applied.ok) return { ...validation, status: "rejected", reason: applied.error ?? applied.stderr }; + const rejected = options.evaluation.outcome === "rejected" ? [options.evaluation.reason] : []; + const blocked = options.evaluation.outcome === "blocked" ? [options.evaluation.reason] : []; + if (!options.evaluation.checks.some((check) => check.required)) blocked.push("No required behavior check was selected."); + for (const check of options.evaluation.checks) { + const result = await runCheck(check, candidate.path, options.signal); + validation.checks.push({ ...check, stage: "candidate", result }); + throwIfAborted(options.signal); + if (!result.ok) { + if (result.failure.kind === "exit") rejected.push(`${check.file}: ${result.failure.message}`); + else if (check.required) blocked.push(`${check.file}: ${result.failure.message}`); + } + if (check.regression) { + const original = await worktrees.add(options.signal, options.baseline); + if ("error" in original) { blocked.push(`Baseline worktree setup failed: ${original.error}`); continue; } + const testApplied = await worktrees.applyPatch(original.path, check.regression.baselinePatch, options.signal); + if (!testApplied.ok) { blocked.push(`Baseline regression setup failed: ${testApplied.error ?? testApplied.stderr}`); continue; } + const baselineResult = await runCheck(check, original.path, options.signal); + validation.checks.push({ ...check, stage: "baseline", result: baselineResult }); + throwIfAborted(options.signal); + if (baselineResult.ok || baselineResult.failure.kind !== "exit" || !check.regression.expectedFailure.trim() || + !(baselineResult.stdout + baselineResult.stderr).includes(check.regression.expectedFailure)) { + blocked.push(`${check.file}: baseline did not reproduce the expected failure: ${check.regression.expectedFailure}`); + } + } + } + // Tests must not silently replace the candidate under evaluation. + const after = await worktrees.capturePatch(candidate.path, candidate.baselineOid, options.signal); + if ("error" in after) blocked.push(`Candidate capture failed: ${after.error}`); + else if (after.patch !== options.patch) blocked.push("Validation checks changed the candidate patch."); + if (rejected.length > 0) return { ...validation, status: "rejected", reason: [...rejected, ...blocked].join("\n") }; + if (blocked.length > 0) return { ...validation, status: "blocked", reason: blocked.join("\n") }; + return { ...validation, status: "verified", reason: "Independent evaluation and all required checks passed." }; +} + +function runCheck(check: PatchEvaluation["checks"][number], cwd: string, signal?: AbortSignal): Promise { + return runBoundedProcess({ file: check.file, args: check.args, cwd, signal, timeoutMs: 120_000, maxBufferBytes: 1 << 20, + abortError: "Validation aborted", timeoutError: "Validation timed out", maxBufferError: "Validation output exceeded its limit", + exitError: (stderr, code) => stderr.trim() || `Validation exited with ${code}` }); +} diff --git a/.pi/extensions/pi-workflow-engine/src/review/review-fix-workflow.ts b/.pi/extensions/pi-workflow-engine/src/review/review-fix-workflow.ts index 749c285..98ccf18 100644 --- a/.pi/extensions/pi-workflow-engine/src/review/review-fix-workflow.ts +++ b/.pi/extensions/pi-workflow-engine/src/review/review-fix-workflow.ts @@ -1,5 +1,8 @@ +import { initialPatchValidation, PatchEvaluationSchema, validateCandidatePatch, type PatchValidation } from "./patch-validation.ts"; +import { isFatalWorkflowError } from "../cancellation.ts"; +import { unknownErrorMessage } from "../unknown-error.ts"; import type { ParallelSettledError } from "../concurrency.ts"; -import type { LoadedWorkflow, WorkflowApi, WorkflowModule } from "../types.ts"; +import type { IsolatedAgentResult, LoadedWorkflow, WorkflowApi, WorkflowModule } from "../types.ts"; import type { WorktreeBaseline } from "../worktree.ts"; import { loadWorkflow } from "../workflow-module.ts"; import { serializeReviewIssue, type ReviewIssue } from "./review-issues.ts"; @@ -14,6 +17,7 @@ export interface ReviewFixPreview { readonly result: string; readonly patch: string; readonly changed: boolean; + readonly validation: PatchValidation; } export interface ReviewFixFailure { @@ -28,7 +32,7 @@ export interface ReviewFixWorkflowResult { readonly fixes: readonly ReviewFixOutcome[]; } -export type ReviewFixWorkflowApi = Pick; +export type ReviewFixWorkflowApi = Pick; /** Build an ephemeral workflow that generates one isolated patch preview per finding. */ export function createReviewFixWorkflow( @@ -40,9 +44,9 @@ export function createReviewFixWorkflow( meta: { name: "code-review-fix-previews", description: "Generate isolated patch previews for selected code-review findings.", - phases: [{ title: REVIEW_FIX_PHASE }], + phases: [{ title: REVIEW_FIX_PHASE }, { title: "Validate patch previews" }], }, - default: (api) => runReviewFixWorkflow(api, issues, context), + default: (api) => runReviewFixWorkflow(api, issues, context, baseline), }; return loadWorkflow( module, @@ -76,6 +80,7 @@ export async function runReviewFixWorkflow( api: ReviewFixWorkflowApi, issues: readonly ReviewIssue[], context: ReviewContext | undefined, + baseline: WorktreeBaseline, ): Promise { api.phase(REVIEW_FIX_PHASE); const settled = await api.parallel( @@ -84,16 +89,18 @@ export async function runReviewFixWorkflow( isolation: "worktree", label: `fix:${issue.id}`, phase: REVIEW_FIX_PHASE, - thinkingLevel: "medium", + profile: "medium", cacheKey: `review-fix:${issue.id}`, tools: [...REVIEW_FIX_TOOLS], toolHints: ["search"], }); + const validation = await evaluateReviewFix(api, { issue, context, baseline, isolated }); return { findingId: issue.id, result: isolated.result, patch: isolated.patch, changed: isolated.changed, + validation, }; }), { settled: true }, @@ -103,11 +110,11 @@ export async function runReviewFixWorkflow( entry.ok ? entry.value : { findingId: issues[index]!.id, error: entry.error }, ); const successful = fixes.filter(isReviewFixPreview); - const changed = successful.filter((fix) => fix.changed).length; + const count = (status: PatchValidation["status"]) => successful.filter((fix) => fix.validation.status === status).length; const failed = fixes.length - successful.length; return { - summary: `Generated ${changed} patch preview(s); ${successful.length - changed} finding(s) needed no changes; ${failed} attempt(s) failed.`, + summary: `${count("verified")} verified candidate(s); ${count("rejected")} rejected; ${count("blocked")} blocked; ${count("no-patch")} no-patch; ${failed} attempt(s) failed.`, fixes, }; } @@ -115,3 +122,32 @@ export async function runReviewFixWorkflow( function isReviewFixPreview(outcome: ReviewFixOutcome): outcome is ReviewFixPreview { return "patch" in outcome; } + +async function evaluateReviewFix(api: ReviewFixWorkflowApi, input: { + issue: ReviewIssue; context: ReviewContext | undefined; baseline: WorktreeBaseline; isolated: IsolatedAgentResult; +}): Promise { + const { issue, context, baseline, isolated } = input; + const validation = initialPatchValidation(baseline, isolated.baselineOid, isolated.patch); + if (!isolated.patch.trim()) return validation; + if (!isolated.baselineOid) return { ...validation, reason: "Candidate has no recorded baseline identity." }; + if (!context?.snapshot) return { ...validation, reason: "Reviewed snapshot identity is unavailable." }; + if (validation.baselineFingerprint !== context.snapshot.baselineFingerprint) { + return { ...validation, status: "rejected", reason: "Stale reviewed baseline identity." }; + } + try { + const evaluated = await api.agent( + `Independently evaluate a candidate repair. Your fresh worktree contains the exact reviewed baseline plus the captured patch. The implementer's report is not validation evidence. Inspect the finding, callers and tests. Reject incorrect repairs; return blocked if required validation is unavailable. Select at most six focused deterministic checks with executable and argument arrays. Require at least one meaningful behavior check. If a regression test is applicable, supply a test-only baselinePatch and specific expectedFailure so the engine can prove it fails before the repair and passes after. Do not edit, install dependencies, commit or change branches. The engine will execute checks independently.\nFinding: ${JSON.stringify(serializeReviewIssue(issue))}\nBaseline: ${isolated.baselineOid}\nPatch SHA-256: ${validation.patchHash}\nPatch:\n${isolated.patch}`, + { isolation: "worktree", candidatePatch: { baselineOid: isolated.baselineOid, patch: isolated.patch }, + label: `evaluate:${issue.id}`, phase: "Validate patch previews", profile: "medium", resume: "off", + tools: ["read", "bash", "grep", "find", "ls"], toolHints: ["search"], schema: PatchEvaluationSchema }, + ); + if (evaluated.baselineOid !== isolated.baselineOid || evaluated.patch !== isolated.patch) { + return { ...validation, status: "rejected", reason: "Evaluator changed the candidate or used a different baseline.", evaluation: evaluated.result }; + } + return await validateCandidatePatch({ cwd: api.cwd, baseline, expectedFingerprint: context.snapshot.baselineFingerprint, + baselineOid: isolated.baselineOid, patch: isolated.patch, evaluation: evaluated.result, signal: api.signal }); + } catch (error) { + if (isFatalWorkflowError(error, api.signal)) throw error; + return { ...validation, status: "blocked", reason: unknownErrorMessage(error) }; + } +} diff --git a/.pi/extensions/pi-workflow-engine/src/review/review-format.ts b/.pi/extensions/pi-workflow-engine/src/review/review-format.ts index 59c878a..0bf7bfb 100644 --- a/.pi/extensions/pi-workflow-engine/src/review/review-format.ts +++ b/.pi/extensions/pi-workflow-engine/src/review/review-format.ts @@ -55,9 +55,10 @@ export function renderIssueDetails(issue: ReviewIssue, theme: Theme): string { export function renderIssueDetailLines(issue: ReviewIssue, theme: Theme, width: number): string[] { const finding = issue.finding; - const metadata = `${finding.category} · severity ${finding.severity} · confidence ${finding.confidence}`; + const metadata = `${finding.category} · severity ${finding.severity} · confidence ${finding.confidence}${finding.verdict ? ` · ${finding.verdict}` : ""}`; const lines = [`${theme.fg("accent", issue.id)} ${theme.fg("text", finding.summary)}`]; lines.push(...fieldLines("Metadata", metadata, width, theme)); + if (finding.sourceCandidateIds?.length) lines.push(...fieldLines("Sources", finding.sourceCandidateIds.join(", "), width, theme)); lines.push(...fieldLines("Location", formatIssueLocation(issue), width, theme, "accent")); lines.push(...fieldLines("Impact", finding.impact, width, theme)); lines.push(...fieldLines("Evidence", finding.evidence.join("; ") || "(none cited)", width, theme)); diff --git a/.pi/extensions/pi-workflow-engine/src/review/review-issues.ts b/.pi/extensions/pi-workflow-engine/src/review/review-issues.ts index 7f4da63..331ebc0 100644 --- a/.pi/extensions/pi-workflow-engine/src/review/review-issues.ts +++ b/.pi/extensions/pi-workflow-engine/src/review/review-issues.ts @@ -37,7 +37,7 @@ export interface SerializedReviewIssue { export function toReviewIssues(name: string, report: Pick): ReviewIssue[] { return report.findings.map((finding, index) => { - const location = finding.locations[0]; + const location = finding.reviewAnchor ?? finding.locations[0]; return { id: formatIssueId(index), index, diff --git a/.pi/extensions/pi-workflow-engine/src/types.ts b/.pi/extensions/pi-workflow-engine/src/types.ts index f91104f..28e8fed 100644 --- a/.pi/extensions/pi-workflow-engine/src/types.ts +++ b/.pi/extensions/pi-workflow-engine/src/types.ts @@ -98,6 +98,7 @@ export type AgentToolHint = "search" | "external-search"; export type AgentResumePolicy = "read-only" | "off"; export interface IsolatedAgentResult { + readonly baselineOid?: string; readonly result: T; readonly patch: string; readonly changed: boolean; @@ -152,6 +153,8 @@ export interface AgentOptions { resumeInputs?: readonly string[]; /** Run this agent in a disposable git worktree and return its patch with the result. */ isolation?: "worktree"; + /** Apply a captured candidate to a fresh isolated baseline before evaluation. */ + candidatePatch?: { readonly baselineOid: string; readonly patch: string }; /** Allowlist of concrete tool names the agent may use (e.g. ["read", "bash"]). */ tools?: string[]; /** diff --git a/.pi/extensions/pi-workflow-engine/src/ui/workflow-result-renderer.ts b/.pi/extensions/pi-workflow-engine/src/ui/workflow-result-renderer.ts index 347bbe5..fb1d63d 100644 --- a/.pi/extensions/pi-workflow-engine/src/ui/workflow-result-renderer.ts +++ b/.pi/extensions/pi-workflow-engine/src/ui/workflow-result-renderer.ts @@ -62,15 +62,18 @@ function renderAdvisoryResult( metadata?: WorkflowRunDisplayMetadata, perf?: WorkflowPerfDetails, ): string { - const icon = theme.fg("success", "✓"); + const incomplete = result.status === "incomplete"; + const icon = incomplete ? theme.fg("warning", "⚠") : theme.fg("success", "✓"); const title = theme.fg("accent", theme.bold(`Workflow: ${name}`)); const lines = [`${icon} ${title}`, theme.fg("muted", result.summary)]; + if (result.coverage?.length) lines.push(theme.fg("dim", result.coverage.map((stage) => `${stage.stage}: ${stage.completed}/${stage.expected} complete, ${stage.failed} failed`).join(" · "))); + if (expanded) for (const gap of result.gaps ?? []) lines.push(theme.fg("warning", gap)); const stats = statsLine(result.stats, theme); if (stats) lines.push(stats); pushWorkflowDetailLines(lines, theme, { usage, metadata, perf }); if (result.findings.length === 0) { - lines.push(theme.fg("success", "No findings.")); + lines.push(incomplete ? theme.fg("warning", "No verified findings; coverage is incomplete.") : theme.fg("success", "No findings.")); if (expanded && result.nextSteps.length > 0) renderNextSteps(result.nextSteps, lines, theme); return lines.join("\n"); } diff --git a/.pi/extensions/pi-workflow-engine/src/workflow-advisory-utils.ts b/.pi/extensions/pi-workflow-engine/src/workflow-advisory-utils.ts index 704759b..dfb674e 100644 --- a/.pi/extensions/pi-workflow-engine/src/workflow-advisory-utils.ts +++ b/.pi/extensions/pi-workflow-engine/src/workflow-advisory-utils.ts @@ -1,5 +1,5 @@ -import { AdvisoryCandidatesSchema, AdvisoryVerdictSchema, type AdvisoryCandidate, type AdvisoryFinding, type AdvisoryLocation, type AdvisoryReport, type AdvisoryVerdict } from "./advisory-schema.ts"; -import { compactResults } from "./concurrency.ts"; +import { AdvisoryCandidatesSchema, AdvisoryVerdictSchema, type AdvisoryCandidate, type IdentifiedAdvisoryCandidate, type AdvisoryLocation, type AdvisoryReport, type AdvisoryVerdict } from "./advisory-schema.ts"; +import { AdvisorySynthesisSchema, SYNTHESIS_ID_INSTRUCTIONS, withAdvisoryCoverage, collectAdvisoryStage, dedupeCandidates, identifyCandidates, uniqueLocations, type AdvisoryStageCoverage, type AdvisorySynthesis } from "./advisory-evidence.ts"; import type { AgentOptions, WorkflowApi, WorkflowProgressEvent, WorkflowRunStats } from "./types.ts"; export interface AdvisoryLens { @@ -8,17 +8,19 @@ export interface AdvisoryLens { text: string; } -export type AdvisoryVerified = Candidate & { +export type AdvisoryVerified = IdentifiedAdvisoryCandidate & { verdict: AdvisoryVerdict["verdict"]; evidence: string[]; confidence?: AdvisoryVerdict["confidence"]; + challenge?: import("./advisory-challenge.ts").ChallengeRecord; }; -export interface LensVerificationPipelineResult { - verified: Verified[]; +export interface LensVerificationPipelineResult { + verified: AdvisoryVerified[]; rawCandidates: number; dropped: number; refuted: number; + coverage: AdvisoryStageCoverage[]; } /** Default concrete read/inspect tools for advisory workflows. */ @@ -47,20 +49,14 @@ export function publishVerifiedKeptProgress( api.log(`${verified} verified → ${kept} kept`); } -export type AdvisorySchedulingMode = "pipeline" | "finder-barrier"; - -export interface LensVerificationPipelineOptions { - api: Pick; - lenses: readonly Lens[]; +export interface LensVerificationPipelineOptions { + api: Pick; + lenses: readonly AdvisoryLens[]; perLens: number; - tools?: AgentOptions["tools"]; - toolHints?: AgentOptions["toolHints"]; - finderPhase?: string; - verifierPhase?: string; - schedulingMode?: AdvisorySchedulingMode; - finderPrompt(lens: Lens): string; - verifierPrompt(candidate: AdvisoryCandidate): string; - makeVerified(candidate: AdvisoryCandidate, lens: Lens, verdict: AdvisoryVerdict): Verified; + finderPhase?: "Find" | "Hypothesize"; + finderPrompt(lens: AdvisoryLens): string; + verifierPrompt(candidate: IdentifiedAdvisoryCandidate): string; + boundCandidate?(candidate: IdentifiedAdvisoryCandidate, lens: AdvisoryLens): IdentifiedAdvisoryCandidate | undefined; } export interface AdvisoryBackfillDefaults { @@ -68,117 +64,77 @@ export interface AdvisoryBackfillDefaults { recommendation?: string; } -interface FoundForLens { - lens: Lens; - candidates: AdvisoryCandidate[]; -} - -interface NovelCandidate { - lens: Lens; - candidate: AdvisoryCandidate; -} - -export async function runLensVerificationPipeline( - options: LensVerificationPipelineOptions, -): Promise> { - const { - api, - lenses, - perLens, - tools = DEFAULT_ADVISORY_TOOLS, - toolHints = DEFAULT_ADVISORY_TOOL_HINTS, - finderPhase = "Find", - verifierPhase = "Verify", - schedulingMode = "pipeline", - finderPrompt, - verifierPrompt, - makeVerified, - } = options; - const seen = new Set(); +/** Finish all discovery before verification competes for the shared agent limit. */ +export async function runLensVerificationPipeline( + options: LensVerificationPipelineOptions, +): Promise { + const { api, lenses, perLens, finderPhase = "Find", finderPrompt, verifierPrompt, boundCandidate } = options; + const coverage: AdvisoryStageCoverage[] = []; let rawCandidates = 0; - let dropped = 0; let refuted = 0; - - const findForLens = async (lens: Lens): Promise> => { - const found = await api.agent(finderPrompt(lens), { - phase: finderPhase, - label: `find:${lens.label}`, - tools, - toolHints, - profile: "small", - schema: AdvisoryCandidatesSchema, + api.phase(finderPhase); + const found = await collectAdvisoryStage(api, "Find", lenses.map((lens) => ({ id: lens.label, run: async () => { + const result = await api.agent(finderPrompt(lens), { + phase: finderPhase, label: `${finderPhase.toLowerCase()}:${lens.label}`, + tools: DEFAULT_ADVISORY_TOOLS, toolHints: DEFAULT_ADVISORY_TOOL_HINTS, + profile: "small", schema: AdvisoryCandidatesSchema, }); - const candidates = (found?.candidates ?? []).slice(0, perLens); - rawCandidates += candidates.length; - api.progress({ type: "counter_delta", key: "candidates", label: "candidates", delta: candidates.length }); + const raw = identifyCandidates(result.candidates.slice(0, perLens), lens.label); + rawCandidates += raw.length; + api.progress({ type: "counter_delta", key: "candidates", label: "candidates", delta: raw.length }); + const candidates = boundCandidate ? raw.flatMap((candidate) => { + const bounded = boundCandidate(candidate, lens); + return bounded ? [bounded] : []; + }) : raw; for (const candidate of candidates) { - api.progress({ - type: "lane_item", - lane: "Candidates", - title: candidate.summary, - subtitle: formatLocation(candidate), - status: "pending", - details: candidate.impact, - }); + api.progress({ type: "lane_item", lane: finderPhase === "Hypothesize" ? "Hypotheses" : "Candidates", + title: candidate.summary, subtitle: formatLocation(candidate), status: "pending", details: candidate.impact }); } - return { lens, candidates }; - }; - - const dedupeFound = (found: FoundForLens): NovelCandidate[] => { - const novel = candidatesNovelToRun(found.candidates, seen).map((candidate) => ({ lens: found.lens, candidate })); - const droppedForLens = found.candidates.length - novel.length; - if (droppedForLens > 0) { - dropped += droppedForLens; - api.progress({ type: "counter_delta", key: "dropped", label: "dropped", delta: droppedForLens }); - api.log(`find:${found.lens.label}: dropped ${droppedForLens} duplicate candidate(s)`); - } - return novel; - }; - - const verifyCandidate = async ({ lens, candidate }: NovelCandidate): Promise => { - const location = primaryLocation(candidate); - const judged = await api.agent(verifierPrompt(candidate), { - phase: verifierPhase, - label: `verify:${location.file.split("/").pop() ?? location.file}`, - tools, - toolHints, - profile: "small", - schema: AdvisoryVerdictSchema, - }); - if (!judged) return null; - recordVerdictProgress(api.progress, candidate, judged, () => { - refuted += 1; - }); - return makeVerified(candidate, lens, judged); - }; - - if (schedulingMode === "finder-barrier") { - const found = await api.parallel(lenses.map((lens) => async () => findForLens(lens))); - const novel = compactResults(found).flatMap(dedupeFound); - const verdicts = await api.parallel(novel.map((entry) => async () => verifyCandidate(entry))); - return { verified: compactResults(verdicts), rawCandidates, dropped, refuted }; + return candidates; + } })), coverage); + const candidates = dedupeCandidates(found.flat()); + const dropped = rawCandidates - candidates.length; + if (dropped > 0) { + api.progress({ type: "counter_delta", key: "dropped", label: "dropped", delta: dropped }); + api.log(`Dropped ${dropped} duplicate or out-of-scope candidate(s)`); } - - const perLensVerified = await api.pipeline( - lenses, - async (_prev, lens) => findForLens(lens), - async (found) => { - const novel = dedupeFound(found); - const verdicts = await api.parallel(novel.map((entry) => async () => verifyCandidate(entry))); - return compactResults(verdicts); + api.phase("Verify"); + const verified = await collectAdvisoryStage(api, "Verify", candidates.map((candidate) => ({ + id: candidate.candidateId, candidate, run: async (): Promise => { + const location = primaryLocation(candidate); + const judged = await api.agent(`${verifierPrompt(candidate)}\nCandidate record: ${JSON.stringify(candidate)}`, { + phase: "Verify", label: `verify:${location.file.split("/").pop() ?? location.file}`, + tools: DEFAULT_ADVISORY_TOOLS, toolHints: DEFAULT_ADVISORY_TOOL_HINTS, + profile: "small", schema: AdvisoryVerdictSchema, + }); + recordVerdictProgress(api.progress, candidate, judged, () => { refuted += 1; }); + return { ...candidate, verdict: judged.verdict, evidence: judged.evidence, confidence: judged.confidence }; }, - ); + })), coverage); + return { verified, rawCandidates, dropped, refuted, coverage }; +} - return { verified: compactResults(perLensVerified).flat(), rawCandidates, dropped, refuted }; +export async function synthesizeAdvisoryReport( + api: Pick, + prompt: string, + ranked: readonly AdvisoryVerified[], + coverage: AdvisoryStageCoverage[], +): Promise { + api.phase("Synthesize"); + const [report] = await collectAdvisoryStage(api, "Synthesize", [{ id: "synthesize", run: () => api.agent( + SYNTHESIS_ID_INSTRUCTIONS + prompt, + { phase: "Synthesize", label: "synthesize", tools: [], profile: "medium", resume: "read-only", schema: AdvisorySynthesisSchema }, + ) }], coverage); + return resolveAdvisorySynthesis(report, ranked, { + impact: "Impact not restated by verification.", + recommendation: "Inspect the cited evidence and validate the smallest repair.", + }, coverage); } -function candidatesNovelToRun(candidates: readonly AdvisoryCandidate[], seen: Set): AdvisoryCandidate[] { - return candidates.filter((candidate) => { - const key = advisoryDedupKey(candidate); - if (seen.has(key)) return false; - seen.add(key); - return true; - }); +export function finishAdvisoryReport( + report: T, coverage: AdvisoryStageCoverage[], verification: AdvisoryVerified[] = [], +) { + return { ...withAdvisoryCoverage(report, coverage), verification }; } export function primaryLocation(candidate: Pick): AdvisoryLocation { @@ -200,52 +156,17 @@ export function normalizePath(path: string): string { return path.replace(/^\.\//, "").replace(/^[ab]\//, ""); } -export function advisoryDedupKey(candidate: AdvisoryCandidate): string { - const location = primaryLocation(candidate); - const lineKey = location.line != null ? Math.round(location.line / 5) * 5 : "file"; - return `${candidate.category}:${normalizePath(location.file)}:${lineKey}:${candidate.summary.slice(0, 60).toLowerCase()}`; -} - -export function verdictLane(verdict: AdvisoryVerdict["verdict"]): string { - switch (verdict) { - case "CONFIRMED": - return "Confirmed"; - case "PLAUSIBLE": - return "Plausible"; - case "REFUTED": - return "Refuted"; - } -} - -export function verdictStatus(verdict: AdvisoryVerdict["verdict"]): "success" | "warning" | "error" { - switch (verdict) { - case "CONFIRMED": - return "success"; - case "PLAUSIBLE": - return "warning"; - case "REFUTED": - return "error"; - } -} +const VERDICT_PRESENTATION = { + CONFIRMED: { lane: "Confirmed", status: "success", confidence: "high" }, + PLAUSIBLE: { lane: "Plausible", status: "warning", confidence: "medium" }, + NOT_SUBSTANTIATED: { lane: "Unresolved", status: "warning", confidence: "medium" }, + REFUTED: { lane: "Refuted", status: "error", confidence: "low" }, +} satisfies Record; +}>; export function verdictConfidence(verdict: AdvisoryVerdict["verdict"]): "high" | "medium" | "low" { - switch (verdict) { - case "CONFIRMED": - return "high"; - case "PLAUSIBLE": - return "medium"; - case "REFUTED": - return "low"; - } -} - -export function sameFinding(candidate: Pick, finding: Pick): boolean { - return findingLocationKey(candidate) === findingLocationKey(finding); -} - -export function findingLocationKey(value: Pick | Pick): string { - const location = primaryLocation(value); - return `${normalizePath(location.file)}:${location.line ?? "file"}`; + return VERDICT_PRESENTATION[verdict].confidence; } export function recordVerdictProgress( @@ -261,32 +182,64 @@ export function recordVerdictProgress( } progress({ type: "lane_item", - lane: verdictLane(verdict.verdict), + lane: VERDICT_PRESENTATION[verdict.verdict].lane, title: candidate.summary, subtitle: formatLocation(candidate), - status: verdictStatus(verdict.verdict), + status: VERDICT_PRESENTATION[verdict.verdict].status, details: formatEvidence(verdict.evidence), }); } export function backfillAdvisoryFindings( - findings: AdvisoryReport["findings"], + findings: AdvisorySynthesis["findings"], ranked: readonly Source[], defaults: AdvisoryBackfillDefaults, ): AdvisoryReport["findings"] { - const rankedByLocation = new Map(); - for (const candidate of ranked) { - const key = findingLocationKey(candidate); - if (!rankedByLocation.has(key)) rankedByLocation.set(key, candidate); - } + const sources = new Map(ranked.flatMap((source) => source.sourceCandidateIds.map((id) => [id, source] as const))); + const used = new Set(); + return findings.flatMap((selection) => { + const ids = [...new Set(selection.sourceCandidateIds)]; + if (ids.length === 0 || ids.some((id) => !sources.has(id) || used.has(id))) return []; + const records = [...new Set(ids.map((id) => sources.get(id)!))]; + if (records.some((record) => record.verdict === "REFUTED")) return []; + const sourceCandidateIds = [...new Set(records.flatMap((record) => record.sourceCandidateIds))]; + if (sourceCandidateIds.some((id) => used.has(id))) return []; + sourceCandidateIds.forEach((id) => used.add(id)); + const first = records[0]!; + return [{ + sourceCandidateIds, + summary: [...new Set(records.map((record) => record.summary))].join("; "), + category: first.category, + severity: selection.severity, + verdict: records.some((record) => record.verdict === "NOT_SUBSTANTIATED") ? "NOT_SUBSTANTIATED" as const : records.some((record) => record.verdict === "PLAUSIBLE") ? "PLAUSIBLE" as const : "CONFIRMED" as const, + confidence: records.every((record) => record.verdict === "CONFIRMED") ? "high" as const : "medium" as const, + locations: uniqueLocations(records.flatMap((record) => record.locations)), + ...(first.reviewAnchor ? { reviewAnchor: first.reviewAnchor } : {}), + evidence: [...new Set(records.flatMap((record) => [...(record.discoveryEvidence ?? []), ...record.evidence]))], + impact: [...new Set(records.map((record) => record.impact || defaults.impact))].join("\n"), + recommendation: selection.recommendation || first.recommendation || defaults.recommendation || "", + }]; + }); +} - return findings.map((finding) => { - const source = rankedByLocation.get(findingLocationKey(finding)); +/** Retain verified records if synthesis fails, and expose invalid ID selections as a gap. */ +export function resolveAdvisorySynthesis( + report: AdvisorySynthesis | undefined, + ranked: readonly Source[], + defaults: AdvisoryBackfillDefaults, + coverage: AdvisoryStageCoverage[], +): AdvisoryReport { + if (!report) { return { - ...finding, - evidence: finding.evidence.length > 0 ? finding.evidence : (source?.evidence ?? []), - impact: finding.impact || source?.impact || defaults.impact, - recommendation: finding.recommendation || source?.recommendation || defaults.recommendation || "", + summary: "Synthesis unavailable; verified records retained.", + findings: backfillAdvisoryFindings(ranked.map((finding) => ({ sourceCandidateIds: finding.sourceCandidateIds, severity: "medium", recommendation: finding.recommendation ?? defaults.recommendation ?? "Inspect verifier evidence." })), ranked, defaults), + nextSteps: ["Inspect verifier evidence or rerun synthesis."], }; - }); + } + const findings = backfillAdvisoryFindings(report.findings, ranked, defaults); + if (findings.length !== report.findings.length) { + coverage.push({ stage: "Synthesis provenance", expected: report.findings.length, completed: findings.length, + failed: report.findings.length - findings.length, failures: [{ branch: "synthesize", reason: "Discarded invalid, repeated, or unverified candidate IDs." }] }); + } + return { ...report, findings }; } diff --git a/.pi/extensions/pi-workflow-engine/src/worktree.ts b/.pi/extensions/pi-workflow-engine/src/worktree.ts index f0454c4..87efc3a 100644 --- a/.pi/extensions/pi-workflow-engine/src/worktree.ts +++ b/.pi/extensions/pi-workflow-engine/src/worktree.ts @@ -154,6 +154,13 @@ export class WorktreeRegistry { }); } + async applyPatch(path: string, patch: string, signal?: AbortSignal): Promise { + return this.runner.runGit({ + cwd: path, args: ["apply", "--index", "--binary", "-"], + stdin: patch, signal, timeoutMs: this.timeoutMs, + }); + } + async validatePatch(path: string, candidate: WorktreePatch, signal?: AbortSignal): Promise { return await validateWorktreePatch({ worktreePath: path, diff --git a/.pi/extensions/pi-workflow-engine/workflows/code-review.ts b/.pi/extensions/pi-workflow-engine/workflows/code-review.ts index 425de94..a68dc37 100644 --- a/.pi/extensions/pi-workflow-engine/workflows/code-review.ts +++ b/.pi/extensions/pi-workflow-engine/workflows/code-review.ts @@ -1,26 +1,21 @@ +import { challengeFindings, parseChallengeArgs } from "../src/advisory-challenge.ts"; import { Type } from "typebox"; import { - AdvisoryCandidatesSchema, - AdvisoryReportSchema, - AdvisoryVerdictSchema, - type AdvisoryCandidate, - type AdvisoryVerdict, -} from "../src/advisory-schema.ts"; -import { - backfillAdvisoryFindings, + type AdvisoryVerified, + type AdvisoryLens, + synthesizeAdvisoryReport, + finishAdvisoryReport, formatEvidence, formatLocation, normalizePath, - primaryLocation, publishVerifiedKeptProgress, - recordVerdictProgress, + runLensVerificationPipeline, verdictConfidence, DEFAULT_ADVISORY_TOOL_HINTS, DEFAULT_ADVISORY_TOOLS, } from "../src/workflow-advisory-utils.ts"; -import { compactResults } from "../src/concurrency.ts"; import { formatReviewDiffTarget, parseAllowedDiffCommand } from "../src/review-diff-target.ts"; -import { buildCodeReviewScopeBlock, dedupeCodeReviewCandidates } from "../src/review/code-review-orchestration.ts"; +import { buildCodeReviewScopeBlock } from "../src/review/code-review-orchestration.ts"; import type { ReviewContext } from "../src/review/review-report.ts"; import { captureReviewMaterial, type ReviewMaterialCaptureResult } from "../src/review/review-snapshot.ts"; import type { WorkflowApi, WorkflowMeta, WorkflowRunStats } from "../src/types.ts"; @@ -28,7 +23,7 @@ import type { WorkflowApi, WorkflowMeta, WorkflowRunStats } from "../src/types.t export const meta: WorkflowMeta = { name: "code-review", description: "Fan-out review of the branch's open PR (or branch vs main): scope → per-angle find → independent verify → synthesize.", - phases: [{ title: "Scope" }, { title: "Find" }, { title: "Verify" }, { title: "Synthesize" }], + phases: [{ title: "Scope" }, { title: "Find" }, { title: "Verify" }, { title: "Challenge" }, { title: "Synthesize" }], }; // ─── Schemas (the contracts that make orchestration plain code) ─── @@ -39,30 +34,15 @@ const ScopeSchema = Type.Object({ conventions: Type.Optional(Type.String({ description: "Relevant AGENTS.md / project conventions" })), }); -type Candidate = AdvisoryCandidate; - -interface Angle { - label: string; - kind: "bug" | "cleanup"; - text: string; -} -interface Verified extends Candidate { - verdict: AdvisoryVerdict["verdict"]; - evidence: string[]; - kind: "bug" | "cleanup"; -} - // The review lenses — this is the part you customise to your codebase's real failure modes. -const ANGLES: Angle[] = [ - { label: "logic-bugs", kind: "bug", text: "Off-by-one errors, wrong conditionals, incorrect return values, broken control flow." }, - { label: "error-paths", kind: "bug", text: "Unhandled errors, swallowed exceptions, missing awaits, partial failure leaving inconsistent state." }, - { label: "edge-cases", kind: "bug", text: "Empty/null inputs, boundary values, concurrency races, resource leaks." }, - { label: "simplification", kind: "cleanup", text: "Dead code, needless complexity, duplicated logic, clearer equivalents." }, - { label: "conventions", kind: "cleanup", text: "Violations of the project conventions noted in scope (naming, idioms, banned patterns)." }, +const ANGLES: AdvisoryLens[] = [ + { label: "logic-bugs", category: "bug", text: "Off-by-one errors, wrong conditionals, incorrect return values, broken control flow." }, + { label: "error-paths", category: "bug", text: "Unhandled errors, swallowed exceptions, missing awaits, partial failure leaving inconsistent state." }, + { label: "edge-cases", category: "bug", text: "Empty/null inputs, boundary values, concurrency races, resource leaks." }, + { label: "simplification", category: "cleanup", text: "Dead code, needless complexity, duplicated logic, clearer equivalents." }, + { label: "conventions", category: "cleanup", text: "Violations of the project conventions noted in scope (naming, idioms, banned patterns)." }, ]; -const TOOLS = DEFAULT_ADVISORY_TOOLS; -const TOOL_HINTS = DEFAULT_ADVISORY_TOOL_HINTS; const PER_ANGLE = 6; /** Parse a unified diff into the set of added/changed new-file line numbers per file. */ @@ -106,8 +86,9 @@ export interface CodeReviewDependencies { } export default async function run(api: WorkflowApi, dependencies: CodeReviewDependencies = {}): Promise { - const { agent, parallel, phase, log, progress, args, cwd, signal } = api; - const target = args.trim(); + const { agent, phase, log, progress, args, cwd, signal } = api; + const challengeConfig = parseChallengeArgs(args); + const target = challengeConfig.args.trim(); let fileCount = 0; let rawCandidateCount = 0; let droppedCandidateCount = 0; @@ -138,7 +119,7 @@ export default async function run(api: WorkflowApi, dependencies: CodeReviewDepe "Then: list the changed files, summarize the change in one paragraph (mention the PR if one was found), " + "and read any relevant AGENTS.md or project docs noting conventions a reviewer should know.\n" + "Return diffCommand exactly as a reviewer should run it. Structured output only.", - { phase: "Scope", label: "scope", tools: TOOLS, toolHints: TOOL_HINTS, profile: "medium", schema: ScopeSchema }, + { phase: "Scope", label: "scope", tools: DEFAULT_ADVISORY_TOOLS, toolHints: DEFAULT_ADVISORY_TOOL_HINTS, profile: "medium", schema: ScopeSchema }, ); if (!scope) { @@ -184,7 +165,7 @@ export default async function run(api: WorkflowApi, dependencies: CodeReviewDepe : {}), }; - const scopeBlock = buildCodeReviewScopeBlock({ + const scopeBlock = `Reviewed snapshot identity: ${JSON.stringify(reviewContext.snapshot ?? "unavailable")}\n` + buildCodeReviewScopeBlock({ diffCommand, files: scope.files, summary: scope.summary, @@ -193,113 +174,59 @@ export default async function run(api: WorkflowApi, dependencies: CodeReviewDepe target, }); - // ─── Find barrier → dedup → Verify ─── - phase("Find"); - const perAngle = await parallel( - ANGLES.map((angle) => async () => { - const found = await agent( - `## Code-review finder — ${angle.label}\n\n${scopeBlock}\n` + - `Review the change through ONLY this lens:\n${angle.text}\n` + - "Only flag issues on lines that are part of the diff above (run the diff command if it is not shown). " + - "You may read surrounding files for context, but never report issues in unchanged code. " + - `Surface up to ${PER_ANGLE} candidates. Use category exactly "${angle.kind}". Each candidate must include a one-line summary, ` + - "locations with the changed file and a line that appears in the diff, and impact describing the concrete failure or maintenance scenario. " + - "Pass through anything with a nameable impact — a separate verifier judges them next. Structured output only.", - { phase: "Find", label: `find:${angle.label}`, tools: TOOLS, toolHints: TOOL_HINTS, profile: "small", schema: AdvisoryCandidatesSchema }, - ); - const raw = (found?.candidates ?? []).slice(0, PER_ANGLE); - rawCandidateCount += raw.length; - progress({ type: "counter_delta", key: "candidates", label: "candidates", delta: raw.length }); - const bounded = raw.filter((candidate) => inDiff(changed, primaryLocation(candidate).file, primaryLocation(candidate).line)); - const dropped = raw.length - bounded.length; - if (dropped > 0) { - droppedCandidateCount += dropped; - progress({ type: "counter_delta", key: "dropped", label: "dropped", delta: dropped }); - log(`find:${angle.label}: dropped ${dropped} out-of-diff candidate(s)`); - } - for (const candidate of bounded) { - progress({ - type: "lane_item", - lane: "Candidates", - title: candidate.summary, - subtitle: formatLocation(candidate), - status: "pending", - details: candidate.impact, - }); - } - return { angle, candidates: bounded }; - }), - ); - - // Dedup after all finders complete so verifier agents cannot consume the global cap before full candidate discovery. - const novel = dedupeCodeReviewCandidates(compactResults(perAngle)); - - phase("Verify"); - const verdicts = await parallel( - novel.map(({ angle, candidate }) => async (): Promise => { - const location = primaryLocation(candidate); - const judged = await agent( - `## Code-review verifier\n\n${scopeBlock}\n## Candidate\n` + - `Location: ${formatLocation(candidate)}\n` + - `Category: ${candidate.category}\nSummary: ${candidate.summary}\nImpact: ${candidate.impact}\n\n` + - "Run the diff command, read the relevant file(s), and return exactly one verdict (CONFIRMED / PLAUSIBLE / REFUTED) " + - "with evidence quoting the line(s). Default toward REFUTED if you cannot substantiate it. Structured output only.", - { - phase: "Verify", - label: `verify:${location.file.split("/").pop() ?? location.file}`, - tools: TOOLS, - toolHints: TOOL_HINTS, - profile: "small", - schema: AdvisoryVerdictSchema, - }, - ); - if (!judged) return null; - recordVerdictProgress(progress, candidate, judged); - return { ...candidate, verdict: judged.verdict, evidence: judged.evidence, kind: angle.kind }; - }), - ); - - const verified = compactResults(verdicts); + const pipelineResult = await runLensVerificationPipeline({ + api, + lenses: ANGLES, + perLens: PER_ANGLE, + boundCandidate: (candidate, lens) => { + const anchor = candidate.reviewAnchor ?? candidate.locations.find((location) => inDiff(changed, location.file, location.line)); + return anchor && inDiff(changed, anchor.file, anchor.line) + ? { ...candidate, category: lens.category, reviewAnchor: anchor } : undefined; + }, + finderPrompt: (lens) => + `## Code-review finder — ${lens.label}\n\n${scopeBlock}\n` + + `Review the change through ONLY this lens:\n${lens.text}\n` + + "Only flag issues on lines that are part of the diff above (run the diff command if it is not shown). " + + "Set reviewAnchor to the changed line causing the issue; locations and discoveryEvidence may include unchanged callers and other files. " + + `Surface up to ${PER_ANGLE} candidates. Use category exactly "${lens.category}". Each candidate must include a one-line summary, ` + + "locations with the changed file and a line that appears in the diff, and impact describing the concrete failure or maintenance scenario. " + + "Pass through anything with a nameable impact — a separate verifier judges them next. Structured output only.", + verifierPrompt: (candidate) => + `## Code-review verifier\n\n${scopeBlock}\n## Candidate\n` + + `Location: ${formatLocation(candidate)}\n` + + `Category: ${candidate.category}\nSummary: ${candidate.summary}\nImpact: ${candidate.impact}\n\n` + + "Run the diff command, read the relevant file(s), and return exactly one verdict (CONFIRMED / PLAUSIBLE / NOT_SUBSTANTIATED / REFUTED) " + + "with evidence quoting the line(s). Use NOT_SUBSTANTIATED when evidence is insufficient; use REFUTED only for concrete disproof. Structured output only.", + }); + rawCandidateCount += pipelineResult.rawCandidates; + droppedCandidateCount += pipelineResult.dropped; + const { coverage } = pipelineResult; + const verified = await challengeFindings(api, pipelineResult.verified, scopeBlock, challengeConfig.options, coverage); const surviving = verified.filter((finding) => finding.verdict !== "REFUTED"); const stats = makeStats(verified.length, surviving.length); publishVerifiedKeptProgress({ progress, log }, verified.length, surviving.length); if (surviving.length === 0) { - return { summary: "No findings survived verification.", findings: [], nextSteps: ["No code-review action is recommended from this workflow run."], stats, reviewContext }; + return finishAdvisoryReport({ summary: "No findings survived verification.", findings: [], nextSteps: ["No code-review action is recommended from this workflow run."], stats, reviewContext }, coverage, verified); } // ─── Synthesize: rank, merge, report ─── - phase("Synthesize"); - const rank = (finding: Verified): number => (finding.kind === "cleanup" ? 2 : 0) + (finding.verdict === "PLAUSIBLE" ? 1 : 0); + const rank = (finding: AdvisoryVerified): number => (finding.category === "cleanup" ? 2 : 0) + (finding.verdict !== "CONFIRMED" ? 1 : 0); const ranked = [...surviving].sort((a, b) => rank(a) - rank(b)); const block = ranked .map( (finding, index) => - `### [${index}] ${formatLocation(finding)} (${finding.verdict}${finding.kind === "cleanup" ? ", cleanup" : ""})\n` + - `Category: ${finding.kind}\nConfidence: ${verdictConfidence(finding.verdict)}\n` + + `### [${index}] IDs: ${finding.sourceCandidateIds.join(", ")} ${formatLocation(finding)} (${finding.verdict}${finding.category === "cleanup" ? ", cleanup" : ""})\n` + + `Category: ${finding.category}\nConfidence: ${verdictConfidence(finding.verdict)}\n` + `${finding.summary}\nImpact: ${finding.impact}\nEvidence: ${formatEvidence(finding.evidence)}`, ) .join("\n\n"); - const report = await agent( + const resolved = await synthesizeAdvisoryReport(api, `## Synthesis: final code-review report\n\n${ranked.length} findings survived independent verification.\n\n${block}\n\n` + "Merge findings with the same root cause, rank most-severe first (correctness bugs above cleanups), and produce the final advisory report. " + - "Return summary, findings, and nextSteps. For each finding: category must be bug or cleanup; severity is impact level (low/medium/high), not category; " + - "confidence must be high for CONFIRMED and medium for PLAUSIBLE; copy locations and evidence arrays from the verified finding; impact is the concrete failure or maintenance scenario; recommendation is an advisory fix direction, not an edit. Structured output only.", - { - phase: "Synthesize", - label: "synthesize", - tools: [], - profile: "medium", - resume: "read-only", - schema: AdvisoryReportSchema, - }, + "Return summary, ID selections with severity (low/medium/high) and advisory recommendation, and nextSteps. Evidence and confidence are reconstructed from verified records. Structured output only.", + ranked, coverage, ); - - if (!report) return { summary: "Synthesis produced no output.", findings: [], nextSteps: ["Re-run the workflow or inspect verifier evidence manually."], stats, reviewContext }; - - const findings = backfillAdvisoryFindings(report.findings, ranked, { - impact: "Impact not restated by synthesis.", - }); - return { ...report, findings, stats, reviewContext }; + return finishAdvisoryReport({ ...resolved, stats: { ...stats, kept: resolved.findings.length }, reviewContext }, coverage, verified); } diff --git a/.pi/extensions/pi-workflow-engine/workflows/diagnose.ts b/.pi/extensions/pi-workflow-engine/workflows/diagnose.ts index baff03a..0fe2955 100644 --- a/.pi/extensions/pi-workflow-engine/workflows/diagnose.ts +++ b/.pi/extensions/pi-workflow-engine/workflows/diagnose.ts @@ -1,30 +1,24 @@ +import { challengeFindings, parseChallengeArgs } from "../src/advisory-challenge.ts"; import { Type } from "typebox"; import { - AdvisoryCandidatesSchema, - AdvisoryReportSchema, - AdvisoryVerdictSchema, - type AdvisoryCandidate, - type AdvisoryVerdict, -} from "../src/advisory-schema.ts"; -import { - advisoryDedupKey as dedupKey, - backfillAdvisoryFindings, + type AdvisoryVerified, + type AdvisoryLens, + synthesizeAdvisoryReport, + finishAdvisoryReport, emptyAdvisoryReport, formatEvidence, formatLocation, - primaryLocation, publishVerifiedKeptProgress, - recordVerdictProgress, + runLensVerificationPipeline, DEFAULT_ADVISORY_TOOL_HINTS, DEFAULT_ADVISORY_TOOLS, } from "../src/workflow-advisory-utils.ts"; -import { compactResults } from "../src/concurrency.ts"; import type { WorkflowApi, WorkflowMeta, WorkflowRunStats } from "../src/types.ts"; export const meta: WorkflowMeta = { name: "diagnose", description: "Advisory-only bug diagnosis: scope symptoms → competing hypotheses → independent verify → synthesize likely root causes.", - phases: [{ title: "Scope" }, { title: "Hypothesize" }, { title: "Verify" }, { title: "Synthesize" }], + phases: [{ title: "Scope" }, { title: "Hypothesize" }, { title: "Verify" }, { title: "Challenge" }, { title: "Synthesize" }], }; const ScopeSchema = Type.Object({ @@ -35,25 +29,7 @@ const ScopeSchema = Type.Object({ constraints: Type.Optional(Type.String({ description: "Safety constraints, missing evidence, or commands intentionally not run." })), }); -interface HypothesisLens { - label: string; - category: string; - text: string; -} - -type Candidate = AdvisoryCandidate; - -interface Hypothesis extends Candidate { - lens: HypothesisLens; -} - -interface Verified extends Hypothesis { - verdict: AdvisoryVerdict["verdict"]; - evidence: string[]; - confidence?: AdvisoryVerdict["confidence"]; -} - -const HYPOTHESIS_LENSES: HypothesisLens[] = [ +const HYPOTHESIS_LENSES: AdvisoryLens[] = [ { label: "recent-change", category: "regression", text: "A recent code change broke a previously working path or changed an implicit contract." }, { label: "control-flow", category: "root-cause", text: "Incorrect branching, ordering, async flow, data flow, or state transition causes the symptom." }, { label: "configuration", category: "configuration", text: "Configuration, environment, package scripts, or runtime assumptions differ from what the code expects." }, @@ -61,13 +37,12 @@ const HYPOTHESIS_LENSES: HypothesisLens[] = [ { label: "test-fixture", category: "test-fixture", text: "The failure is caused by test setup, fixtures, mocks, generated files, or stale local state rather than product code." }, ]; -const TOOLS = DEFAULT_ADVISORY_TOOLS; -const TOOL_HINTS = DEFAULT_ADVISORY_TOOL_HINTS; const PER_LENS = 4; export default async function run(api: WorkflowApi): Promise { - const { agent, parallel, phase, log, progress, args } = api; - const symptom = args.trim(); + const { agent, phase, log, progress, args } = api; + const challengeConfig = parseChallengeArgs(args); + const symptom = challengeConfig.args.trim(); let fileCount = 0; let rawCandidateCount = 0; let droppedCandidateCount = 0; @@ -90,15 +65,15 @@ export default async function run(api: WorkflowApi): Promise { "Inspect relevant files, package/test configuration, and safe diagnostic commands. " + "Safe commands are read-only commands such as status, grep, listing files, typecheck/test commands, or commands explicitly requested by the user. " + "Do not run mutation, install, commit, network, or destructive commands. Return scoped files, observations, and constraints. Structured output only.", - { phase: "Scope", label: "scope", tools: TOOLS, toolHints: TOOL_HINTS, profile: "medium", schema: ScopeSchema }, + { phase: "Scope", label: "scope", tools: DEFAULT_ADVISORY_TOOLS, toolHints: DEFAULT_ADVISORY_TOOL_HINTS, profile: "medium", schema: ScopeSchema }, ); if (!scope) { - return emptyAdvisoryReport( + return finishAdvisoryReport(emptyAdvisoryReport( "Diagnosis could not establish a scope.", ["Provide the failing command, error message, or regression description and rerun diagnose."], makeStats(0, 0), - ); + ), []); } fileCount = scope.files.length; @@ -113,88 +88,48 @@ export default async function run(api: WorkflowApi): Promise { `## Observations\n${scope.observations.map((observation) => `- ${observation}`).join("\n") || "(none)"}\n\n` + `## Constraints\n${scope.constraints ?? "(none noted)"}\n`; - phase("Hypothesize"); - const perLens = await parallel( - HYPOTHESIS_LENSES.map((lens) => async (): Promise => { - const found = await agent( - `## Diagnose hypothesis generator — ${lens.label}\n\n${scopeBlock}\n` + - "This workflow is advisory-only: diagnose and recommend validation/fix plans, but do not edit files.\n" + - `Consider ONLY this hypothesis lens:\n${lens.text}\n\n` + - `Surface up to ${PER_LENS} root-cause hypotheses. Use category exactly "${lens.category}". ` + - "Each hypothesis must include a one-line summary, locations, impact explaining how it produces the symptom, and an optional recommendation for the next validation step. Structured output only.", - { phase: "Hypothesize", label: `hypothesize:${lens.label}`, tools: TOOLS, toolHints: TOOL_HINTS, profile: "small", schema: AdvisoryCandidatesSchema }, - ); - const candidates = (found?.candidates ?? []).slice(0, PER_LENS).map((candidate) => ({ ...candidate, lens })); - rawCandidateCount += candidates.length; - progress({ type: "counter_delta", key: "candidates", label: "candidates", delta: candidates.length }); - for (const candidate of candidates) { - progress({ - type: "lane_item", - lane: "Hypotheses", - title: candidate.summary, - subtitle: formatLocation(candidate), - status: "pending", - details: candidate.impact, - }); - } - return candidates; - }), - ); - - const hypotheses = dedupe(compactResults(perLens).flat(), (dropped) => { - droppedCandidateCount += dropped; - progress({ type: "counter_delta", key: "dropped", label: "dropped", delta: dropped }); + const pipelineResult = await runLensVerificationPipeline({ + api, + lenses: HYPOTHESIS_LENSES, + perLens: PER_LENS, + finderPhase: "Hypothesize", + finderPrompt: (lens) => + `## Diagnose hypothesis generator — ${lens.label}\n\n${scopeBlock}\n` + + "This workflow is advisory-only: diagnose and recommend validation/fix plans, but do not edit files.\n" + + `Consider ONLY this hypothesis lens:\n${lens.text}\n\n` + + `Surface up to ${PER_LENS} root-cause hypotheses. Use category exactly "${lens.category}". ` + + "Each hypothesis must include a one-line summary, locations, impact explaining how it produces the symptom, and an optional recommendation for the next validation step. Structured output only.", + verifierPrompt: (candidate) => + `## Diagnose verifier\n\n${scopeBlock}\n## Hypothesis\n` + + `Location: ${formatLocation(candidate)}\nCategory: ${candidate.category}\nSummary: ${candidate.summary}\nImpact: ${candidate.impact}\n` + + `Recommended validation: ${candidate.recommendation ?? "(none supplied)"}\n\n` + + "Read relevant files and, when useful, run only safe read-only diagnostic commands from the scoped command list or commands explicitly requested by the user. " + + "Do not run mutation, install, commit, network, or destructive commands. Return CONFIRMED, PLAUSIBLE, NOT_SUBSTANTIATED, or REFUTED with evidence. " + + "Use NOT_SUBSTANTIATED when evidence is missing; REFUTED requires disproof. Structured output only.", }); - - phase("Verify"); - const verified = compactResults( - await parallel( - hypotheses.map((hypothesis) => async (): Promise => { - const location = primaryLocation(hypothesis); - const judged = await agent( - `## Diagnose verifier\n\n${scopeBlock}\n## Hypothesis\n` + - `Location: ${formatLocation(hypothesis)}\nCategory: ${hypothesis.category}\nSummary: ${hypothesis.summary}\nImpact: ${hypothesis.impact}\n` + - `Recommended validation: ${hypothesis.recommendation ?? "(none supplied)"}\n\n` + - "Read relevant files and, when useful, run only safe read-only diagnostic commands from the scoped command list or commands explicitly requested by the user. " + - "Do not run mutation, install, commit, network, or destructive commands. Return CONFIRMED, PLAUSIBLE, or REFUTED with evidence. " + - "Default toward REFUTED if evidence does not connect the hypothesis to the symptom. Structured output only.", - { - phase: "Verify", - label: `verify:${location.file.split("/").pop() ?? location.file}`, - tools: TOOLS, - toolHints: TOOL_HINTS, - profile: "small", - schema: AdvisoryVerdictSchema, - }, - ); - if (!judged) return null; - recordVerdictProgress(progress, hypothesis, judged, () => { - refutedCandidateCount += 1; - }); - return { ...hypothesis, verdict: judged.verdict, evidence: judged.evidence, confidence: judged.confidence }; - }), - ), - ); - + rawCandidateCount += pipelineResult.rawCandidates; + droppedCandidateCount += pipelineResult.dropped; + refutedCandidateCount += pipelineResult.refuted; + const { coverage } = pipelineResult; + const verified = await challengeFindings(api, pipelineResult.verified, scopeBlock, challengeConfig.options, coverage); const surviving = verified.filter((finding) => finding.verdict !== "REFUTED"); const refuted = verified.filter((finding) => finding.verdict === "REFUTED"); const stats = makeStats(verified.length, surviving.length); publishVerifiedKeptProgress({ progress, log }, verified.length, surviving.length); if (surviving.length === 0) { - return emptyAdvisoryReport( + return finishAdvisoryReport(emptyAdvisoryReport( "No root-cause hypothesis survived verification.", ["Capture the exact failing command and error output.", "Rerun diagnose with a narrower symptom or more evidence."], stats, - ); + ), coverage, verified); } - phase("Synthesize"); const ranked = [...surviving].sort((a, b) => rank(a) - rank(b)); const block = ranked .map( (finding, index) => - `### [${index}] ${formatLocation(finding)} (${finding.verdict}, ${finding.category})\n` + + `### [${index}] IDs: ${finding.sourceCandidateIds.join(", ")} ${formatLocation(finding)} (${finding.verdict}, ${finding.category})\n` + `${finding.summary}\nImpact: ${finding.impact}\nEvidence: ${formatEvidence(finding.evidence)}\nValidation/fix plan: ${finding.recommendation ?? "(none supplied)"}`, ) .join("\n\n"); @@ -203,46 +138,17 @@ export default async function run(api: WorkflowApi): Promise { .map((finding) => `- ${finding.summary} — REFUTED because ${formatEvidence(finding.evidence)}`) .join("\n"); - const report = await agent( + const resolved = await synthesizeAdvisoryReport(api, `## Synthesis: final diagnosis report\n\n${ranked.length} hypotheses survived independent verification.\n\n${block}\n\n` + `## Refuted hypotheses for context\n${refutedBlock || "(none recorded)"}\n\n` + - "Produce the shared advisory report shape. Only include confirmed or plausible root causes in findings. " + - "Use categories such as root-cause, regression, configuration, dependency, or test-fixture. " + + "Select confirmed, plausible or explicitly unresolved root causes by ID. " + "Recommendation must be a validation/fix plan, not a patch. nextSteps must be the minimum commands or code inspections needed to confirm the top diagnosis. Structured output only.", - { - phase: "Synthesize", - label: "synthesize", - tools: [], - profile: "medium", - resume: "read-only", - schema: AdvisoryReportSchema, - }, + ranked, coverage, ); - - if (!report) return emptyAdvisoryReport("Synthesis produced no output.", ["Inspect verifier evidence manually or rerun diagnose with a narrower symptom."], stats); - - const findings = backfillAdvisoryFindings(report.findings, ranked, { - impact: "Impact not restated by synthesis.", - recommendation: "Validate this diagnosis with the smallest safe reproduction command.", - }); - return { ...report, findings, stats }; -} - -function dedupe(candidates: Hypothesis[], onDropped: (dropped: number) => void): Hypothesis[] { - const seen = new Set(); - const novel: Hypothesis[] = []; - for (const candidate of candidates) { - const key = dedupKey(candidate); - if (seen.has(key)) continue; - seen.add(key); - novel.push(candidate); - } - const dropped = candidates.length - novel.length; - if (dropped > 0) onDropped(dropped); - return novel; + return finishAdvisoryReport({ ...resolved, stats: { ...stats, kept: resolved.findings.length } }, coverage, verified); } -function rank(finding: Verified): number { +function rank(finding: AdvisoryVerified): number { if (finding.verdict === "CONFIRMED") return 0; return 1; } diff --git a/.pi/extensions/pi-workflow-engine/workflows/perf-review.ts b/.pi/extensions/pi-workflow-engine/workflows/perf-review.ts index 66a04b7..d055e8f 100644 --- a/.pi/extensions/pi-workflow-engine/workflows/perf-review.ts +++ b/.pi/extensions/pi-workflow-engine/workflows/perf-review.ts @@ -1,11 +1,10 @@ +import { challengeFindings, parseChallengeArgs } from "../src/advisory-challenge.ts"; import { Type } from "typebox"; import { - AdvisoryReportSchema, - type AdvisoryCandidate, - type AdvisoryVerdict, -} from "../src/advisory-schema.ts"; -import { - backfillAdvisoryFindings, + type AdvisoryVerified, + type AdvisoryLens, + synthesizeAdvisoryReport, + finishAdvisoryReport, emptyAdvisoryReport, formatEvidence, formatLocation, @@ -19,7 +18,7 @@ import type { WorkflowApi, WorkflowMeta, WorkflowRunStats } from "../src/types.t export const meta: WorkflowMeta = { name: "perf-review", description: "Advisory-only performance review: scope slow path → per-lens bottleneck hypotheses → verify evidence → synthesize measurements and safe optimizations.", - phases: [{ title: "Scope" }, { title: "Find" }, { title: "Verify" }, { title: "Synthesize" }], + phases: [{ title: "Scope" }, { title: "Find" }, { title: "Verify" }, { title: "Challenge" }, { title: "Synthesize" }], }; const ScopeSchema = Type.Object({ @@ -30,22 +29,7 @@ const ScopeSchema = Type.Object({ knownMeasurements: Type.Optional(Type.String({ description: "Existing measurements, timings, or explicit lack of measurements." })), }); -interface PerfLens { - label: string; - category: string; - text: string; -} - -type Candidate = AdvisoryCandidate; - -interface Verified extends Candidate { - verdict: AdvisoryVerdict["verdict"]; - evidence: string[]; - confidence?: AdvisoryVerdict["confidence"]; - lens: PerfLens; -} - -const PERF_LENSES: PerfLens[] = [ +const PERF_LENSES: AdvisoryLens[] = [ { label: "algorithmic", category: "algorithmic", text: "Complexity, repeated scans, avoidable nested loops, or data-structure choices that grow poorly with input size." }, { label: "io", category: "io", text: "Filesystem, subprocess, network, or other I/O costs on hot paths or startup paths." }, { label: "concurrency", category: "concurrency", text: "Unnecessary serialization, missing batching, excessive fan-out, contention, or concurrency limits." }, @@ -54,13 +38,12 @@ const PERF_LENSES: PerfLens[] = [ { label: "measurement", category: "measurement", text: "Missing, misleading, noisy, or insufficient benchmark/measurement design." }, ]; -const TOOLS = DEFAULT_ADVISORY_TOOLS; -const TOOL_HINTS = DEFAULT_ADVISORY_TOOL_HINTS; const PER_LENS = 4; export default async function run(api: WorkflowApi): Promise { - const { agent, parallel, pipeline, phase, log, progress, args } = api; - const target = args.trim() || "repository performance"; + const { agent, phase, log, progress, args } = api; + const challengeConfig = parseChallengeArgs(args); + const target = challengeConfig.args.trim() || "repository performance"; let fileCount = 0; let rawCandidateCount = 0; let droppedCandidateCount = 0; @@ -81,15 +64,15 @@ export default async function run(api: WorkflowApi): Promise { "Inspect repository structure, scripts, likely hot-path files, and any existing benchmark or measurement commands. " + "Prefer identifying what to measure before claiming bottlenecks. Return files, commands, summary, and known measurements or the lack of them. " + `This workflow will fan out across ${PERF_LENSES.length} lenses with up to ${PER_LENS} candidates per lens. Structured output only.`, - { phase: "Scope", label: "scope", tools: TOOLS, toolHints: TOOL_HINTS, profile: "medium", schema: ScopeSchema }, + { phase: "Scope", label: "scope", tools: DEFAULT_ADVISORY_TOOLS, toolHints: DEFAULT_ADVISORY_TOOL_HINTS, profile: "medium", schema: ScopeSchema }, ); if (!scope || scope.files.length === 0) { - return emptyAdvisoryReport( + return finishAdvisoryReport(emptyAdvisoryReport( "No performance-relevant files were identified.", ["Provide a slow command, workload, file path, or user-visible latency concern to review."], makeStats(0, 0), - ); + ), []); } fileCount = scope.files.length; @@ -104,14 +87,10 @@ export default async function run(api: WorkflowApi): Promise { `## Summary\n${scope.summary}\n\n## Known measurements\n${scope.knownMeasurements ?? "(none known)"}\n` + (args.trim() ? `\n## User instructions (verbatim)\n${args.trim()}\n` : ""); - phase("Find"); const pipelineResult = await runLensVerificationPipeline({ - api: { agent, parallel, pipeline, progress, log }, + api, lenses: PERF_LENSES, - schedulingMode: "finder-barrier", perLens: PER_LENS, - tools: TOOLS, - toolHints: TOOL_HINTS, finderPrompt: (lens) => `## Perf-review finder — ${lens.label}\n\n${scopeBlock}\n` + "This workflow is advisory-only: identify bottleneck hypotheses, measurement gaps, and safe optimization directions, but do not edit files.\n" + @@ -124,67 +103,46 @@ export default async function run(api: WorkflowApi): Promise { `Location: ${formatLocation(candidate)}\nCategory: ${candidate.category}\nSummary: ${candidate.summary}\nImpact: ${candidate.impact}\n` + `Recommendation: ${candidate.recommendation ?? "(none supplied)"}\n\n` + "Read relevant files and package/scripts. Run only safe read-only measurement or inspection commands when useful. " + - "Return CONFIRMED, PLAUSIBLE, or REFUTED with evidence from code, scripts, config, or measurement output. " + + "Return CONFIRMED, PLAUSIBLE, NOT_SUBSTANTIATED, or REFUTED with evidence from code, scripts, config, or measurement output. " + "Default toward PLAUSIBLE or REFUTED when no measurement exists; do not overstate a bottleneck. Structured output only.", - makeVerified: (candidate, lens, judged): Verified => ({ - ...candidate, - verdict: judged.verdict, - evidence: judged.evidence, - confidence: judged.confidence, - lens, - }), }); rawCandidateCount += pipelineResult.rawCandidates; droppedCandidateCount += pipelineResult.dropped; refutedCandidateCount += pipelineResult.refuted; - const verified = pipelineResult.verified; + const { coverage } = pipelineResult; + const verified = await challengeFindings(api, pipelineResult.verified, scopeBlock, challengeConfig.options, coverage); const surviving = verified.filter((finding) => finding.verdict !== "REFUTED"); const stats = makeStats(verified.length, surviving.length); publishVerifiedKeptProgress({ progress, log }, verified.length, surviving.length); if (surviving.length === 0) { - return emptyAdvisoryReport( + return finishAdvisoryReport(emptyAdvisoryReport( "No performance finding survived verification.", ["Add or run a focused measurement for the target workload before optimizing.", "Rerun perf-review with benchmark output or a narrower slow path."], stats, - ); + ), coverage, verified); } - phase("Synthesize"); const ranked = [...surviving].sort((a, b) => rank(a) - rank(b)); const block = ranked .map( (finding, index) => - `### [${index}] ${formatLocation(finding)} (${finding.verdict}, ${finding.category})\n` + + `### [${index}] IDs: ${finding.sourceCandidateIds.join(", ")} ${formatLocation(finding)} (${finding.verdict}, ${finding.category})\n` + `${finding.summary}\nImpact: ${finding.impact}\nEvidence: ${formatEvidence(finding.evidence)}\nRecommendation: ${finding.recommendation ?? "(none supplied)"}`, ) .join("\n\n"); - const report = await agent( + const resolved = await synthesizeAdvisoryReport(api, `## Synthesis: final perf-review report\n\n${ranked.length} candidates survived independent verification.\n\n${block}\n\n` + - "Produce the shared advisory report shape. Categories should be algorithmic, io, concurrency, startup, allocation, or measurement when applicable. " + + "Select findings by ID. " + "Severity is expected performance impact for the target workload. Prefer measurement recommendations before optimization recommendations when evidence is weak. " + "Recommendations must be safe advisory next actions, not patches. Include risky optimizations to avoid in recommendations or nextSteps when relevant. Structured output only.", - { - phase: "Synthesize", - label: "synthesize", - tools: [], - profile: "medium", - resume: "read-only", - schema: AdvisoryReportSchema, - }, + ranked, coverage, ); - - if (!report) return emptyAdvisoryReport("Synthesis produced no output.", ["Inspect verifier evidence manually or rerun perf-review with a narrower target."], stats); - - const findings = backfillAdvisoryFindings(report.findings, ranked, { - impact: "Performance impact not restated by synthesis.", - recommendation: "Measure the target workload before changing code.", - }); - return { ...report, findings, stats }; + return finishAdvisoryReport({ ...resolved, stats: { ...stats, kept: resolved.findings.length } }, coverage, verified); } -function rank(finding: Verified): number { +function rank(finding: AdvisoryVerified): number { const verdictRank = finding.verdict === "CONFIRMED" ? 0 : 1; const measurementPenalty = finding.category === "measurement" ? 1 : 0; return verdictRank + measurementPenalty; diff --git a/.pi/extensions/pi-workflow-engine/workflows/refactor-scout.ts b/.pi/extensions/pi-workflow-engine/workflows/refactor-scout.ts index 5eb4a1f..31f6a06 100644 --- a/.pi/extensions/pi-workflow-engine/workflows/refactor-scout.ts +++ b/.pi/extensions/pi-workflow-engine/workflows/refactor-scout.ts @@ -1,11 +1,10 @@ +import { challengeFindings, parseChallengeArgs } from "../src/advisory-challenge.ts"; import { Type } from "typebox"; import { - AdvisoryReportSchema, - type AdvisoryCandidate, - type AdvisoryVerdict, -} from "../src/advisory-schema.ts"; -import { - backfillAdvisoryFindings, + type AdvisoryVerified, + type AdvisoryLens, + synthesizeAdvisoryReport, + finishAdvisoryReport, emptyAdvisoryReport, formatEvidence, formatLocation, @@ -19,7 +18,7 @@ import type { WorkflowApi, WorkflowMeta, WorkflowRunStats } from "../src/types.t export const meta: WorkflowMeta = { name: "refactor-scout", description: "Advisory-only refactor scout: scope → per-lens find → independent verify → synthesize safe refactor opportunities.", - phases: [{ title: "Scope" }, { title: "Find" }, { title: "Verify" }, { title: "Synthesize" }], + phases: [{ title: "Scope" }, { title: "Find" }, { title: "Verify" }, { title: "Challenge" }, { title: "Synthesize" }], }; const ScopeSchema = Type.Object({ @@ -29,22 +28,7 @@ const ScopeSchema = Type.Object({ conventions: Type.Optional(Type.String({ description: "Relevant project conventions from AGENTS.md / docs." })), }); -interface RefactorLens { - label: string; - category: string; - text: string; -} - -type Candidate = AdvisoryCandidate; - -interface Verified extends Candidate { - verdict: AdvisoryVerdict["verdict"]; - evidence: string[]; - confidence?: AdvisoryVerdict["confidence"]; - lens: RefactorLens; -} - -const REFACTOR_LENSES: RefactorLens[] = [ +const REFACTOR_LENSES: AdvisoryLens[] = [ { label: "duplication", category: "duplication", text: "Repeated logic, copy-pasted structures, or near-duplicate flows that could share one clearer implementation." }, { label: "complexity", category: "complexity", text: "Oversized functions, tangled control flow, or abstractions that make local reasoning harder than necessary." }, { label: "type-safety", category: "type-safety", text: "Weak typing, avoidable casts, unchecked shapes, or places stronger types would prevent mistakes." }, @@ -53,13 +37,12 @@ const REFACTOR_LENSES: RefactorLens[] = [ { label: "conventions", category: "conventions", text: "Departures from project conventions, naming, dependency rules, or local idioms." }, ]; -const TOOLS = DEFAULT_ADVISORY_TOOLS; -const TOOL_HINTS = DEFAULT_ADVISORY_TOOL_HINTS; const PER_LENS = 5; export default async function run(api: WorkflowApi): Promise { - const { agent, parallel, pipeline, phase, log, progress, args } = api; - const target = args.trim() || "."; + const { agent, phase, log, progress, args } = api; + const challengeConfig = parseChallengeArgs(args); + const target = challengeConfig.args.trim() || "."; let fileCount = 0; let rawCandidateCount = 0; let droppedCandidateCount = 0; @@ -80,15 +63,15 @@ export default async function run(api: WorkflowApi): Promise { "Inspect repository structure, the target path or module, and relevant AGENTS.md / project docs conventions. " + "Return the concrete files that should be considered, a short summary, and any conventions that affect refactor advice. " + `This workflow will fan out across ${REFACTOR_LENSES.length} lenses with up to ${PER_LENS} candidates per lens. Structured output only.`, - { phase: "Scope", label: "scope", tools: TOOLS, toolHints: TOOL_HINTS, profile: "medium", schema: ScopeSchema }, + { phase: "Scope", label: "scope", tools: DEFAULT_ADVISORY_TOOLS, toolHints: DEFAULT_ADVISORY_TOOL_HINTS, profile: "medium", schema: ScopeSchema }, ); if (!scope || scope.files.length === 0) { - return emptyAdvisoryReport( + return finishAdvisoryReport(emptyAdvisoryReport( "No files were identified for refactor scouting.", ["Provide a target path, module, or subsystem to scout for refactor opportunities."], makeStats(0, 0), - ); + ), []); } fileCount = scope.files.length; @@ -101,14 +84,10 @@ export default async function run(api: WorkflowApi): Promise { `## Summary\n${scope.summary}\n\n## Conventions\n${scope.conventions ?? "(none noted)"}\n` + (args.trim() ? `\n## User instructions (verbatim)\n${args.trim()}\n` : ""); - phase("Find"); const pipelineResult = await runLensVerificationPipeline({ - api: { agent, parallel, pipeline, progress, log }, + api, lenses: REFACTOR_LENSES, - schedulingMode: "finder-barrier", perLens: PER_LENS, - tools: TOOLS, - toolHints: TOOL_HINTS, finderPrompt: (lens) => `## Refactor-scout finder — ${lens.label}\n\n${scopeBlock}\n` + "This workflow is advisory-only: do not edit files and do not propose broad rewrites.\n" + @@ -120,65 +99,44 @@ export default async function run(api: WorkflowApi): Promise { `## Refactor-scout verifier\n\n${scopeBlock}\n## Candidate\n` + `Location: ${formatLocation(candidate)}\nCategory: ${candidate.category}\nSummary: ${candidate.summary}\nImpact: ${candidate.impact}\n` + `Recommendation: ${candidate.recommendation ?? "(none supplied)"}\n\n` + - "Read the relevant files and return CONFIRMED, PLAUSIBLE, or REFUTED. " + + "Read the relevant files and return CONFIRMED, PLAUSIBLE, NOT_SUBSTANTIATED, or REFUTED. " + "Default toward REFUTED if the opportunity is generic, too broad, not evidenced by code, or lacks a safe first step. " + "Evidence must quote or cite code. Structured output only.", - makeVerified: (candidate, lens, judged): Verified => ({ - ...candidate, - verdict: judged.verdict, - evidence: judged.evidence, - confidence: judged.confidence, - lens, - }), }); rawCandidateCount += pipelineResult.rawCandidates; droppedCandidateCount += pipelineResult.dropped; refutedCandidateCount += pipelineResult.refuted; - const verified = pipelineResult.verified; + const { coverage } = pipelineResult; + const verified = await challengeFindings(api, pipelineResult.verified, scopeBlock, challengeConfig.options, coverage); const surviving = verified.filter((finding) => finding.verdict !== "REFUTED"); const stats = makeStats(verified.length, surviving.length); publishVerifiedKeptProgress({ progress, log }, verified.length, surviving.length); if (surviving.length === 0) { - return emptyAdvisoryReport("No refactor opportunities survived verification.", ["Leave the scoped code unchanged unless a human reviewer has additional context."], stats); + return finishAdvisoryReport(emptyAdvisoryReport("No refactor opportunities survived verification.", ["Leave the scoped code unchanged unless a human reviewer has additional context."], stats), coverage, verified); } - phase("Synthesize"); const ranked = [...surviving].sort((a, b) => rank(a) - rank(b)); const block = ranked .map( (finding, index) => - `### [${index}] ${formatLocation(finding)} (${finding.verdict}, ${finding.category})\n` + + `### [${index}] IDs: ${finding.sourceCandidateIds.join(", ")} ${formatLocation(finding)} (${finding.verdict}, ${finding.category})\n` + `${finding.summary}\nImpact: ${finding.impact}\nEvidence: ${formatEvidence(finding.evidence)}\nSafe first step: ${finding.recommendation ?? "(none supplied)"}`, ) .join("\n\n"); - const report = await agent( + const resolved = await synthesizeAdvisoryReport(api, `## Synthesis: final refactor-scout report\n\n${ranked.length} opportunities survived independent verification.\n\n${block}\n\n` + "Merge findings with the same root cause and rank highest leverage / lowest risk first. " + - "Return the shared advisory report shape. Categories should come from the verified candidates. " + - "Severity is maintenance or future-correctness impact. Confidence is high for CONFIRMED, medium for PLAUSIBLE unless verifier confidence says otherwise. " + + "Select findings by ID. " + + "Severity is maintenance or future-correctness impact. " + "Recommendations must be safe first refactor steps, not rewrites. Include concrete nextSteps for the host developer. Structured output only.", - { - phase: "Synthesize", - label: "synthesize", - tools: [], - profile: "medium", - resume: "read-only", - schema: AdvisoryReportSchema, - }, + ranked, coverage, ); - - if (!report) return emptyAdvisoryReport("Synthesis produced no output.", ["Inspect verifier evidence manually or rerun the workflow with a narrower target."], stats); - - const findings = backfillAdvisoryFindings(report.findings, ranked, { - impact: "Impact not restated by synthesis.", - recommendation: "Choose a small, behavior-preserving refactor first step.", - }); - return { ...report, findings, stats }; + return finishAdvisoryReport({ ...resolved, stats: { ...stats, kept: resolved.findings.length } }, coverage, verified); } -function rank(finding: Verified): number { +function rank(finding: AdvisoryVerified): number { const verdictRank = finding.verdict === "CONFIRMED" ? 0 : 1; const categoryRank = finding.category === "dead-code" || finding.category === "conventions" ? 2 : 0; return verdictRank + categoryRank; diff --git a/TODO.md b/TODO.md deleted file mode 100644 index 9270b88..0000000 --- a/TODO.md +++ /dev/null @@ -1,4 +0,0 @@ -# TODO.md - -1. [x] Add dynamax inline workflow authoring -2. [ ] refactor to be .pi format diff --git a/USAGE.md b/USAGE.md index 82684ae..97a91f8 100644 --- a/USAGE.md +++ b/USAGE.md @@ -543,3 +543,62 @@ failures, timeouts, and host aborts are never provider-retried. - **Slow run**: lower fan-out, set `thinkingLevel`, or reduce `--concurrency`. - **Budget exhausted**: narrow the target, raise `--budget`, reduce fan-out/concurrency, or guard custom loops with `api.budget.remaining()`. - **Duplicate command/tool warnings while developing**: avoid loading both the global package and the working copy. + +### Advisory evidence and coverage + +`code-review`, `diagnose`, `refactor-scout`, and `perf-review` retain candidate IDs +from discovery through verification and synthesis. Findings contain +`sourceCandidateIds`; merged findings retain all contributing IDs, locations, and +evidence. Code review uses `reviewAnchor` for the changed line that caused the +finding. Other `locations` can identify unchanged callers or related files. + +These results include `status` (`complete` or `incomplete`), `coverage` with +expected/completed/failed branch counts and failure reasons, `gaps`, and the +`verification` records. Failed verification is not a refutation. Verifiers use +`NOT_SUBSTANTIATED` when evidence is insufficient and `REFUTED` for concrete +disproof. A failed finder or verifier prevents a clean conclusion. If synthesis +fails, the result retains verified records and reports incomplete coverage. +Synthesis selects or merges IDs; the workflow reconstructs evidence-bearing +fields from those records. + +### Selective adversarial challenges + +Add `--challenge` to an advisory workflow to challenge up to three uncertain or +high-risk findings. `--challenge=N` requires a non-negative integer and sets the +limit, capped at ten; zero disables it. Invalid values stop the workflow before +agents start. Ordinary confirmed, low-risk findings bypass this stage. + +```text +/workflow code-review --challenge=2 +/workflow diagnose --challenge failing retry test +``` + +The challenger tries to disprove the finding or identify an alternative cause. +A separate adjudicator receives both sides. Unresolved disagreement remains +`NOT_SUBSTANTIATED`; finding no counterexample does not upgrade confidence. +Custom workflows can use the `challengeFindings` recipe with `maxChallenges` +and a `shouldChallenge` predicate, including selection based on domain-specific +severity. It uses the existing `agent` and settled `parallel` calls. + +### Independently validated repair candidates + +The results viewer's fix action produces candidate patches in disposable +worktrees. Each candidate retains the implementer's report separately from its +`validation` record: reviewed baseline fingerprint, baseline commit, patch +SHA-256, evaluator decision, executed commands, output, and process results. + +A fresh evaluator receives the exact reviewed baseline plus captured patch. +The engine reconstructs another workspace and executes the evaluator's focused +checks. Where applicable, a test-only patch and expected failure text let it +check that a regression fails on the original baseline and passes after repair. +Outcomes are `verified`, `rejected`, `blocked`, or `no-patch`. Missing required +validation blocks verification. An empty patch does not prove that a finding +needs no repair. Rejected and blocked candidates retain their patch and failure +evidence for inspection. Cleanup failures also retain the validation evidence and +record a `cleanupError`; a passed candidate becomes `blocked` if cleanup fails, +while a rejected candidate remains `rejected`. + +Isolated agent results now include `baselineOid`. For independent evaluation, +`agent()` accepts `candidatePatch: { baselineOid, patch }` with +`isolation: "worktree"`; the engine checks the fresh baseline before applying +it. Worktree isolation separates filesystem state; it is not a security sandbox. diff --git a/tests/advisory-provenance.test.ts b/tests/advisory-provenance.test.ts new file mode 100644 index 0000000..b59318b --- /dev/null +++ b/tests/advisory-provenance.test.ts @@ -0,0 +1,136 @@ +import assert from "node:assert/strict"; +import { test } from "bun:test"; +import { bindParallel, pipeline } from "../.pi/extensions/pi-workflow-engine/src/concurrency.ts"; +import { identifyCandidates, dedupeCandidates, type AdvisoryStageCoverage } from "../.pi/extensions/pi-workflow-engine/src/advisory-evidence.ts"; +import { challengeFindings, parseChallengeArgs } from "../.pi/extensions/pi-workflow-engine/src/advisory-challenge.ts"; +import type { AdvisoryCandidate, AdvisoryReport } from "../.pi/extensions/pi-workflow-engine/src/advisory-schema.ts"; +import type { AdvisoryVerified } from "../.pi/extensions/pi-workflow-engine/src/workflow-advisory-utils.ts"; +import type { WorkflowApi, AgentOptions } from "../.pi/extensions/pi-workflow-engine/src/types.ts"; +import codeReview from "../.pi/extensions/pi-workflow-engine/workflows/code-review.ts"; +import diagnose from "../.pi/extensions/pi-workflow-engine/workflows/diagnose.ts"; +import perfReview from "../.pi/extensions/pi-workflow-engine/workflows/perf-review.ts"; +import refactorScout from "../.pi/extensions/pi-workflow-engine/workflows/refactor-scout.ts"; + +const candidate: AdvisoryCandidate = { summary: "Wrong condition", category: "bug", locations: [{ file: "src/app.ts", line: 2 }], impact: "Drops requests" }; +function apiFor(handler: (prompt: string, options: AgentOptions) => unknown): WorkflowApi { + return { agent: (async (prompt: string, options: AgentOptions = {}) => handler(prompt, options)) as WorkflowApi["agent"], + parallel: bindParallel({}), pipeline, phase() {}, log() {}, progress() {}, args: "", cwd: process.cwd(), signal: undefined, + budget: { total: null, spent: () => 0, remaining: () => Infinity }, workflow: async () => undefined }; +} +const material = async () => ({ ok: true as const, diff: "diff --git a/src/app.ts b/src/app.ts\n--- a/src/app.ts\n+++ b/src/app.ts\n@@ -1 +1,2 @@\n old\n+new\n", snapshot: { status: "unavailable" as const, reason: "test" } }); +const workflows = [ + { name: "code-review", run: (api: WorkflowApi) => codeReview(api, { captureReviewMaterial: material }) }, + { name: "diagnose", run: diagnose }, { name: "perf-review", run: perfReview }, { name: "refactor-scout", run: refactorScout }, +]; +for (const workflow of workflows) { + for (const failure of ["finder", "verifier"] as const) { + test(`${workflow.name} preserves ${failure} failure and cannot report a clean result`, async () => { + let finders = 0; + const api = apiFor((_prompt, options) => { + if (options.label === "scope") return { diffCommand: "git diff", symptom: "lost requests", target: "src", files: ["src/app.ts"], commands: [], observations: [], summary: "scope" }; + if (options.label?.startsWith("find:") || options.label?.startsWith("hypothesize:")) { + if (++finders === 1) { + if (failure === "finder") throw new Error("finder unavailable"); + return { candidates: [candidate] }; + } + return { candidates: [] }; + } + if (options.label?.startsWith("verify:")) throw new Error("verifier unavailable"); + throw new Error("unexpected call"); + }); + const result = await workflow.run(api) as AdvisoryReport & { status: string; coverage: AdvisoryStageCoverage[]; gaps: string[] }; + assert.equal(result.status, "incomplete"); + assert.match(result.summary, /Incomplete/); + assert.doesNotMatch(result.summary, /No findings survived/); + assert.equal(result.coverage.find((stage) => stage.stage === (failure === "finder" ? "Find" : "Verify"))?.failed, 1); + assert.ok(result.gaps.some((gap) => gap.includes(`${failure} unavailable`))); + }); + } +} + +test("distinct defects on the same range reach verification and cross-file evidence keeps a changed-line anchor", async () => { + const candidates = [candidate, { ...candidate, summary: "Independent leak", locations: [{ file: "src/caller.ts", line: 90 }, ...candidate.locations], reviewAnchor: candidate.locations[0] }]; + let finders = 0; + let verifiers = 0; + const api = apiFor((prompt, options) => { + if (options.label === "scope") return { diffCommand: "git diff", files: ["src/app.ts"], summary: "scope" }; + if (options.label?.startsWith("find:")) return { candidates: ++finders === 1 ? candidates : [] }; + if (options.label?.startsWith("verify:")) { verifiers++; return { verdict: "CONFIRMED", evidence: ["src/caller.ts:90 reaches the changed condition"] }; } + const ids = [...prompt.matchAll(/IDs: ([a-f0-9, ]+)/g)].map((match) => match[1]!.trim().split(", ")); + return { summary: "Two defects", findings: ids.map((sourceCandidateIds) => ({ sourceCandidateIds, severity: "medium", recommendation: "Repair" })), nextSteps: [] }; + }); + const result = await codeReview(api, { captureReviewMaterial: material }) as AdvisoryReport; + assert.equal(verifiers, 2); + assert.equal(result.findings.length, 2); + assert.ok(result.findings.some((finding) => finding.locations.some((location) => location.file === "src/caller.ts"))); + assert.ok(result.findings.every((finding) => finding.reviewAnchor?.file === "src/app.ts")); +}); + +test("near-exact duplicates from different lenses preserve all identities and discovery evidence", () => { + const first = identifyCandidates([{ ...candidate, discoveryEvidence: ["first evidence"] }], "first")[0]!; + const second = identifyCandidates([{ ...candidate, summary: " WRONG condition. ", discoveryEvidence: ["second evidence"] }], "second")[0]!; + const merged = dedupeCandidates([first, second]); + assert.equal(merged.length, 1); + assert.deepEqual(merged[0]?.sourceCandidateIds, [first.candidateId!, second.candidateId!]); + assert.deepEqual(merged[0]?.discoveryEvidence, ["first evidence", "second evidence"]); + assert.deepEqual(identifyCandidates([candidate], "same"), identifyCandidates([candidate], "same")); +}); + +for (const outcome of ["refuted", "unresolved", "upheld"] as const) { + test(`selective challenge preserves ${outcome} and both sides of evidence`, async () => { + const coverage: AdvisoryStageCoverage[] = []; + const findings: AdvisoryVerified[] = [ + { ...identifyCandidates([candidate], "low")[0]!, verdict: "CONFIRMED", evidence: ["low-risk proof"] }, + { ...identifyCandidates([candidate], "uncertain")[0]!, verdict: "PLAUSIBLE", evidence: ["verifier evidence"] }, + ]; + const calls: string[] = []; + const api = apiFor((prompt, options) => { + calls.push(options.label!); + if (options.label?.startsWith("challenge:")) { + assert.match(prompt, /DISPROVE/); + return { outcome: outcome === "upheld" ? "no-counterexample" : "counterexample", evidence: ["challenger evidence"], experiment: "observed experiment" }; + } + assert.match(prompt, /verifier evidence/); + assert.match(prompt, /challenger evidence/); + return { outcome, evidence: ["adjudicator evidence"], reason: "reason" }; + }); + const result = await challengeFindings(api, findings, "snapshot context", { maxChallenges: 1 }, coverage); + assert.equal(calls.length, 2); + assert.deepEqual(result[0], findings[0]); + assert.equal(result[1]?.verdict, outcome === "refuted" ? "REFUTED" : outcome === "unresolved" ? "NOT_SUBSTANTIATED" : "PLAUSIBLE"); + assert.ok(result[1]?.evidence.includes("verifier evidence")); + assert.ok(result[1]?.evidence.includes("challenger evidence")); + assert.equal(result[1]?.challenge?.status, "complete"); + }); +} + +test("challenge is opt-in, bounded, and failures preserve the candidate as unresolved", async () => { + assert.equal(parseChallengeArgs("src").options.maxChallenges, 0); + assert.equal(parseChallengeArgs("src --challenge=99").options.maxChallenges, 10); + assert.equal(parseChallengeArgs("--challenge src").args, "src"); + const coverage: AdvisoryStageCoverage[] = []; + const finding: AdvisoryVerified = { ...identifyCandidates([candidate], "lens")[0]!, verdict: "PLAUSIBLE", evidence: ["original"] }; + const result = await challengeFindings(apiFor(() => { throw new Error("provider unavailable"); }), [finding], "snapshot", { maxChallenges: 1 }, coverage); + assert.equal(result[0]?.verdict, "NOT_SUBSTANTIATED"); + assert.equal(result[0]?.challenge?.status, "failed"); + assert.equal(coverage[0]?.failed, 1); +}); + +for (const value of ["", "-1", "abc", "1.5", "Infinity"]) { + test(`invalid challenge value ${JSON.stringify(value)} fails before discovery`, async () => { + const args = `src --challenge=${value}`; + assert.throws(() => parseChallengeArgs(args), /Invalid --challenge value/); + let calls = 0; + const api = apiFor(() => { calls++; throw new Error("Agent must not start"); }); + api.args = args; + await assert.rejects(codeReview(api), /Invalid --challenge value/); + assert.equal(calls, 0); + }); +} + +test("challenge parser preserves valid defaults, zero and bounded integer limits", () => { + assert.deepEqual(parseChallengeArgs("src --challenge"), { args: "src", options: { maxChallenges: 3 } }); + assert.deepEqual(parseChallengeArgs("src --challenge=0"), { args: "src", options: { maxChallenges: 0 } }); + assert.deepEqual(parseChallengeArgs("--challenge=2 src"), { args: "src", options: { maxChallenges: 2 } }); + assert.deepEqual(parseChallengeArgs("src --challenge=99"), { args: "src", options: { maxChallenges: 10 } }); +}); diff --git a/tests/advisory-scheduling.test.ts b/tests/advisory-scheduling.test.ts index ee92bed..e950774 100644 --- a/tests/advisory-scheduling.test.ts +++ b/tests/advisory-scheduling.test.ts @@ -1,16 +1,10 @@ import assert from "node:assert/strict"; import { test } from "bun:test"; -import type { AdvisoryCandidate, AdvisoryVerdict } from "../.pi/extensions/pi-workflow-engine/src/advisory-schema.ts"; -import { bindParallel, pipeline } from "../.pi/extensions/pi-workflow-engine/src/concurrency.ts"; +import type { AdvisoryCandidate } from "../.pi/extensions/pi-workflow-engine/src/advisory-schema.ts"; +import { bindParallel } from "../.pi/extensions/pi-workflow-engine/src/concurrency.ts"; import { runLensVerificationPipeline, type AdvisoryLens } from "../.pi/extensions/pi-workflow-engine/src/workflow-advisory-utils.ts"; import type { AgentOptions, WorkflowApi } from "../.pi/extensions/pi-workflow-engine/src/types.ts"; -interface Verified extends AdvisoryCandidate { - verdict: AdvisoryVerdict["verdict"]; - evidence: string[]; - lens: AdvisoryLens; -} - const lenses: AdvisoryLens[] = [ { label: "alpha", category: "bug", text: "alpha lens" }, { label: "beta", category: "bug", text: "beta lens" }, @@ -28,20 +22,18 @@ test("finder-barrier scheduling starts all finders before verifiers", async () = return { verdict: "CONFIRMED", evidence: [`evidence for ${label}`], confidence: "high" }; }) as WorkflowApi["agent"]; - const result = await runLensVerificationPipeline({ + const result = await runLensVerificationPipeline({ api: { agent, parallel: bindParallel({ limit: 10 }), - pipeline, + phase() {}, progress() {}, log() {}, }, lenses, perLens: 2, - schedulingMode: "finder-barrier", finderPrompt: (lens) => `find ${lens.label}`, verifierPrompt: (candidate) => `verify ${candidate.summary}`, - makeVerified: (candidate, lens, verdict) => ({ ...candidate, lens, verdict: verdict.verdict, evidence: verdict.evidence }), }); assert.equal(result.verified.length, 2); @@ -58,3 +50,31 @@ function candidateFor(label: string): AdvisoryCandidate { impact: `impact ${label}`, }; } + +test("verifier output cannot replace discovery identities or evidence fields", async () => { + const candidate = candidateFor("original"); + const agent = (async (_prompt: string, opts?: AgentOptions) => { + if (opts?.label?.startsWith("find:")) return { candidates: [candidate] }; + return { + verdict: "CONFIRMED", evidence: ["verified evidence"], + candidateId: "invented", sourceCandidateIds: ["invented"], + summary: "invented summary", category: "invented", locations: [], + reviewAnchor: { file: "invented.ts", line: 1 }, discoveryEvidence: ["invented evidence"], + }; + }) as WorkflowApi["agent"]; + const result = await runLensVerificationPipeline({ + api: { agent, parallel: bindParallel({}), phase() {}, progress() {}, log() {} }, + lenses: lenses.slice(0, 1), perLens: 1, + finderPrompt: () => "find", verifierPrompt: () => "verify", + }); + assert.equal(result.verified.length, 1); + const record = result.verified[0]!; + assert.notEqual(record.candidateId, "invented"); + assert.deepEqual(record.sourceCandidateIds, [record.candidateId]); + assert.equal(record.summary, candidate.summary); + assert.equal(record.category, candidate.category); + assert.deepEqual(record.locations, candidate.locations); + assert.equal(record.reviewAnchor, undefined); + assert.equal(record.discoveryEvidence, undefined); + assert.deepEqual(record.evidence, ["verified evidence"]); +}); diff --git a/tests/advisory-utils.test.ts b/tests/advisory-utils.test.ts index 6e90cf8..93d0fbf 100644 --- a/tests/advisory-utils.test.ts +++ b/tests/advisory-utils.test.ts @@ -1,31 +1,16 @@ import assert from "node:assert/strict"; import { test } from "bun:test"; -import type { AdvisoryFinding } from "../.pi/extensions/pi-workflow-engine/src/advisory-schema.ts"; import { backfillAdvisoryFindings, emptyAdvisoryReport, - findingLocationKey, publishVerifiedKeptProgress, - sameFinding, type AdvisoryVerified, } from "../.pi/extensions/pi-workflow-engine/src/workflow-advisory-utils.ts"; -function finding(file: string, line: number, overrides: Partial = {}): AdvisoryFinding { - return { - summary: "summary", - category: "bug", - severity: "medium", - confidence: "medium", - locations: [{ file, line }], - evidence: [], - impact: "", - recommendation: "", - ...overrides, - }; -} - function verified(file: string, line: number, evidence: string[], impact: string, recommendation: string): AdvisoryVerified { return { + candidateId: "a", + sourceCandidateIds: ["a"], summary: "candidate", category: "bug", locations: [{ file, line }], @@ -57,32 +42,30 @@ test("shared advisory report and verified progress helpers preserve the common c assert.deepEqual(logs, ["3 verified → 2 kept"]); }); -test("findingLocationKey normalizes diff prefixes", () => { - assert.equal(findingLocationKey({ locations: [{ file: "a/src/app.ts", line: 10 }] }), "src/app.ts:10"); - assert.equal(sameFinding({ locations: [{ file: "b/src/app.ts", line: 10 }] }, finding("src/app.ts", 10)), true); +test("synthesis merges IDs and reconstructs all evidence from verified sources", () => { + const source = { ...verified("src/app.ts", 10, ["first evidence"], "first impact", "first recommendation"), candidateId: "a" }; + const other = { ...verified("src/caller.ts", 30, ["second evidence"], "second impact", "second recommendation"), candidateId: "b", sourceCandidateIds: ["b"] }; + const selection = { sourceCandidateIds: ["a", "b"], severity: "high" as const, recommendation: "Fix shared root cause", evidence: ["invented"] }; + const [result] = backfillAdvisoryFindings([selection], [source, other], { impact: "default" }); + assert.deepEqual(result?.sourceCandidateIds, ["a", "b"]); + assert.deepEqual(result?.evidence, ["first evidence", "second evidence"]); + assert.deepEqual(result?.locations, [...source.locations, ...other.locations]); + assert.equal(result?.impact, "first impact\nsecond impact"); }); -test("backfillAdvisoryFindings uses indexed first-ranked source", () => { - const source = verified("src/app.ts", 10, ["first evidence"], "first impact", "first recommendation"); - const duplicate = verified("./src/app.ts", 10, ["second evidence"], "second impact", "second recommendation"); - const [backfilled] = backfillAdvisoryFindings([finding("b/src/app.ts", 10)], [source, duplicate], { - impact: "default impact", - recommendation: "default recommendation", - }); - - assert.deepEqual(backfilled?.evidence, ["first evidence"]); - assert.equal(backfilled?.impact, "first impact"); - assert.equal(backfilled?.recommendation, "first recommendation"); +test("synthesis cannot invent an extra finding or select a refuted source", () => { + const source = { ...verified("src/app.ts", 10, ["disproof"], "impact", "recommendation"), candidateId: "a", verdict: "REFUTED" as const }; + const selection = (id: string) => ({ sourceCandidateIds: [id], severity: "high" as const, recommendation: "Invented repair" }); + assert.deepEqual(backfillAdvisoryFindings([selection("hallucination"), selection("a")], [source], { impact: "default" }), []); }); -test("backfillAdvisoryFindings preserves existing finding fields before defaults", () => { - const [backfilled] = backfillAdvisoryFindings( - [finding("src/missing.ts", 1, { evidence: ["existing"], impact: "existing impact", recommendation: "existing recommendation" })], - [], - { impact: "default impact", recommendation: "default recommendation" }, - ); - - assert.deepEqual(backfilled?.evidence, ["existing"]); - assert.equal(backfilled?.impact, "existing impact"); - assert.equal(backfilled?.recommendation, "existing recommendation"); +test("failed synthesis retains verified evidence and unknown selections become a visible gap", async () => { + const { resolveAdvisorySynthesis } = await import("../.pi/extensions/pi-workflow-engine/src/workflow-advisory-utils.ts"); + const source = { ...verified("src/app.ts", 10, ["proof"], "impact", "repair"), candidateId: "a" }; + const fallback = resolveAdvisorySynthesis(undefined, [source], { impact: "default" }, []); + assert.deepEqual(fallback.findings[0]?.evidence, ["proof"]); + const coverage: import("../.pi/extensions/pi-workflow-engine/src/advisory-evidence.ts").AdvisoryStageCoverage[] = []; + const result = resolveAdvisorySynthesis({ summary: "invented", findings: [{ sourceCandidateIds: ["not-found"], severity: "high", recommendation: "invented" }], nextSteps: [] }, [source], { impact: "default" }, coverage); + assert.equal(result.findings.length, 0); + assert.equal(coverage[0]?.failed, 1); }); diff --git a/tests/agent-runner-workspace-tools.test.ts b/tests/agent-runner-workspace-tools.test.ts index 11d2c3b..52303a2 100644 --- a/tests/agent-runner-workspace-tools.test.ts +++ b/tests/agent-runner-workspace-tools.test.ts @@ -44,9 +44,9 @@ test("runAgent with worktree isolation creates an isolated cwd and returns a pat isolation: "worktree", }); - assert.deepEqual(result, { result: "done", patch: "diff --git a/file b/file\n", changed: true }); + assert.deepEqual(result, { baselineOid: "a".repeat(40), result: "done", patch: "diff --git a/file b/file\n", changed: true }); assert.notEqual(observedCwd, repoCwd); - assert.ok(observedCwd.startsWith("/tmp/pi-workflow-")); + assert.ok(observedCwd.startsWith(join(tmpdir(), "pi-workflow-"))); assert.deepEqual(commandNames(calls), [ "rev-parse --is-inside-work-tree", "worktree add", @@ -87,7 +87,7 @@ test("isolated agents that mutate the main repository are not recorded", async ( "hello", { isolation: "worktree" }, ); - assert.deepEqual(result, { result: "done", patch: "", changed: false }); + assert.deepEqual(result, { baselineOid: "a".repeat(40), result: "done", patch: "", changed: false }); assert.equal(records, 0); } finally { await rm(repoCwd, { recursive: true, force: true }); @@ -113,7 +113,7 @@ test("resume off disables journal reads and writes for isolated agents", async ( "hello", { label: "isolated-off", isolation: "worktree", resume: "off" }, ); - assert.deepEqual(result, { result: "done", patch: "", changed: false }); + assert.deepEqual(result, { baselineOid: "a".repeat(40), result: "done", patch: "", changed: false }); assert.equal(journalCalls, 0); }); @@ -163,7 +163,7 @@ test("runAgent reports isolated worktree cleanup failures without masking succes isolation: "worktree", }); - assert.deepEqual(result, { result: "done", patch: "diff --git a/file b/file\n", changed: true }); + assert.deepEqual(result, { baselineOid: "a".repeat(40), result: "done", patch: "diff --git a/file b/file\n", changed: true }); assert.ok(progress.events.includes("log:isolated: failed to remove isolated worktree (busy)")); assert.equal(registry.size, 1); }); diff --git a/tests/builtin-workflows.test.ts b/tests/builtin-workflows.test.ts index 31b9320..0a4a8ef 100644 --- a/tests/builtin-workflows.test.ts +++ b/tests/builtin-workflows.test.ts @@ -1,3 +1,4 @@ +import { identifyCandidates } from "../.pi/extensions/pi-workflow-engine/src/advisory-evidence.ts"; import assert from "node:assert/strict"; import { test } from "bun:test"; import codeReview from "../.pi/extensions/pi-workflow-engine/workflows/code-review.ts"; @@ -191,7 +192,7 @@ test("code-review verifies one candidate and passes evidence into synthesis", as emptyCandidates(), emptyCandidates(), { verdict: "CONFIRMED", evidence: ["src/example.ts:12 proves the bug"], confidence: "high" }, - report("One confirmed bug.", [finding("confirmed bug", "bug")]), + report("One confirmed bug.", [{ ...finding("confirmed bug", "bug"), sourceCandidateIds: identifyCandidates([surviving], "logic-bugs")[0]!.sourceCandidateIds }]), ]); const result = asReportResult(await codeReview(api, { @@ -248,7 +249,7 @@ test("refactor-scout runs all finder agents before verifier agents", async () => emptyCandidates(), emptyCandidates(), { verdict: "PLAUSIBLE", evidence: ["Two duplicated branches."], confidence: "medium" }, - report("One refactor opportunity.", [finding("extract duplicate helper", "duplication", "medium")]), + report("One refactor opportunity.", [{ ...finding("extract duplicate helper", "duplication", "medium"), sourceCandidateIds: identifyCandidates([opportunity], "duplication")[0]!.sourceCandidateIds }]), ]); const result = asReportResult(await refactorScout(api)); @@ -289,7 +290,7 @@ test("diagnose keeps refuted hypotheses out of final findings", async () => { emptyCandidates(), { verdict: "REFUTED", evidence: ["Fixture is current."], confidence: "low" }, { verdict: "CONFIRMED", evidence: ["Condition is inverted."], confidence: "high" }, - report("One root cause.", [finding("wrong branch condition", "root-cause")]), + report("One root cause.", [{ ...finding("wrong branch condition", "root-cause"), sourceCandidateIds: identifyCandidates([refuted, confirmed], "recent-change")[1]!.sourceCandidateIds }]), ]); const result = asReportResult(await diagnose(api)); @@ -343,7 +344,7 @@ test("perf-review keeps weak measurement findings advisory", async () => { emptyCandidates(), { candidates: [measurementGap] }, { verdict: "PLAUSIBLE", evidence: ["No benchmark output is checked in."], confidence: "low" }, - report("Measurement gap only.", [measurementFinding]), + report("Measurement gap only.", [{ ...measurementFinding, sourceCandidateIds: identifyCandidates([measurementGap], "measurement")[0]!.sourceCandidateIds }]), ]); const result = asReportResult(await perfReview(api)); diff --git a/tests/changedlines.test.ts b/tests/changedlines.test.ts index 4542ef5..d4569c8 100644 --- a/tests/changedlines.test.ts +++ b/tests/changedlines.test.ts @@ -1,9 +1,9 @@ +import { dedupeCandidates, identifyCandidates } from "../.pi/extensions/pi-workflow-engine/src/advisory-evidence.ts"; import assert from "node:assert/strict"; import { test } from "bun:test"; import { changedLines, inDiff } from "../.pi/extensions/pi-workflow-engine/workflows/code-review.ts"; import { buildCodeReviewScopeBlock, - dedupeCodeReviewCandidates, } from "../.pi/extensions/pi-workflow-engine/src/review/code-review-orchestration.ts"; function lines(map: Map>, file: string): number[] { @@ -61,13 +61,11 @@ test("code-review candidate deduplication retains first discovery order", () => const duplicate = candidate("duplicate", "./src/app.ts", 12); const distinct = candidate("distinct", "src/app.ts", 30); - assert.deepEqual(dedupeCodeReviewCandidates([ - { angle: "logic", candidates: [first] }, - { angle: "edge", candidates: [duplicate, distinct] }, - ]), [ - { angle: "logic", candidate: first }, - { angle: "edge", candidate: distinct }, - ]); + const discovered = [ + ...identifyCandidates([first], "logic"), + ...identifyCandidates([duplicate, distinct], "edge"), + ]; + assert.deepEqual(dedupeCandidates(discovered), discovered); }); test("changedLines records multi-hunk edits and new files", () => { diff --git a/tests/concurrency.test.ts b/tests/concurrency.test.ts index d882898..1c76cd5 100644 --- a/tests/concurrency.test.ts +++ b/tests/concurrency.test.ts @@ -531,3 +531,66 @@ test("pipeline propagates a fatal abort", async () => { /stop/, ); }); + +test("Semaphore reserves the released slot before a newcomer microtask", async () => { + const semaphore = new Semaphore(1); + const gate = Promise.withResolvers(); + const order: string[] = []; + let active = 0; + let peak = 0; + const work = (name: string) => async () => { + order.push(name); + peak = Math.max(peak, ++active); + await Promise.resolve(); + await Promise.resolve(); + await Promise.resolve(); + active--; + }; + const a = semaphore.run(() => gate.promise); + const b = semaphore.run(work("B")); + gate.resolve(); + const c = Promise.resolve().then(() => semaphore.run(work("C"))); + await Promise.all([a, b, c]); + assert.equal(peak, 1); + assert.deepEqual(order, ["B", "C"]); +}); + +test("Semaphore hands a selected cancelled reservation onward", async () => { + const semaphore = new Semaphore(1); + const gate = Promise.withResolvers(); + const controller = new AbortController(); + const a = semaphore.run(() => gate.promise); + const b = semaphore.run(async () => assert.fail("cancelled work ran"), { signal: controller.signal }); + const rejected = assert.rejects(b, /selected/); + gate.resolve(); + queueMicrotask(() => controller.abort(new WorkflowAbortError("selected"))); + const c = semaphore.run(async () => "next"); + await Promise.all([a, rejected]); + assert.equal(await c, "next"); + assert.equal(await semaphore.run(async () => "still available"), "still available"); +}); + +test("Semaphore maintains capacity and FIFO through repeated cancellation interleavings", async () => { + for (let round = 0; round < 30; round++) { + const semaphore = new Semaphore(1 + round % 4); + const controllers = Array.from({ length: 30 }, () => new AbortController()); + const admitted: number[] = []; + let active = 0; + let peak = 0; + const work = controllers.map((controller, index) => semaphore.run(async () => { + admitted.push(index); + peak = Math.max(peak, ++active); + for (let tick = 0; tick <= (index + round) % 5; tick++) await Promise.resolve(); + active--; + }, { signal: controller.signal })); + const settled = Promise.allSettled(work); + for (let i = 0; i < controllers.length; i++) { + if ((i + round) % 3 === 0) controllers[i]!.abort(); + await Promise.resolve(); + } + await settled; + assert.ok(peak <= 1 + round % 4); + assert.deepEqual(admitted, [...admitted].sort((a, b) => a - b)); + assert.equal(await semaphore.run(async () => 42), 42); + } +}); diff --git a/tests/patch-validation.test.ts b/tests/patch-validation.test.ts new file mode 100644 index 0000000..9ff96a5 --- /dev/null +++ b/tests/patch-validation.test.ts @@ -0,0 +1,223 @@ +import assert from "node:assert/strict"; +import { test } from "bun:test"; +import { mkdtemp, readFile, writeFile, rm } from "node:fs/promises"; +import { watch } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { execFileSync } from "node:child_process"; +import { validateCandidatePatch, type PatchEvaluation } from "../.pi/extensions/pi-workflow-engine/src/review/patch-validation.ts"; +import { fingerprintReviewWorktreeBaseline } from "../.pi/extensions/pi-workflow-engine/src/review/review-snapshot.ts"; +import { runReviewFixWorkflow } from "../.pi/extensions/pi-workflow-engine/src/review/review-fix-workflow.ts"; +import { createAgentWorkspace } from "../.pi/extensions/pi-workflow-engine/src/agent-workspace.ts"; +import { WorktreeRegistry, captureWorktreePatch } from "../.pi/extensions/pi-workflow-engine/src/worktree.ts"; +import { bindParallel } from "../.pi/extensions/pi-workflow-engine/src/concurrency.ts"; +import { toReviewIssues } from "../.pi/extensions/pi-workflow-engine/src/review/review-issues.ts"; +import type { AgentOptions, WorkflowApi } from "../.pi/extensions/pi-workflow-engine/src/types.ts"; + +async function fixture() { + const cwd = await mkdtemp(join(tmpdir(), "patch-validation-")); + const git = (...args: string[]) => execFileSync("git", args, { cwd, encoding: "utf8" }).trim(); + git("init", "-q"); + await writeFile(join(cwd, "value.txt"), "broken\n"); + git("add", "."); + git("-c", "user.name=Test", "-c", "user.email=test@example.invalid", "-c", "commit.gpgSign=false", "commit", "-qm", "baseline"); + const baseline = { ref: git("rev-parse", "HEAD") }; + await writeFile(join(cwd, "value.txt"), "fixed\n"); + const captured = await captureWorktreePatch({ worktreePath: cwd, baselineOid: baseline.ref }); + assert.ok(!("error" in captured)); + git("restore", "value.txt"); + return { cwd, baseline, expectedFingerprint: fingerprintReviewWorktreeBaseline(baseline), baselineOid: baseline.ref, patch: captured.patch, + cleanup: () => rm(cwd, { recursive: true, force: true }) }; +} +const check = { file: process.execPath, args: ["-e", "const fs = require('node:fs'); if(fs.readFileSync('value.txt','utf8') !== 'fixed\\n') { console.error('value remains broken'); process.exit(1); }"], required: true }; +const accepted: PatchEvaluation = { outcome: "accepted", reason: "Repair addresses the condition", checks: [check] }; + +for (const scenario of ["verified", "rejected", "blocked", "stale", "wrong-baseline", "no-patch"] as const) { + test(`candidate validation: ${scenario}`, async () => { + const repo = await fixture(); + try { + const evaluation: PatchEvaluation = scenario === "rejected" ? { ...accepted, checks: [{ ...check, args: ["-e", "console.error('regression'); process.exit(1)"] }] } + : scenario === "blocked" ? { ...accepted, checks: [check, { ...check, file: "unavailable-validation-tool-123" }] } : accepted; + const result = await validateCandidatePatch({ ...repo, evaluation, + ...(scenario === "stale" ? { expectedFingerprint: "stale" } : {}), + ...(scenario === "wrong-baseline" ? { baselineOid: "b".repeat(40) } : {}), + ...(scenario === "no-patch" ? { patch: "" } : {}), + }); + assert.equal(result.status, scenario === "stale" || scenario === "wrong-baseline" ? "rejected" : scenario); + assert.equal(result.baselineFingerprint, repo.expectedFingerprint); + assert.match(result.patchHash, /^[a-f0-9]{64}$/); + if (scenario === "verified") assert.equal(result.checks[0]?.result.ok, true); + if (scenario === "rejected") { + assert.equal(result.checks[0]?.result.ok, false); + assert.match(result.checks[0]?.result.stderr ?? "", /regression/); + } + if (scenario === "no-patch") assert.match(result.reason, /does not establish/); + assert.equal(await readFile(join(repo.cwd, "value.txt"), "utf8"), "broken\n"); + const worktrees = execFileSync("git", ["worktree", "list", "--porcelain"], { cwd: repo.cwd, encoding: "utf8" }); + assert.equal((worktrees.match(/^worktree /gm) ?? []).length, 1); + } finally { await repo.cleanup(); } + }); +} + +test("regression validation observes intended baseline failure and repaired success", async () => { + const repo = await fixture(); + try { + const testPatch = "diff --git a/regression.txt b/regression.txt\nnew file mode 100644\n--- /dev/null\n+++ b/regression.txt\n@@ -0,0 +1 @@\n+regression fixture\n"; + const result = await validateCandidatePatch({ ...repo, evaluation: { ...accepted, checks: [{ ...check, regression: { baselinePatch: testPatch, expectedFailure: "value remains broken" } }] } }); + assert.equal(result.status, "verified"); + assert.deepEqual(result.checks.map((entry) => [entry.stage, entry.result.ok]), [["candidate", true], ["baseline", false]]); + const invalid = await validateCandidatePatch({ ...repo, evaluation: { ...accepted, checks: [{ ...check, regression: { baselinePatch: testPatch, expectedFailure: "unrelated failure" } }] } }); + assert.equal(invalid.status, "blocked"); + } finally { await repo.cleanup(); } +}); + +test("blocked validation retains the baseline regression setup failure", async () => { + const repo = await fixture(); + try { + const result = await validateCandidatePatch({ ...repo, evaluation: { + ...accepted, checks: [{ ...check, regression: { baselinePatch: "invalid patch", expectedFailure: "value remains broken" } }], + } }); + assert.equal(result.status, "blocked"); + assert.match(result.reason, /Baseline regression setup failed:/); + assert.match(result.reason, /No valid patches/); + assert.deepEqual(result.checks.map((entry) => [entry.stage, entry.result.ok]), [["candidate", true]]); + } finally { await repo.cleanup(); } +}); + +test("fix workflow uses a fresh evaluator, retains rejected patches, and cannot trust implementer success claims", async () => { + const repo = await fixture(); + const registry = new WorktreeRegistry(repo.cwd); + const paths: string[] = []; + const issues = toReviewIssues("code-review", { findings: [{ summary: "Wrong value", category: "bug", severity: "high", confidence: "high", locations: [{ file: "value.txt", line: 1 }], evidence: ["broken value"], impact: "request failure", recommendation: "repair" }] }); + try { + const agent = (async (_prompt: string, options: AgentOptions) => { + const workspace = await createAgentWorkspace({ cwd: repo.cwd, worktrees: registry, signal: undefined, progress: { log() {} } }, { ...options, worktreeBaseline: repo.baseline }, options.label!); + paths.push(workspace.cwd); + try { + if (options.label?.startsWith("fix:")) { + await writeFile(join(workspace.cwd, "value.txt"), "fixed\n"); + return await workspace.wrapResult("VERIFIED: all checks passed (implementer claim)"); + } + assert.equal(await readFile(join(workspace.cwd, "value.txt"), "utf8"), "fixed\n"); + assert.notEqual(workspace.cwd, paths[0]); + return await workspace.wrapResult({ outcome: "rejected", reason: "Repair breaks a caller", checks: [check] }); + } finally { await workspace.dispose(); } + }) as WorkflowApi["agent"]; + const result = await runReviewFixWorkflow({ agent, parallel: bindParallel({}), phase() {}, signal: undefined, cwd: repo.cwd }, issues, + { workflowName: "code-review", target: "", files: ["value.txt"], diffTarget: { kind: "git", args: [] }, snapshot: { baselineFingerprint: repo.expectedFingerprint, diffFingerprint: "a".repeat(64) } }, repo.baseline); + assert.equal(paths.length, 2); + const preview = result.fixes[0]!; + assert.ok("patch" in preview); + assert.equal(preview.validation.status, "rejected"); + assert.equal(preview.patch, repo.patch); + assert.match(preview.result, /VERIFIED/); + assert.match(result.summary, /0 verified/); + } finally { await registry.removeAll(); await repo.cleanup(); } +}); + +test("evaluator setup rejects a stale candidate before starting and releases its worktree", async () => { + const repo = await fixture(); + const registry = new WorktreeRegistry(repo.cwd); + try { + await assert.rejects(createAgentWorkspace({ cwd: repo.cwd, worktrees: registry, signal: undefined, progress: { log() {} } }, + { isolation: "worktree", worktreeBaseline: repo.baseline, candidatePatch: { baselineOid: "b".repeat(40), patch: repo.patch } }, "evaluator"), /baseline differs/); + assert.equal(registry.size, 0); + } finally { await registry.removeAll(); await repo.cleanup(); } +}); + +test("evaluator reconstruction includes the reviewed dirty snapshot", async () => { + const repo = await fixture(); + const registry = new WorktreeRegistry(repo.cwd); + try { + const dirtyPatch = "diff --git a/reviewed.txt b/reviewed.txt\nnew file mode 100644\n--- /dev/null\n+++ b/reviewed.txt\n@@ -0,0 +1 @@\n+reviewed dirty state\n"; + const baseline = { ...repo.baseline, patch: dirtyPatch }; + const prepared = await registry.add(undefined, baseline); + assert.ok(!("error" in prepared)); + const result = await validateCandidatePatch({ ...repo, baseline, expectedFingerprint: fingerprintReviewWorktreeBaseline(baseline), baselineOid: prepared.baselineOid, + evaluation: { ...accepted, checks: [{ ...check, args: ["-e", "const fs=require('node:fs'); if(fs.readFileSync('value.txt','utf8') !== 'fixed\\n' || fs.readFileSync('reviewed.txt','utf8') !== 'reviewed dirty state\\n') process.exit(1)"] }] } }); + assert.equal(result.status, "verified"); + assert.equal(result.baselineOid, prepared.baselineOid); + } finally { await registry.removeAll(); await repo.cleanup(); } +}); + +test("fatal evaluator cancellation aborts the fix workflow instead of becoming blocked", async () => { + const { WorkflowAbortError } = await import("../.pi/extensions/pi-workflow-engine/src/cancellation.ts"); + const baseline = { ref: "a".repeat(40) }; + const issues = toReviewIssues("code-review", { findings: [{ summary: "bug", category: "bug", severity: "high", confidence: "high", locations: [], evidence: [], impact: "impact", recommendation: "repair" }] }); + const agent = (async (_prompt: string, options: AgentOptions) => { + if (options.label?.startsWith("fix:")) return { result: "done", patch: "candidate", changed: true, baselineOid: baseline.ref }; + throw new WorkflowAbortError("evaluator cancelled"); + }) as WorkflowApi["agent"]; + await assert.rejects(runReviewFixWorkflow({ agent, parallel: bindParallel({}), phase() {}, signal: undefined, cwd: process.cwd() }, issues, + { workflowName: "code-review", target: "", files: [], diffTarget: { kind: "git", args: [] }, snapshot: { baselineFingerprint: fingerprintReviewWorktreeBaseline(baseline), diffFingerprint: "a".repeat(64) } }, baseline), /evaluator cancelled/); +}); + +for (const status of ["verified", "rejected", "blocked"] as const) { + test(`cleanup failure preserves ${status} validation evidence in the fix workflow`, async () => { + const repo = await fixture(); + const evaluation: PatchEvaluation = { + outcome: status === "blocked" ? "blocked" : "accepted", + reason: status === "blocked" ? "Required service is unavailable" : "Independent evaluation complete", + checks: [{ + file: process.execPath, + args: ["-e", `require('node:child_process').execFileSync('git', ['worktree', 'lock', process.cwd()]); console.error('observed check output'); process.exit(${status === "rejected" ? 1 : 0})`], + required: true, + }], + }; + const issues = toReviewIssues("code-review", { findings: [{ summary: "Wrong value", category: "bug", severity: "high", confidence: "high", locations: [], evidence: [], impact: "failure", recommendation: "repair" }] }); + const agent = (async (_prompt: string, options: AgentOptions) => ({ + result: options.label?.startsWith("fix:") ? "Implementation complete" : evaluation, + patch: repo.patch, changed: true, baselineOid: repo.baselineOid, + })) as WorkflowApi["agent"]; + try { + const result = await runReviewFixWorkflow({ agent, parallel: bindParallel({}), phase() {}, cwd: repo.cwd, signal: undefined }, issues, + { workflowName: "code-review", target: "", files: [], diffTarget: { kind: "git", args: [] }, snapshot: { baselineFingerprint: repo.expectedFingerprint, diffFingerprint: "a".repeat(64) } }, repo.baseline); + const preview = result.fixes[0]!; + assert.ok("patch" in preview); + assert.equal(preview.patch, repo.patch); + assert.equal(preview.validation.status, status === "verified" ? "blocked" : status); + assert.deepEqual(preview.validation.evaluation, evaluation); + assert.equal(preview.validation.checks.length, 1); + assert.equal(preview.validation.checks[0]?.result.ok, status !== "rejected"); + assert.match(preview.validation.checks[0]?.result.stderr ?? "", /observed check output/); + assert.match(preview.validation.cleanupError ?? "", /locked working tree/); + assert.match(preview.validation.reason, /Cleanup failed:/); + if (status === "blocked") assert.match(preview.validation.reason, /Required service is unavailable/); + if (status === "rejected") assert.match(preview.validation.reason, /observed check output/); + assert.match(result.summary, /0 verified/); + } finally { + await removeLockedWorktrees(repo.cwd); + await repo.cleanup(); + } + }); +} + +async function removeLockedWorktrees(cwd: string): Promise { + const list = execFileSync("git", ["worktree", "list", "--porcelain"], { cwd, encoding: "utf8" }); + for (const line of list.split("\n")) { + if (!line.startsWith("worktree ") || line.slice(9) === cwd) continue; + const path = line.slice(9); + execFileSync("git", ["worktree", "unlock", path], { cwd }); + execFileSync("git", ["worktree", "remove", "--force", path], { cwd }); + } +} + +test("cleanup failure does not replace cancellation during validation", async () => { + const repo = await fixture(); + const controller = new AbortController(); + const cancelled = new Error("Validation cancelled by user"); + const watcher = watch(repo.cwd, (_event, filename) => { + if (filename === "locked-marker") controller.abort(cancelled); + }); + try { + await assert.rejects(validateCandidatePatch({ ...repo, signal: controller.signal, evaluation: { + ...accepted, checks: [{ file: process.execPath, required: true, args: ["-e", + `require('node:child_process').execFileSync('git', ['worktree', 'lock', process.cwd()]); require('node:fs').writeFileSync(${JSON.stringify(join(repo.cwd, "locked-marker"))}, 'ready'); setTimeout(() => {}, 30000);`, + ] }], + } }), (error) => error === cancelled); + } finally { + watcher.close(); + await removeLockedWorktrees(repo.cwd); + await repo.cleanup(); + } +}); diff --git a/tests/review-actions.test.ts b/tests/review-actions.test.ts index 845fe5b..f2b682d 100644 --- a/tests/review-actions.test.ts +++ b/tests/review-actions.test.ts @@ -1,3 +1,4 @@ +import { initialPatchValidation } from "../.pi/extensions/pi-workflow-engine/src/review/patch-validation.ts"; import assert from "node:assert/strict"; import { test } from "bun:test"; import type { AdvisoryReport } from "../.pi/extensions/pi-workflow-engine/src/advisory-schema.ts"; @@ -156,7 +157,10 @@ test("fix workflow keeps finding ids, isolated patches, and per-finding failures changed: true, }; }; + const baseline = { ref: "a".repeat(40) }; const api: ReviewFixWorkflowApi = { + cwd: process.cwd(), + signal: undefined, get parallel() { parallelRead = true; return parallel; @@ -167,7 +171,7 @@ test("fix workflow keeps finding ids, isolated patches, and per-finding failures agent: agent as ReviewFixWorkflowApi["agent"], }; - const result = await runReviewFixWorkflow(api, issues, context); + const result = await runReviewFixWorkflow(api, issues, context, baseline); assert.deepEqual(phases, ["Generate patch previews"]); assert.equal(parallelRead, true); @@ -175,7 +179,7 @@ test("fix workflow keeps finding ids, isolated patches, and per-finding failures assert.equal(calls[0]?.options.isolation, "worktree"); assert.equal(calls[0]?.options.label, "fix:R001"); assert.equal(calls[0]?.options.phase, "Generate patch previews"); - assert.equal(calls[0]?.options.thinkingLevel, "medium"); + assert.equal(calls[0]?.options.profile, "medium"); assert.ok(calls[0]?.options.tools?.includes("edit")); assert.ok(calls[0]?.options.tools?.includes("write")); assert.deepEqual(calls[0]?.options.toolHints, ["search"]); @@ -188,12 +192,13 @@ test("fix workflow keeps finding ids, isolated patches, and per-finding failures result: "Updated src/app.ts and ran bun test tests/retry.test.ts (passed).", patch: "diff --git a/src/app.ts b/src/app.ts\n+fixed\n", changed: true, + validation: { ...initialPatchValidation(baseline, undefined, "diff --git a/src/app.ts b/src/app.ts\n+fixed\n"), reason: "Candidate has no recorded baseline identity." }, }); assert.deepEqual(result.fixes[1], { findingId: "R002", error: { name: "Error", message: "validation environment unavailable" }, }); - assert.match(result.summary, /Generated 1 patch preview\(s\)/); + assert.match(result.summary, /1 blocked/); assert.match(result.summary, /1 attempt\(s\) failed/); assert.deepEqual(JSON.parse(JSON.stringify(result)), result); }); diff --git a/tests/sub-workflow.test.ts b/tests/sub-workflow.test.ts index 9ee7618..6066021 100644 --- a/tests/sub-workflow.test.ts +++ b/tests/sub-workflow.test.ts @@ -232,7 +232,7 @@ test("engine execution metadata injects the reviewed baseline into isolated work const result = await runWorkflowWithContext(rc, progress, mod, "", contextOpts()); - assert.deepEqual(result, { result: "ok", patch: "", changed: false }); + assert.deepEqual(result, { baselineOid: "0123456789012345678901234567890123456789", result: "ok", patch: "", changed: false }); assert.match(sessionCwd ?? "", /pi-workflow-/); const add = gitCalls.find((call) => call.args[0] === "worktree" && call.args[1] === "add"); assert.equal(add?.args.at(-1), baseline.ref); diff --git a/tests/workflow-run-store.test.ts b/tests/workflow-run-store.test.ts index 7cb1672..dc33b06 100644 --- a/tests/workflow-run-store.test.ts +++ b/tests/workflow-run-store.test.ts @@ -269,7 +269,7 @@ test("project run store atomically reloads records and isolates corrupt or futur assert.equal(await store.load("future-version"), undefined); assert.equal(await store.load("wrong-path"), undefined); assert.equal(await store.load("invalid-state-fields"), undefined); - assert.deepEqual((await store.list()).map((record) => record.runId), ["future-fields", "kept"]); + assert.deepEqual((await store.list()).map((record) => record.runId).sort(), ["future-fields", "kept"]); } finally { await rm(cwd, { recursive: true, force: true }); } diff --git a/tests/workflow-ui.test.ts b/tests/workflow-ui.test.ts index f309ed2..fa6afa3 100644 --- a/tests/workflow-ui.test.ts +++ b/tests/workflow-ui.test.ts @@ -471,3 +471,16 @@ const validReport = { nextSteps: ["Inspect src/app.ts retry loop", "Add a retry-boundary regression test"], stats: { files: 2, candidates: 3, verified: 1, kept: 1 }, }; + +test("incomplete advisory coverage cannot render a green clean-review result", () => { + const rendered = renderWorkflowResultText("code-review", { + summary: "Incomplete review", findings: [], nextSteps: ["Rerun missing work"], status: "incomplete", + coverage: [{ stage: "Verify", expected: 2, completed: 0, failed: 2, failures: [{ branch: "a", reason: "provider failed" }, { branch: "b", reason: "provider failed" }] }], + gaps: ["Verify/a: provider failed", "Verify/b: provider failed"], + }, true, createTestTheme()); + assert.match(rendered, /⚠/); + assert.match(rendered, /Verify: 0\/2 complete, 2 failed/); + assert.match(rendered, /coverage is incomplete/); + assert.match(rendered, /Verify\/a: provider failed/); + assert.doesNotMatch(rendered, /✓|No findings\./); +});