feat(rag): audit document ingestion, deletion, and chunk purges

[hamza-56]

Closes: RAG document ingestion and deletion leave no audit record

What

Records audit events for every RAG corpus mutation (ingestion attempts, document deletions, and cleanup chunk purges), so the provenance of retrieval content is reconstructable and attributable.

Changes

• feat(core): declare rag.document_ingested, rag.document_deleted, and rag.chunks_purged mutation event types and expose them through sparkth.lib.audit.events
• feat(rag): audit every ingest_document attempt (success joins the chunk-store transaction fail-closed; ineligible/unextractable files record a failure in their own transaction before the error propagates)
• feat(core): record document soft-deletion in the caller's transaction from soft_delete_document
• feat(rag): record a system-actor purge event per cleanup run, committed atomically with the hard delete
• test(rag): audit capture tests for ingestion (success, zero-chunk, unsupported type, scanned PDF) and cleanup (purge run, no-op run)
• test(core): audit capture tests for document soft-deletion (recorded, and no event for a missing id)

How to Test

1. uv run pytest tests/rag/test_ingestion_audit.py tests/rag/test_cleanup.py tests/core/test_documents.py
2. make backend.up.dev, ingest a document through the Google Drive plugin, then delete it and run the cleanup job
3. make db-shell: SELECT category, action, outcome, actor_type, target_id, new_values, old_values FROM audit_events WHERE category = 'rag'; shows the ingestion, deletion, and purge records

Notes

No migration needed (the audit_events table already exists). Actor and source attribution come from the ambient audit context, so REST/MCP-originated ingestions are attributed to the acting user automatically; the cleanup job stamps a system actor labeled rag-cleanup.

This PR description was written with the assistance of an LLM (Claude).

[hamza-56]

@claude-review

[claude]

Claude finished @hamza-56's task in 3m 19s —— View job

---

Claude Code Review

• Read changed files & gather context
• Review correctness & error handling
• Review transaction/atomicity semantics
• Check test coverage
• Verify docs/conventions (CLAUDE.md)
• Post review feedback

Solid, well-scoped PR that closes a real provenance gap (#502). The event-type taxonomy is clean, the transaction semantics are thought through (success joins the chunk-store transaction fail-closed; soft-delete rides the caller's transaction; the purge commits atomically), and the tests are readable and cover the happy paths plus the interesting edge cases. Three things to address, none of them large.

🟠 Medium

1. Cleanup re-records chunks_purged for the same documents on every run — sparkth/rag/cleanup.py:67-87
Soft-deleted Document rows are never hard-deleted by cleanup, so they keep matching is_deleted == True on subsequent runs. After the first run their links are gone, so nothing is actually purged — yet the code still records a rag.chunks_purged event with purged_chunk_count: 0 and the same document_ids, once per run, indefinitely. That's misleading audit noise. Guard the record (and ideally the deletes) on real work having happened this run — e.g. capture the link-delete rowcount and only record when orphan_chunk_ids or rowcount. See inline for a snippet.

2. "Every attempt is audited" isn't quite true — sparkth/rag/ingestion/__init__.py:60-64,107
Only UnsupportedFileTypeError and ScannedPDFError produce a failure event. An unexpected error from extraction, chunking, or store_and_link_chunks (e.g. SQLAlchemyError) propagates with no audit record — the exact gap the PR aims to close. Either narrow the docstring to match the code, or record a failure for the broader case and re-raise (staying fail-closed, no bare except Exception). Code and docstring should agree.

🟡 Low

3. Docstring formatting — sparkth/core/documents/service.py:89-94
Leading spaces and a line starting with , so ... read as a mis-wrapped sentence. Suggested rewrite inline.

4. Test coverage for the no-op-with-deleted-docs case — tests/rag/test_cleanup.py
test_noop_run_records_no_audit_event only covers the no soft-deleted documents path. There's no assertion for the "deleted docs exist but nothing left to purge" case (issue #1). Adding one would lock in the corrected behaviour.

👍 Nits / notes (non-blocking)

• sparkth/rag/* correctly imports through the sparkth.lib.audit façade; sparkth/core/documents/service.py imports from sparkth.core.audit.* directly. Both are legal (core-tier code), just worth a mental note for consistency.
• Nice touch using scrub_error_detail on the failure path so error text can't leak secrets into the trail.
• The _ingested_event / _success_change helpers keep ingest_document readable — good factoring.

Assessment

Approve-with-changes. Item 1 is the one I'd want fixed before merge (it actively pollutes the audit trail); item 2 is a correctness/claim mismatch that should be reconciled; 3 and 4 are quick polish. No security issues found; no migration needed as stated.
· feat/rag-audit-events

[claude]

Medium — "every attempt is audited" doesn't hold for unexpected failures.

The docstring (lines 60–64) states every attempt is audited, but a failure event is only recorded for UnsupportedFileTypeError and ScannedPDFError. Anything else that can raise between here and the commit — extract_to_markdown hitting a corrupt/parse error, DocumentChunker().chunk raising, or a SQLAlchemyError from store_and_link_chunks — propagates with no audit record. For a provenance control (EU AI Act Art. 12), a failed ingestion that leaves no trace is exactly the gap the PR set out to close.

I understand the repo rule forbids swallowing via bare except Exception, so two acceptable options:

1. Narrow the claim — change the docstring to say only eligibility/extraction failures of these known types are recorded as failure, so the guarantee matches the code.
2. Broaden coverage while staying fail-closed — record the failure event for a wider (still explicit) set and re-raise, so unexpected errors are on record too.

Either is fine, but the docstring and the code should agree. Right now they don't.

[hamza-56]

Reconciled by narrowing the claim (option 1) in db90834. The docstring now states that a failure event is recorded only for the two declared eligibility/extraction errors, and that anything else (extractor parse error, chunker error, database error from the chunk write) propagates without a record — noting that nothing entered the corpus in those cases, so the trail stays accurate about the corpus itself but is not a complete log of attempts.

[claude]

Low — docstring formatting. The leading spaces and the line that begins with , so ... read as a mis-wrapped sentence:

    """Soft-delete a Document by id. Does not commit.

    Records a ``rag.document_deleted`` audit event in the caller's transaction,
    so the deletion and its record commit or roll back together. A missing id
    is a no-op and records nothing.
    """

[hamza-56]

Fixed in db90834, applied as suggested.

[hamza-56]

All four addressed in db90834.

1. Repeat chunks_purged records — the record and the link delete are now guarded on candidate_chunk_ids, so a run that purges nothing writes nothing.
2. "Every attempt is audited" — good catch, the docstring was overpromising. Narrowed it to the two declared errors; anything else propagates without a record (nothing lands in the corpus either way).
3. Docstring formatting — applied as suggested.
4. Tests — two new cases in TestCleanupAudit covering repeat runs and deletes with nothing linked.

uv run pytest tests/rag tests/core/test_documents.py green (318), ruff and mypy clean.