RELEASE-FIX-F: the swallowed half of the F-771 tab family (F-775) - #50
Open
AminDhouib wants to merge 2 commits into
Open
AminDhouib wants to merge 2 commits into
AminDhouib wants to merge 2 commits into
Conversation
…ngs)
Pins only -- no src edit in this commit, so CI records the RED.
F-771 was loud: list_tabs raised a bare TypeError and the user knew. The three
sites pinned here share the exact same nodriver-0.47 mechanism -- update_targets()
re-appends a rediscovered target as a raw Connection (core/browser.py:561-583)
and Browser.tabs hands it back despite its List[Tab] annotation (137-142) -- but
each is wrapped in a broad `except Exception` that turns the failure into a
plausible-looking fallback. The user sees no error and wrong behavior.
Two tiers, per plan_RELEASE_FIX_F section 3:
1. tests/test_browser_manager_tab_rediscovery.py (hermetic, fast unit lane).
Drives the real BrowserManager against a browser.tabs holding raw
Connection-likes. Today, RED with the product's own failures:
- F-775a get_navigation_tab
WARNING browser_manager.get_navigation_tab: Tab health check failed for
i1: object FakeDiscoveredTarget can't be used in 'await' expression
... then `assert navigation_tab is tracked` fails (a FakeTab from
_replace_main_tab came back instead) and `assert browser.get_calls == []`
fails with [('about:blank', True)] -- the silent replacement AND the leak.
- F-775b close_tab
WARNING browser_manager.close_tab: 'types.SimpleNamespace' object has no
attribute 'close' -> assert False is True
- F-775c switch_to_tab
WARNING browser_manager.switch_to_tab: 'types.SimpleNamespace' object has
no attribute 'bring_to_front' -> assert False is True
2. tests/test_e2e_interaction.py (real Chrome). _force_rediscovery() drops a
target from nodriver's inventory exactly as its TargetDestroyed handler does
(core/browser.py:222-231) and lets update_targets() re-append it, then
ASSERTS the result is a genuine Connection and not a Tab -- a probe that
silently failed to reproduce the shape would make the pins vacuous. Local
Chrome does not produce it on its own (FIX-E established that).
The F-775a pins assert tab IDENTITY and tab COUNT, never absence of an
exception: a no-raise pin passes against today's silent-fallback behavior and
therefore proves nothing.
Harness lives in tests/fakes.py, THE hermetic home, extended (never forked):
FakeBrowser gains the browser-level `connection` seam (a FakeTab, so
send_calls/cdp_frames record the CDP command name AND its arguments) plus
get()/get_calls that model tab creation for the count assertion; FakeTab gains
Tab's __await__ and a real target; and fake_target now carries a real
cdp.target.TargetID rather than a bare str, because every by-id CDP command
serialises it with target_id.to_json() (cdp/target.py:258).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Never await a browser.tabs entry, never call a Tab-only method on one, and never hand one to a caller that will. Address the target BY ID over the browser connection instead -- that works for every object type nodriver puts in browser.tabs. As in FIX-E the fix is deletion, not an isinstance branch: a type switch here would be a second way to do one thing (CLAUDE.md convention 4). F-775a get_navigation_tab (the one that mattered) -- browser_manager.py:1102. The code found the tracked tab correctly BY TARGET ID and then awaited it as a "liveness check". For a rediscovered target that await raised, the broad handler concluded the tab was "missing or invalid" -- it was not, it had just been found -- and called _replace_main_tab(close_existing=False). Invisibly, on every navigation after any close_tab: the user's navigation landed in a DIFFERENT tab, the abandoned tab leaked, and NAVIGATION_RECYCLE_THRESHOLD accounting was distorted by the spurious replacements. The await is deleted; the loop is now purely a presence check on the target id and the method returns the caller's own tracked Tab. Returning the browser.tabs entry instead would only move the failure: navigate() immediately calls the Tab-only get()/evaluate() on whatever it is handed. The `browser.tabs[0]` adoption fallback is deleted for exactly that reason. It was a second, weaker way to do what _replace_main_tab is the one home for, and deleting the await alone would have made it strictly worse: the adopted object can be a raw Connection, and the AttributeError navigate() would then raise is not matched by _is_recoverable_navigation_error, so it would escape instead of self-healing. It also hijacked an unrelated user tab. Control now falls through to _replace_main_tab, which always yields a real in-process Tab. F-775b close_tab (:1382) and F-775d close_instance (:861) -- Connection has no close(); __getattr__ delegates to the TargetInfo, so the AttributeError was swallowed into `return False` for a tab that is perfectly closeable, and a user could not close such a tab through the tool at all. Both now send cdp.target.close_target(target_id) on browser.connection. Verified against the pinned nodriver, not guessed: Tab.close() is exactly `self.send(cdp.target.close_target(target_id=self.target.target_id))` (core/tab.py:1027-1034), close_target emits Target.closeTarget (cdp/target.py:248-264), and Browser.connection is where nodriver itself sends Target-domain commands (core/browser.py:251, 558). F-775c switch_to_tab (:1339) -- bring_to_front() is a Tab-only alias for Tab.activate(), i.e. cdp.target.activate_target (core/tab.py:1096-1106, cdp/target.py:193-208), so switching reported failure for a target that activates fine. Now sent by id on the browser connection. No `except Exception` block was widened, narrowed or tidied -- they are what hid these defects. What changed is that the guarded calls can no longer raise AttributeError/TypeError for a rediscovered target; the handlers keep their existing role for genuine CDP failures, and their KEEP bool contracts are untouched. The two M10a F-181 pins in test_silent_excepts_log.py are re-pointed at the new failure seam (browser.connection.send) with the guarantee they assert -- log, do not swallow -- unchanged. The duplicated find-tab-by-id loop in switch_to_tab/close_tab becomes one home, _find_tab. browser_manager.py is 1531 LOC, its exact pre-FIX-F size, against the frozen 1532 cap: the deletions pay for the helper. No cap padded. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Member
Author
CI: 23/23 green on
|
| cell | result |
|---|---|
| quality (ruff / ty 76-baseline / vulture / suppression owners / file budgets) | pass |
| unit-tests — Linux, Windows, macOS × py3.11/3.12/3.13 (9) | pass |
| coverage — Linux / Windows / macOS | pass |
| integration — Linux / Windows / macOS | pass |
| transport — Linux / Windows | pass |
| offline-stealth — Linux / Windows / macOS | pass |
| transport-known-gaps | pass |
| release-gate (aggregate) | pass |
The two real-Chrome forced-rediscovery probes (test_navigation_survives_a_forced_rediscovery, test_close_and_switch_survive_a_forced_rediscovery) run in the integration cell, so F-775a/b/c are proven against a genuine nodriver Connection on all three operating systems.
There is no macOS transport cell — that remains the declared F-773 gap, and no macOS transport coverage is claimed here.
Not merged. Held at the human merge gate.
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
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.
Closes F-775 — the three silent siblings of F-771. Stacked on
audit/release-fix-e(PR #49). Plan:audit/stage2/plan_RELEASE_FIX_F.md(added in the first commit). Human holds the merge gate; nothing merged here.F-771 was loud — it raised, so the user knew. These sites are wrapped in broad
except Exceptionhandlers that turn the same failure into a plausible-looking fallback: no error, wrong behaviour.The principle
Never
awaitabrowser.tabsentry, never call aTab-only method on one, and never hand one to a caller that will. Address the target by id overbrowser.connection— that works for every object type nodriver puts inbrowser.tabs. As in FIX-E the fix is deletion, not anisinstancebranch: a type switch here would be a second way to do one thing (CLAUDE.md convention 4).Per site
F-775a
get_navigation_tab— silent tab abandonment + tab leak (the one that mattered)The code found the tracked tab correctly by target id, then
awaited it as a "liveness check". For a rediscovered target that await raised, the handler concluded the tab was "missing or invalid" — it was not, it had just been found — and called_replace_main_tab(close_existing=False). Invisibly, on every navigation after anyclose_tab: navigation landed in a different tab, the abandoned tab leaked, andNAVIGATION_RECYCLE_THRESHOLDaccounting was distorted.The await is deleted; the loop is now purely a presence check on the target id and the method returns the caller's own tracked
Tab. Returning thebrowser.tabsentry instead would only move the failure —navigate()immediately calls theTab-onlyget()/evaluate()on whatever it is handed.The
browser.tabs[0]adoption fallback is deleted for the same reason (declared, not hidden — see "Judgement calls" below).F-775b
close_tab/ F-775dclose_instance— a lying failureConnectionhas noclose();__getattr__delegates to theTargetInfo, so theAttributeErrorwas swallowed intoreturn Falsefor a tab that is perfectly closeable — a user could not close such a tab through the tool at all. Both now sendcdp.target.close_target(target_id)onbrowser.connection.Verified against the installed nodriver, not guessed:
Tab.close()isself.send(cdp.target.close_target(target_id=self.target.target_id))—nodriver/core/tab.py:1027-1034close_targetemitsTarget.closeTarget—nodriver/cdp/target.py:248-264Browser.connectionis where nodriver itself sends Target-domain commands —nodriver/core/browser.py:251,:558F-775c
switch_to_tab— a lying failurebring_to_front()is aTab-only alias forTab.activate(), i.e.cdp.target.activate_target(core/tab.py:1096-1106,cdp/target.py:193-208), so switching reported failure for a target that activates fine. Now sent by id on the browser connection.No handler was widened, narrowed or tidied
They are what hid these defects. What changed is that the guarded calls can no longer raise
AttributeError/TypeErrorfor a rediscovered target; the handlers keep their existing role for genuine CDP failures and their KEEPboolcontracts are untouched. The two M10a F-181 pins intest_silent_excepts_log.pyare re-pointed at the new failure seam (browser.connection.send) with the guarantee they assert — log, do not swallow — unchanged.RED → GREEN
Two tiers, both landed RED in commit 1 (
080d16d, pins only, so CI records the RED) and GREEN in commit 2 (c2b94ae).Hermetic (
tests/test_browser_manager_tab_rediscovery.py, fast unit lane) — RED with the product's own failures:Real Chrome (
tests/test_e2e_interaction.py) —_force_rediscovery()drops a target from nodriver's inventory exactly as itsTargetDestroyedhandler does (core/browser.py:222-231) and letsupdate_targets()re-append it, then asserts the result is a genuineConnectionand not aTab— a probe that silently failed to reproduce the shape would make the pins vacuous. Against the pre-fixsrcthese are RED with a real nodriverConnection:That id mismatch is the silent tab abandonment, reproduced live.
The F-775a assertion, specifically
Per the plan, F-775a is pinned by tab identity and tab count, never by absence of an exception (a no-raise pin passes against the silent fallback and proves nothing):
navigation_tab is tracked;browser.get_calls == [];len(browser.tabs) == 2(unchanged);_instances[id]["tab"] is tracked_get_tab_target_id(navigation_tab) == tab_id; after a real navigation,set(list_tabs ids) == before("navigation opened or abandoned a tab"); the instance still tracks the same idLoad-bearing
Each removed line restored individually; the specific pin goes red with the exact original error:
await candidate_tabobject FakeDiscoveredTarget can't be used in 'await' expression→ wrong object,[('about:blank', True)]browser.tabs[0]adoptionassert <FakeAttachedTab …> is not <FakeAttachedTab …>await target_tab.close()'…' object has no attribute 'close'→assert False is Trueawait target_tab.bring_to_front()'…' object has no attribute 'bring_to_front'→assert False is TrueJudgement calls I am flagging rather than burying
The
browser.tabs[0]adoption fallback inget_navigation_tabis deleted, which is more than "delete theawait". Deleting the await alone would have been strictly worse: the adopted object can be a rawConnection, and theAttributeErrornavigate()would then raise is not matched by_is_recoverable_navigation_error, so it would escape instead of self-healing. It was also a second, weaker way to do what_replace_main_tabis the one home for, and it hijacked an unrelated user tab. Control now falls through to_replace_main_tab, which always yields a real in-processTab. Pinned bytest_navigation_never_adopts_a_browser_tabs_entry_as_main_tab. Behaviour change: when the tracked tab is genuinely gone, a fresh tab is opened instead of adopting a surviving one.switch_to_tabstill stores thebrowser.tabsentry as the instance main tab. Activation is fixed (the actual defect: switching failed). Storing a rawConnectionthere remains latent — every laterTab-only call on it would fail loudly. Per the plan's instruction ("if storing it is unsafe, say so and route it rather than inventing a conversion"), this is declared, not fixed here.Residual, same family, outside this plan's four sites:
_replace_main_tabawaits the objectbrowser.get(..., new_tab=True)returned (browser_manager.py:1030).Browser.getreturns whatever is inbrowser.targetsfor that id (core/browser.py:255-261), which is aConnectionifupdate_targets()won the race against theTargetCreatedhandler. Loud if it fires, so lower severity — routed, not silently patched.F-775d has no dedicated pin.
close_instanceis not drivable hermetically without new teardown scaffolding; it is exercised by every integration test'sfinally: close_instance(11/11 green) and shares the verified CDP call. Declared gap.The macOS "close_tab returned True while the target survived" flake FIX-E observed was not reproducible here (Windows green, including a bounded 10 s poll asserting the target really dies:
assert doomed_id not in remaining, "close_tab lied: the target survived"). Not characterized; not claimed closed.Gates
ty check --exit-zero-on-warning src/stealth_chrome_devtools_mcp/: 76 diagnostics = baselinebrowser_manager.py: 1531/1532 LOC — exactly its pre-FIX-F size. The deletions pay for the_find_tabhelper (the duplicated find-by-id loop inswitch_to_tab/close_tabbecomes one home). No cap padded; the 1-line headroom is preserved.-m "not integration": 772 passed, 1 skipped--no-verifynever used.macOS transport remains the declared F-773 gap — no macOS transport coverage is claimed.