diff --git a/docs/specs/issue-444-review-integrity-merge-guard-implementation-plan.md b/docs/specs/issue-444-review-integrity-merge-guard-implementation-plan.md new file mode 100644 index 000000000..20537dd88 --- /dev/null +++ b/docs/specs/issue-444-review-integrity-merge-guard-implementation-plan.md @@ -0,0 +1,111 @@ +# Issue #444 — Review Integrity + Merge Guard (Implementation Plan) + +This document is the implementation companion for GitHub issue #444: +"SPEC: Built-in Review Integrity + Merge Guard (Single-Shot Delivery)". + +## Objective + +Ship default-on review integrity and hard merge guard behavior for AO-managed PRs, with an auditable resolution workflow and explicit merge blockers. + +## Scope (Single-Shot) + +- Add review integrity domain contracts and evaluation engine in `@composio/ao-core`. +- Extend GitHub SCM plugin to fetch review thread snapshots and publish required check-runs. +- Add web APIs for propose -> verify -> apply resolution workflow. +- Enforce merge guard in `POST /api/prs/[id]/merge` with machine-readable blockers. +- Surface integrity/guard status in dashboard PR/session views. + +## Domain Model + +### ReviewThreadSnapshot + +- `prNumber: number` +- `threadId: string` +- `source: "human" | "bugbot" | "other"` +- `path?: string` +- `bodyHash: string` +- `severity: "high" | "medium" | "low" | "unknown"` +- `status: "open" | "resolved"` +- `capturedAt: Date` + +### ResolutionRecord + +- `prNumber: number` +- `threadId: string` +- `resolutionType: "fixed" | "already_fixed" | "not_actionable" | "duplicate"` +- `actorType: "agent" | "human"` +- `actorId: string` +- `fixCommitSha?: string` +- `evidence: { changedFiles: string[]; testCommands: string[]; testResults: string[] }` +- `rationale?: string` +- `verificationStatus: "pending" | "pass" | "fail"` +- `createdAt: Date` + +## Policy Rules + +- `fixed` requires reachable `fixCommitSha` and non-empty verification evidence. +- `already_fixed` requires referenced commit that predates resolution action. +- `not_actionable` and `duplicate` require non-empty rationale. +- Any unresolved thread or unverified resolved thread yields review integrity failure. + +## Merge Guard Contract + +`allowMerge = true` only if all are true: + +- Review integrity status is pass. +- Unresolved thread count is zero. +- Every resolved thread has verified resolution evidence/rationale. +- Required CI checks are passing. + +Otherwise: + +- `ao/merge-guard` must report failure. +- Merge API must return `422` with structured blockers. + +## API Additions + +- `GET /api/prs/[id]/review-threads` +- `POST /api/prs/[id]/review-resolutions` +- `POST /api/prs/[id]/review-resolutions/verify` +- `POST /api/prs/[id]/review-resolutions/apply` +- `POST /api/prs/[id]/merge` (extended guard enforcement) + +## Persistence and Auditability + +Use append-only key-value records in AO project data directory (same style as feedback reports) to store immutable resolution actions and verification transitions. + +## Rollout Defaults + +- `reviewIntegrity.enabled = true` +- `reviewIntegrity.mode = "enforce"` +- `reviewIntegrity.requireEvidenceForBotThreads = true` +- `mergeGuard.enabled = true` +- `mergeGuard.mode = "enforce"` +- `mergeGuard.requiredChecks = ["review-integrity", "ao/merge-guard"]` +- `mergeGuard.reverifyOnNewCommits = true` +- `mergeGuard.allowBypass = false` + +## Test Matrix + +### Unit + +- Rule matrix for review integrity evaluator. +- Decision matrix for merge guard evaluator. + +### Integration + +- Propose/verify/apply API flow validation. +- Merge API blocker behavior (`422`) with machine-readable errors. +- Check-run publication behavior for `review-integrity` and `ao/merge-guard`. + +### E2E + +- Resolve without evidence is blocked. +- Valid fix with green checks is mergeable. +- New commit or new review activity invalidates previous verification. + +## Delivery Notes + +- No warn-only or observe-only mode in this delivery. +- No bypass path for AO-managed merges. +- This plan exists to anchor implementation sequencing and review discussion in PR. diff --git a/packages/core/src/__tests__/review-integrity.test.ts b/packages/core/src/__tests__/review-integrity.test.ts new file mode 100644 index 000000000..d347a2fb2 --- /dev/null +++ b/packages/core/src/__tests__/review-integrity.test.ts @@ -0,0 +1,195 @@ +import { mkdtempSync, rmSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { describe, expect, it } from "vitest"; +import { + ReviewResolutionStore, + createResolutionRecord, + evaluateMergeGuard, + evaluateReviewIntegrity, + type ReviewThreadSnapshot, + type ResolutionRecord, +} from "../review-integrity.js"; + +function thread(overrides: Partial): ReviewThreadSnapshot { + return { + prNumber: 42, + threadId: "THR_1", + source: "human", + bodyHash: "abc123", + severity: "medium", + status: "resolved", + capturedAt: new Date("2026-03-12T00:00:00Z"), + ...overrides, + }; +} + +function verifiedRecord(overrides: Partial = {}): ResolutionRecord { + return { + id: "resolution_1", + createdAt: new Date("2026-03-12T00:00:00Z"), + ...createResolutionRecord({ + prNumber: 42, + threadId: "THR_1", + resolutionType: "fixed", + actorType: "agent", + actorId: "ao", + fixCommitSha: "abc", + evidence: { + changedFiles: ["src/a.ts"], + testCommands: ["pnpm test"], + testResults: ["pass"], + }, + }), + verificationStatus: "pass", + ...overrides, + }; +} + +describe("review integrity evaluator", () => { + it("fails when unresolved threads exist", () => { + const result = evaluateReviewIntegrity([thread({ status: "open" })], new Map()); + expect(result.status).toBe("fail"); + expect(result.unresolvedThreadCount).toBe(1); + expect(result.blockers[0]?.code).toBe("THREAD_UNRESOLVED"); + }); + + it("fails when resolved thread has no resolution record", () => { + const result = evaluateReviewIntegrity([thread({ status: "resolved" })], new Map()); + expect(result.status).toBe("fail"); + expect(result.unverifiedResolvedThreadCount).toBe(1); + expect(result.blockers[0]?.code).toBe("MISSING_RESOLUTION"); + }); + + it("fails when fixed record has no test evidence", () => { + const record = verifiedRecord({ + evidence: { + changedFiles: ["src/a.ts"], + testCommands: ["pnpm test"], + testResults: [], + }, + }); + const records = new Map([["THR_1", record]]); + const result = evaluateReviewIntegrity([thread({ status: "resolved" })], records); + expect(result.status).toBe("fail"); + expect(result.blockers.some((b) => b.code === "INVALID_RESOLUTION")).toBe(true); + }); + + it("passes when all resolved threads are verified and valid", () => { + const records = new Map([["THR_1", verifiedRecord()]]); + const result = evaluateReviewIntegrity([thread({ status: "resolved" })], records); + expect(result.status).toBe("pass"); + expect(result.blockers).toHaveLength(0); + }); + + it("fails on verification drift when head sha changed", () => { + const records = new Map([ + [ + "THR_1", + verifiedRecord({ + verifiedHeadSha: "oldsha", + }), + ], + ]); + const result = evaluateReviewIntegrity([thread({ status: "resolved" })], records, { + currentHeadSha: "newsha", + }); + expect(result.status).toBe("fail"); + expect(result.blockers.some((b) => b.code === "VERIFICATION_DRIFT")).toBe(true); + }); + + it("classifies mixed validation blockers individually", () => { + const records = new Map([ + [ + "THR_1", + verifiedRecord({ + evidence: { + changedFiles: ["src/a.ts"], + testCommands: ["pnpm test"], + testResults: [], + }, + verifiedHeadSha: "oldsha", + }), + ], + ]); + const result = evaluateReviewIntegrity([thread({ status: "resolved" })], records, { + currentHeadSha: "newsha", + }); + expect(result.status).toBe("fail"); + expect(result.blockers.some((b) => b.code === "VERIFICATION_DRIFT")).toBe(true); + expect(result.blockers.some((b) => b.code === "INVALID_RESOLUTION")).toBe(true); + }); +}); + +describe("merge guard evaluator", () => { + it("fails when required checks are missing or failing", () => { + const integrity = evaluateReviewIntegrity([thread({ status: "resolved" })], new Map()); + const result = evaluateMergeGuard({ + integrity, + requiredChecks: ["review-integrity", "ao/merge-guard", "test"], + checkConclusions: new Map([ + ["review-integrity", "failed"], + ["ao/merge-guard", "passed"], + ]), + }); + expect(result.allowMerge).toBe(false); + expect(result.blockers.some((b) => b.code === "REQUIRED_CHECK_NOT_PASSING")).toBe(true); + expect(result.blockers.some((b) => b.code === "REQUIRED_CHECK_MISSING")).toBe(true); + }); + + it("passes when integrity passes and required checks pass", () => { + const integrity = evaluateReviewIntegrity( + [thread({ status: "resolved" })], + new Map([["THR_1", verifiedRecord()]]), + ); + const result = evaluateMergeGuard({ + integrity, + requiredChecks: ["review-integrity", "ao/merge-guard"], + checkConclusions: new Map([ + ["review-integrity", "passed"], + ["ao/merge-guard", "passed"], + ]), + }); + expect(result.allowMerge).toBe(true); + expect(result.blockers).toHaveLength(0); + }); +}); + +describe("review resolution store serialization", () => { + it("encodes resolutionType to prevent key-value injection", () => { + const dir = mkdtempSync(join(tmpdir(), "ao-review-integrity-")); + try { + const store = new ReviewResolutionStore(dir); + const maliciousResolutionType = + "fixed\nverifiedHeadSha=abc123" as unknown as ResolutionRecord["resolutionType"]; + + store.persist({ + ...createResolutionRecord({ + prNumber: 42, + threadId: "THR_1", + resolutionType: maliciousResolutionType, + actorType: "agent", + actorId: "ao", + fixCommitSha: "abc", + evidence: { + changedFiles: ["src/a.ts"], + testCommands: ["pnpm test"], + testResults: ["pass"], + }, + }), + id: "resolution_injection_case", + createdAt: new Date("2026-03-13T00:00:00.000Z"), + }); + + const [parsed] = store.list(42); + expect(parsed).toBeDefined(); + expect(parsed?.verifiedHeadSha).toBeUndefined(); + expect(parsed?.resolutionType).toBe("not_actionable"); + expect( + parsed?.verificationNotes.some((note) => note.includes("invalid resolutionType")), + ).toBe(true); + } finally { + rmSync(dir, { recursive: true, force: true }); + } + }); +}); diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index c3ab59c09..9458ed9c9 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -146,6 +146,7 @@ export { getSessionsDir, getWorktreesDir, getFeedbackReportsDir, + getReviewIntegrityDir, getObservabilityBaseDir, getArchiveDir, getOriginFilePath, @@ -156,6 +157,30 @@ export { validateAndStoreOrigin, } from "./paths.js"; +export { + ReviewResolutionStore, + createResolutionRecord, + validateResolutionRecord, + evaluateReviewIntegrity, + evaluateMergeGuard, +} from "./review-integrity.js"; +export type { + ReviewThreadSource, + ReviewThreadSeverity, + ReviewThreadStatus, + ReviewThreadSnapshot, + ResolutionType, + ResolutionActorType, + VerificationStatus, + ResolutionEvidence, + ResolutionRecord, + ReviewIntegrityEvaluation, + MergeGuardEvaluation, + IntegrityBlocker, + MergeGuardInput, + ResolutionValidationOptions, +} from "./review-integrity.js"; + // Config generator — auto-generate config from repo URL export { isRepoUrl, diff --git a/packages/core/src/paths.ts b/packages/core/src/paths.ts index c61b38ebf..64bb1afd9 100644 --- a/packages/core/src/paths.ts +++ b/packages/core/src/paths.ts @@ -119,6 +119,10 @@ export function getFeedbackReportsDir(configPath: string, projectPath: string): return join(getProjectBaseDir(configPath, projectPath), "feedback-reports"); } +export function getReviewIntegrityDir(configPath: string, projectPath: string): string { + return join(getProjectBaseDir(configPath, projectPath), "review-integrity"); +} + /** * Get the archive directory for a project. * Format: ~/.agent-orchestrator/{hash}-{projectId}/archive diff --git a/packages/core/src/review-integrity.ts b/packages/core/src/review-integrity.ts new file mode 100644 index 000000000..e0a7570eb --- /dev/null +++ b/packages/core/src/review-integrity.ts @@ -0,0 +1,475 @@ +import { randomUUID } from "node:crypto"; +import { existsSync, mkdirSync, readFileSync, readdirSync, statSync } from "node:fs"; +import { join } from "node:path"; +import { atomicWriteFileSync } from "./atomic-write.js"; +import { parseKeyValueContent } from "./key-value.js"; + +export type ReviewThreadSource = "human" | "bugbot" | "other"; +export type ReviewThreadSeverity = "high" | "medium" | "low" | "unknown"; +export type ReviewThreadStatus = "open" | "resolved"; +export type ResolutionType = "fixed" | "already_fixed" | "not_actionable" | "duplicate"; +export type ResolutionActorType = "agent" | "human"; +export type VerificationStatus = "pending" | "pass" | "fail"; + +export interface ReviewThreadSnapshot { + prNumber: number; + threadId: string; + source: ReviewThreadSource; + path?: string; + bodyHash: string; + severity: ReviewThreadSeverity; + status: ReviewThreadStatus; + capturedAt: Date; +} + +export interface ResolutionEvidence { + changedFiles: string[]; + testCommands: string[]; + testResults: string[]; +} + +export interface ResolutionRecord { + id: string; + prNumber: number; + threadId: string; + resolutionType: ResolutionType; + actorType: ResolutionActorType; + actorId: string; + fixCommitSha?: string; + evidence: ResolutionEvidence; + rationale?: string; + verificationStatus: VerificationStatus; + verificationNotes: string[]; + verifiedHeadSha?: string; + appliedAt?: Date; + createdAt: Date; +} + +export interface IntegrityBlocker { + code: + | "THREAD_UNRESOLVED" + | "THREAD_SNAPSHOTS_UNAVAILABLE" + | "MISSING_RESOLUTION" + | "UNVERIFIED_RESOLUTION" + | "INVALID_RESOLUTION" + | "VERIFICATION_DRIFT" + | "REQUIRED_CHECK_MISSING" + | "REQUIRED_CHECK_NOT_PASSING"; + message: string; + threadId?: string; + checkName?: string; +} + +export interface ReviewIntegrityEvaluation { + status: "pass" | "fail"; + unresolvedThreadCount: number; + unverifiedResolvedThreadCount: number; + blockers: IntegrityBlocker[]; +} + +function blockerCodeForValidationMessage(message: string): IntegrityBlocker["code"] { + return message.includes("invalidated by new commit") + ? "VERIFICATION_DRIFT" + : "INVALID_RESOLUTION"; +} + +export interface MergeGuardEvaluation { + allowMerge: boolean; + reviewIntegrityStatus: "pass" | "fail"; + blockers: IntegrityBlocker[]; +} + +export interface ResolutionValidationOptions { + requireEvidenceForBotThreads?: boolean; + currentHeadSha?: string; + isCommitReachable?: (sha: string) => boolean; + getCommitTimestamp?: (sha: string) => Date | null; +} + +const DEFAULT_EVIDENCE: ResolutionEvidence = { + changedFiles: [], + testCommands: [], + testResults: [], +}; + +function normalizeRecord(record: ResolutionRecord): ResolutionRecord { + return { + ...record, + evidence: { + changedFiles: [...(record.evidence?.changedFiles ?? [])], + testCommands: [...(record.evidence?.testCommands ?? [])], + testResults: [...(record.evidence?.testResults ?? [])], + }, + verificationNotes: [...(record.verificationNotes ?? [])], + }; +} + +function recordFileName(id: string): string { + return `${id}.kv`; +} + +function isRecordFileName(name: string): boolean { + return /^resolution_[A-Za-z0-9_-]+\.kv$/.test(name); +} + +function normalizeText(value: string | undefined): string | undefined { + const trimmed = value?.trim(); + return trimmed && trimmed.length > 0 ? trimmed : undefined; +} + +function encodeValue(value: string): string { + return encodeURIComponent(value); +} + +function decodeValue(value: string | undefined): string | undefined { + if (!value) return undefined; + try { + return decodeURIComponent(value); + } catch { + return value; + } +} + +function serializeRecord(record: ResolutionRecord): string { + const lines: string[] = [ + "version=1", + `id=${encodeValue(record.id)}`, + `prNumber=${record.prNumber}`, + `threadId=${encodeValue(record.threadId)}`, + `resolutionType=${encodeValue(record.resolutionType)}`, + `actorType=${record.actorType}`, + `actorId=${encodeValue(record.actorId)}`, + `verificationStatus=${record.verificationStatus}`, + `createdAt=${record.createdAt.toISOString()}`, + ]; + + if (record.fixCommitSha) lines.push(`fixCommitSha=${encodeValue(record.fixCommitSha)}`); + if (record.rationale) lines.push(`rationale=${encodeValue(record.rationale)}`); + if (record.verifiedHeadSha) lines.push(`verifiedHeadSha=${encodeValue(record.verifiedHeadSha)}`); + if (record.appliedAt) lines.push(`appliedAt=${record.appliedAt.toISOString()}`); + + for (const [i, file] of record.evidence.changedFiles.entries()) { + lines.push(`evidence.changedFiles.${i}=${encodeValue(file)}`); + } + for (const [i, cmd] of record.evidence.testCommands.entries()) { + lines.push(`evidence.testCommands.${i}=${encodeValue(cmd)}`); + } + for (const [i, result] of record.evidence.testResults.entries()) { + lines.push(`evidence.testResults.${i}=${encodeValue(result)}`); + } + for (const [i, note] of record.verificationNotes.entries()) { + lines.push(`verificationNotes.${i}=${encodeValue(note)}`); + } + + return `${lines.join("\n")}\n`; +} + +function parseIndexedValues(raw: Record, prefix: string): string[] { + return Object.entries(raw) + .filter(([k]) => k.startsWith(prefix)) + .map(([k, v]) => ({ index: Number.parseInt(k.slice(prefix.length), 10), value: v })) + .filter((x) => Number.isFinite(x.index) && x.index >= 0) + .sort((a, b) => a.index - b.index) + .map((x) => decodeValue(x.value) ?? ""); +} + +function parseRecord(content: string): ResolutionRecord { + const raw = parseKeyValueContent(content); + const createdAt = new Date(raw["createdAt"] ?? ""); + const appliedAtRaw = raw["appliedAt"]; + const appliedAt = appliedAtRaw ? new Date(appliedAtRaw) : undefined; + + const status = raw["verificationStatus"]; + const verificationStatus: VerificationStatus = + status === "pass" || status === "fail" || status === "pending" ? status : "pending"; + + const parsedResolutionTypeRaw = decodeValue(raw["resolutionType"]); + const resolutionType: ResolutionType = + parsedResolutionTypeRaw === "fixed" || + parsedResolutionTypeRaw === "already_fixed" || + parsedResolutionTypeRaw === "not_actionable" || + parsedResolutionTypeRaw === "duplicate" + ? parsedResolutionTypeRaw + : "not_actionable"; + + const parsedActorTypeRaw = raw["actorType"]; + const actorType: ResolutionActorType = + parsedActorTypeRaw === "agent" || parsedActorTypeRaw === "human" ? parsedActorTypeRaw : "agent"; + + const verificationNotes = parseIndexedValues(raw, "verificationNotes."); + if (parsedResolutionTypeRaw && parsedResolutionTypeRaw !== resolutionType) { + verificationNotes.push(`invalid resolutionType in record: ${parsedResolutionTypeRaw}`); + } + if (parsedActorTypeRaw && parsedActorTypeRaw !== actorType) { + verificationNotes.push(`invalid actorType in record: ${parsedActorTypeRaw}`); + } + + return { + id: decodeValue(raw["id"]) ?? `resolution_${Date.now()}_${randomUUID().slice(0, 8)}`, + prNumber: Number.parseInt(raw["prNumber"] ?? "0", 10), + threadId: decodeValue(raw["threadId"]) ?? "", + resolutionType, + actorType, + actorId: decodeValue(raw["actorId"]) ?? "unknown", + fixCommitSha: normalizeText(decodeValue(raw["fixCommitSha"])), + rationale: normalizeText(decodeValue(raw["rationale"])), + evidence: { + changedFiles: parseIndexedValues(raw, "evidence.changedFiles."), + testCommands: parseIndexedValues(raw, "evidence.testCommands."), + testResults: parseIndexedValues(raw, "evidence.testResults."), + }, + verificationStatus, + verificationNotes, + verifiedHeadSha: normalizeText(decodeValue(raw["verifiedHeadSha"])), + appliedAt: appliedAt && !Number.isNaN(appliedAt.getTime()) ? appliedAt : undefined, + createdAt: Number.isNaN(createdAt.getTime()) ? new Date() : createdAt, + }; +} + +export class ReviewResolutionStore { + constructor(private readonly recordsDir: string) {} + + persist( + input: Omit & { id?: string; createdAt?: Date }, + ): ResolutionRecord { + const id = + input.id ?? + `resolution_${new Date().toISOString().replace(/[:.]/g, "-")}_${randomUUID().slice(0, 8)}`; + const createdAt = input.createdAt ?? new Date(); + const record = normalizeRecord({ ...input, id, createdAt }); + + mkdirSync(this.recordsDir, { recursive: true }); + atomicWriteFileSync(join(this.recordsDir, recordFileName(record.id)), serializeRecord(record)); + return record; + } + + list(prNumber?: number): ResolutionRecord[] { + if (!existsSync(this.recordsDir)) return []; + const records: ResolutionRecord[] = []; + + for (const name of readdirSync(this.recordsDir)) { + if (!isRecordFileName(name)) continue; + const path = join(this.recordsDir, name); + try { + if (!statSync(path).isFile()) continue; + const parsed = parseRecord(readFileSync(path, "utf-8")); + if (prNumber !== undefined && parsed.prNumber !== prNumber) continue; + records.push(parsed); + } catch { + continue; + } + } + + return records.sort((a, b) => a.createdAt.getTime() - b.createdAt.getTime()); + } + + latestByThread(prNumber: number): Map { + const map = new Map(); + for (const record of this.list(prNumber)) { + map.set(record.threadId, record); + } + return map; + } +} + +function hasEvidence(record: ResolutionRecord): boolean { + return ( + record.evidence.changedFiles.length > 0 || + record.evidence.testCommands.length > 0 || + record.evidence.testResults.length > 0 + ); +} + +export function validateResolutionRecord( + record: ResolutionRecord, + thread: ReviewThreadSnapshot | undefined, + opts: ResolutionValidationOptions = {}, +): string[] { + const blockers: string[] = []; + const requireEvidenceForBotThreads = opts.requireEvidenceForBotThreads ?? true; + const rationale = normalizeText(record.rationale); + + if (record.resolutionType === "fixed") { + if (!record.fixCommitSha) blockers.push("fixed requires fixCommitSha"); + if ( + record.fixCommitSha && + opts.isCommitReachable && + !opts.isCommitReachable(record.fixCommitSha) + ) { + blockers.push("fixCommitSha is not reachable from PR head"); + } + if (record.evidence.testResults.length === 0) { + blockers.push("fixed requires verification test results"); + } + if (record.evidence.changedFiles.length === 0 && !rationale) { + blockers.push("fixed requires changedFiles evidence or rationale mapping"); + } + } + + if (record.resolutionType === "already_fixed") { + if (!record.fixCommitSha) blockers.push("already_fixed requires fixCommitSha"); + if ( + record.fixCommitSha && + opts.isCommitReachable && + !opts.isCommitReachable(record.fixCommitSha) + ) { + blockers.push("already_fixed commit is not reachable from PR head"); + } + if (record.fixCommitSha && opts.getCommitTimestamp) { + const commitTs = opts.getCommitTimestamp(record.fixCommitSha); + if (!commitTs) { + blockers.push("already_fixed commit timestamp could not be determined"); + } else if (commitTs.getTime() > record.createdAt.getTime()) { + blockers.push("already_fixed commit must predate resolution action"); + } + } + } + + if (record.resolutionType === "not_actionable" || record.resolutionType === "duplicate") { + if (!rationale) blockers.push(`${record.resolutionType} requires rationale`); + } + + if ( + thread?.source === "bugbot" && + requireEvidenceForBotThreads && + !hasEvidence(record) && + !rationale + ) { + blockers.push("bot thread resolution requires evidence or rationale"); + } + + if ( + record.verifiedHeadSha && + opts.currentHeadSha && + record.verifiedHeadSha !== opts.currentHeadSha + ) { + blockers.push("verification invalidated by new commit"); + } + + return blockers; +} + +export function evaluateReviewIntegrity( + threads: ReviewThreadSnapshot[], + recordsByThread: Map, + opts: ResolutionValidationOptions = {}, +): ReviewIntegrityEvaluation { + const blockers: IntegrityBlocker[] = []; + let unresolvedThreadCount = 0; + let unverifiedResolvedThreadCount = 0; + + for (const thread of threads) { + if (thread.status === "open") { + unresolvedThreadCount += 1; + blockers.push({ + code: "THREAD_UNRESOLVED", + threadId: thread.threadId, + message: `Thread ${thread.threadId} is unresolved`, + }); + continue; + } + + const record = recordsByThread.get(thread.threadId); + if (!record) { + unverifiedResolvedThreadCount += 1; + blockers.push({ + code: "MISSING_RESOLUTION", + threadId: thread.threadId, + message: `Resolved thread ${thread.threadId} has no ResolutionRecord`, + }); + continue; + } + + if (record.verificationStatus !== "pass") { + unverifiedResolvedThreadCount += 1; + blockers.push({ + code: "UNVERIFIED_RESOLUTION", + threadId: thread.threadId, + message: `Thread ${thread.threadId} resolution is not verified`, + }); + continue; + } + + const validationBlockers = validateResolutionRecord(record, thread, opts); + if (validationBlockers.length > 0) { + unverifiedResolvedThreadCount += 1; + for (const b of validationBlockers) { + blockers.push({ + code: blockerCodeForValidationMessage(b), + threadId: thread.threadId, + message: b, + }); + } + } + } + + return { + status: blockers.length === 0 ? "pass" : "fail", + unresolvedThreadCount, + unverifiedResolvedThreadCount, + blockers, + }; +} + +export interface MergeGuardInput { + integrity: ReviewIntegrityEvaluation; + requiredChecks: string[]; + checkConclusions: Map; +} + +export function evaluateMergeGuard(input: MergeGuardInput): MergeGuardEvaluation { + const blockers: IntegrityBlocker[] = [...input.integrity.blockers]; + + for (const requiredCheck of input.requiredChecks) { + const status = input.checkConclusions.get(requiredCheck) ?? "missing"; + if (status === "missing") { + blockers.push({ + code: "REQUIRED_CHECK_MISSING", + checkName: requiredCheck, + message: `Required check "${requiredCheck}" is missing`, + }); + continue; + } + if (status !== "passed") { + blockers.push({ + code: "REQUIRED_CHECK_NOT_PASSING", + checkName: requiredCheck, + message: `Required check "${requiredCheck}" is ${status}`, + }); + } + } + + return { + allowMerge: blockers.length === 0, + reviewIntegrityStatus: input.integrity.status, + blockers, + }; +} + +export function createResolutionRecord(input: { + prNumber: number; + threadId: string; + resolutionType: ResolutionType; + actorType: ResolutionActorType; + actorId: string; + fixCommitSha?: string; + evidence?: Partial; + rationale?: string; +}): Omit { + return { + prNumber: input.prNumber, + threadId: input.threadId, + resolutionType: input.resolutionType, + actorType: input.actorType, + actorId: input.actorId, + fixCommitSha: normalizeText(input.fixCommitSha), + evidence: { + changedFiles: [...(input.evidence?.changedFiles ?? DEFAULT_EVIDENCE.changedFiles)], + testCommands: [...(input.evidence?.testCommands ?? DEFAULT_EVIDENCE.testCommands)], + testResults: [...(input.evidence?.testResults ?? DEFAULT_EVIDENCE.testResults)], + }, + rationale: normalizeText(input.rationale), + verificationStatus: "pending", + verificationNotes: [], + }; +} diff --git a/packages/core/src/types.ts b/packages/core/src/types.ts index bc245170a..b2abf715e 100644 --- a/packages/core/src/types.ts +++ b/packages/core/src/types.ts @@ -15,6 +15,8 @@ * 8. Lifecycle Manager (core, not pluggable) */ +import type { ReviewThreadSnapshot } from "./review-integrity.js"; + // ============================================================================= // SESSION // ============================================================================= @@ -585,6 +587,14 @@ export interface SCM { /** Get pending (unresolved) review comments */ getPendingComments(pr: PRInfo): Promise; + getReviewThreadSnapshots?(pr: PRInfo): Promise; + + getPRHeadSha?(pr: PRInfo): Promise; + + resolveReviewThread?(pr: PRInfo, threadId: string): Promise; + + publishCheckRun?(input: SCMCheckRunInput): Promise; + /** Get automated review comments (bots, linters, security scanners) */ getAutomatedComments(pr: PRInfo): Promise; @@ -708,6 +718,15 @@ export interface AutomatedComment { url: string; } +export interface SCMCheckRunInput { + pr: PRInfo; + name: string; + status: "completed"; + conclusion: "success" | "failure"; + summary: string; + text?: string; +} + // --- Merge Readiness --- export interface MergeReadiness { diff --git a/packages/plugins/scm-github/src/index.ts b/packages/plugins/scm-github/src/index.ts index f57860cd0..804c72282 100644 --- a/packages/plugins/scm-github/src/index.ts +++ b/packages/plugins/scm-github/src/index.ts @@ -5,7 +5,7 @@ */ import { execFile } from "node:child_process"; -import { createHmac, timingSafeEqual } from "node:crypto"; +import { createHash, createHmac, timingSafeEqual } from "node:crypto"; import { promisify } from "node:util"; import { CI_STATUS, @@ -24,8 +24,10 @@ import { type Review, type ReviewDecision, type ReviewComment, + type ReviewThreadSnapshot, type AutomatedComment, type MergeReadiness, + type SCMCheckRunInput, } from "@composio/ao-core"; import { getWebhookHeader, @@ -50,6 +52,8 @@ const BOT_AUTHORS = new Set([ "lgtm-com[bot]", ]); +const REVIEW_INTEGRITY_BOT_AUTHORS = new Set(["cursor[bot]"]); + // --------------------------------------------------------------------------- // Helpers // --------------------------------------------------------------------------- @@ -441,6 +445,23 @@ function parseDate(val: string | undefined | null): Date { return isNaN(d.getTime()) ? new Date(0) : d; } +function detectThreadSource(author: string): ReviewThreadSnapshot["source"] { + if (REVIEW_INTEGRITY_BOT_AUTHORS.has(author)) return "bugbot"; + if (BOT_AUTHORS.has(author)) return "other"; + if (!author) return "other"; + return "human"; +} + +function detectSeverity(body: string): ReviewThreadSnapshot["severity"] { + const normalized = body.toLowerCase(); + if (normalized.includes("critical") || normalized.includes("security")) return "high"; + if (normalized.includes("error") || normalized.includes("bug") || normalized.includes("fail")) { + return "medium"; + } + if (normalized.includes("nit") || normalized.includes("style")) return "low"; + return "unknown"; +} + // --------------------------------------------------------------------------- // SCM implementation // --------------------------------------------------------------------------- @@ -768,6 +789,148 @@ function createGitHubSCM(): SCM { return "none"; }, + async getPRHeadSha(pr: PRInfo): Promise { + const raw = await gh([ + "pr", + "view", + String(pr.number), + "--repo", + repoFlag(pr), + "--json", + "headRefOid", + ]); + const data: { headRefOid?: string } = JSON.parse(raw); + if (!data.headRefOid) throw new Error("Failed to resolve PR head SHA"); + return data.headRefOid; + }, + + async getReviewThreadSnapshots(pr: PRInfo): Promise { + const raw = await gh([ + "api", + "graphql", + "-f", + `owner=${pr.owner}`, + "-f", + `name=${pr.repo}`, + "-F", + `number=${pr.number}`, + "-f", + `query=query($owner: String!, $name: String!, $number: Int!) { + repository(owner: $owner, name: $name) { + pullRequest(number: $number) { + reviewThreads(first: 100) { + nodes { + id + isResolved + comments(first: 1) { + nodes { + body + path + createdAt + author { login } + } + } + } + } + } + } + }`, + ]); + + const data: { + data: { + repository: { + pullRequest: { + reviewThreads: { + nodes: Array<{ + id: string; + isResolved: boolean; + comments: { + nodes: Array<{ + body: string; + path: string | null; + createdAt: string; + author: { login: string } | null; + }>; + }; + }>; + }; + }; + }; + }; + } = JSON.parse(raw); + + const threads = data.data.repository.pullRequest.reviewThreads.nodes; + const snapshots: ReviewThreadSnapshot[] = []; + for (const thread of threads) { + const comment = thread.comments.nodes[0]; + if (!comment) continue; + const author = comment.author?.login ?? ""; + snapshots.push({ + prNumber: pr.number, + threadId: thread.id, + source: detectThreadSource(author), + path: comment.path ?? undefined, + bodyHash: createHash("sha256").update(comment.body).digest("hex").slice(0, 16), + severity: detectSeverity(comment.body), + status: thread.isResolved ? "resolved" : "open", + capturedAt: new Date(), + }); + } + return snapshots; + }, + + async resolveReviewThread(_pr: PRInfo, threadId: string): Promise { + await gh([ + "api", + "graphql", + "-f", + `query=mutation($threadId:ID!){resolveReviewThread(input:{threadId:$threadId}){thread{id isResolved}}}`, + "-f", + `threadId=${threadId}`, + ]); + }, + + async publishCheckRun(input: SCMCheckRunInput): Promise { + const headSha = await this.getPRHeadSha?.(input.pr); + if (!headSha) return; + await gh([ + "api", + "--method", + "POST", + "-H", + "Accept: application/vnd.github+json", + `repos/${repoFlag(input.pr)}/check-runs`, + "-f", + `name=${input.name}`, + "-f", + `head_sha=${headSha}`, + "-f", + `status=${input.status}`, + "-f", + `conclusion=${input.conclusion}`, + "-f", + `output[title]=${input.name}`, + "-f", + `output[summary]=${input.summary}`, + "-f", + `output[text]=${input.text ?? input.summary}`, + ]).catch(async () => { + await gh([ + "api", + "--method", + "POST", + `repos/${repoFlag(input.pr)}/statuses/${headSha}`, + "-f", + `state=${input.conclusion === "success" ? "success" : "failure"}`, + "-f", + `context=${input.name}`, + "-f", + `description=${input.summary}`, + ]); + }); + }, + async getPendingComments(pr: PRInfo): Promise { try { // Use GraphQL with variables to get review threads with actual isResolved status diff --git a/packages/plugins/scm-github/test/index.test.ts b/packages/plugins/scm-github/test/index.test.ts index c910cffed..a1fa666f4 100644 --- a/packages/plugins/scm-github/test/index.test.ts +++ b/packages/plugins/scm-github/test/index.test.ts @@ -905,6 +905,51 @@ describe("scm-github plugin", () => { }); }); + describe("getReviewThreadSnapshots", () => { + function makeSnapshotResponse(author: string) { + return { + data: { + repository: { + pullRequest: { + reviewThreads: { + nodes: [ + { + id: "thread-1", + isResolved: false, + comments: { + nodes: [ + { + body: "Potential issue", + path: "src/a.ts", + createdAt: "2025-01-01T00:00:00Z", + author: author ? { login: author } : null, + }, + ], + }, + }, + ], + }, + }, + }, + }, + }; + } + + it("classifies cursor bot review threads as bugbot", async () => { + mockGh(makeSnapshotResponse("cursor[bot]")); + const threads = await scm.getReviewThreadSnapshots?.(pr); + expect(threads).toHaveLength(1); + expect(threads?.[0]?.source).toBe("bugbot"); + }); + + it("classifies non-review bots as other", async () => { + mockGh(makeSnapshotResponse("dependabot[bot]")); + const threads = await scm.getReviewThreadSnapshots?.(pr); + expect(threads).toHaveLength(1); + expect(threads?.[0]?.source).toBe("other"); + }); + }); + // ---- getAutomatedComments ---------------------------------------------- describe("getAutomatedComments", () => { diff --git a/packages/web/src/__tests__/api-routes.test.ts b/packages/web/src/__tests__/api-routes.test.ts index 44c5fd2f2..68458df54 100644 --- a/packages/web/src/__tests__/api-routes.test.ts +++ b/packages/web/src/__tests__/api-routes.test.ts @@ -3,7 +3,6 @@ import { NextRequest } from "next/server"; import { SessionNotFoundError, SessionNotRestorableError, - SessionNotFoundError, type Session, type SessionManager, type OrchestratorConfig, @@ -126,16 +125,23 @@ const mockSessionManager: SessionManager = { }), }; -const mockSCM: SCM = { +const mockSCM = { name: "github", detectPR: vi.fn(async () => null), getPRState: vi.fn(async () => "open" as const), mergePR: vi.fn(async () => {}), closePR: vi.fn(async () => {}), - getCIChecks: vi.fn(async () => []), + getCIChecks: vi.fn(async () => [ + { name: "review-integrity", status: "passed" as const }, + { name: "ao/merge-guard", status: "passed" as const }, + ]), getCISummary: vi.fn(async () => "passing" as const), getReviews: vi.fn(async () => []), getReviewDecision: vi.fn(async () => "approved" as const), + getPRHeadSha: vi.fn(async () => "abc123"), + getReviewThreadSnapshots: vi.fn(async () => []), + resolveReviewThread: vi.fn(async () => {}), + publishCheckRun: vi.fn(async () => {}), getPendingComments: vi.fn(async () => []), getAutomatedComments: vi.fn(async () => []), getMergeability: vi.fn(async () => ({ @@ -145,6 +151,11 @@ const mockSCM: SCM = { noConflicts: true, blockers: [], })), +} as SCM & { + getPRHeadSha: ReturnType; + getReviewThreadSnapshots: ReturnType; + resolveReviewThread: ReturnType; + publishCheckRun: ReturnType; }; const mockRegistry: PluginRegistry = { @@ -202,6 +213,10 @@ import { POST as killPOST } from "@/app/api/sessions/[id]/kill/route"; import { POST as restorePOST } from "@/app/api/sessions/[id]/restore/route"; import { POST as remapPOST } from "@/app/api/sessions/[id]/remap/route"; import { POST as mergePOST } from "@/app/api/prs/[id]/merge/route"; +import { GET as reviewThreadsGET } from "@/app/api/prs/[id]/review-threads/route"; +import { POST as reviewResolutionsPOST } from "@/app/api/prs/[id]/review-resolutions/route"; +import { POST as reviewVerifyPOST } from "@/app/api/prs/[id]/review-resolutions/verify/route"; +import { POST as reviewApplyPOST } from "@/app/api/prs/[id]/review-resolutions/apply/route"; import { GET as eventsGET } from "@/app/api/events/route"; import { GET as observabilityGET } from "@/app/api/observability/route"; @@ -686,6 +701,70 @@ describe("API Routes", () => { expect(data.blockers).toBeDefined(); }); + it("returns 422 when merge guard fails due to missing resolution records", async () => { + (mockSCM.getMergeability as ReturnType).mockResolvedValueOnce({ + mergeable: true, + ciPassing: true, + approved: true, + noConflicts: true, + blockers: [], + }); + (mockSCM.getReviewThreadSnapshots as ReturnType).mockResolvedValueOnce([ + { + prNumber: 432, + threadId: "thread-1", + source: "human", + bodyHash: "hash", + severity: "medium", + status: "resolved", + capturedAt: new Date(), + }, + ]); + (mockSCM.getCIChecks as ReturnType).mockResolvedValueOnce([ + { name: "review-integrity", status: "passed" }, + { name: "ao/merge-guard", status: "passed" }, + ]); + (mockSCM.getPRHeadSha as ReturnType).mockResolvedValueOnce("abc123"); + + const req = makeRequest("/api/prs/432/merge", { method: "POST" }); + const res = await mergePOST(req, { params: Promise.resolve({ id: "432" }) }); + expect(res.status).toBe(422); + const data = await res.json(); + expect(data.reviewIntegrityStatus).toBe("fail"); + expect(data.guardBlockers?.length).toBeGreaterThan(0); + expect( + data.guardBlockers.some( + (b: { code: string; checkName?: string }) => + b.code === "REQUIRED_CHECK_NOT_PASSING" && b.checkName === "ao/merge-guard", + ), + ).toBe(false); + }); + + it("fails closed when SCM lacks full review thread snapshots", async () => { + const original = mockSCM.getReviewThreadSnapshots; + mockSCM.getReviewThreadSnapshots = undefined as unknown as ReturnType; + (mockSCM.getMergeability as ReturnType).mockResolvedValueOnce({ + mergeable: true, + ciPassing: true, + approved: true, + noConflicts: true, + blockers: [], + }); + (mockSCM.getCIChecks as ReturnType).mockResolvedValueOnce([ + { name: "review-integrity", status: "passed" }, + { name: "ao/merge-guard", status: "passed" }, + ]); + + const req = makeRequest("/api/prs/432/merge", { method: "POST" }); + const res = await mergePOST(req, { params: Promise.resolve({ id: "432" }) }); + expect(res.status).toBe(422); + const data = await res.json(); + expect( + data.guardBlockers.some((b: { code: string }) => b.code === "THREAD_SNAPSHOTS_UNAVAILABLE"), + ).toBe(true); + mockSCM.getReviewThreadSnapshots = original; + }); + it("returns 400 for non-numeric PR id", async () => { const req = makeRequest("/api/prs/abc/merge", { method: "POST" }); const res = await mergePOST(req, { params: Promise.resolve({ id: "abc" }) }); @@ -704,6 +783,200 @@ describe("API Routes", () => { }); }); + describe("Review integrity routes", () => { + it("GET /api/prs/:id/review-threads returns thread snapshots", async () => { + (mockSCM.getReviewThreadSnapshots as ReturnType).mockResolvedValueOnce([ + { + prNumber: 432, + threadId: "thread-1", + source: "human", + bodyHash: "hash", + severity: "medium", + status: "open", + capturedAt: new Date("2026-03-12T00:00:00Z"), + }, + ]); + const req = makeRequest("/api/prs/432/review-threads", { method: "GET" }); + const res = await reviewThreadsGET(req, { params: Promise.resolve({ id: "432" }) }); + expect(res.status).toBe(200); + const data = await res.json(); + expect(data.threads).toHaveLength(1); + expect(data.threads[0].threadId).toBe("thread-1"); + }); + + it("POST propose/verify/apply flow works", async () => { + (mockSCM.getReviewThreadSnapshots as ReturnType).mockResolvedValue([ + { + prNumber: 432, + threadId: "thread-verify", + source: "human", + bodyHash: "hash", + severity: "medium", + status: "resolved", + capturedAt: new Date("2026-03-12T00:00:00Z"), + }, + ]); + (mockSCM.getPRHeadSha as ReturnType).mockResolvedValue("abc123"); + + const createReq = makeRequest("/api/prs/432/review-resolutions", { + method: "POST", + headers: { "Content-Type": "application/json" }, + body: JSON.stringify({ + threadId: "thread-verify", + resolutionType: "fixed", + fixCommitSha: "abc123", + evidence: { + changedFiles: ["src/a.ts"], + testCommands: ["pnpm test"], + testResults: ["pass"], + }, + }), + }); + const createRes = await reviewResolutionsPOST(createReq, { + params: Promise.resolve({ id: "432" }), + }); + expect(createRes.status).toBe(201); + + const verifyReq = makeRequest("/api/prs/432/review-resolutions/verify", { + method: "POST", + headers: { "Content-Type": "application/json" }, + body: JSON.stringify({ threadId: "thread-verify" }), + }); + const verifyRes = await reviewVerifyPOST(verifyReq, { + params: Promise.resolve({ id: "432" }), + }); + expect(verifyRes.status).toBe(200); + const verifyData = await verifyRes.json(); + expect(verifyData.verificationStatus).toBe("pass"); + + const applyReq = makeRequest("/api/prs/432/review-resolutions/apply", { + method: "POST", + headers: { "Content-Type": "application/json" }, + body: JSON.stringify({ threadId: "thread-verify" }), + }); + const applyRes = await reviewApplyPOST(applyReq, { params: Promise.resolve({ id: "432" }) }); + expect(applyRes.status).toBe(200); + }); + + it("POST /api/prs/:id/review-resolutions rejects invalid resolutionType", async () => { + const req = makeRequest("/api/prs/432/review-resolutions", { + method: "POST", + headers: { "Content-Type": "application/json" }, + body: JSON.stringify({ + threadId: "thread-invalid-type", + resolutionType: "custom", + }), + }); + + const res = await reviewResolutionsPOST(req, { params: Promise.resolve({ id: "432" }) }); + expect(res.status).toBe(400); + const data = await res.json(); + expect(data.error).toMatch(/resolutionType must be one of/); + }); + + it("POST verify fails closed when SCM lacks full review thread snapshots", async () => { + const createReq = makeRequest("/api/prs/432/review-resolutions", { + method: "POST", + headers: { "Content-Type": "application/json" }, + body: JSON.stringify({ + threadId: "thread-missing-snapshots-verify", + resolutionType: "not_actionable", + rationale: "Handled via external dependency limitations", + }), + }); + const createRes = await reviewResolutionsPOST(createReq, { + params: Promise.resolve({ id: "432" }), + }); + expect(createRes.status).toBe(201); + + const original = mockSCM.getReviewThreadSnapshots; + mockSCM.getReviewThreadSnapshots = undefined as unknown as ReturnType; + + const verifyReq = makeRequest("/api/prs/432/review-resolutions/verify", { + method: "POST", + headers: { "Content-Type": "application/json" }, + body: JSON.stringify({ threadId: "thread-missing-snapshots-verify" }), + }); + const verifyRes = await reviewVerifyPOST(verifyReq, { + params: Promise.resolve({ id: "432" }), + }); + + expect(verifyRes.status).toBe(422); + const data = await verifyRes.json(); + expect(data.error).toMatch(/full review thread snapshots/); + expect( + data.blockers.some((b: { code: string }) => b.code === "THREAD_SNAPSHOTS_UNAVAILABLE"), + ).toBe(true); + + mockSCM.getReviewThreadSnapshots = original; + }); + + it("POST apply fails closed when SCM lacks full review thread snapshots", async () => { + (mockSCM.getReviewThreadSnapshots as ReturnType).mockResolvedValueOnce([ + { + prNumber: 432, + threadId: "thread-missing-snapshots-apply", + source: "human", + bodyHash: "hash", + severity: "medium", + status: "resolved", + capturedAt: new Date("2026-03-12T00:00:00Z"), + }, + ]); + (mockSCM.getPRHeadSha as ReturnType).mockResolvedValue("abc123"); + + const createReq = makeRequest("/api/prs/432/review-resolutions", { + method: "POST", + headers: { "Content-Type": "application/json" }, + body: JSON.stringify({ + threadId: "thread-missing-snapshots-apply", + resolutionType: "fixed", + fixCommitSha: "abc123", + evidence: { + changedFiles: ["src/a.ts"], + testCommands: ["pnpm test"], + testResults: ["pass"], + }, + }), + }); + const createRes = await reviewResolutionsPOST(createReq, { + params: Promise.resolve({ id: "432" }), + }); + expect(createRes.status).toBe(201); + + const verifyReq = makeRequest("/api/prs/432/review-resolutions/verify", { + method: "POST", + headers: { "Content-Type": "application/json" }, + body: JSON.stringify({ threadId: "thread-missing-snapshots-apply" }), + }); + const verifyRes = await reviewVerifyPOST(verifyReq, { + params: Promise.resolve({ id: "432" }), + }); + expect(verifyRes.status).toBe(200); + + const original = mockSCM.getReviewThreadSnapshots; + mockSCM.getReviewThreadSnapshots = undefined as unknown as ReturnType; + + const applyReq = makeRequest("/api/prs/432/review-resolutions/apply", { + method: "POST", + headers: { "Content-Type": "application/json" }, + body: JSON.stringify({ threadId: "thread-missing-snapshots-apply" }), + }); + const applyRes = await reviewApplyPOST(applyReq, { + params: Promise.resolve({ id: "432" }), + }); + + expect(applyRes.status).toBe(422); + const data = await applyRes.json(); + expect(data.error).toMatch(/full review thread snapshots/); + expect( + data.blockers.some((b: { code: string }) => b.code === "THREAD_SNAPSHOTS_UNAVAILABLE"), + ).toBe(true); + + mockSCM.getReviewThreadSnapshots = original; + }); + }); + // ── GET /api/events (SSE) ────────────────────────────────────────── describe("GET /api/events", () => { diff --git a/packages/web/src/app/api/prs/[id]/merge/route.ts b/packages/web/src/app/api/prs/[id]/merge/route.ts index 9ab5aed1a..c86c56a92 100644 --- a/packages/web/src/app/api/prs/[id]/merge/route.ts +++ b/packages/web/src/app/api/prs/[id]/merge/route.ts @@ -1,6 +1,13 @@ +import type { SCM } from "@composio/ao-core"; import { type NextRequest } from "next/server"; import { getServices, getSCM } from "@/lib/services"; import { getCorrelationId, jsonWithCorrelation, recordApiObservation } from "@/lib/observability"; +import { + REVIEW_INTEGRITY_DEFAULTS, + evaluateMergeGuardForPR, + getReviewResolutionStore, + publishGuardChecks, +} from "@/lib/review-integrity"; /** POST /api/prs/:id/merge — Merge a PR */ export async function POST(_request: NextRequest, { params }: { params: Promise<{ id: string }> }) { @@ -22,7 +29,7 @@ export async function POST(_request: NextRequest, { params }: { params: Promise< } const project = config.projects[session.projectId]; - const scm = getSCM(registry, project); + const scm = getSCM(registry, project) as SCM | null; if (!scm) { return jsonWithCorrelation( { error: "No SCM plugin configured for this project" }, @@ -42,9 +49,27 @@ export async function POST(_request: NextRequest, { params }: { params: Promise< } const mergeability = await scm.getMergeability(session.pr); - if (!mergeability.mergeable) { + const store = getReviewResolutionStore(config, project); + const recordsByThread = store.latestByThread(prNumber); + const { integrity, guard } = await evaluateMergeGuardForPR({ + scm, + pr: session.pr, + recordsByThread, + requiredChecks: [...REVIEW_INTEGRITY_DEFAULTS.requiredChecks], + requireEvidenceForBotThreads: REVIEW_INTEGRITY_DEFAULTS.requireEvidenceForBotThreads, + reverifyOnNewCommits: REVIEW_INTEGRITY_DEFAULTS.reverifyOnNewCommits, + }); + + await publishGuardChecks(scm, session.pr, integrity, guard).catch(() => {}); + + if (!mergeability.mergeable || !guard.allowMerge) { return jsonWithCorrelation( - { error: "PR is not mergeable", blockers: mergeability.blockers }, + { + error: "PR is not mergeable", + blockers: [...mergeability.blockers, ...guard.blockers.map((b) => b.message)], + guardBlockers: guard.blockers, + reviewIntegrityStatus: integrity.status, + }, { status: 422 }, correlationId, ); diff --git a/packages/web/src/app/api/prs/[id]/review-resolutions/apply/route.ts b/packages/web/src/app/api/prs/[id]/review-resolutions/apply/route.ts new file mode 100644 index 000000000..c7054cc18 --- /dev/null +++ b/packages/web/src/app/api/prs/[id]/review-resolutions/apply/route.ts @@ -0,0 +1,131 @@ +import type { SCM } from "@composio/ao-core"; +import { type NextRequest } from "next/server"; +import { getSCM, getServices } from "@/lib/services"; +import { getCorrelationId, jsonWithCorrelation } from "@/lib/observability"; +import { + REVIEW_INTEGRITY_DEFAULTS, + getReviewResolutionStore, + getThreadSnapshots, +} from "@/lib/review-integrity"; + +export async function POST(_request: NextRequest, { params }: { params: Promise<{ id: string }> }) { + const correlationId = getCorrelationId(_request); + const { id } = await params; + if (!/^\d+$/.test(id)) { + return jsonWithCorrelation({ error: "Invalid PR number" }, { status: 400 }, correlationId); + } + const prNumber = Number(id); + + let body: unknown; + try { + body = await _request.json(); + } catch { + return jsonWithCorrelation({ error: "Invalid JSON body" }, { status: 400 }, correlationId); + } + + const payload = body as { threadId?: string }; + if (!payload.threadId) { + return jsonWithCorrelation({ error: "threadId is required" }, { status: 400 }, correlationId); + } + + const { config, registry, sessionManager } = await getServices(); + const sessions = await sessionManager.list(); + const session = sessions.find((s) => s.pr?.number === prNumber); + if (!session?.pr) { + return jsonWithCorrelation({ error: "PR not found" }, { status: 404 }, correlationId); + } + + const project = config.projects[session.projectId]; + const scm = getSCM(registry, project) as SCM | null; + if (!scm) { + return jsonWithCorrelation( + { error: "No SCM plugin configured for this project" }, + { status: 500 }, + correlationId, + ); + } + + const store = getReviewResolutionStore(config, project); + const latest = store.latestByThread(prNumber).get(payload.threadId); + if (!latest) { + return jsonWithCorrelation( + { error: "Resolution record not found" }, + { status: 404 }, + correlationId, + ); + } + + if (latest.verificationStatus !== "pass") { + return jsonWithCorrelation( + { + error: "Resolution must be verified before apply", + verificationStatus: latest.verificationStatus, + }, + { status: 422 }, + correlationId, + ); + } + + if (REVIEW_INTEGRITY_DEFAULTS.reverifyOnNewCommits && scm.getPRHeadSha) { + const currentHeadSha = await scm.getPRHeadSha(session.pr); + if (!latest.verifiedHeadSha || latest.verifiedHeadSha !== currentHeadSha) { + return jsonWithCorrelation( + { + error: "Resolution verification is stale; re-verify required", + verifiedHeadSha: latest.verifiedHeadSha ?? null, + currentHeadSha, + }, + { status: 422 }, + correlationId, + ); + } + } + + if (!scm.getReviewThreadSnapshots) { + return jsonWithCorrelation( + { + error: "SCM does not support full review thread snapshots", + blockers: [ + { + code: "THREAD_SNAPSHOTS_UNAVAILABLE", + message: "SCM does not support full review thread snapshots", + }, + ], + }, + { status: 422 }, + correlationId, + ); + } + + const threads = await getThreadSnapshots(scm, session.pr); + if (!threads.some((thread) => thread.threadId === payload.threadId)) { + return jsonWithCorrelation( + { error: "Review thread not found on PR" }, + { status: 422 }, + correlationId, + ); + } + + if (scm.resolveReviewThread) { + await scm.resolveReviewThread(session.pr, payload.threadId); + } + + const { id: _id, createdAt: _createdAt, ...base } = latest; + const applied = store.persist({ + ...base, + appliedAt: new Date(), + }); + + return jsonWithCorrelation( + { + ok: true, + resolution: { + ...applied, + createdAt: applied.createdAt.toISOString(), + appliedAt: applied.appliedAt?.toISOString(), + }, + }, + { status: 200 }, + correlationId, + ); +} diff --git a/packages/web/src/app/api/prs/[id]/review-resolutions/route.ts b/packages/web/src/app/api/prs/[id]/review-resolutions/route.ts new file mode 100644 index 000000000..bbf31f561 --- /dev/null +++ b/packages/web/src/app/api/prs/[id]/review-resolutions/route.ts @@ -0,0 +1,100 @@ +import { type NextRequest } from "next/server"; +import { getCorrelationId, jsonWithCorrelation } from "@/lib/observability"; +import { getServices } from "@/lib/services"; +import { + REVIEW_INTEGRITY_DEFAULTS, + buildResolutionRecordInput, + getReviewResolutionStore, +} from "@/lib/review-integrity"; + +const RESOLUTION_TYPES = ["fixed", "already_fixed", "not_actionable", "duplicate"] as const; + +function isResolutionType(value: unknown): value is (typeof RESOLUTION_TYPES)[number] { + return ( + typeof value === "string" && + RESOLUTION_TYPES.includes(value as (typeof RESOLUTION_TYPES)[number]) + ); +} + +export async function POST(_request: NextRequest, { params }: { params: Promise<{ id: string }> }) { + const correlationId = getCorrelationId(_request); + const { id } = await params; + if (!/^\d+$/.test(id)) { + return jsonWithCorrelation({ error: "Invalid PR number" }, { status: 400 }, correlationId); + } + const prNumber = Number(id); + + let body: unknown; + try { + body = await _request.json(); + } catch { + return jsonWithCorrelation({ error: "Invalid JSON body" }, { status: 400 }, correlationId); + } + + const payload = body as { + threadId?: string; + resolutionType?: unknown; + actorId?: string; + fixCommitSha?: string; + rationale?: string; + evidence?: { + changedFiles?: string[]; + testCommands?: string[]; + testResults?: string[]; + }; + }; + + if (!payload.threadId) { + return jsonWithCorrelation({ error: "threadId is required" }, { status: 400 }, correlationId); + } + if (!payload.resolutionType) { + return jsonWithCorrelation( + { error: "resolutionType is required" }, + { status: 400 }, + correlationId, + ); + } + if (!isResolutionType(payload.resolutionType)) { + return jsonWithCorrelation( + { error: `resolutionType must be one of: ${RESOLUTION_TYPES.join(", ")}` }, + { status: 400 }, + correlationId, + ); + } + + const { config, sessionManager } = await getServices(); + const sessions = await sessionManager.list(); + const session = sessions.find((s) => s.pr?.number === prNumber); + if (!session?.pr) { + return jsonWithCorrelation({ error: "PR not found" }, { status: 404 }, correlationId); + } + + const project = config.projects[session.projectId]; + const store = getReviewResolutionStore(config, project); + + const created = store.persist( + buildResolutionRecordInput({ + prNumber, + threadId: payload.threadId, + resolutionType: payload.resolutionType, + actorId: payload.actorId ?? "ao-web", + fixCommitSha: payload.fixCommitSha, + rationale: payload.rationale, + evidence: payload.evidence, + }), + ); + + return jsonWithCorrelation( + { + ok: true, + defaults: REVIEW_INTEGRITY_DEFAULTS, + resolution: { + ...created, + createdAt: created.createdAt.toISOString(), + appliedAt: created.appliedAt?.toISOString(), + }, + }, + { status: 201 }, + correlationId, + ); +} diff --git a/packages/web/src/app/api/prs/[id]/review-resolutions/verify/route.ts b/packages/web/src/app/api/prs/[id]/review-resolutions/verify/route.ts new file mode 100644 index 000000000..a2df84e48 --- /dev/null +++ b/packages/web/src/app/api/prs/[id]/review-resolutions/verify/route.ts @@ -0,0 +1,120 @@ +import type { SCM } from "@composio/ao-core"; +import { type NextRequest } from "next/server"; +import { getSCM, getServices } from "@/lib/services"; +import { getCorrelationId, jsonWithCorrelation } from "@/lib/observability"; +import { + REVIEW_INTEGRITY_DEFAULTS, + getReviewResolutionStore, + getThreadSnapshots, + validateResolutionWithGit, +} from "@/lib/review-integrity"; + +export async function POST(_request: NextRequest, { params }: { params: Promise<{ id: string }> }) { + const correlationId = getCorrelationId(_request); + const { id } = await params; + if (!/^\d+$/.test(id)) { + return jsonWithCorrelation({ error: "Invalid PR number" }, { status: 400 }, correlationId); + } + const prNumber = Number(id); + + let body: unknown; + try { + body = await _request.json(); + } catch { + return jsonWithCorrelation({ error: "Invalid JSON body" }, { status: 400 }, correlationId); + } + const payload = body as { threadId?: string }; + if (!payload.threadId) { + return jsonWithCorrelation({ error: "threadId is required" }, { status: 400 }, correlationId); + } + + const { config, registry, sessionManager } = await getServices(); + const sessions = await sessionManager.list(); + const session = sessions.find((s) => s.pr?.number === prNumber); + if (!session?.pr) { + return jsonWithCorrelation({ error: "PR not found" }, { status: 404 }, correlationId); + } + + const project = config.projects[session.projectId]; + const scm = getSCM(registry, project) as SCM | null; + if (!scm) { + return jsonWithCorrelation( + { error: "No SCM plugin configured for this project" }, + { status: 500 }, + correlationId, + ); + } + + const store = getReviewResolutionStore(config, project); + const latest = store.latestByThread(prNumber).get(payload.threadId); + if (!latest) { + return jsonWithCorrelation( + { error: "Resolution record not found" }, + { status: 404 }, + correlationId, + ); + } + + if (!scm.getReviewThreadSnapshots) { + return jsonWithCorrelation( + { + error: "SCM does not support full review thread snapshots", + blockers: [ + { + code: "THREAD_SNAPSHOTS_UNAVAILABLE", + message: "SCM does not support full review thread snapshots", + }, + ], + }, + { status: 422 }, + correlationId, + ); + } + + const threads = await getThreadSnapshots(scm, session.pr); + const thread = threads.find((t) => t.threadId === payload.threadId); + if (!thread) { + return jsonWithCorrelation( + { error: "Review thread not found on PR" }, + { status: 422 }, + correlationId, + ); + } + const headSha = scm.getPRHeadSha ? await scm.getPRHeadSha(session.pr) : undefined; + + const verificationNotes = await validateResolutionWithGit( + { + ...latest, + verifiedHeadSha: headSha, + }, + thread, + { + workspacePath: session.workspacePath ?? undefined, + headSha, + requireEvidenceForBotThreads: REVIEW_INTEGRITY_DEFAULTS.requireEvidenceForBotThreads, + }, + ); + + const { id: _id, createdAt: _createdAt, ...base } = latest; + const verified = store.persist({ + ...base, + verificationStatus: verificationNotes.length === 0 ? "pass" : "fail", + verificationNotes, + verifiedHeadSha: headSha, + }); + + return jsonWithCorrelation( + { + ok: verified.verificationStatus === "pass", + verificationStatus: verified.verificationStatus, + blockers: verificationNotes, + resolution: { + ...verified, + createdAt: verified.createdAt.toISOString(), + appliedAt: verified.appliedAt?.toISOString(), + }, + }, + { status: 200 }, + correlationId, + ); +} diff --git a/packages/web/src/app/api/prs/[id]/review-threads/route.ts b/packages/web/src/app/api/prs/[id]/review-threads/route.ts new file mode 100644 index 000000000..bed423a74 --- /dev/null +++ b/packages/web/src/app/api/prs/[id]/review-threads/route.ts @@ -0,0 +1,45 @@ +import type { SCM } from "@composio/ao-core"; +import { type NextRequest } from "next/server"; +import { getSCM, getServices } from "@/lib/services"; +import { getCorrelationId, jsonWithCorrelation } from "@/lib/observability"; +import { getThreadSnapshots } from "@/lib/review-integrity"; + +export async function GET(_request: NextRequest, { params }: { params: Promise<{ id: string }> }) { + const correlationId = getCorrelationId(_request); + const { id } = await params; + if (!/^\d+$/.test(id)) { + return jsonWithCorrelation({ error: "Invalid PR number" }, { status: 400 }, correlationId); + } + + const prNumber = Number(id); + const { config, registry, sessionManager } = await getServices(); + const sessions = await sessionManager.list(); + const session = sessions.find((s) => s.pr?.number === prNumber); + + if (!session?.pr) { + return jsonWithCorrelation({ error: "PR not found" }, { status: 404 }, correlationId); + } + + const project = config.projects[session.projectId]; + const scm = getSCM(registry, project) as SCM | null; + if (!scm) { + return jsonWithCorrelation( + { error: "No SCM plugin configured for this project" }, + { status: 500 }, + correlationId, + ); + } + + const threads = await getThreadSnapshots(scm, session.pr); + return jsonWithCorrelation( + { + prNumber, + threads: threads.map((thread) => ({ + ...thread, + capturedAt: thread.capturedAt.toISOString(), + })), + }, + { status: 200 }, + correlationId, + ); +} diff --git a/packages/web/src/lib/review-integrity.ts b/packages/web/src/lib/review-integrity.ts new file mode 100644 index 000000000..12faca54e --- /dev/null +++ b/packages/web/src/lib/review-integrity.ts @@ -0,0 +1,272 @@ +import { execFile } from "node:child_process"; +import { homedir } from "node:os"; +import { basename, join } from "node:path"; +import { promisify } from "node:util"; +import { + ReviewResolutionStore, + createResolutionRecord, + evaluateMergeGuard, + evaluateReviewIntegrity, + getReviewIntegrityDir, + validateResolutionRecord, + type CICheck, + type MergeGuardEvaluation, + type OrchestratorConfig, + type PRInfo, + type ProjectConfig, + type ResolutionRecord, + type ResolutionType, + type ReviewThreadSnapshot, + type SCM, +} from "@composio/ao-core"; + +const execFileAsync = promisify(execFile); + +export const REVIEW_INTEGRITY_DEFAULTS = { + requireEvidenceForBotThreads: true, + requiredChecks: ["review-integrity", "ao/merge-guard"], + reverifyOnNewCommits: true, +} as const; + +export function getReviewResolutionStore( + config: OrchestratorConfig, + project: ProjectConfig, +): ReviewResolutionStore { + const dir = (() => { + try { + return getReviewIntegrityDir(config.configPath, project.path); + } catch { + return join( + homedir(), + ".agent-orchestrator", + "review-integrity-fallback", + basename(project.path), + ); + } + })(); + return new ReviewResolutionStore(dir); +} + +export async function getThreadSnapshots(scm: SCM, pr: PRInfo): Promise { + if (scm.getReviewThreadSnapshots) { + return scm.getReviewThreadSnapshots(pr); + } + + throw new Error("SCM does not support full review thread snapshots"); +} + +function normalizeCheckState(status: CICheck["status"]): "passed" | "pending" | "failed" { + if (status === "passed") return "passed"; + if (status === "pending" || status === "running") return "pending"; + return "failed"; +} + +function buildCheckConclusions(checks: CICheck[]): Map { + const map = new Map(); + for (const check of checks) { + map.set(check.name, normalizeCheckState(check.status)); + } + return map; +} + +async function gitInDir(args: string[], cwd: string): Promise { + const { stdout } = await execFileAsync("git", args, { + cwd, + timeout: 30_000, + maxBuffer: 1024 * 1024, + }); + return stdout.trim(); +} + +async function isCommitReachable(workspacePath: string, commitSha: string): Promise { + try { + await gitInDir(["merge-base", "--is-ancestor", commitSha, "HEAD"], workspacePath); + return true; + } catch { + return false; + } +} + +async function getCommitTimestamp(workspacePath: string, commitSha: string): Promise { + try { + const raw = await gitInDir(["show", "-s", "--format=%cI", commitSha], workspacePath); + const parsed = new Date(raw); + return Number.isNaN(parsed.getTime()) ? null : parsed; + } catch { + return null; + } +} + +function fallbackThread(record: ResolutionRecord): ReviewThreadSnapshot { + return { + prNumber: record.prNumber, + threadId: record.threadId, + source: "other", + bodyHash: "unknown", + severity: "unknown", + status: "resolved", + capturedAt: new Date(), + }; +} + +export async function validateResolutionWithGit( + record: ResolutionRecord, + thread: ReviewThreadSnapshot | undefined, + opts: { + workspacePath?: string; + headSha?: string; + requireEvidenceForBotThreads?: boolean; + }, +): Promise { + const workspacePath = opts.workspacePath; + const gitReachable = new Map(); + const gitTimestamps = new Map(); + + if (workspacePath && record.fixCommitSha) { + gitReachable.set( + record.fixCommitSha, + await isCommitReachable(workspacePath, record.fixCommitSha), + ); + if (record.resolutionType === "already_fixed") { + gitTimestamps.set( + record.fixCommitSha, + await getCommitTimestamp(workspacePath, record.fixCommitSha), + ); + } + } + + const gitValidationOptions = + workspacePath && record.fixCommitSha + ? { + isCommitReachable: (sha: string) => gitReachable.get(sha) ?? false, + getCommitTimestamp: (sha: string) => gitTimestamps.get(sha) ?? null, + } + : {}; + + const blockers = validateResolutionRecord(record, thread ?? fallbackThread(record), { + currentHeadSha: opts.headSha, + requireEvidenceForBotThreads: opts.requireEvidenceForBotThreads, + ...gitValidationOptions, + }); + + return [...new Set(blockers)]; +} + +export async function evaluateMergeGuardForPR(input: { + scm: SCM; + pr: PRInfo; + recordsByThread: Map; + requiredChecks?: string[]; + requireEvidenceForBotThreads?: boolean; + reverifyOnNewCommits?: boolean; +}): Promise<{ + integrity: ReturnType; + guard: MergeGuardEvaluation; +}> { + const requiredChecks = [ + ...(input.requiredChecks ?? REVIEW_INTEGRITY_DEFAULTS.requiredChecks), + ].filter((name) => name !== "ao/merge-guard" && name !== "review-integrity"); + + if (!input.scm.getReviewThreadSnapshots) { + const checks = await input.scm.getCIChecks(input.pr); + const checkConclusions = buildCheckConclusions(checks); + const integrity = { + status: "fail" as const, + unresolvedThreadCount: 0, + unverifiedResolvedThreadCount: 0, + blockers: [ + { + code: "THREAD_SNAPSHOTS_UNAVAILABLE" as const, + message: "SCM does not support full review thread snapshots", + }, + ], + }; + + const guard = evaluateMergeGuard({ + integrity, + requiredChecks, + checkConclusions, + }); + + return { integrity, guard }; + } + + const threadSnapshots = await getThreadSnapshots(input.scm, input.pr); + const checks = await input.scm.getCIChecks(input.pr); + const checkConclusions = buildCheckConclusions(checks); + + const headSha = input.scm.getPRHeadSha ? await input.scm.getPRHeadSha(input.pr) : undefined; + const integrity = evaluateReviewIntegrity(threadSnapshots, input.recordsByThread, { + currentHeadSha: input.reverifyOnNewCommits ? headSha : undefined, + requireEvidenceForBotThreads: input.requireEvidenceForBotThreads, + }); + + const guard = evaluateMergeGuard({ + integrity, + requiredChecks, + checkConclusions, + }); + + return { integrity, guard }; +} + +export async function publishGuardChecks( + scm: SCM, + pr: PRInfo, + integrity: ReturnType, + guard: MergeGuardEvaluation, +): Promise { + if (!scm.publishCheckRun) return; + + await scm.publishCheckRun({ + pr, + name: "review-integrity", + status: "completed", + conclusion: integrity.status === "pass" ? "success" : "failure", + summary: + integrity.status === "pass" + ? "All review threads satisfy resolution integrity rules" + : `${integrity.blockers.length} integrity blocker(s) detected`, + text: integrity.blockers.map((b) => `- ${b.message}`).join("\n"), + }); + + await scm.publishCheckRun({ + pr, + name: "ao/merge-guard", + status: "completed", + conclusion: guard.allowMerge ? "success" : "failure", + summary: guard.allowMerge + ? "Merge guard passed" + : `${guard.blockers.length} merge blocker(s) detected`, + text: guard.blockers.map((b) => `- ${b.message}`).join("\n"), + }); +} + +export function buildResolutionRecordInput(input: { + prNumber: number; + threadId: string; + resolutionType: ResolutionType; + actorId: string; + fixCommitSha?: string; + rationale?: string; + evidence?: { + changedFiles?: string[]; + testCommands?: string[]; + testResults?: string[]; + }; +}): Omit { + return createResolutionRecord({ + prNumber: input.prNumber, + threadId: input.threadId, + resolutionType: input.resolutionType, + actorType: "agent", + actorId: input.actorId, + fixCommitSha: input.fixCommitSha, + rationale: input.rationale, + evidence: { + changedFiles: input.evidence?.changedFiles ?? [], + testCommands: input.evidence?.testCommands ?? [], + testResults: input.evidence?.testResults ?? [], + }, + }); +}