From 3166e2b2308af6b87355bb93f189db40a69c33f5 Mon Sep 17 00:00:00 2001 From: AndreasS Date: Fri, 2 Oct 2026 21:49:30 +0200 Subject: [PATCH 1/2] feat(policy): read a review record and map it onto merge evidence The merge driver refuses every merge until a verdict exists for the head, but nothing yet recorded what a reviewer actually concluded. A model's claim that it reviewed something is not a record of a review. .skein/review.json is that record, read through a closed schema so an unknown key is refused rather than dropped. Each refusal names its own reason token, keeping "nobody has reviewed yet" distinguishable from "a review that does not count" instead of collapsing both into no verdict. The head SHA carries the weight. A verdict is refused unless it names the SHA it covers, so a stale LGTM cannot cover a new head, and a verdict with no SHA attached cannot read as an approval. The mapper stamps the review SHA from headSHA, leaves a record with no verdict as no verdict rather than a pass, and leaves gates and CI to the driver, which measures them against a SHA. Not yet wired into the loop or merge driver; this only establishes the record and the mapping. Each of the five guards was mutated in turn and observed to fail exactly the test that owns it. --- fork/manifest.json | 2 + packages/opencode/src/policy/review-record.ts | 174 ++++++++++++++++ .../test/policy/review-record.test.ts | 191 ++++++++++++++++++ 3 files changed, 367 insertions(+) create mode 100644 packages/opencode/src/policy/review-record.ts create mode 100644 packages/opencode/test/policy/review-record.test.ts diff --git a/fork/manifest.json b/fork/manifest.json index fa2d79bc925f..f9ea483df4ca 100644 --- a/fork/manifest.json +++ b/fork/manifest.json @@ -88,6 +88,7 @@ "packages/opencode/src/policy/merge-run.ts", "packages/opencode/src/policy/publish-policy.ts", "packages/opencode/src/policy/push-run.ts", + "packages/opencode/src/policy/review-record.ts", "packages/opencode/src/provider/balance.ts", "packages/opencode/src/server/routes/instance/httpapi/groups/agents.ts", "packages/opencode/src/server/routes/instance/httpapi/groups/gallery.ts", @@ -242,6 +243,7 @@ "packages/opencode/test/policy/publish-policy-git.test.ts", "packages/opencode/test/policy/publish-policy.test.ts", "packages/opencode/test/policy/push-run.test.ts", + "packages/opencode/test/policy/review-record.test.ts", "packages/opencode/test/session/unattended.test.ts", "packages/opencode/test/tool/send-peer-message-ladder.test.ts", "packages/tui/src/component/dialog-gallery.tsx", diff --git a/packages/opencode/src/policy/review-record.ts b/packages/opencode/src/policy/review-record.ts new file mode 100644 index 000000000000..32129405e886 --- /dev/null +++ b/packages/opencode/src/policy/review-record.ts @@ -0,0 +1,174 @@ +export * as ReviewRecord from "./review-record" + +import fs from "fs" +import path from "path" +import { Effect, Result, Schema } from "effect" +import { PublishPolicy } from "./publish-policy" +import { PublishDrivers } from "./drivers" + +// The review record: what a reviewer said about one exact commit. +// +// It exists because the merge driver refuses every merge until a verdict exists +// for the head being merged, and because the verdict has to be attributable to +// something that was actually written down. A model's claim that it reviewed +// something is not a record of a review; this is. +// +// The head SHA is the load-bearing field. A verdict for an earlier commit does not +// cover the head, so every field that carries an opinion carries the commit it is +// about, and a record whose head does not match what is being merged is refused by +// the consumer rather than silently accepted here. +// +// Gates and CI evidence deliberately do NOT live in this file. They are produced +// by the driver, measured against a SHA, and mixing a reviewer's opinion with a +// machine's measurement in one record would make it impossible to tell which +// claim failed when a merge is refused. + +export const RECORD_FILE = [".skein", "review.json"] + +/** A full object id. Abbreviations are refused: two commits can share a prefix. */ +const FULL_SHA = /^[0-9a-f]{40}$/ + +/** + * Every way reading a record can refuse, as a distinct named value. + * + * Named for the same reason `PublishPolicy.Reason` is: Phase 4 will log this in + * the session's line, and a log that can only be read by pattern-matching prose is + * a log nobody can alert on. + */ +export const Reason = { + /** No record at the expected path. */ + absent: "review-absent", + /** The file was not readable as JSON. */ + unreadable: "review-unreadable", + /** The file did not satisfy the closed schema. */ + malformed: "review-malformed", + /** `headSHA` is not a full 40-character object id. */ + headNotFullSha: "review-head-not-full-sha", + /** `verdict` is set but `reviewedSHA` is missing or does not equal `headSHA`. */ + verdictNotForHead: "review-verdict-not-for-head", + /** The record's round is below 1. */ + roundInvalid: "review-round-invalid", +} as const + +export type Reason = (typeof Reason)[keyof typeof Reason] + +const Severity = Schema.Literals(["blocking", "advisory"]) + +const Finding = Schema.Struct({ + file: Schema.String, + line: Schema.Number, + severity: Severity, + text: Schema.String, +}) + +const Record_ = Schema.Struct({ + headSHA: Schema.String, + base: Schema.String, + reviewer: Schema.Struct({ harness: Schema.String, model: Schema.String }), + independence: Schema.Literals(["independent", "same-model"]), + /** Optional: a round that ended without a verdict is a valid record, and is not a pass. */ + verdict: Schema.optional(Schema.Literals(["LGTM", "NEEDS_WORK"])), + /** Required whenever `verdict` is set; must equal `headSHA`. Enforced in `validate`. */ + reviewedSHA: Schema.optional(Schema.String), + findings: Schema.Array(Finding), + round: Schema.Number, +}) + +export type Record = typeof Record_.Type +export type Finding = typeof Finding.Type + +/** + * Validates an already-parsed record. + * + * Separate from the schema because the interesting rules are cross-field — a + * verdict whose SHA is not the head — and a closed schema cannot express those. + * Every failure names a reason rather than returning a bare false, because the + * consumer has to be able to tell "no review yet" from "a review that does not + * count". + */ +export function validate(input: unknown): Result.Result { + const decoded = Effect.runSync( + Effect.result( + Schema.decodeUnknownEffect(Record_, { + errors: "all", + // Closed: a typo, or a field added later and silently ignored, is the failure + // mode this whole module exists to prevent. + onExcessProperty: "error", + propertyOrder: "original", + })(input), + ), + ) + if (Result.isFailure(decoded)) return Result.fail(Reason.malformed) + const record = decoded.success + + if (!FULL_SHA.test(record.headSHA)) return Result.fail(Reason.headNotFullSha) + if (!Number.isInteger(record.round) || record.round < 1) return Result.fail(Reason.roundInvalid) + // A verdict without the SHA it covers is the single most dangerous shape this + // record can take: it reads as an approval and covers nothing. + if (record.verdict !== undefined && (record.reviewedSHA === undefined || record.reviewedSHA !== record.headSHA)) + return Result.fail(Reason.verdictNotForHead) + return Result.succeed(record) +} + +export type Loaded = + | { readonly ok: true; readonly record: Record; readonly source: string } + | { readonly ok: false; readonly reason: Reason; readonly detail: string; readonly source: string } + +/** + * Reads `.skein/review.json` for a directory. + * + * An absent record and an invalid one are deliberately distinguishable here, at + * the boundary, because "no review yet" and "a review that does not count" are + * different facts for whoever is waiting on a merge. Downstream, both become "no + * verdict" — but the reason token says which. + */ +export function load(directory: string): Effect.Effect { + const source = path.join(directory, ...RECORD_FILE) + const refuse = (reason: Reason, detail: string): Loaded => ({ ok: false, reason, detail, source }) + if (!fs.existsSync(source)) return Effect.succeed(refuse(Reason.absent, `no review record at ${source}`)) + + let raw: unknown + try { + raw = JSON.parse(fs.readFileSync(source, "utf8")) + } catch (err) { + return Effect.succeed(refuse(Reason.unreadable, `could not parse ${source}: ${String(err)}`)) + } + const validated = validate(raw) + if (Result.isFailure(validated)) return Effect.succeed(refuse(validated.failure, `${source}: ${validated.failure}`)) + return Effect.succeed({ ok: true, record: validated.success, source }) +} + +/** + * Maps a record onto the merge driver's evidence shape. + * + * Two rules, both deliberate: + * - the verdict's SHA is the record's `headSHA`, never `reviewedSHA`, because + * `validate` has already established they are equal and the driver should not + * have to know that + * - a record with no verdict yields no `reviewVerdict` at all, rather than a + * passing one. "The reviewer replied without a token" is not an approval. + * + * Gates, CI and mergeBase are left absent: they are the driver's to measure. + */ +export function toMergeEvidence(record: Record): PublishDrivers.MergeEvidence { + return { + headSHA: record.headSHA, + reviewVerdict: + record.verdict === undefined ? undefined : { verdict: record.verdict, sha: record.headSHA }, + } +} + +/** Convenience: read a directory and map it, answering "is there a verdict for this head". */ +export function evidenceFor(directory: string): Effect.Effect<{ + evidence: PublishDrivers.MergeEvidence + reason?: Reason +}> { + return Effect.gen(function* () { + const loaded = yield* load(directory) + if (!loaded.ok) return { evidence: { headSHA: "" }, reason: loaded.reason } + return { evidence: toMergeEvidence(loaded.record) } + }) +} + +/** Kept so the policy's reason vocabulary stays the single place a caller looks. */ +export type PolicyReason = PublishPolicy.Reason diff --git a/packages/opencode/test/policy/review-record.test.ts b/packages/opencode/test/policy/review-record.test.ts new file mode 100644 index 000000000000..3ad63827ce6e --- /dev/null +++ b/packages/opencode/test/policy/review-record.test.ts @@ -0,0 +1,191 @@ +import { describe, expect, test } from "bun:test" +import { Effect } from "effect" +import { mkdtemp, mkdir, writeFile } from "fs/promises" +import { tmpdir } from "os" +import path from "path" +import { ReviewRecord } from "@/policy/review-record" +import { PublishDrivers } from "@/policy/drivers" +import type { PublishPolicy } from "@/policy/publish-policy" + +const POLICY: PublishPolicy.Policy = { + version: 1, + repo: "example/repo", + visibility: "private", + commit: { branches: ["loop/*"] }, + push: { remotes: ["origin"], branches: ["loop/*"] }, + merge: { into: ["dev"], method: "squash", requires: ["gates", "review"], by: ["integrator"] }, +} + + +// Real files on disk, because the reader's job is to be honest about what is +// actually written — a fixture validated in memory would not exercise the paths, +// the JSON parse, or the "file is absent" case that a merge waits on. + +const HEAD = "a".repeat(40) +const OTHER = "b".repeat(40) + +const VALID: ReviewRecord.Record = { + headSHA: HEAD, + base: OTHER, + reviewer: { harness: "opencode", model: "some-other-family" }, + independence: "independent", + verdict: "LGTM", + reviewedSHA: HEAD, + findings: [{ file: "src/x.ts", line: 12, severity: "advisory", text: "consider naming this" }], + round: 1, +} + +async function withRecord(contents: unknown | string) { + const dir = await mkdtemp(path.join(tmpdir(), "review-record-")) + await mkdir(path.join(dir, ".skein"), { recursive: true }) + if (contents !== undefined) + await writeFile(path.join(dir, ".skein", "review.json"), typeof contents === "string" ? contents : JSON.stringify(contents)) + return dir +} + +const load = (dir: string) => Effect.runPromise(ReviewRecord.load(dir)) +const reasonOf = async (contents: unknown | string) => { + const result = await load(await withRecord(contents)) + if (result.ok) throw new Error("expected a refusal, got a record") + return result.reason +} + +describe("review record reading", () => { + test("reads a valid record", async () => { + const result = await load(await withRecord(VALID)) + expect(result.ok).toBe(true) + if (!result.ok) throw new Error("unreachable") + expect(result.record.headSHA).toBe(HEAD) + expect(result.record.verdict).toBe("LGTM") + expect(result.source).toContain(path.join(".skein", "review.json")) + }) + + test("an absent record is distinguishable from an invalid one", async () => { + // Two different facts for whoever waits on a merge: nobody has reviewed yet, + // versus a review that does not count. Collapsing them would make a retry + // look pointless. + expect(await reasonOf(undefined)).toBe(ReviewRecord.Reason.absent) + expect(await reasonOf({ ...VALID, round: 0 })).toBe(ReviewRecord.Reason.roundInvalid) + }) + + test("unparseable JSON is its own reason, not malformed", async () => { + // A truncated write should not read as "the schema rejected it". + expect(await reasonOf("{ not json")).toBe(ReviewRecord.Reason.unreadable) + }) + + test("an unknown key is refused rather than ignored", async () => { + // Closed schema: a field this version does not understand must not be + // silently dropped, or a future field could change the meaning of a record. + expect(await reasonOf({ ...VALID, verdictToken: "LGTM" })).toBe(ReviewRecord.Reason.malformed) + }) + + test("an abbreviated head SHA is refused", async () => { + // Two commits can share a prefix, so an abbreviation cannot identify a head. + expect(await reasonOf({ ...VALID, headSHA: "abc1234", reviewedSHA: "abc1234" })).toBe( + ReviewRecord.Reason.headNotFullSha, + ) + }) + + test("a verdict with no reviewedSHA is refused", async () => { + // The most dangerous shape this record can take: it reads as an approval and + // covers nothing. + const { reviewedSHA: _dropped, ...withoutSha } = VALID + expect(await reasonOf(withoutSha)).toBe(ReviewRecord.Reason.verdictNotForHead) + }) + + test("a verdict for a different SHA is refused", async () => { + // A stale review: LGTM for an earlier commit must not cover the new head. + expect(await reasonOf({ ...VALID, reviewedSHA: OTHER })).toBe(ReviewRecord.Reason.verdictNotForHead) + }) + + test("a record with no verdict at all is valid", async () => { + // A round that ended without a verdict is a real outcome worth recording. What + // matters is that it does not become a pass, which the mapper tests pin. + const { verdict: _v, reviewedSHA: _s, ...noVerdict } = VALID + const result = await load(await withRecord(noVerdict)) + expect(result.ok).toBe(true) + }) + + test("a finding with an unknown severity is refused", async () => { + expect( + await reasonOf({ ...VALID, findings: [{ file: "x", line: 1, severity: "nit", text: "t" }] }), + ).toBe(ReviewRecord.Reason.malformed) + }) + + test("every named reason is distinct", () => { + // If two refusals collapsed to one token, a caller could not tell them apart + // and a log could not be alerted on them separately. + expect(new Set(Object.values(ReviewRecord.Reason)).size).toBe(Object.values(ReviewRecord.Reason).length) + }) +}) + +describe("mapping onto the merge driver's evidence", () => { + test("the verdict carries the record's headSHA", () => { + // Never `reviewedSHA`: validate has already proven they are equal, and the + // driver should not have to know that. + const evidence = ReviewRecord.toMergeEvidence(VALID) + expect(evidence.headSHA).toBe(HEAD) + expect(evidence.reviewVerdict).toEqual({ verdict: "LGTM", sha: HEAD }) + }) + + test("no verdict yields no reviewVerdict at all, not a passing one", () => { + // The whole point: "the reviewer replied without a token" is not an approval. + const { verdict: _v, reviewedSHA: _s, ...noVerdict } = VALID + const evidence = ReviewRecord.toMergeEvidence(noVerdict) + expect(evidence.reviewVerdict).toBeUndefined() + expect(evidence.headSHA).toBe(HEAD) + }) + + test("gates, CI and mergeBase are left to the driver", () => { + // Mixing a reviewer's opinion with a machine's measurement in one record would + // make it impossible to tell which claim failed when a merge is refused. + const evidence = ReviewRecord.toMergeEvidence(VALID) + expect(evidence.gates).toBeUndefined() + expect(evidence.ci).toBeUndefined() + expect(evidence.mergeBase).toBeUndefined() + }) + + test("NEEDS_WORK maps through as NEEDS_WORK, not as an absence", () => { + const evidence = ReviewRecord.toMergeEvidence({ ...VALID, verdict: "NEEDS_WORK" }) + expect(evidence.reviewVerdict?.verdict).toBe("NEEDS_WORK") + }) + + test("the real driver refuses a record with no verdict", () => { + // The property that matters, checked against the actual consumer rather than a + // type assertion: a review record without a verdict must not become a pass. + const { verdict: _v, reviewedSHA: _s, ...noVerdict } = VALID + const refusal = PublishDrivers.mayMerge({ + policy: POLICY, + target: "dev", + actor: "integrator", + evidence: { + ...ReviewRecord.toMergeEvidence(noVerdict), + mergeBase: "b".repeat(40), + gates: { passed: true, sha: HEAD }, + }, + }) + expect(refusal.ok).toBe(false) + if (refusal.ok) throw new Error("unreachable") + expect(refusal.reason).toContain("no recorded review verdict") + }) + + test("the real driver accepts a record whose verdict covers the head", () => { + const ok = PublishDrivers.mayMerge({ + policy: POLICY, + target: "dev", + actor: "integrator", + evidence: { + ...ReviewRecord.toMergeEvidence(VALID), + mergeBase: "b".repeat(40), + gates: { passed: true, sha: HEAD }, + }, + }) + expect(ok).toEqual({ ok: true }) + }) + + test("an absent record yields no verdict, with its reason", async () => { + const out = await Effect.runPromise(ReviewRecord.evidenceFor(await withRecord(undefined))) + expect(out.reason).toBe(ReviewRecord.Reason.absent) + expect(out.evidence.reviewVerdict).toBeUndefined() + }) +}) From 197dc588652857dc15b7914d50d975a6da26700d Mon Sep 17 00:00:00 2001 From: AndreasS Date: Fri, 2 Oct 2026 21:56:19 +0200 Subject: [PATCH 2/2] feat(policy): address review feedback on the review record MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four changes from the independent read. Drop reviewedSHA. The spec says an approval applies to "the SHA it recorded", and that SHA is headSHA — a review is made for one head. A second field naming the same commit is an invariant that can only ever agree with the first, and the driver already refuses any record whose headSHA is not the head it is merging. It protected against a state that cannot arise, at the cost of a rule. Carry independence onto the evidence. It was being read and discarded, so a same-model review mapped to exactly the same passing verdict as an independent one and "the reviewer differs from the author" was unenforceable. It is now an optional field on MergeEvidence.reviewVerdict, mapped but not yet enforced: the policy knob that decides whether same-model suffices comes later, and it cannot be decided from data that was not recorded. Require reviewer.sessionID. The record lives in the author's own working tree and a model can write a file, so today the author could write its own LGTM. The session id is on the record and charset-checked, but it is still only a claim: binding it to an authenticated identity is the writer's job and belongs to the next change, which will also fence .skein/review.json from model edits. Validate base as a full object id, since the record anchors a base..head range. Findings: line is Schema.Int rather than Schema.Number, which observed as accepting 0, -3, 1.5, NaN and Infinity. A present line must be at least 1, and a blocking finding must carry one, because a blocking finding nobody can point at cannot be fixed; an advisory finding may omit it, since a design or test-gap finding has no location. Eight guards mutated in turn; every one observed red and each failed only the test that owns it. The head and base checks share FULL_SHA, so that one mutation trips both by design. No writer in this change: it needs the authenticity design first. --- packages/opencode/src/policy/drivers.ts | 12 +- packages/opencode/src/policy/review-record.ts | 94 +++++++----- .../test/policy/review-record.test.ts | 134 ++++++++++++++---- 3 files changed, 175 insertions(+), 65 deletions(-) diff --git a/packages/opencode/src/policy/drivers.ts b/packages/opencode/src/policy/drivers.ts index 977962198031..f5348d24cf52 100644 --- a/packages/opencode/src/policy/drivers.ts +++ b/packages/opencode/src/policy/drivers.ts @@ -127,7 +127,17 @@ export interface MergeEvidence { readonly headSHA: string readonly gates?: { readonly passed: boolean; readonly sha: string } readonly ci?: { readonly passed: boolean; readonly sha: string } - readonly reviewVerdict?: { readonly verdict: "LGTM" | "NEEDS_WORK"; readonly sha: string } + /** + * `independence` is carried, not yet enforced: whether a same-model review + * satisfies the review requirement is a policy knob, and it cannot be decided + * from data that was not recorded. Absent on evidence assembled by callers that + * predate the field. + */ + readonly reviewVerdict?: { + readonly verdict: "LGTM" | "NEEDS_WORK" + readonly sha: string + readonly independence?: "independent" | "same-model" + } /** The merge base of target and head, as the caller computed it. The executor recomputes it. */ readonly mergeBase?: string } diff --git a/packages/opencode/src/policy/review-record.ts b/packages/opencode/src/policy/review-record.ts index 32129405e886..672c431730c4 100644 --- a/packages/opencode/src/policy/review-record.ts +++ b/packages/opencode/src/policy/review-record.ts @@ -3,23 +3,23 @@ export * as ReviewRecord from "./review-record" import fs from "fs" import path from "path" import { Effect, Result, Schema } from "effect" -import { PublishPolicy } from "./publish-policy" import { PublishDrivers } from "./drivers" // The review record: what a reviewer said about one exact commit. // // It exists because the merge driver refuses every merge until a verdict exists -// for the head being merged, and because the verdict has to be attributable to -// something that was actually written down. A model's claim that it reviewed -// something is not a record of a review; this is. +// for the head being merged, and because that verdict has to be attributable to +// something written down. A model's claim that it reviewed something is not a +// record of a review; this is. // -// The head SHA is the load-bearing field. A verdict for an earlier commit does not -// cover the head, so every field that carries an opinion carries the commit it is -// about, and a record whose head does not match what is being merged is refused by -// the consumer rather than silently accepted here. +// The head SHA is the load-bearing field, and it is the ONLY sha a verdict +// carries. A review is made for one head, so a verdict in a record whose headSHA +// is H is about H — a second field naming the same commit would be an invariant +// that can only ever agree with the first, and the driver already refuses any +// record whose headSHA is not the head it is merging. // // Gates and CI evidence deliberately do NOT live in this file. They are produced -// by the driver, measured against a SHA, and mixing a reviewer's opinion with a +// by the driver and measured against a SHA; mixing a reviewer's opinion with a // machine's measurement in one record would make it impossible to tell which // claim failed when a merge is refused. @@ -28,10 +28,17 @@ export const RECORD_FILE = [".skein", "review.json"] /** A full object id. Abbreviations are refused: two commits can share a prefix. */ const FULL_SHA = /^[0-9a-f]{40}$/ +/** + * Session ids are constrained to the same charset as lead grant ids: this value + * ends up in a comparison against an authenticated identity, so it must not be a + * place to smuggle structure. + */ +const SAFE_ID = /^[A-Za-z0-9_-]{1,64}$/ + /** * Every way reading a record can refuse, as a distinct named value. * - * Named for the same reason `PublishPolicy.Reason` is: Phase 4 will log this in + * Named for the same reason `PublishPolicy.Reason` is: a later phase logs this in * the session's line, and a log that can only be read by pattern-matching prose is * a log nobody can alert on. */ @@ -44,8 +51,14 @@ export const Reason = { malformed: "review-malformed", /** `headSHA` is not a full 40-character object id. */ headNotFullSha: "review-head-not-full-sha", - /** `verdict` is set but `reviewedSHA` is missing or does not equal `headSHA`. */ - verdictNotForHead: "review-verdict-not-for-head", + /** `base` is not a full 40-character object id. */ + baseNotFullSha: "review-base-not-full-sha", + /** `reviewer.sessionID` is missing or outside the safe id charset. */ + reviewerSessionInvalid: "review-reviewer-session-invalid", + /** A finding's `line` is present but is not a line number (zero or negative). */ + findingLineInvalid: "review-finding-line-invalid", + /** A blocking finding names no `line`, so it cannot be acted on. */ + findingLineMissing: "review-finding-line-missing", /** The record's round is below 1. */ roundInvalid: "review-round-invalid", } as const @@ -56,7 +69,12 @@ const Severity = Schema.Literals(["blocking", "advisory"]) const Finding = Schema.Struct({ file: Schema.String, - line: Schema.Number, + /** + * `Schema.Int` rather than `Schema.Number`: the latter accepts 1.5, NaN and + * Infinity. Whether a number is a line at all (>0) is a per-finding rule and + * lives in `validate`, alongside the rule that a blocking finding must have one. + */ + line: Schema.optional(Schema.Int), severity: Severity, text: Schema.String, }) @@ -64,12 +82,10 @@ const Finding = Schema.Struct({ const Record_ = Schema.Struct({ headSHA: Schema.String, base: Schema.String, - reviewer: Schema.Struct({ harness: Schema.String, model: Schema.String }), + reviewer: Schema.Struct({ harness: Schema.String, model: Schema.String, sessionID: Schema.String }), independence: Schema.Literals(["independent", "same-model"]), /** Optional: a round that ended without a verdict is a valid record, and is not a pass. */ verdict: Schema.optional(Schema.Literals(["LGTM", "NEEDS_WORK"])), - /** Required whenever `verdict` is set; must equal `headSHA`. Enforced in `validate`. */ - reviewedSHA: Schema.optional(Schema.String), findings: Schema.Array(Finding), round: Schema.Number, }) @@ -81,18 +97,18 @@ export type Finding = typeof Finding.Type * Validates an already-parsed record. * * Separate from the schema because the interesting rules are cross-field — a - * verdict whose SHA is not the head — and a closed schema cannot express those. - * Every failure names a reason rather than returning a bare false, because the - * consumer has to be able to tell "no review yet" from "a review that does not - * count". + * blocking finding without a location, a round below 1 — and a closed schema + * cannot express those. Every failure names a reason rather than returning a bare + * false, because the consumer has to be able to tell "no review yet" from "a + * review that does not count". */ export function validate(input: unknown): Result.Result { const decoded = Effect.runSync( Effect.result( Schema.decodeUnknownEffect(Record_, { errors: "all", - // Closed: a typo, or a field added later and silently ignored, is the failure - // mode this whole module exists to prevent. + // Closed: a typo, or a field added later and silently ignored, is the + // failure mode this whole module exists to prevent. onExcessProperty: "error", propertyOrder: "original", })(input), @@ -102,11 +118,18 @@ export function validate(input: unknown): Result.Result { const record = decoded.success if (!FULL_SHA.test(record.headSHA)) return Result.fail(Reason.headNotFullSha) + if (!FULL_SHA.test(record.base)) return Result.fail(Reason.baseNotFullSha) + if (!SAFE_ID.test(record.reviewer.sessionID)) return Result.fail(Reason.reviewerSessionInvalid) if (!Number.isInteger(record.round) || record.round < 1) return Result.fail(Reason.roundInvalid) - // A verdict without the SHA it covers is the single most dangerous shape this - // record can take: it reads as an approval and covers nothing. - if (record.verdict !== undefined && (record.reviewedSHA === undefined || record.reviewedSHA !== record.headSHA)) - return Result.fail(Reason.verdictNotForHead) + for (const finding of record.findings) { + if (finding.line === undefined) { + // An advisory finding may legitimately be about a design or a test gap, which + // has no location. A blocking one that cannot be pointed at cannot be fixed. + if (finding.severity === "blocking") return Result.fail(Reason.findingLineMissing) + continue + } + if (finding.line < 1) return Result.fail(Reason.findingLineInvalid) + } return Result.succeed(record) } @@ -134,17 +157,19 @@ export function load(directory: string): Effect.Effect { return Effect.succeed(refuse(Reason.unreadable, `could not parse ${source}: ${String(err)}`)) } const validated = validate(raw) - if (Result.isFailure(validated)) return Effect.succeed(refuse(validated.failure, `${source}: ${validated.failure}`)) + if (Result.isFailure(validated)) + return Effect.succeed(refuse(validated.failure, `${source}: ${validated.failure}`)) return Effect.succeed({ ok: true, record: validated.success, source }) } /** * Maps a record onto the merge driver's evidence shape. * - * Two rules, both deliberate: - * - the verdict's SHA is the record's `headSHA`, never `reviewedSHA`, because - * `validate` has already established they are equal and the driver should not - * have to know that + * Three rules, all deliberate: + * - the verdict's SHA is the record's `headSHA`, the only sha the verdict covers + * - `independence` travels with the verdict so a same-model review is visible as + * such downstream. A policy knob will later decide whether same-model is + * enough; the data has to exist before anything can decide it. * - a record with no verdict yields no `reviewVerdict` at all, rather than a * passing one. "The reviewer replied without a token" is not an approval. * @@ -154,7 +179,9 @@ export function toMergeEvidence(record: Record): PublishDrivers.MergeEvidence { return { headSHA: record.headSHA, reviewVerdict: - record.verdict === undefined ? undefined : { verdict: record.verdict, sha: record.headSHA }, + record.verdict === undefined + ? undefined + : { verdict: record.verdict, sha: record.headSHA, independence: record.independence }, } } @@ -169,6 +196,3 @@ export function evidenceFor(directory: string): Effect.Effect<{ return { evidence: toMergeEvidence(loaded.record) } }) } - -/** Kept so the policy's reason vocabulary stays the single place a caller looks. */ -export type PolicyReason = PublishPolicy.Reason diff --git a/packages/opencode/test/policy/review-record.test.ts b/packages/opencode/test/policy/review-record.test.ts index 3ad63827ce6e..55a085ff7a52 100644 --- a/packages/opencode/test/policy/review-record.test.ts +++ b/packages/opencode/test/policy/review-record.test.ts @@ -16,30 +16,32 @@ const POLICY: PublishPolicy.Policy = { merge: { into: ["dev"], method: "squash", requires: ["gates", "review"], by: ["integrator"] }, } - -// Real files on disk, because the reader's job is to be honest about what is -// actually written — a fixture validated in memory would not exercise the paths, -// the JSON parse, or the "file is absent" case that a merge waits on. - const HEAD = "a".repeat(40) -const OTHER = "b".repeat(40) +const BASE = "b".repeat(40) +const REVIEWER_SESSION = "ses_abc123" const VALID: ReviewRecord.Record = { headSHA: HEAD, - base: OTHER, - reviewer: { harness: "opencode", model: "some-other-family" }, + base: BASE, + reviewer: { harness: "opencode", model: "some-other-family", sessionID: REVIEWER_SESSION }, independence: "independent", verdict: "LGTM", - reviewedSHA: HEAD, findings: [{ file: "src/x.ts", line: 12, severity: "advisory", text: "consider naming this" }], round: 1, } +// Real files on disk, because the reader's job is to be honest about what is +// actually written — a fixture validated in memory would not exercise the paths, +// the JSON parse, or the "file is absent" case a merge waits on. + async function withRecord(contents: unknown | string) { const dir = await mkdtemp(path.join(tmpdir(), "review-record-")) await mkdir(path.join(dir, ".skein"), { recursive: true }) if (contents !== undefined) - await writeFile(path.join(dir, ".skein", "review.json"), typeof contents === "string" ? contents : JSON.stringify(contents)) + await writeFile( + path.join(dir, ".skein", "review.json"), + typeof contents === "string" ? contents : JSON.stringify(contents), + ) return dir } @@ -81,27 +83,36 @@ describe("review record reading", () => { test("an abbreviated head SHA is refused", async () => { // Two commits can share a prefix, so an abbreviation cannot identify a head. - expect(await reasonOf({ ...VALID, headSHA: "abc1234", reviewedSHA: "abc1234" })).toBe( - ReviewRecord.Reason.headNotFullSha, - ) + expect(await reasonOf({ ...VALID, headSHA: "abc1234" })).toBe(ReviewRecord.Reason.headNotFullSha) }) - test("a verdict with no reviewedSHA is refused", async () => { - // The most dangerous shape this record can take: it reads as an approval and - // covers nothing. - const { reviewedSHA: _dropped, ...withoutSha } = VALID - expect(await reasonOf(withoutSha)).toBe(ReviewRecord.Reason.verdictNotForHead) + test("an abbreviated base is refused", async () => { + // The record names a `base..head` range; an abbreviated base cannot anchor one. + expect(await reasonOf({ ...VALID, base: "dev" })).toBe(ReviewRecord.Reason.baseNotFullSha) }) - test("a verdict for a different SHA is refused", async () => { - // A stale review: LGTM for an earlier commit must not cover the new head. - expect(await reasonOf({ ...VALID, reviewedSHA: OTHER })).toBe(ReviewRecord.Reason.verdictNotForHead) + test("a record with no reviewer session is refused", async () => { + // The record lives in the author's own working tree and a model can write a + // file. Until a writer binds this to an authenticated identity it is only a + // claim, but a record that omits it cannot even be compared later. + const { sessionID: _dropped, ...noSession } = VALID.reviewer + expect(await reasonOf({ ...VALID, reviewer: noSession })).toBe(ReviewRecord.Reason.malformed) + }) + + test("a reviewer session with unsafe characters is refused", async () => { + // This value ends up in a comparison against an authenticated identity, so it + // must not be a place to smuggle structure. + for (const sessionID of ["ses/../other", "ses 123", "x".repeat(65), ""]) { + expect(await reasonOf({ ...VALID, reviewer: { ...VALID.reviewer, sessionID } })).toBe( + ReviewRecord.Reason.reviewerSessionInvalid, + ) + } }) test("a record with no verdict at all is valid", async () => { // A round that ended without a verdict is a real outcome worth recording. What // matters is that it does not become a pass, which the mapper tests pin. - const { verdict: _v, reviewedSHA: _s, ...noVerdict } = VALID + const { verdict: _v, ...noVerdict } = VALID const result = await load(await withRecord(noVerdict)) expect(result.ok).toBe(true) }) @@ -112,6 +123,45 @@ describe("review record reading", () => { ).toBe(ReviewRecord.Reason.malformed) }) + test("a blocking finding must name a line", async () => { + // A blocking finding nobody can point at cannot be fixed. + expect( + await reasonOf({ ...VALID, findings: [{ file: "x", severity: "blocking", text: "t" }] }), + ).toBe(ReviewRecord.Reason.findingLineMissing) + }) + + test("an advisory finding may have no line", async () => { + // A design or test-gap finding genuinely has no location. + const result = await load( + await withRecord({ ...VALID, findings: [{ file: "x", severity: "advisory", text: "t" }] }), + ) + expect(result.ok).toBe(true) + }) + + test("zero and negative lines are refused", async () => { + // Observed: `Schema.Number` accepts 0, -3, 1.5, NaN and Infinity. + for (const line of [0, -3]) { + expect(await reasonOf({ ...VALID, findings: [{ file: "x", line, severity: "advisory", text: "t" }] })).toBe( + ReviewRecord.Reason.findingLineInvalid, + ) + } + }) + + test("a non-integer line is refused", async () => { + // NaN and Infinity cannot survive JSON.parse, so they are only reachable + // through the exported validate; the file path still has to reject 1.5. + expect(await reasonOf({ ...VALID, findings: [{ file: "x", line: 1.5, severity: "advisory", text: "t" }] })).toBe( + ReviewRecord.Reason.malformed, + ) + for (const line of [Number.NaN, Number.POSITIVE_INFINITY]) { + const result = ReviewRecord.validate({ + ...VALID, + findings: [{ file: "x", line, severity: "advisory", text: "t" }], + }) + expect(result._tag).toBe("Failure") + } + }) + test("every named reason is distinct", () => { // If two refusals collapsed to one token, a caller could not tell them apart // and a log could not be alerted on them separately. @@ -121,16 +171,25 @@ describe("review record reading", () => { describe("mapping onto the merge driver's evidence", () => { test("the verdict carries the record's headSHA", () => { - // Never `reviewedSHA`: validate has already proven they are equal, and the - // driver should not have to know that. + // The only sha the verdict covers: a review is made for one head. const evidence = ReviewRecord.toMergeEvidence(VALID) expect(evidence.headSHA).toBe(HEAD) - expect(evidence.reviewVerdict).toEqual({ verdict: "LGTM", sha: HEAD }) + expect(evidence.reviewVerdict?.sha).toBe(HEAD) + expect(evidence.reviewVerdict?.verdict).toBe("LGTM") + }) + + test("independence travels with the verdict", () => { + // A same-model review must be visible as same-model, or "the reviewer differs + // from the author" cannot be enforced by anything downstream. + const independent = ReviewRecord.toMergeEvidence(VALID) + expect(independent.reviewVerdict?.independence).toBe("independent") + const sameModel = ReviewRecord.toMergeEvidence({ ...VALID, independence: "same-model" }) + expect(sameModel.reviewVerdict?.independence).toBe("same-model") }) test("no verdict yields no reviewVerdict at all, not a passing one", () => { // The whole point: "the reviewer replied without a token" is not an approval. - const { verdict: _v, reviewedSHA: _s, ...noVerdict } = VALID + const { verdict: _v, ...noVerdict } = VALID const evidence = ReviewRecord.toMergeEvidence(noVerdict) expect(evidence.reviewVerdict).toBeUndefined() expect(evidence.headSHA).toBe(HEAD) @@ -153,14 +212,14 @@ describe("mapping onto the merge driver's evidence", () => { test("the real driver refuses a record with no verdict", () => { // The property that matters, checked against the actual consumer rather than a // type assertion: a review record without a verdict must not become a pass. - const { verdict: _v, reviewedSHA: _s, ...noVerdict } = VALID + const { verdict: _v, ...noVerdict } = VALID const refusal = PublishDrivers.mayMerge({ policy: POLICY, target: "dev", actor: "integrator", evidence: { ...ReviewRecord.toMergeEvidence(noVerdict), - mergeBase: "b".repeat(40), + mergeBase: BASE, gates: { passed: true, sha: HEAD }, }, }) @@ -176,7 +235,24 @@ describe("mapping onto the merge driver's evidence", () => { actor: "integrator", evidence: { ...ReviewRecord.toMergeEvidence(VALID), - mergeBase: "b".repeat(40), + mergeBase: BASE, + gates: { passed: true, sha: HEAD }, + }, + }) + expect(ok).toEqual({ ok: true }) + }) + + test("the driver still refuses a same-model verdict it was not told to accept", () => { + // `independence` is carried, not yet enforced: today's driver accepts a + // same-model LGTM. Pinning today's behaviour so the later policy knob is a + // deliberate change rather than a silent drift. + const ok = PublishDrivers.mayMerge({ + policy: POLICY, + target: "dev", + actor: "integrator", + evidence: { + ...ReviewRecord.toMergeEvidence({ ...VALID, independence: "same-model" }), + mergeBase: BASE, gates: { passed: true, sha: HEAD }, }, })