Repository navigation
macOS port P1-A: ZoomClient and VirtualMic seams - #49
Merged
Merged
Conversation
Four tasks, all Windows-gated, no macOS code: split the virtual mic into a FrameSink seam and a Windows backend, give its send-window law its first six tests, define the ZoomClient seam, and move the app onto it. Two scoping decisions the design spec did not anticipate, both owner-ruled 2026-09-05. First, three of ZoomClient's getters return raw Windows SDK types and their only consumers -- roster, breakout, chat_signals -- are themselves Windows-only, so putting them on the seam would drag the SDK across it to serve code that cannot run on the far side. They stay on the concrete class. Second, the two getters that DO belong become factories (MakeTalkbackSdk, InstallVirtualMic), so no SDK pointer crosses the line at all and each backend builds its own adapter. The cost is honest: main.cpp is no longer #ifdef-free in the way the spec promised -- it holds a ZoomClientWin* for those three. That is a smaller lie than pretending the SDK types are portable. Split out of P1 because ZoomClient is ~910 lines with 40+ event overrides and its macOS backend is net-new SDK work; mixing a zero-risk Windows refactor with that made one plan nobody could review as a unit. P1-B writes both macOS backends, the headless probe, and the live proof. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
CI proves the Windows tree compiles and the unit tests pass. It cannot prove the app still joins a meeting and keys a channel, because none of that runs in CI -- and nobody has actually RUN zcomms.exe since the TalkbackSdk seam rewired main.cpp's live construction path. This closes that gap and is the acceptance test for P1-A, which rewires it much harder. Twelve checks, all machine-readable, driven through the existing control API (POST /act, GET /events). The two that matter most are the pair the 2026-08-29 no-audio hunt turned on: chsends proves Zoom ACCEPTED the sends, txpeak proves they carried audio rather than silence. Either alone is exactly the reading that cost a day -- "fails == 0" was true the whole time nobody could hear anything. Also asserts what that day established as delivery law 1: sdkmic must be true, because a muted client's sends are accepted and inaudible. And that sends STOP on release -- a key that looks shut while still transmitting is the worst failure this product has. Honest about its limit: it proves everything up to the SDK boundary and cannot prove a human heard anything. That leg needs a real Zoom client on the channel, or tools/tap-detect.
ZoomMicSource did two jobs -- Zoom's IZoomSDKVirtualAudioMicEvent and the FrameSink the TX pacer drives. Only the second is portable, so only the second is above the seam now. The four-callback contract, the teardown order and the lock discipline move to the Windows backend unchanged; the crash they prevent (a TX thread reaching a revoked sender at 20 ms cadence) is the reason none of it was allowed to drift.
macos-port-p1 ran zero CI for its entire history: both workflows' push trigger allow-listed [main, macos-port], which does not include this branch. A branch allow-list silently removes the gate on any branch nobody remembered to add -- discovered only because a refactor commit here showed no check-runs at all. The Windows-correctness argument for this whole port is "the change is mechanical PLUS CI"; a gate that never runs demotes that to just "mechanical". Trade-off accepted: a same-repo PR now produces two runs per workflow (push + pull_request) instead of relying on an allow-list that can go stale again.
Fallout from the mic seam refactor: zoom_client.{h,cpp}, main.cpp and the
Spike A harness still referenced mic_source.h and ZoomMicSource, both
renamed to mic_source_win.h / ZoomMicSourceWin. None of these are compiled
on macOS, so nothing here could have caught it locally -- this branch's
first-ever CI run (see the prior commit) is what surfaced it, as
Windows build error C1083 on zoom_client.h's now-missing include.
Mechanical rename only: parameter type and comment text, nothing else.
The parameter stays a raw pointer with caller-owned lifetime -- turning
InstallVirtualMic into a factory is separate, planned work.
First tests for this path -- it could not be constructed without the SDK until the seam landed. Pins the contract CLAUDE.md calls load-bearing: send() is legal only between onMicStartSend and onMicStopSend, a revoked sender refuses rather than crashing, initialised and sending answer different operator questions, and a refusal is counted with its code so "fails=0" can never again mean both "all good" and "nothing was sent". Every pin mutation-proved.
Review finding: the !have_sender_ half of Send()'s guard had never failed any committed test -- under the documented four-callback lifecycle, Uninitialize always clears can_send_ before have_sender_, so no legal sequence produces have_sender_==false && can_send_==true, and the guard only defends an out-of-contract SDK call order (onMicStartSend before onMicInitialize). Adds a seventh test that constructs exactly that state; mutation 1 (drop !have_sender_ from Send's guard) now fails it. Also corrects two docstrings that claimed more isolation than their assertions provide: "send before initialize" pins the fresh-mic can_send_==false state, not the have_sender_ guard specifically (both gate identically when can_send_ is already false); "revoked sender" pins the have_sender_/initialised_ half of Uninitialize, with the can_send_ half proven separately by "re-initialising reopens...". No assertions changed, no src/ changes.
Portable subset only. GetTalkbackController and InstallVirtualMic were raw-pointer getters and are now factories, so no SDK pointer crosses the line. The participants/BO/chat controllers deliberately do NOT come with them: they return Windows SDK types and their only consumers are Windows-only files this phase does not port, so they stay on the concrete Windows class rather than dragging the SDK across the seam. MeetingStatus collapses to a normalised MeetingState -- session_alive() keeps its exact 2026-08-30 shape, because treating a breakout hop as 'meeting over' tore the session down on the first live room change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
ZoomClient's implementation becomes ZoomClientWin behind the abstract seam. Two raw-pointer getters become factories -- MakeTalkbackSdk and InstallVirtualMic -- so no SDK pointer crosses the line and each backend builds its own adapter. The participants/BO/chat getters stay on the concrete class, because their consumers are Windows-only and putting them on the seam would drag the Windows SDK across it to serve code that cannot run on the far side. Behaviour is unchanged: the status enum is mapped, not reinterpreted, and session_alive() keeps its exact 2026-08-30 shape. zoom_client_win.cpp is a new file rather than a git-detected move: the sibling seam commit (f113f9d) had already trimmed zoom_client.cpp down to just the shared MeetingStateName() helper, so the old ~650-line Windows implementation only survives in that commit's parent. The full old body was pulled from there, adapted, and written to the new path. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
CI caught a second consumer the seam migration missed: spikes/a-tx- latency/src/main.cpp instantiates the concrete Zoom client directly and predates the product's TalkbackSdk/TalkbackChannels abstraction -- it drives Zoom's talkback controller through its own single-channel ZoomTalkbackSource instead. Restored GetTalkbackController() as a Windows-only extra on ZoomClientWin (same justification as the participants/BO/chat getters: its only consumer is itself Windows- only), and mirrored main.cpp's mechanical edits here: zoom_client.h -> zoom_client_win.h, ZoomClient -> ZoomClientWin, status()/ MeetingStatusName -> state()/MeetingStateName, and the mic install site switched from an out-param bool to the unique_ptr<VirtualMic> factory. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
- Record the one behavioural consequence the review flagged: main.cpp's panel chip now renders six raw Zoom statuses (DISCONNECTING, LOCKED, UNLOCKED, WEBINAR_PROMOTE, WEBINAR_DEPROMOTE, UNKNOWN) as IDLE, because MeetingState collapses them -- a Task 3 seam decision, not a Task 4 regression. session_alive() is already false for all six, so the operator-visible impact is at most one published frame per teardown, and zoom_client_win.cpp's diagnostic log line still uses the raw, 15-case MeetingStatusName. Filed as N1 in a new followups doc, with a pointer comment at the call site. - Removed two dead SDK-adapter includes the seam migration should have dropped: talkback_sdk_win.h from src/app/main.cpp and mic_source_win.h from spikes/a-tx-latency/src/main.cpp. Verified unused first (grepped both files for TalkbackSdkWin/ZoomMicSourceWin -- zero hits). - Fixed two stale comments in spikes/a-tx-latency/CMakeLists.txt still naming zoom_client instead of zoom_client_win. - Added a CLAUDE.md lesson: a "does anything else call this" grep in this repo must cover src/, spikes/ AND tests/ -- spikes/a-tx-latency links zcomms_zoom and builds on Windows CI, so a consumer living there breaks the build exactly like one in src/. Cost two red Windows runs (Task 1 and Task 4) before being called out. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Six-item fix wave from the P1-A final review, all documentation or comment-only so nothing on Windows changes behaviourally: 1. tools/smoke-windows.ps1: Start-Process -ArgumentList joins its array with an unquoted space, so the default display name "ZComms smoke" split into two argv entries and the app exited 2 on the bare "smoke". Quote free-text values (name, passcode) before appending them. 2. docs/plans/2026-09-04-macos-port.md: dated amendment recording that §3's "zero #ifdef" / no-concrete-type promise does not hold -- it was already false pre-branch (WebView2 ifdefs) and P1-A adds a concrete ZoomClientWin plus four Windows-only members with no factory. Sizes §7's P2 as bigger than stated: it must seam roster/breakout/ chat_signals first. 3. src/zoom/zoom_client.h: give last_fail_code()/FailReason() the same "never branched on, not comparable across platforms" disclaimer VirtualMic::last_error() already carries, so all three seams state the convention identically. 4. docs/plans/2026-09-05-macos-port-p1a-followups.md: expanded from one item to the full carried-forward set (N1 with a corrected 20s blast radius, a B2 re-filing now that MakeTalkbackSdk() can return null, the CI-trigger story that falsifies P0-followups' P3, deferred minors, and a P1-B gate to verify the mono law live). Pointer added to the P0-followups doc's B2 entry. 5. Corrected two stale comments: zoom_client.h's getter count (three -> four, naming GetTalkbackController) and spike A's now-false claim that both transports live for the run's lifetime. 6. Pointed README's build section at the smoke test and documented its unstated sign-in prerequisite in its own .NOTES. Verified: cmake --build build-mac and ctest --test-dir build-mac both green (2/2 tests). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… rewrite on main) Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
iamfatness
force-pushed
the
macos-port-p1
branch
from
October 8, 2026 20:39
97ac6da to
ab5d20d
Compare
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
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.
Second phase of the macOS port. Puts two more boundaries behind abstract seams so a macOS backend can be written against them, and gives the virtual mic's send-window law its first tests.
No macOS backend exists yet and nothing has talked to a live meeting. That is deliberate — it is P1-B.
What's here
VirtualMicseam —ZoomMicSourcedid two jobs, implementing Zoom'sIZoomSDKVirtualAudioMicEventand being theFrameSinkthe TX pacer drives at 20 ms. Only the second is portable, so only the second is above the seam now.ZoomClientseam — the SDK lifecycle, portable subset. Two raw-pointer getters became factories (MakeTalkbackSdk,InstallVirtualMic), so no SDK pointer crosses the line.tools/smoke-windows.ps1) — CI proves the tree compiles; nothing proves the app still joins a meeting and keys a channel. This does, through the existing control API.Reviewer notes
Windows behaviour is unchanged.
zoom_client_win.cppwas reconstructed rather thangit mv'd — the seam commit had already replaced that path — so rename detection does not cover it. The final review audited all 20 hunks againstf113f9d~1: exactly one definition removed, none dropped, no changed guard, lock scope or ordering.session_alive()'s live set is preserved as a set equality, so the 2026-08-30 breakout-hop fix is intact.CI was not running on this branch for its first three commits. Both workflows were pinned to
[main, macos-port]. Fixed in39c3b60; the very first run afterwards went red on real fallout from the mic rename and309dfbcclosed it.The zero-
#ifdefpromise is formally broken.docs/plans/2026-09-04-macos-port.md§3 now carries a dated amendment.main.cppis more Windows-wired than before, because three controller getters return Windows SDK types and their consumers are Windows-only. The consequence for P2 is recorded.Before merging
Please enable branch protection on
mainrequiringWindows build & test. The final review's triage moved this from deferred to must-fix, quoting the P0 doc's own words: "Fine for P0… Not fine from P1 onward, where Windows edits stop being mechanical." This branch demonstrated the failure mode.Deferred
docs/plans/2026-09-05-macos-port-p1a-followups.mdcarries the full set, including a P1-B gate: the mono law lives below the seam in each backend and nothing pins it at the seam — a macOS backend could declare stereo, every test would pass, and per Law 5 the SDK returns success while delivering nothing audible.