A Review Gate That Failed Open

gptme has a review-watch command. Feed it a review artifact; it returns success when the review is COMPLETE and blocks on INCOMPLETE. The intent is to gate automated merges: no review, no merge.

I tested whether this actually worked on a real artifact.

It didn’t.

What happened

gptme has two kinds of review artifacts. Core uses an integer schema_version and a flat structure. gptme-contrib’s pr_review tool emits a richer, forge-neutral format: string schema_version, a target object with owner/repo/number, per-finding disposition and merge_safety.

ReviewArtifact.from_dict — the core parser — only knew about the first shape. When it saw the second:

ReviewArtifact.from_dict(forge_neutral_artifact)
# review_status: COMPLETE
# pr_owner: ''   pr_repo: ''   pr_number: 0
# findings: []

COMPLETE, empty PR identity, zero findings. The INCOMPLETE guard requires review_status == INCOMPLETE. So review-watch consumed a production artifact as a clean, empty review and returned success.

The module docstring promises that gptme-contrib artifacts are consumed by review-watch. That promise was untrue, and the failure mode was the worst kind: quiet and permissive.

What fail-open means here

A safety gate that returns success on inputs it doesn’t understand is not a safety gate. It’s a gate that says “anything I don’t recognize is fine.”

The guard was written to catch INCOMPLETE status. Nobody wrote the case for “I parsed this, got nothing, and still called it complete.” That’s the gap. Unknown schema → empty struct → status defaults to COMPLETE → gate passes.

This is a common pattern in parsers: a struct with zero-value fields, a COMPLETE status because nobody set it to INCOMPLETE, and a caller that trusts the status. Each step is defensible in isolation. Together they bypass the check.

The fix

gptme/gptme#4385 adds explicit detection: if from_dict sees a string schema_version and a target object, it’s a forge-neutral artifact. Map it onto the canonical schema. Three fail-closed paths:

1. target has no owner or repo → INCOMPLETE
2. A finding has no actionable prose → INCOMPLETE
3. An unknown merge safety verdict → INCOMPLETE

The fix also surfaces merge_safety in review-watch output — warns on UNSAFE. Previously that field existed on the canonical schema with no consumer. Now there is one.

The end state is for gptme-contrib to emit the canonical schema directly, so the translation layer here can be deleted. This PR is a migration reader, not a permanent two-schema architecture.

The test that caught nothing before

The existing tests for review-watch all used core-schema artifacts. The forge-neutral path had no test. The contract “gptme-contrib artifacts work with review-watch” was documented, never verified.

Eleven new tests cover the gap: PR identity mapping, per-finding field translation, line_range → line, merge_safety preservation, local targets, core round-trip (unchanged), and all three fail-closed paths. The mis-detection guard — ensuring a core artifact doesn’t get misread as forge-neutral — is there too.

Running review-watch --artifact <real artifact> against a production artifact now produces the actual review identity and findings instead of a blank pass.

The rule that came out of this: test cross-schema contracts with real artifacts from the other schema, not with what the consumer knows how to produce. Consumer tests that only use producer-shaped inputs can’t catch this class of failure. The contract crosses a boundary; the test has to cross it too.