From 7b4e24f5edbaef2b41b40d129e25ebffa0cfc683 Mon Sep 17 00:00:00 2001 From: Sasa Junuzovic <44276455+microsasa@users.noreply.github.com> Date: Thu, 23 Apr 2026 15:10:56 -0700 Subject: [PATCH 1/2] fix: update stale implementation.md sections and add symbol-existence tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Update three stale documentation sections in implementation.md: 1. Replace 'Fallback to blocking input()' with the current threaded _start_input_reader_thread() approach (daemon thread, SimpleQueue, _FALLBACK_EOF sentinel, lazy initialization). 2. Rename _FileChangeHandler → FileChangeHandler, correct module to interactive.py, update code snippet to use WATCHDOG_DEBOUNCE_SECS and match current signatures. 3. Update observer section to reference start_observer()/stop_observer() in interactive.py instead of inline Observer() construction. Add test_implementation_md_symbols_exist_in_expected_modules to tests/test_docs.py verifying that key symbol names referenced in implementation.md actually exist in their stated modules. Closes #1063 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/copilot_usage/docs/implementation.md | 33 ++++++++++---------- tests/test_docs.py | 38 ++++++++++++++++++++++++ 2 files changed, 54 insertions(+), 17 deletions(-) diff --git a/src/copilot_usage/docs/implementation.md b/src/copilot_usage/docs/implementation.md index 88a2d64..fd7a97c 100644 --- a/src/copilot_usage/docs/implementation.md +++ b/src/copilot_usage/docs/implementation.md @@ -248,41 +248,40 @@ def _read_line_nonblocking(timeout: float = 0.5) -> str | None: This is **Unix only** — `select()` on stdin doesn't work on Windows. The 500ms timeout allows the main loop to check for file-change events between input polls. -### Fallback to blocking `input()` +### Fallback to threaded `_start_input_reader_thread()` -If `select()` raises `ValueError` or `OSError` (e.g. stdin is piped, not a real TTY, or during testing), the loop falls back to blocking `input()` (in `cli.py`): +If `select()` raises `ValueError` or `OSError` (e.g. stdin is piped, not a real TTY, or during testing), the loop starts a daemon thread via `_start_input_reader_thread()` (in `cli.py`) that feeds lines into a `queue.SimpleQueue`: ```python except (ValueError, OSError): - try: - line = input().strip() - except (EOFError, KeyboardInterrupt): - break + fallback_queue = _start_input_reader_thread() + line = None ``` +The daemon thread calls `input()` in a loop, placing stripped lines on the queue. When stdin is exhausted or an unrecoverable error occurs, it posts the `_FALLBACK_EOF` sentinel and exits. The main loop then reads from `fallback_queue.get(timeout=0.5)` instead of `_read_line_nonblocking`, so that `change_event` auto-refresh keeps working during the fallback. The queue and thread are created lazily on first `ValueError`/`OSError` and are local to that `_interactive_loop` call — they never outlive it. + ### Watchdog file observer -A `watchdog.Observer` watches `~/.copilot/session-state/` recursively for **any** filesystem change — new session directories, lockfile creation/deletion, `events.jsonl` writes, etc. (in `cli.py`): +A `watchdog.Observer` watches `~/.copilot/session-state/` recursively for **any** filesystem change — new session directories, lockfile creation/deletion, `events.jsonl` writes, etc. The observer is created and started by `start_observer()` in `interactive.py`: ```python -observer = Observer() -observer.schedule(handler, str(session_path), recursive=True) -observer.daemon = True -observer.start() +observer = start_observer(session_path, change_event) ``` -The observer watches the session-state directory; if the directory doesn't exist at startup, no observer is created and auto-refresh is simply skipped. +`start_observer()` returns a `Stoppable` handle (or `None` when the observer cannot be started, e.g. inotify watch limit exhausted). The corresponding `stop_observer()` tears it down in a `finally` block. If the session-state directory doesn't exist at startup, no observer is created and auto-refresh is simply skipped. -### `_FileChangeHandler` with 2-second debounce +### `FileChangeHandler` with `WATCHDOG_DEBOUNCE_SECS` debounce -`_FileChangeHandler` (in `cli.py`) triggers on any filesystem event in the session-state tree and enforces a 2-second debounce using `time.monotonic()`: +`FileChangeHandler` (public, in `interactive.py`) triggers on any filesystem event in the session-state tree and enforces a debounce using `time.monotonic()` and the `WATCHDOG_DEBOUNCE_SECS` constant: ```python -def dispatch(self, event): +def dispatch(self, event: object) -> None: now = time.monotonic() - if now - self._last_trigger > 2.0: + with self._lock: + if now - self._last_trigger <= WATCHDOG_DEBOUNCE_SECS: + return self._last_trigger = now - self._change_event.set() + self._change_event.set() ``` Each trigger causes a full `get_all_sessions()` re-read, picking up new sessions, closed sessions, and updated event data. The debounce prevents rapid redraws during high-frequency event writes (e.g. tool execution loops producing many events per second). Manual refresh (`r`) is still available as a fallback. diff --git a/tests/test_docs.py b/tests/test_docs.py index 722796b..34997d7 100644 --- a/tests/test_docs.py +++ b/tests/test_docs.py @@ -1,3 +1,4 @@ +import importlib import re from pathlib import Path @@ -225,3 +226,40 @@ def test_architecture_first_pass_mentions_resume_detection() -> None: assert "resume" in description.lower(), ( "_first_pass() description in architecture.md must mention resume detection" ) + + +# --- Symbol-existence checks for implementation.md --- + + +_IMPL_SYMBOL_EXPECTATIONS: list[tuple[str, str, bool]] = [ + # (symbol_name, module_path, is_public) + ("FileChangeHandler", "copilot_usage.interactive", True), + ("_start_input_reader_thread", "copilot_usage.cli", False), + ("start_observer", "copilot_usage.interactive", True), + ("stop_observer", "copilot_usage.interactive", True), + ("WATCHDOG_DEBOUNCE_SECS", "copilot_usage.interactive", True), +] + + +def test_implementation_md_symbols_exist_in_expected_modules() -> None: + """Symbol names referenced in implementation.md must exist in the + expected modules — prevents future renames from silently drifting.""" + for symbol, module_path, is_public in _IMPL_SYMBOL_EXPECTATIONS: + # Verify the symbol is mentioned in implementation.md + assert symbol in _IMPL_MD, ( + f"implementation.md does not mention '{symbol}' — " + f"expected a reference to {module_path}.{symbol}" + ) + # Verify the symbol actually exists in the stated module + mod = importlib.import_module(module_path) + assert hasattr(mod, symbol), ( + f"implementation.md references '{symbol}' in {module_path}, " + f"but the symbol does not exist in that module" + ) + # Public symbols must be listed in __all__ + if is_public: + mod_all = getattr(mod, "__all__", []) + assert symbol in mod_all, ( + f"implementation.md references '{symbol}' as a public " + f"export of {module_path}, but it is not in __all__" + ) From 88f6b13941b6aaed5627ac55bf198cc706a529cd Mon Sep 17 00:00:00 2001 From: Sasa Junuzovic <44276455+microsasa@users.noreply.github.com> Date: Thu, 23 Apr 2026 15:20:59 -0700 Subject: [PATCH 2/2] fix: address review comments - Correct fallback trigger wording to match actual failure modes (non-selectable stdin on Windows, detached stdin in tests) - Accurately document daemon thread lifecycle limitations (no explicit teardown; may outlive _interactive_loop call) - Strengthen symbol-existence test to verify doc attributes each symbol to the correct module file, not just symbol presence Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/copilot_usage/docs/implementation.md | 4 ++-- tests/test_docs.py | 13 +++++++++++++ 2 files changed, 15 insertions(+), 2 deletions(-) diff --git a/src/copilot_usage/docs/implementation.md b/src/copilot_usage/docs/implementation.md index fd7a97c..e0e716c 100644 --- a/src/copilot_usage/docs/implementation.md +++ b/src/copilot_usage/docs/implementation.md @@ -250,7 +250,7 @@ This is **Unix only** — `select()` on stdin doesn't work on Windows. The 500ms ### Fallback to threaded `_start_input_reader_thread()` -If `select()` raises `ValueError` or `OSError` (e.g. stdin is piped, not a real TTY, or during testing), the loop starts a daemon thread via `_start_input_reader_thread()` (in `cli.py`) that feeds lines into a `queue.SimpleQueue`: +If `select()` raises `ValueError` or `OSError` (e.g. stdin is not selectable, notably on Windows, or stdin is detached during testing), the loop starts a daemon thread via `_start_input_reader_thread()` (in `cli.py`) that feeds lines into a `queue.SimpleQueue`: ```python except (ValueError, OSError): @@ -258,7 +258,7 @@ except (ValueError, OSError): line = None ``` -The daemon thread calls `input()` in a loop, placing stripped lines on the queue. When stdin is exhausted or an unrecoverable error occurs, it posts the `_FALLBACK_EOF` sentinel and exits. The main loop then reads from `fallback_queue.get(timeout=0.5)` instead of `_read_line_nonblocking`, so that `change_event` auto-refresh keeps working during the fallback. The queue and thread are created lazily on first `ValueError`/`OSError` and are local to that `_interactive_loop` call — they never outlive it. +The daemon thread calls `input()` in a loop, placing stripped lines on the queue. When stdin is exhausted or an unrecoverable error occurs, it posts the `_FALLBACK_EOF` sentinel and exits. The main loop then reads from `fallback_queue.get(timeout=0.5)` instead of `_read_line_nonblocking`, so that `change_event` auto-refresh keeps working during the fallback. The queue is created lazily on first `ValueError`/`OSError` and is local to that `_interactive_loop` call. The reader thread is also started lazily, but because it is a daemon thread that can block in `input()`, there is no explicit teardown path here: it may remain alive after `_interactive_loop` returns until stdin reaches EOF/errors or the process exits. Being a daemon means it does not prevent process shutdown. ### Watchdog file observer diff --git a/tests/test_docs.py b/tests/test_docs.py index 34997d7..4450b21 100644 --- a/tests/test_docs.py +++ b/tests/test_docs.py @@ -250,6 +250,19 @@ def test_implementation_md_symbols_exist_in_expected_modules() -> None: f"implementation.md does not mention '{symbol}' — " f"expected a reference to {module_path}.{symbol}" ) + # Verify the doc attributes the symbol to the correct module. + # The doc uses short filenames (e.g. ``cli.py``, ``interactive.py``), + # so derive the expected filename from the dotted module path. + expected_file = module_path.rsplit(".", 1)[-1] + ".py" + idx = _IMPL_MD.index(symbol) + window_start = max(0, idx - 500) + window_end = min(len(_IMPL_MD), idx + 500) + window = _IMPL_MD[window_start:window_end] + assert expected_file in window, ( + f"implementation.md mentions '{symbol}' but does not attribute it " + f"to '{expected_file}' nearby — expected the module reference " + f"within ~500 chars of the symbol" + ) # Verify the symbol actually exists in the stated module mod = importlib.import_module(module_path) assert hasattr(mod, symbol), (