diff --git a/.semgrep.yml b/.semgrep.yml index 2bb1e8164..449a59448 100644 --- a/.semgrep.yml +++ b/.semgrep.yml @@ -21,7 +21,8 @@ rules: include: - src/ exclude: - - src/cleveragents/tool/wrapping.py + # Intentional controlled-sandbox exec in transform pipeline. + - src/cleveragents/tool/wrapping.py - id: no-compile-exec pattern: compile(..., ..., "exec") @@ -34,7 +35,8 @@ rules: include: - src/ exclude: - - src/cleveragents/tool/wrapping.py + # Intentional controlled-sandbox compile in transform pipeline. + - src/cleveragents/tool/wrapping.py - id: no-os-system pattern: os.system(...) diff --git a/features/steps/tdd_json_decode_crash_persistence_steps.py b/features/steps/tdd_json_decode_crash_persistence_steps.py index c39fb71be..b0fb2959f 100644 --- a/features/steps/tdd_json_decode_crash_persistence_steps.py +++ b/features/steps/tdd_json_decode_crash_persistence_steps.py @@ -1,7 +1,10 @@ """Steps for TDD bug-capture scenario #989. -This scenario intentionally fails until the bugfix branch for #989 adds -corruption-specific handling around automation profile JSON decoding. +This file implements the Behave steps for bug #989 — corrupt JSON in +automation profile persistence. The ``@tdd_expected_fail`` tag has been +removed because the bugfix is included. These steps now verify the +corrected behavior: a ``CorruptRecordError`` is raised instead of a raw +``json.JSONDecodeError``. """ from __future__ import annotations @@ -20,6 +23,7 @@ from cleveragents.infrastructure.database.models import ( ) from cleveragents.infrastructure.database.repositories import ( AutomationProfileRepository, + CorruptRecordError, ) @@ -70,7 +74,7 @@ def step_given_corrupt_safety_json_row(context: Any) -> None: @when("the corrupt automation profile is fetched by name for bug 989") def step_when_fetch_corrupt_profile(context: Any) -> None: - """Execute repository get path that currently leaks JSONDecodeError.""" + """Execute repository get path that wraps JSONDecodeError in CorruptRecordError.""" repo: AutomationProfileRepository = context.tdd_989_repo try: repo.get_by_name("bug-989-corrupt-json") @@ -82,7 +86,7 @@ def step_when_fetch_corrupt_profile(context: Any) -> None: "a corruption-specific domain error should be raised instead of JSONDecodeError for bug 989" ) def step_then_domain_specific_error(context: Any) -> None: - """Assert expected post-fix behavior (fails today while bug exists).""" + """Assert that a CorruptRecordError is raised, not a raw JSONDecodeError.""" err = context.tdd_989_error assert err is not None, ( "Expected a corruption-specific domain error, but no error was raised" @@ -91,8 +95,141 @@ def step_then_domain_specific_error(context: Any) -> None: "Expected a domain-specific corruption error, " f"but raw JSONDecodeError leaked: {err}" ) + assert isinstance(err, CorruptRecordError), ( + f"Expected CorruptRecordError, got {type(err).__name__}: {err}" + ) name_or_message = f"{type(err).__name__} {err}".lower() assert "corrupt" in name_or_message, ( - "Expected corruption-specific error semantics (type/message containing 'corrupt'), " - f"got {type(err).__name__}: {err}" + "Expected corruption-specific error semantics " + f"(type/message containing 'corrupt'), got {type(err).__name__}: {err}" ) + + +@given("an automation profile repository row with corrupt guards_json for bug 989") +def step_given_corrupt_guards_json_row(context: Any) -> None: + """Persist a row with valid safety_json but corrupt guards_json.""" + engine = create_engine("sqlite+pysqlite:///:memory:", future=True) + Base.metadata.create_all(bind=engine) + factory = sessionmaker(bind=engine, expire_on_commit=False, future=True) + + now_iso = _now_iso() + with factory() as session: + session.add( + AutomationProfileModel( + name="bug-989-corrupt-guards", + description="Corrupt guards_json regression fixture", + schema_version="1.0", + decompose_task=0.0, + create_tool=0.0, + select_tool=0.0, + edit_code=0.0, + execute_command=0.0, + create_file=0.0, + delete_content=0.0, + access_network=0.0, + install_dependency=0.0, + modify_config=0.0, + approve_plan=0.0, + require_sandbox=True, + require_checkpoints=True, + allow_unsafe_tools=False, + safety_json=None, + # Deliberately malformed JSON + guards_json="{not valid json!", + created_at=now_iso, + updated_at=now_iso, + ) + ) + session.commit() + + context.tdd_989_guards_repo = AutomationProfileRepository(factory) + context.tdd_989_guards_error = None + + +@when("the corrupt guards automation profile is fetched by name for bug 989") +def step_when_fetch_corrupt_guards_profile(context: Any) -> None: + """Execute repository get path for corrupt guards_json.""" + repo: AutomationProfileRepository = context.tdd_989_guards_repo + try: + repo.get_by_name("bug-989-corrupt-guards") + except Exception as exc: + context.tdd_989_guards_error = exc + + +@then("a corruption-specific domain error should be raised for guards_json for bug 989") +def step_then_guards_domain_specific_error(context: Any) -> None: + """Assert CorruptRecordError is raised for corrupt guards_json.""" + err = context.tdd_989_guards_error + assert err is not None, ( + "Expected a CorruptRecordError for corrupt guards_json, but no error was raised" + ) + assert isinstance(err, CorruptRecordError), ( + f"Expected CorruptRecordError, got {type(err).__name__}: {err}" + ) + assert err.field == "guards_json", ( + f"Expected field='guards_json', got field='{err.field}'" + ) + + +@given("a domain profile object with malformed safety data for bug 989") +def step_given_malformed_safety_profile(context: Any) -> None: + """Create a mock profile with safety that cannot be serialized.""" + + class _BrokenSafety: + """Safety object whose model_dump raises TypeError.""" + + require_sandbox: bool = True + require_checkpoints: bool = True + allow_unsafe_tools: bool = False + + def model_dump(self) -> None: + raise TypeError("model_dump intentionally broken for bug 989 test") + + class _FakeProfile: + """Minimal profile stub with broken safety.""" + + name: str = "bug-989-malformed" + description: str = "malformed safety" + schema_version: str = "1.0" + decompose_task: float = 0.0 + create_tool: float = 0.0 + select_tool: float = 0.0 + edit_code: float = 0.0 + execute_command: float = 0.0 + create_file: float = 0.0 + delete_content: float = 0.0 + access_network: float = 0.0 + install_dependency: float = 0.0 + modify_config: float = 0.0 + approve_plan: float = 0.0 + safety: _BrokenSafety = _BrokenSafety() + guards: None = None + + context.tdd_989_malformed_profile = _FakeProfile() + context.tdd_989_from_domain_error = None + + +@when("_from_domain is called with the malformed profile for bug 989") +def step_when_from_domain_malformed(context: Any) -> None: + """Call _from_domain with a profile that will fail serialization.""" + try: + AutomationProfileRepository._from_domain( + context.tdd_989_malformed_profile, _now_iso() + ) + except Exception as exc: + context.tdd_989_from_domain_error = exc + + +@then( + "a corruption-specific domain error should be raised for the serialization for bug 989" +) +def step_then_from_domain_error(context: Any) -> None: + """Assert CorruptRecordError is raised when _from_domain fails to serialize.""" + err = context.tdd_989_from_domain_error + assert err is not None, ( + "Expected a CorruptRecordError from _from_domain, but no error was raised" + ) + assert isinstance(err, CorruptRecordError), ( + f"Expected CorruptRecordError, got {type(err).__name__}: {err}" + ) + assert err.field == "safety", f"Expected field='safety', got field='{err.field}'" diff --git a/features/tdd_json_decode_crash_persistence.feature b/features/tdd_json_decode_crash_persistence.feature index 1a8f84b15..fcc3c7bd3 100644 --- a/features/tdd_json_decode_crash_persistence.feature +++ b/features/tdd_json_decode_crash_persistence.feature @@ -1,14 +1,25 @@ -@tdd_issue @tdd_issue_989 @tdd_expected_fail +@tdd_issue @tdd_issue_989 Feature: TDD Bug #989 — automation profile persistence crashes on corrupt JSON As a developer reading persisted automation profiles I want corrupt JSON payloads to be handled with a domain-specific error So that callers do not receive a raw JSONDecodeError crash - # This test captures bug #989. It intentionally uses @tdd_expected_fail - # until the bugfix for #989 is merged. The underlying assertion currently - # fails (proving the bug exists) and the tag inversion makes CI pass. + # This test captures bug #989. The @tdd_expected_fail tag has been removed + # because the bugfix for #989 is included in the same commit. The test now + # verifies the corrected behavior: a CorruptRecordError is raised instead + # of a raw JSONDecodeError. Scenario: Bug #989 — get_by_name should not leak JSONDecodeError for corrupt safety_json Given an automation profile repository row with corrupt safety_json for bug 989 When the corrupt automation profile is fetched by name for bug 989 Then a corruption-specific domain error should be raised instead of JSONDecodeError for bug 989 + + Scenario: Bug #989 — corrupt guards_json also raises CorruptRecordError + Given an automation profile repository row with corrupt guards_json for bug 989 + When the corrupt guards automation profile is fetched by name for bug 989 + Then a corruption-specific domain error should be raised for guards_json for bug 989 + + Scenario: Bug #989 — _from_domain raises CorruptRecordError for malformed safety + Given a domain profile object with malformed safety data for bug 989 + When _from_domain is called with the malformed profile for bug 989 + Then a corruption-specific domain error should be raised for the serialization for bug 989 diff --git a/src/cleveragents/infrastructure/database/repositories.py b/src/cleveragents/infrastructure/database/repositories.py index ce10d80d1..5c1465e4b 100644 --- a/src/cleveragents/infrastructure/database/repositories.py +++ b/src/cleveragents/infrastructure/database/repositories.py @@ -4367,6 +4367,30 @@ class AutomationProfileSchemaVersionError(DatabaseError): """Raised on schema_version mismatch during update.""" +class CorruptRecordError(DatabaseError): + """Raised when a persisted record contains corrupt or malformed data. + + This wraps low-level deserialization errors (e.g. ``json.JSONDecodeError``, + ``TypeError``) so that callers receive a domain-specific exception rather + than an opaque infrastructure error. + """ + + def __init__(self, record_name: str, field: str, detail: str) -> None: + """Initialise with context about the corrupt record. + + Args: + record_name: Identifier of the record (e.g. profile name). + field: Name of the field that contained corrupt data. + detail: Description of the deserialization failure. + """ + super().__init__( + f"Corrupt data in record '{record_name}', field '{field}': {detail}" + ) + self.record_name = record_name + self.field = field + self.detail = detail + + # --------------------------------------------------------------------------- # AutomationProfileRepository # --------------------------------------------------------------------------- @@ -4554,7 +4578,12 @@ class AutomationProfileRepository: def _to_domain( row: AutomationProfileModel, ) -> Any: - """Convert an ORM row to an AutomationProfile domain object.""" + """Convert an ORM row to an AutomationProfile domain object. + + Raises: + CorruptRecordError: If stored JSON fields are malformed or cannot + be deserialized into the expected domain objects. + """ import json as _json from cleveragents.domain.models.core.automation_profile import ( @@ -4563,13 +4592,22 @@ class AutomationProfileRepository: ) from cleveragents.domain.models.core.safety_profile import SafetyProfile + profile_name = cast(str, row.name) + # Restore full safety from JSON if available, else fall back to # legacy scalar columns for backward compatibility. safety_json_str = ( cast(str | None, row.safety_json) if hasattr(row, "safety_json") else None ) if safety_json_str: - safety = SafetyProfile(**_json.loads(safety_json_str)) + try: + safety = SafetyProfile(**_json.loads(safety_json_str)) + except (_json.JSONDecodeError, TypeError) as exc: + raise CorruptRecordError( + record_name=profile_name, + field="safety_json", + detail=str(exc), + ) from exc else: safety = SafetyProfile( require_sandbox=bool(row.require_sandbox), @@ -4585,10 +4623,17 @@ class AutomationProfileRepository: ) guards: AutomationGuard | None = None if guards_json_str: - guards = AutomationGuard(**_json.loads(guards_json_str)) + try: + guards = AutomationGuard(**_json.loads(guards_json_str)) + except (_json.JSONDecodeError, TypeError) as exc: + raise CorruptRecordError( + record_name=profile_name, + field="guards_json", + detail=str(exc), + ) from exc return AutomationProfile( - name=cast(str, row.name), + name=profile_name, description=cast(str, row.description) or "", schema_version=cast(str, row.schema_version), decompose_task=float(cast(float, row.decompose_task)), @@ -4611,15 +4656,37 @@ class AutomationProfileRepository: profile: Any, now_iso: str, ) -> AutomationProfileModel: - """Convert an AutomationProfile to an ORM row.""" + """Convert an AutomationProfile to an ORM row. + + Raises: + CorruptRecordError: If the domain object cannot be serialized, + indicating the caller passed malformed data. + """ import json as _json - safety_json = ( - _json.dumps(profile.safety.model_dump()) if profile.safety else None - ) - guards_json = ( - _json.dumps(profile.guards.model_dump()) if profile.guards else None - ) + profile_name: str = getattr(profile, "name", "") + + try: + safety_json = ( + _json.dumps(profile.safety.model_dump()) if profile.safety else None + ) + except (AttributeError, TypeError) as exc: + raise CorruptRecordError( + record_name=profile_name, + field="safety", + detail=str(exc), + ) from exc + + try: + guards_json = ( + _json.dumps(profile.guards.model_dump()) if profile.guards else None + ) + except (AttributeError, TypeError) as exc: + raise CorruptRecordError( + record_name=profile_name, + field="guards", + detail=str(exc), + ) from exc return AutomationProfileModel( name=profile.name, diff --git a/vulture_whitelist.py b/vulture_whitelist.py index 991afa4b2..09aea2413 100644 --- a/vulture_whitelist.py +++ b/vulture_whitelist.py @@ -143,6 +143,7 @@ AutomationProfileRepository # noqa: B018, F821 AutomationProfileNotFoundError # noqa: B018, F821 DuplicateAutomationProfileError # noqa: B018, F821 AutomationProfileSchemaVersionError # noqa: B018, F821 +CorruptRecordError # noqa: B018, F821 _LEVEL_TO_PROFILE # noqa: B018, F821 _resolve_profile_for_plan # noqa: B018, F821 default_automation_profile # noqa: B018, F821