supersede_evidence never sets superseded_by

claude-bot commented

2026-07-28 06:00:27 +00:00

Severity: MEDIUM

The bug

POST /api/evidence/{id}/supersede (backend/app/api/routes/photos.py:333-355) sets
is_active = False and stops:

ev = evidence_repo.get_by_id(evidence_id)
if ev is None: raise HTTPException(404, ...)
ev.is_active = False

It never sets superseded_by. That column is written nowhere in the codebase — the evidence
chain the schema was explicitly designed around is never linked.

EvidenceRepository.supersede, which does set it correctly, is dead code with no call sites.

The route also emits no projection update and no audit context, and performs no
review_version check.

Why it matters

DateEvidence.superseded_by exists so that retiring evidence preserves why and by what. The
constraint explanation UI (#20) and the provenance chain depend on this being populated. Right
now superseding silently breaks the chain, and there is no rebuild path to detect it.

Fix

Route the operation through EvidenceRepository.supersede so superseded_by is set.
Require and check review_version like every sibling write.
Emit an audit event with before/after context.
Update the photo projection if the superseded evidence affected it.

Collection scoping for this route is covered separately in the security milestone.

Done when

superseded_by is populated on every supersede
The dead repository method is used, or removed
Supersede participates in conflict checking and audit
A test asserts the chain links correctly and the original row survives

References

backend/app/api/routes/photos.py:333-355
backend/app/repositories/evidence.py (supersede, unused)
backend/app/models/models.py (DateEvidence.superseded_by)

## Severity: MEDIUM ## The bug `POST /api/evidence/{id}/supersede` (`backend/app/api/routes/photos.py:333-355`) sets `is_active = False` and stops: ```python ev = evidence_repo.get_by_id(evidence_id) if ev is None: raise HTTPException(404, ...) ev.is_active = False ``` It never sets `superseded_by`. That column is written **nowhere in the codebase** — the evidence chain the schema was explicitly designed around is never linked. `EvidenceRepository.supersede`, which does set it correctly, is dead code with no call sites. The route also emits no projection update and no audit context, and performs no `review_version` check. ## Why it matters `DateEvidence.superseded_by` exists so that retiring evidence preserves *why* and *by what*. The constraint explanation UI (#20) and the provenance chain depend on this being populated. Right now superseding silently breaks the chain, and there is no rebuild path to detect it. ## Fix - Route the operation through `EvidenceRepository.supersede` so `superseded_by` is set. - Require and check `review_version` like every sibling write. - Emit an audit event with before/after context. - Update the photo projection if the superseded evidence affected it. Collection scoping for this route is covered separately in the security milestone. ## Done when - [ ] `superseded_by` is populated on every supersede - [ ] The dead repository method is used, or removed - [ ] Supersede participates in conflict checking and audit - [ ] A test asserts the chain links correctly and the original row survives ## References - `backend/app/api/routes/photos.py:333-355` - `backend/app/repositories/evidence.py` (`supersede`, unused) - `backend/app/models/models.py` (`DateEvidence.superseded_by`)

claude-bot added this to the v0.2.0 milestone

2026-07-28 06:00:27 +00:00

claude-bot added the

bug

backend

labels

2026-07-28 06:00:27 +00:00

claude-bot referenced this issue

2026-07-28 06:07:51 +00:00

Add optimistic-concurrency (409 review_version_conflict) tests #9

claude-bot referenced this issue

2026-07-28 06:07:54 +00:00

Add repository and projection-service tests #6

rbrooks referenced this issue from a commit

2026-08-01 05:31:06 +00:00

Link the evidence supersession chain and fix reruns

rbrooks closed this issue

2026-08-01 05:31:06 +00:00

claude-bot referenced this issue

2026-08-01 05:52:37 +00:00

review_version guard is check-then-write and races #77

claude-bot commented

2026-08-01 05:53:39 +00:00

Author

Done in a239b8e. All four "done when" items covered.

Supersede now goes through EvidenceRepository.supersede — the method that was written with the schema, is correct, and had no call sites at all. The route set is_active = False by hand, so superseded_by was written nowhere in the codebase and the chain was empty everywhere.

One design call worth your eye: the successor is optional. The issue says "superseded_by is populated on every supersede", which only holds if every supersede has a replacement. I don't think it does. Retiring and replacing are different acts — a reviewer who can see an OCR reading is nonsense has nothing to put in its place, and requiring one would put a fiction in the chain. NULL means "retired, nothing replaced it", which is a fact worth recording rather than a broken link. Push back if you'd rather force a successor.

The shape that actually produces chains is the other half: POST /photos/{id}/evidence/manual now accepts supersedes, so "that reading was wrong, here is the correct one" is a single transaction. Splitting it into two requests leaves a window where the photo has neither piece of evidence or both. That's also the shape the worker will use when a rerun produces better evidence than the run before it.

Guards, all tested: a successor on another photo (400 — linking across photos corrupts the chain rather than recording it), a self-reference (400), a double-supersede (409), and a bad supersedes id leaving no orphan replacement behind.

Audit context carries old/new summaries and the successor id.

On "update the photo projection if the superseded evidence affected it": there is nothing to do there today. The current_effective_date_* fields are projected from decisions, not evidence — evidence feeds a reviewer's judgement, not the projection directly. What supersede does now is bump review_version (added in #70), which is the mechanism that actually tells a reviewer mid-decision that the ground moved. If constraint propagation (#19) later derives projections from evidence, this becomes real and should be revisited.

Note that #79 lists this route as one of the two places that bypass the projection path entirely; that half is addressed there, where the fold is defined in one place.

The conflict check and collection scoping landed in #70, as you noted they would.

Done in a239b8e. All four "done when" items covered. Supersede now goes through `EvidenceRepository.supersede` — the method that was written with the schema, is correct, and had **no call sites at all**. The route set `is_active = False` by hand, so `superseded_by` was written nowhere in the codebase and the chain was empty everywhere. **One design call worth your eye: the successor is optional.** The issue says "`superseded_by` is populated on every supersede", which only holds if every supersede has a replacement. I don't think it does. Retiring and replacing are different acts — a reviewer who can see an OCR reading is nonsense has nothing to put in its place, and requiring one would put a fiction in the chain. `NULL` means "retired, nothing replaced it", which is a fact worth recording rather than a broken link. Push back if you'd rather force a successor. **The shape that actually produces chains is the other half:** `POST /photos/{id}/evidence/manual` now accepts `supersedes`, so "that reading was wrong, here is the correct one" is a single transaction. Splitting it into two requests leaves a window where the photo has neither piece of evidence or both. That's also the shape the worker will use when a rerun produces better evidence than the run before it. Guards, all tested: a successor on another photo (400 — linking across photos corrupts the chain rather than recording it), a self-reference (400), a double-supersede (409), and a bad `supersedes` id leaving no orphan replacement behind. Audit context carries `old`/`new` summaries and the successor id. **On "update the photo projection if the superseded evidence affected it":** there is nothing to do there today. The `current_effective_date_*` fields are projected from *decisions*, not evidence — evidence feeds a reviewer's judgement, not the projection directly. What supersede does now is bump `review_version` (added in #70), which is the mechanism that actually tells a reviewer mid-decision that the ground moved. If constraint propagation (#19) later derives projections from evidence, this becomes real and should be revisited. Note that #79 lists this route as one of the two places that bypass the projection path entirely; that half is addressed there, where the fold is defined in one place. The conflict check and collection scoping landed in #70, as you noted they would.

rbrooks referenced this issue from a commit

2026-08-02 07:02:09 +00:00

Check the concurrency protocol across the whole write surface

claude-bot referenced this issue

2026-08-02 07:17:21 +00:00

Add optimistic-concurrency (409 review_version_conflict) tests #9

claude-bot referenced this issue

2026-08-04 05:09:24 +00:00

Superseded evidence is distinguished by opacity alone #138

claude-bot referenced this issue

2026-08-04 15:13:17 +00:00

Missing chrome: no logout, no upload UI, manual evidence unreachable #101