fix: global migrations#117
Closed
pedronauck wants to merge 13 commits into
Closed
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (64)
📒 Files selected for processing (181)
WalkthroughThis PR adds a model-catalog subsystem (types, sources, service, merge, redaction), extends ACP with session config-options and reasoning-effort handling, expands API/extension/CLI surfaces for provider model listing/refresh/status, wires the model catalog into daemon boot/runtime and servers, replaces ProviderConfig.DefaultModel with a nested Models block, and inserts/reorders global DB migrations plus tests that validate migration ordering and an append-only migration contract. ChangesSchema Migrations and Validation
(See walkthrough above for full feature summary.) Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
✨ Finishing Touches📝 Generate docstrings
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 (1)
internal/store/globaldb/global_db.go (1)
849-876:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winKeep
memv2_memory_eventsat its shipped version.This reorders recorded history by moving
memv2_memory_eventsbehind new entries. Any database that already applied version 18 asmemv2_memory_eventswill now fail reopen with a migration integrity mismatch when version 18 is checked againstadd_task_review_gate_schema. These new migrations need to be appended after the existing tail, not inserted ahead of it.Suggested fix
{ - Version: 18, - Name: "add_task_review_gate_schema", - Up: migrateTaskReviewGateSchema, - Checksum: "2026-05-05-add-task-review-gate-schema", + Version: 18, + Name: "memv2_memory_events", + Up: migrateMemoryV2Events, + Checksum: "2026-05-05-memv2-memory-events", }, { - Version: 19, - Name: "add_notification_cursors", - Up: migrateNotificationCursors, - Checksum: "2026-05-05-add-notification-cursors", + Version: 19, + Name: "add_task_review_gate_schema", + Up: migrateTaskReviewGateSchema, + Checksum: "2026-05-05-add-task-review-gate-schema", }, { - Version: 20, - Name: "add_bridge_task_subscriptions", - Up: migrateBridgeTaskSubscriptions, - Checksum: "2026-05-05-add-bridge-task-subscriptions", + Version: 20, + Name: "add_notification_cursors", + Up: migrateNotificationCursors, + Checksum: "2026-05-05-add-notification-cursors", }, { - Version: 21, - Name: "rebuild_network_conversation_containers", - Up: migrateNetworkConversationContainers, - Checksum: "2026-05-05-rebuild-network-conversation-containers", + Version: 21, + Name: "add_bridge_task_subscriptions", + Up: migrateBridgeTaskSubscriptions, + Checksum: "2026-05-05-add-bridge-task-subscriptions", }, { - Version: 22, - Name: "memv2_memory_events", - Up: migrateMemoryV2Events, - Checksum: "2026-05-05-memv2-memory-events", + Version: 22, + Name: "rebuild_network_conversation_containers", + Up: migrateNetworkConversationContainers, + Checksum: "2026-05-05-rebuild-network-conversation-containers", },As per coding guidelines: "SQLite migration registries are append-only. Never insert, reorder, rename, renumber, or change an existing migration identity after it may have been applied."
🤖 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 `@internal/store/globaldb/global_db.go` around lines 849 - 876, The migration list has reordered the existing "memv2_memory_events" migration (migrateMemoryV2Events) from its shipped Version 18, causing integrity mismatches; restore memv2_memory_events to its original Version 18 position and append the new migrations (migrateTaskReviewGateSchema, migrateNotificationCursors, migrateBridgeTaskSubscriptions, migrateNetworkConversationContainers) after the current tail so no existing migration identity (name/Version/Checksum) is moved or renumbered; update the Version values for the newly appended migrations so they follow the highest existing Version number instead of inserting before memv2_memory_events.
🤖 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 `@internal/store/globaldb/global_db_test.go`:
- Around line 245-358: The test currently asserts the full migration list
(expectedGlobalMigrationIdentities) equals globalSchemaMigrations which was
reordered in this PR; instead change the test to pin only the historically
shipped prefix and allow new migrations to be appended: update
expectedGlobalMigrationIdentities (or replace it with
expectedGlobalMigrationPrefix) to reflect the last shipped identities (the
original versions/names/checksums that were released), modify
assertGlobalSchemaMigrationDefinitions and assertAppliedGlobalMigrationOrder to
check that the first N entries match that pinned prefix (comparing
Version/Name/Checksum) and that any additional migrations are simply appended
(i.e., lengths may be >= prefix length), and add/adjust tests in
TestGlobalSchemaMigrationsAreAppendOnlyContract to cover fresh DB,
upgrade/reopen, and the recorded-prefix scenario using the pinned prefix helper.
---
Outside diff comments:
In `@internal/store/globaldb/global_db.go`:
- Around line 849-876: The migration list has reordered the existing
"memv2_memory_events" migration (migrateMemoryV2Events) from its shipped Version
18, causing integrity mismatches; restore memv2_memory_events to its original
Version 18 position and append the new migrations (migrateTaskReviewGateSchema,
migrateNotificationCursors, migrateBridgeTaskSubscriptions,
migrateNetworkConversationContainers) after the current tail so no existing
migration identity (name/Version/Checksum) is moved or renumbered; update the
Version values for the newly appended migrations so they follow the highest
existing Version number instead of inserting before memv2_memory_events.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 264a70a4-ca00-45da-8548-48a445739a1c
⛔ Files ignored due to path filters (8)
.codex/ledger/2026-05-06-MEMORY-sqlite-migration-drift.mdis excluded by!**/*.md.codex/plans/2026-05-06-sqlite-migration-append-only.mdis excluded by!**/*.mdAGENTS.mdis excluded by!**/*.mdCLAUDE.mdis excluded by!**/*.mddocs/_memory/lessons/L-021-schema-migration-identity-is-append-only.mdis excluded by!**/*.mddocs/_memory/lessons/README.mdis excluded by!**/*.mdinternal/AGENTS.mdis excluded by!**/*.mdinternal/CLAUDE.mdis excluded by!**/*.md
📒 Files selected for processing (5)
internal/store/globaldb/global_db.gointernal/store/globaldb/global_db_heartbeat_test.gointernal/store/globaldb/global_db_network_conversations_test.gointernal/store/globaldb/global_db_soul_test.gointernal/store/globaldb/global_db_test.go
Comment on lines
+245
to
+358
| func TestGlobalSchemaMigrationsAreAppendOnlyContract(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| t.Run("Should keep known migration identities stable", func(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| assertGlobalSchemaMigrationDefinitions(t, globalSchemaMigrations) | ||
| }) | ||
| } | ||
|
|
||
| type expectedGlobalMigrationIdentity struct { | ||
| version int | ||
| name string | ||
| checksum string | ||
| } | ||
|
|
||
| func expectedGlobalMigrationIdentities() []expectedGlobalMigrationIdentity { | ||
| return []expectedGlobalMigrationIdentity{ | ||
| { | ||
| version: 1, | ||
| name: "create_global_schema", | ||
| checksum: "70e2c16c9d32e692891ab71d075ca823782626e7c9f6ffbbc88c5d662704e089", | ||
| }, | ||
| {version: 2, name: "add_session_failure_diagnostics", checksum: "2026-04-24-add-session-failure-diagnostics"}, | ||
| {version: 3, name: "add_automation_scheduler_state", checksum: "2026-04-24-add-automation-scheduler-state"}, | ||
| {version: 4, name: "add_mcp_auth_tokens", checksum: "2026-04-25-add-mcp-auth-tokens"}, | ||
| {version: 5, name: "add_tool_process_records", checksum: "2026-04-24-add-tool-process-records"}, | ||
| {version: 6, name: "add_memory_operation_scope", checksum: "2026-04-25-add-memory-operation-scope"}, | ||
| {version: 7, name: "add_task_run_claim_lease_schema", checksum: "2026-04-26-add-task-run-claim-lease-schema"}, | ||
| {version: 8, name: "add_session_lineage_metadata", checksum: "2026-04-26-add-session-lineage-metadata"}, | ||
| { | ||
| version: 9, | ||
| name: "rename_environment_columns_to_sandbox", | ||
| checksum: "2026-04-28-rename-environment-columns-to-sandbox", | ||
| }, | ||
| {version: 10, name: "add_vault_secrets", checksum: "2026-05-01-add-vault-secrets"}, | ||
| {version: 11, name: "unify_secret_refs", checksum: "2026-05-01-unify-secret-refs"}, | ||
| {version: 12, name: "add_agent_soul_snapshots", checksum: "2026-05-02-add-agent-soul-snapshots"}, | ||
| {version: 13, name: "add_agent_heartbeat_storage", checksum: "2026-05-02-add-agent-heartbeat-storage"}, | ||
| {version: 14, name: "add_event_summary_lineage", checksum: "2026-05-04-add-event-summary-lineage"}, | ||
| { | ||
| version: 15, | ||
| name: "rebuild_event_summaries_for_global_payloads", | ||
| checksum: "2026-05-04-rebuild-event-summaries-for-global-payloads", | ||
| }, | ||
| { | ||
| version: 16, | ||
| name: "rename_actor_ref_columns_to_actor_id", | ||
| checksum: "2026-05-04-rename-actor-ref-columns-to-actor-id", | ||
| }, | ||
| { | ||
| version: 17, | ||
| name: "add_task_orchestration_profile_schema", | ||
| checksum: "2026-05-05-add-task-orchestration-profile-schema", | ||
| }, | ||
| {version: 18, name: "add_task_review_gate_schema", checksum: "2026-05-05-add-task-review-gate-schema"}, | ||
| {version: 19, name: "add_notification_cursors", checksum: "2026-05-05-add-notification-cursors"}, | ||
| {version: 20, name: "add_bridge_task_subscriptions", checksum: "2026-05-05-add-bridge-task-subscriptions"}, | ||
| { | ||
| version: 21, | ||
| name: "rebuild_network_conversation_containers", | ||
| checksum: "2026-05-05-rebuild-network-conversation-containers", | ||
| }, | ||
| {version: 22, name: "memv2_memory_events", checksum: "2026-05-05-memv2-memory-events"}, | ||
| } | ||
| } | ||
|
|
||
| func assertGlobalSchemaMigrationDefinitions(t *testing.T, migrations []store.Migration) { | ||
| t.Helper() | ||
|
|
||
| want := expectedGlobalMigrationIdentities() | ||
| if got := len(migrations); got != len(want) { | ||
| t.Fatalf("globalSchemaMigrations length = %d, want %d", got, len(want)) | ||
| } | ||
| for index, expected := range want { | ||
| got := migrations[index] | ||
| if got.Version != expected.version || got.Name != expected.name || got.Checksum != expected.checksum { | ||
| t.Fatalf( | ||
| "globalSchemaMigrations[%d] = version %d name %q checksum %q, want version %d name %q checksum %q", | ||
| index, | ||
| got.Version, | ||
| got.Name, | ||
| got.Checksum, | ||
| expected.version, | ||
| expected.name, | ||
| expected.checksum, | ||
| ) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| func assertAppliedGlobalMigrationOrder(t *testing.T, records []store.MigrationRecord) { | ||
| t.Helper() | ||
|
|
||
| want := expectedGlobalMigrationIdentities() | ||
| if got := len(records); got != len(want) { | ||
| t.Fatalf("schema_migrations length = %d, want %d", got, len(want)) | ||
| } | ||
| for index, expected := range want { | ||
| got := records[index] | ||
| if got.Version != expected.version || got.Name != expected.name || got.Checksum != expected.checksum { | ||
| t.Fatalf( | ||
| "schema_migrations[%d] = version %d name %q checksum %q, want version %d name %q checksum %q", | ||
| index, | ||
| got.Version, | ||
| got.Name, | ||
| got.Checksum, | ||
| expected.version, | ||
| expected.name, | ||
| expected.checksum, | ||
| ) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Make this contract reflect the last shipped migration history, not the reordered list in this PR.
As written, the new expected identities bless version 18 as add_task_review_gate_schema and version 22 as memv2_memory_events, so the suite passes on fresh databases while missing the real upgrade break for databases that already recorded the old version 18 identity. This contract should pin the historical prefix that was already shipped, then assert that any new entries are appended after it.
As per coding guidelines: "Migration drift fixes require observed-history tests. Cover fresh DB, upgrade/reopen, and the real recorded migration prefix that failed."
🤖 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 `@internal/store/globaldb/global_db_test.go` around lines 245 - 358, The test
currently asserts the full migration list (expectedGlobalMigrationIdentities)
equals globalSchemaMigrations which was reordered in this PR; instead change the
test to pin only the historically shipped prefix and allow new migrations to be
appended: update expectedGlobalMigrationIdentities (or replace it with
expectedGlobalMigrationPrefix) to reflect the last shipped identities (the
original versions/names/checksums that were released), modify
assertGlobalSchemaMigrationDefinitions and assertAppliedGlobalMigrationOrder to
check that the first N entries match that pinned prefix (comparing
Version/Name/Checksum) and that any additional migrations are simply appended
(i.e., lengths may be >= prefix length), and add/adjust tests in
TestGlobalSchemaMigrationsAreAppendOnlyContract to cover fresh DB,
upgrade/reopen, and the recorded-prefix scenario using the pinned prefix helper.
Adds a new web/src/systems/model-catalog/ system (adapters, query keys/options, hooks, derive helper) and consumes daemon catalog rows from the new-session dialog and Settings > Providers, with stale, error, refresh, and ACP config-option precedence handled centrally. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Removes flat default_model/supported_models/supports_reasoning_effort claims from provider/config/agent docs and documents the daemon-owned model catalog (native HTTP/UDS endpoints, /api/openai/v1/models projection, refresh lifetime/coalescing rules, extension model.source contract) plus the new model_catalog and provider models.discovery config sections. Regenerates the provider models CLI reference and ride-along cli-reference reformat output from make codegen + make cli-docs. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds the release-grade QA artifacts for the provider model catalog program (Tasks 01-11) under .compozy/tasks/provider-model-catalog/qa/: coverage matrix mapping every TechSpec safety invariant, ADR, task, public surface and failure mode to concrete cases; master test plan with charter, environment, entry/exit criteria, scenario contract and verification commands; tiered regression suite; 33 concrete test cases (SMOKE-001 + TC-FUNC-001..015 + TC-INT-001..006 + TC-PERF-001..002 + TC-SEC-001..002 + TC-UI-001..003 + TC-REG-001..002 + TC-SCEN-001..002); bug template; verification report template. Task 13 can execute the plan from an isolated agh-qa-bootstrap lab without inventing scenarios. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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 by CodeRabbit
New Features
Tests