fix: deleted seed profiles, models, and cleared automation stay deleted#3573
Conversation
Every seeding pass is idempotent by presence: seedDefaults writes a gated-in template whenever its row is missing, and backfillNewModels recreates any known model a configured provider lacks. Both run at startup and again after a provider is created or updated, and deletes are hard deletes with no record — so deleting a bundled default undid itself, often within the same session. SeedTombstoneStore records what the user deleted in one settings row, and both passes skip those identities. Model identity is provider-scoped (provider:providerModelId) rather than row-scoped, because backfill matches on providerModelId under a provider and the row id may be deterministic or a UUID depending on whether it came from FTUE, manual setup, or sync. Only user-initiated deletions record a tombstone. deleteConfig takes recordTombstone (default true); removeOrphanedDefaultSeeds and the model rows removed by a provider cascade both pass false, because both want their rows back — the orphan pass re-seeds when the provider becomes usable again, and re-adding a provider must bring its models with it. Also removes the skill-assignment backfill in upgradeExisting. Its guard was skillAssignments.isEmpty, so clearing every assignment — the obvious way to say 'stop doing things automatically' — was exactly what restored them with automate: true on the next launch. Automation defaults are now written only when a profile is first seeded.
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis change adds persistent tombstones for deleted bundled AI profiles and models, prevents default skill assignments from being restored after clearing, wires tombstone storage through initialization paths, updates tests and documentation, and releases version 0.9.1067. ChangesAI seed persistence
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant AiConfigRepository
participant SeedTombstoneStore
participant ProfileSeedingService
User->>AiConfigRepository: delete bundled profile or model
AiConfigRepository->>SeedTombstoneStore: remember seed identity
ProfileSeedingService->>SeedTombstoneStore: deletedIdentities()
SeedTombstoneStore-->>ProfileSeedingService: tombstoned identities
ProfileSeedingService->>ProfileSeedingService: skip deleted seeds
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
lib/features/ai/util/model_prepopulation_service.dart (1)
69-84: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument the tombstone skip in the method docstring.
The docstring above (steps 1–4) still describes the pre-tombstone behavior; a reader can't tell that user-deleted models are permanently skipped here.
♻️ Suggested docstring addition
/// This method: /// 1. Looks up known models for the provider type + /// 2. Skips models the user deleted (recorded in [SeedTombstoneStore]) - /// 2. Generates unique IDs for each model - /// 3. Checks if usable rows with those provider-native model IDs exist - /// 4. Creates only the models that don't exist yet + /// 3. Generates unique IDs for each remaining model + /// 4. Checks if usable rows with those provider-native model IDs exist + /// 5. Creates only the models that don't exist yetAs per coding guidelines: "Whenever touching a function, consider whether its docstring needs updating."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/features/ai/util/model_prepopulation_service.dart` around lines 69 - 84, Update the method docstring governing the model prepopulation flow to document that models marked as deleted in the tombstone store are skipped and will not be recreated on later launches. Keep the existing steps and behavior unchanged, and reference the tombstone check used by the prepopulation method.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/features/ai/repository/ai_config_repository.dart`:
- Around line 83-90: Make deletion, tombstone updates, and profile seeding
mutually atomic for each identity. In
lib/features/ai/repository/ai_config_repository.dart:83-90, coordinate the
delete, _rememberSeedTombstone, and seed operation so a concurrent seed cannot
recreate a deleted config. In
lib/features/ai/util/seed_tombstone_store.dart:61-72, make remember and forget
atomic across store instances using a transaction or shared keyed coordinator,
preserving concurrent identities. In
lib/features/ai/util/profile_seeding_service.dart:115-141, perform the tombstone
check and seed write under the same coordination, and add a regression test
covering concurrent deletion and seeding.
In `@test/features/ai/test_utils.dart`:
- Around line 442-448: Update the helper that creates the SeedTombstoneStore to
retain the in-memory SettingsDb instance and register a test teardown that
closes it before returning the store. Preserve the existing identity seeding
behavior while ensuring each setUp-created database is released after the test.
---
Nitpick comments:
In `@lib/features/ai/util/model_prepopulation_service.dart`:
- Around line 69-84: Update the method docstring governing the model
prepopulation flow to document that models marked as deleted in the tombstone
store are skipped and will not be recreated on later launches. Keep the existing
steps and behavior unchanged, and reference the tombstone check used by the
prepopulation method.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: a2d9c187-8b25-41c5-b64a-e854e83824c7
📒 Files selected for processing (21)
CHANGELOG.mdflatpak/com.matthiasn.lotti.metainfo.xmllib/features/agents/state/agent_providers.dartlib/features/ai/README.mdlib/features/ai/repository/ai_config_repository.dartlib/features/ai/state/ai_config_initialization.dartlib/features/ai/state/settings/inference_provider_form_controller.dartlib/features/ai/ui/settings/ai_settings_page.dartlib/features/ai/ui/settings/inference_provider_edit_page.dartlib/features/ai/util/model_prepopulation_service.dartlib/features/ai/util/profile_seeding_service.dartlib/features/ai/util/seed_tombstone_store.dartpubspec.yamltest/features/ai/repository/ai_config_repository_test.darttest/features/ai/state/ai_config_initialization_test.darttest/features/ai/state/settings/inference_provider_form_controller_test.darttest/features/ai/test_utils.darttest/features/ai/util/model_prepopulation_service_test.darttest/features/ai/util/profile_seeding_service_slots_test.darttest/features/ai/util/profile_seeding_service_test.darttest/features/ai/util/seed_tombstone_store_test.dart
💤 Files with no reviewable changes (1)
- lib/features/ai/ui/settings/ai_settings_page.dart
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3573 +/- ##
==========================================
- Coverage 99.22% 99.14% -0.08%
==========================================
Files 1781 1782 +1
Lines 130336 130408 +72
==========================================
- Hits 129322 129296 -26
- Misses 1014 1112 +98
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Review follow-up on two ordering hazards. Concurrent ledger mutations could lose an identity: the repository builds a fresh store per deletion and a synced delete can land while a user delete is in flight, so both read the same prior set and the second write drops the first's identity — silently reviving that seed. All mutations now run through one static queue shared by every store instance in the isolate, and a failed write no longer wedges the writes queued behind it. A deletion landing after a seeding pass read the ledger but before it wrote the row left the row resurrected with a tombstone recorded, and nothing would clean it up because seeding only ever creates. The row and the tombstone live in different databases, so no transaction can span them; instead seedDefaults and prepopulateModelsForProvider re-read the ledger after writing and drop anything tombstoned while they ran, restoring the invariant at the end of each pass. The re-read is skipped when the pass wrote nothing, which is the common case. Also closes the in-memory settings database the test helper opened per setUp.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/features/ai/util/profile_seeding_service.dart (1)
251-266: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
upgradeExistingdocstring still describes the removed skill-assignment backfill.The doc comment says the method "then backfills default skill assignments only for default profiles whose assignments are empty," but lines 313-318 now explicitly remove that behavior — the PR's whole point is that clearing assignments must not be restored. Leaving the stale docstring risks a future contributor "fixing" the missing backfill and reintroducing this exact bug.
📝 Proposed docstring fix
/// old Local Power seed from Ollama to oMLX, migrates untouched Melious image /// generation and transcription to the Flux 2 Klein 9B and Whisper Large v3 - /// defaults, moves untouched Melious profiles to Qwen thinking, GLM 5.2 - /// high-end, and Voxtral transcription defaults, then backfills default - /// skill assignments only for default profiles whose assignments are empty. + /// defaults, and moves untouched Melious profiles to Qwen thinking, GLM 5.2 + /// high-end, and Voxtral transcription defaults. Default skill assignments + /// are deliberately never backfilled here — only seeding a profile at + /// creation writes automation defaults.As per path instructions: "Whenever touching a function, consider whether its docstring needs updating."
Also applies to: 313-318
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/features/ai/util/profile_seeding_service.dart` around lines 251 - 266, Update the doc comment for upgradeExisting to remove the statement that it backfills default skill assignments for profiles with empty assignments. Keep the remaining descriptions of profile healing and migration behavior accurate, without reintroducing or documenting skill-assignment restoration.Source: Path instructions
🧹 Nitpick comments (1)
test/features/ai/util/profile_seeding_service_test.dart (1)
129-142: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the repeated "no deletion happened" mock verification.
The same
verifyNever(() => mockRepo.deleteConfig(any(), fromSync: any(named: 'fromSync'), recordTombstone: any(named: 'recordTombstone')))block is copy-pasted across six tests. A shared helper would reduce duplication and centralize the matcher if thedeleteConfigsignature changes again.♻️ Proposed helper extraction
+ void verifyNoDeletion() { + verifyNever( + () => mockRepo.deleteConfig( + any(), + fromSync: any(named: 'fromSync'), + recordTombstone: any(named: 'recordTombstone'), + ), + ); + } + test( 'leaves profiles alone when nothing was tombstoned mid-pass', () async { await service.seedDefaults(); - verifyNever( - () => mockRepo.deleteConfig( - any(), - fromSync: any(named: 'fromSync'), - recordTombstone: any(named: 'recordTombstone'), - ), - ); + verifyNoDeletion(); }, );Repeat the
verifyNoDeletion()call at the other five sites (713-719, 738-744, 780-786, 836-842, 864-870).As per path instructions: "Extract shared widget pump/setup helpers, repeated mock stubs, and complex state into helpers or test-bench classes; pass varying values as named parameters."
Also applies to: 713-719, 738-744, 780-786, 836-842, 864-870
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/features/ai/util/profile_seeding_service_test.dart` around lines 129 - 142, Extract the repeated verifyNever deleteConfig matcher into a shared verifyNoDeletion() helper near the test setup, then replace the inline verification blocks in all six affected tests with calls to that helper. Preserve the existing any(), fromSync, and recordTombstone matching behavior.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@lib/features/ai/util/profile_seeding_service.dart`:
- Around line 251-266: Update the doc comment for upgradeExisting to remove the
statement that it backfills default skill assignments for profiles with empty
assignments. Keep the remaining descriptions of profile healing and migration
behavior accurate, without reintroducing or documenting skill-assignment
restoration.
---
Nitpick comments:
In `@test/features/ai/util/profile_seeding_service_test.dart`:
- Around line 129-142: Extract the repeated verifyNever deleteConfig matcher
into a shared verifyNoDeletion() helper near the test setup, then replace the
inline verification blocks in all six affected tests with calls to that helper.
Preserve the existing any(), fromSync, and recordTombstone matching behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 4c26c615-ee10-4345-a0ca-f351c01007eb
📒 Files selected for processing (7)
lib/features/ai/README.mdlib/features/ai/util/model_prepopulation_service.dartlib/features/ai/util/profile_seeding_service.dartlib/features/ai/util/seed_tombstone_store.darttest/features/ai/test_utils.darttest/features/ai/util/profile_seeding_service_test.darttest/features/ai/util/seed_tombstone_store_test.dart
🚧 Files skipped from review as they are similar to previous changes (3)
- test/features/ai/test_utils.dart
- lib/features/ai/util/model_prepopulation_service.dart
- lib/features/ai/README.md
The profile-seeding race had a regression test; its model-backfill mirror did not, which Codecov caught as three uncovered lines. Adds the equivalent test and lifts the late-tombstone store double into the shared AI test utils so both suites use one implementation.
Follow-up to the seed tombstones merged in #3573, which introduced this gap. Onboarding binds each area to a well-known seeded profile id unconditionally. Once deletions are remembered, a user who had deleted that bundled profile and later re-ran onboarding got a category pointing at a row seeding now refuses to recreate: _resolveModel returns noModel and the first task degrades to title-only. Onboarding is re-runnable from the maintenance page and the onboarding settings panel, so this is reachable rather than theoretical. The key step now clears that provider's tombstone before running the FTUE setup, so the seeding it triggers can recreate the profile. Deliberately setting the provider up again is the 'bring it back' signal the store's forget was built for; every other path still honors the deletion. Exposes the store as seedTombstoneStoreProvider rather than reaching into getIt from a widget, which also lets tests substitute one. Also fixes a defect in the tombstone serialization from that same PR: the mutation queue was a single static, so it was shared across every database in the isolate. In tests one stalled mutation against a discarded database wedged every later write, and the onboarding flow hung mid-connect. The queue is now keyed by settings database, which is both what the serialization actually needs and test-safe.
Supersedes the SeedTombstoneStore from #3573. Deleting an AI config now stamps deletedAt on the row and re-saves it, so the row itself is the tombstone. The ledger was the wrong shape. It lived in settings.sqlite while the config lives in ai_config.sqlite, so no transaction could span a deletion and a seeding check — the race CodeRabbit raised, which I could only mitigate with a post-pass reconciliation. It was local state, so a device that never observed the delete could re-seed and sync the row back. And two stores writing the same row could lose an identity, which needed a serialization queue that itself shipped with a cross-database bug. The field costs neither of the things I assumed ruled it out. ai_configs stores the whole config in a serialized TEXT column, so a new freezed field lands in that JSON and schemaVersion stays 1. SyncMessage.aiConfig already carries the whole config, so the deletion replicates on the existing path. It also matches how the journal domain deletes synced entities (CategoryRepository). Reads split by intent: getConfigById/getConfigsByType hide deleted rows by default, the seeding passes ask for them so a deleted row reads as present and blocks re-creation, and watchConfigsByType always filters. Two paths keep hard-deleting through hardDeleteConfig — removeOrphanedDefaultSeeds and the provider cascade — because both want their rows back later. Deletes SeedTombstoneStore, its test, seedTombstoneStoreProvider, the settings key, recordTombstone, the mutation queue, and the mid-pass reconciliation in both seeding passes. Also makes the delete service's undo explicit: it re-saved a pre-delete snapshot, which happened to clear the stamp but would silently revert any change made during the undo window. It now calls restoreConfig.
Supersedes the SeedTombstoneStore from #3573. Deleting an AI config now stamps deletedAt on the row and re-saves it, so the row itself is the tombstone. The ledger was the wrong shape. It lived in settings.sqlite while the config lives in ai_config.sqlite, so no transaction could span a deletion and a seeding check — the race CodeRabbit raised, which I could only mitigate with a post-pass reconciliation. It was local state, so a device that never observed the delete could re-seed and sync the row back. And two stores writing the same row could lose an identity, which needed a serialization queue that itself shipped with a cross-database bug. The field costs neither of the things I assumed ruled it out. ai_configs stores the whole config in a serialized TEXT column, so a new freezed field lands in that JSON and schemaVersion stays 1. SyncMessage.aiConfig already carries the whole config, so the deletion replicates on the existing path. It also matches how the journal domain deletes synced entities (CategoryRepository). Reads split by intent: getConfigById/getConfigsByType hide deleted rows by default, the seeding passes ask for them so a deleted row reads as present and blocks re-creation, and watchConfigsByType always filters. Two paths keep hard-deleting through hardDeleteConfig — removeOrphanedDefaultSeeds and the provider cascade — because both want their rows back later. Deletes SeedTombstoneStore, its test, seedTombstoneStoreProvider, the settings key, recordTombstone, the mutation queue, and the mid-pass reconciliation in both seeding passes. Also makes the delete service's undo explicit: it re-saved a pre-delete snapshot, which happened to clear the stamp but would silently revert any change made during the undo window. It now calls restoreConfig.
Summary
Final piece of the inference-profile audit (after #3568 and #3570): seeded data no longer revives after the user deletes it.
SeedTombstoneStore— one settings row recording deleted seed identitiesseedDefaults()andprepopulateModelsForProvider()skip tombstoned identitiesAiConfigRepository.deleteConfiggainsrecordTombstone(default true) and writes the recordupgradeExisting()skill-assignment backfillWhy
Every seeding pass is idempotent by presence, and deletes are hard deletes with no record — so deletion had no memory:
seedDefaults()backfillNewModels()upgradeExisting()skill backfillautomate: trueThat last one is the worst of the three: the guard was
skillAssignments.isEmpty, so clearing every assignment — the obvious way to say "stop doing things automatically" — was exactly what triggered restoring them. It's deleted outright; automation defaults are now written only when a profile is first seeded.Two details that decide whether this works
Model identity is provider-scoped, not row-scoped. Backfill matches on
providerModelIdunder a provider, and the row id may be deterministic or a UUID depending on whether it came from FTUE, manual setup, or sync. A tombstone keyed by row id would miss the recreated row, somodelKeykeys onprovider:providerModelId— the identity backfill actually compares. Pinned by a test.Only user-initiated deletions are recorded. Two paths delete rows the app wants back:
removeOrphanedDefaultSeeds()re-seeds when a provider becomes usable again, and a provider cascade must return its models when you re-add that provider. Both passrecordTombstone: false. Getting this backwards would mean re-adding a provider yields no models — also pinned by tests.Sync
Tombstones are local state rather than a synced entity, but a peer applying a synced delete records its own, because deletes arrive through the same repository path. Residual gap, stated plainly: a device that never observes the delete event at all may re-seed and sync the row back. Closing that fully means a synced tombstone entity — a schema migration plus a new sync message type — which I did not want to bundle into this change. Documented in the store's doc comment and the AI README.
Cost
deleteConfignow reads the row before deleting it, to build the identity (a model's provider-native id only exists on the row). One extra read on an interactive, rare action; skipped entirely whenrecordTombstone: false.Verification
fvm flutter analyze lib test— no issues foundfvm flutter test test/features/ai/ test/features/agents/— 9410 passedseed_tombstone_store.dartat 100% line coverage (verified againstcoverage/lcov.info)Three existing tests in
profile_seeding_service_slots_test.dartasserted the removed backfill. Two were rewritten to assert the opposite contract (cleared assignments stay cleared; ambiguous slots are left alone and nothing is written), and the third — which only covered sanitising of backfilled assignments — was deleted along with the code path.hasSlotForSkillTypekeeps its own matrix test and is still used by the oMLX transcription upgrade.Audit complete
This closes
lotti3-8f7. All four findings are now shipped or tracked: the consent gate and auto-wake (#3568), the in-use badge (#3570), and seed revival (here).Summary by CodeRabbit
Bug Fixes
Documentation
Release & Packaging