[SECURITY] Centralized error management system - closes #60 by Ibrahim-3d · Pull Request #81 · sjackson0109/PowerTraderAI

Ibrahim-3d · 2026-05-17T20:32:30Z

Summary

Creates pt_error_handler.py — an application-wide singleton error handler built on top of the existing pt_errors.ErrorHandler. Provides a global error bus where any module can report exceptions, and UI/notification layers can subscribe to severity levels.

Changes

New: app/pt_error_handler.py — ApplicationErrorHandler singleton, handle(), on_critical(), on_error(), configure_gui_alerts() helpers
New: app/test_error_handler.py — 18 tests

Key features

Singleton: all modules share one get_handler() instance
Callbacks: on_critical(cb), on_error(cb), on_warning(cb) — register GUI alerts, email notifications, etc.
Module suppression: suppress_module("pt_trader") silences callbacks from known noisy modules
Query API: get_recent_errors(), get_critical_errors(), get_summary()

Usage

from pt_error_handler import handle, on_critical, configure_gui_alerts

# Startup: wire up GUI alerts
configure_gui_alerts(
    critical_callback=lambda r: messagebox.showerror("Critical Error", r.user_message),
)

# Anywhere in the codebase:
try:
    place_order(symbol, qty)
except Exception as exc:
    handle(exc, context={"symbol": symbol, "qty": qty})

Test plan

Closes #60

Copilot

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

Adds a centralized, application-wide error handling singleton with callback-based notifications and a small query API, along with unit tests to validate core behavior.

Changes:

Introduces ApplicationErrorHandler singleton wrapping pt_errors.ErrorHandler with per-severity callbacks and suppression.
Adds module-level convenience functions (get_handler, handle, on_critical/on_error/on_warning, configure_gui_alerts).
Adds unittest coverage for singleton behavior, callback registration, query API, and GUI callback wiring.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 7 comments.

File	Description
app/pt_error_handler.py	Implements the centralized singleton error handler, callback routing, suppression, and query helpers.
app/test_error_handler.py	Adds `unittest` tests for singleton initialization, handling behavior, callbacks, suppression, and query APIs.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Ibrahim-3d · 2026-05-18T01:04:50Z

Local CI Simulation Results

Ran all workflow steps locally against this branch prior to owner approval. Results:

Code Quality & Testing

Check	Tool	Result
Code formatting	`black --check`	✅ Pass — 0 files need reformatting
Import ordering	`isort --check`	✅ Pass
Linting (hard errors)	`flake8 --select=E9,F63,F7,F82`	✅ Pass — 0 syntax/undefined-name errors
Linting (extended)	`flake8 --exit-zero`	✅ Pass — 0 warnings in new files
Unit tests	`pytest`	✅ All tests pass (see table below)

Unit Test Results

File	Tests	Result
`test_circuit_breaker.py`	20	✅ All pass
`test_credentials_rotation.py`	30	✅ All pass
`test_error_handler.py`	22	✅ All pass
`test_backup_validation.py`	35	✅ All pass
`test_database_manager.py`	29	✅ All pass
`test_security_logger.py`	25	✅ All pass

CI/CD Pipeline

Workflow	Status	Reason
`Code Quality & Testing`	⏳ Awaiting owner approval	Fork PR — first-time contributor gate
`PowerTrader AI+ CI/CD`	⏳ Awaiting owner approval	Fork PR — first-time contributor gate
`Project Management`	⏳ Awaiting owner approval	Fork PR — first-time contributor gate

All three workflows are queued with action_required status. This is GitHub's security gate for PRs from fork contributors. Once approved by @sjackson0109, they will execute against the same code that was verified locally above.

Ibrahim-3d · 2026-05-18T01:20:13Z

All 7 Copilot review comments addressed. Inline replies posted directly on each thread. All threads resolved.

Note: Threads are resolved and collapsed. To view any thread, open the Files changed tab and click "Show resolved" or the collapsed thread indicator.

#	File	Topic	Status
1	`pt_error_handler.py`	`_ensure_init` not thread-safe - race on concurrent first call	Resolved
2	`pt_error_handler.py`	`handle_critical` mutates severity after ErrorHandler aggregated counts	Resolved
3	`pt_error_handler.py`	Callback exceptions logged without traceback	Resolved
4	`pt_error_handler.py`	`ErrorCategory` and `PowerTraderError` imported but never used	Resolved
5	`pt_error_handler.py`	`if severity:` truthiness check should be `if severity is not None:`	Resolved
6	`test_error_handler.py`	Warning callback test is non-assertive - always passes	Resolved
7	`test_error_handler.py`	Suppression test patches module after callbacks already fired	Resolved

ApplicationErrorHandler singleton wraps pt_errors.ErrorHandler with per-severity notification callbacks, module suppression, and a global error bus. Module-level shortcuts (handle, on_critical, on_error, configure_gui_alerts) make adoption drop-in for existing modules. - 18 unit tests, all passing

…verage - _ensure_init uses double-checked locking (class-level _init_lock) to be thread-safe - handle_critical updates error_counts for CRITICAL so get_critical_errors is consistent - ErrorCategory/PowerTraderError unused imports removed - configure_gui_alerts uses 'is not None' instead of truthiness check for callbacks - Callback exceptions now logged with exc_info=True for full traceback - Tests: non-assertive tests replaced with deterministic assertions - Tests: suppress_module test patches handle_error to set module before _fire_callbacks - Tests: added warning callback, unregister_all, gui alerts error-level tests

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.

- Fix module docstring example: handler.handle_error() -> handler.handle() - handle(reraise=True): use sys.exc_info() to detect if error is the active exception; bare raise preserves traceback exactly when called from an except block, fallback to raise error otherwise - handle_critical: decrement original severity count before incrementing CRITICAL to prevent double-counting in error_counts/get_summary() - _suppressed_modules: protect add/discard/read with _cb_lock so concurrent suppress_module/unsuppress_module/_fire_callbacks cannot race

Ibrahim-3d · 2026-05-19T09:32:04Z

Manual Testing Guide — PR #81: Centralized Error Management

Prerequisites

git fetch fork && git checkout feat/60-centralized-error-management
cd app

1. Run unit tests

python -m pytest test_error_handler.py -v

Expected: All 22 tests pass.

2. Smoke test — handle + callback fires

python -c "
from pt_error_handler import get_handler, on_warning, ApplicationErrorHandler
from pt_errors import ErrorSeverity

ApplicationErrorHandler.reset_singleton()
fired = []
on_warning(lambda r: fired.append(r))

try:
    raise ValueError('test warning')
except ValueError as e:
    report = get_handler().handle(e)

print('severity:', report.severity)
print('callbacks fired:', len(fired))
assert len(fired) == 1
print('PASS')
ApplicationErrorHandler.reset_singleton()
"

3. Verify reraise preserves traceback (key fix)

python -c "
import traceback
from pt_error_handler import get_handler, ApplicationErrorHandler

ApplicationErrorHandler.reset_singleton()
def deep():
    raise RuntimeError('original location')

try:
    deep()
except RuntimeError as e:
    try:
        get_handler().handle(e, reraise=True)
    except RuntimeError:
        tb = traceback.format_exc()
        print('traceback contains deep():', 'deep' in tb)
        assert 'deep' in tb, 'original traceback lost!'
        print('PASS')
ApplicationErrorHandler.reset_singleton()
"

4. Verify handle_critical no double-count (key fix)

python -c "
from pt_error_handler import get_handler, ApplicationErrorHandler
from pt_errors import ErrorSeverity

ApplicationErrorHandler.reset_singleton()
h = get_handler()
try:
    raise ValueError('low severity error')
except ValueError as e:
    h.handle_critical(e)

summary = h.get_summary()
total = sum(summary.get('error_counts', {}).values())
print('total error_counts:', total)
assert total == 1, f'double-counted! total={total}'
print('PASS')
ApplicationErrorHandler.reset_singleton()
"

5. Verify suppression is thread-safe

python -c "
import threading
from pt_error_handler import get_handler, ApplicationErrorHandler

ApplicationErrorHandler.reset_singleton()
h = get_handler()
errors = []

def worker():
    for _ in range(50):
        h.suppress_module('pt_test')
        h.unsuppress_module('pt_test')

threads = [threading.Thread(target=worker) for _ in range(4)]
[t.start() for t in threads]
[t.join() for t in threads]
print('no race condition errors:', len(errors) == 0)
ApplicationErrorHandler.reset_singleton()
"

Rollback

git checkout main -- app/pt_error_handler.py app/test_error_handler.py

Ibrahim-3d · 2026-05-20T18:55:15Z

/gemini review

sjackson0109

An outstanding contribution towards effective error handling.

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.

sjackson0109 · 2026-05-20T20:52:50Z

Ready to merge - subject to the open/non-resolved reviewer (copilot) comments.

- pt_errors.ErrorHandler.handle_error: new severity_override parameter applied BEFORE logging so escalated errors are recorded at the final severity from the start, not at the original severity followed by a post-hoc mutation. - pt_errors.ErrorHandler.error_counts: changed to nested Dict[category, Dict[severity, int]] so callers can ask "how many TRADING_ERROR/HIGH events" without scanning error_reports. Added ErrorHandler.category_total() helper for flat totals. - get_error_summary: now derives the flat 'categories' / 'severities' views from the nested counts so a single structure drives every reporting surface; also exposes 'by_category_severity' for callers that want the nested breakdown. - ApplicationErrorHandler.handle_critical: simplified to call handle_error(..., severity_override=CRITICAL); removes the manual error_counts arithmetic and the post-handle severity mutation. - ApplicationErrorHandler suppression: normalize both stored entries and ErrorReport.module by stripping a trailing '.py' so callers can pass either 'pt_trader' or 'pt_trader.py' and either form matches the filename that ErrorHandler captures from the traceback. - test_error_handler: replace the bare monkeypatch in test_suppress_module_blocks_callbacks with patch.object + addCleanup so the original method is always restored. Added test_suppress_module_accepts_filename_and_module_name.

Ibrahim-3d · 2026-05-21T08:54:35Z

@sjackson0109 round-3 done — addressed in ad8bbfd:

#	Issue	Fix
1	`handle_critical` logged at original severity then mutated	Added `severity_override` param to `ErrorHandler.handle_error`; applied before `_log_error` so escalated errors log at CRITICAL from the start
2	error_counts keyed by category — owner wanted nested per-severity	`error_counts: Dict[category, Dict[severity, int]]` nested counters. `category_total()` helper for flat sums. `get_error_summary()` exposes `by_category_severity`
3	Module suppression: `pt_trader` vs `pt_trader.py` mismatch	Both spellings collapse to the same suppressed entry — `suppress_module` strips `.py` on the way in, `_is_module_suppressed` strips `.py` from `report.module` before lookup. New test locks it in
4	Test monkeypatched without cleanup	`patch.object(... side_effect=...)` + `self.addCleanup(patcher.stop)` — restoration is guaranteed regardless of test outcome

24 error-handler tests pass locally (incl. new test_suppress_module_accepts_filename_and_module_name). Threads resolved.

Ibrahim-3d · 2026-05-21T14:04:10Z

Quick heads up - please hold this one. While auditing my own PRs I noticed I changed the shape of ErrorHandler.error_counts from a flat dict to a nested one, which would silently break anyone reading that attribute directly. Reverting the shape and adding the nested view as a new attribute. Will ping back when pushed.

Copilot

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

…ety) - pt_error_handler.py:2 — strip stray '+' from module title so generated docs render 'PowerTraderAI Centralized Error Management System'. - clear_history() now also clears error_counts_by_severity so get_summary().severities / by_category_severity start from zero after a reset instead of compounding across clears. - ApplicationErrorHandler now holds a dedicated _state_lock around every call that mutates or reads the underlying ErrorHandler state (handle, handle_critical, get_summary, get_recent_errors, get_critical_errors, clear_history) so the thread-safety promise in the class docstring actually holds under concurrent access. Callbacks still fire outside the lock to avoid holding it across user code.

Copilot

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

Copilot

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

comment only, easily rectified Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> Signed-off-by: Simon Jackson <admin@jacksonfamily.me>

Copilot

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

sjackson0109 · 2026-05-25T18:33:21Z

Once the last copilot review is answered+resolved, then this PR can merge.

…orrect counter thread-safety comment - get_recent_errors() now returns [] for limit <= 0 instead of returning a wrong subset via negative-index slicing; added regression test. - Corrected the error_counts comment in ErrorHandler: the two counters stay consistent within a single _update_error_counts call, but ErrorHandler is not itself thread-safe (the ApplicationErrorHandler wrapper serialises access via _state_lock).

Ibrahim-3d · 2026-05-26T12:16:17Z

Copilot review follow-up (commit `901ee97`)

#	File:Line	Issue	Resolution
1	pt_error_handler.py:get_recent_errors	Negative `limit` returns wrong subset via `[-limit:]` slice	Added `if limit <= 0: return []` guard + regression test
2	pt_error_handler.py:get_recent_errors	Duplicate of #1	Same fix
3	pt_error_handler.py:get_recent_errors	Duplicate of #1	Same fix
4	pt_errors.py	'atomically / cannot drift' comment overstates (no lock)	Comment corrected: counters consistent per call; ErrorHandler not thread-safe, ApplicationErrorHandler wrapper serialises via `_state_lock`

Black + flake8 clean, 24 unit tests pass (was 23).

Copilot

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

…ename Doc said 'not the filename' then 'either spelling works'; code normalizes both, so the docstring now states either form is accepted. Also drops a stray em dash.

sjackson0109 · 2026-05-26T19:24:15Z

Another great piece of work @Ibrahim-3d

Got some more feature tickets on there way.. i did a little brainstorming :)

Copilot AI review requested due to automatic review settings May 17, 2026 20:32

Ibrahim-3d requested a review from sjackson0109 as a code owner May 17, 2026 20:32

Copilot AI reviewed May 17, 2026

View reviewed changes

sjackson0109 self-assigned this May 17, 2026

sjackson0109 added component-risk Risk management component-monitoring Monitoring and observability components component-architecture System architecture and design components labels May 17, 2026

Ibrahim-3d added 3 commits May 18, 2026 21:34

fix: Black formatting

337b55a

Copilot AI review requested due to automatic review settings May 18, 2026 18:39

Ibrahim-3d force-pushed the feat/60-centralized-error-management branch from 396db34 to 337b55a Compare May 18, 2026 18:39

Copilot AI reviewed May 18, 2026

View reviewed changes

Comment thread app/pt_error_handler.py Outdated

Comment thread app/pt_error_handler.py

Comment thread app/pt_error_handler.py Outdated

Comment thread app/pt_error_handler.py Outdated

Comment thread app/test_error_handler.py Outdated

sjackson0109 added error-handling debugging labels May 18, 2026

Merge branch 'main' into feat/60-centralized-error-management

0ecbdae

Copilot AI review requested due to automatic review settings May 20, 2026 20:43

Copilot started reviewing on behalf of sjackson0109 May 20, 2026 20:43 View session

sjackson0109 previously approved these changes May 20, 2026

View reviewed changes

Copilot AI reviewed May 20, 2026

View reviewed changes

Comment thread app/pt_error_handler.py Outdated

Comment thread app/pt_error_handler.py

Comment thread app/pt_error_handler.py Outdated

Comment thread app/test_error_handler.py Outdated

Merge branch 'main' into feat/60-centralized-error-management

0a7f51c

sjackson0109 requested a review from Copilot May 23, 2026 19:07

Copilot started reviewing on behalf of sjackson0109 May 23, 2026 19:07 View session

Copilot AI reviewed May 23, 2026

View reviewed changes

Comment thread app/pt_error_handler.py Outdated

Comment thread app/pt_error_handler.py Outdated

Comment thread app/pt_error_handler.py

Ibrahim-3d and others added 2 commits May 24, 2026 13:25

Merge branch 'main' into feat/60-centralized-error-management

5390431

Copilot AI review requested due to automatic review settings May 24, 2026 19:19

Copilot started reviewing on behalf of sjackson0109 May 24, 2026 19:19 View session

sjackson0109 previously approved these changes May 24, 2026

View reviewed changes

Copilot AI reviewed May 24, 2026

View reviewed changes

Comment thread app/pt_error_handler.py

sjackson0109 added 2 commits May 24, 2026 22:15

Merge branch 'main' into feat/60-centralized-error-management

c31e570

Merge branch 'main' into feat/60-centralized-error-management

63157f3

Copilot AI review requested due to automatic review settings May 25, 2026 18:10

Copilot started reviewing on behalf of sjackson0109 May 25, 2026 18:10 View session

Copilot AI reviewed May 25, 2026

View reviewed changes

Comment thread app/pt_error_handler.py Outdated

Comment thread app/pt_error_handler.py

Comment thread app/pt_errors.py Outdated

Potential fix for pull request finding

23eee97

comment only, easily rectified Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> Signed-off-by: Simon Jackson <admin@jacksonfamily.me>

sjackson0109 dismissed their stale review via 23eee97 May 25, 2026 18:18

Copilot AI review requested due to automatic review settings May 25, 2026 18:18

Copilot started reviewing on behalf of sjackson0109 May 25, 2026 18:18 View session

Copilot AI reviewed May 25, 2026

View reviewed changes

Comment thread app/pt_error_handler.py

sjackson0109 and others added 2 commits May 26, 2026 01:06

Merge branch 'main' into feat/60-centralized-error-management

aa2aca3

Copilot AI review requested due to automatic review settings May 26, 2026 12:15

Copilot started reviewing on behalf of Ibrahim-3d May 26, 2026 12:21 View session

Copilot AI reviewed May 26, 2026

View reviewed changes

Align suppress_module docstring with code: accepts module name or fil…

9bd7c6e

…ename Doc said 'not the filename' then 'either spelling works'; code normalizes both, so the docstring now states either form is accepted. Also drops a stray em dash.

sjackson0109 approved these changes May 26, 2026

View reviewed changes

sjackson0109 merged commit 36c74d3 into sjackson0109:main May 26, 2026
11 checks passed

Conversation

Ibrahim-3d commented May 17, 2026

Summary

Changes

Key features

Usage

Test plan

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Ibrahim-3d commented May 18, 2026

Local CI Simulation Results

Code Quality & Testing

Unit Test Results

CI/CD Pipeline

Uh oh!

Ibrahim-3d commented May 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Ibrahim-3d commented May 19, 2026

Manual Testing Guide — PR #81: Centralized Error Management

Prerequisites

1. Run unit tests

2. Smoke test — handle + callback fires

3. Verify reraise preserves traceback (key fix)

4. Verify handle_critical no double-count (key fix)

5. Verify suppression is thread-safe

Rollback

Uh oh!

Ibrahim-3d commented May 20, 2026

Uh oh!

sjackson0109 left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sjackson0109 commented May 20, 2026

Uh oh!

Ibrahim-3d commented May 21, 2026

Uh oh!

Ibrahim-3d commented May 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Ibrahim-3d commented May 18, 2026 •

edited

Loading

Ibrahim-3d commented May 21, 2026 •

edited

Loading

Copilot review follow-up (commit `901ee97`)