Fix: Add exception handling for ACP notification callbacks (#1050)

[tbrandenburg]

Summary

The ACP handler's onNotification callback was invoked without exception protection, allowing user callback errors to crash the Node.js process.

Root Cause

The notification callback (added in PR #1031) is invoked directly from an EventEmitter data handler without try-catch protection. If the callback throws, it crashes the process.

Changes

File	Change
src/core/target-handlers/acp-handler.ts:142-150	Wrapped callback invocation in try-catch with console.error logging
tests/unit/core/target-handlers/acp-handler.test.ts:540-570	Added test case to verify exception handling

Testing

• Type check passes
• Unit tests pass (27/27 for ACP handler)
• Full test suite passes (419 tests)
• Lint passes
• New test verifies callback exceptions are caught and logged

Validation

npm run type-check && npm test acp-handler && npm run lint

Issue

Fixes #1050

---

📋 Implementation Details

Implementation followed artifact:

.claude/PRPs/issues/issue-1050.md

Deviations from plan:

• Changed jest.spyOn to vi.spyOn in test (Vitest framework instead of Jest)
• Added empty callback to mockImplementation() for proper spy behavior

Key decisions:

• Used console.error for logging (matches existing error handler patterns)
• Process continues after callback error (graceful degradation)
• No custom error types added (keeping it simple as per artifact scope)

---

Automated implementation from investigation artifact

[tbrandenburg]

🔍 Automated Code Review

Summary

All checks pass. The implementation correctly addresses the root cause and follows existing codebase patterns. No blocking issues identified.

Findings

✅ Strengths

• Root cause fixed: Try-catch wrapper prevents callback exceptions from crashing the process
• Pattern consistency: Uses console.error matching other error handlers in the same file (lines 156, 162, 171)
• Defensive programming: Isolates user-provided code errors, continues processing subsequent notifications
• Test coverage: New test verifies both exception catching and error logging behavior
• Zero regression: All 419 tests pass, including the new callback exception test

⚠️ Observations (non-blocking)

• The error variable in the catch block is untyped (line 147), which is common in TypeScript catch blocks. console.error handles any type correctly, so this is fine. Could optionally add type checking in the future if stricter error handling is desired.

🔒 Security

• No security concerns identified
• Error logging doesn't expose sensitive data
• Prevents denial-of-service via callback crash (defensive programming)

Checklist

• Fix addresses root cause from investigation
• Code follows codebase patterns
• Tests cover the change (callback exception handling)
• No obvious bugs introduced
• Edge cases handled (multiple notifications, any error type, optional callback)

---

Self-reviewed by Claude • Ready for human review