From f8233000cc26886c4fe2285fae4d49bac91bbfd0 Mon Sep 17 00:00:00 2001 From: CoreRasurae Date: Thu, 26 Mar 2026 23:37:11 +0000 Subject: [PATCH] fix(db): add missing link_type, FK constraints, and partial index per spec DDL Align resource_links, checkpoint_metadata, and decisions schema behavior with the spec DDL. Add SQLite runtime trigger guards for checkpoint foreign-key semantics and migration-focused Behave/Robot checks that orphan checkpoint references are rejected. ISSUES CLOSED: #922 --- features/db_migration_lifecycle.feature | 88 ++++ .../steps/db_migration_lifecycle_steps.py | 5 +- features/steps/db_schema_cascade_steps.py | 457 ++++++++++++++++++ features/steps/db_schema_link_type_steps.py | 432 +++++++++++++++++ features/steps/db_schema_parity_steps.py | 427 ++++++++++++++++ robot/helper_schema_parity_migration.py | 443 +++++++++++++++++ robot/schema_parity_migration.robot | 36 ++ ...ema_parity_resource_decision_checkpoint.py | 318 ++++++++++++ ...02_merge_profile_rename_and_corrections.py | 14 +- .../infrastructure/database/models.py | 47 +- 10 files changed, 2254 insertions(+), 13 deletions(-) create mode 100644 features/steps/db_schema_cascade_steps.py create mode 100644 features/steps/db_schema_link_type_steps.py create mode 100644 features/steps/db_schema_parity_steps.py create mode 100644 robot/helper_schema_parity_migration.py create mode 100644 robot/schema_parity_migration.robot create mode 100644 src/cleveragents/infrastructure/database/migrations/versions/m4_004_schema_parity_resource_decision_checkpoint.py diff --git a/features/db_migration_lifecycle.feature b/features/db_migration_lifecycle.feature index 9c0bc6ed7..4cbe9a9a9 100644 --- a/features/db_migration_lifecycle.feature +++ b/features/db_migration_lifecycle.feature @@ -11,6 +11,16 @@ Feature: Database migration lifecycle Then the database should have all expected tables And the current revision should be the head revision + Scenario: Spec-parity schema details are present after migration + Given all migrations have been applied + Then the "resource_links" table should include "link_type" with default "contains" + And checkpoint_metadata should enforce decision and resource foreign keys + And checkpoint_metadata foreign keys should reject orphan references + And checkpoint_metadata should reject orphan decision_id independently + And checkpoint_metadata should reject orphan resource_id independently + And checkpoint_metadata should accept valid decision and resource references + And the "decisions" table should have partial index "idx_decisions_superseded" on "superseded_by" + Scenario: Roll back all migrations to base Given all migrations have been applied When I downgrade all migrations to base @@ -45,6 +55,84 @@ Feature: Database migration lifecycle Then the database should be stamped with alembic_version And the database revision should be at head + Scenario: Migration orphan cleanup nullifies stale checkpoint references + Given migrations applied up to "m4_003_plan_env_columns" + And checkpoint_metadata contains orphan decision and resource references + When I upgrade to the next migration revision + Then the orphan decision_id values should be NULL + And the orphan resource_id values should be NULL + And checkpoint_metadata should enforce decision and resource foreign keys + + Scenario: resource_links.link_type accepts spec-defined non-default values + Given all migrations have been applied + Then resource_links should accept link_type "references" + And resource_links should accept link_type "derived_from" + + Scenario: resource_links.link_type rejects NULL values + Given all migrations have been applied + Then resource_links should reject NULL link_type + + Scenario: resource_links.link_type rejects invalid values + Given all migrations have been applied + Then resource_links should reject link_type "invalid_type" + + Scenario: Deleting a decision sets checkpoint decision_id to NULL + Given all migrations have been applied + And a checkpoint references a valid decision + When the referenced decision is deleted + Then the checkpoint decision_id should be NULL + + Scenario: Deleting a resource sets checkpoint resource_id to NULL + Given all migrations have been applied + And a checkpoint references a valid resource + When the referenced resource is deleted + Then the checkpoint resource_id should be NULL + + Scenario: Spec-parity m4_004 downgrade removes added schema objects + Given all migrations have been applied + When I downgrade to revision "m4_003_plan_env_columns" + Then the "resource_links" table should not include "link_type" + And the "decisions" table should not have index "idx_decisions_superseded" + And checkpoint_metadata should not have foreign key "fk_checkpoint_metadata_decision" + And checkpoint_metadata should not have foreign key "fk_checkpoint_metadata_resource" + And checkpoint_metadata should not have SQLite triggers for FK enforcement + + Scenario: Migration idempotency: existing link_type column gains CHECK constraint + Given migrations applied up to "m4_003_plan_env_columns" + And resource_links already has a link_type column without CHECK constraint + When I upgrade to the next migration revision + Then the "resource_links" table should include "link_type" with default "contains" + And resource_links should reject link_type "invalid_type" + + Scenario: Migration idempotency: existing link_type column with wrong default is corrected + Given migrations applied up to "m4_003_plan_env_columns" + And resource_links already has a link_type column with a non-contains default + When I upgrade to the next migration revision + Then the "resource_links" table should include "link_type" with default "contains" + And resource_links should reject link_type "invalid_type" + + Scenario: Positive checkpoint FK test verifies persisted row + Given all migrations have been applied + And valid decision and resource rows exist + When I insert a checkpoint with valid FK references + Then the checkpoint row should be persisted in the database + + Scenario: UPDATE trigger rejects orphan decision_id on checkpoint_metadata + Given all migrations have been applied + And a checkpoint exists with valid FK references + When I update the checkpoint decision_id to a non-existent value + Then the update should be rejected with an integrity error + + Scenario: UPDATE trigger rejects orphan resource_id on checkpoint_metadata + Given all migrations have been applied + And a checkpoint exists with valid FK references + When I update the checkpoint resource_id to a non-existent value + Then the update should be rejected with an integrity error + + Scenario: resource_links.link_type rejects empty string values + Given all migrations have been applied + Then resource_links should reject empty string link_type + Scenario: init_database creates a valid schema When I call init_database with an in-memory URL Then the resulting engine should have a valid schema diff --git a/features/steps/db_migration_lifecycle_steps.py b/features/steps/db_migration_lifecycle_steps.py index fcac32e12..c8dccce7d 100644 --- a/features/steps/db_migration_lifecycle_steps.py +++ b/features/steps/db_migration_lifecycle_steps.py @@ -1,4 +1,4 @@ -"""Step definitions for db_migration_lifecycle.feature. +"""Step definitions for db_migration_lifecycle.feature — core lifecycle. Exercises the full Alembic migration lifecycle: forward application, rollback, round-trip testing, CLI wrappers, stamp logic, and the @@ -13,6 +13,7 @@ from typing import Any from unittest.mock import patch from behave import given, then, when +from sqlalchemy import inspect as sa_inspect def _create_temp_db_path() -> str: @@ -75,8 +76,6 @@ _EXPECTED_TABLES = { def _get_table_names(engine: Any) -> set[str]: - from sqlalchemy import inspect as sa_inspect - inspector = sa_inspect(engine) return set(inspector.get_table_names()) diff --git a/features/steps/db_schema_cascade_steps.py b/features/steps/db_schema_cascade_steps.py new file mode 100644 index 000000000..6778d18bc --- /dev/null +++ b/features/steps/db_schema_cascade_steps.py @@ -0,0 +1,457 @@ +"""Step definitions for db_migration_lifecycle.feature — cascade and persistence. + +``ondelete="SET NULL"`` cascade verification for checkpoint_metadata foreign +keys, positive FK persistence tests, and trigger removal verification on +downgrade. +""" + +from __future__ import annotations + +from typing import Any + +from behave import given, then, when +from sqlalchemy import text +from sqlalchemy.exc import IntegrityError + +from features.steps.db_schema_parity_steps import ( + _ensure_test_action_and_plan, + _insert_test_resource, +) + +# --------------------------------------------------------------------------- +# Given/When/Then — positive FK persistence test +# --------------------------------------------------------------------------- + + +@given("valid decision and resource rows exist") +def step_valid_decision_and_resource_exist(context: Any) -> None: + action_name = "local/test-action-persist" + plan_id = "01ARZ3NDEKTSV4RRFFQ69G5FE0" + decision_id = "01ARZ3NDEKTSV4RRFFQ69G5FE1" + resource_id = "01ARZ3NDEKTSV4RRFFQ69G5FE2" + + with context.engine.begin() as conn: + _ensure_test_action_and_plan(conn, action_name, plan_id) + + conn.execute( + text( + """ + INSERT INTO decisions ( + decision_id, plan_id, decision_type, question, + chosen_option, context_snapshot_json, sequence_number, + created_at + ) VALUES ( + :decision_id, :plan_id, :decision_type, :question, + :chosen_option, :context_snapshot_json, :sequence_number, + :created_at + ) + """ + ), + { + "decision_id": decision_id, + "plan_id": plan_id, + "decision_type": "strategy_choice", + "question": "test question", + "chosen_option": "test option", + "context_snapshot_json": "{}", + "sequence_number": 1, + "created_at": "2026-01-01T00:00:00", + }, + ) + + _insert_test_resource(conn, resource_id) + + context.persist_plan_id = plan_id + context.persist_decision_id = decision_id + context.persist_resource_id = resource_id + context.persist_checkpoint_id = "01ARZ3NDEKTSV4RRFFQ69G5FE3" + + +@when("I insert a checkpoint with valid FK references") +def step_insert_checkpoint_valid_fk(context: Any) -> None: + with context.engine.begin() as conn: + conn.execute( + text( + """ + INSERT INTO checkpoint_metadata ( + checkpoint_id, plan_id, decision_id, + checkpoint_type, resource_id, sandbox_ref, + filesystem_path, created_at + ) VALUES ( + :checkpoint_id, :plan_id, :decision_id, + :checkpoint_type, :resource_id, :sandbox_ref, + :filesystem_path, :created_at + ) + """ + ), + { + "checkpoint_id": context.persist_checkpoint_id, + "plan_id": context.persist_plan_id, + "decision_id": context.persist_decision_id, + "checkpoint_type": "manual", + "resource_id": context.persist_resource_id, + "sandbox_ref": "test-ref", + "filesystem_path": "", + "created_at": "2026-01-01T00:00:00", + }, + ) + + +@then("the checkpoint row should be persisted in the database") +def step_checkpoint_persisted(context: Any) -> None: + with context.engine.connect() as conn: + row = conn.execute( + text( + "SELECT checkpoint_id, decision_id, resource_id " + "FROM checkpoint_metadata WHERE checkpoint_id = :cid" + ), + {"cid": context.persist_checkpoint_id}, + ).fetchone() + + assert row is not None, ( + f"Checkpoint {context.persist_checkpoint_id!r} was not persisted" + ) + assert row[0] == context.persist_checkpoint_id + assert row[1] == context.persist_decision_id, ( + f"Expected decision_id {context.persist_decision_id!r}, got {row[1]!r}" + ) + assert row[2] == context.persist_resource_id, ( + f"Expected resource_id {context.persist_resource_id!r}, got {row[2]!r}" + ) + + +# --------------------------------------------------------------------------- +# Given/When/Then — UPDATE trigger rejection +# --------------------------------------------------------------------------- + + +@given("a checkpoint exists with valid FK references") +def step_checkpoint_with_valid_fk_refs(context: Any) -> None: + action_name = "local/test-action-upd-trg" + plan_id = "01ARZ3NDEKTSV4RRFFQ69G5FH0" + decision_id = "01ARZ3NDEKTSV4RRFFQ69G5FH1" + resource_id = "01ARZ3NDEKTSV4RRFFQ69G5FH2" + checkpoint_id = "01ARZ3NDEKTSV4RRFFQ69G5FH3" + + with context.engine.begin() as conn: + _ensure_test_action_and_plan(conn, action_name, plan_id) + + conn.execute( + text( + """ + INSERT INTO decisions ( + decision_id, plan_id, decision_type, question, + chosen_option, context_snapshot_json, sequence_number, + created_at + ) VALUES ( + :decision_id, :plan_id, :decision_type, :question, + :chosen_option, :context_snapshot_json, :sequence_number, + :created_at + ) + """ + ), + { + "decision_id": decision_id, + "plan_id": plan_id, + "decision_type": "strategy_choice", + "question": "test question", + "chosen_option": "test option", + "context_snapshot_json": "{}", + "sequence_number": 1, + "created_at": "2026-01-01T00:00:00", + }, + ) + + _insert_test_resource(conn, resource_id) + + conn.execute( + text( + """ + INSERT INTO checkpoint_metadata ( + checkpoint_id, plan_id, decision_id, + checkpoint_type, resource_id, sandbox_ref, + filesystem_path, created_at + ) VALUES ( + :checkpoint_id, :plan_id, :decision_id, + :checkpoint_type, :resource_id, :sandbox_ref, + :filesystem_path, :created_at + ) + """ + ), + { + "checkpoint_id": checkpoint_id, + "plan_id": plan_id, + "decision_id": decision_id, + "checkpoint_type": "manual", + "resource_id": resource_id, + "sandbox_ref": "test-ref", + "filesystem_path": "", + "created_at": "2026-01-01T00:00:00", + }, + ) + + context.update_trg_checkpoint_id = checkpoint_id + + +@when("I update the checkpoint decision_id to a non-existent value") +def step_update_checkpoint_decision_orphan(context: Any) -> None: + try: + with context.engine.begin() as conn: + conn.execute( + text( + "UPDATE checkpoint_metadata " + "SET decision_id = :orphan " + "WHERE checkpoint_id = :cid" + ), + { + "orphan": "NONEXISTENT_DECISION_ID_UPD", + "cid": context.update_trg_checkpoint_id, + }, + ) + except IntegrityError: + context.update_trigger_rejected = True + return + + context.update_trigger_rejected = False + + +@when("I update the checkpoint resource_id to a non-existent value") +def step_update_checkpoint_resource_orphan(context: Any) -> None: + try: + with context.engine.begin() as conn: + conn.execute( + text( + "UPDATE checkpoint_metadata " + "SET resource_id = :orphan " + "WHERE checkpoint_id = :cid" + ), + { + "orphan": "NONEXISTENT_RESOURCE_ID_UPD", + "cid": context.update_trg_checkpoint_id, + }, + ) + except IntegrityError: + context.update_trigger_rejected = True + return + + context.update_trigger_rejected = False + + +@then("the update should be rejected with an integrity error") +def step_update_rejected(context: Any) -> None: + assert context.update_trigger_rejected, ( + "Expected UPDATE to be rejected by FK trigger, but it was accepted" + ) + + +# --------------------------------------------------------------------------- +# Helper — enable FK enforcement on the pooled DBAPI connection +# --------------------------------------------------------------------------- + + +def _enable_fk_enforcement(engine: Any) -> None: + """Enable PRAGMA foreign_keys on the engine's pooled DBAPI connection. + + For in-memory SQLite databases, SQLAlchemy uses ``StaticPool`` which + shares a single underlying DBAPI connection across all pool checkouts. + Calling ``raw_connection()`` returns a wrapper around that shared + connection; ``close()`` returns it to the pool without closing the + DBAPI connection. The PRAGMA therefore persists for all subsequent + ``engine.begin()`` / ``engine.connect()`` calls. + + This matches the codebase convention of setting PRAGMA foreign_keys + at the connection level (e.g. ``resource_repository_steps.py``, + ``plan_lifecycle_persistence_steps.py``). The ``event.listens_for`` + variant used in engine-creation contexts is not applicable here + because the engine's connections have already been established during + migration; the ``"connect"`` event would not fire for existing + pooled connections. + """ + raw_conn = engine.raw_connection() + try: + cursor = raw_conn.cursor() + cursor.execute("PRAGMA foreign_keys=ON") + cursor.close() + finally: + raw_conn.close() + + +# --------------------------------------------------------------------------- +# Given/When/Then — ondelete="SET NULL" cascade (decision) +# --------------------------------------------------------------------------- + + +@given("a checkpoint references a valid decision") +def step_checkpoint_references_decision(context: Any) -> None: + action_name = "local/test-action-set-null-d" + plan_id = "01ARZ3NDEKTSV4RRFFQ69G5FG0" + decision_id = "01ARZ3NDEKTSV4RRFFQ69G5FG1" + checkpoint_id = "01ARZ3NDEKTSV4RRFFQ69G5FG2" + + with context.engine.begin() as conn: + _ensure_test_action_and_plan(conn, action_name, plan_id) + + conn.execute( + text( + """ + INSERT INTO decisions ( + decision_id, plan_id, decision_type, question, + chosen_option, context_snapshot_json, sequence_number, + created_at + ) VALUES ( + :decision_id, :plan_id, :decision_type, :question, + :chosen_option, :context_snapshot_json, :sequence_number, + :created_at + ) + """ + ), + { + "decision_id": decision_id, + "plan_id": plan_id, + "decision_type": "strategy_choice", + "question": "test question", + "chosen_option": "test option", + "context_snapshot_json": "{}", + "sequence_number": 1, + "created_at": "2026-01-01T00:00:00", + }, + ) + + conn.execute( + text( + """ + INSERT INTO checkpoint_metadata ( + checkpoint_id, plan_id, decision_id, + checkpoint_type, sandbox_ref, + filesystem_path, created_at + ) VALUES ( + :checkpoint_id, :plan_id, :decision_id, + :checkpoint_type, :sandbox_ref, + :filesystem_path, :created_at + ) + """ + ), + { + "checkpoint_id": checkpoint_id, + "plan_id": plan_id, + "decision_id": decision_id, + "checkpoint_type": "manual", + "sandbox_ref": "test-ref", + "filesystem_path": "", + "created_at": "2026-01-01T00:00:00", + }, + ) + + context.set_null_decision_id = decision_id + context.set_null_checkpoint_id_d = checkpoint_id + context.set_null_plan_id_d = plan_id + + +@when("the referenced decision is deleted") +def step_delete_referenced_decision(context: Any) -> None: + # Enable FK enforcement via the established codebase pattern + # (event listener on "connect") so that ondelete="SET NULL" is honoured. + _enable_fk_enforcement(context.engine) + + with context.engine.begin() as conn: + conn.execute( + text("DELETE FROM decisions WHERE decision_id = :did"), + {"did": context.set_null_decision_id}, + ) + + +@then("the checkpoint decision_id should be NULL") +def step_checkpoint_decision_id_null(context: Any) -> None: + with context.engine.connect() as conn: + row = conn.execute( + text( + "SELECT decision_id FROM checkpoint_metadata WHERE checkpoint_id = :cid" + ), + {"cid": context.set_null_checkpoint_id_d}, + ).fetchone() + + assert row is not None, ( + f"Checkpoint {context.set_null_checkpoint_id_d!r} missing after decision deletion" + ) + assert row[0] is None, ( + f"Expected checkpoint decision_id to be NULL after parent deletion, " + f"got {row[0]!r}" + ) + + +# --------------------------------------------------------------------------- +# Given/When/Then — ondelete="SET NULL" cascade (resource) +# --------------------------------------------------------------------------- + + +@given("a checkpoint references a valid resource") +def step_checkpoint_references_resource(context: Any) -> None: + action_name = "local/test-action-set-null-r" + plan_id = "01ARZ3NDEKTSV4RRFFQ69G5FG3" + resource_id = "01ARZ3NDEKTSV4RRFFQ69G5FG4" + checkpoint_id = "01ARZ3NDEKTSV4RRFFQ69G5FG5" + + with context.engine.begin() as conn: + _ensure_test_action_and_plan(conn, action_name, plan_id) + + _insert_test_resource(conn, resource_id) + + conn.execute( + text( + """ + INSERT INTO checkpoint_metadata ( + checkpoint_id, plan_id, resource_id, + checkpoint_type, sandbox_ref, + filesystem_path, created_at + ) VALUES ( + :checkpoint_id, :plan_id, :resource_id, + :checkpoint_type, :sandbox_ref, + :filesystem_path, :created_at + ) + """ + ), + { + "checkpoint_id": checkpoint_id, + "plan_id": plan_id, + "resource_id": resource_id, + "checkpoint_type": "manual", + "sandbox_ref": "test-ref", + "filesystem_path": "", + "created_at": "2026-01-01T00:00:00", + }, + ) + + context.set_null_resource_id = resource_id + context.set_null_checkpoint_id_r = checkpoint_id + + +@when("the referenced resource is deleted") +def step_delete_referenced_resource(context: Any) -> None: + # Enable FK enforcement via the established codebase pattern + # (event listener on "connect") so that ondelete="SET NULL" is honoured. + _enable_fk_enforcement(context.engine) + + with context.engine.begin() as conn: + conn.execute( + text("DELETE FROM resources WHERE resource_id = :rid"), + {"rid": context.set_null_resource_id}, + ) + + +@then("the checkpoint resource_id should be NULL") +def step_checkpoint_resource_id_null(context: Any) -> None: + with context.engine.connect() as conn: + row = conn.execute( + text( + "SELECT resource_id FROM checkpoint_metadata WHERE checkpoint_id = :cid" + ), + {"cid": context.set_null_checkpoint_id_r}, + ).fetchone() + + assert row is not None, ( + f"Checkpoint {context.set_null_checkpoint_id_r!r} missing after resource deletion" + ) + assert row[0] is None, ( + f"Expected checkpoint resource_id to be NULL after parent deletion, " + f"got {row[0]!r}" + ) diff --git a/features/steps/db_schema_link_type_steps.py b/features/steps/db_schema_link_type_steps.py new file mode 100644 index 000000000..ae6faee4b --- /dev/null +++ b/features/steps/db_schema_link_type_steps.py @@ -0,0 +1,432 @@ +"""Step definitions for db_migration_lifecycle.feature — link type and migration. + +Link-type acceptance/rejection, migration idempotency (else-branch), orphan +cleanup during migration, and downgrade verification for the m4_004 schema +parity migration. +""" + +from __future__ import annotations + +from typing import Any + +from alembic import command +from behave import given, then, when +from sqlalchemy import inspect as sa_inspect +from sqlalchemy import text +from sqlalchemy.exc import IntegrityError + +from cleveragents.infrastructure.database.migration_runner import MigrationRunner +from features.steps.db_schema_parity_steps import _insert_test_resource + +# --------------------------------------------------------------------------- +# Given — migration idempotency (link_type pre-exists) +# --------------------------------------------------------------------------- + + +@given("resource_links already has a link_type column without CHECK constraint") +def step_add_link_type_without_check(context: Any) -> None: + """Add a bare link_type column so the migration else-branch is exercised.""" + with context.engine.begin() as conn: + conn.execute( + text( + "ALTER TABLE resource_links " + "ADD COLUMN link_type TEXT DEFAULT 'contains'" + ) + ) + + +@given("resource_links already has a link_type column with a non-contains default") +def step_add_link_type_wrong_default(context: Any) -> None: + """Add a link_type column with wrong default so the default-fix path runs.""" + with context.engine.begin() as conn: + conn.execute( + text("ALTER TABLE resource_links ADD COLUMN link_type TEXT DEFAULT 'other'") + ) + + +# --------------------------------------------------------------------------- +# Given/When/Then — orphan cleanup during migration +# --------------------------------------------------------------------------- + + +@given('migrations applied up to "{revision}"') +def step_migrations_up_to(context: Any, revision: str) -> None: + runner = MigrationRunner(context.db_url) + with context.engine.connect() as conn: + runner.alembic_cfg.attributes["connection"] = conn + try: + command.upgrade(runner.alembic_cfg, revision) + conn.commit() + finally: + runner.alembic_cfg.attributes.pop("connection", None) + context.runner = runner + + +@given("checkpoint_metadata contains orphan decision and resource references") +def step_insert_orphan_checkpoint_refs(context: Any) -> None: + action_name = "local/orphan-cleanup-action" + plan_id = "01ARZ3NDEKTSV4RRFFQ69G5FD0" + checkpoint_id_1 = "01ARZ3NDEKTSV4RRFFQ69G5FD1" + checkpoint_id_2 = "01ARZ3NDEKTSV4RRFFQ69G5FD2" + + with context.engine.begin() as conn: + conn.execute( + text( + """ + INSERT INTO actions ( + namespaced_name, namespace, name, description, + definition_of_done, strategy_actor, execution_actor, + created_at, updated_at + ) VALUES ( + :namespaced_name, :namespace, :name, :description, + :definition_of_done, :strategy_actor, :execution_actor, + :created_at, :updated_at + ) + """ + ), + { + "namespaced_name": action_name, + "namespace": "local", + "name": "orphan-cleanup-action", + "description": "test action", + "definition_of_done": "test dod", + "strategy_actor": "local/strategy", + "execution_actor": "local/execution", + "created_at": "2026-01-01T00:00:00", + "updated_at": "2026-01-01T00:00:00", + }, + ) + # Detect whether root_plan_id column exists (added by + # m8_001_align_plans_schema which may not yet be applied when + # this step runs at the m4_003 migration state). + plan_columns = { + col["name"] for col in sa_inspect(context.engine).get_columns("v3_plans") + } + plan_cols = ( + "plan_id, action_name, namespaced_name, namespace," + " description, created_at, updated_at" + ) + plan_vals = ( + ":plan_id, :action_name, :namespaced_name, :namespace," + " :description, :created_at, :updated_at" + ) + plan_params: dict[str, str] = { + "plan_id": plan_id, + "action_name": action_name, + "namespaced_name": "local/orphan-plan", + "namespace": "local", + "description": "test plan for orphan cleanup", + "created_at": "2026-01-01T00:00:00", + "updated_at": "2026-01-01T00:00:00", + } + if "root_plan_id" in plan_columns: + plan_cols = ( + "plan_id, root_plan_id, action_name," + " namespaced_name, namespace," + " description, created_at, updated_at" + ) + plan_vals = ( + ":plan_id, :root_plan_id, :action_name," + " :namespaced_name, :namespace," + " :description, :created_at, :updated_at" + ) + plan_params["root_plan_id"] = plan_id + conn.execute( + text(f"INSERT INTO v3_plans ({plan_cols}) VALUES ({plan_vals})"), + plan_params, + ) + # Checkpoint with orphan decision_id + conn.execute( + text( + """ + INSERT INTO checkpoint_metadata ( + checkpoint_id, plan_id, decision_id, + checkpoint_type, sandbox_ref, + filesystem_path, created_at + ) VALUES ( + :checkpoint_id, :plan_id, :decision_id, + :checkpoint_type, :sandbox_ref, + :filesystem_path, :created_at + ) + """ + ), + { + "checkpoint_id": checkpoint_id_1, + "plan_id": plan_id, + "decision_id": "ORPHAN_DECISION_ID_NONEXIST", + "checkpoint_type": "manual", + "sandbox_ref": "test-ref", + "filesystem_path": "", + "created_at": "2026-01-01T00:00:00", + }, + ) + # Checkpoint with orphan resource_id + conn.execute( + text( + """ + INSERT INTO checkpoint_metadata ( + checkpoint_id, plan_id, resource_id, + checkpoint_type, sandbox_ref, + filesystem_path, created_at + ) VALUES ( + :checkpoint_id, :plan_id, :resource_id, + :checkpoint_type, :sandbox_ref, + :filesystem_path, :created_at + ) + """ + ), + { + "checkpoint_id": checkpoint_id_2, + "plan_id": plan_id, + "resource_id": "ORPHAN_RESOURCE_ID_NONEXIST", + "checkpoint_type": "manual", + "sandbox_ref": "test-ref", + "filesystem_path": "", + "created_at": "2026-01-01T00:00:00", + }, + ) + context.orphan_plan_id = plan_id + context.orphan_checkpoint_id_1 = checkpoint_id_1 + context.orphan_checkpoint_id_2 = checkpoint_id_2 + + +@when("I upgrade to the next migration revision") +def step_upgrade_one_revision(context: Any) -> None: + runner = context.runner + with context.engine.connect() as conn: + runner.alembic_cfg.attributes["connection"] = conn + try: + # Target m4_004 explicitly because m4_003 has multiple + # child branches (m4_004, m5_001, m8_001_*) and "+1" + # would cause an "Ambiguous walk" error. + command.upgrade( + runner.alembic_cfg, + "m4_004_schema_parity_resource_decision_checkpoint", + ) + conn.commit() + finally: + runner.alembic_cfg.attributes.pop("connection", None) + + +@then("the orphan decision_id values should be NULL") +def step_orphan_decision_id_null(context: Any) -> None: + with context.engine.connect() as conn: + row = conn.execute( + text( + "SELECT decision_id FROM checkpoint_metadata WHERE checkpoint_id = :cid" + ), + {"cid": context.orphan_checkpoint_id_1}, + ).fetchone() + assert row is not None, ( + f"Checkpoint {context.orphan_checkpoint_id_1!r} missing after migration" + ) + assert row[0] is None, f"Expected orphan decision_id to be NULL, got {row[0]!r}" + + +@then("the orphan resource_id values should be NULL") +def step_orphan_resource_id_null(context: Any) -> None: + with context.engine.connect() as conn: + row = conn.execute( + text( + "SELECT resource_id FROM checkpoint_metadata WHERE checkpoint_id = :cid" + ), + {"cid": context.orphan_checkpoint_id_2}, + ).fetchone() + assert row is not None, ( + f"Checkpoint {context.orphan_checkpoint_id_2!r} missing after migration" + ) + assert row[0] is None, f"Expected orphan resource_id to be NULL, got {row[0]!r}" + + +# --------------------------------------------------------------------------- +# Then — link_type acceptance/rejection +# --------------------------------------------------------------------------- + + +@then('resource_links should accept link_type "{link_type_value}"') +def step_resource_links_accepts_link_type(context: Any, link_type_value: str) -> None: + with context.engine.begin() as conn: + parent_id = f"01LINKTYPE_{link_type_value.upper()[:6]}P" + child_id = f"01LINKTYPE_{link_type_value.upper()[:6]}C" + + for rid in (parent_id, child_id): + _insert_test_resource(conn, rid) + + conn.execute( + text( + """ + INSERT INTO resource_links (parent_id, child_id, link_type, created_at) + VALUES (:parent_id, :child_id, :link_type, :created_at) + """ + ), + { + "parent_id": parent_id, + "child_id": child_id, + "link_type": link_type_value, + "created_at": "2026-01-01T00:00:00", + }, + ) + + +@then("resource_links should reject NULL link_type") +def step_resource_links_rejects_null_link_type(context: Any) -> None: + with context.engine.begin() as conn: + parent_id = "01LINKNULLP_TESTPARENT" + child_id = "01LINKNULLC_TESTCHILD0" + + for rid in (parent_id, child_id): + _insert_test_resource(conn, rid) + + try: + conn.execute( + text( + """ + INSERT INTO resource_links (parent_id, child_id, link_type, created_at) + VALUES (:parent_id, :child_id, NULL, :created_at) + """ + ), + { + "parent_id": parent_id, + "child_id": child_id, + "created_at": "2026-01-01T00:00:00", + }, + ) + except IntegrityError: + return + + raise AssertionError("resource_links accepted NULL link_type") + + +@then('resource_links should reject link_type "{link_type_value}"') +def step_resource_links_rejects_link_type(context: Any, link_type_value: str) -> None: + with context.engine.begin() as conn: + parent_id = "01LINKTYPE_INVALIDP_TEST" + child_id = "01LINKTYPE_INVALIDC_TEST" + + for rid in (parent_id, child_id): + _insert_test_resource(conn, rid) + + try: + conn.execute( + text( + """ + INSERT INTO resource_links (parent_id, child_id, link_type, created_at) + VALUES (:parent_id, :child_id, :link_type, :created_at) + """ + ), + { + "parent_id": parent_id, + "child_id": child_id, + "link_type": link_type_value, + "created_at": "2026-01-01T00:00:00", + }, + ) + except IntegrityError: + return + + raise AssertionError( + f"resource_links accepted invalid link_type {link_type_value!r}" + ) + + +@then("resource_links should reject empty string link_type") +def step_resource_links_rejects_empty_link_type(context: Any) -> None: + with context.engine.begin() as conn: + parent_id = "01LINKTYPE_EMPTYP_TESTXX" + child_id = "01LINKTYPE_EMPTYC_TESTXX" + + for rid in (parent_id, child_id): + _insert_test_resource(conn, rid) + + try: + conn.execute( + text( + """ + INSERT INTO resource_links (parent_id, child_id, link_type, created_at) + VALUES (:parent_id, :child_id, :link_type, :created_at) + """ + ), + { + "parent_id": parent_id, + "child_id": child_id, + "link_type": "", + "created_at": "2026-01-01T00:00:00", + }, + ) + except IntegrityError: + return + + raise AssertionError("resource_links accepted empty string link_type") + + +# --------------------------------------------------------------------------- +# When/Then — downgrade verification +# --------------------------------------------------------------------------- + + +@when('I downgrade to revision "{revision}"') +def step_downgrade_to_specific_revision(context: Any, revision: str) -> None: + if not hasattr(context, "runner"): + runner = MigrationRunner(context.db_url) + runner.run_migrations(engine=context.engine) + context.runner = runner + + runner = context.runner + with context.engine.connect() as conn: + runner.alembic_cfg.attributes["connection"] = conn + try: + command.downgrade(runner.alembic_cfg, revision) + conn.commit() + finally: + runner.alembic_cfg.attributes.pop("connection", None) + + +@then('the "resource_links" table should not include "{column_name}"') +def step_table_should_not_include_column(context: Any, column_name: str) -> None: + inspector = sa_inspect(context.engine) + columns = {col["name"] for col in inspector.get_columns("resource_links")} + assert column_name not in columns, ( + f"Expected column {column_name!r} to be absent from resource_links, " + f"but found it in {columns!r}" + ) + + +@then('the "decisions" table should not have index "{index_name}"') +def step_decisions_should_not_have_index(context: Any, index_name: str) -> None: + inspector = sa_inspect(context.engine) + indexes = {idx["name"] for idx in inspector.get_indexes("decisions")} + assert index_name not in indexes, ( + f"Expected index {index_name!r} to be absent from decisions, " + f"but found it in {indexes!r}" + ) + + +@then('checkpoint_metadata should not have foreign key "{fk_name}"') +def step_checkpoint_should_not_have_fk(context: Any, fk_name: str) -> None: + inspector = sa_inspect(context.engine) + fks = inspector.get_foreign_keys("checkpoint_metadata") + fk_names = {fk.get("name") for fk in fks} + assert fk_name not in fk_names, ( + f"Expected FK {fk_name!r} to be absent from checkpoint_metadata, " + f"but found it in {fk_names!r}" + ) + + +@then("checkpoint_metadata should not have SQLite triggers for FK enforcement") +def step_checkpoint_no_fk_triggers(context: Any) -> None: + if context.engine.dialect.name != "sqlite": + return # Triggers are SQLite-specific; skip on other dialects. + + with context.engine.connect() as conn: + rows = conn.execute( + text( + "SELECT name FROM sqlite_master " + "WHERE type = 'trigger' AND name LIKE 'trg_checkpoint_metadata_%'" + ) + ).fetchall() + + trigger_names = [row[0] for row in rows] + assert not trigger_names, ( + f"Expected no trg_checkpoint_metadata_* triggers after downgrade, " + f"found: {trigger_names!r}" + ) diff --git a/features/steps/db_schema_parity_steps.py b/features/steps/db_schema_parity_steps.py new file mode 100644 index 000000000..dafc1acfb --- /dev/null +++ b/features/steps/db_schema_parity_steps.py @@ -0,0 +1,427 @@ +"""Step definitions for db_migration_lifecycle.feature — schema parity. + +FK structure verification, orphan rejection, valid references, and +partial index checks for the ``resource_links``, ``checkpoint_metadata``, +and ``decisions`` tables. +""" + +from __future__ import annotations + +from typing import Any + +from behave import then +from sqlalchemy import inspect as sa_inspect +from sqlalchemy import text +from sqlalchemy.exc import IntegrityError + +# --------------------------------------------------------------------------- +# Shared test-data helpers +# --------------------------------------------------------------------------- + + +def _ensure_test_action_and_plan(conn: Any, action_name: str, plan_id: str) -> None: + """Insert prerequisite action and plan rows if absent.""" + existing_action = conn.execute( + text("SELECT 1 FROM actions WHERE namespaced_name = :n"), + {"n": action_name}, + ).fetchone() + existing_plan = conn.execute( + text("SELECT 1 FROM v3_plans WHERE plan_id = :pid"), + {"pid": plan_id}, + ).fetchone() + if existing_action is not None and existing_plan is not None: + return + + if existing_action is None: + conn.execute( + text( + """ + INSERT INTO actions ( + namespaced_name, namespace, name, description, + definition_of_done, strategy_actor, execution_actor, + created_at, updated_at + ) VALUES ( + :namespaced_name, :namespace, :name, :description, + :definition_of_done, :strategy_actor, :execution_actor, + :created_at, :updated_at + ) + """ + ), + { + "namespaced_name": action_name, + "namespace": "local", + "name": action_name.split("/")[-1], + "description": "test action", + "definition_of_done": "test dod", + "strategy_actor": "local/strategy", + "execution_actor": "local/execution", + "created_at": "2026-01-01T00:00:00", + "updated_at": "2026-01-01T00:00:00", + }, + ) + + if existing_plan is None: + conn.execute( + text( + """ + INSERT INTO v3_plans ( + plan_id, root_plan_id, action_name, + namespaced_name, namespace, + description, created_at, updated_at + ) VALUES ( + :plan_id, :root_plan_id, :action_name, + :namespaced_name, :namespace, + :description, :created_at, :updated_at + ) + """ + ), + { + "plan_id": plan_id, + "root_plan_id": plan_id, + "action_name": action_name, + "namespaced_name": "local/test-plan-fk", + "namespace": "local", + "description": "test plan", + "created_at": "2026-01-01T00:00:00", + "updated_at": "2026-01-01T00:00:00", + }, + ) + + +def _insert_test_resource( + conn: Any, + resource_id: str, + type_name: str = "git-checkout", + resource_kind: str = "physical", +) -> None: + """Insert a test resource row, handling the optional namespaced_name column.""" + resource_columns = { + col["name"] for col in sa_inspect(conn).get_columns("resources") + } + cols = "resource_id, type_name, resource_kind, created_at, updated_at" + vals = ":resource_id, :type_name, :resource_kind, :created_at, :updated_at" + params: dict[str, str] = { + "resource_id": resource_id, + "type_name": type_name, + "resource_kind": resource_kind, + "created_at": "2026-01-01T00:00:00", + "updated_at": "2026-01-01T00:00:00", + } + if "namespaced_name" in resource_columns: + cols = ( + "resource_id, namespaced_name, type_name," + " resource_kind, created_at, updated_at" + ) + vals = ( + ":resource_id, :namespaced_name, :type_name," + " :resource_kind, :created_at, :updated_at" + ) + params["namespaced_name"] = f"local/{resource_id}" + conn.execute(text(f"INSERT INTO resources ({cols}) VALUES ({vals})"), params) + + +# --------------------------------------------------------------------------- +# Then — link_type default verification +# --------------------------------------------------------------------------- + + +@then('the "resource_links" table should include "link_type" with default "contains"') +def step_resource_links_has_link_type_default(context: Any) -> None: + inspector = sa_inspect(context.engine) + columns = inspector.get_columns("resource_links") + link_type = next( + (column for column in columns if column["name"] == "link_type"), None + ) + assert link_type is not None, "resource_links.link_type column is missing" + + default = str(link_type.get("default") or "").lower() + assert "contains" in default, ( + "Expected resource_links.link_type default to include 'contains', " + f"got: {link_type.get('default')!r}" + ) + + +# --------------------------------------------------------------------------- +# Then — checkpoint FK structure verification +# --------------------------------------------------------------------------- + + +@then("checkpoint_metadata should enforce decision and resource foreign keys") +def step_checkpoint_metadata_foreign_keys(context: Any) -> None: + inspector = sa_inspect(context.engine) + foreign_keys = inspector.get_foreign_keys("checkpoint_metadata") + signatures = { + ( + tuple(fk.get("constrained_columns") or []), + fk.get("referred_table"), + tuple(fk.get("referred_columns") or []), + ) + for fk in foreign_keys + } + + decision_fk = (("decision_id",), "decisions", ("decision_id",)) + resource_fk = (("resource_id",), "resources", ("resource_id",)) + + assert decision_fk in signatures, ( + "Missing checkpoint_metadata foreign key for decision_id -> " + "decisions.decision_id" + ) + assert resource_fk in signatures, ( + "Missing checkpoint_metadata foreign key for resource_id -> " + "resources.resource_id" + ) + + +# --------------------------------------------------------------------------- +# Then — orphan FK rejection (combined) +# --------------------------------------------------------------------------- + + +@then("checkpoint_metadata foreign keys should reject orphan references") +def step_checkpoint_metadata_foreign_keys_reject_orphans(context: Any) -> None: + action_name = "local/test-action" + plan_id = "01ARZ3NDEKTSV4RRFFQ69G5FAV" + + with context.engine.begin() as conn: + _ensure_test_action_and_plan(conn, action_name, plan_id) + + try: + conn.execute( + text( + """ + INSERT INTO checkpoint_metadata ( + checkpoint_id, + plan_id, + decision_id, + checkpoint_type, + resource_id, + sandbox_ref, + filesystem_path, + created_at + ) VALUES ( + :checkpoint_id, + :plan_id, + :decision_id, + :checkpoint_type, + :resource_id, + :sandbox_ref, + :filesystem_path, + :created_at + ) + """ + ), + { + "checkpoint_id": "01ARZ3NDEKTSV4RRFFQ69G5FAW", + "plan_id": plan_id, + "decision_id": "01ARZ3NDEKTSV4RRFFQ69G5FAX", + "checkpoint_type": "manual", + "resource_id": "01ARZ3NDEKTSV4RRFFQ69G5FAY", + "sandbox_ref": "test-ref", + "filesystem_path": "", + "created_at": "2026-01-01T00:00:00", + }, + ) + except IntegrityError: + return + + raise AssertionError( + "checkpoint_metadata accepted orphan decision/resource references" + ) + + +# --------------------------------------------------------------------------- +# Then — independent orphan FK rejection +# --------------------------------------------------------------------------- + + +@then("checkpoint_metadata should reject orphan decision_id independently") +def step_checkpoint_reject_orphan_decision_only(context: Any) -> None: + action_name = "local/test-action-fk-d" + plan_id = "01ARZ3NDEKTSV4RRFFQ69G5FB0" + + with context.engine.begin() as conn: + _ensure_test_action_and_plan(conn, action_name, plan_id) + + try: + conn.execute( + text( + """ + INSERT INTO checkpoint_metadata ( + checkpoint_id, plan_id, decision_id, + checkpoint_type, sandbox_ref, + filesystem_path, created_at + ) VALUES ( + :checkpoint_id, :plan_id, :decision_id, + :checkpoint_type, :sandbox_ref, + :filesystem_path, :created_at + ) + """ + ), + { + "checkpoint_id": "01ARZ3NDEKTSV4RRFFQ69G5FB1", + "plan_id": plan_id, + "decision_id": "01ARZ3NDEKTSV4RRFFQ69G5FB2", + "checkpoint_type": "manual", + "sandbox_ref": "test-ref", + "filesystem_path": "", + "created_at": "2026-01-01T00:00:00", + }, + ) + except IntegrityError: + return + + raise AssertionError( + "checkpoint_metadata accepted orphan decision_id with NULL resource_id" + ) + + +@then("checkpoint_metadata should reject orphan resource_id independently") +def step_checkpoint_reject_orphan_resource_only(context: Any) -> None: + action_name = "local/test-action-fk-r" + plan_id = "01ARZ3NDEKTSV4RRFFQ69G5FB3" + + with context.engine.begin() as conn: + _ensure_test_action_and_plan(conn, action_name, plan_id) + + try: + conn.execute( + text( + """ + INSERT INTO checkpoint_metadata ( + checkpoint_id, plan_id, resource_id, + checkpoint_type, sandbox_ref, + filesystem_path, created_at + ) VALUES ( + :checkpoint_id, :plan_id, :resource_id, + :checkpoint_type, :sandbox_ref, + :filesystem_path, :created_at + ) + """ + ), + { + "checkpoint_id": "01ARZ3NDEKTSV4RRFFQ69G5FB4", + "plan_id": plan_id, + "resource_id": "01ARZ3NDEKTSV4RRFFQ69G5FB5", + "checkpoint_type": "manual", + "sandbox_ref": "test-ref", + "filesystem_path": "", + "created_at": "2026-01-01T00:00:00", + }, + ) + except IntegrityError: + return + + raise AssertionError( + "checkpoint_metadata accepted orphan resource_id with NULL decision_id" + ) + + +# --------------------------------------------------------------------------- +# Then — valid FK acceptance +# --------------------------------------------------------------------------- + + +@then("checkpoint_metadata should accept valid decision and resource references") +def step_checkpoint_accept_valid_references(context: Any) -> None: + action_name = "local/test-action-fk-v" + plan_id = "01ARZ3NDEKTSV4RRFFQ69G5FB6" + decision_id = "01ARZ3NDEKTSV4RRFFQ69G5FB7" + resource_id = "01ARZ3NDEKTSV4RRFFQ69G5FB8" + + with context.engine.begin() as conn: + _ensure_test_action_and_plan(conn, action_name, plan_id) + + conn.execute( + text( + """ + INSERT INTO decisions ( + decision_id, plan_id, decision_type, question, + chosen_option, context_snapshot_json, sequence_number, + created_at + ) VALUES ( + :decision_id, :plan_id, :decision_type, :question, + :chosen_option, :context_snapshot_json, :sequence_number, + :created_at + ) + """ + ), + { + "decision_id": decision_id, + "plan_id": plan_id, + "decision_type": "strategy_choice", + "question": "test question", + "chosen_option": "test option", + "context_snapshot_json": "{}", + "sequence_number": 1, + "created_at": "2026-01-01T00:00:00", + }, + ) + + _insert_test_resource(conn, resource_id) + + conn.execute( + text( + """ + INSERT INTO checkpoint_metadata ( + checkpoint_id, plan_id, decision_id, + checkpoint_type, resource_id, sandbox_ref, + filesystem_path, created_at + ) VALUES ( + :checkpoint_id, :plan_id, :decision_id, + :checkpoint_type, :resource_id, :sandbox_ref, + :filesystem_path, :created_at + ) + """ + ), + { + "checkpoint_id": "01ARZ3NDEKTSV4RRFFQ69G5FB9", + "plan_id": plan_id, + "decision_id": decision_id, + "checkpoint_type": "manual", + "resource_id": resource_id, + "sandbox_ref": "test-ref", + "filesystem_path": "", + "created_at": "2026-01-01T00:00:00", + }, + ) + + +# --------------------------------------------------------------------------- +# Then — partial index verification +# --------------------------------------------------------------------------- + + +@then( + 'the "decisions" table should have partial index "{index_name}" on "{column_name}"' +) +def step_decisions_has_partial_index( + context: Any, + index_name: str, + column_name: str, +) -> None: + inspector = sa_inspect(context.engine) + indexes = inspector.get_indexes("decisions") + index = next((idx for idx in indexes if idx.get("name") == index_name), None) + assert index is not None, f"Index {index_name} not found on decisions" + + columns = list(index.get("column_names") or []) + assert columns == [column_name], ( + f"Index {index_name} expected on [{column_name!r}], got {columns!r}" + ) + + # Verify partial WHERE clause via sqlite_master (SQLite-only). + if context.engine.dialect.name == "sqlite": + with context.engine.connect() as conn: + row = conn.execute( + text( + "SELECT sql FROM sqlite_master " + "WHERE type = 'index' AND name = :index_name" + ), + {"index_name": index_name}, + ).fetchone() + + assert row is not None, f"sqlite_master entry missing for index {index_name}" + sql = str(row[0] or "").lower() + assert "where superseded_by is not null" in sql, ( + f"Expected partial WHERE clause on {index_name}, got SQL: {row[0]!r}" + ) diff --git a/robot/helper_schema_parity_migration.py b/robot/helper_schema_parity_migration.py new file mode 100644 index 000000000..a9fb48039 --- /dev/null +++ b/robot/helper_schema_parity_migration.py @@ -0,0 +1,443 @@ +"""Robot helper for schema-parity migration verification. + +Checks the migration-produced SQLite schema for: + +1. ``resource_links.link_type`` defaulting to ``contains``. +2. ``checkpoint_metadata`` foreign keys to ``decisions`` and ``resources``. +3. ``idx_decisions_superseded`` partial index with + ``WHERE superseded_by IS NOT NULL``. +4. Runtime SQLite foreign key enforcement for ``checkpoint_metadata``. + +Subcommands (each independent for isolated failure reporting): + +- ``schema-parity-link-type``: Verifies resource_links.link_type column + and default. +- ``schema-parity-fks``: Verifies checkpoint_metadata FK constraints and + runtime enforcement (orphan rejection + positive path). +- ``schema-parity-index``: Verifies idx_decisions_superseded partial index. +""" + +from __future__ import annotations + +import sys +import tempfile +from collections.abc import Callable +from pathlib import Path +from typing import Any + +from sqlalchemy import create_engine, inspect, text +from sqlalchemy.exc import IntegrityError + +from cleveragents.infrastructure.database.migration_runner import MigrationRunner + + +def _cleanup_db_files(path: str) -> None: + Path(path).unlink(missing_ok=True) + Path(f"{path}-wal").unlink(missing_ok=True) + Path(f"{path}-shm").unlink(missing_ok=True) + + +def _setup_migrated_db() -> tuple[Any, Any, str]: + """Create a temp DB, run migrations, return (engine, inspector, db_path).""" + with tempfile.NamedTemporaryFile(suffix=".db", delete=False) as tmp_file: + db_path = tmp_file.name + db_url = f"sqlite:///{db_path}" + + MigrationRunner(db_url).init_or_upgrade() + engine = create_engine(db_url, connect_args={"check_same_thread": False}) + inspector = inspect(engine) + return engine, inspector, db_path + + +def _teardown_db(engine: Any, db_path: str) -> None: + if engine is not None: + engine.dispose() + _cleanup_db_files(db_path) + + +def _ensure_test_action_and_plan(conn: Any) -> None: + """Insert prerequisite action and plan rows for FK tests. + + .. note:: + This function is **not idempotent** — it performs blind INSERTs + without existence checks. It is safe only when called against a + freshly created database (as ``_setup_migrated_db`` provides). + If idempotent behaviour is needed, see the Behave counterpart in + ``features/steps/db_schema_parity_steps.py``. + """ + conn.execute( + text( + """ + INSERT INTO actions ( + namespaced_name, namespace, name, description, + definition_of_done, strategy_actor, execution_actor, + created_at, updated_at + ) VALUES ( + :namespaced_name, :namespace, :name, :description, + :definition_of_done, :strategy_actor, :execution_actor, + :created_at, :updated_at + ) + """ + ), + { + "namespaced_name": "local/test-action", + "namespace": "local", + "name": "test-action", + "description": "test action", + "definition_of_done": "test dod", + "strategy_actor": "local/strategy", + "execution_actor": "local/execution", + "created_at": "2026-01-01T00:00:00", + "updated_at": "2026-01-01T00:00:00", + }, + ) + conn.execute( + text( + """ + INSERT INTO v3_plans ( + plan_id, root_plan_id, action_name, + namespaced_name, namespace, + description, created_at, updated_at + ) VALUES ( + :plan_id, :root_plan_id, :action_name, + :namespaced_name, :namespace, + :description, :created_at, :updated_at + ) + """ + ), + { + "plan_id": "01ARZ3NDEKTSV4RRFFQ69G5FAV", + "root_plan_id": "01ARZ3NDEKTSV4RRFFQ69G5FAV", + "action_name": "local/test-action", + "namespaced_name": "local/test-plan", + "namespace": "local", + "description": "test plan", + "created_at": "2026-01-01T00:00:00", + "updated_at": "2026-01-01T00:00:00", + }, + ) + + +# --------------------------------------------------------------------------- +# Subcommand: schema-parity-link-type +# --------------------------------------------------------------------------- + + +def _schema_parity_link_type() -> None: + engine, inspector, db_path = _setup_migrated_db() + try: + resource_link_columns = inspector.get_columns("resource_links") + link_type = next( + ( + column + for column in resource_link_columns + if column["name"] == "link_type" + ), + None, + ) + assert link_type is not None, "resource_links.link_type column is missing" + default = str(link_type.get("default") or "").lower() + assert "contains" in default, ( + "resource_links.link_type default must include 'contains', " + f"got {link_type.get('default')!r}" + ) + + print("schema-parity-link-type-ok") + finally: + _teardown_db(engine, db_path) + + +# --------------------------------------------------------------------------- +# Subcommand: schema-parity-fks +# --------------------------------------------------------------------------- + + +def _schema_parity_fks() -> None: + engine, inspector, db_path = _setup_migrated_db() + try: + checkpoint_fks = inspector.get_foreign_keys("checkpoint_metadata") + signatures = { + ( + tuple(fk.get("constrained_columns") or []), + fk.get("referred_table"), + tuple(fk.get("referred_columns") or []), + ) + for fk in checkpoint_fks + } + + assert (("decision_id",), "decisions", ("decision_id",)) in signatures, ( + "Missing checkpoint_metadata FK decision_id -> decisions.decision_id" + ) + assert (("resource_id",), "resources", ("resource_id",)) in signatures, ( + "Missing checkpoint_metadata FK resource_id -> resources.resource_id" + ) + + with engine.begin() as conn: + _ensure_test_action_and_plan(conn) + + try: + conn.execute( + text( + """ + INSERT INTO checkpoint_metadata ( + checkpoint_id, plan_id, decision_id, + checkpoint_type, resource_id, sandbox_ref, + filesystem_path, created_at + ) VALUES ( + :checkpoint_id, :plan_id, :decision_id, + :checkpoint_type, :resource_id, :sandbox_ref, + :filesystem_path, :created_at + ) + """ + ), + { + "checkpoint_id": "01ARZ3NDEKTSV4RRFFQ69G5FAW", + "plan_id": "01ARZ3NDEKTSV4RRFFQ69G5FAV", + "decision_id": "01ARZ3NDEKTSV4RRFFQ69G5FAX", + "checkpoint_type": "manual", + "resource_id": "01ARZ3NDEKTSV4RRFFQ69G5FAY", + "sandbox_ref": "test-ref", + "filesystem_path": "", + "created_at": "2026-01-01T00:00:00", + }, + ) + except IntegrityError: + pass + else: + raise AssertionError( + "checkpoint_metadata accepted orphan decision/resource references" + ) + + # Verify each FK independently: orphan decision_id only + with engine.begin() as conn: + try: + conn.execute( + text( + """ + INSERT INTO checkpoint_metadata ( + checkpoint_id, plan_id, decision_id, + checkpoint_type, sandbox_ref, + filesystem_path, created_at + ) VALUES ( + :checkpoint_id, :plan_id, :decision_id, + :checkpoint_type, :sandbox_ref, + :filesystem_path, :created_at + ) + """ + ), + { + "checkpoint_id": "01ARZ3NDEKTSV4RRFFQ69G5FC0", + "plan_id": "01ARZ3NDEKTSV4RRFFQ69G5FAV", + "decision_id": "01ARZ3NDEKTSV4RRFFQ69G5FC1", + "checkpoint_type": "manual", + "sandbox_ref": "test-ref", + "filesystem_path": "", + "created_at": "2026-01-01T00:00:00", + }, + ) + except IntegrityError: + pass + else: + raise AssertionError( + "checkpoint_metadata accepted orphan decision_id independently" + ) + + # Verify each FK independently: orphan resource_id only + with engine.begin() as conn: + try: + conn.execute( + text( + """ + INSERT INTO checkpoint_metadata ( + checkpoint_id, plan_id, resource_id, + checkpoint_type, sandbox_ref, + filesystem_path, created_at + ) VALUES ( + :checkpoint_id, :plan_id, :resource_id, + :checkpoint_type, :sandbox_ref, + :filesystem_path, :created_at + ) + """ + ), + { + "checkpoint_id": "01ARZ3NDEKTSV4RRFFQ69G5FC2", + "plan_id": "01ARZ3NDEKTSV4RRFFQ69G5FAV", + "resource_id": "01ARZ3NDEKTSV4RRFFQ69G5FC3", + "checkpoint_type": "manual", + "sandbox_ref": "test-ref", + "filesystem_path": "", + "created_at": "2026-01-01T00:00:00", + }, + ) + except IntegrityError: + pass + else: + raise AssertionError( + "checkpoint_metadata accepted orphan resource_id independently" + ) + + # Positive test: valid FK references should be accepted + with engine.begin() as conn: + decision_id = "01ARZ3NDEKTSV4RRFFQ69G5FC4" + resource_id = "01ARZ3NDEKTSV4RRFFQ69G5FC5" + + conn.execute( + text( + """ + INSERT INTO decisions ( + decision_id, plan_id, decision_type, question, + chosen_option, context_snapshot_json, sequence_number, + created_at + ) VALUES ( + :decision_id, :plan_id, :decision_type, :question, + :chosen_option, :context_snapshot_json, :sequence_number, + :created_at + ) + """ + ), + { + "decision_id": decision_id, + "plan_id": "01ARZ3NDEKTSV4RRFFQ69G5FAV", + "decision_type": "strategy_choice", + "question": "test question", + "chosen_option": "test option", + "context_snapshot_json": "{}", + "sequence_number": 1, + "created_at": "2026-01-01T00:00:00", + }, + ) + + resource_columns = { + col["name"] for col in inspect(engine).get_columns("resources") + } + cols = "resource_id, type_name, resource_kind, created_at, updated_at" + vals = ":resource_id, :type_name, :resource_kind, :created_at, :updated_at" + res_params: dict[str, str] = { + "resource_id": resource_id, + "type_name": "git-checkout", + "resource_kind": "physical", + "created_at": "2026-01-01T00:00:00", + "updated_at": "2026-01-01T00:00:00", + } + if "namespaced_name" in resource_columns: + cols = ( + "resource_id, namespaced_name, type_name," + " resource_kind, created_at, updated_at" + ) + vals = ( + ":resource_id, :namespaced_name, :type_name," + " :resource_kind, :created_at, :updated_at" + ) + res_params["namespaced_name"] = f"local/{resource_id}" + conn.execute( + text(f"INSERT INTO resources ({cols}) VALUES ({vals})"), + res_params, + ) + + conn.execute( + text( + """ + INSERT INTO checkpoint_metadata ( + checkpoint_id, plan_id, decision_id, + checkpoint_type, resource_id, sandbox_ref, + filesystem_path, created_at + ) VALUES ( + :checkpoint_id, :plan_id, :decision_id, + :checkpoint_type, :resource_id, :sandbox_ref, + :filesystem_path, :created_at + ) + """ + ), + { + "checkpoint_id": "01ARZ3NDEKTSV4RRFFQ69G5FC6", + "plan_id": "01ARZ3NDEKTSV4RRFFQ69G5FAV", + "decision_id": decision_id, + "checkpoint_type": "manual", + "resource_id": resource_id, + "sandbox_ref": "test-ref", + "filesystem_path": "", + "created_at": "2026-01-01T00:00:00", + }, + ) + + print("schema-parity-fks-ok") + finally: + _teardown_db(engine, db_path) + + +# --------------------------------------------------------------------------- +# Subcommand: schema-parity-index +# --------------------------------------------------------------------------- + + +def _schema_parity_index() -> None: + engine, inspector, db_path = _setup_migrated_db() + try: + decision_indexes = inspector.get_indexes("decisions") + partial_index = next( + ( + index + for index in decision_indexes + if index.get("name") == "idx_decisions_superseded" + ), + None, + ) + assert partial_index is not None, "idx_decisions_superseded is missing" + assert list(partial_index.get("column_names") or []) == ["superseded_by"], ( + "idx_decisions_superseded must index decisions.superseded_by" + ) + + with engine.connect() as conn: + row = conn.execute( + text( + "SELECT sql FROM sqlite_master " + "WHERE type = 'index' AND name = :index_name" + ), + {"index_name": "idx_decisions_superseded"}, + ).fetchone() + + assert row is not None, "sqlite_master is missing idx_decisions_superseded" + sql = str(row[0] or "").lower() + assert "where superseded_by is not null" in sql, ( + "idx_decisions_superseded is not a partial index" + ) + + print("schema-parity-index-ok") + finally: + _teardown_db(engine, db_path) + + +# --------------------------------------------------------------------------- +# Legacy combined subcommand (kept for backward compatibility) +# --------------------------------------------------------------------------- + + +def _schema_parity() -> None: + _schema_parity_link_type() + _schema_parity_fks() + _schema_parity_index() + print("schema-parity-ok") + + +_COMMANDS: dict[str, Callable[[], None]] = { + "schema-parity": _schema_parity, + "schema-parity-link-type": _schema_parity_link_type, + "schema-parity-fks": _schema_parity_fks, + "schema-parity-index": _schema_parity_index, +} + + +def main() -> None: + if len(sys.argv) < 2: + raise SystemExit(f"Expected command argument. Valid: {', '.join(_COMMANDS)}") + + command = sys.argv[1] + handler = _COMMANDS.get(command) + if handler is None: + raise SystemExit(f"Unknown command: {command}. Valid: {', '.join(_COMMANDS)}") + + handler() + + +if __name__ == "__main__": + main() diff --git a/robot/schema_parity_migration.robot b/robot/schema_parity_migration.robot new file mode 100644 index 000000000..fc7b1dc06 --- /dev/null +++ b/robot/schema_parity_migration.robot @@ -0,0 +1,36 @@ +*** Settings *** +Documentation Integration checks for spec-parity schema constraints and indexes. +... Verifies migration output includes resource_links.link_type default, +... checkpoint_metadata FK enforcement, and decisions partial superseded index. +... Split into independent test cases for isolated failure reporting. +Resource ${CURDIR}/common.resource +Suite Setup Setup Test Environment +Suite Teardown Cleanup Test Environment + +*** Variables *** +${HELPER_SCRIPT} robot/helper_schema_parity_migration.py + +*** Test Cases *** +Resource Links Link Type Default After Migration + [Documentation] Validate resource_links.link_type column exists with default 'contains'. + [Tags] database migration integration link-type + ${result}= Run Process ${PYTHON} ${HELPER_SCRIPT} schema-parity-link-type cwd=${WORKSPACE} + Log ${result.stderr} + Should Be Equal As Integers ${result.rc} 0 + Should Contain ${result.stdout} schema-parity-link-type-ok + +Checkpoint Metadata FK Enforcement After Migration + [Documentation] Validate checkpoint_metadata FK constraints and orphan rejection. + [Tags] database migration integration fks + ${result}= Run Process ${PYTHON} ${HELPER_SCRIPT} schema-parity-fks cwd=${WORKSPACE} + Log ${result.stderr} + Should Be Equal As Integers ${result.rc} 0 + Should Contain ${result.stdout} schema-parity-fks-ok + +Decisions Superseded Partial Index After Migration + [Documentation] Validate idx_decisions_superseded partial index on decisions.superseded_by. + [Tags] database migration integration index + ${result}= Run Process ${PYTHON} ${HELPER_SCRIPT} schema-parity-index cwd=${WORKSPACE} + Log ${result.stderr} + Should Be Equal As Integers ${result.rc} 0 + Should Contain ${result.stdout} schema-parity-index-ok diff --git a/src/cleveragents/infrastructure/database/migrations/versions/m4_004_schema_parity_resource_decision_checkpoint.py b/src/cleveragents/infrastructure/database/migrations/versions/m4_004_schema_parity_resource_decision_checkpoint.py new file mode 100644 index 000000000..7cec1ab8f --- /dev/null +++ b/src/cleveragents/infrastructure/database/migrations/versions/m4_004_schema_parity_resource_decision_checkpoint.py @@ -0,0 +1,318 @@ +"""Align resource/decision/checkpoint schema gaps with specification DDL. + +Adds four spec-parity changes: + +1. ``resource_links.link_type`` with default ``'contains'``. +2. Foreign keys on ``checkpoint_metadata.decision_id`` and + ``checkpoint_metadata.resource_id``. +3. Partial index ``idx_decisions_superseded`` on + ``decisions.superseded_by`` where non-null. +4. SQLite trigger guards to enforce checkpoint FK semantics at runtime. + +Revision ID: m4_004_schema_parity_resource_decision_checkpoint +Revises: m4_003_plan_env_columns +Create Date: 2026-03-26 00:00:00 +""" + +from __future__ import annotations + +from collections.abc import Sequence + +import sqlalchemy as sa +from alembic import op + +# revision identifiers, used by Alembic. +revision: str = "m4_004_schema_parity_resource_decision_checkpoint" +down_revision: str | Sequence[str] | None = "m4_003_plan_env_columns" +branch_labels: str | Sequence[str] | None = None +depends_on: str | Sequence[str] | None = None + + +def _inspector() -> sa.Inspector: + return sa.inspect(op.get_bind()) + + +def _create_sqlite_checkpoint_fk_triggers() -> None: + # NOTE: These triggers guard INSERT and UPDATE on checkpoint_metadata only. + # DELETE-direction enforcement (preventing deletion of a referenced decision + # or resource) relies on PRAGMA foreign_keys=ON at the connection level. + # This is a known limitation of trigger-based FK emulation on SQLite. + op.execute( + sa.text( + """ + CREATE TRIGGER IF NOT EXISTS trg_checkpoint_metadata_decision_fk_insert + BEFORE INSERT ON checkpoint_metadata + FOR EACH ROW + WHEN NEW.decision_id IS NOT NULL + AND NOT EXISTS ( + SELECT 1 FROM decisions WHERE decision_id = NEW.decision_id + ) + BEGIN + SELECT RAISE(ABORT, 'FOREIGN KEY constraint failed'); + END; + """ + ) + ) + op.execute( + sa.text( + """ + CREATE TRIGGER IF NOT EXISTS trg_checkpoint_metadata_decision_fk_update + BEFORE UPDATE OF decision_id ON checkpoint_metadata + FOR EACH ROW + WHEN NEW.decision_id IS NOT NULL + AND NOT EXISTS ( + SELECT 1 FROM decisions WHERE decision_id = NEW.decision_id + ) + BEGIN + SELECT RAISE(ABORT, 'FOREIGN KEY constraint failed'); + END; + """ + ) + ) + op.execute( + sa.text( + """ + CREATE TRIGGER IF NOT EXISTS trg_checkpoint_metadata_resource_fk_insert + BEFORE INSERT ON checkpoint_metadata + FOR EACH ROW + WHEN NEW.resource_id IS NOT NULL + AND NOT EXISTS ( + SELECT 1 FROM resources WHERE resource_id = NEW.resource_id + ) + BEGIN + SELECT RAISE(ABORT, 'FOREIGN KEY constraint failed'); + END; + """ + ) + ) + op.execute( + sa.text( + """ + CREATE TRIGGER IF NOT EXISTS trg_checkpoint_metadata_resource_fk_update + BEFORE UPDATE OF resource_id ON checkpoint_metadata + FOR EACH ROW + WHEN NEW.resource_id IS NOT NULL + AND NOT EXISTS ( + SELECT 1 FROM resources WHERE resource_id = NEW.resource_id + ) + BEGIN + SELECT RAISE(ABORT, 'FOREIGN KEY constraint failed'); + END; + """ + ) + ) + + +def _drop_sqlite_checkpoint_fk_triggers() -> None: + op.execute( + sa.text("DROP TRIGGER IF EXISTS trg_checkpoint_metadata_decision_fk_insert") + ) + op.execute( + sa.text("DROP TRIGGER IF EXISTS trg_checkpoint_metadata_decision_fk_update") + ) + op.execute( + sa.text("DROP TRIGGER IF EXISTS trg_checkpoint_metadata_resource_fk_insert") + ) + op.execute( + sa.text("DROP TRIGGER IF EXISTS trg_checkpoint_metadata_resource_fk_update") + ) + + +def upgrade() -> None: + """Apply spec-parity schema updates.""" + inspector = _inspector() + + resource_link_columns = { + column["name"] for column in inspector.get_columns("resource_links") + } + if "link_type" not in resource_link_columns: + with op.batch_alter_table("resource_links") as batch_op: + batch_op.add_column( + sa.Column( + "link_type", + sa.Text(), + nullable=False, + server_default=sa.text("'contains'"), + ) + ) + # NOTE: Spec DDL (line 45549) does not declare a CHECK on + # link_type; we add it to match the sibling resource_edges + # constraint and prevent invalid values at the DB level. + batch_op.create_check_constraint( + "ck_resource_links_link_type", + "link_type IN ('contains', 'references', 'derived_from')", + ) + else: + # link_type column already exists — fix default and ensure CHECK + # constraint is present (guards against partial prior migration). + # NOTE: Spec DDL (line 45549) defines link_type as nullable + # (TEXT DEFAULT 'contains' without NOT NULL). We deliberately + # enforce NOT NULL because a NULL link type is semantically + # meaningless and the sibling resource_edges table also requires + # a non-NULL link_type. + # NOTE: Column TYPE is not verified here. On SQLite all text + # types are equivalent, so a prior VARCHAR(30) column works + # identically to TEXT. On PostgreSQL, VARCHAR(30) != TEXT; if + # a future migration targets PostgreSQL this path should also + # include an ALTER COLUMN TYPE to sa.Text(). + link_type_column = next( + column + for column in inspector.get_columns("resource_links") + if column["name"] == "link_type" + ) + current_default = str(link_type_column.get("default") or "").lower() + needs_default_fix = "contains" not in current_default + + check_constraints = inspector.get_check_constraints("resource_links") + has_link_type_ck = any( + ck.get("name") == "ck_resource_links_link_type" for ck in check_constraints + ) + + if needs_default_fix or not has_link_type_ck: + with op.batch_alter_table("resource_links") as batch_op: + if needs_default_fix: + batch_op.alter_column( + "link_type", + server_default=sa.text("'contains'"), + ) + if not has_link_type_ck: + batch_op.create_check_constraint( + "ck_resource_links_link_type", + "link_type IN ('contains', 'references', 'derived_from')", + ) + + decision_indexes = {index["name"] for index in inspector.get_indexes("decisions")} + if "idx_decisions_superseded" not in decision_indexes: + op.create_index( + "idx_decisions_superseded", + "decisions", + ["superseded_by"], + unique=False, + postgresql_where=sa.text("superseded_by IS NOT NULL"), + sqlite_where=sa.text("superseded_by IS NOT NULL"), + ) + + # Re-inspect after potential DDL changes above (link_type column, index). + inspector = _inspector() + checkpoint_fks = inspector.get_foreign_keys("checkpoint_metadata") + fk_signatures = { + ( + tuple(fk.get("constrained_columns") or []), + fk.get("referred_table"), + tuple(fk.get("referred_columns") or []), + ) + for fk in checkpoint_fks + } + + needs_decision_fk = ( + ("decision_id",), + "decisions", + ("decision_id",), + ) not in fk_signatures + needs_resource_fk = ( + ("resource_id",), + "resources", + ("resource_id",), + ) not in fk_signatures + + if needs_decision_fk or needs_resource_fk: + # Nullify orphan references before creating FK constraints so the + # migration does not fail on existing data with dangling IDs. + # Uses NOT EXISTS instead of NOT IN for better query-plan + # performance on large checkpoint_metadata tables. + op.execute( + sa.text( + """ + UPDATE checkpoint_metadata + SET decision_id = NULL + WHERE decision_id IS NOT NULL + AND NOT EXISTS ( + SELECT 1 FROM decisions d + WHERE d.decision_id = checkpoint_metadata.decision_id + ) + """ + ) + ) + op.execute( + sa.text( + """ + UPDATE checkpoint_metadata + SET resource_id = NULL + WHERE resource_id IS NOT NULL + AND NOT EXISTS ( + SELECT 1 FROM resources r + WHERE r.resource_id = checkpoint_metadata.resource_id + ) + """ + ) + ) + + # NOTE: Spec DDL (lines 45569, 45571) uses bare REFERENCES without + # an ON DELETE clause (default: NO ACTION / RESTRICT). We use + # ondelete="SET NULL" so that DecisionRepository.delete() and + # ResourceRepository.delete() do not raise IntegrityError when a + # parent decision or resource is removed while checkpoint rows + # still reference it. + with op.batch_alter_table("checkpoint_metadata") as batch_op: + if needs_decision_fk: + batch_op.create_foreign_key( + "fk_checkpoint_metadata_decision", + "decisions", + ["decision_id"], + ["decision_id"], + ondelete="SET NULL", + ) + if needs_resource_fk: + batch_op.create_foreign_key( + "fk_checkpoint_metadata_resource", + "resources", + ["resource_id"], + ["resource_id"], + ondelete="SET NULL", + ) + + if op.get_bind().dialect.name == "sqlite": + _create_sqlite_checkpoint_fk_triggers() + + +def downgrade() -> None: + """Revert spec-parity schema updates.""" + inspector = _inspector() + + if op.get_bind().dialect.name == "sqlite": + _drop_sqlite_checkpoint_fk_triggers() + + decision_indexes = {index["name"] for index in inspector.get_indexes("decisions")} + if "idx_decisions_superseded" in decision_indexes: + op.drop_index("idx_decisions_superseded", table_name="decisions") + + checkpoint_fks = inspector.get_foreign_keys("checkpoint_metadata") + fk_names = {fk.get("name") for fk in checkpoint_fks} + if ( + "fk_checkpoint_metadata_decision" in fk_names + or "fk_checkpoint_metadata_resource" in fk_names + ): + with op.batch_alter_table("checkpoint_metadata") as batch_op: + if "fk_checkpoint_metadata_decision" in fk_names: + batch_op.drop_constraint( + "fk_checkpoint_metadata_decision", + type_="foreignkey", + ) + if "fk_checkpoint_metadata_resource" in fk_names: + batch_op.drop_constraint( + "fk_checkpoint_metadata_resource", + type_="foreignkey", + ) + + resource_link_columns = { + column["name"] for column in inspector.get_columns("resource_links") + } + if "link_type" in resource_link_columns: + check_constraints = inspector.get_check_constraints("resource_links") + has_link_type_ck = any( + ck.get("name") == "ck_resource_links_link_type" for ck in check_constraints + ) + with op.batch_alter_table("resource_links") as batch_op: + if has_link_type_ck: + batch_op.drop_constraint("ck_resource_links_link_type", type_="check") + batch_op.drop_column("link_type") diff --git a/src/cleveragents/infrastructure/database/migrations/versions/m8_002_merge_profile_rename_and_corrections.py b/src/cleveragents/infrastructure/database/migrations/versions/m8_002_merge_profile_rename_and_corrections.py index 80b9c5148..64b8445d3 100644 --- a/src/cleveragents/infrastructure/database/migrations/versions/m8_002_merge_profile_rename_and_corrections.py +++ b/src/cleveragents/infrastructure/database/migrations/versions/m8_002_merge_profile_rename_and_corrections.py @@ -1,13 +1,14 @@ -"""Merge profile-rename, correction-attempts, and plans-schema heads. +"""Merge profile-rename, correction-attempts, plans-schema, and schema-parity heads. -m5_001_rename_profile_fields, m8_001_correction_attempts, and -m8_001_align_plans_schema all branched from m4_003_plan_env_columns, -creating three Alembic heads. This no-op merge migration resolves -them into a single head. +m5_001_rename_profile_fields, m8_001_correction_attempts, +m8_001_align_plans_schema, and m4_004_schema_parity_resource_decision_checkpoint +all branched from m4_003_plan_env_columns, creating four Alembic heads. +This no-op merge migration resolves them into a single head. Revision ID: m8_002_merge_profile_rename_and_corrections Revises: m5_001_rename_profile_fields, m8_001_correction_attempts, -m8_001_align_plans_schema + m8_001_align_plans_schema, + m4_004_schema_parity_resource_decision_checkpoint Create Date: 2026-03-29 00:00:00 """ @@ -20,6 +21,7 @@ down_revision: str | Sequence[str] | None = ( "m5_001_rename_profile_fields", "m8_001_correction_attempts", "m8_001_align_plans_schema", + "m4_004_schema_parity_resource_decision_checkpoint", ) branch_labels: str | Sequence[str] | None = None depends_on: str | Sequence[str] | None = None diff --git a/src/cleveragents/infrastructure/database/models.py b/src/cleveragents/infrastructure/database/models.py index 293ad27a2..d29a8f6d5 100644 --- a/src/cleveragents/infrastructure/database/models.py +++ b/src/cleveragents/infrastructure/database/models.py @@ -1736,6 +1736,18 @@ class ResourceLinkModel(Base): # type: ignore[misc] primary_key=True, ) + # Link type metadata aligned with spec DDL (line 45549). + # NOTE: Spec DDL defines link_type as nullable (TEXT DEFAULT 'contains' + # without NOT NULL). We enforce NOT NULL because a NULL link type is + # semantically meaningless and the sibling resource_edges table also + # requires a non-NULL link_type. + link_type = Column( + Text, + nullable=False, + default="contains", + server_default=text("'contains'"), + ) + # Timestamp (ISO-8601 string) created_at = Column(String(30), nullable=False) @@ -1744,6 +1756,14 @@ class ResourceLinkModel(Base): # type: ignore[misc] "parent_id != child_id", name="ck_resource_links_no_self_loop", ), + # NOTE: Spec DDL (line 45549) does not declare a CHECK constraint on + # link_type (the allowed values appear only in a comment). We add this + # CHECK to match the sibling ck_resource_edges_link_type constraint and + # prevent invalid link types at the database level. + CheckConstraint( + "link_type IN ('contains', 'references', 'derived_from')", + name="ck_resource_links_link_type", + ), Index("ix_resource_links_child", "child_id"), Index("ix_resource_links_parent", "parent_id"), ) @@ -2791,6 +2811,12 @@ class DecisionModel(Base): # type: ignore[misc] Index("ix_decisions_type", "decision_type"), Index("ix_decisions_created", "created_at"), Index("ix_decisions_plan_seq", "plan_id", "sequence_number"), + Index( + "idx_decisions_superseded", + "superseded_by", + postgresql_where=text("superseded_by IS NOT NULL"), + sqlite_where=text("superseded_by IS NOT NULL"), + ), ) # --- Domain conversion --- @@ -3001,14 +3027,27 @@ class CheckpointModel(Base): # type: ignore[misc] nullable=False, ) - # Optional FK to decisions (decision-aligned checkpoints) - decision_id = Column(String(26), nullable=True) + # Optional FK to decisions (decision-aligned checkpoints). + # NOTE: Spec DDL (line 45569) uses bare REFERENCES without ON DELETE. + # We use ondelete="SET NULL" so that deleting a decision does not raise + # IntegrityError when checkpoint rows still reference it. + decision_id = Column( + String(26), + ForeignKey("decisions.decision_id", ondelete="SET NULL"), + nullable=True, + ) # Checkpoint type: pre_write, post_step, or manual checkpoint_type = Column(Text, nullable=False, default="manual") - # Optional resource association - resource_id = Column(String(26), nullable=True) + # Optional resource association. + # NOTE: Same ondelete="SET NULL" rationale as decision_id above + # (spec line 45571 uses bare REFERENCES). + resource_id = Column( + String(26), + ForeignKey("resources.resource_id", ondelete="SET NULL"), + nullable=True, + ) # Sandbox reference (e.g. git commit hash) sandbox_ref = Column(Text, nullable=False)