fix(schema): review-artifact rejects soft-fail runtime_metadata + add missing finding-resolution schema #72

New issue

Closed

opened 2026-06-08 18:07:21 +02:00 by claude · 0 comments

claude commented

2026-06-08 18:07:21 +02:00

Collaborator

Context

PR #71 (gemini, closes #62) wired jsonschema.validate() against real CLI outputs and immediately surfaced two real contract drifts. Per #62's discipline, #71 did NOT loosen the schemas — it documented the gaps via assertRaises(ValidationError). This issue tracks the actual fix.

Drift 1 — `review-artifact.schema.json` rejects soft-fail `runtime_metadata`

PR #54 (Swiss Cheese Layer 8) made review_run emit runtime_metadata with error_kind, error_detail, fell_back, model_used on the soft-fail path. But spec/schemas/review-artifact.schema.json predates #54 and rejects these fields (likely additionalProperties: false on runtime_metadata, or the property isn't modeled at all).

Effect: a legitimate soft-fail review artifact fails its own schema. The schema is stricter than the code it's supposed to describe.

Fix: extend review-artifact.schema.json to model runtime_metadata including the soft-fail fields (error_kind enum: transport|unparseable, error_detail string, fell_back bool, model_used string). Match the actual shape emitted by review_run.py + resolve_findings.py (SOFT_FAIL_KINDS).

Drift 2 — `finding-resolution.schema.json` is missing

resolve_findings() emits schema_version: "patchwarden.finding_resolution.v1" but there is no spec/schemas/finding-resolution.schema.json. #71 had to validate the registry schema in that slot instead.

Fix: add spec/schemas/finding-resolution.schema.json modelling the resolve_findings() output (verdict PASS/HOLD, findings[], blockers[] incl. the soft_fail_review_unreliable shape, target, registry_id/version). Add a spec/schemas/examples/finding-resolution.*.json example to match convention.

After the fix

Flip the two assertRaises(jsonschema.ValidationError) cases in tests/test_artifact_schema_contract.py to positive jsonschema.validate() calls.
Add a contract test validating a real soft-fail review artifact + a real resolve-findings verdict against their (now correct) schemas.

Constraints

stdlib-only in src/patchwarden/ (jsonschema is test-only, landed in #71's test extras).
This DOES modify spec/schemas/ (policy_governance class) — it's a deliberate contract fix, so route through normal operator review. NOT a schema_version bump (still .v1; we're making the schema match what .v1 already emits).
unittest, not pytest.

Spec sources

spec/schemas/review-artifact.schema.json
src/patchwarden/review_run.py (soft-fail runtime_metadata), src/patchwarden/resolve_findings.py (SOFT_FAIL_*)
tests/test_artifact_schema_contract.py (the assertRaises cases to flip)
PR #71 (where the drifts were found), PR #54 (which introduced soft-fail)

Filed 2026-06-08 by claude after supervising gemini's #62 PR. Good first issue for a cousin — clear, bounded, test-backed.

## Context PR #71 (gemini, closes #62) wired `jsonschema.validate()` against real CLI outputs and **immediately surfaced two real contract drifts**. Per #62's discipline, #71 did NOT loosen the schemas — it documented the gaps via `assertRaises(ValidationError)`. This issue tracks the actual fix. ## Drift 1 — `review-artifact.schema.json` rejects soft-fail `runtime_metadata` PR #54 (Swiss Cheese Layer 8) made `review_run` emit `runtime_metadata` with `error_kind`, `error_detail`, `fell_back`, `model_used` on the soft-fail path. But `spec/schemas/review-artifact.schema.json` predates #54 and rejects these fields (likely `additionalProperties: false` on `runtime_metadata`, or the property isn't modeled at all). **Effect:** a *legitimate* soft-fail review artifact fails its own schema. The schema is stricter than the code it's supposed to describe. **Fix:** extend `review-artifact.schema.json` to model `runtime_metadata` including the soft-fail fields (`error_kind` enum: `transport|unparseable`, `error_detail` string, `fell_back` bool, `model_used` string). Match the actual shape emitted by `review_run.py` + `resolve_findings.py` (`SOFT_FAIL_KINDS`). ## Drift 2 — `finding-resolution.schema.json` is missing `resolve_findings()` emits `schema_version: "patchwarden.finding_resolution.v1"` but there is **no** `spec/schemas/finding-resolution.schema.json`. #71 had to validate the registry schema in that slot instead. **Fix:** add `spec/schemas/finding-resolution.schema.json` modelling the `resolve_findings()` output (verdict PASS/HOLD, findings[], blockers[] incl. the `soft_fail_review_unreliable` shape, target, registry_id/version). Add a `spec/schemas/examples/finding-resolution.*.json` example to match convention. ## After the fix - Flip the two `assertRaises(jsonschema.ValidationError)` cases in `tests/test_artifact_schema_contract.py` to positive `jsonschema.validate()` calls. - Add a contract test validating a real soft-fail review artifact + a real resolve-findings verdict against their (now correct) schemas. ## Constraints - stdlib-only in `src/patchwarden/` (jsonschema is test-only, landed in #71's test extras). - This DOES modify `spec/schemas/` (policy_governance class) — it's a deliberate contract fix, so route through normal operator review. NOT a `schema_version` bump (still `.v1`; we're making the schema match what `.v1` already emits). - unittest, not pytest. ## Spec sources - `spec/schemas/review-artifact.schema.json` - `src/patchwarden/review_run.py` (soft-fail `runtime_metadata`), `src/patchwarden/resolve_findings.py` (`SOFT_FAIL_*`) - `tests/test_artifact_schema_contract.py` (the assertRaises cases to flip) - PR #71 (where the drifts were found), PR #54 (which introduced soft-fail) --- Filed 2026-06-08 by claude after supervising gemini's #62 PR. Good first issue for a cousin — clear, bounded, test-backed.

Rows
Columns