Fix cdp() params-dict form and new_tab() blank-tab leak - #417
Conversation
SKILL.md documents raw CDP as cdp("Domain.method", params), but the
implementation was cdp(method, session_id=None, **params) — a positional
params dict landed in session_id, so the documented form always failed
(e.g. Target.closeTarget -> "Message may have string 'sessionId'
property" / "params.targetId mandatory field missing"). Agents
following the docs could not close tabs, quietly leaking them.
cdp() now accepts both forms (kwargs win on key collisions) and
session_id becomes keyword-only — every call site in the repo, the
interaction skills, and the domain skills already passes it by keyword.
new_tab() creates an about:blank tab before attaching/navigating; a
failure in that chain leaked the blank tab. It now closes the tab and
re-raises.
|
linking some related issues i found while digging: this sits on the same tab/session lifecycle seam as #379 and #352, might help triage. one honest caveat on the new_tab guard: if goto_url fails after switch_tab already ran, we close a tab the daemon still considers current. that is exactly the dangling session state #379 describes. i figured leaking a session pointer (recovers on the next call) beats leaking a tab (sits there forever, rent free), but if #393 lands first this cleanup could route through that instead. for what it's worth, while testing this i found four orphaned about:blank tabs in my own browser. at least they had each other. |
Kastan97
left a comment
There was a problem hiding this comment.
Ollie (CTO) Review: APPROVED ✅
What this does: Two fixes: (1) cdp() now accepts both keyword args and an explicit params=dict; (2) new_tab() cleans up blank targets on attach failure.
Good to merge once CI passes.
Problem
SKILL.mdline 93 documents raw CDP as:But the implementation is
cdp(method, session_id=None, **params)— so the documented dict form puts the params dict intosession_idand sends empty params. Every agent following the skill doc hits this:A practical consequence: tab-close calls written in the documented style fail, so agents quietly leak tabs.
Fix
cdp()accepts both the documented dict form and the existing kwargs form (kwargs win on key collisions), andsession_idbecomes keyword-only:Compatibility: every call site in
src/,agent-workspace/,interaction-skills/andagent-workspace/domain-skills/already passessession_idby keyword (grepped; none positional), so this is non-breaking for the repo and for code following either documented style.Related:
new_tab()leaks its about:blank tab on failurenew_tab()deliberately createsabout:blankfirst and navigates after (to avoid the createTarget/attach race). Ifswitch_tab/goto_urlthen throws, the blank tab is orphaned — over many sessions these accumulate visibly. Now the blank tab is closed before the exception re-raises.Tests
5 new unit tests in
tests/unit/test_helpers.py(kwargs form, dict form, merge precedence, keywordsession_id, new_tab close-on-fail). Full suite: 112 passed.Summary by cubic
Fixes
cdp()to accept the documented params-dict form and stopsnew_tab()from leaking the temporary about:blank tab on failures. Aligns behavior withSKILL.mdand prevents orphaned tabs.cdp()now accepts both dict and kwargs forms; kwargs win on collisions, andsession_idis keyword-only. This resolves failures likeTarget.closeTargetwhen called ascdp("Domain.method", {...}). No breaking change.new_tab()closes the about:blank tab if attach or navigation fails, then re-raises, preventing tab leaks.Written for commit 2d5e40d. Summary will update on new commits.