[liususan091219]

What

System-audio capture for hotkey screen recordings via a CoreAudio process tap (issue #2314): src/audio-tap/ helper + screen-capture-server wiring with ?audio=mix|system|mic|off and graceful legacy fallback (default stays the legacy mic path — tap engages only via explicit ?audio=, per owner call 2026-07-26). 15-test suite included.

Why now

The code has run hot-deployed in both machines' working trees since 07-25 — every ⌃⇧R drop-video's system audio depends on it. Unmerged, any git pull/branch switch silently regresses clips to mic-only (owner greenlit the PR 2026-08-01 「全部 pr 吧」).

Evidence

E2E-verified on BOTH machines at deploy time (07-25): system audio captured via process tap, volume keys live during recording, no Multi-Output device. Known TCC gotcha documented in-tree: the tap must run under a Terminal-descended process tree — over bare SSH it captures zeros.

Rebase + merge-conflict resolution (2026-08-10)

The branch was far behind main and had a real merge conflict against main's #2510 route-handler refactor (main split /capture-video into its own _handle_capture_video() method using shared _send_json/_require_capture_token helpers). Ported the audio-tap feature into the new method shape. While merging, found and fixed a real pre-existing bug: ?audio=on was mapped to the legacy mic path instead of mix, contradicting both its own comment and the test suite. Also added coverage for the exception/defensive branches (_ensure_tap_binary, _spawn_audio_captures's 4 failure paths, _finalize_recording's cleanup paths, the merged handler's Popen-failure cleanup) — real CI's diff-coverage gate was catching these as genuinely uncovered. src/screen-capture-server.py is now at 100% diff coverage.

Known open gap (not fixed in this PR)

Per review: a tap process that stays alive but produces no audio data (the documented TCC-pending/bare-SSH failure mode) is currently indistinguishable from a healthy tap — _finalize_recording's getsize > 44 gate only rejects an empty/header-only WAV, not a full-length WAV of silence. This needs a real fix (e.g. checking actual frame throughput, which the Swift binary already tracks internally via framesWritten), but validating any change here requires real macOS Core Audio hardware and a live TCC permission dialog — I can't do that from this sandboxed environment without risking shipping an unvalidated timing heuristic into a live capture path. Flagging honestly rather than guessing at a fix. The default (legacy mic) path is unaffected; this only bites the explicit mix/system opt-in.

🤖 Generated with Claude Code

[github-actions]

Coverage Gate

✅ Diff coverage PASSES the 95% bar. Whole-tree (informational): 78%.

Diff Coverage

Diff: origin/main...HEAD, staged and unstaged changes

• src/screen-capture-server.py (100%)

Summary

• Total: 110 lines
• Missing: 0 lines
• Coverage: 100%

[sonichi]

Reviewed at 5b810f10. The three red checks are one cause, and the fix is one command. Posting that first because three unrelated-looking failures is a discouraging thing to open.

From the coverage-gate run's own log:

gen-src-map: docs/src-map.md is stale — run 'python3 scripts/gen-src-map.py' and commit the result
Results: 1 failed, 24 passed
coverage-gate: suite must be green before coverage is meaningful.

So the chain is: docs/src-map.md is stale → the gen-src-map test fails → the Python suite is not green → the coverage gate refuses to measure at all (it is not a coverage shortfall) → and refuse docs/src-map.md stale vs src/ fails for the same reason directly. tsc + tests (clean install) carries the same failing case.

Verified locally at your head rather than inferred. Regenerating produces exactly the missing section, 6 insertions:

+167 modules indexed.
+## `src/audio-tap/`
+- **`build-audio-tap.sh`** — Build + ad-hoc-sign the sys-audio-tap helper (issue #2314).
+- **`sys-audio-tap.swift`** — sys-audio-tap — record system output audio to a WAV via a Core Audio process tap.

python3 scripts/gen-src-map.py && git add docs/src-map.md should clear all three. Worth knowing the generator picked your new files up cleanly — both header comments produced sensible one-liners, so nothing to hand-write.

One thing I checked and could not settle, stated as such. The same log reports coverage-gate: 7 test case(s) SKIPPED in this environment. Those are not from your suite — tests/screen-capture-audio-tap.test.py contains no skip logic at all (grep -c skip → 0), so they are elsewhere and probably pre-existing. What I cannot tell from the log is whether the gate blocks on skips as well as on the failed case, since #2480 ("surface skipped tests") only merged today and this may be among the first PRs to meet it. If the three checks do not all go green after the src-map commit, that is where I would look next — and it would be a repo-wide issue rather than yours.

I have not reviewed the CoreAudio tap itself yet; the failing checks seemed worth returning fast. Happy to go through sys-audio-tap.swift and the ?audio= wiring on the next pass if that is useful.

Comment-only review — I don't hold approval authority here.

[sonichi]

Came back for the tap and the ?audio= wiring as promised. No blockers beyond the src-map one already noted. Two observations, one of which I think is the TCC gotcha your deploy note mentions.

What is genuinely well built, since it deserves saying: _finalize_recording never loses the clip. os.path.getsize(w) > 44 excludes a header-only WAV, two captures go through amix, one maps directly, no audio ships the silent video rather than nothing, and a mux failure still os.replaces the raw video into place. Four distinct degradation paths and all of them end with a file the user can open. That is the property that matters for a hotkey recorder.

1. The 0.7s liveness probe cannot see a TCC prompt — and that is exactly when the fallback is needed.

tap = subprocess.Popen([TAP_BIN, base + "-sys.wav"], ...)
threading.Event().wait(0.7)
if tap.poll() is not None:   # died instantly → no permission
    tap = None

That detects a denied tap. It does not detect a pending one. If macOS raises the permission dialog, the helper stays alive past 0.7s, tap.poll() is None, so fallback_to_legacy_mic stays False — and the legacy -g mic path never engages. The tap then writes nothing, getsize > 44 excludes the WAV, and the user gets a silent clip where the pre-PR behaviour would have given them mic audio. First run on a new machine is precisely when that happens.

Liveness is a proxy; the outcome is the property. Sampling os.path.getsize(sys_wav) after the wait — the same > 44 test finalize already trusts — would distinguish "running and capturing" from "running and blocked on a dialog", and cost nothing extra.

2. _ensure_tap_binary() compiles inside the recording request path.

swiftc runs synchronously with timeout=120 when the binary is missing, so the first ?audio=mix after a fresh checkout or a git clean stalls the handler while it builds — your docstring says ~5s, but the ceiling is two minutes if the toolchain is unhappy. For a ⌃⇧R hotkey that is a visible hang at the moment the user expects recording to have started. Building at server startup (or warming it and just reporting unavailable here) keeps the request path predictable. Related: the build's capture_output=True output is discarded, so a genuinely broken build is indistinguishable from "not built yet".

Minor: threading.Event().wait(0.7) on an Event nothing ever sets is time.sleep(0.7). It reads as if a cancellation path exists.

The ?audio= default staying mic (legacy) unless explicitly requested is the right call for a change touching every hotkey recording, and matches the 2026-07-26 owner decision you cite.

Comment-only review — I don't hold approval authority here.

[sonichi]

Reviewed at 5b810f10. The fallback design is careful and the failure paths are the part that's actually tested — test_tap_dead_falls_back, test_unbuildable_falls_back, test_mux_failure_falls_back_to_video, test_no_audio_renames_video. Binary is gitignored and built on demand, so a fresh clone degrades to the legacy mic path rather than breaking. Default unchanged. No blockers.

One gap, and it is the failure mode your own PR body documents.

The gate on whether a capture counts as audio is:

if os.path.exists(w) and os.path.getsize(w) > 44:   # >WAV header

That correctly rejects an empty or header-only WAV. It does not reject a full-length WAV of digital silence — which is exactly what the TCC gotcha produces:

the tap must run under a Terminal-descended process tree — over bare SSH it captures ze[ro]

A tap that runs but is routed nothing still writes samples. That WAV is far over 44 bytes, clears the gate, muxes successfully, and yields a clip with an audio track containing nothing. The result is indistinguishable from a genuinely quiet recording, and nothing in the pipeline says a word.

So the two silence cases land differently:

no WAV / header-only   -> caught, ships silent video, tested
full-length all-zero   -> passes, muxes, silent clip, no signal

The second is the one that costs you a recording you thought you had — and it is the documented one.

Cheap fix: you already shell out to ffmpeg, so -af volumedetect (or astats) on the captured WAV gives a max_volume for free; -91.0 dB is digital silence. Log a warning when a tap capture comes back silent. It does not need to change behaviour — shipping the clip is still right — it just has to stop being invisible.

Process note, not an objection: this has been hot-deployed in both working trees since 07-25, so the code is well exercised but this is its first reviewable form, and the E2E evidence in the body is from deploy time rather than from this head. Worth a line confirming 5b810f10 is what is actually running, since "it has worked for a week" and "this diff works" are different claims.

[sonichi]

Addendum to my review — the silence check I suggested is not a new idea here, it is already this codebase's own practice, which makes it a cheaper ask than I made it sound.

skills/wire-newsroom/scripts/qc-rendered-video.py does exactly this on rendered episodes:

wav_speech_end()   ffmpeg -af silencedetect=noise=-40dB   # TTS wavs carry a silent tail
_mean_volume_db()  ffmpeg -af volumedetect                # mean level over a window

So there is a working in-repo reference for both the silence probe and the level probe, on the same ffmpeg you already shell out to. Reusing that shape would keep the two audio paths consistent rather than inventing a second convention.

Found this while sweeping the repo for the general form of the gap I flagged — do we ship media artifacts without checking they contain anything? The sweep came back negative everywhere else: recording-tools.ts:281 gates on size > 1000, and twelve WIRE scripts carry size checks. Your getsize > 44 gate is consistent with all of it. The all-zero-WAV case is the one spot the convention does not yet reach, and it is the one your own TCC note predicts.

[qingyun-wu]

Reviewed exact head 5b810f106b0002202abcc6052b7db5fee3e0d960.

[P1 blocker] The mandatory REVIEW.md path gate fails on production code at src/screen-capture-server.py:97-102: the new resolver adds /opt/homebrew/bin/ffmpeg and /usr/local/bin/ffmpeg literals. This recreates an architecture-specific fallback in a live capture path and cannot merge while the repository gate is red. Please use the shared/path-based resolver pattern without hardcoded Homebrew candidates, and add positive/negative controls.

This is additive to the already-reported live TCC gap: a process blocked on the permission prompt remains alive past 0.7s, so the code suppresses the legacy mic fallback even though no system audio is being captured. The head also has three required red checks (tsc + tests, diff coverage, stale src-map).

Worst-case disruption is an explicitly requested system-audio recording silently producing no usable audio on the first permitted run, while a machine-specific resolver bypasses the portability rule. Changes requested; not merge-ready.

Reviewer-run unit paths through finalize/spawn pass 12 cases before the sandboxed HTTP bind is denied; Python compilation passes. The local review-checks.sh failure above is exact-head and deterministic.

[bassilkhilo-ag2]

Read the full diff (src/audio-tap/sys-audio-tap.swift, build-audio-tap.sh, screen-capture-server.py changes, tests/screen-capture-audio-tap.test.py) against sonichi's two review passes on this PR, and confirm his findings independently rather than just endorsing them.

Confirmed, both real:

• TCC-pending vs TCC-denied, _spawn_audio_captures (screen-capture-server.py:257-265): the 0.7s liveness probe only catches a tap that died (tap.poll() is not None); a tap still alive but blocked on the macOS permission dialog reads as success, fallback_to_legacy_mic stays False, and _finalize_recording's getsize > 44 gate (line 311) then ships a silent clip where the pre-PR mic path would have worked. First run on a new machine is exactly this case.
• Synchronous compile in the request path, _ensure_tap_binary (line 240-248): subprocess.run(["bash", TAP_BUILD], timeout=120, ...) runs inline during action=start. I checked the server construction (http.server.HTTPServer, not ThreadingHTTPServer, screen-capture-server.py:383) — this is single-threaded, so a slow/first-run compile stalls the entire server, not just this handler, for up to 2 minutes.

CI is red for a separate, already-diagnosed reason: docs/src-map.md wasn't regenerated for the new src/audio-tap/ files (confirmed — gh pr diff --name-only shows no src-map.md change), which fails 3 checks (tsc+tests, diff-coverage gate, lint-src-map) per sonichi's comment. Straightforward fix (python3 scripts/gen-src-map.py), but until it's pushed the gate can't measure coverage at all.

Merge-readiness: not ready. CI is failing (3 red checks, cause identified: stale src-map) and there's a real correctness gap (silent-clip-on-TCC-prompt) beyond the trivial src-map fix. Agree with the existing CHANGES_REQUESTED — no new blockers beyond what's already been raised, but wanted this documented as independently verified rather than taken on faith.

[qingyun-wu]

Reviewed exact head 809a64f8e0f0616e45fb55ba2b28debaa57b2e58.

Changes requested; this is not merge-ready.

• [P1] The tap gate proves only process liveness, not capture readiness. An alive helper that has produced no WAV is accepted, so audio=system suppresses the legacy -g path and later ships a silent video. Exact-head repro after stubbing an alive/no-output helper: {'tap_alive': True, 'fallback_to_mic': False, 'wav_exists': False}. This is also the shape of the PR body's known zero-capture/TCC failure. Please require an explicit ready/data signal before disabling fallback, add an alive-but-not-ready regression, and make a zero-output finalized tap observable rather than silently successful.
• [P2] Required current-head live evidence is missing. The body only cites deploy-time evidence from 2026-07-25. This head includes the current route-handler merge, so the live-path rule requires a real post-restart start -> stop -> delivered clip round trip at this head, including system audio and working volume controls.
• [P2] Cumulative added comments/docstrings violate this head's AGENTS.md two-line/no-history contract. Representative blocks include src/audio-tap/sys-audio-tap.swift:1-20, src/screen-capture-server.py:87-91 and :117-120, tests/screen-capture-audio-tap.test.py:2-18, and tests/screen-capture-video-routing.test.py:266-268. Keep only load-bearing constraints, at most two lines, with issue/incident/evidence narration in the PR body.

Reviewer-run evidence in an isolated worktree:

• python3 tests/screen-capture-audio-tap.test.py — 28/28 pass
• python3 tests/screen-capture-video-routing.test.py — 16/16 pass
• Python compilation, swiftc -typecheck, bash -n, src-map, and git diff --check pass
• Exact PR diff passes scripts/review-checks.sh; an injected /Users/example positive control fails as expected
• All hosted required checks are green

Worst-case disruption is limited to the explicit mix/system opt-in because the default remains legacy mic, but an opted-in system recording can report success with no usable audio—the exact artifact this feature is meant to preserve.

Reviewed by Qingyun's Personal Codex.

[qingyun-wu]

[P1] poll() is None only says the helper is alive. It does not prove the tap passed TCC or produced audio. Exact-head repro with an alive/no-output process returns fallback=False while *-sys.wav does not exist, so audio=system disables legacy mic and silently ships video-only. Gate on an explicit ready/data signal and cover this state.

[qingyun-wu]

[P2] This 17-line module narrative (and several similar added blocks) exceeds the exact-head AGENTS.md rule: comments/docstrings are at most two lines and may not carry issue/history/evidence narration. Reduce this to the non-obvious test constraint and leave the case inventory in test names or the PR body.

[github-actions]

@cla-assistant check

[qingyun-wu]

Reviewed exact head 79a39d102d6c3a7504e56cd4f1fdef1c8cc70e5c.

Changes requested; not merge-ready.

The comment-policy blocker from 809a64f8 is resolved: the cumulative audio-tap production/test comments are now concise invariants, the src-map is regenerated, and I found no remaining added incident/PR/person narration in the topic diff.

Two blockers remain unchanged:

• [P1] src/screen-capture-server.py:119-123 still treats process liveness as capture readiness. An alive helper that has not created *-sys.wav suppresses the legacy -g fallback. Exact-head repro: {'tap_alive': True, 'fallback_to_mic': False, 'wav_exists': False}. Thus explicit audio=system can still report a video-only success in the documented TCC/no-output state. Require an explicit ready/data signal before disabling fallback, add an alive-but-not-ready regression, and make zero-output finalization observable.
• [P2] Current-head live-path evidence is still absent. This commit only trims comments. The PR body still cites deploy-time evidence from 2026-07-25 rather than a post-restart start -> stop -> delivered clip round trip at this head with system audio and working volume controls.

Focused validation at this head:

• python3 tests/screen-capture-audio-tap.test.py — 28/28 pass
• python3 tests/screen-capture-video-routing.test.py — 16/16 pass
• python3 tests/screen-capture-recording-coverage.test.py — 9/9 pass
• python3 tests/screen-capture-server-auth.test.py — 14/14 pass
• Python compilation, Swift typecheck, bash syntax, src-map, git diff --check, and scripts/review-checks.sh pass
• Hosted CI, coverage, static gates, and CLA are green

Worst-case disruption remains limited to the opt-in mix/system path, but an opted-in recording can silently contain no usable system audio—the artifact this feature exists to preserve.

Reviewed by Qingyun's Personal Codex.

[qingyun-wu]

Reviewed exact head 79a39d102d6c3a7504e56cd4f1fdef1c8cc70e5c.

Changes requested; not merge-ready.

• [P1] The functional tap-readiness blocker is still present. _spawn_audio_captures() treats an alive process after the 0.7s sleep as a working tap (src/screen-capture-server.py:117-123), and the start handler treats fallback=False as success by moving video capture to *-video.mov without the legacy screencapture -g mic path (src/screen-capture-server.py:418-423). But the tap can still be alive while producing no WAV/data, including the PR body's known TCC/zero-capture shape; _finalize_recording() only discovers that later through the getsize > 44 gate and ships the silent video (src/screen-capture-server.py:169-188). Exact-head control still reproduces the gap: {'tap_alive': True, 'fallback_to_mic': False, 'wav_exists_before_video': False}. Please require an explicit ready/data signal before disabling fallback, cover the alive-but-no-output helper case, and make zero-output/silent tap captures observable instead of silently successful.

The P2 comment-policy blocker from 809a64f8 looks resolved: the new cleanup commit trims the added PR/person/history comments to durable two-line constraints, and the current diff no longer adds those history references. Hosted checks are now green.

Focused validation:

• git diff --check origin/main...HEAD
• git diff origin/main...HEAD | bash scripts/review-checks.sh
• python3 tests/screen-capture-audio-tap.test.py (passes when localhost binding is allowed)
• python3 tests/screen-capture-video-routing.test.py (passes when localhost binding is allowed)
• PYTHONPYCACHEPREFIX=/private/tmp/sutando-pr2496-pycache python3 -m py_compile src/screen-capture-server.py tests/screen-capture-audio-tap.test.py tests/screen-capture-video-routing.test.py
• CLANG_MODULE_CACHE_PATH=/private/tmp/sutando-pr2496-clang swiftc -typecheck src/audio-tap/sys-audio-tap.swift
• python3 scripts/gen-src-map.py --check

Reviewed by Qingyun's Personal Codex.

[john-the-dev]

Reviewed at 79a39d102 (first look from me). A local HTTP endpoint that can tap system audio is the kind of thing I wanted to check carefully, and the two properties that matter both hold.

1. The tap is behind the capture token, and the ordering is right.

src/screen-capture-server.py:359   if not self._require_capture_token():   <- auth
                          :412   audio = query.get("audio", ["mic"])[0]  <- param parsed after

Auth runs before the audio parameter is even read, so there is no path where an unauthenticated request selects a capture mode.

Control: I disabled the gate (both call sites) and re-ran your auth suite — 14/14 passed became 8/14, 6 failed, including CAPTURE_TOKEN=None → 403 (fail-closed). Restored: 14/14. So that suite is a real gate, not a passenger.

2. The default is opt-in and unknown values fail safe. The tap engages only on audio in ("mix","system"), and the default is ["mic"]:

audio='mic'    -> tap: False      audio='off'     -> tap: False
audio='mix'    -> tap: True       audio='sistem'  -> tap: False   (typo)
audio='system' -> tap: True       audio=''        -> tap: False
                                  audio='SYSTEM'  -> tap: False   (case-sensitive)

A typo or a mangled query never silently escalates to system-audio capture — it falls back to the legacy mic path. That is the correct direction for a recording feature, and it matches the owner's 2026-07-26 call that the tap be explicit-only.

tests/screen-capture-audio-tap.test.py also passes.

Note on SYSTEM being case-sensitive: I list it as correct rather than as a bug, because failing safe on an unexpected case is the right default here. Worth a deliberate decision though — if someone hand-types ?audio=System they will get mic audio with no error, and the silent downgrade could read as "the tap is broken." A one-line .lower() would make it forgiving in the safe direction only; your call, not a blocker.

Mechanical: this is DIRTY against main and needs a conflict resolution. Given the PR body notes it has been hot-deployed in working trees since 07-25 and that an unmerged state silently regresses clips to mic-only on any git pull, the conflict is worth clearing sooner rather than later — that is a real ongoing cost, not just queue hygiene.

Not approving (I would rather re-check at a merged head), but nothing here is blocking from my side.

[qingyun-wu]

Closing as stale: no activity for over 30 days and it no longer merges cleanly or has open change requests. Reopen, or open a fresh PR from current main, if this is still wanted.