From 19ded088b06edf093f789caa10e44faad3a6fb12 Mon Sep 17 00:00:00 2001 From: John McCardle Date: Wed, 26 Nov 2025 10:26:30 -0500 Subject: [PATCH] feat: Exit on first Python callback exception (closes #133) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit By default, McRogueFace now exits with code 1 on the first unhandled exception in timer, click, key, or animation callbacks. This prevents repeated exception output that wastes resources in AI-driven development. Changes: - Add exit_on_exception config flag (default: true) - Add --continue-after-exceptions CLI flag to preserve old behavior - Update exception handlers in Timer, PyCallable, and Animation - Signal game loop via McRFPy_API atomic flags - Return proper exit code from main() Before: Timer exceptions repeated 1000+ times until timeout After: Single traceback, clean exit with code 1 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- src/Animation.cpp | 10 ++++-- src/CommandLineParser.cpp | 8 +++++ src/GameEngine.cpp | 5 +++ src/GameEngine.h | 1 + src/McRFPy_API.cpp | 22 ++++++++++++- src/McRFPy_API.h | 7 +++++ src/McRogueFaceConfig.h | 4 +++ src/PyCallable.cpp | 16 ++++++++-- src/Timer.cpp | 11 +++++-- src/main.cpp | 15 ++++++++- tests/notes/test_exception_exit.py | 31 ++++++++++++++++++ tests/notes/test_exception_exit_manual.sh | 38 +++++++++++++++++++++++ 12 files changed, 160 insertions(+), 8 deletions(-) create mode 100644 tests/notes/test_exception_exit.py create mode 100755 tests/notes/test_exception_exit_manual.sh diff --git a/src/Animation.cpp b/src/Animation.cpp index 4f74750..03f5b6f 100644 --- a/src/Animation.cpp +++ b/src/Animation.cpp @@ -3,6 +3,7 @@ #include "UIEntity.h" #include "PyAnimation.h" #include "McRFPy_API.h" +#include "GameEngine.h" #include "PythonObjectCache.h" #include #include @@ -368,9 +369,14 @@ void Animation::triggerCallback() { Py_DECREF(args); if (!result) { - // Print error but don't crash + std::cerr << "Animation callback raised an exception:" << std::endl; PyErr_Print(); - PyErr_Clear(); // Clear the error state + PyErr_Clear(); + + // Check if we should exit on exception + if (McRFPy_API::game && McRFPy_API::game->getConfig().exit_on_exception) { + McRFPy_API::signalPythonException(); + } } else { Py_DECREF(result); } diff --git a/src/CommandLineParser.cpp b/src/CommandLineParser.cpp index 3e69b1b..cad5398 100644 --- a/src/CommandLineParser.cpp +++ b/src/CommandLineParser.cpp @@ -121,6 +121,12 @@ CommandLineParser::ParseResult CommandLineParser::parse(McRogueFaceConfig& confi current_arg++; continue; } + + if (arg == "--continue-after-exceptions") { + config.exit_on_exception = false; + current_arg++; + continue; + } // If no flags matched, treat as positional argument (script name) if (arg[0] != '-') { @@ -160,6 +166,8 @@ void CommandLineParser::print_help() { << " --audio-off : disable audio\n" << " --audio-on : enable audio (even in headless mode)\n" << " --screenshot [path] : take a screenshot in headless mode\n" + << " --continue-after-exceptions : don't exit on Python callback exceptions\n" + << " (default: exit on first exception)\n" << "\n" << "Arguments:\n" << " file : program read from script file\n" diff --git a/src/GameEngine.cpp b/src/GameEngine.cpp index 3ac3cec..a012f26 100644 --- a/src/GameEngine.cpp +++ b/src/GameEngine.cpp @@ -289,6 +289,11 @@ void GameEngine::run() if (config.auto_exit_after_exec && timers.empty()) { running = false; } + + // Check if a Python exception has signaled exit + if (McRFPy_API::shouldExit()) { + running = false; + } } // Clean up before exiting the run loop diff --git a/src/GameEngine.h b/src/GameEngine.h index 4721bb8..c4a99ff 100644 --- a/src/GameEngine.h +++ b/src/GameEngine.h @@ -153,6 +153,7 @@ public: std::shared_ptr getTimer(const std::string& name); void setWindowScale(float); bool isHeadless() const { return headless; } + const McRogueFaceConfig& getConfig() const { return config; } void processEvent(const sf::Event& event); // Window property accessors diff --git a/src/McRFPy_API.cpp b/src/McRFPy_API.cpp index b095e52..b58df75 100644 --- a/src/McRFPy_API.cpp +++ b/src/McRFPy_API.cpp @@ -27,6 +27,9 @@ std::shared_ptr McRFPy_API::default_font; std::shared_ptr McRFPy_API::default_texture; PyObject* McRFPy_API::mcrf_module; +// Exception handling state +std::atomic McRFPy_API::exception_occurred{false}; +std::atomic McRFPy_API::exit_code{0}; static PyMethodDef mcrfpyMethods[] = { @@ -1144,6 +1147,23 @@ PyObject* McRFPy_API::_getMetrics(PyObject* self, PyObject* args) { // Add general metrics PyDict_SetItemString(dict, "current_frame", PyLong_FromLong(game->getFrame())); PyDict_SetItemString(dict, "runtime", PyFloat_FromDouble(game->runtime.getElapsedTime().asSeconds())); - + return dict; } + +// Exception handling implementation +void McRFPy_API::signalPythonException() { + // Check if we should exit on exception (consult config via game) + if (game && !game->isHeadless()) { + // In windowed mode, respect the config setting + // Access config through game engine - but we need to check the config + } + + // For now, always signal - the game loop will check the config + exception_occurred.store(true); + exit_code.store(1); +} + +bool McRFPy_API::shouldExit() { + return exception_occurred.load(); +} diff --git a/src/McRFPy_API.h b/src/McRFPy_API.h index 6b32dcf..6841fd2 100644 --- a/src/McRFPy_API.h +++ b/src/McRFPy_API.h @@ -2,6 +2,7 @@ #include "Common.h" #include "Python.h" #include +#include #include "PyFont.h" #include "PyTexture.h" @@ -85,4 +86,10 @@ public: static void triggerSceneChange(const std::string& from_scene, const std::string& to_scene); static void updatePythonScenes(float dt); static void triggerResize(int width, int height); + + // Exception handling - signal game loop to exit on unhandled Python exceptions + static std::atomic exception_occurred; + static std::atomic exit_code; + static void signalPythonException(); // Called by exception handlers + static bool shouldExit(); // Checked by game loop }; diff --git a/src/McRogueFaceConfig.h b/src/McRogueFaceConfig.h index 9534df6..ed4e9d2 100644 --- a/src/McRogueFaceConfig.h +++ b/src/McRogueFaceConfig.h @@ -31,6 +31,10 @@ struct McRogueFaceConfig { // Auto-exit when no timers remain (for --headless --exec automation) bool auto_exit_after_exec = false; + + // Exception handling: exit on first Python callback exception (default: true) + // Use --continue-after-exceptions to disable + bool exit_on_exception = true; }; #endif // MCROGUEFACE_CONFIG_H \ No newline at end of file diff --git a/src/PyCallable.cpp b/src/PyCallable.cpp index 0a3fb53..6fed830 100644 --- a/src/PyCallable.cpp +++ b/src/PyCallable.cpp @@ -1,4 +1,6 @@ #include "PyCallable.h" +#include "McRFPy_API.h" +#include "GameEngine.h" PyCallable::PyCallable(PyObject* _target) { @@ -51,9 +53,14 @@ void PyClickCallable::call(sf::Vector2f mousepos, std::string button, std::strin PyObject* retval = PyCallable::call(args, NULL); if (!retval) { - std::cout << "ClickCallable has raised an exception. It's going to STDERR and being dropped:" << std::endl; + std::cerr << "Click callback raised an exception:" << std::endl; PyErr_Print(); PyErr_Clear(); + + // Check if we should exit on exception + if (McRFPy_API::game && McRFPy_API::game->getConfig().exit_on_exception) { + McRFPy_API::signalPythonException(); + } } else if (retval != Py_None) { std::cout << "ClickCallable returned a non-None value. It's not an error, it's just not being saved or used." << std::endl; @@ -81,9 +88,14 @@ void PyKeyCallable::call(std::string key, std::string action) PyObject* retval = PyCallable::call(args, NULL); if (!retval) { - std::cout << "KeyCallable has raised an exception. It's going to STDERR and being dropped:" << std::endl; + std::cerr << "Key callback raised an exception:" << std::endl; PyErr_Print(); PyErr_Clear(); + + // Check if we should exit on exception + if (McRFPy_API::game && McRFPy_API::game->getConfig().exit_on_exception) { + McRFPy_API::signalPythonException(); + } } else if (retval != Py_None) { std::cout << "KeyCallable returned a non-None value. It's not an error, it's just not being saved or used." << std::endl; diff --git a/src/Timer.cpp b/src/Timer.cpp index 0784f13..8873a39 100644 --- a/src/Timer.cpp +++ b/src/Timer.cpp @@ -1,6 +1,8 @@ #include "Timer.h" #include "PythonObjectCache.h" #include "PyCallable.h" +#include "McRFPy_API.h" +#include "GameEngine.h" Timer::Timer(PyObject* _target, int _interval, int now, bool _once) : callback(std::make_shared(_target)), interval(_interval), last_ran(now), @@ -51,10 +53,15 @@ bool Timer::test(int now) Py_DECREF(args); if (!retval) - { - std::cout << "Timer callback has raised an exception. It's going to STDERR and being dropped:" << std::endl; + { + std::cerr << "Timer callback raised an exception:" << std::endl; PyErr_Print(); PyErr_Clear(); + + // Check if we should exit on exception + if (McRFPy_API::game && McRFPy_API::game->getConfig().exit_on_exception) { + McRFPy_API::signalPythonException(); + } } else if (retval != Py_None) { std::cout << "Timer returned a non-None value. It's not an error, it's just not being saved or used." << std::endl; diff --git a/src/main.cpp b/src/main.cpp index 3652e6c..4908e8c 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -44,6 +44,11 @@ int run_game_engine(const McRogueFaceConfig& config) if (Py_IsInitialized()) { McRFPy_API::api_shutdown(); } + + // Return exception exit code if a Python exception signaled exit + if (McRFPy_API::shouldExit()) { + return McRFPy_API::exit_code.load(); + } return 0; } @@ -181,9 +186,13 @@ int run_python_interpreter(const McRogueFaceConfig& config, int argc, char* argv // Run the game engine after script execution engine->run(); - + McRFPy_API::api_shutdown(); delete engine; + // Return exception exit code if signaled + if (McRFPy_API::shouldExit()) { + return McRFPy_API::exit_code.load(); + } return result; } else if (config.interactive_mode) { @@ -207,6 +216,10 @@ int run_python_interpreter(const McRogueFaceConfig& config, int argc, char* argv engine->run(); McRFPy_API::api_shutdown(); delete engine; + // Return exception exit code if signaled + if (McRFPy_API::shouldExit()) { + return McRFPy_API::exit_code.load(); + } return 0; } diff --git a/tests/notes/test_exception_exit.py b/tests/notes/test_exception_exit.py new file mode 100644 index 0000000..348c88b --- /dev/null +++ b/tests/notes/test_exception_exit.py @@ -0,0 +1,31 @@ +#!/usr/bin/env python3 +"""Test for --continue-after-exceptions behavior (Issue #133) + +This test verifies that: +1. By default, unhandled exceptions in timer callbacks cause immediate exit with code 1 +2. With --continue-after-exceptions, exceptions are logged but execution continues +""" + +import mcrfpy +import sys + +def timer_that_raises(runtime): + """A timer callback that raises an exception""" + raise ValueError("Intentional test exception") + +# Create a test scene +mcrfpy.createScene("test") +mcrfpy.setScene("test") + +# Schedule the timer - it will fire after 50ms +mcrfpy.setTimer("raise_exception", timer_that_raises, 50) + +# This test expects: +# - Default behavior: exit with code 1 after first exception +# - With --continue-after-exceptions: continue running (would need timeout or explicit exit) +# +# The test runner should: +# 1. Run without --continue-after-exceptions and expect exit code 1 +# 2. Run with --continue-after-exceptions and expect it to not exit immediately + +print("Test initialized - timer will raise exception in 50ms") diff --git a/tests/notes/test_exception_exit_manual.sh b/tests/notes/test_exception_exit_manual.sh new file mode 100755 index 0000000..78c2d8b --- /dev/null +++ b/tests/notes/test_exception_exit_manual.sh @@ -0,0 +1,38 @@ +#!/bin/bash +# Manual test for --continue-after-exceptions feature (Issue #133) +# +# This test must be run manually because it verifies exit codes +# rather than test output. + +echo "Testing --continue-after-exceptions feature..." +echo + +cd "$(dirname "$0")/../../build" + +# Test 1: Default behavior - should exit with code 1 on first exception +echo "Test 1: Default behavior (exit on first exception)" +timeout 5 ./mcrogueface --headless --exec ../tests/notes/test_exception_exit.py 2>&1 +EXIT_CODE=$? +echo "Exit code: $EXIT_CODE" +if [ $EXIT_CODE -eq 1 ]; then + echo "[PASS] Exit code is 1 as expected" +else + echo "[FAIL] Expected exit code 1, got $EXIT_CODE" + exit 1 +fi +echo + +# Test 2: --continue-after-exceptions - should keep running until timeout +echo "Test 2: --continue-after-exceptions (continue after exception)" +timeout 1 ./mcrogueface --headless --continue-after-exceptions --exec ../tests/notes/test_exception_exit.py 2>&1 | tail -5 +EXIT_CODE=${PIPESTATUS[0]} +echo "Exit code: $EXIT_CODE" +if [ $EXIT_CODE -eq 124 ]; then + echo "[PASS] Timeout killed it (exit code 124) - continued running as expected" +else + echo "[FAIL] Expected exit code 124 (timeout), got $EXIT_CODE" + exit 1 +fi +echo + +echo "All tests PASSED!"