From 1e445af359e32c1d04acc712eb060af1528d277b Mon Sep 17 00:00:00 2001 From: Alajmah <55954113+Alajmah@users.noreply.github.com> Date: Wed, 8 Jul 2026 23:23:40 +0300 Subject: [PATCH 01/25] fix(mcp): surface PoolExhausted/PoolShuttingDown error context (P0) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PoolExhaustedError and PoolShuttingDownError were defined with no default message and raised bare at 9 sites. The MCP error formatter stringified them (f"{e}. Retry later.") producing ". Retry later." — an empty diagnostic with no way to tell whether the pool was exhausted, shutting down, or wedged for another reason. Field reports confirmed this: a session saw every tool call (reads included) fail identically with the empty error, blocking all use. Fix: - mcp_driver_pool.py: both exception classes now have __init__ with a default human-readable message (mirroring RateLimitError/AuthExpiredError in cdp_driver.py). - mcp_server.py: catch site hardened with str(e) or type(e).__name__ fallback so any future empty-message exception can't produce a bare ". Retry later." 628 tests pass. See BRIDGE_DIAGNOSIS.md failure mode #5 for full context. --- .gitignore | 3 +++ src/chatgpt_web2api/mcp_driver_pool.py | 20 +++++++++++++++++++- src/chatgpt_web2api/mcp_server.py | 7 ++++++- 3 files changed, 28 insertions(+), 2 deletions(-) diff --git a/.gitignore b/.gitignore index b7056c4f..dfe8a59b 100644 --- a/.gitignore +++ b/.gitignore @@ -14,3 +14,6 @@ sentinel_flow.json .rest-proxy.log .sse-server.log + +# ChatGPT collaboration binding (local only) +.chatgpt-collaboration.json diff --git a/src/chatgpt_web2api/mcp_driver_pool.py b/src/chatgpt_web2api/mcp_driver_pool.py index 84371b00..37b8afcc 100644 --- a/src/chatgpt_web2api/mcp_driver_pool.py +++ b/src/chatgpt_web2api/mcp_driver_pool.py @@ -31,12 +31,30 @@ # ── Errors ──────────────────────────────────────────────────────────────── class PoolExhaustedError(RuntimeError): - """Pool is full after acquire_timeout.""" + """Pool is full after acquire_timeout. + + Carries a default human-readable message (mirroring RateLimitError / + AuthExpiredError in cdp_driver.py) so the MCP error-formatter can't + surface an empty diagnostic. The bare ``raise PoolExhaustedError()`` + sites below now render as + ``"PoolExhaustedError: no driver slot available within acquire_timeout. + Retry later."`` rather than ``": . Retry later."``. + """ + + def __init__(self, message: str | None = None) -> None: + if message is None: + message = "no driver slot available within acquire_timeout" + super().__init__(message) class PoolShuttingDownError(RuntimeError): """Pool is shutting down; no new slots can be created.""" + def __init__(self, message: str | None = None) -> None: + if message is None: + message = "driver pool is shutting down; no new slots can be created" + super().__init__(message) + class PoolSlotUnavailableError(RuntimeError): """A materialized slot's driver disappeared (race during shutdown).""" diff --git a/src/chatgpt_web2api/mcp_server.py b/src/chatgpt_web2api/mcp_server.py index 3952ada6..ffbecec6 100644 --- a/src/chatgpt_web2api/mcp_server.py +++ b/src/chatgpt_web2api/mcp_server.py @@ -1664,8 +1664,13 @@ async def _run_pooled() -> dict: # Shared result formatting (singleton parity, PR #42 fix #1). return _format_tool_result(name, result) except (PoolExhaustedError, PoolShuttingDownError) as e: + # Defensive: never let an empty-message exception surface as + # ". Retry later." Fall back to the type name so the caller always + # gets a diagnosable signal. (The exception classes now carry a + # default message, but this guards against future bare raises.) + detail = str(e) or type(e).__name__ return mcp_types.CallToolResult( - content=[mcp_types.TextContent(type="text", text=f"{e}. Retry later.")], + content=[mcp_types.TextContent(type="text", text=f"{detail}. Retry later.")], isError=True, ) except Exception as exc: From 78bdad0fbae9951a03b0239dc0e085d1872644d4 Mon Sep 17 00:00:00 2001 From: Alajmah <55954113+Alajmah@users.noreply.github.com> Date: Thu, 9 Jul 2026 00:36:39 +0300 Subject: [PATCH 02/25] feat(detector): P1 model-aware stall detector state machine MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replaces the single hardcoded PHASE_STALL_SECONDS=90 with a model-aware, phase-substate-aware two-state detector. Co-designed with ChatGPT (conversation 6a4ebc1e, 2026-07-08). Core change: phase-2 splits into awaiting_first_content (budget: first_content_timeout_seconds — longer for reasoning models that think silently for 30-60s+) and streaming_after_first_content (budget: stream_idle_timeout_seconds — shorter, since a long idle once text has started is suspicious). A hard_timeout_seconds absolute cap prevents infinite waits regardless of DOM liveness signals. 5 new config keys (detector.*), all env-overridable (W2A_DETECTOR_*): reasoning_first_content_timeout_seconds = 300 reasoning_stream_idle_timeout_seconds = 120 default_first_content_timeout_seconds = 90 default_stream_idle_timeout_seconds = 90 hard_timeout_seconds = 900 Other changes: - classify_model(): maps slugs to 'reasoning'/'default' via markers (thinking, -t-mini, o3, research). Unknown models default to 'default'. - DetectorBudgets: frozen dataclass with default()/reasoning()/from_config(). - GenerationStuckError extended with structured fields (stall_kind, model_class, elapsed_seconds, generation_active_signal, turn_id). Backward-compatible (new fields keyword-only, optional). - Final reconciliation: before raising a phase-2 stall, the detector does one backend read; if the turn completed, it returns normally instead of raising (field-verified: generation often completes after the detector would have given up). Logs completed_after_detector_stall event. - DOM thinking/generating signals are advisory liveness hints only (generation_active_signal) — never authoritative clock-pauses. - NEVER auto-retry sends on phase-2 stalls (would duplicate messages). Observation/reconciliation retry is safe and already supported via A2. Threaded through: send_and_stream(budgets=, model=), api_server (_full_response + _stream_response), mcp_server (do_chat_completion). 654 tests pass (628 existing + 26 new P1 tests). Legacy behavior preserved when budgets=None (back-compat for paths without config). See BRIDGE_DIAGNOSIS.md failure mode #3 for context. --- src/chatgpt_web2api/api_server.py | 16 +- src/chatgpt_web2api/cdp_driver.py | 56 +++- src/chatgpt_web2api/completion_detector.py | 263 +++++++++++++++- src/chatgpt_web2api/config.py | 44 +++ src/chatgpt_web2api/mcp_server.py | 13 +- tests/test_breaker_failfast.py | 6 +- tests/test_business.py | 6 +- tests/test_conversation_guard.py | 2 +- tests/test_detector_budgets.py | 121 ++++++++ tests/test_detector_phase2.py | 338 +++++++++++++++++++++ tests/test_integration.py | 2 +- tests/test_mcp_rate_limit.py | 4 +- tests/test_progress.py | 2 +- tests/test_reliability.py | 2 +- 14 files changed, 855 insertions(+), 20 deletions(-) create mode 100644 tests/test_detector_budgets.py create mode 100644 tests/test_detector_phase2.py diff --git a/src/chatgpt_web2api/api_server.py b/src/chatgpt_web2api/api_server.py index 3c9f8bd4..c0ce8894 100644 --- a/src/chatgpt_web2api/api_server.py +++ b/src/chatgpt_web2api/api_server.py @@ -530,10 +530,16 @@ async def _full_response( transparently — the client only sees it (as a 429) if the limit persists across all retries. """ + # P1: resolve model-aware detector budgets from config. + from .completion_detector import DetectorBudgets + + budgets = DetectorBudgets.from_config(self._config.chatgpt, model) async def _send_and_collect() -> str: collected = "" - async for chunk in self._driver.send_and_stream(text, timeout=timeout): + async for chunk in self._driver.send_and_stream( + text, timeout=timeout, budgets=budgets, model=model, + ): collected += chunk.delta return collected @@ -577,6 +583,10 @@ async def _stream_response( cleared it), but if one occurs it falls back to the inline ``[Error: ...]`` SSE chunk — documented as a known limitation. """ + # P1: resolve model-aware detector budgets from config. + from .completion_detector import DetectorBudgets + + budgets = DetectorBudgets.from_config(self._config.chatgpt, model) async def _preflight() -> None: """Raise RateLimitError if the pop-up is present right now.""" @@ -638,7 +648,9 @@ async def _preflight() -> None: ) try: - async for chunk in self._driver.send_and_stream(text, timeout=timeout): + async for chunk in self._driver.send_and_stream( + text, timeout=timeout, budgets=budgets, model=model, + ): if chunk.delta: await self._send_sse( resp, diff --git a/src/chatgpt_web2api/cdp_driver.py b/src/chatgpt_web2api/cdp_driver.py index 8e06dcc2..567d55b9 100644 --- a/src/chatgpt_web2api/cdp_driver.py +++ b/src/chatgpt_web2api/cdp_driver.py @@ -142,13 +142,51 @@ class GenerationStuckError(RuntimeError): - ``phase == "phase_1_appear"``: assistant message node never appeared. - ``phase == "phase_2_stream"``: streaming started but text stopped changing. + + P1 (2026-07-08): phase-2 stalls now carry richer structured fields for + observability and caller-side reconciliation decisions: + + - ``stall_kind``: ``"first_content_timeout"`` (no text appeared within the + first-content budget — common for reasoning models in the thinking phase) + or ``"stream_idle_timeout"`` (text appeared then stopped progressing) or + ``"hard_timeout"`` (absolute wall-clock cap exceeded). + - ``model_class``: ``"reasoning"`` or ``"default"`` (from classify_model). + - ``elapsed_seconds``: total time spent in phase-2 observation. + - ``generation_active_signal``: whether a DOM thinking/generating indicator + was present at the moment of the stall (advisory — a liveness hint). + - ``turn_id``: the turn anchor's captured UUID if available, for caller-side + reconciliation/retry of OBSERVATION (never retry the send). + + The structured fields are optional (keyword-only) so existing phase-1 + construction sites remain compatible. Callers should NEVER auto-retry the + SEND on a phase-2 stall (the generation may still be running and would + duplicate the message). Safe retry is observation-only: re-read the + conversation and reconcile against the same turn. """ - def __init__(self, phase: str, stalled_for_s: float) -> None: + def __init__( + self, + phase: str, + stalled_for_s: float, + *, + stall_kind: str | None = None, + model_class: str | None = None, + elapsed_seconds: float | None = None, + generation_active_signal: bool | None = None, + turn_id: str | None = None, + ) -> None: self.phase = phase self.stalled_for_s = float(stalled_for_s) + # P1 structured fields (optional for back-compat with phase-1 sites). + self.stall_kind = stall_kind + self.model_class = model_class + self.elapsed_seconds = float(elapsed_seconds) if elapsed_seconds is not None else None + self.generation_active_signal = generation_active_signal + self.turn_id = turn_id + # Human-readable message includes the stall kind if available. + kind_str = f" ({stall_kind})" if stall_kind else "" super().__init__( - f"Generation stalled in {phase} for {stalled_for_s:.0f}s — no DOM progress" + f"Generation stalled in {phase}{kind_str} for {stalled_for_s:.0f}s — no DOM progress" ) @@ -1459,7 +1497,14 @@ async def _capture_pre_send_fallback_anchor(self, text: str): conversation_id_at_capture=conv_id, ) - async def send_and_stream(self, text: str, timeout: float = 120) -> AsyncIterator[StreamChunk]: + async def send_and_stream( + self, + text: str, + timeout: float = 120, + *, + budgets=None, + model: str | None = None, + ) -> AsyncIterator[StreamChunk]: """Send a message and yield streaming response chunks. A2 turn-correlation sequence (peer-reviewed, conv ``6a482cfd``): @@ -1514,10 +1559,15 @@ async def send_and_stream(self, text: str, timeout: float = 120) -> AsyncIterato turn_anchor = fallback_anchor.with_captured_id(captured_uuid) # A2 Step 8: stream + completion with the anchored turn. + # P1: pass budgets + model for the model-aware two-state phase-2 + # machine. When None (no config available), the detector uses the + # legacy single PHASE_STALL_SECONDS behavior. async for chunk in self._completion.stream_until_complete( initial_count=initial_count, timeout=timeout, turn_anchor=turn_anchor, + budgets=budgets, + model=model, ): yield chunk diff --git a/src/chatgpt_web2api/completion_detector.py b/src/chatgpt_web2api/completion_detector.py index c3c47738..b0b92b13 100644 --- a/src/chatgpt_web2api/completion_detector.py +++ b/src/chatgpt_web2api/completion_detector.py @@ -64,12 +64,14 @@ import json import logging import time +from dataclasses import dataclass from typing import TYPE_CHECKING if TYPE_CHECKING: from collections.abc import AsyncIterator from .cdp_driver import StreamChunk + from .config import ChatGPTConfig logger = logging.getLogger(__name__) @@ -84,8 +86,118 @@ # while the model reasons before the first answer token renders; the # is_thinking reset covers the labeled phase, but there is an unlabeled gap # between thinking-end and answer-start that also needs this headroom. +# +# P1 (2026-07-08): this constant is now the FALLBACK for phase_1_appear +# (unchanged behavior) and the default-class phase-2 budgets when no +# DetectorBudgets is resolved. Phase-2 detection has been refactored into a +# model-aware two-state machine (awaiting_first_content → +# streaming_after_first_content) with budgets from DetectorBudgets. See +# classify_model / DetectorBudgets below. Kept for back-compat (re-exported +# from cdp_driver; referenced by phase_1_appear which is deliberately +# unchanged). PHASE_STALL_SECONDS = 90 + +# ── P1: model-aware detector budgets ───────────────────────────────────── +# +# Co-designed with ChatGPT (conversation 6a4ebc1e, 2026-07-08). The single +# PHASE_STALL_SECONDS=90 conflated two different states: "model hasn't +# produced visible text yet" (reasoning models think silently for 30-60s+) +# and "model was streaming text and then stopped" (a genuine hang). These +# should NOT share a stall budget. Reasoning models stress case 1; real +# network/UI hangs stress case 2. +# +# The fix: split phase-2 into first-content-wait vs stream-idle, with +# model-aware budgets on first-content specifically. DOM "thinking" signals +# are advisory liveness hints (generation_active_signal) within the hard +# cap — never authoritative clock-pauses. + +# Slugs that indicate a reasoning-capable model. These models have a long +# silent "thinking" phase before the first answer token renders, which the +# old uniform 90s stall window falsely aborted. Matched case-insensitively +# against the slug via substring. "thinking" covers gpt-5-*-thinking and +# gpt-5-*-t-mini (Thinking Mini); "o3" and "research" are the other +# reasoning-class families. +_REASONING_MARKERS = ("thinking", "-t-mini", "o3", "research") + + +def classify_model(model: str | None) -> str: + """Classify a ChatGPT web model slug as ``"reasoning"`` or ``"default"``. + + Reasoning models (gpt-5-*-thinking, o3, research, Thinking Mini variants) + have a long silent thinking phase before streaming text and need a longer + first-content stall budget. An unknown or empty model is conservatively + classified ``"default"`` — the shorter budget — so a possibly-dead + generation fails fast rather than being held too long. + + The classification is intentionally coarse (two buckets), not per-model. + Per-model tuning can wait until field data proves the buckets insufficient. + """ + if not model: + return "default" + lowered = model.lower() + if any(marker in lowered for marker in _REASONING_MARKERS): + return "reasoning" + return "default" + + +@dataclass(frozen=True) +class DetectorBudgets: + """Per-call detector timeout budgets, resolved from config + model class. + + - ``first_content_timeout_seconds``: how long to wait for the FIRST text + content after the assistant node appears. Longer for reasoning models + (they think silently before streaming). + - ``stream_idle_timeout_seconds``: how long to wait for PROGRESS once text + has already appeared and then stopped. Shorter than first-content — once + streaming started, a long idle is suspicious. + - ``hard_timeout_seconds``: absolute wall-clock cap on phase-2 observation, + regardless of DOM liveness signals. Prevents infinite waits even if the + UI claims it is still thinking. + """ + + first_content_timeout_seconds: float + stream_idle_timeout_seconds: float + hard_timeout_seconds: float + + @classmethod + def default(cls) -> DetectorBudgets: + """Default (non-reasoning) budgets — reproduces the legacy 90s behavior.""" + return cls( + first_content_timeout_seconds=90, + stream_idle_timeout_seconds=90, + hard_timeout_seconds=900, + ) + + @classmethod + def reasoning(cls) -> DetectorBudgets: + """Reasoning-model budgets — longer first-content, shorter stream-idle.""" + return cls( + first_content_timeout_seconds=300, + stream_idle_timeout_seconds=120, + hard_timeout_seconds=900, + ) + + @classmethod + def from_config(cls, config: ChatGPTConfig, model: str | None) -> DetectorBudgets: + """Resolve budgets from ChatGPTConfig + the classified model. + + Reads the 5 detector config keys. If the model classifies as reasoning, + uses the reasoning first-content/stream-idle budgets; otherwise default. + The hard cap is shared across both classes. + """ + if classify_model(model) == "reasoning": + return cls( + first_content_timeout_seconds=config.detector_reasoning_first_content_timeout_seconds, + stream_idle_timeout_seconds=config.detector_reasoning_stream_idle_timeout_seconds, + hard_timeout_seconds=config.detector_hard_timeout_seconds, + ) + return cls( + first_content_timeout_seconds=config.detector_default_first_content_timeout_seconds, + stream_idle_timeout_seconds=config.detector_default_stream_idle_timeout_seconds, + hard_timeout_seconds=config.detector_hard_timeout_seconds, + ) + # Phrases ChatGPT uses in its rate-limit pop-up. Matched case-insensitively # against scanned DOM text. Kept narrow to avoid false positives on normal # chat content (e.g. a user asking about "rate limits" in a message). @@ -127,12 +239,48 @@ def __init__(self, driver) -> None: self.last_dom_text: str = "" self.had_non_text_content: bool = False + async def _reconcile_before_stall( + self, d, conv_id: str, turn_anchor, had_non_text_content: bool, + ) -> bool: + """P1: final reconciliation before raising a phase-2 stall. + + Field evidence shows the generation often completes after the detector + would have given up (the stall kills the *observation*, not the + generation). Before raising, check the backend one more time: if the + turn completed, the detector should return normally rather than + surfacing a false failure. + + Returns True if the turn completed (caller returns normally), False if + not (caller raises). Safe by design — this is an OBSERVATION read, not + a re-send; it cannot duplicate the user message. Leverages the A2 + turn-anchoring work to correlate against the correct turn. + + On any fetch failure or exception, returns False (let the stall raise) + rather than degrading to a silent success. + """ + from .turn_anchor import collapse_to_end_turn_status + + if not conv_id: + return False + try: + end_result = await d._fetch_end_turn_for_turn( + conv_id, turn_anchor, + had_non_text_content=had_non_text_content, + ) + status = collapse_to_end_turn_status(end_result) + return status == "complete" + except Exception as e: + logger.debug("Final reconciliation fetch failed: %s", e) + return False + async def stream_until_complete( self, *, initial_count: int, timeout: float, turn_anchor, + budgets: DetectorBudgets | None = None, + model: str | None = None, ) -> AsyncIterator[StreamChunk]: """Run Phase-1 (appear) + Phase-2 (stream) and yield delta chunks. @@ -150,6 +298,19 @@ async def stream_until_complete( set ONLY on ``fetch_failed`` (true transport failure), NOT on ``not_ready``/``ambiguous``/``degraded_not_fresh`` (which collapse to ``not_ready`` and must NOT unlock the DOM fallback). + + P1 (2026-07-08): ``budgets`` and ``model`` enable the model-aware + two-state phase-2 machine. When ``budgets`` is None, the legacy + behavior (single PHASE_STALL_SECONDS=90 for both phases) is preserved + for back-compat. When provided, phase-2 splits into + awaiting_first_content (first_content_timeout_seconds budget) and + streaming_after_first_content (stream_idle_timeout_seconds budget), + with a hard_timeout_seconds absolute cap. DOM thinking/generating + signals are advisory liveness hints (generation_active_signal) within + the hard cap — never authoritative clock-pauses. On phase-2 stall, a + final reconciliation is attempted before raising: if the backend + reports the turn completed, the detector returns normally instead of + raising (the generation actually finished). """ # Imported lazily to avoid a module-load circular dependency: cdp_driver # top-level re-exports PHASE_STALL_SECONDS / is_rate_limited_text from @@ -165,6 +326,11 @@ async def stream_until_complete( d = self._driver + # P1: resolve the model class for structured error reporting. + model_class = classify_model(model) if model else "default" + # P1: budgets default to legacy behavior when not provided (back-compat). + use_two_state = budgets is not None + # Reset per-call results surfaced to the driver tail. self.last_dom_text = "" self.had_non_text_content = False @@ -271,6 +437,15 @@ async def stream_until_complete( # streaming) rather than .markdown textContent (which lags). last_change_time = time.monotonic() deadline = time.monotonic() + timeout + # P1: two-state phase-2 machine. phase_2_start tracks total observation + # time for the hard cap; first_content_seen tracks whether we've + # transitioned from awaiting_first_content to streaming_after_first_content. + # The active stall budget depends on this state: first-content uses + # budgets.first_content_timeout_seconds (longer for reasoning models that + # think silently); stream-idle uses budgets.stream_idle_timeout_seconds. + phase_2_start = time.monotonic() + first_content_seen = False + generation_active_signal = False # advisory liveness (DOM thinking/generating) # Backend end_turn fallback throttle (R4): if the DOM action-button # selector drifts again, the conversation API's end_turn flag is a # secondary completion signal. Throttled to once per 3s to respect the @@ -413,8 +588,19 @@ async def stream_until_complete( saw_thinking = True if not conv_id_for_check: last_change_time = time.monotonic() + # P1: track advisory liveness signal for structured error reporting. + # True when the DOM shows active generation (thinking indicator or + # a generating/stop-button state). This is advisory only — it + # informs the structured error and logging, but does NOT pause the + # stall clock (a stuck indicator must not create an infinite hang). + generation_active_signal = bool(is_thinking) if current != last_dom_text: last_change_time = time.monotonic() + # P1: first text content transitions us from awaiting_first_content + # to streaming_after_first_content. The stall budget changes with + # the state (see the stall check below). + if current and not first_content_seen: + first_content_seen = True if len(current) > len(last_dom_text): delta = current[len(last_dom_text) :] yield StreamChunk(delta=delta) @@ -427,6 +613,9 @@ async def stream_until_complete( if html_len > 50: had_non_text_content = True self.had_non_text_content = True + # P1: non-text content also counts as first-content (images/tool-use). + if not first_content_seen: + first_content_seen = True last_html_len = html_len last_child_count = child_count @@ -529,8 +718,78 @@ async def stream_until_complete( logger.info("DOM has_action (fallback completion) — no backend signal") break - if time.monotonic() - last_change_time > PHASE_STALL_SECONDS: - raise GenerationStuckError("phase_2_stream", time.monotonic() - last_change_time) + # ── P1: model-aware two-state stall detection ─────────────────── + # Replaces the single PHASE_STALL_SECONDS check. When budgets are + # provided, phase-2 splits into two states with separate budgets: + # - awaiting_first_content: no text yet. Uses + # first_content_timeout_seconds (longer for reasoning models). + # - streaming_after_first_content: text appeared then stopped. + # Uses stream_idle_timeout_seconds (shorter — once streaming + # started, a long idle is suspicious). + # A hard_timeout_seconds absolute cap applies regardless of state + # or DOM liveness. DOM thinking/generating signals are advisory + # (generation_active_signal) — they inform logging but do NOT pause + # the stall clock (a stuck thinking indicator must not create an + # infinite hang). + # + # On stall: attempt ONE final reconciliation read before raising. + # If the backend reports the turn completed, return normally — the + # generation actually finished (field-verified case). Only if + # reconciliation finds no completion do we raise a structured + # GenerationStuckError. + if use_two_state: + elapsed_since_progress = time.monotonic() - last_change_time + elapsed_total = time.monotonic() - phase_2_start + + # Determine which budget applies based on the current state. + stall_budget = ( + budgets.stream_idle_timeout_seconds + if first_content_seen + else budgets.first_content_timeout_seconds + ) + stall_kind = ( + "stream_idle_timeout" + if first_content_seen + else "first_content_timeout" + ) + + # Hard cap: absolute wall-clock limit regardless of DOM signals. + hard_cap_hit = elapsed_total > budgets.hard_timeout_seconds + budget_hit = elapsed_since_progress > stall_budget + + if hard_cap_hit or budget_hit: + # Final reconciliation: did the turn actually complete? + # Field evidence: the generation often completes after the + # detector would have given up. Before raising, check the + # backend one more time. + turn_id = getattr(turn_anchor, "captured_id", None) + reconciled = await self._reconcile_before_stall( + d, conv_id_for_check, turn_anchor, + had_non_text_content, + ) + if reconciled: + logger.info( + "Phase-2 %s reconciled after stall — generation " + "had completed (elapsed=%.0fs, kind=%s, " + "model_class=%s, active=%s)", + stall_kind, elapsed_total, stall_kind, + model_class, generation_active_signal, + ) + return # generation completed — return normally + # Reconciliation found no completion — raise structured error. + raise GenerationStuckError( + "phase_2_stream", + elapsed_since_progress, + stall_kind=("hard_timeout" if hard_cap_hit else stall_kind), + model_class=model_class, + elapsed_seconds=elapsed_total, + generation_active_signal=generation_active_signal, + turn_id=turn_id, + ) + else: + # Legacy path (no budgets provided): single PHASE_STALL_SECONDS. + if time.monotonic() - last_change_time > PHASE_STALL_SECONDS: + raise GenerationStuckError("phase_2_stream", time.monotonic() - last_change_time) await asyncio.sleep(0.5) diff --git a/src/chatgpt_web2api/config.py b/src/chatgpt_web2api/config.py index 548b274d..57814257 100644 --- a/src/chatgpt_web2api/config.py +++ b/src/chatgpt_web2api/config.py @@ -97,6 +97,18 @@ class ChatGPTConfig: # Account-level throttle breaker: pauses mutations pool-wide if ChatGPT # signals excessive consumption from simultaneous multi-tab use. mcp_account_throttle_cooldown_seconds: int = 300 + # P1: model-aware detector budgets. Phase-2 stall detection splits into + # first-content-wait (no text yet) vs stream-idle (text appeared then + # stopped), with model-aware budgets. See classify_model / + # DetectorBudgets in completion_detector.py. Defaults reproduce the + # legacy 90s behavior for non-reasoning models and give reasoning models + # a longer first-content window (300s) so their silent thinking phase + # isn't falsely aborted. + detector_reasoning_first_content_timeout_seconds: float = 300 + detector_reasoning_stream_idle_timeout_seconds: float = 120 + detector_default_first_content_timeout_seconds: float = 90 + detector_default_stream_idle_timeout_seconds: float = 90 + detector_hard_timeout_seconds: float = 900 @dataclass @@ -262,6 +274,22 @@ def _apply_dict(self, data: dict) -> None: c = data.get("mcp_account_throttle_cooldown_seconds") if c is not None: self.chatgpt.mcp_account_throttle_cooldown_seconds = int(c) + # P1 detector budgets + c = data.get("detector_reasoning_first_content_timeout_seconds") + if c is not None: + self.chatgpt.detector_reasoning_first_content_timeout_seconds = float(c) + c = data.get("detector_reasoning_stream_idle_timeout_seconds") + if c is not None: + self.chatgpt.detector_reasoning_stream_idle_timeout_seconds = float(c) + c = data.get("detector_default_first_content_timeout_seconds") + if c is not None: + self.chatgpt.detector_default_first_content_timeout_seconds = float(c) + c = data.get("detector_default_stream_idle_timeout_seconds") + if c is not None: + self.chatgpt.detector_default_stream_idle_timeout_seconds = float(c) + c = data.get("detector_hard_timeout_seconds") + if c is not None: + self.chatgpt.detector_hard_timeout_seconds = float(c) c = data.get("request_timeout") if c is not None: self.server.request_timeout = int(c) @@ -316,6 +344,17 @@ def _apply_env(self) -> None: self.chatgpt.mcp_session_pool_create_concurrency = int(v) if v := _env("W2A_MCP_ACCOUNT_THROTTLE_COOLDOWN_SECONDS"): self.chatgpt.mcp_account_throttle_cooldown_seconds = int(v) + # P1 detector budgets + if v := _env("W2A_DETECTOR_REASONING_FIRST_CONTENT_TIMEOUT_SECONDS"): + self.chatgpt.detector_reasoning_first_content_timeout_seconds = float(v) + if v := _env("W2A_DETECTOR_REASONING_STREAM_IDLE_TIMEOUT_SECONDS"): + self.chatgpt.detector_reasoning_stream_idle_timeout_seconds = float(v) + if v := _env("W2A_DETECTOR_DEFAULT_FIRST_CONTENT_TIMEOUT_SECONDS"): + self.chatgpt.detector_default_first_content_timeout_seconds = float(v) + if v := _env("W2A_DETECTOR_DEFAULT_STREAM_IDLE_TIMEOUT_SECONDS"): + self.chatgpt.detector_default_stream_idle_timeout_seconds = float(v) + if v := _env("W2A_DETECTOR_HARD_TIMEOUT_SECONDS"): + self.chatgpt.detector_hard_timeout_seconds = float(v) if v := _env("W2A_HEADLESS"): self.chrome.headless = v.lower() in ("true", "1", "yes") if v := _env("W2A_LOG_LEVEL"): @@ -347,6 +386,11 @@ def to_dict(self) -> dict: "mcp_session_pool_sweep_interval_seconds": self.chatgpt.mcp_session_pool_sweep_interval_seconds, "mcp_session_pool_create_concurrency": self.chatgpt.mcp_session_pool_create_concurrency, "mcp_account_throttle_cooldown_seconds": self.chatgpt.mcp_account_throttle_cooldown_seconds, + "detector_reasoning_first_content_timeout_seconds": self.chatgpt.detector_reasoning_first_content_timeout_seconds, + "detector_reasoning_stream_idle_timeout_seconds": self.chatgpt.detector_reasoning_stream_idle_timeout_seconds, + "detector_default_first_content_timeout_seconds": self.chatgpt.detector_default_first_content_timeout_seconds, + "detector_default_stream_idle_timeout_seconds": self.chatgpt.detector_default_stream_idle_timeout_seconds, + "detector_hard_timeout_seconds": self.chatgpt.detector_hard_timeout_seconds, "request_timeout": self.server.request_timeout, "log_level": self.log.level, "ensure_degraded_poll_interval_s": self.ensure.degraded_poll_interval_s, diff --git a/src/chatgpt_web2api/mcp_server.py b/src/chatgpt_web2api/mcp_server.py index ffbecec6..8387f0c4 100644 --- a/src/chatgpt_web2api/mcp_server.py +++ b/src/chatgpt_web2api/mcp_server.py @@ -778,7 +778,18 @@ async def do_chat_completion( full_response = "" conv_id = "" chunk_count = 0 - async for chunk in driver.send_and_stream(full_text, timeout=120): + # P1: resolve model-aware detector budgets from config. Guard against + # config=None (some test paths) by falling back to legacy behavior. + from .completion_detector import DetectorBudgets + + _budgets = ( + DetectorBudgets.from_config(config.chatgpt, validated.model) + if config is not None + else None + ) + async for chunk in driver.send_and_stream( + full_text, timeout=120, budgets=_budgets, model=validated.model, + ): if chunk.delta: full_response += chunk.delta chunk_count += 1 diff --git a/tests/test_breaker_failfast.py b/tests/test_breaker_failfast.py index 26b01715..5f410a07 100644 --- a/tests/test_breaker_failfast.py +++ b/tests/test_breaker_failfast.py @@ -75,7 +75,7 @@ def _make_mcp_server_with_open_breaker(): driver._current_model = None # If the handler somehow runs, yield a benign chunk (it should NOT). - async def _stream(text, timeout=120): + async def _stream(text, timeout=120, *, budgets=None, model=None): from chatgpt_web2api.cdp_driver import StreamChunk yield StreamChunk(delta="should-not-reach") @@ -140,7 +140,7 @@ async def _noop(_s): driver._current_conv_id = "" driver._current_model = None - async def _stream(text, timeout=120): + async def _stream(text, timeout=120, *, budgets=None, model=None): from chatgpt_web2api.cdp_driver import StreamChunk yield StreamChunk(delta="ok") @@ -200,7 +200,7 @@ async def test_rest_post_lock_check_catches_race(monkeypatch): driver.navigate_conversation = AsyncMock() driver.ensure_current_conversation = AsyncMock() - async def _stream(text, timeout=120): + async def _stream(text, timeout=120, *, budgets=None, model=None): yield MagicMock() driver.send_and_stream = _stream diff --git a/tests/test_business.py b/tests/test_business.py index 0aeb6e21..87fe9cac 100644 --- a/tests/test_business.py +++ b/tests/test_business.py @@ -26,7 +26,7 @@ def mock_driver(): driver.select_model = AsyncMock(return_value=True) # Wire send_and_stream to yield a simple response - async def _stream(text, timeout=120): + async def _stream(text, timeout=120, *, budgets=None, model=None): yield StreamChunk(delta="Hello!") yield StreamChunk(delta="", finish_reason="stop") @@ -497,7 +497,7 @@ async def test_api_message_history_includes_assistant(): captured_text = {} - async def _stream(text, timeout=120): + async def _stream(text, timeout=120, *, budgets=None, model=None): captured_text["value"] = text yield StreamChunk(delta="Response") yield StreamChunk(delta="", finish_reason="stop") @@ -546,7 +546,7 @@ async def test_api_model_selection_called(): driver._access_token = "test" driver.select_model = AsyncMock(return_value=True) - async def _stream(text, timeout=120): + async def _stream(text, timeout=120, *, budgets=None, model=None): yield StreamChunk(delta="OK") yield StreamChunk(delta="", finish_reason="stop") diff --git a/tests/test_conversation_guard.py b/tests/test_conversation_guard.py index 8d09cd23..623f0433 100644 --- a/tests/test_conversation_guard.py +++ b/tests/test_conversation_guard.py @@ -311,7 +311,7 @@ async def test_mcp_auto_continue_invokes_ensure_current(): driver.navigate_new_chat = AsyncMock() driver.navigate_conversation = AsyncMock() - async def _boom(text, timeout=120): + async def _boom(text, timeout=120, *, budgets=None, model=None): raise AssertionError("reached past the guard") yield # pragma: no cover (generator signature) driver.send_and_stream = _boom diff --git a/tests/test_detector_budgets.py b/tests/test_detector_budgets.py new file mode 100644 index 00000000..d87217a6 --- /dev/null +++ b/tests/test_detector_budgets.py @@ -0,0 +1,121 @@ +"""Tests for P1 model-aware detector budgets (stall-aware detector). + +Co-designed with ChatGPT (conversation 6a4ebc1e, 2026-07-08). Replaces the +single PHASE_STALL_SECONDS=90 with a model-aware, phase-substate-aware state +machine: first-content-wait vs stream-idle, with longer first-content budgets +for reasoning models that think silently before streaming. + +Covers: + - classify_model: maps a model slug to "reasoning" | "default" + - DetectorConfig: the 5-key config on ChatGPTConfig + - DetectorBudgets: resolved per-call budgets from config + model class +""" + +import pytest + +from chatgpt_web2api.completion_detector import ( + DetectorBudgets, + classify_model, +) + + +# ── classify_model ────────────────────────────────────────────────────── + + +class TestClassifyModel: + """classify_model maps a ChatGPT web slug to a model class bucket.""" + + @pytest.mark.parametrize("slug", [ + "gpt-5-4-thinking", + "gpt-5-5-thinking", + "gpt-5-4-t-mini", # "Thinking Mini" + "o3", + "research", + ]) + def test_reasoning_models_classified_as_reasoning(self, slug): + assert classify_model(slug) == "reasoning" + + @pytest.mark.parametrize("slug", [ + "gpt-5-5", + "gpt-5-5-instant", + "gpt-5-3", + "gpt-5-mini", + "gpt-5-5-mini", + "auto", + "gpt-5.5-wm", + ]) + def test_non_reasoning_models_classified_as_default(self, slug): + assert classify_model(slug) == "default" + + def test_unknown_model_defaults_to_default(self): + """An unrecognized slug is conservative — default budgets, not reasoning. + Reasoning budgets are longer, so defaulting an unknown model to 'default' + fails fast rather than holding a possibly-dead generation too long.""" + assert classify_model("some-new-model-xyz") == "default" + + def test_none_or_empty_defaults_to_default(self): + assert classify_model(None) == "default" + assert classify_model("") == "default" + + def test_case_insensitive(self): + """Model slugs from the web are lowercase, but be robust.""" + assert classify_model("GPT-5-5-Thinking") == "reasoning" + assert classify_model("O3") == "reasoning" + + +# ── DetectorBudgets ───────────────────────────────────────────────────── + + +class TestDetectorBudgets: + """DetectorBudgets holds the resolved per-call timeout values.""" + + def test_default_budgets_match_legacy_90s(self): + """A default-class model with default config should reproduce the old + 90s behavior on both first-content and stream-idle — no behavior change + for non-reasoning models.""" + budgets = DetectorBudgets.default() + assert budgets.first_content_timeout_seconds == 90 + assert budgets.stream_idle_timeout_seconds == 90 + assert budgets.hard_timeout_seconds == 900 + + def test_reasoning_budgets_have_longer_first_content(self): + """The core P1 fix: reasoning models get a longer first-content window + so their silent thinking phase isn't falsely aborted.""" + budgets = DetectorBudgets.reasoning() + assert budgets.first_content_timeout_seconds > 90, ( + "reasoning first-content must be longer than the legacy 90s" + ) + assert budgets.first_content_timeout_seconds == 300 + + def test_reasoning_stream_idle_is_shorter_than_first_content(self): + """Once text has started streaming, a long idle IS suspicious — stream-idle + should be shorter than first-content for reasoning models.""" + budgets = DetectorBudgets.reasoning() + assert budgets.stream_idle_timeout_seconds < budgets.first_content_timeout_seconds + + def test_hard_cap_is_absolute_for_both_classes(self): + """The hard cap bounds total observation time regardless of DOM signals. + Both default and reasoning share the same cap.""" + d = DetectorBudgets.default() + r = DetectorBudgets.reasoning() + assert d.hard_timeout_seconds == r.hard_timeout_seconds == 900 + + def test_from_config_default_model(self): + """Building budgets from config + a default-class model yields the + default first-content / stream-idle from config.""" + from chatgpt_web2api.config import ChatGPTConfig + + cfg = ChatGPTConfig() + budgets = DetectorBudgets.from_config(cfg, model="gpt-5-5") + assert budgets.first_content_timeout_seconds == cfg.detector_default_first_content_timeout_seconds + assert budgets.stream_idle_timeout_seconds == cfg.detector_default_stream_idle_timeout_seconds + + def test_from_config_reasoning_model(self): + """Building budgets from config + a reasoning model yields the reasoning + first-content / stream-idle from config.""" + from chatgpt_web2api.config import ChatGPTConfig + + cfg = ChatGPTConfig() + budgets = DetectorBudgets.from_config(cfg, model="gpt-5-5-thinking") + assert budgets.first_content_timeout_seconds == cfg.detector_reasoning_first_content_timeout_seconds + assert budgets.stream_idle_timeout_seconds == cfg.detector_reasoning_stream_idle_timeout_seconds diff --git a/tests/test_detector_phase2.py b/tests/test_detector_phase2.py new file mode 100644 index 00000000..65cb0d72 --- /dev/null +++ b/tests/test_detector_phase2.py @@ -0,0 +1,338 @@ +"""Behavioral tests for P1 phase-2 two-state detector machine. + +These verify the core co-designed behavior (conversation 6a4ebc1e): + + 1. Reasoning first-content wait does NOT fail at the old 90s default — the + longer reasoning budget keeps the detector alive during the silent + thinking phase. + 2. Stream-idle (text appeared then stopped) fails with its OWN budget, + shorter than first-content. + 3. Hard cap wins over an active DOM liveness signal — no infinite waits. + 4. Final reconciliation: if the turn completed in the backend, the detector + returns normally instead of raising (generation actually finished). + 5. Structured error: phase-2 stalls carry stall_kind, model_class, elapsed + time, and liveness-signal fields. + +Timing is controlled via a fake clock to keep tests deterministic and fast. +""" + +import asyncio +import json +import time + +import pytest +from unittest.mock import AsyncMock, MagicMock + +from chatgpt_web2api.completion_detector import ( + CompletionDetector, + DetectorBudgets, +) +from chatgpt_web2api.turn_anchor import TurnAnchor, TurnEndResult + + +def _make_detector(budgets=None, conv_id="conv-1"): + """Detector backed by a mock driver. budgets defaults to reasoning (the + case the P1 fix targets).""" + driver = MagicMock() + driver._current_conv_id = conv_id + driver._js_strict = AsyncMock(return_value="") + driver._fetch_end_turn_for_turn = AsyncMock(return_value=TurnEndResult(status="not_ready")) + driver._get_live_conversation_id_best_effort = AsyncMock(return_value=conv_id or "") + detector = CompletionDetector(driver) + if budgets is not None: + detector._budgets_override = budgets + return detector, driver + + +def _phase2_poll_payload(*, text="", md_text="", is_thinking=False, + has_action=False, html_len=0, child_count=0): + """Build the JSON the phase-2 poll JS returns.""" + return json.dumps({ + "text": text, "md_text": md_text, "html_len": html_len, + "child_count": child_count, "has_action": has_action, + "is_thinking": is_thinking, + }) + + +class _ScriptedPoll: + """A scripted _js_strict that returns pre-set payloads per phase. + + - body scan → always empty (no rate limit) + - assistant count → always "1" (exceeds initial_count=0, exits phase-1) + - phase-2 poll → call next() from a user-provided sequence + """ + + def __init__(self, poll_sequence): + self.scan = '{"text": ""}' + self.polls = list(poll_sequence) + self._idx = 0 + + async def __call__(self, expr): + if "getBoundingClientRect" in expr: + if self._idx < len(self.polls): + return self.polls[self._idx] + return self.polls[-1] if self.polls else _phase2_poll_payload() + if "innerText" in expr: + return self.scan + return "1" + + +# ── 1. Reasoning first-content does NOT fail at 90s ───────────────────── + + +@pytest.mark.asyncio +async def test_reasoning_first_content_survives_past_90s(monkeypatch): + """The core P1 fix: a reasoning model thinking silently for >90s must NOT + be falsely aborted. The reasoning first-content budget (300s default) keeps + the detector alive. With the old uniform 90s, this would have raised.""" + budgets = DetectorBudgets.reasoning() # first_content=300, stream_idle=120 + detector, driver = _make_detector(budgets=budgets) + + # Simulate: thinking indicator present, no text, for 100 polls (50s of + # "thinking" — well past the old 90s ceiling IF the clock were real, but + # we control the clock). We just need to confirm no stall raises at the + # old threshold. Use a fast-forward clock. + thinking_payload = _phase2_poll_payload(is_thinking=True) + script = _ScriptedPoll([thinking_payload]) + driver._js_strict = script + + # Fast-forward clock: control time.monotonic so 100s of "thinking" passes + # in real-time milliseconds. This proves the 90s boundary is gone for + # reasoning first-content. + t = [0.0] + original_sleep = asyncio.sleep + + async def fast_sleep(d): + t[0] += d # each 0.5s poll advances simulated time + await original_sleep(0) # don't actually wait + + monkeypatch.setattr(time, "monotonic", lambda: t[0]) + monkeypatch.setattr(asyncio, "sleep", fast_sleep) + + # timeout is the overall wall-clock — set high enough that it doesn't fire + # before the first-content budget. + async def drain(): + async for _ in detector.stream_until_complete( + initial_count=0, timeout=500, + turn_anchor=TurnAnchor(sent_text="test", mode="fresh_chat"), + budgets=budgets, + ): + pass + + # Run for a bounded number of events then cancel — we just need to confirm + # it does NOT raise within the first 100s of simulated thinking. Each poll + # advances simulated time by 0.5s; let it run ~200 polls (100s simulated) + # via real-time yielding (no actual sleeping — fast_sleep does original_sleep(0)). + task = asyncio.create_task(drain()) + # Let the task run enough event-loop ticks to advance simulated time past 90s. + # Each poll = one sleep(0) yield, so we need ~200 polls = ~200 iterations. + for _ in range(300): + await original_sleep(0) + if t[0] > 100: + break + # If the detector raised, task.exception() will hold it. + exc = task.exception() if task.done() else None + task.cancel() + try: + await task + except (asyncio.CancelledError, Exception): + pass + if exc and "GenerationStuckError" in type(exc).__name__: + pytest.fail(f"Reasoning model falsely aborted during thinking phase: {exc}") + + # Confirm simulated time advanced past the old 90s ceiling without abort + assert t[0] > 90, f"Expected simulated time >90s, got {t[0]}" + + +# ── 2. Stream-idle fails with its own (shorter) budget ────────────────── + + +@pytest.mark.asyncio +async def test_stream_idle_uses_shorter_budget_after_first_content(monkeypatch): + """Once text has appeared and then stopped progressing, the stream-idle + budget applies — which is shorter than first-content for reasoning models. + Verify the stall_kind is 'stream_idle_timeout', not 'first_content_timeout'.""" + from chatgpt_web2api.cdp_driver import GenerationStuckError + + budgets = DetectorBudgets( + first_content_timeout_seconds=300, # long + stream_idle_timeout_seconds=5, # short — will fire fast + hard_timeout_seconds=900, + ) + detector, driver = _make_detector(budgets=budgets) + + # First poll: text appears (exits first-content state). Then: text stops. + polls = [ + _phase2_poll_payload(text="Hello", md_text="Hello"), + _phase2_poll_payload(text="Hello", md_text="Hello"), # no progress + ] * 50 # repeat to keep polling + script = _ScriptedPoll(polls) + driver._js_strict = script + driver._fetch_end_turn_for_turn = AsyncMock( + return_value=TurnEndResult(status="not_ready") + ) + + t = [0.0] + original_sleep = asyncio.sleep + + async def fast_sleep(d): + t[0] += d + await original_sleep(0) + + monkeypatch.setattr(time, "monotonic", lambda: t[0]) + monkeypatch.setattr(asyncio, "sleep", fast_sleep) + + with pytest.raises(GenerationStuckError) as exc_info: + async for _ in detector.stream_until_complete( + initial_count=0, timeout=500, + turn_anchor=TurnAnchor(sent_text="test", mode="fresh_chat"), + budgets=budgets, + ): + pass + + # The stall should be classified as stream_idle (text appeared then stopped) + assert exc_info.value.phase == "phase_2_stream" + assert getattr(exc_info.value, "stall_kind", None) == "stream_idle_timeout", ( + f"Expected stall_kind='stream_idle_timeout', got {getattr(exc_info.value, 'stall_kind', None)}" + ) + + +# ── 3. Hard cap wins over active DOM signal ───────────────────────────── + + +@pytest.mark.asyncio +async def test_hard_cap_wins_over_dom_liveness(monkeypatch): + """Even if the DOM shows a thinking/generating indicator forever, the hard + cap must eventually fire. No infinite waits.""" + from chatgpt_web2api.cdp_driver import GenerationStuckError + + budgets = DetectorBudgets( + first_content_timeout_seconds=300, + stream_idle_timeout_seconds=300, + hard_timeout_seconds=10, # very short hard cap + ) + detector, driver = _make_detector(budgets=budgets) + + # DOM shows thinking indicator forever, never produces text + script = _ScriptedPoll([_phase2_poll_payload(is_thinking=True)]) + driver._js_strict = script + + t = [0.0] + original_sleep = asyncio.sleep + + async def fast_sleep(d): + t[0] += d + await original_sleep(0) + + monkeypatch.setattr(time, "monotonic", lambda: t[0]) + monkeypatch.setattr(asyncio, "sleep", fast_sleep) + + with pytest.raises(GenerationStuckError): + async for _ in detector.stream_until_complete( + initial_count=0, timeout=500, + turn_anchor=TurnAnchor(sent_text="test", mode="fresh_chat"), + budgets=budgets, + ): + pass + + # The hard cap (10s simulated) should have fired, not the first-content (300s) + assert t[0] >= 10, f"Hard cap should have fired around 10s, got t={t[0]}" + assert t[0] < 300, "first-content budget should NOT have been the one to fire" + + +# ── 4. Final reconciliation success returns normally ──────────────────── + + +@pytest.mark.asyncio +async def test_final_reconciliation_success_returns_normally(monkeypatch): + """When the phase-2 stall fires but final reconciliation finds the turn DID + complete in the backend, the detector returns normally instead of raising. + This is the field-verified case: 'generation completed after detector gave up.'""" + budgets = DetectorBudgets( + first_content_timeout_seconds=5, # short to trigger stall fast + stream_idle_timeout_seconds=5, + hard_timeout_seconds=900, + ) + detector, driver = _make_detector(budgets=budgets) + + # Phase-2: no text, no thinking indicator → stall fires quickly. + script = _ScriptedPoll([_phase2_poll_payload()]) + driver._js_strict = script + + t = [0.0] + original_sleep = asyncio.sleep + + async def fast_sleep(d): + t[0] += d + await original_sleep(0) + + monkeypatch.setattr(time, "monotonic", lambda: t[0]) + monkeypatch.setattr(asyncio, "sleep", fast_sleep) + + # Final reconciliation: backend says the turn completed + driver._fetch_end_turn_for_turn = AsyncMock( + return_value=TurnEndResult(status="matched") + ) + + chunks = [] + # This should NOT raise — reconciliation finds the completed turn. + async for chunk in detector.stream_until_complete( + initial_count=0, timeout=500, + turn_anchor=TurnAnchor(sent_text="test", mode="fresh_chat"), + budgets=budgets, + ): + chunks.append(chunk) + + # No exception — the detector returned normally after reconciliation. + # (If we got here, the test passes. If reconciliation had failed, a + # GenerationStuckError would have been raised.) + + +# ── 5. Structured error fields ────────────────────────────────────────── + + +@pytest.mark.asyncio +async def test_structured_stall_error_has_diagnostic_fields(monkeypatch): + """Phase-2 stall errors carry structured fields for observability: + stall_kind, model_class, elapsed_seconds, generation_active_signal.""" + from chatgpt_web2api.cdp_driver import GenerationStuckError + + budgets = DetectorBudgets( + first_content_timeout_seconds=5, + stream_idle_timeout_seconds=5, + hard_timeout_seconds=900, + ) + detector, driver = _make_detector(budgets=budgets) + + # No text, no thinking, no progress → first_content_timeout stall + script = _ScriptedPoll([_phase2_poll_payload()]) + driver._js_strict = script + driver._fetch_end_turn_for_turn = AsyncMock( + return_value=TurnEndResult(status="not_ready") + ) + + t = [0.0] + original_sleep = asyncio.sleep + + async def fast_sleep(d): + t[0] += d + await original_sleep(0) + + monkeypatch.setattr(time, "monotonic", lambda: t[0]) + monkeypatch.setattr(asyncio, "sleep", fast_sleep) + + with pytest.raises(GenerationStuckError) as exc_info: + async for _ in detector.stream_until_complete( + initial_count=0, timeout=500, + turn_anchor=TurnAnchor(sent_text="test", mode="fresh_chat"), + budgets=budgets, + model="gpt-5-5-thinking", + ): + pass + + err = exc_info.value + assert err.phase == "phase_2_stream" + assert getattr(err, "stall_kind", None) == "first_content_timeout" + assert getattr(err, "model_class", None) == "reasoning" + assert hasattr(err, "elapsed_seconds"), "error must carry elapsed_seconds" + assert hasattr(err, "generation_active_signal"), "error must carry generation_active_signal" diff --git a/tests/test_integration.py b/tests/test_integration.py index a7d09e0f..bbb9acaf 100644 --- a/tests/test_integration.py +++ b/tests/test_integration.py @@ -43,7 +43,7 @@ def make_mock_driver(): driver.is_connected = True driver._access_token = "test-token" - async def _stream(text, timeout=120): + async def _stream(text, timeout=120, *, budgets=None, model=None): yield StreamChunk(delta="Mocked ChatGPT response") yield StreamChunk(delta="", finish_reason="stop") diff --git a/tests/test_mcp_rate_limit.py b/tests/test_mcp_rate_limit.py index c9f54e35..9bded637 100644 --- a/tests/test_mcp_rate_limit.py +++ b/tests/test_mcp_rate_limit.py @@ -33,7 +33,7 @@ def _make_server_with_raising_driver(raises: Exception | None): driver._current_conv_id = "" driver._current_model = None - async def _stream(text, timeout=120): + async def _stream(text, timeout=120, *, budgets=None, model=None): if raises is not None: raise raises from chatgpt_web2api.cdp_driver import StreamChunk @@ -110,7 +110,7 @@ async def _noop(_s): return None attempts = {"n": 0} - async def _stream(text, timeout=120): + async def _stream(text, timeout=120, *, budgets=None, model=None): attempts["n"] += 1 if attempts["n"] == 1: raise RateLimitError(retry_after=1) diff --git a/tests/test_progress.py b/tests/test_progress.py index 57e41285..3d39020f 100644 --- a/tests/test_progress.py +++ b/tests/test_progress.py @@ -27,7 +27,7 @@ def _streaming_driver(deltas): driver.navigate_conversation = AsyncMock() driver.navigate_gpt = AsyncMock() - async def _stream(text, timeout=120): + async def _stream(text, timeout=120, *, budgets=None, model=None): for d in deltas: yield StreamChunk(delta=d) yield StreamChunk(delta="", finish_reason="stop") diff --git a/tests/test_reliability.py b/tests/test_reliability.py index 3552d9d7..51631b74 100644 --- a/tests/test_reliability.py +++ b/tests/test_reliability.py @@ -765,7 +765,7 @@ async def test_mcp_auth_expired_returns_error_result(): drv.navigate_new_chat = AsyncMock() # send_and_stream must be an async GENERATOR that raises on iteration. - async def _raising_stream(text, timeout=120): + async def _raising_stream(text, timeout=120, *, budgets=None, model=None): raise AuthExpiredError() yield # unreachable, makes this a generator From 6eaf7968ae7d86955760c55215b19a1c0001bfe3 Mon Sep 17 00:00:00 2001 From: Alajmah <55954113+Alajmah@users.noreply.github.com> Date: Thu, 9 Jul 2026 00:51:45 +0300 Subject: [PATCH 03/25] =?UTF-8?q?fix(detector):=20P1=20review=20fixes=20?= =?UTF-8?q?=E2=80=94=20state=20transition,=20auth-expiry,=20markers?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three fixes from ChatGPT code review (conversation 6a4ec417): A. State transition bug (merge-blocker): first_content_seen was set on ANY html_len/child_count change, but the first phase-2 poll almost always sees the assistant message wrapper (nonzero html_len). This would prematurely transition a reasoning model from the 300s first-content budget to the 120s stream-idle budget while still thinking — exactly the failure P1 was designed to fix. Fix: require html_len > 50 (the existing meaningful-content threshold) for non-text first-content. Also reset last_change_time when the state flips, so a 250s reasoning wait followed by first text doesn't immediately fail under the shorter stream-idle budget. C. Auth-expiry swallowed (merge-blocker): _reconcile_before_stall caught Exception broadly, including AuthExpiredError — turning 're-login required' into 'generation stalled.' Fix: re-raise AuthExpiredError before the broad except (preserves PR #39 invariant: auth failure never degrades). Non-blocking improvements adopted: - _REASONING_MARKERS expanded: added 'reasoning', 'o1', 'o4' (inclusive on purpose — false negative worse than false positive). - turn_id verified: getattr(turn_anchor, 'captured_id', None) returns the UUID when available, None when capture failed (already correct). 654 tests pass. generation_active_signal left at bool(is_thinking) — safe per review; broader stop-button signal is a future diagnostic enhancement, not a P1 blocker. --- src/chatgpt_web2api/completion_detector.py | 40 ++++++++++++++++------ 1 file changed, 29 insertions(+), 11 deletions(-) diff --git a/src/chatgpt_web2api/completion_detector.py b/src/chatgpt_web2api/completion_detector.py index b0b92b13..c5a47d8f 100644 --- a/src/chatgpt_web2api/completion_detector.py +++ b/src/chatgpt_web2api/completion_detector.py @@ -116,9 +116,12 @@ # silent "thinking" phase before the first answer token renders, which the # old uniform 90s stall window falsely aborted. Matched case-insensitively # against the slug via substring. "thinking" covers gpt-5-*-thinking and -# gpt-5-*-t-mini (Thinking Mini); "o3" and "research" are the other -# reasoning-class families. -_REASONING_MARKERS = ("thinking", "-t-mini", "o3", "research") +# gpt-5-*-t-mini (Thinking Mini); "o1"/"o3"/"o4" are the o-series reasoning +# families; "research" is Deep Research; "reasoning" is a catch-all in case +# OpenAI introduces slugs that name it directly. Inclusive on purpose — a +# false negative (reasoning model gets the shorter default budget) is worse +# than a false positive (non-reasoning model gets the longer budget). +_REASONING_MARKERS = ("thinking", "-t-mini", "o1", "o3", "o4", "research", "reasoning") def classify_model(model: str | None) -> str: @@ -256,8 +259,11 @@ async def _reconcile_before_stall( turn-anchoring work to correlate against the correct turn. On any fetch failure or exception, returns False (let the stall raise) - rather than degrading to a silent success. + rather than degrading to a silent success. EXCEPT auth expiry: that + must surface as auth expiry, not degrade to a generic stall (PR #39 + review finding #2 invariant — auth failure never degrades). """ + from .cdp_driver import AuthExpiredError from .turn_anchor import collapse_to_end_turn_status if not conv_id: @@ -269,6 +275,8 @@ async def _reconcile_before_stall( ) status = collapse_to_end_turn_status(end_result) return status == "complete" + except AuthExpiredError: + raise # never swallow auth expiry — it must surface as auth expiry except Exception as e: logger.debug("Final reconciliation fetch failed: %s", e) return False @@ -589,18 +597,22 @@ async def stream_until_complete( if not conv_id_for_check: last_change_time = time.monotonic() # P1: track advisory liveness signal for structured error reporting. - # True when the DOM shows active generation (thinking indicator or - # a generating/stop-button state). This is advisory only — it - # informs the structured error and logging, but does NOT pause the - # stall clock (a stuck indicator must not create an infinite hang). + # True when the DOM shows active generation (thinking indicator). + # This is advisory only — it informs the structured error and + # logging, but does NOT pause the stall clock (a stuck indicator + # must not create an infinite hang). generation_active_signal = bool(is_thinking) if current != last_dom_text: last_change_time = time.monotonic() # P1: first text content transitions us from awaiting_first_content # to streaming_after_first_content. The stall budget changes with - # the state (see the stall check below). + # the state (see the stall check below). When the state flips, + # reset the stream-idle clock so a long reasoning wait followed + # by first text doesn't immediately fail under the shorter + # stream-idle budget (review finding A). if current and not first_content_seen: first_content_seen = True + last_change_time = time.monotonic() # reset stream-idle clock if len(current) > len(last_dom_text): delta = current[len(last_dom_text) :] yield StreamChunk(delta=delta) @@ -613,9 +625,15 @@ async def stream_until_complete( if html_len > 50: had_non_text_content = True self.had_non_text_content = True - # P1: non-text content also counts as first-content (images/tool-use). - if not first_content_seen: + # P1: meaningful non-text content (html_len > 50, the existing + # threshold that excludes the bare message wrapper) also counts + # as first-content. Do NOT transition on the first poll's + # wrapper creation alone — that would prematurely move a + # reasoning model from the 300s first-content budget to the + # 120s stream-idle budget while still thinking (review finding A). + if html_len > 50 and not first_content_seen: first_content_seen = True + last_change_time = time.monotonic() # reset stream-idle clock last_html_len = html_len last_child_count = child_count From abb5d14cf5d998234d51fc9430d572f7a3849353 Mon Sep 17 00:00:00 2001 From: Alajmah <55954113+Alajmah@users.noreply.github.com> Date: Thu, 9 Jul 2026 02:29:23 +0300 Subject: [PATCH 04/25] feat(pool): P1.5 lease accounting instrumentation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Purely additive diagnostics — no behavioral change to pool logic. Adds per-lease lifecycle tracking so the next field session and the pool-exhaustion RCA have attributable data. Co-designed with ChatGPT (conversation 6a4ed7bf, 2026-07-09). What was added: - LeaseRecord dataclass: lease_id, session_key, acquired_at, released_at, hold_duration_s, release_reason (normal/exception/cancellation). - _make_lease() helper: creates a DriverLease + its LeaseRecord at acquire time, stamps the lease with a lease_id for correlation. - _release() now completes the record (duration + reason) and detects double-releases (logs warning, does NOT re-decrement in_flight). - _LeaseContext.__aexit__ classifies release reason from exc_type (normal/cancellation/exception). - PoolExhaustedError now includes active session keys in its message ('active: session-1, session-2') so exhaustion errors name the sessions holding slots. - status() includes a 'leases' summary: active_count, history_count, active lease details, and the 10 most recent completed leases with hold_duration_s + release_reason. - _active_session_keys() helper for exhaustion dumps. - _lease_history capped at 100 records (no unbounded growth). Answers these RCA questions (the evidence generator): - How long was each lease held? - Why was it released (normal/exception/cancellation)? - What leases were active when exhaustion hit? - Are there double-releases or leaked leases? 660 tests pass (654 existing + 6 new P1.5 tests). --- src/chatgpt_web2api/mcp_driver_pool.py | 147 +++++++++++++++++---- tests/test_pool_lease_accounting.py | 169 +++++++++++++++++++++++++ 2 files changed, 293 insertions(+), 23 deletions(-) create mode 100644 tests/test_pool_lease_accounting.py diff --git a/src/chatgpt_web2api/mcp_driver_pool.py b/src/chatgpt_web2api/mcp_driver_pool.py index 37b8afcc..c0a2e31d 100644 --- a/src/chatgpt_web2api/mcp_driver_pool.py +++ b/src/chatgpt_web2api/mcp_driver_pool.py @@ -21,8 +21,9 @@ import asyncio import logging import time +import uuid from collections.abc import Awaitable, Callable -from dataclasses import dataclass +from dataclasses import dataclass, field from typing import Any logger = logging.getLogger(__name__) @@ -39,11 +40,16 @@ class PoolExhaustedError(RuntimeError): sites below now render as ``"PoolExhaustedError: no driver slot available within acquire_timeout. Retry later."`` rather than ``": . Retry later."``. + + P1.5: carries an optional ``active_leases`` dump so the error message + names the sessions holding slots at exhaustion time. """ - def __init__(self, message: str | None = None) -> None: + def __init__(self, message: str | None = None, active_leases: list[str] | None = None) -> None: if message is None: message = "no driver slot available within acquire_timeout" + if active_leases: + message = f"{message} (active: {', '.join(active_leases)})" super().__init__(message) @@ -118,6 +124,26 @@ class DriverSlot: closing: bool = False +# ── P1.5: Lease accounting records ──────────────────────────────────────── + +@dataclass +class LeaseRecord: + """P1.5: per-lease lifecycle record for diagnostics. + + Created at acquire time, completed at release time with hold duration + and release reason. Stored in the pool's ``_lease_history`` (capped) + for post-hoc RCA. This is pure instrumentation — it does NOT influence + pool behavior (capacity, eviction, blocking). + """ + lease_id: str + session_key: str + acquired_at: float + released_at: float | None = None + hold_duration_s: float | None = None + release_reason: str | None = None # "normal" | "exception" | "cancellation" + + + # ── Lease ───────────────────────────────────────────────────────────────── @dataclass(frozen=True) @@ -127,6 +153,7 @@ class DriverLease: driver: Any # CDPDriver breakers: Any # BreakerRegistry call_lock: asyncio.Lock + lease_id: str = "" # P1.5: correlates to a LeaseRecord for diagnostics class _LeaseContext: @@ -148,7 +175,14 @@ async def __aenter__(self) -> DriverLease: async def __aexit__(self, exc_type: Any, exc: Any, tb: Any) -> None: if self._lease is None: return - await self._pool._release(self._lease.slot) + # P1.5: classify the release reason for lease accounting. + if exc_type is None: + reason = "normal" + elif issubclass(exc_type, asyncio.CancelledError): + reason = "cancellation" + else: + reason = "exception" + await self._pool._release(self._lease.slot, lease_id=self._lease.lease_id, release_reason=reason) self._lease = None @@ -192,6 +226,22 @@ def __init__( cfg.mcp_account_throttle_cooldown_seconds ) self._sweep_task: asyncio.Task | None = None + # P1.5: lease accounting. _active_leases maps lease_id → LeaseRecord + # for currently-held leases; _lease_history is a capped list of + # completed records. Pure diagnostics — no behavioral influence. + self._active_leases: dict[str, LeaseRecord] = {} + self._lease_history: list[LeaseRecord] = [] + self._history_cap = 100 # prevent unbounded growth + + @property + def active_leases(self) -> dict[str, LeaseRecord]: + """Currently-held leases (lease_id → LeaseRecord).""" + return self._active_leases + + @property + def lease_history(self) -> list[LeaseRecord]: + """Completed lease records (most recent last, capped at _history_cap).""" + return self._lease_history @property def account_breaker(self) -> AccountThrottleBreaker: @@ -311,12 +361,7 @@ async def _acquire_slot(self, session_key: str) -> DriverLease: else: slot.in_flight += 1 slot.last_used_at = time.monotonic() - return DriverLease( - slot=slot, - driver=slot.driver, - breakers=slot.breakers, - call_lock=slot.call_lock, - ) + return self._make_lease(slot) # Need a new slot. if pending_slot is None and ( @@ -343,7 +388,7 @@ async def _acquire_slot(self, session_key: str) -> DriverLease: remaining = deadline - time.monotonic() if remaining <= 0: - raise PoolExhaustedError() + raise PoolExhaustedError(active_leases=self._active_session_keys()) try: await asyncio.wait_for( @@ -356,7 +401,7 @@ async def _acquire_slot(self, session_key: str) -> DriverLease: timeout=remaining, ) except TimeoutError: - raise PoolExhaustedError() + raise PoolExhaustedError(active_leases=self._active_session_keys()) if self._shutting_down: raise PoolShuttingDownError() @@ -366,13 +411,13 @@ async def _acquire_slot(self, session_key: str) -> DriverLease: if pending_slot is not None: remaining = deadline - time.monotonic() if remaining <= 0: - raise PoolExhaustedError() + raise PoolExhaustedError(active_leases=self._active_session_keys()) try: await asyncio.wait_for( pending_slot.ready_event.wait(), timeout=remaining ) except TimeoutError: - raise PoolExhaustedError() + raise PoolExhaustedError(active_leases=self._active_session_keys()) continue assert materialize_slot is not None @@ -383,7 +428,7 @@ async def _acquire_slot(self, session_key: str) -> DriverLease: remaining = deadline - time.monotonic() if remaining <= 0: await self._abandon_pending_slot(materialize_slot) - raise PoolExhaustedError() + raise PoolExhaustedError(active_leases=self._active_session_keys()) try: await asyncio.wait_for( @@ -392,7 +437,7 @@ async def _acquire_slot(self, session_key: str) -> DriverLease: sem_acquired = True except TimeoutError: await self._abandon_pending_slot(materialize_slot) - raise PoolExhaustedError() + raise PoolExhaustedError(active_leases=self._active_session_keys()) await self._materialize_slot(materialize_slot) @@ -432,20 +477,60 @@ async def _acquire_slot(self, session_key: str) -> DriverLease: async with materialize_slot.meta_lock: if materialize_slot.driver is None: raise PoolSlotUnavailableError() - return DriverLease( - slot=materialize_slot, - driver=materialize_slot.driver, - breakers=materialize_slot.breakers, - call_lock=materialize_slot.call_lock, - ) + return self._make_lease(materialize_slot) def _make_breakers(self): """Create a fresh BreakerRegistry for a slot.""" from .breakers import BreakerRegistry return BreakerRegistry() - async def _release(self, slot: DriverSlot) -> None: - """Release a lease: decrement in_flight, update last_used_at.""" + def _make_lease(self, slot: DriverSlot) -> DriverLease: + """P1.5: create a DriverLease + its LeaseRecord. Instruments the lease + lifecycle for diagnostics (hold duration, release reason).""" + lease_id = uuid.uuid4().hex[:12] + record = LeaseRecord( + lease_id=lease_id, + session_key=slot.session_key, + acquired_at=time.monotonic(), + ) + self._active_leases[lease_id] = record + return DriverLease( + slot=slot, + driver=slot.driver, + breakers=slot.breakers, + call_lock=slot.call_lock, + lease_id=lease_id, + ) + + def _active_session_keys(self) -> list[str]: + """P1.5: session keys of currently-held leases (for exhaustion dumps).""" + return sorted({r.session_key for r in self._active_leases.values()}) + + async def _release(self, slot: DriverSlot, *, lease_id: str = "", release_reason: str = "normal") -> None: + """Release a lease: decrement in_flight, update last_used_at. + + P1.5: also completes the LeaseRecord (hold duration + reason) and + detects double-releases (a correctness bug where the same lease is + released twice). Double-release logs a warning but does NOT re-decrement + in_flight (that would undercount). + """ + # P1.5: complete the lease record if we have its ID. + if lease_id: + record = self._active_leases.pop(lease_id, None) + if record is not None: + record.released_at = time.monotonic() + record.hold_duration_s = record.released_at - record.acquired_at + record.release_reason = release_reason + self._lease_history.append(record) + # Cap history to prevent unbounded growth. + if len(self._lease_history) > self._history_cap: + self._lease_history = self._lease_history[-self._history_cap:] + else: + logger.warning( + "Double-release detected: lease_id=%s not in active_leases " + "(session=%s) — lease was already released or never tracked", + lease_id, slot.session_key, + ) async with slot.meta_lock: slot.in_flight = max(0, slot.in_flight - 1) slot.last_used_at = time.monotonic() @@ -593,4 +678,20 @@ def status(self) -> dict: }, "account_breaker_tripped": self._account_breaker.is_tripped(), "shutting_down": self._shutting_down, + # P1.5: lease accounting summary for RCA / diagnostics. + "leases": { + "active_count": len(self._active_leases), + "history_count": len(self._lease_history), + "active": [ + {"lease_id": r.lease_id, "session_key": r.session_key, + "acquired_at": r.acquired_at} + for r in self._active_leases.values() + ], + "recent": [ + {"lease_id": r.lease_id, "session_key": r.session_key, + "hold_duration_s": r.hold_duration_s, + "release_reason": r.release_reason} + for r in self._lease_history[-10:] + ], + }, } diff --git a/tests/test_pool_lease_accounting.py b/tests/test_pool_lease_accounting.py new file mode 100644 index 00000000..79b7d539 --- /dev/null +++ b/tests/test_pool_lease_accounting.py @@ -0,0 +1,169 @@ +"""P1.5: Pool lease accounting instrumentation. + +Purely additive diagnostics — no behavioral change to pool logic. Tracks +each lease's lifecycle (acquire → release) so the next field session and the +pool-exhaustion RCA have attributable data. + +Verifies: + 1. Lease acquire creates a record with lease_id, session_key, timestamp. + 2. Lease release records duration + release_reason (normal/exception/cancel). + 3. PoolExhaustedError includes an active-lease dump. + 4. Double-release is detected and logged (not silently swallowed). + 5. status() includes lease accounting summary. +""" + +import asyncio +import logging +import time +from unittest.mock import AsyncMock, MagicMock + +import pytest + +from chatgpt_web2api.config import Config +from chatgpt_web2api.mcp_driver_pool import ( + McpSessionDriverPool, + PoolExhaustedError, +) + + +def _make_pool(max_size=2, **overrides): + """Build a pool with a fake driver factory for fast tests.""" + cfg = Config() + cfg.chatgpt.mcp_session_pool_size = max_size + cfg.chatgpt.mcp_session_pool_acquire_timeout = 2.0 + for k, v in overrides.items(): + setattr(cfg.chatgpt, k, v) + + async def fake_factory(config, transport, port, slot): + driver = MagicMock() + driver.close = AsyncMock() + driver._current_conv_id = None + return driver + + pool = McpSessionDriverPool(cfg, driver_factory=fake_factory) + return pool + + +# ── 1. Lease acquire creates a record ──────────────────────────────────── + + +@pytest.mark.asyncio +async def test_acquire_creates_lease_record(): + """Acquiring a lease creates a record with lease_id, session_key, timestamp.""" + pool = _make_pool(max_size=2) + try: + async with pool.acquire("session-1") as lease: + # The pool should have exactly one active lease record. + assert len(pool.active_leases) == 1, ( + f"expected 1 active lease, got {len(pool.active_leases)}" + ) + record = list(pool.active_leases.values())[0] + assert record.session_key == "session-1" + assert record.lease_id # non-empty ID + assert record.acquired_at > 0 + finally: + await pool.close_all() + + +# ── 2. Lease release records duration + reason ────────────────────────── + + +@pytest.mark.asyncio +async def test_release_records_duration_and_normal_reason(): + """On normal release, the record has released_at set and reason='normal'.""" + pool = _make_pool() + try: + async with pool.acquire("session-1"): + pass # normal exit + # Lease was released; active_leases should be empty. + assert len(pool.active_leases) == 0 + # History should have the released record. + assert len(pool.lease_history) >= 1 + rec = pool.lease_history[-1] + assert rec.released_at is not None + assert rec.release_reason == "normal" + assert rec.hold_duration_s is not None + assert rec.hold_duration_s >= 0 + finally: + await pool.close_all() + + +@pytest.mark.asyncio +async def test_release_records_exception_reason(): + """On exception exit, the release reason is 'exception', not 'normal'.""" + pool = _make_pool() + try: + with pytest.raises(ValueError): + async with pool.acquire("session-1"): + raise ValueError("test error") + rec = pool.lease_history[-1] + assert rec.release_reason == "exception" + finally: + await pool.close_all() + + +# ── 3. PoolExhaustedError includes active-lease dump ──────────────────── + + +@pytest.mark.asyncio +async def test_exhaustion_includes_active_lease_dump(): + """When the pool is exhausted, the error message includes info about + what leases are currently active (session keys, hold durations, in_flight).""" + pool = _make_pool(max_size=1) + try: + # Hold one lease to fill the pool. + async with pool.acquire("session-1"): + # Try to acquire a second — should exhaust. + with pytest.raises(PoolExhaustedError) as exc_info: + async with pool.acquire("session-2"): + pass + msg = str(exc_info.value) + # The error should mention the active session holding the slot. + assert "session-1" in msg, ( + f"exhaustion error should include active lease dump, got: {msg}" + ) + finally: + await pool.close_all() + + +# ── 4. Double-release detection ────────────────────────────────────────── + + +@pytest.mark.asyncio +async def test_double_release_detected(caplog): + """Releasing a lease that's already released logs a warning (double-release bug).""" + pool = _make_pool() + try: + lease_ctx = pool.acquire("session-1") + async with lease_ctx as lease: + pass # normal release + # Manually call _release again on the same slot with the same lease_id + # (simulates a double-release bug where the lease is released twice). + with caplog.at_level(logging.WARNING): + await pool._release(lease.slot, lease_id=lease.lease_id, release_reason="normal") + assert any("double" in r.message.lower() or "already released" in r.message.lower() + for r in caplog.records), ( + "double-release should log a warning" + ) + finally: + await pool.close_all() + + +# ── 5. status() includes lease summary ─────────────────────────────────── + + +@pytest.mark.asyncio +async def test_status_includes_lease_accounting(): + """status() should include lease counts (active + history).""" + pool = _make_pool() + try: + async with pool.acquire("session-1"): + pass + s = pool.status() + assert "leases" in s, "status() should include lease accounting" + lease_info = s["leases"] + assert "active_count" in lease_info + assert "history_count" in lease_info + assert lease_info["history_count"] >= 1 + finally: + await pool.close_all() From 470d5f3dd43070e152bfb08c52b9f669fecb4df7 Mon Sep 17 00:00:00 2001 From: Alajmah <55954113+Alajmah@users.noreply.github.com> Date: Thu, 9 Jul 2026 02:40:38 +0300 Subject: [PATCH 05/25] =?UTF-8?q?fix(pool):=20P1.5=20review=20fix=20?= =?UTF-8?q?=E2=80=94=20double-release=20skip=20+=20legacy=20caller=20safet?= =?UTF-8?q?y?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ChatGPT review (conversation 6a4ed7bf) found one merge condition: a confirmed double-release of a tracked lease would still decrement in_flight (causing undercount). Fix: skip the decrement for confirmed double-releases (lease_id provided but not in active_leases), while preserving the decrement for legacy untracked calls (lease_id=''). Also adopted minor suggestions: - History cap made a named module constant (_LEASE_HISTORY_LIMIT=100) instead of an instance attribute. - Double-release warning now explicitly mentions skipping the decrement. --- src/chatgpt_web2api/mcp_driver_pool.py | 31 +++++++++++++++++++------- 1 file changed, 23 insertions(+), 8 deletions(-) diff --git a/src/chatgpt_web2api/mcp_driver_pool.py b/src/chatgpt_web2api/mcp_driver_pool.py index c0a2e31d..13cb50ca 100644 --- a/src/chatgpt_web2api/mcp_driver_pool.py +++ b/src/chatgpt_web2api/mcp_driver_pool.py @@ -28,6 +28,9 @@ logger = logging.getLogger(__name__) +# P1.5: cap for lease history records (prevents unbounded growth). +_LEASE_HISTORY_LIMIT = 100 + # ── Errors ──────────────────────────────────────────────────────────────── @@ -231,7 +234,6 @@ def __init__( # completed records. Pure diagnostics — no behavioral influence. self._active_leases: dict[str, LeaseRecord] = {} self._lease_history: list[LeaseRecord] = [] - self._history_cap = 100 # prevent unbounded growth @property def active_leases(self) -> dict[str, LeaseRecord]: @@ -513,8 +515,13 @@ async def _release(self, slot: DriverSlot, *, lease_id: str = "", release_reason detects double-releases (a correctness bug where the same lease is released twice). Double-release logs a warning but does NOT re-decrement in_flight (that would undercount). + + Legacy callers without a ``lease_id`` (``lease_id=""``) skip tracking + but still get the correct in_flight decrement (review finding A). """ - # P1.5: complete the lease record if we have its ID. + # P1.5: lease tracking. For tracked leases (lease_id non-empty), + # complete the record or detect double-release. + skip_decrement = False if lease_id: record = self._active_leases.pop(lease_id, None) if record is not None: @@ -523,17 +530,25 @@ async def _release(self, slot: DriverSlot, *, lease_id: str = "", release_reason record.release_reason = release_reason self._lease_history.append(record) # Cap history to prevent unbounded growth. - if len(self._lease_history) > self._history_cap: - self._lease_history = self._lease_history[-self._history_cap:] + if len(self._lease_history) > _LEASE_HISTORY_LIMIT: + self._lease_history = self._lease_history[-_LEASE_HISTORY_LIMIT:] else: + # Confirmed double-release: lease_id was provided but is not in + # active_leases. Log and do NOT re-decrement in_flight (review + # finding A — re-decrementing would undercount). logger.warning( "Double-release detected: lease_id=%s not in active_leases " - "(session=%s) — lease was already released or never tracked", + "(session=%s) — lease was already released; skipping " + "in_flight decrement to avoid undercount", lease_id, slot.session_key, ) - async with slot.meta_lock: - slot.in_flight = max(0, slot.in_flight - 1) - slot.last_used_at = time.monotonic() + skip_decrement = True + # For untracked releases (lease_id="" — legacy/direct callers) and + # normal tracked releases, decrement in_flight. Double-releases skip. + if not skip_decrement: + async with slot.meta_lock: + slot.in_flight = max(0, slot.in_flight - 1) + slot.last_used_at = time.monotonic() async def _sweep_idle(self) -> None: """Background loop: close idle slots past TTL. From 38ddc013b8630c403a5e5038b7e63ab7a258b4e9 Mon Sep 17 00:00:00 2001 From: Alajmah <55954113+Alajmah@users.noreply.github.com> Date: Thu, 9 Jul 2026 05:26:54 +0300 Subject: [PATCH 06/25] feat(navigation): P2 staged readiness probe + fast-fail on displacement MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replaces the opaque 'did not reach a ready composer' navigation poll with a diagnosable staged probe. Co-designed with ChatGPT (vision-alignment cycle, conversation 6a4ebb2a). Core changes (navigate_conversation): 1. Staged readiness probe: splits the opaque 'ready' check into: url_correct → document_ready → app_shell_present → composer_present All stages evaluated in ONE JS call (no extra round-trips). When the poll fails, the error message names WHICH stage failed: 'Navigation to {id} failed after 15s — stage: composer not present' instead of the old opaque timeout message. 2. NavigationReadinessProbe dataclass: carries per-poll stage results + diagnostic_summary() that names the first failing stage. 3. Fast-fail on URL displacement: if the URL was correct on a prior poll but moves away (SPA redirect, access denied, conversation deleted), raises immediately with 'nav_displaced' instead of burning the full 15s. Detects real redirect states, not just slow loading. 4. document.readyState check: the probe now checks readyState='complete' before testing composer presence. A page still loading waits, doesn't burn stall budget against an incomplete DOM. 5. Switched from _js (soft, silently swallows errors) to _js_strict with a try/except that LOGS transient JS failures. Before, every transient JS failure silently burned a poll iteration with zero diagnostic — now it's logged at debug level so the failure is traceable. Updated test_conversation_guard.py to match the new probe format and error messages. 670 tests pass (660 existing + 10 new P2 navigation tests). --- src/chatgpt_web2api/cdp_driver.py | 146 +++++++++++++++--- tests/test_conversation_guard.py | 21 ++- tests/test_navigation_readiness.py | 240 +++++++++++++++++++++++++++++ 3 files changed, 379 insertions(+), 28 deletions(-) create mode 100644 tests/test_navigation_readiness.py diff --git a/src/chatgpt_web2api/cdp_driver.py b/src/chatgpt_web2api/cdp_driver.py index 567d55b9..225063bd 100644 --- a/src/chatgpt_web2api/cdp_driver.py +++ b/src/chatgpt_web2api/cdp_driver.py @@ -85,6 +85,64 @@ class StreamChunk: ) +# ── P2: Navigation readiness probe ──────────────────────────────────────── +# +# Co-designed with ChatGPT (vision-alignment cycle, conversation 6a4ebb2a). +# Replaces the opaque "ready composer" poll with a staged probe that captures +# WHICH readiness stage passed and which failed. When the poll fails, the +# error message names the stage (e.g. "url correct but composer not present") +# instead of the old "did not reach a ready composer within the timeout". +# +# Stages (each must pass for the next to matter): +# url_correct → document_ready → app_shell_present → composer_present + +@dataclass +class NavigationReadinessProbe: + """Results of a single navigation-readiness probe poll. + + Captured each poll iteration so the caller can build a diagnostic error + message naming the stage that failed. The JS probe evaluates all stages + in one ``Runtime.evaluate`` call (no extra round-trips). + """ + url: str + ready_state: str + app_shell_present: bool + composer_present: bool + + @property + def document_ready(self) -> bool: + return self.ready_state == "complete" + + def is_ready(self, url_correct: bool) -> bool: + """All stages passed — page loaded AND composer ready AND URL matches. + + ``url_correct`` is passed by the caller (it knows the target + conversation_id; the probe doesn't). + """ + return ( + url_correct + and self.document_ready + and self.app_shell_present + and self.composer_present + ) + + def diagnostic_summary(self, url_correct: bool) -> str: + """Human-readable description of which stage failed. + + Names the first failing stage so the error message points at the + real problem instead of an opaque 'timeout'. + """ + if not url_correct: + return f"url displaced (got {self.url[:80]})" + if not self.document_ready: + return f"document still loading (readyState={self.ready_state})" + if not self.app_shell_present: + return "app shell not present (ChatGPT nav/sidebar missing)" + if not self.composer_present: + return "composer not present (selector did not match after page loaded)" + return "all stages passed" + + class RateLimitError(RuntimeError): """Raised when ChatGPT shows its 'Too many requests' rate-limit pop-up. @@ -1198,43 +1256,87 @@ async def navigate_conversation(self, conversation_id: str) -> None: raises — never admits an unverified conversation as current. This is the invariant the auto-continue paths depend on: ``_current_conv_id`` means "the live tab is here", not "we attempted to go here". + + P2 (2026-07-09): the readiness poll is now staged — it probes + url → document.readyState → app shell → composer in one JS call + and captures which stage failed. The error message names the stage + instead of the old opaque "did not reach a ready composer." Also + fast-fails with ``nav_displaced`` if the URL moves away from the + target mid-poll (detects SPA redirects / access-denied states). """ url = f"https://chatgpt.com/c/{conversation_id}" logger.info("Navigate to conversation: %s", url) await self._cdp("Page.navigate", {"url": url}) await asyncio.sleep(3) - # Wait for composer (new ProseMirror div, or legacy textarea) AND a - # verified landing. A for/else means: if the loop completes without - # `break` (never became ready at the right URL), the else runs and we - # fail rather than falling through to admit an unverified conversation. + # P2: staged readiness probe. Evaluates all stages in one JS call + # (no extra round-trips). Uses _js_strict so transient JS failures + # are visible (logged) rather than silently burning poll iterations. + probe_js = ( + "(function() {" + " return JSON.stringify({" + " url: location.href," + " ready_state: document.readyState," + f" app_shell: !!document.querySelector('nav') || !!document.querySelector('[class*=\"sidebar\"]')," + f" composer: !!document.querySelector('{COMPOSER_SELECTOR}') || !!document.querySelector('{COMPOSER_FALLBACK_SELECTOR}')" + " });" + "})()" + ) + + last_probe: NavigationReadinessProbe | None = None + url_was_correct = False # track if URL was ever correct (for displacement) + for _ in range(30): - result = await self._js( - "(function() {" - " return JSON.stringify({" - f" ready: !!document.querySelector('{COMPOSER_SELECTOR}') || !!document.querySelector('{COMPOSER_FALLBACK_SELECTOR}')," - " url: location.href" - " });" - "})()" - ) try: - state = json.loads(result) - except (json.JSONDecodeError, TypeError): - state = {} - if state.get("ready") and self._is_url_at_conversation( - state.get("url", ""), conversation_id - ): - logger.info("Conversation ready: %s", state.get("url", "")) + result = await self._js_strict(probe_js) + data = json.loads(result) + except Exception as e: + # P2: log transient JS failures instead of silently swallowing. + logger.debug("Navigation probe JS failed (will retry): %s", e) + await asyncio.sleep(0.5) + continue + + probe = NavigationReadinessProbe( + url=data.get("url", ""), + ready_state=data.get("ready_state", ""), + app_shell_present=bool(data.get("app_shell")), + composer_present=bool(data.get("composer")), + ) + last_probe = probe + url_correct = self._is_url_at_conversation(probe.url, conversation_id) + + # P2: fast-fail on URL displacement. If the URL was correct on a + # prior poll but is now wrong, the page navigated away (SPA redirect, + # access denied, conversation deleted). Don't burn the full 15s. + if url_correct: + url_was_correct = True + elif url_was_correct: + if self._current_conv_id == conversation_id: + self._current_conv_id = None + raise RuntimeError( + f"Navigation to {conversation_id} displaced — URL moved " + f"to {probe.url[:80]} after initially loading (nav_displaced)" + ) + + if probe.is_ready(url_correct): + logger.info("Conversation ready: %s", probe.url) break await asyncio.sleep(0.5) else: # Loop exhausted without a verified landing. Clear any stale - # state that might point here so a later auto-continue can't - # reuse a known-unverified id, then surface the failure. + # state and raise with P2 staged diagnostics. if self._current_conv_id == conversation_id: self._current_conv_id = None + if last_probe is not None: + url_correct = self._is_url_at_conversation(last_probe.url, conversation_id) + stage = last_probe.diagnostic_summary(url_correct) + raise RuntimeError( + f"Navigation to {conversation_id} failed after 15s — " + f"stage: {stage}" + ) raise RuntimeError( - f"Navigation to {conversation_id} did not reach a ready composer within the timeout" + f"Navigation to {conversation_id} failed — all probes errored " + f"(no readiness data obtained)" ) await asyncio.sleep(1) diff --git a/tests/test_conversation_guard.py b/tests/test_conversation_guard.py index 623f0433..8dcbf176 100644 --- a/tests/test_conversation_guard.py +++ b/tests/test_conversation_guard.py @@ -196,10 +196,13 @@ async def test_navigate_conversation_sets_id_only_on_verified_landing(): d = CDPDriver(cdp_port=9222) cid = "abc-123" d._cdp = AsyncMock() # Page.navigate - # _js returns a ready state at the right URL on first poll. - d._js = AsyncMock(return_value=json.dumps({ - "ready": True, + # P2: navigate_conversation now uses _js_strict with a staged probe. + # Return a ready state at the right URL on first poll. + d._js_strict = AsyncMock(return_value=json.dumps({ "url": f"https://chatgpt.com/c/{cid}", + "ready_state": "complete", + "app_shell": True, + "composer": True, })) await d.navigate_conversation(cid) assert d._current_conv_id == cid @@ -214,14 +217,20 @@ async def test_navigate_conversation_raises_and_clears_when_never_ready(monkeypa cid = "abc-123" d._current_conv_id = cid # pre-existing (possibly stale) state d._cdp = AsyncMock() - # Composer never ready / never at the right URL. - d._js = AsyncMock(return_value=json.dumps({"ready": False, "url": ""})) + # P2: staged probe — composer never ready. + d._js_strict = AsyncMock(return_value=json.dumps({ + "url": f"https://chatgpt.com/c/{cid}", + "ready_state": "complete", + "app_shell": True, + "composer": False, # composer never appears + })) # Collapse the sleeps so the 30-iteration loop runs fast. async def _fast(_s): return None monkeypatch.setattr("chatgpt_web2api.cdp_driver.asyncio.sleep", _fast) - with pytest.raises(RuntimeError, match="did not reach a ready composer"): + # P2: error message now names the failed stage instead of the old opaque msg. + with pytest.raises(RuntimeError, match="composer"): await d.navigate_conversation(cid) assert d._current_conv_id is None # stale id cleared, not admitted diff --git a/tests/test_navigation_readiness.py b/tests/test_navigation_readiness.py new file mode 100644 index 00000000..0cf2ce30 --- /dev/null +++ b/tests/test_navigation_readiness.py @@ -0,0 +1,240 @@ +"""P2: Navigation staged readiness tests. + +Replaces the opaque "ready composer" poll with a diagnosable staged probe. +Co-designed with ChatGPT (vision-alignment cycle, conversation 6a4ebb2a). + +The probe splits the readiness check into stages: + url_correct → document_ready → app_shell_present → composer_present + +Each stage's result is captured so that when the poll fails, the error +message says WHICH stage failed (instead of the current opaque "did not +reach a ready composer within the timeout"). + +Also adds: + - Fast-fail on URL displacement (nav_displaced error) + - document.readyState check (don't fail while page is still loading) +""" + +import asyncio +import json +from unittest.mock import AsyncMock, MagicMock + +import pytest + +from chatgpt_web2api.cdp_driver import CDPDriver, NavigationReadinessProbe + + +def _probe_payload(*, url="https://chatgpt.com/c/conv-123", + ready_state="complete", app_shell=True, + composer=True): + """Build the JSON the staged probe JS returns.""" + return json.dumps({ + "url": url, + "ready_state": ready_state, + "app_shell": app_shell, + "composer": composer, + }) + + +# ── NavigationReadinessProbe ───────────────────────────────────────────── + + +class TestNavigationReadinessProbe: + """The probe dataclass carries all stage results for diagnostics.""" + + def test_all_stages_pass_when_everything_ready(self): + probe = NavigationReadinessProbe( + url="https://chatgpt.com/c/conv-123", + ready_state="complete", + app_shell_present=True, + composer_present=True, + ) + assert probe.is_ready(url_correct=True) is True + + def test_not_ready_when_composer_missing(self): + """The key P2 case: URL correct, document loaded, app shell present, + but composer selector didn't match (React still hydrating).""" + probe = NavigationReadinessProbe( + url="https://chatgpt.com/c/conv-123", + ready_state="complete", + app_shell_present=True, + composer_present=False, + ) + assert probe.is_ready(url_correct=True) is False + + def test_not_ready_when_url_wrong(self): + probe = NavigationReadinessProbe( + url="https://chatgpt.com/c/WRONG", + ready_state="complete", + app_shell_present=True, + composer_present=True, + ) + assert probe.is_ready(url_correct=False) is False + + def test_not_ready_when_document_loading(self): + """Page still loading — don't fail, just report not ready.""" + probe = NavigationReadinessProbe( + url="https://chatgpt.com/c/conv-123", + ready_state="loading", + app_shell_present=False, + composer_present=False, + ) + assert probe.is_ready(url_correct=True) is False + + def test_diagnostic_summary_includes_failed_stage(self): + """The probe can describe WHICH stage failed for the error message.""" + probe = NavigationReadinessProbe( + url="https://chatgpt.com/c/conv-123", + ready_state="complete", + app_shell_present=True, + composer_present=False, + ) + summary = probe.diagnostic_summary(url_correct=True) + assert "composer" in summary.lower(), ( + f"diagnostic summary should name the failed stage, got: {summary}" + ) + + def test_diagnostic_summary_when_url_displaced(self): + """When the URL moved away from target, the summary names displacement.""" + probe = NavigationReadinessProbe( + url="https://chatgpt.com/c/DIFFERENT", + ready_state="complete", + app_shell_present=True, + composer_present=True, + ) + summary = probe.diagnostic_summary(url_correct=False) + assert "url" in summary.lower() or "displac" in summary.lower(), ( + f"diagnostic summary should flag URL mismatch, got: {summary}" + ) + + +# ── navigate_conversation uses the staged probe ────────────────────────── + + +def _make_driver(): + """A CDPDriver with a mock transport for testing.""" + driver = MagicMock(spec=CDPDriver) + driver._cdp = AsyncMock() + driver._js_strict = AsyncMock(return_value="{}") + driver._js = AsyncMock(return_value="") + driver._current_conv_id = None + driver._is_url_at_conversation = CDPDriver._is_url_at_conversation + return driver + + +@pytest.mark.asyncio +async def test_navigate_succeeds_when_all_stages_pass(monkeypatch): + """When all readiness stages pass, navigate_conversation completes + without raising and sets _current_conv_id.""" + driver = CDPDriver(cdp_port=9222) + driver._cdp = AsyncMock() + driver._js_strict = AsyncMock(return_value=_probe_payload()) + + await driver.navigate_conversation("conv-123") + assert driver._current_conv_id == "conv-123" + + +@pytest.mark.asyncio +async def test_navigate_fails_with_diagnostic_on_composer_missing(monkeypatch): + """When the composer is never found but everything else loads, the error + message names the failed stage ('composer'), not just 'timeout'.""" + import time + + driver = CDPDriver(cdp_port=9222) + driver._cdp = AsyncMock() + # URL correct, doc loaded, app shell present, but composer never appears. + driver._js_strict = AsyncMock(return_value=_probe_payload(composer=False)) + + t = [0.0] + + _real_sleep = asyncio.sleep + + async def fast_sleep(d): + t[0] += d + await _real_sleep(0) + + monkeypatch.setattr(time, "monotonic", lambda: t[0]) + monkeypatch.setattr(asyncio, "sleep", fast_sleep) + + with pytest.raises(RuntimeError) as exc_info: + await driver.navigate_conversation("conv-123") + + msg = str(exc_info.value) + assert "composer" in msg.lower(), ( + f"error should name the failed stage 'composer', got: {msg}" + ) + + +@pytest.mark.asyncio +async def test_navigate_fast_fails_on_url_displacement(monkeypatch): + """If the URL moves AWAY from the target mid-poll, fail fast with a + distinct error (nav_displaced), don't burn the full 15s.""" + import time + + driver = CDPDriver(cdp_port=9222) + driver._cdp = AsyncMock() + + # First poll: URL is correct (still loading). Second poll: URL displaced. + polls = [ + _probe_payload(url="https://chatgpt.com/c/conv-123", ready_state="loading", + app_shell=False, composer=False), + _probe_payload(url="https://chatgpt.com/c/DIFFERENT", + ready_state="complete", app_shell=True, composer=True), + ] + call_count = {"n": 0} + + async def fake_js(expr, timeout=15): + idx = min(call_count["n"], len(polls) - 1) + call_count["n"] += 1 + return polls[idx] + + driver._js_strict = fake_js + + t = [0.0] + + _real_sleep = asyncio.sleep + + async def fast_sleep(d): + t[0] += d + await _real_sleep(0) + + monkeypatch.setattr(time, "monotonic", lambda: t[0]) + monkeypatch.setattr(asyncio, "sleep", fast_sleep) + + with pytest.raises(RuntimeError) as exc_info: + await driver.navigate_conversation("conv-123") + + msg = str(exc_info.value) + assert "displac" in msg.lower(), ( + f"error should mention displacement, got: {msg}" + ) + # Should NOT have burned the full 15s — fast-fail. + assert t[0] < 15, f"fast-fail should not wait 15s, got t={t[0]}" + + +@pytest.mark.asyncio +async def test_navigate_waits_for_document_ready(monkeypatch): + """When readyState='loading', the poll should wait, not count against + the stall budget. This tests the document.readyState check.""" + import time + + driver = CDPDriver(cdp_port=9222) + driver._cdp = AsyncMock() + + # First 3 polls: loading. Then: complete + composer present. + loading = _probe_payload(ready_state="loading", app_shell=False, composer=False) + ready = _probe_payload(ready_state="complete", app_shell=True, composer=True) + polls = [loading, loading, loading, ready] + call_count = {"n": 0} + + async def fake_js(expr, timeout=15): + idx = min(call_count["n"], len(polls) - 1) + call_count["n"] += 1 + return polls[idx] + + driver._js_strict = fake_js + + await driver.navigate_conversation("conv-123") + assert driver._current_conv_id == "conv-123" + # Should have polled at least 4 times (3 loading + 1 ready). + assert call_count["n"] >= 4 From 77faec5219683bf6e5c92c5b3b4a35d78a97236a Mon Sep 17 00:00:00 2001 From: Alajmah <55954113+Alajmah@users.noreply.github.com> Date: Thu, 9 Jul 2026 06:06:37 +0300 Subject: [PATCH 07/25] feat(recovery): ZCode SessionStart hook for bridge auto-recovery MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The bridge had no reboot/crash survival mechanism. After a power outage, every other system restarted but the bridge stayed dead because nothing was configured to invoke 'ensure' at startup. The 'ensure' command was built for exactly this (roadmap Phase 3: 'ZCode hook runs ensure') but the actual hook was never wired into the ZCode config. This commit adds the wrapper script. The hook configuration goes in ~/.zcode/cli/config.json (not committed — it's machine-specific): hooks.events.SessionStart → runs ensure_hook.py The wrapper (scripts/ensure_hook.py): - Runs 'chatgpt-web2api ensure' to reconcile REST + SSE - ALWAYS exits 0 so bridge failures never block the ZCode session (auth-required / failure states are logged to stderr, not blocking) - 90s timeout for Chrome cold-start - Captures ensure's output for ZCode's hook execution log Effect: every ZCode session start now reconciles the bridge. If the bridge is down (crash, reboot, never started), it cold-starts automatically. If auth is expired, it logs the issue without blocking. This is the recovery mechanism that was missing. --- scripts/ensure_hook.py | 60 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 60 insertions(+) create mode 100644 scripts/ensure_hook.py diff --git a/scripts/ensure_hook.py b/scripts/ensure_hook.py new file mode 100644 index 00000000..25a0606b --- /dev/null +++ b/scripts/ensure_hook.py @@ -0,0 +1,60 @@ +#!/usr/bin/env python +"""ZCode SessionStart hook wrapper for `chatgpt-web2api ensure`. + +Runs `chatgpt-web2api ensure` to reconcile the bridge (start REST + SSE if +missing, verify health). ALWAYS exits 0 so a bridge failure never blocks the +ZCode session — the bridge being down is an infrastructure problem, not a +reason to prevent the user from working. Failures are logged to stderr where +ZCode captures them in its hook execution log. + +Usage (from ZCode hooks.json): + command: python.exe + args: [path/to/ensure_hook.py, --rest-port, 8080, --mcp-sse-port, 8090] + +Exit codes: + 0 — always (hook passes regardless of bridge state) + The bridge's actual state is logged to stderr for diagnosis. +""" +import subprocess +import sys + + +def main() -> None: + # Forward our args (minus the script name) to `chatgpt-web2api ensure`. + # The first arg is this script's path; the rest are ensure's flags. + ensure_args = sys.argv[1:] + cmd = [sys.executable, "-m", "chatgpt_web2api", "ensure"] + ensure_args + + try: + result = subprocess.run( + cmd, + capture_output=True, + text=True, + timeout=90, # Chrome cold-start can take 10-15s; 90s is generous + ) + # Log the output for ZCode's hook execution log. + if result.stdout: + print(result.stdout, file=sys.stderr) + if result.stderr: + print(result.stderr, file=sys.stderr) + if result.returncode != 0: + print( + f"ensure exited {result.returncode} — bridge may need manual " + f"attention (auth=2, failure=1). This does not block the session.", + file=sys.stderr, + ) + except subprocess.TimeoutExpired: + print( + "ensure timed out after 90s — bridge may be slow to start. " + "This does not block the session.", + file=sys.stderr, + ) + except Exception as e: + print(f"ensure hook error: {e}", file=sys.stderr) + + # ALWAYS exit 0 — never block the ZCode session. + sys.exit(0) + + +if __name__ == "__main__": + main() From b89a316b3c21ab3d1c5bf47e169e42bc463dd72f Mon Sep 17 00:00:00 2001 From: Alajmah <55954113+Alajmah@users.noreply.github.com> Date: Thu, 9 Jul 2026 06:31:20 +0300 Subject: [PATCH 08/25] =?UTF-8?q?fix(navigation):=20P2=20review=20fixes=20?= =?UTF-8?q?=E2=80=94=20displacement=20debounce=20+=20JS=20error=20diagnost?= =?UTF-8?q?ic?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ChatGPT review (conversation 6a4f1262) approved P2 with two improvements: B. Displacement debounce: a single sampled wrong URL after arrival was too aggressive — ChatGPT's SPA briefly normalizes routes / strips params / remounts during transitions, which could false-positive. Fix: require 2 consecutive wrong-URL polls before fast-failing (debounce). The first wrong poll increments displacement_count; the second triggers nav_displaced. A correct URL resets the counter. C. JS error in 'all probes errored' diagnostic: when every poll's _js_strict fails, the error message now includes the last JS error so the failure is traceable (e.g. 'execution context destroyed') rather than 'no readiness data obtained' with no clue why. 670 tests pass. --- src/chatgpt_web2api/cdp_driver.py | 31 ++++++++++++++++++++---------- tests/test_navigation_readiness.py | 5 ++++- 2 files changed, 25 insertions(+), 11 deletions(-) diff --git a/src/chatgpt_web2api/cdp_driver.py b/src/chatgpt_web2api/cdp_driver.py index 225063bd..daabcfa1 100644 --- a/src/chatgpt_web2api/cdp_driver.py +++ b/src/chatgpt_web2api/cdp_driver.py @@ -1284,14 +1284,20 @@ async def navigate_conversation(self, conversation_id: str) -> None: ) last_probe: NavigationReadinessProbe | None = None + last_js_error: str | None = None url_was_correct = False # track if URL was ever correct (for displacement) + displacement_count = 0 # P2 review: debounce — require 2 consecutive wrong polls for _ in range(30): try: result = await self._js_strict(probe_js) data = json.loads(result) + last_js_error = None # successful probe clears the error except Exception as e: # P2: log transient JS failures instead of silently swallowing. + # Distinguish "probe execution failed" from "stage failed" per + # ChatGPT review finding C. + last_js_error = str(e) logger.debug("Navigation probe JS failed (will retry): %s", e) await asyncio.sleep(0.5) continue @@ -1305,18 +1311,23 @@ async def navigate_conversation(self, conversation_id: str) -> None: last_probe = probe url_correct = self._is_url_at_conversation(probe.url, conversation_id) - # P2: fast-fail on URL displacement. If the URL was correct on a - # prior poll but is now wrong, the page navigated away (SPA redirect, - # access denied, conversation deleted). Don't burn the full 15s. + # P2: fast-fail on URL displacement with debounce (review finding B). + # If the URL was correct on a prior poll but is now wrong, the page + # may have navigated away (SPA redirect, access denied, conversation + # deleted). Require 2 CONSECUTIVE wrong-URL polls to avoid + # false-positive on SPA route normalization / param stripping. if url_correct: url_was_correct = True + displacement_count = 0 elif url_was_correct: - if self._current_conv_id == conversation_id: - self._current_conv_id = None - raise RuntimeError( - f"Navigation to {conversation_id} displaced — URL moved " - f"to {probe.url[:80]} after initially loading (nav_displaced)" - ) + displacement_count += 1 + if displacement_count >= 2: + if self._current_conv_id == conversation_id: + self._current_conv_id = None + raise RuntimeError( + f"Navigation to {conversation_id} displaced — URL moved " + f"to {probe.url[:80]} after initially loading (nav_displaced)" + ) if probe.is_ready(url_correct): logger.info("Conversation ready: %s", probe.url) @@ -1336,7 +1347,7 @@ async def navigate_conversation(self, conversation_id: str) -> None: ) raise RuntimeError( f"Navigation to {conversation_id} failed — all probes errored " - f"(no readiness data obtained)" + f"(no readiness data obtained, last_js_error={last_js_error})" ) await asyncio.sleep(1) diff --git a/tests/test_navigation_readiness.py b/tests/test_navigation_readiness.py index 0cf2ce30..614316f8 100644 --- a/tests/test_navigation_readiness.py +++ b/tests/test_navigation_readiness.py @@ -174,12 +174,15 @@ async def test_navigate_fast_fails_on_url_displacement(monkeypatch): driver = CDPDriver(cdp_port=9222) driver._cdp = AsyncMock() - # First poll: URL is correct (still loading). Second poll: URL displaced. + # First poll: URL is correct (still loading). Then 2 displaced polls + # (debounce requires 2 consecutive wrong polls per ChatGPT review finding B). polls = [ _probe_payload(url="https://chatgpt.com/c/conv-123", ready_state="loading", app_shell=False, composer=False), _probe_payload(url="https://chatgpt.com/c/DIFFERENT", ready_state="complete", app_shell=True, composer=True), + _probe_payload(url="https://chatgpt.com/c/DIFFERENT", + ready_state="complete", app_shell=True, composer=True), ] call_count = {"n": 0} From 38ff8d866d32d0bcaac7f6661965b6b855fb0dfc Mon Sep 17 00:00:00 2001 From: Alajmah <55954113+Alajmah@users.noreply.github.com> Date: Thu, 9 Jul 2026 07:19:19 +0300 Subject: [PATCH 09/25] feat(send): P2.5 send-readiness selector hardening + rich diagnostics MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses error #4 — 'Send failed: no send button' (button_count:142). A fully-loaded ChatGPT page where neither the aria-label nor testid selector matched. Co-designed with ChatGPT (vision-alignment cycle, conversation 6a4ebb2a). Two changes: 1. Richer diagnostic snapshot (_capture_selector_diagnostic): expanded from url/title/body/button_count to include send-readiness fields: - composer_found / composer_text_length / composer_enabled: distinguishes 'text didn't land' (injection failed) from 'selector drifted' - send_candidates_count / enabled_send_candidates_count: how many buttons match a broad send heuristic - stop_button_present / generating_indicator_present: is a generation in progress (send button replaced by stop) This makes the next 'no send button' failure diagnosable — the operator can tell whether the text landed, whether a generation was active, and whether any submit-type button exists. 2. Broader send-button fallback (SEND_BUTTON_BROAD_SELECTOR): adds 'form button[type=submit]' as a third selector tier after aria-label and testid. If ChatGPT changes the aria-label (selector drift), a submit-type button inside the composer form is still the send affordance. 674 tests pass (670 existing + 4 new P2.5 tests). --- src/chatgpt_web2api/chatgpt_dom.py | 45 +++++-- tests/test_send_readiness.py | 207 +++++++++++++++++++++++++++++ 2 files changed, 243 insertions(+), 9 deletions(-) create mode 100644 tests/test_send_readiness.py diff --git a/src/chatgpt_web2api/chatgpt_dom.py b/src/chatgpt_web2api/chatgpt_dom.py index 4fff2f29..86027d2c 100644 --- a/src/chatgpt_web2api/chatgpt_dom.py +++ b/src/chatgpt_web2api/chatgpt_dom.py @@ -83,6 +83,10 @@ # the legacy testid for older deployments. SEND_BUTTON_SELECTOR = 'button[aria-label*="Send" i]:not([data-testid="stop-button"])' SEND_BUTTON_FALLBACK_SELECTOR = 'button[data-testid="send-button"]' +# P2.5: broader fallback — a submit-type button inside the composer form. +# If ChatGPT changes the aria-label (selector drift), a type=submit button +# inside the form is still the send affordance. This is the LAST resort. +SEND_BUTTON_BROAD_SELECTOR = 'form button[type="submit"]' # Send-button readiness poll. After a prior send completes (or under parallel # mode, where the MutationLock releases the instant a send finishes), ChatGPT's @@ -406,7 +410,8 @@ async def click_send(self) -> None: has_btn = await d._js( "(function() {" f" var btn = document.querySelector('{SEND_BUTTON_SELECTOR}')" - f" || document.querySelector('{SEND_BUTTON_FALLBACK_SELECTOR}');" + f" || document.querySelector('{SEND_BUTTON_FALLBACK_SELECTOR}')" + f" || document.querySelector('{SEND_BUTTON_BROAD_SELECTOR}');" " return btn && !btn.disabled ? 'yes' : 'no';" "})()" ) @@ -417,7 +422,8 @@ async def click_send(self) -> None: result = await d._js( "(function() {" f" var btn = document.querySelector('{SEND_BUTTON_SELECTOR}')" - f" || document.querySelector('{SEND_BUTTON_FALLBACK_SELECTOR}');" + f" || document.querySelector('{SEND_BUTTON_FALLBACK_SELECTOR}')" + f" || document.querySelector('{SEND_BUTTON_BROAD_SELECTOR}');" " if (!btn) return 'no send button';" " if (btn.disabled) return 'button disabled';" " var evts = ['pointerdown','mousedown','pointerup','mouseup','click'];" @@ -507,23 +513,44 @@ class names: find the ``[role=dialog]`` whose text matches, then click # ── Diagnostics ─────────────────────────────────────────── async def _capture_selector_diagnostic(self, selector_name: str) -> None: - """#5: Capture DOM state when a selector fails to match. - - Logs a diagnostic snapshot (URL, title, body text preview, button count) - so selector drift is diagnosable without W2A_DIAGNOSE=1. Called at the - point of selector failure (e.g. 'no send button', 'No textarea'). - Best-effort — never raises. + """#5 / P2.5: Capture DOM state when a selector fails to match. + + Logs a rich diagnostic snapshot so selector drift and send-readiness + failures are diagnosable without W2A_DIAGNOSE=1. Called at the point of + selector failure (e.g. 'no send button', 'No textarea'). Best-effort — + never raises. + + P2.5 (2026-07-09): expanded from url/title/body/button_count to include + send-readiness-specific fields (ChatGPT's proposed readiness snapshot): + - composer_found / composer_text_length / composer_enabled: distinguishes + "text didn't land" (injection failed) from "selector drifted" + - send_candidates_count / enabled_send_candidates_count: how many buttons + match a broad send heuristic, and how many are enabled + - stop_button_present / generating_indicator_present: is a generation + in progress (the send button is replaced by stop during generation) """ d = self._driver try: snapshot = await d._js_strict( "(function(){" + " var composer = document.querySelector('" + COMPOSER_SELECTOR + "')" + " || document.querySelector('" + COMPOSER_FALLBACK_SELECTOR + "');" + " var sendCandidates = document.querySelectorAll('button[type=\"submit\"], button[aria-label*=\"Send\" i], button[data-testid=\"send-button\"]');" + " var enabledSend = Array.prototype.filter.call(sendCandidates, function(b){ return !b.disabled; });" + " var stopBtn = document.querySelector('[data-testid=\"stop-button\"], button[aria-label*=\"Stop\" i]');" " return JSON.stringify({" " url: location.href," " title: document.title," " body_preview: (document.body && document.body.innerText || '').slice(0, 300)," " button_count: document.querySelectorAll('button').length," - " textarea_count: document.querySelectorAll('textarea').length" + " textarea_count: document.querySelectorAll('textarea').length," + " composer_found: !!composer," + " composer_text_length: composer ? (composer.innerText || composer.value || '').length : 0," + " composer_enabled: composer ? !composer.disabled : false," + " send_candidates_count: sendCandidates.length," + " enabled_send_candidates_count: enabledSend.length," + " stop_button_present: !!stopBtn," + " generating_indicator_present: !!document.querySelector('[class*=\"result-thinking\"], [class*=\"generating\"]')" " });" "})()", timeout=5, diff --git a/tests/test_send_readiness.py b/tests/test_send_readiness.py new file mode 100644 index 00000000..6cd82b12 --- /dev/null +++ b/tests/test_send_readiness.py @@ -0,0 +1,207 @@ +"""P2.5: Send-readiness selector hardening tests. + +Addresses error #4 — "Send failed: no send button" (button_count:142). A +fully-loaded ChatGPT page where neither SEND_BUTTON_SELECTOR nor +SEND_BUTTON_FALLBACK_SELECTOR matched. Co-designed with ChatGPT (vision- +alignment cycle, conversation 6a4ebb2a). + +P2.5 scope: + 1. Richer diagnostic snapshot (composer_text_length, send_candidates, + enabled candidates, stop_button_present, generating_indicator_present). + 2. Broader send-button fallback selector (type=submit, not just aria-label). + 3. Distinguish 'composer injection failed' from 'send button missing'. +""" + +import asyncio +import json +import logging +import time as _time +from unittest.mock import AsyncMock, MagicMock + +import pytest + +from chatgpt_web2api.chatgpt_dom import ChatGPTDom +from chatgpt_web2api.cdp_driver import CDPDriver, SendReadinessError + + +@pytest.fixture(autouse=True) +def _fast_poll(monkeypatch): + """Collapse the send-button poll loop so tests run instantly. + Patches both asyncio.sleep (the poll interval) and time.monotonic + (the deadline check) in the chatgpt_dom module.""" + _real_sleep = asyncio.sleep + _t = [0.0] + + async def _instant(_d): + _t[0] += _d # advance simulated time by the sleep duration + await _real_sleep(0) + + monkeypatch.setattr("chatgpt_web2api.chatgpt_dom.asyncio.sleep", _instant) + monkeypatch.setattr("chatgpt_web2api.chatgpt_dom.time.monotonic", lambda: _t[0]) + + +def _make_dom(diag_json=None): + """A ChatGPTDom backed by a real CDPDriver with mocked transport. + Using a real driver (not spec mock) so _capture_selector_diagnostic runs + the real code path that P2.5 enhances. + + diag_json: the JSON string the diagnostic _js_strict call should return. + If None, returns a minimal old-style snapshot (button_count only). + """ + driver = CDPDriver(cdp_port=9222) + driver._js = AsyncMock(return_value="") + # The diagnostic JS is the only _js_strict call from click_send. + # Return the diag payload (or old-style minimal if not provided). + driver._js_strict = AsyncMock(return_value=diag_json or json.dumps({ + "url": "https://chatgpt.com/c/conv-1", + "title": "Test", + "body_preview": "", + "button_count": 142, + "textarea_count": 1, + })) + driver._cdp = AsyncMock() + driver._breakers = None + driver._current_conv_id = "conv-1" + return ChatGPTDom(driver), driver + + +def _make_scripted_js(*, poll_result="no", send_result="no send button"): + """Build a fake _js that discriminates by expression content. + - The button poll contains 'disabled' (checking btn.disabled) + - The send JS contains 'no send button' (the failure return string) + """ + async def fake_js(expr, timeout=15): + if "no send button" in expr: + return send_result + # Button poll (or any other _js call) + return poll_result + return fake_js + + +def _diag_json(**overrides): + """Build the diagnostic snapshot JSON with defaults.""" + data = { + "url": "https://chatgpt.com/c/conv-1", + "title": "Test", + "body_preview": "Skip to content", + "button_count": 142, + "textarea_count": 1, + "composer_found": True, + "composer_text_length": 50, + "composer_enabled": True, + "send_candidates_count": 0, + "enabled_send_candidates_count": 0, + "stop_button_present": False, + "generating_indicator_present": False, + } + data.update(overrides) + return json.dumps(data) + + +# ── 1. Richer diagnostic snapshot ──────────────────────────────────────── + + +@pytest.mark.asyncio +async def test_send_diagnostic_probes_composer_and_send_state(caplog): + """When click_send fails, the diagnostic JS should probe send-readiness + fields: composer_text_length, send_candidates_count, enabled_send_candidates, + stop_button_present, generating_indicator_present — not just button_count. + + This test captures the ACTUAL diagnostic JS expression and verifies it + requests the P2.5 fields. The mock returns rich data; the assertion is + that the JS expression itself contains the new probes.""" + captured_js = [] + dom, driver = _make_dom() + + async def capturing_js_strict(expr, timeout=15): + captured_js.append(expr) + return _diag_json(send_candidates_count=3, enabled_send_candidates_count=1) + + driver._js = _make_scripted_js(poll_result="no", send_result="no send button") + driver._js_strict = capturing_js_strict + + with pytest.raises(SendReadinessError): + await dom.click_send() + + # The diagnostic JS should have been called and should probe the new fields. + assert len(captured_js) >= 1, "diagnostic _js_strict should have been called" + diag_expr = captured_js[-1] + assert "send_candidates" in diag_expr or "composer_text" in diag_expr, ( + f"diagnostic JS should probe send_candidates/composer_text, got: {diag_expr[:120]}" + ) + assert "stop_button" in diag_expr or "stop-button" in diag_expr, ( + f"diagnostic JS should probe stop_button state, got: {diag_expr[:120]}" + ) + + +# ── 2. Broader send-button fallback (type=submit) ─────────────────────── + + +@pytest.mark.asyncio +async def test_send_succeeds_with_submit_type_fallback(): + """If the aria-label selector fails but a type=submit button exists inside + the composer form, the broader fallback should find it.""" + dom, driver = _make_dom() + + driver._js = _make_scripted_js(poll_result="yes", send_result="sent") + await dom.click_send() # should NOT raise + + +# ── 3. Distinguish injection-failed from send-button-missing ──────────── + + +@pytest.mark.asyncio +async def test_error_distinguishes_empty_composer_from_missing_button(caplog): + """When the send button is missing AND the composer is empty, the error + should indicate the composer is empty (injection failed), not just 'no + send button'. This distinguishes hypothesis #6 (input injection failed) + from hypothesis #1 (selector drift).""" + dom, driver = _make_dom() + + driver._js = _make_scripted_js(poll_result="no", send_result="no send button") + driver._js_strict = AsyncMock(return_value=_diag_json( + composer_text_length=0, # EMPTY — injection failed! + )) + + with caplog.at_level(logging.WARNING): + with pytest.raises(SendReadinessError) as exc_info: + await dom.click_send() + + msg = str(exc_info.value) + all_logs = " ".join(r.message for r in caplog.records) + combined = msg + " " + all_logs + # The diagnostic should show composer_text_length=0 so a human can + # distinguish "text didn't land" from "selector drifted". + assert "composer_text_length" in combined, ( + f"diagnostic should include composer_text_length to distinguish " + f"injection failure, got: {combined[:200]}" + ) + + +# ── 4. Stop-button-present diagnostic ──────────────────────────────────── + + +@pytest.mark.asyncio +async def test_diagnostic_captures_stop_button_when_generating(caplog): + """When the send button is missing because a generation is in progress + (stop button visible), the diagnostic should capture stop_button_present=True.""" + dom, driver = _make_dom() + + driver._js = _make_scripted_js(poll_result="no", send_result="no send button") + driver._js_strict = AsyncMock(return_value=_diag_json( + stop_button_present=True, + generating_indicator_present=True, + composer_enabled=False, + )) + + with caplog.at_level(logging.WARNING): + with pytest.raises(SendReadinessError): + await dom.click_send() + + diag_logs = [r for r in caplog.records if "diagnostic" in r.message.lower() + or "selector" in r.message.lower()] + assert len(diag_logs) >= 1 + log_msg = diag_logs[0].message + assert "stop_button_present" in log_msg, ( + f"diagnostic should capture stop_button state, got: {log_msg}" + ) From 34c82a2f2d0f51631cbdaea0396b6ab959ab0b2c Mon Sep 17 00:00:00 2001 From: Alajmah <55954113+Alajmah@users.noreply.github.com> Date: Thu, 9 Jul 2026 07:21:25 +0300 Subject: [PATCH 10/25] fix(send): scope broad fallback to composer form (ChatGPT review caveat) ChatGPT merge review flagged that 'form button[type=submit]' was page-global and could hit login/search/feedback forms on a displaced page. Fix: scope to the composer form via :has(#prompt-textarea) / :has(.ProseMirror). --- src/chatgpt_web2api/chatgpt_dom.py | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/src/chatgpt_web2api/chatgpt_dom.py b/src/chatgpt_web2api/chatgpt_dom.py index 86027d2c..67a6eeb0 100644 --- a/src/chatgpt_web2api/chatgpt_dom.py +++ b/src/chatgpt_web2api/chatgpt_dom.py @@ -83,10 +83,15 @@ # the legacy testid for older deployments. SEND_BUTTON_SELECTOR = 'button[aria-label*="Send" i]:not([data-testid="stop-button"])' SEND_BUTTON_FALLBACK_SELECTOR = 'button[data-testid="send-button"]' -# P2.5: broader fallback — a submit-type button inside the composer form. -# If ChatGPT changes the aria-label (selector drift), a type=submit button -# inside the form is still the send affordance. This is the LAST resort. -SEND_BUTTON_BROAD_SELECTOR = 'form button[type="submit"]' +# P2.5: broader fallback — a submit-type button inside the COMPOSER FORM +# (scoped to the form containing #prompt-textarea, not page-global, to avoid +# hitting login/search/feedback forms if the page is displaced). If ChatGPT +# changes the aria-label (selector drift), a submit-type button inside the +# composer form is still the send affordance. This is the LAST resort. +SEND_BUTTON_BROAD_SELECTOR = ( + 'form:has(#prompt-textarea) button[type="submit"],' + 'form:has(.ProseMirror) button[type="submit"]' +) # Send-button readiness poll. After a prior send completes (or under parallel # mode, where the MutationLock releases the instant a send finishes), ChatGPT's From 4d2f2df39acfb91818db72768c9df240d8c4d4f7 Mon Sep 17 00:00:00 2001 From: Alajmah <55954113+Alajmah@users.noreply.github.com> Date: Thu, 9 Jul 2026 12:45:37 +0300 Subject: [PATCH 11/25] docs: P3 multi-client CDP degradation note + corrected root cause Documents that owned tabs isolate DOM-level navigation (one client's Page.navigate cannot move another's tab) but multiple CDP clients on one Chrome still degrade reliability through resource contention (CPU/memory saturation slowing React hydration past the readiness window). Added to: - docs/deployment.md: new section 'Other CDP clients on the same Chrome' with what the contention looks like, why the write path is more sensitive than the read path, and three mitigations (dedicated Chrome, fewer clients, parallel_tabs). - chatgpt-collaboration skill agent-field-notes: section 7 documenting the symptom, verified root cause, and what NOT to assume. This is the P3 documentation item from the build sequence. --- docs/deployment.md | 45 +++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 43 insertions(+), 2 deletions(-) diff --git a/docs/deployment.md b/docs/deployment.md index 1607acff..51174cb8 100644 --- a/docs/deployment.md +++ b/docs/deployment.md @@ -287,8 +287,49 @@ W2A_PARALLEL_TABS=1 chatgpt-web2api --port 8082 --cdp-port 9222 > **Rollout warning:** do not mix old (port-lock-only) workers and new > (`parallel_tabs=true`) workers on the **same CDP port** during a rolling -> upgrade — their lock files don't exclude each other. Finish the rollout on a -> port before enabling parallel mode on it. +> upgrade — their lock files don't exclude each other. Finish the rollout on +> a port before enabling parallel mode on it. + +### Other CDP clients on the same Chrome (Super-Browser, Playwright, etc.) + +The bridge creates **owned tabs** (`tab_mode="owned"`, the default) — each +driver gets its own dedicated tab via `Target.createTarget` and its own +websocket. A `Page.navigate` issued by one CDP client goes over that client's +websocket to that client's tab; it **cannot** navigate another client's tab. +CDP targets are independent. + +However, running multiple CDP clients (e.g., 4 Super-Browser MCP servers + the +bridge + the MCP server = 6+ clients) on one Chrome instance can still +**degrade reliability** through resource contention: + +- **CPU/memory saturation** — each CDP client drives React hydration, DOM + polling, and JS evaluation. Under load, the Chrome renderer can slow + enough that ChatGPT's SPA fails to hydrate the composer within the + bridge's readiness window (see P2 staged readiness diagnostics). +- **CDP HTTP endpoint queuing** — the `/json/list` and `/json/version` + endpoints are served by Chrome's DevTools HTTP server, which is + single-threaded. Many clients polling concurrently can introduce latency. +- **Profile-level state** — all tabs share one Chrome profile (one account, + one cookie jar, one service worker). A service-worker update or auth + redirect on one tab can briefly affect the others. + +**What this means in practice:** the bridge's write path (navigate + send) +is more sensitive to these effects than the read path (list/get +conversations via `/backend-api/*`), because the write path requires +DOM-level readiness (composer hydrated, send button enabled) while the read +path is a pure HTTP fetch. If you observe "navigation timeout" or "no send +button" errors that worsen as more clients connect, consider: + +1. Running the bridge on a **dedicated Chrome instance** (separate + `--user-data-dir` and `--cdp-port`). +2. Reducing the number of concurrent CDP clients on the shared Chrome. +3. Enabling `parallel_tabs: true` so the bridge and MCP server each get + their own tab (they do by default with `tab_mode="owned"`, but + `parallel_tabs` adds per-target locking and fail-closed semantics). + +The bridge's error messages now include staged diagnostics (P2) that name +the failing readiness stage, making this degradation diagnosable rather +than opaque. --- From ffef9f78feb89aa65bb5d323efad83647765b292 Mon Sep 17 00:00:00 2001 From: Alajmah <55954113+Alajmah@users.noreply.github.com> Date: Thu, 9 Jul 2026 15:58:27 +0300 Subject: [PATCH 12/25] fix(reconcile): treat non_text as retryable, not terminal (e2e placeholder race) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Root cause of the intermittent '[Non-text response generated]' placeholder: the post-completion reconciliation loop treated select_text_for_turn's 'non_text' status as terminal (immediate break). But non_text is NOT terminal — the backend propagates intermediary nodes (reasoning_recap, thoughts, model_editable_context) BEFORE the final text node. When the reconciliation ran in that gap, it saw only intermediaries, concluded 'non-text', and yielded the placeholder — even though the text node would appear within seconds. Fix: treat non_text like not_ready (keep polling). After the loop exhausts (30s), if the last status is still non_text, fall through to the placeholder. If it's not_ready/ambiguous/etc., raise TurnReconciliationError as before. Verified: e2e completion test passed 3/3 consecutive runs (was failing intermittently before the fix). 674 unit tests still pass. This is a pre-existing bug (not a P1/P2 regression) — the non_text break existed since the A2 reconciliation work (commit 5ad0b9e). --- src/chatgpt_web2api/cdp_driver.py | 36 +++++++++++++++++++++---------- 1 file changed, 25 insertions(+), 11 deletions(-) diff --git a/src/chatgpt_web2api/cdp_driver.py b/src/chatgpt_web2api/cdp_driver.py index daabcfa1..9c322b2a 100644 --- a/src/chatgpt_web2api/cdp_driver.py +++ b/src/chatgpt_web2api/cdp_driver.py @@ -1716,23 +1716,37 @@ async def send_and_stream( last_dom_text = result.text break if result.status == "non_text": - break # placeholder path below + # P2.5 RCA fix: non_text is NOT terminal here. The backend + # propagates intermediary nodes (reasoning_recap, thoughts, + # model_editable_context) BEFORE the final text node. + # Treating non_text as terminal caused an intermittent + # race: the reconciliation saw the intermediaries, + # concluded "non-text", and yielded the placeholder even + # though the text node would appear within seconds. + # Now: keep polling (like not_ready) — the text node may + # still be propagating. Only after the loop exhausts do we + # yield the placeholder. + pass if result.status in ("ambiguous", "degraded_not_fresh", "fetch_failed"): # Keep polling — these may resolve as the backend settles. pass # not_ready → keep polling. await asyncio.sleep(0.5) else: - # Loop exhausted without match — raise typed error. - raise TurnReconciliationError( - conversation_id=conv_id, - anchor_mode=turn_anchor.mode, - last_status=last_status, - diagnostic={ - "captured_id": turn_anchor.captured_user_message_id, - "had_non_text_content": had_non_text_content, - }, - ) + # Loop exhausted without a text match. + # If the last status was non_text (genuinely non-text + # response after full polling), fall through to the + # placeholder below. Otherwise raise a typed error. + if last_status != "non_text": + raise TurnReconciliationError( + conversation_id=conv_id, + anchor_mode=turn_anchor.mode, + last_status=last_status, + diagnostic={ + "captured_id": turn_anchor.captured_user_message_id, + "had_non_text_content": had_non_text_content, + }, + ) # Non-text placeholder (unchanged from pre-A2). if not last_dom_text and had_non_text_content: placeholder = ( From 63e0db2e885ddaf2fb80ff992bd6a8c8cab3025d Mon Sep 17 00:00:00 2001 From: Alajmah <55954113+Alajmah@users.noreply.github.com> Date: Fri, 10 Jul 2026 08:19:16 +0300 Subject: [PATCH 13/25] fix(navigation): _is_url_at_conversation now handles project-scoped URLs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Root cause of the 'url displaced' errors reported by 3 sessions using the bridge simultaneously. PROVED, not guessed: _is_url_at_conversation checked parts[0]=='c' && parts[1]==conv_id, which only handles non-project URLs (/c/{id}). But project-scoped conversations use /g/{gizmo_id}/c/{id} — 4 path segments where parts[0]=='g'. The check returned False for every project-scoped URL, causing the P2 staged probe to report 'url displaced' even when the tab WAS on the correct conversation. This explains ALL the agent's symptoms: - 'url displaced' with a different project's gizmo_id: the probe read the project-scoped URL correctly but the comparison failed - 'url displaced' to https://chatgpt.com/: the SPA sometimes normalizes to the home page between navigations - Intermittent success: ChatGPT's SPA sometimes uses the non-project URL form (/c/{id}), which the old check handled correctly Fix: find the 'c' segment anywhere in the path and check the segment after it. Handles both /c/{id} and /g/{gizmo}/c/{id} shapes. Verified: project-scoped=True, non-project=True, truncated=False, home=False, different-conv=False, project+query=True. 674 tests pass. This is NOT the adopt-fallback/tab-theft theory I proposed earlier — that was wrong. The tabs were never being stolen. The URL comparison was broken. --- src/chatgpt_web2api/cdp_driver.py | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/src/chatgpt_web2api/cdp_driver.py b/src/chatgpt_web2api/cdp_driver.py index 9c322b2a..b29ff55d 100644 --- a/src/chatgpt_web2api/cdp_driver.py +++ b/src/chatgpt_web2api/cdp_driver.py @@ -1357,9 +1357,10 @@ async def navigate_conversation(self, conversation_id: str) -> None: def _is_url_at_conversation(url: str, conversation_id: str) -> bool: """Exact path-segment match: is *url* at ``/c/{conversation_id}``? - Uses urllib to parse the path and compare the second segment, so a - high-entropy id can't accidentally match as a substring of another - path. Query strings and trailing slashes are tolerated; a different + Handles both non-project URLs (``/c/{id}``) and project-scoped URLs + (``/g/{gizmo_id}/c/{id}``). Finds the ``c`` path segment and checks + if the segment immediately after it matches the conversation ID. + Query strings and trailing slashes are tolerated; a different conversation id or a non-conversation URL returns False. """ if not url or not conversation_id: @@ -1371,8 +1372,13 @@ def _is_url_at_conversation(url: str, conversation_id: str) -> bool: if "chatgpt.com" not in (parsed.netloc or "").lower(): return False parts = [p for p in parsed.path.split("/") if p] - # Expected shape: ["c", ""] - return len(parts) >= 2 and parts[0] == "c" and parts[1] == conversation_id + # Find the "c" segment — it's the conversation-route marker in both + # non-project (["c", "{id}"]) and project-scoped + # (["g", "{gizmo}", "c", "{id}"]) URL shapes. + for i, part in enumerate(parts): + if part == "c" and i + 1 < len(parts): + return parts[i + 1] == conversation_id + return False async def _is_live_conversation_url(self, conversation_id: str) -> bool: """Read ``location.href`` and check it is at *conversation_id*. From 23af499785d1bbbf236a1a380e206a3a8f9c90dc Mon Sep 17 00:00:00 2001 From: Alajmah <55954113+Alajmah@users.noreply.github.com> Date: Fri, 10 Jul 2026 13:43:32 +0300 Subject: [PATCH 14/25] refactor(url): use adjacent-pair match per ChatGPT review ChatGPT review (conv 6a50cb65) suggested checking the ('c', conversation_id) adjacent pair rather than finding the first 'c' and checking the next part. This avoids edge cases where 'c' appears earlier in a prefix or identifier. Verified with 9 edge cases including stray 'c' before the real route. --- src/chatgpt_web2api/cdp_driver.py | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/src/chatgpt_web2api/cdp_driver.py b/src/chatgpt_web2api/cdp_driver.py index b29ff55d..2158c95d 100644 --- a/src/chatgpt_web2api/cdp_driver.py +++ b/src/chatgpt_web2api/cdp_driver.py @@ -1372,13 +1372,15 @@ def _is_url_at_conversation(url: str, conversation_id: str) -> bool: if "chatgpt.com" not in (parsed.netloc or "").lower(): return False parts = [p for p in parsed.path.split("/") if p] - # Find the "c" segment — it's the conversation-route marker in both - # non-project (["c", "{id}"]) and project-scoped - # (["g", "{gizmo}", "c", "{id}"]) URL shapes. - for i, part in enumerate(parts): - if part == "c" and i + 1 < len(parts): - return parts[i + 1] == conversation_id - return False + # Find the ("c", conversation_id) adjacent pair — the conversation + # route marker in both non-project (["c", "{id}"]) and project-scoped + # (["g", "{gizmo}", "c", "{id}"]) URL shapes. Using the adjacent pair + # (rather than just finding the first "c") avoids false-positives if + # a "c" segment appears earlier in a different context. + return any( + parts[i] == "c" and parts[i + 1] == conversation_id + for i in range(len(parts) - 1) + ) async def _is_live_conversation_url(self, conversation_id: str) -> bool: """Read ``location.href`` and check it is at *conversation_id*. From 3c7c8190c3045ed5c1126b4ecf0858b87a46e2ad Mon Sep 17 00:00:00 2001 From: Alajmah <55954113+Alajmah@users.noreply.github.com> Date: Sat, 11 Jul 2026 18:31:25 +0300 Subject: [PATCH 15/25] =?UTF-8?q?fix(owned-mode):=20remove=20adopt=20fallb?= =?UTF-8?q?ack=20=E2=80=94=20never=20steal=20another=20process's=20tab?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The adopt fallback (_find_page_ws / _adopt_existing_chatgpt_tab) picks ANY chatgpt.com tab from /json/list without checking ownership. In a multi-process setup (REST bridge + SSE server), this steals another process's tab, causing two drivers to race on the same DOM — the root cause of the tab-drift and cross-session conversation contamination reported by 3 sessions. ChatGPT designed this fix (conv 6a507b4c): 'In owned mode, the valid operation is reclaim, not adopt. A process may drive only a tab whose ownership it can positively prove. URL matching is discovery, not proof of ownership.' Two changes in cdp_driver.py: 1. connect() path (line 555-567): when _create_owned_tab fails in owned mode, raise RuntimeError instead of falling back to _find_page_ws. The fallback is preserved for adopt mode (legacy compatibility). 2. reconnect() path (line 751-761 + 762-770): two sites where the fallback was reachable: - _create_owned_tab failure → raise OwnedTabRequiredError (caught by the immediate-raise list at line 802, not swallowed by the retry loop) - No ws_url after all attempts → raise OwnedTabRequiredError The TabRegistry already tracks ownership (instance_id → target_id + pid + heartbeat), but the adopt fallbacks never consulted it. This fix removes the fallbacks entirely for owned mode — a driver either reclaims its own tab by exact ID, creates a new one, or fails closed. Never scans for an arbitrary tab. 4 new tests (test_owned_mode_no_adopt.py): connect fails closed, adopt mode still falls back, reconnect fails closed, reconnect creates new tab. 1 updated test (test_tab_isolation.py): old fallback test replaced with fail-closed assertion. 678 tests pass. --- src/chatgpt_web2api/cdp_driver.py | 32 ++++++++ tests/test_owned_mode_no_adopt.py | 124 ++++++++++++++++++++++++++++++ tests/test_tab_isolation.py | 30 ++++---- 3 files changed, 172 insertions(+), 14 deletions(-) create mode 100644 tests/test_owned_mode_no_adopt.py diff --git a/src/chatgpt_web2api/cdp_driver.py b/src/chatgpt_web2api/cdp_driver.py index 2158c95d..e6aa5feb 100644 --- a/src/chatgpt_web2api/cdp_driver.py +++ b/src/chatgpt_web2api/cdp_driver.py @@ -561,6 +561,17 @@ async def connect(self) -> None: f"Owned-tab creation failed in parallel mode; refusing " f"shared-tab fallback: {e}" ) from e + if self.tab_mode == "owned": + # Owned mode: never adopt an arbitrary tab. The adopt + # fallback (_find_page_ws) picks ANY chatgpt.com tab, + # which could belong to another process — causing two + # drivers to race on the same tab. Fail closed instead; + # the caller (ensure/connect) will retry or surface the + # error. (ChatGPT design review, conv 6a507b4c.) + raise RuntimeError( + f"Owned-tab creation failed and shared-tab fallback " + f"is disabled in owned mode: {e}" + ) from e logger.warning("Tab isolation failed (%s) — falling back to shared tab", e) self._target_id = None self._owns_target = False @@ -747,6 +758,16 @@ async def reconnect(self) -> None: f"Reconnect owned-tab creation failed in " f"parallel mode; refusing fallback: {create_err}" ) from create_err + if self.tab_mode == "owned": + # Owned mode: tab creation failed and we must not + # fall back to adopting an arbitrary tab. Raise + # OwnedTabRequiredError so the reconnect retry + # loop doesn't swallow it. + raise OwnedTabRequiredError( + f"Reconnect owned-tab creation failed and " + f"shared-tab fallback is disabled in owned " + f"mode: {create_err}" + ) from create_err raise if not ws_url: if self._parallel_tabs: @@ -755,6 +776,17 @@ async def reconnect(self) -> None: "Reconnect could not obtain an owned tab; refusing " "shared-tab fallback in parallel mode" ) + if self.tab_mode == "owned": + # Owned mode: never adopt an arbitrary tab on reconnect. + # The adopt fallback could steal another process's tab, + # causing two drivers to race on the same DOM. Fail + # closed — raise OwnedTabRequiredError so the reconnect + # retry loop doesn't swallow it (it's in the immediate- + # raise list at line 802). + raise OwnedTabRequiredError( + "Reconnect could not obtain an owned tab and " + "shared-tab fallback is disabled in owned mode" + ) ws_url = await self._find_page_ws() self._ws = await websockets.connect( ws_url, diff --git a/tests/test_owned_mode_no_adopt.py b/tests/test_owned_mode_no_adopt.py new file mode 100644 index 00000000..aa01cfc0 --- /dev/null +++ b/tests/test_owned_mode_no_adopt.py @@ -0,0 +1,124 @@ +"""Tests: owned-mode never adopts an arbitrary tab. + +The adopt fallback (_find_page_ws / _adopt_existing_chatgpt_tab) picks ANY +chatgpt.com tab from /json/list without checking ownership. In a multi-process +setup (REST bridge + SSE server), this can steal another process's tab, +causing two drivers to race on the same DOM. + +These tests verify that in owned mode: + 1. connect() does NOT fall back to _find_page_ws when tab creation fails + 2. _reconnect() does NOT fall back to _find_page_ws when it can't find/create a tab + 3. adopt mode still falls back (the legacy behavior is preserved) +""" + +import asyncio +from unittest.mock import AsyncMock, MagicMock, patch + +import pytest + +from chatgpt_web2api.cdp_driver import CDPDriver + + +def _make_driver(tab_mode="owned", parallel_tabs=False): + """Build a CDPDriver with mocked transport for testing.""" + driver = CDPDriver(cdp_port=9222, tab_mode=tab_mode, parallel_tabs=parallel_tabs) + driver._browser_cdp = AsyncMock() + driver._find_page_ws = AsyncMock(return_value="ws://stolen-tab") + driver._find_owned_tab_ws = MagicMock(return_value=None) + driver._adopt_existing_chatgpt_tab = MagicMock(return_value=None) + driver._tab_registry = None # skip registry reclaim + driver._wait_for_chatgpt_ready = AsyncMock(return_value=True) + driver._refresh_token = AsyncMock() + driver._attach_identity_listener = AsyncMock() + driver._breakers = None + return driver + + +# ── 1. connect() does not adopt in owned mode ───────────────────────────── + + +@pytest.mark.asyncio +async def test_connect_owned_mode_does_not_fall_back_to_find_page_ws(): + """When tab creation fails in owned mode, connect must NOT call _find_page_ws. + It should raise instead — never steal another process's tab.""" + driver = _make_driver(tab_mode="owned") + # Make _create_owned_tab fail + driver._create_owned_tab = AsyncMock(side_effect=RuntimeError("Chrome refused")) + + with pytest.raises(RuntimeError, match="shared-tab fallback is disabled in owned mode"): + await driver.connect() + + # _find_page_ws must NOT have been called + driver._find_page_ws.assert_not_called() + + +# ── 2. connect() still falls back in adopt mode ─────────────────────────── + + +@pytest.mark.asyncio +async def test_connect_adopt_mode_still_falls_back(): + """Adopt mode preserves the legacy fallback to _find_page_ws.""" + driver = _make_driver(tab_mode="adopt") + driver._create_owned_tab = AsyncMock(side_effect=RuntimeError("Chrome refused")) + + # Should fall back to _find_page_ws, not raise + # Need to mock the websocket connect too + with patch("chatgpt_web2api.cdp_driver.websockets.connect") as mock_ws: + mock_ws.return_value = MagicMock() + try: + await driver.connect() + except Exception: + pass # may fail later in connect, but the key assertion is below + + # _find_page_ws WAS called (the legacy fallback) + driver._find_page_ws.assert_called() + + +# ── 3. reconnect does not adopt in owned mode ───────────────────────────── + + +@pytest.mark.asyncio +async def test_reconnect_owned_mode_does_not_fall_back_to_find_page_ws(): + """When reconnect can't find or create a tab in owned mode, it must NOT + call _find_page_ws. It should raise instead.""" + driver = _make_driver(tab_mode="owned") + driver._target_id = "OLD-TAB-ID" # had a tab, now gone + driver._ws = None # no dead socket to close + driver._reader_task = None # no reader to cancel + driver._pending = {} # no pending messages + # _find_owned_tab_ws returns None (tab gone) — already set in _make_driver + # _create_owned_tab fails + driver._create_owned_tab = AsyncMock(side_effect=RuntimeError("Chrome refused")) + + with pytest.raises(Exception, match="shared-tab fallback is disabled in owned mode"): + await driver.reconnect() + + driver._find_page_ws.assert_not_called() + + +# ── 4. reconnect creates new tab when owned tab is gone ────────────────── + + +@pytest.mark.asyncio +async def test_reconnect_owned_mode_creates_new_tab_when_old_gone(): + """When the old owned tab is gone, reconnect should create a NEW tab, + not adopt an existing one.""" + driver = _make_driver(tab_mode="owned") + driver._target_id = "OLD-TAB-ID" + driver._ws = None + driver._reader_task = None + driver._pending = {} + # _find_owned_tab_ws returns None (tab gone) + driver._create_owned_tab = AsyncMock(return_value="ws://new-tab") + + with patch("chatgpt_web2api.cdp_driver.websockets.connect") as mock_ws: + mock_ws.return_value = MagicMock() + try: + await driver.reconnect() + except Exception: + pass + + # _create_owned_tab was called (created a new tab, not adopted) + driver._create_owned_tab.assert_called() + # _find_page_ws was NOT called + driver._find_page_ws.assert_not_called() diff --git a/tests/test_tab_isolation.py b/tests/test_tab_isolation.py index 9469d251..056e56bc 100644 --- a/tests/test_tab_isolation.py +++ b/tests/test_tab_isolation.py @@ -120,31 +120,33 @@ async def fake_browser_cdp(method, params=None, timeout=10): # ── 3. Fallback to shared tab when createTarget fails ───────────────── @pytest.mark.asyncio -async def test_connect_falls_back_to_shared_tab(): - """When _browser_cdp raises, connect() falls back to _find_page_ws().""" +async def test_connect_owned_mode_fails_closed_on_tab_creation_failure(): + """When _create_owned_tab fails in owned mode, connect() must NOT fall + back to _find_page_ws. It raises — never steals another process's tab. + + (Previously connect fell back to a shared tab; that was removed because + it could adopt another process's tab, causing two drivers to race on + the same DOM. See ChatGPT design review, conv 6a507b4c.) + """ d = _make_driver() - # Mock _browser_cdp to fail + # Mock _browser_cdp to fail (Target.createTarget fails) async def failing_browser_cdp(method, params=None, timeout=10): raise ConnectionError("browser WS unavailable") d._browser_cdp = failing_browser_cdp - # Mock _find_page_ws to succeed - async def fake_find_page_ws(): - return "ws://fake/shared-tab" - d._find_page_ws = fake_find_page_ws - # No tab to adopt — forces the createTarget path, which fails and falls - # back to shared-tab mode. + # Mock _find_page_ws — it should NOT be called + d._find_page_ws = AsyncMock(return_value="ws://fake/shared-tab") d._adopt_existing_chatgpt_tab = lambda: None - d._wait_for_chatgpt_ready = AsyncMock() # see test_connect_creates_owned_tab + d._wait_for_chatgpt_ready = AsyncMock() d._refresh_token = AsyncMock() mock_connect, fake_ws = _mock_ws_connect() with patch("chatgpt_web2api.cdp_driver.websockets.connect", mock_connect): - await d.connect() + with pytest.raises(RuntimeError, match="shared-tab fallback is disabled in owned mode"): + await d.connect() - assert d._target_id is None # no owned tab — using shared - assert d._owns_target is False - assert d._ws is not None + # _find_page_ws must NOT have been called — no tab theft + d._find_page_ws.assert_not_called() # ── 4. close does NOT call closeTarget without an owned tab ─────────── From 255046b42bc05419f464e633df170538b3e660e7 Mon Sep 17 00:00:00 2001 From: Alajmah <55954113+Alajmah@users.noreply.github.com> Date: Sat, 11 Jul 2026 19:30:18 +0300 Subject: [PATCH 16/25] fix(owned-mode): use OwnedTabRequiredError consistently in connect path ChatGPT review (conv 6a526e19): connect() raised RuntimeError while reconnect() raised OwnedTabRequiredError. Use OwnedTabRequiredError in both paths so callers have one stable failure contract. --- src/chatgpt_web2api/cdp_driver.py | 9 +++++---- tests/test_owned_mode_no_adopt.py | 2 +- tests/test_tab_isolation.py | 2 +- 3 files changed, 7 insertions(+), 6 deletions(-) diff --git a/src/chatgpt_web2api/cdp_driver.py b/src/chatgpt_web2api/cdp_driver.py index e6aa5feb..6a9c6873 100644 --- a/src/chatgpt_web2api/cdp_driver.py +++ b/src/chatgpt_web2api/cdp_driver.py @@ -565,10 +565,11 @@ async def connect(self) -> None: # Owned mode: never adopt an arbitrary tab. The adopt # fallback (_find_page_ws) picks ANY chatgpt.com tab, # which could belong to another process — causing two - # drivers to race on the same tab. Fail closed instead; - # the caller (ensure/connect) will retry or surface the - # error. (ChatGPT design review, conv 6a507b4c.) - raise RuntimeError( + # drivers to race on the same tab. Fail closed with + # OwnedTabRequiredError (consistent with reconnect path) + # so callers have one stable failure contract. + # (ChatGPT design review, conv 6a507b4c + 6a526e19.) + raise OwnedTabRequiredError( f"Owned-tab creation failed and shared-tab fallback " f"is disabled in owned mode: {e}" ) from e diff --git a/tests/test_owned_mode_no_adopt.py b/tests/test_owned_mode_no_adopt.py index aa01cfc0..b72b9606 100644 --- a/tests/test_owned_mode_no_adopt.py +++ b/tests/test_owned_mode_no_adopt.py @@ -45,7 +45,7 @@ async def test_connect_owned_mode_does_not_fall_back_to_find_page_ws(): # Make _create_owned_tab fail driver._create_owned_tab = AsyncMock(side_effect=RuntimeError("Chrome refused")) - with pytest.raises(RuntimeError, match="shared-tab fallback is disabled in owned mode"): + with pytest.raises(Exception, match="shared-tab fallback is disabled in owned mode"): await driver.connect() # _find_page_ws must NOT have been called diff --git a/tests/test_tab_isolation.py b/tests/test_tab_isolation.py index 056e56bc..13c8d360 100644 --- a/tests/test_tab_isolation.py +++ b/tests/test_tab_isolation.py @@ -142,7 +142,7 @@ async def failing_browser_cdp(method, params=None, timeout=10): d._refresh_token = AsyncMock() mock_connect, fake_ws = _mock_ws_connect() with patch("chatgpt_web2api.cdp_driver.websockets.connect", mock_connect): - with pytest.raises(RuntimeError, match="shared-tab fallback is disabled in owned mode"): + with pytest.raises(Exception, match="shared-tab fallback is disabled in owned mode"): await d.connect() # _find_page_ws must NOT have been called — no tab theft From c92df8a5916900bff16c4c506fc170499a56da95 Mon Sep 17 00:00:00 2001 From: Alajmah <55954113+Alajmah@users.noreply.github.com> Date: Sat, 11 Jul 2026 23:41:45 +0300 Subject: [PATCH 17/25] =?UTF-8?q?fix(pool):=20closing-slot=20race=20?= =?UTF-8?q?=E2=80=94=20don't=20discard=20active=5Fkeys=20for=20replacement?= =?UTF-8?q?=20slots?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ChatGPT found this by reading the actual code on GitHub (conv 6a52a623, PR #44 review). The sweeper's cleanup path unconditionally discarded the session key from _active_keys after closing an idle slot — even when a replacement slot for the same session had been created during the close. The race: 1. Idle sweeper marks slot closing=True, starts driver.close() (slow) 2. Same session reacquires → pool creates replacement slot, overwrites _slots[session_key], adds to _active_keys (no-op, key already in set) 3. Old driver.close() finishes → sweeper deletes _slots mapping (identity check correctly skips the replacement) BUT unconditionally discards _active_keys → replacement is now invisible to capacity accounting Consequences: pool cap no longer enforced, orphaned drivers accumulate, close_all() may miss live drivers, status diagnostics underreport. Fix: only discard _active_keys when the closing slot IS still the current mapping (extend the existing identity check to cover _active_keys too). Regression test: pool_size=2, session acquires → goes idle → sweeper starts closing → session reacquires during close → after old close finishes, replacement must still be in _active_keys. 679 tests pass. --- src/chatgpt_web2api/mcp_driver_pool.py | 8 ++- tests/test_pool_closing_slot_race.py | 98 ++++++++++++++++++++++++++ 2 files changed, 105 insertions(+), 1 deletion(-) create mode 100644 tests/test_pool_closing_slot_race.py diff --git a/src/chatgpt_web2api/mcp_driver_pool.py b/src/chatgpt_web2api/mcp_driver_pool.py index 13cb50ca..2d585107 100644 --- a/src/chatgpt_web2api/mcp_driver_pool.py +++ b/src/chatgpt_web2api/mcp_driver_pool.py @@ -597,9 +597,15 @@ async def _sweep_idle(self) -> None: async with slot.meta_lock: slot.in_flight = 0 slot.closing = True + # Only delete the mapping and discard the active key if + # this slot is STILL the current mapping for its session. + # If a replacement was created while we were closing, the + # identity check prevents us from deleting the replacement + # OR discarding the key the replacement needs. + # (ChatGPT review, conv 6a52a623 — closing-slot race.) if self._slots.get(slot.session_key) is slot: del self._slots[slot.session_key] - self._active_keys.discard(slot.session_key) + self._active_keys.discard(slot.session_key) self._capacity_available.notify_all() async def close_all(self) -> None: diff --git a/tests/test_pool_closing_slot_race.py b/tests/test_pool_closing_slot_race.py new file mode 100644 index 00000000..e59f2b26 --- /dev/null +++ b/tests/test_pool_closing_slot_race.py @@ -0,0 +1,98 @@ +"""Regression test: closing-slot replacement race (ChatGPT review, conv 6a52a623). + +When the idle sweeper is closing a slot and the same session makes a new request +before the close finishes, the pool creates a replacement slot for the same +session key. But when the old close finishes, the sweeper unconditionally +discards the session key from _active_keys — even though the replacement is live. + +This test verifies the fix: + 1. The sweeper only discards _active_keys when the closing slot IS still the + current mapping (identity check extends to _active_keys). + 2. A session that reacquires while its old slot is closing does not lose + capacity accounting. +""" + +import asyncio +from unittest.mock import AsyncMock, MagicMock + +import pytest + +from chatgpt_web2api.config import Config +from chatgpt_web2api.mcp_driver_pool import McpSessionDriverPool + + +def _make_pool(max_size=2, ttl=0.01, sweep_interval=0.1): + """Build a pool with a fake driver factory. TTL is very short so the + sweeper closes idle slots almost immediately. Sweep interval is short + so the sweeper runs quickly.""" + cfg = Config() + cfg.chatgpt.mcp_session_pool_size = max_size + cfg.chatgpt.mcp_session_pool_acquire_timeout = 5.0 + cfg.chatgpt.mcp_session_pool_ttl_seconds = ttl + cfg.chatgpt.mcp_session_pool_sweep_interval_seconds = sweep_interval + + close_events = [] + + async def fake_factory(config, transport, port, slot): + driver = MagicMock() + # Make close slow so we can interleave a reacquisition + async def slow_close(): + close_events.append(("close_start", slot.session_key)) + await asyncio.sleep(0.5) + close_events.append(("close_done", slot.session_key)) + + driver.close = slow_close + driver._current_conv_id = None + return driver + + pool = McpSessionDriverPool(cfg, driver_factory=fake_factory) + return pool, close_events + + +@pytest.mark.asyncio +async def test_reacquire_during_close_does_not_lose_capacity(): + """When a session reacquires while its old slot is closing, the replacement + slot must remain in _active_keys after the old close completes. + + ChatGPT found that the sweeper unconditionally discards _active_keys even + when the slot is no longer the current mapping. This test reproduces that + race: with pool_size=2, session A acquires, goes idle (sweeper starts + closing), then reacquires before close finishes. After the old close + completes, session A's replacement must still be in _active_keys. + """ + pool, close_events = _make_pool(max_size=2, ttl=0.01) + + try: + # Session A acquires a slot. + async with pool.acquire("session-A"): + pass # Release immediately → slot goes idle + + # Start the sweeper — it checks every 0.1s, TTL is 0.01s, so the + # first sweep at ~0.1s will mark the slot closing and start slow_close. + await pool.start_sweeper() + + # Wait long enough for the sweeper to mark the slot closing AND start + # the slow driver.close() (which takes 0.5s). + await asyncio.sleep(0.2) + + # At this point the old slot should be closing=True and driver.close() + # should be in progress. Session A reacquires. + async with pool.acquire("session-A") as lease: + assert lease is not None + # While holding the replacement, wait for the old close to finish. + await asyncio.sleep(1.0) + + # After the old close is done, session-A's replacement must still be + # tracked in _active_keys. If the sweeper discarded it unconditionally + # (the bug), _active_keys would be empty and capacity accounting is wrong. + assert "session-A" in pool._active_keys, ( + "Replacement slot lost from _active_keys after old close completed — " + "the sweeper discarded it unconditionally (the race ChatGPT found)" + ) + + # Verify capacity is correct: the replacement should count against the cap. + # A new session should still be able to acquire (pool_size=2, one slot used). + async with pool.acquire("session-B"): + assert "session-B" in pool._active_keys + finally: + await pool.close_all() From b3f8f3b661f82b047417bc1d2844cc773c87986b Mon Sep 17 00:00:00 2001 From: Alajmah <55954113+Alajmah@users.noreply.github.com> Date: Sun, 12 Jul 2026 02:58:15 +0300 Subject: [PATCH 18/25] style: fix lint warnings in new test files (unused imports, sort order) --- tests/test_detector_budgets.py | 1 - tests/test_detector_phase2.py | 2 +- tests/test_navigation_readiness.py | 1 - tests/test_owned_mode_no_adopt.py | 1 - tests/test_pool_closing_slot_race.py | 2 +- tests/test_pool_lease_accounting.py | 4 +--- tests/test_send_readiness.py | 5 ++--- 7 files changed, 5 insertions(+), 11 deletions(-) diff --git a/tests/test_detector_budgets.py b/tests/test_detector_budgets.py index d87217a6..018be006 100644 --- a/tests/test_detector_budgets.py +++ b/tests/test_detector_budgets.py @@ -18,7 +18,6 @@ classify_model, ) - # ── classify_model ────────────────────────────────────────────────────── diff --git a/tests/test_detector_phase2.py b/tests/test_detector_phase2.py index 65cb0d72..99317860 100644 --- a/tests/test_detector_phase2.py +++ b/tests/test_detector_phase2.py @@ -19,9 +19,9 @@ import asyncio import json import time +from unittest.mock import AsyncMock, MagicMock import pytest -from unittest.mock import AsyncMock, MagicMock from chatgpt_web2api.completion_detector import ( CompletionDetector, diff --git a/tests/test_navigation_readiness.py b/tests/test_navigation_readiness.py index 614316f8..851dff1e 100644 --- a/tests/test_navigation_readiness.py +++ b/tests/test_navigation_readiness.py @@ -219,7 +219,6 @@ async def fast_sleep(d): async def test_navigate_waits_for_document_ready(monkeypatch): """When readyState='loading', the poll should wait, not count against the stall budget. This tests the document.readyState check.""" - import time driver = CDPDriver(cdp_port=9222) driver._cdp = AsyncMock() diff --git a/tests/test_owned_mode_no_adopt.py b/tests/test_owned_mode_no_adopt.py index b72b9606..e8b4ccfa 100644 --- a/tests/test_owned_mode_no_adopt.py +++ b/tests/test_owned_mode_no_adopt.py @@ -11,7 +11,6 @@ 3. adopt mode still falls back (the legacy behavior is preserved) """ -import asyncio from unittest.mock import AsyncMock, MagicMock, patch import pytest diff --git a/tests/test_pool_closing_slot_race.py b/tests/test_pool_closing_slot_race.py index e59f2b26..7c707a98 100644 --- a/tests/test_pool_closing_slot_race.py +++ b/tests/test_pool_closing_slot_race.py @@ -13,7 +13,7 @@ """ import asyncio -from unittest.mock import AsyncMock, MagicMock +from unittest.mock import MagicMock import pytest diff --git a/tests/test_pool_lease_accounting.py b/tests/test_pool_lease_accounting.py index 79b7d539..1c443161 100644 --- a/tests/test_pool_lease_accounting.py +++ b/tests/test_pool_lease_accounting.py @@ -12,9 +12,7 @@ 5. status() includes lease accounting summary. """ -import asyncio import logging -import time from unittest.mock import AsyncMock, MagicMock import pytest @@ -52,7 +50,7 @@ async def test_acquire_creates_lease_record(): """Acquiring a lease creates a record with lease_id, session_key, timestamp.""" pool = _make_pool(max_size=2) try: - async with pool.acquire("session-1") as lease: + async with pool.acquire("session-1"): # The pool should have exactly one active lease record. assert len(pool.active_leases) == 1, ( f"expected 1 active lease, got {len(pool.active_leases)}" diff --git a/tests/test_send_readiness.py b/tests/test_send_readiness.py index 6cd82b12..ef6500e2 100644 --- a/tests/test_send_readiness.py +++ b/tests/test_send_readiness.py @@ -15,13 +15,12 @@ import asyncio import json import logging -import time as _time -from unittest.mock import AsyncMock, MagicMock +from unittest.mock import AsyncMock import pytest -from chatgpt_web2api.chatgpt_dom import ChatGPTDom from chatgpt_web2api.cdp_driver import CDPDriver, SendReadinessError +from chatgpt_web2api.chatgpt_dom import ChatGPTDom @pytest.fixture(autouse=True) From 22840eae49c1e03ecb6163e55f4bf311e19590f4 Mon Sep 17 00:00:00 2001 From: Alajmah <55954113+Alajmah@users.noreply.github.com> Date: Sun, 12 Jul 2026 04:02:25 +0300 Subject: [PATCH 19/25] style: fix unused imports in source files (ruff) --- src/chatgpt_web2api/cdp_driver.py | 1 - src/chatgpt_web2api/mcp_driver_pool.py | 2 +- 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/src/chatgpt_web2api/cdp_driver.py b/src/chatgpt_web2api/cdp_driver.py index 6a9c6873..51010803 100644 --- a/src/chatgpt_web2api/cdp_driver.py +++ b/src/chatgpt_web2api/cdp_driver.py @@ -84,7 +84,6 @@ class StreamChunk: SEND_BUTTON_SELECTOR, ) - # ── P2: Navigation readiness probe ──────────────────────────────────────── # # Co-designed with ChatGPT (vision-alignment cycle, conversation 6a4ebb2a). diff --git a/src/chatgpt_web2api/mcp_driver_pool.py b/src/chatgpt_web2api/mcp_driver_pool.py index 2d585107..041e28f8 100644 --- a/src/chatgpt_web2api/mcp_driver_pool.py +++ b/src/chatgpt_web2api/mcp_driver_pool.py @@ -23,7 +23,7 @@ import time import uuid from collections.abc import Awaitable, Callable -from dataclasses import dataclass, field +from dataclasses import dataclass from typing import Any logger = logging.getLogger(__name__) From 5a2869bacc2831374c7b3b0f6b4dab7dd5691c46 Mon Sep 17 00:00:00 2001 From: Alajmah <55954113+Alajmah@users.noreply.github.com> Date: Sun, 12 Jul 2026 07:14:22 +0300 Subject: [PATCH 20/25] =?UTF-8?q?fix(composer):=20P0=20text=20verification?= =?UTF-8?q?=20round-trip=20=E2=80=94=20recursive=20extractor,=20NFC,=20tra?= =?UTF-8?q?iling=20newline?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ChatGPT code review (conv 6a52f0f3, reading actual code from GitHub) found 4 defects in _verify_composer_text. All fixed: 1. Recursive DOM extractor (high confidence): replaced child.textContent join with a recursive node walker that emits \n for
elements. The old code lost line breaks inside

tags (

line1
line2

→ 'line1line2'). Now correctly extracts 'line1\nline2'. 2. NFC normalization (confirmed defect): added unicodedata.normalize('NFC', ...) to both actual and expected text. Composed/decomposed Unicode sequences (e.g., é as 'e'+U+0301 vs U+00E9) now match correctly. Brings the verifier to parity with the turn-anchor matcher which already uses NFC. 3. No unconditional trailing-newline strip (definite correctness bug): removed the JS-side 'joined.replace(/\n$/, '')' that corrupted prompts ending in \n. The Python-side tolerance (accept actual == expected OR actual == expected+\n) handles editor-added trailing newlines without stripping user content. 4. (Polling improvement deferred to type_message — the verifier itself is a single-shot read; the polling belongs in the caller's retry loop.) 6 new tests covering:
inside

, multiline blocks, NFC combining accents, trailing newline, em-dashes and curly quotes, standard multiline. 684 tests pass. --- src/chatgpt_web2api/chatgpt_dom.py | 86 ++++++++------- tests/test_composer_verification.py | 161 ++++++++++++++++++++++++++++ 2 files changed, 209 insertions(+), 38 deletions(-) create mode 100644 tests/test_composer_verification.py diff --git a/src/chatgpt_web2api/chatgpt_dom.py b/src/chatgpt_web2api/chatgpt_dom.py index 67a6eeb0..e665e930 100644 --- a/src/chatgpt_web2api/chatgpt_dom.py +++ b/src/chatgpt_web2api/chatgpt_dom.py @@ -52,6 +52,7 @@ import json import logging import time +import unicodedata from .breakers import BreakerKind @@ -328,24 +329,25 @@ async def _detect_select_all_modifier(self) -> int: async def _verify_composer_text(self, selector: str, expected: str) -> bool: """Canonical-equality check: does the composer hold *expected*? - Compares editor-visible text with canonical normalization (CRLF→LF, - NBSP→space, tolerate a trailing block newline ProseMirror adds) — but - does NOT broadly collapse internal whitespace, since that would hide - real corruption of code/YAML/Markdown indentation. + Compares editor-visible text with canonical normalization (NFC, CRLF→LF, + NBSP→space) and tolerates at most one editor-added trailing newline. For a contenteditable ProseMirror div, neither ``innerText`` nor - ``textContent`` reconstructs what the user typed across line breaks: - ``\\n`` in the input becomes a new ``

`` block, and ProseMirror - renders blank-line paragraphs as ``


``. ``innerText`` then - emits *several* newlines per block boundary (measured: a 2-newline - input read back as 5), while ``textContent`` joins blocks with - *nothing* (0 newlines). Both fail canonical equality for any - multi-line prompt. The fix is a block-aware extractor: join each - top-level block child's text with a single ``\\n``, and treat an - empty block (the ``
``-only paragraph from a blank line) as one - newline. This reconstructs the typed text faithfully for both - single-line and multi-line input. Legacy ``