From 7a4a8827d2b4b115d17ed61c11abcbc62e1d32eb Mon Sep 17 00:00:00 2001 From: AndreasS Date: Sat, 3 Oct 2026 20:41:20 +0200 Subject: [PATCH] feat(policy): a review gate that refuses an author's own verdict MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The reader shipped in abadec9aea and enforces nothing: the record is a plain file in the author's working tree, and a model can write a file. This adds the two pieces that make it mean something, and is explicit about the piece it cannot fix. write() stamps the reviewer from the caller's own session identity, never from a model-supplied argument, so a model can record a verdict but cannot record one *as* somebody else. It validates against the reader's own closed schema before writing — a record rejected here would be rejected on read too, and the round trip would look like a review that never happened rather than a refused write — and renames into place so a reader never sees a truncated file. gate() supplies the authorSessionID that mayMerge was already asking for but that no caller passed, so #111 stops being inert. It carries the record's own base as mergeBase, letting the driver apply its own unrelated-histories rule rather than second-guessing it. Does NOT close the hole, and this is pinned as a test rather than left in a comment: a record forged by shell naming a real reviewer is still accepted. The gate compares two ids; it cannot tell a real review from a fabricated one. Deny the path to models is task 1b.2 and does not change this for bash. Only OS separation would, and nothing here claims it. That is why the surrounding controls are bounded claims rather than proof of authorship. Identity comes from ctx.sessionID — in-process, not parsed from a message — so this does not wait on callback confirmation, which is not merged. Mutation-checked, each failing only the test that owns it: dropping the author comparison, and having the writer skip validation. test/policy 166 pass / 8 skip / 0 fail, typecheck clean, fork:verify clean. --- fork/manifest.json | 2 + openspec/changes/review-on-done/tasks.md | 13 +- packages/opencode/src/policy/review-gate.ts | 150 +++++++++++++++ .../opencode/test/policy/review-gate.test.ts | 178 ++++++++++++++++++ 4 files changed, 340 insertions(+), 3 deletions(-) create mode 100644 packages/opencode/src/policy/review-gate.ts create mode 100644 packages/opencode/test/policy/review-gate.test.ts diff --git a/fork/manifest.json b/fork/manifest.json index cb59d6e2be0e..9fb757c8882e 100644 --- a/fork/manifest.json +++ b/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", diff --git a/openspec/changes/review-on-done/tasks.md b/openspec/changes/review-on-done/tasks.md index 0a899354140c..5ec57a2b778e 100644 --- a/openspec/changes/review-on-done/tasks.md +++ b/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 diff --git a/packages/opencode/src/policy/review-gate.ts b/packages/opencode/src/policy/review-gate.ts new file mode 100644 index 000000000000..c042a0f1e132 --- /dev/null +++ b/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 { + 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", { + 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", { + reason: Schema.String, + detail: Schema.String, +}) {} \ No newline at end of file diff --git a/packages/opencode/test/policy/review-gate.test.ts b/packages/opencode/test/policy/review-gate.test.ts new file mode 100644 index 000000000000..6d1d38c439a3 --- /dev/null +++ b/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): 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 }) + }) +}) \ No newline at end of file