feat(v0): resolve_findings detects review_run soft-fail (Swiss Cheese Layer 8) #54

Merged

pdurlej merged 1 commit from claude/patchwarden-resolver-soft-fail-detection into main

2026-05-27 14:32:45 +02:00

claude commented

2026-05-27 14:31:01 +02:00

Collaborator

What

Closes Swiss Cheese Layer 8 hand-off gap (Scenariusz B z docs/operations/code-vs-vision-snapshot-2026-05-27.md).

When a reviewer lane runs with fail_on_missing=False or fail_on_unparseable=False, review_run records the failure in runtime_metadata.error_kind ("transport" or "unparseable") and emits empty findings. Before this PR, resolve_findings ignored that field — so a soft-failed Ollama call looked indistinguishable from a clean review. Operator could merge a "green" PR that was never actually reviewed.

This is PR A of 3 defense-hardening PRs spun off the 2026-05-27 crosscheck.

Code changes

src/patchwarden/resolve_findings.py (+22 lines):

New constants SOFT_FAIL_KINDS = {"transport", "unparseable"} + SOFT_FAIL_BLOCKER_CODE = "soft_fail_review_unreliable"
After processing each artifact's findings, check runtime_metadata.error_kind. If matches → append artifact-bound blocker:

{
    "code": "soft_fail_review_unreliable",
    "finding_id": None,
    "finding_type_id": None,
    "severity": "blocker",
    "state": "manual_only",
    "reviewer_id": "<artifact's reviewer.agent_name>",
    "error_kind": "transport" | "unparseable",
    "error_detail": "<truncated to 200 chars>",
}

Sort key updated for finding_id=None: (item.get("finding_id") or "", item["code"])

Test changes — 138 → 144 green (+6)

tests/test_resolve_findings.py:

New helper soft_fail_artifact(error_kind, ...) mirrors what review_run actually emits when lane has fail_on_*=False.
New SoftFailDetectionTests class:

Test	Asserts
`test_transport_error_kind_holds`	HOLD verdict + blocker recorded with all fields
`test_unparseable_error_kind_holds`	Same for unparseable, error_detail preserved
`test_error_detail_is_truncated_to_200_chars`	Bound on noisy error messages
`test_findings_and_soft_fail_produce_both_blockers`	Combo: real finding blocker + soft-fail blocker on same PR
`test_clean_artifact_with_runtime_metadata_does_not_add_soft_fail_blocker`	Regression: runtime_metadata present without error_kind still PASS
`test_unknown_error_kind_value_does_not_trigger_blocker`	Only transport/unparseable trigger; future kinds need explicit opt-in to SOFT_FAIL_KINDS

All 4 existing ResolveFindingsTests still pass (no regression on non-soft-fail paths).

Why this matters (D20)

D20 says LLM reviewers suspect, Patchwarden decides. This PR makes the "Patchwarden decides" half honest: when the LLM couldn't suspect anything (because Ollama was down), Patchwarden must decide HOLD, not PASS by default. Otherwise the policy beats LLM vibes principle (P2) silently degrades into "policy trusts empty answers" — opposite of what fail-closed defaults mean.

Schema impact

patchwarden.finding_resolution.v1 blockers list now contains two shapes:

Shape	Fields	When
finding-bound (existing)	code + finding_id + finding_type_id + severity + state	Per finding from artifact
artifact-bound (new)	code + finding_id=null + reviewer_id + error_kind + error_detail	Per soft-failed artifact

Backwards-compatible for consumers that read unknown fields tolerantly. No schema_version bump.

Swiss Cheese context

PR	Layer	Status
THIS PR A	Layer 8 hand-off (Scenariusz B)	⏳ awaiting merge
PR B (next)	Layer 6 architectural lint (Scenariusz C)	queued
PR C (last)	Layer 1 secret-by-filename doc (Scenariusz A)	queued

Atomic per ADR-0017

2 files: resolve_findings.py (+22), test_resolve_findings.py (+125)
No new module, no new dependency, stdlib-only
base=main, no stacking

NOT breaking M2 gate

This is defense hardening of existing v0, not a new feature. Per docs/decisions.md D16 + M2 milestone notes, hardening existing capability is in-scope; adding new capability is parked. PR A closes a real gap in the existing review_run ↔ resolve_findings contract.

Token-accounting

~3-4% weekly Opus (sacred path edit + 6 new tests + recon of resolver helpers).

## What Closes Swiss Cheese **Layer 8** hand-off gap (Scenariusz B z `docs/operations/code-vs-vision-snapshot-2026-05-27.md`). When a reviewer lane runs with `fail_on_missing=False` or `fail_on_unparseable=False`, `review_run` records the failure in `runtime_metadata.error_kind` (`"transport"` or `"unparseable"`) and emits empty findings. **Before this PR, `resolve_findings` ignored that field** — so a soft-failed Ollama call looked indistinguishable from a clean review. Operator could merge a "green" PR that was never actually reviewed. This is **PR A of 3 defense-hardening PRs** spun off the 2026-05-27 crosscheck. ## Code changes `src/patchwarden/resolve_findings.py` (+22 lines): - New constants `SOFT_FAIL_KINDS = {"transport", "unparseable"}` + `SOFT_FAIL_BLOCKER_CODE = "soft_fail_review_unreliable"` - After processing each artifact's findings, check `runtime_metadata.error_kind`. If matches → append artifact-bound blocker: ```python { "code": "soft_fail_review_unreliable", "finding_id": None, "finding_type_id": None, "severity": "blocker", "state": "manual_only", "reviewer_id": "<artifact's reviewer.agent_name>", "error_kind": "transport" | "unparseable", "error_detail": "<truncated to 200 chars>", } ``` - Sort key updated for `finding_id=None`: `(item.get("finding_id") or "", item["code"])` ## Test changes — 138 → 144 green (+6) `tests/test_resolve_findings.py`: - New helper `soft_fail_artifact(error_kind, ...)` mirrors what `review_run` actually emits when lane has `fail_on_*=False`. - New `SoftFailDetectionTests` class: | Test | Asserts | |---|---| | `test_transport_error_kind_holds` | HOLD verdict + blocker recorded with all fields | | `test_unparseable_error_kind_holds` | Same for unparseable, error_detail preserved | | `test_error_detail_is_truncated_to_200_chars` | Bound on noisy error messages | | `test_findings_and_soft_fail_produce_both_blockers` | Combo: real finding blocker + soft-fail blocker on same PR | | `test_clean_artifact_with_runtime_metadata_does_not_add_soft_fail_blocker` | **Regression**: runtime_metadata present without error_kind still PASS | | `test_unknown_error_kind_value_does_not_trigger_blocker` | Only transport/unparseable trigger; future kinds need explicit opt-in to SOFT_FAIL_KINDS | All 4 existing `ResolveFindingsTests` still pass (no regression on non-soft-fail paths). ## Why this matters (D20) D20 says **LLM reviewers suspect, Patchwarden decides**. This PR makes the "Patchwarden decides" half honest: when the LLM **couldn't** suspect anything (because Ollama was down), Patchwarden must decide HOLD, not PASS by default. Otherwise the **policy beats LLM vibes** principle (P2) silently degrades into "policy trusts empty answers" — opposite of what fail-closed defaults mean. ## Schema impact `patchwarden.finding_resolution.v1` `blockers` list now contains **two shapes**: | Shape | Fields | When | |---|---|---| | finding-bound (existing) | code + finding_id + finding_type_id + severity + state | Per finding from artifact | | artifact-bound (new) | code + finding_id=null + reviewer_id + error_kind + error_detail | Per soft-failed artifact | **Backwards-compatible** for consumers that read unknown fields tolerantly. No `schema_version` bump. ## Swiss Cheese context | PR | Layer | Status | |---|---|---| | **THIS PR A** | Layer 8 hand-off (Scenariusz B) | ⏳ awaiting merge | | PR B (next) | Layer 6 architectural lint (Scenariusz C) | queued | | PR C (last) | Layer 1 secret-by-filename doc (Scenariusz A) | queued | ## Atomic per ADR-0017 - 2 files: `resolve_findings.py` (+22), `test_resolve_findings.py` (+125) - No new module, no new dependency, stdlib-only - `base=main`, no stacking ## NOT breaking M2 gate This is **defense hardening of existing v0**, not a new feature. Per [`docs/decisions.md`](../docs/decisions.md) D16 + M2 milestone notes, hardening existing capability is in-scope; adding new capability is parked. PR A closes a real gap in the **existing** review_run ↔ resolve_findings contract. ## Token-accounting ~3-4% weekly Opus (sacred path edit + 6 new tests + recon of resolver helpers).

claude added 1 commit

2026-05-27 14:31:01 +02:00

feat(v0): resolve_findings detects review_run soft-fail (Swiss Cheese Layer 8) d69b8febc1

When a reviewer lane runs with fail_on_missing=False or
fail_on_unparseable=False, review_run records the failure in
runtime_metadata.error_kind ("transport" or "unparseable") and emits
empty findings. Before this PR, resolve_findings ignored that field —
so a soft-failed Ollama call looked indistinguishable from a clean
review, and a downstream operator could merge a "green" PR that was
never actually reviewed.

This closes Swiss Cheese Layer 8 hand-off gap (Scenariusz B in the
2026-05-27 vision-vs-code crosscheck).

## What changes

`src/patchwarden/resolve_findings.py`:
- New constants `SOFT_FAIL_KINDS = {"transport", "unparseable"}` and
  `SOFT_FAIL_BLOCKER_CODE = "soft_fail_review_unreliable"`.
- After processing each artifact's findings, check `artifact["runtime_metadata"]["error_kind"]`.
  If it matches SOFT_FAIL_KINDS, append a non-finding-bound blocker:

  ```python
  {
      "code": "soft_fail_review_unreliable",
      "finding_id": None,
      "finding_type_id": None,
      "severity": "blocker",
      "state": "manual_only",
      "reviewer_id": "<artifact's reviewer.agent_name>",
      "error_kind": "transport" | "unparseable",
      "error_detail": "<truncated to 200 chars>",
  }
  ```
- Sort key for blockers updated to handle `finding_id=None`:
  `(item.get("finding_id") or "", item["code"])`.

`tests/test_resolve_findings.py`:
- New helper `soft_fail_artifact(error_kind, ...)` mirrors what
  `review_run` actually emits.
- New `SoftFailDetectionTests` class (6 tests):
  - `test_transport_error_kind_holds` — HOLD verdict + blocker recorded
  - `test_unparseable_error_kind_holds` — same for unparseable
  - `test_error_detail_is_truncated_to_200_chars` — bound on noisy errors
  - `test_findings_and_soft_fail_produce_both_blockers` — combo: real
    finding blocker + soft-fail blocker on the same PR
  - `test_clean_artifact_with_runtime_metadata_does_not_add_soft_fail_blocker`
    — regression: runtime_metadata present without error_kind is still PASS
  - `test_unknown_error_kind_value_does_not_trigger_blocker` — only
    transport/unparseable trigger; future kinds need explicit opt-in

All 4 existing ResolveFindingsTests still pass (no regression on
non-soft-fail paths).

## Why this matters

D20 says LLM reviewers suspect, Patchwarden decides. This PR makes the
"Patchwarden decides" half honest: when the LLM **couldn't** suspect
anything (because Ollama was down), Patchwarden must decide HOLD, not
PASS by default. Otherwise the "policy beats LLM vibes" principle (P2)
silently degrades into "policy trusts empty answers" — the opposite of
what fail-closed defaults mean.

## Schema impact

`patchwarden.finding_resolution.v1` blockers list now contains two
shapes:
- finding-bound (existing): code + finding_id + finding_type_id +
  severity + state
- artifact-bound (new): code + finding_id=null + reviewer_id +
  error_kind + error_detail (severity always "blocker", state always
  "manual_only")

Backwards-compatible for consumers that read unknown fields tolerantly.
No schema_version bump.

## Tests

138 → 144 green (+6 new, all pass on first run).

## Atomic per ADR-0017

- 2 files: resolve_findings.py (+22 lines), test_resolve_findings.py
  (+125 lines).
- No new module, no new dependency, stdlib-only.
- base=main, no stacking on prior PRs.

## Swiss Cheese context

This is one of three defense-hardening PRs spun off the 2026-05-27
crosscheck (PR A of A/B/C). The crosscheck identified Layer 8 as the
highest-priority gap — most likely to silently produce a green merge
on a broken Patchwarden run. PR B (D20 architectural lint test) and
PR C (secret-by-filename limitation doc) follow.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>