refactor(global-db): remove persist probe and dedupe cursor authority - #649
Conversation
The observation adapter embedded a cfg(test)-only persist probe: a counter struct, a probe_count! macro pair, a cfg-gated field on GlobalDbObservationStore, and call sites threaded through the production persist path, so the production struct's shape differed under cfg(test). Delete the probe entirely and assert at the real seam instead: the adapter's runtime dispatch boundary is now the ObservationRuntimeDispatch trait (production impl: DatabaseRuntimeClientV1, identical struct shape in every build), and the test harness wraps it in a counting double. Zero stored-observation reads prove no decode, collision classification, or payload-revision probe (all require the stored row), and zero submits prove no canonical command digest; the collision tests keep every no-rework assertion and gain non-vacuity anchors showing the counted seam observes the very calls under assertion.
refused_scan_frontier ended in `.ok()`, so an ObservationCursorAdvance construction failure silently became "not at the scan frontier": no coverage recorded, and the refused record re-read forever — the exact loop terminal refusal coverage exists to break. Return ObservationStoreResult<Option<..>>: Ok(None) stays the not-at-frontier verdict (covered replay, stale expected view — both pinned by existing tests), while any construction failure now propagates as the typed store error. For validated writes the failure is unreachable — ObservationWrite::new already proves the exact expected-to-next transition the advance re-derives — so no valid flow changes; the doc records that invariant and the error path guards against it ever being weakened.
record_refusal_with_coverage hand-mirrored the runtime cursor-advance authority "statement for statement" — a duplicate spelling that would drift the moment either side changed. Extract the shared statements into one canonical set, repository::observation_cursor_authority in tracedecay-rusqlite-runtime (cursor read, advance-ledger record, ledger read-back verification predicate, cursor commit). Both executors of the authority — the runtime write path (ObservationExecutor::execute_write / execute_cursor_advance) and the adapter's atomic refusal-marker + coverage transaction — now execute exactly these statements; only the transport differs (rusqlite savepoint vs guarded engine transaction). The atomicity contract is unchanged: marker insert, admission_refused ledger row, in-transaction ledger verification, and cursor move stay one transaction with the in-transaction frontier compare-and-set, and any failure rolls the whole transaction back.
reconcile_collided_observation_provenance repeated the correlated owner-lookup subquery four times inside one INSERT, re-implementing the aggregation that initially populates temp.observation_projection_output_state in state.rs — two spellings of the same ownership derivation that would drift independently. Render both from one definition: output_state_aggregation_sql in state.rs is now the only spelling of the aggregation, taking an optional provenance filter. Cache initialization executes it unfiltered; the new reaggregate_output_state_for_output executes it scoped to one exact output, and rebuild.rs's collided-provenance reconciliation calls that helper instead of carrying its own copy. Statement semantics, index usage, and error operations are unchanged.
|
b445ed7
into
codex/tracedecay-total-redesign-plan
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6492579c68
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| /// prove the terminal-refusal fast path repeats no stored-row read (and thus | ||
| /// no decode, classification, or revision probing) and no submit (and thus no | ||
| /// canonical command digest) by counting here. | ||
| pub(crate) trait ObservationRuntimeDispatch: Clone + Send + Sync { |
There was a problem hiding this comment.
In every production build this abstraction has no caller beyond the forwarding DatabaseRuntimeClientV1 implementation: the file explicitly says the generic runtime exists so tests can observe it, while the only alternate implementation and constructor are cfg(test). This threads a generic through both store traits and expands the public adapter type solely for test instrumentation; keep the production adapter concrete and gather this evidence without a test-only production seam.
AGENTS.md reference: AGENTS.md:L81-L83
Useful? React with 👍 / 👎.
| after.submits - before.submits, | ||
| 0, | ||
| "re-admitted terminal collision must not re-probe the payload revision" | ||
| "re-admitted terminal collision must not submit a runtime write, so it never \ | ||
| canonicalizes or hashes a command again" |
There was a problem hiding this comment.
When a re-admission performs command canonicalization or hashing but exits before dispatch_submit—for example, while constructing metadata or an idempotency identity—this counter remains unchanged because CountingObservationRuntime increments it only after those digest sites. The removed probe counted digest execution directly, so this replacement no longer proves the asserted no-hash fast path and can let a rework regression pass; instrument the canonicalization boundary instead of inferring it from submission.
AGENTS.md reference: AGENTS.md:L125-L127
Useful? React with 👍 / 👎.
1 participant
Fixes four code-quality audit findings in
crates/tracedecay-global-dbfrom the review of the collision-coverage work. One commit per finding.Findings
B2 (blocker) — cfg(test) probe embedded in the production adapter
observation_adapter.rscarriedObservationPersistProbeV1, aprobe_count!macro pair, a cfg-gatedpersist_probefield onGlobalDbObservationStore, and probe call sites threaded through the production persist path — the production struct's shape differed undercfg(test).All of it is deleted. The assertion moved to a real seam: the adapter's runtime dispatch boundary is now the
ObservationRuntimeDispatchtrait (production implDatabaseRuntimeClientV1;GlobalDbObservationStore<R = DatabaseRuntimeClientV1>— identical shape in every build, production only ever constructs the default). The test harness wraps that seam inCountingObservationRuntime, a counting double that forwards every read/submit unchanged. The no-rework guarantees stay proven, not weakened: zero stored-observation reads imply no stored-row decode and therefore no collision classification or payload-revision probe (both require the row), and zero submits imply no canonical command digest (every digest call site is unreachable without a stored-row read or a submit). The reworked tests keep every zero-delta assertion and add non-vacuity anchors (the first collision's stored-row read and the re-admission's own frontier cursor read are observed through the counted seam).M1 (major) — hand-mirrored cursor-advance authority
record_refusal_with_coverageduplicated the runtime cursor-advance authority "statement for statement". There is now one spelling:repository::observation_cursor_authorityintracedecay-rusqlite-runtime(cursor read, advance-ledger record, ledger read-back + verification predicate, cursor commit). Both executors —ObservationExecutor::execute_write/execute_cursor_advanceon the runtime writer's savepoint and the adapter's refusal transaction on the guarded engine transaction — execute exactly these statements. The atomicity contract is preserved exactly: marker insert,admission_refusedledger row, in-transaction ledger verification, and cursor move remain one transaction with the in-transaction frontier compare-and-set, and any failure rolls everything back.failed_coverage_advance_leaves_no_visible_refusal_markerandeof_refusal_converges_new_generation_rescans_without_reopeningare green and untouched by this commit.D9 (medium, behavioral) — swallowed constructor error
refused_scan_frontierended inObservationCursorAdvance::for_ordering(...).ok(), so a construction failure silently became "not at the scan frontier" — no coverage recorded, refused record re-read forever. It now returnsObservationStoreResult<Option<_>>:Ok(None)stays the not-at-frontier verdict (covered replay / stale expected view, both pinned by existing tests), and any construction failure propagates as the typed store error. The failure path is not reachable through validated writes —ObservationWrite::newalready validates the exact expected→nextcursor_transition_coverstransition the advance re-derives from the same identity, expected cursor, and range, andAdmissionRefusedwith no receipt always satisfies the reason/receipt check — so no regression test can construct the failing input; the typed surfacing exists to make any future weakening of that invariant loud instead of an infinite re-read loop, and the function doc records the proof.D6 (medium) — quadruplicated ownership-cache aggregation
reconcile_collided_observation_provenance(rebuild.rs) re-implemented the aggregation that initially populatestemp.observation_projection_output_state. Both now render from one definition:output_state_aggregation_sqlinstate.rs(with an optional provenance filter), executed unfiltered by cache initialization and scoped to one output by the newreaggregate_output_state_for_output, which rebuild.rs's reconciliation calls. Statement semantics and index usage unchanged.Verification
cargo test -p tracedecay-global-db: 399 passed, 4 failed — the failures are exactly the four known pre-existing base failures (registered_database_lease_keeps_runtime_alive_after_map_owner_drops,registered_project_graph_binding_retains_only_the_database_weak_proxy,registered_legacy_relations::…::installation_requires_typed_reset_without_mutating_legacy_profile_shape,concurrent_registered_mounts_singleflight_to_one_runtime); zero new failures, all 18observation_collision_testsplus reset/adapter tests green.cargo test -p tracedecay-rusqlite-runtime: all targets green (290 lib tests + integration targets, 0 failed).cargo clippy -p tracedecay-global-db --all-targets -- -D warnings: clean.cargo clippy -p tracedecay-rusqlite-runtime --all-targets -- -D warnings: clean.cargo fmt -p tracedecay-global-db -- --checkandcargo fmt -p tracedecay-rusqlite-runtime -- --check: clean..githooks/commit-msghook was active).Files touched are confined to
crates/tracedecay-global-dbandcrates/tracedecay-rusqlite-runtime(the M1 seam).