Skip to content

Normalize minus key sequences and return 400 for invalid keys - #364

Open
masnwilliams wants to merge 2 commits into
mainfrom
hypeship/normalize-press-key
Open

Normalize minus key sequences and return 400 for invalid keys#364
masnwilliams wants to merge 2 commits into
mainfrom
hypeship/normalize-press-key

Conversation

@masnwilliams

@masnwilliams masnwilliams commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

summary

  • normalize literal hyphens in key combinations to the X11 minus keysym
  • classify xdotool key-conversion failures as client validation errors
  • document punctuation key syntax

why

Agents commonly express zoom-out as Ctrl+-. xdotool requires Ctrl+minus, and treating that mismatch as an internal error causes SDKs to retry a deterministic invalid request.

testing

  • go vet ./...
  • go test -race $(go list ./... | grep -v /e2e$)

Note

Low Risk
Localized to press-key input normalization and HTTP status mapping for xdotool errors; covered by new unit tests with no auth or data-model changes.

Overview
Improves press key handling so common agent input like Ctrl+- works and bad key symbols fail fast with 400 instead of 500.

Before xdotool runs, keys and optional hold_keys are passed through normalizeXdotoolKeySequence, which trims segments and rewrites a lone - to the X11 minus keysym (e.g. Ctrl+-Ctrl+minus). xdotool failures now go through xdotoolKeyError, which treats “Invalid key sequence” / “Failure converting key sequence” output as validationError so PressKey returns a client error; other xdotool failures stay server errors.

OpenAPI PressKeyRequest docs describe punctuation combos and the hyphen alias; embedded swagger is regenerated. Unit tests cover normalization and error classification.

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

@masnwilliams
masnwilliams requested a review from rgarcia September 3, 2026 01:08
@masnwilliams masnwilliams changed the title Normalize literal minus key sequences Normalize minus key sequences and return 400 for invalid keys Sep 3, 2026

@rgarcia rgarcia left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

reviewed — the change itself is safe for existing callers (everything the normalizer rewrites was already a hard xdotool error, and both matched error strings only come from xdotool's forbidden-character check, so they're input-deterministic). requesting changes for the generated spec drift; the rest is optional.

suggestions

  • server/openapi.yaml:6616openapi-3.0.yaml and the embedded spec in lib/oapi/oapi.go weren't regenerated; GetSwagger() on this branch still serves the old keys description. run make oapi-generate and commit
  • server/cmd/api/api/computer.go:40Ctrl++ (zoom in) isn't handled. xdotool drops empty tokens, so it exits 0 and silently presses only Ctrl. consider mapping a trailing empty segment to plus for symmetry with Ctrl+-

questions

  • server/cmd/api/api/computer.go:52 — unknown keysyms (ctrl+wat) still return 200 since xdotool prints No such key name ... Ignoring it. and exits 0. fine, just noting the 400 only covers forbidden punctuation, not all invalid keys

nits

  • server/cmd/api/api/computer.go:178,287hold_keys on click_mouse / move_mouse aren't normalized; optional consistency
  • server/cmd/api/api/computer_test.go:173 — add "-": "minus" and "Ctrl++": "Ctrl++" cases to pin the split/join edges
  • server/cmd/api/api/computer_test.go:195 — fixtures use ctrl+wat, which xdotool wouldn't actually reject; ctrl+- is the realistic example

@masnwilliams

Copy link
Copy Markdown
Contributor Author

addressed the required generated-spec drift in b4e3911: regenerated the downconverted spec and committed the updated lib/oapi/oapi.go, then confirmed GetSwagger() serves the new key description. (openapi-3.0.yaml remains untracked because it is intentionally gitignored as an intermediate.)

also added the bare-minus and unchanged Ctrl++ edge cases and switched the classification fixtures to realistic ctrl+- output. keeping plus normalization and click/move hold_keys normalization out of this PR so the runtime alias remains minus-only; unknown keysyms remain unchanged as noted.

@masnwilliams
masnwilliams requested a review from rgarcia September 3, 2026 02:01

@rgarcia rgarcia left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

approving — verified on b4e3911:

  • lib/oapi/oapi.go regenerated; GetSwagger() now serves the new keys description (checked by loading the embedded spec in a test)
  • openapi-3.0.yaml is gitignored as an intermediate, so no drift there — my earlier note was wrong on that file
  • go test ./cmd/api/api/ and go vet pass locally; Bugbot and CI green on the new commit
  • minus-only scope is fine; Ctrl++ can be a follow-up if agents hit it

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