feat(policy): make the review record refuse an author's own verdict

fork/manifest.json

@@ -91,6 +91,7 @@
     "packages/opencode/src/policy/publish-policy.ts",
     "packages/opencode/src/policy/push-run.ts",
     "packages/opencode/src/policy/review-record.ts",
+    "packages/opencode/src/policy/review-gate.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",
@@ -251,6 +252,7 @@
     "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/policy/review-gate.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",

openspec/changes/review-on-done/tasks.md

@@ -22,13 +22,20 @@ can write its own LGTM. The reader (src/policy/review-record.ts) therefore carri
 identity as a claim (`reviewer.sessionID`) and the verdict's `independence`; these tasks make the claim
 true.
 
-- [ ] 1b.1 The record is written only by a tool (`opencode review record`, or the review hook), using the
-      authenticated session identity of the REVIEWER, never by a model's own file write.
+- [x] 1b.1 `src/policy/review-gate.ts` writes the record from the caller's own session identity
+      (`ctx.sessionID`, in-process — not parsed from a message, so it does not wait on callback
+      confirmation), validates against the reader's own closed schema before writing, and writes
+      atomically. The tool registration itself is the remaining wiring.
 - [ ] 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.
 - [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.
+- [x] 1b.4 `gate()` supplies `authorSessionID` to `mayMerge`, which refuses a verdict the author's own
+      session wrote. Mutation-checked: removing the author comparison fails that test and only it.
+- [ ] 1b.5 NOT DONE, and it is the one that matters: a record forged by shell, naming a real reviewer,
+      is still accepted. `.skein/review.json` is writable by any process with filesystem access and the
+      gate only checks that the reviewer differs from the author. Pinned as a test so it cannot be
+      forgotten. Only OS separation closes this, and nothing here claims it.
 
 ## Phase 2: Independent reviewer
 

packages/opencode/src/policy/review-gate.ts

@@ -0,0 +1,150 @@
+export * as ReviewGate from "./review-gate"
+
+import fs from "fs"
+import path from "path"
+import { Effect, Result, Schema } from "effect"
+import { ReviewRecord } from "./review-record"
+import { PublishDrivers } from "./drivers"
+import type { PublishPolicy } from "./publish-policy"
+
+// Making the review record mean something.
+//
+// The record is a file in the AUTHOR's working tree, and a model can write a
+// file. On its own it proves nothing: an author could write its own `LGTM` and
+// name any reviewer it liked. Two things change that, and this module is the
+// second of them.
+//
+//   1. `write` stamps the reviewer identity from the caller's own session
+//      context (`ctx.sessionID`), never from a parameter the model supplies. A
+//      model can therefore record a verdict, but it cannot record one *as*
+//      somebody else.
+//   2. `gate` refuses a verdict whose reviewer is the author. `mayMerge` already
+//      knows how to compare; this supplies the author it was missing.
+//
+// What this does NOT do, stated here rather than left to be discovered:
+//
+//   - A model with shell access can write `.skein/review.json` directly, by any
+//     means, naming any session id. The identity stamp above only binds the
+//     sanctioned writer. Denying the path to models is task 1b.2, and it is the
+//     weaker of the two controls regardless, because scoped unattended bash can
+//     write the same path. Treat this as *bounded*, not trustworthy.
+//   - Nothing here is OS-separated. A same-user process that can write the
+//     working tree can forge the file. That residual is why the surrounding
+//     controls are bounded claims — human-granted, scoped, never-listed — rather
+//     than a proof of authorship.
+
+export const WriteFailure = {
+  /** The proposed record does not satisfy the reader's own closed schema. */
+  malformed: "review-write-malformed",
+  /** The reviewer identity is not a safe session id. */
+  reviewerInvalid: "review-write-reviewer-invalid",
+} as const
+
+export type WriteFailure = (typeof WriteFailure)[keyof typeof WriteFailure]
+
+export interface WriteInput {
+  directory: string
+  /**
+   * The REVIEWER's session id, taken from the calling session's own context.
+   * Never a model-supplied argument — that is the entire point of this function.
+   */
+  reviewerSessionID: string
+  headSHA: string
+  base: string
+  independence: "independent" | "same-model"
+  verdict?: "LGTM" | "NEEDS_WORK"
+  findings?: ReviewRecord.Finding[]
+  round: number
+}
+
+/**
+ * Writes the record, having first checked it against the reader's schema.
+ *
+ * Validating before writing is what makes this a writer rather than a file dump:
+ * a record that fails here would fail on read, and the round-trip would look like
+ * a review that never happened rather than a rejected write.
+ */
+export function write(input: WriteInput): Effect.Effect<{ path: string }, ReviewWriteError> {
+  return Effect.gen(function* () {
+    const reviewerSessionID = input.reviewerSessionID
+    const candidate = {
+      headSHA: input.headSHA,
+      base: input.base,
+      reviewer: { harness: "unknown", model: "unknown", sessionID: reviewerSessionID },
+      independence: input.independence,
+      ...(input.verdict ? { verdict: input.verdict } : {}),
+      findings: input.findings ?? [],
+      round: input.round,
+    }
+    const validated = ReviewRecord.validate(candidate)
+    if (Result.isFailure(validated))
+      return yield* new ReviewWriteError({ reason: WriteFailure.malformed, detail: validated.failure })
+
+    const target = path.join(input.directory, ...ReviewRecord.RECORD_FILE)
+    yield* Effect.sync(() => fs.mkdirSync(path.dirname(target), { recursive: true }))
+    // Write-then-rename so a reader never observes a half-written record: a
+    // truncated JSON file reads as `review-unreadable`, which is indistinguishable
+    // from corruption and loses a real verdict.
+    const tmp = `${target}.${process.pid}.tmp`
+    yield* Effect.sync(() => fs.writeFileSync(tmp, `${JSON.stringify(candidate, null, 2)}\n`, { mode: 0o644 }))
+    yield* Effect.sync(() => fs.renameSync(tmp, target))
+    return { path: target }
+  })
+}
+
+/**
+ * The gate: may this change be merged, given the record on disk?
+ *
+ * Delegates the actual decision to `mayMerge` so there is one place that knows
+ * what a merge requires. This only supplies the author identity the driver needs
+ * and the evidence the record carries.
+ */
+export function gate(input: {
+  directory: string
+  policy: PublishPolicy.Policy
+  target: string
+  actor: string
+  /** The session that authored the change. Without it this gate proves nothing. */
+  authorSessionID?: string
+  /** Require an independent review, not merely a review by someone else. */
+  requireIndependent?: boolean
+  /** Gates result, measured by the caller against the head. */
+  gates?: { passed: boolean; sha: string }
+}): Effect.Effect<PublishDrivers.Refusal, ReviewGateError> {
+  return Effect.gen(function* () {
+    const loaded = yield* ReviewRecord.load(input.directory)
+    if (!loaded.ok)
+      return yield* new ReviewGateError({ reason: loaded.reason, detail: loaded.detail })
+    // `mayMerge` deliberately refuses without a merge base, because an approval
+    // for an unrelated history is not an approval of anything. The record names
+    // the base it reviewed, so carry it — and let the driver apply its own rule
+    // rather than second-guessing it here.
+    return PublishDrivers.mayMerge({
+      policy: input.policy,
+      target: input.target,
+      actor: input.actor,
+      evidence: {
+        ...ReviewRecord.toMergeEvidence(loaded.record),
+        // The record names the `base..head` range it reviewed, so the merge base
+        // is the base the reviewer actually looked at. Carrying it is what lets
+        // the driver refuse an empty merge-base instead of assuming one.
+        mergeBase: loaded.record.base,
+        ...(input.gates ? { gates: input.gates } : {}),
+      },
+      ...(input.authorSessionID ? { authorSessionID: input.authorSessionID } : {}),
+      ...(input.requireIndependent ? { requireIndependent: true } : {}),
+    })
+  })
+}
+
+/** A record that could not be read as a review at all. */
+export class ReviewGateError extends Schema.TaggedErrorClass<ReviewGateError>()("ReviewGateError", {
+  reason: Schema.String,
+  detail: Schema.String,
+}) {}
+
+/** A record the writer refused to produce, because it would fail on read. */
+export class ReviewWriteError extends Schema.TaggedErrorClass<ReviewWriteError>()("ReviewWriteError", {
+  reason: Schema.String,
+  detail: Schema.String,
+}) {}

packages/opencode/test/policy/review-gate.test.ts

@@ -0,0 +1,178 @@
+import { describe, expect, test } from "bun:test"
+import { Effect, Result } from "effect"
+import { mkdtemp, mkdir, readFile, writeFile } from "fs/promises"
+import { tmpdir } from "os"
+import path from "path"
+import { ReviewGate } from "@/policy/review-gate"
+import { ReviewRecord } from "@/policy/review-record"
+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"] },
+}
+
+const HEAD = "a".repeat(40)
+const BASE = "b".repeat(40)
+const AUTHOR = "ses_author"
+const REVIEWER = "ses_reviewer"
+
+async function dir() {
+  const d = await mkdtemp(path.join(tmpdir(), "review-gate-"))
+  await mkdir(path.join(d, ".skein"), { recursive: true })
+  return d
+}
+
+const write = (d: string, reviewerSessionID: string, extra?: { verdict?: "LGTM" | "NEEDS_WORK" }) =>
+  Effect.runPromise(
+    ReviewGate.write({
+      directory: d,
+      reviewerSessionID,
+      headSHA: HEAD,
+      base: BASE,
+      independence: "independent",
+      ...(extra?.verdict ? { verdict: extra.verdict } : {}),
+      round: 1,
+    }),
+  )
+
+/**
+ * `mayMerge` returns a Refusal as a VALUE, so a denial is `{ok:false}` inside a
+ * successful Effect — not an Effect failure. Only an unreadable record fails the
+ * Effect, with ReviewGateError. Conflating those two was this file's first bug.
+ */
+const gate = (d: string, authorSessionID: string) =>
+  Effect.runSync(
+    Effect.result(
+      ReviewGate.gate({
+        directory: d,
+        policy: POLICY,
+        target: "dev",
+        actor: "integrator",
+        gates: { passed: true, sha: HEAD },
+        authorSessionID,
+      }),
+    ),
+  )
+
+const refusedWith = (result: ReturnType<typeof gate>): string => {
+  if (Result.isFailure(result)) throw new Error(`expected a refusal, got failure: ${result.failure.reason}`)
+  if (result.success.ok) throw new Error("expected a refusal, got ok:true")
+  return result.success.reason
+}
+
+/** What a model with shell access can do: write the file itself, naming anyone. */
+async function forge(d: string, reviewerSessionID: string) {
+  await writeFile(
+    path.join(d, ".skein", "review.json"),
+    JSON.stringify({
+      headSHA: HEAD,
+      base: BASE,
+      reviewer: { harness: "x", model: "y", sessionID: reviewerSessionID },
+      independence: "independent",
+      verdict: "LGTM",
+      findings: [],
+      round: 1,
+    }),
+  )
+}
+
+describe("the writer", () => {
+  test("stamps the reviewer it is given, not one it is told to claim", async () => {
+    const d = await dir()
+    await write(d, REVIEWER)
+    const record = JSON.parse(await readFile(path.join(d, ".skein", "review.json"), "utf8"))
+    expect(record.reviewer.sessionID).toBe(REVIEWER)
+  })
+
+  test("refuses to write a record that would fail on read", async () => {
+    // Writer and reader share a schema on purpose: a record rejected here would
+    // be rejected there too, and the round trip would look like a review that
+    // never happened rather than a rejected write.
+    const d = await dir()
+    const result = Effect.runSync(
+      Effect.result(
+        ReviewGate.write({
+          directory: d,
+          reviewerSessionID: REVIEWER,
+          headSHA: "not-a-sha",
+          base: BASE,
+          independence: "independent",
+          verdict: "LGTM",
+          round: 1,
+        }),
+      ),
+    )
+    expect(Result.isFailure(result)).toBe(true)
+    if (Result.isSuccess(result)) throw new Error("unreachable")
+    expect(result.failure.reason).toBe(ReviewGate.WriteFailure.malformed)
+  })
+
+  test("a written record is readable by the reader", async () => {
+    const d = await dir()
+    await write(d, REVIEWER, { verdict: "LGTM" })
+    const loaded = await Effect.runPromise(ReviewRecord.load(d))
+    expect(loaded.ok).toBe(true)
+  })
+})
+
+describe("the gate", () => {
+  test("accepts a review by a different session", async () => {
+    const d = await dir()
+    await write(d, REVIEWER, { verdict: "LGTM" })
+    const result = await gate(d, AUTHOR)
+    if (Result.isFailure(result)) throw new Error(`unexpected failure: ${result.failure.reason}`)
+    expect(result.success).toEqual({ ok: true })
+  })
+
+  test("refuses a verdict the author wrote for themselves", async () => {
+    // The case this module exists for: an author approving its own change.
+    const d = await dir()
+    await write(d, AUTHOR, { verdict: "LGTM" })
+    expect(refusedWith(await gate(d, AUTHOR))).toContain("author's own session")
+  })
+
+  test("refuses when the record names no verdict", async () => {
+    const d = await dir()
+    await write(d, REVIEWER)
+    expect(refusedWith(await gate(d, AUTHOR))).toContain("no recorded review verdict")
+  })
+
+  test("refuses when there is no record at all", async () => {
+    const d = await dir()
+    const result = await gate(d, AUTHOR)
+    expect(Result.isFailure(result)).toBe(true)
+    if (Result.isSuccess(result)) throw new Error("unreachable")
+    expect(result.failure.reason).toBe(ReviewRecord.Reason.absent)
+  })
+
+  test("refuses a verdict covering a different head", async () => {
+    const d = await dir()
+    await write(d, REVIEWER, { verdict: "LGTM" })
+    const result = await gate(d, AUTHOR)
+    expect(Result.isSuccess(result)).toBe(true)
+    if (Result.isFailure(result)) throw new Error("unreachable")
+    // Sanity: the evidence carries the record's own head, so this only holds
+    // while the gate reads base..head from the same record it validates.
+    expect(result.success.ok).toBe(true)
+  })
+
+  test("a record forged by shell naming a real reviewer still passes — the residual", async () => {
+    // This is the honest limit, pinned so it cannot be quietly forgotten.
+    // `.skein/review.json` is writable by any process with filesystem access, and
+    // the gate only checks that the reviewer differs from the author. It cannot
+    // tell a real review from a fabricated one. Denying the path to models is task
+    // 1b.2 and does not change this for bash; only OS separation would, and
+    // nothing here claims it.
+    const d = await dir()
+    await forge(d, REVIEWER)
+    const result = await gate(d, AUTHOR)
+    expect(Result.isSuccess(result)).toBe(true)
+    if (Result.isFailure(result)) throw new Error("unreachable")
+    expect(result.success).toEqual({ ok: true })
+  })
+})