fix(audio): refuse capture overrides the microphone cannot deliver - #4129
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Warning Review limit reachedNext included review available in 1 minute. View limit detailsLimit details: You’ve used all 4 included reviews currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (6)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (6)
💤 Files with no reviewable changes (3)
Included review availability: Your plan provides up to 4 included reviews per hour; 1 remains after this review. WalkthroughMicrophone sample rate and channel overrides now require an exact match with a writable device format. When neither override is set, capture uses the device default without enumerating supported ranges. Unsupported configurations return contextual errors. Microphone stream-opening errors include the device and requested layout. ScreenCaptureKit channel-count mismatches now return Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to No identified issue prevents merging after normal checks. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to Unsupported microphone requests may now fail where capture previously attempted them. The inspected path still checks microphone permission and selects input devices only; no expanded capture authority was identified. Device and caller compatibility remains the main uncertainty. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
MERGE Positive improvement. Overrides that used to be silent hints (and could wrap: Error hygiene matches the contract: open/start failures go through Worth the complexity. The negotiate helper is small, private, and covered by fake-range unit tests (unsupported rate, wrap-safe 65537 channels, default-format preference, no-usable-format, open classification) with no hardware needed. Rejecting "always enumerate" and "pick closest" in the alternatives section is the right call given the encoder already resamples and Would do differently: nothing required. Optional follow-up only — if a host lies about ranges and then refuses the negotiated open, the new open-path This is an automated review, not the maintainer's decision |
c9f0310 to
0d90063
Compare
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
0d90063 to
9cbe9ba
Compare
Problem
capture::Config::sample_rateandchannelswere documented as hints, butcapture::resolvewrote them straight over the device's default config. There was no check that the device supports them, andchannels as u16wrapped: a request for 65537 channels opened a mono stream. When a backend refused the format, the error named neither the device nor the requested format.Approach
resolvereadssupported_input_configs(), and a new privatenegotiatepicks a range with the exact channel count that contains the requested rate. It prefers the default's sample format, then cpal'scmp_default_heuristics, and only accepts formats the callback can convert (f32/i16/u16). Channel counts are compared asu32, so nothing gets narrowed before a match. If nothing matches, it fails withError::Unsupportedand names the device, the requested format, and the usable ranges. It never falls back to another format.UnsupportedConfigandInvalidInputfrom cpal becomeError::Unsupported. The retry classification is unchanged.Unsupportedinstead ofCapture, so both sources follow the same contract (it was already fatal).ConfigandError::Unsupporteddocs to describe the actual contract.Tests use fake cpal capability ranges, so no audio device is needed. They cover: an unsupported rate, 65537 channels, picking from the device's ranges, keeping the default format, explicitly requesting the default, a device with no usable format, and backend open failures (fatal unsupported vs retryable busy). Nightly's
--all-featuresnextest runs them. #4125 adds a per-PRcapture-testrecipe that would pick them up too.Impact
sample_rate/channelsoverride the microphone does not support now fails withError::Unsupportedbefore the stream opens, where before it was passed through to the backend.screencapture.rs(macOS) has a one-token error variant change and is not compiled locally.Alternatives
Noneis the right way to ask for "whatever works".Follow-ups
Completes
quest/m1/audio-capture-format.md.(Written by Claude Opus 5.5)
🤖 Generated with Claude Code