diff --git a/.gitignore b/.gitignore index e066e2b25..7ed955d18 100644 --- a/.gitignore +++ b/.gitignore @@ -181,5 +181,5 @@ report.html .agent-orchestration agents-test -# Generated test reports (CI artifacts) +# Generated test reports (CI artifacts) — build artifacts, not to be committed test_reports/ diff --git a/CHANGELOG.md b/CHANGELOG.md index 64841acd4..cf8faef13 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -267,6 +267,16 @@ The format follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). are also protected. The DI container registration as `providers.Singleton` is now correct and safe. +- **Migration Prompt Safe Default on Failure** (#7503): Fixed + `MigrationRunner._default_prompt_for_migration` silently auto-approving destructive + database migrations when the interactive prompt raised any exception. The handler now + catches only `(IOError, OSError, EOFError)` (broken stdin / non-interactive pipe), + logs a `WARNING` instead of a `DEBUG` message, and returns `False` (reject) so that + migrations are never applied without explicit user consent. `KeyboardInterrupt` is + re-raised so Ctrl-C always works. Non-interactive environments (stdin not a TTY) also + now return `False` by default; use `CLEVERAGENTS_AUTO_APPLY_MIGRATIONS=true` or the + `--yes` CLI flag to approve automatically. + - **Validation Gate Empty-Run Guard** (#7508): Fixed `ApplyValidationSummary.all_required_passed` returning `True` when zero validations were run, silently bypassing the apply gate. The property now returns `False` when the validation result set is empty (`is_empty` is `True`), ensuring diff --git a/CONTRIBUTORS.md b/CONTRIBUTORS.md index ff7795e84..c422dfdea 100644 --- a/CONTRIBUTORS.md +++ b/CONTRIBUTORS.md @@ -20,6 +20,7 @@ Below are some of the specific details of various contributions. * HAL 9000 has contributed the bug-hunt-pool-supervisor non-blocking tracking fix: updated step 5 to be best-effort and added rule 9 to prevent the automation-tracking-manager call from blocking the main supervisor loop. * HAL 9000 has contributed the plugin entry point security hardening fix (#7476): enforced entry point allowlist validation before importing plugin modules to prevent malicious plugin loading. * HAL 9000 has contributed the benchmark workflow separation (#9040): moved the benchmark-regression job out of the default PR workflow into a dedicated scheduled workflow, reducing median PR CI turnaround time from 99-132 minutes to under 30 minutes. +* HAL 9000 has contributed automated bug fixes, security improvements, and migration safety enhancements including the migration prompt safe-default fix (#7503). * This project was made possible thanks to considerable donation of time, money, and resources by CleverThis, Inc. * HAL 9000 has contributed automated bug fixes, CLI output formatting improvements, and ongoing maintenance as part of the CleverAgents automation system. * HAL 9000 has contributed the file edit encoding parameter fix (PR #8258 / issue #7559). diff --git a/features/consolidated_misc.feature b/features/consolidated_misc.feature index 3222b921c..f1caf6b8e 100644 --- a/features/consolidated_misc.feature +++ b/features/consolidated_misc.feature @@ -1767,10 +1767,15 @@ Feature: Consolidated Misc Then the default prompt should return the typer confirmation result - Scenario: Default prompt auto-approves after interactive prompt failure + Scenario: Default prompt rejects migration after interactive prompt IOError Given a migration runner configured for "sqlite:///:memory:" - When the default migration prompt raises an error while prompting - Then the default prompt should auto approve after the failure + When the default migration prompt raises an IOError while prompting + Then the default prompt should reject migration after the IO failure + + Scenario: Default prompt rejects migration in non-interactive environment + Given a migration runner configured for "sqlite:///:memory:" + When I evaluate the default migration prompt in a non-interactive environment + Then the default prompt should reject without interaction # ============================================================ diff --git a/features/steps/migration_runner_steps.py b/features/steps/migration_runner_steps.py index 46c7d1f47..ec0018297 100644 --- a/features/steps/migration_runner_steps.py +++ b/features/steps/migration_runner_steps.py @@ -621,8 +621,166 @@ def step_then_prompt_returns_typer(context) -> None: assert "Apply migrations interactively?" in args[0] -@when("the default migration prompt raises an error while prompting") -def step_when_prompt_errors(context) -> None: +@when("the default migration prompt raises an IOError while prompting") +def step_when_prompt_io_errors(context) -> None: + snapshot = _snapshot_env( + ["CI", "BEHAVE_TESTING", "CLEVERAGENTS_AUTO_APPLY_MIGRATIONS"] + ) + fake_stdin = MagicMock() + fake_stdin.isatty.return_value = True + confirm_calls: list[Any] = [] + + def confirm(*args: Any, **kwargs: Any) -> bool: + confirm_calls.append((args, kwargs)) + raise OSError("broken stdin") + + error_typer = MagicMock() + error_typer.confirm.side_effect = confirm + try: + os.environ.pop("CI", None) + os.environ.pop("BEHAVE_TESTING", None) + os.environ["CLEVERAGENTS_AUTO_APPLY_MIGRATIONS"] = "" + with ( + patch("sys.stdin", fake_stdin), + patch.dict(sys.modules, {"typer": error_typer}), + ): + context.prompt_result = MigrationRunner._default_prompt_for_migration( + "Apply migrations after IO error?" + ) + finally: + _restore_env(snapshot) + + context.prompt_error_confirm_calls = list(confirm_calls) + + +@then("the default prompt should reject migration after the IO failure") +def step_then_prompt_reject_after_io_error(context) -> None: + assert context.prompt_result is False + assert len(getattr(context, "prompt_error_confirm_calls", [])) == 1 + + +@when("I evaluate the default migration prompt in a non-interactive environment") +def step_when_prompt_non_interactive(context) -> None: + snapshot = _snapshot_env( + ["CI", "BEHAVE_TESTING", "CLEVERAGENTS_AUTO_APPLY_MIGRATIONS"] + ) + fake_stdin = MagicMock() + fake_stdin.isatty.return_value = False + try: + os.environ.pop("CI", None) + os.environ.pop("BEHAVE_TESTING", None) + os.environ["CLEVERAGENTS_AUTO_APPLY_MIGRATIONS"] = "" + with patch("sys.stdin", fake_stdin): + context.prompt_result = MigrationRunner._default_prompt_for_migration( + "Apply migrations in non-interactive mode?" + ) + finally: + _restore_env(snapshot) + + +@then("the default prompt should reject without interaction") +def step_then_prompt_reject_non_interactive(context) -> None: + assert context.prompt_result is False + + +@when("the default migration prompt raises an EOFError while prompting") +def step_when_prompt_eof_errors(context) -> None: + snapshot = _snapshot_env( + ["CI", "BEHAVE_TESTING", "CLEVERAGENTS_AUTO_APPLY_MIGRATIONS"] + ) + fake_stdin = MagicMock() + fake_stdin.isatty.return_value = True + confirm_calls: list[Any] = [] + + def confirm(*args: Any, **kwargs: Any) -> bool: + confirm_calls.append((args, kwargs)) + raise EOFError("unexpected end of input") + + error_typer = MagicMock() + error_typer.confirm.side_effect = confirm + try: + os.environ.pop("CI", None) + os.environ.pop("BEHAVE_TESTING", None) + os.environ["CLEVERAGENTS_AUTO_APPLY_MIGRATIONS"] = "" + with ( + patch("sys.stdin", fake_stdin), + patch.dict(sys.modules, {"typer": error_typer}), + ): + context.prompt_result = MigrationRunner._default_prompt_for_migration( + "Apply migrations after EOF error?" + ) + finally: + _restore_env(snapshot) + + context.prompt_eof_confirm_calls = list(confirm_calls) + + +@then("the default prompt should reject migration after the EOF failure") +def step_then_prompt_reject_after_eof_error(context) -> None: + assert context.prompt_result is False + assert len(getattr(context, "prompt_eof_confirm_calls", [])) == 1 + + +@when("the default migration prompt receives a KeyboardInterrupt") +def step_when_prompt_keyboard_interrupt(context) -> None: + snapshot = _snapshot_env( + ["CI", "BEHAVE_TESTING", "CLEVERAGENTS_AUTO_APPLY_MIGRATIONS"] + ) + fake_stdin = MagicMock() + fake_stdin.isatty.return_value = True + + def confirm(*args: Any, **kwargs: Any) -> bool: + raise KeyboardInterrupt() + + interrupt_typer = MagicMock() + interrupt_typer.confirm.side_effect = confirm + context.keyboard_interrupt_raised = False + try: + os.environ.pop("CI", None) + os.environ.pop("BEHAVE_TESTING", None) + os.environ["CLEVERAGENTS_AUTO_APPLY_MIGRATIONS"] = "" + with ( + patch("sys.stdin", fake_stdin), + patch.dict(sys.modules, {"typer": interrupt_typer}), + ): + try: + MigrationRunner._default_prompt_for_migration( + "Apply migrations with Ctrl-C?" + ) + except KeyboardInterrupt: + context.keyboard_interrupt_raised = True + finally: + _restore_env(snapshot) + + +@then("the KeyboardInterrupt should propagate from the migration prompt") +def step_then_keyboard_interrupt_propagates(context) -> None: + assert context.keyboard_interrupt_raised is True + + +@when( + 'I evaluate the default migration prompt with CLEVERAGENTS_AUTO_APPLY_MIGRATIONS set to "{value}"' +) +def step_when_prompt_auto_apply_env(context, value: str) -> None: + snapshot = _snapshot_env( + ["CI", "BEHAVE_TESTING", "CLEVERAGENTS_AUTO_APPLY_MIGRATIONS"] + ) + fake_stdin = MagicMock() + fake_stdin.isatty.return_value = False + try: + os.environ.pop("CI", None) + os.environ.pop("BEHAVE_TESTING", None) + os.environ["CLEVERAGENTS_AUTO_APPLY_MIGRATIONS"] = value + with patch("sys.stdin", fake_stdin): + context.prompt_result = MigrationRunner._default_prompt_for_migration( + "Apply migrations with env var set?" + ) + finally: + _restore_env(snapshot) + + +@when("the default migration prompt raises an unexpected exception while prompting") +def step_when_prompt_unexpected_exception(context) -> None: snapshot = _snapshot_env( ["CI", "BEHAVE_TESTING", "CLEVERAGENTS_AUTO_APPLY_MIGRATIONS"] ) @@ -645,18 +803,18 @@ def step_when_prompt_errors(context) -> None: patch.dict(sys.modules, {"typer": error_typer}), ): context.prompt_result = MigrationRunner._default_prompt_for_migration( - "Apply migrations after errors?" + "Apply migrations after unexpected failure?" ) finally: _restore_env(snapshot) - context.prompt_error_confirm_calls = list(confirm_calls) + context.prompt_unexpected_confirm_calls = list(confirm_calls) -@then("the default prompt should auto approve after the failure") -def step_then_prompt_auto_after_error(context) -> None: - assert context.prompt_result is True - assert len(getattr(context, "prompt_error_confirm_calls", [])) == 1 +@then("the default prompt should reject migration after the unexpected failure") +def step_then_prompt_reject_after_unexpected_failure(context) -> None: + assert context.prompt_result is False + assert len(getattr(context, "prompt_unexpected_confirm_calls", [])) == 1 @given("the MigrationRunner class is available") diff --git a/features/tdd_migration_prompt_auto_approve_7503.feature b/features/tdd_migration_prompt_auto_approve_7503.feature new file mode 100644 index 000000000..d02e7446a --- /dev/null +++ b/features/tdd_migration_prompt_auto_approve_7503.feature @@ -0,0 +1,44 @@ +@tdd_issue @tdd_issue_7503 +Feature: TDD Issue #7503 — migration_runner _default_prompt_for_migration must reject on exception + As a database administrator + I want the migration runner to refuse migrations when the interactive prompt fails + So that destructive schema migrations are never applied without explicit user consent + + The root cause is that `_default_prompt_for_migration` in migration_runner.py + catches all exceptions with `except Exception` and returns `True` (auto-approve). + This means any prompt failure (broken stdin, IOError, EOFError) silently applies + destructive migrations to production databases without user confirmation. + + The fix narrows the exception handler to `(IOError, OSError, EOFError)`, logs at + WARNING level, and returns `False` (reject) instead of `True` (auto-approve). + `KeyboardInterrupt` is re-raised so Ctrl-C always works. + + Scenario: Prompt failure due to OSError rejects migration + Given a migration runner configured for "sqlite:///:memory:" + When the default migration prompt raises an IOError while prompting + Then the default prompt should reject migration after the IO failure + + Scenario: Prompt failure due to EOFError rejects migration + Given a migration runner configured for "sqlite:///:memory:" + When the default migration prompt raises an EOFError while prompting + Then the default prompt should reject migration after the EOF failure + + Scenario: Prompt failure due to unexpected exception rejects migration + Given a migration runner configured for "sqlite:///:memory:" + When the default migration prompt raises an unexpected exception while prompting + Then the default prompt should reject migration after the unexpected failure + + Scenario: Non-interactive environment rejects migration by default + Given a migration runner configured for "sqlite:///:memory:" + When I evaluate the default migration prompt in a non-interactive environment + Then the default prompt should reject without interaction + + Scenario: KeyboardInterrupt propagates from migration prompt + Given a migration runner configured for "sqlite:///:memory:" + When the default migration prompt receives a KeyboardInterrupt + Then the KeyboardInterrupt should propagate from the migration prompt + + Scenario: Explicit auto-approve env var still approves migration + Given a migration runner configured for "sqlite:///:memory:" + When I evaluate the default migration prompt with CLEVERAGENTS_AUTO_APPLY_MIGRATIONS set to "true" + Then the default prompt should auto approve without interaction diff --git a/src/cleveragents/infrastructure/database/migration_runner.py b/src/cleveragents/infrastructure/database/migration_runner.py index a1cbecfc5..c731c1c50 100644 --- a/src/cleveragents/infrastructure/database/migration_runner.py +++ b/src/cleveragents/infrastructure/database/migration_runner.py @@ -329,10 +329,18 @@ class MigrationRunner: def _default_prompt_for_migration(message: str) -> bool: """Request user approval before applying migrations. - Uses an interactive prompt when running in a TTY. In CI or other - non-interactive contexts, migrations are auto-approved to avoid - hanging execution. Set CLEVERAGENTS_AUTO_APPLY_MIGRATIONS to - "true" to auto-approve explicitly. + Uses an interactive prompt when running in a TTY. In CI or other + non-interactive contexts the function returns ``False`` (reject) to + prevent destructive migrations from being applied without explicit + consent. Set ``CLEVERAGENTS_AUTO_APPLY_MIGRATIONS`` to ``"true"`` + (or ``"1"`` / ``"yes"``) to approve automatically, or pass + ``--yes`` to the CLI command. + + If the interactive prompt raises any exception other than + ``KeyboardInterrupt`` (e.g. broken stdin or runtime errors from + Typer), the function logs a ``WARNING`` with the exception detail + and returns ``False`` — the safe default. ``KeyboardInterrupt`` + is re-raised so that Ctrl-C always works. """ import os import sys @@ -365,11 +373,17 @@ class MigrationRunner: ), default=False, ) - except Exception: - # Fall through to auto-approval below if prompting fails - logging.getLogger(__name__).debug( - "Interactive prompt failed, auto-approving migrations" + except KeyboardInterrupt: + raise + except Exception as exc: + logging.getLogger(__name__).warning( + "Interactive prompt failed (%s) — refusing migration. " + "Use --yes flag or set CLEVERAGENTS_AUTO_APPLY_MIGRATIONS=true " + "to force.", + exc, ) + return False - # Non-interactive or prompt failure: auto-approve to avoid blocking - return True + # Non-interactive environment (stdin is not a TTY): reject by default. + # Use --yes flag or set CLEVERAGENTS_AUTO_APPLY_MIGRATIONS=true to approve. + return False