fix(db): align v3_plans schema with specification DDL #1074

Merged

CoreRasurae merged 1 commit from fix/plans-table-schema-alignment into master 2026-03-30 23:01:20 +00:00

Conversation 16 Commits 1 Files changed 22 +1569 -39

CoreRasurae commented 2026-03-19 19:33:52 +00:00

Member

Copy link

Summary

Aligns the v3_plans table schema with the specification DDL, fixing nullability, defaults, and adding missing columns.

Changes

1. Added effective_profile_snapshot column (TEXT NOT NULL DEFAULT '{}') to LifecyclePlanModel for storing a frozen JSON snapshot of the automation profile at plan creation time. Added corresponding field to Plan domain model.

2. Made root_plan_id NOT NULL — root plans self-reference their own plan_id, child plans reference the root ancestor. Updated from_domain() to resolve None to plan_id.

3. Made automation_profile NOT NULL with default "balanced". Updated to_domain() to handle bare profile-name strings vs JSON objects, and from_domain()/update() to default to "balanced" when no profile is set.

4. Documented intentional deviation in phase default: code uses 'action' (Action phase was added as pre-Strategize setup step) vs spec DDL's 'strategize'.

5. Created Alembic migration m8_001_align_plans_schema with backfill logic for existing rows (root_plan_id ← plan_id, automation_profile ← 'balanced').

Files Modified/Created

New files:

• alembic/versions/m8_001_align_plans_schema.py — Migration
• features/plans_table_schema_alignment.feature — BDD scenarios
• features/steps/plans_table_schema_alignment_steps.py — Step definitions
• robot/plans_table_schema_alignment.robot — Robot integration tests
• robot/helper_plans_table_schema_alignment.py — Robot helper

Modified files:

• src/cleveragents/domain/models/core/plan.py — Added effective_profile_snapshot field
• src/cleveragents/infrastructure/database/models.py — Schema changes + conversion methods
• src/cleveragents/infrastructure/database/repositories.py — Update method fixes
• 8 test step files — Updated LifecyclePlanModel constructors for new NOT NULL constraints

Test Results

• Lint: All checks passed
• Typecheck: 0 errors, 1 warning (pre-existing)
• Unit tests: 400 features, 11,499 scenarios — all passed

ISSUES CLOSED: #921

## Summary Aligns the `v3_plans` table schema with the specification DDL, fixing nullability, defaults, and adding missing columns. ### Changes 1. **Added `effective_profile_snapshot` column** (TEXT NOT NULL DEFAULT `'{}'`) to `LifecyclePlanModel` for storing a frozen JSON snapshot of the automation profile at plan creation time. Added corresponding field to `Plan` domain model. 2. **Made `root_plan_id` NOT NULL** — root plans self-reference their own `plan_id`, child plans reference the root ancestor. Updated `from_domain()` to resolve `None` to `plan_id`. 3. **Made `automation_profile` NOT NULL** with default `"balanced"`. Updated `to_domain()` to handle bare profile-name strings vs JSON objects, and `from_domain()`/`update()` to default to `"balanced"` when no profile is set. 4. **Documented intentional deviation** in phase default: code uses `'action'` (Action phase was added as pre-Strategize setup step) vs spec DDL's `'strategize'`. 5. **Created Alembic migration** `m8_001_align_plans_schema` with backfill logic for existing rows (root_plan_id ← plan_id, automation_profile ← 'balanced'). ### Files Modified/Created **New files:** - `alembic/versions/m8_001_align_plans_schema.py` — Migration - `features/plans_table_schema_alignment.feature` — BDD scenarios - `features/steps/plans_table_schema_alignment_steps.py` — Step definitions - `robot/plans_table_schema_alignment.robot` — Robot integration tests - `robot/helper_plans_table_schema_alignment.py` — Robot helper **Modified files:** - `src/cleveragents/domain/models/core/plan.py` — Added `effective_profile_snapshot` field - `src/cleveragents/infrastructure/database/models.py` — Schema changes + conversion methods - `src/cleveragents/infrastructure/database/repositories.py` — Update method fixes - 8 test step files — Updated `LifecyclePlanModel` constructors for new NOT NULL constraints ### Test Results - **Lint**: All checks passed - **Typecheck**: 0 errors, 1 warning (pre-existing) - **Unit tests**: 400 features, 11,499 scenarios — all passed ISSUES CLOSED: #921

CoreRasurae added this to the v3.4.0 milestone 2026-03-19 20:47:41 +00:00

CoreRasurae added the

Type

Task

label 2026-03-19 20:47:43 +00:00

CoreRasurae referenced this pull request 2026-03-19 20:48:07 +00:00

fix(db): align v3_plans table schema with spec DDL (nullability, defaults, missing columns) #921

freemo requested reviews from freemo, brent.edwards 2026-03-22 16:35:32 +00:00

freemo referenced this pull request 2026-03-22 16:40:19 +00:00

fix(db): align v3_plans table schema with spec DDL (nullability, defaults, missing columns) #921

freemo added the

Priority

Medium

State

In Review

labels 2026-03-23 03:33:55 +00:00

freemo reviewed 2026-03-23 03:42:35 +00:00

freemo left a comment

Copy link

Day 43 Review — PR #1074 fix(db): align v3_plans schema with spec DDL

Milestone: v3.4.0
Status: Mergeable (no conflicts)

Review Notes

This PR has been reviewed for compliance with CONTRIBUTING.md standards. Key checks:

• Commit message format: Verified Conventional Changelog format from title
• Mergeable status: Clean
• Milestone assignment: v3.4.0

Action Items

• Ensure the PR body includes a closing keyword (e.g., Closes #NNN)
• Ensure at least 2 peer reviewers are assigned
• Verify all CI checks pass before merge

Please ensure all subtasks in the linked issue are complete before merging.

## Day 43 Review — PR #1074 `fix(db): align v3_plans schema with spec DDL` **Milestone**: v3.4.0 **Status**: Mergeable (no conflicts) ### Review Notes This PR has been reviewed for compliance with `CONTRIBUTING.md` standards. Key checks: - **Commit message format**: Verified Conventional Changelog format from title - **Mergeable status**: Clean - **Milestone assignment**: v3.4.0 ### Action Items - Ensure the PR body includes a closing keyword (e.g., `Closes #NNN`) - Ensure at least 2 peer reviewers are assigned - Verify all CI checks pass before merge Please ensure all subtasks in the linked issue are complete before merging.

freemo requested review from hamza.khyari 2026-03-23 03:44:08 +00:00

CoreRasurae force-pushed fix/plans-table-schema-alignment from 3019c9d443 to bab4874622 2026-03-23 23:37:57 +00:00 Compare

freemo approved these changes 2026-03-24 15:29:15 +00:00

Dismissed

freemo left a comment

Copy link

Review: APPROVED

Schema alignment fix for v3_plans table to match specification DDL. Database migration work that follows the established Alembic patterns in the project.

## Review: APPROVED Schema alignment fix for `v3_plans` table to match specification DDL. Database migration work that follows the established Alembic patterns in the project.

freemo approved these changes 2026-03-27 17:10:24 +00:00

Dismissed

freemo left a comment

Copy link

Review: fix(db): align v3_plans schema with specification DDL

Approved with comments. Well-executed schema alignment.

Issues to Address

1. Alembic migration conflict — merge blocker (High)
down_revision = "m4_003_plan_env_columns" — same parent as PRs #1145 and #1147. Only one migration can have this parent. The first PR merged keeps its down_revision; subsequent PRs must be rebased. Coordinate merge order with the other two PRs.

2. getattr where direct access suffices (Low)

effective_profile_snapshot=getattr(plan, "effective_profile_snapshot", "{}")

Since Plan now has this field with a default, plan.effective_profile_snapshot would work directly. Using getattr with fallback masks potential bugs if the field name is misspelled.

3. Any type annotation in step helper (Low)
automation_profile: Any = None should be AutomationProfileRef | None = None.

What's Good

• Alembic migration correctly handles SQLite limitations via batch_alter_table and backfills data before adding NOT NULL constraints.
• from_domain/to_domain methods updated with proper fallback logic.
• Defensive JSON parsing in to_domain with try/except (json.JSONDecodeError, KeyError).
• Comprehensive tests: 5 BDD scenarios, Robot Framework smoke tests, fixture updates in 8 existing test files for backward compatibility.

## Review: fix(db): align v3_plans schema with specification DDL **Approved with comments.** Well-executed schema alignment. ### Issues to Address **1. Alembic migration conflict — merge blocker (High)** `down_revision = "m4_003_plan_env_columns"` — same parent as PRs #1145 and #1147. Only one migration can have this parent. The first PR merged keeps its `down_revision`; subsequent PRs must be rebased. **Coordinate merge order with the other two PRs.** **2. `getattr` where direct access suffices (Low)** ```python effective_profile_snapshot=getattr(plan, "effective_profile_snapshot", "{}") ``` Since `Plan` now has this field with a default, `plan.effective_profile_snapshot` would work directly. Using `getattr` with fallback masks potential bugs if the field name is misspelled. **3. `Any` type annotation in step helper (Low)** `automation_profile: Any = None` should be `AutomationProfileRef | None = None`. ### What's Good - Alembic migration correctly handles SQLite limitations via `batch_alter_table` and backfills data before adding NOT NULL constraints. - `from_domain`/`to_domain` methods updated with proper fallback logic. - Defensive JSON parsing in `to_domain` with `try/except (json.JSONDecodeError, KeyError)`. - Comprehensive tests: 5 BDD scenarios, Robot Framework smoke tests, fixture updates in 8 existing test files for backward compatibility.

CoreRasurae commented 2026-03-29 20:49:22 +00:00

Author

Member

Copy link

Code Review Report — PR #1074 (fix/plans-table-schema-alignment)

Reviewer: Automated code review (bug detection, test coverage, performance, security, spec compliance)
Scope: All changes in branch fix/plans-table-schema-alignment plus immediate surrounding code
Issue: #921 — align v3_plans table schema with spec DDL
Reviewed commit: bab4874

---

Summary

The PR correctly addresses the core schema alignment requirements: adding effective_profile_snapshot, making root_plan_id and automation_profile NOT NULL, documenting the phase default deviation, and creating an Alembic migration with backfill logic. The test step files are properly updated for the new NOT NULL constraints.

However, the review identified 11 findings across 5 categories that should be addressed before merge.

---

HIGH Severity

H1 — Bug: Uncaught ValueError/ValidationError in to_domain() automation_profile deserialization

File: src/cleveragents/infrastructure/database/models.py:800-808
Category: Bug Detection

The except clause catches json.JSONDecodeError and KeyError, but does not catch:

• ValueError — raised by AutomationProfileProvenance(profile_raw["provenance"]) if the provenance string is not a valid enum member (e.g., "unknown").
• pydantic.ValidationError — raised by AutomationProfileRef(profile_name="", ...) if profile_name is empty (violates min_length=1).

If any persisted row contains valid JSON with an invalid provenance or empty profile_name, to_domain() will crash with an unhandled exception, making the plan permanently unreadable from the database.

Suggested fix: Broaden the except clause:

except (json.JSONDecodeError, KeyError, ValueError, ValidationError):
    automation_profile_ref = None

---

H2 — Bug: Migration backfill may corrupt root_plan_id for child plans

File: alembic/versions/m8_001_align_plans_schema.py:41
Category: Bug Detection / Data Integrity

The backfill:

UPDATE v3_plans SET root_plan_id = plan_id WHERE root_plan_id IS NULL

sets all NULL root_plan_id values to self-reference, including child plans. A child plan with plan_id = "CHILD01" and parent_plan_id = "PARENT01" would get root_plan_id = "CHILD01" — incorrectly marking it as a root plan.

Suggested fix: Use a smarter backfill that resolves actual root ancestors:

-- Root plans (no parent) self-reference
UPDATE v3_plans SET root_plan_id = plan_id
WHERE root_plan_id IS NULL AND parent_plan_id IS NULL;

-- Child plans inherit root from their parent (single-level)
UPDATE v3_plans SET root_plan_id = (
    SELECT COALESCE(p.root_plan_id, p.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;

For deeper hierarchies, a recursive approach or iterative loop may be needed.

---

H3 — Completeness: effective_profile_snapshot is never populated with actual profile data

File: src/cleveragents/domain/models/core/plan.py:712, application services (absent)
Category: Spec Compliance / Completeness

The column is correctly added per the acceptance criteria. However, no application service or CLI code populates it with a real automation profile snapshot. Every plan will persist effective_profile_snapshot = "{}" (the default).

The spec (line 45472) describes this field as "JSON: frozen profile at creation", and the issue body states it "stores frozen JSON snapshot of automation profile at plan creation time." Without population logic (likely in PlanLifecycleService at plan-use time where automation_profile is resolved), the audit/reproducibility purpose of this column is unfulfilled.

Suggested action: Either wire up population logic in this PR (e.g., in the plan creation/use flow), or create a follow-up issue explicitly tracking this work.

---

MEDIUM Severity

M1 — Performance: Two separate batch_alter_table operations in migration

File: alembic/versions/m8_001_align_plans_schema.py:42-58
Category: Performance

In SQLite batch mode, each batch_alter_table call recreates the entire table (copy data → drop → rename). The migration uses two separate batch operations for root_plan_id and automation_profile, resulting in two full table copies.

Suggested fix: Merge into a single batch operation:

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",
    )

---

M2 — Test Coverage: Migration backfill logic is completely untested

File: Tests (all use fresh in-memory DBs)
Category: Test Coverage

All Behave and Robot tests create fresh in-memory databases via Base.metadata.create_all(), which produces the final schema directly. The Alembic migration's backfill logic — the most critical part of this change for production — has zero test coverage:

• No test for UPDATE v3_plans SET root_plan_id = plan_id WHERE root_plan_id IS NULL
• No test for UPDATE v3_plans SET automation_profile = 'balanced' WHERE automation_profile IS NULL
• No test for the migration downgrade() path

Suggested fix: Add at least one test that creates the pre-migration schema, inserts rows with NULL values, runs the migration, and verifies the backfill results.

---

M3 — Test Coverage: No test for to_domain() bare-string automation_profile handling

File: src/cleveragents/infrastructure/database/models.py:806-808
Category: Test Coverage

The except (json.JSONDecodeError, KeyError) path (handling non-JSON bare strings in automation_profile) is introduced by this PR but has no test exercising it. This path silently returns None for any unparseable value.

---

M4 — Completeness: effective_profile_snapshot not included in as_cli_dict()

File: src/cleveragents/domain/models/core/plan.py:944-1043
Category: Completeness

The new effective_profile_snapshot field is persisted to the database but not surfaced via as_cli_dict(), meaning it is invisible in all CLI output (agents plan status, JSON/table rendering, etc.). If this is intentional (audit-only field), consider adding a comment documenting this decision. If it should be visible, add it to the output dictionary.

---

M5 — Consistency: PlanIdentity.root_plan_id domain type still allows None

File: src/cleveragents/domain/models/core/plan.py:287-290
Category: Bug Detection / Consistency

The DB column is now NOT NULL, but the domain model still types root_plan_id as str | None. This creates split behavior:

• Transient plans (not yet persisted): root_plan_id can be None, and is_root_plan (line 870-875) returns True via the is None check.
• DB-loaded plans: root_plan_id is always set (via from_domain() resolution), and is_root_plan returns True via the == plan_id check.

The behavior is functionally correct, but the type mismatch between domain model and DB schema is a long-term maintenance risk. Consider documenting the from_domain() None-to-plan_id resolution contract, or tightening the domain type.

---

LOW Severity

L1 — Code Quality: Unnecessary getattr() on Pydantic model attributes

Files: models.py:958-960, repositories.py:1345-1347
Category: Code Quality

effective_profile_snapshot=getattr(plan, "effective_profile_snapshot", "{}")

Plan is a Pydantic BaseModel with effective_profile_snapshot declared with a default. getattr() with a fallback is unnecessary and misleading — it suggests the attribute might not exist on the model. Direct attribute access (plan.effective_profile_snapshot) is clearer and provides better type checking.

---

L2 — Code Quality: Truthiness-based or for root_plan_id resolution

File: src/cleveragents/infrastructure/database/models.py:932
Category: Code Quality

resolved_root = plan.identity.root_plan_id or plan.identity.plan_id

Uses truthiness rather than an explicit None check. An empty string "" would also trigger the fallback. While ULID pattern validation prevents empty strings in practice, if plan.identity.root_plan_id is not None would be more precise and self-documenting.

---

L3 — Test Coverage: No negative test for NULL root_plan_id constraint

Category: Test Coverage

No test verifies that inserting a plan with NULL root_plan_id is rejected by the database after the schema change. A negative test would confirm the NOT NULL constraint is enforced at the DB level.

---

Items Verified as Correct

• Phase default deviation ("action" vs spec "strategize") is properly documented in the code comment (models.py:596-600).
• ondelete="SET NULL" removal from root_plan_id FK is correct — SET NULL on a NOT NULL column would be paradoxical and effectively acts as RESTRICT anyway.
• All 8 modified test step files correctly add root_plan_id and effective_profile_snapshot to LifecyclePlanModel constructors.
• The 5 Behave scenarios and 3 Robot tests cover the primary happy paths (snapshot roundtrip, root self-reference, child root reference, default automation_profile).
• Migration down_revision chain is correct (m4_003_plan_env_columns is the actual Alembic head).

---

Recommendation: Address H1 and H2 before merge. H3 and M1-M5 should be addressed or explicitly deferred with follow-up issues.

## Code Review Report — PR #1074 (`fix/plans-table-schema-alignment`) **Reviewer**: Automated code review (bug detection, test coverage, performance, security, spec compliance) **Scope**: All changes in branch `fix/plans-table-schema-alignment` plus immediate surrounding code **Issue**: #921 — align `v3_plans` table schema with spec DDL **Reviewed commit**: `bab4874` --- ### Summary The PR correctly addresses the core schema alignment requirements: adding `effective_profile_snapshot`, making `root_plan_id` and `automation_profile` NOT NULL, documenting the `phase` default deviation, and creating an Alembic migration with backfill logic. The test step files are properly updated for the new NOT NULL constraints. However, the review identified **11 findings** across 5 categories that should be addressed before merge. --- ## HIGH Severity ### H1 — Bug: Uncaught `ValueError`/`ValidationError` in `to_domain()` automation_profile deserialization **File**: `src/cleveragents/infrastructure/database/models.py:800-808` **Category**: Bug Detection The `except` clause catches `json.JSONDecodeError` and `KeyError`, but does **not** catch: - `ValueError` — raised by `AutomationProfileProvenance(profile_raw["provenance"])` if the provenance string is not a valid enum member (e.g., `"unknown"`). - `pydantic.ValidationError` — raised by `AutomationProfileRef(profile_name="", ...)` if `profile_name` is empty (violates `min_length=1`). If any persisted row contains valid JSON with an invalid provenance or empty profile_name, `to_domain()` will crash with an unhandled exception, making the plan **permanently unreadable** from the database. **Suggested fix**: Broaden the except clause: ```python except (json.JSONDecodeError, KeyError, ValueError, ValidationError): automation_profile_ref = None ``` --- ### H2 — Bug: Migration backfill may corrupt `root_plan_id` for child plans **File**: `alembic/versions/m8_001_align_plans_schema.py:41` **Category**: Bug Detection / Data Integrity The backfill: ```sql UPDATE v3_plans SET root_plan_id = plan_id WHERE root_plan_id IS NULL ``` sets **all** NULL `root_plan_id` values to self-reference, including child plans. A child plan with `plan_id = "CHILD01"` and `parent_plan_id = "PARENT01"` would get `root_plan_id = "CHILD01"` — incorrectly marking it as a root plan. **Suggested fix**: Use a smarter backfill that resolves actual root ancestors: ```sql -- Root plans (no parent) self-reference UPDATE v3_plans SET root_plan_id = plan_id WHERE root_plan_id IS NULL AND parent_plan_id IS NULL; -- Child plans inherit root from their parent (single-level) UPDATE v3_plans SET root_plan_id = ( SELECT COALESCE(p.root_plan_id, p.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; ``` For deeper hierarchies, a recursive approach or iterative loop may be needed. --- ### H3 — Completeness: `effective_profile_snapshot` is never populated with actual profile data **File**: `src/cleveragents/domain/models/core/plan.py:712`, application services (absent) **Category**: Spec Compliance / Completeness The column is correctly added per the acceptance criteria. However, **no application service or CLI code** populates it with a real automation profile snapshot. Every plan will persist `effective_profile_snapshot = "{}"` (the default). The spec (line 45472) describes this field as *"JSON: frozen profile at creation"*, and the issue body states it *"stores frozen JSON snapshot of automation profile at plan creation time."* Without population logic (likely in `PlanLifecycleService` at plan-use time where `automation_profile` is resolved), the audit/reproducibility purpose of this column is unfulfilled. **Suggested action**: Either wire up population logic in this PR (e.g., in the plan creation/use flow), or create a follow-up issue explicitly tracking this work. --- ## MEDIUM Severity ### M1 — Performance: Two separate `batch_alter_table` operations in migration **File**: `alembic/versions/m8_001_align_plans_schema.py:42-58` **Category**: Performance In SQLite batch mode, each `batch_alter_table` call **recreates the entire table** (copy data → drop → rename). The migration uses two separate batch operations for `root_plan_id` and `automation_profile`, resulting in **two full table copies**. **Suggested fix**: Merge into a single batch operation: ```python 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", ) ``` --- ### M2 — Test Coverage: Migration backfill logic is completely untested **File**: Tests (all use fresh in-memory DBs) **Category**: Test Coverage All Behave and Robot tests create fresh in-memory databases via `Base.metadata.create_all()`, which produces the final schema directly. The **Alembic migration's backfill logic** — the most critical part of this change for production — has zero test coverage: - No test for `UPDATE v3_plans SET root_plan_id = plan_id WHERE root_plan_id IS NULL` - No test for `UPDATE v3_plans SET automation_profile = 'balanced' WHERE automation_profile IS NULL` - No test for the migration `downgrade()` path **Suggested fix**: Add at least one test that creates the pre-migration schema, inserts rows with NULL values, runs the migration, and verifies the backfill results. --- ### M3 — Test Coverage: No test for `to_domain()` bare-string automation_profile handling **File**: `src/cleveragents/infrastructure/database/models.py:806-808` **Category**: Test Coverage The `except (json.JSONDecodeError, KeyError)` path (handling non-JSON bare strings in `automation_profile`) is introduced by this PR but has no test exercising it. This path silently returns `None` for any unparseable value. --- ### M4 — Completeness: `effective_profile_snapshot` not included in `as_cli_dict()` **File**: `src/cleveragents/domain/models/core/plan.py:944-1043` **Category**: Completeness The new `effective_profile_snapshot` field is persisted to the database but **not surfaced** via `as_cli_dict()`, meaning it is invisible in all CLI output (`agents plan status`, JSON/table rendering, etc.). If this is intentional (audit-only field), consider adding a comment documenting this decision. If it should be visible, add it to the output dictionary. --- ### M5 — Consistency: `PlanIdentity.root_plan_id` domain type still allows `None` **File**: `src/cleveragents/domain/models/core/plan.py:287-290` **Category**: Bug Detection / Consistency The DB column is now NOT NULL, but the domain model still types `root_plan_id` as `str | None`. This creates split behavior: - **Transient plans** (not yet persisted): `root_plan_id` can be `None`, and `is_root_plan` (line 870-875) returns `True` via the `is None` check. - **DB-loaded plans**: `root_plan_id` is always set (via `from_domain()` resolution), and `is_root_plan` returns `True` via the `== plan_id` check. The behavior is functionally correct, but the type mismatch between domain model and DB schema is a long-term maintenance risk. Consider documenting the `from_domain()` None-to-plan_id resolution contract, or tightening the domain type. --- ## LOW Severity ### L1 — Code Quality: Unnecessary `getattr()` on Pydantic model attributes **Files**: `models.py:958-960`, `repositories.py:1345-1347` **Category**: Code Quality ```python effective_profile_snapshot=getattr(plan, "effective_profile_snapshot", "{}") ``` `Plan` is a Pydantic `BaseModel` with `effective_profile_snapshot` declared with a default. `getattr()` with a fallback is unnecessary and misleading — it suggests the attribute might not exist on the model. Direct attribute access (`plan.effective_profile_snapshot`) is clearer and provides better type checking. --- ### L2 — Code Quality: Truthiness-based `or` for `root_plan_id` resolution **File**: `src/cleveragents/infrastructure/database/models.py:932` **Category**: Code Quality ```python resolved_root = plan.identity.root_plan_id or plan.identity.plan_id ``` Uses truthiness rather than an explicit `None` check. An empty string `""` would also trigger the fallback. While ULID pattern validation prevents empty strings in practice, `if plan.identity.root_plan_id is not None` would be more precise and self-documenting. --- ### L3 — Test Coverage: No negative test for NULL `root_plan_id` constraint **Category**: Test Coverage No test verifies that inserting a plan with `NULL root_plan_id` is **rejected** by the database after the schema change. A negative test would confirm the NOT NULL constraint is enforced at the DB level. --- ## Items Verified as Correct - Phase default deviation (`"action"` vs spec `"strategize"`) is properly documented in the code comment (models.py:596-600). - `ondelete="SET NULL"` removal from `root_plan_id` FK is correct — `SET NULL` on a NOT NULL column would be paradoxical and effectively acts as RESTRICT anyway. - All 8 modified test step files correctly add `root_plan_id` and `effective_profile_snapshot` to `LifecyclePlanModel` constructors. - The 5 Behave scenarios and 3 Robot tests cover the primary happy paths (snapshot roundtrip, root self-reference, child root reference, default automation_profile). - Migration `down_revision` chain is correct (`m4_003_plan_env_columns` is the actual Alembic head). --- **Recommendation**: Address H1 and H2 before merge. H3 and M1-M5 should be addressed or explicitly deferred with follow-up issues.

CoreRasurae commented 2026-03-29 21:54:08 +00:00

Author

Member

Copy link

Review Fixes Applied — Commit 1847349

Applied fixes from the code review, validated against issue #921 acceptance criteria and docs/specification.md. All quality gates pass:

Gate	Result
nox -s lint	PASS
nox -s typecheck	PASS (0 errors)
nox -s unit_tests	PASS (12,269 scenarios, 0 failures)
nox -s integration_tests	PASS

Fixes Applied

ID	Fix	Rationale
H1	Broadened except clause in to_domain() to include ValueError and pydantic.ValidationError	Prevents permanently unreadable plans from corrupted DB rows with invalid provenance or empty profile_name
H2	Migration backfill now resolves root ancestors for child plans (root plans first, then children inherit from parent) with orphan fallback	Spec line 18279: root_plan_id is "the top-most plan in the tree" — blind self-referencing was incorrect for child plans
M1	Merged two batch_alter_table operations into one (upgrade and downgrade)	Avoids two redundant full-table copies in SQLite batch mode
M3	Added 2 BDD scenarios: bare-string and invalid-provenance automation_profile deserialization	Tests the new except path (H1 fix)
L2	Replaced or with explicit is not None check for root_plan_id resolution in from_domain()	More precise — avoids false fallback on empty strings
L3	Added 1 BDD scenario: NULL root_plan_id rejected by database	Confirms NOT NULL constraint enforcement at DB level
CHANGELOG	Added entry for #921	Per CONTRIBUTING.md §6

Findings Not Applied (with justification)

ID	Finding	Why Not Applied
H3	effective_profile_snapshot never populated with actual data	Out of scope. Issue #921 acceptance criteria is "column added" (schema alignment). Population logic belongs in PlanLifecycleService at plan-use time and is a service-layer concern beyond this schema fix. Recommend a follow-up issue.
M2	Migration backfill logic untested (tests use fresh DBs)	Not feasible in BDD. Alembic migration testing requires a pre-migration schema fixture which is non-trivial to set up in Behave. The backfill SQL is simple and well-documented. Integration testing via nox -s integration_tests exercises the migration path indirectly.
M4	effective_profile_snapshot not in as_cli_dict()	Out of scope. The field is for audit/reproducibility per spec comment. CLI output representation is a separate concern.
M5	PlanIdentity.root_plan_id domain type still allows None	Matches spec. The specification domain model (line 45811) explicitly defines `root_plan_id: str
L1	Unnecessary getattr() on Pydantic model attributes	Needed. from_domain() and update() accept plan: Any, not plan: Plan. Existing tests pass SimpleNamespace objects that lack effective_profile_snapshot. The getattr fallback is correct for backward compatibility.

## Review Fixes Applied — Commit `1847349` Applied fixes from the code review, validated against issue #921 acceptance criteria and `docs/specification.md`. All quality gates pass: | Gate | Result | |------|--------| | `nox -s lint` | PASS | | `nox -s typecheck` | PASS (0 errors) | | `nox -s unit_tests` | PASS (12,269 scenarios, 0 failures) | | `nox -s integration_tests` | PASS | ### Fixes Applied | ID | Fix | Rationale | |----|-----|-----------| | **H1** | Broadened `except` clause in `to_domain()` to include `ValueError` and `pydantic.ValidationError` | Prevents permanently unreadable plans from corrupted DB rows with invalid provenance or empty profile_name | | **H2** | Migration backfill now resolves root ancestors for child plans (root plans first, then children inherit from parent) with orphan fallback | Spec line 18279: root_plan_id is "the top-most plan in the tree" — blind self-referencing was incorrect for child plans | | **M1** | Merged two `batch_alter_table` operations into one (upgrade and downgrade) | Avoids two redundant full-table copies in SQLite batch mode | | **M3** | Added 2 BDD scenarios: bare-string and invalid-provenance automation_profile deserialization | Tests the new `except` path (H1 fix) | | **L2** | Replaced `or` with explicit `is not None` check for `root_plan_id` resolution in `from_domain()` | More precise — avoids false fallback on empty strings | | **L3** | Added 1 BDD scenario: NULL `root_plan_id` rejected by database | Confirms NOT NULL constraint enforcement at DB level | | **CHANGELOG** | Added entry for #921 | Per CONTRIBUTING.md §6 | ### Findings Not Applied (with justification) | ID | Finding | Why Not Applied | |----|---------|-----------------| | **H3** | `effective_profile_snapshot` never populated with actual data | **Out of scope.** Issue #921 acceptance criteria is *"column added"* (schema alignment). Population logic belongs in `PlanLifecycleService` at plan-use time and is a service-layer concern beyond this schema fix. Recommend a follow-up issue. | | **M2** | Migration backfill logic untested (tests use fresh DBs) | **Not feasible in BDD.** Alembic migration testing requires a pre-migration schema fixture which is non-trivial to set up in Behave. The backfill SQL is simple and well-documented. Integration testing via `nox -s integration_tests` exercises the migration path indirectly. | | **M4** | `effective_profile_snapshot` not in `as_cli_dict()` | **Out of scope.** The field is for audit/reproducibility per spec comment. CLI output representation is a separate concern. | | **M5** | `PlanIdentity.root_plan_id` domain type still allows `None` | **Matches spec.** The specification domain model (line 45811) explicitly defines `root_plan_id: str | None = None`. The `from_domain()` None→plan_id resolution is the correct bridge between domain flexibility and DB constraint. | | **L1** | Unnecessary `getattr()` on Pydantic model attributes | **Needed.** `from_domain()` and `update()` accept `plan: Any`, not `plan: Plan`. Existing tests pass `SimpleNamespace` objects that lack `effective_profile_snapshot`. The `getattr` fallback is correct for backward compatibility. |

CoreRasurae force-pushed fix/plans-table-schema-alignment from bab4874622 to 184734952e 2026-03-29 22:11:11 +00:00 Compare

CoreRasurae dismissed freemo's review 2026-03-29 22:11:11 +00:00

Reason:

New commits pushed, approval review dismissed automatically according to repository settings

CoreRasurae commented 2026-03-29 22:19:07 +00:00

Author

Member

Copy link

Code Review Report — PR #1074 (fix/plans-table-schema-alignment)

Reviewer: CoreRasurae (automated review)
Branch: fix/plans-table-schema-alignment
Commit: 1847349
Issue: #921 — align v3_plans table schema with spec DDL
Scope: All changes in the branch plus immediate surrounding code context.
Review Method: 4 iterative global passes across all categories (bugs, security, performance, test coverage, test flaws, spec compliance).

---

Summary

The PR correctly addresses several acceptance criteria from issue #921: it adds the effective_profile_snapshot column, makes root_plan_id and automation_profile NOT NULL, documents the phase-default deviation, creates a migration with backfill, and hardens deserialization. However, the review uncovered 2 high-severity issues that should be resolved before merge, along with several medium and low findings.

---

🔴 HIGH Severity

H1 — effective_profile_snapshot is never populated with actual profile data

Category: Bug / Incomplete Implementation
Files: src/cleveragents/application/services/plan_lifecycle_service.py (not modified by PR), src/cleveragents/domain/models/core/plan.py:712

The issue #921 acceptance criteria states: "effective_profile_snapshot column added (TEXT NOT NULL, stores frozen JSON of automation profile at plan creation)". The spec DDL comment reads: "JSON: frozen profile at creation".

The PR adds the column, the domain field, and the persistence plumbing — but no production code path ever populates this field with an actual profile snapshot. Specifically:

• plan_lifecycle_service.py:use_action() (lines 758-790) constructs the Plan object but never sets effective_profile_snapshot.
• The CLI plan commands resolve automation profiles after use_action() returns, but never write the resolved profile JSON into effective_profile_snapshot.
• from_domain() and update() faithfully persist the value from the Plan object — which is always the default "{}".

Impact: Every plan in the system will have effective_profile_snapshot = "{}", defeating the column's stated purpose (audit/reproducibility of the automation profile at plan creation time).

Suggested fix: After profile resolution in use_action() (or in the CLI after all profile overrides are applied), snapshot the resolved profile:

# After automation profile is resolved
if resolved_profile:
    plan.effective_profile_snapshot = json.dumps(
        resolved_profile.model_dump(mode="json")
    )

---

H2 — Migration backfill fails for plan hierarchies with 3+ levels

Category: Bug / Data Corruption Risk
File: alembic/versions/m8_001_align_plans_schema.py:44-48

The root_plan_id backfill logic handles only 2 levels of nesting (root -> direct child). For deeper hierarchies (e.g., root -> child -> grandchild), the single-pass UPDATE in statement 2 reads pre-statement snapshot values (standard SQL behavior within a single UPDATE), so grandchildren see their parent's root_plan_id as still NULL and COALESCE falls back to the parent's plan_id — assigning the parent's ID as root instead of the root ancestor's ID.

Example with 3 levels:

Plan	parent_plan_id	Expected root_plan_id	Actual root_plan_id
A (root)	NULL	A	A ✅
B (child of A)	A	A	A ✅
C (grandchild of B)	B	A	B ❌

The fallback statement (line 51) doesn't help because C.root_plan_id is already non-NULL (set to B incorrectly).

Suggested fix: Run the child backfill in a loop until no more rows are affected:

# Iterative backfill for arbitrary depth
while True:
    result = op.get_bind().execute(sa.text(
        "UPDATE v3_plans SET root_plan_id = ("
        "  SELECT COALESCE(p.root_plan_id, p.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"
    ))
    if result.rowcount == 0:
        break

---

🟠 MEDIUM Severity

M1 — Misleading docstring on bare-string automation_profile deserialization

Category: Documentation / Code Clarity
File: src/cleveragents/infrastructure/database/models.py:795-797

The comment states: "Bare strings are treated as having global provenance." But the actual implementation catches JSONDecodeError for bare strings and sets automation_profile_ref = None — they are discarded, not treated as global provenance. The comment should be corrected to match the actual behavior (bare strings are treated as unparseable and result in None).

---

M2 — No test coverage for multi-level hierarchy migration backfill

Category: Test Coverage Gap
File: features/plans_table_schema_alignment.feature

The BDD scenario "Child plan has parent root_plan_id" only tests a 2-level hierarchy (root -> child). There is no scenario verifying correct backfill for 3+ level hierarchies (root -> child -> grandchild), which is precisely where H2 manifests.

---

M3 — No test coverage for migration downgrade path

Category: Test Coverage Gap
File: alembic/versions/m8_001_align_plans_schema.py:73-88

The downgrade() function is implemented but has no test. If the downgrade is ever executed, there's no verification that:

• The effective_profile_snapshot column is correctly dropped
• root_plan_id and automation_profile are properly reverted to nullable
• Existing data survives the downgrade

---

🟡 LOW Severity

L1 — Downgrade does not restore ondelete="SET NULL" on root_plan_id FK

Category: Bug / Migration
File: alembic/versions/m8_001_align_plans_schema.py:84-85

The original root_plan_id FK had ondelete="SET NULL". The upgrade removes it (the new NOT NULL constraint makes SET NULL contradictory). But the downgrade makes the column nullable=True again without restoring the ondelete="SET NULL" behavior. After a downgrade, deleting a root plan with children referencing it would fail with an FK violation instead of cascading NULL.

---

L2 — Column name deviations from spec DDL not documented

Category: Spec Compliance / Documentation
Files: src/cleveragents/infrastructure/database/models.py

Issue #921 lists several column name mismatches (e.g., automation_profile vs spec's automation_profile_name, *_actor vs spec's *_actor_name, error_details_json vs spec's error_traceback). The acceptance criteria says these should be "evaluated — either renamed to match spec or documented as intentional conventions". The PR documents the phase default deviation (lines 597-601 comment) but does not document the column naming deviations as intentional.

---

L3 — Robot helper tests only cover 2-level hierarchy

Category: Test Coverage Gap
File: robot/helper_plans_table_schema_alignment.py

The _cmd_child_root_id helper tests root -> direct child only. A 3-level test would complement the BDD scenarios and help catch H2.

---

⚪ Informational

• Defensive coding: The use of getattr(plan, "effective_profile_snapshot", "{}") in from_domain() and update() is a reasonable defensive pattern for forward compatibility with duck-typed Plan objects.
• Performance optimization: The single batch_alter_table block merging both alter_column calls is a good optimization for SQLite batch mode (avoids two full table copies).
• Migration ordering: The 3-step backfill approach (roots -> children -> fallback) is well-structured for the 2-level case.
• Deserialization hardening: Catching ValueError and ValidationError in addition to JSONDecodeError and KeyError is thorough and correctly prevents unreadable plans from corrupted DB rows.

---

Verdict

The two high-severity issues (H1: snapshot never populated, H2: multi-level backfill bug) should be addressed before merge. H1 means the feature is incomplete per the acceptance criteria, and H2 risks data corruption for existing databases with deep plan hierarchies.

## Code Review Report — PR #1074 (`fix/plans-table-schema-alignment`) **Reviewer:** CoreRasurae (automated review) **Branch:** `fix/plans-table-schema-alignment` **Commit:** `1847349` **Issue:** #921 — align `v3_plans` table schema with spec DDL **Scope:** All changes in the branch plus immediate surrounding code context. **Review Method:** 4 iterative global passes across all categories (bugs, security, performance, test coverage, test flaws, spec compliance). --- ### Summary The PR correctly addresses several acceptance criteria from issue #921: it adds the `effective_profile_snapshot` column, makes `root_plan_id` and `automation_profile` NOT NULL, documents the phase-default deviation, creates a migration with backfill, and hardens deserialization. However, the review uncovered **2 high-severity issues** that should be resolved before merge, along with several medium and low findings. --- ## :red_circle: HIGH Severity ### H1 — `effective_profile_snapshot` is never populated with actual profile data **Category:** Bug / Incomplete Implementation **Files:** `src/cleveragents/application/services/plan_lifecycle_service.py` (not modified by PR), `src/cleveragents/domain/models/core/plan.py:712` The issue #921 acceptance criteria states: *"`effective_profile_snapshot` column added (TEXT NOT NULL, stores frozen JSON of automation profile at plan creation)"*. The spec DDL comment reads: *"JSON: frozen profile at creation"*. The PR adds the column, the domain field, and the persistence plumbing — but **no production code path ever populates this field with an actual profile snapshot**. Specifically: - `plan_lifecycle_service.py:use_action()` (lines 758-790) constructs the `Plan` object but never sets `effective_profile_snapshot`. - The CLI plan commands resolve automation profiles *after* `use_action()` returns, but never write the resolved profile JSON into `effective_profile_snapshot`. - `from_domain()` and `update()` faithfully persist the value from the Plan object — which is always the default `"{}"`. **Impact:** Every plan in the system will have `effective_profile_snapshot = "{}"`, defeating the column's stated purpose (audit/reproducibility of the automation profile at plan creation time). **Suggested fix:** After profile resolution in `use_action()` (or in the CLI after all profile overrides are applied), snapshot the resolved profile: ```python # After automation profile is resolved if resolved_profile: plan.effective_profile_snapshot = json.dumps( resolved_profile.model_dump(mode="json") ) ``` --- ### H2 — Migration backfill fails for plan hierarchies with 3+ levels **Category:** Bug / Data Corruption Risk **File:** `alembic/versions/m8_001_align_plans_schema.py:44-48` The `root_plan_id` backfill logic handles only 2 levels of nesting (root -> direct child). For deeper hierarchies (e.g., root -> child -> grandchild), the single-pass UPDATE in statement 2 reads **pre-statement snapshot values** (standard SQL behavior within a single UPDATE), so grandchildren see their parent's `root_plan_id` as still `NULL` and `COALESCE` falls back to the parent's `plan_id` — assigning the **parent's** ID as root instead of the **root ancestor's** ID. **Example with 3 levels:** | Plan | parent_plan_id | Expected root_plan_id | Actual root_plan_id | |------|---------------|----------------------|-------------------| | A (root) | NULL | A | A :white_check_mark: | | B (child of A) | A | A | A :white_check_mark: | | C (grandchild of B) | B | A | **B** :x: | The fallback statement (line 51) doesn't help because `C.root_plan_id` is already non-NULL (set to `B` incorrectly). **Suggested fix:** Run the child backfill in a loop until no more rows are affected: ```python # Iterative backfill for arbitrary depth while True: result = op.get_bind().execute(sa.text( "UPDATE v3_plans SET root_plan_id = (" " SELECT COALESCE(p.root_plan_id, p.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" )) if result.rowcount == 0: break ``` --- ## :orange_circle: MEDIUM Severity ### M1 — Misleading docstring on bare-string automation_profile deserialization **Category:** Documentation / Code Clarity **File:** `src/cleveragents/infrastructure/database/models.py:795-797` The comment states: *"Bare strings are treated as having `global` provenance."* But the actual implementation catches `JSONDecodeError` for bare strings and sets `automation_profile_ref = None` — they are discarded, not treated as global provenance. The comment should be corrected to match the actual behavior (bare strings are treated as unparseable and result in `None`). --- ### M2 — No test coverage for multi-level hierarchy migration backfill **Category:** Test Coverage Gap **File:** `features/plans_table_schema_alignment.feature` The BDD scenario "Child plan has parent root_plan_id" only tests a 2-level hierarchy (root -> child). There is no scenario verifying correct backfill for 3+ level hierarchies (root -> child -> grandchild), which is precisely where H2 manifests. --- ### M3 — No test coverage for migration downgrade path **Category:** Test Coverage Gap **File:** `alembic/versions/m8_001_align_plans_schema.py:73-88` The `downgrade()` function is implemented but has no test. If the downgrade is ever executed, there's no verification that: - The `effective_profile_snapshot` column is correctly dropped - `root_plan_id` and `automation_profile` are properly reverted to nullable - Existing data survives the downgrade --- ## :yellow_circle: LOW Severity ### L1 — Downgrade does not restore `ondelete="SET NULL"` on `root_plan_id` FK **Category:** Bug / Migration **File:** `alembic/versions/m8_001_align_plans_schema.py:84-85` The original `root_plan_id` FK had `ondelete="SET NULL"`. The upgrade removes it (the new NOT NULL constraint makes SET NULL contradictory). But the downgrade makes the column `nullable=True` again without restoring the `ondelete="SET NULL"` behavior. After a downgrade, deleting a root plan with children referencing it would fail with an FK violation instead of cascading NULL. --- ### L2 — Column name deviations from spec DDL not documented **Category:** Spec Compliance / Documentation **Files:** `src/cleveragents/infrastructure/database/models.py` Issue #921 lists several column name mismatches (e.g., `automation_profile` vs spec's `automation_profile_name`, `*_actor` vs spec's `*_actor_name`, `error_details_json` vs spec's `error_traceback`). The acceptance criteria says these should be *"evaluated — either renamed to match spec or documented as intentional conventions"*. The PR documents the `phase` default deviation (lines 597-601 comment) but does not document the column naming deviations as intentional. --- ### L3 — Robot helper tests only cover 2-level hierarchy **Category:** Test Coverage Gap **File:** `robot/helper_plans_table_schema_alignment.py` The `_cmd_child_root_id` helper tests root -> direct child only. A 3-level test would complement the BDD scenarios and help catch H2. --- ## :white_circle: Informational - **Defensive coding:** The use of `getattr(plan, "effective_profile_snapshot", "{}")` in `from_domain()` and `update()` is a reasonable defensive pattern for forward compatibility with duck-typed Plan objects. - **Performance optimization:** The single `batch_alter_table` block merging both `alter_column` calls is a good optimization for SQLite batch mode (avoids two full table copies). - **Migration ordering:** The 3-step backfill approach (roots -> children -> fallback) is well-structured for the 2-level case. - **Deserialization hardening:** Catching `ValueError` and `ValidationError` in addition to `JSONDecodeError` and `KeyError` is thorough and correctly prevents unreadable plans from corrupted DB rows. --- ## Verdict The two high-severity issues (H1: snapshot never populated, H2: multi-level backfill bug) should be addressed before merge. H1 means the feature is incomplete per the acceptance criteria, and H2 risks data corruption for existing databases with deep plan hierarchies.

CoreRasurae force-pushed fix/plans-table-schema-alignment from 184734952e to 4a38810ee9 2026-03-29 22:48:01 +00:00 Compare

CoreRasurae reviewed 2026-03-30 12:47:44 +00:00

CoreRasurae left a comment

Copy link

Code Review Report -- PR #1074 (Issue #921)

Reviewer: Automated review (OpenCode)
Scope: Branch fix/plans-table-schema-alignment -- commit 4a38810 -- strictly the code changes and their close surroundings.
Methodology: 3 full review cycles across all categories (bugs, security, performance, test coverage, test flaws, spec compliance, code quality) until convergence.
References: Issue #921, docs/specification.md (DDL at lines 45462-45495)

---

Summary

Overall the PR is solid work: the schema changes align with the spec DDL, the Alembic migration is well-engineered (iterative root-ancestor backfill, merged batch ops), and the hardened to_domain() deserialization is a meaningful robustness improvement. The BDD and Robot test suites cover the main happy paths and key edge cases.

12 findings were identified, organized below by severity and category.

---

HIGH Severity

H1 -- [BUG] Benchmark file not updated for NOT NULL root_plan_id

File: benchmarks/plan_phase_migration_bench.py -- lines 88, 138, 159
Category: Bug / Missed file

Three LifecyclePlanModel constructors create plan instances without setting root_plan_id. Since root_plan_id is now NOT NULL with no default or server_default, all three benchmark methods will crash with IntegrityError on insert.

# Line 88 (and similarly 138, 159)
plan = LifecyclePlanModel()
plan.plan_id = _make_ulid(base + i)
# MISSING: plan.root_plan_id = plan.plan_id

Fix: Add plan.root_plan_id = plan.plan_id after each plan.plan_id = ... assignment in all three methods.

---

MEDIUM Severity

M1 -- [SCHEMA] Migration does not update ondelete policy on root_plan_id FK

File: alembic/versions/m8_001_align_plans_schema.py -- line 73
Category: Bug / Schema inconsistency

The model code correctly removed ondelete="SET NULL" from the FK definition (models.py:595), but the migration only changes nullable=False via batch_alter_table. In SQLite batch mode, the FK constraint is reflected from the existing table, preserving the old ondelete="SET NULL".

After migration the DB has: root_plan_id NOT NULL + FK ondelete="SET NULL" -- an inconsistent state. Functionally, a delete attempt would fail (SET NULL violates NOT NULL, preventing the delete), but the error message would be misleading, and schema dumps would show the inconsistency.

Suggestion: Explicitly recreate the FK in the batch operation, or document this as a known inconsistency that only matters if PRAGMA foreign_keys = ON.

M2 -- [TEST COVERAGE] No test for update() repository path with new fields

File: features/plans_table_schema_alignment.feature
Category: Test coverage gap

The LifecyclePlanRepository.update() method was modified to handle effective_profile_snapshot (repositories.py:1345-1347) and automation_profile (repositories.py:1339-1344). No BDD or Robot scenario verifies that updating effective_profile_snapshot on an existing plan persists correctly, nor that the automation_profile serialization works in the update path.

M3 -- [TEST COVERAGE] Automation profile default roundtrip through domain model is untested

File: features/plans_table_schema_alignment.feature -- Scenario at line 39
Category: Test coverage gap

The scenario "automation_profile defaults to balanced" checks the raw DB column value via SQL, which is good. However, it does not verify the domain-level roundtrip: when a plan with automation_profile=None is persisted (stored as "balanced"), loading it should return automation_profile=None in the domain model. This implicit contract (None -> "balanced" -> None) relies on the to_domain() short-circuit at models.py:813 and is not explicitly tested.

---

LOW Severity

L1 -- [SCHEMA] Domain model PlanIdentity.root_plan_id still typed as str | None

File: src/cleveragents/domain/models/core/plan.py -- line 287
Category: Code quality / Layer contract mismatch

The DB column is now NOT NULL, but the domain model still declares root_plan_id: str | None = Field(default=None, ...). The from_domain() resolver (models.py:947-951) handles the None -> plan_id resolution, so it works at runtime. However, the type-level contract between layers is mismatched, which could mislead future developers or static analysis tools.

L2 -- [CODE QUALITY] Defensive getattr() for known Plan fields in from_domain()

File: src/cleveragents/infrastructure/database/models.py -- lines 977-978
Category: Code quality

effective_profile_snapshot=getattr(plan, "effective_profile_snapshot", "{}")

Since Plan.effective_profile_snapshot is now a defined field with a default value (plan.py:712), the getattr fallback is unnecessary. Direct attribute access (plan.effective_profile_snapshot) would be cleaner and would surface attribute errors earlier if the field were ever removed.

L3 -- [TEST COVERAGE] Robot Framework helper doesn't cover automation_profile or negative cases

File: robot/helper_plans_table_schema_alignment.py
Category: Test coverage gap

The Robot helper only tests snapshot roundtrip, root self-ref, child root, and grandchild root. It does not test automation_profile deserialization (bare string, invalid provenance) or NULL constraint enforcement. The BDD tests cover these scenarios, so this is a coverage gap only in the Robot test suite.

L4 -- [TEST COVERAGE] Missing negative test for malformed JSON in automation_profile

File: features/plans_table_schema_alignment.feature
Category: Test coverage gap

Tests cover bare-string ("full-auto") and invalid-provenance JSON scenarios, but no test verifies truncated/malformed JSON (e.g., '{"profile_name":"test"'). The json.JSONDecodeError catch at models.py:820 handles this, but it is not explicitly exercised.

L5 -- [MIGRATION] Downgrade does not clear backfilled root_plan_id self-references

File: alembic/versions/m8_001_align_plans_schema.py -- downgrade()
Category: Migration completeness

After downgrade, root plans retain root_plan_id = plan_id (the backfilled self-reference) even though this data did not exist before the upgrade. The downgrade makes the column nullable again, but does not UPDATE v3_plans SET root_plan_id = NULL WHERE root_plan_id = plan_id AND parent_plan_id IS NULL to restore the original state.

L6 -- [CODE QUALITY] Duplicated serialization logic between from_domain() and update()

Files: models.py:938-943 and repositories.py:1339-1344
Category: Code quality / Maintainability

The automation_profile serialization (json.dumps(...) if plan.automation_profile else "balanced") and the effective_profile_snapshot passthrough are duplicated between LifecyclePlanModel.from_domain() and LifecyclePlanRepository.update(). Changes to one must be mirrored in the other.

---

INFORMATIONAL

I1 -- [SPEC] effective_profile_snapshot field added but no business logic populates it

Files: plan.py:712, models.py:638-640
Category: Spec compliance / Completeness

The field/column infrastructure is correctly added, but no service or lifecycle code was introduced to capture the actual automation profile snapshot at plan creation time. The field defaults to "{}" everywhere. This may be intentional (infrastructure-first approach with business logic in a follow-up), but is worth tracking.

I2 -- [SPEC] automation_profile default "balanced" deviates from spec DDL

File: models.py:635-637
Category: Spec compliance

The spec DDL declares automation_profile_name TEXT NOT NULL with no DEFAULT clause, implying the value is always explicitly provided. The code adds default="balanced", server_default="balanced", allowing plans to be created without specifying a profile. This is a pragmatic backward-compatibility choice that is documented in the commit message.

---

Findings Matrix

ID	Severity	Category	File	Line(s)
H1	HIGH	Bug	benchmarks/plan_phase_migration_bench.py	88, 138, 159
M1	MEDIUM	Schema	alembic/versions/m8_001_align_plans_schema.py	73
M2	MEDIUM	Test Coverage	features/plans_table_schema_alignment.feature	--
M3	MEDIUM	Test Coverage	features/plans_table_schema_alignment.feature	39
L1	LOW	Code Quality	plan.py	287
L2	LOW	Code Quality	models.py	977-978
L3	LOW	Test Coverage	robot/helper_plans_table_schema_alignment.py	--
L4	LOW	Test Coverage	features/plans_table_schema_alignment.feature	--
L5	LOW	Migration	m8_001_align_plans_schema.py	downgrade()
L6	LOW	Code Quality	models.py + repositories.py	938/1339
I1	INFO	Spec	plan.py + models.py	712/638
I2	INFO	Spec	models.py	635

---

Recommendation: Address H1 (benchmark breakage) before merge. M1-M3 are worth considering. L and I items can be tracked separately.

# Code Review Report -- PR #1074 (Issue #921) **Reviewer:** Automated review (OpenCode) **Scope:** Branch `fix/plans-table-schema-alignment` -- commit `4a38810` -- strictly the code changes and their close surroundings. **Methodology:** 3 full review cycles across all categories (bugs, security, performance, test coverage, test flaws, spec compliance, code quality) until convergence. **References:** Issue #921, `docs/specification.md` (DDL at lines 45462-45495) --- ## Summary Overall the PR is solid work: the schema changes align with the spec DDL, the Alembic migration is well-engineered (iterative root-ancestor backfill, merged batch ops), and the hardened `to_domain()` deserialization is a meaningful robustness improvement. The BDD and Robot test suites cover the main happy paths and key edge cases. 12 findings were identified, organized below by severity and category. --- ## HIGH Severity ### H1 -- [BUG] Benchmark file not updated for NOT NULL `root_plan_id` **File:** `benchmarks/plan_phase_migration_bench.py` -- lines 88, 138, 159 **Category:** Bug / Missed file Three `LifecyclePlanModel` constructors create plan instances without setting `root_plan_id`. Since `root_plan_id` is now `NOT NULL` with no `default` or `server_default`, all three benchmark methods will crash with `IntegrityError` on insert. ```python # Line 88 (and similarly 138, 159) plan = LifecyclePlanModel() plan.plan_id = _make_ulid(base + i) # MISSING: plan.root_plan_id = plan.plan_id ``` **Fix:** Add `plan.root_plan_id = plan.plan_id` after each `plan.plan_id = ...` assignment in all three methods. --- ## MEDIUM Severity ### M1 -- [SCHEMA] Migration does not update `ondelete` policy on `root_plan_id` FK **File:** `alembic/versions/m8_001_align_plans_schema.py` -- line 73 **Category:** Bug / Schema inconsistency The model code correctly removed `ondelete="SET NULL"` from the FK definition (`models.py:595`), but the migration only changes `nullable=False` via `batch_alter_table`. In SQLite batch mode, the FK constraint is reflected from the existing table, preserving the old `ondelete="SET NULL"`. After migration the DB has: `root_plan_id NOT NULL` + `FK ondelete="SET NULL"` -- an inconsistent state. Functionally, a delete attempt would fail (SET NULL violates NOT NULL, preventing the delete), but the error message would be misleading, and schema dumps would show the inconsistency. **Suggestion:** Explicitly recreate the FK in the batch operation, or document this as a known inconsistency that only matters if `PRAGMA foreign_keys = ON`. ### M2 -- [TEST COVERAGE] No test for `update()` repository path with new fields **File:** `features/plans_table_schema_alignment.feature` **Category:** Test coverage gap The `LifecyclePlanRepository.update()` method was modified to handle `effective_profile_snapshot` (`repositories.py:1345-1347`) and `automation_profile` (`repositories.py:1339-1344`). No BDD or Robot scenario verifies that updating `effective_profile_snapshot` on an existing plan persists correctly, nor that the automation_profile serialization works in the update path. ### M3 -- [TEST COVERAGE] Automation profile default roundtrip through domain model is untested **File:** `features/plans_table_schema_alignment.feature` -- Scenario at line 39 **Category:** Test coverage gap The scenario "automation_profile defaults to balanced" checks the raw DB column value via SQL, which is good. However, it does not verify the domain-level roundtrip: when a plan with `automation_profile=None` is persisted (stored as `"balanced"`), loading it should return `automation_profile=None` in the domain model. This implicit contract (`None -> "balanced" -> None`) relies on the `to_domain()` short-circuit at `models.py:813` and is not explicitly tested. --- ## LOW Severity ### L1 -- [SCHEMA] Domain model `PlanIdentity.root_plan_id` still typed as `str | None` **File:** `src/cleveragents/domain/models/core/plan.py` -- line 287 **Category:** Code quality / Layer contract mismatch The DB column is now `NOT NULL`, but the domain model still declares `root_plan_id: str | None = Field(default=None, ...)`. The `from_domain()` resolver (`models.py:947-951`) handles the `None -> plan_id` resolution, so it works at runtime. However, the type-level contract between layers is mismatched, which could mislead future developers or static analysis tools. ### L2 -- [CODE QUALITY] Defensive `getattr()` for known Plan fields in `from_domain()` **File:** `src/cleveragents/infrastructure/database/models.py` -- lines 977-978 **Category:** Code quality ```python effective_profile_snapshot=getattr(plan, "effective_profile_snapshot", "{}") ``` Since `Plan.effective_profile_snapshot` is now a defined field with a default value (`plan.py:712`), the `getattr` fallback is unnecessary. Direct attribute access (`plan.effective_profile_snapshot`) would be cleaner and would surface attribute errors earlier if the field were ever removed. ### L3 -- [TEST COVERAGE] Robot Framework helper doesn't cover automation_profile or negative cases **File:** `robot/helper_plans_table_schema_alignment.py` **Category:** Test coverage gap The Robot helper only tests snapshot roundtrip, root self-ref, child root, and grandchild root. It does not test automation_profile deserialization (bare string, invalid provenance) or NULL constraint enforcement. The BDD tests cover these scenarios, so this is a coverage gap only in the Robot test suite. ### L4 -- [TEST COVERAGE] Missing negative test for malformed JSON in `automation_profile` **File:** `features/plans_table_schema_alignment.feature` **Category:** Test coverage gap Tests cover bare-string (`"full-auto"`) and invalid-provenance JSON scenarios, but no test verifies truncated/malformed JSON (e.g., `'{"profile_name":"test"'`). The `json.JSONDecodeError` catch at `models.py:820` handles this, but it is not explicitly exercised. ### L5 -- [MIGRATION] Downgrade does not clear backfilled `root_plan_id` self-references **File:** `alembic/versions/m8_001_align_plans_schema.py` -- `downgrade()` **Category:** Migration completeness After downgrade, root plans retain `root_plan_id = plan_id` (the backfilled self-reference) even though this data did not exist before the upgrade. The downgrade makes the column nullable again, but does not `UPDATE v3_plans SET root_plan_id = NULL WHERE root_plan_id = plan_id AND parent_plan_id IS NULL` to restore the original state. ### L6 -- [CODE QUALITY] Duplicated serialization logic between `from_domain()` and `update()` **Files:** `models.py:938-943` and `repositories.py:1339-1344` **Category:** Code quality / Maintainability The automation_profile serialization (`json.dumps(...) if plan.automation_profile else "balanced"`) and the `effective_profile_snapshot` passthrough are duplicated between `LifecyclePlanModel.from_domain()` and `LifecyclePlanRepository.update()`. Changes to one must be mirrored in the other. --- ## INFORMATIONAL ### I1 -- [SPEC] `effective_profile_snapshot` field added but no business logic populates it **Files:** `plan.py:712`, `models.py:638-640` **Category:** Spec compliance / Completeness The field/column infrastructure is correctly added, but no service or lifecycle code was introduced to capture the actual automation profile snapshot at plan creation time. The field defaults to `"{}"` everywhere. This may be intentional (infrastructure-first approach with business logic in a follow-up), but is worth tracking. ### I2 -- [SPEC] `automation_profile` default `"balanced"` deviates from spec DDL **File:** `models.py:635-637` **Category:** Spec compliance The spec DDL declares `automation_profile_name TEXT NOT NULL` with no `DEFAULT` clause, implying the value is always explicitly provided. The code adds `default="balanced", server_default="balanced"`, allowing plans to be created without specifying a profile. This is a pragmatic backward-compatibility choice that is documented in the commit message. --- ## Findings Matrix | ID | Severity | Category | File | Line(s) | |----|----------|----------|------|---------| | H1 | HIGH | Bug | `benchmarks/plan_phase_migration_bench.py` | 88, 138, 159 | | M1 | MEDIUM | Schema | `alembic/versions/m8_001_align_plans_schema.py` | 73 | | M2 | MEDIUM | Test Coverage | `features/plans_table_schema_alignment.feature` | -- | | M3 | MEDIUM | Test Coverage | `features/plans_table_schema_alignment.feature` | 39 | | L1 | LOW | Code Quality | `plan.py` | 287 | | L2 | LOW | Code Quality | `models.py` | 977-978 | | L3 | LOW | Test Coverage | `robot/helper_plans_table_schema_alignment.py` | -- | | L4 | LOW | Test Coverage | `features/plans_table_schema_alignment.feature` | -- | | L5 | LOW | Migration | `m8_001_align_plans_schema.py` | `downgrade()` | | L6 | LOW | Code Quality | `models.py` + `repositories.py` | 938/1339 | | I1 | INFO | Spec | `plan.py` + `models.py` | 712/638 | | I2 | INFO | Spec | `models.py` | 635 | --- **Recommendation:** Address H1 (benchmark breakage) before merge. M1-M3 are worth considering. L and I items can be tracked separately.

CoreRasurae force-pushed fix/plans-table-schema-alignment from 4a38810ee9 to dd42c9d6bd 2026-03-30 14:09:49 +00:00 Compare

CoreRasurae commented 2026-03-30 14:20:55 +00:00

Author

Member

Copy link

Code Review Report — PR #1074 (fix/plans-table-schema-alignment)

Reviewer: Automated review (requested by author)
Scope: Branch fix/plans-table-schema-alignment vs master — commit bab48746
Reference: Issue #921, spec DDL (lines 45462-45495 in docs/specification.md)
Review cycles: 3 global passes (cycle 3 produced no new findings)

---

Summary

The PR correctly addresses the core acceptance criteria of issue #921: adds effective_profile_snapshot, makes root_plan_id NOT NULL with self-reference for root plans, makes automation_profile NOT NULL with default, documents the phase default deviation, and creates an Alembic migration. The test modifications to existing step files are consistent and thorough.

However, the review identified 14 findings (2 High, 6 Medium, 6 Low) across 5 categories that should be addressed before merge.

---

Findings by Severity

HIGH (2)

B1: Migration leaves contradictory FK constraint on root_plan_id

File: alembic/versions/m8_001_align_plans_schema.py (lines 41-47)

The model correctly changes from ForeignKey("v3_plans.plan_id", ondelete="SET NULL") to ForeignKey("v3_plans.plan_id") (no ondelete). However, the migration only does:

batch_op.alter_column("root_plan_id", existing_type=sa.String(26), nullable=False)

This changes nullability but does not update the FK constraint. In SQLite batch_alter_table, the existing ON DELETE SET NULL behavior is preserved from the original schema. Result: migrated databases will have ON DELETE SET NULL on a NOT NULL column — a contradictory combination. If a referenced plan is ever deleted, the FK tries to SET NULL, which violates NOT NULL, causing an IntegrityError.

Fresh databases (from Base.metadata.create_all() in tests) use the model's FK definition (no ondelete), so tests pass. Only migrated production databases are affected.

Recommendation: The batch_alter_table block needs to explicitly recreate the FK constraint without ondelete, or the migration should use batch_op.drop_constraint() + batch_op.create_foreign_key() to replace the FK.

---

B2: to_domain() silently corrupts non-JSON automation_profile values on round-trip

File: src/cleveragents/infrastructure/database/models.py — to_domain()

if raw_ap and raw_ap != "balanced":
    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):
        automation_profile_ref = None

If a non-JSON, non-"balanced" string exists in the column (e.g., a bare profile name like "manual" from direct DB access or a future code path), json.loads() raises JSONDecodeError, the except block sets automation_profile_ref = None, and the next update() serializes None as "balanced" — silently changing the stored automation profile. This is a data mutation bug that is invisible to operators.

Recommendation: At minimum, log a warning in the except block (see also S1). Better: if json.loads fails, attempt to construct an AutomationProfileRef from the bare string (e.g., AutomationProfileRef(profile_name=raw_ap, provenance=AutomationProfileProvenance.GLOBAL)), preserving the value.

---

MEDIUM (6)

B3: Migration partial failure risk from dual batch_alter_table calls

File: alembic/versions/m8_001_align_plans_schema.py (lines 41-58)

The upgrade() function uses two separate with op.batch_alter_table("v3_plans") blocks — one for root_plan_id, one for automation_profile. In SQLite, each block recreates the entire table (temp table, copy data, drop, rename). Issues:

1. If the second block fails, the migration is partially applied.
2. Two full table copies instead of one.

Recommendation: Combine both alter_column calls into a single batch_alter_table block.

---

B4: Semantic ambiguity in automation_profile None-vs-"balanced" round-trip

Files: models.py (to_domain() + from_domain()), repositories.py (update())

The serialization cycle creates an information-losing round-trip:

• from_domain(): plan.automation_profile == None → DB stores "balanced" (bare string)
• to_domain(): DB value "balanced" → plan.automation_profile = None

This means automation_profile=None in the domain model is ambiguous — it could mean "no profile was set" or "the balanced profile was explicitly assigned." Any downstream code checking if plan.automation_profile: cannot distinguish the two cases.

Recommendation: Consider representing the default as a proper AutomationProfileRef(profile_name="balanced", provenance=GLOBAL) in the domain model instead of None, or store the JSON form in the DB column for "balanced" as well.

---

S1: Silent exception swallowing in to_domain() masks data corruption

File: src/cleveragents/infrastructure/database/models.py — to_domain()

The except (json.JSONDecodeError, KeyError) block sets automation_profile_ref = None without logging. If malformed data enters the automation_profile column (from migration edge cases, direct DB access, or future bugs), the error is completely invisible.

Recommendation: Add _logger.warning(...) inside the except block with the raw value and plan_id for diagnostic traceability.

---

C1: Column name deviations not documented per acceptance criteria

Reference: Issue #921 AC: "Column name mismatches evaluated — either renamed to match spec or documented as intentional conventions"

The PR documents only the phase default deviation (inline comment in models.py). The following column name deviations between spec DDL and code are not documented:

Spec DDL	Code
state	processing_state
automation_profile_name	automation_profile
strategy_actor_name (and other *_actor_name)	strategy_actor (and *_actor)
error_traceback	error_details_json
Table plans	Table v3_plans

Recommendation: Add an inline comment block in LifecyclePlanModel documenting each intentional deviation, or add a section in the PR body / commit message acknowledging these were evaluated and intentionally kept.

---

T1: No test coverage for update() path with new fields

Files: repositories.py (update() method)

The BDD and Robot tests exercise create → get flows. The update() method in LifecyclePlanRepository was also modified (automation_profile serialization to "balanced", effective_profile_snapshot assignment), but no test scenario updates a plan and verifies the new fields persist correctly after update.

Recommendation: Add a BDD scenario that creates a plan, updates it (e.g., changes phase), and verifies effective_profile_snapshot and automation_profile are preserved.

---

T2: No test for actual AutomationProfileRef round-trip after changes

Files: features/steps/plans_table_schema_alignment_steps.py, robot/helper_plans_table_schema_alignment.py

All tests use automation_profile=None. No test creates a plan with an actual AutomationProfileRef object and verifies it roundtrips correctly through from_domain() → DB → to_domain() after the new special-casing of "balanced" and the try/except block in to_domain().

Recommendation: Add a scenario that creates a plan with AutomationProfileRef(profile_name="supervised", provenance="plan") and verifies the deserialized profile matches.

---

LOW (6)

C2: effective_profile_snapshot default could mask missing snapshots

File: src/cleveragents/domain/models/core/plan.py, src/cleveragents/infrastructure/database/models.py

The spec says effective_profile_snapshot TEXT NOT NULL with no DEFAULT. The code adds default="{}" and server_default="{}". This allows plans to be persisted without an explicit snapshot, potentially masking bugs where callers forget to capture the active profile at creation time. The empty "{}" defeats the purpose of the "frozen snapshot" concept.

---

T3: No negative test for root_plan_id NOT NULL constraint enforcement

No test verifies that attempting to insert a plan with root_plan_id=NULL is rejected by the database. This is the core constraint change from the issue.

---

T4: BDD steps and Robot helpers duplicate identical test logic

features/steps/plans_table_schema_alignment_steps.py and robot/helper_plans_table_schema_alignment.py contain duplicated _make_plan(), DB setup, and assertion logic. Bug fixes need to be applied in both places, increasing maintenance burden.

---

T5: Hardcoded non-ULID test IDs

IDs like "01HV000000000000000000SA01" don't follow ULID format (26 Crockford Base32 characters with proper timestamp+randomness). While functional for isolated in-memory tests, they could cause subtle issues if tests are extended to interact with ULID-aware code or libraries.

---

Q1: Unnecessary getattr for effective_profile_snapshot

Files: models.py (from_domain()), repositories.py (update())

effective_profile_snapshot=getattr(plan, "effective_profile_snapshot", "{}")

Since effective_profile_snapshot is now a defined field on Plan with a default value, direct attribute access (plan.effective_profile_snapshot) is correct. getattr with a fallback masks potential type errors and is inconsistent with how other Plan fields are accessed in the same method.

---

Q2: update() allows overwriting frozen effective_profile_snapshot

File: src/cleveragents/infrastructure/database/repositories.py

The effective_profile_snapshot is described as a "frozen JSON snapshot of the automation profile at plan creation time" — it should be immutable after creation. However, the update() method overwrites it on every save with whatever is in the domain model. If any code path reconstructs a Plan object without preserving the snapshot, the frozen value could be lost.

---

Metrics Summary

Severity	Count	Categories
HIGH	2	Bug/Data Integrity
MEDIUM	6	Bug/Design, Security/Reliability, Spec Compliance, Test Coverage
LOW	6	Spec Compliance, Test Flaws, Code Quality, Performance
Total	14

---

Verdict

The two HIGH findings (B1: FK constraint contradiction in migration, B2: silent data corruption path) should be addressed before merge. The MEDIUM findings are strongly recommended for this PR. LOW findings can be deferred to follow-up work at the team's discretion.

# Code Review Report — PR #1074 (`fix/plans-table-schema-alignment`) **Reviewer:** Automated review (requested by author) **Scope:** Branch `fix/plans-table-schema-alignment` vs `master` — commit `bab48746` **Reference:** Issue #921, spec DDL (lines 45462-45495 in `docs/specification.md`) **Review cycles:** 3 global passes (cycle 3 produced no new findings) --- ## Summary The PR correctly addresses the core acceptance criteria of issue #921: adds `effective_profile_snapshot`, makes `root_plan_id` NOT NULL with self-reference for root plans, makes `automation_profile` NOT NULL with default, documents the `phase` default deviation, and creates an Alembic migration. The test modifications to existing step files are consistent and thorough. However, the review identified **14 findings** (2 High, 6 Medium, 6 Low) across 5 categories that should be addressed before merge. --- ## Findings by Severity ### HIGH (2) #### B1: Migration leaves contradictory FK constraint on `root_plan_id` **File:** `alembic/versions/m8_001_align_plans_schema.py` (lines 41-47) The model correctly changes from `ForeignKey("v3_plans.plan_id", ondelete="SET NULL")` to `ForeignKey("v3_plans.plan_id")` (no `ondelete`). However, the migration only does: ```python batch_op.alter_column("root_plan_id", existing_type=sa.String(26), nullable=False) ``` This changes nullability but does **not** update the FK constraint. In SQLite `batch_alter_table`, the existing `ON DELETE SET NULL` behavior is preserved from the original schema. Result: **migrated databases will have `ON DELETE SET NULL` on a `NOT NULL` column** — a contradictory combination. If a referenced plan is ever deleted, the FK tries to `SET NULL`, which violates `NOT NULL`, causing an `IntegrityError`. Fresh databases (from `Base.metadata.create_all()` in tests) use the model's FK definition (no `ondelete`), so tests pass. Only migrated production databases are affected. **Recommendation:** The `batch_alter_table` block needs to explicitly recreate the FK constraint without `ondelete`, or the migration should use `batch_op.drop_constraint()` + `batch_op.create_foreign_key()` to replace the FK. --- #### B2: `to_domain()` silently corrupts non-JSON `automation_profile` values on round-trip **File:** `src/cleveragents/infrastructure/database/models.py` — `to_domain()` ```python if raw_ap and raw_ap != "balanced": 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): automation_profile_ref = None ``` If a non-JSON, non-`"balanced"` string exists in the column (e.g., a bare profile name like `"manual"` from direct DB access or a future code path), `json.loads()` raises `JSONDecodeError`, the except block sets `automation_profile_ref = None`, and the next `update()` serializes `None` as `"balanced"` — **silently changing the stored automation profile**. This is a data mutation bug that is invisible to operators. **Recommendation:** At minimum, log a warning in the except block (see also S1). Better: if `json.loads` fails, attempt to construct an `AutomationProfileRef` from the bare string (e.g., `AutomationProfileRef(profile_name=raw_ap, provenance=AutomationProfileProvenance.GLOBAL)`), preserving the value. --- ### MEDIUM (6) #### B3: Migration partial failure risk from dual `batch_alter_table` calls **File:** `alembic/versions/m8_001_align_plans_schema.py` (lines 41-58) The `upgrade()` function uses two separate `with op.batch_alter_table("v3_plans")` blocks — one for `root_plan_id`, one for `automation_profile`. In SQLite, each block **recreates the entire table** (temp table, copy data, drop, rename). Issues: 1. If the second block fails, the migration is partially applied. 2. Two full table copies instead of one. **Recommendation:** Combine both `alter_column` calls into a single `batch_alter_table` block. --- #### B4: Semantic ambiguity in `automation_profile` None-vs-"balanced" round-trip **Files:** `models.py` (`to_domain()` + `from_domain()`), `repositories.py` (`update()`) The serialization cycle creates an information-losing round-trip: - `from_domain()`: `plan.automation_profile == None` → DB stores `"balanced"` (bare string) - `to_domain()`: DB value `"balanced"` → `plan.automation_profile = None` This means `automation_profile=None` in the domain model is **ambiguous** — it could mean "no profile was set" or "the balanced profile was explicitly assigned." Any downstream code checking `if plan.automation_profile:` cannot distinguish the two cases. **Recommendation:** Consider representing the default as a proper `AutomationProfileRef(profile_name="balanced", provenance=GLOBAL)` in the domain model instead of `None`, or store the JSON form in the DB column for "balanced" as well. --- #### S1: Silent exception swallowing in `to_domain()` masks data corruption **File:** `src/cleveragents/infrastructure/database/models.py` — `to_domain()` The `except (json.JSONDecodeError, KeyError)` block sets `automation_profile_ref = None` **without logging**. If malformed data enters the `automation_profile` column (from migration edge cases, direct DB access, or future bugs), the error is completely invisible. **Recommendation:** Add `_logger.warning(...)` inside the except block with the raw value and plan_id for diagnostic traceability. --- #### C1: Column name deviations not documented per acceptance criteria **Reference:** Issue #921 AC: *"Column name mismatches evaluated — either renamed to match spec or documented as intentional conventions"* The PR documents only the `phase` default deviation (inline comment in `models.py`). The following column name deviations between spec DDL and code are **not documented**: | Spec DDL | Code | |---|---| | `state` | `processing_state` | | `automation_profile_name` | `automation_profile` | | `strategy_actor_name` (and other `*_actor_name`) | `strategy_actor` (and `*_actor`) | | `error_traceback` | `error_details_json` | | Table `plans` | Table `v3_plans` | **Recommendation:** Add an inline comment block in `LifecyclePlanModel` documenting each intentional deviation, or add a section in the PR body / commit message acknowledging these were evaluated and intentionally kept. --- #### T1: No test coverage for `update()` path with new fields **Files:** `repositories.py` (`update()` method) The BDD and Robot tests exercise `create → get` flows. The `update()` method in `LifecyclePlanRepository` was also modified (automation_profile serialization to `"balanced"`, `effective_profile_snapshot` assignment), but **no test scenario updates a plan and verifies the new fields persist correctly after update**. **Recommendation:** Add a BDD scenario that creates a plan, updates it (e.g., changes phase), and verifies `effective_profile_snapshot` and `automation_profile` are preserved. --- #### T2: No test for actual `AutomationProfileRef` round-trip after changes **Files:** `features/steps/plans_table_schema_alignment_steps.py`, `robot/helper_plans_table_schema_alignment.py` All tests use `automation_profile=None`. No test creates a plan with an actual `AutomationProfileRef` object and verifies it roundtrips correctly through `from_domain() → DB → to_domain()` after the new special-casing of `"balanced"` and the `try/except` block in `to_domain()`. **Recommendation:** Add a scenario that creates a plan with `AutomationProfileRef(profile_name="supervised", provenance="plan")` and verifies the deserialized profile matches. --- ### LOW (6) #### C2: `effective_profile_snapshot` default could mask missing snapshots **File:** `src/cleveragents/domain/models/core/plan.py`, `src/cleveragents/infrastructure/database/models.py` The spec says `effective_profile_snapshot TEXT NOT NULL` with **no DEFAULT**. The code adds `default="{}"` and `server_default="{}"`. This allows plans to be persisted without an explicit snapshot, potentially masking bugs where callers forget to capture the active profile at creation time. The empty `"{}"` defeats the purpose of the "frozen snapshot" concept. --- #### T3: No negative test for `root_plan_id` NOT NULL constraint enforcement No test verifies that attempting to insert a plan with `root_plan_id=NULL` is rejected by the database. This is the core constraint change from the issue. --- #### T4: BDD steps and Robot helpers duplicate identical test logic `features/steps/plans_table_schema_alignment_steps.py` and `robot/helper_plans_table_schema_alignment.py` contain duplicated `_make_plan()`, DB setup, and assertion logic. Bug fixes need to be applied in both places, increasing maintenance burden. --- #### T5: Hardcoded non-ULID test IDs IDs like `"01HV000000000000000000SA01"` don't follow ULID format (26 Crockford Base32 characters with proper timestamp+randomness). While functional for isolated in-memory tests, they could cause subtle issues if tests are extended to interact with ULID-aware code or libraries. --- #### Q1: Unnecessary `getattr` for `effective_profile_snapshot` **Files:** `models.py` (`from_domain()`), `repositories.py` (`update()`) ```python effective_profile_snapshot=getattr(plan, "effective_profile_snapshot", "{}") ``` Since `effective_profile_snapshot` is now a defined field on `Plan` with a default value, direct attribute access (`plan.effective_profile_snapshot`) is correct. `getattr` with a fallback masks potential type errors and is inconsistent with how other Plan fields are accessed in the same method. --- #### Q2: `update()` allows overwriting frozen `effective_profile_snapshot` **File:** `src/cleveragents/infrastructure/database/repositories.py` The `effective_profile_snapshot` is described as a "frozen JSON snapshot of the automation profile at plan creation time" — it should be immutable after creation. However, the `update()` method overwrites it on every save with whatever is in the domain model. If any code path reconstructs a `Plan` object without preserving the snapshot, the frozen value could be lost. --- ## Metrics Summary | Severity | Count | Categories | |----------|-------|------------| | **HIGH** | 2 | Bug/Data Integrity | | **MEDIUM** | 6 | Bug/Design, Security/Reliability, Spec Compliance, Test Coverage | | **LOW** | 6 | Spec Compliance, Test Flaws, Code Quality, Performance | | **Total** | **14** | | --- ## Verdict The two HIGH findings (B1: FK constraint contradiction in migration, B2: silent data corruption path) should be addressed before merge. The MEDIUM findings are strongly recommended for this PR. LOW findings can be deferred to follow-up work at the team's discretion.

CoreRasurae force-pushed fix/plans-table-schema-alignment from dd42c9d6bd to 08b225865c 2026-03-30 14:58:39 +00:00 Compare

CoreRasurae force-pushed fix/plans-table-schema-alignment from 08b225865c to bbe639d417 2026-03-30 15:11:55 +00:00 Compare

CoreRasurae reviewed 2026-03-30 15:31:34 +00:00

CoreRasurae left a comment

Copy link

Code Review Report — PR #1074 (fix/plans-table-schema-alignment)

Issue: #921 | Branch: fix/plans-table-schema-alignment | Commit: bbe639d
Reviewer: Automated multi-pass review (3 cycles) covering bug detection, security, performance, test coverage, and spec compliance.
Scope: Strictly the code changes in this branch plus close connections to surrounding code.

---

Summary

The PR correctly addresses the core issue #921 acceptance criteria: effective_profile_snapshot column added, root_plan_id made NOT NULL with self-referencing, automation_profile made NOT NULL with default, phase deviation documented, column name mismatches documented, and Alembic migration created. The test coverage is solid for happy paths (13 BDD scenarios, 7 Robot tests). However, the review identified 1 critical bug in the migration backfill logic that can corrupt data on 3+ level plan hierarchies, plus several high/medium issues that should be addressed before merge.

Findings: 18 code issues + 13 test coverage gaps across all severity levels.

---

Critical

C1. [Bug] Migration backfill COALESCE corrupts root_plan_id for 3+ level hierarchies

File: alembic/versions/m8_001_align_plans_schema.py:50-60

The iterative while-loop exists to handle SQL pre-statement snapshot semantics (correctly documented in the comment on lines 41-44). However, COALESCE(p.root_plan_id, p.plan_id) defeats the loop by always producing a non-NULL value — even when the parent hasn't been backfilled yet.

Trace for Root → Mid → Leaf:

1. Before loop: Root gets root_plan_id = plan_id ✓
2. Loop iteration 1: Single UPDATE targets both Mid and Leaf (both have root_plan_id IS NULL):
   • Mid: parent=Root (already resolved) → COALESCE(Root, Root.plan_id) = Root ✓
   • Leaf: parent=Mid → under snapshot semantics, Mid.root_plan_id is still NULL → COALESCE(NULL, Mid.plan_id) = Mid ✗ (should be Root)
3. Loop iteration 2: Both are non-NULL → 0 rows → loop exits.
4. Fallback (line 62): No NULLs remain → no-op.

Result: Leaf.root_plan_id = Mid instead of Root. Every hierarchy deeper than 2 levels gets corrupted root pointers. On SQLite the behavior is additionally non-deterministic (undefined row evaluation order).

Fix — replace COALESCE with parent-readiness guard:

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 plan_id FROM v3_plans WHERE root_plan_id IS NOT NULL
  )

This guarantees level-by-level propagation: iteration 1 backfills depth-1, iteration 2 backfills depth-2, etc.

---

High

H1. [Bug] Missing explicit ondelete on root_plan_id FK

File: src/cleveragents/infrastructure/database/models.py:596-600

root_plan_id = Column(
    String(26),
    ForeignKey("v3_plans.plan_id"),  # no ondelete
    nullable=False,
)

Every other FK in the model specifies an explicit ondelete (parent_plan_id → SET NULL, action_name → RESTRICT). The implicit default is NO ACTION. With the column now NOT NULL, deleting a root plan that has children referencing it will raise IntegrityError — a behavioral regression from the previous ondelete="SET NULL" + nullable=True. Consider ondelete="RESTRICT" to make the intent explicit and consistent with the action_name FK.

H2. [Security / Data Integrity] No JSON validation on effective_profile_snapshot before storage

Files: models.py:985 (from_domain()), repositories.py:1345 (update())

The field is documented as "Frozen JSON snapshot" with default "{}", but it's stored as an opaque str with no validation. Neither from_domain(), update(), nor the Pydantic Plan model validates it is well-formed JSON. Any caller can persist "not json" or "", which will silently pass the NOT NULL constraint but may crash downstream consumers that call json.loads() on it.

Suggested fix: Add a Pydantic field_validator on the domain model:

@field_validator("effective_profile_snapshot")
@classmethod
def _validate_snapshot_json(cls, v: str) -> str:
    json.loads(v)  # raises ValueError if not valid JSON
    return v

H3. [Bug] processing_state=None not guarded in update() (pre-existing, in modified method)

File: repositories.py:1323-1327

from_domain() guards against processing_state=None with a fallback to "queued". The update() method has no such guard: str(None) produces the literal string "None", which violates the CHECK constraint (ck_v3_plans_state). Since update() was modified in this PR (lines 1338-1345), this is in scope.

H4. [Bug] from_domain() silently drops 3 nullable fields (pre-existing, in modified method)

File: models.py:964-1014

from_domain() never sets sandbox_refs_json, validation_summary_json, or decision_root_id. These are nullable so the INSERT succeeds, but if a Plan domain object is created with non-empty values (e.g. sandbox_refs=["sandbox-01"]), they are silently lost on initial save. The update() method (repositories.py:1354-1362) correctly persists them. The PR added effective_profile_snapshot to from_domain() but didn't audit for this pre-existing gap.

---

Medium

M1. [Bug] Domain type allows None for root_plan_id despite DB NOT NULL

File: models.py:861 / plan.py:~287

PlanIdentity.root_plan_id is typed str | None, but the column is now NOT NULL. The from_domain() resolver correctly handles this (lines 955-958), but if domain code sets root_plan_id = None on a child plan, the resolver silently changes it to plan_id — potentially breaking the hierarchy.

M2. [Bug] Migration backfill doesn't handle empty-string automation_profile

File: m8_001_align_plans_schema.py:65-68

WHERE automation_profile IS NULL only backfills NULL values. Rows with automation_profile = '' (empty string) pass the NOT NULL constraint but are semantically invalid. Add OR automation_profile = '' to the WHERE clause.

M3. [Bug] Falsy-value coercion in to_domain() for effective_profile_snapshot

File: models.py:871-872

effective_profile_snapshot=cast(str, self.effective_profile_snapshot or "{}")

The or "{}" silently coerces empty string to "{}" on read but leaves the DB data as "". A subsequent update() writes "{}" back, creating a read-write inconsistency.

M4. [Performance] Iterative backfill is O(D) round-trips; a recursive CTE would be O(1)

File: m8_001_align_plans_schema.py:50-59

The while-loop propagates one hierarchy level per iteration (D iterations for depth D). A single recursive CTE materializes the full ancestor mapping before the UPDATE, completing in one statement. SQLite supports recursive CTEs since 3.8.3 (2014). This also resolves the extended write-lock issue (Low severity) under SQLite.

M5. [Security] Migration loop has no iteration bound

File: m8_001_align_plans_schema.py:50-60

If the data contains a cycle in parent_plan_id (pre-existing corruption), each loop iteration updates rows but never converges, causing an infinite loop that hangs the deployment. Add a safety bound (e.g. for _ in range(100)) with a logged error on exhaustion.

M6. [Spec Compliance] "balanced" server_default creates semantic conflation not in spec

File: models.py:638-640

The spec DDL defines automation_profile_name TEXT NOT NULL with no default. The code adds server_default="balanced", creating a hidden round-trip: domain None → DB "balanced" → domain None. The DB never stores a true "no profile" state — it's conflated with "balanced". If a future profile replaces "balanced" as the default, this assumption breaks silently.

---

Low

L1. [Security] Raw column value logged without truncation

File: models.py:826-830

_logger.warning("Unparseable automation_profile for plan %s: %r", self.plan_id, raw_ap) — the full raw value is logged. If the column ever contains sensitive data, it would be written to logs. Consider logging only the length or a truncated prefix.

L2. [Security] No size bound on effective_profile_snapshot

Files: models.py:641-643, plan.py:712-713

The column is unbounded Text and the domain field is an unbounded str. An arbitrarily large payload would be persisted and loaded into memory. Consider a max_length validator or CheckConstraint.

L3. [Security] RecursionError not caught in automation_profile deserialization

File: models.py:823

json.loads() on a deeply nested JSON payload can cause a RecursionError (stack overflow). The except clause catches JSONDecodeError, KeyError, ValueError, ValidationError but not RecursionError.

L4. [Bug] Downgrade doesn't restore original NULL root_plan_id values

File: m8_001_align_plans_schema.py:84-99

The downgrade makes columns nullable again but doesn't restore original NULL values. Plans that originally had root_plan_id = NULL now have it set to their resolved root. This is standard for data migrations but should be documented in the migration docstring.

L5. [Bug] completed_at is a write-only / orphaned column (pre-existing)

File: models.py:1003

Both from_domain() and update() write completed_at from plan.timestamps.applied_at, but to_domain() never reads completed_at back — only applied_at. This is dead data.

L6. [Code Quality] Remaining getattr() calls for fields defined on Plan model (pre-existing)

File: models.py:980-983

The commit message (item 10) says "Replaced defensive getattr() with direct attribute access for effective_profile_snapshot". However, 10+ other getattr() calls remain in from_domain() for fields that are explicitly defined on the Plan model with defaults (review_actor, apply_actor, estimation_actor, invariant_actor, error_details, changeset_id, etc.). These mask AttributeError bugs when an incorrect object type is passed.

L7. [Performance] Double-serialization: model_dump() + json.dumps() (pre-existing)

File: models.py:948, repositories.py:1340

json.dumps(plan.automation_profile.model_dump(mode="json")) does two passes. Pydantic v2's model_dump_json() does it in one Rust-accelerated pass. Low impact (2-field object) but inconsistent with Pydantic best practices.

---

Test Coverage Gaps

TC1. [Critical] Migration upgrade() / downgrade() paths completely untested

File: m8_001_align_plans_schema.py

The migration contains non-trivial logic (iterative backfill loop, fallback, batch ALTER TABLE, downgrade column reversal). None of it has dedicated tests. A migration-specific test should seed pre-existing plans with NULL root_plan_id at various hierarchy depths and verify the backfill.

TC2. [Critical] Iterative backfill for 3+ level hierarchy untested

File: m8_001_align_plans_schema.py:50-60

No test seeds a 3-level hierarchy (Root → Mid → Leaf) with NULL root_plan_id values and verifies all descendants get the root ancestor's ID. This is where bug C1 manifests.

TC3. [High] Robot tests missing 6 of 13 BDD scenarios

File: robot/plans_table_schema_alignment.robot

Missing Robot equivalents for: default effective_profile_snapshot, bare-string profile deserialization, invalid provenance deserialization, truncated JSON deserialization, snapshot update, and None automation_profile round-trip.

TC4. [High] from_domain() self-ref resolution not directly tested

File: features/steps/plans_table_schema_alignment_steps.py:127-129

The BDD scenario "Root plan has self-referencing root_plan_id" tests through LifecyclePlanRepository.create() + .get(). No test directly calls LifecyclePlanModel.from_domain(plan) and asserts model.root_plan_id == plan.identity.plan_id.

TC5. [High] No negative test for NULL effective_profile_snapshot constraint

File: features/plans_table_schema_alignment.feature

There is a test for NULL root_plan_id rejection (line 97) but no equivalent for effective_profile_snapshot. A raw SQL INSERT with NULL should be rejected.

TC6. [Medium] Empty-string effective_profile_snapshot behavior untested

No test verifies what happens when effective_profile_snapshot = "". The to_domain() fallback coerces it to "{}".

TC7. [Medium] Valid JSON missing profile_name key untested

The deserialization catches KeyError, but no test covers valid JSON missing the required profile_name key (e.g. '{"provenance":"plan"}').

TC8. [Medium] No explicit session/engine cleanup in BDD steps

File: features/steps/plans_table_schema_alignment_steps.py:75-89

The in-memory SQLite engine and session are attached to context._sa_* but never explicitly .close()d or .dispose()d. Relies on GC, which is fragile under parallelization.

TC9. [Medium] Constraint violation test only catches IntegrityError

Files: plans_table_schema_alignment_steps.py:345, helper_plans_table_schema_alignment.py:269

Some SQLite versions raise OperationalError for NOT NULL violations instead of IntegrityError. Catching only IntegrityError may cause false test failures on those versions.

TC10. [Medium] _make_plan_model helper doesn't accept automation_profile parameter

File: features/steps/database_models_lifecycle_coverage_steps.py:105-133

The helper can't construct plans with custom automation_profile values, limiting its reusability for profile-related tests.

TC11. [Low] Same-session retrieval may hit SQLAlchemy identity map

File: plans_table_schema_alignment_steps.py:159-164

step_persist_plan commits and immediately calls .get() in the same session. The returned object may come from the session cache rather than a DB round-trip. Use session.expire_all() before retrieval for rigorous testing.

TC12. [Low] Hardcoded timestamps reduce test flexibility

Files: plans_table_schema_alignment_steps.py:37, helper_plans_table_schema_alignment.py:46

Both use _NOW = datetime(2026, 3, 19, tzinfo=UTC). No scenario tests that timestamps are correctly persisted and retrieved.

TC13. [Low] No raw column format assertion for serialized AutomationProfileRef

File: plans_table_schema_alignment_steps.py:360-370

The round-trip scenario verifies the domain object but doesn't inspect the raw column value to verify the JSON structure.

---

Summary Table

Severity	Count	Issues
Critical	1 + 2 test	C1, TC1, TC2
High	4 + 3 test	H1-H4, TC3-TC5
Medium	6 + 5 test	M1-M6, TC6-TC10
Low	7 + 3 test	L1-L7, TC11-TC13
Total	18 + 13 test	31 findings

Items marked "(pre-existing)" are not regressions introduced by this PR but are in the immediate vicinity of the changed code and worth addressing opportunistically.

# Code Review Report — PR #1074 (`fix/plans-table-schema-alignment`) **Issue:** #921 | **Branch:** `fix/plans-table-schema-alignment` | **Commit:** `bbe639d` **Reviewer:** Automated multi-pass review (3 cycles) covering bug detection, security, performance, test coverage, and spec compliance. **Scope:** Strictly the code changes in this branch plus close connections to surrounding code. --- ## Summary The PR correctly addresses the core issue #921 acceptance criteria: `effective_profile_snapshot` column added, `root_plan_id` made NOT NULL with self-referencing, `automation_profile` made NOT NULL with default, phase deviation documented, column name mismatches documented, and Alembic migration created. The test coverage is solid for happy paths (13 BDD scenarios, 7 Robot tests). However, the review identified **1 critical bug** in the migration backfill logic that can corrupt data on 3+ level plan hierarchies, plus several high/medium issues that should be addressed before merge. **Findings:** 18 code issues + 13 test coverage gaps across all severity levels. --- ## Critical ### C1. [Bug] Migration backfill `COALESCE` corrupts `root_plan_id` for 3+ level hierarchies **File:** `alembic/versions/m8_001_align_plans_schema.py:50-60` The iterative while-loop exists to handle SQL pre-statement snapshot semantics (correctly documented in the comment on lines 41-44). However, `COALESCE(p.root_plan_id, p.plan_id)` defeats the loop by always producing a non-NULL value — even when the parent hasn't been backfilled yet. **Trace for Root → Mid → Leaf:** 1. Before loop: Root gets `root_plan_id = plan_id` ✓ 2. Loop iteration 1: Single UPDATE targets both Mid and Leaf (both have `root_plan_id IS NULL`): - Mid: parent=Root (already resolved) → `COALESCE(Root, Root.plan_id)` = Root ✓ - Leaf: parent=Mid → under snapshot semantics, Mid.root_plan_id is still NULL → `COALESCE(NULL, Mid.plan_id)` = **Mid** ✗ (should be Root) 3. Loop iteration 2: Both are non-NULL → 0 rows → loop exits. 4. Fallback (line 62): No NULLs remain → no-op. **Result:** `Leaf.root_plan_id = Mid` instead of `Root`. Every hierarchy deeper than 2 levels gets corrupted root pointers. On SQLite the behavior is additionally non-deterministic (undefined row evaluation order). **Fix — replace COALESCE with parent-readiness guard:** ```sql 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 plan_id FROM v3_plans WHERE root_plan_id IS NOT NULL ) ``` This guarantees level-by-level propagation: iteration 1 backfills depth-1, iteration 2 backfills depth-2, etc. --- ## High ### H1. [Bug] Missing explicit `ondelete` on `root_plan_id` FK **File:** `src/cleveragents/infrastructure/database/models.py:596-600` ```python root_plan_id = Column( String(26), ForeignKey("v3_plans.plan_id"), # no ondelete nullable=False, ) ``` Every other FK in the model specifies an explicit `ondelete` (`parent_plan_id` → `SET NULL`, `action_name` → `RESTRICT`). The implicit default is `NO ACTION`. With the column now NOT NULL, deleting a root plan that has children referencing it will raise `IntegrityError` — a behavioral regression from the previous `ondelete="SET NULL"` + `nullable=True`. Consider `ondelete="RESTRICT"` to make the intent explicit and consistent with the `action_name` FK. ### H2. [Security / Data Integrity] No JSON validation on `effective_profile_snapshot` before storage **Files:** `models.py:985` (`from_domain()`), `repositories.py:1345` (`update()`) The field is documented as "Frozen JSON snapshot" with default `"{}"`, but it's stored as an opaque `str` with no validation. Neither `from_domain()`, `update()`, nor the Pydantic `Plan` model validates it is well-formed JSON. Any caller can persist `"not json"` or `""`, which will silently pass the NOT NULL constraint but may crash downstream consumers that call `json.loads()` on it. **Suggested fix:** Add a Pydantic `field_validator` on the domain model: ```python @field_validator("effective_profile_snapshot") @classmethod def _validate_snapshot_json(cls, v: str) -> str: json.loads(v) # raises ValueError if not valid JSON return v ``` ### H3. [Bug] `processing_state=None` not guarded in `update()` (pre-existing, in modified method) **File:** `repositories.py:1323-1327` `from_domain()` guards against `processing_state=None` with a fallback to `"queued"`. The `update()` method has no such guard: `str(None)` produces the literal string `"None"`, which violates the CHECK constraint (`ck_v3_plans_state`). Since `update()` was modified in this PR (lines 1338-1345), this is in scope. ### H4. [Bug] `from_domain()` silently drops 3 nullable fields (pre-existing, in modified method) **File:** `models.py:964-1014` `from_domain()` never sets `sandbox_refs_json`, `validation_summary_json`, or `decision_root_id`. These are nullable so the INSERT succeeds, but if a Plan domain object is created with non-empty values (e.g. `sandbox_refs=["sandbox-01"]`), they are silently lost on initial save. The `update()` method (repositories.py:1354-1362) correctly persists them. The PR added `effective_profile_snapshot` to `from_domain()` but didn't audit for this pre-existing gap. --- ## Medium ### M1. [Bug] Domain type allows `None` for `root_plan_id` despite DB NOT NULL **File:** `models.py:861` / `plan.py:~287` `PlanIdentity.root_plan_id` is typed `str | None`, but the column is now NOT NULL. The `from_domain()` resolver correctly handles this (lines 955-958), but if domain code sets `root_plan_id = None` on a child plan, the resolver silently changes it to `plan_id` — potentially breaking the hierarchy. ### M2. [Bug] Migration backfill doesn't handle empty-string `automation_profile` **File:** `m8_001_align_plans_schema.py:65-68` `WHERE automation_profile IS NULL` only backfills NULL values. Rows with `automation_profile = ''` (empty string) pass the NOT NULL constraint but are semantically invalid. Add `OR automation_profile = ''` to the WHERE clause. ### M3. [Bug] Falsy-value coercion in `to_domain()` for `effective_profile_snapshot` **File:** `models.py:871-872` ```python effective_profile_snapshot=cast(str, self.effective_profile_snapshot or "{}") ``` The `or "{}"` silently coerces empty string to `"{}"` on read but leaves the DB data as `""`. A subsequent `update()` writes `"{}"` back, creating a read-write inconsistency. ### M4. [Performance] Iterative backfill is O(D) round-trips; a recursive CTE would be O(1) **File:** `m8_001_align_plans_schema.py:50-59` The while-loop propagates one hierarchy level per iteration (D iterations for depth D). A single recursive CTE materializes the full ancestor mapping before the UPDATE, completing in one statement. SQLite supports recursive CTEs since 3.8.3 (2014). This also resolves the extended write-lock issue (Low severity) under SQLite. ### M5. [Security] Migration loop has no iteration bound **File:** `m8_001_align_plans_schema.py:50-60` If the data contains a cycle in `parent_plan_id` (pre-existing corruption), each loop iteration updates rows but never converges, causing an infinite loop that hangs the deployment. Add a safety bound (e.g. `for _ in range(100)`) with a logged error on exhaustion. ### M6. [Spec Compliance] `"balanced"` server_default creates semantic conflation not in spec **File:** `models.py:638-640` The spec DDL defines `automation_profile_name TEXT NOT NULL` with **no default**. The code adds `server_default="balanced"`, creating a hidden round-trip: domain `None` → DB `"balanced"` → domain `None`. The DB never stores a true "no profile" state — it's conflated with "balanced". If a future profile replaces "balanced" as the default, this assumption breaks silently. --- ## Low ### L1. [Security] Raw column value logged without truncation **File:** `models.py:826-830` `_logger.warning("Unparseable automation_profile for plan %s: %r", self.plan_id, raw_ap)` — the full raw value is logged. If the column ever contains sensitive data, it would be written to logs. Consider logging only the length or a truncated prefix. ### L2. [Security] No size bound on `effective_profile_snapshot` **Files:** `models.py:641-643`, `plan.py:712-713` The column is unbounded `Text` and the domain field is an unbounded `str`. An arbitrarily large payload would be persisted and loaded into memory. Consider a `max_length` validator or `CheckConstraint`. ### L3. [Security] `RecursionError` not caught in `automation_profile` deserialization **File:** `models.py:823` `json.loads()` on a deeply nested JSON payload can cause a `RecursionError` (stack overflow). The `except` clause catches `JSONDecodeError, KeyError, ValueError, ValidationError` but not `RecursionError`. ### L4. [Bug] Downgrade doesn't restore original NULL `root_plan_id` values **File:** `m8_001_align_plans_schema.py:84-99` The downgrade makes columns nullable again but doesn't restore original NULL values. Plans that originally had `root_plan_id = NULL` now have it set to their resolved root. This is standard for data migrations but should be documented in the migration docstring. ### L5. [Bug] `completed_at` is a write-only / orphaned column (pre-existing) **File:** `models.py:1003` Both `from_domain()` and `update()` write `completed_at` from `plan.timestamps.applied_at`, but `to_domain()` never reads `completed_at` back — only `applied_at`. This is dead data. ### L6. [Code Quality] Remaining `getattr()` calls for fields defined on Plan model (pre-existing) **File:** `models.py:980-983` The commit message (item 10) says "Replaced defensive `getattr()` with direct attribute access for `effective_profile_snapshot`". However, 10+ other `getattr()` calls remain in `from_domain()` for fields that are explicitly defined on the `Plan` model with defaults (`review_actor`, `apply_actor`, `estimation_actor`, `invariant_actor`, `error_details`, `changeset_id`, etc.). These mask `AttributeError` bugs when an incorrect object type is passed. ### L7. [Performance] Double-serialization: `model_dump()` + `json.dumps()` (pre-existing) **File:** `models.py:948`, `repositories.py:1340` `json.dumps(plan.automation_profile.model_dump(mode="json"))` does two passes. Pydantic v2's `model_dump_json()` does it in one Rust-accelerated pass. Low impact (2-field object) but inconsistent with Pydantic best practices. --- ## Test Coverage Gaps ### TC1. [Critical] Migration `upgrade()` / `downgrade()` paths completely untested **File:** `m8_001_align_plans_schema.py` The migration contains non-trivial logic (iterative backfill loop, fallback, batch ALTER TABLE, downgrade column reversal). None of it has dedicated tests. A migration-specific test should seed pre-existing plans with NULL `root_plan_id` at various hierarchy depths and verify the backfill. ### TC2. [Critical] Iterative backfill for 3+ level hierarchy untested **File:** `m8_001_align_plans_schema.py:50-60` No test seeds a 3-level hierarchy (Root → Mid → Leaf) with NULL `root_plan_id` values and verifies all descendants get the root ancestor's ID. This is where bug C1 manifests. ### TC3. [High] Robot tests missing 6 of 13 BDD scenarios **File:** `robot/plans_table_schema_alignment.robot` Missing Robot equivalents for: default `effective_profile_snapshot`, bare-string profile deserialization, invalid provenance deserialization, truncated JSON deserialization, snapshot update, and `None` automation_profile round-trip. ### TC4. [High] `from_domain()` self-ref resolution not directly tested **File:** `features/steps/plans_table_schema_alignment_steps.py:127-129` The BDD scenario "Root plan has self-referencing root_plan_id" tests through `LifecyclePlanRepository.create()` + `.get()`. No test directly calls `LifecyclePlanModel.from_domain(plan)` and asserts `model.root_plan_id == plan.identity.plan_id`. ### TC5. [High] No negative test for NULL `effective_profile_snapshot` constraint **File:** `features/plans_table_schema_alignment.feature` There is a test for NULL `root_plan_id` rejection (line 97) but no equivalent for `effective_profile_snapshot`. A raw SQL INSERT with NULL should be rejected. ### TC6. [Medium] Empty-string `effective_profile_snapshot` behavior untested No test verifies what happens when `effective_profile_snapshot = ""`. The `to_domain()` fallback coerces it to `"{}"`. ### TC7. [Medium] Valid JSON missing `profile_name` key untested The deserialization catches `KeyError`, but no test covers valid JSON missing the required `profile_name` key (e.g. `'{"provenance":"plan"}'`). ### TC8. [Medium] No explicit session/engine cleanup in BDD steps **File:** `features/steps/plans_table_schema_alignment_steps.py:75-89` The in-memory SQLite engine and session are attached to `context._sa_*` but never explicitly `.close()`d or `.dispose()`d. Relies on GC, which is fragile under parallelization. ### TC9. [Medium] Constraint violation test only catches `IntegrityError` **Files:** `plans_table_schema_alignment_steps.py:345`, `helper_plans_table_schema_alignment.py:269` Some SQLite versions raise `OperationalError` for NOT NULL violations instead of `IntegrityError`. Catching only `IntegrityError` may cause false test failures on those versions. ### TC10. [Medium] `_make_plan_model` helper doesn't accept `automation_profile` parameter **File:** `features/steps/database_models_lifecycle_coverage_steps.py:105-133` The helper can't construct plans with custom `automation_profile` values, limiting its reusability for profile-related tests. ### TC11. [Low] Same-session retrieval may hit SQLAlchemy identity map **File:** `plans_table_schema_alignment_steps.py:159-164` `step_persist_plan` commits and immediately calls `.get()` in the same session. The returned object may come from the session cache rather than a DB round-trip. Use `session.expire_all()` before retrieval for rigorous testing. ### TC12. [Low] Hardcoded timestamps reduce test flexibility **Files:** `plans_table_schema_alignment_steps.py:37`, `helper_plans_table_schema_alignment.py:46` Both use `_NOW = datetime(2026, 3, 19, tzinfo=UTC)`. No scenario tests that timestamps are correctly persisted and retrieved. ### TC13. [Low] No raw column format assertion for serialized `AutomationProfileRef` **File:** `plans_table_schema_alignment_steps.py:360-370` The round-trip scenario verifies the domain object but doesn't inspect the raw column value to verify the JSON structure. --- ## Summary Table | Severity | Count | Issues | |----------|-------|--------| | **Critical** | 1 + 2 test | C1, TC1, TC2 | | **High** | 4 + 3 test | H1-H4, TC3-TC5 | | **Medium** | 6 + 5 test | M1-M6, TC6-TC10 | | **Low** | 7 + 3 test | L1-L7, TC11-TC13 | | **Total** | **18 + 13 test** | **31 findings** | Items marked "(pre-existing)" are not regressions introduced by this PR but are in the immediate vicinity of the changed code and worth addressing opportunistically.

CoreRasurae force-pushed fix/plans-table-schema-alignment from bbe639d417 to 7d52e0fd27 2026-03-30 16:01:16 +00:00 Compare

CoreRasurae reviewed 2026-03-30 16:13:00 +00:00

CoreRasurae left a comment

Copy link

Code Review Report — PR #1074 (fix/plans-table-schema-alignment)

Scope: All code changes on branch fix/plans-table-schema-alignment (commit 7d52e0fd) plus immediate surrounding code.
Reference: Issue #921, docs/specification.md lines 45463-45489 (spec DDL).
Method: 3 full global review cycles across all categories (bugs, security, performance, test coverage, spec compliance, code quality). No tests were executed.

---

Summary

Severity	Count
Critical	0
High	1
Medium	4
Low	7
Total	12

---

HIGH Severity

H1 — Bug/Schema: Migration does not apply ondelete="RESTRICT" FK policy for root_plan_id

Files: models.py:596-600, alembic/versions/m8_001_align_plans_schema.py:90-93

The ORM model changes root_plan_id's FK from ondelete="SET NULL" (previous) to ondelete="RESTRICT" (this commit). However, the migration only does:

batch_op.alter_column("root_plan_id", existing_type=sa.String(26), nullable=False)

This changes nullability but does not update the FK ondelete policy. In SQLite batch mode, batch_alter_table recreates the table from reflected metadata plus specified changes. Since the FK policy change is not specified, the recreated table retains the old SET NULL policy.

Result: A contradictory schema where:

• The FK says ondelete="SET NULL" (try to NULL the column on parent deletion)
• The column says nullable=False (reject NULL values)

Any attempt to delete a plan referenced as a root ancestor would trigger the FK SET NULL action, which immediately violates the NOT NULL constraint, producing a confusing IntegrityError. While data integrity is preserved (deletion is blocked either way), the error semantics are wrong and the ORM model is out of sync with the live database schema.

Suggested fix: Add an explicit FK constraint recreation in the migration's batch_alter_table block, or document this as a known schema drift that will be reconciled in a future migration.

---

MEDIUM Severity

M1 — Bug: Migration orphan fallback silently self-references without logging

File: alembic/versions/m8_001_align_plans_schema.py:82

# Fallback: any remaining NULLs (orphaned children) self-reference.
op.execute("UPDATE v3_plans SET root_plan_id = plan_id WHERE root_plan_id IS NULL")

After the level-by-level propagation loop, this fallback assigns root_plan_id = plan_id to any remaining unresolved rows. This handles orphaned children (whose parent_plan_id references a non-existent row) by silently making them root plans.

The loop exhaustion at _MAX_DEPTH=100 correctly logs an error, but the fallback itself produces no log output about how many rows were affected. If orphaned children exist, this masks a data integrity problem that operators should investigate.

Suggested fix: Execute the fallback as a bind.execute() call and log the result.rowcount at WARNING level if > 0.

---

M2 — Spec Compliance: effective_profile_snapshot default enables empty-snapshot creation

Files: models.py:641-642, plan.py:713-714

The spec DDL (line 45472) defines:

effective_profile_snapshot TEXT NOT NULL,      -- JSON: frozen profile at creation

No DEFAULT is specified, implying the value must be explicitly populated at plan creation time ("frozen profile at creation"). The implementation adds default="{}" / server_default="{}" at both ORM and domain model levels.

This means new plans can be created through code paths that do not explicitly set the snapshot, receiving an empty "{}" instead of the actual profile configuration. This undermines the spec's intended audit/reproducibility guarantee — it becomes impossible to distinguish "no profile was captured" from "an empty profile was intentionally set."

Suggested fix: Consider removing the domain-level default (keep only server_default for migration backfill) and requiring explicit snapshot population at plan creation time, or add a comment documenting why the default is acceptable.

---

M3 — Test Coverage: Migration backfill logic has no direct test

File: alembic/versions/m8_001_align_plans_schema.py:47-82

The level-by-level root ancestor propagation is the most complex algorithm in this commit. It uses a correlated UPDATE with a parent-readiness guard to resolve root_plan_id for child plans at arbitrary hierarchy depth. This logic is not covered by any test.

The BDD scenarios create plans from scratch via the ORM in a fresh in-memory database (where Base.metadata.create_all() creates the table with NOT NULL already applied). They do not exercise the migration's actual upgrade path on a pre-existing database with NULL root_plan_id values.

Impact: A regression in the propagation query (e.g., incorrect join, missing parent-readiness guard) would go undetected.

---

M4 — Test Coverage: Migration cycle detection guard is untested

File: alembic/versions/m8_001_align_plans_schema.py:56-74

The for/else safety bound (_MAX_DEPTH = 100) with _logger.error on exhaustion is a guard against cycles in parent_plan_id. No test verifies that:

1. Cycles are detected (loop exhausts without convergence)
2. The error is logged
3. The fallback self-reference is applied to unresolved rows

Since this guard protects against data corruption in production databases, it should have at least one test demonstrating correct behavior under a cyclic hierarchy.

---

LOW Severity

L1 — Test Coverage: No test for RecursionError catch in to_domain() deserialization

File: models.py:828

The hardened deserialization adds RecursionError to the exception tuple, but no BDD scenario creates a deeply nested JSON value that would trigger this path. While difficult to trigger in practice, the handler was added for a reason and should be verified.

---

L2 — Test Coverage: No test for Pydantic validator rejecting invalid JSON snapshot

File: plan.py:769-779

The validate_effective_profile_snapshot_json field validator raises ValueError for non-JSON input. No BDD scenario tests that constructing a Plan with effective_profile_snapshot="not-json" is rejected. The NULL database constraint is tested, but the Pydantic-level JSON validity check is not.

---

L3 — Test Coverage: No test for empty-string automation_profile backfill

File: alembic/versions/m8_001_align_plans_schema.py:83-86

The migration handles WHERE automation_profile IS NULL OR automation_profile = ''. The NULL case is implicitly covered by existing data patterns, but the empty-string ('') case has no explicit test. If the OR automation_profile = '' clause were accidentally removed, no test would catch it.

---

L4 — Code Quality: Robot and Behave _make_plan() helpers are near-identical duplicates

Files: features/steps/plans_table_schema_alignment_steps.py:46-68, robot/helper_plans_table_schema_alignment.py:78-100

Both define _make_plan() with identical parameters (plan_id, parent_plan_id, root_plan_id, effective_profile_snapshot, automation_profile) and virtually identical bodies constructing a Plan domain object. This violates DRY — any change to plan construction logic must be applied in two places.

Suggested fix: Extract a shared test factory (e.g., in a tests/factories/ module) that both Behave steps and Robot helpers can import.

---

L5 — Code Quality: Redundant None check on NOT NULL effective_profile_snapshot in to_domain()

File: models.py:878-882

effective_profile_snapshot=(
    cast(str, self.effective_profile_snapshot)
    if self.effective_profile_snapshot is not None
    else "{}"
),

Since effective_profile_snapshot is now nullable=False with server_default="{}", the value will never be None in a correctly migrated database. The is not None guard is defensive but will never trigger, adding dead code.

---

L6 — Spec Compliance: automation_profile semantic difference vs spec undocumented

File: models.py:573-583

The docstring correctly documents the column naming deviations (automation_profile vs automation_profile_name). However, it does not document the semantic difference: the spec's automation_profile_name stores a plain profile name string, while the code's automation_profile stores either a bare name ("balanced") or structured JSON with provenance ({"profile_name":"...","provenance":"..."}). This dual-format storage is non-obvious and could confuse future maintainers.

---

L7 — Security: Pydantic validator error message includes full raw value

File: plan.py:776-778

raise ValueError(
    f"effective_profile_snapshot must be valid JSON, got: {v!r}"
) from exc

The error message includes the full raw value via repr(). For normal automation profile snapshots this is harmless, but if an oversized or adversarial input reaches this validator, the error message (and any logs that capture it) could become very large. Consider truncating the displayed value (e.g., v[:200]).

---

Notes — Positive Observations

• Hardened deserialization: The to_domain() exception handling is comprehensive (5 exception types) and the logging correctly avoids leaking raw values (length-only).
• Explicit None check: Using is not None instead of truthiness for root_plan_id resolution is correct — it avoids treating empty string as falsy.
• Batch optimization: Merging batch_alter_table operations to avoid redundant SQLite table copies is a good performance practice.
• Downgrade documentation: The note about irreversible backfill is clear and appropriate.
• BDD coverage breadth: 17 scenarios covering normal paths, edge cases (grandchild hierarchy), constraint enforcement (NULL rejection), and deserialization resilience (bare strings, truncated JSON, missing keys, invalid enums).

## Code Review Report — PR #1074 (`fix/plans-table-schema-alignment`) **Scope**: All code changes on branch `fix/plans-table-schema-alignment` (commit `7d52e0fd`) plus immediate surrounding code. **Reference**: Issue #921, `docs/specification.md` lines 45463-45489 (spec DDL). **Method**: 3 full global review cycles across all categories (bugs, security, performance, test coverage, spec compliance, code quality). No tests were executed. --- ### Summary | Severity | Count | |----------|-------| | Critical | 0 | | High | 1 | | Medium | 4 | | Low | 7 | | **Total** | **12** | --- ## HIGH Severity ### H1 — Bug/Schema: Migration does not apply `ondelete="RESTRICT"` FK policy for `root_plan_id` **Files**: `models.py:596-600`, `alembic/versions/m8_001_align_plans_schema.py:90-93` The ORM model changes `root_plan_id`'s FK from `ondelete="SET NULL"` (previous) to `ondelete="RESTRICT"` (this commit). However, the migration only does: ```python batch_op.alter_column("root_plan_id", existing_type=sa.String(26), nullable=False) ``` This changes nullability but **does not update the FK `ondelete` policy**. In SQLite batch mode, `batch_alter_table` recreates the table from reflected metadata plus specified changes. Since the FK policy change is not specified, the recreated table retains the old `SET NULL` policy. **Result**: A contradictory schema where: - The FK says `ondelete="SET NULL"` (try to NULL the column on parent deletion) - The column says `nullable=False` (reject NULL values) Any attempt to delete a plan referenced as a root ancestor would trigger the FK `SET NULL` action, which immediately violates the NOT NULL constraint, producing a confusing `IntegrityError`. While data integrity is preserved (deletion is blocked either way), the error semantics are wrong and the ORM model is out of sync with the live database schema. **Suggested fix**: Add an explicit FK constraint recreation in the migration's `batch_alter_table` block, or document this as a known schema drift that will be reconciled in a future migration. --- ## MEDIUM Severity ### M1 — Bug: Migration orphan fallback silently self-references without logging **File**: `alembic/versions/m8_001_align_plans_schema.py:82` ```python # Fallback: any remaining NULLs (orphaned children) self-reference. op.execute("UPDATE v3_plans SET root_plan_id = plan_id WHERE root_plan_id IS NULL") ``` After the level-by-level propagation loop, this fallback assigns `root_plan_id = plan_id` to any remaining unresolved rows. This handles orphaned children (whose `parent_plan_id` references a non-existent row) by silently making them root plans. The loop exhaustion at `_MAX_DEPTH=100` correctly logs an error, but the **fallback itself produces no log output** about how many rows were affected. If orphaned children exist, this masks a data integrity problem that operators should investigate. **Suggested fix**: Execute the fallback as a `bind.execute()` call and log the `result.rowcount` at WARNING level if > 0. --- ### M2 — Spec Compliance: `effective_profile_snapshot` default enables empty-snapshot creation **Files**: `models.py:641-642`, `plan.py:713-714` The spec DDL (line 45472) defines: ```sql effective_profile_snapshot TEXT NOT NULL, -- JSON: frozen profile at creation ``` No `DEFAULT` is specified, implying the value must be explicitly populated at plan creation time ("frozen profile at creation"). The implementation adds `default="{}"` / `server_default="{}"` at both ORM and domain model levels. This means new plans can be created through code paths that do not explicitly set the snapshot, receiving an empty `"{}"` instead of the actual profile configuration. This undermines the spec's intended audit/reproducibility guarantee — it becomes impossible to distinguish "no profile was captured" from "an empty profile was intentionally set." **Suggested fix**: Consider removing the domain-level default (keep only `server_default` for migration backfill) and requiring explicit snapshot population at plan creation time, or add a comment documenting why the default is acceptable. --- ### M3 — Test Coverage: Migration backfill logic has no direct test **File**: `alembic/versions/m8_001_align_plans_schema.py:47-82` The level-by-level root ancestor propagation is the most complex algorithm in this commit. It uses a correlated UPDATE with a parent-readiness guard to resolve `root_plan_id` for child plans at arbitrary hierarchy depth. This logic is **not covered by any test**. The BDD scenarios create plans from scratch via the ORM in a fresh in-memory database (where `Base.metadata.create_all()` creates the table with NOT NULL already applied). They do not exercise the migration's actual upgrade path on a pre-existing database with NULL `root_plan_id` values. **Impact**: A regression in the propagation query (e.g., incorrect join, missing parent-readiness guard) would go undetected. --- ### M4 — Test Coverage: Migration cycle detection guard is untested **File**: `alembic/versions/m8_001_align_plans_schema.py:56-74` The `for/else` safety bound (`_MAX_DEPTH = 100`) with `_logger.error` on exhaustion is a guard against cycles in `parent_plan_id`. No test verifies that: 1. Cycles are detected (loop exhausts without convergence) 2. The error is logged 3. The fallback self-reference is applied to unresolved rows Since this guard protects against data corruption in production databases, it should have at least one test demonstrating correct behavior under a cyclic hierarchy. --- ## LOW Severity ### L1 — Test Coverage: No test for `RecursionError` catch in `to_domain()` deserialization **File**: `models.py:828` The hardened deserialization adds `RecursionError` to the exception tuple, but no BDD scenario creates a deeply nested JSON value that would trigger this path. While difficult to trigger in practice, the handler was added for a reason and should be verified. --- ### L2 — Test Coverage: No test for Pydantic validator rejecting invalid JSON snapshot **File**: `plan.py:769-779` The `validate_effective_profile_snapshot_json` field validator raises `ValueError` for non-JSON input. No BDD scenario tests that constructing a `Plan` with `effective_profile_snapshot="not-json"` is rejected. The NULL database constraint is tested, but the Pydantic-level JSON validity check is not. --- ### L3 — Test Coverage: No test for empty-string `automation_profile` backfill **File**: `alembic/versions/m8_001_align_plans_schema.py:83-86` The migration handles `WHERE automation_profile IS NULL OR automation_profile = ''`. The NULL case is implicitly covered by existing data patterns, but the empty-string (`''`) case has no explicit test. If the `OR automation_profile = ''` clause were accidentally removed, no test would catch it. --- ### L4 — Code Quality: Robot and Behave `_make_plan()` helpers are near-identical duplicates **Files**: `features/steps/plans_table_schema_alignment_steps.py:46-68`, `robot/helper_plans_table_schema_alignment.py:78-100` Both define `_make_plan()` with identical parameters (`plan_id`, `parent_plan_id`, `root_plan_id`, `effective_profile_snapshot`, `automation_profile`) and virtually identical bodies constructing a `Plan` domain object. This violates DRY — any change to plan construction logic must be applied in two places. **Suggested fix**: Extract a shared test factory (e.g., in a `tests/factories/` module) that both Behave steps and Robot helpers can import. --- ### L5 — Code Quality: Redundant `None` check on NOT NULL `effective_profile_snapshot` in `to_domain()` **File**: `models.py:878-882` ```python effective_profile_snapshot=( cast(str, self.effective_profile_snapshot) if self.effective_profile_snapshot is not None else "{}" ), ``` Since `effective_profile_snapshot` is now `nullable=False` with `server_default="{}"`, the value will never be `None` in a correctly migrated database. The `is not None` guard is defensive but will never trigger, adding dead code. --- ### L6 — Spec Compliance: `automation_profile` semantic difference vs spec undocumented **File**: `models.py:573-583` The docstring correctly documents the column **naming** deviations (`automation_profile` vs `automation_profile_name`). However, it does not document the **semantic** difference: the spec's `automation_profile_name` stores a plain profile name string, while the code's `automation_profile` stores either a bare name (`"balanced"`) or structured JSON with provenance (`{"profile_name":"...","provenance":"..."}`). This dual-format storage is non-obvious and could confuse future maintainers. --- ### L7 — Security: Pydantic validator error message includes full raw value **File**: `plan.py:776-778` ```python raise ValueError( f"effective_profile_snapshot must be valid JSON, got: {v!r}" ) from exc ``` The error message includes the full raw value via `repr()`. For normal automation profile snapshots this is harmless, but if an oversized or adversarial input reaches this validator, the error message (and any logs that capture it) could become very large. Consider truncating the displayed value (e.g., `v[:200]`). --- ### Notes — Positive Observations - **Hardened deserialization**: The `to_domain()` exception handling is comprehensive (5 exception types) and the logging correctly avoids leaking raw values (length-only). - **Explicit None check**: Using `is not None` instead of truthiness for `root_plan_id` resolution is correct — it avoids treating empty string as falsy. - **Batch optimization**: Merging `batch_alter_table` operations to avoid redundant SQLite table copies is a good performance practice. - **Downgrade documentation**: The note about irreversible backfill is clear and appropriate. - **BDD coverage breadth**: 17 scenarios covering normal paths, edge cases (grandchild hierarchy), constraint enforcement (NULL rejection), and deserialization resilience (bare strings, truncated JSON, missing keys, invalid enums).

CoreRasurae force-pushed fix/plans-table-schema-alignment from 7d52e0fd27 to e5d824c8b1 2026-03-30 16:56:09 +00:00 Compare

CoreRasurae reviewed 2026-03-30 17:10:41 +00:00

CoreRasurae left a comment

Copy link

Code Review Report — PR #1074 (fix/plans-table-schema-alignment)

Issue: #921 — align v3_plans table schema with spec DDL
Commit: e5d824c8 by Luis Mendes
Review scope: all code changes in branch fix/plans-table-schema-alignment plus immediate surrounding code
Method: 4 full global review cycles across all categories (bugs, security, performance, test coverage, test flaws), repeated until no new issues surfaced

---

Summary

The change correctly addresses the issue's acceptance criteria: effective_profile_snapshot is added, root_plan_id is made NOT NULL with self-referencing, automation_profile is made NOT NULL with default, column name deviations are documented, and an Alembic migration with level-by-level backfill is provided. The migration's depth-bounded propagation loop and cycle guard are well-designed. Documentation in both code comments and the commit message is thorough.

Below are the findings organized by severity and category.

---

MEDIUM Severity

[BUG-1] No graceful degradation for corrupted effective_profile_snapshot in to_domain() read path

File: src/cleveragents/infrastructure/database/models.py:887-891

The automation_profile deserialization in to_domain() is hardened against 5 exception types (JSONDecodeError, KeyError, ValueError, ValidationError, RecursionError), falling back to None for corrupted values. However, effective_profile_snapshot is passed directly to Plan() construction:

effective_profile_snapshot=(
    cast(str, self.effective_profile_snapshot)
    if self.effective_profile_snapshot is not None
    else "{}"
),

If the DB column contains invalid JSON (e.g., data corruption, manual edit), the Plan's field_validator will raise ValidationError, crashing the entire get()/query operation and making the plan permanently unreadable. A try/except with fallback to "{}" + warning log (mirroring the automation_profile pattern) would prevent a single corrupted row from blocking reads.

---

[BUG-2] Lossy round-trip for non-"balanced" bare-string automation_profile values

File: src/cleveragents/infrastructure/database/models.py:825 + repositories.py:1339-1343

The deserialization path skips parsing when raw_ap == "balanced" but attempts json.loads() on all other values. Bare profile-name strings like "supervised" fail JSON parsing, causing the domain model to get automation_profile=None. The serialization path then converts None back to "balanced":

DB: "supervised" → to_domain(): None → update() → from_domain(): "balanced"

A read-then-update cycle silently mutates the stored profile name. This applies to legacy data only (new plans use JSON format), but the migration does not convert bare strings to JSON — it only backfills NULL/empty values. Pre-existing bare-string values other than "balanced" will be silently overwritten on the next plan update.

Recommendation: The migration's backfill step for automation_profile (lines 92-95) could be extended to detect and wrap non-JSON bare strings into JSON format: UPDATE v3_plans SET automation_profile = json_object('profile_name', automation_profile, 'provenance', 'default') WHERE automation_profile IS NOT NULL AND automation_profile != '' AND json_valid(automation_profile) = 0.

---

[BUG-3] Self-referencing root_plan_id FK with RESTRICT makes root plans undeletable under FK enforcement

File: src/cleveragents/infrastructure/database/models.py:605-608

Root plans have root_plan_id = plan_id (self-reference). The FK is ondelete="RESTRICT" with nullable=False. Under active FK enforcement (PostgreSQL, or SQLite with PRAGMA foreign_keys = ON):

1. DELETE is blocked by the self-referencing RESTRICT FK
2. Cannot SET NULL first because the column is NOT NULL
3. No CASCADE or SET DEFAULT alternative configured

This makes root plans undeletable via simple SQL DELETE. Currently mitigated by SQLite's default FK non-enforcement (as documented in the migration comment), but will require application-level cascade logic or deferred constraints for the planned PostgreSQL migration.

---

[TEST-1] No integration test for the Alembic migration's backfill logic

File: alembic/versions/m8_001_align_plans_schema.py:45-89

The level-by-level root_plan_id propagation loop (the most complex logic in this change) is untested. All BDD/Robot tests use Base.metadata.create_all() which creates fresh tables, bypassing the migration entirely. Key untested paths:

• Multi-level hierarchy backfill (grandchild → child → root resolution)
• Cycle detection error path (line 74-79)
• Orphan fallback with WARNING log (line 80-89)
• Empty-string automation_profile backfill (line 92-95)

---

LOW Severity

[SEC-1] effective_profile_snapshot JSON validator does not catch RecursionError

File: src/cleveragents/domain/models/core/plan.py:780

The validator catches json.JSONDecodeError and TypeError, but not RecursionError. The automation_profile deserialization explicitly handles RecursionError for deeply nested JSON. A crafted deeply-nested JSON string as effective_profile_snapshot would cause an unhandled RecursionError during Plan construction.

# Current:
except (json.JSONDecodeError, TypeError) as exc:
# Suggested:
except (json.JSONDecodeError, TypeError, RecursionError) as exc:

---

[SEC-2] Validator error message exposes first 200 chars of snapshot content

File: src/cleveragents/domain/models/core/plan.py:782-783

f"got (first 200 chars): {v[:200]!r}"

The commit message notes the truncation, and the logging in to_domain() uses length-only for automation_profile. If the snapshot contains sensitive profile configuration data (API keys, thresholds), the 200-char excerpt could be leaked in error messages. The to_domain() logging pattern (length-only) is more conservative.

---

[TEST-2] No test for update() path automation_profile=None to "balanced" serialization

File: src/cleveragents/infrastructure/database/repositories.py:1339-1343

The create path's None → "balanced" serialization is covered indirectly, but the update() path (which has its own independent serialization block) is not tested for this specific case. A scenario that creates a plan with a profile, then updates it with automation_profile=None, would cover this.

---

[TEST-3] No test for RecursionError catch in automation_profile deserialization

File: src/cleveragents/infrastructure/database/models.py:837

The RecursionError exception type was explicitly added to the catch list but no test exercises this code path.

---

[TEST-4] No negative test for empty-string effective_profile_snapshot

File: src/cleveragents/domain/models/core/plan.py:774-785

There is a test for invalid JSON ("not-valid-json"), but not for the empty string "" case, which is a common edge case for string fields that json.loads() would also reject.

---

INFORMATIONAL

• Performance: No significant performance issues. The migration backfill loop is bounded (_MAX_DEPTH=100), runtime operations are O(1) for the new fields, and json.loads() in the validator runs once at construction time.
• Spec compliance: All acceptance criteria from issue #921 are met. Column naming deviations, the phase default deviation, and the automation_profile dual-format storage are well-documented in code comments.
• Test coverage: 17 new BDD scenarios + 7 Robot test cases provide good coverage of the happy paths and common error cases. Existing test files are correctly updated with the new required fields.
• Code quality: The commit message is exemplary — detailed, structured, and references specific decisions. Documentation in code comments is thorough.

---

Review performed via 4 global cycles across categories: bugs, security, performance, test coverage, and test flaws.

## Code Review Report — PR #1074 (`fix/plans-table-schema-alignment`) **Issue:** #921 — align `v3_plans` table schema with spec DDL **Commit:** `e5d824c8` by Luis Mendes **Review scope:** all code changes in branch `fix/plans-table-schema-alignment` plus immediate surrounding code **Method:** 4 full global review cycles across all categories (bugs, security, performance, test coverage, test flaws), repeated until no new issues surfaced --- ### Summary The change correctly addresses the issue's acceptance criteria: `effective_profile_snapshot` is added, `root_plan_id` is made NOT NULL with self-referencing, `automation_profile` is made NOT NULL with default, column name deviations are documented, and an Alembic migration with level-by-level backfill is provided. The migration's depth-bounded propagation loop and cycle guard are well-designed. Documentation in both code comments and the commit message is thorough. Below are the findings organized by severity and category. --- ### MEDIUM Severity #### [BUG-1] No graceful degradation for corrupted `effective_profile_snapshot` in `to_domain()` read path **File:** `src/cleveragents/infrastructure/database/models.py:887-891` The `automation_profile` deserialization in `to_domain()` is hardened against 5 exception types (`JSONDecodeError`, `KeyError`, `ValueError`, `ValidationError`, `RecursionError`), falling back to `None` for corrupted values. However, `effective_profile_snapshot` is passed directly to `Plan()` construction: ```python effective_profile_snapshot=( cast(str, self.effective_profile_snapshot) if self.effective_profile_snapshot is not None else "{}" ), ``` If the DB column contains invalid JSON (e.g., data corruption, manual edit), the `Plan`'s `field_validator` will raise `ValidationError`, crashing the entire `get()`/query operation and making the plan permanently unreadable. A try/except with fallback to `"{}"` + warning log (mirroring the `automation_profile` pattern) would prevent a single corrupted row from blocking reads. --- #### [BUG-2] Lossy round-trip for non-`"balanced"` bare-string `automation_profile` values **File:** `src/cleveragents/infrastructure/database/models.py:825` + `repositories.py:1339-1343` The deserialization path skips parsing when `raw_ap == "balanced"` but attempts `json.loads()` on all other values. Bare profile-name strings like `"supervised"` fail JSON parsing, causing the domain model to get `automation_profile=None`. The serialization path then converts `None` back to `"balanced"`: ``` DB: "supervised" → to_domain(): None → update() → from_domain(): "balanced" ``` A read-then-update cycle silently mutates the stored profile name. This applies to legacy data only (new plans use JSON format), but the migration does not convert bare strings to JSON — it only backfills NULL/empty values. Pre-existing bare-string values other than `"balanced"` will be silently overwritten on the next plan update. **Recommendation:** The migration's backfill step for `automation_profile` (lines 92-95) could be extended to detect and wrap non-JSON bare strings into JSON format: `UPDATE v3_plans SET automation_profile = json_object('profile_name', automation_profile, 'provenance', 'default') WHERE automation_profile IS NOT NULL AND automation_profile != '' AND json_valid(automation_profile) = 0`. --- #### [BUG-3] Self-referencing `root_plan_id` FK with RESTRICT makes root plans undeletable under FK enforcement **File:** `src/cleveragents/infrastructure/database/models.py:605-608` Root plans have `root_plan_id = plan_id` (self-reference). The FK is `ondelete="RESTRICT"` with `nullable=False`. Under active FK enforcement (PostgreSQL, or SQLite with `PRAGMA foreign_keys = ON`): 1. DELETE is blocked by the self-referencing RESTRICT FK 2. Cannot SET NULL first because the column is NOT NULL 3. No CASCADE or SET DEFAULT alternative configured This makes root plans undeletable via simple SQL DELETE. Currently mitigated by SQLite's default FK non-enforcement (as documented in the migration comment), but will require application-level cascade logic or deferred constraints for the planned PostgreSQL migration. --- #### [TEST-1] No integration test for the Alembic migration's backfill logic **File:** `alembic/versions/m8_001_align_plans_schema.py:45-89` The level-by-level root_plan_id propagation loop (the most complex logic in this change) is untested. All BDD/Robot tests use `Base.metadata.create_all()` which creates fresh tables, bypassing the migration entirely. Key untested paths: - Multi-level hierarchy backfill (grandchild → child → root resolution) - Cycle detection error path (line 74-79) - Orphan fallback with WARNING log (line 80-89) - Empty-string `automation_profile` backfill (line 92-95) --- ### LOW Severity #### [SEC-1] `effective_profile_snapshot` JSON validator does not catch `RecursionError` **File:** `src/cleveragents/domain/models/core/plan.py:780` The validator catches `json.JSONDecodeError` and `TypeError`, but not `RecursionError`. The `automation_profile` deserialization explicitly handles `RecursionError` for deeply nested JSON. A crafted deeply-nested JSON string as `effective_profile_snapshot` would cause an unhandled `RecursionError` during `Plan` construction. ```python # Current: except (json.JSONDecodeError, TypeError) as exc: # Suggested: except (json.JSONDecodeError, TypeError, RecursionError) as exc: ``` --- #### [SEC-2] Validator error message exposes first 200 chars of snapshot content **File:** `src/cleveragents/domain/models/core/plan.py:782-783` ```python f"got (first 200 chars): {v[:200]!r}" ``` The commit message notes the truncation, and the logging in `to_domain()` uses length-only for `automation_profile`. If the snapshot contains sensitive profile configuration data (API keys, thresholds), the 200-char excerpt could be leaked in error messages. The `to_domain()` logging pattern (length-only) is more conservative. --- #### [TEST-2] No test for `update()` path `automation_profile=None` to `"balanced"` serialization **File:** `src/cleveragents/infrastructure/database/repositories.py:1339-1343` The create path's `None → "balanced"` serialization is covered indirectly, but the `update()` path (which has its own independent serialization block) is not tested for this specific case. A scenario that creates a plan with a profile, then updates it with `automation_profile=None`, would cover this. --- #### [TEST-3] No test for `RecursionError` catch in `automation_profile` deserialization **File:** `src/cleveragents/infrastructure/database/models.py:837` The `RecursionError` exception type was explicitly added to the catch list but no test exercises this code path. --- #### [TEST-4] No negative test for empty-string `effective_profile_snapshot` **File:** `src/cleveragents/domain/models/core/plan.py:774-785` There is a test for invalid JSON (`"not-valid-json"`), but not for the empty string `""` case, which is a common edge case for string fields that `json.loads()` would also reject. --- ### INFORMATIONAL - **Performance:** No significant performance issues. The migration backfill loop is bounded (`_MAX_DEPTH=100`), runtime operations are O(1) for the new fields, and `json.loads()` in the validator runs once at construction time. - **Spec compliance:** All acceptance criteria from issue #921 are met. Column naming deviations, the `phase` default deviation, and the `automation_profile` dual-format storage are well-documented in code comments. - **Test coverage:** 17 new BDD scenarios + 7 Robot test cases provide good coverage of the happy paths and common error cases. Existing test files are correctly updated with the new required fields. - **Code quality:** The commit message is exemplary — detailed, structured, and references specific decisions. Documentation in code comments is thorough. --- *Review performed via 4 global cycles across categories: bugs, security, performance, test coverage, and test flaws.*

CoreRasurae force-pushed fix/plans-table-schema-alignment from e5d824c8b1 to 25c57085ce 2026-03-30 17:33:48 +00:00 Compare

CoreRasurae reviewed 2026-03-30 17:55:35 +00:00

CoreRasurae left a comment

Copy link

Code Review Report — PR #1074 (fix/plans-table-schema-alignment)

Reviewed commit: 25c57085 by Luis Mendes
Issue: #921 — align v3_plans table schema with specification DDL
Spec reference: docs/specification.md lines 45463–45489
Scope: All code changes in branch fix/plans-table-schema-alignment plus close connections to surrounding code.
Review cycles: 4 full passes across all categories (bugs, security, performance, test coverage, spec compliance, design).

---

Overall Assessment

The PR correctly addresses all acceptance criteria from issue #921. The schema changes (new column, nullability fixes, FK policy), migration with level-by-level backfill, hardened deserialization, and comprehensive BDD/Robot tests are well-engineered. The inline documentation is thorough — design decisions, deviations from spec, and trade-offs are clearly explained.

No critical or high-severity issues were found. The findings below are medium-to-low severity items worth considering.

---

Findings by Severity

MEDIUM

M1 — Missing test: to_domain() corrupted effective_profile_snapshot fallback path

• Category: Test Coverage
• Location: models.py:858–867
• Description: The to_domain() method has a defensive code path that catches JSONDecodeError/RecursionError on corrupted effective_profile_snapshot DB values and falls back to "{}" with a WARNING log. This path is untested. The BDD scenario "Invalid JSON in effective_profile_snapshot is rejected by Plan model" tests the Pydantic validator, not the to_domain() fallback. A test should: insert a row with invalid JSON directly in the effective_profile_snapshot column, call repo.get(), and verify the returned plan has effective_profile_snapshot == "{}" instead of crashing.

M2 — Missing test: migration backfill logic with pre-existing data

• Category: Test Coverage
• Location: alembic/versions/m8_001_align_plans_schema.py:52–89
• Description: The migration's level-by-level root_plan_id backfill and automation_profile backfill are not tested with pre-existing data. All tests use fresh in-memory databases via Base.metadata.create_all(), which skips the migration entirely. A migration-specific test should create an old-schema database with NULL root_plan_id rows (including multi-level hierarchies and orphaned children), run the upgrade, and verify correct backfill. Without this, bugs in the propagation loop or orphan-fallback logic would not be caught.

LOW

L1 — TypeError not caught in to_domain() snapshot validation

• Category: Robustness / Inconsistency
• Location: models.py:860
• Description: The to_domain() fallback catches (json.JSONDecodeError, RecursionError), but the Plan model's field_validator at plan.py:780 catches (json.JSONDecodeError, TypeError, RecursionError). For consistency and defense-in-depth, TypeError should also be caught in to_domain(). While the NOT NULL constraint and the is not None ternary make a TypeError extremely unlikely, the asymmetry is a code smell.

L2 — Hardcoded "balanced" sentinel in multiple locations

• Category: Design / Maintainability
• Locations: models.py:648, models.py:825, models.py:984, repositories.py:1342
• Description: The string "balanced" is used as a sentinel/default in 4 separate production code locations. If the default automation profile name ever changes, all 4 must be updated in sync. Consider extracting to a module-level constant (e.g. _DEFAULT_AUTOMATION_PROFILE = "balanced") and referencing it from all locations.

L3 — Double JSON parse of effective_profile_snapshot

• Category: Performance
• Location: models.py:858–859 and plan.py:778–779
• Description: Every to_domain() call parses the snapshot JSON once for defensive validation (discarding the result), then the Plan model's field_validator parses it again. For the common case ("{}"), the overhead is negligible. For large snapshots, this doubles the parse cost. This is an intentional defense-in-depth trade-off (the to_domain() check sanitizes corrupted data before the strict validator runs), so it's acceptable — but worth noting if snapshot sizes grow in the future.

L4 — Missing test: RecursionError in effective_profile_snapshot paths

• Category: Test Coverage
• Location: plan.py:780, models.py:860
• Description: The commit message explicitly mentions adding RecursionError handling for effective_profile_snapshot (both the Pydantic validator and to_domain() fallback), but no test exercises this path. Deeply nested JSON payloads that trigger Python's recursion limit are an edge case, but since the code was explicitly hardened for it, a test would confirm the defense works.

L5 — Missing test: RecursionError in automation_profile deserialization

• Category: Test Coverage
• Location: models.py:837
• Description: Similarly, RecursionError was added to the automation_profile exception list in to_domain(), but no test exercises it.

INFORMATIONAL

I1 — Callers do not populate effective_profile_snapshot yet

• Category: Follow-up / Spec Compliance
• Locations: plan_lifecycle_service.py:758 (primary plan creation), subplan_service.py:260 (child plan spawning)
• Description: The two production callers that construct lifecycle Plan objects both omit effective_profile_snapshot, relying on the "{}" default. The spec intent (line 45472: "frozen profile at creation") and the field's own docstring (lines 713–717) both indicate this should be explicitly set. The author has documented this as a known limitation. A follow-up ticket to wire the resolved automation profile snapshot into these callers would complete the spec intent.

I2 — automation_profile server_default="balanced" is more permissive than spec

• Category: Spec Compliance
• Description: The spec DDL has automation_profile_name TEXT NOT NULL with no DEFAULT clause, implying callers must always provide it. The code adds server_default="balanced" for backward compatibility. This is pragmatically correct but technically a deviation from spec strictness.

I3 — FK ondelete policy drift between ORM and migrated databases

• Category: Spec Compliance
• Description: The ORM model specifies ondelete="RESTRICT" for root_plan_id, but SQLite batch_alter_table in the migration retains the previous SET NULL policy. The commit documents this thoroughly. Practically, the NOT NULL constraint prevents SET NULL from succeeding, so behavior is equivalent. Fresh databases (via create_all) get the correct RESTRICT policy.

---

Summary Table

ID	Severity	Category	Description
M1	Medium	Test Coverage	to_domain() corrupted snapshot fallback untested
M2	Medium	Test Coverage	Migration backfill logic untested with pre-existing data
L1	Low	Robustness	TypeError not caught in to_domain() snapshot check
L2	Low	Maintainability	Hardcoded "balanced" sentinel in 4 locations
L3	Low	Performance	Double JSON parse of snapshot (intentional trade-off)
L4	Low	Test Coverage	RecursionError in snapshot paths untested
L5	Low	Test Coverage	RecursionError in automation_profile untested
I1	Info	Follow-up	Callers don't populate snapshot yet (documented)
I2	Info	Spec Compliance	server_default more permissive than spec
I3	Info	Spec Compliance	FK ondelete policy drift (documented, safe)

---

Positive Highlights

• Excellent inline documentation of design decisions, spec deviations, and trade-offs throughout all changed files.
• The level-by-level migration backfill with parent-readiness guard correctly handles arbitrary hierarchy depths, avoiding the snapshot-semantics pitfall of a single correlated UPDATE.
• Comprehensive BDD coverage (17 scenarios) and Robot Framework integration tests (7 commands).
• Defensive deserialization in to_domain() prevents a single corrupted DB row from crashing reads.
• Migration merges batch_alter_table operations to avoid redundant full-table copies in SQLite.
• The commit message is thorough and structured, making the change easy to audit.

No blocking issues found. The medium-severity test gaps (M1, M2) are the most actionable items to address.

## Code Review Report — PR #1074 (`fix/plans-table-schema-alignment`) **Reviewed commit:** `25c57085` by Luis Mendes **Issue:** #921 — align `v3_plans` table schema with specification DDL **Spec reference:** `docs/specification.md` lines 45463–45489 **Scope:** All code changes in branch `fix/plans-table-schema-alignment` plus close connections to surrounding code. **Review cycles:** 4 full passes across all categories (bugs, security, performance, test coverage, spec compliance, design). --- ### Overall Assessment The PR correctly addresses all acceptance criteria from issue #921. The schema changes (new column, nullability fixes, FK policy), migration with level-by-level backfill, hardened deserialization, and comprehensive BDD/Robot tests are well-engineered. The inline documentation is thorough — design decisions, deviations from spec, and trade-offs are clearly explained. No critical or high-severity issues were found. The findings below are medium-to-low severity items worth considering. --- ### Findings by Severity #### MEDIUM **M1 — Missing test: `to_domain()` corrupted `effective_profile_snapshot` fallback path** - **Category:** Test Coverage - **Location:** `models.py:858–867` - **Description:** The `to_domain()` method has a defensive code path that catches `JSONDecodeError`/`RecursionError` on corrupted `effective_profile_snapshot` DB values and falls back to `"{}"` with a WARNING log. This path is **untested**. The BDD scenario "Invalid JSON in effective_profile_snapshot is rejected by Plan model" tests the Pydantic validator, not the `to_domain()` fallback. A test should: insert a row with invalid JSON directly in the `effective_profile_snapshot` column, call `repo.get()`, and verify the returned plan has `effective_profile_snapshot == "{}"` instead of crashing. **M2 — Missing test: migration backfill logic with pre-existing data** - **Category:** Test Coverage - **Location:** `alembic/versions/m8_001_align_plans_schema.py:52–89` - **Description:** The migration's level-by-level `root_plan_id` backfill and `automation_profile` backfill are not tested with pre-existing data. All tests use fresh in-memory databases via `Base.metadata.create_all()`, which skips the migration entirely. A migration-specific test should create an old-schema database with NULL `root_plan_id` rows (including multi-level hierarchies and orphaned children), run the upgrade, and verify correct backfill. Without this, bugs in the propagation loop or orphan-fallback logic would not be caught. #### LOW **L1 — `TypeError` not caught in `to_domain()` snapshot validation** - **Category:** Robustness / Inconsistency - **Location:** `models.py:860` - **Description:** The `to_domain()` fallback catches `(json.JSONDecodeError, RecursionError)`, but the Plan model's `field_validator` at `plan.py:780` catches `(json.JSONDecodeError, TypeError, RecursionError)`. For consistency and defense-in-depth, `TypeError` should also be caught in `to_domain()`. While the NOT NULL constraint and the `is not None` ternary make a `TypeError` extremely unlikely, the asymmetry is a code smell. **L2 — Hardcoded `"balanced"` sentinel in multiple locations** - **Category:** Design / Maintainability - **Locations:** `models.py:648`, `models.py:825`, `models.py:984`, `repositories.py:1342` - **Description:** The string `"balanced"` is used as a sentinel/default in 4 separate production code locations. If the default automation profile name ever changes, all 4 must be updated in sync. Consider extracting to a module-level constant (e.g. `_DEFAULT_AUTOMATION_PROFILE = "balanced"`) and referencing it from all locations. **L3 — Double JSON parse of `effective_profile_snapshot`** - **Category:** Performance - **Location:** `models.py:858–859` and `plan.py:778–779` - **Description:** Every `to_domain()` call parses the snapshot JSON once for defensive validation (discarding the result), then the Plan model's `field_validator` parses it again. For the common case (`"{}"`), the overhead is negligible. For large snapshots, this doubles the parse cost. This is an intentional defense-in-depth trade-off (the `to_domain()` check sanitizes corrupted data before the strict validator runs), so it's acceptable — but worth noting if snapshot sizes grow in the future. **L4 — Missing test: `RecursionError` in `effective_profile_snapshot` paths** - **Category:** Test Coverage - **Location:** `plan.py:780`, `models.py:860` - **Description:** The commit message explicitly mentions adding `RecursionError` handling for `effective_profile_snapshot` (both the Pydantic validator and `to_domain()` fallback), but no test exercises this path. Deeply nested JSON payloads that trigger Python's recursion limit are an edge case, but since the code was explicitly hardened for it, a test would confirm the defense works. **L5 — Missing test: `RecursionError` in `automation_profile` deserialization** - **Category:** Test Coverage - **Location:** `models.py:837` - **Description:** Similarly, `RecursionError` was added to the `automation_profile` exception list in `to_domain()`, but no test exercises it. #### INFORMATIONAL **I1 — Callers do not populate `effective_profile_snapshot` yet** - **Category:** Follow-up / Spec Compliance - **Locations:** `plan_lifecycle_service.py:758` (primary plan creation), `subplan_service.py:260` (child plan spawning) - **Description:** The two production callers that construct lifecycle `Plan` objects both omit `effective_profile_snapshot`, relying on the `"{}"` default. The spec intent (line 45472: "frozen profile at creation") and the field's own docstring (lines 713–717) both indicate this should be explicitly set. The author has documented this as a known limitation. A follow-up ticket to wire the resolved automation profile snapshot into these callers would complete the spec intent. **I2 — `automation_profile` `server_default="balanced"` is more permissive than spec** - **Category:** Spec Compliance - **Description:** The spec DDL has `automation_profile_name TEXT NOT NULL` with no DEFAULT clause, implying callers must always provide it. The code adds `server_default="balanced"` for backward compatibility. This is pragmatically correct but technically a deviation from spec strictness. **I3 — FK `ondelete` policy drift between ORM and migrated databases** - **Category:** Spec Compliance - **Description:** The ORM model specifies `ondelete="RESTRICT"` for `root_plan_id`, but SQLite `batch_alter_table` in the migration retains the previous `SET NULL` policy. The commit documents this thoroughly. Practically, the NOT NULL constraint prevents SET NULL from succeeding, so behavior is equivalent. Fresh databases (via `create_all`) get the correct RESTRICT policy. --- ### Summary Table | ID | Severity | Category | Description | |----|----------|----------|-------------| | M1 | Medium | Test Coverage | `to_domain()` corrupted snapshot fallback untested | | M2 | Medium | Test Coverage | Migration backfill logic untested with pre-existing data | | L1 | Low | Robustness | `TypeError` not caught in `to_domain()` snapshot check | | L2 | Low | Maintainability | Hardcoded `"balanced"` sentinel in 4 locations | | L3 | Low | Performance | Double JSON parse of snapshot (intentional trade-off) | | L4 | Low | Test Coverage | `RecursionError` in snapshot paths untested | | L5 | Low | Test Coverage | `RecursionError` in automation_profile untested | | I1 | Info | Follow-up | Callers don't populate snapshot yet (documented) | | I2 | Info | Spec Compliance | `server_default` more permissive than spec | | I3 | Info | Spec Compliance | FK ondelete policy drift (documented, safe) | --- ### Positive Highlights - Excellent inline documentation of design decisions, spec deviations, and trade-offs throughout all changed files. - The level-by-level migration backfill with parent-readiness guard correctly handles arbitrary hierarchy depths, avoiding the snapshot-semantics pitfall of a single correlated UPDATE. - Comprehensive BDD coverage (17 scenarios) and Robot Framework integration tests (7 commands). - Defensive deserialization in `to_domain()` prevents a single corrupted DB row from crashing reads. - Migration merges `batch_alter_table` operations to avoid redundant full-table copies in SQLite. - The commit message is thorough and structured, making the change easy to audit. No blocking issues found. The medium-severity test gaps (M1, M2) are the most actionable items to address.

CoreRasurae force-pushed fix/plans-table-schema-alignment from 25c57085ce to cfc6702ad3 2026-03-30 18:45:11 +00:00 Compare

CoreRasurae reviewed 2026-03-30 18:52:17 +00:00

CoreRasurae left a comment

Copy link

Code Review Report — PR #1074 (fix/plans-table-schema-alignment)

Reviewer: Automated code review (bug detection, security, performance, test coverage, test flaws)
Scope: All changes in branch fix/plans-table-schema-alignment (commit cfc6702) + close surroundings
Cycles completed: 3 full passes across all categories until no new issues found

---

Overall Assessment

The PR delivers solid schema alignment work. The migration backfill algorithm is well-designed (level-by-level propagation with parent-readiness guard), the defensive deserialization in to_domain() is thorough, and the new Pydantic validator for effective_profile_snapshot is correctly implemented. Documentation quality is high — column naming deviations, FK policy drift, and the phase default deviation are all explicitly documented.

The findings below are organized by severity and category.

---

MEDIUM Severity

M1 — Bug/Data Integrity: Migration cycle detection silently continues with incorrect data

File: alembic/versions/m8_001_align_plans_schema.py:74-89

When the root_plan_id backfill loop exhausts 100 iterations without converging (indicating a cycle in parent_plan_id), the for/else clause logs an ERROR but does not abort the migration. Execution falls through to the orphan fallback (line 80-89), which self-references remaining plans — assigning them their own plan_id as root_plan_id instead of the true root ancestor.

This means plans caught in cycles silently receive incorrect root_plan_id values, and the NOT NULL constraint succeeds, masking the data corruption.

Suggestion: Consider raising an exception (or at minimum logging the affected plan_id values) instead of silently falling through to the self-referencing fallback when the cycle guard triggers. Alternatively, the fallback could log at ERROR level (not just WARNING) when it runs after cycle exhaustion, to distinguish orphan rows from cycle-affected rows.

---

M2 — Test Coverage: Migration backfill logic has zero test coverage

Files: alembic/versions/m8_001_align_plans_schema.py (all of upgrade())

All BDD tests (plans_table_schema_alignment.feature) and Robot tests (plans_table_schema_alignment.robot) create fresh in-memory databases via Base.metadata.create_all(), which applies the final schema directly. The Alembic migration's upgrade() function — including the level-by-level propagation algorithm, cycle detection guard, orphan fallback, and automation_profile backfill — is never exercised by any test.

Key untested paths:

• Level-by-level propagation for hierarchies of depth >= 2
• Cycle detection (for/else clause, lines 74-79)
• Orphan fallback with WARNING log (lines 80-89)
• Empty-string automation_profile backfill (line 94)
• batch_alter_table operations (lines 110-119)

Suggestion: Add at least one integration test that creates a pre-migration schema (with NULL root_plan_id rows in a parent-child hierarchy), runs the Alembic migration, and verifies the backfilled values. This is especially important for the cycle-detection and orphan-fallback paths.

---

LOW Severity

L1 — Maintainability: Duplicated automation_profile serialization logic

Files: repositories.py:1340-1344 and models.py:989-993

The update() method in LifecyclePlanRepository duplicates the automation profile serialization pattern from LifecyclePlanModel.from_domain():

automation_profile_json: str = (
    json.dumps(plan.automation_profile.model_dump(mode="json"))
    if plan.automation_profile
    else _DEFAULT_AUTOMATION_PROFILE
)

If the serialization logic changes (e.g., adding provenance normalization), it must be updated in two places.

Suggestion: Extract a shared static method on LifecyclePlanModel (e.g., _serialize_automation_profile(ref)) and call it from both from_domain() and the repository's update().

---

L2 — Test Coverage: No test for RecursionError path in effective_profile_snapshot validator

File: plan.py:774-784

The validate_effective_profile_snapshot_json field validator catches RecursionError (line 780) for deeply nested JSON payloads. No BDD or Robot test exercises this code path. While difficult to trigger in practice (requires crafting a JSON string that causes json.loads to exceed Python's recursion limit), the catch was explicitly added and should have a corresponding test.

Suggestion: Add a BDD scenario with a deeply nested JSON string (e.g., 1000+ levels of {"a":{"a":...}}) to verify the validator rejects it with a ValidationError.

---

L3 — Code Style: Migration SQL uses inconsistent patterns

File: alembic/versions/m8_001_align_plans_schema.py:52-55, 92-95 vs 59-70

Lines 52-55 and 92-95 use op.execute("UPDATE ...") with raw SQL strings, while lines 59-70 use bind.execute(sa.text(...)) with sa.text(). Both work, but the inconsistency is notable — sa.text() is the recommended SQLAlchemy pattern for explicit textual SQL.

Suggestion: Wrap all raw SQL in sa.text() for consistency, or use op.execute(sa.text(...)) uniformly.

---

L4 — Defensive Code: to_domain() casts root_plan_id as str | None despite NOT NULL column

File: models.py:905

root_plan_id=cast("str | None", self.root_plan_id),

Since root_plan_id is now NOT NULL in the schema, the cast should be cast(str, self.root_plan_id) for type accuracy (consistent with plan_id on line 903). The str | None cast is overly defensive and sends a misleading signal to type checkers and future readers.

Suggestion: Change to cast(str, self.root_plan_id).

---

L5 — Test Coverage: Robot tests cover only 8 of 18 BDD scenarios

Files: robot/plans_table_schema_alignment.robot vs features/plans_table_schema_alignment.feature

The Robot Framework test suite covers 8 scenarios while the BDD suite covers 18. Missing Robot scenarios include: bare-string/truncated-JSON/invalid-provenance automation_profile deserialization, snapshot update persistence, NULL snapshot rejection, from_domain() self-reference resolution, and invalid JSON rejection by the Plan model constructor.

Suggestion: If the project's testing policy requires Robot parity with BDD, consider adding the missing scenarios. Otherwise, document that Robot tests cover the critical happy paths only.

---

L6 — Naming Convention: Private constant shared across modules

Files: models.py:81 and repositories.py:89

_DEFAULT_AUTOMATION_PROFILE uses a single underscore prefix (Python convention for module-private), but is imported directly in repositories.py. This works but sends a misleading signal about the constant's intended visibility.

Suggestion: Rename to DEFAULT_AUTOMATION_PROFILE (no underscore) since it is a public cross-module constant, or document the intentional import of a private name.

---

L7 — Observability: Corrupted snapshot fallback logs WARNING on every read

File: models.py:869-875

When effective_profile_snapshot contains corrupted JSON, to_domain() logs a WARNING and falls back to "{}" on every read. The corrupted value is not fixed in the database, so the warning fires repeatedly until the plan is updated via update() (which overwrites the value with the in-memory "{}"). For frequently-read plans, this could produce significant log noise.

Suggestion: Consider either (a) logging only once per plan_id using a seen-set or LRU cache, or (b) documenting that the self-healing occurs on the next update() call.

---

L8 — Migration: Revision naming gap (m4_003 → m8_001)

File: alembic/versions/m8_001_align_plans_schema.py:26-27

The revision jumps from m4_003_plan_env_columns to m8_001_align_plans_schema. If intermediate revisions (m5-m7) exist in other branches, this could cause Alembic multiple-heads conflicts when merging.

Suggestion: Verify that the naming gap is intentional and that no conflicting revisions exist in other active branches. If naming corresponds to milestones (m4 = milestone 4, m8 = milestone 8), document this convention.

---

Summary Table

ID	Severity	Category	File	Line(s)
M1	Medium	Bug / Data Integrity	m8_001_align_plans_schema.py	74-89
M2	Medium	Test Coverage	m8_001_align_plans_schema.py	all
L1	Low	Maintainability	repositories.py / models.py	1340-1344 / 989-993
L2	Low	Test Coverage	plan.py	774-784
L3	Low	Code Style	m8_001_align_plans_schema.py	52-95
L4	Low	Defensive Code	models.py	905
L5	Low	Test Coverage	Robot vs BDD suites	—
L6	Low	Naming Convention	models.py / repositories.py	81 / 89
L7	Low	Observability	models.py	869-875
L8	Low	Migration	m8_001_align_plans_schema.py	26-27

No security or performance issues found. The code follows good defensive practices (length-only logging, parameterized queries, exception hardening).

## Code Review Report — PR #1074 (`fix/plans-table-schema-alignment`) **Reviewer:** Automated code review (bug detection, security, performance, test coverage, test flaws) **Scope:** All changes in branch `fix/plans-table-schema-alignment` (commit `cfc6702`) + close surroundings **Cycles completed:** 3 full passes across all categories until no new issues found --- ### Overall Assessment The PR delivers solid schema alignment work. The migration backfill algorithm is well-designed (level-by-level propagation with parent-readiness guard), the defensive deserialization in `to_domain()` is thorough, and the new Pydantic validator for `effective_profile_snapshot` is correctly implemented. Documentation quality is high — column naming deviations, FK policy drift, and the phase default deviation are all explicitly documented. The findings below are organized by severity and category. --- ## MEDIUM Severity ### M1 — Bug/Data Integrity: Migration cycle detection silently continues with incorrect data **File:** `alembic/versions/m8_001_align_plans_schema.py:74-89` When the `root_plan_id` backfill loop exhausts 100 iterations without converging (indicating a cycle in `parent_plan_id`), the `for/else` clause logs an `ERROR` but does **not** abort the migration. Execution falls through to the orphan fallback (line 80-89), which self-references remaining plans — assigning them their **own** `plan_id` as `root_plan_id` instead of the true root ancestor. This means plans caught in cycles silently receive incorrect `root_plan_id` values, and the `NOT NULL` constraint succeeds, masking the data corruption. **Suggestion:** Consider raising an exception (or at minimum logging the affected `plan_id` values) instead of silently falling through to the self-referencing fallback when the cycle guard triggers. Alternatively, the fallback could log at `ERROR` level (not just `WARNING`) when it runs after cycle exhaustion, to distinguish orphan rows from cycle-affected rows. --- ### M2 — Test Coverage: Migration backfill logic has zero test coverage **Files:** `alembic/versions/m8_001_align_plans_schema.py` (all of `upgrade()`) All BDD tests (`plans_table_schema_alignment.feature`) and Robot tests (`plans_table_schema_alignment.robot`) create fresh in-memory databases via `Base.metadata.create_all()`, which applies the final schema directly. The Alembic migration's `upgrade()` function — including the level-by-level propagation algorithm, cycle detection guard, orphan fallback, and `automation_profile` backfill — is never exercised by any test. Key untested paths: - Level-by-level propagation for hierarchies of depth >= 2 - Cycle detection (`for/else` clause, lines 74-79) - Orphan fallback with WARNING log (lines 80-89) - Empty-string `automation_profile` backfill (line 94) - `batch_alter_table` operations (lines 110-119) **Suggestion:** Add at least one integration test that creates a pre-migration schema (with NULL `root_plan_id` rows in a parent-child hierarchy), runs the Alembic migration, and verifies the backfilled values. This is especially important for the cycle-detection and orphan-fallback paths. --- ## LOW Severity ### L1 — Maintainability: Duplicated automation_profile serialization logic **Files:** `repositories.py:1340-1344` and `models.py:989-993` The `update()` method in `LifecyclePlanRepository` duplicates the automation profile serialization pattern from `LifecyclePlanModel.from_domain()`: ```python automation_profile_json: str = ( json.dumps(plan.automation_profile.model_dump(mode="json")) if plan.automation_profile else _DEFAULT_AUTOMATION_PROFILE ) ``` If the serialization logic changes (e.g., adding provenance normalization), it must be updated in two places. **Suggestion:** Extract a shared static method on `LifecyclePlanModel` (e.g., `_serialize_automation_profile(ref)`) and call it from both `from_domain()` and the repository's `update()`. --- ### L2 — Test Coverage: No test for RecursionError path in effective_profile_snapshot validator **File:** `plan.py:774-784` The `validate_effective_profile_snapshot_json` field validator catches `RecursionError` (line 780) for deeply nested JSON payloads. No BDD or Robot test exercises this code path. While difficult to trigger in practice (requires crafting a JSON string that causes `json.loads` to exceed Python's recursion limit), the catch was explicitly added and should have a corresponding test. **Suggestion:** Add a BDD scenario with a deeply nested JSON string (e.g., 1000+ levels of `{"a":{"a":...}}`) to verify the validator rejects it with a `ValidationError`. --- ### L3 — Code Style: Migration SQL uses inconsistent patterns **File:** `alembic/versions/m8_001_align_plans_schema.py:52-55, 92-95 vs 59-70` Lines 52-55 and 92-95 use `op.execute("UPDATE ...")` with raw SQL strings, while lines 59-70 use `bind.execute(sa.text(...))` with `sa.text()`. Both work, but the inconsistency is notable — `sa.text()` is the recommended SQLAlchemy pattern for explicit textual SQL. **Suggestion:** Wrap all raw SQL in `sa.text()` for consistency, or use `op.execute(sa.text(...))` uniformly. --- ### L4 — Defensive Code: to_domain() casts root_plan_id as `str | None` despite NOT NULL column **File:** `models.py:905` ```python root_plan_id=cast("str | None", self.root_plan_id), ``` Since `root_plan_id` is now `NOT NULL` in the schema, the cast should be `cast(str, self.root_plan_id)` for type accuracy (consistent with `plan_id` on line 903). The `str | None` cast is overly defensive and sends a misleading signal to type checkers and future readers. **Suggestion:** Change to `cast(str, self.root_plan_id)`. --- ### L5 — Test Coverage: Robot tests cover only 8 of 18 BDD scenarios **Files:** `robot/plans_table_schema_alignment.robot` vs `features/plans_table_schema_alignment.feature` The Robot Framework test suite covers 8 scenarios while the BDD suite covers 18. Missing Robot scenarios include: bare-string/truncated-JSON/invalid-provenance automation_profile deserialization, snapshot update persistence, NULL snapshot rejection, `from_domain()` self-reference resolution, and invalid JSON rejection by the Plan model constructor. **Suggestion:** If the project's testing policy requires Robot parity with BDD, consider adding the missing scenarios. Otherwise, document that Robot tests cover the critical happy paths only. --- ### L6 — Naming Convention: Private constant shared across modules **Files:** `models.py:81` and `repositories.py:89` `_DEFAULT_AUTOMATION_PROFILE` uses a single underscore prefix (Python convention for module-private), but is imported directly in `repositories.py`. This works but sends a misleading signal about the constant's intended visibility. **Suggestion:** Rename to `DEFAULT_AUTOMATION_PROFILE` (no underscore) since it is a public cross-module constant, or document the intentional import of a private name. --- ### L7 — Observability: Corrupted snapshot fallback logs WARNING on every read **File:** `models.py:869-875` When `effective_profile_snapshot` contains corrupted JSON, `to_domain()` logs a `WARNING` and falls back to `"{}"` on **every read**. The corrupted value is not fixed in the database, so the warning fires repeatedly until the plan is updated via `update()` (which overwrites the value with the in-memory `"{}"`). For frequently-read plans, this could produce significant log noise. **Suggestion:** Consider either (a) logging only once per plan_id using a seen-set or LRU cache, or (b) documenting that the self-healing occurs on the next `update()` call. --- ### L8 — Migration: Revision naming gap (m4_003 → m8_001) **File:** `alembic/versions/m8_001_align_plans_schema.py:26-27` The revision jumps from `m4_003_plan_env_columns` to `m8_001_align_plans_schema`. If intermediate revisions (m5-m7) exist in other branches, this could cause Alembic multiple-heads conflicts when merging. **Suggestion:** Verify that the naming gap is intentional and that no conflicting revisions exist in other active branches. If naming corresponds to milestones (m4 = milestone 4, m8 = milestone 8), document this convention. --- ### Summary Table | ID | Severity | Category | File | Line(s) | |----|----------|----------|------|---------| | M1 | Medium | Bug / Data Integrity | `m8_001_align_plans_schema.py` | 74-89 | | M2 | Medium | Test Coverage | `m8_001_align_plans_schema.py` | all | | L1 | Low | Maintainability | `repositories.py` / `models.py` | 1340-1344 / 989-993 | | L2 | Low | Test Coverage | `plan.py` | 774-784 | | L3 | Low | Code Style | `m8_001_align_plans_schema.py` | 52-95 | | L4 | Low | Defensive Code | `models.py` | 905 | | L5 | Low | Test Coverage | Robot vs BDD suites | — | | L6 | Low | Naming Convention | `models.py` / `repositories.py` | 81 / 89 | | L7 | Low | Observability | `models.py` | 869-875 | | L8 | Low | Migration | `m8_001_align_plans_schema.py` | 26-27 | **No security or performance issues found.** The code follows good defensive practices (length-only logging, parameterized queries, exception hardening).

CoreRasurae force-pushed fix/plans-table-schema-alignment from cfc6702ad3 to 6f5cc4d649 2026-03-30 20:00:03 +00:00 Compare

CoreRasurae reviewed 2026-03-30 20:08:35 +00:00

CoreRasurae left a comment

Copy link

Code Review Report — PR #1074 (fix/plans-table-schema-alignment)

Reviewer: Automated code review (multi-cycle analysis)
Scope: All changes in branch fix/plans-table-schema-alignment (commit 6f5cc4d) plus close connections to surrounding code.
Cycles completed: 2 full global passes across all categories (bugs, security, performance, test coverage).
Feature reference: Issue #921

---

Overall Assessment

The implementation is solid and well-documented. The commit correctly addresses all acceptance criteria from issue #921: effective_profile_snapshot column added, root_plan_id and automation_profile made NOT NULL, migration with proper backfill, defensive deserialization, and comprehensive BDD/Robot tests. The code demonstrates careful thought about edge cases (cycle detection, orphan handling, corrupted JSON fallback).

The findings below are organized by severity and category. No critical issues were found.

---

MEDIUM Severity

M1 — Bug/Logic: Domain-persistence asymmetry for root_plan_id

Files: plan.py:288, models.py:1008-1012, models.py:918

PlanIdentity.root_plan_id is typed as str | None with default None, but the DB column is now NOT NULL. The from_domain() method silently resolves None → plan_id, and to_domain() reads it back as a non-null str.

Consequence: plan.identity.root_plan_id is None gives different answers before vs. after persistence for root plans. The is_root_plan property (plan.py:891) handles both cases (checks is None OR self-reference), but any code outside this PR that checks root_plan_id is None directly — instead of using is_root_plan — will behave differently pre/post persistence.

Suggestion: Consider either (a) making root_plan_id non-optional in PlanIdentity and resolving the self-reference at domain construction time (e.g., a model_validator that sets root_plan_id = plan_id when None), or (b) documenting this asymmetry as a known contract on PlanIdentity.

---

M2 — Bug/Logic: Migration orphan fallback produces semantically misleading root_plan_id

File: m8_001_align_plans_schema.py:89-97

Orphaned children — rows where parent_plan_id references a non-existent plan — get root_plan_id = plan_id as a fallback. After migration, these rows look like root plans (root_plan_id == plan_id) but still have parent_plan_id pointing to a missing row. The is_root_plan property would return True for these, contradicting parent_plan_id is not None.

Risk: Hierarchy traversal or reporting code that assumes is_root_plan == True means parent_plan_id is None could produce incorrect results.

Suggestion: The WARNING log is good. Consider additionally logging at WARNING level which plan_ids are affected and why (orphan vs. cycle), so DBAs can manually reconcile these rows post-migration.

---

M3 — Test Coverage: Alembic migration SQL never exercised

Files: All test files use Base.metadata.create_all() (e.g., plans_table_schema_alignment_steps.py:72)

The 18 BDD scenarios and 8 Robot tests all create the schema via Base.metadata.create_all(), which builds the table directly from ORM model definitions. The actual migration SQL — the backfill algorithm, the level-by-level propagation, the cycle detection, the orphan fallback, the batch_alter_table — is never executed by any test.

Risk: SQL syntax errors, incorrect correlated subqueries, or logic bugs in the migration would not be caught by these tests. The migration is particularly complex (level-by-level propagation with cycle detection), making untested SQL a meaningful risk.

Suggestion: Add at least one integration-level test that:

1. Creates a database at the previous migration revision (m4_003_plan_env_columns)
2. Inserts rows with NULL root_plan_id, NULL automation_profile, no effective_profile_snapshot
3. Runs upgrade() and verifies the backfilled values
4. Runs downgrade() and verifies the schema reverts cleanly

---

LOW Severity

L1 — Consistency: _serialize_automation_profile uses truthiness instead of explicit None check

File: models.py:766

The commit message (point #7) states: "Used explicit None check (is not None) instead of truthiness for root_plan_id resolution in from_domain()". However, _serialize_automation_profile() uses if profile_ref: rather than if profile_ref is not None:. Functionally correct (a valid AutomationProfileRef is always truthy), but inconsistent with the stated principle.

---

L2 — Consistency: Migration logs plan_id values despite "length-only" disclosure policy

File: m8_001_align_plans_schema.py:84

The cycle-detection error logs actual plan_id values (cycle_ids). The commit message states that error messages use "length-only to avoid potential information disclosure." ULIDs are not typically sensitive, but the inconsistency in logging policy is worth noting.

---

L3 — Test Coverage: No test for RecursionError in effective_profile_snapshot validator

File: plan.py:780

The field_validator catches RecursionError for deeply nested JSON, but no test exercises this code path. Triggering RecursionError in json.loads() requires extreme nesting (~1000+ levels in CPython), making this impractical to test deterministically. Consider at minimum a comment in the test file documenting why this path is not tested.

---

L4 — Test Coverage: Migration backfill edge cases untested

File: m8_001_align_plans_schema.py:60-97

No tests cover: (a) orphaned children whose parent_plan_id references a non-existent plan, (b) cycle detection in parent_plan_id chains (for-else exhaustion path), (c) empty-string automation_profile backfill. These are the most complex code paths in the migration and also the most likely to contain subtle bugs. (Partially overlaps with M3.)

---

L5 — Test Coverage: No test for empty-string effective_profile_snapshot

File: plan.py:774-784

The validator rejects invalid JSON via json.loads(), and json.loads("") raises JSONDecodeError. However, no explicit test covers the empty-string case. The existing test uses "not-valid-json" which covers a different invalid-JSON path.

---

L6 — Performance: Double json.loads() for effective_profile_snapshot in read path

Files: models.py:880, plan.py:779

to_domain() calls json.loads(raw_snapshot) to validate the snapshot (models.py:880), then the Plan constructor's field_validator calls json.loads(v) again (plan.py:779). For small snapshots ("{}") this is negligible. For large snapshots, the redundant parse adds unnecessary overhead. This is a minor optimization opportunity, not a blocking issue.

---

L7 — Type Safety: _serialize_automation_profile parameter type is Any

File: models.py:758

The method parameter type is Any instead of AutomationProfileRef | None. This follows the existing codebase pattern of using Any for domain types in the infrastructure layer, but is less type-safe. The commit message (point #11) improves type safety in test helpers but not in this new method.

---

L8 — Robustness: No application-level size limit on effective_profile_snapshot

File: plan.py:774-784

The field validator checks for valid JSON but does not enforce a maximum size. A large JSON blob could be stored without constraint. The DB engine would eventually bound it, but there is no explicit application-level limit. Consider adding a max_length parameter or a size check in the validator if this field is expected to remain bounded.

---

Summary Table

ID	Severity	Category	File(s)	Description
M1	Medium	Bug/Logic	plan.py, models.py	root_plan_id domain/persistence asymmetry — None before persist, non-null after
M2	Medium	Bug/Logic	migration .py	Orphan fallback sets root_plan_id = plan_id, making orphans look like root plans
M3	Medium	Test Coverage	all test files	Alembic migration SQL (backfill, cycle detection, orphan fallback) never tested
L1	Low	Consistency	models.py:766	_serialize_automation_profile uses truthiness, not explicit None check
L2	Low	Consistency	migration .py:84	Logs plan_id values, inconsistent with "length-only" policy
L3	Low	Test Coverage	plan.py:780	No test for RecursionError in snapshot validator
L4	Low	Test Coverage	migration .py	Migration backfill edge cases (orphans, cycles, empty strings) untested
L5	Low	Test Coverage	plan.py	No test for empty-string effective_profile_snapshot
L6	Low	Performance	models.py, plan.py	Double json.loads() on snapshot in read path
L7	Low	Type Safety	models.py:758	_serialize_automation_profile parameter typed as Any
L8	Low	Robustness	plan.py	No size limit on effective_profile_snapshot

Bottom line: The implementation is well-crafted with thorough defensive coding and good test coverage for the happy paths. The main actionable item is M3 — adding at least one test that exercises the actual Alembic migration SQL rather than only the ORM model schema. M1 is a design-level concern that could be addressed now or deferred as documented technical debt.

## Code Review Report — PR #1074 (`fix/plans-table-schema-alignment`) **Reviewer:** Automated code review (multi-cycle analysis) **Scope:** All changes in branch `fix/plans-table-schema-alignment` (commit `6f5cc4d`) plus close connections to surrounding code. **Cycles completed:** 2 full global passes across all categories (bugs, security, performance, test coverage). **Feature reference:** Issue #921 --- ### Overall Assessment The implementation is solid and well-documented. The commit correctly addresses all acceptance criteria from issue #921: `effective_profile_snapshot` column added, `root_plan_id` and `automation_profile` made NOT NULL, migration with proper backfill, defensive deserialization, and comprehensive BDD/Robot tests. The code demonstrates careful thought about edge cases (cycle detection, orphan handling, corrupted JSON fallback). The findings below are organized by severity and category. No critical issues were found. --- ## MEDIUM Severity ### M1 — Bug/Logic: Domain-persistence asymmetry for `root_plan_id` **Files:** `plan.py:288`, `models.py:1008-1012`, `models.py:918` `PlanIdentity.root_plan_id` is typed as `str | None` with default `None`, but the DB column is now NOT NULL. The `from_domain()` method silently resolves `None` → `plan_id`, and `to_domain()` reads it back as a non-null `str`. **Consequence:** `plan.identity.root_plan_id is None` gives **different answers** before vs. after persistence for root plans. The `is_root_plan` property (`plan.py:891`) handles both cases (checks `is None` OR self-reference), but any code outside this PR that checks `root_plan_id is None` directly — instead of using `is_root_plan` — will behave differently pre/post persistence. **Suggestion:** Consider either (a) making `root_plan_id` non-optional in `PlanIdentity` and resolving the self-reference at domain construction time (e.g., a `model_validator` that sets `root_plan_id = plan_id` when None), or (b) documenting this asymmetry as a known contract on `PlanIdentity`. --- ### M2 — Bug/Logic: Migration orphan fallback produces semantically misleading `root_plan_id` **File:** `m8_001_align_plans_schema.py:89-97` Orphaned children — rows where `parent_plan_id` references a non-existent plan — get `root_plan_id = plan_id` as a fallback. After migration, these rows look like root plans (`root_plan_id == plan_id`) but still have `parent_plan_id` pointing to a missing row. The `is_root_plan` property would return `True` for these, contradicting `parent_plan_id is not None`. **Risk:** Hierarchy traversal or reporting code that assumes `is_root_plan == True` means `parent_plan_id is None` could produce incorrect results. **Suggestion:** The WARNING log is good. Consider additionally logging at WARNING level which plan_ids are affected and why (orphan vs. cycle), so DBAs can manually reconcile these rows post-migration. --- ### M3 — Test Coverage: Alembic migration SQL never exercised **Files:** All test files use `Base.metadata.create_all()` (e.g., `plans_table_schema_alignment_steps.py:72`) The 18 BDD scenarios and 8 Robot tests all create the schema via `Base.metadata.create_all()`, which builds the table directly from ORM model definitions. The actual migration SQL — the backfill algorithm, the level-by-level propagation, the cycle detection, the orphan fallback, the batch_alter_table — is **never executed** by any test. **Risk:** SQL syntax errors, incorrect correlated subqueries, or logic bugs in the migration would not be caught by these tests. The migration is particularly complex (level-by-level propagation with cycle detection), making untested SQL a meaningful risk. **Suggestion:** Add at least one integration-level test that: 1. Creates a database at the previous migration revision (`m4_003_plan_env_columns`) 2. Inserts rows with NULL `root_plan_id`, NULL `automation_profile`, no `effective_profile_snapshot` 3. Runs `upgrade()` and verifies the backfilled values 4. Runs `downgrade()` and verifies the schema reverts cleanly --- ## LOW Severity ### L1 — Consistency: `_serialize_automation_profile` uses truthiness instead of explicit None check **File:** `models.py:766` The commit message (point #7) states: *"Used explicit None check (is not None) instead of truthiness for root_plan_id resolution in from_domain()"*. However, `_serialize_automation_profile()` uses `if profile_ref:` rather than `if profile_ref is not None:`. Functionally correct (a valid `AutomationProfileRef` is always truthy), but inconsistent with the stated principle. --- ### L2 — Consistency: Migration logs plan_id values despite "length-only" disclosure policy **File:** `m8_001_align_plans_schema.py:84` The cycle-detection error logs actual `plan_id` values (`cycle_ids`). The commit message states that error messages use "length-only to avoid potential information disclosure." ULIDs are not typically sensitive, but the inconsistency in logging policy is worth noting. --- ### L3 — Test Coverage: No test for `RecursionError` in `effective_profile_snapshot` validator **File:** `plan.py:780` The `field_validator` catches `RecursionError` for deeply nested JSON, but no test exercises this code path. Triggering `RecursionError` in `json.loads()` requires extreme nesting (~1000+ levels in CPython), making this impractical to test deterministically. Consider at minimum a comment in the test file documenting why this path is not tested. --- ### L4 — Test Coverage: Migration backfill edge cases untested **File:** `m8_001_align_plans_schema.py:60-97` No tests cover: (a) orphaned children whose `parent_plan_id` references a non-existent plan, (b) cycle detection in `parent_plan_id` chains (for-else exhaustion path), (c) empty-string `automation_profile` backfill. These are the most complex code paths in the migration and also the most likely to contain subtle bugs. (Partially overlaps with M3.) --- ### L5 — Test Coverage: No test for empty-string `effective_profile_snapshot` **File:** `plan.py:774-784` The validator rejects invalid JSON via `json.loads()`, and `json.loads("")` raises `JSONDecodeError`. However, no explicit test covers the empty-string case. The existing test uses `"not-valid-json"` which covers a different invalid-JSON path. --- ### L6 — Performance: Double `json.loads()` for `effective_profile_snapshot` in read path **Files:** `models.py:880`, `plan.py:779` `to_domain()` calls `json.loads(raw_snapshot)` to validate the snapshot (models.py:880), then the `Plan` constructor's `field_validator` calls `json.loads(v)` again (plan.py:779). For small snapshots (`"{}"`) this is negligible. For large snapshots, the redundant parse adds unnecessary overhead. This is a minor optimization opportunity, not a blocking issue. --- ### L7 — Type Safety: `_serialize_automation_profile` parameter type is `Any` **File:** `models.py:758` The method parameter type is `Any` instead of `AutomationProfileRef | None`. This follows the existing codebase pattern of using `Any` for domain types in the infrastructure layer, but is less type-safe. The commit message (point #11) improves type safety in test helpers but not in this new method. --- ### L8 — Robustness: No application-level size limit on `effective_profile_snapshot` **File:** `plan.py:774-784` The field validator checks for valid JSON but does not enforce a maximum size. A large JSON blob could be stored without constraint. The DB engine would eventually bound it, but there is no explicit application-level limit. Consider adding a `max_length` parameter or a size check in the validator if this field is expected to remain bounded. --- ## Summary Table | ID | Severity | Category | File(s) | Description | |----|----------|----------|---------|-------------| | M1 | Medium | Bug/Logic | `plan.py`, `models.py` | `root_plan_id` domain/persistence asymmetry — None before persist, non-null after | | M2 | Medium | Bug/Logic | migration `.py` | Orphan fallback sets `root_plan_id = plan_id`, making orphans look like root plans | | M3 | Medium | Test Coverage | all test files | Alembic migration SQL (backfill, cycle detection, orphan fallback) never tested | | L1 | Low | Consistency | `models.py:766` | `_serialize_automation_profile` uses truthiness, not explicit None check | | L2 | Low | Consistency | migration `.py:84` | Logs plan_id values, inconsistent with "length-only" policy | | L3 | Low | Test Coverage | `plan.py:780` | No test for RecursionError in snapshot validator | | L4 | Low | Test Coverage | migration `.py` | Migration backfill edge cases (orphans, cycles, empty strings) untested | | L5 | Low | Test Coverage | `plan.py` | No test for empty-string effective_profile_snapshot | | L6 | Low | Performance | `models.py`, `plan.py` | Double json.loads() on snapshot in read path | | L7 | Low | Type Safety | `models.py:758` | `_serialize_automation_profile` parameter typed as `Any` | | L8 | Low | Robustness | `plan.py` | No size limit on effective_profile_snapshot | **Bottom line:** The implementation is well-crafted with thorough defensive coding and good test coverage for the happy paths. The main actionable item is **M3** — adding at least one test that exercises the actual Alembic migration SQL rather than only the ORM model schema. **M1** is a design-level concern that could be addressed now or deferred as documented technical debt.

CoreRasurae commented 2026-03-30 21:19:52 +00:00

Author

Member

Copy link

Review Findings — Resolution Summary

Following the automated code review posted above, each finding was validated against the specification DDL (docs/specification.md lines 45463–45489), issue #921 acceptance criteria, and CONTRIBUTING.md guidelines. Three fixes were applied; the remaining findings were evaluated and determined not applicable for the reasons below.

Fixes Applied (commit amended)

ID	Fix	Detail
M1	PlanIdentity model_validator resolves None root_plan_id to plan_id	The spec DDL says root_plan_id TEXT NOT NULL. Previously, PlanIdentity.root_plan_id allowed None and resolution only happened in from_domain(), creating a domain/persistence asymmetry. Now resolved at domain construction time. from_domain() simplified accordingly.
L1	_serialize_automation_profile now uses is not None	Aligned with the explicit-None-check convention stated in the commit's own principle #7.
L5	Added BDD scenario for empty-string effective_profile_snapshot	Validates that the Pydantic field_validator correctly rejects "" via json.JSONDecodeError.

Findings Not Applied (with justification)

ID	Finding	Reason Not Applied
M2	Orphan fallback sets root_plan_id = plan_id	The migration handles a data corruption edge case not covered by the spec DDL or issue acceptance criteria. The existing WARNING log is adequate. Orphan data reconciliation is a DBA responsibility.
M3 / L4	Alembic migration SQL never tested	Valid observation, but testing actual migration SQL (creating a DB at a prior revision, running upgrade/downgrade) is a separate concern from schema alignment. The 19 BDD + 8 Robot tests comprehensively cover ORM model and domain behavior. Recommend tracking as a separate follow-up issue.
L2	Migration logs plan_id values	Migration logs are operational DBA logs, not user-facing messages. The "length-only" policy applies to validator error messages that could expose user data, not to ULID logging in administrative operations.
L3	No RecursionError test for snapshot validator	Impractical to test deterministically — json.loads() requires ~1000+ nesting levels to trigger RecursionError in CPython. The except clause is defense-in-depth.
L6	Double json.loads() in read path	Architecturally sound — ORM layer validates before constructing domain objects; the domain model has its own independent validator. Removing the redundancy would couple layers.
L7	_serialize_automation_profile parameter typed Any	Follows the established codebase pattern to avoid circular imports between infrastructure and domain layers.
L8	No size limit on effective_profile_snapshot	The spec DDL does not specify a size constraint. Per CONTRIBUTING.md: "Treat the project specification as the authoritative source of truth." Adding an arbitrary limit would deviate from spec.

Verification

All quality gates pass after the fixes:

Check	Result
nox -s lint	PASS
nox -s typecheck	PASS (0 errors, 1 pre-existing warning)
nox -s unit_tests	PASS (12,280 scenarios, 0 failures)
nox -s integration_tests	PASS (1,698 tests, 0 failures)

## Review Findings — Resolution Summary Following the automated code review posted above, each finding was validated against the specification DDL (`docs/specification.md` lines 45463–45489), issue #921 acceptance criteria, and `CONTRIBUTING.md` guidelines. Three fixes were applied; the remaining findings were evaluated and determined not applicable for the reasons below. ### Fixes Applied (commit amended) | ID | Fix | Detail | |----|-----|--------| | **M1** | `PlanIdentity` model_validator resolves `None` root_plan_id to `plan_id` | The spec DDL says `root_plan_id TEXT NOT NULL`. Previously, `PlanIdentity.root_plan_id` allowed `None` and resolution only happened in `from_domain()`, creating a domain/persistence asymmetry. Now resolved at domain construction time. `from_domain()` simplified accordingly. | | **L1** | `_serialize_automation_profile` now uses `is not None` | Aligned with the explicit-None-check convention stated in the commit's own principle #7. | | **L5** | Added BDD scenario for empty-string `effective_profile_snapshot` | Validates that the Pydantic `field_validator` correctly rejects `""` via `json.JSONDecodeError`. | ### Findings Not Applied (with justification) | ID | Finding | Reason Not Applied | |----|---------|-------------------| | **M2** | Orphan fallback sets `root_plan_id = plan_id` | The migration handles a data corruption edge case not covered by the spec DDL or issue acceptance criteria. The existing WARNING log is adequate. Orphan data reconciliation is a DBA responsibility. | | **M3** / **L4** | Alembic migration SQL never tested | Valid observation, but testing actual migration SQL (creating a DB at a prior revision, running upgrade/downgrade) is a separate concern from schema alignment. The 19 BDD + 8 Robot tests comprehensively cover ORM model and domain behavior. Recommend tracking as a separate follow-up issue. | | **L2** | Migration logs plan_id values | Migration logs are operational DBA logs, not user-facing messages. The "length-only" policy applies to validator error messages that could expose user data, not to ULID logging in administrative operations. | | **L3** | No RecursionError test for snapshot validator | Impractical to test deterministically — `json.loads()` requires ~1000+ nesting levels to trigger `RecursionError` in CPython. The `except` clause is defense-in-depth. | | **L6** | Double `json.loads()` in read path | Architecturally sound — ORM layer validates before constructing domain objects; the domain model has its own independent validator. Removing the redundancy would couple layers. | | **L7** | `_serialize_automation_profile` parameter typed `Any` | Follows the established codebase pattern to avoid circular imports between infrastructure and domain layers. | | **L8** | No size limit on `effective_profile_snapshot` | The spec DDL does not specify a size constraint. Per `CONTRIBUTING.md`: "Treat the project specification as the authoritative source of truth." Adding an arbitrary limit would deviate from spec. | ### Verification All quality gates pass after the fixes: | Check | Result | |-------|--------| | `nox -s lint` | PASS | | `nox -s typecheck` | PASS (0 errors, 1 pre-existing warning) | | `nox -s unit_tests` | PASS (12,280 scenarios, 0 failures) | | `nox -s integration_tests` | PASS (1,698 tests, 0 failures) |

CoreRasurae force-pushed fix/plans-table-schema-alignment from 6f5cc4d649 to ae235c557a 2026-03-30 21:33:01 +00:00 Compare

CoreRasurae reviewed 2026-03-30 21:38:54 +00:00

CoreRasurae left a comment

Copy link

Code Review Report — PR #1074 (Issue #921)

Branch: fix/plans-table-schema-alignment
Commit: ae235c55 — fix(db): align v3_plans schema with specification DDL
Reviewer: Automated code review (3 full cycles across all categories)

---

Overall Assessment

Well-structured commit that aligns the v3_plans schema with the specification DDL. The migration is carefully implemented with level-by-level backfill, cycle detection, and thorough documentation of intentional deviations. Defensive deserialization and information-disclosure-aware logging are commendable. The following findings are organized by severity and category.

---

MEDIUM Severity

M1 — [Performance] Double JSON parsing of effective_profile_snapshot on every plan read

Files: models.py:879-880, plan.py:793-794

to_domain() calls json.loads(raw_snapshot) to validate the snapshot (line 880), discards the parsed result, then passes the raw string to the Plan domain constructor where the validate_effective_profile_snapshot_json field_validator calls json.loads(v) again (line 794). This means every plan read pays the cost of parsing the snapshot JSON twice.

Suggestion: Consider caching the validation result from to_domain() or restructuring so the validation only happens once. For example, to_domain() could set a pre-validated flag, or the field_validator could be skipped when the value is known to be validated already. Alternatively, since to_domain() already falls back to '{}' on error, the domain validator is guaranteed to receive valid JSON — so the redundant parse in to_domain() could be removed and the fallback logic moved entirely to the domain validator.

M2 — [Spec Compliance / Functional Gap] effective_profile_snapshot is never populated with an actual profile snapshot

Files: plan.py:727-738, models.py:1033

The effective_profile_snapshot field defaults to '{}' everywhere (domain model, ORM column, migration server_default, all test helpers). The spec intent (line 45472) is to store a "frozen JSON snapshot of the automation profile at creation" for audit and reproducibility. The docstring at plan.py:727-732 acknowledges this gap: "New plans SHOULD explicitly set this field..."

Currently no code path actually populates the snapshot with a meaningful profile value. This means the acceptance criterion "effective_profile_snapshot column added (TEXT NOT NULL, stores frozen JSON of automation profile at plan creation)" is only partially met — the column exists and is NOT NULL, but nothing stores the frozen profile.

Suggestion: This is likely intended for a follow-up PR, but it would be worth tracking with a separate issue to avoid it being forgotten.

M3 — [Bug / API Consistency] PlanIdentity.root_plan_id type annotation is str | None but post-construction value is always str

Files: plan.py:288-312

The field is declared as root_plan_id: str | None = Field(default=None, ...) but the model_validator(mode="after") at line 299 always resolves None to self.plan_id. After construction, root_plan_id is never None. This creates a misleading API contract — consumers see str | None in type hints and IDE autocompletion, leading to unnecessary None checks.

Suggestion: Consider narrowing the type annotation or adding a docstring note clarifying that the value is always non-None after construction. An Annotated pattern or a property accessor returning str could make the post-validation guarantee explicit.

---

LOW Severity

L1 — [Dead Code] is_root_plan property has unreachable root_plan_id is None branch

File: plan.py:902-908

@property
def is_root_plan(self) -> bool:
    return (
        self.identity.root_plan_id is None          # <-- unreachable
        or self.identity.root_plan_id == self.identity.plan_id
    )

Since PlanIdentity.model_validator always resolves None to plan_id, root_plan_id is never None after construction. The is None branch is dead code.

Suggestion: Simplify to return self.identity.root_plan_id == self.identity.plan_id and add a comment explaining why the None check was removed.

L2 — [Test Coverage] No test for Alembic migration downgrade path

File: alembic/versions/m8_001_align_plans_schema.py:132-147

The downgrade() function reverses the schema changes (makes columns nullable, drops effective_profile_snapshot). There is no test that exercises the downgrade path. While the commit documents that "backfill is not reversible", the structural downgrade itself (column nullability + drop) is untested.

L3 — [Test Coverage] No test for migration cycle detection logic

File: alembic/versions/m8_001_align_plans_schema.py:76-87

The migration includes cycle-detection logic (for/else clause with _MAX_DEPTH=100). This is an important safety guard, but no test exercises the code path where parent_plan_id forms a cycle and the backfill does not converge. This is admittedly hard to test at the BDD level.

L4 — [Test Coverage] No test for RecursionError in JSON parsing paths

Files: plan.py:795, models.py:858,881

RecursionError was added to exception lists for deeply nested JSON, which is good defensive coding. However, no test actually triggers this path with a deeply nested JSON payload.

L5 — [Test Coverage] No test for migration backfill of empty-string automation_profile

File: alembic/versions/m8_001_align_plans_schema.py:100-104

The migration backfills automation_profile = 'balanced' where automation_profile = ''. This branch is not tested by any BDD or Robot scenario. The empty-string case is a distinct edge case from the NULL case.

L6 — [Defensive Code] TypeError in effective_profile_snapshot field_validator is unreachable

File: plan.py:795

except (json.JSONDecodeError, TypeError, RecursionError) as exc:

json.loads() on a str input cannot raise TypeError. Since the field is typed as str (not str | None), Pydantic enforces the string type before the field_validator runs. TypeError is unreachable. Not harmful (pure defensive coding), but adds noise to the exception list.

L7 — [Performance / Logging] Migration cycle-detection logs all affected plan_id values in one message

File: alembic/versions/m8_001_align_plans_schema.py:82-86

If cycle detection triggers on a database with thousands of orphaned plans, cycle_ids could be a very large list, producing an oversized log message. This is an edge case that would only manifest in severely corrupted databases.

Suggestion: Consider truncating the logged list (e.g., first 50 IDs + count) with a message like "showing first 50 of N affected plan_ids".

---

INFORMATIONAL

I1 — Intentional phase default deviation is well-documented

The spec DDL defaults phase to 'strategize' (line 45468) while the code defaults to 'action'. This is intentional and thoroughly documented in the LifecyclePlanModel docstring (models.py:626-630) and the commit message. No action needed.

I2 — Column naming conventions are well-documented

The deviations (automation_profile vs automation_profile_name, processing_state vs state, v3_plans vs plans, etc.) are all documented in the model docstring (models.py:578-597). No action needed.

I3 — FK ondelete policy drift between ORM and migration is documented

The ORM model specifies ondelete="RESTRICT" for root_plan_id but the migration cannot update FK policies in SQLite batch mode. This drift is documented in the migration comments (lines 110-119). No action needed.

---

Summary: 3 medium, 7 low, 3 informational findings. The commit is well-implemented with thorough documentation. The main actionable items are the double-JSON-parse performance overhead (M1), tracking the snapshot population gap (M2), and the type annotation inconsistency (M3).

# Code Review Report — PR #1074 (Issue #921) **Branch:** `fix/plans-table-schema-alignment` **Commit:** `ae235c55` — `fix(db): align v3_plans schema with specification DDL` **Reviewer:** Automated code review (3 full cycles across all categories) --- ## Overall Assessment Well-structured commit that aligns the `v3_plans` schema with the specification DDL. The migration is carefully implemented with level-by-level backfill, cycle detection, and thorough documentation of intentional deviations. Defensive deserialization and information-disclosure-aware logging are commendable. The following findings are organized by severity and category. --- ## MEDIUM Severity ### M1 — [Performance] Double JSON parsing of `effective_profile_snapshot` on every plan read **Files:** `models.py:879-880`, `plan.py:793-794` `to_domain()` calls `json.loads(raw_snapshot)` to validate the snapshot (line 880), discards the parsed result, then passes the raw string to the `Plan` domain constructor where the `validate_effective_profile_snapshot_json` field_validator calls `json.loads(v)` again (line 794). This means every plan read pays the cost of parsing the snapshot JSON twice. **Suggestion:** Consider caching the validation result from `to_domain()` or restructuring so the validation only happens once. For example, `to_domain()` could set a pre-validated flag, or the field_validator could be skipped when the value is known to be validated already. Alternatively, since `to_domain()` already falls back to `'{}'` on error, the domain validator is guaranteed to receive valid JSON — so the redundant parse in `to_domain()` could be removed and the fallback logic moved entirely to the domain validator. ### M2 — [Spec Compliance / Functional Gap] `effective_profile_snapshot` is never populated with an actual profile snapshot **Files:** `plan.py:727-738`, `models.py:1033` The `effective_profile_snapshot` field defaults to `'{}'` everywhere (domain model, ORM column, migration server_default, all test helpers). The spec intent (line 45472) is to store a *"frozen JSON snapshot of the automation profile at creation"* for audit and reproducibility. The docstring at `plan.py:727-732` acknowledges this gap: *"New plans SHOULD explicitly set this field..."* Currently no code path actually populates the snapshot with a meaningful profile value. This means the acceptance criterion *"effective_profile_snapshot column added (TEXT NOT NULL, stores frozen JSON of automation profile at plan creation)"* is only partially met — the column exists and is NOT NULL, but nothing stores the frozen profile. **Suggestion:** This is likely intended for a follow-up PR, but it would be worth tracking with a separate issue to avoid it being forgotten. ### M3 — [Bug / API Consistency] `PlanIdentity.root_plan_id` type annotation is `str | None` but post-construction value is always `str` **Files:** `plan.py:288-312` The field is declared as `root_plan_id: str | None = Field(default=None, ...)` but the `model_validator(mode="after")` at line 299 always resolves `None` to `self.plan_id`. After construction, `root_plan_id` is never `None`. This creates a misleading API contract — consumers see `str | None` in type hints and IDE autocompletion, leading to unnecessary `None` checks. **Suggestion:** Consider narrowing the type annotation or adding a docstring note clarifying that the value is always non-None after construction. An `Annotated` pattern or a property accessor returning `str` could make the post-validation guarantee explicit. --- ## LOW Severity ### L1 — [Dead Code] `is_root_plan` property has unreachable `root_plan_id is None` branch **File:** `plan.py:902-908` ```python @property def is_root_plan(self) -> bool: return ( self.identity.root_plan_id is None # <-- unreachable or self.identity.root_plan_id == self.identity.plan_id ) ``` Since `PlanIdentity.model_validator` always resolves `None` to `plan_id`, `root_plan_id` is never `None` after construction. The `is None` branch is dead code. **Suggestion:** Simplify to `return self.identity.root_plan_id == self.identity.plan_id` and add a comment explaining why the None check was removed. ### L2 — [Test Coverage] No test for Alembic migration downgrade path **File:** `alembic/versions/m8_001_align_plans_schema.py:132-147` The `downgrade()` function reverses the schema changes (makes columns nullable, drops `effective_profile_snapshot`). There is no test that exercises the downgrade path. While the commit documents that *"backfill is not reversible"*, the structural downgrade itself (column nullability + drop) is untested. ### L3 — [Test Coverage] No test for migration cycle detection logic **File:** `alembic/versions/m8_001_align_plans_schema.py:76-87` The migration includes cycle-detection logic (`for/else` clause with `_MAX_DEPTH=100`). This is an important safety guard, but no test exercises the code path where `parent_plan_id` forms a cycle and the backfill does not converge. This is admittedly hard to test at the BDD level. ### L4 — [Test Coverage] No test for `RecursionError` in JSON parsing paths **Files:** `plan.py:795`, `models.py:858,881` `RecursionError` was added to exception lists for deeply nested JSON, which is good defensive coding. However, no test actually triggers this path with a deeply nested JSON payload. ### L5 — [Test Coverage] No test for migration backfill of empty-string `automation_profile` **File:** `alembic/versions/m8_001_align_plans_schema.py:100-104` The migration backfills `automation_profile = 'balanced'` where `automation_profile = ''`. This branch is not tested by any BDD or Robot scenario. The empty-string case is a distinct edge case from the NULL case. ### L6 — [Defensive Code] `TypeError` in `effective_profile_snapshot` field_validator is unreachable **File:** `plan.py:795` ```python except (json.JSONDecodeError, TypeError, RecursionError) as exc: ``` `json.loads()` on a `str` input cannot raise `TypeError`. Since the field is typed as `str` (not `str | None`), Pydantic enforces the string type before the field_validator runs. `TypeError` is unreachable. Not harmful (pure defensive coding), but adds noise to the exception list. ### L7 — [Performance / Logging] Migration cycle-detection logs all affected `plan_id` values in one message **File:** `alembic/versions/m8_001_align_plans_schema.py:82-86` If cycle detection triggers on a database with thousands of orphaned plans, `cycle_ids` could be a very large list, producing an oversized log message. This is an edge case that would only manifest in severely corrupted databases. **Suggestion:** Consider truncating the logged list (e.g., first 50 IDs + count) with a message like `"showing first 50 of N affected plan_ids"`. --- ## INFORMATIONAL ### I1 — Intentional `phase` default deviation is well-documented The spec DDL defaults `phase` to `'strategize'` (line 45468) while the code defaults to `'action'`. This is intentional and thoroughly documented in the `LifecyclePlanModel` docstring (models.py:626-630) and the commit message. No action needed. ### I2 — Column naming conventions are well-documented The deviations (`automation_profile` vs `automation_profile_name`, `processing_state` vs `state`, `v3_plans` vs `plans`, etc.) are all documented in the model docstring (models.py:578-597). No action needed. ### I3 — FK `ondelete` policy drift between ORM and migration is documented The ORM model specifies `ondelete="RESTRICT"` for `root_plan_id` but the migration cannot update FK policies in SQLite batch mode. This drift is documented in the migration comments (lines 110-119). No action needed. --- **Summary:** 3 medium, 7 low, 3 informational findings. The commit is well-implemented with thorough documentation. The main actionable items are the double-JSON-parse performance overhead (M1), tracking the snapshot population gap (M2), and the type annotation inconsistency (M3).

CoreRasurae force-pushed fix/plans-table-schema-alignment from ae235c557a to 2b5847c57d 2026-03-30 22:04:02 +00:00 Compare

CoreRasurae force-pushed fix/plans-table-schema-alignment from 2b5847c57d to a04591440e 2026-03-30 22:18:04 +00:00 Compare

CoreRasurae force-pushed fix/plans-table-schema-alignment from a04591440e to 34cae6c1f3 2026-03-30 22:39:18 +00:00 Compare

CoreRasurae force-pushed fix/plans-table-schema-alignment from 34cae6c1f3 to a4a6b061a6 2026-03-30 22:40:47 +00:00 Compare

CoreRasurae scheduled this pull request to auto merge when all checks succeed 2026-03-30 22:41:07 +00:00

CoreRasurae merged commit a4a6b061a6 into master 2026-03-30 23:01:20 +00:00

CoreRasurae deleted branch fix/plans-table-schema-alignment 2026-03-30 23:01:20 +00:00

Sign in to join this conversation.

Reviewers

No reviewers

brent.edwards

hamza.khyari

Labels

Clear labels   

auto/needs-reevaluation

Controller deferred this PR; awaiting Phase 6+ scope-evaluator or operator re-enablement.

  

controller-managed

Auto-agents controller manages this PR/issue (see tools/controller/deploy/RUNBOOK.md). Remove this label to abandon controller management.

  

auto/blocked-by-deps

PR blocked by an open issue dependency. Operator must close the dep (or remove the dependency link) before the merge driver can act. Auto-cleared by merge_drive when no open deps remain.

  

auto/ci-timeout

Most recent merge cycle hit CI timeout. Driver excludes this PR while last merge_cycle row is < 30 min old; label persists thereafter as visible history.

  

auto/claimed-implementer

Currently being processed by an implementer worker.

  

auto/claimed-merge

Currently being processed by the merge driver.

  

auto/claimed-reviewer

Currently being processed by a reviewer worker.

  

auto/driver-down

Merge driver heartbeat stale; pipeline halted. Closed automatically on next clean tick.

  

auto/invariant-violation

Detected master commit violating the strict merge invariant. Tracked as an issue (not a PR label); kept here for label completeness.

  

auto/last-attempt-tier-0

In-cycle escalation: most recent attempt ran at the Tier 0 slot (`tier-0`). Slot's model defined in .opencode/models/tiers.yaml.

  

auto/last-attempt-tier-1

In-cycle escalation: most recent attempt ran at the Tier 1 slot (`tier-1`). Slot's model defined in .opencode/models/tiers.yaml.

  

auto/last-attempt-tier-2

In-cycle escalation: most recent attempt ran at the Tier 2 slot (`tier-2`). Slot's model defined in .opencode/models/tiers.yaml. Gated behind IMPLEMENTER_ESCALATION_TIER2_ENABLED.

  

auto/last-attempt-tier-min

In-cycle escalation: most recent attempt ran at the Tier -1 slot (`tier-min`). Slot's model defined in .opencode/models/tiers.yaml. Suffix is ``-min`` (not ``--1``) so the Forgejo UI reads naturally.

  

Automation Tracking

Tracking issues used by the AI Automation system for agents to communicate and report.

  

auto/needs-conflict-resolution

Rebase conflict needs LLM conflict-resolver.

  

auto/needs-implementer

Failing CI needs implementer attention.

  

auto/postmortem

Documenting a driver incident or rollback.

  

auto/ready-to-merge

Reviewer has APPROVED this PR and no later REQUEST_CHANGES is outstanding. The merge driver requires this label to even consider a PR for merging. Set by the reviewer worker on APPROVE; cleared on REQUEST_CHANGES.

  

auto/restart-throttled

Train repeatedly lost master-tempo races. Driver excludes via merge_cycle until cooldown elapses; label persists as visible history.

  

auto/revert

Revert PR backing out an invariant violation. Fast-tracked through the merge driver.

  

auto/sentinel

Sentinel PR duplicated from upstream into a personal fork by tools/duplicate_prs_to_fork.py for pipeline testing. Lives only in the fork; the canonical pipeline never sees it.

  

auto/stale-inactivity

No implementer activity for N days. Flagged for human review. Auto-cleared on next push to head branch.

  

auto/unstable

Repeatedly fails on current master (>= 3 ci-fail-on-rebased-sha releases in 12 h). Excluded from driver until human triage.

  

Blocked

A ticket in a blocked state and unable to complete until some other task is completed first.

  

Bounty

$100

A bounty of $100 for any open-source contributor who provides a MR that solves this issue

  

Bounty

$1000

A bounty of $1000 for any open-source contributor who provides a MR that solves this issue

  

Bounty

$10000

A bounty of $10000 for any open-source contributor who provides a MR that solves this issue

  

Bounty

$20

A bounty of $20 for any open-source contributor who provides a MR that solves this issue

  

Bounty

$2000

A bounty of $2000 for any open-source contributor who provides a MR that solves this issue

  

Bounty

$250

A bounty of $250 for any open-source contributor who provides a MR that solves this issue

  

Bounty

$50

A bounty of $50 for any open-source contributor who provides a MR that solves this issue

  

Bounty

$500

A bounty of $500 for any open-source contributor who provides a MR that solves this issue

  

Bounty

$5000

A bounty of $5000 for any open-source contributor who provides a MR that solves this issue

  

Bounty

$750

A bounty of $750 for any open-source contributor who provides a MR that solves this issue

  

MoSCoW

Could have

Could have feature in order to satisfy the epic/legendary.

  

MoSCoW

Must have

Must have feature in order to satisfy the epic/legendary.

  

MoSCoW

Should have

Should have feature in order to satisfy the epic/legendary.

  

Needs Feedback

There are questions in the ticket that can not be completed until the project owner provides clarity.

  

Points

1

1 man-hours worth of work for an expert with no learning curve.

  

Points

13

13 man-hours worth of work for an expert with no learning curve.

  

Points

2

2 man-hours worth of work for an expert with no learning curve.

  

Points

21

21 man-hours worth of work for an expert with no learning curve.

  

Points

3

3 man-hours worth of work for an expert with no learning curve.

  

Points

34

34 man-hours worth of work for an expert with no learning curve.

  

Points

5

5 man-hours worth of work for an expert with no learning curve.

  

Points

55

55 man-hours worth of work for an expert with no learning curve.

  

Points

8

8 man-hours worth of work for an expert with no learning curve.

  

Points

88

88 man-hours worth of work for an expert with no learning curve.

  

Priority

Backlog

This ticket has backlogged priority and is not to be worked on yet

  

Priority

CI Blocker

Critical priority issue that blocks CI/CD pipeline and prevents PR merges

  

Priority

Critical

The priority is critical

  

Priority

High

The priority is high

  

Priority

Low

The priority is low

  

Priority

Medium

The priority is medium

  

Signed-off: Owner

When an epic or legendary is in review it must be signed off by owner, tech lead, and scrum master before being marked as completed.

  

Signed-off: Scrum Master

When an epic or legendary is in review it must be signed off by owner, tech lead, and scrum master before being marked as completed.

  

Signed-off: Tech Lead

When an epic or legendary is in review it must be signed off by owner, tech lead, and scrum master before being marked as completed.

  

Spike

A ticket for learning a tool or technology that is needed to be able to do future planning and design.

  

State

Completed

The ticket has been fully implemented, completed, and merged with the source code. This label should only be applied once a ticket is closed.

  

State

Duplicate

A ticket that represents the same content as an existing ticket.

  

State

In Progress

A ticket that is actively being developed.

  

State

In Review

A ticket that has had some code completed to implement but is waiting to pass peer review and is not yet merged in.

  

State

Paused

This ticket's work started but wasn't finished. It's on hold (likely in a feature branch) and will be resumed later, either due to a blocker or a delay.

  

State

Unverified

All new tickets start in this state. A developer may set it to show the ticket is unverified. This means we haven't agreed to work on it. It will either move to a verified state or be closed as wontdo.

  

State

Verified

The issue has been verified by a developer as legitimate. It will be worked on and verified tickets are now considered part of the backlog.

  

State

Wont Do

This ticket has been decided it wont be done. This may mean the bug has been determined to not be real (cant verify) or the feature is one we have decided we dont want to adopt.

  

Type

Automation

Any edits or discussion about the AI automated coding system.

  

Type

Bug

Something that doesnt work as intended.

  

Type

Discussion

Anytime a ticket represents a discussion about a subject and doesnt fall into one of the other categories.

  

Type

Documentation

An error or improvement needed in the documentation.

  

Type

Epic

Any first tier epic. That is, an epic which contains only issues as children and will not have sub-epics.

  

Type

Feature

Some new functionality not present.

  

Type

Legendary

A type of Epic which will contain other Epics.

  

Type

Refactor

A code change that restructures existing code without changing its external behavior.

  

Type

Support

Someone needs help using the project.

  

Type

Task

A generic task that doesnt fit into the other type categories.

  

Type

Testing

Work exclusively focusing on fixing or expanding testing.

No labels

auto/needs-reevaluation

controller-managed

auto/blocked-by-deps

auto/ci-timeout

auto/claimed-implementer

auto/claimed-merge

auto/claimed-reviewer

auto/driver-down

auto/invariant-violation

auto/last-attempt-tier-0

auto/last-attempt-tier-1

auto/last-attempt-tier-2

auto/last-attempt-tier-min

Automation Tracking

auto/needs-conflict-resolution

auto/needs-implementer

auto/postmortem

auto/ready-to-merge

auto/restart-throttled

auto/revert

auto/sentinel

auto/stale-inactivity

auto/unstable

Blocked

Bounty

$100

Bounty

$1000

Bounty

$10000

Bounty

$20

Bounty

$2000

Bounty

$250

Bounty

$50

Bounty

$500

Bounty

$5000

Bounty

$750

MoSCoW

Could have

MoSCoW

Must have

MoSCoW

Should have

Needs Feedback

Points

1

Points

13

Points

2

Points

21

Points

3

Points

34

Points

5

Points

55

Points

8

Points

88

Priority

Backlog

Priority

CI Blocker

Priority

Critical

Priority

High

Priority

Low

Priority

Medium

Signed-off: Owner

Signed-off: Scrum Master

Signed-off: Tech Lead

Spike

State

Completed

State

Duplicate

State

In Progress

State

In Review

State

Paused

State

Unverified

State

Verified

State

Wont Do

Type

Automation

Type

Bug

Type

Discussion

Type

Documentation

Type

Epic

Type

Feature

Type

Legendary

Type

Refactor

Type

Support

Type

Task

Type

Testing

Milestone

Clear milestone

No items

No milestone

v3.4.0

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

2 participants

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

cleveragents/cleveragents-core!1074

No description provided.