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