Skip to content

Statement log follow-ups from the #771 panel: re-readable secret collector (F13) and six Minors #774

Open

Description

@0x054

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; assert records.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 private secret_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::enabled at the FFI entry). Add a dated "Amended" sentence there and drop "still" from the :38 amendment.
  • F17 GLOSSARY.md Statement log: still "every statement ferro sends"; the pool's after_connect / after_release statements 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) → REDACTED zip lives in two Python consumers; one function in _sql_log for 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.sql fails 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 NULL bound to a secret column is never masked (the hashed set ignores NULL).

Residual risk noted

Panel reports: Round 1 #771 (comment); Round 2 in the orchestrator's session (unposted at merge time).

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    needs-triageNeeds evaluation by a maintainer

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions