fix: tolerate a non-dict evidence_json on findings read (no more 500)

[branover]

What

A finding's evidence_json is a JSON column, so a well-formed row deserializes to a dict. But a legacy / hand-edited / double-encoded row can read back as a JSON string, and the many (evidence or {}).get(...) read sites then raise AttributeError: 'str' object has no attribute 'get'. The effect is a hard 500: a single malformed finding takes down the entire GET /api/projects/{id} listing (via is_verified inside finding_dict), and the MCP list_findings / get_finding reads carry the identical latent crash.

File ".../engine/findings/findings.py", line 32, in is_verified
    return bool((((evidence or {}).get("extra") or {}).get("verification") or {}).get("verified"))
AttributeError: 'str' object has no attribute 'get'

Why it happens

No current write path produces a string — every persist goes through the Pydantic Finding/Evidence models and model_dump(), which reject a non-object evidence. So the bad row predates this code (older HexGraph version or a manual edit) in a long-lived DB. The defect being fixed is not the write but the read fragility: one malformed row should never 500 a whole listing.

The fix

Add coerce_evidence() (one source of truth in engine/findings/findings.py):

• a dict passes through unchanged (the normal path — zero behavior change),
• a JSON string is best-effort parsed and, if it decodes to an object, recovered (so a double-encoded dict keeps its data, including a real verified flag),
• anything else (garbage string, list, scalar, None) degrades to {}.

Route the read surfaces through it: is_verified, row_to_payload (so the API/CLI/export evidence is always an object), and the MCP list_findings / get_finding readers.

No schema change — this is defensive read-path code only — so no migration is required. No UI behavior changes (the SPA now receives a well-formed object instead of an occasional 500), so no ux-contract.md entry.

Verification

• New tests/test_evidence_coerce.py — coerce_evidence / is_verified against dict / double-encoded-dict / garbage-string / non-object-JSON / None.
• New test_project_payload_tolerates_string_evidence_json in tests/test_hidden_targets.py — reproduces the exact crash end-to-end: a finding whose evidence_json round-trips to a string now returns 200, not 500; a double-encoded dict is recovered (verified=True), a garbage string degrades to {} / verified=False.
• Full just test fast tier green: 1407 passed, 4 skipped, 14 deselected. The target→task→finding→graph loop is untouched (pure read coercion), so just demo was not required.

🤖 Generated with Claude Code

[branover]

Merge-gate review — PR #250 fix: tolerate a non-dict evidence_json on findings read

Independent reviewer (did not author this code). Ran /code-review high --comment and /security-review on git diff main...HEAD.

Verdict: APPROVE

The change correctly and minimally fixes the reported 500. No blocking issues. One non-blocking completeness note (below), posted inline.

---

What I verified

• Root-cause fix is correct. coerce_evidence() handles every input shape: dict → passthrough (identity-preserving), JSON-encoded dict string → recovered, unparseable string / JSON non-object (list, scalar) / None / int → {}. The except (ValueError, TypeError) covers JSONDecodeError. Behavior is backward-compatible with the prior evidence or {} (empty string and empty dict still collapse to {}).
• All crashing read sites on the reported path are covered. GET /api/projects/{id} builds findings via _shared.finding_dict → is_verified(f.evidence_json) (now coerced) + row_to_payload(f) (now coerced). Both are routed through coerce_evidence. The MCP list_findings / get_finding readers are likewise routed.
• Test quality is high — the regression genuinely reproduces the original 500. I temporarily reverted coerce_evidence to a pre-fix passthrough; tests/test_hidden_targets.py::test_project_payload_tolerates_string_evidence_json then fails with the exact AttributeError: 'str' object has no attribute 'get', and the unit tests fail too. With the fix in place, all 17 tests pass. The regression asserts both buckets: a garbage-string finding coerces to {} / verified=False, and a double-encoded dict is recovered with its verification intact.
• No migration needed — no schema change (the column type and model are untouched; this is a read-time coercion only).
• No ux-contract change needed — no UI behavior change; the endpoint returns the same shape, just without 500ing on a bad row.
• Security invariants intact. No new deserialization risk (json.loads, not pickle/yaml; result type-checked). No change to the loopback bind, sandbox boundary, target-byte handling, secret handling, or the opt-in execution/egress policy seam. Net effect reduces attack surface (closes an unhandled-exception path). No HIGH/MEDIUM security findings.

Non-blocking finding (code-review, altitude) — posted inline

• src/hexgraph/engine/findings/findings.py — the fix is applied at three call sites rather than at the column-read boundary, so the same malformed string-evidence_json row still crashes ~15 other read/mutation sites that do dict(f.evidence_json or {}) or (f.evidence_json or {}).get(...) directly (e.g. api/routers/findings.py:114 POST verify, engine/fuzz/campaigns.py:801/851/872, engine/build/source.py:329, engine/re/solving.py:245, engine/findings/reachability.py:394). Note dict('somestring') raises ValueError, so the dict(... or {}) sites are not safe either. Most are write-time task paths (far lower exposure than a browse listing), so this is legitimately out of scope for the stated fix and not a merge blocker — but worth a follow-up to route all reads through coerce_evidence() (or coerce once at the column boundary) so one bad row can't resurface on a different endpoint.

Tests

pytest tests/test_evidence_coerce.py tests/test_hidden_targets.py → 17 passed (in the worktree's own venv + isolated HEXGRAPH_HOME). Full fast tier reported green (1407 passed).

Ready to merge

Yes, pending CI green and the standard squash-merge. The inline note is a non-blocking follow-up suggestion, not a change request.

Verdict is in this comment body; per repo policy this is a --comment review, never an --approve/--request-changes state on an own PR.