fix(extension): handlers iterate ALL matching tabs instead of just the first

[chrischall]

Summary

Follow-up to #49 (merged): the four verb handlers in background.ts (fetch, read_cookies legacy, read_local_storage / read_session_storage, read_indexed_db) were each picking the first matching tab via tabs.find() and calling chrome.tabs.sendMessage on it. If that first tab was a pre-reload page that no longer has a content script — chrome.tabs.sendMessage then throws "Receiving end does not exist" — the call fails even when a freshly-loaded tab with the content script also exists elsewhere in the user's tab list.

Concrete repro: in live testing of #49's fixes, zillow_search succeeded but compass and redfin failed because their first-matched tab was an older page that pre-dated the extension reload. Refreshing those pages fixed it, but that's brittle and surprising.

Fix

Extracted sendToFirstResponsiveTab(matcher, buildMessage, tabUrlForError) and pointed all four handlers at it. The helper:

• Iterates every matching tab in chrome.tabs.query order
• Returns the first successful response
• Skips tabs that throw "Receiving end does not exist" and continues — those are the no-content-script pre-reload tabs that were silently shadowing fresh ones
• Fails immediately on any other throw — those are real faults worth surfacing (permission denied, frame closed, etc.)
• On full miss (no tab matched at all OR every match lacked a content script), returns a structured no-tab result with an actionable error message that tells the user to refresh the page

So the failure mode is preserved (and improved with a clearer message) when there really IS no working tab, but the common "I just reloaded the extension" case self-heals automatically.

Test plan

• npm test — 484/484 pass
• npm run typecheck — clean
• npm run build --workspaces --if-present — clean
• Manual: reload the fetchproxy extension at chrome://extensions/ while pages from each declared domain are open in the browser, then call MCP tools — should succeed without the user having to refresh the pre-reload tabs first.

Verified live end-to-end: after this fix, zillow_search + redfin_get_market_report + compass_healthcheck all round-trip successfully through three concurrent peer MCPs against real sites with the stale-tab issue present.

🤖 Generated with Claude Code

[claude]

Claude finished @chrischall's task in 5m 14s —— View job

---

Code Review

• Gather context (CLAUDE.md read)
• Read PR diff
• Review for correctness, edge cases, conventions, test coverage
• Post inline findings (3 inline comments)
• Post summary verdict

---

Overall: the fix is correct and the approach is sound. The sendToFirstResponsiveTab helper cleanly encodes the three terminal states, the fallback logic is right (skip "Receiving end" errors, fail fast on anything else), and all four verb-handler call sites are updated consistently. No important bugs found.

Three nits surfaced (all inline):

1. sendToFirstResponsiveTab has no unit tests — 6 code paths, none covered. Worth adding tests for the "one stale + one fresh tab" success case and the "all tabs stale" no-tab case in particular (line 1087).

2. New error template not reflected in classify-fetch-error.test.ts — the file's own header says every background.ts error template must be explicitly tested there; the new "all URL-matched tabs lacked content script" string is missing (line 1085).

3. matches.length overcounts tabs with non-numeric IDs — minor, since Chrome tabs always have IDs in practice; trivially fixable by folding the typeof match.id === 'number' guard into the initial .filter() (line 1054).

[claude]

🟡 Nit — no tests for sendToFirstResponsiveTab

This helper has six distinct code paths and none are covered by the test suite:

1. matches.length === 0 → immediate no-tab
2. typeof match.id !== 'number' → silent skip
3. sendMessage succeeds → response
4. sendMessage throws "Receiving end does not exist" → skip + continue
5. sendMessage throws something else → throw
6. All tabs exhausted with lastNoListener set → no-tab with embedded error

background.test.ts currently only tests handleServerHello. Since sendToFirstResponsiveTab is the security-sensitive routing path for ALL four verb handlers, dedicated unit tests here would be valuable — especially for cases 4 (the core new behaviour) and 6 (the final no-tab path with embedded error string). Fix this →

[claude]

🟡 Nit — new error template not reflected in classify-fetch-error.test.ts

The error-kind.ts header and classify-fetch-error.test.ts both state that every error template emitted by background.ts should be explicitly covered:

"every change to extension-emitted error templates needs to be reflected here in one place"

The "all URL-matched tabs lacked a content script" error introduced here (the lastNoListener !== null path) is a new template. It happens to be correctly classified as content_script_unreachable by the existing Receiving end does not exist check (because lastNoListener embeds that string), but this is implicit — the test file has no case for it.

A downstream engineer reading classify-fetch-error.test.ts to understand what error formats the extension emits would miss this new template. Worth adding a table entry like:

[
  'all URL-matched tabs lacked content script (new sendToFirstResponsiveTab format)',
  'no tab matching https://www.compass.com/ has the fetchproxy content script loaded ' +
    '(2 URL matches, none responded). Refresh the page in your browser to inject the content script, ' +
    'then retry. Last error: Error: Could not establish connection. Receiving end does not exist.',
  'content_script_unreachable',
],

[claude]

🟡 Nit — matches.length overcounts tabs that will be silently skipped

Tabs where typeof match.id !== 'number' are included in matches (via .filter()) and therefore in matches.length, but they are silently continue-d in the loop without a sendMessage attempt. The final error message says "N URL matches, none responded" — if some of those N had no numeric ID, the count is slightly misleading (they never actually received a message).

In practice Chrome tabs always have numeric IDs, so this edge won't surface for users. But for accuracy the count could be computed after the loop, or the filter could also exclude no-ID tabs:

Suggested change

	const matches = tabs.filter((t) => t.url && matcher(t.url));
	const matches = tabs.filter((t) => t.url && typeof t.id === 'number' && matcher(t.url));

Then the typeof match.id !== 'number' guard inside the loop becomes unreachable (which TypeScript will confirm) and can be dropped.

[claude]

Claude finished @chrischall's task in 2m 45s —— View job

---

Code Review (re-run on new push)

• Gather context (CLAUDE.md read)
• Read new commits / diff since last review
• Check if previous nits were addressed
• Review for new issues (correctness, coverage, conventions)
• Post findings and verdict

---

Re-review summary: The new push (830da43) is a plain merge from main — no substantive code changes. The three nits from the previous review are still open and no new issues were introduced.

Standing nits (all 🟡, none blocking):

1. sendToFirstResponsiveTab has no unit tests — 6 distinct code paths, none covered. The most important cases: one-stale-one-fresh tab succeeds on the second; all-stale returns no-tab with embedded error string. Fix this →

2. New error template not reflected in classify-fetch-error.test.ts — The "all URL-matched tabs lacked content script" message (starting no tab matching … has the fetchproxy content script loaded) gets implicitly classified as content_script_unreachable (the embedded "Receiving end does not exist" string in the Last error: suffix fires the first regex). The classification is correct, but the test table has no case for this new template format. Fix this →

3. matches.length overcounts tabs that will be silently skipped — Tabs with non-numeric IDs pass the .filter() and inflate matches.length, so the "N URL matches, none responded" error message can overcount. Trivially fixed by folding the ID check into the initial filter. Fix this →

Overall verdict: warn. The fix is correct and the approach is sound. No important bugs found.

[claude]

Claude finished @chrischall's task in 1m 38s —— View job

---

Code Review (re-review on new push)

• Gather context (CLAUDE.md, previous review nits)
• Read PR diff and changed files
• Check if previous nits were addressed
• Review for correctness, edge cases, conventions, test coverage
• Post findings and verdict

---

Re-review summary: The new push (e96968a) addresses all three nits from the previous review. No new issues found.

Nit resolution:

#	Previous nit	Status
1	sendToFirstResponsiveTab had no unit tests	✅ Added send-to-first-responsive-tab.test.ts with 6 cases covering all code paths: zero matches, first-tab success, stale-then-fresh fallback, all-stale no-tab, non-receiving-end throw, and no-id filter exclusion.
2	New error template not in classify-fetch-error.test.ts	✅ Added test case for the "all URL-matched tabs lacked content script" template, pinning its classification as content_script_unreachable.
3	matches.length overcounted tabs with non-numeric IDs	✅ typeof t.id === 'number' folded into the .filter(); inner-loop guard replaced with a narrowing cast + comment.

Additional checks on this push:

• No remaining tabs.find() calls in background.ts — all four verb handlers (fetch, read_cookies legacy, read_local_storage/read_session_storage, read_indexed_db) use sendToFirstResponsiveTab.
• captureRequestHeader correctly left unchanged — it uses chrome.webRequest (passive listener), not chrome.tabs.sendMessage.
• sendToFirstResponsiveTab is exported from background.ts but NOT re-exported from index.ts — correct, the export is test-only.
• The SendToFirstResponsiveTabResult type is exported alongside the function — clean API boundary for tests.
• All four handler call sites follow the same three-branch dispatch pattern (no-tab → error response, throw → verb-prefixed error, response → cast + forward) — consistent.

Overall verdict: pass. The fix is correct, all previous nits are resolved, test coverage now exercises every code path in the helper, and the classifier test pins the new error template. No important bugs found.