From 97c1007bb5c1dfb28c24e4de698f4b6bae448d75 Mon Sep 17 00:00:00 2001 From: CoreRasurae Date: Thu, 26 Mar 2026 09:37:57 +0000 Subject: [PATCH 01/10] feat(events): wire domain services to emit missing EventBus events Add TokenAuthMiddleware to emit AUTH_SUCCESS/AUTH_FAILURE with spec-aligned audit details and wire it through the DI container using server.token resolution. Add Behave and Robot coverage for auth event emission and end-to-end audit persistence, and update audit subscriber producer notes and changelog. ISSUES CLOSED: #714 --- CHANGELOG.md | 17 +- features/auth_middleware_events.feature | 63 ++++++ features/mocks/__init__.py | 2 + features/mocks/recording_event_bus.py | 25 +++ .../steps/auth_middleware_events_steps.py | 165 +++++++++++++++ robot/audit_service_wiring.robot | 11 +- robot/helper_audit_wiring.py | 100 +++++++++ src/cleveragents/application/container.py | 27 +++ .../services/audit_event_subscriber.py | 10 +- .../application/services/auth_middleware.py | 189 ++++++++++++++++++ src/cleveragents/shared/redaction.py | 1 + 11 files changed, 600 insertions(+), 10 deletions(-) create mode 100644 features/auth_middleware_events.feature create mode 100644 features/mocks/recording_event_bus.py create mode 100644 features/steps/auth_middleware_events_steps.py create mode 100644 src/cleveragents/application/services/auth_middleware.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 672c4835e..f4bae43db 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,8 @@ The format follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). Changed `wf10_batch.robot` to be less likely to create files, and `plan_generation_graph.robot` to give more test answers. +## [Unreleased] + - Hardened the TDD bug-fix quality gate for issue #629: PR parsing now requires whole-word closing keywords (avoids false positives like "prefixes #12"), TDD bug tag discovery now uses exact token matching @@ -40,6 +42,20 @@ Changed `wf10_batch.robot` to be less likely to create files, and cleanup conflict with `cli_init_yes_flag_steps.py`); 2 new Behave scenarios covering multi-line PR description parsing and non-string `pr_diff` type guard. + +- Added `TokenAuthMiddleware` and DI wiring to emit missing auth domain + events (`AUTH_SUCCESS`, `AUTH_FAILURE`) during token checks, including + spec-aligned audit details (`user_identity`, `attempted_identity`, + `ip_address`, `token_prefix`, `failure_reason`) and best-effort publish + behavior. Auth token prefixes now persist to audit logs without + redaction-key collisions, and short tokens are masked (`***...`) to + prevent full token disclosure. Added Behave coverage and Robot + audit-pipeline integration tests for auth event persistence. (#714) + +- Added TDD bug-capture E2E tests for bug #1028 — ACMS indexing pipeline not + wired into CLI. Four Robot Framework E2E tests prove ContextTierService starts + empty on every CLI invocation. Tests use ``@tdd_expected_fail`` until the bug + fix is merged. (#1029) - Added Fix-then-Revalidate orchestration loop for required validations: bounded retry with configurable limits (0--100 per Safety Profile), strategy revision escalation via `auto_strategy_revision` float @@ -52,7 +68,6 @@ Changed `wf10_batch.robot` to be less likely to create files, and counter, spec-required `validation_summary` and `final_validation_results` fields on the result model, DI container -## [Unreleased] - **fix(tui): rename ActorSelectionOverlay._render to _refresh_display (issue #11039)** — `ActorSelectionOverlay._render()` shadows Textual's `Widget._render()` which must return a `Strip`. In textual >=1.0, layout calls `get_content_height()` `self._render()` gets `None` `AttributeError: 'NoneType' object has no attribute 'get_height'`. Renamed the method to `_refresh_display()` and updated all four internal call sites (`show()`, `move_up()`, `move_down()`, `set_search()`) to use the new name. - **Structural Component Output Validation** (#8164): Replaces exact character matching with structural component checking for output validation. Implements three validators covering plan tree output, decision CLI dicts, and structured session snapshots. The `validate_plan_tree` function validates node dicts for required keys (`decision_id`, `type`, `sequence`, `question`, `children`), ULID format, correct types, and sibling ordering. The `validate_decision_dict` function validates decision CLI output against the `Decision.as_cli_dict()` schema with field presence, type, ULID pattern, confidence range [0..1], and boolean field checks. The `validate_structured_output` function validates the StructuredOutput envelope for `command`, `session_id` (ULID), status membership, `exit_code`, and elements integrity. A unified dispatcher (`validate_structured_component_output`) enables routing by target_type. BDD test coverage added in `features/structural_validation.feature`. [Epic #8137](https://git.cleverthis.com/cleveragents/cleveragents-core/issues/8137) diff --git a/features/auth_middleware_events.feature b/features/auth_middleware_events.feature new file mode 100644 index 000000000..3c19e8e39 --- /dev/null +++ b/features/auth_middleware_events.feature @@ -0,0 +1,63 @@ +Feature: Auth middleware emits security events + As a platform operator + I want authentication outcomes emitted as domain events + So the audit pipeline captures auth success and failure details + + Scenario: Successful authentication emits AUTH_SUCCESS + Given an auth middleware with expected token "tok_secret_123" + And an auth recording event bus + When I authenticate with token "tok_secret_123" identity "alice@example.com" and ip "10.0.0.5" + Then the auth result should be true + And the auth event bus should have exactly 1 event + And the latest auth event type should be AUTH_SUCCESS + And the latest auth event should contain detail "user_identity" with value "alice@example.com" + And the latest auth event should contain detail "ip_address" with value "10.0.0.5" + And the latest auth event should contain detail "token_prefix" with value "tok_se..." + + Scenario: Short token success masks token_prefix + Given an auth middleware with expected token "short" + And an auth recording event bus + When I authenticate with token "short" identity "alice@example.com" and ip "10.0.0.5" + Then the auth result should be true + And the latest auth event type should be AUTH_SUCCESS + And the latest auth event should contain detail "token_prefix" with value "***..." + + Scenario: Failed authentication emits AUTH_FAILURE + Given an auth middleware with expected token "tok_secret_123" + And an auth recording event bus + When I authenticate with token "tok_wrong_999" identity "alice@example.com" and ip "10.0.0.5" + Then the auth result should be false + And the auth event bus should have exactly 1 event + And the latest auth event type should be AUTH_FAILURE + And the latest auth event should contain detail "attempted_identity" with value "alice@example.com" + And the latest auth event should contain detail "failure_reason" with value "invalid_token" + And the latest auth event should contain detail "token_prefix" with value "tok_wr..." + + Scenario: Missing configured token emits AUTH_FAILURE + Given an auth middleware with no configured token + And an auth recording event bus + When I authenticate with token "tok_any_111" identity "bob@example.com" and ip "10.0.0.9" + Then the auth result should be false + And the latest auth event type should be AUTH_FAILURE + And the latest auth event should contain detail "failure_reason" with value "token_not_configured" + + Scenario: Empty authentication token is rejected before emission + Given an auth middleware with expected token "tok_secret_123" + And an auth recording event bus + When I try to authenticate with an empty token + Then an auth ValueError should be raised + And the auth event bus should have exactly 0 event + + Scenario: Auth middleware events persist through AuditEventSubscriber + Given auth middleware is wired to AuditEventSubscriber with expected token "tok_secret_123" + When I authenticate with token "tok_secret_123" identity "carol@example.com" and ip "10.1.2.3" + And I authenticate with token "tok_bad_123" identity "dave@example.com" and ip "10.1.2.4" + Then the auth result history should be "true,false" + And the audit log should contain 1 auth_success entry from middleware + And the latest auth_success audit entry should contain detail "ip_address" with value "10.1.2.3" + And the latest auth_success audit entry should contain detail "token_prefix" with value "tok_se..." + And the audit log should contain 1 auth_failure entry from middleware + And the latest auth_failure audit entry should contain detail "ip_address" with value "10.1.2.4" + And the latest auth_failure audit entry should contain detail "attempted_identity" with value "dave@example.com" + And the latest auth_failure audit entry should contain detail "failure_reason" with value "invalid_token" + And the latest auth_failure audit entry should contain detail "token_prefix" with value "tok_ba..." diff --git a/features/mocks/__init__.py b/features/mocks/__init__.py index 43b8f3660..566279ea7 100644 --- a/features/mocks/__init__.py +++ b/features/mocks/__init__.py @@ -9,6 +9,7 @@ from .fake_provider import FakeProviderInfo, FakeProviderRegistry from .lsp_transport_mock import MockLspTransport, parse_lsp_responses from .mock_ai_provider import MockAIProvider from .mock_mcp_transport import MockMCPTransport +from .recording_event_bus import RecordingEventBus from .transient_fail_audit_service import TransientFailAuditService # NOTE: tdd_test_helpers is NOT re-exported here because it imports @@ -22,6 +23,7 @@ __all__ = [ "MockAIProvider", "MockLspTransport", "MockMCPTransport", + "RecordingEventBus", "TransientFailAuditService", "parse_lsp_responses", ] diff --git a/features/mocks/recording_event_bus.py b/features/mocks/recording_event_bus.py new file mode 100644 index 000000000..88d38ff8e --- /dev/null +++ b/features/mocks/recording_event_bus.py @@ -0,0 +1,25 @@ +"""Minimal in-memory event bus used for Behave assertions.""" + +from __future__ import annotations + +from collections.abc import Callable + +from cleveragents.infrastructure.events.models import DomainEvent +from cleveragents.infrastructure.events.types import EventType + + +class RecordingEventBus: + """Minimal in-memory event bus used for Behave assertions.""" + + def __init__(self) -> None: + self.events: list[DomainEvent] = [] + + def emit(self, event: DomainEvent) -> None: + self.events.append(event) + + def subscribe( + self, + event_type: EventType, + handler: Callable[[DomainEvent], None], + ) -> None: + _ = (event_type, handler) diff --git a/features/steps/auth_middleware_events_steps.py b/features/steps/auth_middleware_events_steps.py new file mode 100644 index 000000000..74ac53607 --- /dev/null +++ b/features/steps/auth_middleware_events_steps.py @@ -0,0 +1,165 @@ +"""Step definitions for auth_middleware_events.feature.""" + +from __future__ import annotations + +from behave import given, then, when +from behave.runner import Context +from sqlalchemy import create_engine +from sqlalchemy.orm import sessionmaker + +from cleveragents.application.services.audit_event_subscriber import ( + AuditEventSubscriber, +) +from cleveragents.application.services.audit_service import AuditService +from cleveragents.application.services.auth_middleware import TokenAuthMiddleware +from cleveragents.config.settings import Settings +from cleveragents.infrastructure.database.models import Base +from cleveragents.infrastructure.events.reactive import ReactiveEventBus +from cleveragents.infrastructure.events.types import EventType +from features.mocks.recording_event_bus import RecordingEventBus + + +def _fresh_audit_service() -> AuditService: + Settings._instance = None + settings = Settings(database_url="sqlite:///:memory:") + engine = create_engine("sqlite:///:memory:", echo=False) + Base.metadata.create_all(engine) + session = sessionmaker(bind=engine)() + return AuditService(settings=settings, session=session) + + +@given('an auth middleware with expected token "{token}"') +def step_auth_middleware_with_token(context: Context, token: str) -> None: + context.expected_token = token + + +@given("an auth middleware with no configured token") +def step_auth_middleware_no_token(context: Context) -> None: + context.expected_token = None + + +@given("an auth recording event bus") +def step_auth_recording_event_bus(context: Context) -> None: + context.event_bus = RecordingEventBus() + context.auth_middleware = TokenAuthMiddleware( + expected_token=context.expected_token, + event_bus=context.event_bus, + ) + context.auth_result_history = [] + + +@given('auth middleware is wired to AuditEventSubscriber with expected token "{token}"') +def step_auth_middleware_audit_pipeline(context: Context, token: str) -> None: + context.audit_service = _fresh_audit_service() + context.event_bus = ReactiveEventBus() + context.audit_subscriber = AuditEventSubscriber( + audit_service=context.audit_service, + event_bus=context.event_bus, + ) + context.auth_middleware = TokenAuthMiddleware( + expected_token=token, + event_bus=context.event_bus, + ) + context.auth_result_history = [] + + +@when('I authenticate with token "{token}" identity "{identity}" and ip "{ip_address}"') +def step_authenticate( + context: Context, token: str, identity: str, ip_address: str +) -> None: + context.auth_result = context.auth_middleware.authenticate( + token, + identity=identity, + ip_address=ip_address, + ) + context.auth_result_history.append(context.auth_result) + + +@when("I try to authenticate with an empty token") +def step_try_authenticate_empty_token(context: Context) -> None: + context.auth_error = None + try: + context.auth_middleware.authenticate("", identity="empty@example.com") + except ValueError as exc: + context.auth_error = exc + + +@then("the auth result should be true") +def step_auth_result_true(context: Context) -> None: + assert context.auth_result is True + + +@then("the auth result should be false") +def step_auth_result_false(context: Context) -> None: + assert context.auth_result is False + + +@then("the auth event bus should have exactly {count:d} event") +def step_auth_event_count(context: Context, count: int) -> None: + assert len(context.event_bus.events) == count, ( + f"Expected {count} events, got {len(context.event_bus.events)}" + ) + + +@then("the latest auth event type should be {event_type_name}") +def step_latest_auth_event_type(context: Context, event_type_name: str) -> None: + expected = EventType[event_type_name] + actual = context.event_bus.events[-1].event_type + assert actual == expected, f"Expected event type {expected}, got {actual}" + + +@then( + 'the latest auth event should contain detail "{key}" with value "{expected_value}"' +) +def step_latest_auth_event_detail( + context: Context, key: str, expected_value: str +) -> None: + details = context.event_bus.events[-1].details + assert details.get(key) == expected_value, ( + f"Expected {key}={expected_value!r}, got {details.get(key)!r}" + ) + + +@then("an auth ValueError should be raised") +def step_auth_value_error(context: Context) -> None: + assert context.auth_error is not None, ( + "Expected ValueError but no exception was raised" + ) + assert isinstance(context.auth_error, ValueError), ( + f"Expected ValueError, got {type(context.auth_error).__name__}" + ) + + +@then('the auth result history should be "{history_csv}"') +def step_auth_result_history(context: Context, history_csv: str) -> None: + expected = [part.strip().lower() == "true" for part in history_csv.split(",")] + assert context.auth_result_history == expected, ( + f"Expected history {expected}, got {context.auth_result_history}" + ) + + +@then("the audit log should contain 1 auth_success entry from middleware") +def step_audit_has_auth_success(context: Context) -> None: + entries = context.audit_service.list_entries(event_type="auth_success") + assert len(entries) == 1, f"Expected 1 auth_success entry, got {len(entries)}" + + +@then("the audit log should contain 1 auth_failure entry from middleware") +def step_audit_has_auth_failure(context: Context) -> None: + entries = context.audit_service.list_entries(event_type="auth_failure") + assert len(entries) == 1, f"Expected 1 auth_failure entry, got {len(entries)}" + + +@then( + 'the latest {audit_event_type} audit entry should contain detail "{key}" with value "{expected_value}"' +) +def step_latest_audit_entry_detail( + context: Context, audit_event_type: str, key: str, expected_value: str +) -> None: + entries = context.audit_service.list_entries(event_type=audit_event_type) + assert entries, f"Expected at least one {audit_event_type} audit entry" + details = entries[0].details + assert details.get(key) == expected_value, ( + f"Expected {audit_event_type}.{key}={expected_value!r}, " + f"got {details.get(key)!r}" + ) diff --git a/robot/audit_service_wiring.robot b/robot/audit_service_wiring.robot index 8f73c67c4..e1dda5c45 100644 --- a/robot/audit_service_wiring.robot +++ b/robot/audit_service_wiring.robot @@ -57,7 +57,6 @@ E2E Plan Lifecycle Through Audit Pipeline Log ${result.stderr} Should Be Equal As Integers ${result.rc} 0 Should Contain ${result.stdout} E2E_OK - User Identity Field Propagates Through Audit Pipeline [Documentation] DomainEvent.user_identity propagates via AuditEventSubscriber to AuditService DB entry [Tags] observability audit identity @@ -87,3 +86,13 @@ User Identity Field Precedence Over Details Log ${result.stderr} Should Be Equal As Integers ${result.rc} 0 Should Contain ${result.stdout} IDENTITY_PRECEDENCE_OK + +Auth Middleware Emits Audit Events + [Documentation] End-to-end: TokenAuthMiddleware emits AUTH_SUCCESS/AUTH_FAILURE through audit pipeline + [Tags] observability audit auth + ${result}= Run Process ${PYTHON} ${HELPER} auth_middleware_pipeline + ... cwd=${WORKSPACE} + Log ${result.stdout} + Log ${result.stderr} + Should Be Equal As Integers ${result.rc} 0 + Should Contain ${result.stdout} AUTH_PIPELINE_OK diff --git a/robot/helper_audit_wiring.py b/robot/helper_audit_wiring.py index 3bad1f020..0b2c3e2a0 100644 --- a/robot/helper_audit_wiring.py +++ b/robot/helper_audit_wiring.py @@ -23,6 +23,9 @@ from cleveragents.application.services.audit_event_subscriber import ( # noqa: from cleveragents.application.services.audit_service import ( # noqa: E402 AuditService, ) +from cleveragents.application.services.auth_middleware import ( # noqa: E402 + TokenAuthMiddleware, +) from cleveragents.application.services.plan_lifecycle_service import ( # noqa: E402 PlanLifecycleService, ) @@ -323,6 +326,102 @@ def user_identity_field_precedence() -> None: print("IDENTITY_PRECEDENCE_OK") +def auth_middleware_pipeline() -> None: + """E2E: TokenAuthMiddleware -> EventBus -> AuditEventSubscriber -> DB.""" + svc = _make_audit_service() + bus = ReactiveEventBus() + AuditEventSubscriber(audit_service=svc, event_bus=bus) + middleware = TokenAuthMiddleware(expected_token="tok_secret_123", event_bus=bus) + + success = middleware.authenticate( + "tok_secret_123", + identity="carol@example.com", + ip_address="10.1.2.3", + ) + failure = middleware.authenticate( + "tok_bad_123", + identity="dave@example.com", + ip_address="10.1.2.4", + ) + if not success or failure: + print( + "FAIL: unexpected auth boolean results " + f"success={success} failure={failure}", + file=sys.stderr, + ) + sys.exit(1) + + success_entries = svc.list_entries(event_type="auth_success") + failure_entries = svc.list_entries(event_type="auth_failure") + if len(success_entries) != 1: + print( + f"FAIL: expected 1 auth_success entry, got {len(success_entries)}", + file=sys.stderr, + ) + sys.exit(1) + if len(failure_entries) != 1: + print( + f"FAIL: expected 1 auth_failure entry, got {len(failure_entries)}", + file=sys.stderr, + ) + sys.exit(1) + + s_entry = success_entries[0] + f_entry = failure_entries[0] + s_details = s_entry.details + f_details = f_entry.details + if s_entry.user_identity != "carol@example.com": + print( + f"FAIL: unexpected auth_success identity {s_entry.user_identity!r}", + file=sys.stderr, + ) + sys.exit(1) + if f_entry.user_identity != "unauthenticated": + print( + f"FAIL: unexpected auth_failure identity {f_entry.user_identity!r}", + file=sys.stderr, + ) + sys.exit(1) + if s_details.get("ip_address") != "10.1.2.3": + print( + f"FAIL: unexpected auth_success ip {s_details.get('ip_address')!r}", + file=sys.stderr, + ) + sys.exit(1) + if s_details.get("token_prefix") != "tok_se...": + print( + "FAIL: auth_success token_prefix mismatch", + file=sys.stderr, + ) + sys.exit(1) + if f_details.get("ip_address") != "10.1.2.4": + print( + f"FAIL: unexpected auth_failure ip {f_details.get('ip_address')!r}", + file=sys.stderr, + ) + sys.exit(1) + if f_details.get("attempted_identity") != "dave@example.com": + print( + "FAIL: auth_failure attempted_identity mismatch", + file=sys.stderr, + ) + sys.exit(1) + if f_details.get("failure_reason") != "invalid_token": + print( + f"FAIL: unexpected auth_failure reason {f_details.get('failure_reason')!r}", + file=sys.stderr, + ) + sys.exit(1) + if f_details.get("token_prefix") != "tok_ba...": + print( + "FAIL: auth_failure token_prefix mismatch", + file=sys.stderr, + ) + sys.exit(1) + + print("AUTH_PIPELINE_OK") + + # --------------------------------------------------------------------------- # Dispatch # --------------------------------------------------------------------------- @@ -336,6 +435,7 @@ _COMMANDS = { "user_identity_field": user_identity_field, "user_identity_details_fallback": user_identity_details_fallback, "user_identity_field_precedence": user_identity_field_precedence, + "auth_middleware_pipeline": auth_middleware_pipeline, } if __name__ == "__main__": diff --git a/src/cleveragents/application/container.py b/src/cleveragents/application/container.py index a206c60f5..ab1b47517 100644 --- a/src/cleveragents/application/container.py +++ b/src/cleveragents/application/container.py @@ -23,6 +23,7 @@ from cleveragents.application.services.audit_event_subscriber import ( AuditEventSubscriber, ) from cleveragents.application.services.audit_service import AuditService +from cleveragents.application.services.auth_middleware import TokenAuthMiddleware from cleveragents.application.services.automation_profile_service import ( AutomationProfileService, ) @@ -410,6 +411,25 @@ def _resolve_auto_reindex() -> bool: return True +def _resolve_server_token() -> str | None: + """Resolve ``server.token`` from config, returning ``None`` when unset.""" + try: + svc = ConfigService() + resolved = svc.resolve("server.token") + raw = resolved.value + if raw is None: + return None + token = str(raw).strip() + return token if token else None + except Exception as exc: + _logger.warning( + "server_token_resolution_failed", + error_type=type(exc).__name__, + error_message=redact_value(str(exc)), + ) + return None + + def _build_analyzer_registry() -> AnalyzerRegistry: """Build an AnalyzerRegistry with built-in analyzers registered.""" from cleveragents.domain.models.acms.python_analyzer import PythonAnalyzer @@ -667,6 +687,13 @@ class Container(containers.DeclarativeContainer): # Event Bus - Singleton (one shared instance for the process) event_bus = providers.Singleton(ReactiveEventBus) + # Per-request auth middleware with fresh token resolution from config. + auth_middleware = providers.Factory( + TokenAuthMiddleware, + expected_token=providers.Callable(_resolve_server_token), + event_bus=event_bus, + ) + # Services - Factory (new instance per request with injected dependencies) project_service = providers.Factory( ProjectService, diff --git a/src/cleveragents/application/services/audit_event_subscriber.py b/src/cleveragents/application/services/audit_event_subscriber.py index 215cacf89..91ef7af11 100644 --- a/src/cleveragents/application/services/audit_event_subscriber.py +++ b/src/cleveragents/application/services/audit_event_subscriber.py @@ -37,19 +37,13 @@ _logger = structlog.get_logger(__name__) SECURITY_EVENT_MAP: dict[EventType, str] = { EventType.PLAN_APPLIED: "plan_applied", EventType.PLAN_CANCELLED: "plan_cancelled", - # NOTE: RESOURCE_MODIFIED has no producing service yet — the tool - # execution framework does not emit this event. The subscriber handler - # is intentionally registered so that audit entries are automatically - # created once tool-write event emission is implemented. + # Emitted by ResourceFileWatcher when indexed resources change. EventType.RESOURCE_MODIFIED: "resource_modified", EventType.CORRECTION_APPLIED: "correction_applied", EventType.CONFIG_CHANGED: "config_changed", EventType.ENTITY_DELETED: "entity_deleted", EventType.SESSION_CREATED: "session_created", - # NOTE: AUTH_SUCCESS and AUTH_FAILURE have no producing service yet — - # server-mode authentication is not implemented. The subscriber handlers - # are registered so that audit entries are automatically created once - # server auth emits these events. + # Emitted by TokenAuthMiddleware during bearer-token checks. EventType.AUTH_SUCCESS: "auth_success", EventType.AUTH_FAILURE: "auth_failure", } diff --git a/src/cleveragents/application/services/auth_middleware.py b/src/cleveragents/application/services/auth_middleware.py new file mode 100644 index 000000000..508964c3d --- /dev/null +++ b/src/cleveragents/application/services/auth_middleware.py @@ -0,0 +1,189 @@ +"""Authentication middleware that emits audit-friendly auth events. + +This service encapsulates bearer-token authentication checks and emits +``AUTH_SUCCESS`` / ``AUTH_FAILURE`` domain events through an injected +``EventBus`` so the audit pipeline can persist login outcomes. + +Spec alignment: + - docs/specification.md § Event System + - docs/specification.md § Audit Logging (auth_success / auth_failure) +""" + +from __future__ import annotations + +import hmac +from typing import TYPE_CHECKING + +import structlog + +from cleveragents.infrastructure.events.models import DomainEvent +from cleveragents.infrastructure.events.types import EventType +from cleveragents.shared.redaction import redact_value + +if TYPE_CHECKING: + from cleveragents.infrastructure.events.protocol import EventBus + +_logger = structlog.get_logger(__name__) +_TOKEN_PREFIX_LEN = 6 +_SHORT_TOKEN_MASK = "***..." + + +def _token_prefix(token: str) -> str: + """Return a safe token prefix for audit details. + + Never returns the full token value. Tokens shorter than or equal to the + prefix length are collapsed to a constant mask to avoid full disclosure. + """ + if len(token) <= _TOKEN_PREFIX_LEN: + return _SHORT_TOKEN_MASK + return f"{token[:_TOKEN_PREFIX_LEN]}..." + + +def _normalize_optional_text(field_name: str, value: str | None) -> str | None: + """Normalize optional text fields with guard-first validation.""" + if value is None: + return None + if not isinstance(value, str): + raise TypeError(f"{field_name} must be a string or None") + normalized = value.strip() + if not normalized: + raise ValueError(f"{field_name} must not be empty when provided") + return normalized + + +class TokenAuthMiddleware: + """Authenticate bearer tokens and emit AUTH_* events. + + This class is intentionally transport-agnostic. A future server-mode + middleware can delegate authentication decisions to this component and + inherit consistent event emission semantics. + """ + + def __init__( + self, + expected_token: str | None, + event_bus: EventBus | None = None, + ) -> None: + """Initialize middleware with an expected token and optional event bus. + + Args: + expected_token: Configured bearer token for successful auth. + event_bus: Event bus used for AUTH_SUCCESS/AUTH_FAILURE emission. + """ + self._expected_token = _normalize_optional_text( + "expected_token", expected_token + ) + self._event_bus = event_bus + + def authenticate( + self, + token: str, + *, + identity: str | None = None, + ip_address: str | None = None, + ) -> bool: + """Authenticate *token* and emit a corresponding auth event. + + Args: + token: Bearer token supplied by the caller. + identity: Optional user identity / principal for audit context. + ip_address: Optional client IP address for audit context. + + Returns: + ``True`` if authentication succeeds, otherwise ``False``. + """ + if not isinstance(token, str): + raise TypeError("token must be a string") + normalized_token = token.strip() + if not normalized_token: + raise ValueError("token must not be empty") + + normalized_identity = _normalize_optional_text("identity", identity) + normalized_ip = _normalize_optional_text("ip_address", ip_address) + + if self._expected_token is None: + self._emit_auth_failure( + attempted_identity=normalized_identity, + ip_address=normalized_ip, + token_prefix=_token_prefix(normalized_token), + reason="token_not_configured", + ) + return False + + if hmac.compare_digest(normalized_token, self._expected_token): + self._emit_auth_success( + user_identity=normalized_identity, + ip_address=normalized_ip, + token_prefix=_token_prefix(normalized_token), + ) + return True + + self._emit_auth_failure( + attempted_identity=normalized_identity, + ip_address=normalized_ip, + token_prefix=_token_prefix(normalized_token), + reason="invalid_token", + ) + return False + + def _emit_auth_success( + self, + *, + user_identity: str | None, + ip_address: str | None, + token_prefix: str, + ) -> None: + """Emit AUTH_SUCCESS if an event bus is configured.""" + if self._event_bus is None: + return + details: dict[str, str] = { + "user_identity": user_identity or "unknown", + "token_prefix": token_prefix, + } + if ip_address is not None: + details["ip_address"] = ip_address + self._emit_event(EventType.AUTH_SUCCESS, details) + + def _emit_auth_failure( + self, + *, + attempted_identity: str | None, + ip_address: str | None, + token_prefix: str, + reason: str, + ) -> None: + """Emit AUTH_FAILURE if an event bus is configured.""" + if self._event_bus is None: + return + details: dict[str, str] = { + "attempted_identity": attempted_identity or "unknown", + "user_identity": "unauthenticated", + "failure_reason": reason, + "token_prefix": token_prefix, + } + if ip_address is not None: + details["ip_address"] = ip_address + self._emit_event(EventType.AUTH_FAILURE, details) + + def _emit_event(self, event_type: EventType, details: dict[str, str]) -> None: + """Emit one auth event and swallow publish-side failures.""" + bus = self._event_bus + if bus is None: + return + try: + bus.emit( + DomainEvent( + event_type=event_type, + details=details, + ) + ) + except Exception as exc: # pragma: no cover - defensive logging path + _logger.warning( + "auth_event_emit_failed", + event_type=event_type.value, + error_type=type(exc).__name__, + error_message=redact_value(str(exc)), + ) + + +__all__ = ["TokenAuthMiddleware"] diff --git a/src/cleveragents/shared/redaction.py b/src/cleveragents/shared/redaction.py index 25486dfe6..283ba6da4 100644 --- a/src/cleveragents/shared/redaction.py +++ b/src/cleveragents/shared/redaction.py @@ -42,6 +42,7 @@ _SENSITIVE_SUBSTRINGS: set[str] = { # though they contain a sensitive substring (e.g. "token_count"). _FALSE_POSITIVE_KEYS: set[str] = { "token_count", + "token_prefix", "token_limit", "token_usage", "input_tokens", From bf52a9c64854ed3f7d11fe05a2202e9dd85c9aa4 Mon Sep 17 00:00:00 2001 From: Jeffrey Phillips Freeman Date: Tue, 12 May 2026 05:51:17 +0000 Subject: [PATCH 02/10] fix(invariant): restore ACTION scope in merge_invariants and InvariantSet.merge precedence chain MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The 4-tier invariant precedence chain (plan > action > project > global) was broken at the domain layer — merge_invariants() and InvariantSet.merge() only accepted 3 parameters (plan, project, global), silently dropping all action- scoped invariants. Added action_invariants as a fourth parameter with proper backward compatibility (default to empty list). Updated module docstrings, InvariantScope docstring, and InvariantService.get_effective_invariants() to reflect the correct precedence chain. Added comprehensive BDD test scenarios covering four-tier merge precedence, action-before-project ordering, and effective invariant computation with all four scopes. ISSUES CLOSED: #9126 --- CHANGELOG.md | 3 +- CONTRIBUTORS.md | 2 +- features/consolidated_domain_models.feature | 103 +++++++++++++++++- features/steps/invariant_models_steps.py | 93 ++++++++++++++++ .../application/services/invariant_service.py | 25 ++++- .../domain/models/core/invariant.py | 53 ++++++--- 6 files changed, 254 insertions(+), 25 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f4bae43db..1a0d7a0c4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -201,7 +201,8 @@ Changed `wf10_batch.robot` to be less likely to create files, and were cleaned up in `features/actor_add_update_enforcement.feature` so the tests report correctly now that the underlying bug has been fixed. -- **Resolved Behave AmbiguousStep collisions in step definitions** (#4186): Renamed +- **Fixed `merge_invariants()` missing ACTION scope — 4-tier precedence restored** (#9126): Updated ``merge_invariants()`` and ``InvariantSet.merge()`` to accept a fourth parameter ``action_invariants`` alongside plan, project, and global tiers. The module docstring, ``InvariantScope`` docstring, and ``InvariantService.get_effective_invariants()`` now all reflect the correct precedence chain: ``plan > action > project > global``. Added ``action_name`` parameter to ``get_effective_invariants()`` so action-scoped invariants are collected and passed through the merge pipeline instead of silently dropped. All docstrings across both files were corrected from ``plan > project > global`` to ``plan > action > project > global``. Comprehensive Behave scenarios added covering four-tier merge precedence, action-before-project ordering, action override of project with same text, and effective invariant computation with all four scopes. + step texts to avoid case-sensitive collisions between different step modules that prevented all Behave tests from loading. Renamed steps in `edge_case_plan_steps.py`, `plan_executor_coverage_boost_steps.py`, diff --git a/CONTRIBUTORS.md b/CONTRIBUTORS.md index bfa0381b6..c33eb0f9b 100644 --- a/CONTRIBUTORS.md +++ b/CONTRIBUTORS.md @@ -15,7 +15,7 @@ Below are some of the specific details of various contributions. * Jeffrey Phillips Freeman has acted as Lead Developer, daily contributor, and Project Owner. -* Jeffrey Phillips Freeman has contributed an implementation for the invariant propagation fix (PR #10881 / issue #9131): added `_propagate_invariant_decisions()` to `SubplanService` to propagate all `invariant_enforced` decisions from parent plans to child plan decision trees during subplan spawn, satisfying the specification requirement for invariant propagation across hierarchical plan execution. +* Jeffrey Phillips Freeman has contributed the invariant merge precedence fix (#9126): restored the missing ACTION scope in ``merge_invariants()`` and ``InvariantSet.merge()``, corrected all module docstrings from ``plan > project > global`` to the spec-compliant ``plan > action > project > global``, and added comprehensive BDD test coverage for four-tier merge precedence. * Brent E. Edwards has contributed quality assurance, test coverage, and CI pipeline improvements. * 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. diff --git a/features/consolidated_domain_models.feature b/features/consolidated_domain_models.feature index c58469481..416a1b5f8 100644 --- a/features/consolidated_domain_models.feature +++ b/features/consolidated_domain_models.feature @@ -418,7 +418,7 @@ Feature: Consolidated Domain Models Scenario: InvariantScope has four values Then InvariantScope should have values "global, project, action, plan" - # === Merge Precedence (plan > project > global) === + # === Merge Precedence (plan > action > project > global) === Scenario: Merge with no duplicates preserves all invariants @@ -480,6 +480,75 @@ Feature: Consolidated Domain Models And the merged invariant at index 0 should have text "LOG ALL CHANGES" + Scenario: Merge with all four scopes preserves action tier — Issue #9126 + Given I have plan invariants + | text | source | + | Plan rule | plan1 | + And I have action invariants + | text | source | + | Action rule | action1 | + And I have project invariants + | text | source | + | Project rule | proj1 | + And I have global invariants + | text | source | + | Global rule | system | + When I merge the invariants + Then the merged set should have 4 invariants + And plan invariants appear before action invariants in merge + And action invariants appear before project invariants in merge + And the merged invariant at index 0 should have text "Plan rule" + And the merged invariant at index 1 should have text "Action rule" + And the merged invariant at index 2 should have text "Project rule" + And the merged invariant at index 3 should have text "Global rule" + + + Scenario: Action invariant overrides project with same text + Given I have plan invariants + | text | source | + And I have action invariants + | text | source | + | Log all changes | action1 | + And I have project invariants + | text | source | + | Log all changes | proj1 | + And I have global invariants + | text | source | + When I merge the invariants + Then the merged set should have 1 invariants + And the merged invariant at index 0 should have scope "action" + + + Scenario: Action invariant is included in de-duplication with plan override + Given I have plan invariants + | text | source | + | Log all changes | plan1 | + And I have action invariants + | text | source | + | Log all changes | action1 | + And I have project invariants + | text | source | + And I have global invariants + | text | source | + When I merge the invariants + Then the merged set should have 1 invariants + And the merged invariant at index 0 should have scope "plan" + + + Scenario: Inactive action invariants are excluded from merge + Given I have plan invariants with an inactive entry + And I have action invariants + | text | source | + | Active | action1 | + And I have project invariants + | text | source | + And I have global invariants + | text | source | + When I merge the invariants + Then the merged set should have 1 invariants + And the merged invariant at index 0 should have scope "action" + + Scenario: Inactive invariants are excluded from merge Given I have plan invariants with an inactive entry And I have project invariants @@ -492,7 +561,7 @@ Feature: Consolidated Domain Models # === InvariantSet merge class method === - Scenario: InvariantSet.merge produces correct result +Scenario: InvariantSet.merge produces correct result Given I have plan invariants | text | source | | Plan rule | plan1 | @@ -505,7 +574,25 @@ Feature: Consolidated Domain Models When I merge using InvariantSet Then the invariant set should have 3 invariants - # === Service: Add/List/Remove === + + Scenario: InvariantSet.merge with four-tier precedence — Issue #9126 + Given I have plan invariants + | text | source | + | Plan rule | plan1 | + And I have action invariants + | text | source | + | Action rule | action1 | + And I have project invariants + | text | source | + | Project rule | proj1 | + And I have global invariants + | text | source | + | Global rule | system | + When I merge using InvariantSet + Then the invariant set should have 4 invariants + + + # === Service: Add/List/Remove === Scenario: Service add invariant @@ -574,6 +661,16 @@ Feature: Consolidated Domain Models And the effective set should contain global invariants last + Scenario: Effective invariants include action scope — Issue #9126 + Given an invariant service with invariants at all four scopes + When I get effective invariants for plan "plan1", action "action1", and project "proj1" + Then the effective set should contain plan invariants first + And the effective set should contain action invariants second + And the effective set should contain project invariants after action + And the effective set should contain global invariants last + And the effective set should have 4 invariants + + Scenario: Effective invariants de-duplicate across scopes Given an invariant service with duplicate text across scopes When I get effective invariants for plan "plan1" and project "proj1" diff --git a/features/steps/invariant_models_steps.py b/features/steps/invariant_models_steps.py index b1909d647..00a73a3a6 100644 --- a/features/steps/invariant_models_steps.py +++ b/features/steps/invariant_models_steps.py @@ -135,6 +135,13 @@ def step_plan_invariants(context): context.plan_invariants = _parse_invariant_table(context, InvariantScope.PLAN) +@given("I have action invariants") +def step_action_invariants(context): + context.action_invariants = _parse_invariant_table( + context, InvariantScope.ACTION + ) + + @given("I have project invariants") def step_project_invariants(context): context.project_invariants = _parse_invariant_table(context, InvariantScope.PROJECT) @@ -160,6 +167,7 @@ def step_plan_invariants_inactive(context): def step_merge(context): context.merged = merge_invariants( getattr(context, "plan_invariants", []), + getattr(context, "action_invariants", []), getattr(context, "project_invariants", []), getattr(context, "global_invariants", []), ) @@ -180,6 +188,43 @@ def step_merged_scope(context, idx, scope): assert context.merged[idx].scope.value == scope +@then("action invariants appear before project invariants in merge") +def step_action_before_project(context): + """Verify ACTION tier comes before PROJECT tier in the merged result.""" + action_invs = [i for i in context.merged if i.scope == InvariantScope.ACTION] + project_invs = [i for i in context.merged if i.scope == InvariantScope.PROJECT] + assert len(action_invs) > 0, "No action invariants found in merged result" + assert len(project_invs) > 0, "No project invariants found in merged result" + first_action_idx = context.merged.index(action_invs[0]) + first_project_idx = context.merged.index(project_invs[0]) + assert first_action_idx < first_project_idx, ( + f"Action invariant at index {first_action_idx} " + f"should appear before project invariant at index {first_project_idx}" + ) + + +@then("plan invariants appear before action invariants in merge") +def step_plan_before_action(context): + """Verify PLAN tier comes before ACTION tier in the merged result.""" + plan_invs = [i for i in context.merged if i.scope == InvariantScope.PLAN] + action_invs = [i for i in context.merged if i.scope == InvariantScope.ACTION] + assert len(plan_invs) > 0, "No plan invariants found in merged result" + assert len(action_invs) > 0, "No action invariants found in merged result" + first_plan_idx = context.merged.index(plan_invs[0]) + first_action_idx = context.merged.index(action_invs[0]) + assert first_plan_idx < first_action_idx, ( + f"Plan invariant at index {first_plan_idx} " + f"should appear before action invariant at index {first_action_idx}" + ) + + +@then("action invariants are preserved in merge result") +def step_action_invariants_preserved(context): + """Verify that action-scoped invariants appear in the merged output.""" + action_invs = [i for i in context.merged if i.scope == InvariantScope.ACTION] + assert len(action_invs) > 0, "Expected action-scoped invariants in merge result" + + # ================================================================ # InvariantSet # ================================================================ @@ -189,6 +234,7 @@ def step_merged_scope(context, idx, scope): def step_merge_invariant_set(context): inv_set = InvariantSet.merge( getattr(context, "plan_invariants", []), + getattr(context, "action_invariants", []), getattr(context, "project_invariants", []), getattr(context, "global_invariants", []), ) @@ -389,6 +435,15 @@ def step_service_all_scopes(context): context.service.add_invariant("Plan rule", InvariantScope.PLAN, "plan1") +@given("an invariant service with invariants at all four scopes") +def step_service_all_four_scopes(context): + context.service = InvariantService() + context.service.add_invariant("Global rule", InvariantScope.GLOBAL, "system") + context.service.add_invariant("Project rule", InvariantScope.PROJECT, "proj1") + context.service.add_invariant("Action rule", InvariantScope.ACTION, "action1") + context.service.add_invariant("Plan rule", InvariantScope.PLAN, "plan1") + + @when('I get effective invariants for plan "{plan_id}" and project "{project}"') def step_effective(context, plan_id, project): context.effective = context.service.get_effective_invariants( @@ -396,6 +451,15 @@ def step_effective(context, plan_id, project): ) +@when( + 'I get effective invariants for plan "{plan_id}", action "{action}", and project "{project}"' +) +def step_effective_with_action(context, plan_id, action, project): + context.effective = context.service.get_effective_invariants( + plan_id=plan_id, action_name=action, project_name=project + ) + + @then("the effective set should contain plan invariants first") def step_effective_plan_first(context): plan_invs = [i for i in context.effective if i.scope == InvariantScope.PLAN] @@ -404,6 +468,35 @@ def step_effective_plan_first(context): assert first_plan_idx == 0 +@then("the effective set should contain action invariants second") +def step_effective_action_second(context): + """Verify ACTION tier appears after PLAN and before PROJECT.""" + plan_invs = [i for i in context.effective if i.scope == InvariantScope.PLAN] + action_invs = [i for i in context.effective if i.scope == InvariantScope.ACTION] + project_invs = [i for i in context.effective if i.scope == InvariantScope.PROJECT] + assert len(plan_invs) > 0, "No plan invariants found" + assert len(action_invs) > 0, "No action invariants found — bug #9126 not fixed!" + assert len(project_invs) > 0, "No project invariants found" + first_action_idx = context.effective.index(action_invs[0]) + if plan_invs: + last_plan_idx = context.effective.index(plan_invs[-1]) + assert first_action_idx > last_plan_idx + first_project_idx = context.effective.index(project_invs[0]) + assert first_project_idx > first_action_idx + + +@then("the effective set should contain project invariants after action") +def step_effective_project_after_action(context): + """Verify PROJECT tier appears after ACTION tier.""" + action_invs = [i for i in context.effective if i.scope == InvariantScope.ACTION] + project_invs = [i for i in context.effective if i.scope == InvariantScope.PROJECT] + assert len(action_invs) > 0, "No action invariants found" + assert len(project_invs) > 0, "No project invariants found" + first_proj_idx = context.effective.index(project_invs[0]) + last_action_idx = context.effective.index(action_invs[-1]) + assert first_proj_idx > last_action_idx + + @then("the effective set should contain project invariants second") def step_effective_project_second(context): proj_invs = [i for i in context.effective if i.scope == InvariantScope.PROJECT] diff --git a/src/cleveragents/application/services/invariant_service.py b/src/cleveragents/application/services/invariant_service.py index e941b999e..b5f31d718 100644 --- a/src/cleveragents/application/services/invariant_service.py +++ b/src/cleveragents/application/services/invariant_service.py @@ -11,8 +11,8 @@ a dict keyed by invariant ID. ## Merge Precedence -Effective invariants are computed using plan > project > global order. -See ``merge_invariants`` for de-duplication semantics. +Effective invariants are computed using plan > action > project > global +order. See ``merge_invariants`` for de-duplication semantics. Based on ``docs/specification.md`` and implementation plan Stage M3.5. """ @@ -167,16 +167,22 @@ class InvariantService: def get_effective_invariants( self, plan_id: str | None = None, + action_name: str | None = None, project_name: str | None = None, ) -> list[Invariant]: - """Return the merged precedence chain for a plan/project context. + """Return the merged precedence chain for a plan/action/project context. Collects active invariants from each scope tier and merges them - using plan > project > global precedence. + using plan > action > project > global precedence. Args: plan_id: Optional plan identifier to collect plan-scoped invariants. + action_name: Optional action name to collect action-scoped + invariants (promoted to plan-level during reconciliation). + When ``None``, the action tier is omitted for backward + compatibility. Pass ``"*"`` to include all action-scoped + invariants regardless of source name. project_name: Optional project name to collect project-scoped invariants. @@ -191,6 +197,13 @@ class InvariantService: if inv.scope == InvariantScope.PLAN and (plan_id is None or inv.source_name == plan_id) ] + action_invs = [ + inv + for inv in active + if inv.scope == InvariantScope.ACTION + and action_name is not None # Only include when explicitly requested + and (inv.source_name == action_name or action_name == "*") + ] project_invs = [ inv for inv in active @@ -199,7 +212,9 @@ class InvariantService: ] global_invs = [inv for inv in active if inv.scope == InvariantScope.GLOBAL] - return merge_invariants(plan_invs, project_invs, global_invs) + return merge_invariants( + plan_invs, action_invs, project_invs, global_invs + ) def enforce_invariants( self, diff --git a/src/cleveragents/domain/models/core/invariant.py b/src/cleveragents/domain/models/core/invariant.py index 1880bb668..feddf0f13 100644 --- a/src/cleveragents/domain/models/core/invariant.py +++ b/src/cleveragents/domain/models/core/invariant.py @@ -2,9 +2,10 @@ Invariants are natural-language constraints on plan execution, scoped at global, project, action, or plan level. When an action is used, its -invariants are promoted to plan-level. The runtime precedence chain is: +invariants are promoted to plan-level and participate in the merge. +The runtime precedence chain is: - plan > project > global + plan > action > project > global They are reconciled by the Invariant Reconciliation Actor at the start of Strategize and recorded as ``invariant_enforced`` decisions. @@ -21,8 +22,8 @@ of Strategize and recorded as ``invariant_enforced`` decisions. ## Merge Precedence When computing the effective set of invariants for a plan, the merge -order is **plan > project > global**. Duplicate texts (case-insensitive) -are de-duplicated, keeping the highest-precedence copy. +order is **plan > action > project > global**. Duplicate texts +(case-insensitive) are de-duplicated, keeping the highest-precedence copy. Based on ``docs/specification.md`` and implementation plan Stage M3.5. """ @@ -39,8 +40,10 @@ from ulid import ULID class InvariantScope(StrEnum): """Scope at which an invariant applies. - Precedence (highest to lowest): PLAN > PROJECT > GLOBAL. - ACTION invariants are promoted to PLAN scope at ``plan use`` time. + Precedence (highest to lowest): PLAN > ACTION > PROJECT > GLOBAL. + ACTION invariants are promoted to plan-level and participate in the + merge during reconciliation, sitting between PLAN and PROJECT in the + precedence chain. """ GLOBAL = "global" @@ -137,10 +140,11 @@ class InvariantSet(BaseModel): def merge( cls, plan_invariants: list[Invariant], - project_invariants: list[Invariant], - global_invariants: list[Invariant], + action_invariants: list[Invariant] | None = None, + project_invariants: list[Invariant] | None = None, + global_invariants: list[Invariant] | None = None, ) -> InvariantSet: - """Merge invariants respecting plan > project > global precedence. + """Merge invariants respecting plan > action > project > global precedence. De-duplicates by text (case-insensitive), keeping the copy from the highest-precedence tier. Within each tier, source ordering @@ -148,6 +152,9 @@ class InvariantSet(BaseModel): Args: plan_invariants: Plan-level invariants (highest precedence). + action_invariants: Action-scoped invariants (second-highest + precedence; promoted to plan-level when an + action is used). project_invariants: Project-level invariants. global_invariants: Global-level invariants (lowest precedence). @@ -156,7 +163,12 @@ class InvariantSet(BaseModel): """ return cls( invariants=tuple( - merge_invariants(plan_invariants, project_invariants, global_invariants) + merge_invariants( + plan_invariants, + action_invariants or [], + project_invariants or [], + global_invariants or [], + ) ) ) @@ -165,10 +177,11 @@ class InvariantSet(BaseModel): def merge_invariants( plan_invariants: list[Invariant], - project_invariants: list[Invariant], - global_invariants: list[Invariant], + action_invariants: list[Invariant] | None = None, + project_invariants: list[Invariant] | None = None, + global_invariants: list[Invariant] | None = None, ) -> list[Invariant]: - """Merge invariants implementing plan > project > global precedence. + """Merge invariants implementing plan > action > project > global precedence. De-duplicates by text (case-insensitive). The first occurrence (from the highest-precedence tier) wins. Within each tier, the @@ -176,8 +189,13 @@ def merge_invariants( Args: plan_invariants: Plan-level invariants (highest precedence). - project_invariants: Project-level invariants. + action_invariants: Action-scoped invariants (second-highest + precedence; promoted to plan level when an action is used). + Defaults to empty list when ``None``. + project_invariants: Project-level invariants. Defaults to empty + list when ``None``. global_invariants: Global-level invariants (lowest precedence). + Defaults to empty list when ``None``. Returns: A de-duplicated list of invariants in precedence order. @@ -185,7 +203,12 @@ def merge_invariants( seen: set[str] = set() result: list[Invariant] = [] - for inv_list in (plan_invariants, project_invariants, global_invariants): + for inv_list in ( + plan_invariants, + action_invariants or [], + project_invariants or [], + global_invariants or [], + ): for inv in inv_list: if not inv.active: continue From 761622f746bfdcf68f44522803ea7d41f952ecda Mon Sep 17 00:00:00 2001 From: CleverThis Date: Thu, 14 May 2026 01:12:36 +0000 Subject: [PATCH 03/10] fix(invariant): pass action_name in list_invariants when scope is ACTION Fixes list_invariants(effective=True) to forward action_name to get_effective_invariants when scope is ACTION, ensuring action-scoped invariants are included in effective invariant lists. Also applies ruff formatting to the return statement in get_effective_invariants. Addresses reviewer observation about list_invariants gap. Refs: #9126 --- features/steps/invariant_models_steps.py | 4 +--- src/cleveragents/application/services/invariant_service.py | 5 ++--- 2 files changed, 3 insertions(+), 6 deletions(-) diff --git a/features/steps/invariant_models_steps.py b/features/steps/invariant_models_steps.py index 00a73a3a6..9963ba0ea 100644 --- a/features/steps/invariant_models_steps.py +++ b/features/steps/invariant_models_steps.py @@ -137,9 +137,7 @@ def step_plan_invariants(context): @given("I have action invariants") def step_action_invariants(context): - context.action_invariants = _parse_invariant_table( - context, InvariantScope.ACTION - ) + context.action_invariants = _parse_invariant_table(context, InvariantScope.ACTION) @given("I have project invariants") diff --git a/src/cleveragents/application/services/invariant_service.py b/src/cleveragents/application/services/invariant_service.py index b5f31d718..9427a2c40 100644 --- a/src/cleveragents/application/services/invariant_service.py +++ b/src/cleveragents/application/services/invariant_service.py @@ -124,6 +124,7 @@ class InvariantService: if effective: return self.get_effective_invariants( plan_id=source_name if scope == InvariantScope.PLAN else None, + action_name=source_name if scope == InvariantScope.ACTION else None, project_name=source_name if scope == InvariantScope.PROJECT else None, ) @@ -212,9 +213,7 @@ class InvariantService: ] global_invs = [inv for inv in active if inv.scope == InvariantScope.GLOBAL] - return merge_invariants( - plan_invs, action_invs, project_invs, global_invs - ) + return merge_invariants(plan_invs, action_invs, project_invs, global_invs) def enforce_invariants( self, From f4cea72248034c53bd517798453f7bf441bc209c Mon Sep 17 00:00:00 2001 From: CleverThis Date: Fri, 15 May 2026 01:43:32 +0000 Subject: [PATCH 04/10] fix(ci): update all merge_invariants callers to use 4-param signature The PR #11143 adds action_invariants as a 4th parameter to merge_invariants() and InvariantSet.merge(), but two call sites were not updated: - benchmarks/invariant_merge_bench.py: 5 calls with 3 positional args - robot/helper_m3_e2e_verification.py: 2 calls using keyword args All call sites now pass action_invariants=[] for backward-compatible empty-action behavior. --- benchmarks/invariant_merge_bench.py | 10 +++++----- robot/helper_m3_e2e_verification.py | 2 ++ 2 files changed, 7 insertions(+), 5 deletions(-) diff --git a/benchmarks/invariant_merge_bench.py b/benchmarks/invariant_merge_bench.py index d68db24cf..87e5e853b 100644 --- a/benchmarks/invariant_merge_bench.py +++ b/benchmarks/invariant_merge_bench.py @@ -74,7 +74,7 @@ class MergeSmallSuite: def time_merge_small(self) -> None: """Benchmark merge with ~13 invariants.""" - merge_invariants(self.plan, self.project, self.global_invs) + merge_invariants(self.plan, [], self.project, self.global_invs) class MergeMediumSuite: @@ -88,7 +88,7 @@ class MergeMediumSuite: def time_merge_medium(self) -> None: """Benchmark merge with ~50 invariants.""" - merge_invariants(self.plan, self.project, self.global_invs) + merge_invariants(self.plan, [], self.project, self.global_invs) class MergeLargeSuite: @@ -102,7 +102,7 @@ class MergeLargeSuite: def time_merge_large(self) -> None: """Benchmark merge with ~250 invariants.""" - merge_invariants(self.plan, self.project, self.global_invs) + merge_invariants(self.plan, [], self.project, self.global_invs) class MergeDeduplicationSuite: @@ -137,7 +137,7 @@ class MergeDeduplicationSuite: def time_merge_dedup(self) -> None: """Benchmark merge with 60 invariants, all duplicates.""" - merge_invariants(self.plan, self.project, self.global_invs) + merge_invariants(self.plan, [], self.project, self.global_invs) class InvariantSetMergeSuite: @@ -151,7 +151,7 @@ class InvariantSetMergeSuite: def time_invariant_set_merge(self) -> None: """Benchmark InvariantSet.merge().""" - InvariantSet.merge(self.plan, self.project, self.global_invs) + InvariantSet.merge(self.plan, [], self.project, self.global_invs) class ServiceEffectiveSuite: diff --git a/robot/helper_m3_e2e_verification.py b/robot/helper_m3_e2e_verification.py index bfd06a911..84e9d78c1 100644 --- a/robot/helper_m3_e2e_verification.py +++ b/robot/helper_m3_e2e_verification.py @@ -867,6 +867,7 @@ def invariants_enforced_during_strategize() -> None: merged = merge_invariants( plan_invariants=[plan_inv], + action_invariants=[], project_invariants=[project_inv], global_invariants=[global_inv], ) @@ -890,6 +891,7 @@ def invariants_enforced_during_strategize() -> None: invariant_set = InvariantSet.merge( plan_invariants=[plan_inv], + action_invariants=[], project_invariants=[project_inv], global_invariants=[global_inv], ) From 5c5309f35d565fec75db48bb8595a50ba4a652aa Mon Sep 17 00:00:00 2001 From: CleverThis Date: Sat, 16 May 2026 00:13:40 +0000 Subject: [PATCH 05/10] fix(ci): add missing effective set count step definition The Behave scenario at line 671 of consolidated_domain_models.feature asserts 'the effective set should have {count:d} invariants' but no step handler existed, causing UndefinedStepError and CI failure. Adds the missing step: @then('the effective set should have {count:d} invariants') to step_invariant_models_steps.py, mirroring the existing invariant-set count pattern. --- features/steps/invariant_models_steps.py | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/features/steps/invariant_models_steps.py b/features/steps/invariant_models_steps.py index 9963ba0ea..d5c8b025c 100644 --- a/features/steps/invariant_models_steps.py +++ b/features/steps/invariant_models_steps.py @@ -526,6 +526,13 @@ def step_no_duplicates(context): assert len(texts) == len(set(texts)), f"Duplicates found: {texts}" +@then("the effective set should have {count:d} invariants") +def step_effective_count(context, count): + assert len(context.effective) == count, ( + f"Expected {count}, got {len(context.effective)}" + ) + + # ================================================================ # Enforcement # ================================================================ From 86f96f299e2ea0673290c293f7f9856d272ec2fe Mon Sep 17 00:00:00 2001 From: CleverThis Date: Fri, 15 May 2026 05:18:37 +0000 Subject: [PATCH 06/10] fix(cli): Mask database URL credentials in agents info CLI output (#8395) --- features/db_url_sanitisation.feature | 53 ++++++++++++ features/steps/db_url_sanitisation_steps.py | 93 +++++++++++++++++++++ src/cleveragents/cli/commands/system.py | 38 ++++++++- 3 files changed, 183 insertions(+), 1 deletion(-) create mode 100644 features/db_url_sanitisation.feature create mode 100644 features/steps/db_url_sanitisation_steps.py diff --git a/features/db_url_sanitisation.feature b/features/db_url_sanitisation.feature new file mode 100644 index 000000000..a5f12ae1f --- /dev/null +++ b/features/db_url_sanitisation.feature @@ -0,0 +1,53 @@ +Feature: Database URL sanitisation — credentials must never be exposed + + As a CleverAgents operator + I want database URLs in CLI output to have their credentials masked + So that running ``agents info`` never leaks passwords or API tokens + +@tdd_bug_8395 + Scenario Outline: PostgreSQL URL with user and password — all are masked + Given the database url is "" + When I run sanitise_db_url + Then the sanitised database url should be "" + + Examples: + | url | expected | + | postgresql://user:secret@localhost/mydb | postgresql://***:***@localhost/mydb | + | postgresql://admin:p%40ssw0rd@db.example.com:5432/agents | postgresql://***:***@db.example.com:5432/agents | + | postgresql://readonly:r0ad_only@pg.cluster.internal:5433/production | postgresql://***:***@pg.cluster.internal:5433/production | + + Scenario Outline: MySQL URL with credentials — all are masked + Given the database url is "" + When I run sanitise_db_url + Then the sanitised database url should be "" + + Examples: + | url | expected | + | mysql://app:s3cret@mysql.internal:3306/agents | mysql://***:***@mysql.internal:3306/agents | + | mysql+pymysql://root:toor@localhost/testdb | mysql+pymysql://***:***@localhost/testdb | + + Scenario Outline: SQLite URLs — remain unchanged (no credentials) + Given the database url is "" + When I run sanitise_db_url + Then the sanitised database url should be "" + + Examples: + | url | expected | + | sqlite:///data/cleveragents.db | sqlite:///data/cleveragents.db | + | sqlite:////absolute/path/to/db.sqlite | sqlite:////absolute/path/to/db.sqlite | + | memory | memory | + + Scenario: Username-only URL — password is still masked + Given the database url is "postgres://deploy@db.example.com/prod" + When I run sanitise_db_url + Then the sanitised database url should be "postgres://***:***@db.example.com/prod" + + Scenario: SQLite URL without credentials remains unchanged + Given the database url is "sqlite:///test.db" + When I run sanitise_db_url + Then the sanitised database url should be "sqlite:///test.db" + + Scenario: build_info_data returns sanitised database URL + Given a mock settings object with database url "postgresql://admin:supersecret@host.example.com:5432/mydb" + When I call build_info_data + Then the database field in info data should be "postgresql://***:***@host.example.com:5432/mydb" diff --git a/features/steps/db_url_sanitisation_steps.py b/features/steps/db_url_sanitisation_steps.py new file mode 100644 index 000000000..58a1ceb2b --- /dev/null +++ b/features/steps/db_url_sanitisation_steps.py @@ -0,0 +1,93 @@ +"""Step definitions for db_url_sanitisation.feature. + +Tests the ``_sanitise_db_url`` helper and its use in ``build_info_data``. +""" + +from __future__ import annotations + +from pathlib import Path +from tempfile import mkdtemp +from unittest.mock import MagicMock, patch + +from behave import given, then, when +from behave.runner import Context + + +# --------------------------------------------------------------------------- +# Given +# --------------------------------------------------------------------------- + + +@given('the database url is "{url}"') +def step_db_url_given(context: Context, url: str) -> None: + """Store the raw database URL for processing.""" + context.raw_db_url = url + + +@given( + "a mock settings object with database url \"{db_url}\"" +) +def step_mock_settings_with_db_url(context: Context, db_url: str) -> None: + """Build a full mock Settings that ``build_info_data`` expects.""" + tmpdir = mkdtemp() + mock_settings = MagicMock() + mock_settings.database_url = db_url + mock_settings.data_dir = Path(tmpdir) + mock_settings.storage_path = Path(tmpdir) + mock_settings.config_path = Path(tmpdir) / "config.toml" + mock_settings.log_dir = Path(tmpdir) / "logs" + mock_settings.default_automation_profile = "auto" + mock_settings.has_provider_configured = MagicMock(return_value=False) + mock_settings.configured_provider_names = MagicMock(return_value=[]) + mock_settings.debug_enabled = False + context.mock_settings = mock_settings + + +# --------------------------------------------------------------------------- +# When +# --------------------------------------------------------------------------- + + +@when("I run sanitise_db_url") +def step_run_sanitise(context: Context) -> None: + """Call _sanitise_db_url with the stored raw URL.""" + from cleveragents.cli.commands.system import _sanitise_db_url + + context.sanitised_url = _sanitise_db_url(context.raw_db_url) + + +@when("I call build_info_data") +def step_call_build_info(context: Context) -> None: + """Call ``build_info_data`` with the mock settings.""" + from cleveragents.cli.commands.system import build_info_data + + with patch( + "cleveragents.config.settings.get_settings", + return_value=context.mock_settings, + ): + context.info_data = build_info_data() + + +# --------------------------------------------------------------------------- +# Then +# --------------------------------------------------------------------------- + + +@then('the sanitised database url should be "{expected}"') +def step_assert_sanitised_url(context: Context, expected: str) -> None: + """Verify the sanitised URL matches expectation.""" + actual = context.sanitised_url + assert actual == expected, ( + f"Expected '{expected}', got '{actual}'" + ) + + +@then( + "the database field in info data should be \"{expected}\"" +) +def step_assert_info_db_field(context: Context, expected: str) -> None: + """Verify the sanitised URL appears correctly in build_info_data output.""" + actual = context.info_data["database"] + assert actual == expected, ( + f"Expected database field '{expected}', got '{actual}'" + ) diff --git a/src/cleveragents/cli/commands/system.py b/src/cleveragents/cli/commands/system.py index 42bba1fe5..31a3dccc8 100644 --- a/src/cleveragents/cli/commands/system.py +++ b/src/cleveragents/cli/commands/system.py @@ -17,6 +17,7 @@ from datetime import UTC, datetime from enum import StrEnum from pathlib import Path from typing import Any +from urllib.parse import urlparse, urlunparse from sqlalchemy import create_engine from sqlalchemy import inspect as sa_inspect @@ -82,6 +83,41 @@ def _dep_version(package: str) -> str: return "not installed" +def _sanitise_db_url(url: str) -> str: + """Sanitise a database URL by masking credentials. + + Parses the URL and replaces username/password components with masked + placeholders so that CLI output never exposes real credentials. + + Examples:: + + >>> _sanitise_db_url("postgresql://user:secret@localhost/mydb") + 'postgresql://***:***@localhost/mydb' + >>> _sanitise_db_url("sqlite:///path/to/db.sqlite") + 'sqlite:///path/to/db.sqlite' + + Args: + url: The raw database URL. + + Returns: + The sanitised URL with credentials masked (or unchanged if no + username/password is present). + """ + parsed = urlparse(url) + + # If there is no userinfo segment, return as-is (e.g. sqlite URLs) + if not parsed.username and not parsed.password: + return url + + # Rebuild the netloc with masked credentials + userinfo = "***:***" + port_part = f":{parsed.port}" if parsed.port else "" + new_netloc = f"{userinfo}@{parsed.hostname}{port_part}" + + sanitized = parsed._replace(netloc=new_netloc) + return urlunparse(sanitized) + + def build_version_data() -> dict[str, Any]: """Assemble structured data for the ``version`` command.""" return { @@ -145,7 +181,7 @@ def build_info_data() -> dict[str, Any]: "version": __version__, "data_dir": str(data_dir), "config_path": str(config_path), - "database": db_url, + "database": _sanitise_db_url(db_url), "server_mode": server_mode, "platform": f"{platform.system()} {platform.release()} ({platform.machine()})", "automation": settings.default_automation_profile, From 23d73e7fb2114d08ec1efa27939454529a5a17a9 Mon Sep 17 00:00:00 2001 From: Jeffrey Freeman Date: Sat, 16 May 2026 04:23:18 +0000 Subject: [PATCH 07/10] chore: Fix ruff format violations in db_url_sanitisation_steps.py Fix formatting issues detected by CI lint check: - Simplify multi-line decorator arguments to single line - Simplify multi-line assertion error messages to single line This resolves the format --check failure blocking CI. --- features/steps/db_url_sanitisation_steps.py | 16 ++++------------ 1 file changed, 4 insertions(+), 12 deletions(-) diff --git a/features/steps/db_url_sanitisation_steps.py b/features/steps/db_url_sanitisation_steps.py index 58a1ceb2b..9ff628028 100644 --- a/features/steps/db_url_sanitisation_steps.py +++ b/features/steps/db_url_sanitisation_steps.py @@ -24,9 +24,7 @@ def step_db_url_given(context: Context, url: str) -> None: context.raw_db_url = url -@given( - "a mock settings object with database url \"{db_url}\"" -) +@given('a mock settings object with database url "{db_url}"') def step_mock_settings_with_db_url(context: Context, db_url: str) -> None: """Build a full mock Settings that ``build_info_data`` expects.""" tmpdir = mkdtemp() @@ -77,17 +75,11 @@ def step_call_build_info(context: Context) -> None: def step_assert_sanitised_url(context: Context, expected: str) -> None: """Verify the sanitised URL matches expectation.""" actual = context.sanitised_url - assert actual == expected, ( - f"Expected '{expected}', got '{actual}'" - ) + assert actual == expected, f"Expected '{expected}', got '{actual}'" -@then( - "the database field in info data should be \"{expected}\"" -) +@then('the database field in info data should be "{expected}"') def step_assert_info_db_field(context: Context, expected: str) -> None: """Verify the sanitised URL appears correctly in build_info_data output.""" actual = context.info_data["database"] - assert actual == expected, ( - f"Expected database field '{expected}', got '{actual}'" - ) + assert actual == expected, f"Expected database field '{expected}', got '{actual}'" From b3851693c8397406ac4651b0b841f895f7396723 Mon Sep 17 00:00:00 2001 From: Rui Hu Date: Fri, 15 May 2026 07:50:19 +0000 Subject: [PATCH 08/10] fix(reactive): forward actor options block to LLM constructor for custom backend support Two code paths in the reactive actor run pipeline silently discarded the options: block from v3 actor YAML, preventing custom OpenAI-compatible backends (llama.cpp, Ollama, etc.) from being used. Review fixes applied: - Fix 1: Relabeled issue #11223 from Type/Task to Type/Bug; added @tdd_issue/@tdd_issue_11223 tags to all 5 Behave scenarios. - Fix 2: openai_api_key in options now routes through the registry's __api_key_sentinel mechanism so user-provided keys correctly override environment defaults. (stream_router.py) - Fix 3: type: graph actors now propagate actor-level options to individual node configs via setdefault. (config_parser.py) - Fix 4: Options keys are validated against an explicit allowlist; reserved keys (provider_type, model_id) are excluded; unrecognized keys log a WARNING instead of being silently forwarded. (stream_router.py) - Fix 5: Updated _build_from_v3 docstring to list options as a propagated field. (config_parser.py) - Fix 6: Removed inconsistent and options_raw emptiness guard; empty options dicts are now preserved consistently. (config_parser.py) - Fix 7: Reserved keys provider_type and model_id are excluded from the options merge loop to prevent TypeError. (stream_router.py) - Fix 8: Added Behave scenario verifying top-level temperature takes precedence over options duplicate. (consolidated_routing.feature + steps) - Fix 9: Strengthened "no extra kwargs" assertion to assert kwargs == {} directly instead of using an allow-list filter. (stream_router steps) - Fix 10: Strengthened options assertion to exact dict equality. (actor_v3_schema_extended_steps.py) - N1: Comment style aligned to M5: prefix convention. - N2: Type annotations changed from Any to Context (behave.runner). - N3: Added Behave scenario for empty options: {} dict behavior. Tests: 5 new Behave scenarios (3 in actor_v3_schema.feature, 2 in consolidated_routing.feature) with @tdd_issue/@tdd_issue_11223 tags. ISSUES CLOSED: #11223 --- features/actor_v3_schema.feature | 21 ++++ features/consolidated_routing.feature | 21 ++++ .../steps/actor_v3_schema_extended_steps.py | 75 ++++++++++++ ...am_router_unsafe_and_llm_coverage_steps.py | 115 +++++++++++++++++- src/cleveragents/reactive/config_parser.py | 14 ++- src/cleveragents/reactive/stream_router.py | 49 ++++++++ 6 files changed, 291 insertions(+), 4 deletions(-) diff --git a/features/actor_v3_schema.feature b/features/actor_v3_schema.feature index ce50e35a5..8238c8c48 100644 --- a/features/actor_v3_schema.feature +++ b/features/actor_v3_schema.feature @@ -211,3 +211,24 @@ Feature: v3 Actor YAML schema support in CLI and registry Given a v3 graph actor config dict with skills and lsp as dict When I parse the v3 config through ReactiveConfigParser Then each graph node agent should have lsp dict propagated + + # M5: options block forwarded to agent_config in _build_from_v3 (#11223). + @tdd_issue @tdd_issue_11223 + Scenario: ReactiveConfigParser propagates options block to agent config + Given a v3 LLM actor config dict with options block + When I parse the v3 config through ReactiveConfigParser + Then the agent config should contain the options block + + # M5: actor without options block is unaffected (#11223). + @tdd_issue @tdd_issue_11223 + Scenario: ReactiveConfigParser handles v3 actor without options block + Given a v3 LLM actor config dict without options block + When I parse the v3 config through ReactiveConfigParser + Then the agent config should not contain an options key + + # M5: empty options dict is preserved (#11223). + @tdd_issue @tdd_issue_11223 + Scenario: ReactiveConfigParser preserves empty options block + Given a v3 LLM actor config dict with empty options block + When I parse the v3 config through ReactiveConfigParser + Then the agent config should contain an empty options block diff --git a/features/consolidated_routing.feature b/features/consolidated_routing.feature index 49febb07d..61ce7abdd 100644 --- a/features/consolidated_routing.feature +++ b/features/consolidated_routing.feature @@ -1379,3 +1379,24 @@ Feature: Consolidated Routing When the stream router tool agent processes "keep_me" through operation Then the stream router tool agent operation result should be "keep_me" + # M5: SimpleLLMAgent._resolve_llm forwards options to registry.create_llm (#11223). + @tdd_issue @tdd_issue_11223 + Scenario: SimpleLLMAgent forwards options block to LLM constructor + Given a stream router llm agent with options block containing custom base url + When the stream router llm agent resolves the llm + Then the llm constructor should have received the custom base url + + # M5: actor without options block is unaffected (#11223). + @tdd_issue @tdd_issue_11223 + Scenario: SimpleLLMAgent without options block calls LLM constructor without extra kwargs + Given a stream router llm agent without options block + When the stream router llm agent resolves the llm + Then the llm constructor should not have received extra kwargs + + # M5: top-level keys take precedence over duplicates in options (#11223). + @tdd_issue @tdd_issue_11223 + Scenario: SimpleLLMAgent top-level key takes precedence over options duplicate + Given a stream router llm agent with top-level temperature and options block + When the stream router llm agent resolves the llm + Then the llm constructor should have received the top-level temperature + diff --git a/features/steps/actor_v3_schema_extended_steps.py b/features/steps/actor_v3_schema_extended_steps.py index 69857db61..36f53e8c9 100644 --- a/features/steps/actor_v3_schema_extended_steps.py +++ b/features/steps/actor_v3_schema_extended_steps.py @@ -14,6 +14,7 @@ from unittest.mock import patch import yaml from behave import given, then, when +from behave.runner import Context from cleveragents.actor.compiler import ActorCompilationError from cleveragents.actor.config import ActorConfiguration @@ -781,3 +782,77 @@ def step_graph_nodes_have_lsp_dict(context: Any) -> None: assert lsp.get("auto") is True, ( f"Node '{node_id}' expected lsp.auto=True, got {lsp}" ) + + +# ── M5: options block forwarding (#11223) ───────────────────────────────── + + +@given("a v3 LLM actor config dict with options block") +def step_v3_llm_config_with_options(context: Context) -> None: + context.v3_config = { + "name": "local/my-llama-actor", + "type": "llm", + "description": "Local llama.cpp actor", + "provider": "openai", + "model": "my-local-model", + "options": { + "openai_api_base": "http://localhost:8080/v1", + "openai_api_key": "none", + "temperature": 1.0, + }, + "system_prompt": "You are a helpful assistant.", + } + + +@given("a v3 LLM actor config dict without options block") +def step_v3_llm_config_without_options(context: Context) -> None: + context.v3_config = { + "name": "local/standard-actor", + "type": "llm", + "description": "Standard actor without options", + "provider": "openai", + "model": "gpt-4", + "system_prompt": "You are a helpful assistant.", + } + + +@then("the agent config should contain the options block") +def step_agent_config_has_options(context: Context) -> None: + agents = context.reactive_config.agents + agent = next(iter(agents.values())) + options = agent.config.get("options") + assert options == { + "openai_api_base": "http://localhost:8080/v1", + "openai_api_key": "none", + "temperature": 1.0, + }, f"Expected exact options dict, got {options}" + + +@then("the agent config should not contain an options key") +def step_agent_config_no_options(context: Context) -> None: + agents = context.reactive_config.agents + agent = next(iter(agents.values())) + assert "options" not in agent.config, ( + f"Expected no 'options' key in agent config, got {agent.config}" + ) + + +@given("a v3 LLM actor config dict with empty options block") +def step_v3_llm_config_with_empty_options(context: Context) -> None: + context.v3_config = { + "name": "local/empty-options-actor", + "type": "llm", + "description": "Actor with empty options", + "provider": "openai", + "model": "gpt-4", + "options": {}, + "system_prompt": "You are a helpful assistant.", + } + + +@then("the agent config should contain an empty options block") +def step_agent_config_has_empty_options(context: Context) -> None: + agents = context.reactive_config.agents + agent = next(iter(agents.values())) + options = agent.config.get("options") + assert options == {}, f"Expected empty options dict, got {options}" diff --git a/features/steps/stream_router_unsafe_and_llm_coverage_steps.py b/features/steps/stream_router_unsafe_and_llm_coverage_steps.py index ebf7edad3..d7c33c0e1 100644 --- a/features/steps/stream_router_unsafe_and_llm_coverage_steps.py +++ b/features/steps/stream_router_unsafe_and_llm_coverage_steps.py @@ -6,7 +6,8 @@ from __future__ import annotations -from unittest.mock import MagicMock +from typing import Any +from unittest.mock import MagicMock, patch from behave import given, then, when from behave.runner import Context @@ -160,3 +161,115 @@ def step_then_tool_agent_operation_result(context: Context, expected: str) -> No assert context.op_result == expected, ( f"Expected '{expected}', got '{context.op_result}'" ) + + +# --------------------------------------------------------------------------- +# M5: SimpleLLMAgent._resolve_llm forwards options to create_llm (#11223). +# --------------------------------------------------------------------------- + + +@given("a stream router llm agent with options block containing custom base url") +def step_given_llm_agent_with_options(context: Context) -> None: + context.llm_options_agent = SimpleLLMAgent( + "test_options", + { + "provider": "openai", + "model": "my-local-model", + "options": { + "openai_api_base": "http://localhost:8080/v1", + "openai_api_key": "none", + }, + }, + ) + context.create_llm_kwargs: dict[str, Any] = {} + + +@given("a stream router llm agent without options block") +def step_given_llm_agent_without_options(context: Context) -> None: + context.llm_options_agent = SimpleLLMAgent( + "test_no_options", + { + "provider": "openai", + "model": "gpt-4", + }, + ) + context.create_llm_kwargs = {} + + +@when("the stream router llm agent resolves the llm") +def step_when_llm_agent_resolves(context: Context) -> None: + mock_llm = MagicMock() + captured: dict[str, Any] = {} + + def fake_create_llm( + provider_type: Any = None, model_id: Any = None, **kwargs: Any + ) -> Any: + captured["provider_type"] = provider_type + captured["model_id"] = model_id + captured["kwargs"] = kwargs + return mock_llm + + mock_registry = MagicMock() + mock_registry.create_llm.side_effect = fake_create_llm + + with patch( + "cleveragents.reactive.stream_router.get_provider_registry", + return_value=mock_registry, + ): + context.llm_options_agent._resolve_llm() + + context.create_llm_kwargs = captured + + +@then("the llm constructor should have received the custom base url") +def step_then_llm_received_custom_base_url(context: Context) -> None: + kwargs = context.create_llm_kwargs.get("kwargs", {}) + assert "openai_api_base" in kwargs, ( + f"Expected 'openai_api_base' in create_llm kwargs, got {kwargs}" + ) + assert kwargs["openai_api_base"] == "http://localhost:8080/v1", ( + f"Expected openai_api_base='http://localhost:8080/v1', got {kwargs}" + ) + # openai_api_key is routed through __api_key_sentinel so the + # registry can distinguish explicit keys from environment defaults. + assert kwargs.get("__api_key_sentinel") == "none", ( + f"Expected __api_key_sentinel='none' (from options.openai_api_key), " + f"got {kwargs}" + ) + + +@then("the llm constructor should not have received extra kwargs") +def step_then_llm_no_extra_kwargs(context: Context) -> None: + kwargs = context.create_llm_kwargs.get("kwargs", {}) + # When neither options nor top-level params are present, nothing + # should be forwarded to the LLM constructor. + assert kwargs == {}, f"Expected empty kwargs in create_llm call, got {kwargs}" + + +@given("a stream router llm agent with top-level temperature and options block") +def step_given_llm_agent_with_precedence(context: Context) -> None: + context.llm_options_agent = SimpleLLMAgent( + "test_precedence", + { + "provider": "openai", + "model": "gpt-4", + "temperature": 0.5, + "options": { + "temperature": 1.0, + "max_tokens": 4096, + }, + }, + ) + context.create_llm_kwargs: dict[str, Any] = {} + + +@then("the llm constructor should have received the top-level temperature") +def step_then_llm_received_top_level_temperature(context: Context) -> None: + kwargs = context.create_llm_kwargs.get("kwargs", {}) + assert kwargs.get("temperature") == 0.5, ( + f"Expected temperature=0.5 (top-level precedence over options), got {kwargs}" + ) + # max_tokens from options should still be forwarded. + assert kwargs.get("max_tokens") == 4096, ( + f"Expected max_tokens=4096 from options, got {kwargs}" + ) diff --git a/src/cleveragents/reactive/config_parser.py b/src/cleveragents/reactive/config_parser.py index f37f4d8ca..ea8a21d8c 100644 --- a/src/cleveragents/reactive/config_parser.py +++ b/src/cleveragents/reactive/config_parser.py @@ -286,9 +286,9 @@ class ReactiveConfigParser: are propagated via the agent config so the runtime can attach them. All v3 fields — ``context_view``, ``memory``, ``context``, - ``env_vars``, ``response_format``, ``lsp_capabilities``, and - ``lsp_context_enrichment`` — are propagated into the agent config - so the runtime can consume them. + ``env_vars``, ``response_format``, ``lsp_capabilities``, + ``lsp_context_enrichment``, and ``options`` — are propagated + into the agent config so the runtime can consume them. """ # m4: guard against None-to-string coercion for all three fields. actor_type = str(data.get("type") or "llm").lower() @@ -375,6 +375,11 @@ class ReactiveConfigParser: if isinstance(response_format, dict): agent_config["response_format"] = response_format + # M5: propagate actor options block for custom backend support (#11223). + options_raw = data.get("options") + if isinstance(options_raw, dict): + agent_config["options"] = dict(options_raw) + if actor_type in ("llm", "tool"): # Single-agent synthesis. rc.agents[actor_name] = AgentConfig( @@ -424,6 +429,9 @@ class ReactiveConfigParser: else dict(lsp_bindings) ) node_config.setdefault("lsp", lsp_copy) + # M5: propagate actor-level options to graph nodes (#11223). + if isinstance(options_raw, dict) and options_raw: + node_config.setdefault("options", dict(options_raw)) nodes_map[node_id] = node_config # Create a reactive agent for each graph node. diff --git a/src/cleveragents/reactive/stream_router.py b/src/cleveragents/reactive/stream_router.py index a02d4950c..6a2082312 100644 --- a/src/cleveragents/reactive/stream_router.py +++ b/src/cleveragents/reactive/stream_router.py @@ -226,6 +226,55 @@ class SimpleLLMAgent: llm_kwargs["max_tokens"] = max_tokens if max_retries is not None: llm_kwargs["max_retries"] = max_retries + # M5: merge options block so custom LLM backend kwargs + # (e.g. openai_api_base, openai_api_key) are forwarded to the + # LLM constructor. Options are applied after the fixed keys so + # that explicit top-level keys (temperature, max_tokens, etc.) + # take precedence over duplicates in options. + # + # openai_api_key is handled specially: the registry uses a + # __api_key_sentinel mechanism to distinguish explicitly + # provided keys from environment-sourced ones. If the user + # supplies openai_api_key in options, we extract it and inject + # it via the sentinel so it overrides the registry's default. + options = dict(self.config.get("options") or {}) + if "openai_api_key" in options: + llm_kwargs["__api_key_sentinel"] = options.pop("openai_api_key") + # Ensure sensitive/reserved ChatOpenAI constructor params cannot + # be injected via a crafted actor YAML. + _ALLOWED_OPTIONS: frozenset[str] = frozenset( + { + "openai_api_base", + "temperature", + "max_tokens", + "timeout", + "top_p", + "frequency_penalty", + "presence_penalty", + } + ) + _RESERVED: frozenset[str] = frozenset({"provider_type", "model_id"}) + for key, value in options.items(): + if key in _RESERVED: + logger_sr.warning( + "Actor '%s' options block contains reserved key '%s' " + "that conflicts with positional arguments; ignoring.", + self.name, + key, + ) + continue + if key not in llm_kwargs: + if key in _ALLOWED_OPTIONS: + llm_kwargs[key] = value + else: + logger_sr.warning( + "Actor '%s' options block contains unrecognized " + "key '%s'; ignoring for security. Allowed keys: " + "%s.", + self.name, + key, + sorted(_ALLOWED_OPTIONS), + ) registry = get_provider_registry() self._llm = registry.create_llm( provider_type=provider, model_id=model, **llm_kwargs From e2167ab8e70ee05708f3354f996bedc201a5f3da Mon Sep 17 00:00:00 2001 From: "hamza.khyari" Date: Fri, 15 May 2026 11:58:23 +0000 Subject: [PATCH 09/10] fix(acms): use project-level hot_max_tokens in execute phase context assembly Fixed _resolve_hot_max_tokens() to read hot_max_tokens from the correct sub-key in context_policy_json. The value is stored under context_policy_json["acms_config"]["hot_max_tokens"] by 'agents project context set --hot-max-tokens', not at the top level. The previous read (config_dict.get("hot_max_tokens")) always returned None, causing the assembler to silently use the global 16K default even when a project-level override was configured. Also adds two Behave regression scenarios with @tdd_issue @tdd_issue_11035 tags that exercise the real DB query code path via a mocked NamespacedProjectModel row, verifying: 1. hot_max_tokens=32000 in acms_config is applied to CoreContextBudget and ContextRequest (override path). 2. Missing hot_max_tokens falls back to the constructor-injected global default of 4096 (fallback path). Module-level import json added to steps file; redundant inline MagicMock import removed. ISSUES CLOSED: #11035 ISSUES CLOSED: #11215 --- CHANGELOG.md | 10 ++ ...e_phase_context_assembler_coverage.feature | 18 +++ ..._phase_context_assembler_coverage_steps.py | 110 ++++++++++++++++++ .../execute_phase_context_assembler.py | 5 +- 4 files changed, 142 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1a0d7a0c4..e20fc86cb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -122,6 +122,16 @@ Changed `wf10_batch.robot` to be less likely to create files, and - **Plan Rollback Command** (#8557): Implemented `agents plan rollback []` for checkpoint-based plan state restoration in Epic #8493. The command restores a plan's sandbox to the state captured at a given checkpoint, discarding all decisions made after that checkpoint. The checkpoint can be specified as an optional positional second argument or via the `--to-checkpoint` named option. Supports `--yes/-y` flag to skip confirmation prompts and `--format/-f` for output format selection (rich/plain/json/yaml). Included with comprehensive BDD test coverage (>= 97%) and spec-aligned output formatting showing rollback summary, changes reverted, impact analysis, and post-rollback state panels. ### Fixed +- **ACMS execute-phase assembler respects project-level hot_max_tokens** (#11035): Fixed + ``_resolve_hot_max_tokens()`` to read ``hot_max_tokens`` from + ``context_policy_json["acms_config"]["hot_max_tokens"]`` — the correct sub-key written + by ``agents project context set --hot-max-tokens``. The previous implementation read + from the top-level key (``config_dict.get("hot_max_tokens")``), which was always + ``None``, causing the assembler to silently fall back to the global 16K default even + when a project-level override was configured. Also adds two Behave regression scenarios + with ``@tdd_issue @tdd_issue_11035`` tags that exercise the DB query code path and + verify the project-level budget is applied to ``CoreContextBudget`` and + ``ContextRequest``. - **Guard cleanup_stale against execute/processing and execute/complete plans** (#11121): ``_create_sandbox_for_plan()`` in ``src/cleveragents/cli/commands/plan.py`` now skips ``GitWorktreeSandbox.cleanup_stale()`` when the plan is in diff --git a/features/execute_phase_context_assembler_coverage.feature b/features/execute_phase_context_assembler_coverage.feature index d4f95a5ba..f9f26069d 100644 --- a/features/execute_phase_context_assembler_coverage.feature +++ b/features/execute_phase_context_assembler_coverage.feature @@ -235,3 +235,21 @@ Feature: Execute-phase context assembler coverage When epcov I call assemble on the plan Then epcov the assembled result should be an AssembledContext And epcov the assembled result should have fragments and metadata + + # ---- _resolve_hot_max_tokens: reads context_policy_json from DB ---- + + @tdd_issue @tdd_issue_11035 + Scenario: epcov assemble uses project-level hot_max_tokens from context_policy_json + Given epcov an assembler with context_policy_json hot_max_tokens 32000 + And epcov a plan with project links + And epcov scoped fragments that pass all filters + When epcov I call assemble on the plan + Then epcov the pipeline received budget max_tokens of 32000 + + @tdd_issue @tdd_issue_11035 + Scenario: epcov assemble falls back to global hot_max_tokens when context_policy_json has no override + Given epcov an assembler with no hot_max_tokens in context_policy_json + And epcov a plan with project links + And epcov scoped fragments that pass all filters + When epcov I call assemble on the plan + Then epcov the pipeline received budget max_tokens of 4096 diff --git a/features/steps/execute_phase_context_assembler_coverage_steps.py b/features/steps/execute_phase_context_assembler_coverage_steps.py index fd8303acd..53a0ddf2d 100644 --- a/features/steps/execute_phase_context_assembler_coverage_steps.py +++ b/features/steps/execute_phase_context_assembler_coverage_steps.py @@ -12,6 +12,7 @@ Exercises all uncovered lines in from __future__ import annotations +import json from typing import Any from unittest.mock import MagicMock, patch @@ -937,3 +938,112 @@ def step_epcov_assembled_has_data(context: Context) -> None: assert result.budget_used >= 0.0, "Expected non-negative budget_used" assert result.context_hash, "Expected non-empty context_hash" assert result.strategies_used, "Expected non-empty strategies_used" + + +# --------------------------------------------------------------------------- +# _resolve_hot_max_tokens: reads hot_max_tokens from context_policy_json +# --------------------------------------------------------------------------- + + +def _make_assembler_with_policy_json( + policy_json: str | None, +) -> ACMSExecutePhaseContextAssembler: + """Build an assembler whose DB session returns a row with *policy_json*.""" + pr = _make_pipeline_result() + pr.fragments = () + pr.total_tokens = 5 + pr.budget_used = 0.1 + pr.strategies_used = ("relevance",) + pr.context_hash = "hash-policy" + pr.preamble = None + pr.provenance_map = {} + + mock_pipeline = MagicMock() + mock_pipeline.assemble.return_value = pr + + tier_service = MagicMock() + # Return a minimal passing fragment so assemble() does not short-circuit + frag = _make_tiered_fragment( + fragment_id="frag-policy", + content="content", + token_count=5, + metadata={"path": "src/good.py"}, + ) + tier_service.get_scoped_view.return_value = [frag] + + # Build a mock row with context_policy_json + mock_row = MagicMock() + mock_row.context_policy_json = policy_json + + # Mock the session so _resolve_hot_max_tokens can query it + mock_session = MagicMock() + mock_session.query.return_value.filter_by.return_value.first.return_value = mock_row + + repo = MagicMock() + repo._session.return_value = mock_session + repo.get_context_policy.return_value = ProjectContextPolicy() + + return ACMSExecutePhaseContextAssembler( + context_tier_service=tier_service, + project_repository=repo, + acms_pipeline=mock_pipeline, + hot_max_tokens=4096, + ) + + +@given("epcov an assembler with context_policy_json hot_max_tokens 32000") +def step_epcov_assembler_policy_json_32k(context: Context) -> None: + """Assembler backed by a DB row with hot_max_tokens=32000 in policy JSON. + + Mirrors the real storage format written by + ``agents project context set --hot-max-tokens 32000``: + ``{"acms_config": {"hot_max_tokens": 32000, ...}, ...}``. + """ + policy_json = json.dumps({"acms_config": {"hot_max_tokens": 32000}}) + context.epcov_assembler = _make_assembler_with_policy_json(policy_json) + + +@given("epcov an assembler with no hot_max_tokens in context_policy_json") +def step_epcov_assembler_policy_json_no_override(context: Context) -> None: + """Assembler backed by a DB row with no hot_max_tokens — global fallback.""" + policy_json = json.dumps({"acms_config": {"other_setting": "value"}}) + context.epcov_assembler = _make_assembler_with_policy_json(policy_json) + + +@then("epcov the pipeline received budget max_tokens of 32000") +def step_epcov_pipeline_budget_32k(context: Context) -> None: + """Verify the pipeline was called with max_tokens=32000 in the budget.""" + if context.epcov_error is not None: + raise AssertionError(f"Unexpected error: {context.epcov_error}") + pipeline = context.epcov_assembler._pipeline + assert pipeline.assemble.called, "Pipeline.assemble() was not called" + call_args = pipeline.assemble.call_args + budget = call_args.kwargs.get("budget") or ( + call_args[1].get("budget") if call_args[1] else None + ) + if budget is None and call_args[0]: + budget = call_args[0][2] if len(call_args[0]) > 2 else None + assert budget is not None, "Could not find budget in pipeline.assemble() call" + assert budget.max_tokens == 32000, ( + f"Expected budget.max_tokens=32000 (from context_policy_json), " + f"got {budget.max_tokens}" + ) + + +@then("epcov the pipeline received budget max_tokens of 4096") +def step_epcov_pipeline_budget_global(context: Context) -> None: + """Verify the pipeline fell back to the global hot_max_tokens=4096.""" + if context.epcov_error is not None: + raise AssertionError(f"Unexpected error: {context.epcov_error}") + pipeline = context.epcov_assembler._pipeline + assert pipeline.assemble.called, "Pipeline.assemble() was not called" + call_args = pipeline.assemble.call_args + budget = call_args.kwargs.get("budget") or ( + call_args[1].get("budget") if call_args[1] else None + ) + if budget is None and call_args[0]: + budget = call_args[0][2] if len(call_args[0]) > 2 else None + assert budget is not None, "Could not find budget in pipeline.assemble() call" + assert budget.max_tokens == 4096, ( + f"Expected budget.max_tokens=4096 (global fallback), got {budget.max_tokens}" + ) diff --git a/src/cleveragents/application/services/execute_phase_context_assembler.py b/src/cleveragents/application/services/execute_phase_context_assembler.py index d0dc746af..da2af9895 100644 --- a/src/cleveragents/application/services/execute_phase_context_assembler.py +++ b/src/cleveragents/application/services/execute_phase_context_assembler.py @@ -116,7 +116,10 @@ class ACMSExecutePhaseContextAssembler(ExecutePhaseContextAssembler): if row is not None and row.context_policy_json is not None: try: config_dict = json.loads(cast(str, row.context_policy_json)) - tokens = config_dict.get("hot_max_tokens") + # hot_max_tokens is stored under the "acms_config" sub-key + # by agents project context set. + acms = config_dict.get("acms_config") or {} + tokens = acms.get("hot_max_tokens") if tokens is not None and isinstance(tokens, int) and tokens > 0: candidates.append(tokens) except (ValueError, TypeError): From 20ad9a46c42e281c09f5c01a533d5e39d6582061 Mon Sep 17 00:00:00 2001 From: CleverThis Date: Sat, 16 May 2026 08:27:35 +0000 Subject: [PATCH 10/10] fix(data-integrity): remove silent argument swap in ValidationAttachmentRepository.attach Fixes a critical data integrity bug where validation_name and resource_id arguments were being silently swapped based on a fragile heuristic when resource_id contained '/'. This caused silent data corruption without any error being raised. The 3-line conditional swap block has been removed from ValidationAttachmentRepository.attach(), ensuring arguments flow directly from caller to the persistence layer in the correct order. --- CHANGELOG.md | 6 ++++++ src/cleveragents/infrastructure/database/repositories.py | 3 --- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e20fc86cb..98811b5b3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,12 @@ Changed `wf10_batch.robot` to be less likely to create files, and ## [Unreleased] +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. + - Hardened the TDD bug-fix quality gate for issue #629: PR parsing now requires whole-word closing keywords (avoids false positives like "prefixes #12"), TDD bug tag discovery now uses exact token matching diff --git a/src/cleveragents/infrastructure/database/repositories.py b/src/cleveragents/infrastructure/database/repositories.py index c8f9e6144..95da8038d 100644 --- a/src/cleveragents/infrastructure/database/repositories.py +++ b/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