From 32cfe7ec605c89ae48a09647a5cf5afd2aab168a Mon Sep 17 00:00:00 2001 From: Xan Torres Date: Tue, 2 Jun 2026 13:08:44 +0300 Subject: [PATCH 1/5] feat(core): signed reviewer_gate snapshot schema + crypto foundation Add an immutable, HMAC-signed reviewer_gate snapshot type alongside the review frontmatter, plus the signing/verification and gate-required helpers it will be enforced with. Inert: nothing reads or writes the snapshot yet. - ReviewerGateSnapshotSchema (reviewer, attempt, verdict, base/end_sha, reviewed_at, findings, signature); optional reviewer_gate field on the review schema so existing review files still parse. - gate/signature: canonical (key-order-stable) payload + HMAC-SHA256 sign/verify, bound to review id, sprint id, attempt, and commit range. - gate/secret: machine-local signing key resolver (~/.repokernel/gate.key). - gate/required: gateRequired predicate + most-restrictive composeVerdict. - New finding codes + catalog entries for gate integrity. --- packages/cli/src/ux/explanations.ts | 36 ++++ packages/core/src/gate/index.ts | 3 + packages/core/src/gate/required.ts | 58 ++++++ packages/core/src/gate/secret.ts | 36 ++++ packages/core/src/gate/signature.ts | 89 ++++++++++ packages/core/src/index.ts | 1 + packages/core/src/schemas/review.ts | 32 ++++ packages/core/src/validator/codes.ts | 6 + .../core/test/reviewerGateSnapshot.test.ts | 165 ++++++++++++++++++ 9 files changed, 426 insertions(+) create mode 100644 packages/core/src/gate/index.ts create mode 100644 packages/core/src/gate/required.ts create mode 100644 packages/core/src/gate/secret.ts create mode 100644 packages/core/src/gate/signature.ts create mode 100644 packages/core/test/reviewerGateSnapshot.test.ts diff --git a/packages/cli/src/ux/explanations.ts b/packages/cli/src/ux/explanations.ts index 5fe4f61..f51e513 100644 --- a/packages/cli/src/ux/explanations.ts +++ b/packages/cli/src/ux/explanations.ts @@ -456,6 +456,42 @@ const CATALOG = { fix: 'Replace the placeholder comment in the reported section with concrete planning detail before review.', command: 'rk inspect --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/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..59afd6e --- /dev/null +++ b/packages/core/src/gate/required.ts @@ -0,0 +1,58 @@ +import { + type Automation, + type Config, + type ReviewerGateConfig, + resolveReviewerGate, +} from '../config/schema.js'; +import type { ReviewVerdict } from '../schemas/review.js'; +import type { Sprint } from '../schemas/sprint.js'; +import { effectiveReviewRequired } from '../validator/helpers.js'; + +/** + * A reviewer-gate snapshot is mandatory for closing `sprint` when the project + * both requires review for it AND configures a default reviewer gate. Anchored + * on config + the sprint's `review_required`, never on the mutable + * `review.reviewer` field, so a snapshot/review cannot dodge the gate by + * renaming its reviewer. Pure. + */ +export function gateRequired( + sprint: Pick, + config: Pick, +): boolean { + return effectiveReviewRequired(sprint, config) && resolveReviewerGate(config.automation) !== null; +} + +/** + * 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..9bbc428 --- /dev/null +++ b/packages/core/src/gate/secret.ts @@ -0,0 +1,36 @@ +import { readFile } from 'node:fs/promises'; +import { homedir } from 'node:os'; +import { join, resolve } from 'node:path'; + +/** 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/`) — never inside the repo, so a repo-bound agent + * cannot read it to forge a snapshot. Same trust boundary as the reviewer + * command pin. + */ +const DEFAULT_GATE_SECRET_PATH = join(homedir(), '.repokernel', 'gate.key'); + +export function gateSecretPath(env: NodeJS.ProcessEnv = process.env): string { + const override = env[GATE_SECRET_ENV]; + if (override && override.length > 0) return resolve(override); + return DEFAULT_GATE_SECRET_PATH; +} + +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 or malformed. Read-only — callers that need to mint a + * secret use the CLI-side `loadOrCreateGateSecret`. + */ +export async function loadGateSecret(env: NodeJS.ProcessEnv = process.env): Promise { + try { + const raw = (await readFile(gateSecretPath(env), '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..cec8e79 --- /dev/null +++ b/packages/core/src/gate/signature.ts @@ -0,0 +1,89 @@ +import { createHmac, timingSafeEqual } from 'node:crypto'; +import type { ReviewerGateSnapshot } 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: ReviewerGateSnapshot['findings']; + 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/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'); + }); +}); From c18c43487429fb75267604601eb4dc967801cbaf Mon Sep 17 00:00:00 2001 From: Xan Torres Date: Tue, 2 Jun 2026 13:30:29 +0300 Subject: [PATCH 2/5] feat: enforce signed reviewer_gate snapshot at close MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The reviewer gate now records its decision only in the signed, per-attempt reviewer_gate snapshot — never in review.verdict. close (and every path that funnels through it: ship, autonomous run, epic close) requires BOTH the built-in/panel review verdict AND the gate snapshot to be green, using most-restrictive-wins composition. The snapshot lives in a frontmatter key no other command writes, so review-sprint, review-panel, review-verdict, and re-review can no longer clear or overwrite the gate decision. close additionally requires the snapshot to be present, signature-valid (machine-local key), bound to the current review attempt, accepted, and content-fresh against its end_sha — always on, not bypassable with --skip-checks. - gate writer records snapshot-only (reviewerGate.ts); reviewed range moves into the snapshot so close stamps review.end_sha with the shipped commit. - gateEnforce.ts: shared close/ship snapshot evaluator (presence, signature, attempt, verdict, freshness), anchored on config not the mutable reviewer. - gateSecret.ts: machine-local signing key, minted on first gate run. - reviewerGateIntegrity validator rule: CI-side structural net for shipped sprints (presence, verdict, attempt, base_sha) — signature is enforced at close, the only path that ships. - status surfaces the gate verdict + attempt. - adversarial tests: overwrite, forgery, attempt bump, snapshot strip, reviewer dodge, --skip-checks stale, legacy migration, validator tamper. --- packages/cli/src/commands/lifecycle.ts | 16 ++ packages/cli/src/commands/status.ts | 10 + packages/cli/src/lifecycle/gateEnforce.ts | 143 ++++++++++ packages/cli/src/lifecycle/gateSecret.ts | 33 +++ packages/cli/src/lifecycle/reviewerGate.ts | 36 ++- packages/cli/test/review.test.ts | 30 +- packages/cli/test/reviewerGate.test.ts | 23 +- packages/cli/test/reviewerGateEnforce.test.ts | 257 ++++++++++++++++++ packages/core/src/gate/secret.ts | 18 +- packages/core/src/gate/signature.ts | 4 +- packages/core/src/validator/index.ts | 1 + packages/core/src/validator/rules/index.ts | 2 + .../validator/rules/reviewerGateIntegrity.ts | 82 ++++++ .../core/test/reviewerGateIntegrity.test.ts | 108 ++++++++ 14 files changed, 731 insertions(+), 32 deletions(-) create mode 100644 packages/cli/src/lifecycle/gateEnforce.ts create mode 100644 packages/cli/src/lifecycle/gateSecret.ts create mode 100644 packages/cli/test/reviewerGateEnforce.test.ts create mode 100644 packages/core/src/validator/rules/reviewerGateIntegrity.ts create mode 100644 packages/core/test/reviewerGateIntegrity.test.ts diff --git a/packages/cli/src/commands/lifecycle.ts b/packages/cli/src/commands/lifecycle.ts index 62ae090..08c0d19 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,21 @@ 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, + }); + 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 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..24b4838 --- /dev/null +++ b/packages/cli/src/lifecycle/gateEnforce.ts @@ -0,0 +1,143 @@ +import { + type Config, + gateRequired, + loadGateSecret, + materialPathGlobs, + type Review, + reviewerGateConfigFor, + type Sprint, + verifyGateSignature, +} from '@repokernel/core'; +import { inScopeFiles } from './diffClassifier.js'; +import { changedFilesSince } from './git.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), bound to the current attempt, accepted, + * and fresh against the committed tree. Otherwise returns the blocking reason. + * + * Always-on: this is not bypassable with `--skip-checks`. Anchored on config + + * the sprint's `review_required`, so a snapshot/review cannot dodge the gate by + * renaming its reviewer. + */ +export async function evaluateReviewerGate(opts: { + readonly checkPath: string; + readonly config: Config; + readonly sprint: Sprint; + readonly review: Review; + readonly env?: NodeJS.ProcessEnv; +}): Promise { + const { checkPath, config, sprint, review } = opts; + if (!gateRequired(sprint, config)) return OK; + + const reviewGateHint = `rk review-gate ${sprint.id}`; + + 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}`, + }, + }; + } + + const secret = await loadGateSecret(opts.env); + if ( + secret === null || + !verifyGateSignature(secret, snapshot, { + review_id: review.id, + sprint_id: review.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 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}`, + }, + }; + } + + const sinceReview = await changedFilesSince(checkPath, snapshot.end_sha); + 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..704521e --- /dev/null +++ b/packages/cli/src/lifecycle/gateSecret.ts @@ -0,0 +1,33 @@ +import { randomBytes } from 'node:crypto'; +import { mkdir, readFile, writeFile } from 'node:fs/promises'; +import { dirname } from 'node:path'; +import { gateSecretPath, loadGateSecret } 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 { + // Lost a race (or the file appeared): trust whatever is now on disk. + const raced = await loadGateSecret(env); + if (raced) return raced; + // Re-read raw in case it exists but failed the strict loader; surface a + // clear failure instead of silently signing with an ephemeral key. + return (await readFile(path, 'utf8')).trim(); + } +} diff --git a/packages/cli/src/lifecycle/reviewerGate.ts b/packages/cli/src/lifecycle/reviewerGate.ts index 8f89c20..6793c7a 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, @@ -617,15 +619,36 @@ export async function runReviewerGate(input: ReviewerGateInput): Promise { - // 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 +656,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(); }, 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..8c6b85a --- /dev/null +++ b/packages/cli/test/reviewerGateEnforce.test.ts @@ -0,0 +1,257 @@ +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 { + 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'): string { + return `${defaultConfigYaml()}automation: + defaultReviewer: ${defaultReviewer} + reviewers: + codex: + authMode: chatgpt +`; +} + +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; +}): 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), + }, + { + 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); + }); +}); diff --git a/packages/core/src/gate/secret.ts b/packages/core/src/gate/secret.ts index 9bbc428..882d2ff 100644 --- a/packages/core/src/gate/secret.ts +++ b/packages/core/src/gate/secret.ts @@ -1,22 +1,22 @@ import { readFile } from 'node:fs/promises'; -import { homedir } from 'node:os'; -import { join, resolve } from 'node:path'; +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/`) — never inside the repo, so a repo-bound agent - * cannot read it to forge a snapshot. Same trust boundary as the reviewer - * command pin. + * 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. */ -const DEFAULT_GATE_SECRET_PATH = join(homedir(), '.repokernel', 'gate.key'); - export function gateSecretPath(env: NodeJS.ProcessEnv = process.env): string { const override = env[GATE_SECRET_ENV]; if (override && override.length > 0) return resolve(override); - return DEFAULT_GATE_SECRET_PATH; + return join(dirname(trustFilePath(env)), 'gate.key'); } const HEX64_RE = /^[a-f0-9]{64}$/u; diff --git a/packages/core/src/gate/signature.ts b/packages/core/src/gate/signature.ts index cec8e79..76ce515 100644 --- a/packages/core/src/gate/signature.ts +++ b/packages/core/src/gate/signature.ts @@ -1,5 +1,5 @@ import { createHmac, timingSafeEqual } from 'node:crypto'; -import type { ReviewerGateSnapshot } from '../schemas/review.js'; +import type { ReviewerGateSnapshot, ReviewFinding } from '../schemas/review.js'; /** * Fields signed by the reviewer gate. The signature binds the verdict to a @@ -18,7 +18,7 @@ export interface GateSignaturePayload { readonly base_sha: string; readonly end_sha: string; readonly reviewed_at: string; - readonly findings: ReviewerGateSnapshot['findings']; + readonly findings: readonly ReviewFinding[]; readonly summary?: string | undefined; } diff --git a/packages/core/src/validator/index.ts b/packages/core/src/validator/index.ts index e89cdae..2ec0693 100644 --- a/packages/core/src/validator/index.ts +++ b/packages/core/src/validator/index.ts @@ -8,6 +8,7 @@ export { type ReviewRequirement, type ReviewRequirementReason, } from './helpers.js'; +export { reviewerGateIntegrityRule } 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..8d23331 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 } from './reviewerGateIntegrity.js'; import { reviewIntegrityRule } from './reviewIntegrity.js'; import { reviewPanelConflictRule } from './reviewPanelConflict.js'; import { reviewRefsRule } from './reviewRefs.js'; @@ -48,6 +49,7 @@ export const rules: readonly ScopedRule[] = [ { scope: 'live', run: reviewIntegrityRule }, { scope: 'live', run: reviewConflictRule }, { scope: 'live', run: reviewPanelConflictRule }, + { scope: 'live', run: reviewerGateIntegrityRule }, { 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..1ff2a8b --- /dev/null +++ b/packages/core/src/validator/rules/reviewerGateIntegrity.ts @@ -0,0 +1,82 @@ +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'; + +/** + * Post-hoc integrity of the 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 is enforced at `rk close`, the only path + * that ships. This rule is the CI-side net that catches a snapshot that was + * removed, downgraded, left on a stale attempt, or whose committed range drifted + * from the shipped sprint after the fact. + */ +export const reviewerGateIntegrityRule: ValidatorRule = ({ graph, parsed, config }) => { + const out: Finding[] = []; + + for (const sprint of parsed.sprints) { + if (sprint.status !== 'shipped') continue; + if (!gateRequired(sprint, config)) continue; + if (!sprint.review_id) continue; // missing-review handled by reviewIntegrityRule + + const review = graph.reviews.get(sprint.review_id); + if (!review) continue; // dangling review_id handled by reviewIntegrityRule + + const snapshot = review.reviewer_gate; + if (!snapshot) { + out.push({ + severity: 'P0', + 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, + }); + continue; + } + + 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 intentionally 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. Content + // freshness against the gate's end_sha is enforced at close (always-on). + 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; +}; diff --git a/packages/core/test/reviewerGateIntegrity.test.ts b/packages/core/test/reviewerGateIntegrity.test.ts new file mode 100644 index 0000000..f03501d --- /dev/null +++ b/packages/core/test/reviewerGateIntegrity.test.ts @@ -0,0 +1,108 @@ +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()) { + 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 }); +} + +const has = (r: Awaited>, code: string) => + r.findings.some((f) => f.code === code); + +describe('reviewerGateIntegrityRule', () => { + it('flags a shipped gated sprint whose review has no reviewer_gate snapshot', async () => { + const r = await validate({}); + expect(has(r, '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); + }); +}); From c381507cca2a961f1b5efd500cf5def9a17bad3f Mon Sep 17 00:00:00 2001 From: Xan Torres Date: Tue, 2 Jun 2026 13:32:11 +0300 Subject: [PATCH 3/5] docs: reviewer_gate snapshot model, signing key, and migration Document the signed per-attempt reviewer_gate snapshot: the machine-local signing key in the trust model, and a changelog entry covering the verdict-storage change, the new validation codes, status surfacing, and the one-time re-gate migration for in-flight reviews. --- CHANGELOG.md | 34 ++++++++++++++++++++++++++++++++++ docs/trust.md | 21 +++++++++++++++++++++ 2 files changed, 55 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8065b89..cd8479c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,6 +3,40 @@ 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: `rk validate` flags a shipped, + gate-required sprint whose snapshot is missing, not accepted, on a stale + attempt, or whose base commit drifted (`REVIEWER_GATE_MISSING`, + `REVIEWER_GATE_NOT_ACCEPTED`, `REVIEWER_GATE_ATTEMPT_MISMATCH`, + `REVIEWER_GATE_STALE`). Signature authenticity is enforced at close. +- `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..f8aef3b 100644 --- a/docs/trust.md +++ b/docs/trust.md @@ -71,6 +71,27 @@ 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 (presence, verdict, attempt, + base_sha) without the key, so CI catches structural tampering even though it + cannot verify the signature. + ## Error kinds | Kind | When | What to do | From cf3136186b999f50466d3848fcde489c95af686c Mon Sep 17 00:00:00 2001 From: Xan Torres Date: Tue, 2 Jun 2026 14:10:41 +0300 Subject: [PATCH 4/5] fix: harden reviewer_gate enforcement against review findings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address a second-round review of the reviewer_gate snapshot. The snapshot direction was right but enforcement signed a narrower thing than it checked and was scattered across close/run/validate; these close the gaps. - Bind verification to the SPRINT BEING CLOSED, not review.sprint_id: a sprint linking another sprint's signed review now fails closed (cross-sprint lift). - Reject a snapshot whose reviewer differs from the review's stamped reviewer (the local key is shared across reviewers); mirror in validation. - gateRequired now considers the linked review: a configured non-default reviewer, or an existing snapshot, makes the gate mandatory — a gated reviewer can no longer be dodged by an ungated project default. - Freshness now invalidates the snapshot when the project config or the sprint's scope (allowed/denied/generated paths) changed since the gate, comparing scope fields at base_sha so status churn is not a false positive. - Autonomous close no longer bypasses the gate: sequential runs the built-in review lane before close; parallel closeAfterMerge enforces the gate (and fails closed when required but unsatisfied). - Gate key: strict-load only (never sign with a malformed key), reject non-regular files (symlink redirection); new GATE_KEY_INVALID error. - Missing-snapshot on shipped history is audit-scope, not live, so upgrading a repo with pre-snapshot shipped sprints does not flood validation. - rk review-gate accepted hint points to review-sprint then close. Adds adversarial tests for cross-sprint lift, reviewer mismatch, non-default reviewer enforcement, and scope/config drift. --- packages/cli/src/commands/lifecycle.ts | 1 + packages/cli/src/commands/reviewGate.ts | 2 +- packages/cli/src/commands/run.ts | 41 ++++++- packages/cli/src/lifecycle/gateEnforce.ts | 94 ++++++++++++--- packages/cli/src/lifecycle/gateSecret.ts | 18 +-- packages/cli/src/lifecycle/parallelRunner.ts | 30 ++++- packages/cli/test/reviewerGateEnforce.test.ts | 109 +++++++++++++++++- packages/core/src/errors/RepoKernelError.ts | 1 + packages/core/src/gate/required.ts | 26 ++++- packages/core/src/gate/secret.ts | 13 ++- packages/core/src/validator/index.ts | 5 +- packages/core/src/validator/rules/index.ts | 3 +- .../validator/rules/reviewerGateIntegrity.ts | 68 ++++++++--- .../core/test/reviewerGateIntegrity.test.ts | 14 ++- 14 files changed, 364 insertions(+), 61 deletions(-) diff --git a/packages/cli/src/commands/lifecycle.ts b/packages/cli/src/commands/lifecycle.ts index 08c0d19..826fe67 100644 --- a/packages/cli/src/commands/lifecycle.ts +++ b/packages/cli/src/commands/lifecycle.ts @@ -682,6 +682,7 @@ export async function runCloseCommand( config: outcome.config, sprint, review, + configFile: relative(cwd, outcome.configPath), }); if (!gateEval.ok) { return err(gateEval.block.code, gateEval.block.message, gateEval.block.hint); diff --git a/packages/cli/src/commands/reviewGate.ts b/packages/cli/src/commands/reviewGate.ts index d31aaf7..8af4370 100644 --- a/packages/cli/src/commands/reviewGate.ts +++ b/packages/cli/src/commands/reviewGate.ts @@ -166,7 +166,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..2fafb86 100644 --- a/packages/cli/src/commands/run.ts +++ b/packages/cli/src/commands/run.ts @@ -54,6 +54,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'; @@ -771,8 +772,44 @@ 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 evalReviewFile = evalOutcome.ok + ? evalOutcome.graph.reviews.get(evalOutcome.graph.sprints.get(sprint.id)?.review_id ?? '') + ?.file + : undefined; + if (evalReviewFile) { + await stagePathsAndCommit( + executionCwd, + [join(executionCwd, evalReviewFile), join(executionCwd, config.paths.registry)], + `chore(rk): record review verdict for ${sprint.id}`, + ); + } + + // 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/lifecycle/gateEnforce.ts b/packages/cli/src/lifecycle/gateEnforce.ts index 24b4838..e1e01b8 100644 --- a/packages/cli/src/lifecycle/gateEnforce.ts +++ b/packages/cli/src/lifecycle/gateEnforce.ts @@ -9,7 +9,8 @@ import { verifyGateSignature, } from '@repokernel/core'; import { inScopeFiles } from './diffClassifier.js'; -import { changedFilesSince } from './git.js'; +import { changedFilesSince, fileAtCommit } from './git.js'; +import { parseSprintScope } from './reviewerGate.js'; export interface GateBlock { readonly code: @@ -31,25 +32,43 @@ 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), bound to the current attempt, accepted, - * and fresh against the committed tree. Otherwise returns the blocking reason. + * 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: this is not bypassable with `--skip-checks`. Anchored on config + - * the sprint's `review_required`, so a snapshot/review cannot dodge the gate by - * renaming its reviewer. + * 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 } = opts; - if (!gateRequired(sprint, config)) return OK; + 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, @@ -73,13 +92,24 @@ export async function evaluateReviewerGate(opts: { }; } + // 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: review.sprint_id, - }) + !verifyGateSignature(secret, snapshot, { review_id: review.id, sprint_id: sprint.id }) ) { return { ok: false, @@ -88,7 +118,7 @@ export async function evaluateReviewerGate(opts: { 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 or signed on another machine`, + : `${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}`, }, }; @@ -117,6 +147,44 @@ export async function evaluateReviewerGate(opts: { } 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, diff --git a/packages/cli/src/lifecycle/gateSecret.ts b/packages/cli/src/lifecycle/gateSecret.ts index 704521e..63e2671 100644 --- a/packages/cli/src/lifecycle/gateSecret.ts +++ b/packages/cli/src/lifecycle/gateSecret.ts @@ -1,7 +1,7 @@ import { randomBytes } from 'node:crypto'; -import { mkdir, readFile, writeFile } from 'node:fs/promises'; +import { mkdir, writeFile } from 'node:fs/promises'; import { dirname } from 'node:path'; -import { gateSecretPath, loadGateSecret } from '@repokernel/core'; +import { gateSecretPath, loadGateSecret, RepoKernelError } from '@repokernel/core'; /** * Read the machine-local gate signing secret, minting one on first use. The @@ -22,12 +22,16 @@ export async function loadOrCreateGateSecret( try { await writeFile(path, `${secret}\n`, { mode: 0o600, flag: 'wx' }); return secret; - } catch { - // Lost a race (or the file appeared): trust whatever is now on disk. + } 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; - // Re-read raw in case it exists but failed the strict loader; surface a - // clear failure instead of silently signing with an ephemeral key. - return (await readFile(path, 'utf8')).trim(); + 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..0ec2f76 100644 --- a/packages/cli/src/lifecycle/parallelRunner.ts +++ b/packages/cli/src/lifecycle/parallelRunner.ts @@ -1,8 +1,9 @@ -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 { 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 +420,31 @@ 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; + 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/test/reviewerGateEnforce.test.ts b/packages/cli/test/reviewerGateEnforce.test.ts index 8c6b85a..9667024 100644 --- a/packages/cli/test/reviewerGateEnforce.test.ts +++ b/packages/cli/test/reviewerGateEnforce.test.ts @@ -13,6 +13,7 @@ import { } 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, @@ -28,13 +29,13 @@ const CHANGES = join(FIXTURES, 'changes.sh'); afterAll(cleanupAllFixtures); -function gatedConfig(defaultReviewer = 'codex'): string { +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 { @@ -64,12 +65,16 @@ 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), + content: + opts.gated === false + ? defaultConfigYaml() + : gatedConfig(opts.defaultReviewer, opts.secondReviewer), }, { path: 'epics/E-001.md', @@ -254,4 +259,102 @@ describe('reviewer-gate enforcement at close', () => { 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 the gate, not bypass it. + it('closeAfterMerge fails closed when the gate snapshot is not accepted', async () => { + const b = await build({ command: CHANGES }); + process.env.CODEX_HOME = b.codexHome; + // runReviewCommand auto-commits its mutations (incl. the snapshot). + await runReviewCommand('S-001', { cwd: b.cwd, dryRun: false, json: false }); // changes_requested snapshot + await expect(closeAfterMerge('S-001', 'R-001', b.cwd)).rejects.toThrow( + /reviewer gate blocked/i, + ); + }); + + it('closeAfterMerge ships when the gate snapshot is accepted', async () => { + const b = await build({ command: ACCEPT }); + process.env.CODEX_HOME = b.codexHome; + await runReviewCommand('S-001', { cwd: b.cwd, dryRun: false, json: false }); // accepted snapshot + await expect(closeAfterMerge('S-001', 'R-001', b.cwd)).resolves.toBeDefined(); + }); }); 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/required.ts b/packages/core/src/gate/required.ts index 59afd6e..10930a4 100644 --- a/packages/core/src/gate/required.ts +++ b/packages/core/src/gate/required.ts @@ -4,22 +4,36 @@ import { type ReviewerGateConfig, resolveReviewerGate, } from '../config/schema.js'; -import type { ReviewVerdict } from '../schemas/review.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 - * both requires review for it AND configures a default reviewer gate. Anchored - * on config + the sprint's `review_required`, never on the mutable - * `review.reviewer` field, so a snapshot/review cannot dodge the gate by - * renaming its reviewer. Pure. + * 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 { - return effectiveReviewRequired(sprint, config) && resolveReviewerGate(config.automation) !== null; + if (!effectiveReviewRequired(sprint, config)) return false; + if (resolveReviewerGate(config.automation) !== null) return true; + if (review && reviewerGateConfigFor(config.automation, review.reviewer) !== undefined) + return true; + if (review?.reviewer_gate != null) return true; + return false; } /** diff --git a/packages/core/src/gate/secret.ts b/packages/core/src/gate/secret.ts index 882d2ff..556722d 100644 --- a/packages/core/src/gate/secret.ts +++ b/packages/core/src/gate/secret.ts @@ -1,4 +1,4 @@ -import { readFile } from 'node:fs/promises'; +import { lstat, readFile } from 'node:fs/promises'; import { dirname, join, resolve } from 'node:path'; import { trustFilePath } from '../trust/loader.js'; @@ -23,12 +23,17 @@ 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 or malformed. Read-only — callers that need to mint a - * secret use the CLI-side `loadOrCreateGateSecret`. + * 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 raw = (await readFile(gateSecretPath(env), 'utf8')).trim(); + const stat = await lstat(path); + if (!stat.isFile()) 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/validator/index.ts b/packages/core/src/validator/index.ts index 2ec0693..47fe2f1 100644 --- a/packages/core/src/validator/index.ts +++ b/packages/core/src/validator/index.ts @@ -8,7 +8,10 @@ export { type ReviewRequirement, type ReviewRequirementReason, } from './helpers.js'; -export { reviewerGateIntegrityRule } from './rules/reviewerGateIntegrity.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 8d23331..3c716a6 100644 --- a/packages/core/src/validator/rules/index.ts +++ b/packages/core/src/validator/rules/index.ts @@ -14,7 +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 } from './reviewerGateIntegrity.js'; +import { reviewerGateIntegrityRule, reviewerGateMissingRule } from './reviewerGateIntegrity.js'; import { reviewIntegrityRule } from './reviewIntegrity.js'; import { reviewPanelConflictRule } from './reviewPanelConflict.js'; import { reviewRefsRule } from './reviewRefs.js'; @@ -50,6 +50,7 @@ export const rules: readonly ScopedRule[] = [ { 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 index 1ff2a8b..5d35c16 100644 --- a/packages/core/src/validator/rules/reviewerGateIntegrity.ts +++ b/packages/core/src/validator/rules/reviewerGateIntegrity.ts @@ -4,36 +4,40 @@ import { FINDING_CODES } from '../codes.js'; import type { ValidatorRule } from '../engine.js'; /** - * Post-hoc integrity of the reviewer_gate snapshot on shipped, gate-required + * 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 is enforced at `rk close`, the only path - * that ships. This rule is the CI-side net that catches a snapshot that was - * removed, downgraded, left on a stale attempt, or whose committed range drifted - * from the shipped sprint after the fact. + * 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 (!gateRequired(sprint, config)) continue; - if (!sprint.review_id) continue; // missing-review handled by reviewIntegrityRule + 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) { + if (!snapshot) continue; // missing handled by reviewerGateMissingRule (audit) + if (!gateRequired(sprint, config, review)) continue; + + if (snapshot.reviewer !== review.reviewer) { out.push({ - severity: 'P0', - code: FINDING_CODES.REVIEWER_GATE_MISSING, - message: `shipped sprint ${sprint.id} requires a reviewer gate but ${review.id} has no reviewer_gate snapshot`, + 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 }, }); - continue; } if (snapshot.verdict !== 'accepted') { @@ -61,10 +65,9 @@ export const reviewerGateIntegrityRule: ValidatorRule = ({ graph, parsed, config } // base_sha is fixed at sprint start; a snapshot whose base differs was - // signed against a different range. end_sha is intentionally 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. Content - // freshness against the gate's end_sha is enforced at close (always-on). + // 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', @@ -80,3 +83,36 @@ export const reviewerGateIntegrityRule: ValidatorRule = ({ graph, parsed, config 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 index f03501d..41d7246 100644 --- a/packages/core/test/reviewerGateIntegrity.test.ts +++ b/packages/core/test/reviewerGateIntegrity.test.ts @@ -59,23 +59,27 @@ function snapshot(extra: Record = {}): Record }; } -async function validate(reviewExtra: Record, config = gatedConfig()) { +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 }); + return validateProject({ cwd: fixture.cwd, scope }); } const has = (r: Awaited>, code: string) => r.findings.some((f) => f.code === code); describe('reviewerGateIntegrityRule', () => { - it('flags a shipped gated sprint whose review has no reviewer_gate snapshot', async () => { - const r = await validate({}); - expect(has(r, 'REVIEWER_GATE_MISSING')).toBe(true); + 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 () => { From 93e615a85e30817296c9e9a8e9064e03b03c112d Mon Sep 17 00:00:00 2001 From: Xan Torres Date: Tue, 2 Jun 2026 14:49:25 +0300 Subject: [PATCH 5/5] fix: close reviewer_gate policy-boundary gaps from review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Second hardening round addressing staff-level boundary findings: - A recorded snapshot is now enforced regardless of current policy: gateRequired short-circuits true when reviewer_gate is present, and close evaluates it even when the sprint is no longer review-required — a post-gate config opt-out or threshold bump can't void a signed verdict. - close pins snapshot.base_sha === sprint.base_sha before scope lookup, so a retargeted base is caught at close, not only post-ship at validate. - rk review-gate refuses a review whose sprint_id targets a different sprint (no snapshot poisoning). - Parallel close (closeAfterMerge) now requires the built-in accepted verdict for review-required sprints in addition to the gate; parallel runs refuse a gated review-required epic upfront (the parallel path runs no review pipeline). - Signing key: the reviewer's findings/summary are scrubbed of the exact key value before persist (HOME is inherited); keys with group/other permissions are rejected on POSIX. - rk review-gate JSON emits gate_verdict + reviewer_gate_recorded + next_actions so automation doesn't treat the gate verdict as close-ready. - Sequential autonomous halts as a review failure when review-sprint records a non-accepted verdict, instead of surfacing a later close failure. - Docs/CHANGELOG clarify that a missing snapshot is audit-scope, present-but-bad is live. Tests: cross-config-drift enforcement, base_sha pin, closeAfterMerge both-lane gating, key redaction path covered. --- CHANGELOG.md | 13 +++-- docs/trust.md | 9 ++-- packages/cli/src/commands/lifecycle.ts | 19 ++++++++ packages/cli/src/commands/reviewGate.ts | 18 +++++++ packages/cli/src/commands/run.ts | 48 +++++++++++++++++-- packages/cli/src/lifecycle/gateEnforce.ts | 15 ++++++ packages/cli/src/lifecycle/parallelRunner.ts | 24 +++++++++- packages/cli/src/lifecycle/reviewerGate.ts | 20 ++++++-- packages/cli/test/reviewerGateEnforce.test.ts | 37 ++++++++++++-- packages/core/src/gate/required.ts | 5 +- packages/core/src/gate/secret.ts | 4 ++ 11 files changed, 188 insertions(+), 24 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index cd8479c..80743ae 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -23,11 +23,14 @@ Format follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). ### Added -- `reviewer_gate` snapshot integrity validation: `rk validate` flags a shipped, - gate-required sprint whose snapshot is missing, not accepted, on a stale - attempt, or whose base commit drifted (`REVIEWER_GATE_MISSING`, - `REVIEWER_GATE_NOT_ACCEPTED`, `REVIEWER_GATE_ATTEMPT_MISMATCH`, - `REVIEWER_GATE_STALE`). Signature authenticity is enforced at close. +- `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 diff --git a/docs/trust.md b/docs/trust.md index f8aef3b..7ee1ebc 100644 --- a/docs/trust.md +++ b/docs/trust.md @@ -88,9 +88,12 @@ cannot be forged, lifted into another review, or replayed across commit ranges. - 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 (presence, verdict, attempt, - base_sha) without the key, so CI catches structural tampering even though it - cannot verify the signature. +- `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 diff --git a/packages/cli/src/commands/lifecycle.ts b/packages/cli/src/commands/lifecycle.ts index 826fe67..958b411 100644 --- a/packages/cli/src/commands/lifecycle.ts +++ b/packages/cli/src/commands/lifecycle.ts @@ -748,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 8af4370..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: '', }; diff --git a/packages/cli/src/commands/run.ts b/packages/cli/src/commands/run.ts index 2fafb86..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'; @@ -404,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, @@ -796,17 +817,36 @@ async function executeRunLoop( return evalResult; } const evalOutcome = await loadProject({ cwd: executionCwd }); - const evalReviewFile = evalOutcome.ok + const evalReview = evalOutcome.ok ? evalOutcome.graph.reviews.get(evalOutcome.graph.sprints.get(sprint.id)?.review_id ?? '') - ?.file : undefined; - if (evalReviewFile) { + if (evalReview?.file) { await stagePathsAndCommit( executionCwd, - [join(executionCwd, evalReviewFile), join(executionCwd, config.paths.registry)], + [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. diff --git a/packages/cli/src/lifecycle/gateEnforce.ts b/packages/cli/src/lifecycle/gateEnforce.ts index e1e01b8..bed39df 100644 --- a/packages/cli/src/lifecycle/gateEnforce.ts +++ b/packages/cli/src/lifecycle/gateEnforce.ts @@ -146,6 +146,21 @@ export async function evaluateReviewerGate(opts: { }; } + // 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 diff --git a/packages/cli/src/lifecycle/parallelRunner.ts b/packages/cli/src/lifecycle/parallelRunner.ts index 0ec2f76..fee1018 100644 --- a/packages/cli/src/lifecycle/parallelRunner.ts +++ b/packages/cli/src/lifecycle/parallelRunner.ts @@ -1,6 +1,12 @@ import { join, relative } from 'node:path'; import type { Epic, Run, RunId, Sprint, SprintId } from '@repokernel/core'; -import { gateRequired, 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'; @@ -426,6 +432,22 @@ export async function closeAfterMerge( // 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, diff --git a/packages/cli/src/lifecycle/reviewerGate.ts b/packages/cli/src/lifecycle/reviewerGate.ts index 6793c7a..f78bcdb 100644 --- a/packages/cli/src/lifecycle/reviewerGate.ts +++ b/packages/cli/src/lifecycle/reviewerGate.ts @@ -617,20 +617,30 @@ 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: allFindings, + findings: safeFindings, base_sha: baseSha, end_sha: endSha, reviewed_at: reviewedAt, ...(summary ? { summary } : {}), }; - const signature = signGatePayload(await loadOrCreateGateSecret(), { + const signature = signGatePayload(gateSecret, { ...snapshot, review_id: review.id, sprint_id: sprint.id, @@ -666,8 +676,8 @@ export async function runReviewerGate(input: ReviewerGateInput): Promise { expect(`${r.stderr}${r.stdout}`).toMatch(/config .*changed|REVIEWER_GATE_STALE/i); }); - // Parallel autonomous close path: must honor the gate, not bypass it. + // 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; - // runReviewCommand auto-commits its mutations (incl. the snapshot). - await runReviewCommand('S-001', { cwd: b.cwd, dryRun: false, json: false }); // changes_requested snapshot + // 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 ships when the gate snapshot is accepted', async () => { + 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; - await runReviewCommand('S-001', { cwd: b.cwd, dryRun: false, json: false }); // accepted snapshot + // 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/gate/required.ts b/packages/core/src/gate/required.ts index 10930a4..f7ea48f 100644 --- a/packages/core/src/gate/required.ts +++ b/packages/core/src/gate/required.ts @@ -28,11 +28,14 @@ export function gateRequired( 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; - if (review?.reviewer_gate != null) return true; return false; } diff --git a/packages/core/src/gate/secret.ts b/packages/core/src/gate/secret.ts index 556722d..3931438 100644 --- a/packages/core/src/gate/secret.ts +++ b/packages/core/src/gate/secret.ts @@ -33,6 +33,10 @@ export async function loadGateSecret(env: NodeJS.ProcessEnv = process.env): Prom 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 {