feat(policy): merge driver refuses an author-written verdict (review-on-done 1b.3)

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.
 

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

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(", ")})`)

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,
+          },
   }
 }
 

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")
   })