diff --git a/docs/all-commands-qa.md b/docs/all-commands-qa.md index 915c430..276072a 100644 --- a/docs/all-commands-qa.md +++ b/docs/all-commands-qa.md @@ -40,10 +40,10 @@ Create `UPLOAD_FILE` with any small text payload. Capture IDs from JSON output w - [ ] Run `$CLI connect`; expect an already-approved rejection that identifies the extension instance. - [ ] Run `$CLI window new "$BASE" --json`; save `WINDOW` and `TAB`. - [ ] Run `$CLI window --json`; expect `WINDOW` in the window list. -- [ ] Run `$CLI window select "id:$WINDOW" --json`; expect `WINDOW` to be selected. +- [ ] Run `$CLI window select "id:$WINDOW" --json`; expect `WINDOW` to be brought forward without establishing a durable target. - [ ] Run `$CLI tab --window "id:$WINDOW" --json`; expect `TAB` in the tab list. - [ ] Run `$CLI tab new "${BASE}?tab2" --window "id:$WINDOW" --json`; save `TAB2`. -- [ ] Run `$CLI tab select "id:$TAB2" --json`; expect `TAB2` to be active. +- [ ] Run `$CLI tab select "id:$TAB2" --json`; expect `TAB2` to be brought forward without establishing a durable target. - [ ] Run `$CLI open "${BASE}?open" --tab "id:$TAB2" --json`; expect `TAB2` to navigate. - [ ] Run `$CLI open --new-tab "${BASE}?tab3" --window "id:$WINDOW" --json`; save `TAB3`. - [ ] Run `$CLI back --tab "id:$TAB2" --json`; expect navigation success. @@ -132,12 +132,12 @@ Create `UPLOAD_FILE` with any small text payload. Capture IDs from JSON output w - [ ] Run `$CLI errors clear --tab "id:$TAB2" --json`; expect success. - [ ] Run `$CLI errors list --tab "id:$TAB2" --json`; expect an error-list result. - [ ] Run `$CLI highlight "#highlight-target" --tab "id:$TAB2" --json`; expect success. -- [ ] Run `$CLI set viewport 1000 700 --tab "id:$TAB2" --json`; expect window dimensions in the response. +- [ ] Run `$CLI set viewport 1000 700 --window "id:$WINDOW" --json`; expect window dimensions in the response. - [ ] Run `$CLI diff title "firefox-cli disposable E2E" --tab "id:$TAB2" --json`; expect `"matches": true`. - [ ] Run `$CLI diff url "${BASE}?open" --tab "id:$TAB2" --json`; expect `"matches": true`. - [ ] Run `$CLI batch '[["fill","#email","batch@example.test"],["click","#submit"],["wait","--text","Submitted batch@example.test","--timeout","5000"],["get","text","#status"]]' --tab "id:$TAB2" --json`; expect all batch steps to pass. - [ ] Run `printf '[["get","title"]]' | $CLI batch --stdin --tab "id:$TAB2" --json`; expect the stdin batch step to pass. -- [ ] Run `$CLI pdf "$PDF" --tab "id:$TAB2" --json`; expect `UNSUPPORTED_CAPABILITY`. +- [ ] Run `$CLI pdf "$PDF" --json`; expect `UNSUPPORTED_CAPABILITY`. - [ ] Run `$CLI close`; expect `UNSUPPORTED_CAPABILITY`. - [ ] Run `$CLI quit`; expect `UNSUPPORTED_CAPABILITY`. - [ ] Run `$CLI exit`; expect `UNSUPPORTED_CAPABILITY`. diff --git a/docs/architecture.md b/docs/architecture.md index 068da58..c2863e5 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -21,7 +21,7 @@ The first approval request creates a pair token. The extension stores the token ## Targeting -Commands resolve targets inside the extension. Default target selection uses the active tab/window. Explicit targets use the indexes printed by `tab`/`window` or Firefox IDs with `id:`. +Commands resolve targets inside the extension. Omitted selectors are accepted only when the required window or tab is unique. Explicit selectors use `active`, the indexes printed by `tab`/`window`, or Firefox IDs with `id:`; a tab ID also identifies its owning window. Private windows are guarded at the extension command boundary. Read-only listings can report private tabs/windows; commands that would mutate private browsing state are rejected. diff --git a/docs/commands.md b/docs/commands.md index 17b226d..9fbb7d7 100644 --- a/docs/commands.md +++ b/docs/commands.md @@ -4,7 +4,7 @@ All commands use the paired Firefox session. Add `--json` for structured output ## Targets -Commands that operate on a page accept: +Commands advertise their supported selectors in `-h`. Selector values are: ```sh --tab active @@ -17,6 +17,10 @@ Commands that operate on a page accept: Bare numeric targets are indexes printed by `firefox-cli tab` and `firefox-cli window`; `id:` targets use Firefox tab or window IDs. +Omitted selectors are accepted only when the required browser surface is unique. Multiple windows require `--window id:` or explicit `--window active`; multiple tabs in a selected window require `--tab id:` or explicit `--tab active`. A globally unique tab ID also identifies its owning window. + +Page-targeted commands accept both selectors. `tab`, `tab new`, `window select`, `window close`, and `set viewport` accept `--window` only. `open --new-tab` resolves only a window. Targetless commands accept neither. Unsupported selector flags fail before a request is sent. + Private windows are listed and readable. Mutating commands against private windows return `UNSUPPORTED_CAPABILITY`. ## Setup And Diagnostics @@ -34,14 +38,14 @@ Private windows are listed and readable. Mutating commands against private windo | Command | Behavior | | --- | --- | -| `firefox-cli tab [--json]` | List tabs. | -| `firefox-cli tab new [url] [--json]` | Open a new tab. | -| `firefox-cli tab select [target] [--json]` | Select a tab. | +| `firefox-cli tab [--window target] [--json]` | List tabs in the selected or only window. | +| `firefox-cli tab new [url] [--window target] [--json]` | Open a new tab in the selected window. | +| `firefox-cli tab select [target] [--json]` | Bring a tab forward to the user; later commands still require explicit selectors when ambiguous. | | `firefox-cli tab close [target] [--json]` | Close a tab. | | `firefox-cli window [--json]` | List windows. | | `firefox-cli window new [url] [--json]` | Open a new window. | -| `firefox-cli window select [target] [--json]` | Select a window. | -| `firefox-cli window close [target] [--json]` | Close a window. | +| `firefox-cli window select [target] [--window target] [--json]` | Bring a window forward to the user; later commands still require explicit selectors when ambiguous. | +| `firefox-cli window close [target] [--window target] [--json]` | Close a selected window. | | `firefox-cli open [--new-tab] [--json]` | Navigate the target tab or open a URL in a new tab. | | `firefox-cli back|forward|reload [--json]` | Run browser navigation in the target tab. | @@ -131,9 +135,9 @@ firefox-cli screenshot [path] [--format png|jpeg] [--screenshot-quality 1-100] [ firefox-cli batch | --stdin [--bail] [--timeout ms] [--max-output bytes] [--json] ``` -`eval` runs in the page main world and returns JSON-serializable values or `undefined`. Screenshot output captures the visible tab as PNG or JPEG; `--full` returns `UNSUPPORTED_CAPABILITY`. +`eval` runs in the page main world and returns JSON-serializable values or `undefined`. Screenshot output captures the visible tab as PNG or JPEG and may activate the selected tab/window; `--full` returns `UNSUPPORTED_CAPABILITY`. -`batch` accepts an array of protocol command objects or CLI argv arrays. Steps run serially. With `--bail`, execution stops after the first failed step; without it, later steps continue and the batch result reports failures. +`batch` accepts an array of protocol command objects or CLI argv arrays. Steps run serially against a once-resolved default target; window-only batches do not require a tab. With `--bail`, execution stops after the first failed step; without it, later steps continue and the batch result reports failures. Example: @@ -160,7 +164,7 @@ Example: | `firefox-cli console|errors list|clear [--json]` | List or clear page console/error capture buffers. | | `firefox-cli highlight [--json]` | Outline an element. | | `firefox-cli notify [--id id] [message...] [--json]` | Show a native Firefox notification. | -| `firefox-cli set viewport <width> <height> [--json]` | Request a target browser window resize and report Firefox's observed window dimensions. Tiling/window-manager rules can prevent the requested size from taking effect. | +| `firefox-cli set viewport <width> <height> [--window target] [--json]` | Request a target browser window resize and report Firefox's observed window dimensions. Tiling/window-manager rules can prevent the requested size from taking effect. | | `firefox-cli diff url|title|snapshot <expected> [--json]` | Compare URL, title, or snapshot text with an expected value. | | `firefox-cli pdf <path> [--json]` | Returns `UNSUPPORTED_CAPABILITY`; Firefox saves PDFs through a browser dialog rather than a requested CLI path. | diff --git a/docs/firefox-cli-spec.md b/docs/firefox-cli-spec.md index 19d4663..c6e270f 100644 --- a/docs/firefox-cli-spec.md +++ b/docs/firefox-cli-spec.md @@ -211,14 +211,20 @@ Startup handshake: Target resolution is owned by the extension. -- `active` means Firefox's active tab in the active/focused normal window at command resolution time. +- `active` is an explicit selector. `--window active` chooses Firefox's focused normal window; `--tab active` chooses the active tab within the selected window. - Window and tab IDs use Firefox IDs in JSON output. - Human-facing indexes are derived from deterministic `browser.windows.getAll({ populate: true })` ordering: focused window first, then remaining windows by Firefox window ID; tabs by index within each window. - Private windows are reported but commands return `UNSUPPORTED_CAPABILITY` unless the extension has private browsing permission and the command is explicitly allowed there. - Container tab metadata should be included when Firefox exposes it, but no persistent container/session model is added. - Each command snapshots its resolved target before execution to avoid active-tab races. +- Selector values are `active`, a non-negative listing index, or `id:<non-negative Firefox ID>`. A route supports neither selector, `--window` only, `--tab` only, or both as advertised by its CLI help; unsupported selector flags fail before dispatch. +- An omitted window selector is accepted only when exactly one normal window exists. A globally unique `--tab id:<id>` also identifies its owning window. Other tab selectors remain window-relative. +- A tab-dependent command accepts an omitted tab selector only when the selected window has exactly one tab. Pass `--tab active` explicitly to choose Firefox's active tab when several tabs exist. +- Ambiguous omission returns `INVALID_TARGET` with `Ambiguous window` or `Ambiguous tab` guidance before browser side effects. +- Page-targeted routes support both dimensions. `tab`, `tab new`, `window select`, `window close`, and `set viewport` are window-scoped. `open --new-tab` resolves only a window. Targetless routes support neither. +- `tab select` and `window select` bring a browser surface forward to the user. They do not establish durable CLI target state or remove the requirement to pass selectors explicitly to later target-dependent commands. -`open <url>` navigates the resolved active tab to match `agent-browser`. Use `tab new [url]` or `open --new-tab <url>` to create a new tab. +`open <url>` navigates the resolved target tab to match `agent-browser`. Use `tab new [url]` or `open --new-tab <url>` to create a new tab. ## Snapshot And Ref Model @@ -278,7 +284,7 @@ Generated DOM events are not trusted user input. Sites that check `event.isTrust Start with visible-tab screenshots. - `screenshot [path]` captures the active visible tab of the target window. -- If a non-active target is requested, the command may activate the target tab/window before capture and must report that side effect in diagnostics. +- If the requested target is not active or focused, the command activates its tab and/or focuses its window before capture and reports those side effects in diagnostics. Invalid target resolution happens before activation. - If activation is impossible or would require unsupported user activation, return `UNSUPPORTED_CAPABILITY`. - Write image bytes through the native host to the requested path; JSON output should include path, format, dimensions when known, and diagnostics. - Support visible-tab PNG and JPEG captures with JPEG quality. @@ -289,7 +295,7 @@ Start with visible-tab screenshots. `batch` executes a serialized command transaction. - Input is a JSON array of argv arrays or command objects. -- Default target is resolved once at batch start unless a step overrides it. +- The required default window and/or tab is resolved once at batch start unless a step overrides it. Window-only batches do not require or inject a tab selector. - Steps share ref registry entries created earlier in the batch. - `--bail` stops at the first failed step. - Output is an ordered result array with per-step success/error, diagnostics, and final exit status. @@ -329,7 +335,7 @@ Global options: - `--json`: emit machine-readable JSON. - `--timeout <ms>`: override command timeout. - `--max-output <chars>`: cap text output. -- `--window <id|index|active>` and `--tab <id|index|active>`: choose a target without hidden sessions. +- `--window <target>` and `--tab <target>`: route-specific target selectors; use only flags advertised by that route's help. - `--debug`: include transport/protocol diagnostics. Setup and diagnostics: @@ -411,12 +417,12 @@ Unsupported unless Firefox provides an equivalent: ## MVP Command Contracts -Every MVP command returns `{ target, diagnostics }` in JSON output. `target` includes resolved window ID, tab ID, URL when available, and title when available. Text output may omit fields that are not useful for agents. +Target-dependent MVP command results include resolved window/tab metadata where their result schema exposes it. Text output may omit fields that are not useful for agents. Navigation: -- `open <url> [--new-tab]`: params are normalized URL and new-tab flag. Result is final target, URL, navigation status, and load state when known. Errors include invalid URL, no active tab, permission denied, navigation timeout, and unsupported restricted page. -- `back`, `forward`, `reload`: params are empty except target and timeout. Result is final URL and load state when known. Errors include no history entry, no active tab, navigation timeout, and restricted page. +- `open <url> [--new-tab]`: params are normalized URL and new-tab flag. Result is final target, URL, navigation status, and load state when known. Errors include invalid URL, ambiguous or missing target, permission denied, navigation timeout, and unsupported restricted page. +- `back`, `forward`, `reload`: params are empty except target and timeout. Result is final URL and load state when known. Errors include no history entry, ambiguous or missing target, navigation timeout, and restricted page. Snapshot: @@ -453,14 +459,14 @@ Waits: Tabs and windows: -- `tab`: lists tabs for the resolved or active window. Result includes ID, index, active flag, title, URL when permitted, window ID, private flag, and container metadata when available. +- `tab`: lists tabs for the selected or only window. Result includes ID, index, active flag, title, URL when permitted, window ID, private flag, and container metadata when available. - `tab new [url]`: creates a new tab. Result is created tab target and URL when provided. - `tab close [id|index|active]`: closes a tab. Result is closed tab ID and next active tab when known. -- `tab select <id|index>`: activates a tab. Result is selected target. +- `tab select [id|index|active]`: brings a tab forward to the user. Result is the selected target; later target-dependent commands still require explicit selectors when their target is ambiguous. - `window`: lists normal windows and their active tabs. Result includes Firefox window IDs, focus/active flags, bounds when available, and tab count. - `window new [url]`: creates a new window. Result is created window ID and active tab ID. - `window close <id|active>`: closes a window. Result is closed window ID. -- `window select <id|index>`: focuses a window. Result is selected window ID and active tab. +- `window select [id|index|active]`: brings a window forward to the user. Result includes refreshed focus metadata and its active tab; later target-dependent commands still require explicit selectors when their target is ambiguous. Screenshots: diff --git a/packages/cli/src/argv-contracts.ts b/packages/cli/src/argv-contracts.ts index ecee97f..88472e7 100644 --- a/packages/cli/src/argv-contracts.ts +++ b/packages/cli/src/argv-contracts.ts @@ -1,7 +1,9 @@ +import { getCliRouteEntries, type CliRouteSelectorDimensions } from "@firefox-cli/protocol"; import { CliUsageError, type CliRouteParserSpec } from "./types.js"; const targetValueOptions = ["--window", "--tab"] as const; const jsonFlags = ["--json"] as const; +const selectorDimensionsByRouteId = new Map(getCliRouteEntries().map(({ route }) => [route.id, route.selectorDimensions])); export const routeParserSpecs = { capabilities: parser("capabilities"), @@ -118,6 +120,7 @@ export const routeParserSpecs = { export type CliRouteParserSpecById = typeof routeParserSpecs; export type CliRouteParserRouteId = keyof CliRouteParserSpecById; +const routeParserSpecsById = new Map<string, CliRouteParserSpec>(Object.entries(routeParserSpecs)); export interface ParsedCliRouteArgs { readonly positionals: readonly string[]; @@ -140,12 +143,16 @@ interface CliRouteArgContext { readonly state: CliRouteArgParserState; } -export function parseCliRouteArgsForRoute(routeId: CliRouteParserRouteId, args: readonly string[]): ParsedCliRouteArgs { - return parseCliRouteArgs(routeParserSpecs[routeId], routeId, args); +export function parseCliRouteArgsForRoute(routeId: string, args: readonly string[]): ParsedCliRouteArgs { + const parserSpec = routeParserSpecsById.get(routeId); + if (parserSpec === undefined) { + throw new Error(`CLI parser references unknown route: ${routeId}`); + } + return parseCliRouteArgs(withRouteSelectorOptions(parserSpec, routeId), routeId, args); } export function parseCliRouteArgv(parserSpec: CliRouteParserSpec, routeId: string, argv: readonly string[]): ParsedCliRouteArgs { - return parseCliRouteArgs(parserSpec, routeId, argv.slice(1)); + return parseCliRouteArgs(withRouteSelectorOptions(parserSpec, routeId), routeId, argv.slice(1)); } function parseCliRouteArgs(parserSpec: CliRouteParserSpec, routeId: string, args: readonly string[]): ParsedCliRouteArgs { @@ -187,20 +194,37 @@ function parseCliRouteArgs(parserSpec: CliRouteParserSpec, routeId: string, args continue; } - if (arg.startsWith("-")) { - if (canTreatUnknownOptionAsPayload(parserSpec, state.positionals.length)) { - state.positionals.push(arg); - continue; - } + if (isTargetValueOption(arg)) { throw new CliUsageError(`Unsupported ${parserSpec.label} option: ${arg}`); } + if (arg.startsWith("-")) { + handleUnknownOption(context, arg); + continue; + } + state.positionals.push(arg); } return { positionals: state.positionals, optionArgs: state.optionArgs, json: state.json }; } +function handleUnknownOption(context: CliRouteArgContext, arg: string): void { + const { parserSpec, state } = context; + if (isTargetValueOption(arg) || !canTreatUnknownOptionAsPayload(parserSpec, state.positionals.length)) { + throw new CliUsageError(`Unsupported ${parserSpec.label} option: ${arg}`); + } + state.positionals.push(arg); +} + +export function rejectTargetSelectorOptions(args: readonly string[], label: string): void { + for (const arg of args) { + if (isTargetValueOption(arg)) { + throw new CliUsageError(`Unsupported ${label} option: ${arg}`); + } + } +} + function handleKnownFlag(context: CliRouteArgContext, arg: string, index: number): void { const { parserSpec, args, state } = context; if (shouldTreatKnownOptionAsPayload({ parserSpec, args, index, width: 1, currentPositionals: state.positionals.length })) { @@ -261,13 +285,42 @@ function parser( return { label, flags: [...jsonFlags, ...(options.flags ?? [])], - valueOptions: [...targetValueOptions, ...(options.valueOptions ?? [])], + valueOptions: [...(options.valueOptions ?? [])], ...(options.optionalValueOptions === undefined ? {} : { optionalValueOptions: options.optionalValueOptions }), ...(options.payload === undefined ? {} : { payload: options.payload }), ...(options.allowDashDashPayload === undefined ? {} : { allowDashDashPayload: options.allowDashDashPayload }), }; } +function withRouteSelectorOptions(parserSpec: CliRouteParserSpec, routeId: string): CliRouteParserSpec { + const selectorDimensions = selectorDimensionsByRouteId.get(routeId); + if (selectorDimensions === undefined) { + throw new Error(`CLI parser references unknown protocol route: ${routeId}`); + } + + return { + ...parserSpec, + valueOptions: [...selectorValueOptions(selectorDimensions), ...parserSpec.valueOptions], + }; +} + +function selectorValueOptions(selectorDimensions: CliRouteSelectorDimensions): readonly string[] { + if (selectorDimensions === "neither") { + return []; + } + if (selectorDimensions === "window") { + return [targetValueOptions[0]]; + } + if (selectorDimensions === "tab") { + return [targetValueOptions[1]]; + } + return targetValueOptions; +} + +function isTargetValueOption(arg: string): arg is (typeof targetValueOptions)[number] { + return arg === targetValueOptions[0] || arg === targetValueOptions[1]; +} + function shouldTreatKnownOptionAsPayload(context: { readonly parserSpec: CliRouteParserSpec; readonly args: readonly string[]; @@ -299,7 +352,7 @@ function buildOptionInventory(specs: Readonly<Record<string, CliRouteParserSpec> readonly optionalValueOptions: ReadonlySet<string>; } { const flags = new Set<string>(); - const valueOptions = new Set<string>(); + const valueOptions = new Set<string>(targetValueOptions); const optionalValueOptions = new Set<string>(); for (const spec of Object.values(specs)) { for (const flag of spec.flags) { diff --git a/packages/cli/src/cli-batch-core.test.ts b/packages/cli/src/cli-batch-core.test.ts index ffa80ce..b29d56e 100644 --- a/packages/cli/src/cli-batch-core.test.ts +++ b/packages/cli/src/cli-batch-core.test.ts @@ -1,7 +1,7 @@ -import { createOkResponse } from "@firefox-cli/protocol"; +import { createErrorResponse, createOkResponse, type RequestEnvelope } from "@firefox-cli/protocol"; import { describe, expect, it } from "vitest"; -import { runCli } from "./index.js"; import { actionElement, baseDependencies } from "./cli-test-support.js"; +import { runCli } from "./index.js"; describe("runCli batch core", () => { it("runs batch command objects with bail, target, timeout, and result limits", async () => { @@ -123,6 +123,40 @@ describe("runCli batch core", () => { }); }); + it("preserves omitted selectors in window-only argv steps", async () => { + let sentRequest: RequestEnvelope | undefined; + + await runCli( + [ + "batch", + JSON.stringify([ + ["tab", "new"], + ["window", "select"], + ["window", "close"], + ]), + ], + { + ...baseDependencies(), + sendRequest: async (request) => { + sentRequest = request; + return createErrorResponse(request.id, { + code: "NATIVE_HOST_UNAVAILABLE", + message: "Expected test transport failure.", + }); + }, + }, + ); + + expect(sentRequest?.command).toBe("batch"); + expect(sentRequest?.params).toMatchObject({ + steps: [ + { command: "tab.new", params: {} }, + { command: "window.select", params: {} }, + { command: "window.close", params: {} }, + ], + }); + }); + it("preserves option-like argv payloads in direct and batch commands", async () => { const dialogOutput = await runCli(["dialog", "accept", "--proceed"], { ...baseDependencies(), diff --git a/packages/cli/src/cli-help.test.ts b/packages/cli/src/cli-help.test.ts index cdb62d6..0ab6bdd 100644 --- a/packages/cli/src/cli-help.test.ts +++ b/packages/cli/src/cli-help.test.ts @@ -23,7 +23,7 @@ describe("CLI help", () => { expect(output.exitCode).toBe(0); expect(output.stdout).toContain("Tabs, windows, and navigation"); expect(output.stdout).toContain("firefox-cli tab [--json]"); - expect(output.stdout).toContain("List tabs with indexes"); + expect(output.stdout).toContain("List tabs for the selected or only Firefox window"); expect(output.stdout).toContain("firefox-cli open [--new-tab] <url> [--json]"); }); @@ -37,6 +37,18 @@ describe("CLI help", () => { expect(output.stdout).toContain("firefox-cli snapshot -i"); }); + it.each([ + ["tab", "tab", "firefox-cli tab select"], + ["window", "window", "firefox-cli window select"], + ] as const)("explains that %s selection only brings the surface forward to the user", async (command, surface, usage) => { + const output = await runCli([command, "select", "--help"], baseDependencies()); + + expect(output.exitCode).toBe(0); + expect(output.stdout).toContain(usage); + expect(output.stdout).toContain(`Bring a ${surface} forward to the user`); + expect(output.stdout).toContain("This does not absolve you from passing `--window`/`--tab` explicitly to every later target-dependent command."); + }); + it("renders wait examples accepted by the wait parser", async () => { const output = await runCli(["wait", "--help"], baseDependencies()); diff --git a/packages/cli/src/cli-tabs-targets.test.ts b/packages/cli/src/cli-tabs-targets.test.ts index 19432ae..9292157 100644 --- a/packages/cli/src/cli-tabs-targets.test.ts +++ b/packages/cli/src/cli-tabs-targets.test.ts @@ -1,10 +1,11 @@ import { writeFile } from "node:fs/promises"; import { join } from "node:path"; -import { createTempDir } from "@firefox-cli/test-support"; import { createErrorResponse, createOkResponse } from "@firefox-cli/protocol"; +import { createTempDir } from "@firefox-cli/test-support"; import { describe, expect, it } from "vitest"; -import { runCli } from "./index.js"; import { actionElement, baseDependencies, targetSummary } from "./cli-test-support.js"; +import { runCli } from "./index.js"; +import { findCliRouteBindingForArgv } from "./route-registry.js"; describe("runCli tabs and targets", () => { it("lists Firefox tabs as JSON", async () => { @@ -145,7 +146,8 @@ describe("runCli tabs and targets", () => { expect(output).toEqual({ exitCode: 0, - stdout: "w7 t42 [0] Example https://example.com/\n", + stdout: + "w7 t42 [0] Example https://example.com/\nBrought this tab forward to the user. This does not absolve you from passing `--window`/`--tab` explicitly to every later target-dependent command.\n", stderr: "", }); }); @@ -210,6 +212,10 @@ describe("runCli tabs and targets", () => { for (const flag of flags) { for (const testCase of cases) { + const binding = findCliRouteBindingForArgv(testCase.argv); + if (binding === undefined || !supportsSelectorFlag(binding.route.selectorDimensions, flag)) { + continue; + } await expect( runCli([...testCase.argv, flag], { ...baseDependencies(), @@ -338,3 +344,10 @@ describe("runCli tabs and targets", () => { }); }); }); + +function supportsSelectorFlag(selectorDimensions: "neither" | "window" | "tab" | "both", flag: "--tab" | "--window"): boolean { + return ( + (flag === "--tab" && (selectorDimensions === "tab" || selectorDimensions === "both")) || + (flag === "--window" && (selectorDimensions === "window" || selectorDimensions === "both")) + ); +} diff --git a/packages/cli/src/cli-target-contract.test.ts b/packages/cli/src/cli-target-contract.test.ts new file mode 100644 index 0000000..a82eb32 --- /dev/null +++ b/packages/cli/src/cli-target-contract.test.ts @@ -0,0 +1,147 @@ +import { createErrorResponse, getCliRouteEntries, type RequestEnvelope } from "@firefox-cli/protocol"; +import { describe, expect, it } from "vitest"; +import { parseCliRouteArgsForRoute } from "./argv-contracts.js"; +import { baseDependencies } from "./cli-test-support.js"; +import { runCli } from "./index.js"; + +describe("CLI route target contracts", () => { + it("accepts selector dimensions declared by each protocol route", () => { + for (const { route } of getCliRouteEntries()) { + for (const [flag, supported] of [ + ["--window", route.selectorDimensions === "window" || route.selectorDimensions === "both"], + ["--tab", route.selectorDimensions === "tab" || route.selectorDimensions === "both"], + ] as const) { + if (supported) { + expect(parseCliRouteArgsForRoute(route.id, [flag, "id:7"]).optionArgs, `${route.id} ${flag}`).toEqual([flag, "id:7"]); + } else { + expect(() => parseCliRouteArgsForRoute(route.id, [flag, "id:7"]), `${route.id} ${flag}`).toThrow(`Unsupported`); + } + } + } + }); + + it.each([ + ["tab list", ["tab", "--tab", "id:42"]], + ["tab new", ["tab", "new", "--tab", "id:42"]], + ["window select", ["window", "select", "--tab", "id:42"]], + ["window close", ["window", "close", "--tab", "id:42"]], + ["set viewport", ["set", "viewport", "1200", "800", "--tab", "id:42"]], + ["targetless capabilities window", ["capabilities", "--window", "id:7"]], + ["targetless capabilities tab", ["capabilities", "--tab", "id:42"]], + ] as const)("rejects unsupported selectors before transport for %s", async (_name, argv) => { + let requestCalls = 0; + + const output = await runCli(argv, { + ...baseDependencies(), + sendRequest: async () => { + requestCalls += 1; + throw new Error(`Unexpected request for ${argv.join(" ")}`); + }, + }); + + expect(output.exitCode).toBe(1); + expect(output.stdout).toBe(""); + expect(output.stderr).toContain("Unsupported"); + expect(requestCalls).toBe(0); + }); + + it.each(["--window", "--tab"] as const)("rejects unsupported doctor selector %s before diagnostics", async (flag) => { + const output = await runCli(["doctor", flag, "id:9"], baseDependencies()); + + expect(output).toEqual({ + exitCode: 1, + stdout: "", + stderr: `Unsupported doctor option: ${flag}\n`, + }); + }); + + it("preserves unsupported doctor selector diagnostics in batch argv", async () => { + const output = await runCli(["batch", JSON.stringify([["doctor", "--window", "id:9"]])], baseDependencies()); + + expect(output).toEqual({ + exitCode: 1, + stdout: "", + stderr: "Invalid batch argv step 0: Unsupported doctor option: --window\n", + }); + }); + + it("serializes supported selectors before and after ordinary and payload positionals", async () => { + const cases: readonly { + readonly name: string; + readonly argv: readonly string[]; + readonly routeLength: number; + readonly dimensions: readonly ("window" | "tab")[]; + }[] = [ + { name: "page navigation", argv: ["open", "https://example.com/"], routeLength: 1, dimensions: ["window", "tab"] }, + { name: "window-only tab creation", argv: ["tab", "new", "https://example.com/"], routeLength: 2, dimensions: ["window"] }, + { name: "payload-bearing interaction", argv: ["fill", "#name", "Nikita"], routeLength: 1, dimensions: ["window", "tab"] }, + { name: "batch request", argv: ["batch", JSON.stringify([["snapshot", "--tab", "id:42"]])], routeLength: 1, dimensions: ["window", "tab"] }, + ]; + + for (const testCase of cases) { + for (const dimension of testCase.dimensions) { + const flag = `--${dimension}`; + for (const placement of ["before", "after"] as const) { + const argv = + placement === "before" + ? [...testCase.argv.slice(0, testCase.routeLength), flag, "id:7", ...testCase.argv.slice(testCase.routeLength)] + : [...testCase.argv, flag, "id:7"]; + let request: RequestEnvelope | undefined; + + await runCli(argv, { + ...baseDependencies(), + sendRequest: async (sent) => { + request = sent; + return createErrorResponse(sent.id, { + code: "NATIVE_HOST_UNAVAILABLE", + message: "Expected test transport failure.", + }); + }, + }); + + expect(request, `${testCase.name} ${dimension} ${placement}`).toMatchObject({ + params: { + target: { + [dimension]: { kind: "id", id: 7 }, + }, + }, + }); + } + } + } + }); + + it("rejects unsupported batch step selectors before transport", async () => { + let requestCalls = 0; + const output = await runCli(["batch", JSON.stringify([["window", "close", "--tab", "id:42"]])], { + ...baseDependencies(), + sendRequest: async () => { + requestCalls += 1; + throw new Error("Unexpected batch request."); + }, + }); + + expect(output.exitCode).toBe(1); + expect(output.stdout).toBe(""); + expect(output.stderr).toContain("Unsupported"); + expect(requestCalls).toBe(0); + }); + + it.each([ + ["notify", ["notify", "title", "--window", "id:7"]], + ["notify batch step", ["batch", JSON.stringify([["notify", "title", "--tab", "id:7"]])]], + ] as const)("rejects target selectors that would otherwise look like payload for %s", async (_name, argv) => { + let requestCalls = 0; + const output = await runCli(argv, { + ...baseDependencies(), + sendRequest: async () => { + requestCalls += 1; + throw new Error("Unexpected request."); + }, + }); + + expect(output.exitCode).toBe(1); + expect(output.stderr).toContain("Unsupported"); + expect(requestCalls).toBe(0); + }); +}); diff --git a/packages/cli/src/cli-target-safety.test.ts b/packages/cli/src/cli-target-safety.test.ts new file mode 100644 index 0000000..e09b783 --- /dev/null +++ b/packages/cli/src/cli-target-safety.test.ts @@ -0,0 +1,70 @@ +import { createErrorResponse, type RequestEnvelope } from "@firefox-cli/protocol"; +import { describe, expect, it } from "vitest"; +import { baseDependencies, targetSummary } from "./cli-test-support.js"; +import { runCli } from "./index.js"; + +describe("runCli target safety", () => { + it("preserves an omitted tab selector for extension ambiguity checks", async () => { + let sentRequest: RequestEnvelope | undefined; + + await runCli(["tab", "select"], { + ...baseDependencies(), + sendRequest: async (request) => { + sentRequest = request; + return createErrorResponse(request.id, { + code: "INVALID_TARGET", + message: "Expected ambiguity test response.", + }); + }, + }); + + expect(sentRequest?.command).toBe("tab.select"); + expect(sentRequest?.params).toEqual({}); + }); + + it("preserves an omitted window selector for extension ambiguity checks", async () => { + let sentRequest: RequestEnvelope | undefined; + + await runCli(["window", "select"], { + ...baseDependencies(), + sendRequest: async (request) => { + sentRequest = request; + return createErrorResponse(request.id, { + code: "INVALID_TARGET", + message: "Expected ambiguity test response.", + }); + }, + }); + + expect(sentRequest?.command).toBe("window.select"); + expect(sentRequest?.params).toEqual({}); + }); + + it("warns that selecting a window only brings it forward to the user", async () => { + const output = await runCli(["window", "select", "id:7"], { + ...baseDependencies(), + sendRequest: async (request) => ({ + protocolVersion: request.protocolVersion, + id: request.id, + ok: true, + result: { + window: { + id: 7, + index: 0, + focused: true, + activeTabId: 42, + tabCount: 1, + }, + target: targetSummary(), + }, + }), + }); + + expect(output).toEqual({ + exitCode: 0, + stdout: + "w7 [0]\nBrought this window forward to the user. This does not absolve you from passing `--window`/`--tab` explicitly to every later target-dependent command.\n", + stderr: "", + }); + }); +}); diff --git a/packages/cli/src/commands/batch.ts b/packages/cli/src/commands/batch.ts index 0213f06..98c2752 100644 --- a/packages/cli/src/commands/batch.ts +++ b/packages/cli/src/commands/batch.ts @@ -1,19 +1,10 @@ -import { - batchParamsSchema, - commandAcceptsProtocolBatchDefaultTarget, - isBatchableCommandId, - MAX_UPLOAD_TOTAL_BYTES, - type BatchParams, - type BatchStep, - type CommandId, - type RequestEnvelope, -} from "@firefox-cli/protocol"; +import { type BatchParams, type BatchStep, batchParamsSchema, isBatchableCommandId, MAX_UPLOAD_TOTAL_BYTES, type RequestEnvelope } from "@firefox-cli/protocol"; import { parseCliRouteArgsForRoute } from "../argv-contracts.js"; import { readProcessStdin } from "../default-dependencies.js"; import { getOptionValue, hasOption, isRecord, optionalTarget, parsePositiveIntegerValue, parseTargetOptions } from "../parse.js"; import { createValidatedRequest } from "../protocol-validation.js"; +import { type CliDependencies, type CliRequestBuildContext, CliUsageError, InvalidBatchArgvCommandError } from "../types.js"; import { createUploadBudget, parseUploadArguments, statUploadFiles, uploadTotalTooLarge } from "../upload.js"; -import { CliUsageError, InvalidBatchArgvCommandError, type CliDependencies, type CliRequestBuildContext } from "../types.js"; interface ParsedBatchArguments { readonly optionArgs: readonly string[]; @@ -183,40 +174,10 @@ async function batchStepFromArgv(argv: readonly string[], index: number, depende return { command: request.command, - params: stripImplicitBatchTarget(request.command, request.params, argv), + params: request.params, }; } function batchArgvReadsStdin(argv: readonly string[]): boolean { return argv[0] === "eval" && argv.includes("--stdin"); } - -function stripImplicitBatchTarget(command: CommandId, params: unknown, argv: readonly string[]): unknown { - if (!isImplicitBatchDefaultTargetCommand(command) || !isRecord(params)) { - return params; - } - - if (hasExplicitTargetInBatchArgv(command, argv)) { - return params; - } - - return Object.fromEntries(Object.entries(params).filter(([key]) => key !== "target")); -} - -function isImplicitBatchDefaultTargetCommand(command: CommandId): boolean { - return commandAcceptsProtocolBatchDefaultTarget(command); -} - -function hasExplicitTargetInBatchArgv(command: CommandId, argv: readonly string[]): boolean { - if (command === "tab.select" || command === "tab.close") { - const positionals = parseCliRouteArgsForRoute(command, argv.slice(1)).positionals; - return positionals[1] !== undefined || hasOption(argv, "--tab") || hasOption(argv, "--window"); - } - - if (command === "window.select" || command === "window.close") { - const positionals = parseCliRouteArgsForRoute(command, argv.slice(1)).positionals; - return positionals[1] !== undefined || hasOption(argv, "--window"); - } - - return false; -} diff --git a/packages/cli/src/commands/tabs-windows.ts b/packages/cli/src/commands/tabs-windows.ts index 96220fb..e6ceb09 100644 --- a/packages/cli/src/commands/tabs-windows.ts +++ b/packages/cli/src/commands/tabs-windows.ts @@ -23,13 +23,15 @@ export function buildTabsRequest(argv: readonly string[]): RequestEnvelope { }); } if (subcommand === "select") { + const selectedTarget = mergeTarget(target, parseOptionalTabTarget(positional[1])); return createValidatedRequest("tab.select", { - target: mergeTarget(target, parseOptionalTabTarget(positional[1], target)), + ...optionalTarget(selectedTarget), }); } if (subcommand === "close") { + const selectedTarget = mergeTarget(target, parseOptionalTabTarget(positional[1])); return createValidatedRequest("tab.close", { - target: mergeTarget(target, parseOptionalTabTarget(positional[1], target)), + ...optionalTarget(selectedTarget), }); } return createValidatedRequest("tabs.list", { @@ -48,13 +50,15 @@ export function buildWindowsRequest(argv: readonly string[]): RequestEnvelope { }); } if (subcommand === "select") { + const selectedTarget = mergeTarget(target, parseOptionalWindowTarget(positional[1])); return createValidatedRequest("window.select", { - target: mergeTarget(target, parseOptionalWindowTarget(positional[1], target)), + ...optionalTarget(selectedTarget), }); } if (subcommand === "close") { + const selectedTarget = mergeTarget(target, parseOptionalWindowTarget(positional[1])); return createValidatedRequest("window.close", { - target: mergeTarget(target, parseOptionalWindowTarget(positional[1], target)), + ...optionalTarget(selectedTarget), }); } return createValidatedRequest("windows.list", {}); diff --git a/packages/cli/src/format.ts b/packages/cli/src/format.ts index 2dcce70..3c3b474 100644 --- a/packages/cli/src/format.ts +++ b/packages/cli/src/format.ts @@ -36,7 +36,7 @@ const formatTabList: CliResponseFormatter<"tabs.list"> = (response, json) => { return json ? ok(`${JSON.stringify(response.result, null, 2)}\n`) : ok(response.result.tabs.map(renderTabSummary).join("")); }; -const formatTabTarget: CliResponseFormatter<"tab.new" | "tab.select" | "open" | "back" | "forward" | "reload"> = (response, json) => { +const formatTabTarget: CliResponseFormatter<"tab.new" | "open" | "back" | "forward" | "reload"> = (response, json) => { if (!response.ok) { return error(formatProtocolError(response.error)); } @@ -44,6 +44,18 @@ const formatTabTarget: CliResponseFormatter<"tab.new" | "tab.select" | "open" | return json ? ok(`${JSON.stringify(response.result, null, 2)}\n`) : ok(`${renderTargetSummary(response.result.target)}\n`); }; +export const formatTabSelect: CliResponseFormatter<"tab.select"> = (response, json) => { + if (!response.ok) { + return error(formatProtocolError(response.error)); + } + + return json + ? ok(`${JSON.stringify(response.result, null, 2)}\n`) + : ok( + `${renderTargetSummary(response.result.target)}\nBrought this tab forward to the user. This does not absolve you from passing \`--window\`/\`--tab\` explicitly to every later target-dependent command.\n`, + ); +}; + const formatTabClose: CliResponseFormatter<"tab.close"> = (response, json) => { if (!response.ok) { return error(formatProtocolError(response.error)); @@ -71,7 +83,7 @@ const formatWindowList: CliResponseFormatter<"windows.list"> = (response, json) ); }; -const formatWindowTarget: CliResponseFormatter<"window.new" | "window.select"> = (response, json) => { +const formatWindowTarget: CliResponseFormatter<"window.new"> = (response, json) => { if (!response.ok) { return error(formatProtocolError(response.error)); } @@ -79,6 +91,18 @@ const formatWindowTarget: CliResponseFormatter<"window.new" | "window.select"> = return json ? ok(`${JSON.stringify(response.result, null, 2)}\n`) : ok(`w${String(response.result.window.id)} [${String(response.result.window.index)}]\n`); }; +export const formatWindowSelect: CliResponseFormatter<"window.select"> = (response, json) => { + if (!response.ok) { + return error(formatProtocolError(response.error)); + } + + return json + ? ok(`${JSON.stringify(response.result, null, 2)}\n`) + : ok( + `w${String(response.result.window.id)} [${String(response.result.window.index)}]\nBrought this window forward to the user. This does not absolve you from passing \`--window\`/\`--tab\` explicitly to every later target-dependent command.\n`, + ); +}; + const formatWindowClose: CliResponseFormatter<"window.close"> = (response, json) => { if (!response.ok) { return error(formatProtocolError(response.error)); diff --git a/packages/cli/src/help.ts b/packages/cli/src/help.ts index 166a03e..4fd187f 100644 --- a/packages/cli/src/help.ts +++ b/packages/cli/src/help.ts @@ -18,19 +18,21 @@ interface HelpGroup { const routeHelpSpecs = { capabilities: helpSpec("List supported command families and browser capability metadata."), connect: helpSpec("Request Firefox control approval through a dedicated approval page."), - "tab.list": helpSpec("List tabs with indexes, ids, active state, titles, and URLs.", [ - "Use listed indexes with `--tab <index>` and ids with `--tab id:<id>`.", - ]), + "tab.list": helpSpec("List tabs for the selected or only Firefox window.", ["Use window indexes with `--window <index>` and ids with `--window id:<id>`."]), "tab.new": helpSpec("Open a new tab, optionally at a URL."), - "tab.select": helpSpec("Activate a tab by index, id, or URL substring."), + "tab.select": helpSpec("Bring a tab forward to the user by index, id, or URL substring.", [ + "This does not absolve you from passing `--window`/`--tab` explicitly to every later target-dependent command.", + ]), "tab.close": helpSpec("Close a tab by index, id, or URL substring."), "window.list": helpSpec("List Firefox windows with indexes, ids, focus state, and tab counts.", [ "Use listed indexes with `--window <index>` and ids with `--window id:<id>`.", ]), "window.new": helpSpec("Open a new Firefox window, optionally at a URL."), - "window.select": helpSpec("Focus a Firefox window by index or id."), + "window.select": helpSpec("Bring a window forward to the user by index or id.", [ + "This does not absolve you from passing `--window`/`--tab` explicitly to every later target-dependent command.", + ]), "window.close": helpSpec("Close a Firefox window by index or id."), - open: helpSpec("Navigate the active tab or create a new tab for a URL.", ["Use `--new-tab` when navigation must not replace the active page."]), + open: helpSpec("Navigate a selected tab or create a new tab for a URL.", ["Use `--new-tab` when navigation must not replace an existing page."]), back: helpSpec("Go back in the target tab history."), forward: helpSpec("Go forward in the target tab history."), reload: helpSpec("Reload the target tab."), @@ -277,7 +279,8 @@ export function renderHelp(): string { "Guidance:", " Use `--json` when another program or agent consumes results.", " Use `firefox-cli snapshot -i` before element actions to get stable `@ref` handles.", - " Use `--tab` and `--window` with active, index values, or id:<id> to target a browser surface.", + " Use only the `--tab` and `--window` options advertised by each command; values are active, index, or id:<id>.", + " Omitted selectors are accepted only when the required window or tab is unique; pass `active` explicitly to choose Firefox's focused/active surface.", " Use `firefox-cli <command> -h` for contextual command help.", "", ].join("\n"); @@ -308,7 +311,8 @@ function renderGroupHelp(group: HelpGroup): string { "", "Guidance:", " Add `--json` for machine-readable output.", - " Use `--tab` and `--window` on browser-targeted commands when active target is not enough.", + " Omitted selectors are accepted only when the required window or tab is unique.", + " Pass `active`, an index, or `id:<id>` through only the selector options advertised by each command.", "", ].join("\n"); } diff --git a/packages/cli/src/parse.ts b/packages/cli/src/parse.ts index d353929..b3776ed 100644 --- a/packages/cli/src/parse.ts +++ b/packages/cli/src/parse.ts @@ -2,6 +2,7 @@ import type { TargetSelector } from "@firefox-cli/protocol"; import { cliArgumentOptionInventory } from "./argv-contracts.js"; import { readFlagValue } from "./parse-options.js"; import { CliUsageError } from "./types.js"; + export { getOptionValue, isOneOf, @@ -225,20 +226,20 @@ export function parseTargetValue(value: string | undefined): NonNullable<TargetS return prefix === "id" ? { kind: "id", id: parsed } : { kind: "index", index: parsed }; } -export function parseOptionalTabTarget(value: string | undefined, base: TargetSelector): TargetSelector { +export function parseOptionalTabTarget(value: string | undefined): TargetSelector { if (value !== undefined) { return { tab: parseTargetValue(value) }; } - return base.tab === undefined ? { tab: { kind: "active" } } : {}; + return {}; } -export function parseOptionalWindowTarget(value: string | undefined, base: TargetSelector): TargetSelector { +export function parseOptionalWindowTarget(value: string | undefined): TargetSelector { if (value !== undefined) { return { window: parseTargetValue(value) }; } - return base.window === undefined ? { window: { kind: "active" } } : {}; + return {}; } export function mergeTarget(base: TargetSelector, override: TargetSelector): TargetSelector { diff --git a/packages/cli/src/route-registry.test.ts b/packages/cli/src/route-registry.test.ts index fea1f4e..a255a8a 100644 --- a/packages/cli/src/route-registry.test.ts +++ b/packages/cli/src/route-registry.test.ts @@ -61,4 +61,14 @@ describe("CLI route registry", () => { expect(help).toContain(` ${binding.help}`); } }); + + it("advertises only protocol-declared selector dimensions in route usage", () => { + for (const binding of Object.values(cliRouteBindings)) { + const expectsWindow = binding.route.selectorDimensions === "window" || binding.route.selectorDimensions === "both"; + const expectsTab = binding.route.selectorDimensions === "tab" || binding.route.selectorDimensions === "both"; + + expect(binding.help.includes("--window <target>"), binding.route.id).toBe(expectsWindow); + expect(binding.help.includes("--tab <target>"), binding.route.id).toBe(expectsTab); + } + }); }); diff --git a/packages/cli/src/route-registry.ts b/packages/cli/src/route-registry.ts index 70abcd9..7bbcde7 100644 --- a/packages/cli/src/route-registry.ts +++ b/packages/cli/src/route-registry.ts @@ -38,7 +38,7 @@ import { buildPdfRequest, buildSetViewportRequest } from "./commands/phase8.js"; import { buildScreenshotRequest } from "./commands/screenshot.js"; import { buildTabsRequest, buildWindowsRequest } from "./commands/tabs-windows.js"; import { buildWaitRequest } from "./commands/wait.js"; -import { cliResponseFormatters, formatApprovalRequest } from "./format.js"; +import { cliResponseFormatters, formatApprovalRequest, formatTabSelect, formatWindowSelect } from "./format.js"; import { getPositionals } from "./parse.js"; import type { CliRequestBuilder, CliResponseFormatter, CliResponseFormatterKind, CliRouteBinding, CliRouteParserSpec } from "./types.js"; @@ -60,11 +60,11 @@ const routeFormatterSpecs = { capabilities: routeFormatter("capabilities", "capabilities", cliResponseFormatters.capabilities), "tab.list": routeFormatter("tabs.list", "tab-list", cliResponseFormatters["tab-list"]), "tab.new": routeFormatter("tab.new", "tab-target", cliResponseFormatters["tab-target"]), - "tab.select": routeFormatter("tab.select", "tab-target", cliResponseFormatters["tab-target"]), + "tab.select": routeFormatter("tab.select", "tab-target", formatTabSelect), "tab.close": routeFormatter("tab.close", "tab-close", cliResponseFormatters["tab-close"]), "window.list": routeFormatter("windows.list", "window-list", cliResponseFormatters["window-list"]), "window.new": routeFormatter("window.new", "window-target", cliResponseFormatters["window-target"]), - "window.select": routeFormatter("window.select", "window-target", cliResponseFormatters["window-target"]), + "window.select": routeFormatter("window.select", "window-target", formatWindowSelect), "window.close": routeFormatter("window.close", "window-close", cliResponseFormatters["window-close"]), open: routeFormatter("open", "tab-target", cliResponseFormatters["tab-target"]), back: routeFormatter("back", "tab-target", cliResponseFormatters["tab-target"]), @@ -143,7 +143,7 @@ function bindCliRoute<RouteId extends keyof RouteFormatterSpecById>( return { route: routeEntry.route, command: formatter.command, - help, + help: withTargetSelectorUsage(help, routeEntry.route.selectorDimensions), parser, formatterKind: formatter.kind, formatter: formatter.formatter, @@ -151,6 +151,14 @@ function bindCliRoute<RouteId extends keyof RouteFormatterSpecById>( }; } +function withTargetSelectorUsage(help: string, selectorDimensions: CliRouteMetadata["selectorDimensions"]): string { + const selectorUsage = [ + ...(selectorDimensions === "window" || selectorDimensions === "both" ? ["[--window <target>]"] : []), + ...(selectorDimensions === "tab" || selectorDimensions === "both" ? ["[--tab <target>]"] : []), + ]; + return selectorUsage.length === 0 ? help : `${help} ${selectorUsage.join(" ")}`; +} + export const cliRouteBindings = { capabilities: bindCliRoute("capabilities", "firefox-cli capabilities [--json]", buildCapabilitiesRequest), "tab.list": bindCliRoute("tab.list", "firefox-cli tab [--json]", buildTabsRequest), @@ -184,7 +192,7 @@ export const cliRouteBindings = { clipboard: bindCliRoute("clipboard", "firefox-cli clipboard read|write|copy|paste [text-or-selector] [--json]", buildClipboardRequest), cookies: bindCliRoute("cookies", "firefox-cli cookies list|get|set|remove <url> [name] [value] [--json]", buildCookiesRequest), storage: bindCliRoute("storage", "firefox-cli storage local|session get|set|remove|clear [key] [value] [--json]", buildStorageRequest), - network: bindCliRoute("network", "firefox-cli network list|clear [--window target] [--tab target] [--json]", buildNetworkRequest), + network: bindCliRoute("network", "firefox-cli network list|clear [--json]", buildNetworkRequest), console: bindCliRoute("console", "firefox-cli console list|clear [--json]", buildLogRequest), errors: bindCliRoute("errors", "firefox-cli errors list|clear [--json]", buildLogRequest), highlight: bindCliRoute("highlight", "firefox-cli highlight <selector|@ref> [--json]", buildHighlightRequest), diff --git a/packages/cli/src/runner.ts b/packages/cli/src/runner.ts index 02e2ad9..ba27fd3 100644 --- a/packages/cli/src/runner.ts +++ b/packages/cli/src/runner.ts @@ -14,6 +14,7 @@ import { InvalidBatchArgvCommandError, } from "./types.js"; import { createUploadBudget } from "./upload.js"; +import { rejectTargetSelectorOptions } from "./argv-contracts.js"; const gatedCapabilitiesByCommand = new Map(gatedCapabilities.map((capability) => [capability.command, capability] as const)); @@ -43,14 +44,17 @@ async function runCliOrThrow(args: readonly string[], dependencies: CliDependenc } if (args[0] === "setup") { + rejectTargetSelectorOptions(args.slice(1), "setup"); return setup(args.slice(1), dependencies, renderHelp); } if (args[0] === "doctor") { + rejectTargetSelectorOptions(args.slice(1), "doctor"); return doctor(args.slice(1), dependencies); } if (args[0] === "unpair") { + rejectTargetSelectorOptions(args.slice(1), "unpair"); await dependencies.clearPairState?.(); return ok("Pair state cleared. Run `firefox-cli connect` to request approval again.\n"); } @@ -103,6 +107,10 @@ async function buildRequestForBinding<C extends CommandId>( async function buildRequestForArgv(argv: readonly string[], dependencies: CliDependencies, context: CliRequestBuildContext): Promise<RequestEnvelope> { const binding = findCliRouteBindingForArgv(argv); if (binding === undefined) { + const command = argv[0]; + if (command === "setup" || command === "doctor" || command === "unpair") { + rejectTargetSelectorOptions(argv.slice(1), command); + } throw new InvalidBatchArgvCommandError(); } diff --git a/packages/extension/src/background-controller-test-cases.ts b/packages/extension/src/background-controller-test-cases.ts index 0a0f05f..7a76197 100644 --- a/packages/extension/src/background-controller-test-cases.ts +++ b/packages/extension/src/background-controller-test-cases.ts @@ -134,10 +134,6 @@ export async function runCase04() { id: "request-1", ok: true, result: { - target: { - windowId: 7, - tabId: 42, - }, tabs: [ { id: 42, diff --git a/packages/extension/src/browser-command/batch.ts b/packages/extension/src/browser-command/batch.ts index 6219421..6eccfc7 100644 --- a/packages/extension/src/browser-command/batch.ts +++ b/packages/extension/src/browser-command/batch.ts @@ -1,14 +1,16 @@ import { - MAX_BATCH_RESULT_BYTES, - MAX_SCREENSHOT_BYTES, + type BatchResult, + type BatchStepResult, + type CommandId, commandAcceptsBatchTimeout, commandAcceptsExtensionBatchDefaultTarget, createRequest, + getCommandTargetSelectorDimensions, isCommandId, + MAX_BATCH_RESULT_BYTES, + MAX_SCREENSHOT_BYTES, parseBatchStepResultAs, parseCommandParamsAs, - type BatchResult, - type BatchStepResult, type RequestEnvelope, type ResponseEnvelope, type ScreenshotResult, @@ -29,11 +31,7 @@ export async function executeBatch( const startedAt = Date.now(); const timeoutMs = command.params.timeoutMs; const maxResultBytes = command.params.maxResultBytes ?? MAX_BATCH_RESULT_BYTES; - const defaultTarget = (await targetContext.resolveTarget(command.params.target)).target; - const defaultSelector: TargetSelector = { - window: { kind: "id", id: defaultTarget.windowId }, - tab: { kind: "id", id: defaultTarget.tabId }, - }; + const defaultSelector = await resolveBatchDefaultSelector(command, targetContext); const steps: BatchStepResult[] = []; let totalScreenshotBytes = 0; @@ -85,7 +83,7 @@ function assertBatchHasTimeRemaining(timeoutMs: number | undefined, remainingMs: function createBatchStepRequest( command: RequestEnvelope<"batch">, index: number, - defaultSelector: TargetSelector, + defaultSelector: TargetSelector | undefined, remainingMs: number | undefined, ): RequestEnvelope | undefined { const step = command.params.steps[index]; @@ -129,18 +127,108 @@ function checkedScreenshotByteTotal(currentBytes: number, stepResult: BatchStepR return nextBytes; } -export function applyBatchStepDefaults(command: string, rawParams: unknown, defaultTarget: TargetSelector, remainingMs: number | undefined): unknown { +export function applyBatchStepDefaults( + command: string, + rawParams: unknown, + defaultTarget: TargetSelector | undefined, + remainingMs: number | undefined, +): unknown { if (!isRecord(rawParams)) { return rawParams; } return { ...rawParams, - ...(commandAcceptsExtensionBatchDefaultTarget(command) && rawParams.target === undefined ? { target: defaultTarget } : {}), + ...defaultTargetOverride(command, rawParams, defaultTarget), ...timeoutOverride(command, rawParams.timeoutMs, remainingMs), }; } +async function resolveBatchDefaultSelector(command: RequestEnvelope<"batch">, targetContext: BrowserTargetContext): Promise<TargetSelector | undefined> { + const resolution = command.params.steps.reduce<TargetResolution>( + (highest, step) => (isCommandId(step.command) ? highestTargetResolution(highest, requestDefaultTargetResolution(step.command, step.params)) : highest), + "none", + ); + if (resolution === "none") { + return undefined; + } + if (resolution === "window") { + const window = await targetContext.resolveTargetWindow(command.params.target); + return { window: { kind: "id", id: window.id } }; + } + + const target = (await targetContext.resolveTarget(command.params.target)).target; + return { + window: { kind: "id", id: target.windowId }, + tab: { kind: "id", id: target.tabId }, + }; +} + +type TargetResolution = "none" | "window" | "tab"; + +function requestDefaultTargetResolution(command: CommandId, params: unknown): TargetResolution { + if (!commandAcceptsExtensionBatchDefaultTarget(command) || !isRecord(params) || params.target !== undefined) { + return "none"; + } + return requestTargetResolution(command, params); +} + +function requestTargetResolution(command: CommandId, params: unknown): TargetResolution { + const selectorDimensions = getCommandTargetSelectorDimensions(command); + if (selectorDimensions === "neither") { + return "none"; + } + if (selectorDimensions === "window") { + return "window"; + } + if (isWindowOnlyRequest(command, params)) { + return "window"; + } + if (isTargetlessRequest(command, params)) { + return "none"; + } + return "tab"; +} + +function isWindowOnlyRequest(command: CommandId, params: unknown): boolean { + return command === "open" && isRecord(params) && params.newTab === true; +} + +function isTargetlessRequest(command: CommandId, params: unknown): boolean { + if (!isRecord(params)) { + return false; + } + const targetlessWait = command === "wait" && (params.kind === "ms" || params.kind === "download"); + const targetlessClipboard = command === "clipboard" && (params.action === "read" || params.action === "write"); + return targetlessWait || targetlessClipboard; +} + +function highestTargetResolution(left: TargetResolution, right: TargetResolution): TargetResolution { + if (left === "tab" || right === "tab") { + return "tab"; + } + return left === "window" || right === "window" ? "window" : "none"; +} + +function defaultTargetOverride( + command: string, + params: Record<string, unknown>, + defaultTarget: TargetSelector | undefined, +): { readonly target?: TargetSelector } { + if (defaultTarget === undefined || !isCommandId(command)) { + return {}; + } + + const resolution = requestDefaultTargetResolution(command, params); + if (resolution === "none") { + return {}; + } + if (resolution === "window") { + return defaultTarget.window === undefined ? {} : { target: { window: defaultTarget.window } }; + } + return { target: defaultTarget }; +} + function timeoutOverride(command: string, existingTimeout: unknown, remainingMs: number | undefined): { readonly timeoutMs?: number } { if (remainingMs === undefined || !commandAcceptsBatchTimeout(command)) { return {}; diff --git a/packages/extension/src/browser-command/target-context.test.ts b/packages/extension/src/browser-command/target-context.test.ts index 9836213..51c026e 100644 --- a/packages/extension/src/browser-command/target-context.test.ts +++ b/packages/extension/src/browser-command/target-context.test.ts @@ -20,13 +20,13 @@ describe("browser target context", () => { const adapter = new FakeBrowserAdapter([windowSnapshot(10, true, [tabSummary(101, 0, true, 10), tabSummary(102, 1, false, 10)])]); const context = createBrowserTargetContext(adapter); - await expect(context.resolveTarget(undefined)).resolves.toMatchObject({ tab: { id: 101 } }); + await expect(context.resolveTarget({ tab: { kind: "active" } })).resolves.toMatchObject({ tab: { id: 101 } }); await adapter.selectTab(102); - await expect(context.resolveTarget(undefined)).resolves.toMatchObject({ tab: { id: 101 } }); + await expect(context.resolveTarget({ tab: { kind: "active" } })).resolves.toMatchObject({ tab: { id: 101 } }); context.invalidate(); - await expect(context.resolveTarget(undefined)).resolves.toMatchObject({ tab: { id: 102 } }); + await expect(context.resolveTarget({ tab: { kind: "active" } })).resolves.toMatchObject({ tab: { id: 102 } }); expect(adapter.listWindowCalls).toBe(2); }); }); diff --git a/packages/extension/src/browser-command/target-context.ts b/packages/extension/src/browser-command/target-context.ts index fe06bfc..7443455 100644 --- a/packages/extension/src/browser-command/target-context.ts +++ b/packages/extension/src/browser-command/target-context.ts @@ -1,7 +1,7 @@ +import type { ResolvedTarget, TargetSelector } from "@firefox-cli/protocol"; import { withBrowserCommandDeadline } from "./deadline.js"; -import { findWindowById, resolveTarget, resolveWindow, toOrderedWindows } from "./targets.js"; +import { findWindowById, resolveTarget, resolveTargetWindow, resolveWindow, toOrderedWindows } from "./targets.js"; import type { BackgroundBrowserAdapter, OrderedWindow, ResolvedBrowserTarget } from "./types.js"; -import type { ResolvedTarget, TargetSelector } from "@firefox-cli/protocol"; interface DeadlineOptions { readonly deadlineMs?: number; @@ -15,6 +15,7 @@ type ResolveOptions = DeadlineOptions & { export interface BrowserTargetContext { getWindows(options?: DeadlineOptions): Promise<readonly OrderedWindow[]>; resolveTarget(selector: TargetSelector | undefined, options?: ResolveOptions): Promise<ResolvedBrowserTarget>; + resolveTargetWindow(selector: TargetSelector | undefined, options?: DeadlineOptions): Promise<OrderedWindow>; resolveWindow(selector: TargetSelector["window"] | undefined, options?: DeadlineOptions): Promise<OrderedWindow>; resolveFreshTarget(selector: TargetSelector, options?: ResolveOptions): Promise<ResolvedTarget>; findWindowById(windowId: number, options?: DeadlineOptions): Promise<OrderedWindow | undefined>; @@ -40,6 +41,7 @@ export function createBrowserTargetContext(adapter: BackgroundBrowserAdapter): B return { getWindows, resolveTarget: async (selector, options = {}) => resolveTarget(await getWindows(options), selector, resolveTargetOptions(options)), + resolveTargetWindow: async (selector, options = {}) => resolveTargetWindow(await getWindows(options), selector), resolveWindow: async (selector, options = {}) => resolveWindow(await getWindows(options), selector), resolveFreshTarget: async (selector, options = {}) => { windows = undefined; diff --git a/packages/extension/src/browser-command/targets.ts b/packages/extension/src/browser-command/targets.ts index 3634ad4..11af74f 100644 --- a/packages/extension/src/browser-command/targets.ts +++ b/packages/extension/src/browser-command/targets.ts @@ -34,6 +34,19 @@ export function resolveTarget( }; } +export function resolveTargetWindow(windows: readonly OrderedWindow[], selector: TargetSelector | undefined): OrderedWindow { + const tabById = findTargetedTabById(windows, selector); + if (tabById !== undefined) { + return tabById.window; + } + + const window = resolveWindow(windows, selector?.window); + if (selector?.tab !== undefined) { + resolveTab(window, selector.tab); + } + return window; +} + function findTargetedTabById( windows: readonly OrderedWindow[], selector: TargetSelector | undefined, @@ -53,6 +66,17 @@ export function resolveWindow(windows: readonly OrderedWindow[], selector: Targe throw new BrowserCommandError("NO_ACTIVE_TAB", "Firefox has no normal browser windows."); } + if (selector === undefined && windows.length > 1) { + throw new BrowserCommandError( + "INVALID_TARGET", + `Ambiguous window: Firefox has ${String(windows.length)} windows. Pass \`--window id:<id>\` or explicitly choose \`--window active\`.`, + { + reason: "ambiguous-window", + windowIds: windows.map((window) => window.id), + }, + ); + } + if (selector === undefined || selector.kind === "active") { const focused = windows.find((window) => window.focused); if (focused !== undefined) { @@ -86,6 +110,18 @@ export function resolveTab(window: OrderedWindow, selector: TargetSelector["tab" throw new BrowserCommandError("NO_ACTIVE_TAB", "Firefox window has no tabs."); } + if (selector === undefined && window.tabs.length > 1) { + throw new BrowserCommandError( + "INVALID_TARGET", + `Ambiguous tab: Firefox window ${String(window.id)} has ${String(window.tabs.length)} tabs. Pass \`--tab id:<id>\` or explicitly choose \`--tab active\`.`, + { + reason: "ambiguous-tab", + windowId: window.id, + tabIds: window.tabs.map((tab) => tab.id), + }, + ); + } + if (selector === undefined || selector.kind === "active") { const active = window.tabs.find((tab) => tab.active); if (active === undefined) { diff --git a/packages/extension/src/browser-commands-batch-targeting.test.ts b/packages/extension/src/browser-commands-batch-targeting.test.ts new file mode 100644 index 0000000..bcacbf2 --- /dev/null +++ b/packages/extension/src/browser-commands-batch-targeting.test.ts @@ -0,0 +1,45 @@ +import { createRequest } from "@firefox-cli/protocol"; +import { describe, expect, it } from "vitest"; +import { handleBrowserRequest } from "./browser-commands.js"; +import { FakeBrowserAdapter, tabSummary, windowSnapshot } from "./browser-commands-test-utils.js"; + +describe("browser batch target defaults", () => { + it("does not resolve an omitted default when every target-dependent step has an explicit target", async () => { + const adapter = new FakeBrowserAdapter([ + windowSnapshot(10, true, [tabSummary(101, 0, true, 10), tabSummary(102, 1, false, 10)]), + windowSnapshot(20, false, [tabSummary(201, 0, true, 20)]), + ]); + + const response = await handleBrowserRequest( + createRequest( + "batch", + { + steps: [ + { + command: "snapshot", + params: { target: { tab: { kind: "id", id: 201 } }, interactiveOnly: true }, + }, + { + command: "window.select", + params: { target: { window: { kind: "id", id: 10 } } }, + }, + ], + }, + "batch-step-targets-only", + ), + adapter, + ); + + expect(response).toMatchObject({ + ok: true, + result: { + ok: true, + steps: [ + { index: 0, command: "snapshot", ok: true }, + { index: 1, command: "window.select", ok: true, result: { window: { id: 10 } } }, + ], + }, + }); + expect(adapter.contentRequests).toEqual([{ tabId: 201, command: "snapshot" }]); + }); +}); diff --git a/packages/extension/src/browser-commands-batch.test.ts b/packages/extension/src/browser-commands-batch.test.ts index 3c340b7..c9b8799 100644 --- a/packages/extension/src/browser-commands-batch.test.ts +++ b/packages/extension/src/browser-commands-batch.test.ts @@ -41,6 +41,62 @@ describe("browser batch command handling", () => { ]); }); + it("runs window-only steps without requiring an unambiguous tab", async () => { + const adapter = new FakeBrowserAdapter([windowSnapshot(10, true, [tabSummary(101, 0, true, 10), tabSummary(103, 1, false, 10)])]); + + const response = await handleBrowserRequest( + createRequest( + "batch", + { + steps: [ + { command: "tab.new", params: { url: "https://qa.example/" } }, + { command: "window.select", params: {} }, + ], + }, + "batch-window-only", + ), + adapter, + ); + + expect(response).toMatchObject({ + ok: true, + result: { + ok: true, + steps: [ + { index: 0, command: "tab.new", ok: true, result: { target: { windowId: 10, tabId: 102 } } }, + { index: 1, command: "window.select", ok: true, result: { window: { id: 10 } } }, + ], + }, + }); + }); + + it("rejects an omitted batch window before running tab-dependent steps", async () => { + const adapter = new FakeBrowserAdapter([ + windowSnapshot(10, true, [tabSummary(101, 0, true, 10)]), + windowSnapshot(20, false, [tabSummary(201, 0, true, 20)]), + ]); + + const response = await handleBrowserRequest( + createRequest( + "batch", + { + steps: [{ command: "snapshot", params: { interactiveOnly: true } }], + }, + "batch-ambiguous-window", + ), + adapter, + ); + + expect(response).toMatchObject({ + ok: false, + error: { + code: "INVALID_TARGET", + message: "Ambiguous window: Firefox has 2 windows. Pass `--window id:<id>` or explicitly choose `--window active`.", + }, + }); + expect(adapter.contentRequests).toEqual([]); + }); + it("preserves step target overrides", async () => { const adapter = new FakeBrowserAdapter([ windowSnapshot(10, true, [tabSummary(101, 0, true, 10)]), @@ -51,6 +107,7 @@ describe("browser batch command handling", () => { createRequest( "batch", { + target: { tab: { kind: "id", id: 101 } }, steps: [ { command: "snapshot", params: {} }, { diff --git a/packages/extension/src/browser-commands-targets.test.ts b/packages/extension/src/browser-commands-targets.test.ts index ee2b92a..9bbd4af 100644 --- a/packages/extension/src/browser-commands-targets.test.ts +++ b/packages/extension/src/browser-commands-targets.test.ts @@ -1,4 +1,6 @@ -import { describe, it } from "vitest"; +import { createRequest } from "@firefox-cli/protocol"; +import { describe, expect, it } from "vitest"; +import { handleBrowserRequest } from "./browser-commands.js"; import { runCase01, runCase02, @@ -11,6 +13,7 @@ import { runCase09, runCase10, } from "./browser-commands-targets-test-cases.js"; +import { FakeBrowserAdapter, tabSummary, windowSnapshot } from "./browser-commands-test-utils.js"; describe("browser command handling", () => { it("routes element state checks to the resolved tab content script and adds target metadata", runCase01); @@ -23,4 +26,211 @@ describe("browser command handling", () => { it("uses the resolved target tab for network-idle waits", runCase08); it("lists and clears network requests for the resolved target tab only", runCase09); it("runs eval in the resolved tab main world and adds target metadata", runCase10); + + it("routes explicit-window navigation, reads, interactions, and screenshots to that window's active tab", async () => { + const adapter = new FakeBrowserAdapter([ + windowSnapshot(10, true, [tabSummary(101, 0, true, 10)]), + windowSnapshot(20, false, [tabSummary(201, 0, true, 20)]), + ]); + const target = { window: { kind: "id" as const, id: 20 } }; + + const responses = [ + await handleBrowserRequest(createRequest("open", { url: "https://qa.example/", newTab: false, target }, "qa-open"), adapter), + await handleBrowserRequest(createRequest("snapshot", { interactiveOnly: true, compact: true, target }, "qa-snapshot"), adapter), + await handleBrowserRequest(createRequest("console", { action: "list", target }, "qa-console"), adapter), + await handleBrowserRequest(createRequest("click", { selector: "#save", target }, "qa-click"), adapter), + await handleBrowserRequest(createRequest("screenshot", { path: "/tmp/qa.png", format: "png", target }, "qa-screenshot"), adapter), + ]; + + for (const response of [responses[0], responses[1], responses[3], responses[4]]) { + expect(response).toMatchObject({ ok: true, result: { target: { windowId: 20, tabId: 201 } } }); + } + expect(responses[2]).toMatchObject({ ok: true, result: { action: "list" } }); + expect(adapter.navigations).toEqual([{ tabId: 201, url: "https://qa.example/" }]); + expect(adapter.contentRequests).toEqual([ + { tabId: 201, command: "snapshot" }, + { tabId: 201, command: "console" }, + { tabId: 201, command: "click" }, + ]); + expect(adapter.captureRequests).toEqual([{ windowId: 20, options: { format: "png" } }]); + expect(adapter.selectedTabs).toEqual([]); + expect(adapter.focusedWindows).toEqual([20]); + expect(adapter.navigations.some((navigation) => navigation.tabId === 101)).toBe(false); + expect(adapter.contentRequests.some((request) => request.tabId === 101)).toBe(false); + }); + + it("rejects an omitted window selector when multiple windows exist", async () => { + const adapter = new FakeBrowserAdapter([ + windowSnapshot(10, true, [tabSummary(101, 0, true, 10)]), + windowSnapshot(20, false, [tabSummary(201, 0, true, 20)]), + ]); + + const response = await handleBrowserRequest(createRequest("open", { url: "https://qa.example/", newTab: false }, "ambiguous-window"), adapter); + + expect(response).toMatchObject({ + ok: false, + error: { + code: "INVALID_TARGET", + message: "Ambiguous window: Firefox has 2 windows. Pass `--window id:<id>` or explicitly choose `--window active`.", + }, + }); + expect(adapter.navigations).toEqual([]); + }); + + it("rejects an omitted tab selector when the selected window has multiple tabs", async () => { + const adapter = new FakeBrowserAdapter([windowSnapshot(10, true, [tabSummary(101, 0, true, 10), tabSummary(102, 1, false, 10)])]); + + const response = await handleBrowserRequest( + createRequest("open", { url: "https://qa.example/", newTab: false, target: { window: { kind: "id", id: 10 } } }, "ambiguous-tab"), + adapter, + ); + + expect(response).toMatchObject({ + ok: false, + error: { + code: "INVALID_TARGET", + message: "Ambiguous tab: Firefox window 10 has 2 tabs. Pass `--tab id:<id>` or explicitly choose `--tab active`.", + }, + }); + expect(adapter.navigations).toEqual([]); + }); + + it("accepts explicit active selectors when several windows and tabs exist", async () => { + const adapter = new FakeBrowserAdapter([ + windowSnapshot(10, true, [tabSummary(101, 0, true, 10), tabSummary(102, 1, false, 10)]), + windowSnapshot(20, false, [tabSummary(201, 0, true, 20)]), + ]); + + const response = await handleBrowserRequest( + createRequest( + "open", + { + url: "https://qa.example/", + newTab: false, + target: { + window: { kind: "active" }, + tab: { kind: "active" }, + }, + }, + "explicit-active", + ), + adapter, + ); + + expect(response).toMatchObject({ + ok: true, + result: { + target: { windowId: 10, tabId: 101 }, + }, + }); + expect(adapter.navigations).toEqual([{ tabId: 101, url: "https://qa.example/" }]); + }); + + it("lists tabs in an explicitly selected window without requiring a tab selector", async () => { + const adapter = new FakeBrowserAdapter([windowSnapshot(10, true, [tabSummary(101, 0, true, 10), tabSummary(102, 1, false, 10)])]); + + const response = await handleBrowserRequest(createRequest("tabs.list", { target: { window: { kind: "id", id: 10 } } }, "list-tabs-window"), adapter); + + expect(response).toMatchObject({ + ok: true, + result: { + tabs: [{ id: 101 }, { id: 102 }], + }, + }); + }); + + it("opens a new tab in the window identified by an explicit tab id", async () => { + const adapter = new FakeBrowserAdapter([ + windowSnapshot(10, true, [tabSummary(101, 0, true, 10)]), + windowSnapshot(20, false, [tabSummary(201, 0, true, 20)]), + ]); + + const response = await handleBrowserRequest( + createRequest( + "open", + { + url: "https://qa.example/", + newTab: true, + target: { tab: { kind: "id", id: 201 } }, + }, + "new-tab-by-tab-id", + ), + adapter, + ); + + expect(response).toMatchObject({ + ok: true, + result: { + target: { windowId: 20, tabId: 102 }, + }, + }); + }); + + it("resizes an explicitly selected window without requiring a tab selector", async () => { + const adapter = new FakeBrowserAdapter([windowSnapshot(10, true, [tabSummary(101, 0, true, 10), tabSummary(102, 1, false, 10)])]); + + const response = await handleBrowserRequest( + createRequest( + "set.viewport", + { + width: 1200, + height: 800, + target: { window: { kind: "id", id: 10 } }, + }, + "resize-window", + ), + adapter, + ); + + expect(response).toMatchObject({ + ok: true, + result: { + window: { id: 10, width: 1200, height: 800 }, + }, + }); + }); + + it.each([ + ["missing window", { window: { kind: "id" as const, id: 999 } }], + ["tab outside selected window", { window: { kind: "id" as const, id: 20 }, tab: { kind: "id" as const, id: 101 } }], + ])("rejects %s before target-dependent side effects", async (_name, target) => { + const adapter = new FakeBrowserAdapter([ + windowSnapshot(10, true, [tabSummary(101, 0, true, 10)]), + windowSnapshot(20, false, [tabSummary(201, 0, true, 20)]), + ]); + + const responses = [ + await handleBrowserRequest(createRequest("open", { url: "https://qa.example/", newTab: false, target }, "invalid-open"), adapter), + await handleBrowserRequest(createRequest("snapshot", { interactiveOnly: true, compact: true, target }, "invalid-snapshot"), adapter), + await handleBrowserRequest(createRequest("click", { selector: "#save", target }, "invalid-click"), adapter), + await handleBrowserRequest(createRequest("screenshot", { path: "/tmp/invalid.png", format: "png", target }, "invalid-screenshot"), adapter), + ]; + + for (const response of responses) { + expect(response).toMatchObject({ ok: false, error: { code: "INVALID_TARGET" } }); + } + expect(adapter.navigations).toEqual([]); + expect(adapter.contentRequests).toEqual([]); + expect(adapter.selectedTabs).toEqual([]); + expect(adapter.focusedWindows).toEqual([]); + expect(adapter.captureRequests).toEqual([]); + }); + + it("reports refreshed focus metadata for window selection without changing later explicit target resolution", async () => { + const adapter = new FakeBrowserAdapter([ + windowSnapshot(10, true, [tabSummary(101, 0, true, 10)]), + windowSnapshot(20, false, [tabSummary(201, 0, true, 20)]), + ]); + + const selected = await handleBrowserRequest(createRequest("window.select", { target: { window: { kind: "id", id: 20 } } }, "select-window-20"), adapter); + const snapshot = await handleBrowserRequest( + createRequest("snapshot", { interactiveOnly: true, compact: true, target: { window: { kind: "id", id: 10 } } }, "snapshot-window-10"), + adapter, + ); + + expect(selected).toMatchObject({ ok: true, result: { window: { id: 20, focused: true }, target: { tabId: 201 } } }); + expect(snapshot).toMatchObject({ ok: true, result: { target: { windowId: 10, tabId: 101 } } }); + expect(adapter.focusedWindows).toEqual([20]); + expect(adapter.contentRequests).toEqual([{ tabId: 101, command: "snapshot" }]); + }); }); diff --git a/packages/extension/src/browser-commands-test-cases.ts b/packages/extension/src/browser-commands-test-cases.ts index 1bdff06..6457371 100644 --- a/packages/extension/src/browser-commands-test-cases.ts +++ b/packages/extension/src/browser-commands-test-cases.ts @@ -1,9 +1,8 @@ -import { PROTOCOL_VERSION, commandSchemas, createRequest, isCommandId } from "@firefox-cli/protocol"; +import { commandSchemas, createRequest, isCommandId, PROTOCOL_VERSION } from "@firefox-cli/protocol"; import { expect } from "vitest"; import { handleBrowserRequest } from "./browser-commands.js"; -import { FakeBrowserAdapter, parseTestBrowserRequest, tabSummary, windowSnapshot } from "./browser-commands-test-utils.js"; - import { browserSmokeRequests } from "./browser-commands-test-smoke.js"; +import { FakeBrowserAdapter, parseTestBrowserRequest, tabSummary, windowSnapshot } from "./browser-commands-test-utils.js"; export async function runCase01() { const expectedCommands = Object.keys(commandSchemas) @@ -76,16 +75,11 @@ export async function runCase02() { export async function runCase03() { const adapter = new FakeBrowserAdapter([windowSnapshot(20, false, [tabSummary(201, 0, true, 20)]), windowSnapshot(10, true, [tabSummary(101, 0, true, 10)])]); - const response = await handleBrowserRequest(createRequest("tabs.list", {}, "request-1"), adapter); + const response = await handleBrowserRequest(createRequest("tabs.list", { target: { window: { kind: "active" } } }, "request-1"), adapter); expect(response).toMatchObject({ ok: true, result: { - target: { - windowId: 10, - windowIndex: 0, - tabId: 101, - }, tabs: [{ id: 101 }], }, }); diff --git a/packages/extension/src/browser-handlers/navigation.ts b/packages/extension/src/browser-handlers/navigation.ts index bc53057..708be22 100644 --- a/packages/extension/src/browser-handlers/navigation.ts +++ b/packages/extension/src/browser-handlers/navigation.ts @@ -8,7 +8,7 @@ type NavigationCommand = "open" | "back" | "forward" | "reload"; export const navigationHandlers: BrowserHandlerMap<NavigationCommand> = { open: async (request, adapter, context) => { if (request.params.newTab) { - const window = await context.targetContext.resolveWindow(request.params.target?.window); + const window = await context.targetContext.resolveTargetWindow(request.params.target); assertMutableWindow(window); const tab = await adapter.createTab({ url: request.params.url, windowId: window.id }); context.targetContext.invalidate(); diff --git a/packages/extension/src/browser-handlers/phase8-browser.ts b/packages/extension/src/browser-handlers/phase8-browser.ts index 83cc2e8..f080117 100644 --- a/packages/extension/src/browser-handlers/phase8-browser.ts +++ b/packages/extension/src/browser-handlers/phase8-browser.ts @@ -1,14 +1,14 @@ import { - createErrorResponseForRequest, - createOkResponse, type ClipboardResult, type CookieResult, + createErrorResponseForRequest, + createOkResponse, type RequestEnvelope, type SetViewportResult, } from "@firefox-cli/protocol"; import { sendContentCommand } from "../browser-command/content-bridge.js"; import { BrowserCommandError } from "../browser-command/errors.js"; -import { toOrderedWindows, toWindowSummary } from "../browser-command/targets.js"; +import { assertMutableWindow, toOrderedWindows, toWindowSummary } from "../browser-command/targets.js"; import type { BrowserHandlerMap } from "./types.js"; type Phase8BrowserCommand = "download" | "clipboard" | "cookies" | "network" | "notify" | "pdf" | "set.viewport"; @@ -117,8 +117,9 @@ export const phase8BrowserHandlers: BrowserHandlerMap<Phase8BrowserCommand> = { }); }, "set.viewport": async (request, adapter, context) => { - const resolved = await context.targetContext.resolveTarget(request.params.target); - const window = await adapter.resizeWindow(resolved.window.id, { + const targetWindow = await context.targetContext.resolveTargetWindow(request.params.target); + assertMutableWindow(targetWindow); + const window = await adapter.resizeWindow(targetWindow.id, { width: request.params.width, height: request.params.height, }); diff --git a/packages/extension/src/browser-handlers/tabs-windows.ts b/packages/extension/src/browser-handlers/tabs-windows.ts index a6e0420..7f0c270 100644 --- a/packages/extension/src/browser-handlers/tabs-windows.ts +++ b/packages/extension/src/browser-handlers/tabs-windows.ts @@ -7,12 +7,9 @@ type TabsWindowsCommand = "tabs.list" | "windows.list" | "tab.new" | "tab.select export const tabsWindowsHandlers: BrowserHandlerMap<TabsWindowsCommand> = { "tabs.list": async (request, _adapter, context) => { - const resolved = await context.targetContext.resolveTarget(request.params.target, { - allowPrivate: true, - }); + const window = await context.targetContext.resolveTargetWindow(request.params.target); return createOkResponse(request, { - target: resolved.target, - tabs: [...resolved.window.tabs], + tabs: [...window.tabs], }); }, "windows.list": async (request, _adapter, context) => { @@ -68,7 +65,7 @@ export const tabsWindowsHandlers: BrowserHandlerMap<TabsWindowsCommand> = { }); }, "window.select": async (request, adapter, context) => { - const window = await context.targetContext.resolveWindow(request.params.target.window); + const window = await context.targetContext.resolveWindow(request.params.target?.window); assertMutableWindow(window); await adapter.focusWindow(window.id); context.targetContext.invalidate(); @@ -83,7 +80,7 @@ export const tabsWindowsHandlers: BrowserHandlerMap<TabsWindowsCommand> = { }); }, "window.close": async (request, adapter, context) => { - const window = await context.targetContext.resolveWindow(request.params.target.window); + const window = await context.targetContext.resolveWindow(request.params.target?.window); assertMutableWindow(window); await adapter.closeWindow(window.id); context.targetContext.invalidate(); diff --git a/packages/protocol/src/batch.ts b/packages/protocol/src/batch.ts index a2f953b..6578997 100644 --- a/packages/protocol/src/batch.ts +++ b/packages/protocol/src/batch.ts @@ -1,8 +1,7 @@ import { z } from "zod"; - +import { addUploadTotalIssue, uploadParamsSchema } from "./browser.js"; import { MAX_BATCH_RESULT_BYTES } from "./constants.js"; import { protocolErrorSchema } from "./core.js"; -import { addUploadTotalIssue, uploadParamsSchema } from "./browser.js"; import { targetSelectorSchema } from "./target.js"; export interface BatchRegistryLookup { @@ -10,18 +9,8 @@ export interface BatchRegistryLookup { readonly isBatchable: (command: string) => boolean; readonly paramsFor: (command: string) => z.ZodType | undefined; readonly resultFor: (command: string) => z.ZodType | undefined; - readonly paramsWithDefaultTarget: (command: string, params: unknown) => BatchDefaultTargetParams; } -type BatchDefaultTargetParams = - | { - readonly found: true; - readonly params: unknown; - } - | { - readonly found: false; - }; - export function createBatchSchemas(registry: BatchRegistryLookup) { const batchStepSchema = z .object({ @@ -59,16 +48,14 @@ export function createBatchSchemas(registry: BatchRegistryLookup) { } const params = paramsSchema.safeParse(step.params); - const paramsWithDefaultTarget = registry.paramsWithDefaultTarget(step.command, step.params); - const fallbackParams = params.success || !paramsWithDefaultTarget.found ? params : paramsSchema.safeParse(paramsWithDefaultTarget.params); - if (!fallbackParams.success) { + if (!params.success) { context.addIssue({ code: "custom", message: "Batch step params are invalid.", path: ["params"], params: { command: step.command, - issues: fallbackParams.error.issues, + issues: params.error.issues, }, }); } diff --git a/packages/protocol/src/browser/output.ts b/packages/protocol/src/browser/output.ts index 53ad30e..37bdada 100644 --- a/packages/protocol/src/browser/output.ts +++ b/packages/protocol/src/browser/output.ts @@ -1,7 +1,7 @@ import { z } from "zod"; import { waitElementSummarySchema } from "../content.js"; -import { resolvedTargetSchema, targetSelectorSchema, windowSummarySchema } from "../target.js"; +import { resolvedTargetSchema, targetSelectorSchema, windowSummarySchema, windowTargetSelectorSchema } from "../target.js"; import { phase8ElementTargetParamsSchema } from "./interactions.js"; export const logActions = ["list", "clear"] as const; @@ -70,7 +70,6 @@ export type NotifyResult = z.infer<typeof notifyResultSchema>; export const pdfParamsSchema = z .object({ - target: targetSelectorSchema.optional(), path: z.string().min(1), }) .strict(); @@ -78,7 +77,7 @@ export const pdfResultSchema = z.object({ path: z.string().min(1) }).strict(); export const setViewportParamsSchema = z .object({ - target: targetSelectorSchema.optional(), + target: windowTargetSelectorSchema.optional(), width: z.number().int().positive().max(10_000), height: z.number().int().positive().max(10_000), }) diff --git a/packages/protocol/src/command-validation.ts b/packages/protocol/src/command-validation.ts index ef96254..0c48c30 100644 --- a/packages/protocol/src/command-validation.ts +++ b/packages/protocol/src/command-validation.ts @@ -1,8 +1,7 @@ import type { z } from "zod"; import type { CommandParams, CommandResult } from "./envelopes.js"; -import { commandAcceptsProtocolBatchDefaultTarget, commandSchemas, type CommandId } from "./registry/index.js"; -import { targetSelectorSchema } from "./target.js"; +import { type CommandId, commandSchemas } from "./registry/index.js"; export type CommandSafeParse<T> = | { @@ -21,38 +20,10 @@ export function safeParseStrictCommandParams(command: CommandId, params: unknown export function safeParseBatchStepCommandParams<C extends CommandId>(command: C, params: unknown): CommandSafeParse<CommandParams<C>>; export function safeParseBatchStepCommandParams(command: CommandId, params: unknown): CommandSafeParse<unknown> { - const parsed = safeParseStrictCommandParams(command, params); - if (parsed.success || !commandAcceptsProtocolBatchDefaultTarget(command)) { - return parsed; - } - - const fallbackParams = paramsWithDefaultTarget(params); - return fallbackParams === undefined ? parsed : safeParseStrictCommandParams(command, fallbackParams); + return safeParseStrictCommandParams(command, params); } export function safeParseCommandResult<C extends CommandId>(command: C, result: unknown): CommandSafeParse<CommandResult<C>>; export function safeParseCommandResult(command: CommandId, result: unknown): CommandSafeParse<unknown> { return commandSchemas[command].result.safeParse(result); } - -function paramsWithDefaultTarget(params: unknown): (Record<string, unknown> & { readonly target: unknown }) | undefined { - if (!isRecord(params)) { - return undefined; - } - - if (params.target !== undefined) { - return undefined; - } - - return { - ...params, - target: targetSelectorSchema.parse({ - window: { kind: "active" }, - tab: { kind: "active" }, - }), - }; -} - -function isRecord(value: unknown): value is Record<string, unknown> { - return typeof value === "object" && value !== null && !Array.isArray(value); -} diff --git a/packages/protocol/src/metadata.ts b/packages/protocol/src/metadata.ts index 2295b91..178f74f 100644 --- a/packages/protocol/src/metadata.ts +++ b/packages/protocol/src/metadata.ts @@ -4,6 +4,7 @@ import type { CapabilityStatus } from "./core.js"; export type CommandOwner = "native-host" | "extension"; export type CommandTargetPolicy = "none" | "optional" | "required" | "mixed"; +export type CliRouteSelectorDimensions = "neither" | "window" | "tab" | "both"; export type CommandContentPolicy = "never" | "always" | "mixed" | "action"; export type CommandTimeoutPolicy = "none" | "command" | "batch"; export type CommandFrameScope = "not-applicable" | "main-frame-only" | "main-frame-with-iframe-diagnostics"; @@ -56,6 +57,7 @@ export interface CliRouteMetadata { readonly id: string; readonly path: readonly [string, ...string[]]; readonly batch: boolean; + readonly selectorDimensions: CliRouteSelectorDimensions; } export interface CliRouteEntry<C extends string = string> { @@ -65,7 +67,6 @@ export interface CliRouteEntry<C extends string = string> { export interface CommandBatchMetadata { readonly allowed: boolean; - readonly protocolDefaultTarget?: boolean; readonly extensionDefaultTarget?: boolean; readonly timeoutRebase?: boolean; } @@ -84,7 +85,7 @@ export interface CommandSchemaMetadata { } export type CommandSchemaEntry = CommandSchemaMetadata & { - readonly params: z.ZodType; + readonly params: z.ZodObject<z.ZodRawShape>; readonly result: z.ZodType; readonly status: CapabilityStatus; }; diff --git a/packages/protocol/src/protocol-metadata-behavior.test.ts b/packages/protocol/src/protocol-metadata-behavior.test.ts index 6313fcb..b245143 100644 --- a/packages/protocol/src/protocol-metadata-behavior.test.ts +++ b/packages/protocol/src/protocol-metadata-behavior.test.ts @@ -3,17 +3,16 @@ import { actionKinds, commandAcceptsBatchTimeout, commandAcceptsExtensionBatchDefaultTarget, - commandAcceptsProtocolBatchDefaultTarget, commandSchemas, - createRequestProtocolMismatchError, createRequest, + createRequestProtocolMismatchError, gatedCapabilities, getCliRouteEntries, - getRequestProtocolCompatibility, getCliRoutes, getCommandCliRoutes, getCommandCompatibilityMetadata, getCommandSecurityMetadata, + getRequestProtocolCompatibility, getRequestProtocolRequirement, isActionCommand, isBatchableCommandId, @@ -23,7 +22,7 @@ import { kernelCapabilities, parseBoundaryRequest, } from "./index.js"; -import { commandIds, sorted, expectedCliRoutesByCommand } from "./protocol-test-support.js"; +import { commandIds, expectedCliRouteSelectorDimensions, expectedCliRoutesByCommand, sorted } from "./protocol-test-support.js"; describe("protocol command metadata", () => { it("uses unique CLI route ids and paths", () => { @@ -35,16 +34,22 @@ describe("protocol command metadata", () => { const routePaths = routes.map((route) => route.path.join("\0")); for (const command of commandIds()) { - expect(getCommandCliRoutes(command)).toEqual(expectedCliRoutesByCommand[command] ?? []); + expect(getCommandCliRoutes(command).map(withoutSelectorDimensions)).toEqual(expectedCliRoutesByCommand[command] ?? []); } - expect(routes).toEqual(expectedRoutes); - expect(routeEntries).toEqual(expectedRouteEntries); + expect(routes.map(withoutSelectorDimensions)).toEqual(expectedRoutes); + expect(routeEntries.map((entry) => ({ ...entry, route: withoutSelectorDimensions(entry.route) }))).toEqual(expectedRouteEntries); expect(new Set(routeIds).size).toBe(routeIds.length); expect(new Set(routePaths).size).toBe(routePaths.length); expect(routes.every((route) => route.path.length > 0)).toBe(true); expect(routes.every((route) => route.path.every((segment) => segment.length > 0))).toBe(true); }); + it("declares the selector dimensions consumed by every CLI route", () => { + for (const route of getCliRoutes()) { + expect(route.selectorDimensions, route.id).toBe(expectedCliRouteSelectorDimensions(route.id)); + } + }); + it("includes all command statuses and gated capabilities in kernel capabilities", () => { for (const command of commandIds()) { expect(kernelCapabilities).toContainEqual({ @@ -71,12 +76,6 @@ describe("protocol command metadata", () => { expect(nonBatchableCommands).toEqual(["hello", "capabilities", "noop", "batch", "pair.approve", "pair.reset", "pair.requestApproval", "pair.openApproval"]); }); - it("marks only required tab/window selectors for protocol batch default targets", () => { - const protocolDefaultCommands = commandIds().filter(commandAcceptsProtocolBatchDefaultTarget); - - expect(protocolDefaultCommands).toEqual(["tab.select", "tab.close", "window.select", "window.close"]); - }); - it("marks extension batch default target commands", () => { const extensionDefaultCommands = commandIds().filter(commandAcceptsExtensionBatchDefaultTarget); @@ -201,6 +200,19 @@ describe("protocol command metadata", () => { }); }); +function withoutSelectorDimensions(route: { + readonly id: string; + readonly path: readonly [string, ...string[]]; + readonly batch: boolean; + readonly selectorDimensions: string; +}): { readonly id: string; readonly path: readonly [string, ...string[]]; readonly batch: boolean } { + return { + id: route.id, + path: route.path, + batch: route.batch, + }; +} + describe("request protocol compatibility", () => { it("derives command protocol requirements from registry metadata", () => { expect(getCommandCompatibilityMetadata("network")).toEqual({ diff --git a/packages/protocol/src/protocol-request.test.ts b/packages/protocol/src/protocol-request.test.ts index ae6ab5a..aeaca53 100644 --- a/packages/protocol/src/protocol-request.test.ts +++ b/packages/protocol/src/protocol-request.test.ts @@ -1,6 +1,6 @@ import { describe, expect, it } from "vitest"; -import { PROTOCOL_VERSION, createRequest, parseBoundaryRequest } from "./index.js"; -import { boundaries, inheritedCommandNames, cliIdentity } from "./protocol-test-support.js"; +import { createRequest, PROTOCOL_VERSION, parseBoundaryRequest } from "./index.js"; +import { boundaries, cliIdentity, inheritedCommandNames } from "./protocol-test-support.js"; describe("parseBoundaryRequest", () => { it.each(boundaries)("validates hello requests across %s", (boundary) => { @@ -104,7 +104,7 @@ describe("parseBoundaryRequest", () => { protocolVersion: PROTOCOL_VERSION, id: "tab-close-1", command: "tab.close", - params: {}, + params: { unexpected: true }, }); expect(parsed).toMatchObject({ @@ -115,6 +115,17 @@ describe("parseBoundaryRequest", () => { }); }); + it("preserves omitted select and close targets for extension ambiguity checks", () => { + const requests = [ + createRequest("tab.select", {}, "tab-select-omitted"), + createRequest("tab.close", {}, "tab-close-omitted"), + createRequest("window.select", {}, "window-select-omitted"), + createRequest("window.close", {}, "window-close-omitted"), + ]; + + expect(requests.map((request) => parseBoundaryRequest("host-to-extension", request))).toEqual(requests.map((request) => ({ ok: true, value: request }))); + }); + it("validates tab list requests", () => { const request = createRequest( "tabs.list", @@ -200,4 +211,113 @@ describe("parseBoundaryRequest", () => { expect(requests.map((request) => parseBoundaryRequest("host-to-extension", request))).toEqual(requests.map((request) => ({ ok: true, value: request }))); }); + + it("rejects tab selectors for window-only browsing commands at direct and batch boundaries", () => { + const invalidRequests: readonly unknown[] = [ + { + protocolVersion: PROTOCOL_VERSION, + id: "tabs-list-tab-target", + command: "tabs.list", + params: { target: { tab: { kind: "id", id: 42 } } }, + }, + { + protocolVersion: PROTOCOL_VERSION, + id: "tab-new-tab-target", + command: "tab.new", + params: { target: { tab: { kind: "id", id: 42 } } }, + }, + { + protocolVersion: PROTOCOL_VERSION, + id: "window-select-tab-target", + command: "window.select", + params: { target: { tab: { kind: "id", id: 42 } } }, + }, + { + protocolVersion: PROTOCOL_VERSION, + id: "window-close-tab-target", + command: "window.close", + params: { target: { tab: { kind: "id", id: 42 } } }, + }, + { + protocolVersion: PROTOCOL_VERSION, + id: "set-viewport-tab-target", + command: "set.viewport", + params: { width: 1200, height: 800, target: { tab: { kind: "id", id: 42 } } }, + }, + { + protocolVersion: PROTOCOL_VERSION, + id: "batch-window-only-tab-target", + command: "batch", + params: { + steps: [ + { command: "tabs.list", params: { target: { tab: { kind: "id", id: 42 } } } }, + { command: "tab.new", params: { target: { tab: { kind: "id", id: 42 } } } }, + { command: "window.select", params: { target: { tab: { kind: "id", id: 42 } } } }, + { command: "window.close", params: { target: { tab: { kind: "id", id: 42 } } } }, + { command: "set.viewport", params: { width: 1200, height: 800, target: { tab: { kind: "id", id: 42 } } } }, + ], + }, + }, + ]; + + for (const request of invalidRequests) { + const parsed = parseBoundaryRequest("host-to-extension", request); + expect(parsed.ok).toBe(false); + if (!parsed.ok) { + expect(parsed.error.code).toBe("INVALID_ENVELOPE"); + } + } + }); + + it("rejects negative selector ids at direct and batch boundaries", () => { + const requests: readonly unknown[] = [ + { + protocolVersion: PROTOCOL_VERSION, + id: "negative-window", + command: "open", + params: { url: "https://example.com/", newTab: false, target: { window: { kind: "id", id: -1 } } }, + }, + { + protocolVersion: PROTOCOL_VERSION, + id: "negative-tab-batch", + command: "batch", + params: { + steps: [{ command: "snapshot", params: { target: { tab: { kind: "id", id: -1 } } } }], + }, + }, + ]; + + for (const request of requests) { + expect(parseBoundaryRequest("host-to-extension", request)).toMatchObject({ + ok: false, + error: { code: "INVALID_ENVELOPE" }, + }); + } + }); + + it("accepts window selectors for window-only browsing commands and full selectors for page commands", () => { + const validRequests = [ + createRequest("tabs.list", { target: { window: { kind: "id", id: 7 } } }, "tabs-list-window-target"), + createRequest("tab.new", { target: { window: { kind: "id", id: 7 } } }, "tab-new-window-target"), + createRequest("window.select", { target: { window: { kind: "id", id: 7 } } }, "window-select-window-target"), + createRequest("window.close", { target: { window: { kind: "id", id: 7 } } }, "window-close-window-target"), + createRequest("set.viewport", { width: 1200, height: 800, target: { window: { kind: "id", id: 7 } } }, "set-viewport-window-target"), + createRequest( + "open", + { + url: "https://example.com/", + newTab: false, + target: { + window: { kind: "id", id: 7 }, + tab: { kind: "id", id: 42 }, + }, + }, + "open-full-target", + ), + ]; + + expect(validRequests.map((request) => parseBoundaryRequest("host-to-extension", request))).toEqual( + validRequests.map((request) => ({ ok: true, value: request })), + ); + }); }); diff --git a/packages/protocol/src/protocol-response-6.test.ts b/packages/protocol/src/protocol-response-6.test.ts index 0d2b3ab..0c3ae2a 100644 --- a/packages/protocol/src/protocol-response-6.test.ts +++ b/packages/protocol/src/protocol-response-6.test.ts @@ -1,20 +1,20 @@ import { describe, expect, it } from "vitest"; import { - PROTOCOL_VERSION, createOkResponse, createRequest, mergeDisjointHandlerMaps, - parseBoundaryRequest, - parseBoundaryResponse, + PROTOCOL_VERSION, parseBatchStepAs, parseBatchStepResultAs, + parseBoundaryRequest, + parseBoundaryResponse, safeParseStrictCommandParams, } from "./index.js"; import { boundaries } from "./protocol-test-support.js"; describe("parseBoundaryResponse", () => { it("parses command-correlated batch steps and results", () => { - expect(safeParseStrictCommandParams("tab.close", {}).success).toBe(false); + expect(safeParseStrictCommandParams("tab.close", {}).success).toBe(true); expect( parseBatchStepAs("tab.close", { @@ -25,12 +25,7 @@ describe("parseBoundaryResponse", () => { ok: true, value: { command: "tab.close", - params: { - target: { - window: { kind: "active" }, - tab: { kind: "active" }, - }, - }, + params: {}, }, }); diff --git a/packages/protocol/src/protocol-test-support.ts b/packages/protocol/src/protocol-test-support.ts index adf9559..9ca8d5d 100644 --- a/packages/protocol/src/protocol-test-support.ts +++ b/packages/protocol/src/protocol-test-support.ts @@ -1,4 +1,4 @@ -import { commandSchemas, type Boundary, type CliRouteMetadata, type CommandId, type ComponentIdentity } from "./index.js"; +import { type Boundary, type CliRouteSelectorDimensions, type CommandId, type ComponentIdentity, commandSchemas } from "./index.js"; export const boundaries: readonly Boundary[] = ["cli-to-host", "host-to-extension", "extension-to-content-script"]; export const inheritedCommandNames = ["toString", "constructor", "__proto__"] as const; @@ -24,7 +24,13 @@ export function sorted(values: readonly string[]): string[] { return [...values].sort((left, right) => left.localeCompare(right)); } -export const expectedCliRoutesByCommand: Partial<Record<CommandId, readonly CliRouteMetadata[]>> = { +interface ExpectedCliRouteMetadata { + readonly id: string; + readonly path: readonly [string, ...string[]]; + readonly batch: boolean; +} + +export const expectedCliRoutesByCommand: Partial<Record<CommandId, readonly ExpectedCliRouteMetadata[]>> = { capabilities: [{ id: "capabilities", path: ["capabilities"], batch: false }], "tabs.list": [{ id: "tab.list", path: ["tab"], batch: true }], "tab.new": [{ id: "tab.new", path: ["tab", "new"], batch: true }], @@ -84,3 +90,23 @@ export const expectedCliRoutesByCommand: Partial<Record<CommandId, readonly CliR "pair.requestApproval": [{ id: "connect", path: ["connect"], batch: false }], "pair.openApproval": [], } as const; + +const selectorDimensionsByRouteId: Readonly<Record<string, CliRouteSelectorDimensions>> = { + capabilities: "neither", + "tab.list": "window", + "tab.new": "window", + "window.list": "neither", + "window.new": "neither", + "window.select": "window", + "window.close": "window", + download: "neither", + cookies: "neither", + notify: "neither", + pdf: "neither", + "set.viewport": "window", + connect: "neither", +}; + +export function expectedCliRouteSelectorDimensions(routeId: string): CliRouteSelectorDimensions { + return selectorDimensionsByRouteId[routeId] ?? "both"; +} diff --git a/packages/protocol/src/registry/actions.ts b/packages/protocol/src/registry/actions.ts index 2e7e1c2..0c04e34 100644 --- a/packages/protocol/src/registry/actions.ts +++ b/packages/protocol/src/registry/actions.ts @@ -23,7 +23,7 @@ export const actionsCommandEntries = defineCommandEntries({ action: true, timeout: "none", batch: { allowed: true, extensionDefaultTarget: true }, - cliRoutes: [{ id: "click", path: ["click"], batch: true }], + cliRoutes: [{ id: "click", path: ["click"], batch: true, selectorDimensions: "both" }], }, dblclick: { params: elementActionParamsSchema, @@ -35,7 +35,7 @@ export const actionsCommandEntries = defineCommandEntries({ action: true, timeout: "none", batch: { allowed: true, extensionDefaultTarget: true }, - cliRoutes: [{ id: "dblclick", path: ["dblclick"], batch: true }], + cliRoutes: [{ id: "dblclick", path: ["dblclick"], batch: true, selectorDimensions: "both" }], }, focus: { params: elementActionParamsSchema, @@ -47,7 +47,7 @@ export const actionsCommandEntries = defineCommandEntries({ action: true, timeout: "none", batch: { allowed: true, extensionDefaultTarget: true }, - cliRoutes: [{ id: "focus", path: ["focus"], batch: true }], + cliRoutes: [{ id: "focus", path: ["focus"], batch: true, selectorDimensions: "both" }], }, hover: { params: elementActionParamsSchema, @@ -59,7 +59,7 @@ export const actionsCommandEntries = defineCommandEntries({ action: true, timeout: "none", batch: { allowed: true, extensionDefaultTarget: true }, - cliRoutes: [{ id: "hover", path: ["hover"], batch: true }], + cliRoutes: [{ id: "hover", path: ["hover"], batch: true, selectorDimensions: "both" }], }, fill: { params: textActionParamsSchema, @@ -71,7 +71,7 @@ export const actionsCommandEntries = defineCommandEntries({ action: true, timeout: "none", batch: { allowed: true, extensionDefaultTarget: true }, - cliRoutes: [{ id: "fill", path: ["fill"], batch: true }], + cliRoutes: [{ id: "fill", path: ["fill"], batch: true, selectorDimensions: "both" }], }, type: { params: textActionParamsSchema, @@ -83,7 +83,7 @@ export const actionsCommandEntries = defineCommandEntries({ action: true, timeout: "none", batch: { allowed: true, extensionDefaultTarget: true }, - cliRoutes: [{ id: "type", path: ["type"], batch: true }], + cliRoutes: [{ id: "type", path: ["type"], batch: true, selectorDimensions: "both" }], }, press: { params: pressParamsSchema, @@ -95,7 +95,7 @@ export const actionsCommandEntries = defineCommandEntries({ action: true, timeout: "none", batch: { allowed: true, extensionDefaultTarget: true }, - cliRoutes: [{ id: "press", path: ["press"], batch: true }], + cliRoutes: [{ id: "press", path: ["press"], batch: true, selectorDimensions: "both" }], }, "keyboard.type": { params: keyboardTextActionParamsSchema, @@ -107,7 +107,7 @@ export const actionsCommandEntries = defineCommandEntries({ action: true, timeout: "none", batch: { allowed: true, extensionDefaultTarget: true }, - cliRoutes: [{ id: "keyboard.type", path: ["keyboard", "type"], batch: true }], + cliRoutes: [{ id: "keyboard.type", path: ["keyboard", "type"], batch: true, selectorDimensions: "both" }], }, "keyboard.inserttext": { params: keyboardTextActionParamsSchema, @@ -119,7 +119,7 @@ export const actionsCommandEntries = defineCommandEntries({ action: true, timeout: "none", batch: { allowed: true, extensionDefaultTarget: true }, - cliRoutes: [{ id: "keyboard.inserttext", path: ["keyboard", "inserttext"], batch: true }], + cliRoutes: [{ id: "keyboard.inserttext", path: ["keyboard", "inserttext"], batch: true, selectorDimensions: "both" }], }, check: { params: elementActionParamsSchema, @@ -131,7 +131,7 @@ export const actionsCommandEntries = defineCommandEntries({ action: true, timeout: "none", batch: { allowed: true, extensionDefaultTarget: true }, - cliRoutes: [{ id: "check", path: ["check"], batch: true }], + cliRoutes: [{ id: "check", path: ["check"], batch: true, selectorDimensions: "both" }], }, uncheck: { params: elementActionParamsSchema, @@ -143,7 +143,7 @@ export const actionsCommandEntries = defineCommandEntries({ action: true, timeout: "none", batch: { allowed: true, extensionDefaultTarget: true }, - cliRoutes: [{ id: "uncheck", path: ["uncheck"], batch: true }], + cliRoutes: [{ id: "uncheck", path: ["uncheck"], batch: true, selectorDimensions: "both" }], }, select: { params: selectParamsSchema, @@ -155,7 +155,7 @@ export const actionsCommandEntries = defineCommandEntries({ action: true, timeout: "none", batch: { allowed: true, extensionDefaultTarget: true }, - cliRoutes: [{ id: "select", path: ["select"], batch: true }], + cliRoutes: [{ id: "select", path: ["select"], batch: true, selectorDimensions: "both" }], }, scroll: { params: scrollParamsSchema, @@ -167,7 +167,7 @@ export const actionsCommandEntries = defineCommandEntries({ action: true, timeout: "none", batch: { allowed: true, extensionDefaultTarget: true }, - cliRoutes: [{ id: "scroll", path: ["scroll"], batch: true }], + cliRoutes: [{ id: "scroll", path: ["scroll"], batch: true, selectorDimensions: "both" }], }, scrollintoview: { params: elementActionParamsSchema, @@ -179,7 +179,7 @@ export const actionsCommandEntries = defineCommandEntries({ action: true, timeout: "none", batch: { allowed: true, extensionDefaultTarget: true }, - cliRoutes: [{ id: "scrollintoview", path: ["scrollintoview"], batch: true }], + cliRoutes: [{ id: "scrollintoview", path: ["scrollintoview"], batch: true, selectorDimensions: "both" }], }, swipe: { params: scrollParamsSchema, @@ -191,6 +191,6 @@ export const actionsCommandEntries = defineCommandEntries({ action: true, timeout: "none", batch: { allowed: true, extensionDefaultTarget: true }, - cliRoutes: [{ id: "swipe", path: ["swipe"], batch: true }], + cliRoutes: [{ id: "swipe", path: ["swipe"], batch: true, selectorDimensions: "both" }], }, }); diff --git a/packages/protocol/src/registry/browsing.ts b/packages/protocol/src/registry/browsing.ts index 3e7627d..3fa934a 100644 --- a/packages/protocol/src/registry/browsing.ts +++ b/packages/protocol/src/registry/browsing.ts @@ -5,16 +5,16 @@ import { tabCloseResultSchema, tabNewParamsSchema, tabNewResultSchema, - tabTargetParamsSchema, tabsListParamsSchema, tabsListResultSchema, + tabTargetParamsSchema, windowCloseResultSchema, windowNewParamsSchema, windowNewResultSchema, windowSelectResultSchema, - windowTargetParamsSchema, windowsListParamsSchema, windowsListResultSchema, + windowTargetParamsSchema, } from "../target.js"; import { defineCommandEntries } from "./define.js"; @@ -29,7 +29,7 @@ export const browsingCommandEntries = defineCommandEntries({ action: false, timeout: "none", batch: { allowed: true, extensionDefaultTarget: true }, - cliRoutes: [{ id: "tab.list", path: ["tab"], batch: true }], + cliRoutes: [{ id: "tab.list", path: ["tab"], batch: true, selectorDimensions: "window" }], }, "tab.new": { params: tabNewParamsSchema, @@ -41,31 +41,31 @@ export const browsingCommandEntries = defineCommandEntries({ action: false, timeout: "none", batch: { allowed: true, extensionDefaultTarget: true }, - cliRoutes: [{ id: "tab.new", path: ["tab", "new"], batch: true }], + cliRoutes: [{ id: "tab.new", path: ["tab", "new"], batch: true, selectorDimensions: "window" }], }, "tab.select": { params: tabTargetParamsSchema, result: tabNewResultSchema, status: "mvp", owner: "extension", - target: "required", + target: "optional", content: "never", action: false, timeout: "none", - batch: { allowed: true, protocolDefaultTarget: true, extensionDefaultTarget: true }, - cliRoutes: [{ id: "tab.select", path: ["tab", "select"], batch: true }], + batch: { allowed: true, extensionDefaultTarget: true }, + cliRoutes: [{ id: "tab.select", path: ["tab", "select"], batch: true, selectorDimensions: "both" }], }, "tab.close": { params: tabTargetParamsSchema, result: tabCloseResultSchema, status: "mvp", owner: "extension", - target: "required", + target: "optional", content: "never", action: false, timeout: "none", - batch: { allowed: true, protocolDefaultTarget: true, extensionDefaultTarget: true }, - cliRoutes: [{ id: "tab.close", path: ["tab", "close"], batch: true }], + batch: { allowed: true, extensionDefaultTarget: true }, + cliRoutes: [{ id: "tab.close", path: ["tab", "close"], batch: true, selectorDimensions: "both" }], }, "windows.list": { params: windowsListParamsSchema, @@ -77,7 +77,7 @@ export const browsingCommandEntries = defineCommandEntries({ action: false, timeout: "none", batch: { allowed: true }, - cliRoutes: [{ id: "window.list", path: ["window"], batch: true }], + cliRoutes: [{ id: "window.list", path: ["window"], batch: true, selectorDimensions: "neither" }], }, "window.new": { params: windowNewParamsSchema, @@ -89,31 +89,31 @@ export const browsingCommandEntries = defineCommandEntries({ action: false, timeout: "none", batch: { allowed: true }, - cliRoutes: [{ id: "window.new", path: ["window", "new"], batch: true }], + cliRoutes: [{ id: "window.new", path: ["window", "new"], batch: true, selectorDimensions: "neither" }], }, "window.select": { params: windowTargetParamsSchema, result: windowSelectResultSchema, status: "mvp", owner: "extension", - target: "required", + target: "optional", content: "never", action: false, timeout: "none", - batch: { allowed: true, protocolDefaultTarget: true, extensionDefaultTarget: true }, - cliRoutes: [{ id: "window.select", path: ["window", "select"], batch: true }], + batch: { allowed: true, extensionDefaultTarget: true }, + cliRoutes: [{ id: "window.select", path: ["window", "select"], batch: true, selectorDimensions: "window" }], }, "window.close": { params: windowTargetParamsSchema, result: windowCloseResultSchema, status: "mvp", owner: "extension", - target: "required", + target: "optional", content: "never", action: false, timeout: "none", - batch: { allowed: true, protocolDefaultTarget: true, extensionDefaultTarget: true }, - cliRoutes: [{ id: "window.close", path: ["window", "close"], batch: true }], + batch: { allowed: true, extensionDefaultTarget: true }, + cliRoutes: [{ id: "window.close", path: ["window", "close"], batch: true, selectorDimensions: "window" }], }, open: { params: openParamsSchema, @@ -125,7 +125,7 @@ export const browsingCommandEntries = defineCommandEntries({ action: false, timeout: "none", batch: { allowed: true, extensionDefaultTarget: true }, - cliRoutes: [{ id: "open", path: ["open"], batch: true }], + cliRoutes: [{ id: "open", path: ["open"], batch: true, selectorDimensions: "both" }], }, back: { params: navigationParamsSchema, @@ -137,7 +137,7 @@ export const browsingCommandEntries = defineCommandEntries({ action: false, timeout: "none", batch: { allowed: true, extensionDefaultTarget: true }, - cliRoutes: [{ id: "back", path: ["back"], batch: true }], + cliRoutes: [{ id: "back", path: ["back"], batch: true, selectorDimensions: "both" }], }, forward: { params: navigationParamsSchema, @@ -149,7 +149,7 @@ export const browsingCommandEntries = defineCommandEntries({ action: false, timeout: "none", batch: { allowed: true, extensionDefaultTarget: true }, - cliRoutes: [{ id: "forward", path: ["forward"], batch: true }], + cliRoutes: [{ id: "forward", path: ["forward"], batch: true, selectorDimensions: "both" }], }, reload: { params: navigationParamsSchema, @@ -161,6 +161,6 @@ export const browsingCommandEntries = defineCommandEntries({ action: false, timeout: "none", batch: { allowed: true, extensionDefaultTarget: true }, - cliRoutes: [{ id: "reload", path: ["reload"], batch: true }], + cliRoutes: [{ id: "reload", path: ["reload"], batch: true, selectorDimensions: "both" }], }, }); diff --git a/packages/protocol/src/registry/content.ts b/packages/protocol/src/registry/content.ts index dabdbe8..ebb0857 100644 --- a/packages/protocol/src/registry/content.ts +++ b/packages/protocol/src/registry/content.ts @@ -28,7 +28,7 @@ export const contentCommandEntries = defineCommandEntries({ future: "docs/iframe-targeting-future.md", }, batch: { allowed: true, extensionDefaultTarget: true }, - cliRoutes: [{ id: "snapshot", path: ["snapshot"], batch: true }], + cliRoutes: [{ id: "snapshot", path: ["snapshot"], batch: true, selectorDimensions: "both" }], }, "ref.resolve": { params: refResolveParamsSchema, @@ -40,7 +40,7 @@ export const contentCommandEntries = defineCommandEntries({ action: false, timeout: "none", batch: { allowed: true, extensionDefaultTarget: true }, - cliRoutes: [{ id: "ref", path: ["ref"], batch: true }], + cliRoutes: [{ id: "ref", path: ["ref"], batch: true, selectorDimensions: "both" }], }, get: { params: getParamsSchema, @@ -52,7 +52,7 @@ export const contentCommandEntries = defineCommandEntries({ action: false, timeout: "none", batch: { allowed: true, extensionDefaultTarget: true }, - cliRoutes: [{ id: "get", path: ["get"], batch: true }], + cliRoutes: [{ id: "get", path: ["get"], batch: true, selectorDimensions: "both" }], }, is: { params: isParamsSchema, @@ -64,7 +64,7 @@ export const contentCommandEntries = defineCommandEntries({ action: false, timeout: "none", batch: { allowed: true, extensionDefaultTarget: true }, - cliRoutes: [{ id: "is", path: ["is"], batch: true }], + cliRoutes: [{ id: "is", path: ["is"], batch: true, selectorDimensions: "both" }], }, wait: { params: waitParamsSchema, @@ -94,6 +94,6 @@ export const contentCommandEntries = defineCommandEntries({ ], }, batch: { allowed: true, extensionDefaultTarget: true, timeoutRebase: true }, - cliRoutes: [{ id: "wait", path: ["wait"], batch: true }], + cliRoutes: [{ id: "wait", path: ["wait"], batch: true, selectorDimensions: "both" }], }, }); diff --git a/packages/protocol/src/registry/core.ts b/packages/protocol/src/registry/core.ts index 417c1cf..25c8334 100644 --- a/packages/protocol/src/registry/core.ts +++ b/packages/protocol/src/registry/core.ts @@ -24,7 +24,7 @@ export const coreCommandEntries = defineCommandEntries({ action: false, timeout: "none", batch: { allowed: false }, - cliRoutes: [{ id: "capabilities", path: ["capabilities"], batch: false }], + cliRoutes: [{ id: "capabilities", path: ["capabilities"], batch: false, selectorDimensions: "neither" }], }, noop: { params: noOpParamsSchema, diff --git a/packages/protocol/src/registry/define.ts b/packages/protocol/src/registry/define.ts index 914a151..cd5246c 100644 --- a/packages/protocol/src/registry/define.ts +++ b/packages/protocol/src/registry/define.ts @@ -1,4 +1,6 @@ -import type { CommandSchemaEntry } from "../metadata.js"; +import { z } from "zod"; + +import type { CliRouteSelectorDimensions, CommandSchemaEntry } from "../metadata.js"; export type CommandRegistryFragment = Readonly<Record<string, CommandSchemaEntry>>; @@ -23,6 +25,45 @@ export function assembleCommandRegistry(...fragments: readonly CommandRegistryFr registry[command] = schema; } } + assertCliRouteSelectorDimensions(registry); return registry; } + +function assertCliRouteSelectorDimensions(registry: Readonly<Record<string, CommandSchemaEntry>>): void { + for (const [command, schema] of Object.entries(registry)) { + for (const route of schema.cliRoutes) { + const selectorDimensions: CliRouteSelectorDimensions = route.selectorDimensions; + const paramsSelectorDimensions = targetSelectorDimensionsAcceptedByCommand(schema); + if (selectorDimensions !== paramsSelectorDimensions) { + throw new Error( + `CLI route selector dimensions disagree with command params: ${command} (${route.id}) declares ${selectorDimensions}, params accept ${paramsSelectorDimensions}.`, + ); + } + } + } +} + +export function targetSelectorDimensionsAcceptedByCommand(schema: CommandSchemaEntry): CliRouteSelectorDimensions { + if (!(schema.params instanceof z.ZodObject)) { + return "neither"; + } + const selectorSchema = schema.params.shape.target; + if (!isZodType(selectorSchema)) return "neither"; + const acceptsWindow = selectorSchema.safeParse({ window: { kind: "active" } }).success; + const acceptsTab = selectorSchema.safeParse({ tab: { kind: "active" } }).success; + if (acceptsWindow && acceptsTab) { + return "both"; + } + if (acceptsWindow) { + return "window"; + } + if (acceptsTab) { + return "tab"; + } + return "neither"; +} + +function isZodType(value: unknown): value is z.ZodType { + return value instanceof z.ZodType; +} diff --git a/packages/protocol/src/registry/index.ts b/packages/protocol/src/registry/index.ts index 6017a8c..731d08e 100644 --- a/packages/protocol/src/registry/index.ts +++ b/packages/protocol/src/registry/index.ts @@ -1,8 +1,6 @@ import type { z } from "zod"; - -import { createBatchSchemas } from "../batch.js"; -import { targetSelectorSchema } from "../target.js"; import type { ActionKind } from "../actions.js"; +import { createBatchSchemas } from "../batch.js"; import type { CliRouteEntry, CliRouteMetadata, @@ -17,7 +15,7 @@ import { actionsCommandEntries } from "./actions.js"; import { browsingCommandEntries } from "./browsing.js"; import { contentCommandEntries } from "./content.js"; import { coreCommandEntries } from "./core.js"; -import { assembleCommandRegistry, defineCommandEntries } from "./define.js"; +import { assembleCommandRegistry, defineCommandEntries, targetSelectorDimensionsAcceptedByCommand } from "./define.js"; import { pairingCommandEntries } from "./pairing.js"; import { phase8CommandEntries } from "./phase8.js"; @@ -39,7 +37,6 @@ const batchSchemas = createBatchSchemas({ isBatchable: (command) => isNonBatchCommandId(command) && nonBatchCommandSchemas[command].batch.allowed, paramsFor: (command) => (isNonBatchCommandId(command) ? nonBatchCommandSchemas[command].params : undefined), resultFor: (command) => (isNonBatchCommandId(command) ? nonBatchCommandSchemas[command].result : undefined), - paramsWithDefaultTarget: (command, params) => batchStepParamsWithDefaultTarget(command, params), }); export const batchStepSchema = batchSchemas.batchStepSchema; @@ -63,7 +60,7 @@ const batchCommandEntries = defineCommandEntries({ action: false, timeout: "batch", batch: { allowed: false }, - cliRoutes: [{ id: "batch", path: ["batch"], batch: false }], + cliRoutes: [{ id: "batch", path: ["batch"], batch: false, selectorDimensions: "both" }], }, }); @@ -110,16 +107,12 @@ export function getCliRouteEntries(): readonly CliRouteEntry[] { ); } -export function isBatchableCommandId(command: string): command is CommandId { - return isCommandId(command) && commandSchemas[command].batch.allowed; +export function getCommandTargetSelectorDimensions(command: CommandId): CliRouteMetadata["selectorDimensions"] { + return targetSelectorDimensionsAcceptedByCommand(commandSchemas[command]); } -export function commandAcceptsProtocolBatchDefaultTarget(command: string): command is CommandId { - if (!isCommandId(command)) { - return false; - } - const batch: CommandBatchMetadata = commandSchemas[command].batch; - return batch.protocolDefaultTarget === true; +export function isBatchableCommandId(command: string): command is CommandId { + return isCommandId(command) && commandSchemas[command].batch.allowed; } export function commandAcceptsExtensionBatchDefaultTarget(command: string): command is CommandId { @@ -239,30 +232,6 @@ function valueAtPath(value: unknown, path: readonly string[]): unknown { }, value); } -function batchStepParamsWithDefaultTarget( - command: string, - params: unknown, -): { readonly found: true; readonly params: Record<string, unknown> & { readonly target: unknown } } | { readonly found: false } { - if (!commandAcceptsProtocolBatchDefaultTarget(command) || !isRecord(params)) { - return { found: false }; - } - - if (params.target !== undefined) { - return { found: false }; - } - - return { - found: true, - params: { - ...params, - target: targetSelectorSchema.parse({ - window: { kind: "active" }, - tab: { kind: "active" }, - }), - }, - }; -} - function isRecord(value: unknown): value is Record<string, unknown> { return typeof value === "object" && value !== null && !Array.isArray(value); } diff --git a/packages/protocol/src/registry/pairing.ts b/packages/protocol/src/registry/pairing.ts index 53e80c1..68372b8 100644 --- a/packages/protocol/src/registry/pairing.ts +++ b/packages/protocol/src/registry/pairing.ts @@ -53,7 +53,7 @@ export const pairingCommandEntries = defineCommandEntries({ ], }, batch: { allowed: false }, - cliRoutes: [{ id: "connect", path: ["connect"], batch: false }], + cliRoutes: [{ id: "connect", path: ["connect"], batch: false, selectorDimensions: "neither" }], }, "pair.openApproval": { params: pairOpenApprovalParamsSchema, diff --git a/packages/protocol/src/registry/phase8.ts b/packages/protocol/src/registry/phase8.ts index 1836a91..39756ba 100644 --- a/packages/protocol/src/registry/phase8.ts +++ b/packages/protocol/src/registry/phase8.ts @@ -58,7 +58,7 @@ export const phase8CommandEntries = defineCommandEntries({ future: "docs/iframe-targeting-future.md", }, batch: { allowed: true, extensionDefaultTarget: true, timeoutRebase: true }, - cliRoutes: [{ id: "eval", path: ["eval"], batch: true }], + cliRoutes: [{ id: "eval", path: ["eval"], batch: true, selectorDimensions: "both" }], }, screenshot: { params: screenshotParamsSchema, @@ -70,7 +70,7 @@ export const phase8CommandEntries = defineCommandEntries({ action: false, timeout: "command", batch: { allowed: true, extensionDefaultTarget: true, timeoutRebase: true }, - cliRoutes: [{ id: "screenshot", path: ["screenshot"], batch: true }], + cliRoutes: [{ id: "screenshot", path: ["screenshot"], batch: true, selectorDimensions: "both" }], }, drag: { params: dragParamsSchema, @@ -82,7 +82,7 @@ export const phase8CommandEntries = defineCommandEntries({ action: true, timeout: "none", batch: { allowed: true, extensionDefaultTarget: true }, - cliRoutes: [{ id: "drag", path: ["drag"], batch: true }], + cliRoutes: [{ id: "drag", path: ["drag"], batch: true, selectorDimensions: "both" }], }, upload: { params: uploadParamsSchema, @@ -94,7 +94,7 @@ export const phase8CommandEntries = defineCommandEntries({ action: true, timeout: "none", batch: { allowed: true, extensionDefaultTarget: true }, - cliRoutes: [{ id: "upload", path: ["upload"], batch: true }], + cliRoutes: [{ id: "upload", path: ["upload"], batch: true, selectorDimensions: "both" }], }, mouse: { params: mouseParamsSchema, @@ -106,7 +106,7 @@ export const phase8CommandEntries = defineCommandEntries({ action: true, timeout: "none", batch: { allowed: true, extensionDefaultTarget: true }, - cliRoutes: [{ id: "mouse", path: ["mouse"], batch: true }], + cliRoutes: [{ id: "mouse", path: ["mouse"], batch: true, selectorDimensions: "both" }], }, keydown: { params: keyEventParamsSchema, @@ -118,7 +118,7 @@ export const phase8CommandEntries = defineCommandEntries({ action: true, timeout: "none", batch: { allowed: true, extensionDefaultTarget: true }, - cliRoutes: [{ id: "keydown", path: ["keydown"], batch: true }], + cliRoutes: [{ id: "keydown", path: ["keydown"], batch: true, selectorDimensions: "both" }], }, keyup: { params: keyEventParamsSchema, @@ -130,7 +130,7 @@ export const phase8CommandEntries = defineCommandEntries({ action: true, timeout: "none", batch: { allowed: true, extensionDefaultTarget: true }, - cliRoutes: [{ id: "keyup", path: ["keyup"], batch: true }], + cliRoutes: [{ id: "keyup", path: ["keyup"], batch: true, selectorDimensions: "both" }], }, find: { params: findParamsSchema, @@ -142,7 +142,7 @@ export const phase8CommandEntries = defineCommandEntries({ action: false, timeout: "none", batch: { allowed: true, extensionDefaultTarget: true }, - cliRoutes: [{ id: "find", path: ["find"], batch: true }], + cliRoutes: [{ id: "find", path: ["find"], batch: true, selectorDimensions: "both" }], }, frame: { params: frameParamsSchema, @@ -159,7 +159,7 @@ export const phase8CommandEntries = defineCommandEntries({ future: "docs/iframe-targeting-future.md", }, batch: { allowed: true, extensionDefaultTarget: true }, - cliRoutes: [{ id: "frame", path: ["frame"], batch: true }], + cliRoutes: [{ id: "frame", path: ["frame"], batch: true, selectorDimensions: "both" }], }, download: { params: downloadParamsSchema, @@ -172,7 +172,7 @@ export const phase8CommandEntries = defineCommandEntries({ timeout: "none", security: { level: "sensitive", reasons: ["downloads"] }, batch: { allowed: true }, - cliRoutes: [{ id: "download", path: ["download"], batch: true }], + cliRoutes: [{ id: "download", path: ["download"], batch: true, selectorDimensions: "neither" }], }, dialog: { params: dialogParamsSchema, @@ -184,7 +184,7 @@ export const phase8CommandEntries = defineCommandEntries({ action: false, timeout: "none", batch: { allowed: true, extensionDefaultTarget: true }, - cliRoutes: [{ id: "dialog", path: ["dialog"], batch: true }], + cliRoutes: [{ id: "dialog", path: ["dialog"], batch: true, selectorDimensions: "both" }], }, clipboard: { params: clipboardParamsSchema, @@ -197,7 +197,7 @@ export const phase8CommandEntries = defineCommandEntries({ timeout: "none", security: { level: "sensitive", reasons: ["clipboard"] }, batch: { allowed: true, extensionDefaultTarget: true }, - cliRoutes: [{ id: "clipboard", path: ["clipboard"], batch: true }], + cliRoutes: [{ id: "clipboard", path: ["clipboard"], batch: true, selectorDimensions: "both" }], }, cookies: { params: cookieParamsSchema, @@ -210,7 +210,7 @@ export const phase8CommandEntries = defineCommandEntries({ timeout: "none", security: { level: "sensitive", reasons: ["cookies"] }, batch: { allowed: true }, - cliRoutes: [{ id: "cookies", path: ["cookies"], batch: true }], + cliRoutes: [{ id: "cookies", path: ["cookies"], batch: true, selectorDimensions: "neither" }], }, storage: { params: storageParamsSchema, @@ -222,7 +222,7 @@ export const phase8CommandEntries = defineCommandEntries({ action: false, timeout: "none", batch: { allowed: true, extensionDefaultTarget: true }, - cliRoutes: [{ id: "storage", path: ["storage"], batch: true }], + cliRoutes: [{ id: "storage", path: ["storage"], batch: true, selectorDimensions: "both" }], }, network: { params: networkParamsSchema, @@ -243,7 +243,7 @@ export const phase8CommandEntries = defineCommandEntries({ ], }, batch: { allowed: true, extensionDefaultTarget: true }, - cliRoutes: [{ id: "network", path: ["network"], batch: true }], + cliRoutes: [{ id: "network", path: ["network"], batch: true, selectorDimensions: "both" }], }, console: { params: consoleParamsSchema, @@ -255,7 +255,7 @@ export const phase8CommandEntries = defineCommandEntries({ action: false, timeout: "none", batch: { allowed: true, extensionDefaultTarget: true }, - cliRoutes: [{ id: "console", path: ["console"], batch: true }], + cliRoutes: [{ id: "console", path: ["console"], batch: true, selectorDimensions: "both" }], }, errors: { params: errorsParamsSchema, @@ -267,7 +267,7 @@ export const phase8CommandEntries = defineCommandEntries({ action: false, timeout: "none", batch: { allowed: true, extensionDefaultTarget: true }, - cliRoutes: [{ id: "errors", path: ["errors"], batch: true }], + cliRoutes: [{ id: "errors", path: ["errors"], batch: true, selectorDimensions: "both" }], }, highlight: { params: highlightParamsSchema, @@ -279,7 +279,7 @@ export const phase8CommandEntries = defineCommandEntries({ action: false, timeout: "none", batch: { allowed: true, extensionDefaultTarget: true }, - cliRoutes: [{ id: "highlight", path: ["highlight"], batch: true }], + cliRoutes: [{ id: "highlight", path: ["highlight"], batch: true, selectorDimensions: "both" }], }, notify: { params: notifyParamsSchema, @@ -300,19 +300,19 @@ export const phase8CommandEntries = defineCommandEntries({ ], }, batch: { allowed: true }, - cliRoutes: [{ id: "notify", path: ["notify"], batch: true }], + cliRoutes: [{ id: "notify", path: ["notify"], batch: true, selectorDimensions: "neither" }], }, pdf: { params: pdfParamsSchema, result: pdfResultSchema, status: "unsupported", owner: "extension", - target: "optional", + target: "none", content: "never", action: false, timeout: "none", batch: { allowed: true }, - cliRoutes: [{ id: "pdf", path: ["pdf"], batch: true }], + cliRoutes: [{ id: "pdf", path: ["pdf"], batch: true, selectorDimensions: "neither" }], }, "set.viewport": { params: setViewportParamsSchema, @@ -324,7 +324,7 @@ export const phase8CommandEntries = defineCommandEntries({ action: false, timeout: "none", batch: { allowed: true, extensionDefaultTarget: true }, - cliRoutes: [{ id: "set.viewport", path: ["set", "viewport"], batch: true }], + cliRoutes: [{ id: "set.viewport", path: ["set", "viewport"], batch: true, selectorDimensions: "window" }], }, diff: { params: diffParamsSchema, @@ -336,6 +336,6 @@ export const phase8CommandEntries = defineCommandEntries({ action: false, timeout: "none", batch: { allowed: true, extensionDefaultTarget: true }, - cliRoutes: [{ id: "diff", path: ["diff"], batch: true }], + cliRoutes: [{ id: "diff", path: ["diff"], batch: true, selectorDimensions: "both" }], }, }); diff --git a/packages/protocol/src/registry/registry.test.ts b/packages/protocol/src/registry/registry.test.ts index 6611c11..53b2e4a 100644 --- a/packages/protocol/src/registry/registry.test.ts +++ b/packages/protocol/src/registry/registry.test.ts @@ -118,6 +118,34 @@ describe("command registry assembly", () => { expect(() => assembleCommandRegistry(duplicate, duplicate)).toThrow("Duplicate command id: duplicate"); }); + it("rejects route dimensions that disagree with the target parameter schema", () => { + const windowOnly = defineCommandEntries({ + mismatch: { + params: z + .object({ + target: z + .object({ + window: z.object({ kind: z.literal("active") }).strict(), + }) + .strict() + .optional(), + }) + .strict(), + result: z.object({ ok: z.literal(true) }).strict(), + status: "mvp", + owner: "extension", + target: "optional", + content: "never", + action: false, + timeout: "none", + batch: { allowed: false }, + cliRoutes: [{ id: "mismatch", path: ["mismatch"], batch: false, selectorDimensions: "both" }], + }, + }); + + expect(() => assembleCommandRegistry(windowOnly)).toThrow("CLI route selector dimensions disagree with command params"); + }); + it("preserves command-specific public protocol types", () => { const assertions: ProtocolTypeAssertions = [true, true, true, true, true, true, true]; diff --git a/packages/protocol/src/target.ts b/packages/protocol/src/target.ts index 1953870..bea4895 100644 --- a/packages/protocol/src/target.ts +++ b/packages/protocol/src/target.ts @@ -14,26 +14,27 @@ export const tabSummarySchema = z .strict(); export type TabSummary = z.infer<typeof tabSummarySchema>; +export const targetDimensionSelectorSchema = z.union([ + z.object({ kind: z.literal("active") }).strict(), + z.object({ kind: z.literal("id"), id: z.number().int().nonnegative() }).strict(), + z.object({ kind: z.literal("index"), index: z.number().int().nonnegative() }).strict(), +]); + export const targetSelectorSchema = z .object({ - window: z - .union([ - z.object({ kind: z.literal("active") }).strict(), - z.object({ kind: z.literal("id"), id: z.number().int() }).strict(), - z.object({ kind: z.literal("index"), index: z.number().int().nonnegative() }).strict(), - ]) - .optional(), - tab: z - .union([ - z.object({ kind: z.literal("active") }).strict(), - z.object({ kind: z.literal("id"), id: z.number().int() }).strict(), - z.object({ kind: z.literal("index"), index: z.number().int().nonnegative() }).strict(), - ]) - .optional(), + window: targetDimensionSelectorSchema.optional(), + tab: targetDimensionSelectorSchema.optional(), }) .strict(); export type TargetSelector = z.infer<typeof targetSelectorSchema>; +export const windowTargetSelectorSchema = z + .object({ + window: targetDimensionSelectorSchema, + }) + .strict(); +export type WindowTargetSelector = z.infer<typeof windowTargetSelectorSchema>; + export const resolvedTargetSchema = z .object({ windowId: z.number().int(), @@ -66,7 +67,7 @@ export type WindowSummary = z.infer<typeof windowSummarySchema>; export const tabsListParamsSchema = z .object({ - target: targetSelectorSchema.optional(), + target: windowTargetSelectorSchema.optional(), }) .strict(); export const tabsListResultSchema = z.object({ @@ -77,12 +78,12 @@ export const tabsListResultSchema = z.object({ export const tabNewParamsSchema = z .object({ url: z.string().min(1).optional(), - target: targetSelectorSchema.optional(), + target: windowTargetSelectorSchema.optional(), }) .strict(); export const tabTargetParamsSchema = z .object({ - target: targetSelectorSchema, + target: targetSelectorSchema.optional(), }) .strict(); export const tabNewResultSchema = z.object({ @@ -104,7 +105,7 @@ export const windowNewParamsSchema = z .strict(); export const windowTargetParamsSchema = z .object({ - target: targetSelectorSchema, + target: windowTargetSelectorSchema.optional(), }) .strict(); export const windowNewResultSchema = z.object({ diff --git a/skills/firefox-cli/SKILL.md b/skills/firefox-cli/SKILL.md index 1a3c930..442b821 100644 --- a/skills/firefox-cli/SKILL.md +++ b/skills/firefox-cli/SKILL.md @@ -51,9 +51,19 @@ Then use the command-specific help for the next operation instead of relying on ## Targeting -By default, commands target Firefox's active tab/window at command resolution time. +Omitted selectors are accepted only when the required window or tab is unique. If several windows or tabs exist, pass an ID or explicitly pass `active`. -Use `firefox-cli tab -h` and `firefox-cli window -h` to discover targeting options. Use tab/window indexes and `id:<id>` values from `firefox-cli tab` and `firefox-cli window` output when the active target is not enough. +Use only the `--window` and `--tab` flags advertised by each command. Values are `active`, an index from `firefox-cli tab` or `firefox-cli window`, or `id:<id>`. + +For isolated work, create a window and target every follow-up command explicitly: + +```bash +firefox-cli window new +firefox-cli open --window id:<window-id> https://example.com +firefox-cli snapshot --window id:<window-id> -i +``` + +`tab select` and `window select` bring the chosen surface forward to the user. They do not establish durable CLI target state or absolve you from passing `--window`/`--tab` explicitly to later target-dependent commands. Screenshots may activate their selected tab/window. ## Setup State ```bash