Handle all errors

AGENTS.md

@@ -22,9 +22,12 @@ src/
   Runtime/Tapper.php         Client side: collects debug info, sends it over the wire, blocks for the reply
   Rpc/                       Minimal JSON-RPC-ish request/response types + blocking socket client
   Server.php                 TUI-process side: unix socket server, decodes requests, mutates AppState
+  SocketPath.php              Resolves the shared unix-socket path
+  LogPath.php                 Resolves tapper.log's path (same directory as the socket)
   Console/
     main.php                 DI container wiring (php-di) for the TUI process
     Application.php           Owns the ReactPHP event loop: render timer, resize timer, input handling
+    ErrorHandler.php            Redirects PHP warnings/notices (and, via bin/tapper, uncaught throwables) to tapper.log instead of stdout, which would otherwise corrupt the raw-mode/alt-screen render
     Component.php             Base class for every UI component (see docs/console-framework.md)
     EventBus.php               Pub/sub used for key/mouse/custom events
     CommandInvoker.php          Thin wrapper around php-di's Invoker (see Commands note below)

bin/tapper

@@ -9,7 +9,8 @@ $app = (require_once __DIR__.'/../src/Console/main.php');
 
 try {
     $app->run();
-} catch (\Exception $e) {
+} catch (\Throwable $e) {
     $app->close();
+    \Tapper\Console\ErrorHandler::logThrowable($e);
     throw $e;
 }

docs/console-framework.md

@@ -61,7 +61,7 @@ A single, app-wide, mutable object. It is the *only* channel by which the socket
 - **Magic properties**: `__get`/`__set` proxy to real private/constructor-promoted properties. The `@property` PHPDoc block at the top of the class is what makes these visible to IDEs/static analysis at all — it is not generated, it must be hand-kept in sync with the constructor's promoted properties. If you add a field to one and forget the other, you get either a silent dynamic property (constructor not updated) or an IDE that can't see a real property (docblock not updated).
 - **Change notification**: every `__set` (outside a batch) calls `notifyChange()` (the single `$change` callback registered once, by `Application::startRendering()`, to flip `shouldDraw = true`) and `callObservers($name)` (per-field subscribers registered via `observe($name, $callable)`).
 - **Field-specific observers**: `observe(string $name, callable $callable)` validates `$name` exists via `get_class_vars($this::class)` at call time (a `RuntimeException` if you typo a field name — this is your only safety net, and it's runtime-only). Multiple components observe overlapping fields today, e.g. `LogList::beforeInit()` observes `logs`, `cursor`, and `live` to keep unread counts and live-follow behavior in sync — read that method as the canonical example of cross-field reactive logic.
-- **Batching**: `deffer()` (sic — note the actual method name has this typo, not "defer") sets a `batching` flag; subsequent `__set` calls accumulate field names into `$changed` instead of notifying immediately. `commit()` flips batching off, replays observers for every accumulated field, then calls `notifyChange()` once. **There is a documented, unresolved bug here** — see the `@TODO` comment directly in `AppState.php`: repeatedly setting the same field while batched (the example given is pressing Enter repeatedly while `tp()->wait()` is paused) causes `$changed` to "overflow" in some way that isn't fully diagnosed. Don't build new batched-write logic on top of `deffer()`/`commit()` until this is understood; prefer direct `__set` (unbatched) if you're unsure.
+- **Batching**: `deffer()` (sic — note the actual method name has this typo, not "defer") sets a `batching` flag; subsequent `__set` calls accumulate field names into `$changed` instead of notifying immediately. `commit()` flips batching off, replays observers for every accumulated field, then calls `notifyChange()` once. `$changed` is keyed by field name (a set, not a list), so repeatedly setting the same field while batched — e.g. pressing Enter many times while a `tp()->wait()` is paused — no longer grows `$changed` unbounded or replays observers once per write; each field's observers replay exactly once per `commit()`.
 
 ## `Support/Scroll.php`
 

docs/known-issues.md

@@ -12,47 +12,38 @@ Verified with an isolated smoke test (no terminal required): booted `Server` dir
 
 Note: `SocketPath::resolve()` still resolves to "the installed package's own directory" (`realpath(__DIR__.'/..')`), same semantics as the original `Server.php` code — when Tapper is installed as a dependency, that's `vendor/tapperphp/tapper/tapper.sock`, not the consuming project's root. That's fine functionally (both processes resolve the same install, so they agree), but if this ever needs to be more conventional (e.g. `sys_get_temp_dir()`-based, to avoid touching `vendor/`), that's a deliberate follow-up, not part of this fix.
 
-### `wait()` listeners on `EventBus` are never cleaned up
+### ~~`wait()` listeners on `EventBus` are never cleaned up~~ — fixed 2026-08-15
 
-See `rpc-protocol.md`'s `wait` section — every `tp(...)->wait()` call adds a permanent `KeyCode::Enter` listener to `EventBus` that's never removed, even after it fires. Two overlapping `wait()` calls will both resolve on the same keypress instead of one at a time. `EventBus` has no unsubscribe mechanism at all today (see below).
+Previously every `tp(...)->wait()` call added a permanent `KeyCode::Enter` listener to `EventBus`, never removed, so overlapping `wait()` calls all resolved on the same keypress instead of one at a time. Fixed in `Server.php`: wait resolvers are now pushed onto a FIFO queue (`$waitResolvers`), and a single `KeyCode::Enter` listener is registered once (`registerWaitListener()`, guarded by `$waitListenerRegistered`) that pops and resolves the oldest pending wait per keypress. No per-call listener is added anymore, so there's nothing to leak, and N overlapping waits now require N separate Enter presses, in order. `EventBus` itself still has no general unsubscribe mechanism — see below, this remains a real gap for any future feature with dynamic listener lifecycles.
 
-### `Application::run()` registers signal handlers after the blocking call that would need them
+### ~~`Application::run()` registers signal handlers after the blocking call that would need them~~ — fixed 2026-08-15
 
-```php
-$this->loop->run();                                   // blocks until loop stops
+`addSignal(SIGINT, ...)`/`addSignal(SIGTERM, ...)` are now registered before `$this->loop->run()`, so they're actually reachable, and their bodies call `$this->close()` (restores raw mode, mouse capture, cursor, alternate screen) instead of `echo 'kill'`. **This alone does not make Ctrl+C work**, though — see below.
 
-$this->loop->addSignal(SIGINT, function () { echo 'kill'; });
-$this->loop->addSignal(SIGTERM, function () { echo 'kill'; });
-```
+### Ctrl+C did nothing even after the signal-handler fix above — fixed 2026-08-15
 
-Both `addSignal` calls are unreachable until `loop->run()` returns, at which point registering them is moot. Move them before `$this->loop->run()`. Also worth deciding what SIGINT/SIGTERM *should* do — right now the handler bodies just `echo 'kill'` rather than calling `$this->close()` to restore the terminal, so a Ctrl+C could leave the terminal in raw/alternate-screen mode.
+Root cause: `Terminal::enableRawMode()` shells out to `stty raw` (`vendor/php-tui/term/src/RawMode/SttyRawMode.php`), which disables `ISIG` at the tty driver level. With `ISIG` off, Ctrl+C is never translated into a `SIGINT` signal in the first place — it arrives as a normal byte, `\x03` (ETX), on stdin, same as any other keypress. So `$this->loop->addSignal(SIGINT, ...)` was structurally unreachable for the Ctrl+C case specifically, regardless of registration order (it still matters for external `kill -INT`/`SIGTERM`, which aren't tty-mediated). Fixed in `Application::startInputHandling()` by treating `"\x03"` the same as the existing `'q'` raw-byte check — both now call `$this->close()`.
 
-### `AppState` batching bug (documented in-code, unresolved)
+### ~~`AppState` batching bug (documented in-code, unresolved)~~ — fixed 2026-08-15
 
-`src/Console/State/AppState.php`, directly above `__set`:
+`$changed` was a list, so `__set`-ing the same field repeatedly while batched (e.g. pressing Enter many times while `tp()->wait()` is paused) appended a duplicate entry per write, growing unbounded and replaying that field's observers once per duplicate on `commit()`. Fixed by keying `$changed` on field name (`$this->changed[$name] = true`) so it behaves as a set; `commit()` iterates `array_keys($this->changed)`. Same fix applied to `appendLog()`'s batched branch (`'logs'` key).
 
-```php
-/*
- * @TODO investigate why `changed` overflows
- * when setting something multiple times,
- * like pressing enter many times
- * when waiting is set on tp
- */
-```
+### ~~`JsonRpcRequest::payload()` references an undefined `$id`~~ — fixed 2026-08-15
 
-Don't build new logic on `deffer()`/`commit()` batching until this is diagnosed — prefer unbatched (direct) `__set` calls if a new feature doesn't clearly need batching.
+Added a real `private ?string $id = null` constructor parameter; `payload()` now reads `$this->id ?? uniqid('rpc_', true)` instead of an undefined `$id` that only worked by accident via `??`'s notice suppression.
 
-### `JsonRpcRequest::payload()` references an undefined `$id`
+### ~~`Server::$id` is `static` inside a container-managed singleton~~ — fixed 2026-08-15
 
-```php
-'id' => $id ?? uniqid('rpc_', true),
-```
+Changed to `private int $id = 0` (instance property), consistent with the rest of the class's DI-managed design.
 
-`$id` is never assigned in scope, so this always evaluates to `uniqid('rpc_', true)` — it works, but only because of PHP's `??` suppressing the undefined-variable notice, not because there's an actual optional-id feature. Either add a real `?string $id = null` constructor parameter to `JsonRpcRequest` if per-request ids are wanted, or simplify to a direct `uniqid('rpc_', true)` call and drop the `??`.
+### ~~PHP warnings/notices printed straight onto the live TUI, corrupting the render~~ — fixed 2026-08-16
 
-### `Server::$id` is `static` inside a container-managed singleton
+Reported case: pressing Space in `LogList` with zero log entries hit `LogList::select()` reading `$this->appState->logs()[$this->appState->cursor]` with an empty `logs` array — an undefined-array-key warning, repeated once per render tick, each one printed raw over the alternate-screen buffer. Two fixes:
 
-`Server` is constructor-injected with `AppState`/`EventBus` (i.e., it's a normal DI-managed instance), but its log-id counter is `private static $id = 0`. Harmless today (one `Server` instance ever exists), but inconsistent with the rest of the class's design and would break if `Server` were ever instantiated more than once (e.g. in a test). Make it an instance property.
+1. Root cause: `LogList::select()` now reads via `?? null` and no-ops when there's no log at the cursor, instead of indexing blind.
+2. General case: `Console\ErrorHandler::install()` (called first thing in `Application::run()`, before raw mode/alt-screen are enabled) installs a `set_error_handler` that logs any warning/notice/deprecation to `tapper.log` (`LogPath::resolve()`, same directory as `tapper.sock`) and sets a 5-second `AppState::errorNotice` banner (rendered by `Header`) instead of letting PHP print to stdout. Uncaught `Throwable`s still propagate normally — `bin/tapper`'s `catch (\Throwable $e)` (widened from `\Exception`) calls `Application::close()` to restore the terminal, logs via `ErrorHandler::logThrowable()`, then rethrows so the trace prints on a normal, restored terminal rather than corrupting the TUI.
+
+Note: this intentionally does *not* revive `Windows/Popup.php` for the "check the logs" notice — see the dead-scaffolding entry below; the banner lives in `Header` via two new `AppState` fields (`errorNotice`, `errorNoticeExpiresAt`) instead.
 
 ## Incomplete abstractions (finish or remove, don't extend as-is)
 
@@ -61,7 +52,7 @@ Don't build new logic on `deffer()`/`commit()` batching until this is diagnosed
 - **`docs/openrpc.json`** — describes a method (`appendLog`, nested `details` param) that doesn't match the implemented protocol (`log`/`wait`, flat params). Regenerate from `Server.php` or delete.
 - **`Windows/Popup.php`** — exists, is instantiated and checked (`Application::draw()` renders it when `isActive()`), but its `view()` just returns an empty `BlockWidget::default()` and nothing in the codebase ever calls `activate()` on it (confirmed by grep — zero hits). Dead scaffolding for a not-yet-built feature (likely a modal/dialog system) — fine to leave, but don't assume it's a working popup system if you go looking for one.
 - **`AppState::typingMode` is only ever set to `false`**, never `true`, anywhere in the codebase (confirmed by grep). `Application::handleEventInTypingMode()` therefore always returns early — its entire body (redirecting character input, exiting on Esc) is currently unreachable dead code, presumably scaffolding for a not-yet-built text-input feature.
-- **`EventBus` has no unsubscribe** — every `listen()` is permanent. This is fine for the fixed component tree Tapper has today (components are never torn down mid-run), but is a real gap the moment anything needs dynamic component lifecycles (the `wait()` bug above is a direct symptom of this gap).
+- **`EventBus` has no unsubscribe** — every `listen()` is permanent. This is fine for the fixed component tree Tapper has today (components are never torn down mid-run), but is a real gap the moment anything needs dynamic component lifecycles. (The `wait()` leak that used to be a direct symptom of this gap is fixed above by having `Server` register one permanent listener instead of one per call — that sidesteps the gap for this one case, it doesn't close it.)
 
 ## Framework-extraction readiness
 

src/Console/Application.php

@@ -52,6 +52,8 @@ public function __construct(
 
     public function run(): int
     {
+        ErrorHandler::install($this->appState);
+
         $this->area = $this->phpTermBackend->size();
         $this->appState->version = 'v0.1.1';
         $this->terminal->execute(Actions::alternateScreenEnable());
@@ -64,15 +66,16 @@ public function run(): int
         $this->startRendering();
         $this->startInputHandling();
         $this->server->run();
-        $this->loop->run();
 
         $this->loop->addSignal(SIGINT, function () {
-            echo 'kill';
+            $this->close();
         });
         $this->loop->addSignal(SIGTERM, function () {
-            echo 'kill';
+            $this->close();
         });
 
+        $this->loop->run();
+
         return 0;
     }
 
@@ -127,7 +130,9 @@ private function startInputHandling(): void
                 $this->handleEventInTypingMode($event, $data);
             }
 
-            if ($data === 'q') {
+            // `stty raw` (enabled by Terminal::enableRawMode()) disables ISIG, so Ctrl+C
+            // never reaches us as SIGINT — it arrives as byte 0x03 (ETX) on stdin instead.
+            if ($data === 'q' || $data === "\x03") {
                 $this->close();
             }
         });

src/Console/Components/Header.php

@@ -4,17 +4,29 @@
 
 namespace Tapper\Console\Components;
 
+use PhpTui\Tui\Color\RgbColor;
 use PhpTui\Tui\Display\Area;
 use PhpTui\Tui\Extension\Core\Widget\BlockWidget;
 use PhpTui\Tui\Extension\Core\Widget\ParagraphWidget;
+use PhpTui\Tui\Style\Style;
 use PhpTui\Tui\Text\Span;
 use PhpTui\Tui\Widget\Borders;
 use PhpTui\Tui\Widget\BorderType;
 use PhpTui\Tui\Widget\Widget;
+use Tapper\Console\CommandAttributes\Periodic;
 use Tapper\Console\Component;
+use Tapper\Console\Palette;
 
 class Header extends Component
 {
+    #[Periodic(1.0)]
+    public function clearExpiredErrorNotice(): void
+    {
+        if ($this->appState->errorNotice !== null && microtime(true) >= $this->appState->errorNoticeExpiresAt) {
+            $this->appState->errorNotice = null;
+        }
+    }
+
     protected function view(Area $area): Widget
     {
         $waiting = $this->appState->pendingWaits > 0;
@@ -41,6 +53,9 @@ protected function view(Area $area): Widget
                         $unread ? Span::fromString(sprintf(' (↓%s)', $this->appState->unread))->yellow() : Span::fromString(''),
                         Span::fromString(' | '),
                         Span::fromString(sprintf('port: %s', $this->appState->port)),
+                        $this->appState->errorNotice !== null
+                            ? Span::styled(' | '.$this->appState->errorNotice, Style::default()->fg(RgbColor::fromHex(Palette::ERROR)))
+                            : Span::fromString(''),
                     ),
                 );
     }

src/Console/Components/LogList.php

@@ -111,7 +111,13 @@ public function pageDown(): void
     #[KeyPressed(' ')]
     public function select(): void
     {
-        $this->appState->previewLog = $this->appState->logs()[$this->appState->cursor];
+        $log = $this->appState->logs()[$this->appState->cursor] ?? null;
+
+        if ($log === null) {
+            return;
+        }
+
+        $this->appState->previewLog = $log;
     }
 
     #[KeyPressed(KeyCode::Enter)]

src/Console/ErrorHandler.php

@@ -0,0 +1,74 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Tapper\Console;
+
+use Tapper\Console\State\AppState;
+use Tapper\LogPath;
+use Throwable;
+
+/**
+ * The TUI runs in raw mode on the terminal's alternate screen, so anything PHP prints
+ * through its default error output (warnings, notices, uncaught-exception traces) gets
+ * interleaved with rendered frames instead of appearing on a normal scrollback — this is
+ * what corrupts the display. `install()` redirects non-fatal errors to a log file and a
+ * short-lived Header notice instead of letting PHP print them. Fatal throwables still
+ * propagate (see `Application::run()`/`bin/tapper`) so the terminal can be restored via
+ * `Application::close()` before anything is printed.
+ */
+final class ErrorHandler
+{
+    private const int NOTICE_SECONDS = 5;
+
+    public static function install(AppState $appState): void
+    {
+        ini_set('display_errors', '0');
+
+        set_error_handler(function (int $severity, string $message, string $file, int $line) use ($appState): bool {
+            if (! (error_reporting() & $severity)) {
+                return true;
+            }
+
+            self::log(sprintf('%s: %s in %s:%d', self::severityLabel($severity), $message, $file, $line));
+            self::notify($appState);
+
+            return true;
+        });
+    }
+
+    public static function logThrowable(Throwable $e): void
+    {
+        self::log(sprintf(
+            "Uncaught %s: %s in %s:%d\n%s",
+            $e::class,
+            $e->getMessage(),
+            $e->getFile(),
+            $e->getLine(),
+            $e->getTraceAsString(),
+        ));
+    }
+
+    private static function log(string $entry): void
+    {
+        $line = sprintf('[%s] %s%s', date('Y-m-d H:i:s'), $entry, PHP_EOL);
+
+        file_put_contents(LogPath::resolve(), $line, FILE_APPEND);
+    }
+
+    private static function notify(AppState $appState): void
+    {
+        $appState->errorNotice = '⚠ error — see tapper.log';
+        $appState->errorNoticeExpiresAt = microtime(true) + self::NOTICE_SECONDS;
+    }
+
+    private static function severityLabel(int $severity): string
+    {
+        return match ($severity) {
+            E_WARNING, E_USER_WARNING => 'Warning',
+            E_NOTICE, E_USER_NOTICE => 'Notice',
+            E_DEPRECATED, E_USER_DEPRECATED => 'Deprecated',
+            default => 'Error',
+        };
+    }
+}