fix(agent): make preset deletion a soft delete#2931
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
daryllimyt
added
fix
|
✅ No security or compliance issues detected. Reviewed everything up to 44a50a0. Security Overview
Detected Code Changes
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7e7dd43e83
ℹ️ 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".
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 17 files
Confidence score: 2/5
- In
tracecat/agent/preset/service.py,resolve_agent_preset_version()can still return a version when a pinnedpreset_version_idpoints to an archived preset, which bypasses the active-preset guard and could let retired configuration continue running in production — block archived presets in the pinned-version path before merging. - In
tracecat/agent/channels/router.py,require_active_preset_for_tokenthrowsTracecatNotFoundErrorwith a generic “not found” message that is then handled by the existingexcept TracecatNotFoundErrorbranch, risking incorrect error mapping/response semantics for inactive presets — raise a distinct error (or adjust the handler) so inactive-vs-missing presets are handled explicitly before merge.
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fa0ac84e85
ℹ️ 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".
There was a problem hiding this comment.
2 issues found across 8 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="tracecat/agent/skill/service.py">
<violation number="1" location="tracecat/agent/skill/service.py:2254">
P2: The archived skill check is only enforced in the LATEST resolution path (`_get_latest_skill_refs_for_preset_version`). In non-LATEST/pinned mode, the exact-version branch of `get_resolved_skill_refs_for_preset_version()` still returns `SkillVersion` rows without verifying `Skill.archived_at`. This means a user can archive a skill and still execute an older pinned preset version that references it, bypassing the new archived-skill restrictions. Consider adding the same `Skill.archived_at` check to the exact-version resolution path for consistency.</violation>
</file>
<file name="tracecat/workspace_sync/adapters/agent_preset.py">
<violation number="1" location="tracecat/workspace_sync/adapters/agent_preset.py:415">
P2: The `archived_at` predicates added here filter the preset rows being imported via `plan_name_swap`, but the subagent target resolution step (`_resolved_subagent_target()`) still queries `AgentPreset` by workspace and slug without an `archived_at IS NULL` condition. If a parent preset references a slug that belongs to an archived preset, the import could persist the archived child's `preset_id` into the active parent's `agents` config, after which runtime resolution would reject the child as not found. Consider applying the same active-preset predicate to subagent target lookups.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 645dfeb16b
ℹ️ 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".
b4ba2f8 to
8c27e97
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8c27e97d4c
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d25d57d44a
ℹ️ 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".
1ea94df to
af9cb5d
Compare
daryllimyt
changed the title
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: af9cb5d8fe
ℹ️ 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".
af9cb5d to
4a091aa
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4a091aae38
ℹ️ 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".
4a091aa to
44a50a0
Compare
jordan-umusu
left a comment
jordan-umusu
left a comment
There was a problem hiding this comment.
LGTM one known concern
2 participants
Summary
deleted_atvia a new columns-onlySoftDeleteMixin, preserving published version history while hiding deleted rows from active product surfaces.uq_agent_preset_workspace_slug_activeon(workspace_id, slug) WHERE deleted_at IS NULL, so a deleted preset frees its slug immediately.This PR is the root of the ENG-1517 stack (version-less resource refs with HEAD-pointer pinning). It was reworked from the earlier
archived_atnaming so presets never need a rename migration later; the follow-up sync PR #2932 was closed as superseded by ENG-1517 stack 3.Review guide
tracecat/db/models.py:SoftDeleteMixin(columns-only, per repo rule) applied toAgentPreset;Skillkeeps its ownarchived_atcolumn untouched (skills migrate in a later PR).alembic/versions/32b7a1f4d9c2_add_agent_preset_soft_delete.py: revises11d479597e08(reparented after restack), addsdeleted_at, swaps the full slug constraint for the partial index. Downgrade is honest: it recreates the full constraint only after checking no reused slugs would violate it, otherwise raises with an explanation.archived_at → deleted_atrename for presets plus the original hardening logic; no behavior change beyond the rename and index swap.Back-compat / rollback
deleted_at).Validation
uv run ruff check ./uv run ruff format --check .— cleanuv run basedpyright --warnings— 0 errors, 0 warningsuv run pytestfocused suites: 187 passed (preset/skill/channels/folders/activities) + 88 passed (workspace-sync acceptance contract), against a live cluster DB with the migration applieduv run alembic heads— single head32b7a1f4d9c2