Compare commits

...

1 Commits

Author SHA1 Message Date
HAL9000 674f4e7339 fix(data-integrity): remove silent argument swap in ValidationAttachmentRepository.attach (#8177)
Fixed a critical data integrity bug where validation_name and resource_id
arguments were being silently swapped based on the fragile heuristic
('/' in resource_id and '/' not in validation_name). This caused silent
data corruption without any error or warning. The conditional swap block
has been removed, ensuring arguments flow directly from caller to the
persistence layer in their declared order. Added comprehensive BDD test
suite (11 scenarios) verifying argument preservation across all boundary
conditions including slash-containing IDs, namespacing, optional params,
and duplicate rejection.
2026-05-13 08:45:15 +00:00
5 changed files with 373 additions and 3 deletions
+2
View File
@@ -79,6 +79,8 @@ The format follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/).
removal. Mocked existing steps to allow remaining V2 features to be
covered/tested.
- **Data integrity fix: remove silent argument swap in ValidationAttachmentRepository.attach** (PR #8177): Fixed a critical data integrity bug where `validation_name` and `resource_id` arguments were being silently swapped based on the fragile heuristic `( "/" in resource_id and "/" not in validation_name )`. This caused silent data corruption without any error or warning. The 2-line conditional swap block has been removed, ensuring arguments flow directly from caller to the persistence layer in their declared order. Added comprehensive BDD test suite (11 scenarios) verifying argument preservation across all boundary conditions including slash-containing IDs, namespacing, optional parameters, and duplicate rejection.
- **TUI Prompt Symbol Mode Awareness** (#6431): The prompt widget now displays a
mode-dependent symbol (`` normal, `/` command, `$` shell, `` multi-line),
implemented via `_PromptSymbolMixin` and `InputMode.MULTILINE`. The widget uses
+1
View File
@@ -36,6 +36,7 @@ Below are some of the specific details of various contributions.
* 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 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 ValidationAttachmentRepository data-integrity fix (PR #8177 / issue #7492): removed the silent argument swap in `ValidationAttachmentRepository.attach()` where `validation_name` and `resource_id` were being swapped based on a fragile `/` heuristic, eliminating data corruption without any error. Added comprehensive BDD test suite (11 scenarios) verifying argument preservation across all boundary conditions including slash-containing IDs, namespacing, optional parameters, and duplicate rejection.
* 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.
* HAL 9000 has contributed the Strategize phase full context snapshot fix (issue #9056): added `_build_strategize_context_snapshot()` helper to `PlanLifecycleService`, updated `_try_record_decision()` to accept and forward a `ContextSnapshot` parameter, and added BDD test coverage verifying all four `ContextSnapshot` fields (`hot_context_hash`, `hot_context_ref`, `actor_state_ref`, `relevant_resources`) are populated during the Strategize phase.
* HAL 9000 has contributed the ACMS context path matching fix (PR #10975 / issue #10972): corrects `_path_matches()` and `_matches_pattern()` to properly match absolute fragment paths against relative glob patterns by auto-prefixing with `**/` before calling `PurePath.full_match()`, preventing silent inefficacy of include/exclude filters for absolute paths in fragment metadata.
@@ -0,0 +1,288 @@
"""Step definitions for TDD issue #7492: ValidationAttachmentRepository attach argument swap fix (PR #8177).
Tests that validation_name and resource_id are preserved in their correct order
without any silent heuristic-based swapping.
"""
from __future__ import annotations
import re
from behave import given, then, when
from behave.runner import Context
from sqlalchemy import create_engine
from sqlalchemy.orm import sessionmaker
from cleveragents.infrastructure.database.models import Base
from cleveragents.infrastructure.database.repositories import (
DuplicateValidationAttachmentError,
ValidationAttachmentRepository,
)
# ---------------------------------------------------------------------------
# Helpers
# ---------------------------------------------------------------------------
def _make_in_memory_repo(context: Context) -> None:
"""Set up an in-memory SQLite database with the validation attachment table."""
engine = create_engine("sqlite:///:memory:", echo=False)
Base.metadata.create_all(engine)
factory = sessionmaker(bind=engine, expire_on_commit=False)
context._va_engine = engine
# Keep a persistent session so all operations share the same connection
session = factory()
context._va_session = session
context.attachment_repo = ValidationAttachmentRepository(
session_factory=lambda: context._va_session,
)
# ---------------------------------------------------------------------------
# Given steps
# ---------------------------------------------------------------------------
@given("a database engine with validation_attachments table schema")
def step_given_db_engine(context: Context) -> None:
"""Create an in-memory SQLite DB with all CleverAgents tables."""
_make_in_memory_repo(context)
@given("a validation attachment repository with session factory")
def step_given_repo(context: Context) -> None:
"""Ensure the repo instance is available on context."""
if not hasattr(context, "attachment_repo"):
_make_in_memory_repo(context)
# ---------------------------------------------------------------------------
# Attach helper — shared logic for scenarios
# ---------------------------------------------------------------------------
def _attach_validation(
context: Context,
validation_name: str,
resource_id: str,
mode: str = "required",
project_name: str | None = None,
plan_id: str | None = None,
args: dict | None = None,
) -> dict:
"""Call attach() and store the result."""
if hasattr(context, "_attach_result"):
del context._attach_result
try:
result = context.attachment_repo.attach(
validation_name=validation_name,
resource_id=resource_id,
mode=mode,
project_name=project_name,
plan_id=plan_id,
args=args,
)
context._attach_result = result # type: ignore[attr-defined]
except Exception as exc:
context._attach_exception = exc # type: ignore[attr-defined]
# ---------------------------------------------------------------------------
# When steps
# ---------------------------------------------------------------------------
@when(
'I attach the validation to the resource in mode "{mode}"',
)
def step_when_attach_basic(context: Context, mode: str) -> None:
"""Attach with provided validation_name and resource_id."""
vn = getattr(context, "_vn", "unknown")
rid = getattr(context, "_rid", "")
_attach_validation(context, vn, rid, mode=mode)
@when(
'I attach the validation to the resource in mode "{mode}" with optional params',
)
def step_when_attach_with_optional_params(context: Context, mode: str = "required") -> None:
"""Attach with optional project_name and plan_id."""
vn = getattr(context, "_vn", "unknown")
rid = getattr(context, "_rid", "")
pn = getattr(context, "_pn", None)
pid = getattr(context, "_pid", None)
_attach_validation(context, vn, rid, mode=mode, project_name=pn, plan_id=pid)
@when("I attach the validation to the resource with optional args")
def step_when_attach_with_args(context: Context) -> None:
"""Attach including an args dict."""
vn = getattr(context, "_vn", "unknown")
rid = getattr(context, "_rid", "")
pn = getattr(context, "_pn", None)
pid = getattr(context, "_pid", None)
args = getattr(context, "_args", {})
_attach_validation(
context, vn, rid, mode="required", project_name=pn, plan_id=pid, args=args
)
@when("I attach the validation with all optional parameters")
def step_when_attach_all_optional(context: Context) -> None:
"""Attach with mode, project_name, plan_id, and args."""
vn = getattr(context, "_vn", "unknown")
rid = getattr(context, "_rid", "")
pn = getattr(context, "_pn", None)
pid = getattr(context, "_pid", None)
args = getattr(context, "_args", {})
mode = getattr(context, "_mode", "required")
_attach_validation(
context, vn, rid, mode=mode, project_name=pn, plan_id=pid, args=args
)
@when("I attach the validation to the resource successfully once")
def step_when_attach_first_time(context: Context) -> None:
"""First attachment — expected to succeed."""
_attach_validation(
context,
getattr(context, "_vn", ""),
getattr(context, "_rid", ""),
mode=getattr(context, "_mode", "required"),
project_name=getattr(context, "_pn", None),
plan_id=getattr(context, "_pid", None),
)
@when("I attempt a second attachment with the same parameters")
def step_when_attach_duplicate(context: Context) -> None:
"""Duplicate attachment — expected to raise DuplicateValidationAttachmentError."""
try:
_attach_validation(
context,
getattr(context, "_vn", ""),
getattr(context, "_rid", ""),
mode=getattr(context, "_mode", "required"),
project_name=getattr(context, "_pn", None),
plan_id=getattr(context, "_pid", None),
)
except DuplicateValidationAttachmentError as exc:
context._attach_exception = exc # type: ignore[attr-defined]
@when('I attach with project_name "{pn}" and plan_id "{pid}"')
def step_when_attach_project_plan(context: Context, pn: str, pid: str) -> None:
"""Attach with specific project_name and plan_id."""
_attach_validation(
context,
getattr(context, "_vn", ""),
getattr(context, "_rid", ""),
mode=getattr(context, "_mode", "required"),
project_name=pn,
plan_id=pid,
)
# ---------------------------------------------------------------------------
# Given steps for parameter setup
# ---------------------------------------------------------------------------
@given('a validation name "{vn}" and resource_id "{rid}"')
def step_given_params(context: Context, vn: str, rid: str) -> None:
"""Store the validation name and resource_id on context."""
context._vn = vn # type: ignore[attr-defined]
context._rid = rid # type: ignore[attr-defined]
@given('optional scope parameters project_name "{pn}" and plan_id "{pid}"')
def step_given_optional_scope(context: Context, pn: str, pid: str) -> None:
"""Set optional project name and plan id."""
context._pn = pn # type: ignore[attr-defined]
context._pid = pid # type: ignore[attr-defined]
@given(
'an args dict with keys "{key1}" (value "{val1}") and "{key2}" (value {val2})',
)
def step_given_args_dict_string(context: Context, key1: str, val1: str, key2: str, val2: int | str) -> None:
"""Create a simple args dict from two keys."""
context._args = {key1: val1, key2: val2} # type: ignore[attr-defined]
@given(
'an args dict with key "{key}" (value {val}), project_name "{pn}", plan_id "{pid}", mode "{mode}"',
)
def step_given_all_optional_params(
context: Context, key: str, val: float, pn: str, pid: str, mode: str
) -> None:
"""Set all optional parameters."""
context._args = {key: val} # type: ignore[attr-defined]
context._pn = pn # type: ignore[attr-defined]
context._pid = pid # type: ignore[attr-defined]
context._mode = mode # type: ignore[attr-defined]
# ---------------------------------------------------------------------------
# Then steps
# ---------------------------------------------------------------------------
@then(
'the attachment should have validation_name exactly "{exp_vn}" and resource_id exactly "{exp_rid}"',
)
def step_then_preserve_args(context: Context, exp_vn: str, exp_rid: str) -> None:
"""Verify arguments are stored in correct order — not swapped."""
assert (
hasattr(context, "_attach_result") # type: ignore[arg-type]
), "Expected attachment to succeed but no result found"
result = context._attach_result # type: ignore[attr-defined]
assert result["validation_name"] == exp_vn, (
f"validation_name mismatch: expected {exp_vn!r}, got {result['validation_name']!r}"
)
assert result["resource_id"] == exp_rid, (
f"resource_id mismatch: expected {exp_rid!r}, got {result['resource_id']!r}"
)
@then("the attachment should have validation_name exactly " "{exp_vn}")
def step_then_preserve_validation_name(context: Context, exp_vn: str) -> None:
"""Verify validation_name is correct."""
assert (
hasattr(context, "_attach_result") # type: ignore[arg-type]
), "Expected attachment to succeed"
result = context._attach_result # type: ignore[attr-defined]
assert result["validation_name"] == exp_vn, (
f"validation_name mismatch: expected {exp_vn!r}, got {result['validation_name']!r}"
)
@then("the attachment_args_json should contain serialized args json")
def step_then_args_serialized(context: Context) -> None:
"""Verify args were serialized correctly."""
assert (
hasattr(context, "_attach_result") # type: ignore[arg-type]
), "Expected attachment to succeed"
result = context._attach_result # type: ignore[attr-defined]
stored_args = result.get("args_json", "") or "{}"
assert "scope" in stored_args, "Expected 'scope' key in serialized args"
@then("a DuplicateValidationAttachmentError should be raised")
def step_then_duplicate_error(context: Context) -> None:
"""Verify duplicate attachment was rejected."""
expected_exc_type = hasattr(context, "_attach_exception") # type: ignore[arg-type]
ctx_exc = getattr(context, "_attach_exception", None)
assert ctx_exc is not None, "Expected DuplicateValidationAttachmentError but no exception raised"
assert isinstance(
ctx_exc, DuplicateValidationAttachmentError
), f"Expected DuplicateValidationAttachmentError, got {type(ctx_exc).__name__}"
@then("both attachments should succeed independently")
def step_then_both_succeed(context: Context) -> None:
"""Verify both attachments were successful."""
has_result = hasattr(context, "_attach_result") # type: ignore[arg-type]
assert has_result, "Expected second attachment to succeed but no result found"
@@ -0,0 +1,82 @@
@tdd_issue @tdd_issue_8177 @phase1 @domain @repository @validation
Feature: ValidationAttachmentRepository argument order preservation (Issue #8177)
As a system operator attaching validations to resources
I want validation_name and resource_id to be stored in the correct order
So that data integrity is preserved without silent corruption
Background:
Given a database engine with "validation_attachments" table schema
And a validation attachment repository with session factory
@validation_attach_basic @preserve_args
Scenario: Simple names with slash in resource_id - no swap occurs after fix
Given a validation name "code-review" and resource_id "file:///project/main.py"
When I attach the validation to the resource in mode "required"
Then the attachment should have validation_name exactly "code-review" and resource_id exactly "file:///project/main.py"
@validation_attach_basic @preserve_args
Scenario: Both contain slashes - arguments preserved unchanged
Given a validation name "scope/lint-check" and resource_id "file:///project/src/module.py"
When I attach the validation to the resource in mode "informational"
Then the attachment should have validation_name exactly "scope/lint-check" and resource_id exactly "file:///project/src/module.py"
@validation_attach_basic @preserve_args
Scenario: Neither contains slashes - arguments preserved unchanged
Given a validation name "check-style" and resource_id "simple-resource-12345"
When I attach the validation to the resource in mode "required"
Then the attachment should have validation_name exactly "check-style" and resource_id exactly "simple-resource-12345"
@validation_attach_basic @preserve_args
Scenario: Only validation_name contains slash - no swap occurs after fix
Given a validation name "security/review" and resource_id "simple-resource-id"
When I attach the validation to the resource in mode "required"
Then the attachment should have validation_name exactly "security/review" and resource_id exactly "simple-resource-id"
@validation_attach_optional_params @preserve_args
Scenario: Optional project_name and plan_id preserved with no swap after fix
Given a validation name "code-review" and resource_id "file:///project/main.py"
And optional scope parameters project_name "main-project" and plan_id "plan-abc-def-"
When I attach the validation to the resource in mode "required" with optional params
Then the attachment should have validation_name exactly "code-review" and resource_id exactly "file:///project/main.py"
@validation_attach_optional_params @preserve_args
Scenario: Args dict serialized correctly without swap after fix
Given a validation name "check-permissions" and resource_id "file:///secure/endpoint.api"
And an args dict with keys "scope" (value "write") and "enforce_level" (value 3)
When I attach the validation to the resource with optional args
Then the attachment_args_json should contain serialized args json
And the attachment should have validation_name exactly "check-permissions"
@validation_attach_optional_params @preserve_args
Scenario: All optional parameters together preserve argument order after fix
Given a validation name "scope/integrity" and resource_id "file:///data/pipeline.step"
And an args dict with key "threshold" (value 0.95), project_name "etl", plan_id "plan-xyz-", mode "informational"
When I attach the validation with all optional parameters
Then the attachment should preserve validation_name exactly "scope/integrity" and resource_id exactly "file:///data/pipeline.step"
@validation_attach_duplicate
Scenario: Duplicate attachments rejected without swap interference after fix
Given a validation name "duplicate-check" and resource_id "file:///dup/resource.py"
When I attach the validation to the resource successfully once
And I attempt a second attachment with the same parameters
Then a DuplicateValidationAttachmentError should be raised
@validation_attach_duplicate
Scenario: Different plan scopes create separate non-duplicate attachments after fix
Given a validation name "plan-scoped" and resource_id "file:///shared/resource.txt"
When I attach with project_name "proj-a" and plan_id "plan-01"
And I attach again with project_name "proj-a" and plan_id "plan-02"
Then both attachments should succeed independently
@validation_attach_edge_cases @preserve_args
Scenario: Slash-containing resource_id does not trigger swap after fix
Given a validation name "final-check" and resource_id "repo/org/project/file.module:class"
When I attach the validation in mode "required"
Then the attachment should have validation_name exactly "final-check" and resource_id exactly "repo/org/project/file.module:class"
@validation_attach_edge_cases @preserve_args
Scenario: Multiple slashes in both parameters preserve original values after fix
Given a validation name "a/b/c/d" and resource_id "x/y/z/w/v"
When I attach the validation in mode "informational"
Then the attachment should have validation_name exactly "a/b/c/d" and resource_id exactly "x/y/z/w/v"
@@ -3916,9 +3916,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