feat(persistence): persist exact text_segment byte spans without 0007 - #99
Conversation
Add typed insert/lookup for the existing 0006 text_segment table so membership can attach to a recovered UTF-8 span instead of raw SQL. Cutoff-eligible document reads keep later-available spans out of a historical fit. No new migration number. Co-authored-by: Seongho Bae <seonghobae@users.noreply.github.com>
# Conflicts: # CHANGELOG.md # crates/persistence_postgres/src/error.rs # crates/persistence_postgres/src/lib.rs # crates/persistence_postgres/src/live_repository.rs # crates/persistence_postgres/tests/live_postgres.rs
|
Current HEAD |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
Current-head RLS fix (5dd50ff): insert_text_segment now binds record.tenant_record_id before rendering/executing the INSERT, matching all sibling tenant-scoped writes. The recording contract asserts the tenant GUC precedes the insert. Local persistence_postgres tests (51), fmt, clippy, and live-sqlx no-run compilation pass. Please re-review this exact head. |
|
Current-head review refresh for 5dd50ff:
|
|
Rebased current head c4ed064 onto origin/main. The changelog and hourly queue conflict were resolved by retaining the text-segment product gap contract and current-main invariants; inherited documentation trailing whitespace was removed. Local merge-tree, git diff --cached --check, and cargo fmt --all -- --check pass. Exact-head hosted checks and required independent approvals remain required before protected merge. |
…25-4fbc-9a53-304c83501192-1885 # Conflicts: # CHANGELOG.md # docs/TRACEABILITY.md # docs/validation/temporal-event-foundation.md
| | Event mention/instance | `event_core` | partial | — | unit + fail-closed promotion | Task 5 / PR #13 | | ||
| | Multiple membership | `membership_core` | partial | — | unit + ESS weights | Task 7 / PR #12 + #25 | | ||
| | Forward transition DAG | `relation_graph` | implemented-main | — | unit + cycle rejection | Task 6 / PR #14 | | ||
| | Bitemporal persistence + live SQL port | `persistence_postgres` | partial | typed `text_segment` SQL | migration contracts + recording transport + optional PgPool + live CI + tenant RLS + `0005`/`0006` + event relation/mention/instance + source-artifact + audit-event + concurrent-write + restore integrity (#37–#44 implemented-main) + typed `text_segment` insert/cutoff lookup (active PR) | Task 8 / PR #16 + #23 + #26 + #27 + #29 + #30–#44 + text-segment SQL | |
There was a problem hiding this comment.
🟡 Duplicate persistence row in capability ledger
The capability table now lists Bitemporal persistence + live SQL port twice: the added row carries Open PR status typed text_segment SQL, while the prior row still reads —. The intended update to the single existing row instead left a stale, contradictory duplicate.
Prompt for agents
The capability ledger in docs/validation/temporal-event-foundation.md now contains two rows for the same capability, Bitemporal persistence + live SQL port (the newly added row at line 21 and the pre-existing row at line 22). The new row updates the Open PR status column to typed text_segment SQL, but the old row with an em dash was not removed. Consolidate these into a single row so the capability appears exactly once, keeping the updated Open PR status and evidence text.
Was this helpful? React with 👍 or 👎 to provide feedback.
| pub fn validate(&self) -> Result<(), PersistenceError> { | ||
| if self.start_byte < 0 || self.end_byte <= self.start_byte { | ||
| return Err(PersistenceError::InvalidTextSegment); | ||
| } | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
📝 Info: Adapter validation duplicates but does not weaken the DB CHECK constraint
TextSegmentRecord::validate (segment_sql.rs) enforces start_byte >= 0 && end_byte > start_byte, which exactly mirrors the physical text_segment_byte_span CHECK (start_byte >= 0 AND end_byte > start_byte) in migrations/0006_typed_membership_assignment.up.sql. The half-open span semantics and fail-closed behavior are consistent between the adapter and the database, so there is no divergence risk here.
Was this helpful? React with 👍 or 👎 to provide feedback.
| /// Returns transport failures. | ||
| pub fn submit_text_segment_by_id( | ||
| &mut self, | ||
| text_segment_id: Uuid, | ||
| ) -> Result<(), PersistenceError> { | ||
| let sql = select_text_segment_by_id_sql(text_segment_id); | ||
| self.session.execute(&sql) | ||
| } | ||
|
|
||
| /// Look up cutoff-eligible text segments for one document identity. | ||
| /// | ||
| /// # Errors | ||
| /// | ||
| /// Returns transport failures. | ||
| pub fn submit_text_segments_for_document_as_of( | ||
| &mut self, | ||
| document_record_id: Uuid, | ||
| knowledge_cutoff: &KnowledgeCutoff, | ||
| ) -> Result<(), PersistenceError> { | ||
| let sql = select_text_segments_for_document_as_of_sql(document_record_id, knowledge_cutoff); | ||
| self.session.execute(&sql) |
There was a problem hiding this comment.
📝 Info: Text-segment lookups also skip tenant binding, but this matches sibling read methods
submit_text_segment_by_id (live_repository.rs) and submit_text_segments_for_document_as_of (:358-365) do not bind the session tenant GUC. Under FORCE RLS a read with an unset GUC fails closed (returns zero rows), so a standalone lookup would silently return nothing. However this matches the established pattern for other read methods (submit_membership_assignments_for_document at :290, submit_source_artifact_by_id at :376, submit_active_analysis_document at :486), which also rely on a previously bound GUC. So it is not a new inconsistency introduced by this PR, only the insert path is anomalous.
Was this helpful? React with 👍 or 👎 to provide feedback.
| /// One append-only exact-span observation on a document. | ||
| /// | ||
| /// Maps to physical `text_segment` from migration `0006`. Byte offsets are | ||
| /// half-open `[start_byte, end_byte)` over the document UTF-8 bytes. | ||
| /// `document_record_id` is required; a foreign key remains a later migration | ||
| /// (`#45` owns `0007`). |
There was a problem hiding this comment.
🔍 text_segment lacks an append-only mutation trigger
text_segment is created in migration 0006 and granted only SELECT, INSERT to tepp_app_runtime, but unlike the append-only identity tables in migration 0004 it has no reject_append_only_mutation statement-level trigger. The PR only adds a SQL adapter and does not touch migrations, so this is out of scope, but if segment spans are intended to be append-only observations (as the doc comment implies: "One append-only exact-span observation"), the absence of a mutation-rejecting trigger means a superuser or future grant change could silently update/delete spans. Worth confirming against the accepted ERD append-only intent.
Was this helpful? React with 👍 or 👎 to provide feedback.
2 participants
Summary
#45owns migration0007and remains the preferred merge; this increment does not allocate0008.text_segmentinsert/lookup on the existing0006table so membership can attach to an exact UTF-8 span instead of raw SQL.[start_byte, end_byte)recovers the knownhellospan fromhello world. Inverted, empty, and negative spans fail closed before SQL is rendered.worldspan out of a February historical fit (ADR 0008 / ADR 0013).Claim boundary
0006columns only. Not adocument_recordforeign key, Unicode scalar columns,segment_type_code, or CSAP/SOC 2 claim.tepp_api,service_tls, or naruon/orchestrator listeners (#87/#90/#92).Test plan
text_segment_sql_contractfailed withE0432/ missingInvalidTextSegmentcargo test -p persistence_postgres --lib --testscargo clippy -p persistence_postgres --all-targets -- -D warningscargo test -p persistence_postgres --features live-sqlx --test live_postgres --no-runpython3 scripts/check_workspace_contract.py,check_docstrings.py,validate_documentation.py