From a4a6b061a60bd819fc23b171fbc4cdfa66988854 Mon Sep 17 00:00:00 2001 From: CoreRasurae Date: Thu, 19 Mar 2026 19:33:20 +0000 Subject: [PATCH] fix(db): align v3_plans schema with specification DDL MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Aligned v3_plans table with specification DDL: 1. Added effective_profile_snapshot column (TEXT NOT NULL) for storing frozen JSON snapshot of automation profile at plan creation time. Added Pydantic field_validator ensuring the value is well-formed JSON. Validator catches RecursionError for deeply nested JSON, consistent with automation_profile deserialization hardening. Validator error message uses length-only to avoid potential information disclosure. Documented that the default "{}" exists for backward compatibility; new plans should explicitly set the snapshot. 2. Made root_plan_id NOT NULL — root plans self-reference their own plan_id, child plans reference the root ancestor. Added explicit ondelete="RESTRICT" FK policy for consistency with other FKs in the model. Documented known FK policy drift between ORM model (RESTRICT) and migrated databases (retained SET NULL) in the migration; data integrity is preserved by the NOT NULL constraint regardless. Moved root_plan_id self-reference resolution into a PlanIdentity model_validator so the domain model is consistent with the DB NOT NULL constraint before and after persistence (previously the resolution only happened in from_domain(), creating an asymmetry where root_plan_id was None in-memory but non-null after round-tripping through the database). 3. Made automation_profile NOT NULL with default "balanced". 4. Documented intentional deviation: phase default is "action" (code) vs "strategize" (spec) because the Action phase was added as a pre-Strategize setup step. 5. Created Alembic migration with backfill logic for existing rows. Root-ancestor backfill uses level-by-level propagation with a parent-readiness guard to correctly resolve plans at arbitrary hierarchy depth (3+ levels). Added safety bound (max 100 iterations) with logged error on exhaustion to guard against cycles in parent_plan_id. Merged batch_alter_table operations to avoid redundant full-table copies in SQLite batch mode. Migration backfill also handles empty-string automation_profile values. Documented downgrade limitation (backfill is not reversible). Orphan-row fallback now logs affected row count at WARNING level. Migration cycle-detection now logs affected plan_id values before the orphan fallback overwrites them. All migration SQL uses sa.text() for consistency with SQLAlchemy best practices. 6. Hardened automation_profile deserialization in to_domain() to catch ValueError (invalid StrEnum provenance), Pydantic ValidationError, and RecursionError (deeply nested JSON) in addition to JSONDecodeError and KeyError, preventing unreadable plans from corrupted DB rows. Applied the same defensive deserialization pattern to effective_profile_snapshot in to_domain(): corrupted JSON falls back to '{}' with a WARNING log instead of crashing the read path. Added TypeError to the effective_profile_snapshot exception list in to_domain() for consistency with the Pydantic validator. Logging of unparseable values uses length only to avoid potential information disclosure. 7. Used explicit None check (is not None) instead of truthiness for root_plan_id resolution in from_domain(), for effective_profile_snapshot in to_domain(), and in _serialize_automation_profile() for consistency. 8. Documented intentional column naming conventions vs spec DDL (e.g. automation_profile vs automation_profile_name, *_actor vs *_actor_name, processing_state vs state, v3_plans vs plans). Documented the semantic difference: automation_profile stores either a bare name or structured JSON with provenance, whereas the spec automation_profile_name stores a plain name. 9. Fixed benchmark plan constructors (plan_phase_migration_bench.py) that were missing the now- required root_plan_id and effective_profile_snapshot fields. 10. Replaced defensive getattr() with direct attribute access for effective_profile_snapshot in from_domain() and update(), since the field is now defined on the Plan domain model. 11. Fixed Any type annotation in test helper _make_plan() to use AutomationProfileRef | None for proper type safety. 12. Added BDD scenarios for PlanIdentity self-reference resolution, NULL effective_profile_snapshot constraint enforcement, valid-JSON-missing-profile_name-key deserialization, invalid-JSON and empty-string snapshot rejection by Pydantic validator, and corrupted effective_profile_snapshot DB fallback in to_domain(). 13. Extracted default automation profile name to a module-level constant (DEFAULT_AUTOMATION_PROFILE) to reduce sentinel duplication across models.py and repositories.py. 14. Centralised automation-profile serialisation into LifecyclePlanModel._serialize_automation_profile() to eliminate duplication between from_domain() and LifecyclePlanRepository.update(). 15. Fixed to_domain() root_plan_id type cast from str | None to str, reflecting the NOT NULL column constraint. 16. Added PlanIdentity model_validator that resolves None root_plan_id to plan_id at domain construction time, ensuring the domain model honours the spec DDL NOT NULL constraint regardless of persistence state. Simplified from_domain() root resolution accordingly. ISSUES CLOSED: #921 --- CHANGELOG.md | 40 ++ alembic/versions/m8_001_align_plans_schema.py | 152 +++++ ...02_merge_profile_rename_and_corrections.py | 12 +- benchmarks/plan_phase_migration_bench.py | 15 +- features/plans_table_schema_alignment.feature | 139 +++++ .../steps/action_repository_coverage_steps.py | 5 +- .../database_models_coverage_boost_steps.py | 10 + .../database_models_coverage_r2_steps.py | 13 +- ...atabase_models_lifecycle_coverage_steps.py | 7 +- .../database_models_new_coverage_steps.py | 1 + .../models_lifecycle_coverage_r2_steps.py | 5 +- .../steps/plan_lifecycle_coverage_steps.py | 2 + features/steps/plan_phase_migration_steps.py | 4 + .../plans_table_schema_alignment_steps.py | 539 ++++++++++++++++++ .../steps/repositories_coverage_r2_steps.py | 2 + features/steps/repositories_coverage_steps.py | 5 +- ...ositories_error_handling_coverage_steps.py | 1 + robot/helper_plans_table_schema_alignment.py | 394 +++++++++++++ robot/plans_table_schema_alignment.robot | 83 +++ src/cleveragents/domain/models/core/plan.py | 42 ++ .../infrastructure/database/models.py | 127 ++++- .../infrastructure/database/repositories.py | 10 +- 22 files changed, 1569 insertions(+), 39 deletions(-) create mode 100644 alembic/versions/m8_001_align_plans_schema.py create mode 100644 features/plans_table_schema_alignment.feature create mode 100644 features/steps/plans_table_schema_alignment_steps.py create mode 100644 robot/helper_plans_table_schema_alignment.py create mode 100644 robot/plans_table_schema_alignment.robot diff --git a/CHANGELOG.md b/CHANGELOG.md index 785623c71..8a7b3991f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -436,6 +436,46 @@ in `execute_plan()` for consistency with phase-state validator. Updated `PlanResumeService` docstring to reflect ERRORED terminality. (#918) +- Aligned `v3_plans` table schema with specification DDL: added + `effective_profile_snapshot` column (TEXT NOT NULL, validated as JSON), + made `root_plan_id` NOT NULL with self-referencing for root plans and + explicit `RESTRICT` FK policy, made `automation_profile` NOT NULL with + default `"balanced"`, and documented the intentional `phase` default + deviation (`"action"` vs spec `"strategize"`). Migration backfill + correctly resolves root ancestors for child plans at arbitrary hierarchy + depth via level-by-level propagation with safety bound. Hardened + `automation_profile` deserialization to catch `RecursionError`. + Hardened `effective_profile_snapshot` deserialization in `to_domain()` + to gracefully fall back to `'{}'` on corrupted JSON, preventing a + single corrupted row from crashing plan reads. Added `RecursionError` + to `effective_profile_snapshot` Pydantic validator for consistency + with `automation_profile` deserialization. Validator error message + now uses length-only to avoid potential information disclosure. + Documented intentional column naming conventions vs spec DDL. + Migration orphan-row fallback now logs affected row count. + Documented FK ondelete policy drift between ORM and migrated schemas. + Documented `automation_profile` dual-format storage semantic. + Added `TypeError` to `effective_profile_snapshot` deserialization + exception list in `to_domain()` for consistency with the Pydantic + validator. Extracted default automation profile name to a + module-level constant (`DEFAULT_AUTOMATION_PROFILE`) to reduce + sentinel duplication across `models.py` and `repositories.py`. + Migration cycle-detection now logs affected `plan_id` values + (truncated to first 50) before the orphan fallback runs. + Migration SQL statements + uniformly use `sa.text()` for consistency. Centralised + automation-profile serialisation into + `LifecyclePlanModel._serialize_automation_profile()` to + eliminate duplication between `from_domain()` and + `LifecyclePlanRepository.update()`. + Moved `root_plan_id` self-reference resolution from + `from_domain()` into a `PlanIdentity` `model_validator` so + the domain model is consistent with the DB ``NOT NULL`` + constraint before and after persistence. + Used explicit ``is not None`` check in + `_serialize_automation_profile()` for consistency with the + explicit-None-check convention used elsewhere in this commit. + (#921) - Fixed `shell=True` subprocess usage in `cli_coverage_steps.py` by replacing with `shlex.split()` and `shell=False` for defense-in-depth command injection prevention, consistent with the existing pattern in diff --git a/alembic/versions/m8_001_align_plans_schema.py b/alembic/versions/m8_001_align_plans_schema.py new file mode 100644 index 000000000..c395c6d16 --- /dev/null +++ b/alembic/versions/m8_001_align_plans_schema.py @@ -0,0 +1,152 @@ +"""Align v3_plans table schema with specification DDL. + +- Add effective_profile_snapshot column (TEXT NOT NULL DEFAULT '{}'). +- Make root_plan_id NOT NULL (backfill with plan_id first). +- Make automation_profile NOT NULL (backfill with 'balanced' first). + +Note on downgrade: the backfill is not reversible. After downgrade, +root plans that previously had ``root_plan_id IS NULL`` will retain +the self-referencing value (``root_plan_id = plan_id``). + +Revision ID: m8_001_align_plans_schema +Revises: m4_003_plan_env_columns +Create Date: 2026-03-19 00:00:00 + +""" + +import logging +from collections.abc import Sequence + +import sqlalchemy as sa +from alembic import op + +_logger = logging.getLogger(__name__) + +# revision identifiers, used by Alembic. +revision: str = "m8_001_align_plans_schema" +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 upgrade() -> None: + """Align v3_plans with spec DDL: new column + nullability fixes.""" + # 1. Add effective_profile_snapshot column + op.add_column( + "v3_plans", + sa.Column( + "effective_profile_snapshot", + sa.Text(), + nullable=False, + server_default="{}", + ), + ) + + # 2. Backfill root_plan_id: root plans (no parent) self-reference; + # child plans inherit root from their parent's root_plan_id. + # The child backfill propagates level-by-level: each iteration + # only updates children whose parent already has root_plan_id + # resolved. This avoids the snapshot-semantics pitfall where a + # single correlated UPDATE would give grandchildren their + # parent's plan_id instead of the root ancestor's ID. + op.execute( + sa.text( + "UPDATE v3_plans SET root_plan_id = plan_id " + "WHERE root_plan_id IS NULL AND parent_plan_id IS NULL" + ) + ) + bind = op.get_bind() + _MAX_DEPTH = 100 + for _depth in range(_MAX_DEPTH): + result = bind.execute( + sa.text( + "UPDATE v3_plans SET root_plan_id = (" + " SELECT p.root_plan_id" + " FROM v3_plans p WHERE p.plan_id = v3_plans.parent_plan_id" + ") WHERE root_plan_id IS NULL" + " AND parent_plan_id IS NOT NULL" + " AND parent_plan_id IN (" + " SELECT p2.plan_id FROM v3_plans p2" + " WHERE p2.root_plan_id IS NOT NULL" + " )" + ) + ) + if result.rowcount == 0: + break + else: + # Log affected plan IDs before the fallback overwrites them. + cycle_rows = bind.execute( + sa.text("SELECT plan_id FROM v3_plans WHERE root_plan_id IS NULL") + ).fetchall() + cycle_ids = [r[0] for r in cycle_rows] + _MAX_LOG_IDS = 50 + display_ids = cycle_ids[:_MAX_LOG_IDS] + _logger.error( + "root_plan_id backfill did not converge after %d iterations " + "— possible cycle in parent_plan_id; affected count=%d, " + "plan_ids (first %d)=%s", + _MAX_DEPTH, + len(cycle_ids), + _MAX_LOG_IDS, + display_ids, + ) + # Fallback: any remaining NULLs (orphaned children) self-reference. + fallback_result = bind.execute( + sa.text("UPDATE v3_plans SET root_plan_id = plan_id WHERE root_plan_id IS NULL") + ) + if fallback_result.rowcount > 0: + _logger.warning( + "root_plan_id fallback: %d orphaned row(s) self-referenced " + "(parent_plan_id points to a missing plan)", + fallback_result.rowcount, + ) + + # 3. Backfill automation_profile with 'balanced' where NULL or empty. + op.execute( + sa.text( + "UPDATE v3_plans SET automation_profile = 'balanced' " + "WHERE automation_profile IS NULL OR automation_profile = ''" + ) + ) + + # 4. Apply NOT NULL constraints in a single batch to avoid + # two full table copies in SQLite batch mode. + # + # NOTE: The ORM model specifies ondelete="RESTRICT" for the + # root_plan_id FK, but SQLite batch_alter_table recreates the + # table from reflected metadata and does not update FK policies + # unless explicitly told to. Fresh databases created via + # Base.metadata.create_all() get the correct RESTRICT policy + # from the ORM model. Migrated databases retain the previous + # FK policy. This is acceptable because SQLite disables FK + # enforcement by default (PRAGMA foreign_keys = OFF) and the + # NOT NULL constraint on root_plan_id independently prevents + # the SET NULL action from succeeding. + with op.batch_alter_table("v3_plans") as batch_op: + batch_op.alter_column( + "root_plan_id", existing_type=sa.String(26), nullable=False + ) + batch_op.alter_column( + "automation_profile", + existing_type=sa.Text(), + nullable=False, + server_default="balanced", + ) + + +def downgrade() -> None: + """Revert v3_plans schema changes.""" + # Reverse order: make columns nullable again, drop new column. + # Single batch to minimise table copies. + with op.batch_alter_table("v3_plans") as batch_op: + batch_op.alter_column( + "automation_profile", + existing_type=sa.Text(), + nullable=True, + server_default=None, + ) + batch_op.alter_column( + "root_plan_id", existing_type=sa.String(26), nullable=True + ) + + op.drop_column("v3_plans", "effective_profile_snapshot") diff --git a/alembic/versions/m8_002_merge_profile_rename_and_corrections.py b/alembic/versions/m8_002_merge_profile_rename_and_corrections.py index d0c5c3da7..cefa13d51 100644 --- a/alembic/versions/m8_002_merge_profile_rename_and_corrections.py +++ b/alembic/versions/m8_002_merge_profile_rename_and_corrections.py @@ -1,11 +1,12 @@ -"""Merge profile-rename and correction-attempts heads. +"""Merge profile-rename, correction-attempts, and plans-schema heads. -Both m5_001_rename_profile_fields and m8_001_correction_attempts branched -from m4_003_plan_env_columns, creating two Alembic heads. This no-op -merge migration resolves them into a single head. +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. Revision ID: m8_002_merge_profile_rename_and_corrections -Revises: m5_001_rename_profile_fields, m8_001_correction_attempts +Revises: m5_001_rename_profile_fields, m8_001_correction_attempts, m8_001_align_plans_schema Create Date: 2026-03-29 00:00:00 """ @@ -17,6 +18,7 @@ revision: str = "m8_002_merge_profile_rename_and_corrections" down_revision: str | Sequence[str] | None = ( "m5_001_rename_profile_fields", "m8_001_correction_attempts", + "m8_001_align_plans_schema", ) branch_labels: str | Sequence[str] | None = None depends_on: str | Sequence[str] | None = None diff --git a/benchmarks/plan_phase_migration_bench.py b/benchmarks/plan_phase_migration_bench.py index ac0c1d5f1..29478420b 100644 --- a/benchmarks/plan_phase_migration_bench.py +++ b/benchmarks/plan_phase_migration_bench.py @@ -86,7 +86,9 @@ class PlanInsertActionPhase: now = _now_iso() for i in range(100): plan = LifecyclePlanModel() - plan.plan_id = _make_ulid(base + i) + pid = _make_ulid(base + i) + plan.plan_id = pid + plan.root_plan_id = pid plan.action_name = "bench/phase-action" plan.namespaced_name = f"bench/plan-{it}-{i}" plan.namespace = "bench" @@ -94,6 +96,7 @@ class PlanInsertActionPhase: plan.processing_state = "queued" plan.description = f"Benchmark plan {it}-{i}" plan.tags_json = "[]" + plan.effective_profile_snapshot = "{}" plan.created_at = now plan.updated_at = now session.add(plan) @@ -136,7 +139,9 @@ class PlanInsertApplyTerminalStates: now = _now_iso() for i in range(50): plan = LifecyclePlanModel() - plan.plan_id = _make_ulid(base + i) + pid = _make_ulid(base + i) + plan.plan_id = pid + plan.root_plan_id = pid plan.action_name = "bench/terminal-action" plan.namespaced_name = f"bench/applied-{it}-{i}" plan.namespace = "bench" @@ -144,6 +149,7 @@ class PlanInsertApplyTerminalStates: plan.processing_state = "applied" plan.description = f"Applied plan {it}-{i}" plan.tags_json = "[]" + plan.effective_profile_snapshot = "{}" plan.created_at = now plan.updated_at = now session.add(plan) @@ -157,7 +163,9 @@ class PlanInsertApplyTerminalStates: now = _now_iso() for i in range(50): plan = LifecyclePlanModel() - plan.plan_id = _make_ulid(base + i) + pid = _make_ulid(base + i) + plan.plan_id = pid + plan.root_plan_id = pid plan.action_name = "bench/terminal-action" plan.namespaced_name = f"bench/constrained-{it}-{i}" plan.namespace = "bench" @@ -165,6 +173,7 @@ class PlanInsertApplyTerminalStates: plan.processing_state = "constrained" plan.description = f"Constrained plan {it}-{i}" plan.tags_json = "[]" + plan.effective_profile_snapshot = "{}" plan.created_at = now plan.updated_at = now session.add(plan) diff --git a/features/plans_table_schema_alignment.feature b/features/plans_table_schema_alignment.feature new file mode 100644 index 000000000..b719318db --- /dev/null +++ b/features/plans_table_schema_alignment.feature @@ -0,0 +1,139 @@ +@feature921 +Feature: v3_plans table schema alignment with spec DDL + As a database administrator + I want the v3_plans table schema to match the specification DDL + So that nullability, defaults, and columns are correct + + Background: + Given a schema-aligned fresh in-memory database + And a schema-aligned prerequisite action "local/schema-test" exists + + @feature921 + Scenario: Plan creation includes effective_profile_snapshot + Given a schema-aligned plan with ID "01HV000000000000000000SA01" + And the plan effective_profile_snapshot is '{"profile":"balanced","thresholds":{}}' + When I persist the schema-aligned plan + Then the retrieved plan effective_profile_snapshot should be '{"profile":"balanced","thresholds":{}}' + + @feature921 + Scenario: Plan creation with default effective_profile_snapshot + Given a schema-aligned plan with ID "01HV000000000000000000SA02" + When I persist the schema-aligned plan + Then the retrieved plan effective_profile_snapshot should be '{}' + + @feature921 + Scenario: Root plan has self-referencing root_plan_id + Given a schema-aligned root plan with ID "01HV000000000000000000SA03" + When I persist the schema-aligned plan + Then the plan root_plan_id should equal the plan_id "01HV000000000000000000SA03" + + @feature921 + Scenario: Child plan has parent root_plan_id + Given a schema-aligned root plan with ID "01HV000000000000000000SA04" + And a schema-aligned child plan with ID "01HV000000000000000000SA05" parent "01HV000000000000000000SA04" root "01HV000000000000000000SA04" + When I persist both schema-aligned plans + Then the child plan root_plan_id should be "01HV000000000000000000SA04" + And the child plan parent_plan_id should be "01HV000000000000000000SA04" + + @feature921 + Scenario: automation_profile defaults to balanced for plans without profile + Given a schema-aligned plan with ID "01HV000000000000000000SA06" and no automation profile + When I persist the schema-aligned plan + Then the persisted automation_profile column should be "balanced" + + @feature921 + Scenario: Bare-string automation_profile deserializes gracefully + Given a schema-aligned plan with ID "01HV000000000000000000SA07" + And the plan automation_profile column is set to bare string "full-auto" + When I load the plan from the database + Then the plan automation_profile ref should be None + + @feature921 + Scenario: Invalid provenance in automation_profile JSON deserializes gracefully + Given a schema-aligned plan with ID "01HV000000000000000000SA08" + And the plan automation_profile column is set to '{"profile_name":"test","provenance":"bogus"}' + When I load the plan from the database + Then the plan automation_profile ref should be None + + @feature921 + Scenario: Grandchild plan carries root ancestor plan_id + Given a schema-aligned root plan with ID "01HV000000000000000000SA10" + And a schema-aligned child plan with ID "01HV000000000000000000SA11" parent "01HV000000000000000000SA10" root "01HV000000000000000000SA10" + And a schema-aligned grandchild plan with ID "01HV000000000000000000SA12" parent "01HV000000000000000000SA11" root "01HV000000000000000000SA10" + When I persist root, child, and grandchild schema-aligned plans + Then the grandchild plan root_plan_id should be "01HV000000000000000000SA10" + And the grandchild plan parent_plan_id should be "01HV000000000000000000SA11" + + @feature921 + Scenario: automation_profile None roundtrips as None through domain model + Given a schema-aligned plan with ID "01HV000000000000000000SA13" and no automation profile + When I persist the schema-aligned plan + Then the persisted automation_profile column should be "balanced" + And the plan automation_profile ref should be None + + @feature921 + Scenario: Truncated JSON in automation_profile deserializes gracefully + Given a schema-aligned plan with ID "01HV000000000000000000SA14" + And the plan automation_profile column is set to '{"profile_name":"test"' + When I load the plan from the database + Then the plan automation_profile ref should be None + + @feature921 + Scenario: Updating effective_profile_snapshot persists correctly + Given a schema-aligned plan with ID "01HV000000000000000000SA15" + And the plan effective_profile_snapshot is '{"profile":"balanced","thresholds":{}}' + When I persist the schema-aligned plan + And I update the plan effective_profile_snapshot to '{"profile":"full-auto","thresholds":{"strategy":0.5}}' + Then the retrieved plan effective_profile_snapshot should be '{"profile":"full-auto","thresholds":{"strategy":0.5}}' + + @feature921 + Scenario: AutomationProfileRef round-trips through persistence + Given a schema-aligned plan "01HV000000000000000000SA16" with profile "supervised" and provenance "plan" + When I persist the schema-aligned plan + Then the plan automation_profile name should be "supervised" + And the plan automation_profile provenance should be "plan" + + @feature921 + Scenario: NULL root_plan_id is rejected by the database + Given a schema-aligned plan with ID "01HV000000000000000000SA09" forced NULL root_plan_id + When I attempt to persist the plan with NULL root_plan_id + Then the database should reject the insert with an integrity error + + @feature921 + Scenario: NULL effective_profile_snapshot is rejected by the database + Given a schema-aligned plan with ID "01HV000000000000000000SA17" forced NULL effective_profile_snapshot + When I attempt to persist the plan with NULL effective_profile_snapshot + Then the database should reject the snapshot insert with an integrity error + + @feature921 + Scenario: PlanIdentity resolves None root_plan_id to plan_id at construction + Given a schema-aligned plan with ID "01HV000000000000000000SA18" and None root_plan_id + Then the domain plan root_plan_id should already equal "01HV000000000000000000SA18" + When I convert the plan via from_domain + Then the ORM model root_plan_id should equal "01HV000000000000000000SA18" + + @feature921 + Scenario: Valid JSON missing profile_name key deserializes gracefully + Given a schema-aligned plan with ID "01HV000000000000000000SA19" + And the plan automation_profile column is set to '{"provenance":"plan"}' + When I load the plan from the database + Then the plan automation_profile ref should be None + + @feature921 + Scenario: Corrupted effective_profile_snapshot in DB falls back to empty JSON + Given a schema-aligned plan with ID "01HV000000000000000000SA20" + And the plan effective_profile_snapshot is '{"profile":"balanced"}' + When I persist the schema-aligned plan + And the effective_profile_snapshot column is corrupted to "not-valid-json{" + And I reload the plan from the database + Then the retrieved plan effective_profile_snapshot should be '{}' + + @feature921 + Scenario: Invalid JSON in effective_profile_snapshot is rejected by Plan model + When I attempt to construct a Plan with effective_profile_snapshot "not-valid-json" + Then the Plan construction should raise a validation error for effective_profile_snapshot + + @feature921 + Scenario: Empty string effective_profile_snapshot is rejected by Plan model + When I attempt to construct a Plan with an empty effective_profile_snapshot + Then the Plan construction should raise a validation error for effective_profile_snapshot diff --git a/features/steps/action_repository_coverage_steps.py b/features/steps/action_repository_coverage_steps.py index f7c75bf38..fca7db8fb 100644 --- a/features/steps/action_repository_coverage_steps.py +++ b/features/steps/action_repository_coverage_steps.py @@ -395,8 +395,10 @@ def step_create_referencing_plan(context: Context) -> None: """Insert a LifecyclePlanModel row that references the saved action.""" session = _get_session(context) now_iso = datetime.now().isoformat() + pid = _next_ulid() plan_model = LifecyclePlanModel( - plan_id=_next_ulid(), + plan_id=pid, + root_plan_id=pid, action_name=str(context.action.namespaced_name), phase="strategize", processing_state="queued", @@ -404,6 +406,7 @@ def step_create_referencing_plan(context: Context) -> None: namespaced_name="local/test-plan", namespace="local", description="Plan referencing the action under test", + effective_profile_snapshot="{}", created_at=now_iso, updated_at=now_iso, tags_json="[]", diff --git a/features/steps/database_models_coverage_boost_steps.py b/features/steps/database_models_coverage_boost_steps.py index 8050694dc..92c5af1e0 100644 --- a/features/steps/database_models_coverage_boost_steps.py +++ b/features/steps/database_models_coverage_boost_steps.py @@ -403,6 +403,7 @@ def step_plan_model_with_automation_profile(context): ) model = LifecyclePlanModel( plan_id=ULID_PLAN_1, + root_plan_id=ULID_PLAN_1, action_name="local/cov-action", namespaced_name="local/profile-plan", namespace="local", @@ -414,6 +415,7 @@ def step_plan_model_with_automation_profile(context): strategy_actor="local/strategy", execution_actor="local/executor", automation_profile=profile_json, + effective_profile_snapshot="{}", reusable=True, read_only=False, created_at=NOW_ISO, @@ -459,6 +461,7 @@ def step_plan_model_with_sandbox_refs(context): _ensure_action_exists(context.cb_session) model = LifecyclePlanModel( plan_id=ULID_PLAN_2, + root_plan_id=ULID_PLAN_2, action_name="local/cov-action", namespaced_name="local/sandbox-plan", namespace="local", @@ -470,6 +473,7 @@ def step_plan_model_with_sandbox_refs(context): strategy_actor="local/strategy", execution_actor="local/executor", sandbox_refs_json=json.dumps(["sandbox-ref-1", "sandbox-ref-2"]), + effective_profile_snapshot="{}", reusable=True, read_only=False, created_at=NOW_ISO, @@ -502,6 +506,7 @@ def step_plan_model_with_validation_summary(context): plan_id = str(ULID()) model = LifecyclePlanModel( plan_id=plan_id, + root_plan_id=plan_id, action_name="local/cov-action", namespaced_name="local/validation-plan", namespace="local", @@ -513,6 +518,7 @@ def step_plan_model_with_validation_summary(context): strategy_actor="local/strategy", execution_actor="local/executor", validation_summary_json=json.dumps(summary), + effective_profile_snapshot="{}", reusable=True, read_only=False, created_at=NOW_ISO, @@ -546,6 +552,7 @@ def step_plan_model_with_error_details(context): plan_id = str(ULID()) model = LifecyclePlanModel( plan_id=plan_id, + root_plan_id=plan_id, action_name="local/cov-action", namespaced_name="local/error-plan", namespace="local", @@ -558,6 +565,7 @@ def step_plan_model_with_error_details(context): execution_actor="local/executor", error_message="Something went wrong", error_details_json=json.dumps(error_details), + effective_profile_snapshot="{}", reusable=True, read_only=False, created_at=NOW_ISO, @@ -590,6 +598,7 @@ def step_plan_model_with_arguments(context): plan_id = str(ULID()) model = LifecyclePlanModel( plan_id=plan_id, + root_plan_id=plan_id, action_name="local/cov-action", namespaced_name="local/args-plan", namespace="local", @@ -600,6 +609,7 @@ def step_plan_model_with_arguments(context): definition_of_done="Tests pass", strategy_actor="local/strategy", execution_actor="local/executor", + effective_profile_snapshot="{}", reusable=True, read_only=False, created_at=NOW_ISO, diff --git a/features/steps/database_models_coverage_r2_steps.py b/features/steps/database_models_coverage_r2_steps.py index 544712753..eab689a65 100644 --- a/features/steps/database_models_coverage_r2_steps.py +++ b/features/steps/database_models_coverage_r2_steps.py @@ -14,6 +14,11 @@ from typing import Any from behave import given, then, when # type: ignore[import-untyped] +from cleveragents.domain.models.core.plan import ( + AutomationProfileProvenance, + AutomationProfileRef, +) + # --------------------------------------------------------------------------- # Helpers # --------------------------------------------------------------------------- @@ -234,7 +239,7 @@ def step_check_arg_strings(context: Any) -> None: def _base_plan_ns( *, processing_state: Any = "queued", - automation_profile: Any = None, + automation_profile: AutomationProfileRef | None = None, namespaced_name: Any = None, error_details: Any = None, ) -> SimpleNamespace: @@ -272,6 +277,7 @@ def _base_plan_ns( tags=[], reusable=True, read_only=False, + effective_profile_snapshot="{}", ) @@ -301,11 +307,6 @@ def step_check_plan_state(context: Any) -> None: @given("a Plan domain object with a non-null automation_profile") def step_plan_with_automation_profile(context: Any) -> None: - from cleveragents.domain.models.core.plan import ( - AutomationProfileProvenance, - AutomationProfileRef, - ) - profile = AutomationProfileRef( profile_name="strict", provenance=AutomationProfileProvenance.ACTION, diff --git a/features/steps/database_models_lifecycle_coverage_steps.py b/features/steps/database_models_lifecycle_coverage_steps.py index 4f63d811c..a2dc306c6 100644 --- a/features/steps/database_models_lifecycle_coverage_steps.py +++ b/features/steps/database_models_lifecycle_coverage_steps.py @@ -141,10 +141,12 @@ def _make_plan_model( - ``tags`` -> ``tags_json`` - Added ``namespace`` column """ + # Root plans self-reference; default when not explicitly provided. + resolved_root = root_plan_id if root_plan_id is not None else plan_id model = LifecyclePlanModel( plan_id=plan_id, parent_plan_id=parent_plan_id, - root_plan_id=root_plan_id, + root_plan_id=resolved_root, action_name=action_name, phase=phase, processing_state=state, @@ -169,6 +171,7 @@ def _make_plan_model( tags_json=tags, reusable=reusable, read_only=read_only, + effective_profile_snapshot="{}", ) # Populate project links via child table if project_names: @@ -845,6 +848,7 @@ def _make_plan_domain( tags=tags, reusable=True, read_only=False, + effective_profile_snapshot="{}", ) @@ -991,6 +995,7 @@ def step_create_plan_domain_null_states(context): tags=[], reusable=True, read_only=False, + effective_profile_snapshot="{}", ) diff --git a/features/steps/database_models_new_coverage_steps.py b/features/steps/database_models_new_coverage_steps.py index 2a4e82d57..77e4acb86 100644 --- a/features/steps/database_models_new_coverage_steps.py +++ b/features/steps/database_models_new_coverage_steps.py @@ -190,6 +190,7 @@ def _make_plan_object( tags=[], reusable=True, read_only=False, + effective_profile_snapshot="{}", ) diff --git a/features/steps/models_lifecycle_coverage_r2_steps.py b/features/steps/models_lifecycle_coverage_r2_steps.py index bbf423883..395d859b3 100644 --- a/features/steps/models_lifecycle_coverage_r2_steps.py +++ b/features/steps/models_lifecycle_coverage_r2_steps.py @@ -100,7 +100,7 @@ def _make_plan_model( model = LifecyclePlanModel( plan_id=_ULID_PLAN, parent_plan_id=None, - root_plan_id=None, + root_plan_id=_ULID_PLAN, action_name=action_name or "", namespaced_name="local/test-plan", namespace="local", @@ -115,7 +115,8 @@ def _make_plan_model( apply_actor=None, estimation_actor=None, invariant_actor=None, - automation_profile=automation_profile, + automation_profile=automation_profile or "balanced", + effective_profile_snapshot="{}", reusable=True, read_only=False, inputs_schema_json=None, diff --git a/features/steps/plan_lifecycle_coverage_steps.py b/features/steps/plan_lifecycle_coverage_steps.py index c92849fe9..be55bb884 100644 --- a/features/steps/plan_lifecycle_coverage_steps.py +++ b/features/steps/plan_lifecycle_coverage_steps.py @@ -725,6 +725,7 @@ def step_uow_add_flush(context: Context) -> None: # Create a raw model and add it via the context model = LifecyclePlanModel() model.plan_id = "01TESTADD00000000000000000" # type: ignore[assignment] + model.root_plan_id = "01TESTADD00000000000000000" # type: ignore[assignment] model.namespaced_name = "local/uow-add-test" # type: ignore[assignment] model.namespace = "local" # type: ignore[assignment] model.name = "uow-add-test" # type: ignore[assignment] @@ -740,6 +741,7 @@ def step_uow_add_flush(context: Context) -> None: model.read_only = False # type: ignore[assignment] model.tags_json = "[]" # type: ignore[assignment] model.sandbox_refs_json = "[]" # type: ignore[assignment] + model.effective_profile_snapshot = "{}" # type: ignore[assignment] now = datetime.now().isoformat() model.created_at = now # type: ignore[assignment] model.updated_at = now # type: ignore[assignment] diff --git a/features/steps/plan_phase_migration_steps.py b/features/steps/plan_phase_migration_steps.py index 70dda56f6..9d1efa751 100644 --- a/features/steps/plan_phase_migration_steps.py +++ b/features/steps/plan_phase_migration_steps.py @@ -87,6 +87,7 @@ def step_insert_plan_with_phase_and_state(context: Any, phase: str, state: str) now = _now_iso() plan = LifecyclePlanModel() plan.plan_id = ulid + plan.root_plan_id = ulid plan.action_name = "local/phase-test-action" plan.namespaced_name = "local/phase-test-plan" plan.namespace = "local" @@ -94,6 +95,7 @@ def step_insert_plan_with_phase_and_state(context: Any, phase: str, state: str) plan.processing_state = state plan.description = "Test plan" plan.tags_json = "[]" + plan.effective_profile_snapshot = "{}" plan.created_at = now plan.updated_at = now try: @@ -155,11 +157,13 @@ def step_insert_plan_default_phase(context: Any) -> None: # Use ORM to create plan without specifying phase — should default to 'action' plan = LifecyclePlanModel() plan.plan_id = ulid + plan.root_plan_id = ulid plan.action_name = "local/phase-test-action" plan.namespaced_name = "local/default-phase-plan" plan.namespace = "local" plan.description = "Default phase plan" plan.tags_json = "[]" + plan.effective_profile_snapshot = "{}" plan.created_at = now plan.updated_at = now session.add(plan) diff --git a/features/steps/plans_table_schema_alignment_steps.py b/features/steps/plans_table_schema_alignment_steps.py new file mode 100644 index 000000000..b2cd69a6e --- /dev/null +++ b/features/steps/plans_table_schema_alignment_steps.py @@ -0,0 +1,539 @@ +"""Step definitions for v3_plans table schema alignment (issue #921). + +Tests the effective_profile_snapshot column, root_plan_id self-referencing, +automation_profile NOT NULL default, bare-string deserialization, and +NULL root_plan_id constraint enforcement. +""" + +from __future__ import annotations + +from datetime import UTC, datetime + +from behave import given, then, when +from behave.runner import Context +from sqlalchemy import create_engine, text +from sqlalchemy.exc import IntegrityError +from sqlalchemy.orm import sessionmaker + +from cleveragents.domain.models.core.action import Action, ActionState +from cleveragents.domain.models.core.plan import ( + AutomationProfileProvenance, + AutomationProfileRef, + NamespacedName, + Plan, + PlanIdentity, + PlanPhase, + PlanTimestamps, + ProcessingState, +) +from cleveragents.infrastructure.database.models import ( + Base, + LifecyclePlanModel, +) +from cleveragents.infrastructure.database.repositories import ( + ActionRepository, + LifecyclePlanRepository, +) + +_NOW = datetime(2026, 3, 19, tzinfo=UTC) + + +def _make_plan( + plan_id: str, + *, + parent_plan_id: str | None = None, + root_plan_id: str | None = None, + effective_profile_snapshot: str = "{}", + automation_profile: AutomationProfileRef | None = None, + action_name: str = "local/schema-test", +) -> Plan: + """Build a Plan domain object for schema alignment tests.""" + return Plan( + identity=PlanIdentity( + plan_id=plan_id, + parent_plan_id=parent_plan_id, + root_plan_id=root_plan_id, + attempt=1, + ), + namespaced_name=NamespacedName(namespace="local", name="schema-test"), + action_name=action_name, + description="Schema alignment test plan", + definition_of_done="All assertions pass", + phase=PlanPhase.ACTION, + processing_state=ProcessingState.QUEUED, + automation_profile=automation_profile, + effective_profile_snapshot=effective_profile_snapshot, + strategy_actor="local/s", + execution_actor="local/e", + timestamps=PlanTimestamps(created_at=_NOW, updated_at=_NOW), + created_by="test-schema", + tags=[], + reusable=True, + read_only=False, + ) + + +def _setup_db(context: Context) -> None: + """Create a fresh in-memory SQLite database and attach to context.""" + engine = create_engine("sqlite:///:memory:", echo=False) + Base.metadata.create_all(engine) + sm = sessionmaker(bind=engine) + session = sm() + context._sa_engine = engine + context._sa_session = session + context._sa_session_factory = lambda: session + context._sa_plan_repo = LifecyclePlanRepository( + session_factory=context._sa_session_factory, + ) + context._sa_action_repo = ActionRepository( + session_factory=context._sa_session_factory, + ) + + +@given("a schema-aligned fresh in-memory database") +def step_setup_db(context: Context) -> None: + _setup_db(context) + + +@given('a schema-aligned prerequisite action "{action_name}" exists') +def step_create_action(context: Context, action_name: str) -> None: + ns = NamespacedName.parse(action_name) + action = Action( + namespaced_name=ns, + description="Schema alignment test action", + definition_of_done="Done", + strategy_actor="local/s", + execution_actor="local/e", + state=ActionState.AVAILABLE, + created_at=_NOW, + updated_at=_NOW, + ) + context._sa_action_repo.create(action) + context._sa_session.commit() + + +@given('a schema-aligned plan with ID "{plan_id}"') +def step_create_plan(context: Context, plan_id: str) -> None: + context._sa_plan = _make_plan(plan_id) + + +@given("the plan effective_profile_snapshot is '{snapshot}'") +def step_set_snapshot(context: Context, snapshot: str) -> None: + context._sa_plan = context._sa_plan.model_copy( + update={"effective_profile_snapshot": snapshot} + ) + + +@given('a schema-aligned root plan with ID "{plan_id}"') +def step_create_root_plan(context: Context, plan_id: str) -> None: + # root_plan_id is None here; from_domain will set it to plan_id + context._sa_plan = _make_plan(plan_id, root_plan_id=None) + + +@given( + 'a schema-aligned child plan with ID "{child_id}" parent "{parent_id}" root "{root_id}"' +) +def step_create_child_plan( + context: Context, child_id: str, parent_id: str, root_id: str +) -> None: + context._sa_child_plan = _make_plan( + child_id, parent_plan_id=parent_id, root_plan_id=root_id + ) + + +@given( + 'a schema-aligned grandchild plan with ID "{gc_id}" parent "{parent_id}" root "{root_id}"' +) +def step_create_grandchild_plan( + context: Context, gc_id: str, parent_id: str, root_id: str +) -> None: + context._sa_grandchild_plan = _make_plan( + gc_id, parent_plan_id=parent_id, root_plan_id=root_id + ) + + +@given('a schema-aligned plan with ID "{plan_id}" and no automation profile') +def step_create_plan_no_profile(context: Context, plan_id: str) -> None: + context._sa_plan = _make_plan(plan_id, automation_profile=None) + + +@when("I persist the schema-aligned plan") +def step_persist_plan(context: Context) -> None: + context._sa_plan_repo.create(context._sa_plan) + context._sa_session.commit() + plan_id = context._sa_plan.identity.plan_id + context._sa_retrieved = context._sa_plan_repo.get(plan_id) + + +@when("I persist both schema-aligned plans") +def step_persist_both(context: Context) -> None: + context._sa_plan_repo.create(context._sa_plan) + context._sa_session.commit() + context._sa_plan_repo.create(context._sa_child_plan) + context._sa_session.commit() + child_id = context._sa_child_plan.identity.plan_id + context._sa_retrieved_child = context._sa_plan_repo.get(child_id) + + +@when("I persist root, child, and grandchild schema-aligned plans") +def step_persist_three_levels(context: Context) -> None: + context._sa_plan_repo.create(context._sa_plan) + context._sa_session.commit() + context._sa_plan_repo.create(context._sa_child_plan) + context._sa_session.commit() + context._sa_plan_repo.create(context._sa_grandchild_plan) + context._sa_session.commit() + gc_id = context._sa_grandchild_plan.identity.plan_id + context._sa_retrieved_grandchild = context._sa_plan_repo.get(gc_id) + + +@when("I update the plan effective_profile_snapshot to '{new_snapshot}'") +def step_update_snapshot(context: Context, new_snapshot: str) -> None: + retrieved = context._sa_retrieved + assert retrieved is not None, "Plan was not retrieved before update" + updated = retrieved.model_copy(update={"effective_profile_snapshot": new_snapshot}) + context._sa_plan_repo.update(updated) + context._sa_session.commit() + plan_id = updated.identity.plan_id + context._sa_retrieved = context._sa_plan_repo.get(plan_id) + + +@then("the retrieved plan effective_profile_snapshot should be '{expected}'") +def step_check_snapshot(context: Context, expected: str) -> None: + retrieved = context._sa_retrieved + assert retrieved is not None, "Plan was not retrieved" + assert retrieved.effective_profile_snapshot == expected, ( + f"Expected snapshot '{expected}', got '{retrieved.effective_profile_snapshot}'" + ) + + +@then('the plan root_plan_id should equal the plan_id "{plan_id}"') +def step_check_root_self_ref(context: Context, plan_id: str) -> None: + retrieved = context._sa_retrieved + assert retrieved is not None, "Plan was not retrieved" + assert retrieved.identity.root_plan_id == plan_id, ( + f"Expected root_plan_id '{plan_id}', got '{retrieved.identity.root_plan_id}'" + ) + + +@then('the child plan root_plan_id should be "{expected}"') +def step_check_child_root(context: Context, expected: str) -> None: + child = context._sa_retrieved_child + assert child is not None, "Child plan was not retrieved" + assert child.identity.root_plan_id == expected, ( + f"Expected root_plan_id '{expected}', got '{child.identity.root_plan_id}'" + ) + + +@then('the grandchild plan root_plan_id should be "{expected}"') +def step_check_grandchild_root(context: Context, expected: str) -> None: + gc = context._sa_retrieved_grandchild + assert gc is not None, "Grandchild plan was not retrieved" + assert gc.identity.root_plan_id == expected, ( + f"Expected root_plan_id '{expected}', got '{gc.identity.root_plan_id}'" + ) + + +@then('the grandchild plan parent_plan_id should be "{expected}"') +def step_check_grandchild_parent(context: Context, expected: str) -> None: + gc = context._sa_retrieved_grandchild + assert gc is not None, "Grandchild plan was not retrieved" + assert gc.identity.parent_plan_id == expected, ( + f"Expected parent_plan_id '{expected}', got '{gc.identity.parent_plan_id}'" + ) + + +@then('the child plan parent_plan_id should be "{expected}"') +def step_check_child_parent(context: Context, expected: str) -> None: + child = context._sa_retrieved_child + assert child is not None, "Child plan was not retrieved" + assert child.identity.parent_plan_id == expected, ( + f"Expected parent_plan_id '{expected}', got '{child.identity.parent_plan_id}'" + ) + + +@then('the persisted automation_profile column should be "{expected}"') +def step_check_automation_profile_col(context: Context, expected: str) -> None: + plan_id = context._sa_plan.identity.plan_id + row = context._sa_session.execute( + text("SELECT automation_profile FROM v3_plans WHERE plan_id = :pid"), + {"pid": plan_id}, + ).fetchone() + assert row is not None, "Row not found in v3_plans" + assert row[0] == expected, ( + f"Expected automation_profile='{expected}', got '{row[0]}'" + ) + + +# --- M3: Bare-string / invalid-provenance automation_profile handling --- + + +@given('the plan automation_profile column is set to bare string "{value}"') +def step_set_bare_string_profile(context: Context, value: str) -> None: + """Persist the plan, then overwrite the column with a bare string.""" + context._sa_plan_repo.create(context._sa_plan) + context._sa_session.commit() + plan_id = context._sa_plan.identity.plan_id + context._sa_session.execute( + text("UPDATE v3_plans SET automation_profile = :val WHERE plan_id = :pid"), + {"val": value, "pid": plan_id}, + ) + context._sa_session.commit() + context._sa_target_plan_id = plan_id + + +@given("the plan automation_profile column is set to '{value}'") +def step_set_invalid_json_profile(context: Context, value: str) -> None: + """Persist the plan, then overwrite the column with arbitrary JSON.""" + context._sa_plan_repo.create(context._sa_plan) + context._sa_session.commit() + plan_id = context._sa_plan.identity.plan_id + context._sa_session.execute( + text("UPDATE v3_plans SET automation_profile = :val WHERE plan_id = :pid"), + {"val": value, "pid": plan_id}, + ) + context._sa_session.commit() + context._sa_target_plan_id = plan_id + + +@when("I load the plan from the database") +def step_load_plan(context: Context) -> None: + plan_id = context._sa_target_plan_id + context._sa_retrieved = context._sa_plan_repo.get(plan_id) + + +@then("the plan automation_profile ref should be None") +def step_check_profile_is_none(context: Context) -> None: + retrieved = context._sa_retrieved + assert retrieved is not None, "Plan was not retrieved" + assert retrieved.automation_profile is None, ( + f"Expected automation_profile=None, got {retrieved.automation_profile!r}" + ) + + +# --- L3: NULL root_plan_id constraint enforcement --- + + +@given('a schema-aligned plan with ID "{plan_id}" forced NULL root_plan_id') +def step_create_plan_null_root(context: Context, plan_id: str) -> None: + context._sa_null_root_plan_id = plan_id + + +@when("I attempt to persist the plan with NULL root_plan_id") +def step_persist_null_root(context: Context) -> None: + plan_id = context._sa_null_root_plan_id + now_iso = _NOW.isoformat() + try: + context._sa_session.execute( + text( + "INSERT INTO v3_plans " + "(plan_id, root_plan_id, action_name, namespaced_name, " + "namespace, phase, processing_state, attempt, description, " + "tags_json, effective_profile_snapshot, automation_profile, " + "reusable, read_only, created_at, updated_at) " + "VALUES (:pid, NULL, :act, :ns, 'local', 'action', 'queued', " + "1, 'test', '[]', '{}', 'balanced', 1, 0, :now, :now)" + ), + { + "pid": plan_id, + "act": "local/schema-test", + "ns": "local/schema-test", + "now": now_iso, + }, + ) + context._sa_session.commit() + context._sa_integrity_error = None + except IntegrityError as exc: + context._sa_session.rollback() + context._sa_integrity_error = exc + + +@then("the database should reject the insert with an integrity error") +def step_check_integrity_error(context: Context) -> None: + assert context._sa_integrity_error is not None, ( + "Expected IntegrityError for NULL root_plan_id, but insert succeeded" + ) + + +# --- T2: AutomationProfileRef round-trip --- + + +@given( + 'a schema-aligned plan "{plan_id}" with profile "{profile}" and provenance "{provenance}"' +) +def step_create_plan_with_profile( + context: Context, plan_id: str, profile: str, provenance: str +) -> None: + ref = AutomationProfileRef( + profile_name=profile, + provenance=AutomationProfileProvenance(provenance), + ) + context._sa_plan = _make_plan(plan_id, automation_profile=ref) + + +@then('the plan automation_profile name should be "{expected}"') +def step_check_profile_name(context: Context, expected: str) -> None: + retrieved = context._sa_retrieved + assert retrieved is not None, "Plan was not retrieved" + assert retrieved.automation_profile is not None, ( + "Expected automation_profile to be set, got None" + ) + assert retrieved.automation_profile.profile_name == expected, ( + f"Expected profile_name='{expected}', " + f"got '{retrieved.automation_profile.profile_name}'" + ) + + +@then('the plan automation_profile provenance should be "{expected}"') +def step_check_profile_provenance(context: Context, expected: str) -> None: + retrieved = context._sa_retrieved + assert retrieved is not None, "Plan was not retrieved" + assert retrieved.automation_profile is not None, ( + "Expected automation_profile to be set, got None" + ) + assert retrieved.automation_profile.provenance.value == expected, ( + f"Expected provenance='{expected}', " + f"got '{retrieved.automation_profile.provenance.value}'" + ) + + +# --- TC5: NULL effective_profile_snapshot constraint enforcement --- + + +@given( + 'a schema-aligned plan with ID "{plan_id}" forced NULL effective_profile_snapshot' +) +def step_create_plan_null_snapshot(context: Context, plan_id: str) -> None: + context._sa_null_snapshot_plan_id = plan_id + + +@when("I attempt to persist the plan with NULL effective_profile_snapshot") +def step_persist_null_snapshot(context: Context) -> None: + plan_id = context._sa_null_snapshot_plan_id + now_iso = _NOW.isoformat() + try: + context._sa_session.execute( + text( + "INSERT INTO v3_plans " + "(plan_id, root_plan_id, action_name, namespaced_name, " + "namespace, phase, processing_state, attempt, description, " + "tags_json, effective_profile_snapshot, automation_profile, " + "reusable, read_only, created_at, updated_at) " + "VALUES (:pid, :pid, :act, :ns, 'local', 'action', 'queued', " + "1, 'test', '[]', NULL, 'balanced', 1, 0, :now, :now)" + ), + { + "pid": plan_id, + "act": "local/schema-test", + "ns": "local/schema-test", + "now": now_iso, + }, + ) + context._sa_session.commit() + context._sa_snapshot_integrity_error = None + except IntegrityError as exc: + context._sa_session.rollback() + context._sa_snapshot_integrity_error = exc + + +@then("the database should reject the snapshot insert with an integrity error") +def step_check_snapshot_integrity_error(context: Context) -> None: + assert context._sa_snapshot_integrity_error is not None, ( + "Expected IntegrityError for NULL effective_profile_snapshot, " + "but insert succeeded" + ) + + +# --- TC4: PlanIdentity model_validator self-reference resolution --- + + +@given('a schema-aligned plan with ID "{plan_id}" and None root_plan_id') +def step_create_plan_none_root(context: Context, plan_id: str) -> None: + context._sa_plan = _make_plan(plan_id, root_plan_id=None) + + +@then('the domain plan root_plan_id should already equal "{expected}"') +def step_check_domain_root_resolved(context: Context, expected: str) -> None: + plan = context._sa_plan + assert plan is not None, "Plan was not created" + assert plan.identity.root_plan_id == expected, ( + f"Expected PlanIdentity.root_plan_id='{expected}' after model_validator, " + f"got '{plan.identity.root_plan_id}'" + ) + + +@when("I convert the plan via from_domain") +def step_convert_from_domain(context: Context) -> None: + context._sa_orm_model = LifecyclePlanModel.from_domain(context._sa_plan) + + +@then('the ORM model root_plan_id should equal "{expected}"') +def step_check_orm_root(context: Context, expected: str) -> None: + model = context._sa_orm_model + assert model is not None, "ORM model was not created" + assert model.root_plan_id == expected, ( + f"Expected ORM root_plan_id='{expected}', got '{model.root_plan_id}'" + ) + + +# --- TC6: Invalid JSON in effective_profile_snapshot --- + + +@when('the effective_profile_snapshot column is corrupted to "{bad_json}"') +def step_corrupt_snapshot_column(context: Context, bad_json: str) -> None: + """Overwrite the effective_profile_snapshot column with invalid JSON.""" + plan_id = context._sa_plan.identity.plan_id + context._sa_session.execute( + text( + "UPDATE v3_plans SET effective_profile_snapshot = :val WHERE plan_id = :pid" + ), + {"val": bad_json, "pid": plan_id}, + ) + context._sa_session.commit() + + +@when("I reload the plan from the database") +def step_reload_plan(context: Context) -> None: + """Expire cached ORM state and reload from DB to pick up raw SQL changes.""" + context._sa_session.expire_all() + plan_id = context._sa_plan.identity.plan_id + context._sa_retrieved = context._sa_plan_repo.get(plan_id) + + +@when('I attempt to construct a Plan with effective_profile_snapshot "{bad_json}"') +def step_construct_plan_bad_snapshot(context: Context, bad_json: str) -> None: + from pydantic import ValidationError as PydanticValidationError + + try: + _make_plan( + "01HV000000000000000000SA21", + effective_profile_snapshot=bad_json, + ) + context._sa_validation_error = None + except PydanticValidationError as exc: + context._sa_validation_error = exc + + +@when("I attempt to construct a Plan with an empty effective_profile_snapshot") +def step_construct_plan_empty_snapshot(context: Context) -> None: + from pydantic import ValidationError as PydanticValidationError + + try: + _make_plan( + "01HV000000000000000000SA22", + effective_profile_snapshot="", + ) + context._sa_validation_error = None + except PydanticValidationError as exc: + context._sa_validation_error = exc + + +@then( + "the Plan construction should raise a validation error " + "for effective_profile_snapshot" +) +def step_check_validation_error(context: Context) -> None: + assert context._sa_validation_error is not None, ( + "Expected a ValidationError for invalid JSON in " + "effective_profile_snapshot, but Plan was created successfully" + ) diff --git a/features/steps/repositories_coverage_r2_steps.py b/features/steps/repositories_coverage_r2_steps.py index 296bd933f..3016ae332 100644 --- a/features/steps/repositories_coverage_r2_steps.py +++ b/features/steps/repositories_coverage_r2_steps.py @@ -184,6 +184,7 @@ def _insert_v3_plan(session: Session, plan_id: str) -> None: now_iso = datetime.now(UTC).isoformat() plan_row = LifecyclePlanModel( plan_id=plan_id, + root_plan_id=plan_id, namespaced_name=f"local/test-plan-{plan_id[:8]}", namespace="local", action_name="local/test-action", @@ -196,6 +197,7 @@ def _insert_v3_plan(session: Session, plan_id: str) -> None: read_only=False, created_by="test", tags_json="[]", + effective_profile_snapshot="{}", created_at=now_iso, updated_at=now_iso, ) diff --git a/features/steps/repositories_coverage_steps.py b/features/steps/repositories_coverage_steps.py index 379d2f227..b21ace53c 100644 --- a/features/steps/repositories_coverage_steps.py +++ b/features/steps/repositories_coverage_steps.py @@ -407,8 +407,10 @@ def step_repos_cov_create_referencing_plan(context: Context, action_name: str) - """Insert a LifecyclePlanModel row referencing the specified action.""" session: Session = context.rc_session now_iso = datetime.now().isoformat() + pid = _next_ulid() plan_model = LifecyclePlanModel( - plan_id=_next_ulid(), + plan_id=pid, + root_plan_id=pid, action_name=action_name, phase="strategize", processing_state="queued", @@ -416,6 +418,7 @@ def step_repos_cov_create_referencing_plan(context: Context, action_name: str) - namespaced_name="local/ref-plan", namespace="local", description="Plan referencing action under test", + effective_profile_snapshot="{}", created_at=now_iso, updated_at=now_iso, tags_json="[]", diff --git a/features/steps/repositories_error_handling_coverage_steps.py b/features/steps/repositories_error_handling_coverage_steps.py index e92eb2a17..b32f05f1c 100644 --- a/features/steps/repositories_error_handling_coverage_steps.py +++ b/features/steps/repositories_error_handling_coverage_steps.py @@ -288,6 +288,7 @@ def _make_fake_plan() -> SimpleNamespace: arguments={}, arguments_order=[], invariants=[], + effective_profile_snapshot="{}", timestamps=SimpleNamespace( created_at=now, updated_at=now, diff --git a/robot/helper_plans_table_schema_alignment.py b/robot/helper_plans_table_schema_alignment.py new file mode 100644 index 000000000..609b6c6d2 --- /dev/null +++ b/robot/helper_plans_table_schema_alignment.py @@ -0,0 +1,394 @@ +"""Robot Framework helper for v3_plans schema alignment tests (issue #921). + +Verifies effective_profile_snapshot, root_plan_id self-reference, and +automation_profile NOT NULL default. + +Usage: + python robot/helper_plans_table_schema_alignment.py snapshot-roundtrip + python robot/helper_plans_table_schema_alignment.py root-self-ref + python robot/helper_plans_table_schema_alignment.py child-root-id +""" + +from __future__ import annotations + +import sys +from datetime import UTC, datetime +from pathlib import Path +from typing import Any + +_SRC = str(Path(__file__).resolve().parents[1] / "src") +if _SRC not in sys.path: + sys.path.insert(0, _SRC) + +from sqlalchemy import create_engine # noqa: E402 +from sqlalchemy.orm import sessionmaker # noqa: E402 + +from cleveragents.domain.models.core.action import ( # noqa: E402 + Action, + ActionState, +) +from cleveragents.domain.models.core.plan import ( # noqa: E402 + AutomationProfileProvenance, + AutomationProfileRef, + NamespacedName, + Plan, + PlanIdentity, + PlanPhase, + PlanTimestamps, + ProcessingState, +) +from cleveragents.infrastructure.database.models import Base # noqa: E402 +from cleveragents.infrastructure.database.repositories import ( # noqa: E402 + ActionRepository, + LifecyclePlanRepository, +) + +_NOW = datetime(2026, 3, 19, tzinfo=UTC) +_ACTION_NAME = "local/schema-test" + + +def _setup() -> tuple[Any, Any, Any]: + """Create in-memory DB and return (session, plan_repo, action_repo).""" + engine = create_engine("sqlite:///:memory:", echo=False) + Base.metadata.create_all(engine) + sm = sessionmaker(bind=engine) + session = sm() + factory = lambda: session # noqa: E731 + plan_repo = LifecyclePlanRepository(session_factory=factory) + action_repo = ActionRepository(session_factory=factory) + + # Create prerequisite action for FK constraint + ns = NamespacedName.parse(_ACTION_NAME) + action = Action( + namespaced_name=ns, + description="Schema alignment test action", + definition_of_done="Done", + strategy_actor="local/s", + execution_actor="local/e", + state=ActionState.AVAILABLE, + created_at=_NOW, + updated_at=_NOW, + ) + action_repo.create(action) + session.commit() + + return session, plan_repo, action_repo + + +def _make_plan( + plan_id: str, + *, + parent_plan_id: str | None = None, + root_plan_id: str | None = None, + effective_profile_snapshot: str = "{}", + automation_profile: AutomationProfileRef | None = None, +) -> Plan: + return Plan( + identity=PlanIdentity( + plan_id=plan_id, + parent_plan_id=parent_plan_id, + root_plan_id=root_plan_id, + attempt=1, + ), + namespaced_name=NamespacedName(namespace="local", name="schema-test"), + action_name=_ACTION_NAME, + description="Schema alignment test plan", + definition_of_done="All assertions pass", + phase=PlanPhase.ACTION, + processing_state=ProcessingState.QUEUED, + automation_profile=automation_profile, + effective_profile_snapshot=effective_profile_snapshot, + strategy_actor="local/s", + execution_actor="local/e", + timestamps=PlanTimestamps(created_at=_NOW, updated_at=_NOW), + created_by="test-schema", + tags=[], + reusable=True, + read_only=False, + ) + + +def _cmd_snapshot_roundtrip() -> int: + """Verify effective_profile_snapshot roundtrips through persistence.""" + session, plan_repo, _ = _setup() + snapshot = '{"profile":"balanced","thresholds":{}}' + plan = _make_plan( + "01HV000000000000000000SR01", + effective_profile_snapshot=snapshot, + ) + plan_repo.create(plan) + session.commit() + + retrieved = plan_repo.get("01HV000000000000000000SR01") + if retrieved is None: + print("FAIL: plan not retrieved") + return 1 + if retrieved.effective_profile_snapshot != snapshot: + print( + f"FAIL: snapshot mismatch: {retrieved.effective_profile_snapshot!r} " + f"!= {snapshot!r}" + ) + return 1 + + print("schema-snapshot-ok") + return 0 + + +def _cmd_root_self_ref() -> int: + """Verify root plan self-references its own plan_id.""" + session, plan_repo, _ = _setup() + plan_id = "01HV000000000000000000SR02" + plan = _make_plan(plan_id, root_plan_id=None) + plan_repo.create(plan) + session.commit() + + retrieved = plan_repo.get(plan_id) + if retrieved is None: + print("FAIL: plan not retrieved") + return 1 + if retrieved.identity.root_plan_id != plan_id: + print( + f"FAIL: root_plan_id={retrieved.identity.root_plan_id!r}, " + f"expected={plan_id!r}" + ) + return 1 + + print("schema-root-self-ref-ok") + return 0 + + +def _cmd_child_root_id() -> int: + """Verify child plan carries parent's root_plan_id.""" + session, plan_repo, _ = _setup() + root_id = "01HV000000000000000000SR03" + child_id = "01HV000000000000000000SR04" + + root_plan = _make_plan(root_id, root_plan_id=None) + plan_repo.create(root_plan) + session.commit() + + child_plan = _make_plan(child_id, parent_plan_id=root_id, root_plan_id=root_id) + plan_repo.create(child_plan) + session.commit() + + retrieved = plan_repo.get(child_id) + if retrieved is None: + print("FAIL: child plan not retrieved") + return 1 + if retrieved.identity.root_plan_id != root_id: + print( + f"FAIL: root_plan_id={retrieved.identity.root_plan_id!r}, " + f"expected={root_id!r}" + ) + return 1 + if retrieved.identity.parent_plan_id != root_id: + print( + f"FAIL: parent_plan_id={retrieved.identity.parent_plan_id!r}, " + f"expected={root_id!r}" + ) + return 1 + + print("schema-child-root-ok") + return 0 + + +def _cmd_grandchild_root_id() -> int: + """Verify grandchild plan carries root ancestor's plan_id (3-level).""" + session, plan_repo, _ = _setup() + root_id = "01HV000000000000000000SR05" + child_id = "01HV000000000000000000SR06" + gc_id = "01HV000000000000000000SR07" + + root_plan = _make_plan(root_id, root_plan_id=None) + plan_repo.create(root_plan) + session.commit() + + child_plan = _make_plan(child_id, parent_plan_id=root_id, root_plan_id=root_id) + plan_repo.create(child_plan) + session.commit() + + gc_plan = _make_plan(gc_id, parent_plan_id=child_id, root_plan_id=root_id) + plan_repo.create(gc_plan) + session.commit() + + retrieved = plan_repo.get(gc_id) + if retrieved is None: + print("FAIL: grandchild plan not retrieved") + return 1 + if retrieved.identity.root_plan_id != root_id: + print( + f"FAIL: root_plan_id={retrieved.identity.root_plan_id!r}, " + f"expected={root_id!r}" + ) + return 1 + if retrieved.identity.parent_plan_id != child_id: + print( + f"FAIL: parent_plan_id={retrieved.identity.parent_plan_id!r}, " + f"expected={child_id!r}" + ) + return 1 + + print("schema-grandchild-root-ok") + return 0 + + +def _cmd_automation_profile_default() -> int: + """Verify automation_profile=None roundtrips as None via 'balanced' default.""" + session, plan_repo, _ = _setup() + plan_id = "01HV000000000000000000SR08" + plan = _make_plan(plan_id) # automation_profile defaults to None + plan_repo.create(plan) + session.commit() + + retrieved = plan_repo.get(plan_id) + if retrieved is None: + print("FAIL: plan not retrieved") + return 1 + if retrieved.automation_profile is not None: + print( + f"FAIL: expected automation_profile=None, " + f"got {retrieved.automation_profile!r}" + ) + return 1 + + # Verify raw column value is 'balanced' + from sqlalchemy import text + + row = session.execute( + text("SELECT automation_profile FROM v3_plans WHERE plan_id = :pid"), + {"pid": plan_id}, + ).fetchone() + if row is None or row[0] != "balanced": + print(f"FAIL: raw column value={row[0] if row else 'N/A'}, expected='balanced'") + return 1 + + print("schema-automation-profile-default-ok") + return 0 + + +def _cmd_null_root_rejected() -> int: + """Verify NULL root_plan_id is rejected by the NOT NULL constraint.""" + from sqlalchemy import text + from sqlalchemy.exc import IntegrityError as SAIntegrityError + + session, _, _ = _setup() + now_iso = _NOW.isoformat() + try: + session.execute( + text( + "INSERT INTO v3_plans " + "(plan_id, root_plan_id, action_name, namespaced_name, " + "namespace, phase, processing_state, attempt, description, " + "tags_json, effective_profile_snapshot, automation_profile, " + "reusable, read_only, created_at, updated_at) " + "VALUES (:pid, NULL, :act, :ns, 'local', 'action', 'queued', " + "1, 'test', '[]', '{}', 'balanced', 1, 0, :now, :now)" + ), + { + "pid": "01HV000000000000000000SR09", + "act": "local/schema-test", + "ns": "local/schema-test", + "now": now_iso, + }, + ) + session.commit() + print( + "FAIL: expected IntegrityError for NULL root_plan_id, but insert succeeded" + ) + return 1 + except SAIntegrityError: + session.rollback() + print("schema-null-root-rejected-ok") + return 0 + + +def _cmd_automation_profile_ref_roundtrip() -> int: + """Verify AutomationProfileRef roundtrips through persistence.""" + session, plan_repo, _ = _setup() + plan_id = "01HV000000000000000000SR10" + ref = AutomationProfileRef( + profile_name="supervised", + provenance=AutomationProfileProvenance.PLAN, + ) + plan = _make_plan(plan_id, automation_profile=ref) + plan_repo.create(plan) + session.commit() + + retrieved = plan_repo.get(plan_id) + if retrieved is None: + print("FAIL: plan not retrieved") + return 1 + if retrieved.automation_profile is None: + print("FAIL: automation_profile is None after round-trip") + return 1 + if retrieved.automation_profile.profile_name != "supervised": + print( + f"FAIL: profile_name={retrieved.automation_profile.profile_name!r}, " + f"expected='supervised'" + ) + return 1 + if retrieved.automation_profile.provenance != AutomationProfileProvenance.PLAN: + print( + f"FAIL: provenance={retrieved.automation_profile.provenance!r}, " + f"expected='plan'" + ) + return 1 + + print("schema-automation-profile-ref-roundtrip-ok") + return 0 + + +def _cmd_corrupted_snapshot_fallback() -> int: + """Verify corrupted effective_profile_snapshot falls back to '{}'.""" + from sqlalchemy import text as sa_text + + session, plan_repo, _ = _setup() + plan_id = "01HV000000000000000000SR11" + plan = _make_plan( + plan_id, + effective_profile_snapshot='{"profile":"balanced"}', + ) + plan_repo.create(plan) + session.commit() + + # Corrupt the column directly + session.execute( + sa_text( + "UPDATE v3_plans SET effective_profile_snapshot = :val WHERE plan_id = :pid" + ), + {"val": "not-valid-json{", "pid": plan_id}, + ) + session.commit() + session.expire_all() + + retrieved = plan_repo.get(plan_id) + if retrieved is None: + print("FAIL: plan not retrieved after corruption") + return 1 + if retrieved.effective_profile_snapshot != "{}": + print( + f"FAIL: expected fallback '{{}}', " + f"got {retrieved.effective_profile_snapshot!r}" + ) + return 1 + + print("schema-corrupted-snapshot-fallback-ok") + return 0 + + +_COMMANDS: dict[str, Any] = { + "snapshot-roundtrip": _cmd_snapshot_roundtrip, + "root-self-ref": _cmd_root_self_ref, + "child-root-id": _cmd_child_root_id, + "grandchild-root-id": _cmd_grandchild_root_id, + "automation-profile-default": _cmd_automation_profile_default, + "automation-profile-ref-roundtrip": _cmd_automation_profile_ref_roundtrip, + "null-root-rejected": _cmd_null_root_rejected, + "corrupted-snapshot-fallback": _cmd_corrupted_snapshot_fallback, +} + +if __name__ == "__main__": + if len(sys.argv) < 2 or sys.argv[1] not in _COMMANDS: + print(f"Usage: {sys.argv[0]} {{{','.join(_COMMANDS)}}}", file=sys.stderr) + sys.exit(2) + sys.exit(_COMMANDS[sys.argv[1]]()) diff --git a/robot/plans_table_schema_alignment.robot b/robot/plans_table_schema_alignment.robot new file mode 100644 index 000000000..28a160b1d --- /dev/null +++ b/robot/plans_table_schema_alignment.robot @@ -0,0 +1,83 @@ +*** Settings *** +Documentation Integration tests for v3_plans schema alignment (issue #921). +... Verifies effective_profile_snapshot, root_plan_id self-reference, +... and automation_profile NOT NULL default. +Resource ${CURDIR}/common.resource +Suite Setup Setup Test Environment +Suite Teardown Cleanup Test Environment + +*** Variables *** +${HELPER} ${CURDIR}/helper_plans_table_schema_alignment.py + +*** Test Cases *** +Plan Includes Effective Profile Snapshot + [Documentation] Create a plan with an explicit profile snapshot and verify persistence. + [Tags] feature921 + ${result}= Run Process ${PYTHON} ${HELPER} snapshot-roundtrip cwd=${WORKSPACE} timeout=120s on_timeout=kill + Log ${result.stdout} + Log ${result.stderr} + Should Be Equal As Integers ${result.rc} 0 + Should Contain ${result.stdout} schema-snapshot-ok + +Root Plan Self References Its Own Plan ID + [Documentation] Root plan must have root_plan_id == plan_id. + [Tags] feature921 + ${result}= Run Process ${PYTHON} ${HELPER} root-self-ref cwd=${WORKSPACE} timeout=120s on_timeout=kill + Log ${result.stdout} + Log ${result.stderr} + Should Be Equal As Integers ${result.rc} 0 + Should Contain ${result.stdout} schema-root-self-ref-ok + +Child Plan Has Parent Root Plan ID + [Documentation] Child plan must carry the root ancestor's plan_id. + [Tags] feature921 + ${result}= Run Process ${PYTHON} ${HELPER} child-root-id cwd=${WORKSPACE} timeout=120s on_timeout=kill + Log ${result.stdout} + Log ${result.stderr} + Should Be Equal As Integers ${result.rc} 0 + Should Contain ${result.stdout} schema-child-root-ok + +Grandchild Plan Carries Root Ancestor Plan ID + [Documentation] Grandchild plan (3-level hierarchy) must carry root ancestor's plan_id. + [Tags] feature921 + ${result}= Run Process ${PYTHON} ${HELPER} grandchild-root-id cwd=${WORKSPACE} timeout=120s on_timeout=kill + Log ${result.stdout} + Log ${result.stderr} + Should Be Equal As Integers ${result.rc} 0 + Should Contain ${result.stdout} schema-grandchild-root-ok + +Automation Profile Defaults To Balanced And Roundtrips As None + [Documentation] Plan with no automation profile stores 'balanced' in DB and loads as None. + [Tags] feature921 + ${result}= Run Process ${PYTHON} ${HELPER} automation-profile-default cwd=${WORKSPACE} timeout=120s on_timeout=kill + Log ${result.stdout} + Log ${result.stderr} + Should Be Equal As Integers ${result.rc} 0 + Should Contain ${result.stdout} schema-automation-profile-default-ok + +Automation Profile Ref Roundtrips Through Persistence + [Documentation] Plan with an explicit AutomationProfileRef persists and loads correctly. + [Tags] feature921 + ${result}= Run Process ${PYTHON} ${HELPER} automation-profile-ref-roundtrip cwd=${WORKSPACE} timeout=120s on_timeout=kill + Log ${result.stdout} + Log ${result.stderr} + Should Be Equal As Integers ${result.rc} 0 + Should Contain ${result.stdout} schema-automation-profile-ref-roundtrip-ok + +NULL Root Plan ID Is Rejected By Database + [Documentation] Inserting a plan with NULL root_plan_id must raise IntegrityError. + [Tags] feature921 + ${result}= Run Process ${PYTHON} ${HELPER} null-root-rejected cwd=${WORKSPACE} timeout=120s on_timeout=kill + Log ${result.stdout} + Log ${result.stderr} + Should Be Equal As Integers ${result.rc} 0 + Should Contain ${result.stdout} schema-null-root-rejected-ok + +Corrupted Effective Profile Snapshot Falls Back To Empty JSON + [Documentation] Corrupted effective_profile_snapshot in DB must fall back to '{}' on read. + [Tags] feature921 + ${result}= Run Process ${PYTHON} ${HELPER} corrupted-snapshot-fallback cwd=${WORKSPACE} timeout=120s on_timeout=kill + Log ${result.stdout} + Log ${result.stderr} + Should Be Equal As Integers ${result.rc} 0 + Should Contain ${result.stdout} schema-corrupted-snapshot-fallback-ok diff --git a/src/cleveragents/domain/models/core/plan.py b/src/cleveragents/domain/models/core/plan.py index b4f85d864..271fcfdec 100644 --- a/src/cleveragents/domain/models/core/plan.py +++ b/src/cleveragents/domain/models/core/plan.py @@ -56,6 +56,7 @@ Based on ``docs/specification.md`` and ADR-004 (Pydantic Validation). from __future__ import annotations +import json import re from datetime import datetime from enum import StrEnum @@ -295,6 +296,21 @@ class PlanIdentity(BaseModel): description="Attempt counter (increments on re-run)", ) + @model_validator(mode="after") + def resolve_root_plan_id(self) -> PlanIdentity: + """Resolve ``root_plan_id`` to ``plan_id`` when ``None``. + + Per the specification DDL, ``root_plan_id`` is ``NOT NULL``: + root plans self-reference their own ``plan_id``; child plans + carry the root ancestor's ``plan_id``. Resolving at domain + construction time keeps the domain model consistent with the + database constraint regardless of whether the object has been + persisted yet. + """ + if self.root_plan_id is None: + self.root_plan_id = self.plan_id + return self + model_config = ConfigDict( str_strip_whitespace=True, validate_assignment=True, @@ -707,6 +723,20 @@ class Plan(BaseModel): description="Multi-project scope tracking and cross-project metadata", ) + # Frozen profile snapshot — JSON snapshot of the automation profile + # resolved at plan-creation time. Used for audit / reproducibility. + # NOTE: The default '{}' exists for backward compatibility with code + # paths that create Plan objects before the snapshot is populated. + # New plans SHOULD explicitly set this field to the resolved profile + # JSON at creation time; the empty default does not satisfy the spec + # intent of capturing a frozen profile for audit purposes. + effective_profile_snapshot: str = Field( + default="{}", + description=( + "Frozen JSON snapshot of the automation profile at plan creation time" + ), + ) + # Metadata created_by: str | None = Field(None, description="User/session that created plan") tags: list[str] = Field(default_factory=list, description="Tags for organization") @@ -756,6 +786,18 @@ class Plan(BaseModel): MAX_REVERSIONS: ClassVar[int] = 3 + @field_validator("effective_profile_snapshot") + @classmethod + def validate_effective_profile_snapshot_json(cls, v: str) -> str: + """Ensure the snapshot is well-formed JSON.""" + try: + json.loads(v) + except (json.JSONDecodeError, TypeError, RecursionError) as exc: + raise ValueError( + f"effective_profile_snapshot must be valid JSON (length={len(v)})" + ) from exc + return v + @model_validator(mode="after") def validate_env_priority_requires_environment(self) -> Plan: """Enforce that execution_env_priority requires execution_environment. diff --git a/src/cleveragents/infrastructure/database/models.py b/src/cleveragents/infrastructure/database/models.py index 5034e910b..d9b4e8ca6 100644 --- a/src/cleveragents/infrastructure/database/models.py +++ b/src/cleveragents/infrastructure/database/models.py @@ -43,6 +43,7 @@ if TYPE_CHECKING: from cleveragents.domain.models.core.checkpoint import Checkpoint from cleveragents.domain.models.core.correction import CorrectionAttemptRecord +from pydantic import ValidationError from sqlalchemy import ( JSON, Boolean, @@ -77,6 +78,11 @@ from cleveragents.domain.models.core import ( _logger = logging.getLogger(__name__) +# Default automation profile name. Used as the NOT NULL server_default +# for the ``automation_profile`` column and as the sentinel value that +# ``to_domain()`` treats as "no structured profile set" (returns None). +DEFAULT_AUTOMATION_PROFILE = "balanced" + # Create base class for all models Base = declarative_base() @@ -571,6 +577,27 @@ class LifecyclePlanModel(Base): # type: ignore[misc] phase/state, execution context, and sandbox references. See: ``src/cleveragents/domain/models/core/plan.py`` + + .. note:: **Column naming conventions (intentional deviations from spec DDL)** + + The spec DDL uses ``automation_profile_name``, ``strategy_actor_name``, + ``execution_actor_name``, ``estimation_actor_name``, ``invariant_actor_name``, + ``state``, and ``error_traceback``. The code uses shorter forms without the + ``_name`` suffix (``automation_profile``, ``strategy_actor``, etc.), + ``processing_state`` (to avoid collision with SQLAlchemy internals), and + ``error_details_json`` (richer structured data than a plain traceback). + The table is named ``v3_plans`` (evolution artifact) vs spec's ``plans``. + These are intentional conventions established before the spec DDL was + formalised and are kept for backward compatibility. + + **Semantic note on ``automation_profile``**: The spec's + ``automation_profile_name`` stores a plain profile name string. + The code's ``automation_profile`` stores either a bare name + (``"balanced"``, the NOT NULL default) or a structured JSON object + (``{"profile_name": "...", "provenance": "..."}``). The dual + format allows tracking provenance (how the profile was resolved) + alongside the name, at the cost of requiring format-aware + deserialization in ``to_domain()``. """ __allow_unmapped__ = True @@ -585,8 +612,8 @@ class LifecyclePlanModel(Base): # type: ignore[misc] ) root_plan_id = Column( String(26), - ForeignKey("v3_plans.plan_id", ondelete="SET NULL"), - nullable=True, + ForeignKey("v3_plans.plan_id", ondelete="RESTRICT"), + nullable=False, ) action_name = Column( String(255), @@ -599,6 +626,11 @@ class LifecyclePlanModel(Base): # type: ignore[misc] namespace = Column(String(100), nullable=False) # Lifecycle phase and processing state (spec-aligned, rebaselined in a5_005) + # NOTE: Spec DDL defaults phase to 'strategize', but the code default is + # 'action'. This is intentional — the Action phase was added as a + # pre-Strategize setup step (template instantiation) that did not exist + # in the original specification. Plans begin in Action and transition to + # Strategize via ``agents plan use``. phase = Column( String(20), nullable=False, default="action", server_default="action" ) @@ -620,7 +652,15 @@ class LifecyclePlanModel(Base): # type: ignore[misc] invariant_actor = Column(String(255), nullable=True) # Policy / profile - automation_profile = Column(Text, nullable=True) + automation_profile = Column( + Text, + nullable=False, + default=DEFAULT_AUTOMATION_PROFILE, + server_default=DEFAULT_AUTOMATION_PROFILE, + ) + effective_profile_snapshot = Column( + Text, nullable=False, default="{}", server_default="{}" + ) # Behavior reusable = Column(Boolean, nullable=False, default=True) @@ -723,6 +763,19 @@ class LifecyclePlanModel(Base): # type: ignore[misc] # -- Domain conversion helpers ------------------------------------------ + @staticmethod + def _serialize_automation_profile(profile_ref: Any) -> str: + """Serialize an ``AutomationProfileRef`` to a JSON column value. + + Returns the JSON representation when *profile_ref* is truthy, + otherwise the module-level default ``DEFAULT_AUTOMATION_PROFILE``. + Centralises the serialisation logic used by both ``from_domain()`` + and ``LifecyclePlanRepository.update()``. + """ + if profile_ref is not None: + return json.dumps(profile_ref.model_dump(mode="json")) + return DEFAULT_AUTOMATION_PROFILE + @staticmethod def _parse_iso(value: str | None) -> datetime | None: """Parse an ISO-8601 string to ``datetime``, returning ``None`` for empty.""" @@ -830,14 +883,57 @@ class LifecyclePlanModel(Base): # type: ignore[misc] else: arguments_dict[arg_name] = None - # Deserialize automation profile + # Deserialize automation profile. The column is NOT NULL with + # server_default 'balanced'. Values are either a JSON object + # (``{"profile_name": "...", "provenance": "..."}``), or a bare + # profile-name string (e.g. ``"balanced"``). Bare strings and + # the default ``"balanced"`` value are treated as unparseable + # and result in ``automation_profile_ref = None``. automation_profile_ref: AutomationProfileRef | None = None - if self.automation_profile is not None: - profile_raw: dict[str, str] = json.loads(cast(str, self.automation_profile)) - automation_profile_ref = AutomationProfileRef( - profile_name=profile_raw["profile_name"], - provenance=AutomationProfileProvenance(profile_raw["provenance"]), + raw_ap = cast(str, self.automation_profile) + if raw_ap and raw_ap != DEFAULT_AUTOMATION_PROFILE: + try: + profile_raw: dict[str, str] = json.loads(raw_ap) + automation_profile_ref = AutomationProfileRef( + profile_name=profile_raw["profile_name"], + provenance=AutomationProfileProvenance(profile_raw["provenance"]), + ) + except ( + json.JSONDecodeError, + KeyError, + ValueError, + ValidationError, + RecursionError, + ): + # Bare profile name string, invalid provenance enum, + # Pydantic validation failure, or deeply nested JSON + # — treat as unparseable. + _logger.warning( + "Unparseable automation_profile for plan %s (length=%d)", + self.plan_id, + len(raw_ap), + ) + automation_profile_ref = None + + # Validate effective_profile_snapshot. The column is NOT NULL + # with server_default '{}', but corrupted rows may contain invalid + # JSON. Mirror the defensive pattern used for automation_profile + # above to prevent a single corrupted row from crashing reads. + raw_snapshot = ( + cast(str, self.effective_profile_snapshot) + if self.effective_profile_snapshot is not None + else "{}" + ) + try: + json.loads(raw_snapshot) + except (json.JSONDecodeError, TypeError, RecursionError): + _logger.warning( + "Unparseable effective_profile_snapshot for plan %s (length=%d); " + "falling back to '{}'", + self.plan_id, + len(raw_snapshot), ) + raw_snapshot = "{}" # Deserialize sandbox refs sandbox_refs_list: list[str] = json.loads( @@ -867,7 +963,7 @@ class LifecyclePlanModel(Base): # type: ignore[misc] identity=PlanIdentity( plan_id=cast(str, self.plan_id), parent_plan_id=cast("str | None", self.parent_plan_id), - root_plan_id=cast("str | None", self.root_plan_id), + root_plan_id=cast(str, self.root_plan_id), attempt=cast(int, self.attempt), ), namespaced_name=NamespacedName.parse(cast(str, self.namespaced_name)), @@ -877,6 +973,7 @@ class LifecyclePlanModel(Base): # type: ignore[misc] phase=phase_enum, processing_state=state_enum, automation_profile=automation_profile_ref, + effective_profile_snapshot=raw_snapshot, strategy_actor=cast("str | None", self.strategy_actor), execution_actor=cast("str | None", self.execution_actor), review_actor=cast("str | None", self.review_actor), @@ -950,12 +1047,13 @@ class LifecyclePlanModel(Base): # type: ignore[misc] ) # Serialize automation profile - automation_profile_json = ( - json.dumps(plan.automation_profile.model_dump(mode="json")) - if plan.automation_profile - else None + automation_profile_json = cls._serialize_automation_profile( + plan.automation_profile, ) + # root_plan_id is resolved by PlanIdentity's model_validator + # (None → plan_id for root plans); always non-None here. + tags_json = json.dumps(plan.tags) resolved_action = action_name or plan.action_name or "" @@ -980,6 +1078,7 @@ class LifecyclePlanModel(Base): # type: ignore[misc] estimation_actor=getattr(plan, "estimation_actor", None), invariant_actor=getattr(plan, "invariant_actor", None), automation_profile=automation_profile_json, + effective_profile_snapshot=plan.effective_profile_snapshot, reusable=plan.reusable, read_only=plan.read_only, execution_environment=plan.execution_environment, diff --git a/src/cleveragents/infrastructure/database/repositories.py b/src/cleveragents/infrastructure/database/repositories.py index 53e075f64..b193719b4 100644 --- a/src/cleveragents/infrastructure/database/repositories.py +++ b/src/cleveragents/infrastructure/database/repositories.py @@ -1371,13 +1371,11 @@ class LifecyclePlanRepository: row.estimation_actor = getattr(plan, "estimation_actor", None) # type: ignore[assignment] row.invariant_actor = getattr(plan, "invariant_actor", None) # type: ignore[assignment] - # Serialize automation profile - automation_profile_json = ( - json.dumps(plan.automation_profile.model_dump(mode="json")) - if plan.automation_profile - else None + # Serialize automation profile — column is NOT NULL, default 'balanced' + row.automation_profile = LifecyclePlanModel._serialize_automation_profile( # type: ignore[assignment] + plan.automation_profile, ) - row.automation_profile = automation_profile_json # type: ignore[assignment] + row.effective_profile_snapshot = plan.effective_profile_snapshot # type: ignore[assignment] row.error_message = plan.error_message # type: ignore[assignment] row.error_details_json = ( # type: ignore[assignment] -- 2.52.0