diff --git a/openspec/changes/review-on-done/tasks.md b/openspec/changes/review-on-done/tasks.md index 79792122fef5..0a899354140c 100644 --- a/openspec/changes/review-on-done/tasks.md +++ b/openspec/changes/review-on-done/tasks.md @@ -26,7 +26,7 @@ true. authenticated session identity of the REVIEWER, never by a model's own file write. - [ ] 1b.2 Deny `edit` and `write` of `.skein/review.json` for ordinary sessions (the fenced-path rules, same mechanism as the other protected paths), with a bypass-shape test. -- [ ] 1b.3 The merge driver refuses a verdict whose `reviewer.sessionID` is the author's session, and +- [x] 1b.3 (driver side: `mayMerge` takes `authorSessionID`/`requireIndependent`; no caller passes the author yet, the merge CLI that will is not written) The merge driver refuses a verdict whose `reviewer.sessionID` is the author's session, and surfaces `independence` so a policy can require `independent`. - [ ] 1b.4 Observed-red test: an author that writes a forged LGTM into its own tree is refused. diff --git a/packages/opencode/src/policy/drivers.ts b/packages/opencode/src/policy/drivers.ts index f5348d24cf52..86f4474f1b90 100644 --- a/packages/opencode/src/policy/drivers.ts +++ b/packages/opencode/src/policy/drivers.ts @@ -137,6 +137,8 @@ export interface MergeEvidence { readonly verdict: "LGTM" | "NEEDS_WORK" readonly sha: string readonly independence?: "independent" | "same-model" + /** Who wrote the verdict, as the record claims. Compared with the author's session by `mayMerge`. */ + readonly reviewerSessionID?: string } /** The merge base of target and head, as the caller computed it. The executor recomputes it. */ readonly mergeBase?: string @@ -180,6 +182,14 @@ export function mayMerge(input: { target: string actor: string evidence: MergeEvidence + /** + * The session that wrote the change. When given, the verdict must name a + * different reviewer session: the record sits in the author's working tree, so + * without this an author's own LGTM would be indistinguishable from a review. + */ + authorSessionID?: string + /** Refuse a verdict whose `independence` is not "independent" (a same-model review). */ + requireIndependent?: boolean }): Refusal { if (!input.policy.merge.by.includes(input.actor)) return { ok: false, reason: `"${input.actor}" may not merge (${input.policy.merge.by.join(", ")})` } @@ -207,6 +217,15 @@ export function mayMerge(input: { ok: false, reason: `the verdict covers ${input.evidence.reviewVerdict.sha} but the head is ${input.evidence.headSHA}`, } + const verdict = input.evidence.reviewVerdict + if (input.authorSessionID !== undefined) { + if (!verdict.reviewerSessionID) + return { ok: false, reason: "the verdict does not say which session reviewed it, so it cannot be told apart from the author's own" } + if (verdict.reviewerSessionID === input.authorSessionID) + return { ok: false, reason: "the verdict was written by the author's own session, not by a reviewer" } + } + if (input.requireIndependent && verdict.independence !== "independent") + return { ok: false, reason: `the review is ${verdict.independence ?? "of unrecorded independence"}, and an independent review is required` } for (const kind of input.policy.merge.requires) { if (kind === "review") continue // checked above, unconditionally: a merge never goes without a verdict const item = kind === "gates" ? input.evidence.gates : input.evidence.ci diff --git a/packages/opencode/src/policy/merge-run.ts b/packages/opencode/src/policy/merge-run.ts index 2f0b1e150479..5d807e1405a9 100644 --- a/packages/opencode/src/policy/merge-run.ts +++ b/packages/opencode/src/policy/merge-run.ts @@ -82,6 +82,9 @@ export async function runMerge(input: { remote: string actor: string evidence: PublishDrivers.MergeEvidence + /** See `mayMerge`: the author's session, so the author cannot approve itself. */ + authorSessionID?: string + requireIndependent?: boolean /** Skip publication and leave the result in the throwaway worktree's object store only. */ dryRun?: boolean git?: Git @@ -92,7 +95,11 @@ export async function runMerge(input: { // The decision, with the same inputs the caller will log. The remote must be one // the policy was verified against (the loader matched its URL to `repo`). - const decision = PublishDrivers.mayMerge({ policy: input.policy, target, actor: input.actor, evidence }) + const decision = PublishDrivers.mayMerge({ policy: input.policy, target, actor: input.actor, + evidence, + authorSessionID: input.authorSessionID, + requireIndependent: input.requireIndependent, + }) if (!decision.ok) return refuse("decision", decision.reason) if (!PublishDrivers.isSafeRefName(remote) || !input.policy.push.remotes.includes(remote)) return refuse("decision", `remote "${remote}" is not one the policy grants (${input.policy.push.remotes.join(", ")})`) diff --git a/packages/opencode/src/policy/review-record.ts b/packages/opencode/src/policy/review-record.ts index 672c431730c4..c0343ae94ee4 100644 --- a/packages/opencode/src/policy/review-record.ts +++ b/packages/opencode/src/policy/review-record.ts @@ -181,7 +181,12 @@ export function toMergeEvidence(record: Record): PublishDrivers.MergeEvidence { reviewVerdict: record.verdict === undefined ? undefined - : { verdict: record.verdict, sha: record.headSHA, independence: record.independence }, + : { + verdict: record.verdict, + sha: record.headSHA, + independence: record.independence, + reviewerSessionID: record.reviewer.sessionID, + }, } } diff --git a/packages/opencode/test/policy/drivers.test.ts b/packages/opencode/test/policy/drivers.test.ts index af6d6459a947..6a1c815353d1 100644 --- a/packages/opencode/test/policy/drivers.test.ts +++ b/packages/opencode/test/policy/drivers.test.ts @@ -163,6 +163,36 @@ describe("merge gate", () => { expect(may()).toEqual({ ok: true }) }) + describe("review authenticity", () => { + const verdictBy = (reviewerSessionID?: string, independence: "independent" | "same-model" = "independent") => ({ + ...ok, + reviewVerdict: { verdict: "LGTM" as const, sha: HEAD, independence, reviewerSessionID }, + }) + + test("refuses a verdict written by the author's own session", () => { + expect(refusal(may({ evidence: verdictBy("ses_author"), authorSessionID: "ses_author" }))).toContain("author's own session") + }) + + test("accepts a verdict from a different session", () => { + expect(may({ evidence: verdictBy("ses_reviewer"), authorSessionID: "ses_author" })).toEqual({ ok: true }) + }) + + test("refuses a verdict that does not say who reviewed it once the author is known", () => { + expect(refusal(may({ evidence: verdictBy(undefined), authorSessionID: "ses_author" }))).toContain("which session reviewed") + expect(refusal(may({ evidence: verdictBy(""), authorSessionID: "ses_author" }))).toContain("which session reviewed") + }) + + test("without an author given the check does not apply (callers that predate it)", () => { + expect(may({ evidence: verdictBy("ses_author") })).toEqual({ ok: true }) + }) + + test("requireIndependent refuses a same-model review and an unrecorded one, and accepts an independent one", () => { + expect(refusal(may({ evidence: verdictBy("r", "same-model"), requireIndependent: true }))).toContain("independent review is required") + expect(refusal(may({ requireIndependent: true }))).toContain("unrecorded") + expect(may({ evidence: verdictBy("r"), requireIndependent: true })).toEqual({ ok: true }) + }) + }) + test("refuses an actor the policy does not list", () => { expect(refusal(may({ actor: "model" }))).toContain("may not merge") })