Repository navigation
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.
…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.
…attempt Red evidence (offline, deterministic): through click_send's real path (_js -> _cdp), an ambiguous socket death re-sent the mutating Runtime.evaluate after one reconnect (2 frames, ids [1,2]). The outer retry_on_rate_limit wrapper re-invoked the full mutating chat operation on RateLimitError at both the REST non-stream seam and the MCP chat dispatch sites. The seam: - CDPTransport._js_mutation: mutating Runtime.evaluate with _retry=False — never replayed; reconnect-class failures and post-dispatch timeouts raise SendOutcomeUnknownError (reconcile-only, retry_safe=False). _js/_js_strict remain replayable observation primitives. - click_send dispatches the click via _js_mutation; readiness poll stays on _js (read). - COMPOSER_SEND_READINESS success boundary moved from the click's JS return (+141ms, proves only synthetic events ran) to submission evidence in send_and_stream: captured UUID (primary, +139ms) or DOM acknowledgment fallback (1B timeline, 2026-09-21). - On the ambiguous path, send_and_stream preserves the in-flight IdentityListener capture (AMBIGUOUS_CAPTURE_WINDOW_S=1.0) onto the error's captured_user_id BEFORE the capture scope closes — evidence preservation, not reconciliation. - single_send opt-in (1 attempt): REST body/metadata; MCP arguments, scoped via _SINGLE_SEND_TOOLS to chat_completion/chat_with_gpt only. Claim: opt-in at-most-one automatic USER-send mutation attempt across transport reconnect ambiguity and outer rate-limit retry; ambiguous outcomes surface explicitly. NOT exactly-once; NOT auto-reconciled. Tests: 18 new (test_no_replay_send, test_single_send_propagation, test_send_outcome_unknown) + harness updates. Full suite 709 passed, 1 pre-existing master failure (test_config_default_parallel_tabs_false, verified by stash). ruff: 10 errors vs 11 master baseline, 0 new. Evidence trail: docs/research/issue51-no-replay-implementation.md, docs/research/chatgpt-web-contract-2026-09.md, experiments 1A/1B.
…sses Phase 0 upstream reconnaissance (2026-09-20/21): - chatgpt-web-contract-2026-09.md — frozen passive delta report + post-freeze addendum (execution order, provisional invariants, experiment results). Key deltas: sectioned loading confirmed (314 backend nodes vs 5-6 mounted), send path now POST /f/conversation, model/effort restructure (GPT-5.6, gpt-5-5 departs 2026-10-14), data-message-model-slug signal, User-message identity confirmed (UUID == backend node id == message id). - experiment-1a-mounting-race.md — 5/5 cold loads: atomic hydration, mount race not observed (scoped), ~6s readyState-to-hydration dead zone. - experiment-1b-send-timeline.md — controlled send: outcome A (UUID +139ms beats DOM ack +437ms), backend skew ~-40ms (June +5.1s constant stale), thinking/streaming classes absent in 1/1 run. - scripts/phase0_*.py — passive recon, 1A mounting-race, 1B instrumentation harnesses (re-runnable; passive/owned-tab safety contracts in docstrings). - .gitignore: captures/ (raw local evidence contains conversation ids).
…pre-push review) First-pass review findings over the ten agreed boundaries. Three mapping gaps found and fixed; the rest verified (inspection or tests): - REST non-stream: SendOutcomeUnknownError fell to the default 500 server_error — the status class OpenAI SDKs auto-retry, inviting a client-side replay of an ambiguous send. Now HTTP 409 with code=send_outcome_unknown, retry_safe=false, captured_user_id when preserved. Caveat documented: some SDKs retry 409 anyway; the code field is the stop signal. - REST streaming: SSE status locks at 200 before the send, so the error surfaced through the generic except as a bare message. Now an explicit inline [Error: send_outcome_unknown ...] marker chunk (same precedent as the mid-stream rate-limit marker), including captured_user_id. - MCP: _map_tool_exception returned None for the new error (fell through to a re-raise, losing the code). Now a structured isError result with the code, preserved UUID, and do-not-resend guidance. Verified without changes: dispatchEvent exists at exactly one code site (the click, on _js_mutation); all USER sends funnel through send_and_stream; the streaming path consumes the mutating generator once with only a read-only preflight retry; classification converts only reconnect-class failures and post-dispatch timeouts (safe false-positive direction); cancellation during the 1s evidence window propagates CancelledError (BaseException) and still closes the capture scope (now tested); per-send scope matching (text hash + conversation + target + sequence, armed inside the MutationLock) prevents cross-send UUID contamination; default mode documented as legacy three-attempt outer retry, not safe at-most-once. Tests: +4 (REST 409 mapping with/without evidence, MCP structured error, cancellation-during-window). Full suite 713 passed / 1 pre-existing master failure. ruff 10 = baseline, 0 new.
Post-certification review state — 2026-09-21Independent delta re-review passed, and the authorized live ambiguous-write certification has now passed on head Live result: one mutating Policy decision for this PR: keep PR #52 is therefore ready for maintainer review. Issue #51 should remain open until this implementation is merged; after merge, its requested narrow capability is satisfied. Automatic UNKNOWN→CONFIRMED reconciliation remains intentionally out of scope. |
![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: d1e2bd30aa
ℹ️ 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".
| timeout=timeout, | ||
| _retry=False, | ||
| ) |
There was a problem hiding this comment.
When an MCP cancellation or request-task cancellation arrives after the WebSocket has accepted this Runtime.evaluate frame but before its response is received, _cdp raises asyncio.CancelledError. On Python 3.11 that inherits directly from BaseException, so neither exception handler below converts it to SendOutcomeUnknownError; the capture scope is closed and the caller receives no do-not-resend signal even though the click may already have executed. This recreates the duplicate-turn risk if the cancelled operation is retried, so cancellation during this await needs the same ambiguity handling while still respecting task cancellation.
Useful? React with 👍 / 👎.
| if self._breakers and (captured_uuid or acknowledged is True): | ||
| self._breakers.record_success(BreakerKind.COMPOSER_SEND_READINESS) |
There was a problem hiding this comment.
When UUID capture is unavailable and _verify_send_acknowledged() returns None (its documented non-blocking path for CDP errors or a transiently missing composer), this condition never records success even if stream_until_complete subsequently observes and reconciles a complete assistant turn. Consequently, earlier readiness failures remain in the rolling window, and a later isolated failure can trip COMPOSER_SEND_READINESS despite successful sends in between. Record success once downstream completion provides definitive submission evidence as a fallback for this inconclusive-probe path.
Useful? React with 👍 / 👎.
|
C:/Program Files/Git/gitwire run review |
|
|
![gitwire-hq[bot]](https://avatars.githubusercontent.com/in/3727207?s=60&v=4){kind=small}
There was a problem hiding this comment.
🤖 AI Code Review — ⚠️ Review incomplete
Confidence: low · Findings: 9
AI judgment: NEEDS DISCUSSION · Evidence: INCOMPLETE · Authority: advisory
Evidence incomplete · 15 of 25 changed files lack complete review evidence (limits reached: max_lines_to_review)
INCOMPLETE — no clean approval was issued. GitWire could not completely review this change: 15 of 25 changed files lacked complete review evidence (limits reached: max_lines_to_review).
Other findings (9)
- [low] Experiment 1B recorder catches builtin TimeoutError, which does not match asyncio.wait_for on Python <=3.10
- [low] live-cert get_response_body calls Network.getResponseBody on a fresh CDP session that never enabled Network
- [low] reconnect_calls counter derives from total websocket connections and mislabels initial connections as reconnects
- [low] Experiment 1B tab discovery requires an uncommitted pre-start snapshot file and can attach to the wrong tab
- [low] live-cert leftover-tab cleanup force-closes any new chatgpt.com tab, not only the experiment's owned tab
🔮 Devil's Advocate (2 turns): 4 missed risks found · 5 upheld
GitWire AI Review Gate (bundle-driven v2) · Structured schema · Scope-validated · Devil's Advocate · AI recommendation — maintainers and repository policy retain authority
Finding details (not annotatable inline)
-
[MEDIUM] Single-send latch may be consumed-on-attempt or reset-on-reconnect, bricking or re-enabling sends incorrectly
The PR's headline feature (opt-in single-send semantics) received zero findings — the review scrutinized only the phase0 harness scripts. The classic failure modes for a single-send guard went unexamined: (a) if the 'sent' latch trips on attempt rather than on confirmed success, any transient failure (network blip, tab reload mid-send) permanently blocks retries for that message; (b) if latch state lives in tab/connection-scoped state, a websocket reconnect or page reload silently resets it, reintroducing the double-send the feature exists to prevent. Both are exactly the regressions this PR could introduce and neither was checked.
-
[LOW] No-replay mutation payload's interaction with model context and prompt assertions is unaddressed
'No-replay send mutations' implies each send is altered (nonce/counter/marker) so duplicate deliveries are distinguishable. The review never asked where that mutation goes: if it reaches ChatGPT's context, every request carries synthetic text that can degrade responses and break any cert/step asserting on sent-content equality; if it's stripped client-side, the stripping logic itself is a new failure point. Either way it's an unexamined surface on the core change.
-
[MEDIUM] Five-for-five findings in harness scripts, zero in the production send path — suspicious review asymmetry
Every finding targets scripts/phase0_*; nothing addresses the bridge code the PR title says changed. Either the core mutation/single-send logic is trivially small and perfect (unlikely to be verifiable from a review that produced five detailed script findings) or the reviewer never audited it. As a minimum, the lack of referenced unit tests for the new semantics should have been flagged as a test gap.
-
[LOW] Certification record is only trustworthy after manual doc corrections, undermining the harness's purpose
Findings #1-#3 collectively show the committed cert tool emits wrong or missing data (bogus reconnect count, dead conv_id lookup) that humans patched in prose. No finding called out the systemic consequence: any future automated consumer of the harness JSON — or any reader trusting it over the doc — inherits falsified certification evidence. The individually-low findings compound into a process-level risk the review left implicit.
| while time.monotonic() < stop_at: | ||
| try: | ||
| raw = await asyncio.wait_for(ws.recv(), timeout=0.5) | ||
| except TimeoutError: |
There was a problem hiding this comment.
[LOW] Experiment 1B recorder catches builtin TimeoutError, which does not match asyncio.wait_for on Python <=3.10
In network_recorder(), raw = await asyncio.wait_for(ws.recv(), timeout=0.5) is guarded by except TimeoutError: continue. Before Python 3.11, asyncio.TimeoutError is a distinct class from the builtin TimeoutError (which derives from OSError), so every 0.5s idle gap raises an uncaught exception that kills the recorder task; since asyncio.gather() is called without return_exceptions=True, the whole run aborts. The sibling scripts handle this correctly (phase0_live_cert_51.py uses except asyncio.TimeoutError, phase0_experiment_1a.py uses except Exception), so this looks like an oversight rather than a supported-floor decision. Fix: catch asyncio.TimeoutError, which is correct on all supported versions.
| "rid": p.get("requestId")}) | ||
|
|
||
|
|
||
| async def get_response_body(ws_url: str, rid: str) -> dict: |
There was a problem hiding this comment.
[LOW] live-cert get_response_body calls Network.getResponseBody on a fresh CDP session that never enabled Network
get_response_body() opens a brand-new WebSocket session to the tab and issues Network.getResponseBody with a requestId that was captured by the separate observer session. Chrome buffers response bodies per-session (only after Network.enable on that session), so a fresh session deterministically returns 'No resource with given identifier found'; the method then returns {'_raw': ''} and conv_id resolution fails, skipping the in-run projection lookup. The certification doc itself records this outcome ('the in-run lookup could not resolve the conversation id'), confirming the defect. Fix: issue Network.getResponseBody on the observer session (e.g., before obs.cancel(), reusing the session that recorded requestWillBeSent), or otherwise derive the conversation id within a session that enabled Network.
| "mutating_Runtime_evaluate_frames": STATE["mutating_frames"], | ||
| "fault_injected": STATE["injected"], | ||
| "websocket_connections_created": STATE["ws_connections"], | ||
| "reconnect_calls": max(0, STATE["ws_connections"] - 1), |
There was a problem hiding this comment.
[LOW] reconnect_calls counter derives from total websocket connections and mislabels initial connections as reconnects
The result dict computes "reconnect_calls": max(0, STATE["ws_connections"] - 1), assuming exactly one non-reconnect websockets.connect call. In the recorded run there were four (driver page socket, browser-level startup sockets, harness observer — as the cert doc notes), so the JSON counter would report 3 'reconnects' while the true driver reconnect count was 0, and the doc had to correct the number manually from logs. Because this harness is committed as the re-runnable certification tool, future runs will keep emitting a misleading key. Fix: count reconnects from the driver's actual reconnect hook or log line, or rename the key (e.g., connections_created_minus_one) so it is not read as a reconnect count.
|
|
||
|
|
||
| def find_experiment_tab() -> str: | ||
| pre = set((OUT_DIR / "pre-1b-tabids.txt").read_text().split()) |
There was a problem hiding this comment.
[LOW] Experiment 1B tab discovery requires an uncommitted pre-start snapshot file and can attach to the wrong tab
find_experiment_tab() reads captures/phase0/pre-1b-tabids.txt, which no committed script creates; on a fresh checkout the harness dies with FileNotFoundError before any work. The fallback heuristic then selects the first tab that is new since the snapshot and has chatgpt.com in its URL — if any other chatgpt.com tab appears concurrently (e.g., created by the live bridge on 8080, which shares Chrome 9222 per the docs), observers attach to and record from the wrong tab, silently invalidating the timeline. Fix: generate the pre-start id list at script startup (one urllib call to /json/list before the bridge starts) or prefer the experiment bridge's owned target id, as phase0_live_cert_51.py already does via service._driver._target_id.
| print(f"service stop: {e}") | ||
| # close leftover owned tab if the service did not | ||
| now = tab_ids() | ||
| leftover = [tid for tid in now if tid not in pre and "chatgpt.com" in now[tid].get("url", "")] |
There was a problem hiding this comment.
[LOW] live-cert leftover-tab cleanup force-closes any new chatgpt.com tab, not only the experiment's owned tab
The end-of-run cleanup closes every target id absent from the pre-run snapshot whose URL contains chatgpt.com. If any non-experiment chatgpt.com tab is opened during the run window (by the operator or by the live bridge sharing Chrome 9222), it will be force-closed, contradicting the script's stated safety contract that user tabs are never touched. Experiment 1A's OwnedTab, which scopes creation and close to a single positively owned target id, is the correct pattern. Fix: close only service._driver._target_id (captured when the tab is identified) rather than diffing the whole target list.
…wnstream-success fallback Codex P1 (blocker): cancelling DURING the mutating Runtime.evaluate escaped as raw asyncio.CancelledError — the frame may already have reached Chrome, but the operation lost the UNKNOWN/do-not-resend classification. Deliberate design, not a one-line catch: CancelledError propagates UNCHANGED (swallowing/converting it would break the asyncio cancellation contract and turn client-disconnect cancellations into 409 responses), and the classification lands on an observable driver side channel: CDPDriver.note_send_outcome_unknown() sets last_send_outcome_unknown (cleared at each send_and_stream start) and logs a structured code=send_outcome_unknown ERROR. No evidence-window wait on this path — cancellation must not be delayed; the capture scope still closes in send_and_stream's finally. Tests: cancellation during mutation → CancelledError propagates, note called with stage=send_mutation, 1 frame, 0 reconnects; cancellation during a READ (_js) → no note (scope check). Codex P2 (fix-before-merge): with no UUID and an inconclusive DOM ack probe, a subsequently COMPLETED turn never cleared COMPOSER_SEND_READINESS failures — the breaker could trip despite interleaved successes. Downstream-success fallback added after stream COMPLETION only (a raised stream is not evidence). Tests: inconclusive + completed turn records success exactly once (and clears the stale side-channel record); UUID path does not double-record; failed stream records nothing. Streaming regression (GitWire thread, finding not valid as stated): _stream_response retries only the read-only rate-limit preflight and consumes send_and_stream exactly once — pinned behaviorally, including the disproving property: a RateLimitError AFTER the generator started does not restart it (one business-send invocation; inline SSE marker is the channel). Suite: 720 passed / 1 pre-existing master failure. ruff: clean.
…, honest counters, owned-tab safety - live_cert: Network.getResponseBody was issued from a NEW CDP session, which had neither Network.enable nor the tracked request — the actual cause of the run's unresolved conv_id. The observer is now a class whose single live session both tracks events and serves command RPCs (getResponseBody rides the same socket). - live_cert: reconnect_calls was derived from total websockets.connect creations, conflating the harness observer and browser-level startup sockets. The driver's reconnect is now wrapped directly; the socket count is kept as explicitly informational. - live_cert: cleanup closed ANY new ChatGPT tab — a safety-contract violation. It now closes only the experiment driver's positively owned _target_id. - exp1b: tab identification no longer depends on the non-committed pre-1b-tabids.txt snapshot (which could select a wrong tab); it derives the owned tab id from the bridge's own log line (Created/Reclaimed owned tab: <ID>) and requires that exact target. - Not acted on: Python <=3.10 TimeoutError note (project requires >=3.11); GitWire single-send-latch / nonce concerns (no latch or nonce exists in this design); committed-ID hygiene in research docs acknowledged as low-severity debt.
|
Reply to the GitWire streaming The finding is not valid as stated. The streaming REST path never wraps the mutating send in Pinned behaviorally in commit
The 'single-send latch' and 'mutated payload/nonce' comments also don't map to this implementation — there is no persistent latch and no nonce injected into prompt content; the causal identity is the outgoing client-message UUID captured by the IdentityListener (certified live: captured UUID == backend node/message id). |
|
/gitwire run review |
|
|
There was a problem hiding this comment.
🤖 AI Code Review — ⚠️ Review incomplete
Confidence: low · Findings: 5
AI judgment: NEEDS DISCUSSION · Evidence: INCOMPLETE · Authority: advisory
Evidence incomplete · 15 of 25 changed files lack complete review evidence (limits reached: max_lines_to_review)
INCOMPLETE — no clean approval was issued. GitWire could not completely review this change: 15 of 25 changed files lacked complete review evidence (limits reached: max_lines_to_review).
Other findings (5)
- [low] Experiment 1B controller's early-stop is dead code — recorders always run the full 300 s
- [low] Author's private project/conversation IDs hardcoded in committed harness and research docs
- [medium] No-replay/single-send dedupe logic in the core send path was never assessed
⚠️ unverified - [medium] No evidence of test coverage for the new opt-in semantics
⚠️ unverified - [low] Harness reproducibility depends on a single live conversation URL
⚠️ unverified
🔮 Devil's Advocate (2 turns): 1 downgraded · 3 missed risks found · 1 upheld
GitWire AI Review Gate (bundle-driven v2) · Structured schema · Scope-validated · Devil's Advocate · AI recommendation — maintainers and repository policy retain authority
Finding details (not annotatable inline)
-
[MEDIUM] No-replay/single-send dedupe logic in the core send path was never assessed
The review spent both findings on phase0 harness scripts and ignored the PR's actual feature: send-mutation semantics. Dedupe/single-send state is classic check-then-act territory. If the 'already sent' marker is keyed too broadly (e.g., content hash without conversation/time scoping), a user who intentionally sends identical text twice gets silently suppressed — message loss. If two requests race, both can pass the check and double-send, which is exactly the replay the flag exists to prevent. Neither the key design nor concurrency behavior was examined.
-
[MEDIUM] No evidence of test coverage for the new opt-in semantics
Neither finding references tests exercising single-send or no-replay paths. Because these semantics gate message delivery (they can drop or duplicate user messages), regressions here are silent and only observable as corrupted conversations. If tests exist outside the diff this weakens considerably, but a review of a semantics-changing flag PR should have confirmed coverage either way.
-
[LOW] Harness reproducibility depends on a single live conversation URL
Adjacent to Finding #1 but distinct: hardcoding CONV_URL to one specific private conversation means phase0_experiment_1a.py breaks permanently if that conversation is deleted or archived, and no one else can re-run the experiment against their own account. The privacy discussion overshadowed the fact that the harness is a one-shot artifact rather than a reusable tool — the same env/argv parameterization fix addresses both.
| stop_at_holder = {"until": t_start + REST_TIMEOUT_S + 60} | ||
|
|
||
| async def net_task() -> None: | ||
| await network_recorder(ws_url, net_events, stop_at_holder["until"]) |
There was a problem hiding this comment.
[LOW] Experiment 1B controller's early-stop is dead code — recorders always run the full 300 s
stop_at_holder["until"] is read once when net_task/dom_task start and passed to network_recorder/dom_sampler as a plain float parameter, so controller()'s later stop_at_holder["until"] = time.monotonic() never reaches the already-running loops. Every invocation therefore records until t_start+300 s regardless of REST completion (~20 s), making GRACE_AFTER_REST_S and the '# stop loops' intent ineffective: the script hangs ~5 minutes after the response and the output JSON accumulates ~4.5 minutes of post-run network/DOM noise that a future reader of the committed harness could mistake for experiment evidence. Fix: pass the holder dict (or an asyncio.Event) and read it each loop iteration, or cancel the recorder tasks after observers_done as phase0_live_cert_51.py already does with obs.cancel().
| import websockets | ||
|
|
||
| CDP = "http://127.0.0.1:9222" | ||
| CONV_URL = ("https://chatgpt.com/g/g-p-6a3a9ddf947481918dcd9f9bbc80017e-" |
There was a problem hiding this comment.
[LOW] Author's private project/conversation IDs hardcoded in committed harness and research docs
scripts/phase0_experiment_1a.py hardcodes the full URL of the author's private project conversation (project id g-p-6a3a9ddf…, conversation id 6a8149d3…), and the committed research docs contain further conversation, message, and sentinel ids (6aaa4c07, 6ab0dff4, b2de19c3-f641-47cd-a4f8-1d7b2ecc473b, 8f471fb2-e658-4313-b8c7-26ee966a19f5). This contradicts the .gitignore change in the same PR, whose comment declares conversation ids/titles 'local only', and the issue51 doc itself classifies this as 'acknowledged low-severity debt'. The ids grant no access without the account's bearer token, so the impact is privacy hygiene rather than credential exposure. Suggestion: parameterize CONV_URL via env/argv in the harness and shorten or strip ids from the docs before merge, matching the repo's own stated intent.
|
/gitwire run review |
|
|
|
/gitwire run review |
|
|
|
✅ GitWire: A review is already published for this head — no new review was posted. Outcome: INCOMPLETE. |
Round-2 review finding (valid, red-proven before the fix): _cdp inserts d._pending[mid] = fut, but CancelledError bypasses both the send-error handler and the TimeoutError handler, so cancelling during the mutating evaluate (the path #51 newly supports) or during any response wait left the entry behind indefinitely on a dead/hanging socket. Not a replay hazard — a state/resource leak on the supported cancellation path. Fix: explicit except asyncio.CancelledError at both await points pops the entry and re-raises unchanged (cancellation contract preserved; the _js_mutation UNKNOWN side-channel behavior is untouched). Both cancellation tests now assert driver._pending == {} afterward — verified red against the previous code, green after the fix. Transport suites (test_cdp_transport, test_cdp_foundation) unchanged and green.
…d at run time - Experiment 1B early-stop (GitWire current-head finding, valid): stop_at_holder['until'] was evaluated and passed to the recorders as a plain float, so the controller's later dict update could not affect the running loops — observers ran to the full REST_TIMEOUT+60 backstop instead of stopping after the post-REST grace period. Now a shared asyncio.Event stops both recorders; the absolute deadline remains as a backstop. This also explains the original run's ~5-minute duration. - Evidence provenance (round-2 review): phase0_live_cert_51.py hard-coded code_head '162c80c ...' and exp1b hard-coded a working-tree description — reruns at newer heads would have produced evidence falsely labeled as an older tree. Both now derive branch@head + dirty state from git at run time (_git_describe), so evidence carries the revision it actually ran against.
…op, run-time provenance)
…ails honestly Round-2 LOW evidence-integrity finding: _git_describe ran git without check=True and against the process CWD, so running outside any repository could produce a misleading 'detached@ (clean)' label, and invoking the harness from inside some OTHER repository would record that repo's branch/SHA — undercutting the provenance property the previous commit established. Fix in both harnesses: git is anchored at THIS repository (repo root derived from the script's own path, passed via git -C), every command must succeed (check=True), an empty head is rejected, and genuine failures return 'git-unavailable'. The dirty flag is documented as TRACKED working-tree state only (--untracked-files=no). Behaviorally verified: from a foreign non-repo CWD both helpers now report this repo's branch@head; a copy of the script outside any repository returns git-unavailable (the old exp1b implementation demonstrated the lying 'detached@ (clean)' label in the same test).
|
@codex review |
|
/gitwire run review |
|
ℹ️ GitWire: AI Review is disabled by repository policy. |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 929baa491f
ℹ️ 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".
| body = json.dumps({ | ||
| "model": "auto", | ||
| "stream": False, | ||
| "project_id": PROJECT_ID, |
There was a problem hiding this comment.
If ChatGPT reports a rate limit after dispatching the sentinel, this request can generate multiple user turns despite the harness's explicit ONE-send safety contract. The non-streaming REST handler defaults to three attempts in _full_response, re-invoking send_and_stream on RateLimitError unless single_send is enabled. Set single_send: true in this payload so a transient throttle cannot duplicate the experimental mutation or contaminate its causal timeline.
Useful? React with 👍 / 👎.
Read this first — the claim, precisely scoped
Provides opt-in at-most-one automatic USER-send mutation attempts for
chat_completionandchat_with_gptacross transport reconnect ambiguity and outer rate-limit retry. Transport-level send mutations are non-replayable for all callers.It does not provide exactly-once delivery, and it does not automatically reconcile ambiguous outcomes.
create_memoryalso sends USER turns internally but deliberately does not participate insingle_send(see scoping below).Five things this PR wants you to notice
Runtime.evaluateafter one reconnect — reproduced deterministically offline (2 frames, ids [1,2], throughclick_send's real path). Now every send click goes through_js_mutation, sent exactly once, never replayed. Read resilience for ordinary observations (_js) is unchanged.single_send=true. With the default (3 attempts), aRateLimitErrorsurfacing after a dispatched send may still re-run the whole turn, including the USER send. That is unchanged master behavior, stated as legacy — the opt-in-vs-default policy decision is deliberately left open (see "Open decision").SendOutcomeUnknownErrorcarriesretry_safe=false/reconciliation_safe=trueand the preserved causal UUID (captured_user_id, from a bounded 1.0 s in-flight capture window before the scope closes). REST non-stream maps it to HTTP 409 withcode=send_outcome_unknown— deliberately noRetry-Afterheader, because the semantic instruction is do not resend. The response also sendsx-should-retry: false— the official OpenAI SDKs check that header before their retryable-status rules (409 is otherwise auto-retried by default), which closes the SDK-layer auto-replay hole the independent review flagged. Clients that ignore the header still have thecodefield;Retry-Afteris deliberately absent.Scoping detail
single_sendis honored bychat_completionandchat_with_gptonly (_SINGLE_SEND_TOOLS).create_memorysits in_CHAT_TOOLSfor rate-limit retry but does not inherit send semantics — memory mutations would need their own effect analysis before getting at-most-once behavior. Pinned by test (test_single_send_scoped_to_chat_send_tools).Streaming note (compatibility extension)
For streaming REST, HTTP status is already committed (200) when the send runs, so an ambiguous outcome surfaces as an inline SSE marker chunk —
[Error: send_outcome_unknown … captured_user_id=…]— mirroring the existing mid-stream rate-limit marker precedent. This is a compatibility extension, not a standard OpenAI stream-error contract; clients must parse the marker.Review shape — the commits
8e319d6— the seam:CDPTransport._js_mutation(sent once,_retry=False; reconnect-class failures and post-dispatch timeouts raiseSendOutcomeUnknownError), driver delegation,click_sendon the mutation primitive, breakerCOMPOSER_SEND_READINESSsuccess moved from the click's JS return to submission evidence (captured UUID primary, DOM-ack fallback),single_sendopt-in. Primary code-review unit.d0995e1— Phase 0 evidence/provenance only (upstream contract report, experiments 1A/1B, recon harnesses). Supporting evidence, not justification for broadening the patch.9534700— pre-push review fixes: the presentation-layer gaps (REST 500→409, SSE inline marker, MCP structured error) where UNKNOWN was losing its semantics, plus the cancellation-during-window test.The narrow review question this PR asks: does this patch enforce the intended mutation invariant without regressing ordinary read/reconnect resilience?
Verification
test_no_replay_send.py(transport contract),test_single_send_propagation.py(seams),test_send_outcome_unknown.py(UUID preservation, breaker boundary trio, scope isolation, surface mappings, cancellation).test_config_default_parallel_tabs_false) fails identically on cleanmaster(verified by stash); pre-existing.ruff: clean (0 errors) after declaringknown-first-partyin pyproject and clearing the inherited baseline; the earlier 10-vs-11 comparison was classification drift between environments, resolved byf94f02e.docs/research/issue51-no-replay-implementation.md,docs/research/experiment-1b-send-timeline.md,docs/research/chatgpt-web-contract-2026-09.md.Review rounds so far
82a37d7(x-should-retry:false on the 409),0a5dd0d(harness Bearer/IIFE),f94f02e(deterministic lint classification + baseline cleared). Verified on GitHub.d1e2bd3; GitWire against611f53a) — P1 cancellation:CancelledErrorpropagates unchanged with the UNKNOWN classification on a driver side channel (54d5593); P2 breaker: downstream-success fallback after stream completion (54d5593); GitWire streaming claim disproven and pinned by tests (evidence comment on the PR); harness defects fixed (9f52fe6). Follow-ups from the current-head pass: CDP pending-entry cancellation cleanup (546c345, red-proven first) and Experiment 1B's early-stop actually stopping + run-time-derived evidence provenance (78b8ed2).Open decision (post-review, not this PR)
Whether
single_sendremains opt-in after live certification or becomes the default safe behavior. Until then the default surface retains the second replay class (outer whole-turn retry). Tracked in #51.Refs #51 (not "Fixes" — closure stays open pending live certification and the default-safety decision).