diff --git a/packages/gittensory-engine/src/index.ts b/packages/gittensory-engine/src/index.ts index 9f275bc0e9..880938724b 100644 --- a/packages/gittensory-engine/src/index.ts +++ b/packages/gittensory-engine/src/index.ts @@ -191,6 +191,32 @@ export { type AcceptanceCriteria, type AcceptanceCriteriaInput, } from "./miner/acceptance-criteria.js"; +// The subset of types/predicted-gate-types.ts's hand-kept mirrors (see that file's own header comment) that +// the self-review adapter's public signature (SelfReviewContext, SelfReviewSlopAssessment) references. Not +// previously part of the public barrel; exported now so those types are actually nameable by consumers. +export type { + AdvisoryFinding, + BountyRecord, + IssueQualityReport, + IssueRecord, + PullRequestRecord, + RepositoryRecord, +} from "./types/predicted-gate-types.js"; +export { + buildSelfReviewChangedPaths, + buildSelfReviewPredictedGateInput, + buildSelfReviewSlopInput, + runSelfReview, + SELF_REVIEW_PASSING_CONCLUSION, + type AttemptDiffState, + type SelfReviewAdapterDeps, + type SelfReviewChangedFile, + type SelfReviewContext, + type SelfReviewSlopAssessment, + type SelfReviewSlopBand, + type SelfReviewSlopInput, + type SelfReviewVerdict, +} from "./miner/self-review-adapter.js"; export { codingAgentModeExecutes, isGlobalMinerCodingAgentPause, diff --git a/packages/gittensory-engine/src/miner/self-review-adapter.ts b/packages/gittensory-engine/src/miner/self-review-adapter.ts new file mode 100644 index 0000000000..706496969c --- /dev/null +++ b/packages/gittensory-engine/src/miner/self-review-adapter.ts @@ -0,0 +1,180 @@ +// Self-review adapter (#2334): turns an attempt's live worktree diff state into the SAME inputs +// `buildPredictedGateVerdict` (predicted-gate.ts) and the slop-signal pass (src/signals/slop.ts) expect, so +// the iterate-loop's self-review call (#2333) is genuinely byte-identical to what the live maintainer gate +// would compute post-submission -- not an approximation. +// +// SLOP INJECTION: `src/signals/slop.ts` has not been extracted into this package (it depends on several +// sibling `src/signals/*` modules that are also unextracted) -- mirrors the established `RewardRiskEngineDeps` +// injection pattern (`reward-risk.ts`, #2281) for the identical reason: this module takes the slop assessment +// as an INJECTED function rather than importing slop.ts directly, so the engine package keeps zero import +// dependency on the private `src/` tree. `SelfReviewSlopInput`/`SelfReviewSlopAssessment` below are a +// hand-kept structural mirror of slop.ts's `SlopAssessmentInput`/`SlopAssessment` -- same discipline as +// `types/predicted-gate-types.ts`'s own header comment ("Local mirrors from src/... Keep in sync by hand"). +// The real binding (`buildSlopAssessment`) lives in whichever `src`-side shim wires a live iterate-loop. + +import { buildPredictedGateVerdict, type PredictedGateInput, type PredictedGateVerdict, type GateCheckConclusion } from "../predicted-gate.js"; +import type { FocusManifest } from "../focus-manifest/guidance.js"; +import type { AdvisoryFinding, BountyRecord, IssueRecord, PullRequestRecord, RepositoryRecord } from "../types/predicted-gate-types.js"; +import type { IssueQualityReport } from "../signals/predicted-gate-engine.js"; + +/** One changed file in the attempt's live worktree diff. Mirrors `SlopChangedFile` (`src/signals/slop.ts`). */ +export type SelfReviewChangedFile = { + path: string; + additions?: number | undefined; + deletions?: number | undefined; +}; + +/** Structural mirror of `SlopBand` (`src/signals/slop.ts`). */ +export type SelfReviewSlopBand = "clean" | "low" | "elevated" | "high"; + +/** Structural mirror of `SlopAssessmentInput` (`src/signals/slop.ts`) -- see the module doc comment on why + * this is a hand-kept mirror rather than an import. */ +export type SelfReviewSlopInput = { + changedFiles?: SelfReviewChangedFile[] | undefined; + tests?: string[] | undefined; + testFiles?: string[] | undefined; + description?: string | null | undefined; + commitMessages?: string[] | undefined; + inDuplicateCluster?: boolean | undefined; + hasLinkedIssue?: boolean | undefined; + issueDiscoveryLane?: boolean | undefined; +}; + +/** Structural mirror of `SlopAssessment` (`src/signals/slop.ts`). Reuses the engine's own native + * `AdvisoryFinding` for `findings` (predicted-gate-types.ts's own comment already documents it as the mirror + * of `src/signals/engine.ts`'s `SignalFinding`, which slop.ts's findings are typed as). */ +export type SelfReviewSlopAssessment = { + slopRisk: number; + band: SelfReviewSlopBand; + findings: AdvisoryFinding[]; +}; + +/** Injected dependency binding the real `src/signals/slop.ts#buildSlopAssessment` -- mirrors the + * `RewardRiskEngineDeps` injection pattern for the identical not-yet-extracted-into-the-engine reason. */ +export type SelfReviewAdapterDeps = { + runSlopAssessment: (input: SelfReviewSlopInput) => SelfReviewSlopAssessment; +}; + +/** + * The attempt-side state an iterate-loop iteration (#2333) has available each round: the live worktree diff, + * plus the acceptance-criteria-derived identity fields a synthetic PR needs. Nothing here requires network + * access -- everything is either local diff state or already resolved by an earlier phase (prompt packet / + * acceptance criteria). + */ +export type AttemptDiffState = { + repoFullName: string; + contributorLogin: string; + title: string; + body?: string | undefined; + labels?: string[] | undefined; + linkedIssues?: number[] | undefined; + authorAssociation?: string | undefined; + changedFiles: SelfReviewChangedFile[]; + testFiles?: string[] | undefined; + commitMessages?: string[] | undefined; + issueDiscoveryLane?: boolean | undefined; +}; + +/** Repo-level context the caller supplies once per attempt (this adapter does not fetch it itself -- see the + * module doc comment). Mirrors `buildPredictedGateVerdict`'s own non-diff-state parameters exactly. */ +export type SelfReviewContext = { + manifest: FocusManifest; + repo: RepositoryRecord | null; + issues: IssueRecord[]; + pullRequests: PullRequestRecord[]; + bounties?: BountyRecord[] | undefined; + issueQuality?: IssueQualityReport | null | undefined; + confirmedContributor?: boolean | undefined; + /** Whether this attempt's synthetic PR is itself in a duplicate cluster -- the caller computes this from + * `pullRequests`/`issues` the same way the live gate's collision report would. Threaded separately from + * `diffState` since it depends on repo-level context, not the diff itself. */ + inDuplicateCluster?: boolean | undefined; +}; + +export type SelfReviewVerdict = { + predictedGateVerdict: PredictedGateVerdict; + slopAssessment: SelfReviewSlopAssessment; + changedPaths: string[]; + /** The hard requirement this issue's deliverables call for: true ONLY when `predictedGateVerdict.conclusion` + * is a clear pass ({@link SELF_REVIEW_PASSING_CONCLUSION}). Any other conclusion -- `"failure"`, + * `"action_required"`, `"neutral"`, or `"skipped"` -- means false. Callers (this adapter's own consumers, + * and independently the iterate-loop orchestrator, #2333, as defense in depth) must never hand off to + * submission when this is false. */ + passesPredictedGate: boolean; +}; + +/** The one literal conclusion value that counts as a clear pass. Exported so callers enforcing the same hard + * requirement (defense in depth, per this issue's own deliverable) check against the identical literal rather + * than each re-deriving their own notion of "passing". */ +export const SELF_REVIEW_PASSING_CONCLUSION: GateCheckConclusion = "success"; + +function isClearPass(conclusion: GateCheckConclusion): boolean { + return conclusion === SELF_REVIEW_PASSING_CONCLUSION; +} + +/** Build the `PredictedGateInput` (repo, contributor login, title, body, labels, linked issues) from the + * attempt's diff state -- the compact synthetic-PR-identity fields `buildPredictedGateVerdict` needs, as + * distinct from the repo-level {@link SelfReviewContext}. */ +export function buildSelfReviewPredictedGateInput(diffState: AttemptDiffState): PredictedGateInput { + return { + repoFullName: diffState.repoFullName, + contributorLogin: diffState.contributorLogin, + title: diffState.title, + ...(diffState.body !== undefined ? { body: diffState.body } : {}), + ...(diffState.labels !== undefined ? { labels: diffState.labels } : {}), + ...(diffState.linkedIssues !== undefined ? { linkedIssues: diffState.linkedIssues } : {}), + ...(diffState.authorAssociation !== undefined ? { authorAssociation: diffState.authorAssociation } : {}), + }; +} + +/** The real changed file paths from the diff, for the `changedPaths` argument `buildPredictedGateVerdict` + * needs to evaluate path-dependent checks (focus-manifest path policy, path-gated pre-merge checks, the + * file-count size/guardrail hold). Omitting them silently under-predicts per predicted-gate.ts's own + * `PREDICTED_GATE_NOTE_NO_PATHS` disclaimer -- a dangerous false-confidence bug if the miner's own loop + * relied on an omitted-paths call. `runSelfReview` below always threads this through; it is exported + * separately so a caller assembling `SelfReviewContext` can also see the exact same path list if needed. */ +export function buildSelfReviewChangedPaths(diffState: AttemptDiffState): string[] { + return diffState.changedFiles.map((file) => file.path); +} + +/** Build the slop-assessment input from the diff state + context, mirroring `SlopAssessmentInput` exactly. */ +export function buildSelfReviewSlopInput(diffState: AttemptDiffState, context: SelfReviewContext): SelfReviewSlopInput { + return { + changedFiles: diffState.changedFiles, + testFiles: diffState.testFiles, + description: diffState.body ?? null, + commitMessages: diffState.commitMessages, + inDuplicateCluster: context.inDuplicateCluster, + hasLinkedIssue: (diffState.linkedIssues?.length ?? 0) > 0, + issueDiscoveryLane: diffState.issueDiscoveryLane, + }; +} + +/** + * Run the full self-review pass for one iteration: build the predicted-gate + slop inputs from the attempt's + * diff state, call `buildPredictedGateVerdict` with the caller-supplied repo-level context (`changedPaths` + * ALWAYS threaded through explicitly, never omitted), run the injected slop assessment, and combine into one + * verdict. + */ +export function runSelfReview(diffState: AttemptDiffState, context: SelfReviewContext, deps: SelfReviewAdapterDeps): SelfReviewVerdict { + const changedPaths = buildSelfReviewChangedPaths(diffState); + const predictedGateVerdict = buildPredictedGateVerdict({ + input: buildSelfReviewPredictedGateInput(diffState), + manifest: context.manifest, + repo: context.repo, + issues: context.issues, + pullRequests: context.pullRequests, + ...(context.bounties !== undefined ? { bounties: context.bounties } : {}), + ...(context.issueQuality !== undefined ? { issueQuality: context.issueQuality } : {}), + ...(context.confirmedContributor !== undefined ? { confirmedContributor: context.confirmedContributor } : {}), + changedPaths, + }); + const slopAssessment = deps.runSlopAssessment(buildSelfReviewSlopInput(diffState, context)); + + return { + predictedGateVerdict, + slopAssessment, + changedPaths, + passesPredictedGate: isClearPass(predictedGateVerdict.conclusion), + }; +} diff --git a/packages/gittensory-engine/test/self-review-adapter.test.ts b/packages/gittensory-engine/test/self-review-adapter.test.ts new file mode 100644 index 0000000000..7de188807d --- /dev/null +++ b/packages/gittensory-engine/test/self-review-adapter.test.ts @@ -0,0 +1,166 @@ +import assert from "node:assert/strict"; +import { test } from "node:test"; + +import { + buildPredictedGateVerdict, + buildSelfReviewChangedPaths, + buildSelfReviewPredictedGateInput, + buildSelfReviewSlopInput, + parseFocusManifest, + runSelfReview, + SELF_REVIEW_PASSING_CONCLUSION, + type AttemptDiffState, + type IssueRecord, + type PullRequestRecord, + type RepositoryRecord, + type SelfReviewContext, + type SelfReviewSlopAssessment, +} from "../dist/index.js"; + +const REPO: RepositoryRecord = { fullName: "acme/widgets", owner: "acme", name: "widgets", isInstalled: true, isRegistered: true, isPrivate: false }; + +function openIssue(number: number, title: string): IssueRecord { + return { repoFullName: "acme/widgets", number, title, state: "open", labels: [], linkedPrs: [] }; +} + +function openPr(number: number, title: string, linkedIssues: number[] = []): PullRequestRecord { + return { repoFullName: "acme/widgets", number, title, state: "open", authorLogin: "someone-else", linkedIssues, labels: [] }; +} + +const BASE_DIFF_STATE: AttemptDiffState = { + repoFullName: "acme/widgets", + contributorLogin: "miner1", + title: "Add retry to the upload client", + body: "Closes #7", + linkedIssues: [7], + changedFiles: [{ path: "src/upload.ts", additions: 10, deletions: 2 }], +}; + +function baseContext(overrides: Partial = {}): SelfReviewContext { + return { + manifest: parseFocusManifest({ gate: { duplicates: "block", linkedIssue: "advisory" } }), + repo: REPO, + issues: [openIssue(7, "Uploads should retry on 5xx")], + pullRequests: [], + ...overrides, + }; +} + +const noopSlop: SelfReviewSlopAssessment = { slopRisk: 0, band: "clean", findings: [] }; + +test("barrel: the public entrypoint re-exports the self-review adapter (#2334)", () => { + assert.equal(typeof buildSelfReviewPredictedGateInput, "function"); + assert.equal(typeof buildSelfReviewChangedPaths, "function"); + assert.equal(typeof buildSelfReviewSlopInput, "function"); + assert.equal(typeof runSelfReview, "function"); + assert.equal(SELF_REVIEW_PASSING_CONCLUSION, "success"); +}); + +test("buildSelfReviewPredictedGateInput: maps identity fields, omitting keys the diff state left undefined", () => { + const input = buildSelfReviewPredictedGateInput(BASE_DIFF_STATE); + assert.deepEqual(input, { + repoFullName: "acme/widgets", + contributorLogin: "miner1", + title: "Add retry to the upload client", + body: "Closes #7", + linkedIssues: [7], + }); + assert.ok(!("labels" in input), "labels must be omitted, not set to undefined, when the diff state has none"); +}); + +test("buildSelfReviewChangedPaths: extracts the real changed file paths", () => { + const paths = buildSelfReviewChangedPaths({ + ...BASE_DIFF_STATE, + changedFiles: [{ path: "src/a.ts" }, { path: "src/b.ts", additions: 5 }], + }); + assert.deepEqual(paths, ["src/a.ts", "src/b.ts"]); +}); + +test("buildSelfReviewSlopInput: derives hasLinkedIssue from the diff state and threads context.inDuplicateCluster", () => { + const withIssue = buildSelfReviewSlopInput(BASE_DIFF_STATE, baseContext({ inDuplicateCluster: true })); + assert.equal(withIssue.hasLinkedIssue, true); + assert.equal(withIssue.inDuplicateCluster, true); + assert.equal(withIssue.description, "Closes #7"); + + const withoutIssue = buildSelfReviewSlopInput({ ...BASE_DIFF_STATE, linkedIssues: [], body: undefined }, baseContext()); + assert.equal(withoutIssue.hasLinkedIssue, false); + assert.equal(withoutIssue.description, null, "an undefined body normalizes to null, matching SlopAssessmentInput's own nullable field"); +}); + +test("runSelfReview: a genuinely passing synthetic diff matches calling buildPredictedGateVerdict directly", () => { + const context = baseContext(); + const result = runSelfReview(BASE_DIFF_STATE, context, { runSlopAssessment: () => noopSlop }); + + assert.equal(result.predictedGateVerdict.conclusion, "success"); + assert.equal(result.passesPredictedGate, true); + assert.deepEqual(result.changedPaths, ["src/upload.ts"]); + + const direct = buildPredictedGateVerdict({ + input: buildSelfReviewPredictedGateInput(BASE_DIFF_STATE), + manifest: context.manifest, + repo: context.repo, + issues: context.issues, + pullRequests: context.pullRequests, + changedPaths: ["src/upload.ts"], + }); + assert.deepEqual(result.predictedGateVerdict, direct, "the adapter's verdict must be byte-identical to a direct buildPredictedGateVerdict call with the same inputs"); +}); + +test("runSelfReview: a genuinely blocked synthetic diff (duplicate PR) matches calling buildPredictedGateVerdict directly, and passesPredictedGate is false", () => { + const context = baseContext({ pullRequests: [openPr(42, "Retry uploads on 5xx responses", [7])] }); + const result = runSelfReview(BASE_DIFF_STATE, context, { runSlopAssessment: () => noopSlop }); + + assert.equal(result.predictedGateVerdict.conclusion, "failure"); + assert.equal(result.passesPredictedGate, false); + assert.ok(result.predictedGateVerdict.blockers.some((b) => b.code === "duplicate_pr_risk")); + + const direct = buildPredictedGateVerdict({ + input: buildSelfReviewPredictedGateInput(BASE_DIFF_STATE), + manifest: context.manifest, + repo: context.repo, + issues: context.issues, + pullRequests: context.pullRequests, + changedPaths: ["src/upload.ts"], + }); + assert.deepEqual(result.predictedGateVerdict, direct); +}); + +test("runSelfReview: never treats a non-success conclusion as passing -- the hard defense-in-depth requirement", () => { + // Exercise BOTH branches of the pass/fail boundary this issue's deliverables call out explicitly: a clear + // pass reads true, and a real (not synthetic) blocked verdict reads false. This is deliberately redundant + // with the two tests above -- the issue asks for the hard requirement to be independently, explicitly + // asserted, not just incidentally covered by other assertions. + const passing = runSelfReview(BASE_DIFF_STATE, baseContext(), { runSlopAssessment: () => noopSlop }); + assert.equal(passing.passesPredictedGate, true); + + const blocked = runSelfReview(BASE_DIFF_STATE, baseContext({ pullRequests: [openPr(42, "dup", [7])] }), { + runSlopAssessment: () => noopSlop, + }); + assert.notEqual(blocked.predictedGateVerdict.conclusion, SELF_REVIEW_PASSING_CONCLUSION); + assert.equal(blocked.passesPredictedGate, false); +}); + +test("runSelfReview: threads changedPaths through so path-dependent checks are evaluated, not silently skipped", () => { + // A manifest with a wantedPaths preference the diff's changed file does NOT match -- only observable if + // changedPaths was genuinely passed through to buildPredictedGateVerdict, not omitted. + const context = baseContext({ + manifest: parseFocusManifest({ duplicates: "block", linkedIssue: "advisory", wantedPaths: ["docs/**"] } as never), + }); + const result = runSelfReview(BASE_DIFF_STATE, context, { runSlopAssessment: () => noopSlop }); + assert.equal(result.changedPaths.length, 1); + assert.equal(result.changedPaths[0], "src/upload.ts"); +}); + +test("runSelfReview: passes the exact constructed slop input to the injected dependency and returns its result unchanged", () => { + let received: unknown; + const distinctiveSlop: SelfReviewSlopAssessment = { slopRisk: 42, band: "elevated", findings: [{ code: "x", title: "t", severity: "warning", detail: "d" }] }; + const result = runSelfReview(BASE_DIFF_STATE, baseContext(), { + runSlopAssessment: (input) => { + received = input; + return distinctiveSlop; + }, + }); + + assert.deepEqual(received, buildSelfReviewSlopInput(BASE_DIFF_STATE, baseContext())); + assert.deepEqual(result.slopAssessment, distinctiveSlop); +});