Repository navigation
Statement log follow-ups from the #771 panel: re-readable secret collector (F13) and six Minors #774
Description
Problem
PR #771 (epic #700, the statement log and .sql()) merged to main at f3e46f8 with the review panel's Round 2 verdict at HUMAN DECISION: no Critical, Spec PASS, Verify green, and one Important plus six Minors left open. This issue carries them so they are not lost.
Important
F13 · src/ferro/_bind_payload.py:72 · the secret_values iterable has an unwritten once-only contract. Writes (Model.save, bulk_create) and raw SQL pass deferred_secret_values(...), a single-shot generator the engine reads at most once and only when ferro.sql is on; query terminals pass compiled.secret_values, an eager tuple. Nothing in the FFI signature (secret_values: Iterable[object] | None) says which rule a caller must follow. Model.save binds one generator into three branches and is correct only because exactly one runs: a refactor that sends two statements with it, or reuses it, hands the second an exhausted iterator, the secret set is empty, and a SecretStr is logged unredacted with no error.
Fix: one collector behind the write and raw entries that returns an object the engine can read any number of times and that computes on first read (a cached lazy sequence), so field_values and the wrapper fold in and the contract is "pass the collector"; terminals pass the same type. Pin it with a test that reads the iterable twice.
Minor
- F14
tests/test_statement_log_run.py:70: the run-lock test does not order the probe before the DDL; assertrecords.index(lock[1]) < min(records.index(r) for r in ddl). - F15
tests/test_statement_log.py:903: the log-off test asserts exact call counts on the privatesecret_values_in; keep the public-seam assertions (emitted == [],writes == []) and drop the totals or make them per-terminal. - F16
docs/adr/0057…:22: the logger section still says Python reads the switch; the engine does (sql_log::enabledat the FFI entry). Add a dated "Amended" sentence there and drop "still" from the :38 amendment. - F17
GLOSSARY.mdStatement log: still "every statement ferro sends"; the pool'safter_connect/after_releasestatements are excluded (ADR-0057 amendment, Connections guide). Say "every statement an operation sends". - F18
docs/adr/0057…:42: the pool-hook amendment uses "## Amendment (date):", a form no other ADR uses; use the dated "Amended …" paragraph form. - F19
docs/adr/0057…:36: the superseded redaction sentence sits unmarked above its amendment; mark it superseded or strike it. - F6 (Round 1)
src/ferro/query/rendered.py:95/src/ferro/_sql_log.py:51: the(params, mask)→REDACTEDzip lives in two Python consumers; one function in_sql_logfor both. - F10 (Round 1): records carry
subject, which the epic's attribute list omits; add it to the epic's Spec deltas. - F9 narrowing: the pool-hook exclusion is a deliberate narrowing of the epic's "every statement Ferro sends"; record it on the epic's plan comment as
**Narrowed:**and in## Spec deltas.
Candidate ADRs (from the panel)
- An emitter or handler error on
ferro.sqlfails the operation after its statement ran, so an autocommit write that committed reports failure (I-6 as applied). - A statement dropped by asyncio cancellation mid-send leaves no record.
- A
NULLbound to a secret column is never masked (the hashed set ignoresNULL).
Residual risk noted
- An emitter error raised inside
LockScope.__aenter__keeps the run lock held until the scope drops (async withnever runs__aexit__when__aenter__raises); predates fix(log): panel round 1 for #764 — run-lock statements in the log, pool hooks decided, operation values documented #773. - A run waiting on another's lock writes one DEBUG record per 100 ms poll.
- The
ferrologger posture change (NullHandler + propagate) needs an upgrade note in the release.
Panel reports: Round 1 #771 (comment); Round 2 in the orchestrator's session (unposted at merge time).
Activity