diff --git a/docs/BRIDGE.md b/docs/BRIDGE.md index 6950815..8b0e9be 100644 --- a/docs/BRIDGE.md +++ b/docs/BRIDGE.md @@ -145,44 +145,32 @@ The implementation is an **automatic candidate mode**, not an arbitrary port sca contiguous numeric range. The fixed-port path remains for debugging, policy, and deliberate isolation. -### User and configuration contract - -- The options page offers **Automatic (recommended)** and **Fixed port**. Automatic is the - default for new installs; the numeric input is shown only for the fixed mode. -- Extension storage keeps the concerns separate: - - `bridgePortMode: "auto" | "fixed"` is the user's setting. - - `bridgePort` is authoritative only in fixed mode. - - `bridgeLastPort` is a separate, non-setting `storage.local` cache of the last - authenticated automatic endpoint. It is excluded from settings change handling, may - improve ordering, and must never narrow the candidate set or become configuration. -- Gullet mirrors that contract. No `--port` flag and no `TABGLUTTON_PORT` means automatic - mode. A numeric `--port` or environment value pins exactly that port. `--port auto` may be - accepted for explicitness, but generated snippets should simply omit the flag. -- Existing numeric MCP configurations stay fixed: Gullet cannot tell whether an explicit - `--port 4588` was once copied as a default or deliberately chosen. Entering automatic mode - requires removing that flag or environment value. The options page's automatic-mode - snippet must not put it back. -- The token remains the bridge realm. Browsers and sidecars with the same token converge on - one hub and can be selected by `browser` / `connectionId`; a different token may establish - a separate hub on another candidate without gaining access to the first. - -### Settings migration - -The extension _can_ distinguish historical defaults from likely custom choices, so its -one-time migration is: - -1. If `bridgePortMode` already exists, preserve it. -2. If it is absent and `bridgePort` is a known historical default (`4588` or `4589`), persist - automatic mode. -3. If it is absent and `bridgePort` is any other valid value, persist fixed mode with that - value unchanged. -4. Preserve the token, enablement, and load permission exactly; changing discovery mode is - not token rotation. - -This intentionally treats someone who manually chose a number that was also a shipped -default as automatic. There is no evidence in the old schema that can recover that intent, -and retaining the stale-default failure for every existing install would defeat the -migration. Fixed mode remains one click away. +### Configuration contract + +- The options page offers **Automatic (recommended)** and **Fixed port**; the numeric input + belongs to fixed mode only. Extension storage keeps `bridgePortMode` (the user's setting) + apart from `bridgePort` (authoritative only in fixed mode) and from `bridgeLastPort` — a + non-setting `storage.local` cache of the last authenticated automatic endpoint, excluded + from settings change handling, which may improve ordering but must never narrow the + candidate set or become configuration. +- Gullet mirrors it: no `--port` and no `TABGLUTTON_PORT` means automatic mode, a numeric + value pins exactly that port, and `--port auto` is accepted for explicitness though + generated snippets simply omit the flag. +- **Existing numeric MCP configurations stay fixed.** Gullet cannot tell whether an explicit + `--port 4588` was copied from a shipped default or deliberately chosen, so entering + automatic mode means removing the flag or environment value — and the options page's + automatic-mode snippet must not put it back. +- The token is the bridge realm. Same-token browsers and sidecars converge on one hub and are + selected by `browser` / `connectionId`; a different token may hold another candidate + without gaining access to the first. + +The extension's one-time settings migration (`storage.ts`, pinned in `tests/storage.test.ts`) +reads an absent `bridgePortMode` as automatic when `bridgePort` is a historical default +(`4588` or `4589`) and as fixed at that value otherwise, preserving the token, enablement, and +load permission — changing discovery mode is not token rotation. It therefore treats someone +who manually chose a number that was also a shipped default as automatic. There is no evidence +in the old schema that can recover that intent, and retaining the stale-default failure for +every existing install would defeat the migration. Fixed mode remains one click away. ### Candidate-set contract @@ -246,30 +234,26 @@ automatic ports neither require nor imply a daemon. ### Extension discovery across candidates -Fixed mode keeps today's single-port behaviour. Automatic mode follows these rules: - -1. Order the scan with `bridgeLastPort` first when it is still a candidate, followed by the - remaining canonical candidates without duplication. -2. On startup, an explicit settings sync, an alarm wake, or an `IDLE_PROBE_MS` tick, probe - one candidate and advance the in-memory cursor. While the background page remains awake, - this completes a full five-port rotation in about 15 seconds. One trigger never becomes - N probes or N blind WebSocket dials. -3. A probe is positive only when the response carries Gullet's marker with a supported - protocol. Foreign listeners are skipped. A marked endpoint gets a WebSocket handshake; - token mismatch or protocol rejection advances to the next marked candidate rather than - latching a global conflict. -4. Cache a candidate only after mutual token proof reaches `hello-ack`. Clear or replace the - cache after a successful connection elsewhere; a cached silent/foreign endpoint is merely - tried first and never blocks fallback. -5. Keep at most one WebSocket dial in flight. The first authenticated connection wins the - pass and cancels the remaining probe work. +Fixed mode keeps today's single-port behaviour. In automatic mode the extension orders the +scan with `bridgeLastPort` first when it is still a candidate, then the remaining canonical +candidates without duplication, and each trigger — startup, an explicit settings sync, an +alarm wake, an `IDLE_PROBE_MS` tick — probes **one** candidate and advances an in-memory +cursor. A full five-port rotation therefore takes about 15 seconds while the page is awake, +and one trigger never becomes N probes or N blind dials. A probe is positive only when the +response carries Gullet's marker with a supported protocol: foreign listeners are skipped, and +a marked endpoint that then fails the token or protocol check advances to the next marked +candidate rather than latching a global conflict. Only mutual proof reaching `hello-ack` +caches a candidate — a cached silent or foreign endpoint is merely tried first and never +blocks fallback — at most one dial is in flight, and the first authenticated connection wins +the pass and cancels the remaining probe work. The HTTP probe remains a safety device, not an absolute gate. A future browser rule could block loopback `fetch` while still allowing WebSockets, so the current blind-dial escape valve survives with a strict bound: after the same instance-only counted misses, an eligible non-idle tick may blind-dial **one** candidate, rotating from the last-known/default choice. The 3s idle loop never increments that counter, and one tick never bursts across the set. -Automatic discovery must not turn one Gecko `FailDelayManager` input into N. +Automatic discovery must not turn one Gecko `FailDelayManager` input into N — which is also +why that counter dies with the page rather than persisting; see Open questions. An unmarked legacy Gullet is indistinguishable from another generic local HTTP service and is therefore foreign. Automatic mode does not weaken the marker check for compatibility; @@ -289,6 +273,31 @@ the old agent session is the upgrade path. - The options-page config snippet omits the port in automatic mode and includes the numeric flag only in fixed mode. +▸ **"Connected on \" and "no browser is connected" can both be true at once.** When +the token changes while an older agent session is still running, the two sidecars are in +different realms: the newer one cannot peer with the older hub — a mismatched token must +never be handed a proof — so it binds a different candidate, and the browser attaches to +whichever realm it finds first. If that is the older one, the extension's badge reports a +healthy connection on a real port while every tool call in the new session insists nothing +is attached. Every component behaves exactly as designed and the pair of facts still reads +as a broken bridge. Observed live; it cost real time to unpick. + +The election already knew: `tryExistingHub` records a compatible-marker candidate it could +not join. `Backend.rivalHubs()` now re-probes the candidates on the "no browser" path — +live rather than from those recorded observations, since a rival can appear long after we +settled — and the tool error names the endpoint it found and points at the token as the +reason two sidecars did not merge. Best-effort by construction: a throw inside the +diagnosis must never replace the error it was explaining. + +**A normal extension update does not cause this.** `bridgeToken` is minted only by an +explicit action in the options page (default `""`, never auto-regenerated) and +`bridgeLastPort` is written to `storage.local` and put first by +`orderedBridgePortCandidates` on the next start — both survive an update. Only an +_uninstall_ clears `storage.local`. That is the path that produced this: a +delete-and-reinstall regenerated the token, and the new token, not the reinstall, is what +split the realms. Worth stating because the endpoint moving looks like update fragility and +is not. + A filesystem rendezvous file is not part of this design. Gullet, Claude, and Codex could all read one, but a WebExtension cannot read an arbitrary config directory. Such a file may be useful later for human diagnostics; it cannot make the two halves discover each other @@ -311,43 +320,27 @@ without native messaging or another fixed bootstrap service. tears it down and starts a fresh full discovery pass; a candidate change alone is not revocation. -### Acceptance and test matrix - -The feature is not complete until all of these discriminate against the old implementation: - -- **Historical split:** an unmarked legacy listener occupies 4588, current Gullet serves - 4589, and an extension migrated from stored 4588 automatically connects to 4589. -- **Many browsers:** Zen and Chrome with one token connect to the same auto-selected hub; - `tabs_list` returns both and a tab-scoped call without `browser` is ambiguous. -- **Many sessions:** two same-token Gullet processes launched concurrently produce one hub - and one peer, including when the first production candidate is foreign. -- **No late compaction split:** a same-token hub on a later candidate is joined even after an - earlier candidate becomes free. -- **Separate realms:** two tokens can occupy different candidates; each browser and MCP - client sees only its matching realm. -- **Failover:** killing the hub makes peers re-elect on an available candidate and extensions - rediscover it without settings changes. -- **Probe discipline:** markerless foreign services receive HTTP only; normal automatic - discovery makes no blind WebSocket attempts; the blocked-fetch escape valve makes at most - one per eligible tick and keeps its counter instance-only. -- **Migration:** historical defaults become automatic, custom values remain fixed, and an - explicit CLI/env port remains fixed. -- **Exhaustion and recovery:** all candidates occupied yields an actionable MCP fault, then - heals after one becomes available without restarting the client. -- **Live engines:** repeat the shared-hub and failover scenarios on current Chrome and Zen, - since event-page suspension, alarm cadence, and Gecko reconnect delay are not faithfully - represented by unit tests. - -Pure candidate ordering, migration, and election decisions should be extracted behind -injectable probes/candidate arrays. Socket tests bind ephemeral ports; production candidate -numbers are validated separately against the documented selection criteria. +### Verification + +Candidate ordering, migration, election, and probe classification are unit-tested behind +injectable probes and candidate arrays; socket tests bind ephemeral ports, so nothing depends +on the production numbers being free, and those numbers are validated separately against the +selection criteria above. + +Three properties are not faithfully represented by any of that, because event-page +suspension, alarm cadence, and Gecko's reconnect delay are not, and are worth re-running on +current Chrome and Zen after changes here: two same-token browsers sharing one auto-selected +hub (with a tab-scoped call refusing to guess between them), hub loss followed by peer +re-election and extension rediscovery without a settings change, and candidate exhaustion +healing in place — an actionable MCP fault while every candidate is occupied, then a normal +session once one frees, with no client restart. ## Wire protocol One JSON object per WebSocket frame (the frame is the delimiter), versioned: -- Sidecar → extension on connect: `{ type: "challenge", proto: 1, server, nonce }`. -- Extension → sidecar: `{ type: "hello", proto: 1, browser: "firefox" | "chrome", +- Sidecar → extension on connect: `{ type: "challenge", proto: 2, server, nonce }`. +- Extension → sidecar: `{ type: "hello", proto: 2, browser: "firefox" | "chrome", extVersion, label, nonce, proof }`. - Sidecar → extension: `{ type: "hello-ack", proto, connectionId, proof }`, or `{ type: "hello-error", error }`. @@ -358,6 +351,21 @@ extVersion, label, nonce, proof }`. activity extends worker lifetime since Chrome 116, below our `minimum_chrome_version`) — control frames the browser answers itself would not. +Protocol 2 is an intentional compatibility boundary. Protocol 1 predates both the default +`tabs_list` limit and `tab_clip`'s `vault` override: an old Gullet would omit the limit and +silently lose tabs when talking to a new extension, while an old extension would ignore the +vault and file into the configured destination. The handshake rejects both mixed-version +pairings instead of allowing either call to appear successful with the wrong result. + +The bump is not the default for a new field, and the test is whether the other end ignoring +it produces a _wrong_ answer or merely a less precise one. `matched` and `query` are +tolerated across versions precisely because an extension that ignores them costs nothing +the sidecar cannot recompute from what it did send — `tabsList` recomputes both. A missing +`limit` returns tabs that were silently dropped, and a missing `vault` files a clip +somewhere the caller did not ask for; neither is recoverable downstream, and both are +reported as success. Recoverable skew is tolerated; a confidently wrong result forces a +bump. + ▸ **The token is not sent.** The sketch had the extension put its token in the hello and the sidecar echo it back, which proves nothing in the return direction. Instead each side proves it knows the token by hashing it against a nonce the _other_ side chose: @@ -369,19 +377,44 @@ different token/nonce split. Shared request/response types live in `src/bridge-protocol.ts`, imported by both the extension and Gullet so the contract is typechecked from one definition. -## Tool surface (v1) +## Tool surface (v2) -| MCP tool | Backing APIs | Notes | -| ------------ | ------------------------------------------------------ | --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | -| `tabs_list` | `tabs.query` | id, title, url, `lastAccessed`, `discarded`, `pinned`; on Firefox also `hidden` (≈ other Zen workspaces). Metadata only — cheap over hundreds of tabs. | -| `tabs_load` | `tabs.reload` + `tabs.onUpdated` | Wakes discarded tabs so they can be read. Batched (≤20), three at a time, under a 30s deadline; per-tab `ready`/`pending`/`failed`. Gated on a settings toggle, default off — answers `not-enabled` until then. | -| `tab_read` | `scripting.executeScript` + existing `clip-current.ts` | Returns Defuddle markdown + metadata. Fails cleanly on discarded tabs (see below). | -| `tab_clip` | existing `clip-format.ts` + `obsidian://new` handoff | Files into the vault exactly as manual Devour does, including the Chrome redirect-page dance. | -| `tabs_close` | `tabs.remove` | Batched, ids deduplicated. Entries (title, url, pinned, window, index, private) are recorded in an undo log in `storage.local` _before_ the removal, and the batch id comes back with the result. | -| `undo_close` | reopen from the log | Safety valve for the one destructive act. Omit the batch id to undo the most recent. | +| MCP tool | Backing APIs | Notes | +| ------------ | ------------------------------------------------------ | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ | +| `tabs_list` | `tabs.query` | id, title, url, `windowId` (hoisted when shared), and — only when true — `lastAccessed`, `discarded`, `pinned`, `active`, `hidden`. Filtered with `query`, ordered with `sort`, capped by `limit`, or collapsed to counts with `groupBy: "domain"`. **On Zen, covers the active workspace only.** See below. | +| `tabs_load` | `tabs.reload` + `tabs.onUpdated` | Wakes discarded tabs so they can be read. Batched (≤20), three at a time, under a 30s deadline; per-tab `ready`/`pending`/`failed`. Gated on a settings toggle, default off — answers `not-enabled` until then. | +| `tab_read` | `scripting.executeScript` + existing `clip-current.ts` | Returns Defuddle markdown + metadata. Fails cleanly on discarded tabs (see below). | +| `tab_clip` | existing `clip-format.ts` + `obsidian://new` handoff | Files into the vault exactly as manual Devour does, including the Chrome redirect-page dance. An optional `vault` overrides the destination for that one call. See below. | +| `tabs_close` | `tabs.remove` | Batched, ids deduplicated. Entries (title, url, pinned, window, index, private) are recorded in an undo log in `storage.local` _before_ the removal, and the batch id comes back with the result. | +| `undo_close` | reopen from the log | Safety valve for the one destructive act. Omit the batch id to undo the most recent. | Deliberately absent: navigate, click, type, evaluate. +▸ **`tab_clip`'s `vault` overrides a destination, it does not change a setting.** The +motivating case is a two-vault user: an agent-managed vault that agents file into by +default, and a main vault that occasionally deserves something directly, without a +staging hop it would only have to be moved out of later. The tempting shape for that is a +tool that writes `obsidianVault` — and it is the wrong one. Settings are the user's, edited +in a UI they can see; a tool that mutates one leaves the extension describing a destination +the user never chose, silently, for every clip after it, including the ones from the popup. +A per-call parameter expresses the same intent and expires by construction, so the blast +radius of an agent's mistake is exactly one note. The tool description therefore says to use +it **only when the user names a vault**, and the result reports the `vault` it filed into on +every clip, override or not — an agent that cannot see where a note went cannot tell the +user, and this is precisely the call where that matters. + +Two things it deliberately does not do. It does not fall back to the configured vault on a +blank string: `obsidianClipRequest` appends `&vault=` only for a truthy value, so a blank +reaching Obsidian means "whichever vault is open" — but silently substituting settings would +report a destination the caller did not ask for. Both readings are wrong, so `""` is a +`bad-request`. And it does not validate that the vault exists, because nothing in a +WebExtension can: the handoff is a URL handed to the OS. An unrecognised name fails inside +Obsidian, where neither end of the bridge can observe it, and the call still reports success +— which is why the parameter's description warns against guessing at a name rather than +relying on an error that will never arrive. What it does check is the one class of mistake +that is decidable from the string alone, via the same `vaultWarningFor` the options page +uses: a filesystem path where a vault name belongs. + ▸ **`tab_load` shipped as `tabs_load`, plural.** It was sketched as a per-tab v1.1 tool. But loading is dominated by the network wait, not by IPC, and the workflow that needs it — "here are the 30 discarded survivors of a metadata cut" — is inherently a batch. One tab per @@ -404,10 +437,167 @@ default off, surfaced as **Agent bridge → "Let agents load unloaded tabs"**. A returns the `not-enabled` code — distinct from `unsupported` because this one has a fix the agent can state to the user. -`tabs_list` with no `browser` argument fans out over every connected browser and tags each -tab with its origin, so discovering what is connected costs no extra round trip. The -tab-scoped tools refuse to guess between two browsers, because ids only mean something -within one. +`tabs_list` with no `browser` argument fans out over every connected browser, so +discovering what is connected costs no extra round trip. The tab-scoped tools refuse to +guess between two browsers, because ids only mean something within one — and for the same +reason a listing targeting two stamps `connectionId` on each returned tab, even when only +one browser matched. It is omitted only when one browser was targeted, because the +top-level `browsers` entry already identifies every returned id then. + +▸ **A listing is budgeted against a model's context, not against the socket.** The +measurement that drove this, from a real 874-tab Zen: 306 KB of JSON in one tool result, +which is past the client's tool-result ceiling — so the agent got _nothing_, and there was +no narrower call available to fall back to. `browser` and `connectionId` were constants +repeated once per tab (13%). `discarded`, `hidden`, `active`, and `pinned` were 17.6% +while carrying eleven `true` values between them — `hidden` was `false` on all 874. + +**Deleting the boilerplate was not enough, and that is the point.** Reconstructing that +listing from its reported per-field byte totals (within 0.5% of the original) and applying +the shape changes alone lands at **215 KB, 252 bytes a tab** — a 29% cut that is still far +past the ceiling, so the call still fails and the agent still gets nothing. What makes it +usable is narrowing. Measured over the same 874 tabs: + +| | whole backlog | per tab | at the default limit | +| --------------------------------------------------------------- | ------------- | ------- | -------------------- | +| original | 306 KB | 363 B | — (no limit existed) | +| constants hoisted, false flags dropped | 215 KB | 252 B | 49 KB | +| `index` dropped, `windowId` hoisted, title clipped, URL trimmed | 158 KB | 185 B | **36 KB** | +| `groupBy: "domain"` | **0.4 KB** | — | — | + +So the shape work is worth having — it halved the per-tab cost, and per-tab cost is what +decides how many tabs fit under a given limit — but it is a constant factor on something +that scales with the user's backlog. Only the filter changes the shape of the problem. + +Five changes, in the order they matter: + +1. **`query`, `limit`, `sort`.** The one that actually mattered: the session that produced + the measurement wanted "the x.com tabs" and had to ask for all 874 to find them. + `query` is a case-insensitive AND over whitespace-separated terms, matched against title + and URL together, so "github pull" finds a tab whose title and URL each carry one term. + Deliberately not a regex: an agent-authored regex is an unbounded backtracking risk on a + thousand strings, and substring terms are what triage actually needs. +2. **`groupBy: "domain"`** — counts only, no tabs. The real triage primitive: one cheap + call says what the backlog is made of and what to pass as `query` next. It honours + `query` too, so it can count one slice rather than the whole backlog, and it gets its + own tighter default limit (`TABS_LIST_DEFAULT_GROUP_LIMIT`, 50): the real 874-tab + browser held **298 distinct domains**, and everything past roughly the fiftieth was a + single tab — 250 rows of noise around the ~20 that describe the backlog. The domain is + the hostname minus `www.`, not the registrable domain: eTLD+1 needs the Public Suffix + List, which `bridge-protocol.ts` cannot take as a dependency and which goes stale, and + `mail.google.com` vs `docs.google.com` is the distinction triage wants anyway. +3. **False and unknown fields are omitted**, not sent. Absent means false; absent + `lastAccessed` means the browser reported none. +4. **Constants are hoisted** out of the tabs — `browser`/`connectionId` into the existing + top-level `browsers` array per the stamping rule above, and `windowId` to the top level + whenever every tab shares one window, which on a single-window Zen is all of them. + `index` is gone outright: it duplicated the array order under `sort: "window"`, meant + nothing under the others, and nothing consumed it — the undo log takes position from + the live `browser.tabs.Tab`, not from a listing. +5. **Titles are clipped and URLs trimmed** (`src/tabs-view.ts`). Titles at + `TAB_TITLE_MAX` (120) with a trailing `…`; there is no gentler cap, because the mean + title in that backlog was ~104 characters, so anything tighter cuts into the body of + the distribution rather than its tail. What a clipped title loses is cheap — titles are + front-loaded and the tail is usually the site suffix (`" | GitHub"`) the URL already + gives you. + + URLs are **not** clipped by default, because a URL cut mid-string stops being a URL: + it cannot be handed back to the user, and two distinct tabs can clip to the same prefix + and read as duplicates. They are trimmed structurally instead — `displayUrl` drops the + click-tracking params (sharing `isTrackingParam` with `normalizeUrl`, so there is one + list), the `www.`, and the trailing slash, which is where long URLs get long. It keeps + the scheme, so the result is still copyable, and keeps the fragment, which for an SPA + is the entire page identity. `TAB_URL_MAX` (200) is a backstop for data: URIs and + pathological paths, not the mechanism. + +Two things about `limit` are load-bearing. It **defaults** to `TABS_LIST_DEFAULT_LIMIT` +(200) rather than being opt-in, because the failure it prevents is total — an unbounded +listing returns nothing usable — and truncation is always visible: `matched` counts what +the filter hit, `truncated` says the answer is partial. And the default `sort` is `recent` +rather than the browser's own window order, which is what makes truncation defensible: the +tail that gets cut is the tabs the user touched longest ago, not an arbitrary slice. + +The filter/sort/limit pipeline (`selectTabs`) lives in `bridge-protocol.ts` and runs +**twice**. In the extension, so a backlog never crosses the socket whole; and again in +Gullet over the merged results, because a limit applied per browser is not the limit the +agent asked for. Running it a second time also means an older extension that ignores +`query` still yields a filtered answer instead of a flood. `groupBy` is the exception: +Gullet asks the extension for the full filtered set and groups it there, since a limit +applied before grouping would corrupt the counts — and the full list crossing loopback +costs nothing, which is the whole point of where the budget actually is. + +**Selection and rendering are separate passes for a reason.** `renderTabs` +(`src/tabs-view.ts`) runs _once_, in Gullet, at the very end — never in the extension, +even though clipping there would shrink the socket frame. Gullet re-applies `query` over +the merged results, and a query matching text that clipping had already removed would +silently drop the exact tab the agent asked for. So every filter sees whole strings and +only the bytes handed to the model are trimmed. This is the same trade as `groupBy`: the +socket is loopback, and loopback bytes are not the budget anyone is spending. + +▸ **`matched` is the browser's, not Gullet's, and recomputing it was a silent lie.** +Gullet read only `tabs` from each browser's reply and let its second `selectTabs` pass +derive `matched` from what arrived. But a current extension truncates to `limit` _before_ +sending, so the tabs that arrive are not the tabs that matched: 200 of 874 came back and +were reported as `matched: 200` with no `truncated` — the agent's one signal that it had +not seen everything, destroyed exactly when there was more to see. It now keeps each +browser's reported `matched`, falling back to its own count only when a browser sends none +(which is how an older extension identifies itself), resolved per connection and summed — +two attached browsers can be different versions. + +This one was **invisible in live testing**, because the browser it was tested against was +0.2.0 and sends everything unfiltered, so page size and match count were the same number. +It would have appeared on first contact with the very build that fixes the loopback cost. +Found by reading the path rather than running it, which is the argument for tracing a +change end to end before signing a build, not after. (The question that prompted the trace +— whether the `Number.POSITIVE_INFINITY` limit used for `groupBy` survives the wire — was +a non-issue: it is spent on a `slice` inside the extension and never reaches +`TabsListResult`, so `JSON.stringify` never gets the chance to turn it into `null`.) + +▸ **The second pass hid a bug from itself, and only a stale extension exposed it.** +`groupBy` grouped the _unfiltered_ merge: `tabs_list { query: "x.com", groupBy: "domain" }` +answered `matched: 874, domains: 298` — the whole backlog, identical to the unfiltered +call. The filter lived inside `selectTabs`, and the grouping branch skipped `selectTabs` +entirely. Against a **current** extension this was invisible, because the extension had +already applied `query` before sending; it only surfaced against a real browser running +0.2.0, which ignores `query` and hands over everything. So the version-skew tolerance that +the second pass exists to provide is exactly what stopped the bug being noticed, and the +skew itself is what revealed it. The filter is now `filterTabs`, called by both paths. +The lesson generalises: a redundant safety pass has to be tested with the primary pass +_disabled_, or it is only ever exercised as a no-op. + +`renderTabs` also tolerates a tab missing `title` or `url` rather than throwing. The +extension guarantees both, but a version-skewed one does not, and one malformed entry must +not destroy a listing of eight hundred — the same reason `tabs_list` keeps a failing +browser's partner. + +▸ **A Zen listing is the active workspace, and `hidden` does not tell you otherwise.** +Measured live on the 874-tab browser: `groupBy: "domain"` answered `matched: 874, +domains: 298`; after a workspace switch the same call answered `matched: 160, domains: 66` +with a disjoint set of domains, and `includeHidden: false` changed neither number. So tabs +in a non-active workspace are **absent from `tabs.query`, not flagged `hidden`** — the +condition `probeHeuristic` in `background.ts` was written to detect (`allInWindow.length +=== visibleInWindow.length`) is simply true here. Both readings hoisted the same +`windowId`, so this is one window enumerating differently, not a second window appearing. + +This is accepted behaviour, not a bug to fix: Zen exposes no workspace API (see AGENTS.md), +and active-workspace scope is the reasonable contract. What was wrong was the _claim_ — +`tabs_list` told agents `hidden: true` meant "another workspace", so an agent seeing 160 +tabs would report them as the user's whole backlog with no hedge, and `matched` reads as +authoritative either way. The tool description and `GULLET_INSTRUCTIONS` now state the +scoping outright. + +The honest remaining gap is that **nothing in the result says which workspace it is**, so +two listings taken minutes apart are not comparable and nothing in the payload reveals it. +Naming the workspace is impossible without an API Zen does not have; the available half- +measure is for the extension to surface `probeHeuristic`'s verdict on the listing itself, +which is a wire change and is not made here. It only became visible at all because the +payload work made two listings small enough to compare at a glance. + +▸ **This may also settle the first-`tabs_list` timeout** in the open questions below. +_Response size_ is one of the two live hypotheses for it, and a first call that used to +serialise ~300 KB into one WebSocket frame now serialises ~36 KB. That is not a fix, and +it is not evidence — but it does mean the symptom recurring at the new size would rule +response size out and leave startup contention, which is the discriminating test that was +otherwise awkward to run. **Restoring is exact where it can be and safe where it cannot.** A batch is recreated in ascending index order within each window; inserting a low index after a high one would diff --git a/gullet/src/backend.ts b/gullet/src/backend.ts index bf70066..c922769 100644 --- a/gullet/src/backend.ts +++ b/gullet/src/backend.ts @@ -29,6 +29,8 @@ export interface BridgeBackend { request(connectionId: string, method: BridgeMethod, params: unknown): Promise; /** Why nothing can be served right now, or null. Re-read on every call. */ fault(): BridgeError | null; + /** Candidate ports held by another Tabglutton hub. Diagnosis, not routing. */ + rivalHubs(): Promise; stop(): void; } @@ -263,6 +265,18 @@ export class Supervisor implements BridgeBackend { // Both roles wait the same first-call window: a peer inherits it inside the // hub it is attached to, a hub applies it here. No caller gets a knob — the // wait lives at the layer that owns it, so the roles cannot diverge. + /** + * Another Tabglutton hub holding one of the candidate ports, if any. + * + * Exists because "the extension says connected" and "no browser is connected" + * are both true when two hubs run with different tokens, and that pair of + * facts reads as a broken bridge rather than as the split it is. Costs a few + * loopback probes and is only ever called to explain a failure. + */ + async rivalHubs(): Promise { + return rivalHubPorts(this.candidatePorts(), this.activePort); + } + async connections(): Promise { await this.waitForSettling(); if (this.peer) return this.peer.connections(); @@ -287,6 +301,26 @@ export class Supervisor implements BridgeBackend { } } +/** + * Candidate ports answering as a Tabglutton hub that is **not** this process. + * + * Only ever asked on the "no browser is connected" path, so the probes cost + * nothing that matters and are done live rather than read from the election's + * observations — a rival can appear long after we settled, which is exactly the + * case worth catching. + * + * A compatible answer here almost always means a token mismatch: a hub sharing + * our token would have been joined as a peer instead of left running beside us. + * That is the diagnosis the caller turns into advice. + */ +async function rivalHubPorts(candidates: number[], activePort: number | null): Promise { + const others = candidates.filter((port) => port !== activePort); + const probes = await Promise.all( + others.map(async (port) => ((await probeCandidate(port)) === "compatible" ? port : null)), + ); + return probes.filter((port): port is number => port !== null); +} + type CandidateProbe = BridgeProbeIdentity | "silent"; async function probeCandidate(port: number): Promise { diff --git a/gullet/src/main.ts b/gullet/src/main.ts index 54724f1..d22d6b2 100644 --- a/gullet/src/main.ts +++ b/gullet/src/main.ts @@ -82,6 +82,7 @@ export async function main( // A port we never bound is the more proximate problem, and fixing the // token would not make this process serve anything either way. startupError: () => backend.fault() ?? tokenError, + rivalHubs: () => backend.rivalHubs(), }), }); diff --git a/gullet/src/tools.ts b/gullet/src/tools.ts index 5ac1e41..bb19e79 100644 --- a/gullet/src/tools.ts +++ b/gullet/src/tools.ts @@ -1,16 +1,25 @@ -// The MCP tool surface (docs/BRIDGE.md "Tool surface (v1)") and its mapping onto +// The MCP tool surface (docs/BRIDGE.md "Tool surface (v2)") and its mapping onto // bridge methods. Read + file + close, and nothing else: no navigation, no // clicking, no typing, no arbitrary script execution. import { asRecord, BridgeRequestError, + filterTabs, + groupTabsByDomain, isBridgeMethod, + parseTabsListParams, + selectTabs, + TABS_LIST_DEFAULT_GROUP_LIMIT, + TABS_LIST_DEFAULT_LIMIT, + TABS_LIST_MAX_LIMIT, TABS_LOAD_MAX_BATCH, toBridgeError, type BridgeError, type BridgeMethod, + type BridgeTab, } from "../../src/bridge-protocol.js"; +import { renderTabs, TAB_TITLE_MAX } from "../../src/tabs-view.js"; import type { McpTool, McpToolResult } from "./mcp.js"; import { selectAll, selectOne, type ConnectionSummary } from "./select.js"; @@ -29,6 +38,12 @@ export interface ToolContext { * on refusing calls the backend had since become able to serve. */ startupError: () => BridgeError | null; + /** + * Candidate ports held by another Tabglutton hub, asked only when there is no + * browser to serve. Optional so tests and any future embedding can omit it — + * it explains a failure, it never changes one. + */ + rivalHubs?: () => Promise; } const BROWSER_PROPERTY = { @@ -41,9 +56,19 @@ const BROWSER_PROPERTY = { export const GULLET_INSTRUCTIONS = `Tabglutton's bridge to the user's open browser tabs. -Triage cheaply: tabs_list returns metadata only and is affordable across hundreds of -tabs, so cut on title, URL, and lastAccessed BEFORE reading anything. Only call tab_read -on the survivors. +Narrow before you list. A backlog here is hundreds to thousands of tabs and a full +listing will not fit in your context, so work down: tabs_list with groupBy: "domain" +first to see what the backlog is made of, then tabs_list with a query to pull just the +tabs you want. tabs_list is metadata only — cut on title, URL, and lastAccessed BEFORE +reading anything, and only call tab_read on the survivors. + +It answers with "matched" and "truncated", so you can always tell a complete answer from +a truncated one. If a listing comes back truncated, narrow the query — do not raise the +limit and do not page through the whole backlog. + +On Zen, every listing is scoped to the active workspace, and nothing in the result says +which one that is. Treat counts as "this workspace", never "your tabs", and expect the +same call to answer differently after the user switches workspace. Most tabs in a large backlog are discarded (unloaded), and tab_read and tab_clip cannot reach those. Wake them with tabs_load first — one call for every survivor you mean to read @@ -61,11 +86,38 @@ export const GULLET_TOOLS: readonly McpTool[] = [ name: "tabs_list", title: "List open tabs", description: - "List the user's open tabs with metadata only — id, title, url, lastAccessed, discarded, pinned, active, window, and (Firefox/Zen) hidden. Cheap enough to run across hundreds of tabs; do your triage here before reading any page. `hidden: true` on Zen usually means the tab lives in another workspace. `discarded: true` means the tab is unloaded and cannot be read.", + `List the user's open tabs with metadata only — id, title, url, lastAccessed, and the flags discarded, pinned, active and (Firefox/Zen) hidden. **Flags appear only when true**: no \`discarded\` key means the tab is loaded. \`discarded: true\` means the tab is unloaded and cannot be read until tabs_load wakes it. \`windowId\` appears at the top level when every tab shares one window, and per tab otherwise.\n\n` + + `**On Zen, a listing covers the active workspace only.** Tabs in other workspaces are not returned at all — not flagged, absent — so \`matched\` counts that workspace, not the browser. Never tell the user how many tabs they have "in total" from this; say which workspace you looked at. Switching workspace changes the answer completely.\n\n` + + `Titles longer than ${TAB_TITLE_MAX} characters are clipped with a trailing "…", and URLs are shortened (tracking parameters and \`www.\` dropped). \`query\` always matches against the **full** title and URL, so a term that was clipped away still finds its tab. Use tab_read for a tab's real content.\n\n` + + `Backlogs are large, so this returns the ${TABS_LIST_DEFAULT_LIMIT} most recently accessed tabs by default and reports \`matched\` (how many the filter actually hit) plus \`truncated: true\` when there were more. Narrow with \`query\` rather than raising \`limit\` — a full listing of a thousand tabs will not fit in your context.\n\n` + + `Start a triage run with \`groupBy: "domain"\`: it returns one row per domain with tab and discarded counts instead of any tabs, which is a few hundred bytes for the whole backlog and tells you what to pass as \`query\` next.`, inputSchema: { type: "object", properties: { ...BROWSER_PROPERTY, + query: { + type: "string", + description: + 'Case-insensitive filter over title and URL. Whitespace splits it into terms that must all match, in either field — "github pull" matches a tab titled "Pull request" at github.com. Do this before raising limit.', + }, + limit: { + type: "integer", + minimum: 1, + maximum: TABS_LIST_MAX_LIMIT, + description: `Max rows to return. Defaults to ${TABS_LIST_DEFAULT_LIMIT} tabs, or ${TABS_LIST_DEFAULT_GROUP_LIMIT} when groupBy is set — a domain histogram has a long tail of one-tab domains.`, + }, + sort: { + type: "string", + enum: ["recent", "oldest", "window"], + description: + 'Order: "recent" (default, most recently accessed first), "oldest" (stale tabs first), or "window" (the order the user sees them in). Combined with limit, "recent" keeps what they were last working on and "oldest" surfaces closing candidates.', + }, + groupBy: { + type: "string", + enum: ["domain"], + description: + "Return per-domain counts instead of tabs: { domain, tabs, discarded, newest }, most tabs first. Honours query, so you can count one slice of the backlog. Answers with `domains` (distinct domains matched) and `matched` (tabs behind them). The cheap first call for triaging a backlog you have not seen.", + }, scope: { type: "string", enum: ["all", "current-window"], @@ -131,7 +183,7 @@ export const GULLET_TOOLS: readonly McpTool[] = [ name: "tab_clip", title: "File a tab into Obsidian", description: - "Save a tab into the user's Obsidian vault as a markdown note with frontmatter — exactly what the Tabglutton popup's Devour does, including per-site subfolders. Requires a vault configured in Tabglutton's settings. Set close: true to close the tab afterwards; that close is undoable via the returned batchId. Filing alone changes nothing in the browser — the tool is annotated destructive because close: true removes the tab.", + "Save a tab into the user's Obsidian vault as a markdown note with frontmatter — exactly what the Tabglutton popup's Devour does, including per-site subfolders. Requires a vault configured in Tabglutton's settings. Set close: true to close the tab afterwards; that close is undoable via the returned batchId. Filing alone changes nothing in the browser — the tool is annotated destructive because close: true removes the tab. The result reports the vault it filed into.", inputSchema: { type: "object", properties: { @@ -141,6 +193,11 @@ export const GULLET_TOOLS: readonly McpTool[] = [ type: "boolean", description: "Close the tab once Obsidian has the note. Defaults to false.", }, + vault: { + type: "string", + description: + "File into this vault instead of the configured one, for this call only — nothing is saved. Use ONLY when the user names a destination vault themselves; never choose one on your own, and never guess at a name. Tabglutton cannot check that the vault exists, so a name Obsidian does not recognise fails inside Obsidian, where neither of us can see it, and this tool still reports success. Pass the name from Obsidian's vault switcher, not a path.", + }, }, required: ["tabId"], additionalProperties: false, @@ -201,7 +258,7 @@ export function createToolCaller( if (fault) throw new BridgeRequestError(fault.code, fault.message); return ok(await route(ctx, name, args)); } catch (err) { - return toolError(err); + return toolError(await explainNoConnection(ctx, err)); } }; } @@ -221,53 +278,7 @@ async function route( const { browser: _browser, ...params } = args; const summaries = await ctx.connections(); - if (name === "tabs_list") { - // Read-only and id-free, so fanning out over every browser is safe and - // saves the agent a round trip to discover what is connected. - const targets = selectAll(summaries, target); - // Each request carries its own catch, so this Promise.all can never reject: - // one browser timing out must not throw away the listing another already - // returned. A half-answer the agent can see the shape of beats no answer, - // and with two browsers attached the healthy one is usually the one being - // triaged anyway. - const perBrowser = await Promise.all( - targets.map(async (conn) => { - try { - const result = (await ctx.request(conn.connectionId, "tabs_list", params)) as { - tabs?: Array>; - }; - const tabs = (result?.tabs ?? []).map((tab) => ({ - ...tab, - browser: conn.label, - connectionId: conn.connectionId, - })); - return { tabs }; - } catch (err) { - const { code, message } = toBridgeError(err); - return { - tabs: [], - failure: { connectionId: conn.connectionId, browser: conn.label, error: code, message }, - }; - } - }), - ); - const failures = perBrowser.map((r) => r.failure).filter((f) => f !== undefined); - // Every browser failed: there is no partial answer to give, and an empty - // `tabs` array would read as "the user has no tabs" rather than as a fault. - if (failures.length === targets.length) { - const first = failures[0]; - throw new BridgeRequestError( - first?.error ?? "internal", - first?.message ?? "tabs_list failed.", - ); - } - // Tabs carry their origin so ids from two browsers can never be confused. - return { - browsers: targets, - tabs: perBrowser.flatMap((r) => r.tabs), - ...(failures.length > 0 ? { failures } : {}), - }; - } + if (name === "tabs_list") return tabsList(ctx, summaries, target, params); // Everything else is tab-scoped: ids only mean something inside one browser. const conn = selectOne(summaries, target); @@ -280,12 +291,168 @@ async function route( }; } +/** + * Fan a listing out over every connected browser and merge the answers. + * + * Read-only and id-free, so fanning out is safe and saves the agent a round trip + * to discover what is connected. The filter/sort/limit pipeline then runs a + * second time here, over the merged set: a limit applied per browser is not the + * limit the agent asked for, and re-running it is also what lets an older + * extension that ignores `query` still produce a filtered answer. + */ +async function tabsList( + ctx: ToolContext, + summaries: ConnectionSummary[], + target: string | undefined, + params: Record, +): Promise { + const listParams = parseTabsListParams(params); + const targets = selectAll(summaries, target); + // Each request carries its own catch, so this Promise.all can never reject: + // one browser timing out must not throw away the listing another already + // returned. A half-answer the agent can see the shape of beats no answer, and + // with two browsers attached the healthy one is usually the one being triaged. + const perBrowser = await Promise.all( + targets.map(async (conn) => { + try { + const result = (await ctx.request(conn.connectionId, "tabs_list", params)) as { + tabs?: BridgeTab[]; + matched?: number; + }; + // `matched` is kept, not recomputed. A current extension truncates to + // `limit` before sending, so the tabs that arrive are not the tabs that + // matched, and its `matched` is the only place the real total survives. + // Recomputing it here reported the size of the page as the size of the + // result — the agent's one signal that it had not seen everything, lost + // exactly when there was more to see. An older extension sends no + // `matched`; that is what `undefined` means, and it is counted below. + return { + conn, + tabs: result?.tabs ?? [], + matched: typeof result?.matched === "number" ? result.matched : undefined, + }; + } catch (err) { + const { code, message } = toBridgeError(err); + const failure = { + connectionId: conn.connectionId, + browser: conn.label, + error: code, + message, + }; + return { conn, tabs: [] as BridgeTab[], matched: undefined, failure }; + } + }), + ); + const failures = perBrowser.map((r) => r.failure).filter((f) => f !== undefined); + // Every browser failed: there is no partial answer to give, and an empty + // `tabs` array would read as "the user has no tabs" rather than as a fault. + if (failures.length === targets.length) { + const first = failures[0]; + throw new BridgeRequestError(first?.error ?? "internal", first?.message ?? "tabs_list failed."); + } + + // Which browser a tab came from is tracked beside the tabs rather than stamped + // on them: with one browser connected — the normal case — the top-level + // `browsers` entry already says it, and repeating a constant string once per + // tab cost 13% of the listing that started this. + const origin = new WeakMap(); + const merged: BridgeTab[] = []; + for (const { conn, tabs } of perBrowser) { + for (const tab of tabs) { + origin.set(tab, conn); + merged.push(tab); + } + } + const head = { browsers: targets, ...(failures.length > 0 ? { failures } : {}) }; + + if (listParams.groupBy === "domain") { + // filterTabs, not `merged`: the extension may be older than this Gullet and + // ignore `query` entirely, and grouping the unfiltered set would answer a + // question nobody asked. + return { ...head, ...groupTabsByDomain(filterTabs(merged, listParams), listParams.limit) }; + } + const selected = selectTabs(merged, listParams); + // Window ids collide across browsers — two can each call their window `1` — + // so one is only worth hoisting when a single browser actually contributed. + const contributors = perBrowser.filter((r) => r.tabs.length > 0).length; + // Rendering happens here and only here — after every filter has seen the whole + // strings. renderTabs preserves order one-for-one, which is what lets the + // origin lookup stay keyed on the tabs that went in. + const view = renderTabs(selected.tabs, { hoistWindow: contributors <= 1 }); + // Ids only mean something inside one browser, so every tab needs its origin + // whenever more than one was targeted: the listing merged those id spaces + // even if only one of them came back with matches. Targeted, not + // `contributors` — the follow-up tab-scoped call has to name a browser either + // way, and the tabs from the one browser that answered are exactly the ids it + // will name it for. connectionId rather than the label, because labels are + // self-reported and two can share one. + const tabs = + targets.length > 1 + ? view.tabs.map((tab, i) => ({ + ...tab, + connectionId: origin.get(selected.tabs[i] as BridgeTab)?.connectionId, + })) + : view.tabs; + // Per browser: its own `matched` when it filtered, otherwise what our filter + // made of everything it sent. Mixing the two is normal — one browser can be + // newer than the other — so this is resolved per connection and then summed, + // never taken from the merged set as a whole. + const matched = perBrowser.reduce( + (sum, r) => sum + (r.matched ?? filterTabs(r.tabs, listParams).length), + 0, + ); + return { + ...head, + ...(view.windowId === undefined ? {} : { windowId: view.windowId }), + tabs, + matched, + ...(matched > tabs.length ? { truncated: true } : {}), + }; +} + // Compact JSON, not pretty-printed: every one of these results goes into a // model's context, and a 300-tab listing does not need indentation. function ok(value: unknown): McpToolResult { return { content: [{ type: "text", text: JSON.stringify(value) }] }; } +/** + * Name the split when "no browser is connected" is true here and false in the + * browser, which is what two hubs with different tokens produce. + * + * The user sees Tabglutton's badge lit and reports the port it names, while + * every tool call insists nothing is attached — a pair of facts that reads as a + * broken bridge rather than as two sidecars that could not join each other. The + * hub election already handles this correctly (a mismatched token must never be + * handed a proof, so it binds elsewhere); all that was missing was saying so. + * + * Observed for real: an older agent session held 4589 with the token from before + * a reinstall, this one bound 20317 with the new one, and the browser attached + * to whichever it found first. + * + * Best-effort by construction — the probes are loopback and this is already the + * failure path, so a throw here must not replace the real error with its own. + */ +async function explainNoConnection(ctx: ToolContext, err: unknown): Promise { + if (!(err instanceof BridgeRequestError) || err.code !== "no-connection" || !ctx.rivalHubs) { + return err; + } + try { + const ports = await ctx.rivalHubs(); + if (ports.length === 0) return err; + return new BridgeRequestError( + err.code, + `${err.message} Another Tabglutton sidecar is already running on ` + + `127.0.0.1:${ports.join(", ")} and the browser may be attached to that one instead. ` + + `They could not merge, which means their tokens differ: check that this project's ` + + `TABGLUTTON_TOKEN matches the token in Tabglutton's settings, then restart the other ` + + `agent session (or this one) so they share a single connection.`, + ); + } catch { + return err; + } +} + function toolError(err: unknown): McpToolResult { const { code, message } = toBridgeError(err); return { diff --git a/gullet/tests/tools.test.ts b/gullet/tests/tools.test.ts index 07affe8..7933b9b 100644 --- a/gullet/tests/tools.test.ts +++ b/gullet/tests/tools.test.ts @@ -66,20 +66,82 @@ describe("tool definitions", () => { }); describe("tabs_list", () => { - test("fans out over every browser and tags each tab with its origin", async () => { - const { call } = caller([zen, chrome], ({ connectionId }) => ({ - tabs: [{ id: connectionId === "conn-1" ? 1 : 2 }], - })); - const result = await call("tabs_list", {}); - expect(payload(result)).toEqual({ + const tab = ( + id: number, + url: string, + lastAccessed?: number, + windowId = 1, + ): Record => ({ + id, + title: `tab ${id}`, + url, + windowId, + index: id, + ...(lastAccessed === undefined ? {} : { lastAccessed }), + }); + /** A tab as it comes back out: index dropped, windowId hoisted, url trimmed. */ + const shown = (id: number, url: string): Record => ({ + id, + title: `tab ${id}`, + url, + }); + + test("fans out over every browser and stamps origin when multiple are targeted", async () => { + const { call } = caller([zen, chrome], ({ connectionId }) => + connectionId === "conn-1" + ? { tabs: [tab(1, "https://a.test/")] } + : { tabs: [tab(2, "https://b.test/")] }, + ); + expect(payload(await call("tabs_list", {}))).toEqual({ browsers: [zen, chrome], + // No hoisted windowId: both browsers call their window 1, and claiming a + // single shared window across two browsers would be a lie. tabs: [ - { id: 1, browser: "Zen", connectionId: "conn-1" }, - { id: 2, browser: "Chrome", connectionId: "conn-2" }, + { ...shown(1, "https://a.test"), windowId: 1, connectionId: "conn-1" }, + { ...shown(2, "https://b.test"), windowId: 1, connectionId: "conn-2" }, ], + matched: 2, }); }); + test("leaves the origin off when only one browser is connected", async () => { + const { call } = caller([zen], () => ({ tabs: [tab(1, "https://a.test/")] })); + // The constants used to be repeated once per tab; `browsers` and the hoisted + // `windowId` already say both. + expect(payload(await call("tabs_list", {}))).toEqual({ + browsers: [zen], + windowId: 1, + tabs: [shown(1, "https://a.test")], + matched: 1, + }); + }); + + test("keeps the origin when only one of two browsers has matches", async () => { + const { call } = caller([zen, chrome], ({ connectionId }) => + connectionId === zen.connectionId ? { tabs: [tab(1, "https://a.test/")] } : { tabs: [] }, + ); + expect(payload(await call("tabs_list", {}))).toEqual({ + browsers: [zen, chrome], + windowId: 1, + tabs: [{ ...shown(1, "https://a.test"), connectionId: zen.connectionId }], + matched: 1, + }); + }); + + test("clips a long title but still matches a query against the full one", async () => { + const buried = `${"x".repeat(200)} needle`; + const { call } = caller([zen], () => ({ + tabs: [{ ...tab(1, "https://a.test/"), title: buried }], + })); + const result = payload(await call("tabs_list", { query: "needle" })) as { + tabs: Array<{ title: string }>; + matched: number; + }; + expect(result.matched).toBe(1); + expect(result.tabs[0]?.title).toEndWith("…"); + expect(result.tabs[0]?.title).not.toContain("needle"); + }); + test("narrows to the named browser", async () => { const { call, sent } = caller([zen, chrome], () => ({ tabs: [] })); await call("tabs_list", { browser: "Chrome" }); @@ -88,14 +150,112 @@ describe("tabs_list", () => { test("forwards its own params but not the routing field", async () => { const { call, sent } = caller([zen], () => ({ tabs: [] })); - await call("tabs_list", { browser: "Zen", scope: "current-window", includeHidden: false }); - expect(sent[0]?.params).toEqual({ scope: "current-window", includeHidden: false }); + await call("tabs_list", { browser: "Zen", scope: "current-window", query: "x" }); + expect(sent[0]?.params).toEqual({ scope: "current-window", query: "x" }); }); test("tolerates a browser that returns no tabs field", async () => { const { call } = caller([zen], () => ({})); expect(payload(await call("tabs_list", {}))).toMatchObject({ tabs: [] }); }); + + test("rejects bad arguments before dialling any browser", async () => { + const { call, sent } = caller([zen], () => ({ tabs: [] })); + const result = await call("tabs_list", { sort: "alphabetical" }); + expect(result.isError).toBe(true); + expect(payload(result)).toMatchObject({ error: "bad-request" }); + expect(sent).toEqual([]); + }); + + // Filtering and truncation run again here, over the merged set: an extension + // that ignored `query` must not flood the agent anyway, and a per-browser + // limit is not the limit the agent asked for. + test("re-applies query and limit across browsers that ignored them", async () => { + const { call } = caller([zen, chrome], ({ connectionId }) => + connectionId === "conn-1" + ? { tabs: [tab(1, "https://x.com/a", 100), tab(2, "https://other.test/", 400)] } + : { tabs: [tab(3, "https://x.com/b", 300), tab(4, "https://x.com/c", 200)] }, + ); + const result = payload(await call("tabs_list", { query: "x.com", limit: 2 })) as { + tabs: Array<{ id: number }>; + matched: number; + truncated: boolean; + }; + expect(result.tabs.map((t) => t.id)).toEqual([3, 4]); + expect(result).toMatchObject({ matched: 3, truncated: true }); + }); + + // A current extension truncates before sending, so the page that arrives is + // not the match count. Recomputing `matched` here reported 2 of 900 as + // "matched: 2" with no `truncated` — the agent's only signal that more + // existed, lost precisely when it did. Invisible against an extension that + // sends everything, which is why it survived a live run. + test("keeps the browser's matched when the browser truncated for us", async () => { + const { call } = caller([zen], () => ({ + tabs: [tab(1, "https://x.com/a", 400), tab(2, "https://x.com/b", 300)], + matched: 900, + truncated: true, + })); + const result = payload(await call("tabs_list", { query: "x.com", limit: 2 })); + expect(result).toMatchObject({ matched: 900, truncated: true }); + }); + + test("falls back to its own count for a browser that sent no matched", async () => { + const { call } = caller([zen], () => ({ + tabs: [tab(1, "https://x.com/a"), tab(2, "https://x.com/b"), tab(3, "https://other.test/")], + })); + // No `matched` on the wire means the browser did not filter, so the honest + // total is what our own filter kept — not the three tabs it handed over. + expect(payload(await call("tabs_list", { query: "x.com" }))).toMatchObject({ matched: 2 }); + }); + + test("sums matched across browsers of different vintages", async () => { + const { call } = caller([zen, chrome], ({ connectionId }) => + connectionId === "conn-1" + ? { tabs: [tab(1, "https://x.com/a", 400)], matched: 500 } + : { tabs: [tab(2, "https://x.com/b", 300), tab(3, "https://no.test/")] }, + ); + // 500 reported by the new one, plus the single tab our filter keeps from + // the old one's three. + expect(payload(await call("tabs_list", { query: "x.com" }))).toMatchObject({ + matched: 501, + truncated: true, + }); + }); + + // Regression, caught live: grouping ran on the unfiltered merge, so a query + // plus groupBy counted the whole backlog. The browser here ignores `query` + // entirely, which is the version skew that exposed it — a newer extension + // pre-filters and would have hidden the bug rather than prevented it. + test("groupBy honours query even when the browser ignored it", async () => { + const { call } = caller([zen], () => ({ + tabs: [tab(1, "https://x.com/a"), tab(2, "https://x.com/b"), tab(3, "https://other.test/c")], + })); + const result = payload(await call("tabs_list", { query: "x.com", groupBy: "domain" })); + expect(result).toMatchObject({ + groups: [{ domain: "x.com", tabs: 2, discarded: 0 }], + domains: 1, + matched: 2, + }); + }); + + test("groupBy: domain answers with counts across every browser and no tabs", async () => { + const { call } = caller([zen, chrome], ({ connectionId }) => + connectionId === "conn-1" + ? { tabs: [tab(1, "https://x.com/a"), tab(2, "https://www.x.com/b")] } + : { tabs: [tab(3, "https://x.com/c"), tab(4, "https://other.test/")] }, + ); + const result = payload(await call("tabs_list", { groupBy: "domain" })); + expect(result).toEqual({ + browsers: [zen, chrome], + groups: [ + { domain: "x.com", tabs: 3, discarded: 0 }, + { domain: "other.test", tabs: 1, discarded: 0 }, + ], + domains: 2, + matched: 4, + }); + }); }); describe("tab-scoped tools", () => { @@ -161,6 +321,46 @@ describe("tab-scoped tools", () => { }); }); +describe("no-connection diagnosis", () => { + // The pair of facts that produced this: the browser's badge said connected on + // 20317 while every tool call here said nothing was attached. + test("names the rival sidecar and points at the token", async () => { + const { call } = caller([], () => ({}), { rivalHubs: async () => [4589] }); + const result = await call("tabs_list", {}); + expect(result.isError).toBe(true); + const { message } = payload(result) as { message: string }; + expect(message).toContain("127.0.0.1:4589"); + expect(message).toContain("TABGLUTTON_TOKEN"); + }); + + test("stays quiet when this really is the only sidecar", async () => { + const { call } = caller([], () => ({}), { rivalHubs: async () => [] }); + const { message } = payload(await call("tabs_list", {})) as { message: string }; + expect(message).not.toContain("127.0.0.1"); + }); + + // The diagnosis is a courtesy on a path that has already failed; it must never + // replace the real error with a failure of its own. + test("survives a probe that throws", async () => { + const { call } = caller([], () => ({}), { + rivalHubs: () => Promise.reject(new Error("loopback refused")), + }); + const result = await call("tabs_list", {}); + expect(payload(result)).toMatchObject({ error: "no-connection" }); + expect((payload(result) as { message: string }).message).not.toContain("loopback refused"); + }); + + test("leaves every other failure untouched", async () => { + const { call } = caller([zen], () => { + throw new BridgeRequestError("timeout", "tabs_list timed out."); + }); + const probed = caller([zen], () => ({}), { rivalHubs: async () => [4589] }); + expect(payload(await call("tabs_list", {}))).toMatchObject({ error: "timeout" }); + // A healthy browser never consults the diagnosis at all. + expect(payload(await probed.call("tabs_list", {}))).not.toHaveProperty("error"); + }); +}); + describe("error handling", () => { test("no connected browser is reported, not swallowed", async () => { const { call } = caller([], () => ({})); @@ -235,8 +435,12 @@ describe("tabs_list with a browser that fails", () => { tabs: Array>; failures: Array>; }; + // Chrome is still a connected target even though its request failed, so the + // surviving tab needs an origin for the follow-up tab-scoped call. This tab + // also has no url — a malformed entry renders empty rather than throwing + // away the listing around it. expect(result.tabs).toEqual([ - { id: 1, title: "kept", browser: zen.label, connectionId: zen.connectionId }, + { id: 1, title: "kept", url: "", connectionId: zen.connectionId }, ]); expect(result.failures).toEqual([ { diff --git a/options/options.html b/options/options.html index a3d7dc6..f8844c6 100644 --- a/options/options.html +++ b/options/options.html @@ -211,8 +211,9 @@

Agent bridge

Access token Shared secret proving the sidecar is yours. It is never sent over the socket — both - sides prove they know it. Copy it into Gullet's environment; regenerating it - disconnects any sidecar still using the old one. + sides prove they know it. Copy it into Gullet's environment, or paste in one you + already have to match another browser or machine. Changing it disconnects any sidecar + still using the old one.
@@ -223,7 +224,19 @@

Agent bridge

puts it, so it reads as belonging to the token rather than as a third action alongside Copy and Generate. -->
- + +