Cover tool/_oauth_callback.py at 100% branch

First module of the tool/ coverage task. It is the one the task was raised
for: _start_callback_server's runtime path had never been exercised, and it
is the only tool/ module with no network dependency, so it needs no mocking.

Driven over real HTTP against 127.0.0.1 on a port the module picks, rather
than with a faked request object. The branches worth protecting only exist
once a socket is involved -- particularly the /favicon.ico fetch that must be
answered without being recorded, since treating it as the callback retires the
server before Google ever redirects and surfaces to the user as a consent
timeout. Both serve-loop exits are covered: a result arriving, and the
deadline passing (monkeypatched negative rather than waiting out 300s).

Writing them surfaced a teardown race, not a bug in the module: calling
server_close() while the worker is still inside handle_request() pulls the
socket out from under it and raises on the thread. Invisible normally and
deterministic under --cov, whose tracer widens the window -- so the tests
retire the thread through the loop's own exit condition before closing, via
one _retire helper. Verified over 6 consecutive --cov runs.

crdt_sync 259 -> 268 passed, still 100%. tool/ is not yet in the coverage
source, so the config flip stays last, per the prompt.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CFSGeWxHPY5rHUGrPqhcmG

crdt-sync/crdt_sync/tests/test_tool_oauth_callback.py

@@ -0,0 +1,201 @@
+"""Tests for the one-shot OAuth redirect catcher in ``tool/_oauth_callback``.
+
+No network access: everything binds 127.0.0.1 on a port the module itself
+picks. The handler is driven over real HTTP rather than with a faked request
+object, because the branches worth protecting here are the ones that only
+appear once a socket is involved -- the favicon fetch that must *not* end the
+flow, and the serve loop's exit conditions, which run on a worker thread.
+"""
+
+from __future__ import annotations
+
+import http.client
+import http.server
+import time
+from typing import TYPE_CHECKING
+
+import pytest
+
+from tool import _oauth_callback as oc
+
+if TYPE_CHECKING:
+    from collections.abc import Iterator
+    import threading
+
+_STATE = "state-the-callback-must-echo"
+
+
+def _get(port: int, path: str) -> tuple[int, bytes]:
+    """Issue one GET against the loopback server and read the whole reply."""
+    conn = http.client.HTTPConnection("127.0.0.1", port, timeout=5)
+    try:
+        conn.request("GET", path)
+        response = conn.getresponse()
+        return response.status, response.read()
+    finally:
+        conn.close()
+
+
+def _retire(srv: http.server.HTTPServer, thread: threading.Thread) -> None:
+    """Stop the serve thread, then close the socket -- in that order.
+
+    ``server_close()`` while the thread is still inside ``handle_request()``
+    pulls the socket out from under it and raises on the worker. Setting a
+    result is what the loop's own exit condition tests, so it retires the
+    thread the same way a real callback does.
+    """
+    oc._CallbackHandler.result = oc._CallbackHandler.result or {"code": "teardown"}
+    thread.join(timeout=5)
+    srv.server_close()
+
+
+@pytest.fixture
+def server() -> Iterator[tuple[int, threading.Thread]]:
+    """Start the callback server on a free port and always tear it down."""
+    port = oc._free_port()
+    srv, thread = oc._start_callback_server(port, _STATE)
+    try:
+        yield port, thread
+    finally:
+        _retire(srv, thread)
+
+
+def test_free_port_returns_a_bindable_loopback_port() -> None:
+    """The port is real and free: binding it again must succeed."""
+    port = oc._free_port()
+
+    assert 1024 < port <= 65535
+    # The socket is closed by the time _free_port returns, so a second server
+    # can take the port. If it could not, the callback server would race the
+    # probe socket and fail to bind.
+    srv, thread = oc._start_callback_server(port, _STATE)
+    _retire(srv, thread)
+
+
+def test_callback_with_matching_state_is_recorded_and_acked(
+    server: tuple[int, threading.Thread],
+) -> None:
+    """The happy path: code + matching state -> 200, and the loop exits."""
+    port, thread = server
+
+    status, body = _get(port, f"/?code=the-code&state={_STATE}")
+
+    assert status == 200
+    assert b"you can close this tab" in body
+    assert oc._CallbackHandler.result == {"code": "the-code", "state": _STATE}
+    # The serve loop's `while not result` condition must now be false, so the
+    # thread retires on its own rather than sitting on the socket.
+    thread.join(timeout=5)
+    assert not thread.is_alive()
+
+
+def test_mismatched_state_is_recorded_but_reported_as_failure(
+    server: tuple[int, threading.Thread],
+) -> None:
+    """A code echoing the wrong state is a failure, not a success."""
+    port, thread = server
+
+    status, body = _get(port, "/?code=the-code&state=not-the-state")
+
+    assert status == 400
+    assert b"Authorization failed" in body
+    # Still recorded: the caller needs to see what came back to explain it.
+    assert oc._CallbackHandler.result["state"] == "not-the-state"
+    thread.join(timeout=5)
+
+
+def test_error_callback_without_a_code_is_reported_as_failure(
+    server: tuple[int, threading.Thread],
+) -> None:
+    """Google reports a declined consent as ``error``, with no ``code``."""
+    port, thread = server
+
+    status, body = _get(port, "/?error=access_denied")
+
+    assert status == 400
+    assert b"Authorization failed" in body
+    assert oc._CallbackHandler.result == {"error": "access_denied"}
+    thread.join(timeout=5)
+
+
+def test_favicon_fetch_is_answered_but_does_not_end_the_flow(
+    server: tuple[int, threading.Thread],
+) -> None:
+    """The regression this module's docstring is about.
+
+    Browsers fetch /favicon.ico against the redirect origin. Treating that as
+    the callback retires the server before Google ever redirects, which the
+    user sees as a consent timeout.
+    """
+    port, thread = server
+
+    status, body = _get(port, "/favicon.ico")
+
+    assert status == 204
+    assert body == b""
+    assert oc._CallbackHandler.result == {}
+    # Still serving: the real callback can still arrive and be recorded.
+    assert thread.is_alive()
+
+    status, _ = _get(port, f"/?code=late-code&state={_STATE}")
+    assert status == 200
+    assert oc._CallbackHandler.result["code"] == "late-code"
+    thread.join(timeout=5)
+
+
+def test_serve_loop_gives_up_once_the_consent_deadline_passes(
+    monkeypatch: pytest.MonkeyPatch,
+) -> None:
+    """The other loop exit: no callback ever arrives.
+
+    The deadline is 300s of wall clock, so it is moved into the past instead
+    of waited out. `server.timeout` bounds handle_request(), which is what
+    lets the loop re-test the deadline at all.
+    """
+    monkeypatch.setattr(oc, "_CONSENT_TIMEOUT_SECONDS", -1)
+
+    port = oc._free_port()
+    srv, thread = oc._start_callback_server(port, _STATE)
+    try:
+        thread.join(timeout=5)
+        assert not thread.is_alive()
+        assert oc._CallbackHandler.result == {}
+    finally:
+        _retire(srv, thread)
+
+
+def test_log_message_is_silenced() -> None:
+    """The default handler logs every request to stderr; this one must not."""
+    # Called unbound with no request context: the override must accept any
+    # arguments and do nothing, which is exactly what makes that safe.
+    assert oc._CallbackHandler.log_message(None, '"GET / HTTP/1.1"', 200) is None
+
+
+def test_starting_the_server_resets_state_from_a_previous_run() -> None:
+    """Class attributes outlive one flow, so a stale result must not leak."""
+    oc._CallbackHandler.result = {"code": "stale-from-an-earlier-flow"}
+    oc._CallbackHandler.expected_state = "stale-state"
+
+    port = oc._free_port()
+    srv, thread = oc._start_callback_server(port, _STATE)
+    try:
+        assert oc._CallbackHandler.result == {}
+        assert oc._CallbackHandler.expected_state == _STATE
+    finally:
+        _retire(srv, thread)
+
+
+def test_time_is_not_slept_away_by_the_fixture() -> None:
+    """Guard against the deadline being computed once at import time.
+
+    If ``deadline`` were module-level rather than per-call, a server started
+    later in the session would inherit an already-expired deadline and never
+    serve. Starting one now and checking it is alive catches that.
+    """
+    port = oc._free_port()
+    srv, thread = oc._start_callback_server(port, _STATE)
+    try:
+        time.sleep(0.05)
+        assert thread.is_alive()
+    finally:
+        _retire(srv, thread)