rbrooks/Circa

Watch

Fork 0

Notes are overwritten with no history and an empty audit record #66

New issue

Closed

opened 2026-07-28 05:57:13 +00:00 by claude-bot · 1 comment

claude-bot commented

2026-07-28 05:57:13 +00:00

Severity: MEDIUM

The bug

backend/app/services/projections.py:79-95 overwrites notes destructively and emits an audit
event carrying no values:

photo.notes_current = new_notes
photo_repo.increment_review_version(photo)
audit_repo.emit(
    event_type=AuditEventType.notes_updated,
    entity_type="photo", entity_id=photo.id,
    actor_id=actor_id, collection_id=photo.collection_id,
)   # no context=

Compare apply_decision, which correctly records full before/after state in context.

Why it matters

Notes are where a reviewer records what a relative told them — often the only surviving record
of who is in a photograph. A reviewer who clears the field, or a script that PATCHes notes to an
empty string across hundreds of photos, destroys that permanently. The audit log proves someone
changed notes but cannot say what was lost or restore it.

Contradicts the spec

docs/circa-spec.md §6.5: "All user-visible edits to dates, notes, duplicate resolutions,
group assignments, and metadata corrections must preserve previous values." Notes are the one
field with no history at all.

Fix

Minimum: include {"old": previous, "new": new_notes} in the audit context.

Better: add an append-only PhotoNoteRevision table mirroring how decisions are handled, and
surface note history in the review workspace alongside the decision history tab.

Done when

Note edits preserve the previous value in a queryable form
Audit events for notes carry before/after context
Note history is visible in the UI
A test asserts an overwrite does not lose the prior text

References

backend/app/services/projections.py:79-95
backend/app/api/routes/photos.py:203-222
docs/circa-spec.md §6.5

## Severity: MEDIUM ## The bug `backend/app/services/projections.py:79-95` overwrites notes destructively and emits an audit event carrying **no values**: ```python photo.notes_current = new_notes photo_repo.increment_review_version(photo) audit_repo.emit( event_type=AuditEventType.notes_updated, entity_type="photo", entity_id=photo.id, actor_id=actor_id, collection_id=photo.collection_id, ) # no context= ``` Compare `apply_decision`, which correctly records full before/after state in `context`. ## Why it matters Notes are where a reviewer records what a relative told them — often the only surviving record of who is in a photograph. A reviewer who clears the field, or a script that PATCHes notes to an empty string across hundreds of photos, destroys that permanently. The audit log proves someone changed notes but cannot say what was lost or restore it. ## Contradicts the spec `docs/circa-spec.md` §6.5: "All user-visible edits to dates, **notes**, duplicate resolutions, group assignments, and metadata corrections must preserve previous values." Notes are the one field with no history at all. ## Fix Minimum: include `{"old": previous, "new": new_notes}` in the audit `context`. Better: add an append-only `PhotoNoteRevision` table mirroring how decisions are handled, and surface note history in the review workspace alongside the decision history tab. ## Done when - [ ] Note edits preserve the previous value in a queryable form - [ ] Audit events for notes carry before/after context - [ ] Note history is visible in the UI - [ ] A test asserts an overwrite does not lose the prior text ## References - `backend/app/services/projections.py:79-95` - `backend/app/api/routes/photos.py:203-222` - `docs/circa-spec.md` §6.5

claude-bot added this to the v0.1.1 milestone

2026-07-28 05:57:13 +00:00

claude-bot added the

bug

backend

security

labels

2026-07-28 05:57:13 +00:00

rbrooks referenced this issue from a commit

2026-07-30 20:41:33 +00:00

Give notes a history instead of overwriting them

rbrooks closed this issue

2026-07-30 20:41:33 +00:00

claude-bot commented

2026-07-30 20:42:19 +00:00

Author

Done in c95fe79. Took the "better" option in the issue rather than the minimum — the append-only table and before/after in the audit context.

Revisions store the value the notes were set to, not the value replaced. That direction is what keeps notes_current derivable from history the way §6.5 requires of every projection; storing the outgoing value would leave the newest text with no row of its own and make the projection unrebuildable.

The old/new duplication into the audit event is deliberate, not redundancy I missed. audit_event is the table protected by #67's append-only triggers, so it is the copy that survives someone who can reach the database. photo_note_revision has no such protection — it is what the UI reads. There's a test that deletes every revision row and reconstructs the lost note from the ledger alone. The context also carries a cleared flag, because emptying the field is the destructive case and the one worth grepping the ledger for.

Migration 005 backfills a genesis revision for notes that predate the table. Without it the loss isn't prevented, only deferred by one edit — the first change after deployment would append the new text and leave what it replaced unrecorded, which is exactly this bug. Those rows are honest about being reconstructed: created_by is NULL (renders as "System" rather than falsely attributing to someone) and created_at uses photo.updated_at, an upper bound rather than the moment the notes were written — the only signal the old schema preserved. apply_notes_updated is the sole writer of notes_current (ingest doesn't touch it), so after the backfill the column is fully accounted for. Tested across the 004→005 boundary, since the normal harness builds at head and can't reach it.

A save that changes nothing now writes nothing and no longer bumps review_version, following apply_flags_updated. A spurious bump invalidates other reviewers' in-flight edits for no reason.

UI: GET /api/photos/{id}/notes/revisions (readable by anyone who may read the photo — viewers still get 404 on unapproved ones), surfaced behind a History toggle in the Notes panel. Cleared revisions render as "Notes cleared" rather than blank.

Unrelated live bug found while wiring the UI, fixed here: #57/#64 removed raw created_by ids from API responses, but ReviewWorkspacePage still called c.created_by.slice(0, 8) — a TypeError on undefined as soon as a photo had a comment, i.e. the Comments tab was crashing on main. tsc couldn't catch it because the TS interfaces still declared the removed field. Interfaces now extend a shared Attribution type. This is a concrete argument for #76 (generate TS types from OpenAPI); flagging it there.

All four "done when" items covered, 30 tests in backend/tests/test_note_history.py.

Not done: photo_note_revision has no append-only triggers of its own. date_decision and date_evidence don't either — the ledger is deliberately the one special-cased table, and widening that is a scope call rather than something to slip in here. Say if you'd like it and I'll open an issue.

Done in c95fe79. Took the "better" option in the issue rather than the minimum — the append-only table *and* before/after in the audit context. **Revisions store the value the notes were set _to_, not the value replaced.** That direction is what keeps `notes_current` derivable from history the way §6.5 requires of every projection; storing the outgoing value would leave the newest text with no row of its own and make the projection unrebuildable. **The old/new duplication into the audit event is deliberate, not redundancy I missed.** `audit_event` is the table protected by #67's append-only triggers, so it is the copy that survives someone who can reach the database. `photo_note_revision` has no such protection — it is what the UI reads. There's a test that deletes every revision row and reconstructs the lost note from the ledger alone. The context also carries a `cleared` flag, because emptying the field is the destructive case and the one worth grepping the ledger for. **Migration `005` backfills a genesis revision for notes that predate the table.** Without it the loss isn't prevented, only deferred by one edit — the first change after deployment would append the new text and leave what it replaced unrecorded, which is exactly this bug. Those rows are honest about being reconstructed: `created_by` is NULL (renders as "System" rather than falsely attributing to someone) and `created_at` uses `photo.updated_at`, an upper bound rather than the moment the notes were written — the only signal the old schema preserved. `apply_notes_updated` is the sole writer of `notes_current` (ingest doesn't touch it), so after the backfill the column is fully accounted for. Tested across the 004→005 boundary, since the normal harness builds at head and can't reach it. **A save that changes nothing now writes nothing** and no longer bumps `review_version`, following `apply_flags_updated`. A spurious bump invalidates other reviewers' in-flight edits for no reason. UI: `GET /api/photos/{id}/notes/revisions` (readable by anyone who may read the photo — viewers still get 404 on unapproved ones), surfaced behind a **History** toggle in the Notes panel. Cleared revisions render as *"Notes cleared"* rather than blank. **Unrelated live bug found while wiring the UI, fixed here:** #57/#64 removed raw `created_by` ids from API responses, but `ReviewWorkspacePage` still called `c.created_by.slice(0, 8)` — a `TypeError` on `undefined` as soon as a photo had a comment, i.e. the Comments tab was crashing on `main`. `tsc` couldn't catch it because the TS interfaces still declared the removed field. Interfaces now extend a shared `Attribution` type. This is a concrete argument for #76 (generate TS types from OpenAPI); flagging it there. All four "done when" items covered, 30 tests in `backend/tests/test_note_history.py`. **Not done:** `photo_note_revision` has no append-only triggers of its own. `date_decision` and `date_evidence` don't either — the ledger is deliberately the one special-cased table, and widening that is a scope call rather than something to slip in here. Say if you'd like it and I'll open an issue.

claude-bot referenced this issue

2026-08-01 06:45:08 +00:00

Failed evidence fetch renders as "No evidence yet." #78

claude-bot referenced this issue

2026-08-03 15:51:00 +00:00

Add an end-to-end reviewer flow test #12