Repository navigation
Reliability session: stall detector, navigation diagnostics, tab isolation, recovery hook - #44
Merged
Merged
Conversation
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.
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.
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.
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).
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.
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).
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.
…iagnostic 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.
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).
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).
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.
…older race) 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).
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.
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.
…s 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 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.
ChatGPT review (conv 6a526e19): connect() raised RuntimeError while reconnect() raised OwnedTabRequiredError. Use OwnedTabRequiredError in both paths so callers have one stable failure contract.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 255046b42b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+634
to
+636
| if html_len > 50 and not first_content_seen: | ||
| first_content_seen = True | ||
| last_change_time = time.monotonic() # reset stream-idle clock |
There was a problem hiding this comment.
For reasoning turns that show only the initial assistant wrapper/thinking UI, innerHTML.length can already exceed 50 before any answer text or real non-text artifact exists (this method even documents that the first poll can have wrapper html_len > 50). This line therefore flips first_content_seen immediately and moves the turn from the 300s reasoning first-content budget to the shorter stream-idle budget while it is still thinking, so long silent reasoning responses can still be reported as stream-idle stalls before first content appears.
Useful? React with 👍 / 👎.
Comment on lines
1721
to
+1724
| timeout=timeout, | ||
| turn_anchor=turn_anchor, | ||
| budgets=budgets, | ||
| model=model, |
There was a problem hiding this comment.
When REST or MCP uses the defaults, reasoning budgets are now passed with first_content_timeout_seconds=300, but the overall timeout reaching this call is still 120s (ServerConfig.request_timeout and MCP's fixed timeout). stream_until_complete uses that timeout as its phase-2 loop deadline, so a reasoning turn that legitimately takes more than 120s before first content stops being observed before the new 300s budget can ever apply, leading to early finalization/error despite the configured detector budget.
Useful? React with 👍 / 👎.
…ent slots 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.
… NFC, trailing newline
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 <br> elements. The old code
lost line breaks inside <p> tags (<p>line1<br>line2</p> → '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: <br> inside <p>, multiline blocks, NFC combining accents,
trailing newline, em-dashes and curly quotes, standard multiline.
684 tests pass.
…e extractor tests ChatGPT review (conv 6a52f0f3) found 2 merge blockers by reading the actual diff: 1. Blank paragraph regression: <p><br></p> (placeholder) emitted \n from the <br> AND \n from the block join = 3 newlines instead of 2. Fix: detect placeholder <br>-only blocks (isPlaceholderBreakBlock) and treat them as empty string. Now: <p>a</p><p><br></p><p>b</p> correctly gives 'a\n\nb'. 2. Tests didn't execute the extractor: all tests mocked _js_strict to return pre-extracted Python strings. The tests proved the comparator, not the JS extraction logic. Fix: added a Python reimplementation of the JS DOM walker (_extract_composer) with realistic ProseMirror DOM shapes: - <p>line1<br>line2</p> → 'line1\nline2' - <p>a</p><p><br></p><p>b</p> → 'a\n\nb' - <p>hello</p><p><br></p> → 'hello\n' - <p>a</p><p><br></p><p><br></p><p>b</p> → 'a\n\n\nb' - <p><span>line1</span><br><strong>line2</strong></p> → 'line1\nline2' - <p>hello</p> → 'hello' Also: documented scope — the extractor handles root-level <p>/<div> blocks with inline descendants (what Input.insertText produces). Does NOT reconstruct nested block boundaries (blockquote, lists). Documented as a known limitation. Removed the no-op polling test (ChatGPT finding 2.3). 687 tests pass.
ChatGPT code review (conv 6a52f0f3) found two defects in the send path: 1. 'Events dispatched' treated as 'message accepted': click_send dispatches synthetic mouse events and returns 'sent' — proving only that the JS ran, not that React accepted the submission. Under load, the click fires but no message is sent. The bridge silently enters completion detection which finds nothing → reconciliation failure. Fix: added _verify_send_acknowledged() — polls for 3s after click_send to check if the user-message DOM count increased AND composer cleared. If UUID was captured, the check is skipped (UUID is sufficient acknowledgment). Returns True (acknowledged), False (not acknowledged → SendReadinessError), or None (probe inconclusive → non-blocking, proceed to completion detection). The None path is important: in mock/test environments where _js_strict returns unexpected shapes, the probe is inconclusive rather than blocking the send. The check is a safety net for the overloaded-page case, not a hard gate that could prevent valid sends. 2. TurnReconciliationError discards the underlying fetch_failed diagnostic: the actual error (CDP timeout, destroyed context, HTTP error) was lost, leaving only 'last_status=fetch_failed'. Fix: track last_diagnostic from _fetch_text_for_turn and include it as 'last_fetch_diagnostic' in the TurnReconciliationError diagnostic dict. 3 new tests: send not acknowledged raises, send acknowledged proceeds, diagnostic preservation in TurnReconciliationError. 690 tests pass.
…diagnostic string ChatGPT review (conv 6a52f0f3) found 3 merge blockers in send-acknowledgment. All fixed: A. No pre-send user-count baseline: the check did userCount > 0, which is always true on existing conversations. Fix: _read_assistant_count_baseline now stores _pre_send_user_count; the acknowledgment probe uses delta comparison (current > pre_send). B. CDP probe failures returned False instead of None: under the exact overload condition this feature targets, all _js_strict calls fail and the probe returns False — blocking a send that actually landed. Fix: track valid_probe_seen; return None when no valid probe was obtained. C. Missing composer treated as empty: composer ? ... : true meant no composer = 'cleared'. Fix: separate composerPresent field; missing composer is inconclusive (continue polling), not acknowledged. Also: TurnReconciliationError.__str__ now includes the fetch error in the message string so agents see it (they only get str(exc) via the API, not the .diagnostic dict). 690 tests pass.
…sent check ChatGPT re-review (conv 6a52f0f3) found one remaining blocker: valid_probe_seen was set BEFORE the composerPresent check. When the composer was missing on every poll (navigation, selector drift), valid_probe_seen was True, so the method returned False (blocking) instead of None (inconclusive). Fix: move valid_probe_seen = True below the composerPresent check. A poll with a missing composer is inconclusive — it doesn't count as a valid probe. Added regression test: missing-composer-on-every-poll returns None.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
17 commits addressing bridge reliability across multi-session usage.
Fixes
Test coverage
678 unit tests (was 628). All passing.
Co-development
Designed and reviewed with ChatGPT across 4 review cycles. 7 bugs caught in review.