diff --git a/CHANGELOG.md b/CHANGELOG.md index 8065b89..80743ae 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,6 +3,43 @@ All notable changes to this project will be documented in this file. Format follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). +## [Unreleased] + +### Changed + +- The reviewer gate now records its verdict in a first-class, signed + `reviewer_gate` snapshot on the review file instead of the shared + `review.verdict` field. The snapshot carries its own reviewer, attempt, + verdict, reviewed commit range, and an HMAC signature keyed by a + machine-local secret (`~/.repokernel/gate.key`). Because no other command + writes that key, `rk review-sprint`, `rk review-panel`, `rk review-verdict`, + and `rk re-review` can no longer clear or overwrite a recorded gate decision. +- `rk close` (and every path through it — `rk ship`, autonomous runs, epic + close) now requires **both** the built-in/panel review verdict **and** the + gate snapshot to be green, using most-restrictive-wins composition. The + snapshot must be present, signature-valid, bound to the current review + attempt, accepted, and content-fresh against its reviewed commit — always on, + not bypassable with `--skip-checks`. + +### Added + +- `reviewer_gate` snapshot integrity validation. Live `rk validate` flags a + shipped, gate-required sprint whose **present** snapshot is not accepted, on a + stale attempt, or base-drifted (`REVIEWER_GATE_NOT_ACCEPTED`, + `REVIEWER_GATE_ATTEMPT_MISMATCH`, `REVIEWER_GATE_STALE`). A **missing** + snapshot on already-shipped history is audit-scope only + (`rk validate --audit` → `REVIEWER_GATE_MISSING`), so upgrading a repo with + pre-snapshot shipped sprints does not flood live validation. Signature + authenticity is enforced at close, the only path that ships. +- `rk status` surfaces the gate verdict and attempt alongside the review. + +### Migration + +- Existing review files parse unchanged (the field is optional). A project that + configures a reviewer gate and has a sprint already in review from before this + change has no snapshot yet; run `rk review-gate ` once to record + one before closing. Projects with no configured reviewer gate are unaffected. + ## [1.32.0] - 2026-06-02 Workflow ergonomics, validation, and ship-safety fixes from a dogfooding diff --git a/docs/trust.md b/docs/trust.md index 9af9db2..7ee1ebc 100644 --- a/docs/trust.md +++ b/docs/trust.md @@ -71,6 +71,30 @@ A grant on the host repo applies to its worktrees. rk reads the worktree's and looks up the trust grant under either path. You grant once, every worktree under that repo inherits. +## Reviewer gate signing key + +When a reviewer gate runs, it records its verdict in a signed `reviewer_gate` +snapshot on the review file. The signature is an HMAC keyed by a machine-local +secret at `~/.repokernel/gate.key` (mode `600`), minted automatically on the +first gate run. It is co-located with the trust file and on the same side of the +boundary: it never enters the repo, so a snapshot committed into a review file +cannot be forged, lifted into another review, or replayed across commit ranges. + +`rk close` verifies the signature against this key. Consequences: + +- Close happens on the machine that ran the gate (which holds the key), so a + legitimate close always verifies. Closing on a machine without the key fails + closed with `REVIEWER_GATE_SIGNATURE_INVALID` — re-run the gate there. +- The path is overridable via `REPOKERNEL_GATE_SECRET_FILE`. When unset it + follows the trust file's directory, so isolating `REPOKERNEL_TRUST_FILE` + (e.g. in tests) isolates the gate key too. +- `rk validate` checks snapshot structure without the key, so CI catches + structural tampering even though it cannot verify the signature. A present + snapshot that is not accepted / on a stale attempt / base-drifted is flagged + at **live** scope; a **missing** snapshot on already-shipped history is + flagged only under `rk validate --audit` (a past close cannot be backfilled, + and forward enforcement happens at close). + ## Error kinds | Kind | When | What to do | diff --git a/packages/cli/src/commands/lifecycle.ts b/packages/cli/src/commands/lifecycle.ts index 62ae090..958b411 100644 --- a/packages/cli/src/commands/lifecycle.ts +++ b/packages/cli/src/commands/lifecycle.ts @@ -24,6 +24,7 @@ import { runConfiguredChecksFromConfig } from '../lifecycle/checks.js'; import { isWorktreeCheckout } from '../lifecycle/controlPaths.js'; import { classifySprintDiff, inScopeFiles } from '../lifecycle/diffClassifier.js'; import { isExternalAgentEnvironment } from '../lifecycle/executionOwnership.js'; +import { evaluateReviewerGate } from '../lifecycle/gateEnforce.js'; import { changedFilesForSprint, changedFilesSince, @@ -670,6 +671,22 @@ export async function runCloseCommand( 'accept the review before closing', ); } + // Reviewer-gate snapshot gate — independent of review.verdict and always + // on (not bypassable with --skip-checks). When the project configures a + // reviewer gate for a review-required sprint, close additionally requires + // a present, signed, current-attempt, accepted, fresh snapshot. The + // snapshot lives in a field no other command writes, so review-sprint / + // panel / review-verdict / re-review cannot clear or satisfy it. + const gateEval = await evaluateReviewerGate({ + checkPath: await resolveCloseCheckPath(id, cwd), + config: outcome.config, + sprint, + review, + configFile: relative(cwd, outcome.configPath), + }); + if (!gateEval.ok) { + return err(gateEval.block.code, gateEval.block.message, gateEval.block.hint); + } // Strong binding for gated reviews: end_sha pins the exact reviewed commit. // Any in-scope file that changed since then is unreviewed CONTENT (a // same-file edit keeps the filename set identical, so the file-set guard @@ -731,6 +748,25 @@ export async function runCloseCommand( ); } } + } else if (sprint.review_id) { + // Not review-required by current policy — but a recorded reviewer_gate + // snapshot is a commitment that a later config change (opt out / raise the + // threshold) must not be able to void. Enforce the gate lane on its own; + // the built-in verdict lane is not required when policy does not require + // review. `evaluateReviewerGate` no-ops when no snapshot is present. + const review = outcome.graph.reviews.get(sprint.review_id); + if (review?.reviewer_gate) { + const gateEval = await evaluateReviewerGate({ + checkPath: await resolveCloseCheckPath(id, cwd), + config: outcome.config, + sprint, + review, + configFile: relative(cwd, outcome.configPath), + }); + if (!gateEval.ok) { + return err(gateEval.block.code, gateEval.block.message, gateEval.block.hint); + } + } } if (opts.dryRun) return dryRunOk('close', { id, from: sprint.status, to: 'shipped' }); diff --git a/packages/cli/src/commands/reviewGate.ts b/packages/cli/src/commands/reviewGate.ts index d31aaf7..875f59a 100644 --- a/packages/cli/src/commands/reviewGate.ts +++ b/packages/cli/src/commands/reviewGate.ts @@ -51,6 +51,15 @@ export async function runReviewerGateForLinkedSprint( const review = outcome.graph.reviews.get(sprint.review_id); if (!review) return blocked(`review ${sprint.review_id} not found`); + // Refuse to run the gate against a review that targets a different sprint — + // it would overwrite that review's snapshot (poisoning the legitimate file). + if (review.sprint_id !== sprint.id) { + return blocked( + `review ${review.id} targets sprint ${review.sprint_id}, not ${sprint.id}`, + `link a review for ${sprint.id} (rk review-create --sprint ${sprint.id})`, + ); + } + // Resolve the gate from the LINKED review's reviewer, not the project default — // `review-create --reviewer ` may have stamped a specific reviewer, and // the gate must run the reviewer the review was actually created for. @@ -141,10 +150,19 @@ function formatGateResult( sprint_id: ctx.sprintId, review_id: ctx.reviewId, reviewer: ctx.reviewerName, + // `verdict` is the GATE verdict (one of two close lanes), NOT a + // close-ready signal. Surface it unambiguously and spell out the next + // step so automation does not treat `accepted` as "ready to close". + gate_verdict: result.verdict, verdict: result.verdict, + reviewer_gate_recorded: true, findings: result.findings, ...(result.summary ? { summary: result.summary } : {}), ...(result.failSoft ? { fail_soft: result.failSoft } : {}), + next_actions: + result.verdict === 'accepted' + ? [`rk review-sprint ${ctx.sprintId}`, `rk close ${ctx.sprintId}`] + : [`rk review-gate ${ctx.sprintId}`], }), stderr: '', }; @@ -166,7 +184,7 @@ function formatGateResult( lines.push( '', result.verdict === 'accepted' - ? `Next: rk close ${ctx.sprintId}` + ? `Next: rk review-sprint ${ctx.sprintId} (built-in lane), then rk close ${ctx.sprintId}` : `Fix findings, commit, then re-run the gate: rk review-gate ${ctx.sprintId}`, ); return { exitCode: result.exitCode, stdout: `${lines.join('\n')}\n`, stderr: '' }; diff --git a/packages/cli/src/commands/run.ts b/packages/cli/src/commands/run.ts index f50d835..e83d0c7 100644 --- a/packages/cli/src/commands/run.ts +++ b/packages/cli/src/commands/run.ts @@ -5,6 +5,7 @@ import { type Config, type EpicId, effectiveReviewer, + effectiveReviewRequired, HALT_REASONS, loadProject, meetsThreshold, @@ -13,6 +14,7 @@ import { RepoKernelError, type Run, type RunSprintRecord, + resolveReviewerGate, runValidators, type SprintId, } from '@repokernel/core'; @@ -54,6 +56,7 @@ import { import { isoNow } from '../templates/time.js'; import { buildChain } from './chain.js'; import { runCloseCommand, runReviewCommand, runStartCommand } from './lifecycle.js'; +import { runReviewSprintCommand } from './reviewSprint.js'; import { type EpicPreflightResult, epicPreflight, renderPreflight } from './runPreflight.js'; import type { CommandResult } from './validate.js'; @@ -403,6 +406,25 @@ export async function runRunCommand(opts: RunCommandOptions): Promise + s.epic_id === opts.epicId && + !['shipped', 'cancelled'].includes(s.status) && + effectiveReviewRequired(s, outcome.config), + ); + if (gated) { + return err( + 'PARALLEL_GATE_UNSUPPORTED', + `epic ${opts.epicId} configures a reviewer gate and ${gated.id} requires review, but parallel runs do not execute the reviewer gate`, + `run sequentially (omit --parallel / set execution_strategy: sequential), or gate + close each sprint manually with rk review-gate`, + ); + } + } return executeParallelRunLoop( run, opRoot, @@ -771,8 +793,63 @@ async function executeRunLoop( return reviewResult; } - // runReviewCommand auto-commits its review-side `.repokernel/` mutations, - // leaving a clean tree for runCloseCommand (which requires one) below. + // Built-in review lane. `rk review` runs the reviewer gate (when one is + // configured), which records ONLY its signed snapshot and leaves + // review.verdict pending. Evaluate the built-in rules so the composed + // close gate (snapshot + review.verdict) can pass, then commit the verdict + // so the tree is clean for runCloseCommand. + const evalResult = await runReviewSprintCommand(sprint.id, { + cwd: executionCwd, + dryRun: false, + json: false, + }); + if (evalResult.exitCode !== EXIT_OK) { + run = await updateRun( + run.id, + { + status: 'failed', + halt_reason: `${HALT_REASONS.REVIEW_FAILED}:${sprint.id}`, + ended_at: isoNow(), + }, + opRoot, + ); + await releaseLane(`epic-${run.epic_id}`, opRoot, run.id); + return evalResult; + } + const evalOutcome = await loadProject({ cwd: executionCwd }); + const evalReview = evalOutcome.ok + ? evalOutcome.graph.reviews.get(evalOutcome.graph.sprints.get(sprint.id)?.review_id ?? '') + : undefined; + if (evalReview?.file) { + await stagePathsAndCommit( + executionCwd, + [join(executionCwd, evalReview.file), join(executionCwd, config.paths.registry)], + `chore(rk): record review verdict for ${sprint.id}`, + ); + } + // `review-sprint` exits 0 even when it records changes_requested/rejected. + // Halt as a REVIEW failure (not a downstream CLOSE failure) so run state + // names the real cause. + if (evalReview && evalReview.verdict !== 'accepted') { + run = await updateRun( + run.id, + { + status: 'failed', + halt_reason: `${HALT_REASONS.REVIEW_FAILED}:${sprint.id}`, + ended_at: isoNow(), + }, + opRoot, + ); + await releaseLane(`epic-${run.epic_id}`, opRoot, run.id); + return err( + 'REVIEW_NOT_ACCEPTED', + `autonomous mode: built-in review verdict for ${sprint.id} was ${evalReview.verdict}`, + `address the findings and re-run, or review ${sprint.id} manually`, + ); + } + + // runReviewCommand + the verdict commit above leave a clean tree for + // runCloseCommand (which requires one) below. const closeResult = await runCloseCommand(sprint.id, { cwd: executionCwd, dryRun: false, diff --git a/packages/cli/src/commands/status.ts b/packages/cli/src/commands/status.ts index df027ea..3ed1af2 100644 --- a/packages/cli/src/commands/status.ts +++ b/packages/cli/src/commands/status.ts @@ -7,6 +7,7 @@ import { loadProject, meetsThreshold, RepoKernelError, + type ReviewerGateVerdict, type ReviewVerdict, resolveNextRunnableSprint, runValidators, @@ -58,6 +59,8 @@ interface CurrentReview { readonly reviewer: string; readonly verdict: ReviewVerdict; readonly review_attempt: number | null; + readonly gate_verdict: ReviewerGateVerdict | null; + readonly gate_attempt: number | null; } interface StatusReport { @@ -418,6 +421,11 @@ function formatStatus( lines.push( ` ${cr.review_id} (${cr.reviewer}) ${cr.verdict}${cr.review_attempt !== null ? ` — attempt ${cr.review_attempt}` : ''}`, ); + if (cr.gate_verdict !== null) { + lines.push( + ` gate: ${cr.gate_verdict}${cr.gate_attempt !== null ? ` (attempt ${cr.gate_attempt})` : ''}`, + ); + } } if (report.registryPath) { lines.push(''); @@ -499,6 +507,8 @@ function currentReviewsOf(graph: Graph): CurrentReview[] { reviewer: review.reviewer, verdict: review.verdict, review_attempt: review.review_attempt ?? null, + gate_verdict: review.reviewer_gate?.verdict ?? null, + gate_attempt: review.reviewer_gate?.review_attempt ?? null, }); } return out.sort((a, b) => a.sprint_id.localeCompare(b.sprint_id)); diff --git a/packages/cli/src/lifecycle/gateEnforce.ts b/packages/cli/src/lifecycle/gateEnforce.ts new file mode 100644 index 0000000..bed39df --- /dev/null +++ b/packages/cli/src/lifecycle/gateEnforce.ts @@ -0,0 +1,226 @@ +import { + type Config, + gateRequired, + loadGateSecret, + materialPathGlobs, + type Review, + reviewerGateConfigFor, + type Sprint, + verifyGateSignature, +} from '@repokernel/core'; +import { inScopeFiles } from './diffClassifier.js'; +import { changedFilesSince, fileAtCommit } from './git.js'; +import { parseSprintScope } from './reviewerGate.js'; + +export interface GateBlock { + readonly code: + | 'REVIEWER_GATE_MISSING' + | 'REVIEWER_GATE_NOT_ACCEPTED' + | 'REVIEWER_GATE_ATTEMPT_MISMATCH' + | 'REVIEWER_GATE_STALE' + | 'REVIEWER_GATE_SIGNATURE_INVALID'; + readonly message: string; + readonly hint: string; +} + +export type GateEvaluation = + | { readonly ok: true } + | { readonly ok: false; readonly block: GateBlock }; + +const OK: GateEvaluation = { ok: true }; + +/** + * Evaluate the signed reviewer_gate snapshot as a close/ship precondition. + * Returns `ok` when no gate is required for the sprint, or when a present + * snapshot is authentic (signature), produced by the review's stamped reviewer, + * bound to the current attempt, accepted, and fresh against the committed tree + * AND the policy/scope inputs that defined it. Otherwise returns the blocking + * reason. + * + * Always-on: not bypassable with `--skip-checks`. The signature is verified + * against the SPRINT BEING CLOSED (`sprint.id`) — never the review's own + * `sprint_id` field — so a sprint pointing at another sprint's signed review + * fails closed. Requirement is anchored on config + `review_required` + the + * linked review, so a gated reviewer cannot be dodged by renaming. + */ +export async function evaluateReviewerGate(opts: { + readonly checkPath: string; + readonly config: Config; + readonly sprint: Sprint; + readonly review: Review; + /** Repo-relative path to the project config — a post-gate edit to it is stale-making. */ + readonly configFile: string; + readonly env?: NodeJS.ProcessEnv; +}): Promise { + const { checkPath, config, sprint, review, configFile } = opts; + if (!gateRequired(sprint, config, review)) return OK; + + const reviewGateHint = `rk review-gate ${sprint.id}`; + + // A review that targets another sprint must never satisfy this sprint's gate. + if (review.sprint_id !== sprint.id) { + return { + ok: false, + block: { + code: 'REVIEWER_GATE_MISSING', + message: `${sprint.id} links review ${review.id}, which targets sprint ${review.sprint_id}`, + hint: `link a review for ${sprint.id} and run ${reviewGateHint}`, + }, + }; + } + + if (reviewerGateConfigFor(config.automation, review.reviewer) === undefined) { + return { + ok: false, + block: { + code: 'REVIEWER_GATE_MISSING', + message: `review ${review.id} reviewer "${review.reviewer}" has no configured reviewer gate, but ${sprint.id} requires one`, + hint: `stamp a configured reviewer (automation.reviewers) and run ${reviewGateHint}`, + }, + }; + } + + const snapshot = review.reviewer_gate; + if (!snapshot) { + return { + ok: false, + block: { + code: 'REVIEWER_GATE_MISSING', + message: `${sprint.id} requires a reviewer gate but ${review.id} has no reviewer_gate snapshot`, + hint: `run the reviewer gate: ${reviewGateHint}`, + }, + }; + } + + // The local signing key is shared across reviewers, so the signature alone + // does not prove WHICH reviewer produced the snapshot. Bind it to the review's + // stamped reviewer explicitly. + if (snapshot.reviewer !== review.reviewer) { + return { + ok: false, + block: { + code: 'REVIEWER_GATE_SIGNATURE_INVALID', + message: `${review.id} is stamped reviewer "${review.reviewer}" but its reviewer_gate was produced by "${snapshot.reviewer}"`, + hint: `run the gate as the stamped reviewer: ${reviewGateHint}`, + }, + }; + } + + const secret = await loadGateSecret(opts.env); + if ( + secret === null || + !verifyGateSignature(secret, snapshot, { review_id: review.id, sprint_id: sprint.id }) + ) { + return { + ok: false, + block: { + code: 'REVIEWER_GATE_SIGNATURE_INVALID', + message: + secret === null + ? `${review.id} has a reviewer_gate snapshot but no local gate key is available to verify it` + : `${review.id} reviewer_gate signature does not verify — the snapshot was forged, lifted from another sprint, or signed on another machine`, + hint: `re-run the gate on the machine that holds the gate key: ${reviewGateHint}`, + }, + }; + } + + if (snapshot.review_attempt !== review.review_attempt) { + return { + ok: false, + block: { + code: 'REVIEWER_GATE_ATTEMPT_MISMATCH', + message: `${review.id} reviewer_gate is for attempt ${snapshot.review_attempt} but the current review attempt is ${review.review_attempt}`, + hint: `re-run the gate for the current attempt: ${reviewGateHint}`, + }, + }; + } + + if (snapshot.verdict !== 'accepted') { + return { + ok: false, + block: { + code: 'REVIEWER_GATE_NOT_ACCEPTED', + message: `${review.id} reviewer_gate verdict is ${snapshot.verdict}`, + hint: `address the gate findings, commit, then re-run: ${reviewGateHint}`, + }, + }; + } + + // The snapshot's base_sha is the signed start of the reviewed range and the + // commit its scope was read at. If the sprint's base_sha has since been + // retargeted (rebase or metadata tamper), the shipped audit range no longer + // matches what was signed — block here, not just post-ship at validate. + if (sprint.base_sha && snapshot.base_sha !== sprint.base_sha) { + return { + ok: false, + block: { + code: 'REVIEWER_GATE_STALE', + message: `${review.id} gated base ${snapshot.base_sha.slice(0, 7)} but ${sprint.id} base_sha is now ${sprint.base_sha.slice(0, 7)}`, + hint: `re-run the reviewer gate against the current base: ${reviewGateHint}`, + }, + }; + } + + const sinceReview = await changedFilesSince(checkPath, snapshot.end_sha); + + // Policy/scope freshness. The gate's verdict is only valid for the scope + // (allowed/denied/generated paths) and the project policy it reviewed. A + // post-gate edit to the project config, or to the sprint's scope fields, + // re-defines what "in scope" or "requires a gate" means — so the snapshot no + // longer vouches for the current tree. The sprint file's status/metadata + // churn (active→review→shipped) is NOT scope, so compare the scope fields + // directly rather than the raw file change. + if (sinceReview.includes(configFile)) { + return { + ok: false, + block: { + code: 'REVIEWER_GATE_STALE', + message: `${review.id} gated ${snapshot.end_sha.slice(0, 7)}; project config ${configFile} changed since`, + hint: `re-run the reviewer gate against the current policy: ${reviewGateHint}`, + }, + }; + } + const scopeAtGate = await fileAtCommit(checkPath, snapshot.base_sha, sprint.file).then((c) => + c === null ? null : parseSprintScope(c), + ); + const norm = (a: readonly string[] | undefined): string => JSON.stringify([...(a ?? [])].sort()); + const scopeDrifted = + scopeAtGate === null || + norm(scopeAtGate.allowed_paths) !== norm(sprint.allowed_paths) || + norm(scopeAtGate.denied_paths) !== norm(sprint.denied_paths) || + norm(scopeAtGate.generated_paths) !== norm(sprint.generated_paths); + if (scopeDrifted) { + return { + ok: false, + block: { + code: 'REVIEWER_GATE_STALE', + message: `${review.id} gated against a different scope than the current ${sprint.file}`, + hint: `re-run the reviewer gate against the current scope: ${reviewGateHint}`, + }, + }; + } + + const changedInScope = inScopeFiles(sinceReview, { + config, + sprint, + rkOwnedGlobs: materialPathGlobs(config), + exemptPaths: [ + sprint.file, + config.paths.registry, + `${config.paths.queues}/${sprint.lane}.md`, + review.file, + ], + }); + if (changedInScope.length > 0) { + return { + ok: false, + block: { + code: 'REVIEWER_GATE_STALE', + message: `${review.id} gated ${snapshot.end_sha.slice(0, 7)}; in-scope files changed since: ${changedInScope.join(', ')}`, + hint: `re-run the reviewer gate: ${reviewGateHint}`, + }, + }; + } + + return OK; +} diff --git a/packages/cli/src/lifecycle/gateSecret.ts b/packages/cli/src/lifecycle/gateSecret.ts new file mode 100644 index 0000000..63e2671 --- /dev/null +++ b/packages/cli/src/lifecycle/gateSecret.ts @@ -0,0 +1,37 @@ +import { randomBytes } from 'node:crypto'; +import { mkdir, writeFile } from 'node:fs/promises'; +import { dirname } from 'node:path'; +import { gateSecretPath, loadGateSecret, RepoKernelError } from '@repokernel/core'; + +/** + * Read the machine-local gate signing secret, minting one on first use. The + * secret lives outside the repo (`~/.repokernel/gate.key`, mode 0600) so a + * repo-bound agent cannot read it to forge a snapshot — the same trust boundary + * as the reviewer command pin. Throws if the secret directory is unwritable + * (the gate fails closed rather than signing with no durable key). + */ +export async function loadOrCreateGateSecret( + env: NodeJS.ProcessEnv = process.env, +): Promise { + const existing = await loadGateSecret(env); + if (existing) return existing; + + const path = gateSecretPath(env); + await mkdir(dirname(path), { recursive: true, mode: 0o700 }); + const secret = randomBytes(32).toString('hex'); + try { + await writeFile(path, `${secret}\n`, { mode: 0o600, flag: 'wx' }); + return secret; + } catch (cause) { + // Lost a create race, or a file already exists. Trust ONLY a strict-valid + // key on disk — never sign with malformed contents (a snapshot signed by a + // bad key could never verify, silently dead-ending close). + const raced = await loadGateSecret(env); + if (raced) return raced; + throw new RepoKernelError( + 'GATE_KEY_INVALID', + `gate signing key at ${path} exists but is not a valid 32-byte hex secret (or is not a regular file); remove it so a fresh key can be minted`, + cause, + ); + } +} diff --git a/packages/cli/src/lifecycle/parallelRunner.ts b/packages/cli/src/lifecycle/parallelRunner.ts index 0863145..fee1018 100644 --- a/packages/cli/src/lifecycle/parallelRunner.ts +++ b/packages/cli/src/lifecycle/parallelRunner.ts @@ -1,8 +1,15 @@ -import { join } from 'node:path'; +import { join, relative } from 'node:path'; import type { Epic, Run, RunId, Sprint, SprintId } from '@repokernel/core'; -import { loadProject, meetsThreshold, runValidators } from '@repokernel/core'; +import { + effectiveReviewRequired, + gateRequired, + loadProject, + meetsThreshold, + runValidators, +} from '@repokernel/core'; import type { AgentRunner, SprintRunResult } from '../agents/types.js'; import { effectiveConcurrencyCap } from './dispatch.js'; +import { evaluateReviewerGate } from './gateEnforce.js'; import { changedFilesForSprint, getCurrentSha, isWorkingTreeClean } from './git.js'; import { git } from './gitExec.js'; import { mutateReviewFrontmatter, mutateSprintFrontmatter, removeSlotFromQueue } from './mutate.js'; @@ -419,6 +426,47 @@ export async function closeAfterMerge( throw new Error(`sprint ${sprintId} not found in epic worktree`); } + // Reviewer-gate enforcement — the parallel close path must honor the same gate + // as runCloseCommand, not bypass it. The merged review carries the signed + // snapshot; verify presence, signature, attempt, verdict, and freshness + // before shipping. Fail closed: a gate-required sprint without a valid + // snapshot is not shipped by the wave (re-run the gate, then close manually). + const gateReview = reviewId ? outcome.graph.reviews.get(reviewId) : undefined; + // Built-in review lane: mirror runCloseCommand — a review-required sprint must + // carry an accepted review.verdict before it ships. The parallel path does not + // run the review pipeline, so this (with the run-start preflight) fails closed + // rather than shipping with a pending verdict. + if (effectiveReviewRequired(sprint, outcome.config)) { + if (!gateReview) { + throw new Error( + `${sprintId} requires review but no review is linked; close via sequential rk run`, + ); + } + if (gateReview.verdict !== 'accepted') { + throw new Error( + `${sprintId} review ${gateReview.id} verdict is ${gateReview.verdict}, not accepted`, + ); + } + } + if (gateReview) { + const gateEval = await evaluateReviewerGate({ + checkPath: epicWorktree, + config: outcome.config, + sprint, + review: gateReview, + configFile: relative(epicWorktree, outcome.configPath), + }); + if (!gateEval.ok) { + throw new Error( + `reviewer gate blocked close of ${sprintId} (${gateEval.block.code}): ${gateEval.block.message}`, + ); + } + } else if (gateRequired(sprint, outcome.config)) { + throw new Error( + `reviewer gate required for ${sprintId} but no review is linked; run rk review-gate ${sprintId}`, + ); + } + const endSha = await getCurrentSha(epicWorktree); const closedAt = new Date().toISOString(); const touched: string[] = []; diff --git a/packages/cli/src/lifecycle/reviewerGate.ts b/packages/cli/src/lifecycle/reviewerGate.ts index 8f89c20..f78bcdb 100644 --- a/packages/cli/src/lifecycle/reviewerGate.ts +++ b/packages/cli/src/lifecycle/reviewerGate.ts @@ -12,6 +12,7 @@ import { type ReviewFinding, SPRINT_ID_RE, type Sprint, + signGatePayload, toErrorMessage, } from '@repokernel/core'; import matter from 'gray-matter'; @@ -25,6 +26,7 @@ import { } from '../security/spawnPolicy.js'; import { isoNow } from '../templates/time.js'; import { classifySprintDiff } from './diffClassifier.js'; +import { loadOrCreateGateSecret } from './gateSecret.js'; import { changedFilesForSprint, diffPatchSince, @@ -615,17 +617,48 @@ export async function runReviewerGate(input: ReviewerGateInput): Promise s.split(gateSecret).join('[REDACTED]'); + const safeFindings: readonly ReviewFinding[] = allFindings.map((f) => ({ + ...f, + message: redact(f.message), + })); + const summary = spawnResult.ok && spawnResult.summary ? redact(spawnResult.summary) : undefined; + + const reviewedAt = isoNow(); + const snapshot = { + reviewer: input.reviewerName, + review_attempt: review.review_attempt ?? 1, + verdict, + findings: safeFindings, + base_sha: baseSha, + end_sha: endSha, + reviewed_at: reviewedAt, + ...(summary ? { summary } : {}), + }; + const signature = signGatePayload(gateSecret, { + ...snapshot, + review_id: review.id, + sprint_id: sprint.id, + }); await withLifecycleScope( { cwd, command: 'reviewer-gate', args: { sprintId: sprint.id } }, async (tx) => { - // review_attempt is owned by `rk re-review`; the gate does not touch it. + // The gate decision AND its reviewed commit range (base_sha/end_sha) live + // ONLY in the signed reviewer_gate snapshot — never in review.verdict / + // findings / base_sha / end_sha, which the built-in eval, panel, and + // manual override own and could otherwise overwrite. review.end_sha stays + // unset so close stamps it with the shipped commit (keeping it consistent + // with sprint.end_sha); the gate's reviewed sha is the snapshot's. + // changed_files/paths_checked remain review-level inputs to the built-in + // eval. review_attempt is owned by `rk re-review`; the gate binds to it. await mutateReviewFrontmatter(join(cwd, review.file), { - verdict, - findings: allFindings, - base_sha: baseSha, - end_sha: endSha, + reviewer_gate: { ...snapshot, signature }, changed_files: changed.files, paths_checked: { allowed_paths_matched: @@ -633,8 +666,7 @@ export async function runReviewerGate(input: ReviewerGateInput): Promise b.category === 'out_of_scope_committed'), denied_paths_clean: !classification.blockers.some((b) => b.category === 'denied_path'), }, - updated_at: isoNow(), - reviewed_at: isoNow(), + updated_at: reviewedAt, }); await tx.refreshRegistry(); }, @@ -644,8 +676,8 @@ export async function runReviewerGate(input: ReviewerGateInput): Promise --full', }, + REVIEWER_GATE_MISSING: { + severity: 'P0', + why: 'The project configures a reviewer gate and this sprint requires review, but the review carries no reviewer_gate snapshot — the gate never ran, or its reviewer has no configured gate.', + expected: + 'A signed reviewer_gate snapshot from a configured reviewer is recorded on the review.', + fix: 'Run the reviewer gate for this sprint to record a snapshot.', + command: 'rk review-gate ', + }, + REVIEWER_GATE_NOT_ACCEPTED: { + severity: 'P0', + why: 'The recorded reviewer_gate verdict is not accepted, so the gate has not cleared the change for shipping.', + expected: 'The reviewer_gate snapshot verdict is accepted.', + fix: 'Address the gate findings, commit, and re-run the reviewer gate.', + command: 'rk review-gate ', + }, + REVIEWER_GATE_STALE: { + severity: 'P1', + why: 'In-scope files changed since the commit the reviewer gate signed off, so the accepted snapshot no longer vouches for the current tree.', + expected: 'No in-scope file has changed since the reviewer_gate end_sha.', + fix: 'Re-run the reviewer gate against the current commit before closing.', + command: 'rk review-gate ', + }, + REVIEWER_GATE_ATTEMPT_MISMATCH: { + severity: 'P1', + why: 'The reviewer_gate snapshot was recorded for an earlier review attempt; a re-review has since bumped the attempt, invalidating the prior gate.', + expected: 'The reviewer_gate review_attempt matches the current review review_attempt.', + fix: 'Re-run the reviewer gate for the current attempt.', + command: 'rk review-gate ', + }, + REVIEWER_GATE_SIGNATURE_INVALID: { + severity: 'P0', + why: 'The reviewer_gate signature does not verify against the machine-local gate key, so the snapshot was forged or the key is unavailable on this machine.', + expected: 'The reviewer_gate signature verifies against the local gate key.', + fix: 'Re-run the reviewer gate on the machine that holds the gate key to record an authentic snapshot.', + command: 'rk review-gate ', + }, } satisfies Record>; export function explainCode(code: string): FindingExplanation | null { diff --git a/packages/cli/test/review.test.ts b/packages/cli/test/review.test.ts index 6cb735c..0743081 100644 --- a/packages/cli/test/review.test.ts +++ b/packages/cli/test/review.test.ts @@ -8,6 +8,7 @@ import { afterAll, afterEach, beforeEach, describe, expect, it } from 'vitest'; import { runCloseCommand, runReviewCommand } from '../src/commands/lifecycle.js'; import { runReviewCreateCommand } from '../src/commands/reviewCreate.js'; import { runReviewGateCommand } from '../src/commands/reviewGate.js'; +import { runReviewSprintCommand } from '../src/commands/reviewSprint.js'; import { cleanupAllFixtures, defaultConfigYaml, @@ -111,6 +112,10 @@ async function reviewData(cwd: string): Promise> { return matter(await readFile(join(cwd, 'reviews/R-001.md'), 'utf8')).data; } +async function gateOf(cwd: string): Promise | undefined> { + return (await reviewData(cwd)).reviewer_gate as Record | undefined; +} + let originalTrust: string | undefined; let originalCodexHome: string | undefined; beforeEach(() => { @@ -131,8 +136,12 @@ describe('rk review (with reviewer gate)', () => { expect(r.stdout).toContain('moved to review'); expect(r.stdout).toContain('accepted'); const data = await reviewData(b.cwd); - expect(data.verdict).toBe('accepted'); - expect(typeof data.end_sha).toBe('string'); + // The gate decision + reviewed range are recorded in the signed snapshot, + // not in review.verdict/end_sha. + expect(data.verdict).toBe('pending'); + const gate = data.reviewer_gate as Record; + expect(gate.verdict).toBe('accepted'); + expect(typeof gate.end_sha).toBe('string'); }); it('behaves exactly as before when no reviewer gate is configured', async () => { @@ -152,7 +161,7 @@ describe('rk review (with reviewer gate)', () => { const r = await runReviewCommand('S-001', { cwd: b.cwd, dryRun: false, json: false }); expect(r.stdout).toContain('moved to review'); expect(r.stdout).toContain('accepted'); // codex gate ran despite default=manual - expect((await reviewData(b.cwd)).verdict).toBe('accepted'); + expect((await gateOf(b.cwd))?.verdict).toBe('accepted'); }); }); @@ -178,7 +187,7 @@ describe('rk review-create', () => { }); expect(r.stdout).toContain('Created R-001'); expect(r.stdout).toContain('changes_requested'); - expect((await reviewData(b.cwd)).verdict).toBe('changes_requested'); + expect((await gateOf(b.cwd))?.verdict).toBe('changes_requested'); }); }); @@ -187,15 +196,18 @@ describe('rk close binding (gate end_sha)', () => { const b = await build({ command: ACCEPT }); process.env.CODEX_HOME = b.codexHome; await runReviewCommand('S-001', { cwd: b.cwd, dryRun: false, json: false }); - expect((await reviewData(b.cwd)).verdict).toBe('accepted'); - // Same filename, new content — a file-SET guard misses this; the end_sha - // content binding must catch it. + expect((await gateOf(b.cwd))?.verdict).toBe('accepted'); + // Built-in lane must also be green for close to reach the gate freshness + // check (most-restrictive-wins composition). + await runReviewSprintCommand('S-001', { cwd: b.cwd, dryRun: false, json: false }); + // Same filename, new content — a file-SET guard misses this; the gate + // end_sha content binding must catch it. await writeFile(join(b.cwd, 'src/foo.ts'), 'export const v = 99;\n', 'utf8'); git(b.cwd, ['add', '.']); commit(b.cwd, 'sneaky same-file edit'); const r = await runCloseCommand('S-001', { cwd: b.cwd, dryRun: false, json: false }); expect(r.exitCode).not.toBe(0); - expect(`${r.stderr}${r.stdout}`).toMatch(/changed since|review-gate|REVIEW_STALE/i); + expect(`${r.stderr}${r.stdout}`).toMatch(/changed since|review-gate|REVIEWER_GATE_STALE/i); }); }); @@ -207,7 +219,7 @@ describe('rk review-gate', () => { expect((await reviewData(b.cwd)).verdict).toBe('pending'); const r = await runReviewGateCommand('S-001', { cwd: b.cwd, json: false }); expect(r.stdout).toContain('accepted'); - expect((await reviewData(b.cwd)).verdict).toBe('accepted'); + expect((await gateOf(b.cwd))?.verdict).toBe('accepted'); }); it('blocks when the review reviewer has no configured gate', async () => { diff --git a/packages/cli/test/reviewerGate.test.ts b/packages/cli/test/reviewerGate.test.ts index d4a4b66..0148daa 100644 --- a/packages/cli/test/reviewerGate.test.ts +++ b/packages/cli/test/reviewerGate.test.ts @@ -329,9 +329,18 @@ describe('runReviewerGate', () => { const out = await runReviewerGate(await gateInput(b)); expect(out.kind === 'recorded' && out.verdict).toBe('accepted'); const data = await readReview(b.cwd); - expect(data.verdict).toBe('accepted'); - expect(data.base_sha).toBe(b.baseSha); - expect(typeof data.end_sha).toBe('string'); + // The gate verdict lives in the signed snapshot, NOT in review.verdict. + expect(data.verdict).toBe('pending'); + const gate = data.reviewer_gate as Record; + expect(gate.verdict).toBe('accepted'); + expect(gate.reviewer).toBe('codex'); + expect(gate.review_attempt).toBe(1); + expect(gate.base_sha).toBe(b.baseSha); + expect(typeof gate.end_sha).toBe('string'); + expect(gate.signature).toMatch(/^[a-f0-9]{64}$/); + // The reviewed range lives in the snapshot, not at the review level. + expect(data.base_sha).toBeUndefined(); + expect(data.end_sha).toBeUndefined(); }); it('fails soft to changes_requested on invalid sentinel output', async () => { @@ -441,7 +450,9 @@ describe('runReviewerGate', () => { process.env.CODEX_HOME = b.codexHome; const out = await runReviewerGate(await gateInput(b)); expect(out.kind === 'recorded' && out.verdict).toBe('changes_requested'); - expect((await readReview(b.cwd)).verdict).toBe('changes_requested'); + expect(((await readReview(b.cwd)).reviewer_gate as Record).verdict).toBe( + 'changes_requested', + ); }); it('fails closed when the sprint scope cannot be resolved at base_sha', async () => { @@ -469,6 +480,8 @@ describe('runReviewerGate', () => { process.env.CODEX_HOME = b.codexHome; const out = await runReviewerGate(await gateInput(b)); expect(out.kind === 'recorded' && out.verdict).toBe('changes_requested'); - expect((await readReview(b.cwd)).verdict).toBe('changes_requested'); + expect(((await readReview(b.cwd)).reviewer_gate as Record).verdict).toBe( + 'changes_requested', + ); }); }); diff --git a/packages/cli/test/reviewerGateEnforce.test.ts b/packages/cli/test/reviewerGateEnforce.test.ts new file mode 100644 index 0000000..51793c3 --- /dev/null +++ b/packages/cli/test/reviewerGateEnforce.test.ts @@ -0,0 +1,387 @@ +import { execFileSync } from 'node:child_process'; +import { mkdtemp, readFile, realpath, writeFile } from 'node:fs/promises'; +import { tmpdir } from 'node:os'; +import { dirname, join } from 'node:path'; +import { fileURLToPath } from 'node:url'; +import matter from 'gray-matter'; +import { afterAll, afterEach, beforeEach, describe, expect, it } from 'vitest'; +import { + runCloseCommand, + runReReviewCommand, + runReviewCommand, + runReviewVerdictCommand, +} from '../src/commands/lifecycle.js'; +import { runReviewCreateCommand } from '../src/commands/reviewCreate.js'; +import { runReviewSprintCommand } from '../src/commands/reviewSprint.js'; +import { closeAfterMerge } from '../src/lifecycle/parallelRunner.js'; +import { + cleanupAllFixtures, + defaultConfigYaml, + fm, + makeFixture, + resetTrustForTest, + seedTrustForCwd, +} from './helpers/fixture.js'; + +const FIXTURES = join(dirname(fileURLToPath(import.meta.url)), 'fixtures', 'codex'); +const ACCEPT = join(FIXTURES, 'accept.sh'); +const CHANGES = join(FIXTURES, 'changes.sh'); + +afterAll(cleanupAllFixtures); + +function gatedConfig(defaultReviewer = 'codex', secondReviewer = false): string { + return `${defaultConfigYaml()}automation: + defaultReviewer: ${defaultReviewer} + reviewers: + codex: + authMode: chatgpt +${secondReviewer ? ' codex2:\n authMode: chatgpt\n' : ''}`; +} + +function git(cwd: string, args: string[]): void { + execFileSync('git', ['-C', cwd, ...args], { stdio: 'pipe' }); +} +function commit(cwd: string, message: string): void { + git(cwd, ['-c', 'user.email=t@t', '-c', 'user.name=t', 'commit', '-q', '-m', message]); +} +function commitAll(cwd: string, message: string): void { + git(cwd, ['add', '-A']); + commit(cwd, message); +} +function sprintFm(extra: Record): string { + return fm({ + id: 'S-001', + title: 'Sprint One', + epic_id: 'E-001', + status: 'active', + lane: 'main', + allowed_paths: ['src/**'], + review_required: true, + ...extra, + }); +} + +async function build(opts: { + readonly command: string; + readonly gated?: boolean; + readonly defaultReviewer?: string; + readonly secondReviewer?: boolean; +}): Promise<{ readonly cwd: string; readonly codexHome: string }> { + const cwd = await realpath( + await makeFixture([ + { + path: 'repokernel.config.yaml', + content: + opts.gated === false + ? defaultConfigYaml() + : gatedConfig(opts.defaultReviewer, opts.secondReviewer), + }, + { + path: 'epics/E-001.md', + content: fm({ id: 'E-001', title: 'E', status: 'active', sprints: ['S-001'] }), + }, + { path: 'sprints/S-001.md', content: sprintFm({}) }, + { path: 'queues/main.md', content: fm({ lane: 'main', slots: [] }) }, + { path: 'src/foo.ts', content: 'export const v = 0;\n' }, + ]), + ); + + git(cwd, ['init', '-q']); + git(cwd, ['config', 'user.email', 'test@repokernel.test']); + git(cwd, ['config', 'user.name', 'rk-test']); + git(cwd, ['add', '.']); + commit(cwd, 'base'); + const baseSha = execFileSync('git', ['-C', cwd, 'rev-parse', 'HEAD']).toString().trim(); + + await writeFile(join(cwd, 'sprints/S-001.md'), sprintFm({ base_sha: baseSha }), 'utf8'); + await writeFile(join(cwd, 'src/foo.ts'), 'export const v = 1;\n', 'utf8'); + git(cwd, ['add', '.']); + commit(cwd, 'work'); + + await seedTrustForCwd(cwd, { + reviewers: { + codex: { + command: opts.command, + args: [], + env_passthrough: ['CODEX_HOME'], + timeout_seconds: 10, + }, + }, + }); + + const codexHome = await mkdtemp(join(tmpdir(), 'rk-codexhome-')); + await writeFile( + join(codexHome, 'auth.json'), + JSON.stringify({ auth_mode: 'chatgpt', tokens: { access_token: 'x' } }), + 'utf8', + ); + return { cwd, codexHome }; +} + +async function reviewData(cwd: string): Promise> { + return matter(await readFile(join(cwd, 'reviews/R-001.md'), 'utf8')).data; +} + +/** Drive the full gated path to an about-to-close state: gate + built-in green, committed. */ +async function reviewAndSprint(cwd: string): Promise { + await runReviewCommand('S-001', { cwd, dryRun: false, json: false }); + await runReviewSprintCommand('S-001', { cwd, dryRun: false, json: false }); + commitAll(cwd, 'record review-sprint verdict'); +} + +let originalTrust: string | undefined; +let originalCodexHome: string | undefined; +beforeEach(() => { + originalTrust = process.env.REPOKERNEL_TRUST_FILE; + originalCodexHome = process.env.CODEX_HOME; +}); +afterEach(() => { + resetTrustForTest(originalTrust); + if (originalCodexHome === undefined) delete process.env.CODEX_HOME; + else process.env.CODEX_HOME = originalCodexHome; +}); + +describe('reviewer-gate enforcement at close', () => { + it('allows close when the gate snapshot AND the built-in verdict are both accepted', async () => { + const b = await build({ command: ACCEPT }); + process.env.CODEX_HOME = b.codexHome; + await reviewAndSprint(b.cwd); + const r = await runCloseCommand('S-001', { cwd: b.cwd, dryRun: false, json: false }); + expect(r.exitCode).toBe(0); + }); + + it('blocks close when review-sprint accepts but the gate snapshot is changes_requested', async () => { + const b = await build({ command: CHANGES }); + process.env.CODEX_HOME = b.codexHome; + // gate → changes_requested snapshot; review-sprint built-in → accepted verdict. + await reviewAndSprint(b.cwd); + expect((await reviewData(b.cwd)).verdict).toBe('accepted'); + expect((await reviewData(b.cwd)).reviewer_gate as Record).toMatchObject({ + verdict: 'changes_requested', + }); + const r = await runCloseCommand('S-001', { cwd: b.cwd, dryRun: false, json: false }); + expect(r.exitCode).not.toBe(0); + expect(`${r.stderr}${r.stdout}`).toMatch(/REVIEWER_GATE_NOT_ACCEPTED|gate verdict/i); + }); + + it('blocks close when the snapshot signature is tampered (forgery)', async () => { + const b = await build({ command: ACCEPT }); + process.env.CODEX_HOME = b.codexHome; + await reviewAndSprint(b.cwd); + // Forge: flip the signature in the committed review file. + const file = join(b.cwd, 'reviews/R-001.md'); + const parsed = matter(await readFile(file, 'utf8')); + const gate = parsed.data.reviewer_gate as Record; + gate.signature = 'f'.repeat(64); + await writeFile(file, matter.stringify(parsed.content, parsed.data), 'utf8'); + commitAll(b.cwd, 'tamper signature'); + const r = await runCloseCommand('S-001', { cwd: b.cwd, dryRun: false, json: false }); + expect(r.exitCode).not.toBe(0); + expect(`${r.stderr}${r.stdout}`).toMatch(/REVIEWER_GATE_SIGNATURE_INVALID|forged/i); + }); + + it('blocks close when re-review bumps the attempt past the gated snapshot', async () => { + const b = await build({ command: ACCEPT }); + process.env.CODEX_HOME = b.codexHome; + // Gate accepted at attempt 1 (snapshot signed for attempt 1). + await runReviewCommand('S-001', { cwd: b.cwd, dryRun: false, json: false }); + expect((await reviewData(b.cwd)).reviewer_gate as Record).toMatchObject({ + review_attempt: 1, + }); + // Force a non-accepted verdict so re-review will reopen, bumping to attempt 2. + await runReviewVerdictCommand('R-001', 'changes_requested', { + cwd: b.cwd, + dryRun: false, + json: false, + }); + await runReReviewCommand('S-001', { cwd: b.cwd, dryRun: false, json: false }); + await runReviewSprintCommand('S-001', { cwd: b.cwd, dryRun: false, json: false }); + commitAll(b.cwd, 'reopen + re-eval'); + // Built-in verdict is accepted again, but the gate snapshot is still attempt 1. + const r = await runCloseCommand('S-001', { cwd: b.cwd, dryRun: false, json: false }); + expect(r.exitCode).not.toBe(0); + expect(`${r.stderr}${r.stdout}`).toMatch(/REVIEWER_GATE_ATTEMPT_MISMATCH|attempt/i); + }); + + it('blocks close when the gate snapshot is cleared and only review.verdict is set', async () => { + const b = await build({ command: ACCEPT }); + process.env.CODEX_HOME = b.codexHome; + await reviewAndSprint(b.cwd); + // Strip the snapshot, leaving review.verdict accepted — the classic + // "trust review.verdict alone" bypass the snapshot model closes. + const file = join(b.cwd, 'reviews/R-001.md'); + const parsed = matter(await readFile(file, 'utf8')); + delete parsed.data.reviewer_gate; + await writeFile(file, matter.stringify(parsed.content, parsed.data), 'utf8'); + commitAll(b.cwd, 'strip snapshot, keep accepted verdict'); + const r = await runCloseCommand('S-001', { cwd: b.cwd, dryRun: false, json: false }); + expect(r.exitCode).not.toBe(0); + expect(`${r.stderr}${r.stdout}`).toMatch(/REVIEWER_GATE_MISSING|reviewer gate/i); + }); + + it('blocks close when the review reviewer has no configured gate (reviewer dodge)', async () => { + const b = await build({ command: ACCEPT }); + process.env.CODEX_HOME = b.codexHome; + // Stamp an ungated reviewer; project still configures the codex gate. + await runReviewCreateCommand({ cwd: b.cwd, sprintId: 'S-001', json: false, reviewer: 'ghost' }); + await runReviewCommand('S-001', { cwd: b.cwd, dryRun: false, json: false }); + await runReviewVerdictCommand('R-001', 'accepted', { cwd: b.cwd, dryRun: false, json: false }); + commitAll(b.cwd, 'ungated reviewer'); + const r = await runCloseCommand('S-001', { cwd: b.cwd, dryRun: false, json: false }); + expect(r.exitCode).not.toBe(0); + expect(`${r.stderr}${r.stdout}`).toMatch(/REVIEWER_GATE_MISSING|ghost|no configured/i); + }); + + it('does not let --skip-checks bypass a stale gate snapshot', async () => { + const b = await build({ command: ACCEPT }); + process.env.CODEX_HOME = b.codexHome; + await reviewAndSprint(b.cwd); + await writeFile(join(b.cwd, 'src/foo.ts'), 'export const v = 42;\n', 'utf8'); + commitAll(b.cwd, 'edit after gate'); + const r = await runCloseCommand('S-001', { + cwd: b.cwd, + dryRun: false, + json: false, + skipChecks: true, + }); + expect(r.exitCode).not.toBe(0); + expect(`${r.stderr}${r.stdout}`).toMatch(/REVIEWER_GATE_STALE|changed since/i); + }); + + it('migration: a non-gated project with a legacy accepted review still closes', async () => { + const b = await build({ command: ACCEPT, gated: false }); + process.env.CODEX_HOME = b.codexHome; + // No reviewer gate configured ⇒ legacy verdict-only path, no snapshot required. + await runReviewCommand('S-001', { cwd: b.cwd, dryRun: false, json: false }); + await runReviewSprintCommand('S-001', { cwd: b.cwd, dryRun: false, json: false }); + commitAll(b.cwd, 'legacy verdict'); + expect((await reviewData(b.cwd)).reviewer_gate).toBeUndefined(); + const r = await runCloseCommand('S-001', { cwd: b.cwd, dryRun: false, json: false }); + expect(r.exitCode).toBe(0); + }); + + it('blocks close of a sprint that links another sprint signed review (cross-sprint lift)', async () => { + const b = await build({ command: ACCEPT }); + process.env.CODEX_HOME = b.codexHome; + await reviewAndSprint(b.cwd); // S-001 + R-001 snapshot signed for sprint_id S-001 + const head = execFileSync('git', ['-C', b.cwd, 'rev-parse', 'HEAD']).toString().trim(); + await writeFile( + join(b.cwd, 'sprints/S-002.md'), + fm({ + id: 'S-002', + title: 'Two', + epic_id: 'E-001', + status: 'review', + lane: 'main', + allowed_paths: ['src/**'], + review_required: true, + review_id: 'R-001', + base_sha: head, + }), + 'utf8', + ); + commitAll(b.cwd, 'S-002 points at R-001'); + const r = await runCloseCommand('S-002', { cwd: b.cwd, dryRun: false, json: false }); + expect(r.exitCode).not.toBe(0); + expect(`${r.stderr}${r.stdout}`).toMatch(/targets sprint S-001|REVIEWER_GATE/i); + }); + + it('blocks close when the snapshot reviewer differs from the stamped reviewer', async () => { + const b = await build({ command: ACCEPT, secondReviewer: true }); + process.env.CODEX_HOME = b.codexHome; + await reviewAndSprint(b.cwd); // snapshot.reviewer = codex + const file = join(b.cwd, 'reviews/R-001.md'); + const parsed = matter(await readFile(file, 'utf8')); + parsed.data.reviewer = 'codex2'; // configured, but not who signed the snapshot + await writeFile(file, matter.stringify(parsed.content, parsed.data), 'utf8'); + commitAll(b.cwd, 're-stamp reviewer'); + const r = await runCloseCommand('S-001', { cwd: b.cwd, dryRun: false, json: false }); + expect(r.exitCode).not.toBe(0); + expect(`${r.stderr}${r.stdout}`).toMatch( + /produced by "codex"|REVIEWER_GATE_SIGNATURE_INVALID/i, + ); + }); + + it('enforces a gate stamped with a configured non-default reviewer (ungated default)', async () => { + const b = await build({ command: CHANGES, defaultReviewer: 'manual' }); + process.env.CODEX_HOME = b.codexHome; + // default reviewer "manual" has no gate, but the review is stamped codex (gated). + await runReviewCreateCommand({ cwd: b.cwd, sprintId: 'S-001', json: false, reviewer: 'codex' }); + await runReviewCommand('S-001', { cwd: b.cwd, dryRun: false, json: false }); + await runReviewSprintCommand('S-001', { cwd: b.cwd, dryRun: false, json: false }); + commitAll(b.cwd, 'gated by non-default reviewer'); + const r = await runCloseCommand('S-001', { cwd: b.cwd, dryRun: false, json: false }); + expect(r.exitCode).not.toBe(0); + expect(`${r.stderr}${r.stdout}`).toMatch(/REVIEWER_GATE_NOT_ACCEPTED|gate verdict/i); + }); + + it('blocks close when the sprint scope changed after the gate', async () => { + const b = await build({ command: ACCEPT }); + process.env.CODEX_HOME = b.codexHome; + await reviewAndSprint(b.cwd); + const sf = join(b.cwd, 'sprints/S-001.md'); + const parsed = matter(await readFile(sf, 'utf8')); + parsed.data.allowed_paths = ['**']; // widen scope post-gate + await writeFile(sf, matter.stringify(parsed.content, parsed.data), 'utf8'); + commitAll(b.cwd, 'widen scope'); + const r = await runCloseCommand('S-001', { cwd: b.cwd, dryRun: false, json: false }); + expect(r.exitCode).not.toBe(0); + expect(`${r.stderr}${r.stdout}`).toMatch(/different scope|REVIEWER_GATE_STALE/i); + }); + + it('blocks close when the project config changed after the gate', async () => { + const b = await build({ command: ACCEPT }); + process.env.CODEX_HOME = b.codexHome; + await reviewAndSprint(b.cwd); + await writeFile(join(b.cwd, 'repokernel.config.yaml'), `${gatedConfig()}# touched\n`, 'utf8'); + commitAll(b.cwd, 'touch config'); + const r = await runCloseCommand('S-001', { cwd: b.cwd, dryRun: false, json: false }); + expect(r.exitCode).not.toBe(0); + expect(`${r.stderr}${r.stdout}`).toMatch(/config .*changed|REVIEWER_GATE_STALE/i); + }); + + // Parallel autonomous close path: must honor BOTH lanes, not bypass them. + it('closeAfterMerge fails closed when the gate snapshot is not accepted', async () => { + const b = await build({ command: CHANGES }); + process.env.CODEX_HOME = b.codexHome; + // Built-in verdict accepted (review-sprint), but the gate snapshot is + // changes_requested — the gate lane must still block. + await reviewAndSprint(b.cwd); + await expect(closeAfterMerge('S-001', 'R-001', b.cwd)).rejects.toThrow( + /reviewer gate blocked/i, + ); + }); + + it('closeAfterMerge fails closed when the built-in verdict is not accepted', async () => { + const b = await build({ command: ACCEPT }); + process.env.CODEX_HOME = b.codexHome; + // Gate accepted, but review.verdict left pending (no review-sprint) — the + // built-in lane must block (closeAfterMerge must not ship a pending review). + await runReviewCommand('S-001', { cwd: b.cwd, dryRun: false, json: false }); + await expect(closeAfterMerge('S-001', 'R-001', b.cwd)).rejects.toThrow(/not accepted/i); + }); + + it('closeAfterMerge ships when both the gate and the built-in verdict are accepted', async () => { + const b = await build({ command: ACCEPT }); + process.env.CODEX_HOME = b.codexHome; + await reviewAndSprint(b.cwd); + await expect(closeAfterMerge('S-001', 'R-001', b.cwd)).resolves.toBeDefined(); + }); + + it('a recorded snapshot is enforced even if config later opts out of review', async () => { + const b = await build({ command: CHANGES }); + process.env.CODEX_HOME = b.codexHome; + await reviewAndSprint(b.cwd); // changes_requested snapshot recorded + // Opt out of review at the project level AFTER the gate ran, keeping the + // reviewer config so only the requirement is weakened. + await writeFile( + join(b.cwd, 'repokernel.config.yaml'), + `${gatedConfig()}policies:\n requireReviewForShipped: false\n`, + 'utf8', + ); + commitAll(b.cwd, 'opt out of review post-gate'); + const r = await runCloseCommand('S-001', { cwd: b.cwd, dryRun: false, json: false }); + expect(r.exitCode).not.toBe(0); + expect(`${r.stderr}${r.stdout}`).toMatch(/REVIEWER_GATE_NOT_ACCEPTED|gate verdict/i); + }); +}); diff --git a/packages/core/src/errors/RepoKernelError.ts b/packages/core/src/errors/RepoKernelError.ts index a0ab847..f9f2ec4 100644 --- a/packages/core/src/errors/RepoKernelError.ts +++ b/packages/core/src/errors/RepoKernelError.ts @@ -11,6 +11,7 @@ export type RepoKernelErrorKind = | 'INVALID_FRONTMATTER' | 'INVALID_SENTINEL_OUTPUT' | 'IO_ERROR' + | 'GATE_KEY_INVALID' | 'SECRET_DETECTED' | 'SECRET_SCAN_FAILED' | 'TRUST_DENIED' diff --git a/packages/core/src/gate/index.ts b/packages/core/src/gate/index.ts new file mode 100644 index 0000000..580c825 --- /dev/null +++ b/packages/core/src/gate/index.ts @@ -0,0 +1,3 @@ +export * from './required.js'; +export * from './secret.js'; +export * from './signature.js'; diff --git a/packages/core/src/gate/required.ts b/packages/core/src/gate/required.ts new file mode 100644 index 0000000..f7ea48f --- /dev/null +++ b/packages/core/src/gate/required.ts @@ -0,0 +1,75 @@ +import { + type Automation, + type Config, + type ReviewerGateConfig, + resolveReviewerGate, +} from '../config/schema.js'; +import type { Review, ReviewVerdict } from '../schemas/review.js'; +import type { Sprint } from '../schemas/sprint.js'; +import { effectiveReviewRequired } from '../validator/helpers.js'; + +/** The review fields that influence whether a gate is required. */ +export type GateRequirementReview = Pick; + +/** + * A reviewer-gate snapshot is mandatory for closing `sprint` when the project + * requires review for it AND a gate applies. A gate applies when the project + * configures a default gate, OR the linked review is stamped with a configured + * (possibly non-default) reviewer, OR a snapshot already exists on the review. + * + * The last two clauses close the dodge where a project's default reviewer has + * no gate but the review was stamped with a gated reviewer (or already carries a + * gate result): such a review must still be enforced, not silently ignored. + * Anchored on config + `review_required` + the linked review — never letting a + * mutable field flip the requirement OFF. Pure. + */ +export function gateRequired( + sprint: Pick, + config: Pick, + review?: GateRequirementReview, +): boolean { + // A recorded snapshot is a commitment: enforce it regardless of the current + // policy. Otherwise a post-gate config edit (opt out of review, or raise the + // threshold above this sprint) would void a signed gate verdict — a bypass. + if (review?.reviewer_gate != null) return true; + if (!effectiveReviewRequired(sprint, config)) return false; + if (resolveReviewerGate(config.automation) !== null) return true; + if (review && reviewerGateConfigFor(config.automation, review.reviewer) !== undefined) + return true; + return false; +} + +/** + * The gate configured for a specific stamped reviewer, if any. When + * `gateRequired` is true but this returns undefined, the review named a + * reviewer with no configured gate — callers fail closed. Pure. + */ +export function reviewerGateConfigFor( + automation: Automation, + reviewerName: string, +): ReviewerGateConfig | undefined { + return automation.reviewers?.[reviewerName]; +} + +const VERDICT_RANK: Record = { + rejected: 3, + changes_requested: 2, + accepted: 1, + pending: 0, +}; + +/** + * Most-restrictive-wins composition of two review verdicts, matching the + * registry merge precedence (`rejected > changes_requested > accepted > + * pending`). Symmetric. Used to surface a single combined verdict for a sprint + * whose gate lane and built-in/panel lane may disagree. Pure. + */ +export function composeVerdict(a: ReviewVerdict, b: ReviewVerdict): ReviewVerdict { + if (a === b) return a; + if (a === 'rejected' || b === 'rejected') return 'rejected'; + if (a === 'changes_requested' || b === 'changes_requested') return 'changes_requested'; + if (a === 'accepted' || b === 'accepted') return 'accepted'; + return 'pending'; +} + +export { VERDICT_RANK }; diff --git a/packages/core/src/gate/secret.ts b/packages/core/src/gate/secret.ts new file mode 100644 index 0000000..3931438 --- /dev/null +++ b/packages/core/src/gate/secret.ts @@ -0,0 +1,45 @@ +import { lstat, readFile } from 'node:fs/promises'; +import { dirname, join, resolve } from 'node:path'; +import { trustFilePath } from '../trust/loader.js'; + +/** Override for the gate signing-secret path (tests, non-default homes). */ +export const GATE_SECRET_ENV = 'REPOKERNEL_GATE_SECRET_FILE'; + +/** + * Machine-local signing secret for reviewer-gate snapshots. Co-located with the + * trust file (`~/.repokernel/gate.key` by default) — never inside the repo, so + * a repo-bound agent cannot read it to forge a snapshot. Same trust boundary as + * the reviewer command pin. Deriving from the trust-file directory means any + * context that isolates the trust file (tests, alternate homes) isolates the + * gate key too, without a second env override. + */ +export function gateSecretPath(env: NodeJS.ProcessEnv = process.env): string { + const override = env[GATE_SECRET_ENV]; + if (override && override.length > 0) return resolve(override); + return join(dirname(trustFilePath(env)), 'gate.key'); +} + +const HEX64_RE = /^[a-f0-9]{64}$/u; + +/** + * Read the gate signing secret. Returns the trimmed hex string, or `null` when + * the file is absent, not a regular file (a symlink could redirect the trust + * boundary), or malformed. Read-only — callers that need to mint a secret use + * the CLI-side `loadOrCreateGateSecret`. Returning `null` fails close + * verification closed. + */ +export async function loadGateSecret(env: NodeJS.ProcessEnv = process.env): Promise { + const path = gateSecretPath(env); + try { + const stat = await lstat(path); + if (!stat.isFile()) return null; + // A signing key readable/writable by group or other defeats its purpose: + // any same-host account could read it and forge snapshots. Reject loose + // permissions on POSIX (the minter writes 0600). Windows ACLs differ — skip. + if (process.platform !== 'win32' && (stat.mode & 0o077) !== 0) return null; + const raw = (await readFile(path, 'utf8')).trim(); + return HEX64_RE.test(raw) ? raw : null; + } catch { + return null; + } +} diff --git a/packages/core/src/gate/signature.ts b/packages/core/src/gate/signature.ts new file mode 100644 index 0000000..76ce515 --- /dev/null +++ b/packages/core/src/gate/signature.ts @@ -0,0 +1,89 @@ +import { createHmac, timingSafeEqual } from 'node:crypto'; +import type { ReviewerGateSnapshot, ReviewFinding } from '../schemas/review.js'; + +/** + * Fields signed by the reviewer gate. The signature binds the verdict to a + * specific review, sprint, attempt, and reviewed commit range so a snapshot + * cannot be lifted into another review (`review_id`/`sprint_id`), replayed + * across a different range (`base_sha`/`end_sha`), or survive a re-review + * (`review_attempt`). `findings`/`summary` are included so they cannot be + * swapped under an otherwise-valid `accepted` signature. + */ +export interface GateSignaturePayload { + readonly review_id: string; + readonly sprint_id: string; + readonly reviewer: string; + readonly review_attempt: number; + readonly verdict: ReviewerGateSnapshot['verdict']; + readonly base_sha: string; + readonly end_sha: string; + readonly reviewed_at: string; + readonly findings: readonly ReviewFinding[]; + readonly summary?: string | undefined; +} + +const PAYLOAD_VERSION = 'rk-gate-v1'; + +/** + * Deterministic JSON: object keys sorted recursively so the canonical bytes are + * independent of insertion order. Required because the snapshot round-trips + * through YAML frontmatter, which does not preserve key order — sign and verify + * must agree byte-for-byte regardless of how the object was reconstructed. + */ +function stableStringify(value: unknown): string { + if (value === null || typeof value !== 'object') return JSON.stringify(value) ?? 'null'; + if (Array.isArray(value)) return `[${value.map(stableStringify).join(',')}]`; + const entries = Object.entries(value as Record) + .filter(([, v]) => v !== undefined) + .sort(([a], [b]) => (a < b ? -1 : a > b ? 1 : 0)); + return `{${entries.map(([k, v]) => `${JSON.stringify(k)}:${stableStringify(v)}`).join(',')}}`; +} + +/** Canonical, signable bytes for a gate snapshot. Pure. */ +export function canonicalGatePayload(payload: GateSignaturePayload): string { + return stableStringify({ + v: PAYLOAD_VERSION, + review_id: payload.review_id, + sprint_id: payload.sprint_id, + reviewer: payload.reviewer, + review_attempt: payload.review_attempt, + verdict: payload.verdict, + base_sha: payload.base_sha, + end_sha: payload.end_sha, + reviewed_at: payload.reviewed_at, + findings: payload.findings, + ...(payload.summary !== undefined ? { summary: payload.summary } : {}), + }); +} + +/** HMAC-SHA256 (hex) of the canonical payload. Pure. */ +export function signGatePayload(secret: string, payload: GateSignaturePayload): string { + return createHmac('sha256', secret).update(canonicalGatePayload(payload)).digest('hex'); +} + +/** + * Constant-time check that `snapshot.signature` matches an HMAC recomputed from + * the snapshot's own fields plus the review/sprint binding. Returns false on any + * mismatch or malformed signature; never throws. Pure. + */ +export function verifyGateSignature( + secret: string, + snapshot: ReviewerGateSnapshot, + binding: { readonly review_id: string; readonly sprint_id: string }, +): boolean { + const expected = signGatePayload(secret, { + review_id: binding.review_id, + sprint_id: binding.sprint_id, + reviewer: snapshot.reviewer, + review_attempt: snapshot.review_attempt, + verdict: snapshot.verdict, + base_sha: snapshot.base_sha, + end_sha: snapshot.end_sha, + reviewed_at: snapshot.reviewed_at, + findings: snapshot.findings, + summary: snapshot.summary, + }); + const a = Buffer.from(expected, 'hex'); + const b = Buffer.from(snapshot.signature, 'hex'); + return a.length === b.length && timingSafeEqual(a, b); +} diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index 5300119..8bcb2ad 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -4,6 +4,7 @@ export * from './api.js'; export * from './config/index.js'; export type { RepoKernelErrorKind } from './errors/RepoKernelError.js'; export { docsUrl, RepoKernelError, toErrorMessage } from './errors/RepoKernelError.js'; +export * from './gate/index.js'; export * from './graph/index.js'; export * from './next/index.js'; export * from './output/index.js'; diff --git a/packages/core/src/schemas/review.ts b/packages/core/src/schemas/review.ts index 321974a..9f37689 100644 --- a/packages/core/src/schemas/review.ts +++ b/packages/core/src/schemas/review.ts @@ -34,6 +34,37 @@ export const ReviewerGateOutputSchema = z .strict(); export type ReviewerGateOutput = z.infer; +/** + * Immutable, signed record of one reviewer-gate run, stored in the review + * frontmatter under `reviewer_gate` — separate from the mutable + * `verdict`/`findings` fields that the built-in rule eval, the panel, and the + * manual override own. The gate is the only producer of this object, so a later + * `review-sprint`/panel/`review-verdict`/`re-review` pass cannot overwrite the + * gate decision: those writers touch sibling keys, not this one. + * + * `signature` is an HMAC-SHA256 (hex) over a canonical payload bound to the + * review id, sprint id, attempt, verdict, base_sha, end_sha, reviewed_at, and + * findings, keyed by a machine-local secret kept outside the repo. A snapshot + * hand-written into a committed review file therefore cannot be forged, lifted + * into another review, or replayed across commit ranges. `review_attempt` binds + * the snapshot to one attempt: `re-review` bumps the attempt, which invalidates + * any earlier snapshot and forces a fresh gate run. + */ +export const ReviewerGateSnapshotSchema = z + .object({ + reviewer: z.string().min(1), + review_attempt: z.number().int().min(1), + verdict: ReviewerGateVerdictSchema, + findings: z.array(ReviewFindingSchema).default([]), + base_sha: ShaSchema, + end_sha: ShaSchema, + reviewed_at: z.string().datetime({ offset: true }), + summary: z.string().min(1).optional(), + signature: z.string().regex(/^[a-f0-9]{64}$/u), + }) + .strict(); +export type ReviewerGateSnapshot = z.infer; + export const ReviewPathsCheckedSchema = z .object({ allowed_paths_matched: z.boolean().optional(), @@ -191,6 +222,7 @@ export const ReviewFrontmatterSchema = z panel_runs: optionalNullable(z.array(PanelRunSchema)), panel_aggregate: optionalNullable(PanelVerdictSchema), panel_policy_snapshot: optionalNullable(PanelPolicySnapshotSchema), + reviewer_gate: optionalNullable(ReviewerGateSnapshotSchema), extras: z.record(z.unknown()).default({}), }) .strict(); diff --git a/packages/core/src/validator/codes.ts b/packages/core/src/validator/codes.ts index 70328cc..5d624e1 100644 --- a/packages/core/src/validator/codes.ts +++ b/packages/core/src/validator/codes.ts @@ -61,6 +61,12 @@ export const FINDING_CODES = { NEXT_MD_LANE_MISMATCH: 'NEXT_MD_LANE_MISMATCH', // Review panel REVIEW_PANEL_VERDICT_CONFLICT: 'REVIEW_PANEL_VERDICT_CONFLICT', + // Reviewer gate snapshot integrity + REVIEWER_GATE_MISSING: 'REVIEWER_GATE_MISSING', + REVIEWER_GATE_NOT_ACCEPTED: 'REVIEWER_GATE_NOT_ACCEPTED', + REVIEWER_GATE_STALE: 'REVIEWER_GATE_STALE', + REVIEWER_GATE_ATTEMPT_MISMATCH: 'REVIEWER_GATE_ATTEMPT_MISMATCH', + REVIEWER_GATE_SIGNATURE_INVALID: 'REVIEWER_GATE_SIGNATURE_INVALID', // Review schema (specific parse failures — emitted in addition to PARSER_FAILURE only when generic context remains) REVIEW_INVALID_VERDICT: 'REVIEW_INVALID_VERDICT', REVIEW_INVALID_FINDING_SHAPE: 'REVIEW_INVALID_FINDING_SHAPE', diff --git a/packages/core/src/validator/index.ts b/packages/core/src/validator/index.ts index e89cdae..47fe2f1 100644 --- a/packages/core/src/validator/index.ts +++ b/packages/core/src/validator/index.ts @@ -8,6 +8,10 @@ export { type ReviewRequirement, type ReviewRequirementReason, } from './helpers.js'; +export { + reviewerGateIntegrityRule, + reviewerGateMissingRule, +} from './rules/reviewerGateIntegrity.js'; export { reviewIntegrityRule } from './rules/reviewIntegrity.js'; export { findingAppliesToTarget, diff --git a/packages/core/src/validator/rules/index.ts b/packages/core/src/validator/rules/index.ts index 98be65d..3c716a6 100644 --- a/packages/core/src/validator/rules/index.ts +++ b/packages/core/src/validator/rules/index.ts @@ -14,6 +14,7 @@ import { queueLaneRule } from './queueLane.js'; import { queueDuplicateRule, queueRefsRule } from './queueRefs.js'; import { queueStatusRule } from './queueStatusRules.js'; import { reviewConflictRule } from './reviewConflict.js'; +import { reviewerGateIntegrityRule, reviewerGateMissingRule } from './reviewerGateIntegrity.js'; import { reviewIntegrityRule } from './reviewIntegrity.js'; import { reviewPanelConflictRule } from './reviewPanelConflict.js'; import { reviewRefsRule } from './reviewRefs.js'; @@ -48,6 +49,8 @@ export const rules: readonly ScopedRule[] = [ { scope: 'live', run: reviewIntegrityRule }, { scope: 'live', run: reviewConflictRule }, { scope: 'live', run: reviewPanelConflictRule }, + { scope: 'live', run: reviewerGateIntegrityRule }, + { scope: 'audit', run: reviewerGateMissingRule }, { scope: 'live', run: sprintEpicMembershipRule }, { scope: 'live', run: sprintReviewByPolicyRule }, { scope: 'live', run: sprintSectionPlaceholderRule }, diff --git a/packages/core/src/validator/rules/reviewerGateIntegrity.ts b/packages/core/src/validator/rules/reviewerGateIntegrity.ts new file mode 100644 index 0000000..5d35c16 --- /dev/null +++ b/packages/core/src/validator/rules/reviewerGateIntegrity.ts @@ -0,0 +1,118 @@ +import { gateRequired } from '../../gate/required.js'; +import type { Finding } from '../../schemas/finding.js'; +import { FINDING_CODES } from '../codes.js'; +import type { ValidatorRule } from '../engine.js'; + +/** + * Live integrity of a PRESENT reviewer_gate snapshot on shipped, gate-required + * sprints. Structural only — it cannot verify the HMAC signature (the + * machine-local key is not available to the pure validator engine, and CI runs + * without it). Signature authenticity + freshness are enforced at `rk close`, + * the only path that ships. This rule catches a snapshot that was downgraded, + * left on a stale attempt, attributed to the wrong reviewer, or whose base + * commit drifted. A MISSING snapshot is handled separately at audit scope + * (`reviewerGateMissingRule`) so upgrading a repo with pre-snapshot shipped + * history does not flood live validation. + */ +export const reviewerGateIntegrityRule: ValidatorRule = ({ graph, parsed, config }) => { + const out: Finding[] = []; + + for (const sprint of parsed.sprints) { + if (sprint.status !== 'shipped') continue; + if (!sprint.review_id) continue; + + const review = graph.reviews.get(sprint.review_id); + if (!review) continue; // dangling review_id handled by reviewIntegrityRule + + const snapshot = review.reviewer_gate; + if (!snapshot) continue; // missing handled by reviewerGateMissingRule (audit) + if (!gateRequired(sprint, config, review)) continue; + + if (snapshot.reviewer !== review.reviewer) { + out.push({ + severity: 'P1', + code: FINDING_CODES.REVIEWER_GATE_SIGNATURE_INVALID, + message: `review ${review.id} is stamped reviewer "${review.reviewer}" but its reviewer_gate was produced by "${snapshot.reviewer}"`, + file: review.file, + entityType: 'review', + entityId: review.id, + data: { review_reviewer: review.reviewer, gate_reviewer: snapshot.reviewer }, + }); + } + + if (snapshot.verdict !== 'accepted') { + out.push({ + severity: 'P0', + code: FINDING_CODES.REVIEWER_GATE_NOT_ACCEPTED, + message: `shipped sprint ${sprint.id} reviewer_gate verdict is ${snapshot.verdict}`, + file: review.file, + entityType: 'review', + entityId: review.id, + data: { verdict: snapshot.verdict }, + }); + } + + if (snapshot.review_attempt !== review.review_attempt) { + out.push({ + severity: 'P1', + code: FINDING_CODES.REVIEWER_GATE_ATTEMPT_MISMATCH, + message: `review ${review.id} reviewer_gate is for attempt ${snapshot.review_attempt} but review_attempt is ${review.review_attempt}`, + file: review.file, + entityType: 'review', + entityId: review.id, + data: { snapshot_attempt: snapshot.review_attempt, review_attempt: review.review_attempt }, + }); + } + + // base_sha is fixed at sprint start; a snapshot whose base differs was + // signed against a different range. end_sha is NOT compared: close advances + // sprint.end_sha past the gate's reviewed commit with its own metadata + // commit, so equality would always fail post-close. + if (sprint.base_sha && snapshot.base_sha !== sprint.base_sha) { + out.push({ + severity: 'P1', + code: FINDING_CODES.REVIEWER_GATE_STALE, + message: `sprint ${sprint.id} base_sha ${sprint.base_sha} does not match reviewer_gate base_sha ${snapshot.base_sha}`, + file: review.file, + entityType: 'review', + entityId: review.id, + data: { sprint_base_sha: sprint.base_sha, gate_base_sha: snapshot.base_sha }, + }); + } + } + + return out; +}; + +/** + * Audit-scope check for a shipped, gate-required sprint whose review carries NO + * reviewer_gate snapshot. Audit (not live) by design: a repo upgraded to the + * snapshot model has historical shipped sprints with no snapshot, and a past + * close cannot be cheaply or safely backfilled. Forward enforcement happens at + * close; this surfaces the gap on demand without breaking `rk validate`. + * Mirrors the `shippedFieldsRule` audit treatment of frozen post-close state. + */ +export const reviewerGateMissingRule: ValidatorRule = ({ graph, parsed, config }) => { + const out: Finding[] = []; + + for (const sprint of parsed.sprints) { + if (sprint.status !== 'shipped') continue; + if (!sprint.review_id) continue; + + const review = graph.reviews.get(sprint.review_id); + if (!review) continue; + if (review.reviewer_gate) continue; + if (!gateRequired(sprint, config, review)) continue; + + out.push({ + severity: 'P1', + code: FINDING_CODES.REVIEWER_GATE_MISSING, + message: `shipped sprint ${sprint.id} requires a reviewer gate but ${review.id} has no reviewer_gate snapshot`, + file: review.file, + entityType: 'review', + entityId: review.id, + }); + } + + return out; +}; diff --git a/packages/core/test/reviewerGateIntegrity.test.ts b/packages/core/test/reviewerGateIntegrity.test.ts new file mode 100644 index 0000000..41d7246 --- /dev/null +++ b/packages/core/test/reviewerGateIntegrity.test.ts @@ -0,0 +1,112 @@ +import { afterAll, describe, expect, it } from 'vitest'; +import { validateProject } from '../src/index.js'; +import { cleanupAllFixtures, defaultConfigYaml, fm, makeFixture } from './helpers/fixture.js'; + +afterAll(cleanupAllFixtures); + +const BASE = 'a'.repeat(40); +const SIG = 'b'.repeat(64); + +function gatedConfig(): string { + return `${defaultConfigYaml()}automation: + defaultReviewer: codex + reviewers: + codex: + authMode: chatgpt +`; +} + +const epic = fm({ id: 'E-001', title: 'e', status: 'active', sprints: ['S-001'] }); + +function shippedSprint(): string { + return fm({ + id: 'S-001', + title: 's', + epic_id: 'E-001', + status: 'shipped', + lane: 'main', + review_required: true, + review_id: 'R-001', + base_sha: BASE, + end_sha: 'c'.repeat(40), + closed_at: '2026-06-02T00:00:00Z', + }); +} + +function review(extra: Record): string { + return fm({ + id: 'R-001', + sprint_id: 'S-001', + verdict: 'accepted', + reviewer: 'codex', + review_attempt: 1, + created_at: '2026-06-01T00:00:00Z', + ...extra, + }); +} + +function snapshot(extra: Record = {}): Record { + return { + reviewer: 'codex', + review_attempt: 1, + verdict: 'accepted', + findings: [], + base_sha: BASE, + end_sha: 'd'.repeat(40), + reviewed_at: '2026-06-01T12:00:00Z', + signature: SIG, + ...extra, + }; +} + +async function validate( + reviewExtra: Record, + config = gatedConfig(), + scope: 'live' | 'all' = 'live', +) { + const fixture = await makeFixture([ + { path: 'repokernel.config.yaml', content: config }, + { path: 'epics/E-001.md', content: epic }, + { path: 'sprints/S-001.md', content: shippedSprint() }, + { path: 'reviews/R-001.md', content: review(reviewExtra) }, + ]); + return validateProject({ cwd: fixture.cwd, scope }); +} + +const has = (r: Awaited>, code: string) => + r.findings.some((f) => f.code === code); + +describe('reviewerGateIntegrityRule', () => { + it('flags a missing snapshot only at audit scope (historical hygiene), not live', async () => { + expect(has(await validate({}, gatedConfig(), 'live'), 'REVIEWER_GATE_MISSING')).toBe(false); + expect(has(await validate({}, gatedConfig(), 'all'), 'REVIEWER_GATE_MISSING')).toBe(true); + }); + + it('flags a snapshot whose verdict is not accepted', async () => { + const r = await validate({ reviewer_gate: snapshot({ verdict: 'changes_requested' }) }); + expect(has(r, 'REVIEWER_GATE_NOT_ACCEPTED')).toBe(true); + }); + + it('flags a snapshot recorded for a stale attempt', async () => { + const r = await validate({ review_attempt: 2, reviewer_gate: snapshot({ review_attempt: 1 }) }); + expect(has(r, 'REVIEWER_GATE_ATTEMPT_MISMATCH')).toBe(true); + }); + + it('flags a snapshot whose base_sha drifts from the sprint', async () => { + const r = await validate({ reviewer_gate: snapshot({ base_sha: 'e'.repeat(40) }) }); + expect(has(r, 'REVIEWER_GATE_STALE')).toBe(true); + }); + + it('passes a clean accepted, current-attempt, base-consistent snapshot', async () => { + const r = await validate({ reviewer_gate: snapshot() }); + expect(has(r, 'REVIEWER_GATE_MISSING')).toBe(false); + expect(has(r, 'REVIEWER_GATE_NOT_ACCEPTED')).toBe(false); + expect(has(r, 'REVIEWER_GATE_ATTEMPT_MISMATCH')).toBe(false); + expect(has(r, 'REVIEWER_GATE_STALE')).toBe(false); + }); + + it('does not fire for a non-gated project (no configured reviewer gate)', async () => { + const r = await validate({}, defaultConfigYaml()); + expect(has(r, 'REVIEWER_GATE_MISSING')).toBe(false); + }); +}); diff --git a/packages/core/test/reviewerGateSnapshot.test.ts b/packages/core/test/reviewerGateSnapshot.test.ts new file mode 100644 index 0000000..9d9ecac --- /dev/null +++ b/packages/core/test/reviewerGateSnapshot.test.ts @@ -0,0 +1,165 @@ +import { describe, expect, it } from 'vitest'; +import type { Config } from '../src/config/index.js'; +import { AutomationSchema } from '../src/config/index.js'; +import { + composeVerdict, + gateRequired, + reviewerGateConfigFor, + signGatePayload, + verifyGateSignature, +} from '../src/gate/index.js'; +import { + type ReviewerGateSnapshot, + ReviewerGateSnapshotSchema, + ReviewFrontmatterSchema, +} from '../src/schemas/index.js'; + +const SECRET = 'a'.repeat(64); +const SHA_A = '1111111111111111111111111111111111111111'; +const SHA_B = '2222222222222222222222222222222222222222'; + +function signedSnapshot(over: Partial = {}): ReviewerGateSnapshot { + const base = { + reviewer: 'codex', + review_attempt: 1, + verdict: 'accepted' as const, + findings: [], + base_sha: SHA_A, + end_sha: SHA_B, + reviewed_at: '2026-06-02T00:00:00.000Z', + ...over, + }; + const signature = signGatePayload(SECRET, { ...base, review_id: 'R-1', sprint_id: 'S-1' }); + return ReviewerGateSnapshotSchema.parse({ ...base, signature }); +} + +const REVIEW_BASE = { + id: 'R-1', + sprint_id: 'S-1', + verdict: 'pending' as const, + reviewer: 'codex', + created_at: '2026-06-02T00:00:00.000Z', +}; + +describe('ReviewerGateSnapshotSchema', () => { + it('parses a well-formed signed snapshot', () => { + const snap = signedSnapshot(); + expect(snap.verdict).toBe('accepted'); + expect(snap.signature).toMatch(/^[a-f0-9]{64}$/); + }); + + it('rejects a non-hex64 signature and unknown keys (strict)', () => { + expect(() => + ReviewerGateSnapshotSchema.parse({ ...signedSnapshot(), signature: 'nope' }), + ).toThrow(); + expect(() => ReviewerGateSnapshotSchema.parse({ ...signedSnapshot(), extra: 1 })).toThrow(); + }); + + it('rejects the pending verdict (a gate always decides)', () => { + expect(() => signedSnapshot({ verdict: 'pending' as never })).toThrow(); + }); +}); + +describe('ReviewFrontmatterSchema migration', () => { + it('parses a legacy review file with no reviewer_gate', () => { + const r = ReviewFrontmatterSchema.parse(REVIEW_BASE); + expect(r.reviewer_gate).toBeUndefined(); + }); + + it('parses a review file carrying a reviewer_gate snapshot', () => { + const r = ReviewFrontmatterSchema.parse({ ...REVIEW_BASE, reviewer_gate: signedSnapshot() }); + expect(r.reviewer_gate?.verdict).toBe('accepted'); + }); + + it('coerces reviewer_gate: null to undefined (optionalNullable)', () => { + const r = ReviewFrontmatterSchema.parse({ ...REVIEW_BASE, reviewer_gate: null }); + expect(r.reviewer_gate).toBeUndefined(); + }); +}); + +describe('gate signature', () => { + it('round-trips: a freshly signed snapshot verifies', () => { + const snap = signedSnapshot(); + expect(verifyGateSignature(SECRET, snap, { review_id: 'R-1', sprint_id: 'S-1' })).toBe(true); + }); + + it('fails when the verdict is tampered after signing', () => { + const snap = { ...signedSnapshot(), verdict: 'changes_requested' as const }; + expect(verifyGateSignature(SECRET, snap, { review_id: 'R-1', sprint_id: 'S-1' })).toBe(false); + }); + + it('fails when lifted into another review (binding mismatch)', () => { + const snap = signedSnapshot(); + expect(verifyGateSignature(SECRET, snap, { review_id: 'R-2', sprint_id: 'S-1' })).toBe(false); + expect(verifyGateSignature(SECRET, snap, { review_id: 'R-1', sprint_id: 'S-2' })).toBe(false); + }); + + it('fails under a different secret', () => { + const snap = signedSnapshot(); + expect(verifyGateSignature('b'.repeat(64), snap, { review_id: 'R-1', sprint_id: 'S-1' })).toBe( + false, + ); + }); + + it('is order-independent for nested finding data (survives YAML key reordering)', () => { + const findings = [{ severity: 'HIGH' as const, message: 'x', data: { a: 1, b: 2 } }]; + const snap = signedSnapshot({ findings }); + const reordered: ReviewerGateSnapshot = { + ...snap, + findings: [{ severity: 'HIGH', message: 'x', data: { b: 2, a: 1 } }], + }; + expect(verifyGateSignature(SECRET, reordered, { review_id: 'R-1', sprint_id: 'S-1' })).toBe( + true, + ); + }); +}); + +describe('gateRequired / composeVerdict', () => { + const policiesOn = { requireReviewForShipped: true } as Config['policies']; + const policiesOff = { requireReviewForShipped: false } as Config['policies']; + const automationGated = AutomationSchema.parse({ + defaultReviewer: 'codex', + reviewers: { codex: {} }, + }); + const automationUngated = AutomationSchema.parse({ defaultReviewer: 'codex' }); + + it('requires a gate when review is required AND a default gate is configured', () => { + expect( + gateRequired( + { id: 'S-1', review_required: true }, + { policies: policiesOn, automation: automationGated }, + ), + ).toBe(true); + }); + + it('does not require a gate when no reviewer gate is configured', () => { + expect( + gateRequired( + { id: 'S-1', review_required: true }, + { policies: policiesOn, automation: automationUngated }, + ), + ).toBe(false); + }); + + it('does not require a gate when review is not required', () => { + expect( + gateRequired( + { id: 'S-1', review_required: false }, + { policies: policiesOff, automation: automationGated }, + ), + ).toBe(false); + }); + + it('resolves the configured gate for a reviewer, undefined for an unconfigured one', () => { + expect(reviewerGateConfigFor(automationGated, 'codex')).toBeDefined(); + expect(reviewerGateConfigFor(automationGated, 'agent')).toBeUndefined(); + }); + + it('composes most-restrictive-wins', () => { + expect(composeVerdict('accepted', 'changes_requested')).toBe('changes_requested'); + expect(composeVerdict('accepted', 'rejected')).toBe('rejected'); + expect(composeVerdict('changes_requested', 'rejected')).toBe('rejected'); + expect(composeVerdict('accepted', 'accepted')).toBe('accepted'); + expect(composeVerdict('accepted', 'pending')).toBe('accepted'); + }); +});