Spike: UltraVNC (Windows) and Apple Screen Sharing (macOS) on hosted runners - #337
Merged
Merged
Conversation
Move the server descriptions, connect/capture helpers and the per-server round trip out of test_servers.py into vncservers.py, so a second family of servers can reuse them rather than copy them. Servers are now grouped (docker, os) and a capture can opt out of the size and screen-content assertions where the host, not us, decides what is behind the framebuffer. Also factor des_encrypt()/reverse_bits() out of rfb.py's challenge response: the VNC password *file* obfuscation needs the same primitives with the key and the plaintext swapped, and should not reimplement them. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01V2PPxkQw8HZ6YUU4hbeigc
Set up the VNC server the OS itself provides -- UltraVNC as a Windows service, Apple Screen Sharing on macOS -- and test vncdotool against it with the same round trip used for the Docker servers. The setup and diagnostic steps are checked-in scripts rather than inline workflow YAML, so they can be read, reviewed and run by hand on a throwaway machine; a README beside each records why each step is the way it is (UltraVNC's mandatory password and service mode, macOS's ARD-only auth and its blank framebuffer). The workflow now only wires those scripts to test_os_servers.py, and the screenshot gallery from the Docker spike works here unchanged. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01V2PPxkQw8HZ6YUU4hbeigc
First CI round on real runners found two things. macOS Screen Sharing took over five seconds to acknowledge a key event, where a container answers in milliseconds, so a per-server timeout replaces the one tuned for Docker on loopback. And the Chocolatey feed intermittently answers with something that isn't valid XML while still exiting 0, so the UltraVNC install retries and judges success by whether winvnc.exe exists. Both findings are recorded in the server READMEs. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01V2PPxkQw8HZ6YUU4hbeigc
Now that #334 has landed, carry its review outcomes across: * the two OS jobs were near-copies differing only by shell and script, so they collapse into one matrix job with fail-fast disabled, which keeps the "one OS failing must not take the other down" property without the duplication; * say in the workflow why the trailing steps are if: always(), since that is the question the shape invites; * probe the port with bash's own /dev/tcp rather than nc, the way the Docker servers now do; * record why an open port *is* sufficient readiness here, given the Docker servers needed a content marker on top of it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01V2PPxkQw8HZ6YUU4hbeigc
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
macOS Screen Sharing is socket-activated: the first connection is what starts the server, and that connection sometimes never finishes its handshake while the next one succeeds in seconds. A longer per-request timeout can't rescue that -- the CI failure was a two-minute timeout on the test's first call, immediately followed by a screenshot step that captured fine on a fresh connection. So readiness is now a whole connection that completed a round trip, not an open port: wait_for_servers.py retries connecting until one succeeds, and CI runs it between setting the server up and testing it. That keeps a timeout inside a test meaningful, and matches how the Docker servers stop reporting ready before they have anything to serve. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01V2PPxkQw8HZ6YUU4hbeigc
CodeQL is right about the first of its two alerts: the helper printed the ini password hex to stdout, and that hex is as good as the password to anyone holding it. It now writes the value to a file that setup.ps1 reads and deletes, so the credential never reaches a step log. The second alert is DES/ECB in des_encrypt(), which RFB specifies for both the auth challenge response and the password file format -- anything stronger simply wouldn't talk to a VNC server. Marked as such in place, with the reason next to it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01V2PPxkQw8HZ6YUU4hbeigc
Tests whether CodeQL's clear-text-storage alert follows the value through a local binding. Same output either way; the name also says what the hex is for at the point it is written. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01V2PPxkQw8HZ6YUU4hbeigc
| # place a credential should end up. | ||
| password, output = argv[1], Path(argv[2]) | ||
| ini_value = vnc_passwd_hex(password) | ||
| output.write_text(ini_value, encoding="ascii") |
This was referenced Aug 13, 2026
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
3 participants
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
Tier 2 of the server compatibility plan: the VNC server the OS itself provides — UltraVNC installed as a Windows service, Apple Screen Sharing on macOS — set up on hosted runners and tested with the same round trip as the Tier 1 Docker servers.
This replaces #335, which proved the approach as a single 425-line workflow with the recipe embedded in it as inline PowerShell, inline Python and comment blocks. Here that content is checked-in scripts and READMEs, and the test harness is shared with the Docker servers rather than written twice.
Reuse rather than a second copy
tests/functional/vncservers.py(new) holds the server descriptions, connect/capture helpers and the per-server round trip.test_servers.py(Docker) andtest_os_servers.py(OS-hosted) are thin registrations over it, as is the screenshot gallery.VNCServergrew the fields the OS servers need, so their differences are data rather than a forked code path:username(ARD auth),size=None(the host's resolution, not one we configure),renders_desktop=False(a blank framebuffer is the expected result, not a failure), andtimeout(an OS-hosted server answers input far more slowly than a container).capture_screenshots.pytakes a server group (docker/os/all), so the gallery and job-summary table work unchanged on Windows and macOS.reverse_bits()anddes_encrypt()are factored out ofrfb.py's challenge response; the UltraVNC password-file script reuses them instead of reimplementing DES.Scripts, not inline YAML
tests/servers/ultravnc/—setup.ps1(install → writeultravnc.ini→ install/start the service → wait for the port),vnc_passwd_hex.py,collect-diagnostics.ps1, and a README recording why each step is what it is.tests/servers/screen-sharing/—setup.sh(create user →kickstart→ hold the display awake → wait for the port),collect-diagnostics.sh, README..github/workflows/spike-os-servers.ymlis wiring only, and the two OSes are one matrix job (fail-fast: false) rather than two near-identical ones.make test-os-serverruns the same tests locally.What each OS proves
Findings, kept next to the code
Each of these cost a CI round and is written into the relevant README: the fixed Chocolatey install path (a recursive
Program Filesscan takes 5+ minutes), the flaky community feed that reports failure while exiting 0, UltraVNC's mandatory password and service-only headless mode, macOS's ARD-only auth, its blank framebuffer, and its multi-second input latency.Verification
Run 31748681595 was green on both
windows-latestandmacos-latestbefore the rebase onto merged #334; the run for this PR's head re-proves it. Locally: unit tests pass, flake8 clean, PowerShell scripts parse, and the shared harness passes end-to-end against live x11vnc servers on both the no-auth and VNC-password paths.Follow-ups
Graduation out of spike status is unchanged from the plan: move both jobs under the change-triggered path-filtered policy, move the throwaway passwords into repository secrets, and investigate attaching a display to the macOS runner so pixels can be asserted there too.
🤖 Generated with Claude Code
https://claude.ai/code/session_01V2PPxkQw8HZ6YUU4hbeigc
Generated by Claude Code