feat(evidence-gate): enforce review evidence

[allyblockcast]

Thinking Path

• Paperclip is the control plane used to move agent work through governed issue states.
• The artifact-evidence gate evaluates whether an issue has reviewable receipts before in_review.
• Phase 1 persisted and logged block verdicts but deliberately allowed the transition.
• BLO-4828 requires Phase 2 enforcement after the detector registry was hardened against production traffic.
• Operators still need a narrow, attributable emergency escape hatch that agents cannot self-authorize.
• This pull request rejects computed block verdicts, preserves fail-open behavior for evaluator failures, and adds a one-hour user-only override.
• The benefit is that narrated completion cannot enter review without evidence while legitimate operator intervention remains auditable.

Linked Issues or Issue Description

Problem / Motivation

Transitions to in_review currently persist verdict: "block" without rejecting the PATCH, allowing evidence-free work into review. This is the Phase 2 enforcement work tracked by Paperclip issue BLO-4828.

Proposed Solution

Return HTTP 422 with the missing evidence shapes for a computed block verdict. Accept only a recent user-authored evidence-gate: override <reason> comment, preserve fail-open behavior for evaluator exceptions, and leave already-in_review updates grandfathered.

Alternatives Considered

Keeping the gate warn-only does not prevent unsupported review transitions. A broader role- or agent-authored override weakens the trust boundary; the selected user-only, one-hour comment contract is narrow and auditable.

Roadmap Alignment

This is an explicitly staged follow-up to the existing artifact-evidence gate and BLO-4828, not duplicate core scope. ROADMAP.md was checked before implementation.

What Changed

• Reject new in_review transitions on a computed block verdict with { "error": "missing-evidence", "missing": [...] } while keeping evaluator failures fail-open.
• Recognize exact, nonblank operator override comments authored by a user within the prior hour; reject agent, expired, future, malformed, and blank overrides.
• Persist and log overridden plus overrideReason in the verdict contract.
• Document the enforcement response and operator override procedure.
• Add focused boundary and response-contract tests.

Verification

• pnpm exec vitest run server/src/__tests__/evidence-gate-wiring.test.ts server/src/__tests__/error-handler.test.ts (20 tests passed).
• pnpm -r typecheck passed.
• pnpm build passed, with existing Vite CSS/chunk warnings.
• pnpm test:run was attempted twice. The first attempt raced workspace-link repair with the concurrently running build. The isolated retry exceeded 20 minutes after an unrelated codex-local-execute.test.ts failure and transient test-database connection/FK errors; no changed-file test failed.

Risks

• This intentionally changes all computed block verdicts from telemetry-only to enforcement; false-positive detector results now stop review transitions.
• The override is comment-based and time-limited. Exact anchoring, user-only authorship, future-time rejection, and a nonblank reason constrain the bypass.
• Evaluation exceptions still fail open so a gate outage cannot wedge all issue transitions.

Model Used

• OpenAI openai/gpt-5.6-sol, tool-enabled software-engineering agent with repository, test, GitHub, and Paperclip access.

Checklist

• I have included a thinking path that traces from project context to this change
• I have specified the model used (with version and capability details)
• I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work
• I have searched GitHub for duplicate or related PRs and linked them above
• I have either (a) linked existing issues with Fixes: # / Closes # / Refs # OR (b) described the issue in-PR following the relevant issue template
• I have run tests locally and documented the repository-level suite failure
• I have added or updated tests where applicable
• If this change affects the UI, I have included before/after screenshots (not applicable; no UI change)
• I have updated relevant documentation to reflect my changes
• I have considered and documented any risks above
• All Paperclip CI gates are green
• Greptile is 5/5 with no open P2s, recommendations, or follow-ups
• I will address all Greptile and reviewer comments before requesting merge

[allyblockcast]

🔗 Paperclip issue: BLO-4828

[allyblockcast]

🔗 Paperclip issue: BLO-4828

[allyblockcast]

@ally Please review the Phase 2 trust boundary and failure semantics in particular: user-only one-hour override parsing, unconditional computed-block enforcement versus fail-open evaluator exceptions, exact 422 response shape, and grandfathering of already-in_review updates. Focused tests are 20/20; full-suite infrastructure failures are documented in the PR.

[allyblockcast]

Hey @allyblockcast[bot]! Before this PR can be reviewed, a few things need attention:

Missing or incomplete:

• No linked issue or inline issue description found — either tag an existing issue with Fixes #NNN / Closes #NNN / Refs #NNN, or describe the underlying issue inline in the PR body following one of our issue templates (https://github.com/paperclipai/paperclip/tree/master/.github/ISSUE_TEMPLATE). See CONTRIBUTING.md → "Link Issues or Describe Them In-PR".

Once updated, push a new commit and these checks will re-run automatically.

— commitperclip

[allyblockcast]

Addressed commitperclip's issue-description finding by restructuring the PR body to follow the feature template explicitly: Problem / Motivation, Proposed Solution, Alternatives Considered, and Roadmap Alignment. The tracked internal issue is BLO-4828; no unrelated GitHub issue is being auto-linked. Re-running the failed review check now.

[allyblockcast]

@ally Follow-up 7c818200 fixes a review-discovered trust-boundary edge case: a valid one-hour user override could be displaced from the evaluator's 10-comment window by later agent chatter. Please re-review the independent bounded override query, shared timestamp boundary, and displaced-override regression test. Focused tests: 21/21; server typecheck passed.

[allyblockcast]

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
reviewed head: 45b017a

Important Issues (1)

• [native-codex] server/src/services/issues.ts:6674 — The new unconditional if (inReviewVerdict?.verdict === "block") throw unprocessable(...) fires for any actor (agent or human) whenever the computed verdict is block, before the code ever reaches the existing flag-gated block a few lines below (experimental.enableInReviewEvidenceGate && shouldBlockUnreviewableInReview(...), issues.ts:6689-6699). shouldBlockUnreviewableInReview (server/src/services/in-review-gate.ts) only ever returns true for the exact same condition (verdict.verdict === "block"), so that predicate — and the enableInReviewEvidenceGate flag that gates it — is now unreachable dead code. Its own docstring/tests still promise "never blocks a human actor" (server/src/__tests__/in-review-gate.test.ts), which is no longer true system-wide once this PR lands, since the new unconditional check upstream already blocks humans too (consistent with this PR's own "operators need an override" framing). The retained comment at issues.ts:6680-6686 ("Wired behind the instance flag enableInReviewEvidenceGate, default off") is now misleading — toggling that flag has zero effect post-merge. Recommend either removing the now-redundant shouldBlockUnreviewableInReview call + flag, or adding a comment explaining it's fully subsumed and kept only for [some reason], so the next person touching this file isn't misled into thinking the flag still gates anything.

Suggestions (2)

• [pr-review-toolkit:code-reviewer] server/src/services/issues.ts:6636-6674 — Because the whole PATCH runs inside db.transaction (issues.ts:6827), throwing on a block verdict means patch.lastEvidenceVerdict is never actually persisted to the issue row for a rejected attempt — only the logger.info line records it. If any operator tooling/dashboards read lastEvidenceVerdict to audit block history, blocked attempts will be invisible there post-Phase-2 (only successful passes/overrides will show up). Worth confirming that's the intended tradeoff (atomicity) vs. wanting a queryable record of rejected attempts.
• [pr-review-toolkit:pr-test-analyzer] CI was still mostly pending at review time (General tests (server 1/4..4/4), Verify serialized server suites (1/4..4/4), e2e), and the PR body itself notes the full pnpm test:run didn't complete cleanly locally. Given this PR flips previously-recorded-only block verdicts into a hard 422 on any in_review PATCH, please confirm those server suites (issues-service.test.ts and friends) are green at this head before merge — a pre-existing integration test that PATCHes an issue straight to in_review without supplying evidence would now fail where it previously passed.

Strengths

• The operator-override matching (evidence-gate-wiring.ts:72-99) is covered by exhaustive boundary tests: agent-authored, expired, future-timestamped, malformed, and blank-reason overrides are each explicitly rejected.
• Verified the override can't be spoofed by an agent: issue_comments.authorUserId/authorAgentId are derived strictly from the authenticated actor server-side (assertIssueCommentAuthorTypeAllowed in issues.ts), not from client-supplied authorType, so the "user-only" override contract actually holds.
• The 422 response-contract change in error-handler.ts is narrowly scoped to the exact missing-evidence shape and doesn't touch any other HttpError path.
• Fail-open behavior for evaluator exceptions is preserved correctly — the new throw only fires on a successfully computed block verdict, not on evaluation failure.

Recommended Action

1. Clarify or clean up the now-dead enableInReviewEvidenceGate path before merge (Important).
2. Confirm the pending server/integration CI suites pass at this head given the behavior change (Suggestion).
3. Consider whether blocked-attempt verdicts should be queryable (Suggestion, non-blocking).

[allyblockcast]

Addressed the important finding in 65f1ed99: removed the now-unreachable shouldBlockUnreviewableInReview call and its misleading instance-flag commentary from issues.ts. The persisted enableInReviewEvidenceGate setting remains untouched for compatibility, but no longer appears to gate Phase 2 enforcement.

The rejected-attempt verdict intentionally rolls back with the PATCH transaction; the structured evidence-gate log remains the audit signal rather than partially committing issue state on a rejected mutation.

Verification: focused evidence-gate/in-review/error-handler Vitest suites pass (27/27), and @paperclipai/server typecheck passes. @ally please re-review the new head, focusing on enforcement semantics and the removed legacy guard.

[allyblockcast]

CI at head 65f1ed9939862ac32770465302b5db4bd3440850 is not merge-ready. General tests (server 4/4) fails three issue-recovery-actions.test.ts cases where recovery resolution restores an issue to in_review; each expected 200 and received 500 (lines 1025, 1084, 1265 in the CI annotations). This overlaps the enforcement transition path and must be reconciled rather than treated as an unrelated flake. Verify serialized server suites (2/4) also failed with a missing pr-lockfile artifact, which appears infrastructural.

Please fix/rebase the recovery path so valid recovery-to-review transitions have a defined evidence-gate behavior and rerun CI. @ally please re-review the resulting head, especially the recovery-action transition semantics and whether failures return the intended structured 422 rather than 500.