diff --git a/tools/workflow-engine/pr-review-lifecycle.test.ts b/tools/workflow-engine/pr-review-lifecycle.test.ts new file mode 100644 index 0000000000..8d785edad2 --- /dev/null +++ b/tools/workflow-engine/pr-review-lifecycle.test.ts @@ -0,0 +1,184 @@ +// Invariant tests for PrReviewLifecycle — producing-side review work substrate. + +import { describe, expect, test } from "bun:test"; +import { + PR_REVIEW_LIFECYCLE_UNIVERSE, + dispatchPrReviewTransition, + isPeerAgentTerritory, + newReviewContext, + type PrReviewLifecycle, + type ReviewFinding, +} from "./pr-review-lifecycle.js"; + +describe("PrReviewLifecycle universe", () => { + test("7 distinct review-lifecycle states", () => { + expect(PR_REVIEW_LIFECYCLE_UNIVERSE.length).toBe(7); + const kinds = PR_REVIEW_LIFECYCLE_UNIVERSE.map((s) => s.kind); + expect(kinds).toContain("observe"); + expect(kinds).toContain("identify-finding"); + expect(kinds).toContain("verify-finding"); // grep-substrate-anchors step + expect(kinds).toContain("post"); + expect(kinds).toContain("conclude"); + }); +}); + +describe("dispatch transitions (happy path)", () => { + const baseContext = newReviewContext(5805, "self", "workflow-engine"); + + test("observe with NO findings → conclude (no-engagement)", () => { + const r = dispatchPrReviewTransition({ kind: "observe" }, baseContext); + expect(r.ok).toBe(true); + if (r.ok) { + expect(r.outcome.nextState.kind).toBe("conclude"); + expect(r.outcome.artifact?.kind).toBe("no-engagement-warranted"); + } + }); + + test("observe WITH findings → identify-finding", () => { + const finding: ReviewFinding = { + prNumber: 5805, + kind: { kind: "substrate-honest-praise", reason: "rank-4 substrate-primitive lands cleanly" }, + content: "lgtm", + substrateAnchors: ["PR #5792 rank-4 substrate-primitive memory"], + }; + const ctx = { ...baseContext, findings: [finding] }; + const r = dispatchPrReviewTransition({ kind: "observe" }, ctx); + expect(r.ok).toBe(true); + if (r.ok) { + expect(r.outcome.nextState.kind).toBe("identify-finding"); + } + }); + + test("identify-finding → compose", () => { + const r = dispatchPrReviewTransition({ kind: "identify-finding" }, baseContext); + expect(r.ok).toBe(true); + if (r.ok) { + expect(r.outcome.nextState.kind).toBe("compose"); + } + }); + + test("compose → verify-finding (grep-substrate-anchors discipline)", () => { + const r = dispatchPrReviewTransition({ kind: "compose" }, baseContext); + expect(r.ok).toBe(true); + if (r.ok) { + expect(r.outcome.nextState.kind).toBe("verify-finding"); + } + }); + + test("verify-finding with substantiated finding → post", () => { + const finding: ReviewFinding = { + prNumber: 5805, + kind: { kind: "bug", severity: "minor" }, + content: "off-by-one suspected", + substrateAnchors: ["line 50: index < len"], // anchor present + }; + const ctx = { ...baseContext, findings: [finding] }; + const r = dispatchPrReviewTransition({ kind: "verify-finding" }, ctx); + expect(r.ok).toBe(true); + if (r.ok) { + expect(r.outcome.nextState.kind).toBe("post"); + } + }); + + test("verify-finding with UNSUBSTANTIATED finding → FindingUnsubstantiated feedback", () => { + const finding: ReviewFinding = { + prNumber: 5805, + kind: { kind: "bug", severity: "minor" }, + content: "vibes off", + substrateAnchors: [], // no anchor — per grep-substrate-anchors rule fails + }; + const ctx = { ...baseContext, findings: [finding] }; + const r = dispatchPrReviewTransition({ kind: "verify-finding" }, ctx); + expect(r.ok).toBe(false); + if (!r.ok) { + expect(r.feedback.kind).toBe("FindingUnsubstantiated"); + } + }); + + test("post → follow-up with review-comment-posted artifact", () => { + const r = dispatchPrReviewTransition({ kind: "post" }, baseContext); + expect(r.ok).toBe(true); + if (r.ok) { + expect(r.outcome.nextState.kind).toBe("follow-up"); + expect(r.outcome.artifact?.kind).toBe("review-comment-posted"); + } + }); + + test("follow-up → conclude", () => { + const r = dispatchPrReviewTransition({ kind: "follow-up" }, baseContext); + expect(r.ok).toBe(true); + if (r.ok) { + expect(r.outcome.nextState.kind).toBe("conclude"); + } + }); + + test("conclude is terminal (stays at conclude)", () => { + const r = dispatchPrReviewTransition({ kind: "conclude" }, baseContext); + expect(r.ok).toBe(true); + if (r.ok) { + expect(r.outcome.nextState.kind).toBe("conclude"); + } + }); +}); + +describe("ReviewFindingKind taxonomy", () => { + test("supports bug + design-question + substrate-engineering + naming + test-gap + praise + docs-gap + composes-with", () => { + const findings: ReviewFinding[] = [ + { prNumber: 1, content: "x", kind: { kind: "bug", severity: "critical" } }, + { prNumber: 1, content: "x", kind: { kind: "design-question", subject: "lifecycle DU shape?" } }, + { prNumber: 1, content: "x", kind: { kind: "substrate-engineering-suggestion", alternative: "prefer Result" } }, + { prNumber: 1, content: "x", kind: { kind: "naming-improvement", current: "foo", proposed: "bar" } }, + { prNumber: 1, content: "x", kind: { kind: "test-gap", uncovered: "edge case X" } }, + { prNumber: 1, content: "x", kind: { kind: "substrate-honest-praise", reason: "composes cleanly" } }, + { prNumber: 1, content: "x", kind: { kind: "documentation-gap", missing: "F# interop note" } }, + { prNumber: 1, content: "x", kind: { kind: "composes-with-substrate", relatedRef: "PR #5805" } }, + ]; + expect(findings.length).toBe(8); + }); +}); + +describe("isPeerAgentTerritory discriminator", () => { + test("self → false (my own work)", () => { + expect(isPeerAgentTerritory("self")).toBe(false); + }); + test("peer-otto → true", () => { + expect(isPeerAgentTerritory("peer-otto")).toBe(true); + }); + test("peer-codex / peer-lior / peer-alexa → true", () => { + expect(isPeerAgentTerritory("peer-codex")).toBe(true); + expect(isPeerAgentTerritory("peer-lior")).toBe(true); + expect(isPeerAgentTerritory("peer-alexa")).toBe(true); + }); + test("human-aaron → true (substantive engagement; don't touch substrate)", () => { + expect(isPeerAgentTerritory("human-operator")).toBe(true); + }); + test("unknown → false (default; treat as substrate-honest mine)", () => { + expect(isPeerAgentTerritory("unknown")).toBe(false); + }); +}); + +describe("newReviewContext constructor", () => { + test("initializes with empty findings + zero observations", () => { + const ctx = newReviewContext(5805, "self", "workflow-engine"); + expect(ctx.prNumber).toBe(5805); + expect(ctx.authorLane).toBe("self"); + expect(ctx.substrateScope).toBe("workflow-engine"); + expect(ctx.findings.length).toBe(0); + expect(ctx.observationsMade).toBe(0); + }); +}); + +describe("type-level PrReviewLifecycle exhaustive switch (compile check)", () => { + test("all 7 variants distinguishable", () => { + const variants: PrReviewLifecycle[] = [ + { kind: "observe" }, + { kind: "identify-finding" }, + { kind: "compose" }, + { kind: "verify-finding" }, + { kind: "post" }, + { kind: "follow-up" }, + { kind: "conclude" }, + ]; + expect(variants.length).toBe(7); + }); +}); diff --git a/tools/workflow-engine/pr-review-lifecycle.ts b/tools/workflow-engine/pr-review-lifecycle.ts new file mode 100644 index 0000000000..c0bfc418a2 --- /dev/null +++ b/tools/workflow-engine/pr-review-lifecycle.ts @@ -0,0 +1,271 @@ +// tools/workflow-engine/pr-review-lifecycle.ts +// +// PrReviewLifecycle — substrate-naming substrate for PRODUCING-side +// review work. Companion to B-0867.20 ReviewLifetime (receiving-side; +// reviewer-feedback gate-state). +// +// Per the human maintainer (2026-05-28): "also does it give you time to look at prs and +// put comments?" — substrate-engineering substrate-engineering substrate +// gap: AutoLoopLifetime (PR #5805) only models SHIP work, not REVIEW +// work. This DU makes producing-side review-substrate explicit. +// +// Composes with: +// - .claude/rules/fighting-past-self-vs-peer-agent-distinguisher-fix-your-own-coordinate-on-peers-dont-punt-by-default.md +// (peer-agent commits = don't touch; peer-agent reviews = substantively engage) +// - .claude/rules/asymmetric-authorship-substrate-entity-defines-consent-channel-recipient-acknowledges.md +// (reviewer AUTHORS feedback; receiving-side ACKNOWLEDGES) +// - .claude/rules/honor-those-that-came-before.md (peer-agent work honored via substantive review) +// - .claude/rules/glass-halo-bidirectional.md (review comments are public substrate; compound) +// - B-0867.20 ReviewLifetime DU (PR #5758) — receiving-side; this PR +// provides producing-side complement +// - AutoLoopLifetime (PR #5805) — will compose; producing-side review +// work becomes additional state-transition in loop substrate + +import { type LifetimeState } from "./world"; + +// ───────────────────────────────────────────────────────────────────── +// PrReviewLifecycle — producing-side state machine +// ───────────────────────────────────────────────────────────────────── + +/** + * PrReviewLifecycle — substrate-naming substrate for PRODUCING-side + * review work. Captures the state machine for reviewing peer PRs + + * composing + posting substantive review comments. + * + * Distinguishes from B-0867.20 ReviewLifetime which captures + * RECEIVING-side gate states (my-PR-under-review states). This DU is + * for when I AM the reviewer of someone else's work. + */ +export interface PrReviewLifecycle extends LifetimeState { + readonly kind: + | "observe" // read PR + diff + context; no engagement yet + | "identify-finding" // substrate-engineering issue / question / praise surfaced + | "compose" // write review comment with substantive content + | "verify-finding" // grep substrate-anchor; check claim before posting (per substrate-honest discipline) + | "post" // ship the review via gh api / GraphQL mutation + | "follow-up" // engage on author's response if any + | "conclude"; // no further engagement; mark as concluded +} + +// ───────────────────────────────────────────────────────────────────── +// Review-finding substrate +// ───────────────────────────────────────────────────────────────────── + +/** + * ReviewFindingKind — substrate-engineering taxonomy of review-comment + * shapes. Each kind has different operational discipline. + */ +export type ReviewFindingKind = + | { kind: "bug"; severity: "critical" | "major" | "minor" } + | { kind: "design-question"; subject: string } + | { kind: "substrate-engineering-suggestion"; alternative: string } + | { kind: "naming-improvement"; current: string; proposed: string } + | { kind: "test-gap"; uncovered: string } + | { kind: "substrate-honest-praise"; reason: string } + | { kind: "documentation-gap"; missing: string } + | { kind: "composes-with-substrate"; relatedRef: string }; + +/** + * ReviewFinding — substrate-engineering observation worth posting as + * review comment. + */ +export interface ReviewFinding { + readonly prNumber: number; + readonly filePath?: string; // optional; some findings are PR-scoped + readonly lineNumber?: number; // optional; some findings are file-scoped + readonly kind: ReviewFindingKind; + readonly content: string; // substantive review-comment body + readonly substrateAnchors?: ReadonlyArray; // grep-substrate-anchors per rule +} + +// ───────────────────────────────────────────────────────────────────── +// Review context + outcome +// ───────────────────────────────────────────────────────────────────── + +/** + * ReviewContext — substrate carried across the review-lifecycle. + */ +export interface ReviewContext { + readonly prNumber: number; + readonly authorLane: "self" | "peer-otto" | "peer-codex" | "peer-lior" | "peer-alexa" | "peer-vera" | "peer-riven" | "peer-amara" | "peer-kestrel" | "peer-prism" | "peer-mika" | "human-operator" | "unknown"; + readonly substrateScope: "workflow-engine" | "encryption" | "zflash" | "ferry-preservation" | "rule-update" | "memory-file" | "infrastructure" | "other"; + readonly findings: ReadonlyArray; + readonly observationsMade: number; // count of observe → identify cycles +} + +/** + * ReviewOutcome — what the review-lifecycle produced. + */ +export interface ReviewOutcome { + readonly nextState: PrReviewLifecycle; + readonly artifact?: { + readonly kind: "review-comment-posted" | "thread-resolved" | "approval-given" | "no-engagement-warranted"; + readonly threadId?: string; + }; +} + +// ───────────────────────────────────────────────────────────────────── +// PrReviewFeedback — asymmetric-authorship per rule +// ───────────────────────────────────────────────────────────────────── + +export type PrReviewFeedback = + | { kind: "PrNotAccessible"; prNumber: number; reason: string } + | { kind: "PeerAgentTerritory"; prNumber: number; lane: ReviewContext["authorLane"] } // don't-touch-commits but review-allowed + | { kind: "FindingUnsubstantiated"; finding: ReviewFinding } // failed verify step + | { kind: "RateLimitExhausted"; budget: "rest" | "graphql"; resetAt: number } + | { kind: "NoActionableFinding"; prNumber: number }; + +export type PrReviewResult = + | { ok: true; outcome: T } + | { ok: false; feedback: PrReviewFeedback }; + +// ───────────────────────────────────────────────────────────────────── +// State transition dispatch +// ───────────────────────────────────────────────────────────────────── + +/** + * Dispatch the next PrReviewLifecycle state given current context. + * + * Per substrate-smoothness rule: exhaustive switch on PrReviewLifecycle + * variants; no if-statement chains. Per asymmetric-authorship: each + * transition AUTHORS feedback channel. + */ +export function dispatchPrReviewTransition( + current: PrReviewLifecycle, + context: ReviewContext, +): PrReviewResult { + switch (current.kind) { + case "observe": + // After observing, identify findings (or conclude if no findings) + if (context.findings.length === 0) { + return { + ok: true, + outcome: { + nextState: { kind: "conclude" }, + artifact: { kind: "no-engagement-warranted" }, + }, + }; + } + return { + ok: true, + outcome: { + nextState: { kind: "identify-finding" }, + }, + }; + + case "identify-finding": + // After identifying, compose the review comment + return { + ok: true, + outcome: { + nextState: { kind: "compose" }, + }, + }; + + case "compose": + // After composing, verify (grep substrate-anchors) + return { + ok: true, + outcome: { + nextState: { kind: "verify-finding" }, + }, + }; + + case "verify-finding": { + // Per grep-substrate-anchors-before-razor-as-metaphysical: every + // finding MUST carry substrate-anchors before advancing to post. + // Both missing `substrateAnchors` AND an empty array mean + // unsubstantiated. Iterate all findings (not just findings[0]). + // Zero findings = NoActionableFinding feedback (can't post review + // when there's nothing to say). + if (context.findings.length === 0) { + return { + ok: false, + feedback: { kind: "NoActionableFinding", prNumber: context.prNumber }, + }; + } + const unsubstantiated = context.findings.find((f) => + f.substrateAnchors === undefined || f.substrateAnchors.length === 0 + ); + if (unsubstantiated !== undefined) { + return { + ok: false, + feedback: { kind: "FindingUnsubstantiated", finding: unsubstantiated }, + }; + } + return { + ok: true, + outcome: { + nextState: { kind: "post" }, + }, + }; + } + + case "post": + // After posting, await follow-up (or conclude if no follow-up expected) + return { + ok: true, + outcome: { + nextState: { kind: "follow-up" }, + artifact: { kind: "review-comment-posted" }, + }, + }; + + case "follow-up": + // After follow-up window, conclude + return { + ok: true, + outcome: { + nextState: { kind: "conclude" }, + }, + }; + + case "conclude": + // Terminal state; stays at conclude + return { + ok: true, + outcome: { + nextState: { kind: "conclude" }, + }, + }; + } +} + +// ───────────────────────────────────────────────────────────────────── +// Reusable substrate exports +// ───────────────────────────────────────────────────────────────────── + +export const PR_REVIEW_LIFECYCLE_UNIVERSE: ReadonlyArray = [ + { kind: "observe" }, + { kind: "identify-finding" }, + { kind: "compose" }, + { kind: "verify-finding" }, + { kind: "post" }, + { kind: "follow-up" }, + { kind: "conclude" }, +]; + +/** + * Determines if a PR is in peer-agent territory (don't touch commits but + * review-allowed per fighting-past-self-vs-peer-agent-distinguisher rule). + */ +export function isPeerAgentTerritory(authorLane: ReviewContext["authorLane"]): boolean { + return authorLane.startsWith("peer-") || authorLane === "human-operator"; +} + +/** + * Empty/initial ReviewContext for starting a new review. + */ +export function newReviewContext( + prNumber: number, + authorLane: ReviewContext["authorLane"], + substrateScope: ReviewContext["substrateScope"], +): ReviewContext { + return { + prNumber, + authorLane, + substrateScope, + findings: [], + observationsMade: 0, + }; +}