Skip to content

FC-6: consolidate strict Firestore transaction fixtures — lenient fakes certify broken code #9840

Description

@Git-on-my-level

Problem

A lenient test fake certifies broken code: #9739 introduced the state-head helper whose read-after-write regression shipped to prod (23 Sentry events, 503s on POST /v3/memories) because the ledger tests' transaction fake allowed orderings real Firestore forbids. #9780 (merged as PR #9783) fixed it and added a strict fixture — _StrictLedgerTransaction + companion db/document/collection types (backend/tests/unit/test_memory_ledger.py:132) — that enforces read-before-write, making the bug class untestable-around. This matters doubly because the watchdog auto-merge gate (#9822 item 5) uses "tests green" as its bar: that gate is exactly as strong as the strictest fake in the path.

Current state of the class, verified:

  • Firestore transactions — the live gap. Two independent strict fixtures now exist: the ledger one (models set + reference get only; no update/delete/queries) and an earlier candidate-lifecycle fake (backend/tests/unit/test_candidate_lifecycle.py:33) that also enforces read-before-write and does support update. Both are private to their test files. Every other transaction test in the suite builds its own ad hoc fake with whatever semantics its author remembered — the fix(memory): preserve trusted state head across ledger writes #9739 conditions exactly.
  • Rust Redis — already closed, correctly. services/redis.rs is tested against a scripted local RESP peer that drops an EVAL response, reconnects, and asserts the non-replayable command executed exactly once, plus generation-replacement and single-flight tests. Server-side recording is the right mechanism (a client-side fake cannot know whether an ambiguous request executed). Nothing to do.
  • Rust SSE — already closed at the right seam. fix(desktop-backend): stop corrupting multi-byte characters in streamed chat replies #9638's regression tests split a UTF-8 codepoint and an SSE frame mid-byte against the real parser. Nothing to do until an end-to-end transport seam exists (tracked in Split chat_completions.rs into routes/chat/ along its real seams — move-only, streaming closure intact #9836's follow-up).

So the actionable gap is one boundary, and the durable part is the default.

Fix: one shared strict Firestore transaction fixture + a strictness default, one small PR

  1. Consolidate the two fixtures into one helper under the existing tests.unit helper convention: reconcile the ledger fixture (read-before-write on set/reference-get) with the candidate-lifecycle fake (update support) into a single strict transaction fixture; both test files consume it. Document its honest coverage boundary in its docstring: models set/update/reference-get ordering; does not model delete, queries, or retry semantics — extend on the next incident, don't speculate.
  2. Strict by default, lenient by named opt-out. The shared fixture fails loudly on constraint violations; a test needing lenient behavior passes an explicit parameter naming what it relaxes — greppable and reviewable.
  3. One guidance bullet in backend/AGENTS.md → Testing: "A fake for a service boundary must enforce that boundary's ordering/constraint semantics — use the shared strict transaction fixture for Firestore transactions; a lenient fake is how fix(memory): preserve trusted state head across ledger writes #9739 reached prod. When a boundary has no strict fixture, prefer a server-side recorder (see the Rust RESP peer in services/redis.rs tests) over a client-side fake."
  4. Watchdog linkage: the auto-merge eligibility audit (Failure-class protocol and recurrence ratchets #9822 item 5) can reference this: transaction-touching backend subsystems are auto-merge-eligible only under the shared strict fixture.
  5. Registry: FC-6 "lenient service fake", closed-by = shared strict fixture + strict-by-default convention; not-covered = boundaries without incident evidence and fake styles (MagicMock, module stubs) that no mechanical check can classify — recurrence there is caught by the sweep, not a ratchet.

Explicitly no ratchet in this issue: "ad hoc fake of a boundary" spans custom classes, MagicMock, fakeredis, monkeypatched globals, and Rust TCP peers — not mechanically specifiable without unacceptable false positives. If two migrations establish a stable banned construct, a narrow ratchet can follow.

Proof

  • The consolidated fixture's self-test replays fix(memory): preserve trusted state head across ledger writes #9739's read-after-write sequence and fails loudly.
  • Both existing suites (memory ledger, candidate lifecycle) pass on the shared fixture with no assertion changes.
  • Any third suite converted opportunistically that starts failing under strictness is triaged: real latent bug (filed) or legitimate leniency (named opt-out).
  • FC-6 registry entry with the fields above.

Cut list

  • No Rust changes — Redis and SSE are verified-covered; extraction of the RESP peer waits for a second consumer.
  • No bulk migration of every transaction test — the two source suites plus opportunistic conversions.
  • No mechanical ratchet (see above).
  • No new fakes for boundaries without incident evidence (GCS, Pinecone, etc.).
  • No watchdog policy change — Failure-class protocol and recurrence ratchets #9822 owns that.

Sizing

One small PR: one shared fixture module (consolidating two existing ones), two test-file migrations, one AGENTS.md bullet, one registry entry.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions