Fix review comments: CLI entry point, concurrency guard, ffmpeg timeout, SDP mono, contact IP, and package hygiene by Copilot · Pull Request #9 · codingjoe/VoIP

Copilot · 2026-03-09T17:39:40Z

Addresses all automated review comments from the initial transcription implementation.

CLI (`voip/main.py`)

Entry point: main = voip (was sip), __main__ calls voip() — exposes the top-level group and -v/--verbose
ConsoleMessageProcessor: Fixed both request_received and response_received to forward correct args to the correct super() method (was calling super().request_received() in both, dropping args)
ImportError message: Fixed grammar and package name (libsip → voip[cli])

Transcription (`voip/whisper.py`)

Concurrency guard: Added _transcribe_task: asyncio.Task | None — audio_received only spawns a task when none is running; the task drains all full chunks in a while loop before clearing the guard
ffmpeg timeout: Replaced .run() with .run_async() + proc.communicate(timeout=decode_timeout_secs) — decode_timeout_secs is now enforced; includes proper subprocess cleanup on timeout and captures stderr for error messages

SDP / RTP (`voip/call.py`)

Mono codec: opus/48000/2 → opus/48000/1 in SDP — consistent with the Ogg container (1 channel) and ffmpeg ac=1 decode path
Contact IP: Added contact_ip kwarg to IncomingCall; IncomingCallProtocol exposes a _contact_ip property (default None), overridden in RegisterProtocol to return the STUN-discovered public IP — avoids advertising 0.0.0.0 in SDP

Tests

test_integration.py: Replaced RegisterProtocol with a minimal _STUNOnlyProtocol to avoid sending unintended SIP REGISTER to stun.l.google.com
test_main.py: Fixed stub key "sip.whisper" → "voip.whisper"
test_whisper.py: Added pytest.importorskip("whisper") guard; updated ffmpeg mocks to run_async/communicate API

Packaging / Docs

pyproject.toml: Added ffmpeg-python to cli extra (required by the transcribe command)
README.md: Fixed install snippet libsip[...] → voip[...]
voip/__init__.py: Re-exported SIP, Request, Response so import voip; voip.SIP works as shown in the README example

💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.

Co-authored-by: codingjoe <[email protected]>

voip/__main__.py

for more information, see https://pre-commit.ci

pyproject.toml

Copilot

Pull request overview

This PR addresses a collection of bug fixes and improvements from an initial transcription implementation review. It fixes the CLI entry point, adds a concurrency guard for transcription tasks, enforces an ffmpeg timeout, corrects SDP codec channels and contact IP handling, and updates packaging/documentation.

Changes:

CLI & imports: Fixed entry point (main = voip), corrected ConsoleMessageProcessor super() calls, and updated error message/package names.
Transcription engine: Added a _transcribe_task guard to prevent overlapping transcriptions and replaced .run() with .run_async() + proc.communicate(timeout=...) for proper ffmpeg timeout enforcement.
SDP/RTP & packaging: Fixed opus/48000/1 (mono) in SDP, added STUN-discovered public IP to SDP via contact_ip, and added ffmpeg-python to the cli extra in pyproject.toml.

Reviewed changes

Copilot reviewed 11 out of 14 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
`voip/whisper.py`	Added `_transcribe_task` concurrency guard; replaced `.run()` with `.run_async()` + `communicate(timeout=...)` for ffmpeg
`voip/call.py`	Fixed SDP codec to `opus/48000/1`; added `contact_ip` kwarg and `_contact_ip` property for SDP IP advertisement
`voip/__main__.py`	Fixed `main = voip`, `super()` call bugs in `ConsoleMessageProcessor`, and ImportError message
`voip/__init__.py`	Re-exported `SIP`, `Request`, `Response`, `SessionInitiationProtocol` for public API
`tests/test_whisper.py`	Updated mocks from `.run()` to `.run_async()`/`communicate`; added `pytest.importorskip("whisper")` guard
`tests/test_main.py`	Fixed stub key from `sip.whisper` to `voip.whisper`
`tests/test_integration.py`	Replaced `RegisterProtocol` with `_STUNOnlyProtocol` to avoid unintended SIP traffic in STUN tests
`tests/test_call.py`	Updated assertion to `opus/48000/1`
`pyproject.toml`	Added `ffmpeg-python` (listed as `python-ffmpeg` — wrong package name) to `cli` extra; removed integration test filter from `addopts`
`docs/images/icon.svg`	Changed stroke color to `"whiteb"` (typo)
`docs/images/logo-light.svg`	SVG layout adjustments
`docs/images/logo-dark.svg`	SVG layout adjustments
`README.md`	Fixed install snippet from `libsip[...]` to `voip[...]`
`.github/workflows/ci.yml`	Added `--cov` flag to CI pytest invocations

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review. Take the survey.

Copilot · 2026-03-09T18:51:38Z

pyproject.toml


 [project.optional-dependencies]
-cli = ["click", "pygments", "openai-whisper", "numpy"]
+cli = ["click", "pygments", "openai-whisper", "numpy", "python-ffmpeg"]


The cli extra specifies python-ffmpeg but the code uses import ffmpeg which is provided by the ffmpeg-python PyPI package (as consistently used in the whisper extra on line 48). python-ffmpeg is a completely different package that does not provide the ffmpeg Python module. Users who install voip[cli] will get the wrong package and receive an ImportError when the transcription command attempts to use ffmpeg.

Copilot · 2026-03-09T18:51:38Z

pyproject.toml

 [tool.pytest.ini_options]
 minversion = "6.0"
-addopts = "--cov --tb=short -rxs -m 'not integration'"
+addopts = "--cov --strict-markers --cov-report=xml --cov-report=term"


The -m 'not integration' filter was removed from addopts, so integration tests (requiring live network access to stun.l.google.com) will now run by default in CI. The CI workflow does not pass -m 'not integration' in any of the pytest invocations, meaning the STUN integration test will be executed against a real network in every CI job. This will cause flaky test failures in offline/restricted environments and slow down the CI on every run. The -m 'not integration' filter should be kept in addopts (or the integration test marker filter should be added to each CI pytest invocation).

Copilot · 2026-03-09T18:51:39Z

tests/test_whisper.py

+            mock_ffmpeg.Error = type("Error", (Exception,), {"stderr": b"error output"})
+            mock_proc = MagicMock()
+            mock_proc.returncode = 1
+            mock_proc.communicate.return_value = (b"", b"error output")
+            mock_ffmpeg.input.return_value.output.return_value.run_async.return_value = mock_proc
            call._decode_opus(b"bad_data")


The test for test_decode_opus__raises_on_ffmpeg_error sets stderr as a class-level attribute on the mock ffmpeg.Error type, but the code under test does raise ffmpeg.Error("ffmpeg", b"", err) passing err as the third positional argument. The mock Error class has no __init__ to assign self.stderr = err, so getattr(exc, 'stderr', b'') returns the class attribute b"error output" rather than the instance's err argument. This means the test does not actually verify that the captured stderr bytes from the subprocess are surfaced in the error message. The test passes but for the wrong reason.

Copilot · 2026-03-09T18:51:39Z

voip/call.py

            local_addr=("0.0.0.0", 0),  # noqa: S104
        )
        local_addr = rtp_transport.get_extra_info("sockname")
+        sdp_ip = self._contact_ip or local_addr[0]


The new sdp_ip = self._contact_ip or local_addr[0] logic in answer() is not covered by any test — there is no test verifying that when contact_ip is provided to IncomingCall, the SDP c= line uses the provided IP rather than the local address. Given the thorough test coverage elsewhere in TestIncomingCall, a test like test_answer__sdp_uses_contact_ip_when_provided would be expected here.

Initial plan

99e4f45

Copilot AI assigned Copilot and codingjoe Mar 9, 2026

Copilot AI mentioned this pull request Mar 9, 2026

Implement call transcription #8

Merged

Copilot started work on behalf of codingjoe March 9, 2026 17:39 View session

Address all review comments: fix CLI, whisper, call, tests, and docs

1e42052

Co-authored-by: codingjoe <[email protected]>

Copilot AI changed the title ~~[WIP] Add call transcription using OpenAI Whisper~~ Fix review comments: CLI entry point, concurrency guard, ffmpeg timeout, SDP mono, contact IP, and package hygiene Mar 9, 2026

Copilot finished work on behalf of codingjoe March 9, 2026 17:52

codingjoe reviewed Mar 9, 2026

View reviewed changes

voip/__main__.py Outdated Show resolved Hide resolved

codingjoe and others added 2 commits March 9, 2026 19:06

Apply suggestion from @codingjoe

50101dd

[pre-commit.ci] auto fixes from pre-commit.com hooks

8dd7d5f

for more information, see https://pre-commit.ci

codingjoe reviewed Mar 9, 2026

View reviewed changes

pyproject.toml Outdated Show resolved Hide resolved

codingjoe added 7 commits March 9, 2026 19:27

Apply suggestion from @codingjoe

cecc817

Update imagery

23b0855

Add module instruction

bca8c89

Set name

9aa8f26

Fix ini options

242f5c1

Add explicit coverage flag

3b37539

Add explitict xml report

28f5910

codingjoe marked this pull request as ready for review March 9, 2026 18:46

Copilot AI review requested due to automatic review settings March 9, 2026 18:46

codingjoe merged commit c8d2d01 into transcribe-call Mar 9, 2026
21 checks passed

Copilot started reviewing on behalf of codingjoe March 9, 2026 18:46 View session

codingjoe deleted the copilot/sub-pr-8 branch March 9, 2026 18:46

Copilot AI reviewed Mar 9, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix review comments: CLI entry point, concurrency guard, ffmpeg timeout, SDP mono, contact IP, and package hygiene#9

Fix review comments: CLI entry point, concurrency guard, ffmpeg timeout, SDP mono, contact IP, and package hygiene#9
codingjoe merged 11 commits intotranscribe-callfrom
copilot/sub-pr-8

Copilot AI commented Mar 9, 2026 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Mar 9, 2026

Uh oh!

Copilot AI Mar 9, 2026

Uh oh!

Copilot AI Mar 9, 2026

Uh oh!

Copilot AI Mar 9, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

Copilot AI commented Mar 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

CLI (voip/__main__.py)

Transcription (voip/whisper.py)

SDP / RTP (voip/call.py)

Tests

Packaging / Docs

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Mar 9, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 9, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 9, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 9, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Copilot AI commented Mar 9, 2026 •

edited

Loading

CLI (`voip/main.py`)

Transcription (`voip/whisper.py`)

SDP / RTP (`voip/call.py`)