feat: first-class signed reviewer_gate snapshot enforced at close#49
Merged
Conversation
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.
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.
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.
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.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cf3136186b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+429
to
+436
| if (gateReview) { | ||
| const gateEval = await evaluateReviewerGate({ | ||
| checkPath: epicWorktree, | ||
| config: outcome.config, | ||
| sprint, | ||
| review: gateReview, | ||
| configFile: relative(epicWorktree, outcome.configPath), | ||
| }); |
There was a problem hiding this comment.
In the parallel close path, this new check verifies only the signed reviewer_gate snapshot and never requires gateReview.verdict === 'accepted' like runCloseCommand does. Because this commit moved the reviewer-gate result out of review.verdict, a gated sprint can have an accepted snapshot while the built-in/panel lane is still pending or has been changed to a non-accepted verdict; closeAfterMerge will still ship it in parallel/autonomous close. Please enforce the normal review verdict here before mutating the sprint to shipped.
Useful? React with 👍 / 👎.
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Context
The native reviewer gate recorded its verdict in the shared, mutable
review.verdict/review.findingsfrontmatter — the same fields owned andoverwritten by
rk review-sprint(built-in rule eval),rk review-panel(panel aggregate),
rk review-verdict(manual), andrk re-review(reset).rk closetrusted a singlereview.verdict === 'accepted'. So a gate'schanges_requested/rejectedcould be silently replaced by a later pass —most damagingly by
rk ship, which re-runsreview-sprint(built-in only,gate-blind) and then closes. The gate was an overwritable suggestion, not a
gate.
This PR models the reviewer gate as a first-class, per-attempt-immutable,
signed
reviewer_gatesnapshot stored separately from the human/built-in/panel verdict, and makes every close path require both the gate and the
built-in/panel verdict to be green (most-restrictive-wins).
Design
reviewer_gate(
reviewer,review_attempt,verdict,findings,base_sha,end_sha,reviewed_at,signature). No other command writes that key, so a verdictpass can't clear or overwrite it.
review.verdictbecomes the built-in/panellane exclusively.
~/.repokernel/gate.key,co-located with the trust file, never in the repo). The payload binds the
review id, the sprint being closed, attempt, verdict, findings, and the
reviewed commit range — so a snapshot can't be forged, lifted into another
review, or replayed across ranges.
closerequires the snapshotto be present, signature-valid, produced by the review's stamped reviewer,
bound to the current attempt, accepted, and content-fresh against its
end_shaand the policy/scope it reviewed — ANDreview.verdict === accepted. Always on; not bypassable with--skip-checks.field: a configured (possibly non-default) reviewer, or an existing snapshot,
makes the gate mandatory — it can't be dodged by renaming the reviewer.
rk close,rk ship, the sequential autonomousrun loop, and the parallel
closeAfterMergepath all enforce the gate (theparallel path fails closed when required but unsatisfied).
whose snapshot is not accepted / on a stale attempt / base-drifted (signature
authenticity is enforced at close, the only path that ships). A missing
snapshot on shipped history is audit-scope, so upgrading a repo doesn't flood
live validation.
What changed
feat(core)—ReviewerGateSnapshotSchema+ optionalreviewer_gatefield;gate/signature.ts(canonical, key-order-stable HMAC),gate/secret.ts(machine-local key),
gate/required.ts(gateRequired+composeVerdict);new finding codes.
feat— gate writes the signed snapshot only;close/ship/ autonomous /parallel enforce it; integrity validator; status surfaces the gate verdict.
docs— trust-model signing-key section, changelog, migration note.fix— hardening from review (see below).Hardening (post-review)
A round of adversarial review found enforcement was signing a narrower thing
than it checked, and was scattered. Fixed:
own (mutable)
sprint_id; reject a sprint that links another sprint's review.(the local key is shared across reviewers).
gateRequirednow considers the linked review (non-default reviewer gate, oran existing snapshot) — a gated reviewer can't be dodged.
scope (allowed/denied/generated paths) changed since the gate (scope compared
at
base_sha, so status churn is not a false positive).closeAfterMergepath enforces the gate instead of bypassing it.rejects non-regular files (symlink redirection).
rk review-gateaccepted hint points toreview-sprintthenclose.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 <sprint-id>once beforeclosing. Projects with no configured reviewer gate are unaffected.
Behavior change
rk review+ gate now leavesreview.verdictpending (the gate records onlyits snapshot). Run
rk review-sprint <sprint-id>beforerk close—rk shipand autonomous runs do this internally.
Test plan
pnpm -r build && pnpm -r typecheck— green.reviewerGateSnapshot,reviewerGateIntegrity).reviewerGateEnforce: overwrite, ship-launder,forgery, attempt bump, snapshot strip, reviewer dodge, cross-sprint lift,
reviewer mismatch, non-default reviewer enforcement, scope/config drift,
--skip-checksstale,closeAfterMergeaccept/block, legacy migration.Out of scope
A full unified
evaluateClosePreconditions()(built-in verdict + clean-tree +gate in one service) — the gate-lane evaluator is centralized; further
consolidation is a follow-up. Dedicated end-to-end tests for the autonomous
loops (heavy worktree harness) beyond the
closeAfterMergeunit coverage.