Compare commits
2 Commits
| Author | SHA1 | Date | |
|---|---|---|---|
| 414daf0122 | |||
| 460e99cb70 |
@@ -3,8 +3,6 @@ name: CI
|
||||
on:
|
||||
push:
|
||||
branches: [master, develop]
|
||||
pull_request:
|
||||
branches: [master, develop]
|
||||
|
||||
vars:
|
||||
docker_prefix: "http://harbor.cleverthis.com/docker/"
|
||||
|
||||
@@ -77,6 +77,14 @@ The format follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/).
|
||||
This wires the previously-isolated `discover_devcontainers()` function into the production
|
||||
code path, enabling the spec's zero-configuration devcontainer experience.
|
||||
|
||||
### Fixed
|
||||
|
||||
- **Data integrity fix: ValidationAttachmentRepository argument swap** (#7492): Fixed
|
||||
a critical data integrity issue in `ValidationAttachmentRepository.attach` where
|
||||
`validation_name` and `resource_id` arguments were being silently swapped based on a
|
||||
fragile heuristic (`"/" in resource_id`). Arguments are now passed in the correct order,
|
||||
ensuring data is stored with proper parameter values.
|
||||
|
||||
### Changed
|
||||
|
||||
- **`agents session list` now displays full 26-character session ULIDs** (#10970): The Rich table
|
||||
|
||||
@@ -31,6 +31,7 @@ Below are some of the specific details of various contributions.
|
||||
* HAL 9000 has contributed the PlanResult.success derivation fix (PR #8214 / issue #7501): replaced the incorrect `error_message is None` heuristic with a dedicated `result_success` column in the plans table, ensuring plans with historical build errors are not incorrectly marked as failed after a successful apply.
|
||||
* HAL 9000 has contributed comprehensive milestone documentation for v3.6.0 (Advanced Concepts & Deferred Features) and v3.7.0 (TUI Implementation) (PR #9903): split into sub-documents covering context strategies, LLM backends, resource types, A2A rename, container tool execution, scope chain resolution, cost/safety budgets, E2E workflow tests, code review examples, plugin architecture, TUI layout, persona system, reference/command input, session management, configuration, and TuiMaterializer integration.
|
||||
* HAL 9000 has contributed the LLMTraceRepository data-integrity fix (PR #8185 / issue #7505): replaced the unconditional `session.commit()` in `LLMTraceRepository.save()` with a dual-path implementation that respects the UnitOfWork pattern — flushing only when an external session is provided, and flushing + committing + closing when operating standalone. This eliminates premature transaction commits, loss of rollback capability, and a docstring/implementation mismatch.
|
||||
* HAL 9000 has contributed the ValidationAttachmentRepository data-integrity fix (PR #8177 / issue #7492): removed the silent argument swap heuristic (`"/" in resource_id`) from `ValidationAttachmentRepository.attach()` that caused validation names and resource IDs to be stored with reversed values, preventing silent data corruption when resource IDs contain slashes. Added comprehensive BDD regression tests covering all boundary conditions.
|
||||
* HAL 9000 has contributed the ACMS Index Data Model and File Traversal Engine (PR #9664 / issue #9579): foundational data structures for indexed context entries with hot/warm/cold/archive storage tier classification, tag system, and a timeout-safe chunked file traversal engine for large projects with 10,000+ files.
|
||||
|
||||
* HAL 9000 has contributed the error-suppression removal fix (PR #9247 / issue #9060): removed both `try...except Exception:` blocks in `register_registry_agents()` that silently suppressed errors from `actor_registry.list_actors()` and the route bridge refresh, enabling exceptions to propagate per CONTRIBUTING.md fail-fast policy. Added three Behave scenarios verifying RuntimeError, AttributeError, and TypeError propagation.
|
||||
@@ -0,0 +1,276 @@
|
||||
"""Step definitions for TDD issue #7492: ValidationAttachmentRepository.attach argument swap bug.
|
||||
|
||||
Tests that the attach() method preserves argument order and does not silently
|
||||
swap validation_name and resource_id when resource_id contains a slash.
|
||||
"""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
import json
|
||||
|
||||
from behave import given, then, when
|
||||
from behave.runner import Context
|
||||
from sqlalchemy import create_engine
|
||||
from sqlalchemy.orm import Session, sessionmaker
|
||||
|
||||
from cleveragents.infrastructure.database.models import Base
|
||||
from cleveragents.infrastructure.database.repositories import (
|
||||
ValidationAttachmentRepository,
|
||||
)
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Background
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
|
||||
@given("a validation attachment repository test environment")
|
||||
def step_test_environment(context: Context) -> None:
|
||||
"""Set up the test environment for validation attachment tests."""
|
||||
# Create an in-memory SQLite database for testing
|
||||
context.engine = create_engine("sqlite:///:memory:")
|
||||
Base.metadata.create_all(context.engine)
|
||||
context.SessionLocal = sessionmaker(bind=context.engine)
|
||||
|
||||
|
||||
@given("a test database for validation attachments")
|
||||
def step_test_database(context: Context) -> None:
|
||||
"""Ensure the test database is ready."""
|
||||
# Database is already created in the previous step
|
||||
pass
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Given: Repository instance
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
|
||||
@given("a validation attachment repository instance")
|
||||
def step_repository_instance(context: Context) -> None:
|
||||
"""Create a ValidationAttachmentRepository instance."""
|
||||
|
||||
def session_factory() -> Session:
|
||||
return context.SessionLocal()
|
||||
|
||||
context.repository = ValidationAttachmentRepository(session_factory)
|
||||
context.last_attachment = None
|
||||
context.last_error = None
|
||||
|
||||
|
||||
@given(
|
||||
'an existing attachment with validation_name="{validation_name}" and resource_id="{resource_id}"'
|
||||
)
|
||||
def step_existing_attachment(
|
||||
context: Context, validation_name: str, resource_id: str
|
||||
) -> None:
|
||||
"""Create an existing attachment in the database."""
|
||||
try:
|
||||
context.repository.attach(
|
||||
validation_name=validation_name,
|
||||
resource_id=resource_id,
|
||||
)
|
||||
except Exception as exc:
|
||||
context.last_error = exc
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# When: Call attach method
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
|
||||
@when("simple-attach validation {validation_name} on resource {resource_id}")
|
||||
def step_call_attach_basic(
|
||||
context: Context, validation_name: str, resource_id: str
|
||||
) -> None:
|
||||
"""Call attach with basic parameters."""
|
||||
try:
|
||||
context.last_attachment = context.repository.attach(
|
||||
validation_name=validation_name,
|
||||
resource_id=resource_id,
|
||||
)
|
||||
context.last_error = None
|
||||
except Exception as exc:
|
||||
context.last_attachment = None
|
||||
context.last_error = exc
|
||||
|
||||
|
||||
@when(
|
||||
"scope-attach validation {validation_name} on resource {resource_id} with project {project_name} and plan {plan_id}"
|
||||
)
|
||||
def step_call_attach_with_scope(
|
||||
context: Context,
|
||||
validation_name: str,
|
||||
resource_id: str,
|
||||
project_name: str,
|
||||
plan_id: str,
|
||||
) -> None:
|
||||
"""Call attach with project_name and plan_id."""
|
||||
try:
|
||||
context.last_attachment = context.repository.attach(
|
||||
validation_name=validation_name,
|
||||
resource_id=resource_id,
|
||||
project_name=project_name,
|
||||
plan_id=plan_id,
|
||||
)
|
||||
context.last_error = None
|
||||
except Exception as exc:
|
||||
context.last_attachment = None
|
||||
context.last_error = exc
|
||||
|
||||
|
||||
@when(
|
||||
"mode-attach validation {validation_name} on resource {resource_id} with mode {mode}"
|
||||
)
|
||||
def step_call_attach_with_mode(
|
||||
context: Context, validation_name: str, resource_id: str, mode: str
|
||||
) -> None:
|
||||
"""Call attach with mode parameter."""
|
||||
try:
|
||||
context.last_attachment = context.repository.attach(
|
||||
validation_name=validation_name,
|
||||
resource_id=resource_id,
|
||||
mode=mode,
|
||||
)
|
||||
context.last_error = None
|
||||
except Exception as exc:
|
||||
context.last_attachment = None
|
||||
context.last_error = exc
|
||||
|
||||
|
||||
@when(
|
||||
"json-attach validation {validation_name} on resource {resource_id} with args {args_json}"
|
||||
)
|
||||
def step_call_attach_with_args(
|
||||
context: Context, validation_name: str, resource_id: str, args_json: str
|
||||
) -> None:
|
||||
"""Call attach with args parameter."""
|
||||
try:
|
||||
args = json.loads(args_json)
|
||||
context.last_attachment = context.repository.attach(
|
||||
validation_name=validation_name,
|
||||
resource_id=resource_id,
|
||||
args=args,
|
||||
)
|
||||
context.last_error = None
|
||||
except Exception as exc:
|
||||
context.last_attachment = None
|
||||
context.last_error = exc
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Then: Verify attachment properties
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
|
||||
@then('the attachment should have validation_name="{expected_value}"')
|
||||
def step_verify_validation_name(context: Context, expected_value: str) -> None:
|
||||
"""Verify the validation_name field."""
|
||||
assert context.last_attachment is not None, "No attachment was created"
|
||||
actual_value = context.last_attachment.get("validation_name")
|
||||
assert actual_value == expected_value, (
|
||||
f"Expected validation_name={expected_value}, got {actual_value}"
|
||||
)
|
||||
|
||||
|
||||
@then('the attachment should have resource_id="{expected_value}"')
|
||||
def step_verify_resource_id(context: Context, expected_value: str) -> None:
|
||||
"""Verify the resource_id field."""
|
||||
assert context.last_attachment is not None, "No attachment was created"
|
||||
actual_value = context.last_attachment.get("resource_id")
|
||||
assert actual_value == expected_value, (
|
||||
f"Expected resource_id={expected_value}, got {actual_value}"
|
||||
)
|
||||
|
||||
|
||||
@then('the attachment should have project_name="{expected_value}"')
|
||||
def step_verify_project_name(context: Context, expected_value: str) -> None:
|
||||
"""Verify the project_name field."""
|
||||
assert context.last_attachment is not None, "No attachment was created"
|
||||
actual_value = context.last_attachment.get("project_name")
|
||||
assert actual_value == expected_value, (
|
||||
f"Expected project_name={expected_value}, got {actual_value}"
|
||||
)
|
||||
|
||||
|
||||
@then('the attachment should have plan_id="{expected_value}"')
|
||||
def step_verify_plan_id(context: Context, expected_value: str) -> None:
|
||||
"""Verify the plan_id field."""
|
||||
assert context.last_attachment is not None, "No attachment was created"
|
||||
actual_value = context.last_attachment.get("plan_id")
|
||||
assert actual_value == expected_value, (
|
||||
f"Expected plan_id={expected_value}, got {actual_value}"
|
||||
)
|
||||
|
||||
|
||||
@then('the attachment should have mode="{expected_value}"')
|
||||
def step_verify_mode(context: Context, expected_value: str) -> None:
|
||||
"""Verify the mode field."""
|
||||
assert context.last_attachment is not None, "No attachment was created"
|
||||
actual_value = context.last_attachment.get("mode")
|
||||
assert actual_value == expected_value, (
|
||||
f"Expected mode={expected_value}, got {actual_value}"
|
||||
)
|
||||
|
||||
|
||||
@then('the attachment should NOT have validation_name="{unexpected_value}"')
|
||||
def step_verify_not_validation_name(context: Context, unexpected_value: str) -> None:
|
||||
"""Verify the validation_name field is NOT a specific value."""
|
||||
assert context.last_attachment is not None, "No attachment was created"
|
||||
actual_value = context.last_attachment.get("validation_name")
|
||||
assert actual_value != unexpected_value, (
|
||||
f"validation_name should NOT be {unexpected_value}, but it is"
|
||||
)
|
||||
|
||||
|
||||
@then('the attachment should NOT have resource_id="{unexpected_value}"')
|
||||
def step_verify_not_resource_id(context: Context, unexpected_value: str) -> None:
|
||||
"""Verify the resource_id field is NOT a specific value."""
|
||||
assert context.last_attachment is not None, "No attachment was created"
|
||||
actual_value = context.last_attachment.get("resource_id")
|
||||
assert actual_value != unexpected_value, (
|
||||
f"resource_id should NOT be {unexpected_value}, but it is"
|
||||
)
|
||||
|
||||
|
||||
@then('the attachment args should contain "{key}"')
|
||||
def step_verify_args_contains_key(context: Context, key: str) -> None:
|
||||
"""Verify the args JSON contains a specific key."""
|
||||
assert context.last_attachment is not None, "No attachment was created"
|
||||
args_json = context.last_attachment.get("args_json")
|
||||
assert args_json is not None, "args_json is None"
|
||||
args = json.loads(args_json)
|
||||
assert key in args, f"args should contain key '{key}', but got {args}"
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Then: Verify result structure
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
|
||||
@then("the result should be a dict")
|
||||
def step_verify_result_is_dict(context: Context) -> None:
|
||||
"""Verify the result is a dictionary."""
|
||||
assert isinstance(context.last_attachment, dict), (
|
||||
f"Expected dict, got {type(context.last_attachment)}"
|
||||
)
|
||||
|
||||
|
||||
@then('the result should have key "{key}"')
|
||||
def step_verify_result_has_key(context: Context, key: str) -> None:
|
||||
"""Verify the result dict has a specific key."""
|
||||
assert context.last_attachment is not None, "No attachment was created"
|
||||
assert key in context.last_attachment, f"Result should have key '{key}'"
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Then: Verify exceptions
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
|
||||
@then("a DuplicateValidationAttachmentError should be raised")
|
||||
def step_verify_duplicate_error(context: Context) -> None:
|
||||
"""Verify that a DuplicateValidationAttachmentError was raised."""
|
||||
assert context.last_error is not None, "Expected an error to be raised"
|
||||
error_name = type(context.last_error).__name__
|
||||
assert error_name == "DuplicateValidationAttachmentError", (
|
||||
f"Expected DuplicateValidationAttachmentError, got {error_name}"
|
||||
)
|
||||
@@ -0,0 +1,116 @@
|
||||
Feature: ValidationAttachmentRepository.attach does not swap arguments
|
||||
As a CleverAgents developer
|
||||
I want the ValidationAttachmentRepository.attach method to preserve argument order
|
||||
So that validation names and resource IDs are stored correctly in the database
|
||||
|
||||
Background:
|
||||
Given a validation attachment repository test environment
|
||||
And a test database for validation attachments
|
||||
|
||||
# --- TDD red phase: prove the bug existed ---
|
||||
|
||||
@tdd_issue
|
||||
@tdd_issue_7492
|
||||
Scenario: Bug: attach silently swaps arguments when resource_id contains slash
|
||||
Given a validation attachment repository instance
|
||||
When simple-attach validation "my-validator" on resource "ns/resource"
|
||||
Then the attachment should have validation_name="my-validator"
|
||||
And the attachment should have resource_id="ns/resource"
|
||||
And the attachment should NOT have validation_name="ns/resource"
|
||||
And the attachment should NOT have resource_id="my-validator"
|
||||
|
||||
# --- TDD green phase: fix the bug ---
|
||||
|
||||
@tdd_issue
|
||||
@tdd_issue_7492
|
||||
Scenario: attach preserves arguments when resource_id contains slash
|
||||
Given a validation attachment repository instance
|
||||
When simple-attach validation "my-validator" on resource "ns/resource"
|
||||
Then the attachment should have validation_name="my-validator"
|
||||
And the attachment should have resource_id="ns/resource"
|
||||
And the attachment should NOT have validation_name="ns/resource"
|
||||
And the attachment should NOT have resource_id="my-validator"
|
||||
|
||||
@tdd_issue
|
||||
@tdd_issue_7492
|
||||
Scenario: attach preserves arguments when both contain slashes
|
||||
Given a validation attachment repository instance
|
||||
When simple-attach validation "ns/my-validator" on resource "ns/resource"
|
||||
Then the attachment should have validation_name="ns/my-validator"
|
||||
And the attachment should have resource_id="ns/resource"
|
||||
|
||||
@tdd_issue
|
||||
@tdd_issue_7492
|
||||
Scenario: attach preserves arguments when neither contains slashes
|
||||
Given a validation attachment repository instance
|
||||
When simple-attach validation "my-validator" on resource "simple-resource"
|
||||
Then the attachment should have validation_name="my-validator"
|
||||
And the attachment should have resource_id="simple-resource"
|
||||
|
||||
@tdd_issue
|
||||
@tdd_issue_7492
|
||||
Scenario: attach preserves arguments when validation_name contains slash
|
||||
Given a validation attachment repository instance
|
||||
When simple-attach validation "ns/my-validator" on resource "simple-resource"
|
||||
Then the attachment should have validation_name="ns/my-validator"
|
||||
And the attachment should have resource_id="simple-resource"
|
||||
|
||||
# --- Edge cases ---
|
||||
|
||||
@tdd_issue
|
||||
@tdd_issue_7492
|
||||
Scenario: attach works with complex namespaced names
|
||||
Given a validation attachment repository instance
|
||||
When simple-attach validation "org/team/validator" on resource "project/resource/subresource"
|
||||
Then the attachment should have validation_name="org/team/validator"
|
||||
And the attachment should have resource_id="project/resource/subresource"
|
||||
|
||||
@tdd_issue
|
||||
@tdd_issue_7492
|
||||
Scenario: attach works with optional project_name and plan_id
|
||||
Given a validation attachment repository instance
|
||||
When scope-attach validation "my-validator" on resource "ns/resource" with project "my-project" and plan "01ARZ3NDEKTSV4RRFFQ69G5FAV"
|
||||
Then the attachment should have validation_name="my-validator"
|
||||
And the attachment should have resource_id="ns/resource"
|
||||
And the attachment should have project_name="my-project"
|
||||
And the attachment should have plan_id="01ARZ3NDEKTSV4RRFFQ69G5FAV"
|
||||
|
||||
@tdd_issue
|
||||
@tdd_issue_7492
|
||||
Scenario: attach works with mode parameter
|
||||
Given a validation attachment repository instance
|
||||
When mode-attach validation "my-validator" on resource "ns/resource" with mode "informational"
|
||||
Then the attachment should have validation_name="my-validator"
|
||||
And the attachment should have resource_id="ns/resource"
|
||||
And the attachment should have mode="informational"
|
||||
|
||||
@tdd_issue
|
||||
@tdd_issue_7492
|
||||
Scenario: attach works with args parameter
|
||||
Given a validation attachment repository instance
|
||||
When json-attach validation "my-validator" on resource "ns/resource" with args {"threshold": 0.8}
|
||||
Then the attachment should have validation_name="my-validator"
|
||||
And the attachment should have resource_id="ns/resource"
|
||||
And the attachment args should contain "threshold"
|
||||
|
||||
# --- Regression: ensure no silent behavior changes ---
|
||||
|
||||
@tdd_issue
|
||||
@tdd_issue_7492
|
||||
Scenario: attach returns correct attachment dict structure
|
||||
Given a validation attachment repository instance
|
||||
When simple-attach validation "my-validator" on resource "ns/resource"
|
||||
Then the result should be a dict
|
||||
And the result should have key "attachment_id"
|
||||
And the result should have key "validation_name"
|
||||
And the result should have key "resource_id"
|
||||
And the result should have key "mode"
|
||||
And the result should have key "created_at"
|
||||
|
||||
@tdd_issue
|
||||
@tdd_issue_7492
|
||||
Scenario: attach raises DuplicateValidationAttachmentError on duplicate
|
||||
Given a validation attachment repository instance
|
||||
And an existing attachment with validation_name="my-validator" and resource_id="ns/resource"
|
||||
When simple-attach validation "my-validator" on resource "ns/resource"
|
||||
Then a DuplicateValidationAttachmentError should be raised
|
||||
@@ -3915,9 +3915,6 @@ class ValidationAttachmentRepository:
|
||||
|
||||
from ulid import ULID as _ULID
|
||||
|
||||
if "/" in resource_id and "/" not in validation_name:
|
||||
validation_name, resource_id = resource_id, validation_name
|
||||
|
||||
session = self._session()
|
||||
try:
|
||||
# Check for existing attachment with same validation+resource+scope
|
||||
|
||||
Reference in New Issue
Block a user