Skip to content

Gate D audit — durable, attributable report feedback

The runnable artifact for OGUR-60, the MVP-1 feedback gate. Re-run it with:

uv run --extra dev python -m pytest tests/unit/api/test_gate_d_audit.py -v

It lives at tests/unit/api/test_gate_d_audit.py, so make test and CI run it on every push with no extra wiring. Every test name is a finding; a green run is the verdict.

Why an audit rather than a build

Gate D's behaviour was built by three earlier issues and never verified in one place:

Acceptance bullet Built by Discharged here by
One report-level box; per-section OUT OGUR-71 test_a_report_level_write_needs_only_text
Persist author, report, document identity, text, timestamp OGUR-69 test_every_persisted_field_comes_from_the_session_not_the_body, test_a_document_scoped_write_persists_the_stable_identity
Anchor to the source-record layer, never Signal.id OGUR-69 test_feedback_carries_no_signal_identity
Reject unauthenticated writes and another client's report OGUR-64 / OGUR-65 test_anonymous_writes_and_reads_are_refused, test_an_ungranted_served_report_is_indistinguishable_from_an_unserved_one
Survives process restart OGUR-67 / OGUR-68 test_feedback_survives_a_process_restart_through_the_http_boundary
seed → feedback → reseed still resolves OGUR-69 test_feedback_still_resolves_after_a_reseed_renumbers_signals
Writable volume backed up daily OGUR-68 test_the_snapshotted_volume_is_the_one_the_database_lives_on

The evidence sat at three different altitudes — store unit tests, API unit tests, and TOML configuration nothing asserted — and two bullets were not actually proved. Both gaps are closed below.

The two bullets that were not proved

Restart durability had never crossed the API. The suite's default database is in-memory SQLite on a StaticPool; it cannot demonstrate persistence, because it dies with the process by construction. tests/unit/store/test_feedback.py::test_feedback_survives_application_restart works around that with a real file, but only at the store layer. The deployed claim is about the API, so the audit's version drives the same ASGI app over a file-backed SQLite, disposes the engine, rebuilds it against the same file, and reads the row back through GET. It carries the session cookie across the restart rather than logging in again: the claim a client cares about is that a redeploy does not sign them out and does not lose what they wrote, and re-authenticating would prove only the second half.

The cross-client refusal was proved to be a 404, not to be the same 404. tests/unit/api/test_authorization.py:71 asserts the status. A 404 whose body, shape, or wording differs between "this report is not yours" and "this report does not exist" is a directory of the other client's landscapes. ogur/api/deps.py:39-42 collapses the two on purpose; the audit compares the two responses with the caller-supplied id masked, and confirms the refused write left no row behind. This is only reachable because two packs are served — with one, the ungranted case has nothing to compare against.

The anchor rule is asserted structurally, not behaviourally

test_feedback_carries_no_signal_identity reads Feedback.__table__ and pins the exact column set. Every other feedback test asserts a behaviour that follows from the no-Signal.id rule, and a re-added signal_id column would disturb none of them: resolution by (document_source, document_source_id) keeps working while a second, reseed-fragile anchor rides alongside it, until the first MVP-2 rebuild orphans it. Adding that column back is one of the falsifications below, and this is the only case that catches it.

The landscape_id that Feedback and Signal share is deliberately not treated as a borrowed key — it is the report scope, it survives a reseed, and it is what scopes resolution rather than what performs it. That scoping has its own case (test_a_reseed_of_another_landscape_does_not_capture_the_feedback): the same NCT is routinely projected into more than one landscape, and without the predicate feedback would silently re-attach to whichever copy the query returned first.

Backups: what the green run does and does not claim

"Backed up daily" has no behaviour to exercise — configuration is the whole artifact — so it is asserted as configuration, across the two files that own the two halves:

  • Dockerfile decides where SQLite writes (DATABASE_URL=sqlite:////data/ogur.db);
  • fly.toml decides what Fly snapshots ([mounts] destination = "/data", snapshot_retention = 14).

The audit parses both and asserts they name the same directory. The failure mode worth catching is not an absent snapshot policy — someone would notice — but a daily snapshot of a volume the database moved off, which passes every review and restores nothing. A companion case pins that docker/entrypoint.sh still refuses to overwrite an existing /data/ogur.db, since a redeploy that re-materialized it from the baked content DB would discard every feedback row while every backup check still passed.

Stated plainly: the daily layer is Fly's automatic volume snapshot. The offsite copy is not daily. scripts/ops/backup_db.sh (make fly-backup) is manual, and docs/deployment.md documents it as "at least weekly and before deploys". The ticket's bullet is satisfied literally; a daily off-Fly copy is not in place. It is named here rather than left to be inferred from a passing test.

The falsifications

A gate nobody has watched fail is not known to work. Each asserted property was removed from main's code and the audit re-run; each broke exactly the case that claims it.

Property removed Failing case
the grant check in require_report_access test_an_ungranted_served_report_is_indistinguishable_from_an_unserved_one
snapshot_retention from fly.toml test_the_snapshotted_volume_is_the_one_the_database_lives_on
DATABASE_URL repointed off /data test_the_snapshotted_volume_is_the_one_the_database_lives_on
a signal_id column re-added to Feedback test_feedback_carries_no_signal_identity
the existing-database guard in docker/entrypoint.sh test_the_writable_volume_is_never_overwritten_on_boot
session.commit() in add_feedback 5 cases, incl. the restart and reseed pair
user_id no longer session-derived test_every_persisted_field_comes_from_the_session_not_the_body

The pack roster is pinned, not inherited

The audit sets EXPLORE_REPORT_PACK_PATH itself, for the same reason the Gate C audit does (gate-c-audit.md). Without it the verdict is a property of the developer's .env: an in-progress pack from an unmerged worktree does not merely add a report — a layer whose evidence_depth has no sentence raises while the shared index is being built, so every pack 503s and this module fails for reasons unrelated to feedback.

Grants are exercised, not waived. grant_all_landscapes() is the right tool for suites whose subject is the pack layer; here the grant check is part of the subject, so the audit writes real UserLandscapeAccess rows.

Deliberately not audited

  • The feedback box's UI states. Pinned by 16 cases in frontend/src/components/explore/__tests__/ReportFeedback.test.tsx, including the unresolved-write states (a 5xx can arrive after the row committed, so the client names the ambiguity instead of guessing). A pytest artifact cannot assert React state; re-deriving it here would duplicate rather than verify.
  • Per-section feedback. Not shipped in MVP-1 — whole-report granularity was the accepted release floor (mvp-scope.md §2.2); per-section is roadmap input. The API carries the (document_source, document_source_id) anchor and the audit exercises it, because the anchor is what any future rebuild depends on — but the shipped surface writes report-level rows only.
  • Idempotency on POST. There is none, which is why the client cannot settle "did my write commit?" on a dropped connection (frontend/src/components/explore/ReportFeedback.tsx). That was a product decision on PR #251 — author plus text is not an identity, and every heuristic built on it either duplicated the row or discarded the analyst's text. Not a gap to close here.