CHANGELOG.md

@@ -5,6 +5,8 @@ The format follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/).
 
 ## [Unreleased]
 
+- **data-integrity: removed silent argument swap in ValidationAttachmentRepository.attach** (#8177 / #7492): Removed the fragile heuristic that silently swapped ``validation_name`` and ``resource_id`` when ``resource_id`` contained ``/`` but ``validation_name`` did not. This swap caused silent data corruption – validation names and resource IDs were stored with reversed values in the persistence layer with no error raised. Arguments now flow directly from caller to storage in their correct positional order.
+
 - **`task-implementor` posts work-started notification comments** (#11031): Both
   the `issue_impl` and `pr_fix` procedures now post an informational "work
   started" comment to the Forgejo issue/PR before beginning implementation.

CONTRIBUTORS.md

@@ -19,7 +19,7 @@ Below are some of the specific details of various contributions.
 * HAL 9000 has contributed automated implementation, bug fixes, and feature development as part of the CleverAgents automation pool.
 * HAL 9000 has contributed concurrency safety improvements, including thread-safe context tier management (issue #7547) for parallel plan execution.
 * HAL 9000 has contributed the plan concurrency race-condition fix (#7989): wired `LockService` into the plan lifecycle, guarding `execute_plan()` and `apply_plan()` with plan-level advisory locks and unique per-invocation owner identities to prevent silent concurrent state corruption.
-<<<<<<< HEAD
+* HAL 9000 has contributed the ValidationAttachmentRepository data-integrity fix (PR #8177 / issue #7492): removed the silent argument-swap heuristic (`"/" in resource_id`) that corrupted validation names and resource IDs when resource IDs contained slashes, replacing it with straightforward parameter-pass-through so arguments flow directly to the persistence layer in their correct positional order.
 * HAL 9000 has contributed the bug-hunt-pool-supervisor non-blocking tracking fix (#7875 / PR #7957): updated step 5 to be best-effort and added rule 9 to prevent the automation-tracking-manager call from blocking the main supervisor loop.
 * Jeffrey Phillips Freeman has contributed the complete AUTO-BUG-POOL to AUTO-BUG-SUP tracking prefix fix across agent-system-specification.md, automation-tracking.md documentation and agent-system-specification.md spec document, replaced with correct `AUTO-BUG-SUP` prefix used by the bug-hunt-pool-supervisor agent (#7875).
 * HAL 9000 has contributed the plugin entry point security hardening fix (#7476): enforced entry point allowlist validation before importing plugin modules to prevent malicious plugin loading.

features/repositories_uncovered_branches.feature

@@ -99,11 +99,11 @@ Feature: Repository uncovered branches and lines
     When repo branch cov I attach validation "local/check-lint" to resource "res-1"
     Then repo branch cov the attachment is returned with correct fields
 
-  Scenario: repo branch cov validation attach with swapped args auto-corrects
+  Scenario: repo branch cov validation attach preserves argument order (no swap)
     Given repo branch cov an in-memory database with tool tables
     And repo branch cov a validation tool "local/check-fmt" exists
     When repo branch cov I attach with validation_name "res/123" and resource_id "local/check-fmt"
-    Then repo branch cov the attachment swaps them correctly
+    Then repo branch cov the attachment preserves argument order
 
   # ── AutomationProfileRepository schema version mismatch ────
 

features/steps/repositories_uncovered_branches_steps.py

@@ -411,9 +411,8 @@ def step_assert_attachment(context: Context):
 
 
 @when('repo branch cov I attach with validation_name "{vn}" and resource_id "{rid}"')
-def step_attach_swapped(context: Context, vn: str, rid: str):
-    """The validation_name contains '/' but resource_id doesn't (or vice versa)
-    so the repo auto-swaps them."""
+def step_attach_preserved(context: Context, vn: str, rid: str):
+    """Arguments are passed through to the repository without any swapping."""
     repo = ValidationAttachmentRepository(
         session_factory=context.rb_session_factory,
     )
@@ -425,12 +424,11 @@ def step_attach_swapped(context: Context, vn: str, rid: str):
     session.commit()
 
 
-@then("repo branch cov the attachment swaps them correctly")
-def step_assert_swapped(context: Context):
+@then("repo branch cov the attachment preserves argument order")
+def step_assert_preserved(context: Context):
     att = context.rb_result
-    # "res/123" has "/" and "local/check-fmt" has "/" too, but the swap
-    # logic checks: "/" in resource_id AND "/" not in validation_name
-    # With both having "/", no swap happens.  Let's just verify it stored.
+    # After removing the silent swap heuristic, arguments must flow
+    # through unchanged: validation_name stays as given, resource_id stays as given.
     assert isinstance(att, dict)
     assert "attachment_id" in att
 

features/steps/validation_argument_order_integrity_steps.py

@@ -0,0 +1,220 @@
+"""Step definitions for ValidationAttachmentRepository argument-order integrity tests.
+
+Regression suite for issue #7492 (PR #8177): verifies that ``validation_name`` and
+``resource_id`` are stored in the database exactly as passed by the caller, with no
+silent swapping heuristic applied regardless of whether values contain ``/`` characters.
+"""
+
+from __future__ import annotations
+
+import json as _json
+from typing import Any
+
+from behave import given, then, when
+
+try:
+    from behave.runner import Context  # type: ignore[attr-defined]
+except ImportError:
+    Context: Any  # type: ignore[misc,assign]
+
+# ---------------------------------------------------------------------------
+# Background helpers (shared with other repo step modules)
+# ---------------------------------------------------------------------------
+
+
+def _make_engine():
+    """Create an in-memory SQLite engine."""
+    from sqlalchemy import create_engine as _ce
+    return _ce("sqlite:///:memory:")
+
+
+def _bootstrap_schema(engine):
+    """Create all CleverAgents tables on the given engine."""
+    from cleveragents.infrastructure.database.models import Base as _Base
+    from ulid import ULID
+
+    session_factory = lambda: __import__("sqlalchemy").scoped_session(
+        __import__("sqlalchemy").sessionmaker(bind=engine),
+    )
+    # Build tables via metadata.create_all which is the standard Flask/SQLAlchemy approach
+    _Base.metadata.create_all(engine)
+    return session_factory
+
+
+# ---------------------------------------------------------------------------
+# Given: shared background
+# ---------------------------------------------------------------------------
+
+
+@given("a database session factory with all required tables")
+def step_session_factory(context: Context) -> None:
+    """Prepare an in-memory SQLite engine with CleverAgents schema."""
+    context.va_engine = _make_engine()
+    context.va_session_factory = _bootstrap_schema(context.va_engine)
+    # Warm up by creating at least one row to force table creation
+    session = context.va_session_factory()
+    from cleveragents.infrastructure.database.models import (  # noqa: F401  — triggers autoload
+        ValidationAttachmentModel,
+    )
+    session.close()
+
+
+@given('a validator tool "{tool_name}" exists in the database')
+def step_create_validator(context: Context, tool_name: str) -> None:
+    """Insert a minimal validation entry into the validator_catalog table."""
+    from cleveragents.infrastructure.database.models import ValidationDefinitionModel
+
+    session = context.va_session_factory()
+    try:
+        session.add(
+            ValidationDefinitionModel(
+                name=tool_name,
+                tool_type="validation",
+                tool_version="0.1.0",
+                description="test validation for argument-order integrity suite",
+                source_file=f"path/to/{tool_name.replace('/', '_')}.py",
+                status="active",
+            ),
+        )
+        session.commit()
+    finally:
+        session.close()
+
+
+# ---------------------------------------------------------------------------
+# When: attach (direct repository calls)
+# ---------------------------------------------------------------------------
+
+
+@when('I attach validation "{validation_name}" to resource "{resource_id}"')
+def step_attach_basic(context: Context, validation_name: str, resource_id: str) -> None:
+    """Attach a validation with only name+resource."""
+    from cleveragents.infrastructure.database.repositories import (
+        ValidationAttachmentRepository as VAR,
+    )
+    repo = VAR(session_factory=context.va_session_factory)
+    result = repo.attach(validation_name=validation_name, resource_id=resource_id)
+    context.va_last_result = result  # type: ignore[attr-defined]
+
+
+@when(
+    'I attach validation "{validation_name}" to resource "{resource_id}" '
+    'with project "{project_name}"'
+)
+def step_attach_with_project(
+    context: Context, validation_name: str, resource_id: str, project_name: str
+) -> None:
+    """Attach with a project_name override."""
+    from cleveragents.infrastructure.database.repositories import (
+        ValidationAttachmentRepository as VAR,
+    )
+    repo = VAR(session_factory=context.va_session_factory)
+    result = repo.attach(
+        validation_name=validation_name,
+        resource_id=resource_id,
+        project_name=project_name,
+    )
+    context.va_last_result = result  # type: ignore[attr-defined]
+
+
+@when(
+    'I attach validation "{validation_name}" to resource "{resource_id}" '
+    'with plan "{plan_id}"'
+)
+def step_attach_with_plan(
+    context: Context, validation_name: str, resource_id: str, plan_id: str
+) -> None:
+    """Attach with a plan_id override."""
+    from cleveragents.infrastructure.database.repositories import (
+        ValidationAttachmentRepository as VAR,
+    )
+    repo = VAR(session_factory=context.va_session_factory)
+    result = repo.attach(
+        validation_name=validation_name,
+        resource_id=resource_id,
+        plan_id=plan_id,
+    )
+    context.va_last_result = result  # type: ignore[attr-defined]
+
+
+@when(
+    'I attach validation "{validation_name}" to resource "{resource_id}" '
+    'with mode "{mode}" and args {args}'
+)
+def step_attach_with_mode_and_args(
+    context: Context, validation_name: str, resource_id: str, mode: str, args: str
+) -> None:
+    """Attach with mode and arbitrary JSON args."""
+    from cleveragents.infrastructure.database.repositories import (
+        ValidationAttachmentRepository as VAR,
+    )
+    repo = VAR(session_factory=context.va_session_factory)
+    result = repo.attach(
+        validation_name=validation_name,
+        resource_id=resource_id,
+        mode=mode,
+        args=_json.loads(args),
+    )
+    context.va_last_result = result  # type: ignore[attr-defined]
+
+
+# ---------------------------------------------------------------------------
+# Then: assertions on the stored attachment
+# ---------------------------------------------------------------------------
+
+
+@then('the stored attachment has validation_name "{expected}"')
+def step_check_validation_name(context: Context, expected: str) -> None:
+    """Verify that validation_name was stored exactly as given."""
+    result = context.va_last_result  # type: ignore[attr-defined]
+    assert isinstance(result, dict), f"Expected dict result, got {type(result)}"
+    assert result["validation_name"] == expected, (
+        f"(validation_name mismatch) expected {expected!r}, "
+        f'got {result["validation_name"]!r}. '
+        "The silent argument-swapping heuristic was NOT removed!"
+    )
+
+
+@then('the stored attachment has resource_id "{expected}"')
+def step_check_resource_id(context: Context, expected: str) -> None:
+    """Verify that resource_id was stored exactly as given."""
+    result = context.va_last_result  # type: ignore[attr-defined]
+    assert isinstance(result, dict), f"Expected dict result, got {type(result)}"
+    assert result["resource_id"] == expected, (
+        f"(resource_id mismatch) expected {expected!r}, "
+        f'got {result["resource_id"]!r}. '
+        "The silent argument-swapping heuristic was NOT removed!"
+    )
+
+
+@then('the stored attachment has project_name "{expected}"')
+def step_check_project_name(context: Context, expected: str) -> None:
+    """Verify that project_name was passed through correctly."""
+    result = context.va_last_result  # type: ignore[attr-defined]
+    assert isinstance(result, dict), f"Expected dict result, got {type(result)}"
+    assert result["project_name"] == expected, (
+        f"(project_name mismatch) expected {expected!r}, "
+        f'got {result["project_name"]!r}'
+    )
+
+
+@then('the stored attachment has plan_id "{expected}"')
+def step_check_plan_id(context: Context, expected: str) -> None:
+    """Verify that plan_id was passed through correctly."""
+    result = context.va_last_result  # type: ignore[attr-defined]
+    assert isinstance(result, dict), f"Expected dict result, got {type(result)}"
+    assert result["plan_id"] == expected, (
+        f"(plan_id mismatch) expected {expected!r}, "
+        f'got {result["plan_id"]!r}'
+    )
+
+
+@then('the stored attachment has mode "{expected}"')
+def step_check_mode(context: Context, expected: str) -> None:
+    """Verify that mode was passed through correctly."""
+    result = context.va_last_result  # type: ignore[attr-defined]
+    assert isinstance(result, dict), f"Expected dict result, got {type(result)}"
+    assert result["mode"] == expected, (
+        f"(mode mismatch) expected {expected!r}, "
+        f'got {result["mode"]!r}'
+    )

features/validation_argument_order_integrity.feature

@@ -0,0 +1,79 @@
+Feature: ValidationAttachmentRepository preserves argument order (issue #7492)
+  As a CleverAgents data integrity engineer
+  I want validation_name and resource_id to be stored exactly as provided
+  So that silent argument swapping does not cause data corruption (#7492)
+
+  Background:
+    Given a database session factory with all required tables
+    And a validator tool "local/check-fmt" exists in the database
+    And a validator tool "local/run-tests" exists in the database
+
+  # ── Core regression: simple IDs without slashes ──────────────────────────────
+
+  Scenario: Attach preserves validation_name and resource_id for simple slash-free IDs
+    When I attach validation "local/check-fmt" to resource "simple-resource-id"
+    Then the stored attachment has validation_name "local/check-fmt"
+    And the stored attachment has resource_id "simple-resource-id"
+
+  # ── Boundary: slash-containing resource_id (the original trigger for the bug) ─
+
+  Scenario: Attach does NOT swap when resource_id contains slash but validation_name does not
+    When I attach validation "local/check-fmt" to resource "repo/main/module.py"
+    Then the stored attachment has validation_name "local/check-fmt"
+    AND the stored attachment has resource_id "repo/main/module.py"
+
+  Scenario: Attach does NOT swap when resource_id is a namespaced reference with slashes
+    When I attach validation "local/run-tests" to resource "org/team/project/file.txt"
+    Then the stored attachment has validation_name "local/run-tests"
+    AND the stored attachment has resource_id "org/team/project/file.txt"
+
+  # ── Boundary: slash in BOTH parameters — no swap occurs either way ────────────
+
+  Scenario: Attach preserves order when both values contain slashes
+    When I attach validation "check/lint" to resource "file/path.py"
+    Then the stored attachment has validation_name "check/lint"
+    AND the stored attachment has resource_id "file/path.py"
+
+  # ── Boundary: slash in validation_name but NOT in resource_id ─────────────────
+
+  Scenario: Attach preserves order when only validation_name contains slash
+    When I attach validation "check/style" to resource "simple-id"
+    Then the stored attachment has validation_name "check/style"
+    AND the stored attachment has resource_id "simple-id"
+
+  # ── Optional parameter combinations with no swap ─────────────────────────────
+
+  Scenario: Attach preserves argument order with project_name override
+    When I attach validation "local/check-fmt" to resource "repo/main/module.py" with project "acme-corp"
+    Then the stored attachment has validation_name "local/check-fmt"
+    AND the stored attachment has resource_id "repo/main/module.py"
+    AND the stored attachment has project_name "acme-corp"
+
+  Scenario: Attach preserves argument order with plan_id override
+    When I attach validation "local/run-tests" to resource "git-checkout/repo" with plan "abc-def-123"
+    Then the stored attachment has validation_name "local/run-tests"
+    AND the stored attachment has resource_id "git-checkout/repo"
+    AND the stored attachment has plan_id "abc-def-123"
+
+  Scenario: Attach preserves argument order with mode and args overrides
+    When I attach validation "local/check-fmt" to resource "repo/main/module.py" with mode "informational" and args {"threshold": 80}
+    Then the stored attachment has validation_name "local/check-fmt"
+    AND the stored attachment has resource_id "repo/main/module.py"
+    AND the stored attachment has mode "informational"
+
+  # ── Edge: empty and minimal identifiers ───────────────────────────────────────
+
+  Scenario: Attach preserves arguments for single-character resource_id
+    When I attach validation "local/lint" to resource "x"
+    Then the stored attachment has validation_name "local/lint"
+    AND the stored attachment has resource_id "x"
+
+  Scenario: Attach preserves arguments for slash-as-resource-id
+    When I attach validation "local/check" to resource "/"
+    Then the stored attachment has validation_name "local/check"
+    AND the stored attachment has resource_id "/"
+
+  Scenario: Attach preserves arguments for deep nested path as resource_id
+    When I attach validation "verify/schema" to resource "a/b/c/d/e/f/g/h"
+    Then the stored attachment has validation_name "verify/schema"
+    AND the stored attachment has resource_id "a/b/c/d/e/f/g/h"

src/cleveragents/infrastructure/database/repositories.py

@@ -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