fix(cli): validate --via-tab before connecting, like the request URL

[chrischall]

Closes #209 — the nit auto-review raised on #207.

The inconsistency

runFetch validates the request URL against the profile before connecting, so a typo is exit 1 with guidance. --via-tab skipped that, so the same class of mistake travelled to the server guard and came back as exit 2 ("bridge error") — after making the user wait on a connection to be told their flag was malformed.

Both now fail identically, before listen():

$ fpx get https://api.x.com/v1 -p x --via-tab 'not a url'
fpx: not a valid URL: "not a url"                                    # exit 1

$ fpx get https://api.x.com/v1 -p x --via-tab https://evil.example/
evil.example is not on this profile's declared domains (…)           # exit 1

Verified by exit code, and the tests assert listen() was never called — a typo shouldn't cost a bridge round-trip.

The server-side guard stays. It's the real boundary for library callers; this just stops the CLI from routing a usage error through it.

Also: corrects the viaTab doc comment

#207 said API hosts "serve no HTML app". That's imprecise, and it doesn't explain why the extension's own advice for the failure — "Refresh the page in your browser to inject the content script" — can't work.

The actual mechanism, confirmed by opening such a tab and inspecting it:

{"url":"chrome-error://chromewebdata/",
 "bodyStart":"This api.creditkarma.com page can't be found…"}

The host 404s at /, so Chrome renders its own document at chrome-error://chromewebdata/, and Chrome never injects content scripts into chrome-error:// pages regardless of the <all_urls> match. chrome.tabs.query still reports the tab's URL as the requested https one — which is exactly why the failure reads "1 URL match, none responded". No amount of reloading fixes it.

Full reproduction and the missing control are in the correction comment on #207. Short version: my original evidence for that PR was weaker than it looked — one success with no matching failure run, and a motivation I'd asserted rather than tested. The conclusion held, but I should have run the control before opening it.

Tests

1157 pass (up from 1154). Three new CLI tests: malformed relay tab, off-domain relay tab, and the accepted case — the first two also asserting no connection attempt.

🤖 Generated with Claude Code

https://claude.ai/code/session_012o2nXwu7tov6j7ciBEpigo

[claude]

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

---

Review complete

• Read CLAUDE.md for conventions
• Read PR diff
• Review packages/cli/src/verbs/fetch.ts changes
• Review packages/server/src/ws-server.ts changes
• Review test coverage in packages/cli/tests/fetch.test.ts
• Post findings and verdict

Posted a full summary review: #210 (comment)

Summary: The fix correctly moves --via-tab validation before the bridge connection, reusing the existing assertUrlOnProfile helper (same domain-matching semantics as the request URL). Traced the control flow confirming the UsageError propagates uncaught to main.ts's handler and maps to exit code 1, matching the PR's claim. New tests correctly assert server.listen is never called for the two rejection cases. The ws-server.ts change is a pure doc-comment correction — verified no logic changed there. No issues found. Could not execute the test suite in this sandbox (Bash approval not granted), noted as a limitation rather than a finding.

Verdict: pass

[claude]

Review

Reviewed the diff against origin/main (git diff origin/main...HEAD) across all three changed files.

packages/cli/src/verbs/fetch.ts — Adds if (cmd.viaTab !== undefined) assertUrlOnProfile(cmd.viaTab, profile); right after the existing URL check, before the server is constructed. This reuses the pre-existing assertUrlOnProfile helper (unchanged in this PR) rather than adding new validation logic, so the domain-matching semantics for --via-tab are identical to the request URL's. Since this throws before the try { server.listen() ... } block, it propagates as an uncaught UsageError, which main.ts's handler (if (err instanceof UsageError), line 128) maps to exit code 1 with the hint — exactly the claimed behavior.

packages/cli/tests/fetch.test.ts — Three new tests cover malformed --via-tab, off-domain --via-tab, and the accepted case, and the first two assert server.listen was never called, which is the right way to prove the fix (no bridge round-trip for a usage error). I confirmed by reading runFetch that the ordering matches: validation happens before makeServer(...)/listen() are ever reached.

packages/server/src/ws-server.ts — Diff is purely a doc-comment rewrite on RequestOpts.viaTab; confirmed no logic changed (assertUrlInDomains / the viaTab handling in request() at lines ~1806-1816 is byte-identical to main). The corrected mechanism (API host 404 → chrome-error://chromewebdata/ → no content-script injection) is a documentation-only claim about browser behavior, not independently verifiable here, but it doesn't affect code correctness either way.

No correctness, convention, or test-coverage issues found. I was not able to execute npm test/vitest in this sandbox (Bash approval required and not granted), so I did not independently re-run the suite — this is a limitation of my environment, not a finding against the PR.

Verdict: pass

[github-actions]

✅ Auto-review verdict: pass — PR correctly validates --via-tab against the profile's domains before connecting, reusing the existing assertUrlOnProfile helper; tests properly assert no bridge connection occurs on rejection, and the ws-server.ts change is a pure doc-comment correction with no logic changes.