feat: document supersession tracking for reasoning chain preservation#457
Open
hartphoenix wants to merge 8 commits intoplastic-labs:mainfrom
Open
feat: document supersession tracking for reasoning chain preservation#457hartphoenix wants to merge 8 commits intoplastic-labs:mainfrom
hartphoenix wants to merge 8 commits intoplastic-labs:mainfrom
Conversation
…ntation queries RepresentationManager._query_documents_recent() and ._query_documents_most_derived() do not filter soft-deleted documents, unlike every other document query function in the codebase. This causes the deriver's working representation to include documents that are being garbage-collected. Refs plastic-labs#444 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…king When a document is replaced by a more informative version (via dedup or specialist knowledge updates), the old document's superseded_by field now points to its replacement. This preserves reasoning chain navigability when source documents are soft-deleted. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
create_documents now returns list[str] (created document IDs) instead of int (count). The ORM objects with IDs already exist in the function after commit; they were simply discarded at the return boundary. This enables callers to reference created documents — needed for supersession linking and for the create_observations tool to return IDs to the LLM. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The create_observations tool response now includes the IDs of created documents, formatted as [id:xxx]. This enables the deduction specialist to reference newly created observations in subsequent tool calls — specifically, to pass a replacement's ID to delete_observations for supersession linking. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
is_rejected_duplicate now returns (bool, replaced_doc_id) so create_documents can set superseded_by on the old document when a more informative near-duplicate replaces it. The link is set within the same transaction before commit. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Superseded documents (superseded_by IS NOT NULL) that have downstream dependents (other docs referencing them in source_ids) are now preserved during reconciler cleanup. Their embeddings are NULLed to prevent HNSW index bloat while retaining content, source_ids, and superseded_by for reasoning chain traversal. Superseded docs without dependents and non-superseded soft-deleted docs are hard-deleted normally. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…oning_chain delete_observations accepts an optional superseded_by parameter so the deduction specialist can link deleted observations to their replacements. get_reasoning_chain follows superseded_by links to display the replacement when a source document has been superseded, instead of reporting "not found in database." Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
re-evaluation design doc Updates the deduction specialist's knowledge update instructions to create the replacement first, read its ID from the tool response, then delete the old observation with superseded_by. Adds a design document describing future dream-time re-evaluation of orphaned reasoning chains. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
WalkthroughThis PR introduces a document supersession mechanism to preserve referential integrity when documents are updated or replaced. It adds a Changes
Sequence Diagram(s)sequenceDiagram
participant Agent as Reasoning Agent
participant ToolHandler as Tool Handler
participant CRUD as CRUD Layer
participant DB as Database
participant VectorStore as Vector Store
Agent->>ToolHandler: create_observations(new_observation)
ToolHandler->>CRUD: create_documents(...)
CRUD->>DB: INSERT new document
CRUD->>ToolHandler: return [new_id]
ToolHandler->>Agent: response with [id:new_id]
Agent->>ToolHandler: delete_observations(old_id, superseded_by=new_id)
ToolHandler->>CRUD: delete_document(..., superseded_by=new_id)
CRUD->>DB: UPDATE old_document SET deleted_at=now(), superseded_by=new_id
CRUD->>ToolHandler: success
Agent->>ToolHandler: get_reasoning_chain(chain_id)
ToolHandler->>CRUD: get_documents_by_ids_include_superseded([chain_id])
CRUD->>DB: SELECT document WHERE id=chain_id OR superseded_by links
CRUD->>ToolHandler: return document (if superseded, include tombstone)
alt Document is superseded
ToolHandler->>CRUD: get_documents_by_ids([superseded_by_id])
CRUD->>DB: SELECT replacement document
CRUD->>ToolHandler: return replacement
ToolHandler->>Agent: chain with (superseded) marker + replacement details
else Document is current
ToolHandler->>Agent: chain with current document
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://cdn.statically.io/img/avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/utils/agent_tools.py (2)
1731-1789:⚠️ Potential issue | 🟡 MinorContradiction observations still lose their sources here.
create_observations()storessource_idsforcontradictionobservations too, but this branch only traverses"deductive"and"inductive". A contradiction with recorded sources will fall through to “None recorded,” so the new reasoning-chain view is incomplete for a supported observation type.Suggested fix
- if level in ("deductive", "inductive") and doc.source_ids: + if level in ("deductive", "inductive", "contradiction") and doc.source_ids: label = "Premises" if level == "deductive" else "Sources"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/agent_tools.py` around lines 1731 - 1789, The logic that renders sources checks only level in ("deductive","inductive") so contradiction observations with doc.source_ids fall through to the "None recorded" branch; update the branch that builds source_lines to also handle contradictions (either include "contradiction" in the tuple: if level in ("deductive","inductive","contradiction") or add a separate elif for level == "contradiction") and set an appropriate label (e.g., label = "Premises" for deductive, "Sources" for inductive, and "Contradicting Sources" or similar for contradiction), then reuse the same live_docs/tombstones traversal that uses crud.get_documents_by_ids, crud.get_documents_by_ids_include_superseded, and appends to output_parts so that doc.source_ids are displayed for contradictions as well (ensure you reference variables level, doc.source_ids, live_docs, tombstones, source_lines, and output_parts when making the change).
729-753:⚠️ Potential issue | 🟠 Major
created_levelscan drift from the IDs that were actually persisted.
crud.create_documents(..., deduplicate=True)can skip items, but this result still buildscreated_levelsfrom every validated input. If something drops out of the middle of the batch,_handle_create_observations()will pair later IDs with the wrong levels and overcount level-based reporting.Suggested fix
- created_ids: list[str] = [] + created_ids: list[str] = [] + created_levels: list[str] = [] if documents: created_ids = await crud.create_documents( db, documents=documents, workspace_name=workspace_name, observer=observer, observed=observed, deduplicate=True, ) + if created_ids: + created_docs = await crud.get_documents_by_ids( + db, workspace_name, created_ids + ) + created_by_id = {doc.id: doc for doc in created_docs} + created_levels = [ + created_by_id[doc_id].level or "explicit" for doc_id in created_ids + ] logger.info( "Created %d observations in %s/%s/%s", len(created_ids), workspace_name, observer, observed, ) return ObservationsCreatedResult( created_count=len(created_ids), created_ids=created_ids, - created_levels=[doc.level for doc in documents], + created_levels=created_levels, failed=failed, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/agent_tools.py` around lines 729 - 753, The code builds created_levels from the original documents list, which can desync when crud.create_documents(..., deduplicate=True) skips items; update the flow so created_levels is derived only from the documents actually persisted: change crud.create_documents to return richer info (e.g. list of created items or tuples with id and level) or have it return created_levels alongside created_ids, then use that returned created_levels (instead of [doc.level for doc in documents]) when constructing ObservationsCreatedResult so created_ids and created_levels remain aligned (adjust call sites referring to crud.create_documents, ObservationsCreatedResult, created_ids, created_levels, and documents accordingly).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/crud/document.py`:
- Around line 916-974: Only mark superseded tombstones as preserved after the
vector deletion succeeds: move the DB update that NULLs embeddings (the
update(models.Document).values(embedding=None) call) into the delete_many
success path and do not count a tombstone as processed if delete_many raised;
additionally, add/flip a durable marker on the Document row (e.g., set a
preserved boolean or preserved_at timestamp column on models.Document) when the
vector delete succeeds so future cleanup passes skip already-preserved rows
(adjust the selection logic in the triage that builds
preserve_ids/delete_candidates to exclude rows where Document.preserved is true
or preserved_at is not null); reference external_vector_store.delete_many,
preserve_ids, and the update(models.Document) call when making these changes.
---
Outside diff comments:
In `@src/utils/agent_tools.py`:
- Around line 1731-1789: The logic that renders sources checks only level in
("deductive","inductive") so contradiction observations with doc.source_ids fall
through to the "None recorded" branch; update the branch that builds
source_lines to also handle contradictions (either include "contradiction" in
the tuple: if level in ("deductive","inductive","contradiction") or add a
separate elif for level == "contradiction") and set an appropriate label (e.g.,
label = "Premises" for deductive, "Sources" for inductive, and "Contradicting
Sources" or similar for contradiction), then reuse the same live_docs/tombstones
traversal that uses crud.get_documents_by_ids,
crud.get_documents_by_ids_include_superseded, and appends to output_parts so
that doc.source_ids are displayed for contradictions as well (ensure you
reference variables level, doc.source_ids, live_docs, tombstones, source_lines,
and output_parts when making the change).
- Around line 729-753: The code builds created_levels from the original
documents list, which can desync when crud.create_documents(...,
deduplicate=True) skips items; update the flow so created_levels is derived only
from the documents actually persisted: change crud.create_documents to return
richer info (e.g. list of created items or tuples with id and level) or have it
return created_levels alongside created_ids, then use that returned
created_levels (instead of [doc.level for doc in documents]) when constructing
ObservationsCreatedResult so created_ids and created_levels remain aligned
(adjust call sites referring to crud.create_documents,
ObservationsCreatedResult, created_ids, created_levels, and documents
accordingly).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8f838a94-362f-401d-81f8-058dfc9f6317
📒 Files selected for processing (13)
docs/design/dream-time-reevaluation.mdmigrations/versions/a3f7b9d2c1e4_add_document_superseded_by.pysrc/crud/__init__.pysrc/crud/document.pysrc/crud/representation.pysrc/dreamer/specialists.pysrc/models.pysrc/utils/agent_tools.pytests/crud/test_document.pytests/crud/test_representation_manager.pytests/deriver/test_vector_reconciliation.pytests/dreamer/test_specialists.pytests/utils/test_agent_tools.py
Comment on lines
+916
to
+974
| # Phase 1: Triage superseded documents | ||
| # Superseded docs with live dependents are preserved as tombstones (embedding NULLed). | ||
| # Superseded docs without dependents and non-superseded docs are hard-deleted. | ||
| preserve_ids: list[str] = [] | ||
| delete_candidates: list[models.Document] = [] | ||
|
|
||
| for doc in documents: | ||
| if doc.superseded_by is not None: | ||
| # Check if this tombstone has live dependents | ||
| children = await get_child_observations( | ||
| db, | ||
| doc.workspace_name, | ||
| doc.id, | ||
| observer=doc.observer, | ||
| observed=doc.observed, | ||
| ) | ||
| if children: | ||
| preserve_ids.append(doc.id) | ||
| else: | ||
| delete_candidates.append(doc) | ||
| else: | ||
| delete_candidates.append(doc) | ||
|
|
||
| # NULL embeddings on preserved tombstones and delete from vector store | ||
| if preserve_ids: | ||
| # Delete vectors for preserved tombstones (they won't need them) | ||
| preserve_by_namespace: dict[str, list[str]] = {} | ||
| for doc in documents: | ||
| if doc.id in preserve_ids: | ||
| namespace = external_vector_store.get_vector_namespace( | ||
| "document", | ||
| doc.workspace_name, | ||
| doc.observer, | ||
| doc.observed, | ||
| ) | ||
| preserve_by_namespace.setdefault(namespace, []).append(doc.id) | ||
|
|
||
| for namespace, ids in preserve_by_namespace.items(): | ||
| try: | ||
| await external_vector_store.delete_many(namespace, ids) | ||
| except Exception as e: | ||
| logger.warning(f"Failed to delete vectors for preserved tombstones from {namespace}: {e}") | ||
|
|
||
| await db.execute( | ||
| update(models.Document) | ||
| .where(models.Document.id.in_(preserve_ids)) | ||
| .values(embedding=None) | ||
| ) | ||
| logger.debug( | ||
| f"Preserved {len(preserve_ids)} superseded tombstones (NULLed embeddings)" | ||
| ) | ||
|
|
||
| # Phase 2: Hard-delete documents that should be removed | ||
| if not delete_candidates: | ||
| if preserve_ids: | ||
| await db.commit() | ||
| return len(preserve_ids) | ||
| await db.rollback() | ||
| return 0 |
Contributor
There was a problem hiding this comment.
Preserved tombstones can keep re-entering cleanup forever.
These rows stay soft-deleted, so this new preservation path has no terminal marker that keeps them out of later cleanup batches. Because the branch also sets embedding=None and counts the row as processed even when delete_many() fails, the reconciler can report progress while repeatedly revisiting the same preserved tombstones and delaying real hard-delete candidates.
Only mark a tombstone as preserved after the vector delete succeeds, and make already-preserved rows ineligible for future cleanup passes.
🧰 Tools
🪛 Ruff (0.15.7)
[warning] 956-956: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/crud/document.py` around lines 916 - 974, Only mark superseded tombstones
as preserved after the vector deletion succeeds: move the DB update that NULLs
embeddings (the update(models.Document).values(embedding=None) call) into the
delete_many success path and do not count a tombstone as processed if
delete_many raised; additionally, add/flip a durable marker on the Document row
(e.g., set a preserved boolean or preserved_at timestamp column on
models.Document) when the vector delete succeeds so future cleanup passes skip
already-preserved rows (adjust the selection logic in the triage that builds
preserve_ids/delete_candidates to exclude rows where Document.preserved is true
or preserved_at is not null); reference external_vector_store.delete_many,
preserve_ids, and the update(models.Document) call when making these changes.
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
1 participant
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
When a document is soft-deleted — via deduplication or the dreamer's deduction specialist —
all downstream documents referencing it via
source_idslose their reasoning chain anchor.This PR transforms soft-deletion from an information loss event into an information gain event
by tracking the replacement relationship.
superseded_bycolumn toDocumentmodel (nullable TEXT, no foreign key constraint —the reconciler hard-deletes unlinked superseded docs, so foreign keys would create cascade fragility)
create_documentsreturns document IDs (was: count) so callers can reference created docscreate_observationstool response includes[id:xxx]for each created documentsuperseded_byon the old doc when a more informative duplicate replaces itdelete_observationsaccepts optionalsuperseded_byparameter for specialist-driven supersessionto prevent HNSW index bloat)
get_reasoning_chainfollowssuperseded_bylinks one hop to display replacementsdocs/design/dream-time-reevaluation.md) describes future work: eager cascade,in-place source_ids update, and full chain deletion
All changes are backward-compatible. The
superseded_bycolumn is nullable with default NULL.Existing code paths that don't reference supersession continue to work unchanged. API schemas
are not modified.
Relates to #403 — supersession tracking provides infrastructure for the specialist improvements
proposed there (contradiction resolution, temporal reasoning)
Test plan
create_documentsreturns list of IDs; returns[]when all dedupedcreate_observationsresponse includes[id:xxx]; no IDs when all failsuperseded_byon replacement; no link on rejection/unique/no-dedupnon-superseded; preserved docs retain content/links
delete_observationswith/withoutsuperseded_by; chain traversal followssupersession; superseded target shows notice; no-supersession backward compat
Summary by CodeRabbit
New Features
Bug Fixes