From 10b4f2180364d0ebf493ddd9dfef1f1dbdf646d7 Mon Sep 17 00:00:00 2001 From: timbrinded <79199034+timbrinded@users.noreply.github.com> Date: Wed, 16 Sep 2026 17:06:19 +0100 Subject: [PATCH 1/5] fix: preserve workflow evidence and independently validate repairs --- .../src/advisory-challenge.ts | 74 +++++++++ .../src/advisory-evidence.ts | 94 ++++++++++++ .../pi-workflow-engine/src/advisory-schema.ts | 15 +- .../pi-workflow-engine/src/agent-workspace.ts | 21 ++- .../pi-workflow-engine/src/concurrency.ts | 18 ++- .../pi-workflow-engine/src/journal.ts | 1 + .../src/review/code-review-orchestration.ts | 21 +-- .../src/review/patch-validation.ts | 93 ++++++++++++ .../src/review/review-fix-workflow.ts | 41 ++++- .../src/review/review-format.ts | 3 +- .../src/review/review-issues.ts | 2 +- .../pi-workflow-engine/src/types.ts | 3 + .../src/ui/workflow-result-renderer.ts | 7 +- .../src/workflow-advisory-utils.ts | 136 ++++++++++------- .../workflows/code-review.ts | 86 ++++++----- .../pi-workflow-engine/workflows/diagnose.ts | 93 ++++++------ .../workflows/perf-review.ts | 56 ++++--- .../workflows/refactor-scout.ts | 56 ++++--- USAGE.md | 56 +++++++ tests/advisory-provenance.test.ts | 117 ++++++++++++++ tests/advisory-utils.test.ts | 43 +++--- tests/agent-runner-workspace-tools.test.ts | 10 +- tests/builtin-workflows.test.ts | 9 +- tests/changedlines.test.ts | 1 + tests/concurrency.test.ts | 63 ++++++++ tests/patch-validation.test.ts | 143 ++++++++++++++++++ tests/review-actions.test.ts | 6 +- tests/sub-workflow.test.ts | 2 +- tests/workflow-run-store.test.ts | 2 +- tests/workflow-ui.test.ts | 13 ++ 30 files changed, 1027 insertions(+), 258 deletions(-) create mode 100644 .pi/extensions/pi-workflow-engine/src/advisory-challenge.ts create mode 100644 .pi/extensions/pi-workflow-engine/src/advisory-evidence.ts create mode 100644 .pi/extensions/pi-workflow-engine/src/review/patch-validation.ts create mode 100644 tests/advisory-provenance.test.ts create mode 100644 tests/patch-validation.test.ts 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..7884ae7 --- /dev/null +++ b/.pi/extensions/pi-workflow-engine/src/advisory-challenge.ts @@ -0,0 +1,74 @@ +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 interface ChallengeRecord { + challenge?: Static; + adjudication?: Static; + status: "complete" | "failed"; +} +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(?:=(\d+))?(?=\s|$)/g, (_match, limit: string | undefined) => { + 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..87d54e5 --- /dev/null +++ b/.pi/extensions/pi-workflow-engine/src/advisory-evidence.ts @@ -0,0 +1,94 @@ +import { createHash } from "node:crypto"; +import { Type, type Static } from "typebox"; +import type { AdvisoryCandidate, 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): AdvisoryCandidate[] { + 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(/[.!?]+$/, ""); +} + +export 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([...candidateIds(previous), ...candidateIds(candidate)])]; + 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 candidateIds(candidate: AdvisoryCandidate): string[] { + return candidate.sourceCandidateIds ?? (candidate.candidateId ? [candidate.candidateId] : []); +} + +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: Type.Union([Type.Literal("low"), Type.Literal("medium"), Type.Literal("high")]), + 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..233f059 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,10 @@ export const AdvisoryLocationSchema = Type.Object({ }); export const AdvisoryCandidateSchema = Type.Object({ + candidateId: Type.Optional(Type.String()), + sourceCandidateIds: Type.Optional(Type.Array(Type.String())), + 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 +42,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 +56,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." })), diff --git a/.pi/extensions/pi-workflow-engine/src/agent-workspace.ts b/.pi/extensions/pi-workflow-engine/src/agent-workspace.ts index f3b6325..055a15e 100644 --- a/.pi/extensions/pi-workflow-engine/src/agent-workspace.ts +++ b/.pi/extensions/pi-workflow-engine/src/agent-workspace.ts @@ -1,6 +1,6 @@ import type { AgentExecutionOptions, AgentProgress } from "./agent-runner-types.ts"; import { unknownErrorMessage } from "./unknown-error.ts"; -import type { WorktreeRegistry } from "./worktree.ts"; +import { spawnGitRunner, type WorktreeRegistry } from "./worktree.ts"; interface AgentWorkspaceBase { readonly cwd: string; @@ -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,22 @@ 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 spawnGitRunner.runGit({ + cwd: worktreePath, args: ["apply", "--binary", "--index", "-"], + stdin: opts.candidatePatch.patch, signal: rc.signal, timeoutMs: 30_000, + }); + 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 +79,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..4b233ea 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,5 +1,5 @@ import type { AdvisoryCandidate } from "../advisory-schema.ts"; -import { normalizePath, primaryLocation } from "../workflow-advisory-utils.ts"; +import { dedupeCandidates } from "../advisory-evidence.ts"; const DIFF_EMBED_CAP = 60_000; @@ -29,21 +29,6 @@ export function buildCodeReviewScopeBlock(input: { 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}`; + return dedupeCandidates(groups.flatMap(({ angle, candidates }) => candidates.map((candidate) => ({ ...candidate, angle })))) + .map(({ angle, ...candidate }) => ({ angle, candidate })); } 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..258c295 --- /dev/null +++ b/.pi/extensions/pi-workflow-engine/src/review/patch-validation.ts @@ -0,0 +1,93 @@ +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, spawnGitRunner, 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; +} +export function initialPatchValidation(baseline: WorktreeBaseline | undefined, baselineOid: string | undefined, patch: string): PatchValidation { + return { status: patch.trim() ? "blocked" : "no-patch", baselineFingerprint: baseline ? fingerprintReviewWorktreeBaseline(baseline) : "unavailable", ...(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." }; +} + +/** Reconstruct the exact reviewed baseline and run evaluator-selected checks ourselves. */ +export async function validateCandidatePatch(options: { + cwd: string; baseline: WorktreeBaseline; expectedFingerprint: string; baselineOid: string; patch: string; + evaluation: PatchEvaluation; signal?: AbortSignal; +}): 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); + try { + 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 spawnGitRunner.runGit({ cwd: candidate.path, args: ["apply", "--index", "--binary", "-"], stdin: options.patch, timeoutMs: 30_000, signal: options.signal }); + if (!applied.ok) return { ...validation, status: "rejected", reason: applied.error ?? applied.stderr }; + let rejected = options.evaluation.outcome === "rejected"; + let blocked = options.evaluation.outcome === "blocked" || !options.evaluation.checks.some((check) => check.required); + 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 = true; + else if (check.required) blocked = true; + } + if (check.regression) { + const original = await worktrees.add(options.signal, options.baseline); + if ("error" in original) { blocked = true; continue; } + const testApplied = await spawnGitRunner.runGit({ cwd: original.path, args: ["apply", "--index", "--binary", "-"], stdin: check.regression.baselinePatch, timeoutMs: 30_000, signal: options.signal }); + if (!testApplied.ok) { blocked = true; validation.reason = `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 = true; + } + } + // Tests must not silently replace the candidate under evaluation. + const after = await worktrees.capturePatch(candidate.path, candidate.baselineOid, options.signal); + if ("error" in after || after.patch !== options.patch) blocked = true; + return { ...validation, status: rejected ? "rejected" : blocked ? "blocked" : "verified", + reason: rejected ? "Independent evaluation or an executed check rejected the candidate." : blocked ? "Required validation could not be completed against the unchanged candidate." : "Independent evaluation and all required checks passed." }; + } catch (error) { + throwIfAborted(options.signal); + return { ...validation, status: "blocked", reason: unknownErrorMessage(error) }; + } finally { + await worktrees.removeAll(); + } +} + +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..4654f81 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,3 +1,7 @@ +import { initialPatchValidation, PatchEvaluationSchema, validateCandidatePatch, type PatchValidation } from "./patch-validation.ts"; +import { fingerprintReviewWorktreeBaseline } from "./review-snapshot.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 { WorktreeBaseline } from "../worktree.ts"; @@ -14,6 +18,7 @@ export interface ReviewFixPreview { readonly result: string; readonly patch: string; readonly changed: boolean; + readonly validation: PatchValidation; } export interface ReviewFixFailure { @@ -28,7 +33,7 @@ export interface ReviewFixWorkflowResult { readonly fixes: readonly ReviewFixOutcome[]; } -export type ReviewFixWorkflowApi = Pick; +export type ReviewFixWorkflowApi = Pick & Partial>; /** Build an ephemeral workflow that generates one isolated patch preview per finding. */ export function createReviewFixWorkflow( @@ -40,9 +45,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 +81,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 +90,39 @@ 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"], }); + let validation = initialPatchValidation(baseline, isolated.baselineOid, isolated.patch); + if (isolated.patch.trim() && baseline && isolated.baselineOid && api.cwd && context?.snapshot) { + if (fingerprintReviewWorktreeBaseline(baseline) !== context.snapshot.baselineFingerprint) { + validation = { ...validation, status: "rejected", reason: "Stale reviewed baseline identity." }; + } else 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) { + validation = { ...validation, status: "rejected", reason: "Evaluator changed the candidate or used a different baseline.", evaluation: evaluated.result }; + } else { + validation = 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; + validation = { ...validation, status: "blocked", reason: unknownErrorMessage(error) }; + } + } return { findingId: issue.id, result: isolated.result, patch: isolated.patch, changed: isolated.changed, + validation, }; }), { settled: true }, @@ -103,11 +132,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, }; } 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..04fe4a8 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 { candidateDedupKey, candidateIds, collectAdvisoryStage, dedupeCandidates, identifyCandidates, uniqueLocations, type AdvisoryStageCoverage, type AdvisorySynthesis } from "./advisory-evidence.ts"; import type { AgentOptions, WorkflowApi, WorkflowProgressEvent, WorkflowRunStats } from "./types.ts"; export interface AdvisoryLens { @@ -12,6 +12,7 @@ export type AdvisoryVerified { @@ -19,6 +20,7 @@ export interface LensVerificationPipelineResult { rawCandidates: number; dropped: number; refuted: number; + coverage: AdvisoryStageCoverage[]; } /** Default concrete read/inspect tools for advisory workflows. */ @@ -94,7 +96,7 @@ export async function runLensVerificationPipeline(); + const coverage: AdvisoryStageCoverage[] = []; let rawCandidates = 0; let dropped = 0; let refuted = 0; @@ -108,7 +110,8 @@ export async function runLensVerificationPipeline): 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 verifyCandidate = async ({ lens, candidate }: NovelCandidate): Promise => { const location = primaryLocation(candidate); - const judged = await api.agent(verifierPrompt(candidate), { + const judged = await api.agent(`${verifierPrompt(candidate)}\nCandidate record: ${JSON.stringify(candidate)}`, { phase: verifierPhase, label: `verify:${location.file.split("/").pop() ?? location.file}`, tools, @@ -145,40 +137,34 @@ export async function runLensVerificationPipeline { refuted += 1; }); return makeVerified(candidate, lens, judged); }; + const verify = async (found: FoundForLens[]): Promise => { + const entries = dedupeCandidates(found.flatMap(({ lens, candidates }) => candidates.map((candidate) => ({ ...candidate, lens })))); + dropped += found.reduce((count, group) => count + group.candidates.length, 0) - entries.length; + const results = await collectAdvisoryStage(api, "Verify", entries.map(({ lens, ...candidate }) => ({ + id: candidate.candidateId!, candidate, run: async () => verifyCandidate({ lens, candidate }), + })), coverage); + return results; + }; + let verified: Verified[]; 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 }; + const found = await collectAdvisoryStage(api, "Find", lenses.map((lens) => ({ id: lens.label, run: () => findForLens(lens) })), coverage); + verified = await verify(found); + } else { + // Each lens can start verification immediately; cross-lens merging waits for synthesis. + const results = await collectAdvisoryStage(api, "Lenses", lenses.map((lens) => ({ id: lens.label, run: async () => { + const found = await collectAdvisoryStage(api, "Find", [{ id: lens.label, run: () => findForLens(lens) }], coverage); + return verify(found); + } })), coverage); + verified = results.flat(); } - - 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); - }, - ); - - return { verified: compactResults(perLensVerified).flat(), rawCandidates, dropped, refuted }; -} - -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; - }); + return { verified, rawCandidates, dropped, refuted, coverage }; } export function primaryLocation(candidate: Pick): AdvisoryLocation { @@ -201,15 +187,15 @@ export function normalizePath(path: string): string { } 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()}`; + return candidateDedupKey(candidate); } export function verdictLane(verdict: AdvisoryVerdict["verdict"]): string { switch (verdict) { case "CONFIRMED": return "Confirmed"; + case "NOT_SUBSTANTIATED": + return "Unresolved"; case "PLAUSIBLE": return "Plausible"; case "REFUTED": @@ -221,6 +207,7 @@ export function verdictStatus(verdict: AdvisoryVerdict["verdict"]): "success" | switch (verdict) { case "CONFIRMED": return "success"; + case "NOT_SUBSTANTIATED": case "PLAUSIBLE": return "warning"; case "REFUTED": @@ -232,6 +219,7 @@ export function verdictConfidence(verdict: AdvisoryVerdict["verdict"]): "high" | switch (verdict) { case "CONFIRMED": return "high"; + case "NOT_SUBSTANTIATED": case "PLAUSIBLE": return "medium"; case "REFUTED": @@ -270,23 +258,55 @@ export function recordVerdictProgress( } 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) => candidateIds(source).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(candidateIds))]; + 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: candidateIds(finding), 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/workflows/code-review.ts b/.pi/extensions/pi-workflow-engine/workflows/code-review.ts index 425de94..4bcbebd 100644 --- a/.pi/extensions/pi-workflow-engine/workflows/code-review.ts +++ b/.pi/extensions/pi-workflow-engine/workflows/code-review.ts @@ -1,13 +1,16 @@ +import { challengeFindings, parseChallengeArgs } from "../src/advisory-challenge.ts"; +import { AdvisorySynthesisSchema, SYNTHESIS_ID_INSTRUCTIONS, collectAdvisoryStage, identifyCandidates, withAdvisoryCoverage, type AdvisoryStageCoverage } from "../src/advisory-evidence.ts"; import { Type } from "typebox"; import { AdvisoryCandidatesSchema, - AdvisoryReportSchema, AdvisoryVerdictSchema, + type AdvisoryReport, type AdvisoryCandidate, type AdvisoryVerdict, } from "../src/advisory-schema.ts"; import { - backfillAdvisoryFindings, + type AdvisoryVerified, + resolveAdvisorySynthesis, formatEvidence, formatLocation, normalizePath, @@ -18,7 +21,6 @@ import { 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 type { ReviewContext } from "../src/review/review-report.ts"; @@ -28,7 +30,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) ─── @@ -106,11 +108,18 @@ 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; + const coverage: AdvisoryStageCoverage[] = []; + let evidenceRecords: AdvisoryVerified[] = []; + const finish = (report: T) => ({ + ...withAdvisoryCoverage(report, coverage), + verification: evidenceRecords, + }); const makeStats = (verified: number, kept: number): WorkflowRunStats => ({ files: fileCount, candidates: rawCandidateCount, @@ -184,7 +193,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, @@ -195,22 +204,26 @@ export default async function run(api: WorkflowApi, dependencies: CodeReviewDepe // ─── Find barrier → dedup → Verify ─── phase("Find"); - const perAngle = await parallel( - ANGLES.map((angle) => async () => { + const perAngle = await collectAdvisoryStage(api, "Find", + ANGLES.map((angle) => ({ id: angle.label, run: 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. " + + "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 "${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); + if (!found) throw new Error("Finder produced no output"); + const raw = identifyCandidates(found.candidates.slice(0, PER_ANGLE), angle.label); 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 bounded = raw.flatMap((candidate) => { + const anchor = candidate.reviewAnchor ?? candidate.locations.find((location) => inDiff(changed, location.file, location.line)); + return anchor && inDiff(changed, anchor.file, anchor.line) ? [{ ...candidate, reviewAnchor: anchor }] : []; + }); const dropped = raw.length - bounded.length; if (dropped > 0) { droppedCandidateCount += dropped; @@ -228,22 +241,22 @@ export default async function run(api: WorkflowApi, dependencies: CodeReviewDepe }); } return { angle, candidates: bounded }; - }), + } })), coverage, ); // Dedup after all finders complete so verifier agents cannot consume the global cap before full candidate discovery. - const novel = dedupeCodeReviewCandidates(compactResults(perAngle)); + const novel = dedupeCodeReviewCandidates(perAngle); phase("Verify"); - const verdicts = await parallel( - novel.map(({ angle, candidate }) => async (): Promise => { + const verdicts = await collectAdvisoryStage(api, "Verify", + novel.map(({ angle, candidate }) => ({ id: candidate.candidateId!, candidate, run: async (): Promise => { const location = primaryLocation(candidate); const judged = await agent( `## Code-review verifier\n\n${scopeBlock}\n## Candidate\n` + - `Location: ${formatLocation(candidate)}\n` + + `Candidate record: ${JSON.stringify(candidate)}\nLocation: ${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.", + "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.", { phase: "Verify", label: `verify:${location.file.split("/").pop() ?? location.file}`, @@ -253,53 +266,52 @@ export default async function run(api: WorkflowApi, dependencies: CodeReviewDepe schema: AdvisoryVerdictSchema, }, ); - if (!judged) return null; + if (!judged) throw new Error("Verifier produced no output"); recordVerdictProgress(progress, candidate, judged); return { ...candidate, verdict: judged.verdict, evidence: judged.evidence, kind: angle.kind }; - }), + } })), coverage, ); - const verified = compactResults(verdicts); + const verified = await challengeFindings(api, verdicts, scopeBlock, challengeConfig.options, coverage); + evidenceRecords = verified; 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 finish({ summary: "No findings survived verification.", findings: [], nextSteps: ["No code-review action is recommended from this workflow run."], stats, reviewContext }); } // ─── Synthesize: rank, merge, report ─── phase("Synthesize"); - const rank = (finding: Verified): number => (finding.kind === "cleanup" ? 2 : 0) + (finding.verdict === "PLAUSIBLE" ? 1 : 0); + const rank = (finding: Verified): number => (finding.kind === "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` + + `### [${index}] IDs: ${finding.sourceCandidateIds?.join(", ")} ${formatLocation(finding)} (${finding.verdict}${finding.kind === "cleanup" ? ", cleanup" : ""})\n` + `Category: ${finding.kind}\nConfidence: ${verdictConfidence(finding.verdict)}\n` + `${finding.summary}\nImpact: ${finding.impact}\nEvidence: ${formatEvidence(finding.evidence)}`, ) .join("\n\n"); - const report = await agent( - `## Synthesis: final code-review report\n\n${ranked.length} findings survived independent verification.\n\n${block}\n\n` + + const [report] = await collectAdvisoryStage(api, "Synthesize", [{ id: "synthesize", run: () => agent( + SYNTHESIS_ID_INSTRUCTIONS + `## 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.", + "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.", { phase: "Synthesize", label: "synthesize", tools: [], profile: "medium", resume: "read-only", - schema: AdvisoryReportSchema, + schema: AdvisorySynthesisSchema, }, - ); + ) }], 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 }; + const resolved = resolveAdvisorySynthesis(report, ranked, { + impact: "Impact not restated by verification.", + recommendation: "Inspect the cited evidence and validate the smallest repair.", + }, coverage); + return finish({ ...resolved, stats: { ...stats, kept: resolved.findings.length }, reviewContext }); } diff --git a/.pi/extensions/pi-workflow-engine/workflows/diagnose.ts b/.pi/extensions/pi-workflow-engine/workflows/diagnose.ts index baff03a..4da8b3d 100644 --- a/.pi/extensions/pi-workflow-engine/workflows/diagnose.ts +++ b/.pi/extensions/pi-workflow-engine/workflows/diagnose.ts @@ -1,14 +1,16 @@ +import { challengeFindings, parseChallengeArgs } from "../src/advisory-challenge.ts"; +import { AdvisorySynthesisSchema, SYNTHESIS_ID_INSTRUCTIONS, collectAdvisoryStage, identifyCandidates, dedupeCandidates, withAdvisoryCoverage, type AdvisoryStageCoverage } from "../src/advisory-evidence.ts"; import { Type } from "typebox"; import { AdvisoryCandidatesSchema, - AdvisoryReportSchema, AdvisoryVerdictSchema, + type AdvisoryReport, type AdvisoryCandidate, type AdvisoryVerdict, } from "../src/advisory-schema.ts"; import { - advisoryDedupKey as dedupKey, - backfillAdvisoryFindings, + type AdvisoryVerified, + resolveAdvisorySynthesis, emptyAdvisoryReport, formatEvidence, formatLocation, @@ -18,13 +20,12 @@ import { 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({ @@ -66,12 +67,19 @@ 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; let refutedCandidateCount = 0; + const coverage: AdvisoryStageCoverage[] = []; + let evidenceRecords: AdvisoryVerified[] = []; + const finish = (report: T) => ({ + ...withAdvisoryCoverage(report, coverage), + verification: evidenceRecords, + }); const makeStats = (verified: number, kept: number): WorkflowRunStats => ({ files: fileCount, candidates: rawCandidateCount, @@ -94,11 +102,11 @@ export default async function run(api: WorkflowApi): Promise { ); if (!scope) { - return emptyAdvisoryReport( + return finish(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; @@ -114,8 +122,8 @@ export default async function run(api: WorkflowApi): Promise { `## Constraints\n${scope.constraints ?? "(none noted)"}\n`; phase("Hypothesize"); - const perLens = await parallel( - HYPOTHESIS_LENSES.map((lens) => async (): Promise => { + const perLens = await collectAdvisoryStage(api, "Find", + HYPOTHESIS_LENSES.map((lens) => ({ id: lens.label, run: 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" + @@ -124,7 +132,8 @@ export default async function run(api: WorkflowApi): Promise { "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 })); + if (!found) throw new Error("Finder produced no output"); + const candidates = identifyCandidates(found.candidates.slice(0, PER_LENS), lens.label).map((candidate) => ({ ...candidate, lens })); rawCandidateCount += candidates.length; progress({ type: "counter_delta", key: "candidates", label: "candidates", delta: candidates.length }); for (const candidate of candidates) { @@ -138,26 +147,25 @@ export default async function run(api: WorkflowApi): Promise { }); } return candidates; - }), + } })), coverage, ); - const hypotheses = dedupe(compactResults(perLens).flat(), (dropped) => { + const hypotheses = dedupe(perLens.flat(), (dropped) => { droppedCandidateCount += dropped; progress({ type: "counter_delta", key: "dropped", label: "dropped", delta: dropped }); }); phase("Verify"); - const verified = compactResults( - await parallel( - hypotheses.map((hypothesis) => async (): Promise => { + const verdicts = await collectAdvisoryStage(api, "Verify", + hypotheses.map((hypothesis) => ({ id: hypothesis.candidateId!, candidate: hypothesis, run: 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` + + `Candidate record: ${JSON.stringify(hypothesis)}\nLocation: ${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.", + "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", label: `verify:${location.file.split("/").pop() ?? location.file}`, @@ -167,26 +175,27 @@ export default async function run(api: WorkflowApi): Promise { schema: AdvisoryVerdictSchema, }, ); - if (!judged) return null; + if (!judged) throw new Error("Verifier produced no output"); recordVerdictProgress(progress, hypothesis, judged, () => { refutedCandidateCount += 1; }); return { ...hypothesis, verdict: judged.verdict, evidence: judged.evidence, confidence: judged.confidence }; - }), - ), + } })), coverage, ); + const verified = await challengeFindings(api, verdicts, scopeBlock, challengeConfig.options, coverage); + evidenceRecords = verified; 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 finish(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, - ); + )); } phase("Synthesize"); @@ -194,7 +203,7 @@ export default async function run(api: WorkflowApi): Promise { 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,11 +212,10 @@ export default async function run(api: WorkflowApi): Promise { .map((finding) => `- ${finding.summary} — REFUTED because ${formatEvidence(finding.evidence)}`) .join("\n"); - const report = await agent( - `## Synthesis: final diagnosis report\n\n${ranked.length} hypotheses survived independent verification.\n\n${block}\n\n` + + const [report] = await collectAdvisoryStage(api, "Synthesize", [{ id: "synthesize", run: () => agent( + SYNTHESIS_ID_INSTRUCTIONS + `## 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", @@ -215,28 +223,19 @@ export default async function run(api: WorkflowApi): Promise { tools: [], profile: "medium", resume: "read-only", - schema: AdvisoryReportSchema, + schema: AdvisorySynthesisSchema, }, - ); - - if (!report) return emptyAdvisoryReport("Synthesis produced no output.", ["Inspect verifier evidence manually or rerun diagnose with a narrower symptom."], stats); + ) }], coverage); - 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 }; + const resolved = resolveAdvisorySynthesis(report, ranked, { + impact: "Impact not restated by verification.", + recommendation: "Inspect the cited evidence and validate the smallest repair.", + }, coverage); + return finish({ ...resolved, stats: { ...stats, kept: resolved.findings.length } }); } 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 novel = dedupeCandidates(candidates); const dropped = candidates.length - novel.length; if (dropped > 0) onDropped(dropped); return novel; diff --git a/.pi/extensions/pi-workflow-engine/workflows/perf-review.ts b/.pi/extensions/pi-workflow-engine/workflows/perf-review.ts index 66a04b7..195a3d4 100644 --- a/.pi/extensions/pi-workflow-engine/workflows/perf-review.ts +++ b/.pi/extensions/pi-workflow-engine/workflows/perf-review.ts @@ -1,11 +1,14 @@ +import { challengeFindings, parseChallengeArgs } from "../src/advisory-challenge.ts"; +import { AdvisorySynthesisSchema, SYNTHESIS_ID_INSTRUCTIONS, collectAdvisoryStage, withAdvisoryCoverage, type AdvisoryStageCoverage } from "../src/advisory-evidence.ts"; import { Type } from "typebox"; import { - AdvisoryReportSchema, + type AdvisoryReport, type AdvisoryCandidate, type AdvisoryVerdict, } from "../src/advisory-schema.ts"; import { - backfillAdvisoryFindings, + type AdvisoryVerified, + resolveAdvisorySynthesis, emptyAdvisoryReport, formatEvidence, formatLocation, @@ -19,7 +22,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({ @@ -60,11 +63,18 @@ 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 challengeConfig = parseChallengeArgs(args); + const target = challengeConfig.args.trim() || "repository performance"; let fileCount = 0; let rawCandidateCount = 0; let droppedCandidateCount = 0; let refutedCandidateCount = 0; + const coverage: AdvisoryStageCoverage[] = []; + let evidenceRecords: AdvisoryVerified[] = []; + const finish = (report: T) => ({ + ...withAdvisoryCoverage(report, coverage), + verification: evidenceRecords, + }); const makeStats = (verified: number, kept: number): WorkflowRunStats => ({ files: fileCount, candidates: rawCandidateCount, @@ -85,11 +95,11 @@ export default async function run(api: WorkflowApi): Promise { ); if (!scope || scope.files.length === 0) { - return emptyAdvisoryReport( + return finish(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; @@ -124,7 +134,7 @@ 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, @@ -137,17 +147,19 @@ export default async function run(api: WorkflowApi): Promise { rawCandidateCount += pipelineResult.rawCandidates; droppedCandidateCount += pipelineResult.dropped; refutedCandidateCount += pipelineResult.refuted; - const verified = pipelineResult.verified; + coverage.push(...pipelineResult.coverage); + const verified = await challengeFindings(api, pipelineResult.verified, scopeBlock, challengeConfig.options, coverage); + evidenceRecords = verified; 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 finish(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, - ); + )); } phase("Synthesize"); @@ -155,14 +167,14 @@ export default async function run(api: WorkflowApi): Promise { 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( - `## 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. " + + const [report] = await collectAdvisoryStage(api, "Synthesize", [{ id: "synthesize", run: () => agent( + SYNTHESIS_ID_INSTRUCTIONS + `## Synthesis: final perf-review report\n\n${ranked.length} candidates survived independent verification.\n\n${block}\n\n` + + "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.", { @@ -171,17 +183,15 @@ export default async function run(api: WorkflowApi): Promise { tools: [], profile: "medium", resume: "read-only", - schema: AdvisoryReportSchema, + schema: AdvisorySynthesisSchema, }, - ); + ) }], 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 }; + const resolved = resolveAdvisorySynthesis(report, ranked, { + impact: "Impact not restated by verification.", + recommendation: "Inspect the cited evidence and validate the smallest repair.", + }, coverage); + return finish({ ...resolved, stats: { ...stats, kept: resolved.findings.length } }); } function rank(finding: Verified): number { diff --git a/.pi/extensions/pi-workflow-engine/workflows/refactor-scout.ts b/.pi/extensions/pi-workflow-engine/workflows/refactor-scout.ts index 5eb4a1f..dedd3a2 100644 --- a/.pi/extensions/pi-workflow-engine/workflows/refactor-scout.ts +++ b/.pi/extensions/pi-workflow-engine/workflows/refactor-scout.ts @@ -1,11 +1,14 @@ +import { challengeFindings, parseChallengeArgs } from "../src/advisory-challenge.ts"; +import { AdvisorySynthesisSchema, SYNTHESIS_ID_INSTRUCTIONS, collectAdvisoryStage, withAdvisoryCoverage, type AdvisoryStageCoverage } from "../src/advisory-evidence.ts"; import { Type } from "typebox"; import { - AdvisoryReportSchema, + type AdvisoryReport, type AdvisoryCandidate, type AdvisoryVerdict, } from "../src/advisory-schema.ts"; import { - backfillAdvisoryFindings, + type AdvisoryVerified, + resolveAdvisorySynthesis, emptyAdvisoryReport, formatEvidence, formatLocation, @@ -19,7 +22,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({ @@ -59,11 +62,18 @@ 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 challengeConfig = parseChallengeArgs(args); + const target = challengeConfig.args.trim() || "."; let fileCount = 0; let rawCandidateCount = 0; let droppedCandidateCount = 0; let refutedCandidateCount = 0; + const coverage: AdvisoryStageCoverage[] = []; + let evidenceRecords: AdvisoryVerified[] = []; + const finish = (report: T) => ({ + ...withAdvisoryCoverage(report, coverage), + verification: evidenceRecords, + }); const makeStats = (verified: number, kept: number): WorkflowRunStats => ({ files: fileCount, candidates: rawCandidateCount, @@ -84,11 +94,11 @@ export default async function run(api: WorkflowApi): Promise { ); if (!scope || scope.files.length === 0) { - return emptyAdvisoryReport( + return finish(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; @@ -120,7 +130,7 @@ 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 => ({ @@ -134,13 +144,15 @@ export default async function run(api: WorkflowApi): Promise { rawCandidateCount += pipelineResult.rawCandidates; droppedCandidateCount += pipelineResult.dropped; refutedCandidateCount += pipelineResult.refuted; - const verified = pipelineResult.verified; + coverage.push(...pipelineResult.coverage); + const verified = await challengeFindings(api, pipelineResult.verified, scopeBlock, challengeConfig.options, coverage); + evidenceRecords = verified; 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 finish(emptyAdvisoryReport("No refactor opportunities survived verification.", ["Leave the scoped code unchanged unless a human reviewer has additional context."], stats)); } phase("Synthesize"); @@ -148,16 +160,16 @@ export default async function run(api: WorkflowApi): Promise { 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( - `## Synthesis: final refactor-scout report\n\n${ranked.length} opportunities survived independent verification.\n\n${block}\n\n` + + const [report] = await collectAdvisoryStage(api, "Synthesize", [{ id: "synthesize", run: () => agent( + SYNTHESIS_ID_INSTRUCTIONS + `## 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", @@ -165,17 +177,15 @@ export default async function run(api: WorkflowApi): Promise { tools: [], profile: "medium", resume: "read-only", - schema: AdvisoryReportSchema, + schema: AdvisorySynthesisSchema, }, - ); + ) }], 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 }; + const resolved = resolveAdvisorySynthesis(report, ranked, { + impact: "Impact not restated by verification.", + recommendation: "Inspect the cited evidence and validate the smallest repair.", + }, coverage); + return finish({ ...resolved, stats: { ...stats, kept: resolved.findings.length } }); } function rank(finding: Verified): number { diff --git a/USAGE.md b/USAGE.md index 82684ae..a800d59 100644 --- a/USAGE.md +++ b/USAGE.md @@ -543,3 +543,59 @@ 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` sets the limit, capped at ten; zero disables +it. 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. + +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..4441018 --- /dev/null +++ b/tests/advisory-provenance.test.ts @@ -0,0 +1,117 @@ +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); +}); diff --git a/tests/advisory-utils.test.ts b/tests/advisory-utils.test.ts index 6e90cf8..336eca2 100644 --- a/tests/advisory-utils.test.ts +++ b/tests/advisory-utils.test.ts @@ -62,27 +62,30 @@ test("findingLocationKey normalizes diff prefixes", () => { assert.equal(sameFinding({ locations: [{ file: "b/src/app.ts", line: 10 }] }, finding("src/app.ts", 10)), true); }); -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 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" }; + 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 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" }, - ); +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" }), []); +}); - 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..f41be8b 100644 --- a/tests/changedlines.test.ts +++ b/tests/changedlines.test.ts @@ -66,6 +66,7 @@ test("code-review candidate deduplication retains first discovery order", () => { angle: "edge", candidates: [duplicate, distinct] }, ]), [ { angle: "logic", candidate: first }, + { angle: "edge", candidate: duplicate }, { angle: "edge", candidate: distinct }, ]); }); 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..e2778a5 --- /dev/null +++ b/tests/patch-validation.test.ts @@ -0,0 +1,143 @@ +import assert from "node:assert/strict"; +import { test } from "bun:test"; +import { mkdtemp, readFile, writeFile, rm } from "node:fs/promises"; +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("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"); + await writeFile(join(workspace.cwd, ".untracked-implementer"), "hidden contamination"); + // Remove a scratch file before capture: evaluator must never inherit it. + await rm(join(workspace.cwd, ".untracked-implementer")); + 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]); + await assert.rejects(readFile(join(workspace.cwd, ".untracked-implementer")), /ENOENT/); + 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() {}, 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() {}, cwd: process.cwd() }, issues, + { workflowName: "code-review", target: "", files: [], diffTarget: { kind: "git", args: [] }, snapshot: { baselineFingerprint: fingerprintReviewWorktreeBaseline(baseline), diffFingerprint: "a".repeat(64) } }, baseline), /evaluator cancelled/); +}); diff --git a/tests/review-actions.test.ts b/tests/review-actions.test.ts index 845fe5b..06f8968 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"; @@ -175,7 +176,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 +189,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(undefined, undefined, "diff --git a/src/app.ts b/src/app.ts\n+fixed\n"), }); 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\./); +}); From 87da35364d85bfc06b5c9810aeab3d0f46da6407 Mon Sep 17 00:00:00 2001 From: timbrinded <79199034+timbrinded@users.noreply.github.com> Date: Wed, 16 Sep 2026 20:01:56 +0100 Subject: [PATCH 2/5] refactor: consolidate advisory stages and repair evaluation --- .../src/advisory-challenge.ts | 18 +- .../src/advisory-evidence.ts | 16 +- .../pi-workflow-engine/src/advisory-schema.ts | 3 +- .../pi-workflow-engine/src/agent-workspace.ts | 9 +- .../src/review/code-review-orchestration.ts | 10 - .../src/review/patch-validation.ts | 35 ++-- .../src/review/review-fix-workflow.ts | 59 +++--- .../src/workflow-advisory-utils.ts | 190 +++++++----------- .../pi-workflow-engine/src/worktree.ts | 7 + .../workflows/code-review.ts | 179 +++++------------ .../pi-workflow-engine/workflows/diagnose.ts | 169 ++++------------ .../workflows/perf-review.ts | 84 ++------ .../workflows/refactor-scout.ts | 82 ++------ tests/advisory-scheduling.test.ts | 44 ++-- tests/advisory-utils.test.ts | 4 +- tests/changedlines.test.ts | 15 +- tests/patch-validation.test.ts | 21 +- tests/review-actions.test.ts | 7 +- 18 files changed, 341 insertions(+), 611 deletions(-) diff --git a/.pi/extensions/pi-workflow-engine/src/advisory-challenge.ts b/.pi/extensions/pi-workflow-engine/src/advisory-challenge.ts index 7884ae7..9bb3710 100644 --- a/.pi/extensions/pi-workflow-engine/src/advisory-challenge.ts +++ b/.pi/extensions/pi-workflow-engine/src/advisory-challenge.ts @@ -13,11 +13,9 @@ const AdjudicationSchema = Type.Object({ evidence: Type.Array(Type.String()), reason: Type.String(), }); -export interface ChallengeRecord { - challenge?: Static; - adjudication?: Static; - status: "complete" | "failed"; -} +export type ChallengeRecord = + | { status: "complete"; challenge: Static; adjudication: Static } + | { status: "failed"; challenge?: Static }; export interface AdvisoryChallengeOptions { maxChallenges: number; shouldChallenge?: (finding: AdvisoryVerified) => boolean; @@ -50,19 +48,19 @@ export async function challengeFindings( 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 () => { + 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" } }); + 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 } }); + 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!, { + 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], @@ -70,5 +68,5 @@ export async function challengeFindings( }); return finding.candidateId; } })), coverage); - return findings.map((finding) => replacements.get(finding.candidateId!) ?? finding); + 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 index 87d54e5..cfd514d 100644 --- a/.pi/extensions/pi-workflow-engine/src/advisory-evidence.ts +++ b/.pi/extensions/pi-workflow-engine/src/advisory-evidence.ts @@ -1,6 +1,6 @@ import { createHash } from "node:crypto"; import { Type, type Static } from "typebox"; -import type { AdvisoryCandidate, AdvisoryReport } from "./advisory-schema.ts"; +import { AdvisorySeveritySchema, type IdentifiedAdvisoryCandidate, type AdvisoryCandidate, type AdvisoryReport } from "./advisory-schema.ts"; import type { WorkflowApi } from "./types.ts"; export interface AdvisoryStageCoverage { @@ -24,7 +24,7 @@ export async function collectAdvisoryStage( return results.flatMap((result) => result.ok ? [result.value] : []); } -export function identifyCandidates(candidates: readonly AdvisoryCandidate[], lens: string): AdvisoryCandidate[] { +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. @@ -36,21 +36,21 @@ function normalizedSummary(summary: string): string { return summary.toLowerCase().replace(/\s+/g, " ").trim().replace(/[.!?]+$/, ""); } -export function candidateDedupKey(candidate: AdvisoryCandidate): string { +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[] { +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([...candidateIds(previous), ...candidateIds(candidate)])]; + 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 ?? [])])]; @@ -59,10 +59,6 @@ export function dedupeCandidates(candidates: readon return [...seen.values()]; } -export function candidateIds(candidate: AdvisoryCandidate): string[] { - return candidate.sourceCandidateIds ?? (candidate.candidateId ? [candidate.candidateId] : []); -} - export function uniqueLocations(locations: AdvisoryCandidate["locations"]): AdvisoryCandidate["locations"] { return [...new Map(locations.map((location) => [JSON.stringify(location), location])).values()]; } @@ -72,7 +68,7 @@ export const AdvisorySynthesisSchema = Type.Object({ summary: Type.String(), findings: Type.Array(Type.Object({ sourceCandidateIds: Type.Array(Type.String(), { minItems: 1 }), - severity: Type.Union([Type.Literal("low"), Type.Literal("medium"), Type.Literal("high")]), + severity: AdvisorySeveritySchema, recommendation: Type.String(), })), nextSteps: Type.Array(Type.String()), diff --git a/.pi/extensions/pi-workflow-engine/src/advisory-schema.ts b/.pi/extensions/pi-workflow-engine/src/advisory-schema.ts index 233f059..a5d52b1 100644 --- a/.pi/extensions/pi-workflow-engine/src/advisory-schema.ts +++ b/.pi/extensions/pi-workflow-engine/src/advisory-schema.ts @@ -20,8 +20,6 @@ export const AdvisoryLocationSchema = Type.Object({ }); export const AdvisoryCandidateSchema = Type.Object({ - candidateId: Type.Optional(Type.String()), - sourceCandidateIds: Type.Optional(Type.Array(Type.String())), discoveryEvidence: Type.Optional(Type.Array(Type.String())), reviewAnchor: Type.Optional(AdvisoryLocationSchema), summary: Type.String({ description: "One-line candidate finding or hypothesis." }), @@ -74,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 055a15e..4cc1028 100644 --- a/.pi/extensions/pi-workflow-engine/src/agent-workspace.ts +++ b/.pi/extensions/pi-workflow-engine/src/agent-workspace.ts @@ -1,6 +1,6 @@ import type { AgentExecutionOptions, AgentProgress } from "./agent-runner-types.ts"; import { unknownErrorMessage } from "./unknown-error.ts"; -import { spawnGitRunner, type WorktreeRegistry } from "./worktree.ts"; +import type { WorktreeRegistry } from "./worktree.ts"; interface AgentWorkspaceBase { readonly cwd: string; @@ -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; } @@ -58,10 +58,7 @@ export async function createAgentWorkspace( try { if (opts.candidatePatch.baselineOid !== added.baselineOid) throw new Error("Candidate baseline differs from evaluator baseline"); if (opts.candidatePatch.patch.trim()) { - const applied = await spawnGitRunner.runGit({ - cwd: worktreePath, args: ["apply", "--binary", "--index", "-"], - stdin: opts.candidatePatch.patch, signal: rc.signal, timeoutMs: 30_000, - }); + 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) { 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 4b233ea..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 { dedupeCandidates } from "../advisory-evidence.ts"; - const DIFF_EMBED_CAP = 60_000; export function buildCodeReviewScopeBlock(input: { @@ -25,10 +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 }> { - return dedupeCandidates(groups.flatMap(({ angle, candidates }) => candidates.map((candidate) => ({ ...candidate, angle })))) - .map(({ angle, ...candidate }) => ({ angle, candidate })); -} diff --git a/.pi/extensions/pi-workflow-engine/src/review/patch-validation.ts b/.pi/extensions/pi-workflow-engine/src/review/patch-validation.ts index 258c295..2102b1e 100644 --- a/.pi/extensions/pi-workflow-engine/src/review/patch-validation.ts +++ b/.pi/extensions/pi-workflow-engine/src/review/patch-validation.ts @@ -2,7 +2,7 @@ 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, spawnGitRunner, type WorktreeBaseline } from "../worktree.ts"; +import { WorktreeRegistry, type WorktreeBaseline } from "../worktree.ts"; import { unknownErrorMessage } from "../unknown-error.ts"; import { fingerprintReviewWorktreeBaseline } from "./review-snapshot.ts"; @@ -30,8 +30,8 @@ export interface PatchValidation { evaluation?: PatchEvaluation; reason: string; } -export function initialPatchValidation(baseline: WorktreeBaseline | undefined, baselineOid: string | undefined, patch: string): PatchValidation { - return { status: patch.trim() ? "blocked" : "no-patch", baselineFingerprint: baseline ? fingerprintReviewWorktreeBaseline(baseline) : "unavailable", ...(baselineOid ? { baselineOid } : {}), +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." }; } @@ -49,35 +49,40 @@ export async function validateCandidatePatch(options: { 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 spawnGitRunner.runGit({ cwd: candidate.path, args: ["apply", "--index", "--binary", "-"], stdin: options.patch, timeoutMs: 30_000, signal: options.signal }); + const applied = await worktrees.applyPatch(candidate.path, options.patch, options.signal); if (!applied.ok) return { ...validation, status: "rejected", reason: applied.error ?? applied.stderr }; - let rejected = options.evaluation.outcome === "rejected"; - let blocked = options.evaluation.outcome === "blocked" || !options.evaluation.checks.some((check) => check.required); + 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 = true; - else if (check.required) blocked = true; + 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 = true; continue; } - const testApplied = await spawnGitRunner.runGit({ cwd: original.path, args: ["apply", "--index", "--binary", "-"], stdin: check.regression.baselinePatch, timeoutMs: 30_000, signal: options.signal }); - if (!testApplied.ok) { blocked = true; validation.reason = `Baseline regression setup failed: ${testApplied.error ?? testApplied.stderr}`; continue; } + 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 = true; + !(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 || after.patch !== options.patch) blocked = true; - return { ...validation, status: rejected ? "rejected" : blocked ? "blocked" : "verified", - reason: rejected ? "Independent evaluation or an executed check rejected the candidate." : blocked ? "Required validation could not be completed against the unchanged candidate." : "Independent evaluation and all required checks passed." }; + 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." }; } catch (error) { throwIfAborted(options.signal); return { ...validation, status: "blocked", reason: unknownErrorMessage(error) }; 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 4654f81..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,9 +1,8 @@ import { initialPatchValidation, PatchEvaluationSchema, validateCandidatePatch, type PatchValidation } from "./patch-validation.ts"; -import { fingerprintReviewWorktreeBaseline } from "./review-snapshot.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"; @@ -33,7 +32,7 @@ export interface ReviewFixWorkflowResult { readonly fixes: readonly ReviewFixOutcome[]; } -export type ReviewFixWorkflowApi = Pick & Partial>; +export type ReviewFixWorkflowApi = Pick; /** Build an ephemeral workflow that generates one isolated patch preview per finding. */ export function createReviewFixWorkflow( @@ -81,7 +80,7 @@ export async function runReviewFixWorkflow( api: ReviewFixWorkflowApi, issues: readonly ReviewIssue[], context: ReviewContext | undefined, - baseline?: WorktreeBaseline, + baseline: WorktreeBaseline, ): Promise { api.phase(REVIEW_FIX_PHASE); const settled = await api.parallel( @@ -95,28 +94,7 @@ export async function runReviewFixWorkflow( tools: [...REVIEW_FIX_TOOLS], toolHints: ["search"], }); - let validation = initialPatchValidation(baseline, isolated.baselineOid, isolated.patch); - if (isolated.patch.trim() && baseline && isolated.baselineOid && api.cwd && context?.snapshot) { - if (fingerprintReviewWorktreeBaseline(baseline) !== context.snapshot.baselineFingerprint) { - validation = { ...validation, status: "rejected", reason: "Stale reviewed baseline identity." }; - } else 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) { - validation = { ...validation, status: "rejected", reason: "Evaluator changed the candidate or used a different baseline.", evaluation: evaluated.result }; - } else { - validation = 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; - validation = { ...validation, status: "blocked", reason: unknownErrorMessage(error) }; - } - } + const validation = await evaluateReviewFix(api, { issue, context, baseline, isolated }); return { findingId: issue.id, result: isolated.result, @@ -144,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/workflow-advisory-utils.ts b/.pi/extensions/pi-workflow-engine/src/workflow-advisory-utils.ts index 04fe4a8..799f2e2 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 { candidateDedupKey, candidateIds, collectAdvisoryStage, dedupeCandidates, identifyCandidates, uniqueLocations, type AdvisoryStageCoverage, type AdvisorySynthesis } from "./advisory-evidence.ts"; +import { AdvisoryCandidatesSchema, AdvisoryVerdictSchema, type AdvisoryCandidate, type IdentifiedAdvisoryCandidate, type AdvisoryFinding, 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,15 +8,15 @@ 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; @@ -49,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 { @@ -70,103 +64,79 @@ 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; +/** 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, }); - if (!found) throw new Error("Finder produced no output"); - const candidates = identifyCandidates(found.candidates.slice(0, perLens), lens.label); - 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 verifyCandidate = async ({ lens, candidate }: NovelCandidate): Promise => { - const location = primaryLocation(candidate); - const judged = await api.agent(`${verifierPrompt(candidate)}\nCandidate record: ${JSON.stringify(candidate)}`, { - phase: verifierPhase, - label: `verify:${location.file.split("/").pop() ?? location.file}`, - tools, - toolHints, - profile: "small", - schema: AdvisoryVerdictSchema, - }); - if (!judged) throw new Error("Verifier produced no output"); - recordVerdictProgress(api.progress, candidate, judged, () => { - refuted += 1; - }); - return makeVerified(candidate, lens, judged); - }; - - const verify = async (found: FoundForLens[]): Promise => { - const entries = dedupeCandidates(found.flatMap(({ lens, candidates }) => candidates.map((candidate) => ({ ...candidate, lens })))); - dropped += found.reduce((count, group) => count + group.candidates.length, 0) - entries.length; - const results = await collectAdvisoryStage(api, "Verify", entries.map(({ lens, ...candidate }) => ({ - id: candidate.candidateId!, candidate, run: async () => verifyCandidate({ lens, candidate }), - })), coverage); - return results; - }; - let verified: Verified[]; - if (schedulingMode === "finder-barrier") { - const found = await collectAdvisoryStage(api, "Find", lenses.map((lens) => ({ id: lens.label, run: () => findForLens(lens) })), coverage); - verified = await verify(found); - } else { - // Each lens can start verification immediately; cross-lens merging waits for synthesis. - const results = await collectAdvisoryStage(api, "Lenses", lenses.map((lens) => ({ id: lens.label, run: async () => { - const found = await collectAdvisoryStage(api, "Find", [{ id: lens.label, run: () => findForLens(lens) }], coverage); - return verify(found); - } })), coverage); - verified = results.flat(); + 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)`); } + 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 }; } +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); +} + +export function finishAdvisoryReport( + report: T, coverage: AdvisoryStageCoverage[], verification: AdvisoryVerified[] = [], +) { + return { ...withAdvisoryCoverage(report, coverage), verification }; +} + export function primaryLocation(candidate: Pick): AdvisoryLocation { return candidate.locations[0] ?? { file: "" }; } @@ -186,10 +156,6 @@ export function normalizePath(path: string): string { return path.replace(/^\.\//, "").replace(/^[ab]\//, ""); } -export function advisoryDedupKey(candidate: AdvisoryCandidate): string { - return candidateDedupKey(candidate); -} - export function verdictLane(verdict: AdvisoryVerdict["verdict"]): string { switch (verdict) { case "CONFIRMED": @@ -262,14 +228,14 @@ export function backfillAdvisoryFindings( ranked: readonly Source[], defaults: AdvisoryBackfillDefaults, ): AdvisoryReport["findings"] { - const sources = new Map(ranked.flatMap((source) => candidateIds(source).map((id) => [id, source] as const))); + 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(candidateIds))]; + 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]!; @@ -299,7 +265,7 @@ export function resolveAdvisorySynthesis( if (!report) { return { summary: "Synthesis unavailable; verified records retained.", - findings: backfillAdvisoryFindings(ranked.map((finding) => ({ sourceCandidateIds: candidateIds(finding), severity: "medium", recommendation: finding.recommendation ?? defaults.recommendation ?? "Inspect verifier evidence." })), ranked, defaults), + 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."], }; } 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 4bcbebd..3281785 100644 --- a/.pi/extensions/pi-workflow-engine/workflows/code-review.ts +++ b/.pi/extensions/pi-workflow-engine/workflows/code-review.ts @@ -1,28 +1,22 @@ import { challengeFindings, parseChallengeArgs } from "../src/advisory-challenge.ts"; -import { AdvisorySynthesisSchema, SYNTHESIS_ID_INSTRUCTIONS, collectAdvisoryStage, identifyCandidates, withAdvisoryCoverage, type AdvisoryStageCoverage } from "../src/advisory-evidence.ts"; +import { type AdvisoryStageCoverage } from "../src/advisory-evidence.ts"; import { Type } from "typebox"; -import { - AdvisoryCandidatesSchema, - AdvisoryVerdictSchema, - type AdvisoryReport, - type AdvisoryCandidate, - type AdvisoryVerdict, -} from "../src/advisory-schema.ts"; import { type AdvisoryVerified, - resolveAdvisorySynthesis, + 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 { 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"; @@ -41,26 +35,13 @@ 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; @@ -115,11 +96,6 @@ export default async function run(api: WorkflowApi, dependencies: CodeReviewDepe let rawCandidateCount = 0; let droppedCandidateCount = 0; const coverage: AdvisoryStageCoverage[] = []; - let evidenceRecords: AdvisoryVerified[] = []; - const finish = (report: T) => ({ - ...withAdvisoryCoverage(report, coverage), - verification: evidenceRecords, - }); const makeStats = (verified: number, kept: number): WorkflowRunStats => ({ files: fileCount, candidates: rawCandidateCount, @@ -202,116 +178,59 @@ export default async function run(api: WorkflowApi, dependencies: CodeReviewDepe target, }); - // ─── Find barrier → dedup → Verify ─── - phase("Find"); - const perAngle = await collectAdvisoryStage(api, "Find", - ANGLES.map((angle) => ({ id: angle.label, run: 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). " + - "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 "${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 }, - ); - if (!found) throw new Error("Finder produced no output"); - const raw = identifyCandidates(found.candidates.slice(0, PER_ANGLE), angle.label); - rawCandidateCount += raw.length; - progress({ type: "counter_delta", key: "candidates", label: "candidates", delta: raw.length }); - const bounded = raw.flatMap((candidate) => { - const anchor = candidate.reviewAnchor ?? candidate.locations.find((location) => inDiff(changed, location.file, location.line)); - return anchor && inDiff(changed, anchor.file, anchor.line) ? [{ ...candidate, reviewAnchor: anchor }] : []; - }); - 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 }; - } })), coverage, - ); - - // Dedup after all finders complete so verifier agents cannot consume the global cap before full candidate discovery. - const novel = dedupeCodeReviewCandidates(perAngle); - - phase("Verify"); - const verdicts = await collectAdvisoryStage(api, "Verify", - novel.map(({ angle, candidate }) => ({ id: candidate.candidateId!, candidate, run: async (): Promise => { - const location = primaryLocation(candidate); - const judged = await agent( - `## Code-review verifier\n\n${scopeBlock}\n## Candidate\n` + - `Candidate record: ${JSON.stringify(candidate)}\nLocation: ${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.", - { - phase: "Verify", - label: `verify:${location.file.split("/").pop() ?? location.file}`, - tools: TOOLS, - toolHints: TOOL_HINTS, - profile: "small", - schema: AdvisoryVerdictSchema, - }, - ); - if (!judged) throw new Error("Verifier produced no output"); - recordVerdictProgress(progress, candidate, judged); - return { ...candidate, verdict: judged.verdict, evidence: judged.evidence, kind: angle.kind }; - } })), coverage, - ); - - const verified = await challengeFindings(api, verdicts, scopeBlock, challengeConfig.options, coverage); - evidenceRecords = verified; + 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; + coverage.push(...pipelineResult.coverage); + 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 finish({ 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 !== "CONFIRMED" ? 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}] IDs: ${finding.sourceCandidateIds?.join(", ")} ${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 collectAdvisoryStage(api, "Synthesize", [{ id: "synthesize", run: () => agent( - SYNTHESIS_ID_INSTRUCTIONS + `## Synthesis: final code-review report\n\n${ranked.length} findings survived independent verification.\n\n${block}\n\n` + + 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, ID selections with severity (low/medium/high) and advisory recommendation, and nextSteps. Evidence and confidence are reconstructed from verified records. Structured output only.", - { - phase: "Synthesize", - label: "synthesize", - tools: [], - profile: "medium", - resume: "read-only", - schema: AdvisorySynthesisSchema, - }, - ) }], coverage); - - const resolved = resolveAdvisorySynthesis(report, ranked, { - impact: "Impact not restated by verification.", - recommendation: "Inspect the cited evidence and validate the smallest repair.", - }, coverage); - return finish({ ...resolved, stats: { ...stats, kept: resolved.findings.length }, reviewContext }); + ranked, coverage, + ); + 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 4da8b3d..4385dfb 100644 --- a/.pi/extensions/pi-workflow-engine/workflows/diagnose.ts +++ b/.pi/extensions/pi-workflow-engine/workflows/diagnose.ts @@ -1,22 +1,16 @@ import { challengeFindings, parseChallengeArgs } from "../src/advisory-challenge.ts"; -import { AdvisorySynthesisSchema, SYNTHESIS_ID_INSTRUCTIONS, collectAdvisoryStage, identifyCandidates, dedupeCandidates, withAdvisoryCoverage, type AdvisoryStageCoverage } from "../src/advisory-evidence.ts"; +import { type AdvisoryStageCoverage } from "../src/advisory-evidence.ts"; import { Type } from "typebox"; -import { - AdvisoryCandidatesSchema, - AdvisoryVerdictSchema, - type AdvisoryReport, - type AdvisoryCandidate, - type AdvisoryVerdict, -} from "../src/advisory-schema.ts"; import { type AdvisoryVerified, - resolveAdvisorySynthesis, + type AdvisoryLens, + synthesizeAdvisoryReport, + finishAdvisoryReport, emptyAdvisoryReport, formatEvidence, formatLocation, - primaryLocation, publishVerifiedKeptProgress, - recordVerdictProgress, + runLensVerificationPipeline, DEFAULT_ADVISORY_TOOL_HINTS, DEFAULT_ADVISORY_TOOLS, } from "../src/workflow-advisory-utils.ts"; @@ -36,25 +30,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." }, @@ -75,11 +51,6 @@ export default async function run(api: WorkflowApi): Promise { let droppedCandidateCount = 0; let refutedCandidateCount = 0; const coverage: AdvisoryStageCoverage[] = []; - let evidenceRecords: AdvisoryVerified[] = []; - const finish = (report: T) => ({ - ...withAdvisoryCoverage(report, coverage), - verification: evidenceRecords, - }); const makeStats = (verified: number, kept: number): WorkflowRunStats => ({ files: fileCount, candidates: rawCandidateCount, @@ -102,11 +73,11 @@ export default async function run(api: WorkflowApi): Promise { ); if (!scope) { - return finish(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), - )); + ), coverage); } fileCount = scope.files.length; @@ -121,89 +92,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 collectAdvisoryStage(api, "Find", - HYPOTHESIS_LENSES.map((lens) => ({ id: lens.label, run: 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 }, - ); - if (!found) throw new Error("Finder produced no output"); - const candidates = identifyCandidates(found.candidates.slice(0, PER_LENS), lens.label).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; - } })), coverage, - ); - - const hypotheses = dedupe(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 verdicts = await collectAdvisoryStage(api, "Verify", - hypotheses.map((hypothesis) => ({ id: hypothesis.candidateId!, candidate: hypothesis, run: async (): Promise => { - const location = primaryLocation(hypothesis); - const judged = await agent( - `## Diagnose verifier\n\n${scopeBlock}\n## Hypothesis\n` + - `Candidate record: ${JSON.stringify(hypothesis)}\nLocation: ${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, NOT_SUBSTANTIATED, or REFUTED with evidence. " + - "Use NOT_SUBSTANTIATED when evidence is missing; REFUTED requires disproof. Structured output only.", - { - phase: "Verify", - label: `verify:${location.file.split("/").pop() ?? location.file}`, - tools: TOOLS, - toolHints: TOOL_HINTS, - profile: "small", - schema: AdvisoryVerdictSchema, - }, - ); - if (!judged) throw new Error("Verifier produced no output"); - recordVerdictProgress(progress, hypothesis, judged, () => { - refutedCandidateCount += 1; - }); - return { ...hypothesis, verdict: judged.verdict, evidence: judged.evidence, confidence: judged.confidence }; - } })), coverage, - ); - - const verified = await challengeFindings(api, verdicts, scopeBlock, challengeConfig.options, coverage); - evidenceRecords = verified; + rawCandidateCount += pipelineResult.rawCandidates; + droppedCandidateCount += pipelineResult.dropped; + refutedCandidateCount += pipelineResult.refuted; + coverage.push(...pipelineResult.coverage); + 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 finish(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}] IDs: ${finding.sourceCandidateIds?.join(", ")} ${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"); @@ -212,36 +142,17 @@ export default async function run(api: WorkflowApi): Promise { .map((finding) => `- ${finding.summary} — REFUTED because ${formatEvidence(finding.evidence)}`) .join("\n"); - const [report] = await collectAdvisoryStage(api, "Synthesize", [{ id: "synthesize", run: () => agent( - SYNTHESIS_ID_INSTRUCTIONS + `## Synthesis: final diagnosis report\n\n${ranked.length} hypotheses survived independent verification.\n\n${block}\n\n` + + 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` + "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: AdvisorySynthesisSchema, - }, - ) }], coverage); - - const resolved = resolveAdvisorySynthesis(report, ranked, { - impact: "Impact not restated by verification.", - recommendation: "Inspect the cited evidence and validate the smallest repair.", - }, coverage); - return finish({ ...resolved, stats: { ...stats, kept: resolved.findings.length } }); -} - -function dedupe(candidates: Hypothesis[], onDropped: (dropped: number) => void): Hypothesis[] { - const novel = dedupeCandidates(candidates); - const dropped = candidates.length - novel.length; - if (dropped > 0) onDropped(dropped); - return novel; + ranked, coverage, + ); + 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 195a3d4..ac053ae 100644 --- a/.pi/extensions/pi-workflow-engine/workflows/perf-review.ts +++ b/.pi/extensions/pi-workflow-engine/workflows/perf-review.ts @@ -1,14 +1,11 @@ import { challengeFindings, parseChallengeArgs } from "../src/advisory-challenge.ts"; -import { AdvisorySynthesisSchema, SYNTHESIS_ID_INSTRUCTIONS, collectAdvisoryStage, withAdvisoryCoverage, type AdvisoryStageCoverage } from "../src/advisory-evidence.ts"; +import { type AdvisoryStageCoverage } from "../src/advisory-evidence.ts"; import { Type } from "typebox"; -import { - type AdvisoryReport, - type AdvisoryCandidate, - type AdvisoryVerdict, -} from "../src/advisory-schema.ts"; import { type AdvisoryVerified, - resolveAdvisorySynthesis, + type AdvisoryLens, + synthesizeAdvisoryReport, + finishAdvisoryReport, emptyAdvisoryReport, formatEvidence, formatLocation, @@ -33,22 +30,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." }, @@ -62,7 +44,7 @@ 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 { agent, phase, log, progress, args } = api; const challengeConfig = parseChallengeArgs(args); const target = challengeConfig.args.trim() || "repository performance"; let fileCount = 0; @@ -70,11 +52,6 @@ export default async function run(api: WorkflowApi): Promise { let droppedCandidateCount = 0; let refutedCandidateCount = 0; const coverage: AdvisoryStageCoverage[] = []; - let evidenceRecords: AdvisoryVerified[] = []; - const finish = (report: T) => ({ - ...withAdvisoryCoverage(report, coverage), - verification: evidenceRecords, - }); const makeStats = (verified: number, kept: number): WorkflowRunStats => ({ files: fileCount, candidates: rawCandidateCount, @@ -95,11 +72,11 @@ export default async function run(api: WorkflowApi): Promise { ); if (!scope || scope.files.length === 0) { - return finish(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), - )); + ), coverage); } fileCount = scope.files.length; @@ -114,14 +91,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" + @@ -136,65 +109,44 @@ export default async function run(api: WorkflowApi): Promise { "Read relevant files and package/scripts. Run only safe read-only measurement or inspection commands when useful. " + "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; coverage.push(...pipelineResult.coverage); const verified = await challengeFindings(api, pipelineResult.verified, scopeBlock, challengeConfig.options, coverage); - evidenceRecords = verified; 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 finish(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}] IDs: ${finding.sourceCandidateIds?.join(", ")} ${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 collectAdvisoryStage(api, "Synthesize", [{ id: "synthesize", run: () => agent( - SYNTHESIS_ID_INSTRUCTIONS + `## Synthesis: final perf-review report\n\n${ranked.length} candidates survived independent verification.\n\n${block}\n\n` + + const resolved = await synthesizeAdvisoryReport(api, + `## Synthesis: final perf-review report\n\n${ranked.length} candidates survived independent verification.\n\n${block}\n\n` + "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: AdvisorySynthesisSchema, - }, - ) }], coverage); - - const resolved = resolveAdvisorySynthesis(report, ranked, { - impact: "Impact not restated by verification.", - recommendation: "Inspect the cited evidence and validate the smallest repair.", - }, coverage); - return finish({ ...resolved, stats: { ...stats, kept: resolved.findings.length } }); + ranked, coverage, + ); + 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 dedd3a2..bcd3c38 100644 --- a/.pi/extensions/pi-workflow-engine/workflows/refactor-scout.ts +++ b/.pi/extensions/pi-workflow-engine/workflows/refactor-scout.ts @@ -1,14 +1,11 @@ import { challengeFindings, parseChallengeArgs } from "../src/advisory-challenge.ts"; -import { AdvisorySynthesisSchema, SYNTHESIS_ID_INSTRUCTIONS, collectAdvisoryStage, withAdvisoryCoverage, type AdvisoryStageCoverage } from "../src/advisory-evidence.ts"; +import { type AdvisoryStageCoverage } from "../src/advisory-evidence.ts"; import { Type } from "typebox"; -import { - type AdvisoryReport, - type AdvisoryCandidate, - type AdvisoryVerdict, -} from "../src/advisory-schema.ts"; import { type AdvisoryVerified, - resolveAdvisorySynthesis, + type AdvisoryLens, + synthesizeAdvisoryReport, + finishAdvisoryReport, emptyAdvisoryReport, formatEvidence, formatLocation, @@ -32,22 +29,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." }, @@ -61,7 +43,7 @@ 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 { agent, phase, log, progress, args } = api; const challengeConfig = parseChallengeArgs(args); const target = challengeConfig.args.trim() || "."; let fileCount = 0; @@ -69,11 +51,6 @@ export default async function run(api: WorkflowApi): Promise { let droppedCandidateCount = 0; let refutedCandidateCount = 0; const coverage: AdvisoryStageCoverage[] = []; - let evidenceRecords: AdvisoryVerified[] = []; - const finish = (report: T) => ({ - ...withAdvisoryCoverage(report, coverage), - verification: evidenceRecords, - }); const makeStats = (verified: number, kept: number): WorkflowRunStats => ({ files: fileCount, candidates: rawCandidateCount, @@ -94,11 +71,11 @@ export default async function run(api: WorkflowApi): Promise { ); if (!scope || scope.files.length === 0) { - return finish(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), - )); + ), coverage); } fileCount = scope.files.length; @@ -111,14 +88,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" + @@ -133,62 +106,41 @@ export default async function run(api: WorkflowApi): Promise { "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; coverage.push(...pipelineResult.coverage); const verified = await challengeFindings(api, pipelineResult.verified, scopeBlock, challengeConfig.options, coverage); - evidenceRecords = verified; 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 finish(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}] IDs: ${finding.sourceCandidateIds?.join(", ")} ${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 collectAdvisoryStage(api, "Synthesize", [{ id: "synthesize", run: () => agent( - SYNTHESIS_ID_INSTRUCTIONS + `## Synthesis: final refactor-scout report\n\n${ranked.length} opportunities survived independent verification.\n\n${block}\n\n` + + 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. " + "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: AdvisorySynthesisSchema, - }, - ) }], coverage); - - const resolved = resolveAdvisorySynthesis(report, ranked, { - impact: "Impact not restated by verification.", - recommendation: "Inspect the cited evidence and validate the smallest repair.", - }, coverage); - return finish({ ...resolved, stats: { ...stats, kept: resolved.findings.length } }); + ranked, coverage, + ); + 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/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 336eca2..e3d920f 100644 --- a/tests/advisory-utils.test.ts +++ b/tests/advisory-utils.test.ts @@ -26,6 +26,8 @@ function finding(file: string, line: number, overrides: Partial 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 }], @@ -64,7 +66,7 @@ test("findingLocationKey normalizes diff prefixes", () => { 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" }; + 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"]); diff --git a/tests/changedlines.test.ts b/tests/changedlines.test.ts index f41be8b..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,14 +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: duplicate }, - { 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/patch-validation.test.ts b/tests/patch-validation.test.ts index e2778a5..66fbe82 100644 --- a/tests/patch-validation.test.ts +++ b/tests/patch-validation.test.ts @@ -70,6 +70,19 @@ test("regression validation observes intended baseline failure and repaired succ } 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); @@ -82,18 +95,14 @@ test("fix workflow uses a fresh evaluator, retains rejected patches, and cannot try { if (options.label?.startsWith("fix:")) { await writeFile(join(workspace.cwd, "value.txt"), "fixed\n"); - await writeFile(join(workspace.cwd, ".untracked-implementer"), "hidden contamination"); - // Remove a scratch file before capture: evaluator must never inherit it. - await rm(join(workspace.cwd, ".untracked-implementer")); 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]); - await assert.rejects(readFile(join(workspace.cwd, ".untracked-implementer")), /ENOENT/); 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() {}, cwd: repo.cwd }, issues, + 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]!; @@ -138,6 +147,6 @@ test("fatal evaluator cancellation aborts the fix workflow instead of becoming b 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() {}, cwd: process.cwd() }, issues, + 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/); }); diff --git a/tests/review-actions.test.ts b/tests/review-actions.test.ts index 06f8968..f2b682d 100644 --- a/tests/review-actions.test.ts +++ b/tests/review-actions.test.ts @@ -157,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; @@ -168,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); @@ -189,7 +192,7 @@ 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(undefined, undefined, "diff --git a/src/app.ts b/src/app.ts\n+fixed\n"), + 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", From a91de6751482949fc81c2a8f6e9dceee10b88d73 Mon Sep 17 00:00:00 2001 From: timbrinded <79199034+timbrinded@users.noreply.github.com> Date: Wed, 16 Sep 2026 20:44:01 +0100 Subject: [PATCH 3/5] fix: retain validation evidence on cleanup failure --- .../src/advisory-challenge.ts | 5 +- .../src/review/patch-validation.ts | 106 +++++++++++------- USAGE.md | 9 +- tests/advisory-provenance.test.ts | 19 ++++ tests/patch-validation.test.ts | 71 ++++++++++++ 5 files changed, 164 insertions(+), 46 deletions(-) diff --git a/.pi/extensions/pi-workflow-engine/src/advisory-challenge.ts b/.pi/extensions/pi-workflow-engine/src/advisory-challenge.ts index 9bb3710..f883174 100644 --- a/.pi/extensions/pi-workflow-engine/src/advisory-challenge.ts +++ b/.pi/extensions/pi-workflow-engine/src/advisory-challenge.ts @@ -24,7 +24,10 @@ export interface AdvisoryChallengeOptions { /** 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(?:=(\d+))?(?=\s|$)/g, (_match, limit: string | undefined) => { + 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 " "; }); diff --git a/.pi/extensions/pi-workflow-engine/src/review/patch-validation.ts b/.pi/extensions/pi-workflow-engine/src/review/patch-validation.ts index 2102b1e..40cfa82 100644 --- a/.pi/extensions/pi-workflow-engine/src/review/patch-validation.ts +++ b/.pi/extensions/pi-workflow-engine/src/review/patch-validation.ts @@ -29,66 +29,88 @@ export interface PatchValidation { 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." }; } -/** Reconstruct the exact reviewed baseline and run evaluator-selected checks ourselves. */ -export async function validateCandidatePatch(options: { +interface PatchValidationOptions { cwd: string; baseline: WorktreeBaseline; expectedFingerprint: string; baselineOid: string; patch: string; evaluation: PatchEvaluation; signal?: AbortSignal; -}): Promise { +} + +/** 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 { - 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." }; + result = await evaluateCandidateInWorktrees(worktrees, options, validation); } catch (error) { throwIfAborted(options.signal); - return { ...validation, status: "blocked", reason: unknownErrorMessage(error) }; + result = { ...validation, status: "blocked", reason: unknownErrorMessage(error) }; } finally { - await worktrees.removeAll(); + 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 { diff --git a/USAGE.md b/USAGE.md index a800d59..97a91f8 100644 --- a/USAGE.md +++ b/USAGE.md @@ -564,8 +564,9 @@ 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` sets the limit, capped at ten; zero disables -it. Ordinary confirmed, low-risk findings bypass this stage. +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 @@ -593,7 +594,9 @@ 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. +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 diff --git a/tests/advisory-provenance.test.ts b/tests/advisory-provenance.test.ts index 4441018..b59318b 100644 --- a/tests/advisory-provenance.test.ts +++ b/tests/advisory-provenance.test.ts @@ -115,3 +115,22 @@ test("challenge is opt-in, bounded, and failures preserve the candidate as unres 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/patch-validation.test.ts b/tests/patch-validation.test.ts index 66fbe82..9ff96a5 100644 --- a/tests/patch-validation.test.ts +++ b/tests/patch-validation.test.ts @@ -1,6 +1,7 @@ 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"; @@ -150,3 +151,73 @@ test("fatal evaluator cancellation aborts the fix workflow instead of becoming b 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(); + } +}); From 34a9b15178b83049e361a4d2e973fff5b339d177 Mon Sep 17 00:00:00 2001 From: timbrinded <79199034+timbrinded@users.noreply.github.com> Date: Thu, 17 Sep 2026 08:57:52 +0100 Subject: [PATCH 4/5] refactor: remove obsolete advisory helpers and duplicate mappings --- .../src/workflow-advisory-utils.ts | 57 ++++--------------- .../workflows/code-review.ts | 8 +-- .../pi-workflow-engine/workflows/diagnose.ts | 10 +--- .../workflows/perf-review.ts | 10 +--- .../workflows/refactor-scout.ts | 10 +--- tests/advisory-utils.test.ts | 22 ------- 6 files changed, 23 insertions(+), 94 deletions(-) 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 799f2e2..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,4 +1,4 @@ -import { AdvisoryCandidatesSchema, AdvisoryVerdictSchema, type AdvisoryCandidate, type IdentifiedAdvisoryCandidate, type AdvisoryFinding, type AdvisoryLocation, type AdvisoryReport, type AdvisoryVerdict } from "./advisory-schema.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"; @@ -156,50 +156,17 @@ export function normalizePath(path: string): string { return path.replace(/^\.\//, "").replace(/^[ab]\//, ""); } -export function verdictLane(verdict: AdvisoryVerdict["verdict"]): string { - switch (verdict) { - case "CONFIRMED": - return "Confirmed"; - case "NOT_SUBSTANTIATED": - return "Unresolved"; - case "PLAUSIBLE": - return "Plausible"; - case "REFUTED": - return "Refuted"; - } -} - -export function verdictStatus(verdict: AdvisoryVerdict["verdict"]): "success" | "warning" | "error" { - switch (verdict) { - case "CONFIRMED": - return "success"; - case "NOT_SUBSTANTIATED": - 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 "NOT_SUBSTANTIATED": - 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( @@ -215,10 +182,10 @@ 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), }); } diff --git a/.pi/extensions/pi-workflow-engine/workflows/code-review.ts b/.pi/extensions/pi-workflow-engine/workflows/code-review.ts index 3281785..a68dc37 100644 --- a/.pi/extensions/pi-workflow-engine/workflows/code-review.ts +++ b/.pi/extensions/pi-workflow-engine/workflows/code-review.ts @@ -1,5 +1,4 @@ import { challengeFindings, parseChallengeArgs } from "../src/advisory-challenge.ts"; -import { type AdvisoryStageCoverage } from "../src/advisory-evidence.ts"; import { Type } from "typebox"; import { type AdvisoryVerified, @@ -44,8 +43,6 @@ const ANGLES: AdvisoryLens[] = [ { 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. */ @@ -95,7 +92,6 @@ export default async function run(api: WorkflowApi, dependencies: CodeReviewDepe let fileCount = 0; let rawCandidateCount = 0; let droppedCandidateCount = 0; - const coverage: AdvisoryStageCoverage[] = []; const makeStats = (verified: number, kept: number): WorkflowRunStats => ({ files: fileCount, candidates: rawCandidateCount, @@ -123,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) { @@ -204,7 +200,7 @@ export default async function run(api: WorkflowApi, dependencies: CodeReviewDepe }); rawCandidateCount += pipelineResult.rawCandidates; droppedCandidateCount += pipelineResult.dropped; - coverage.push(...pipelineResult.coverage); + 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); diff --git a/.pi/extensions/pi-workflow-engine/workflows/diagnose.ts b/.pi/extensions/pi-workflow-engine/workflows/diagnose.ts index 4385dfb..0fe2955 100644 --- a/.pi/extensions/pi-workflow-engine/workflows/diagnose.ts +++ b/.pi/extensions/pi-workflow-engine/workflows/diagnose.ts @@ -1,5 +1,4 @@ import { challengeFindings, parseChallengeArgs } from "../src/advisory-challenge.ts"; -import { type AdvisoryStageCoverage } from "../src/advisory-evidence.ts"; import { Type } from "typebox"; import { type AdvisoryVerified, @@ -38,8 +37,6 @@ const HYPOTHESIS_LENSES: AdvisoryLens[] = [ { 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 { @@ -50,7 +47,6 @@ export default async function run(api: WorkflowApi): Promise { let rawCandidateCount = 0; let droppedCandidateCount = 0; let refutedCandidateCount = 0; - const coverage: AdvisoryStageCoverage[] = []; const makeStats = (verified: number, kept: number): WorkflowRunStats => ({ files: fileCount, candidates: rawCandidateCount, @@ -69,7 +65,7 @@ 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) { @@ -77,7 +73,7 @@ export default async function run(api: WorkflowApi): Promise { "Diagnosis could not establish a scope.", ["Provide the failing command, error message, or regression description and rerun diagnose."], makeStats(0, 0), - ), coverage); + ), []); } fileCount = scope.files.length; @@ -114,7 +110,7 @@ export default async function run(api: WorkflowApi): Promise { rawCandidateCount += pipelineResult.rawCandidates; droppedCandidateCount += pipelineResult.dropped; refutedCandidateCount += pipelineResult.refuted; - coverage.push(...pipelineResult.coverage); + 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"); diff --git a/.pi/extensions/pi-workflow-engine/workflows/perf-review.ts b/.pi/extensions/pi-workflow-engine/workflows/perf-review.ts index ac053ae..d055e8f 100644 --- a/.pi/extensions/pi-workflow-engine/workflows/perf-review.ts +++ b/.pi/extensions/pi-workflow-engine/workflows/perf-review.ts @@ -1,5 +1,4 @@ import { challengeFindings, parseChallengeArgs } from "../src/advisory-challenge.ts"; -import { type AdvisoryStageCoverage } from "../src/advisory-evidence.ts"; import { Type } from "typebox"; import { type AdvisoryVerified, @@ -39,8 +38,6 @@ const PERF_LENSES: AdvisoryLens[] = [ { 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 { @@ -51,7 +48,6 @@ export default async function run(api: WorkflowApi): Promise { let rawCandidateCount = 0; let droppedCandidateCount = 0; let refutedCandidateCount = 0; - const coverage: AdvisoryStageCoverage[] = []; const makeStats = (verified: number, kept: number): WorkflowRunStats => ({ files: fileCount, candidates: rawCandidateCount, @@ -68,7 +64,7 @@ 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) { @@ -76,7 +72,7 @@ export default async function run(api: WorkflowApi): Promise { "No performance-relevant files were identified.", ["Provide a slow command, workload, file path, or user-visible latency concern to review."], makeStats(0, 0), - ), coverage); + ), []); } fileCount = scope.files.length; @@ -113,7 +109,7 @@ export default async function run(api: WorkflowApi): Promise { rawCandidateCount += pipelineResult.rawCandidates; droppedCandidateCount += pipelineResult.dropped; refutedCandidateCount += pipelineResult.refuted; - coverage.push(...pipelineResult.coverage); + 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); diff --git a/.pi/extensions/pi-workflow-engine/workflows/refactor-scout.ts b/.pi/extensions/pi-workflow-engine/workflows/refactor-scout.ts index bcd3c38..31f6a06 100644 --- a/.pi/extensions/pi-workflow-engine/workflows/refactor-scout.ts +++ b/.pi/extensions/pi-workflow-engine/workflows/refactor-scout.ts @@ -1,5 +1,4 @@ import { challengeFindings, parseChallengeArgs } from "../src/advisory-challenge.ts"; -import { type AdvisoryStageCoverage } from "../src/advisory-evidence.ts"; import { Type } from "typebox"; import { type AdvisoryVerified, @@ -38,8 +37,6 @@ const REFACTOR_LENSES: AdvisoryLens[] = [ { 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 { @@ -50,7 +47,6 @@ export default async function run(api: WorkflowApi): Promise { let rawCandidateCount = 0; let droppedCandidateCount = 0; let refutedCandidateCount = 0; - const coverage: AdvisoryStageCoverage[] = []; const makeStats = (verified: number, kept: number): WorkflowRunStats => ({ files: fileCount, candidates: rawCandidateCount, @@ -67,7 +63,7 @@ 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) { @@ -75,7 +71,7 @@ export default async function run(api: WorkflowApi): Promise { "No files were identified for refactor scouting.", ["Provide a target path, module, or subsystem to scout for refactor opportunities."], makeStats(0, 0), - ), coverage); + ), []); } fileCount = scope.files.length; @@ -110,7 +106,7 @@ export default async function run(api: WorkflowApi): Promise { rawCandidateCount += pipelineResult.rawCandidates; droppedCandidateCount += pipelineResult.dropped; refutedCandidateCount += pipelineResult.refuted; - coverage.push(...pipelineResult.coverage); + 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); diff --git a/tests/advisory-utils.test.ts b/tests/advisory-utils.test.ts index e3d920f..93d0fbf 100644 --- a/tests/advisory-utils.test.ts +++ b/tests/advisory-utils.test.ts @@ -1,29 +1,12 @@ 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", @@ -59,11 +42,6 @@ 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"] }; From fe3921e7fe3981097a0d1a8e01a508d8cd10f8fa Mon Sep 17 00:00:00 2001 From: timbrinded <79199034+timbrinded@users.noreply.github.com> Date: Thu, 17 Sep 2026 09:07:32 +0100 Subject: [PATCH 5/5] refactorings --- .gitignore | 1 + TODO.md | 4 ---- 2 files changed, 1 insertion(+), 4 deletions(-) delete mode 100644 TODO.md 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/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