diff --git a/CHANGELOG.md b/CHANGELOG.md index 4aba343af..83d92c0d8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -165,6 +165,12 @@ ensuring data is stored with proper parameter values. ### Fixed +- **fileConfig error handling in alembic env.py** (#7874): Wrapped the `fileConfig()` + call in `alembic/env.py` with a `try/except` block that catches malformed INI logging + configuration and emits a clear, user-actionable error message to stderr (including the + config file path and guidance on the `[loggers]` section) before exiting with code 1. + Includes Behave scenarios for the error-handling logic. + - **`agents project context show` JSON/YAML output** (#6323): Fixed structured output to include spec-required envelope (`command`, `status`, `exit_code`, `data`, `timing`, `messages`) and all four data sections (`context_policy`, `limits`, `summarization`, `current_usage`). Rich output now renders four panels including the new `Current Usage` panel. ### Added @@ -1132,4 +1138,4 @@ iteration` and data corruption under concurrent plan execution. All public - **TUI -- Permission Question Widget**: A new inline `PermissionQuestionWidget` renders permission requests directly in the conversation stream for single-key operations. Users can allow/reject with single-key shortcuts (`a`/`A`/`r`/`R`), - navigate with arrow keys, confirm with `Enter`, or press `v` to open the full \ No newline at end of file + navigate with arrow keys, confirm with `Enter`, or press `v` to open the full diff --git a/CONTRIBUTORS.md b/CONTRIBUTORS.md index fbb10a7e0..6ddd7ce76 100644 --- a/CONTRIBUTORS.md +++ b/CONTRIBUTORS.md @@ -58,3 +58,4 @@ Below are some of the specific details of various contributions. * HAL 9000 has contributed the config-actor combined-format support fix (PR #11232 / issue #11189): added ``_detect_nested_config_actor()``, ``_flatten_config_actor()``, and handling in ``ActorConfiguration.from_blob()`` to transparently flatten the nested ``config.actor`` block from both compact-string and nested-dict forms so v3 detection, schema validation, and canonicalisation see flat data — eliminating the ``"provider is required"`` crash. * HAL 9000 has contributed the actor compiler `actor_ref` field fix (issue #1429): corrected `_map_node()` and `compile_actor()` in `src/cleveragents/actor/compiler.py` to read `actor_ref` from the top-level `NodeDefinition.actor_ref` field instead of `node.config.get("actor_ref")`, resolving silent failures on all SUBGRAPH nodes where `subgraph_refs` was always empty and `NodeConfig.subgraph` was always `None`. * HAL 9000 has contributed the removal of the unsupported executable resource type (PR #3248 / issue #3077): removed `executable` from `LSP_RESOURCE_TYPES` and `BUILTIN_TYPE_NAMES`, updated `agents resource list` CLI table columns to the spec-required `[Name, ID, Type, Phys/Virt, Children, Projects]`, deleted orphaned `examples/resource-types/executable.yaml`, and updated related BDD test coverage. +* HAL 9000 has contributed the alembic fileConfig error handling fix (PR #8288 / issue #7874): wrapped the `fileConfig()` call in `alembic/env.py` with a `try/except` block to catch malformed INI logging configuration and emit clear, actionable error messages to stderr. diff --git a/features/steps/tdd_fileconfig_unhandled_exception_steps.py b/features/steps/tdd_fileconfig_unhandled_exception_steps.py new file mode 100644 index 000000000..2612e015d --- /dev/null +++ b/features/steps/tdd_fileconfig_unhandled_exception_steps.py @@ -0,0 +1,164 @@ +"""Step definitions for alembic fileConfig error handling (issue #7874). + +Each scenario uses its own isolated temp directory, preventing cross-test +contamination. The malformed / valid INI files are written into those dirs +and the ``alembic`` CLI invocation is executed from within them so that +``fileConfig()`` reads directly from the target file under test. +""" + +from __future__ import annotations + +import os +import subprocess +import tempfile +from pathlib import Path +from typing import Any + +from behave import given, then, when + +# Absolute path to the project's alembic migrations package so that the +# subprocess invocations execute the real env.py (not a stub), ensuring the +# production fileConfig() error-handling path is covered by these scenarios. +_MIGRATIONS_DIR = str( + Path(__file__).resolve().parent.parent.parent + / "src" + / "cleveragents" + / "infrastructure" + / "database" + / "migrations" +) + + +def _write_malformed_ini(directory: Path) -> Path: + """Write a deliberately malformed alembic.ini to *directory*.""" + ini = directory / "alembic.ini" + ini.write_text( + "[alembic]\n" + f"script_location = {_MIGRATIONS_DIR}\n" + "\n" + # Reference a logger that does NOT exist anywhere. fileConfig() + # raises ``KeyError`` at startup when the referenced key is absent + # from the INI file, producing an unhandled exception before any + # migration runs. + "[loggers]\n" + "keys = root,invalid_nonexistent_logger\n" + "\n", + ) + return ini + + +def _write_valid_ini(directory: Path) -> Path: + """Write a fully-valid alembic.ini to *directory*.""" + ini = directory / "alembic.ini" + ini.write_text( + "[alembic]\n" + f"script_location = {_MIGRATIONS_DIR}\n" + "\n" + "[loggers]\n" + "keys = root\n" + "\n" + "[handlers]\n" + "keys = stream_handler\n" + "\n" + "[formatters]\n" + "keys = simple\n" + "\n" + "[logger_root]\n" + "level = WARN\n" + "handlers = stream_handler\n" + "\n" + "[handler_stream_handler]\n" + "class = StreamHandler\n" + "args = (sys.stderr,)\n" + "\n" + "[formatter_simple]\n" + "format = %(levelname)s:%(name)s:%(message)s\n", + ) + return ini + + +@given("a fresh temporary directory") +def step_given_fresh_temp_dir(context: Any) -> None: + context.test_dir = Path(tempfile.mkdtemp(prefix="fileconfig_test_")) + + +@when( + "the alembic.ini contains an invalid logger section" + " that references nonexistent loggers" +) +def step_when_invalid_log_section(context: Any) -> None: + """Write a malformed alembic.ini to the test directory.""" + _write_malformed_ini(context.test_dir) + + +@when("the alembic.ini contains a malformed logging configuration") +def step_when_malformed_config(context: Any) -> None: + """Alias for when steps that write a malformed config.""" + _write_malformed_ini(context.test_dir) + + +@when("migrations are invoked normally") +def step_when_invocations_normal(context: Any) -> None: + """Invoke ``alembic current`` to trigger env.py startup (fileConfig).""" + env = { + **os.environ, + "CLEVERAGENTS_DATABASE_URL": f"sqlite:///{context.test_dir}/test.db", + } + result = subprocess.run( + ["alembic", "current"], + cwd=context.test_dir, + capture_output=True, + text=True, + timeout=10, + env=env, + ) + context.migration_result = result + + +@when("the alembic.ini contains a valid logger configuration") +def step_when_valid_config(context: Any) -> None: + """Write a fully-valid alembic.ini to the test directory.""" + _write_valid_ini(context.test_dir) + + +@then( + 'the error output on stderr includes actionable guidance about "[loggers]" section' +) +def step_then_stderr_includes_guidance(context: Any) -> None: + """Assert that the error message contains ``[loggers]`` guidance.""" + result = _assert_invocation(context) + assert result.returncode != 0, ( + f"Expected non-zero exit code on configuration error. Got: {result.returncode}" + ) + stderr = result.stderr + assert "[loggers]" in stderr or "loggers" in stderr.lower(), ( + f"Error must be user-actionable and mention [loggers]. Got:\n{stderr}" + ) + + +@then('the error output on stderr includes the config file path "alembic.ini"') +def step_then_stderr_includes_alembic_ini(context: Any) -> None: + """Assert that the error message contains the config file path.""" + result = _assert_invocation(context) + stderr = result.stderr + assert "alembic.ini" in stderr, f"Error must include 'alembic.ini'. Got:\n{stderr}" + + +@then("no error message appears on stderr") +def step_then_no_stderr_error(context: Any) -> None: + """Assert that the alembic invocation produced no stderr output.""" + result = _assert_invocation(context) + assert not result.stderr.strip(), ( + f"Unexpected stderr when config is valid:\n{result.stderr}" + ) + + +def _assert_invocation(context: Any) -> subprocess.CompletedProcess[str]: + """Utility: assert that a migration invocation has been recorded.""" + if not hasattr(context, "migration_result"): + msg = ( + "No migration result recorded. Did all required *When* steps execute? " + "The test flow must be: Given … → When … → When … → Then …" + ) + raise AssertionError(msg) + return context.migration_result diff --git a/features/tdd_fileconfig_unhandled_exception.feature b/features/tdd_fileconfig_unhandled_exception.feature new file mode 100644 index 000000000..ef1dba2a9 --- /dev/null +++ b/features/tdd_fileconfig_unhandled_exception.feature @@ -0,0 +1,42 @@ +@tdd_issue @tdd_issue_7874 +Feature: Alembic fileConfig error handling (issue #7874) + + As an operator running Alembic migrations, I want to receive a clear, + actionable error message when alembic.ini contains malformed logging + configuration, so that I can diagnose and fix the issue without being + confronted by an unhandled traceback. + + Background: + Given a fresh temporary directory + + Scenario Outline: fileConfig failure exits with non-zero exit code + When the alembic.ini contains an invalid logger section that references nonexistent loggers + And migrations are invoked normally + Then the error output on stderr includes actionable guidance about "[loggers]" section + + Examples: malformed configurations + | description | + | references undefined logger key | + + Scenario Outline: fileConfig failure message includes the config file path + When the alembic.ini contains a malformed logging configuration + And migrations are invoked normally + Then the error output on stderr includes the config file path "alembic.ini" + + Examples: malformed configurations + | description | + | references undefined logger key | + + Scenario Outline: fileConfig failure message is user-actionable + When the alembic.ini contains a malformed logging configuration + And migrations are invoked normally + Then the error output on stderr includes actionable guidance about "[loggers]" section + + Examples: malformed configurations + | description | + | references undefined logger key | + + Scenario: fileConfig succeeds silently when INI file is valid + When the alembic.ini contains a valid logger configuration + And migrations are invoked normally + Then no error message appears on stderr diff --git a/src/cleveragents/infrastructure/database/migrations/env.py b/src/cleveragents/infrastructure/database/migrations/env.py index 44cb4bee3..7d0a68e92 100644 --- a/src/cleveragents/infrastructure/database/migrations/env.py +++ b/src/cleveragents/infrastructure/database/migrations/env.py @@ -1,5 +1,6 @@ import logging import os +import sys from logging.config import fileConfig from pathlib import Path @@ -27,7 +28,23 @@ if config is not None: # application loggers that were created before this point, which causes # test failures when log capture handlers are attached to those loggers. if config.config_file_name is not None: - fileConfig(config.config_file_name, disable_existing_loggers=False) + try: + fileConfig(config.config_file_name, disable_existing_loggers=False) + except Exception as exc: + # fileConfig() can raise configparser.Error, KeyError, ValueError, or + # other exceptions when the INI file is malformed or contains an invalid + # logging configuration section. Per the project's "fail fast" philosophy + # (CONTRIBUTING.md), we add context (file path, nature of error) and + # re-raise so the operator receives a clear, actionable diagnostic rather + # than a raw traceback with no indication of which file caused the failure. + config_path = config.config_file_name + print( + f"ERROR: Failed to configure logging from '{config_path}': {exc}\n" + f"Check that '{config_path}' contains a valid [loggers] section " + f"and that all referenced handlers and formatters are defined.", + file=sys.stderr, + ) + raise SystemExit(1) from exc # Override the database URL from environment or use default # This allows flexible configuration based on deployment