forked from HAL9000/cleveragents-core
hurui200320
ee2024046f
## Summary
`agents plan use` crashed with `sqlite3.IntegrityError: UNIQUE constraint failed: action_arguments.action_name, action_arguments.name` when the action had arguments already registered via `action create`. The root cause was `ActionRepository.update()` using SQLAlchemy's relationship `.clear()` + `.append()` pattern, which deferred the DELETE and processed the INSERT first — triggering a UNIQUE constraint violation when the same `(action_name, name)` pair was being re-inserted.
## Approach
Replace the `.clear()` + `.append()` pattern with explicit bulk `sa_delete()` + `session.flush()` before re-inserting child rows for both `action_arguments` and `action_invariants`. After the flush, expire the relationship collections with `session.expire(row, ["arguments_rel", "invariants_rel"])` so SQLAlchemy reloads from the now-empty database state before appending replacements. This avoids stale identity map references and guarantees the DELETE is committed before any INSERT.
## Key Changes
### Bug fix (`src/cleveragents/infrastructure/database/repositories.py`)
- `ActionRepository.update()` now uses `sa_delete(ActionArgumentModel)` and `sa_delete(ActionInvariantModel)` with `synchronize_session=False`, followed by `session.flush()`, before re-inserting child rows.
- Targeted `session.expire(row, ["arguments_rel", "invariants_rel"])` replaces the removed `.clear()` calls to force collection reload.
### Schema parity (`src/cleveragents/infrastructure/database/models.py`)
- Added `UniqueConstraint("action_name", "position")` to `ActionInvariantModel`.
- Added `UniqueConstraint("action_name", "name")`, `CheckConstraint` for `arg_type`, and `CheckConstraint` for `requirement` to `ActionArgumentModel`.
### Alembic migration (`alembic/versions/a5_006_action_invariants_unique_constraint.py`)
- New migration adds all four constraints to both `action_invariants` and `action_arguments` tables.
- Includes deduplication guards and data normalization so the upgrade succeeds on existing databases with invalid or duplicate rows.
- Uses `batch_alter_table` for SQLite compatibility.
### Tests
- **Behave** (`features/plan_use_action_args_integrity.feature`): 6 scenarios covering the core bug path, zero-argument regression, multiple arguments, reusable action double-use, non-reusable action archival with invariants, and direct repository update.
- **Robot** (`robot/plan_use_action_args_integrity.robot`): Integration test mirroring the Behave scenarios via a helper script.
- **Shared factory** (`features/mocks/test_uow_factory.py`): Extracted `build_test_uow()` from both test suites into a single shared module to eliminate duplication (DRY).
### Minor
- Updated `src/cleveragents/domain/repositories/__init__.py` docstring from table format to bullet list (conflict resolution from rebase).
Closes #4174
Reviewed-on: cleveragents/cleveragents-core#4197
Reviewed-by: HAL 9000 <HAL9000@cleverthis.com>
Co-authored-by: Rui Hu <rui.hu@cleverthis.com>
Co-committed-by: Rui Hu <rui.hu@cleverthis.com>
72 lines
2.3 KiB
Python
72 lines
2.3 KiB
Python
"""Shared in-memory UnitOfWork factory for test isolation.
|
|
|
|
Provides ``build_test_uow`` — a factory that constructs an in-memory
|
|
SQLite-backed ``UnitOfWork`` suitable for both Behave step definitions
|
|
and Robot Framework integration test helpers.
|
|
|
|
Centralising the UoW construction eliminates duplication and ensures
|
|
both test suites exercise the same database setup. Any changes to
|
|
``UnitOfWork.__init__`` must be reflected here in a single place rather
|
|
than in each consuming test file.
|
|
|
|
Used by:
|
|
- ``features/steps/plan_use_action_args_integrity_steps.py``
|
|
- ``robot/helper_plan_use_action_args_integrity.py``
|
|
"""
|
|
|
|
from __future__ import annotations
|
|
|
|
from typing import Any
|
|
|
|
from sqlalchemy import create_engine, event
|
|
from sqlalchemy.engine import Engine
|
|
from sqlalchemy.orm import Session, sessionmaker
|
|
|
|
from cleveragents.infrastructure.database.models import Base
|
|
from cleveragents.infrastructure.database.unit_of_work import UnitOfWork
|
|
|
|
|
|
def build_test_uow() -> tuple[UnitOfWork, sessionmaker[Session], Engine]:
|
|
"""Build an in-memory UoW for testing.
|
|
|
|
Creates a fresh SQLite ``:memory:`` database with all tables, foreign
|
|
key enforcement enabled, and a ``UnitOfWork`` instance that bypasses
|
|
the constructor to avoid running migrations inside the test harness.
|
|
|
|
Returns:
|
|
A three-tuple of ``(uow, session_factory, engine)``.
|
|
"""
|
|
engine = create_engine(
|
|
"sqlite:///:memory:",
|
|
echo=False,
|
|
future=True,
|
|
connect_args={"check_same_thread": False},
|
|
)
|
|
|
|
@event.listens_for(engine, "connect")
|
|
def _enable_fk(dbapi_conn: Any, _rec: Any) -> None:
|
|
cursor = dbapi_conn.cursor()
|
|
cursor.execute("PRAGMA foreign_keys=ON")
|
|
cursor.close()
|
|
|
|
Base.metadata.create_all(engine)
|
|
sf: sessionmaker[Session] = sessionmaker(
|
|
bind=engine,
|
|
expire_on_commit=False,
|
|
autoflush=False,
|
|
autocommit=False,
|
|
class_=Session,
|
|
)
|
|
|
|
uow = UnitOfWork.__new__(UnitOfWork)
|
|
# Keep these attributes in sync with UnitOfWork.__init__ — we bypass
|
|
# the constructor to avoid running migrations inside the test harness.
|
|
uow.database_url = "sqlite:///:memory:"
|
|
uow._engine = engine
|
|
uow._session_factory = sf
|
|
uow._database_initialized = True
|
|
uow._prompt_for_migration = None
|
|
uow._require_confirmation = False
|
|
|
|
return uow, sf, engine
|