hurui200320
ee2024046f
CI / lint (push) Successful in 37s
CI / quality (push) Successful in 53s
CI / typecheck (push) Successful in 57s
CI / security (push) Successful in 58s
CI / helm (push) Successful in 25s
CI / push-validation (push) Successful in 25s
CI / build (push) Successful in 30s
CI / integration_tests (push) Successful in 4m22s
CI / e2e_tests (push) Successful in 4m17s
CI / unit_tests (push) Successful in 5m10s
CI / docker (push) Successful in 1m30s
CI / coverage (push) Successful in 12m18s
CI / status-check (push) Successful in 1s
CI / benchmark-regression (push) Has been skipped
CI / benchmark-publish (push) Has been cancelled
## 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: #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>
102 lines
5.1 KiB
Gherkin
102 lines
5.1 KiB
Gherkin
@domain @repository @action_repository
|
|
Feature: Plan use with action arguments does not crash on UNIQUE constraint
|
|
As a user running 'agents plan use' on an action with arguments
|
|
I want the plan to be created successfully without a database error
|
|
So that the full plan lifecycle works after action create + plan use
|
|
|
|
Background:
|
|
Given a persisted plan lifecycle service for action args integrity tests
|
|
|
|
# ---------------------------------------------------------------------------
|
|
# Core bug fix: plan use with arguments that already exist
|
|
# ---------------------------------------------------------------------------
|
|
|
|
@tdd_issue @tdd_issue_4174
|
|
Scenario: plan use preserves arguments registered via action create
|
|
Given an action "local/args-test" with arguments:
|
|
| name | type | requirement | default |
|
|
| my_arg | string | optional | hello |
|
|
| count | integer| required | |
|
|
When I use the action "local/args-test" to create a plan with arguments:
|
|
| name | value |
|
|
| count | 42 |
|
|
Then the plan should be created successfully in strategize phase
|
|
And the plan should have a valid ULID identifier
|
|
|
|
# ---------------------------------------------------------------------------
|
|
# Regression: actions with zero arguments still work
|
|
# ---------------------------------------------------------------------------
|
|
|
|
@tdd_issue @tdd_issue_4174
|
|
Scenario: plan use succeeds when the action has zero arguments
|
|
Given an action "local/no-args" with no arguments
|
|
When I use the action "local/no-args" to create a plan without arguments
|
|
Then the plan should be created successfully in strategize phase
|
|
And the plan should have a valid ULID identifier
|
|
|
|
# ---------------------------------------------------------------------------
|
|
# Regression: actions with multiple arguments work
|
|
# ---------------------------------------------------------------------------
|
|
|
|
@tdd_issue @tdd_issue_4174
|
|
Scenario: plan use succeeds when the action has multiple arguments
|
|
Given an action "local/multi-args" with arguments:
|
|
| name | type | requirement | default |
|
|
| target | string | required | |
|
|
| verbose | boolean | optional | true |
|
|
| threshold | float | optional | 0.8 |
|
|
When I use the action "local/multi-args" to create a plan with arguments:
|
|
| name | value |
|
|
| target | src/ |
|
|
Then the plan should be created successfully in strategize phase
|
|
|
|
# ---------------------------------------------------------------------------
|
|
# Regression: running plan use twice on the same action does not crash
|
|
# ---------------------------------------------------------------------------
|
|
|
|
@tdd_issue @tdd_issue_4174
|
|
Scenario: plan use twice on the same reusable action does not crash
|
|
Given an action "local/reuse-test" with arguments:
|
|
| name | type | requirement | default |
|
|
| my_arg | string | optional | world |
|
|
When I use the action "local/reuse-test" to create a plan without arguments
|
|
And I use the action "local/reuse-test" to create a second plan without arguments
|
|
Then both plans should be created successfully
|
|
|
|
# ---------------------------------------------------------------------------
|
|
# Core bug path: archive non-reusable action (triggers repository update)
|
|
# ---------------------------------------------------------------------------
|
|
|
|
@tdd_issue @tdd_issue_4174
|
|
Scenario: plan use archives a non-reusable action with arguments and invariants
|
|
Given invariants for the next action:
|
|
| text |
|
|
| Plans must log archival outcomes. |
|
|
And a non-reusable action "local/non-reuse" with arguments:
|
|
| name | type | requirement | default |
|
|
| payload | string | required | |
|
|
| retries | integer| optional | 3 |
|
|
When I use the action "local/non-reuse" to create a plan with arguments:
|
|
| name | value |
|
|
| payload | data |
|
|
Then the plan should be created successfully in strategize phase
|
|
And the action "local/non-reuse" should still have 2 argument(s)
|
|
And the action "local/non-reuse" should still have 1 invariant(s)
|
|
|
|
# ---------------------------------------------------------------------------
|
|
# ActionRepository.update upserts arguments without IntegrityError
|
|
# ---------------------------------------------------------------------------
|
|
|
|
@tdd_issue @tdd_issue_4174
|
|
Scenario: Updating an action with existing arguments does not raise IntegrityError
|
|
Given invariants for the next action:
|
|
| text |
|
|
| Persisted actions must remain valid. |
|
|
Given an action "local/update-args" with arguments:
|
|
| name | type | requirement | default |
|
|
| my_arg | string | optional | hello |
|
|
When the action "local/update-args" is updated through the repository
|
|
Then the repository should not raise an integrity error
|
|
And the action "local/update-args" should still have 1 argument(s)
|
|
And the action "local/update-args" should still have 1 invariant(s)
|