Skip to content

refactor: scaffold the per-command tap registry (8)#34190

Open
davidr-cy wants to merge 4 commits into
davidr/feat/tap-cli/13629-cli-handshakefrom
davidr/feat/tap-cli/tap-command-registry
Open

refactor: scaffold the per-command tap registry (8)#34190
davidr-cy wants to merge 4 commits into
davidr/feat/tap-cli/13629-cli-handshakefrom
davidr/feat/tap-cli/tap-command-registry

Conversation

@davidr-cy

@davidr-cy davidr-cy commented Jun 30, 2026

Copy link
Copy Markdown
Contributor

Additional details

This PR scaffolds the command registry on the app side of the cypress tap feature — the single source of truth that both halves of the binding contract derive from. It sits between the runner binding (TapManager, below) and the per-command implementations (specs/run/tests, in the stacked PRs above): getSchema() serializes the registry to advertise commands to the CLI, and exec dispatches by name into it. Adding a subcommand becomes one sibling module plus one registry entry.

  • Registry (packages/app/src/runner/tap/commands/). New commands/ directory holds the registry. index.ts exports tapCommands — a satisfies Record<string, TapCommandDefinition> map that getSchema() serializes and exec dispatches against. definition.ts provides the defineCommand authoring helper: it captures params/options as literal types (via const type params) and derives the handler signature from them, so a handler is type-checked against its own schema with no annotations (a required { name: 'spec', type: 'string' } param yields params.spec: string). The first real command, health, lands as commands/health.ts, replacing the inline commands.ts that previously held it.
  • Domain vs. dispatch failures (definition.ts + tap-manager.ts). New TapCommandError is the sanctioned way for a handler to report a domain failure — a command that ran but could not do what was asked (no run mounted, no test with that id, no spec at that path). exec now wraps the handler call: a TapCommandError is turned into an { ok: false, code, message } envelope (same downstream path as dispatch failures — rendered on stderr, exit non-zero), while any other throw still escapes as a binding bug. Command-defined codes (e.g. NO_RUN, TEST_NOT_FOUND) are deliberately NOT enumerated in the frozen contract, so the envelope's code widens from a closed union to an open string.
  • Contract clarity (contract.ts). Documents the cross-process binding surface (getSchema / exec), the JSON round-trip requirement, and the version-bump rule. TapExecFailureCode is renamed to the more precise TapExecDispatchFailureCode (the reserved dispatch-level codes), and per-command result shapes (HealthResult) move out of the frozen contract to live with their command in commands/<name>.ts. Adds a TapCommandOptionSchema doc and the --name/-a option shape.
  • Hardening (tap-manager.ts). Command lookup uses Object.prototype.hasOwnProperty.call so a wire-supplied name like constructor cannot resolve to a prototype member.
  • Single-sourced contract (@packages/runner-discovery). The cross-process contract was previously hand-mirrored between the app's contract.ts and cli/lib/tap/contract.ts. This PR moves the shared surface — TAP_PROTOCOL_VERSION, the binding global-name + method-name constants, and the TapCommandParamSchema / OptionSchema / TapSchema / TapExecResult types — into a new pure, import-free packages/runner-discovery/lib/tap-contract.ts, alongside the existing runner-discovery contract that already eliminates server↔CLI hand-mirroring. cli/lib/tap/contract.ts is deleted and the CLI imports from the package root; the app's contract.ts becomes a thin re-export shim that keeps only the app-only TapBindingContract and TapExecDispatchFailureCode. To keep node path out of the app's Vite browser bundle, the app imports the path-free module via a deep-dist entry (@packages/runner-discovery/dist/tap-contract) rather than the package root.

Steps to test

App-side binding logic covered by component specs (cypress:run:ct). New/updated specs:

  • packages/app/src/runner/tap/exec-args.cy.ts — new spec for raw-string → typed coercion of params and options against the schema (required vs. optional, boolean defaulting, type rejection).
  • packages/app/src/runner/tap/tap-manager.cy.ts — updated for the registry-driven getSchema() serialization and exec dispatch, including TapCommandErrorok: false envelope mapping and the own-property command lookup.

The contract move is type-level and verified by the builds: yarn workspace @packages/runner-discovery build && test, yarn workspace @packages/app check-ts, and the CLI rollup build (cd cli && yarn build) all pass.

Manual end-to-end is exercised by the top wiring layer (#34148): start a Cypress instance, open a browser, and run cypress tap health to drive a registry command through the binding.

How has the user experience changed?

No change. This is an internal refactor of the experimental tap binding's app-side structure; it adds no new user-facing command or flow. cypress tap health behaves exactly as before — only the code that backs it was reorganized into the registry.

PR Tasks

  • [na] Is there an associated issue with maintainer approval for PR submission?
  • Have tests been added/updated?
  • [na] Has a PR for user-facing changes been opened in cypress-documentation?
  • [na] Have API changes been updated in the type definitions?

Note

Low Risk
Internal experimental tap scaffolding with no user-facing behavior change; contract consolidation is mostly type-level and covered by existing CLI/app tests.

Overview
Refactors the experimental cypress tap app binding around a commands/ registry (tapCommands, defineCommand, health as the first module) that getSchema and exec both use, replacing the monolithic commands.ts.

TapCommandError is the sanctioned domain failure path: TapManager.exec maps it to { ok: false, code, message } while other handler throws still propagate. Command lookup uses own-property checks so names like constructor cannot hit the prototype.

The CLI–app wire contract (protocol version, binding constants, schema types, TapExecResult) moves into @packages/runner-discovery/lib/tap-contract.ts; the CLI imports from the package root and the duplicated cli/lib/tap/contract is removed. The app contract.ts re-exports the shared surface and keeps app-only TapBindingContract / dispatch failure codes.

Tests: new exec-args.cy.ts; tap-manager.cy.ts updated for registry dispatch, TapCommandError, and hardened lookup.

Reviewed by Cursor Bugbot for commit 05fe25b. Bugbot is set up for automated code reviews on this repo. Configure here.

Restructure the monolithic commands.ts into a commands/ module: the
defineCommand authoring helper and TapCommandError move to definition.ts,
the health command to health.ts, and the registry itself to index.ts. The
TapManager gains the domain-failure (TapCommandError) catch and exec-args
coercion gets its own spec. This is the foundation the per-command modules
(specs, run, tests, commands) register into.
@davidr-cy davidr-cy requested a review from estrada9166 June 30, 2026 20:06
@davidr-cy davidr-cy marked this pull request as ready for review June 30, 2026 20:06
@davidr-cy davidr-cy requested a review from mschile June 30, 2026 20:36
The app re-exported the shared tap contract from the compiled CJS build
(`@packages/runner-discovery/dist/tap-contract`). Because the workspace
package resolves through a symlink to a realpath outside node_modules,
Vite's commonjs transform skips it during the app build and Rollup treats
the CJS file as ESM, failing with "TAP_PROTOCOL_VERSION is not exported".

Re-export from the zero-dependency TypeScript source (`lib/tap-contract`)
instead, matching how the app consumes every other sibling package. The
CLI keeps reading the same contract from the package's CJS build.
@cypress

cypress Bot commented Jun 30, 2026

Copy link
Copy Markdown

cypress    Run #72018

Run Properties:  status check passed Passed #72018  •  git commit 05fe25b347: fix: import the tap contract from source so Vite resolves its exports
Project cypress
Branch Review davidr/feat/tap-cli/tap-command-registry
Run status status check passed Passed #72018
Run duration 19m 27s
Commit git commit 05fe25b347: fix: import the tap contract from source so Vite resolves its exports
Committer David Rowe
View all properties for this run ↗︎

Test results
Tests that failed  Failures 0
Tests that were flaky  Flaky 9
Tests that did not run due to a developer annotating a test with .skip  Pending 1133
Tests that did not run due to a failure in a mocha hook  Skipped 0
Tests that passed  Passing 27516
View all changes introduced in this branch ↗︎
UI Coverage  64%
  Untested elements 27  
  Tested elements 48  
Accessibility  99.01%
  Failed rules  0 critical   3 serious   1 moderate   0 minor
  Failed elements 18  

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants