Skip to content

feat(db): add global soft-delete query filter#2998

Merged
daryllimyt merged 2 commits into
mainfrom
daryl/eng-1519-soft-delete-query-filter
Jul 8, 2026
Merged

feat(db): add global soft-delete query filter#2998
daryllimyt merged 2 commits into
mainfrom
daryl/eng-1519-soft-delete-query-filter

Conversation

@daryllimyt

@daryllimyt daryllimyt commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Adds a global ORM soft-delete filter: a do_orm_execute listener on Session applies with_loader_criteria(SoftDeleteMixin, deleted_at IS NULL, include_aliases=True) to top-level ORM SELECTs, so soft-deleted rows disappear from product queries by default.
  • Opt-out is explicit and narrow: with_deleted(stmt) / the include_deleted execution option, reserved for staging/provenance/sync-reconciliation/erasure helpers (later PRs). Other uses are a review smell.
  • Ships a behavior-pinning coverage suite (9 tests) so the filter's exact semantics are documented executable behavior, not tribal knowledge.

Part of ENG-1517 (version-less resource refs). Stacks on #2931, which introduced SoftDeleteMixin (columns-only) on AgentPreset. This addresses the with_loader_criteria suggestion raised in #2931's review — including its caveats, which are pinned as tests here.

Review guide

  • Start with tracecat/db/soft_delete.py (~80 lines, the whole mechanism): listener guards (is_select and not is_column_load and not is_relationship_load), propagate_to_loaders=False, the INCLUDE_DELETED execution option, and assert_soft_delete_listener_registered().
  • The contract, stated in the module docstring: relationship loads are deliberately unfiltered (UUID/historical paths must keep reaching tombstones); warm Session.get() identity-map hits bypass SQL and therefore the filter; UPDATE/DELETE are unfiltered; Core/text statements are unfiltered.
  • Wiring (db/engine.py, api/app.py): registration is an import side-effect mirroring session_events; the API lifespan asserts registration at startup so a missing import fails fast in prod instead of silently no-op'ing. Worker processes get registration transitively via engine.py.
  • tests/unit/test_soft_delete_filter.py: one test per pinned behavior — entity selects, mapped-column tuple selects (filtered — verified), aliased joins, relationship loads (see tombstones), cold vs warm Session.get(), RLS-bypass sessions (still filtered), UPDATE/DELETE (unfiltered), direct-engine sessions (import wiring canary).
  • Audit: AgentPresetService.get_preset(include_deleted=True) / get_preset_by_slug(include_deleted=True) now also set the execution option. All other deleted_at predicates were audited and deliberately left in place — redundant filtering is rollback-safe, and they're stripped only in a later cleanup once this suite has baked. File/case-comment deleted_at columns don't inherit the mixin and are untouched.

Note

We will be updating the explicit filters introduced in the original PR upstream

Back-compat / rollback

  • No schema change, no API change. Existing explicit predicates keep every active-only path correct even if this PR is reverted.
  • Behavior-neutrality verified: 281 existing tests across preset/skill/channels/folders/sync-contract/session-versioning suites pass unchanged with the listener active.

Validation

  • uv run pytest tests/unit/test_soft_delete_filter.py — 9 passed
  • Regression suites (preset, skill, channels, folders, sync acceptance contract, session versioning) — 281 passed
  • uv run ruff check . / format --check clean; uv run basedpyright --warnings — 0 errors, 0 warnings

Summary by cubic

Adds a global ORM SELECT filter to hide soft-deleted rows by default, with a with_deleted() escape hatch. Part of ENG-1519.

  • New Features

    • Global criteria on SoftDeleteMixin adds deleted_at IS NULL to ORM SELECTs; relationship loads and UPDATE/DELETE remain unfiltered.
    • with_deleted(stmt) for narrow opt-outs; AgentPresetService uses it when include_deleted=True.
    • Listener registered via side-effect import in tracecat.db.engine; api/app.py asserts with assert_soft_delete_listener_registered(). Tests cover SELECTs, joins, relationship loads, cold/warm Session.get(), RLS-bypass, direct-engine sessions, and a subprocess check for engine wiring.
  • Migration

    • No action needed. Only wrap queries with with_deleted(stmt) when tombstones are intentionally required.

Written for commit ab533d6. Summary will update on new commits.

Review in cubic

@daryllimyt daryllimyt added agents LLM agents tests Changes to unit and integration tests feature labels Jul 8, 2026
@daryllimyt
daryllimyt marked this pull request as ready for review July 8, 2026 17:36
@zeropath-ai

zeropath-ai Bot commented Jul 8, 2026

Copy link
Copy Markdown

No security or compliance issues detected. Reviewed everything up to ab533d6.

Security Overview
Detected Code Changes
Change Type Relevant files
Enhancement ► tracecat/agent/preset/service.py
      import with_deleted from tracecat.db.soft_delete
      use with_deleted when include_deleted is true in get_preset/get_preset_by_slug
Enhancement ► tracecat/api/app.py
      import assert_soft_delete_listener_registered
      call assert_soft_delete_listener_registered() in lifespan
Enhancement ► tracecat/db/engine.py
      ensure soft_delete module is imported for listener registration (side effect import)
Enhancement ► tracecat/db/models.py
      update SoftDeleteMixin docstring to mention global ORM SELECT filtering lives in tracecat.db.soft_delete
Enhancement ► tracecat/db/soft_delete.py
▸ new file: implement global ORM criteria for soft-delete, with_deleted helper, and listener registration assertion
Enhancement ► tracecat/db/engine.py
▸ (relation) import side-effect to register listeners via soft_delete
Bug Fix / Refactor ► tracecat/api/app.py
▸ import and use assert_soft_delete_listener_registered to ensure listener presence at startup
Other ► test file addition: tests/unit/test_soft_delete_filter.py (new tests covering soft-delete filtering behavior under various scenarios)

@daryllimyt

Copy link
Copy Markdown
Contributor Author

@codex review

@daryllimyt
daryllimyt requested a review from jordan-umusu July 8, 2026 17:47
@daryllimyt daryllimyt added the priority:critical Critical priority ticket — drop everything label Jul 8, 2026

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All reported issues were addressed across 6 files

Confidence score: 5/5

  • Safe to merge after the addressed issues were fixed.

Reply with feedback, questions, or to request a fix.

Re-trigger cubic

Comment thread tests/unit/test_soft_delete_filter.py
@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. You're on a roll.

Reviewed commit: bb95f5a935

ℹ️ 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".

@jordan-umusu jordan-umusu left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

Comment thread tracecat/db/soft_delete.py Outdated
Comment thread tracecat/db/soft_delete.py Outdated
@daryllimyt
daryllimyt force-pushed the daryl/eng-1519-soft-delete-query-filter branch from 98e6f3a to ab533d6 Compare July 8, 2026 18:32
@daryllimyt
daryllimyt merged commit 74b3246 into main Jul 8, 2026
15 checks passed

Copy link
Copy Markdown
Contributor Author

Merge activity

  • Jul 8, 7:14 PM UTC: A user started a stack merge that includes this pull request via Graphite.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

agents LLM agents feature priority:critical Critical priority ticket — drop everything tests Changes to unit and integration tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants