Conversation
hexfusion
force-pushed
the
feat/1258-store-contracts
branch
from
September 22, 2026 04:22
d963a7f to
2277633
Compare
This was referenced Sep 22, 2026
hexfusion
marked this pull request as ready for review
September 22, 2026 05:48
leseb
requested changes
Sep 22, 2026
leseb
left a comment
leseb
left a comment
Collaborator
There was a problem hiding this comment.
P2 — Conversations cannot use the owner-scoped registry. registry.rs:33 explicitly defers conversation-item access. OwnerScopedStore only exposes Responses operations, so PR #1279 does not satisfy #1258’s requirement that both capabilities be available through typed, owner-scoped handles.
P2 — In-memory resource IDs have different ownership semantics. memory.rs:44 keys responses and conversations by (owner, id). SQL uses globally unique IDs and rejects another owner reusing an existing ID. The test backend therefore accepts ownership collisions production rejects.
P2 — Response plus approval persistence is not atomic. memory.rs:85 inherits the sequential default implementation. A concurrent delete can run after the response write but before approval insertion, leaving orphaned approval data. It also accepts approvals without a corresponding response, unlike SQL. Override the combined operation under one mutex lock.
P2 — In-memory item writes do not enforce SQL invariants. memory.rs:290 and memory.rs:419 accept orphaned or mismatched-owner items. The transactional method also misses duplicate IDs within the same batch, producing records SQL would reject and roll back.
praxis-bot
reviewed
Sep 22, 2026
praxis-bot
left a comment
praxis-bot
left a comment
There was a problem hiding this comment.
PR Review
Clean extraction -- the trait design, owner-scoped facade, blanket PersistedStateBackend impl, contract test suite, and in-memory reference backend are well-structured. One consistency bug in the in-memory backend.
hexfusion
force-pushed
the
feat/1258-store-contracts
branch
from
September 25, 2026 02:52
2277633 to
53f671e
Compare
Extract the persistence contracts from praxis-ai-apis into a new publish=false praxis-ai-store crate with no sqlx/TLS in its dependency graph: the StateOwner identity (from_trusted_parts stays the only constructor, so the crate cannot mint a request-forged owner), the record types and StoreError, the ResponseStore and ConversationItemStore traits, and a PersistedStateBackend supertrait with a blanket impl over any type implementing both halves. Part of praxis-proxy#1258 (epic praxis-proxy#1257). The SQL backends adopt these in a follow-up. Signed-off-by: Sam Batschelet <sbatsche@redhat.com>
…t suite StoreRegistry holds Arc<dyn PersistedStateBackend> so a resolved backend provably implements both trait halves; a Conversations-requiring caller can never receive a response-only backend. OwnerScopedStore binds every request-driven access to one validated owner. The registry stays transport-free (no praxis_filter); the pipeline binding lives in the transport layer. InMemoryStore is a deterministic double for service unit tests: a single mutex makes the multi-step operations (all-or-nothing approval consumption, compare-and-swap, create/delete-items-and-sync) atomic, as the SQL backends achieve with a transaction. The test-support contract_tests suite is the shared conformance harness the in-memory and SQL backends run against. Part of praxis-proxy#1258 (epic praxis-proxy#1257). Signed-off-by: Sam Batschelet <sbatsche@redhat.com>
…fter the response The transport layer (apis) validates an individual owner component as it parses trusted headers, before it has all three parts for from_trusted_parts, so validate_component is now public. The contract suite writes the owning response before recording its pending approvals, so a backend with a foreign key from approvals to responses accepts them. Part of praxis-proxy#1258 (epic praxis-proxy#1257). Signed-off-by: Sam Batschelet <sbatsche@redhat.com>
Depend on praxis-ai-store and re-export its traits, records, owner, and errors from crate::store and crate root, so existing paths keep compiling. Remove the moved trait_def and types modules; split StateOwner out of state_owner.rs (the transport filter and projection stay). The SQL backends implement the moved traits and the ResponseStoreRegistry becomes a transport-bound wrapper over the SQL-free StoreRegistry, holding a combined Arc<dyn PersistedStateBackend> so a resolved backend always has both halves; the PipelineExtension binding lives here, keeping the store crate transport-free. Prove adoption by running the shared contract suite against the SQLite backend. The conversations-filter store path is unchanged (deferred to Part of praxis-proxy#1258 (epic praxis-proxy#1257). Signed-off-by: Sam Batschelet <sbatsche@redhat.com>
…d test-support Nothing in non-test src uses tokio (InMemoryStore is std::sync::Mutex; the tokio::test macros are covered by the dev-dependency), so remove it from the normal dependencies to keep tokio out of every downstream consumer's non-dev closure. Gate pub mod memory behind test-support like contract_tests, so the test double is not built or exported in the default build. Part of praxis-proxy#1258 (epic praxis-proxy#1257). Signed-off-by: Sam Batschelet <sbatsche@redhat.com>
…i-store The registry that used DashMap moved into praxis-ai-store, so apis no longer uses dashmap. Remove it from the dependencies and the store feature; cargo machete flagged it as unused. Part of praxis-proxy#1258 (epic praxis-proxy#1257). Signed-off-by: Sam Batschelet <sbatsche@redhat.com>
The test target ran cargo test --workspace with default features (store-postgres only), so sqlite-backed tests in praxis-test-utils constructed a sqlite ResponseStore against a backend that was never compiled and failed at runtime with backend 'sqlite' is unavailable. Add STORE_ALL_WORKSPACE_FEATURES (already used by coverage-check) so the store crates build with store-all across the workspace run; every crate still runs, none is dropped. Signed-off-by: Sam Batschelet <sbatsche@redhat.com>
…ants The reference double diverged from what the SQL backends enforce, so the shared contract suite could pass a backend the SQL side would reject. - Key responses globally by id; a colliding id from another owner is rejected, not shadowed (SQL PRIMARY KEY (id) + owner-guarded upsert). - Override persist_response_with_pending_approvals to hold one lock across the response and approval writes, so a concurrent delete cannot orphan an approval (the default impl locks twice and leaves that window open). - Reject item writes that orphan, cross owners, or reuse an id within one batch (SQL parent-scoped insert + item PRIMARY KEY). - Expose the conversation-item surface owner-scoped on OwnerScopedStore, alongside the responses surface. Extend the shared contract suite to assert each invariant against both the in-memory and SQLite backends. Signed-off-by: Sam Batschelet <sbatsche@redhat.com>
Relocate PoolConfig, SslMode, and table-name validation from apis/src/store into a new backend_config module in the SQL-free praxis-ai-store crate, so the backend-free build no longer links sqlx or its crypto (sqlx-postgres, sqlite, hkdf, stringprep, and the prohibited md-5). The sqlx conversions stay behind with the SQL backends: apply_pool_config in pool.rs and the SslMode-to-PgSslMode mapping in postgres.rs. The mapping becomes a free function because both types are foreign once SslMode moves, so the From impl would violate the orphan rule. praxis-ai-store stays SQL-free and crypto-free: cargo tree shows no sqlx, rustls, ring, openssl, or md-5 under default or --all-features. Signed-off-by: Sam Batschelet <sbatsche@redhat.com>
…ession main landed response-store compression (praxis-proxy#1182) inside the store region this branch extracts. Carry it across the rebase: add the compression argument to the SQLite contract-suite constructor call, and point the compression benchmark at a now-public to_pg_ssl_mode. The From<SslMode> for PgSslMode impl became an orphan once SslMode moved to praxis-ai-store, so the conversion is a function the benchmark can call. Signed-off-by: Sam Batschelet <sbatsche@redhat.com>
The workspace gained the praxis-ai-store member, but the container and FIPS Containerfiles COPY crate dirs selectively and omitted it, so cargo could not load the workspace manifest in the ubi-image (FIPS) build. Copy store/ into both, matching each file's pattern (manifest+src for the cached build, whole dir for the FIPS build). Signed-off-by: Sam Batschelet <sbatsche@redhat.com>
hexfusion
force-pushed
the
feat/1258-store-contracts
branch
from
September 25, 2026 07:21
144ca20 to
37af7d7
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Extract the SQL-free persisted-state contracts out of praxis-ai-apis into a new internal praxis-ai-store crate: the ResponseStore and ConversationItemStore traits, the PersistedStateBackend supertrait binding both, the persisted record types, StoreError, the StateOwner data type, owner-scoped handles, the typed registry, an in-memory backend, and a shared contract-test suite. The crate carries no sqlx or SQL-client TLS in its dependency graph, so a downstream build can hold the persistence interface without its cryptography. apis adopts the crate by re-export, so persisted records, schemas, and the Responses/Conversations API surface do not change. This is the foundational slice of the store-layer split (#1257).
This branch also carries one build fix at the bottom of the stack. make test ran cargo test --workspace with default features (store-postgres only), so the sqlite-backed tests in praxis-test-utils constructed a sqlite ResponseStore against a backend that was never compiled and failed at runtime with a backend-unavailable error. make test now passes STORE_ALL_WORKSPACE_FEATURES (the variable coverage-check already uses) so the store crates build with store-all and cargo feature unification enables sqlite where those tests run, while keeping every workspace crate in the run rather than carving test into a subset of dedicated targets.
Related issue
Closes #1258
Part of #1257.
Validation
The contracts crate is crypto-free: cargo tree -p praxis-ai-store (default and --all-features) shows no sqlx, rustls, ring, or openssl. The SQL backends adopt the moved traits with no schema or record change and pass the shared contract suite, and the in-memory backend passes the same suite. make lint is clean, and make test at this tip is 7111 passed / 0 failed with zero sqlite-unavailable failures. The only non-deterministic reds anywhere are pre-existing server-spawning integration examples (port/AddrInUse), unrelated to this change.
Checklist
Breaking changes
No behavior break. apis re-exports the moved items at their prior paths, so existing consumers compile unchanged. The one rename, OwnerScopedStore to OwnerScopedResponseStore, is aliased in apis. The addition is the new praxis-ai-store crate.