Normalize minus key sequences and return 400 for invalid keys - #364
Normalize minus key sequences and return 400 for invalid keys#364masnwilliams wants to merge 2 commits into
Conversation
rgarcia
left a comment
There was a problem hiding this comment.
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:6616—openapi-3.0.yamland the embedded spec inlib/oapi/oapi.goweren't regenerated;GetSwagger()on this branch still serves the oldkeysdescription. runmake oapi-generateand commitserver/cmd/api/api/computer.go:40—Ctrl++(zoom in) isn't handled. xdotool drops empty tokens, so it exits 0 and silently presses onlyCtrl. consider mapping a trailing empty segment toplusfor symmetry withCtrl+-
questions
server/cmd/api/api/computer.go:52— unknown keysyms (ctrl+wat) still return 200 since xdotool printsNo 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,287—hold_keyson click_mouse / move_mouse aren't normalized; optional consistencyserver/cmd/api/api/computer_test.go:173— add"-": "minus"and"Ctrl++": "Ctrl++"cases to pin the split/join edgesserver/cmd/api/api/computer_test.go:195— fixtures usectrl+wat, which xdotool wouldn't actually reject;ctrl+-is the realistic example
|
addressed the required generated-spec drift in also added the bare-minus and unchanged |
rgarcia
left a comment
There was a problem hiding this comment.
approving — verified on b4e3911:
lib/oapi/oapi.goregenerated;GetSwagger()now serves the newkeysdescription (checked by loading the embedded spec in a test)openapi-3.0.yamlis gitignored as an intermediate, so no drift there — my earlier note was wrong on that filego test ./cmd/api/api/andgo vetpass locally; Bugbot and CI green on the new commit- minus-only scope is fine;
Ctrl++can be a follow-up if agents hit it
summary
minuskeysymwhy
Agents commonly express zoom-out as
Ctrl+-. xdotool requiresCtrl+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,
keysand optionalhold_keysare passed throughnormalizeXdotoolKeySequence, which trims segments and rewrites a lone-to the X11minuskeysym (e.g.Ctrl+-→Ctrl+minus). xdotool failures now go throughxdotoolKeyError, which treats “Invalid key sequence” / “Failure converting key sequence” output asvalidationErrorsoPressKeyreturns a client error; other xdotool failures stay server errors.OpenAPI
PressKeyRequestdocs 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.