Verify the Wayland paths against real peers, and settle the two decisions they left - #481
Merged
Merged
Conversation
set_clipboard_files, set_clipboard_html, set_clipboard_rtf and set_clipboard_csv raised OverflowError on every call, and the matching readers raised whenever that format was actually present. Each module declared restype but not argtypes, so ctypes passed the pointer-width memory handle as c_int. Declaring only restype fixes the value coming back and leaves the argument going in broken. The prototypes and the open/alloc/lock dance now live once in utils/clipboard/win32_clipboard_api.py, on private WinDLL handles so the declarations cannot leak into other user32 callers in the process. The pure-function tests could not catch this: the byte packing was always correct and nothing exercised the Win32 half. The new test round-trips text, HTML, RTF, CSV and file lists through the real clipboard, and adds a static check that no module calls a handle API without prototypes.
save_window_layout documented "every titled window" but its default lister returned all of them, while restore_window_layout addresses a window by title and skips blank ones. On a real desktop that was 28 entries saved against 15 restorable, so a caller reporting the saved count was over-promising by a factor of two. The lister now passes titled_only=True, and a round-trip test pins save and restore to the same set.
Three related gaps in window handling, all of which failed silently. A window title cannot identify a multi-process application: its windows are named after whatever they display and several of its processes have no window at all. foreground_window_process_id, window_process_id, windows_for_process_id and minimize_windows_for_process answer by ownership instead. Unavailable reads as None rather than a bare 0, which a caller could otherwise match against a process list and hit the System Idle Process. Posted input went to the top-level frame, but keyboard messages are delivered to the control that has focus, and a click belongs to the child under the point in that child's client coordinates. Measured on Character Map: posting to the frame typed nothing, posting to the focused edit typed the character. post_key_to_window and post_click_to_window resolve the real target (GetGUIThreadInfo, ChildWindowFromPointEx) and report whether the messages were queued; a printable key also posts WM_CHAR, without which most edit controls type nothing. send_key_event_to_window and send_mouse_event_to_window are deprecated and delegate to those. They kept reporting success while doing nothing, so leaving them as-is meant leaving a trap. Behaviour changes: the key sender now matches the title as a substring (it required an exact title), and the mouse sender accepts a title as well as the hwnd it always took. All four surfaces are wired: facade, AC_* commands, MCP tools and Script Builder specs.
The module declared none, so the HDC from GetDC — pointer-width — came back through ctypes' default c_int, and the same truncated value was passed on to GetPixel and ReleaseDC: a wrong colour and a leaked DC, with no symptom either way. Same trap as the HWND and HGLOBAL handles elsewhere. The module now owns private user32 / gdi32 handles so the declarations cannot leak into other callers, and its import-time SetProcessDPIAware() call is documented as what it is: process-wide and irreversible.
Command / MCP-tool counts move to 773 and 676 (the doc-count guard measures them), the touched module line counts are re-measured, and the clipboard subpackage row now names the shared Win32 prototype module. Progress.md loses the settled item: the two deprecated window-input functions are no longer a decision, they delegate.
Five container jobs replace assumptions with measurements, and none of them needs the VM three of these items had been recorded as needing: * wayland-verification — a real headless sway session, 27 checks over two output layouts, the second one negative-origin. * eis-verification — the real libeis server over a Unix socket, so the libei sender is read back off the wire, 20 checks. * portal-verification — a real dbus-daemon and real liboeffis against a portal implemented here; ConnectToEIS hands over a live fd to that same libeis, 20 checks including six refusal paths. * ydotool-verification — a real uinput device read back from /dev/input/eventN, 12 checks. * seat-verification — a wlroots session consuming the real ydotool device, cursor position read out of grim -c pixels, 14 checks over the same two layouts. A seat that consumes libinput devices is WLR_BACKENDS=headless,libinput plus LIBSEAT_BACKEND=builtin plus SEATD_VTBOUND=0, with udev up before ydotoold. A portal is whoever owns the bus name. Both were mistaken for things a container cannot do. The jobs fail loudly when uinput or evdev will not load rather than skipping, so a runner that cannot provide them says so.
Two capture defects that only a real compositor could show. A monitor left of or above the primary one puts the layout's top-left pixel at a negative coordinate, and three places assumed (0, 0): size() returned the layout's right edge rather than its width, so every caller composing size with a capture asked for half the desktop; the crop applied when a tier cannot take a region itself cropped in layout coordinates on a layout-origin image, returning black padding; and grab_logical() reported origin (0, 0), so a match on that monitor was reported a monitor's width to the right and the click missed. The backend now publishes layout_origin(), size() returns the bounding box, and the crop subtracts the origin. The xdg-desktop-portal tier could never have worked. A portal Response is directed at the connection that made the call, and the tier listened on a `gdbus monitor` subprocess while calling from a separate `gdbus` invocation — two connections, so the listener was never the addressee. Against a real bus every capture ran its full 30-second timeout. The tier now speaks D-Bus itself on one connection, subscribing to the request path it predicts before it calls, which also drops the gdbus binary requirement: _dbus_client is stdlib-only.
Eleven call sites imported PIL.ImageGrab or mss directly. On Wayland both read the XWayland root, which holds none of the native windows — so OCR, smart waits, visual regression, the recorder, the WebRTC host, the monitor enumerator, the MCP monitor tools and the template cropper each captured a blank or stale screen while reporting success. screen_grabber is now the one place that decides: it hands back an ImageGrab-shaped and an mss-shaped object backed by the compositor's own capture tool under Wayland, and by Pillow / mss everywhere else. Nothing above it changes shape. The diagnostics bundle gains a screen_capture check naming the tier in use, because a Wayland session with no capture helper is the one case where the framework can still move the mouse and see nothing. It also carries cursor_may_be_captured: wlroots composites a software cursor into the buffer wlr-screencopy hands back whenever the backend has no cursor plane, so a locator can find a pointer-shaped hole in its target, and the report that explains that failure should say so.
Four ways this backend claimed to have moved something it had not. ydotool 0.1.x — what Debian bookworm and every current Ubuntu ship under that name — exits 0 for every argument this backend builds and emits nothing at all. Run with check=True, nothing raised. The CLI generation is now classified once per process and an old one is refused by name; a version the probe does not recognise is let through so this cannot block a future release. libei discards absolute motion outside the regions the compositor advertises, without a return code, an event or an error. Region offsets are uint32, so no region can cover the negative layout space a monitor left of the primary one creates: the pointer went nowhere and never reached the ydotool fallback. The sender now maps the point into region space, retries normalised by the layout origin, and refuses what neither covers so _select_input hands over. An emission a live backend refused raised instead of falling back, as libei's own docstring claimed it did not; a chord refused part-way now releases what it already pressed. LibeiUnavailable derives from AutoControlException so it stops escaping every containment boundary, and a completed session is released rather than leaked. Absolute moves through ydotool now subtract layout_origin(): --absolute counts from the layout's top-left corner, not layout (0, 0). What the compositor then does to that displacement is acceleration nobody can read back, so JE_AUTOCONTROL_WAYLAND_POINTER_ACCEL is how an operator declares it — flat moves silently, strict refuses rather than land somewhere else, unset keeps the warn-once default. Scroll also goes through libei now, with the vertical axis negated: ydotool counts detents in the kernel's REL_WHEEL frame and libei in wl_pointer's.
Each was one module holding several, and each split along a seam that already existed rather than by line count: * main_widget.py keeps registration and layout; the screenshot, image detection, recording and script tabs move to their own modules. * webrtc_dialogs.py sheds known-host storage and verification. * remote_desktop/host.py sheds viewer access control, host capture and the per-client handler. * webrtc_host.py sheds authentication and media negotiation. * mcp_server/server.py sheds the wire protocol and client-initiated requests; http_transport takes _notification_message from _protocol. * flow_control.py sheds the data-producing commands (ocr / pdf / shell / sql / otp to var, assert_duration). No behaviour change: the moves are re-exported where callers expect them, and the tests follow the symbols to their new homes.
POST /execute and /execute_file funnelled every executor failure into
500 {"error": "execute_action failed"}, so a client could not tell a
typo in its own request from a broken server.
Every name in the list is now checked before anything runs, nested
flow-control bodies included, and an unrecognised one comes back as
400 naming all of them with nothing executed. /execute_file answers
the same way for a path that is unreadable or holds something that is
not an action list.
Only one process may have the Windows clipboard open at a time, so RuntimeError: OpenClipboard failed escaped whenever anything else was mid-copy — roughly one call in a thousand on a live desktop. win32_clipboard_api.open_clipboard() is now the single place that opens it, retrying for about 200 ms before raising as before.
CLAUDE.md now says which limits a CI job actually rejects — line length, and nothing else — so the section stops being read as a gate. The file-length limit gets its scope written down: new files always, rewrites and growth yes, flat data tables no, everything else grandfathered in Progress.md and nowhere else. test_doc_line_counts.py measures every line count the architecture map quotes and rewrites all of them with --fix. Hand-editing is how the map ended up quoting one subsystem at two sizes at once: most tables had been counting a phantom trailing line per file while the totals counted correctly. Progress.md loses both DECIDE items. Pointer acceleration became an operator declaration; the software cursor in a Wayland capture becomes documentation, because both ways around it need a cursor position Wayland will not give a client, and masking the wrong place is worse than a visible pointer. What is left there is external: two libeis observations with no peer to drive them, the consent dialog as a dialog, and whether a GitHub runner can modprobe uinput and evdev.
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 1419 |
| Duplication | 14 |
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
The negative-origin check swapped `libei._layout_origin`, which has not existed since the lookup moved to `_layout.py`; `_region_point` calls `layout_origin`, the name libei binds on import. Caught by the eis-verification job on its first run against a real EIS server, which is what that job is for. The swap raises AttributeError on a name that is not there, so the check failed loudly rather than passing while measuring nothing.
All five verification jobs ran on a GitHub runner. modprobe uinput evdev works there and systemd-udevd receives kernel uevents inside the container, which had only been measured locally on a WSL2 kernel. The jobs still fail loudly rather than skipping, so a runner kernel that stops providing either will say so.
The one that mattered was the gate: portal_verify built a filesystem path out of a string that had crossed the bus, which reads as user-controlled however the mock produced it. It now builds the path from its own two inputs — the --shot-dir it passed and the file name it expects — and compares the recorded string to that, which is both untainted and a stricter check: a portal writing somewhere else now fails instead of being followed. The rest are quality: * parse_wlr_randr, libei_verify.main and the uinput drain loop were each over the cognitive complexity limit. The first two hold a block of state per iteration, so the per-line fold and the six check closures move out; the third merges two except branches that were always the same branch, since BlockingIOError is an OSError. * _forget_device dropped the copy-then-mutate over the device map for a list of the keys to remove. * eis_server's fixture thread catches Exception rather than BaseException, so an interpreter shutdown is still an exit. * Two dict constructions in _dbus_client, an unused uniform-signature parameter in two places, and a handful of test-only ones: fixtures built before they are installed rather than mutated in place, and constructors hoisted out of pytest.raises blocks so each block names the one call expected to throw. * apt package lists sorted inside the groups they were already in.
Seven places either defaulted XDG_RUNTIME_DIR to "/tmp" or named a file directly under it — a socket path, a capture, a mock portal's shot directory. Inside the images the fallback never ran, because every Dockerfile sets XDG_RUNTIME_DIR; run one of these by hand and it wrote a predictable name into a directory any user on the host can create entries in, which is what the sockets in particular must not do. They now take XDG_RUNTIME_DIR when it is set, creating it 0700, and a fresh mkdtemp when it is not. The mock portal's --shot-dir defaults to the same rather than to /tmp; the driver has always passed one.
The sid source both images add was plain http. apt verifies package signatures either way, so this was not a way in — but the base image already carries the CA certificates pip uses, so there is nothing to trade: `apt-get update` and the `-t sid ydotool` install both complete over https, measured in the same python:3.12-slim these build from.
Every other script under docker/ annotates the same argv-list calls with the reason they are safe; this one was the only file where bandit still had something to say, so the harness now reads consistently and a real finding there will stand out.
`getattr(self, f"_do_{method}", None)` can return any attribute that
happens to carry the name, so checking it against None accepted one that
is not callable and crashed a line later. callable() is the check the
dispatch actually needs.
pylint still reads the call as not-callable, because it infers the type
of a dynamic getattr from the default and cannot narrow past a guard;
that is what the inline disable says.
Codacy honours a nosemgrep marker only when it sits on the exact line it reports, and its taint rule reports the tainted argument rather than the call, so a marker on the subprocess.run() line silenced the audit rule and left the taint one standing. Hoist the argument onto the call line so one marker covers both, and mark the two constructors in the ydotool CLI test, which build a result and an exception rather than launching anything.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this is
The Wayland backend had three things recorded as "needs a VM running a real desktop". None of them did. Replacing that assumption with five container jobs that talk to real peers turned up a set of defects that all shared one shape: a call that reported success while doing nothing, or landing somewhere else.
The jobs
wayland-verificationeis-verificationlibeis.so.1over a Unix socketportal-verificationdbus-daemon+ realliboeffisydotool-verification/dev/input/eventNseat-verificationA seat that consumes libinput devices is
WLR_BACKENDS=headless,libinput+LIBSEAT_BACKEND=builtin+SEATD_VTBOUND=0, with udev up before ydotoold. A portal is whoever owns the bus name. The second layout is negative-origin — a monitor left of the primary one.What they caught
check=True, nothing ever raised.xdg-desktop-portalcapture tier could never have worked. A portalResponseis directed at the connection that made the call; the tier listened on agdbus monitorsubprocess and called from a separategdbusinvocation. Every capture ran its full 30-second timeout.size()returned the layout's right edge rather than its width, the fallback crop cropped in the wrong coordinate space, andgrab_logical()reported origin(0, 0)so matches were reported a monitor's width to the right.--absolutecounts from the layout's top-left corner, not layout(0, 0).PIL.ImageGrabormssdirectly, so on Wayland they read the XWayland root: OCR, smart waits, visual regression, the recorder, the WebRTC host, the monitor enumerator, the MCP monitor tools and the template cropper all captured a blank screen and reported success.The two decisions they left
Both were
DECIDEinProgress.md, and both are the same problem: a compositor setting the library can measure but cannot read.JE_AUTOCONTROL_WAYLAND_POINTER_ACCEL—flatmoves silently,strictrefuses rather than land somewhere else, unset keeps today's warn-once-and-move so nothing that works now stops working. An unrecognised value falls back towarnand says so.wlr-screencopyhands back whenever the backend has no cursor plane, so a locator can find a pointer-shaped hole in its target — Windows and X11 never do this. Both mitigations need a cursor position Wayland will not give a client, and masking the wrong place is worse than a visible pointer. Thescreen_capturediagnostics check now carriescursor_may_be_captured.Also in here
POST /executeanswers 400, not 500, for a command name the executor does not know.CLAUDE.mdnow states which limits CI actually rejects;test_doc_line_counts.py --fixre-measures every line count the architecture map quotes in one pass.Test plan
python -m pytest test/unit_test/headless— 4633 passed, 16 skipped locally.ydotool-verificationandseat-verificationjobs canmodprobe uinput evdevon a GitHub runner. Both fail loudly rather than skipping if they cannot, so the answer will be unambiguous.