fix(ari)!: externalMedia hands back a channel id that finds its stream - #305
Merged
Merged
Conversation
…-its-stream ADR-0060 recorded that correcting the AudioSocket wire format does not make the ARI route work end to end, and said both residuals were "tracked separately". Nothing tracked them. This change is that tracking, and the harvest the #302 close-out owed. The route is broken in two places, both measured against a real Asterisk 22.9.0 and captured in probe-capture.txt: - What ExternalMediaActivity sends today returns HTTP 400 "data can not be empty". The earlier note predicted a timeout; the request never gets that far. data is mandatory for encapsulation=audiosocket. - data becomes the identification UUID the stream table is keyed by, and channelId becomes the ARI Channel.Id. They are different parameters. The probe used distinct byte-order-asymmetric UUIDs so the capture says both which one travels and in which byte order. - Setting both to one UUID makes Channel.Id equal the wire UUID and the lookup hit. Reproduced twice with fresh UUIDs. - CreateExternalMediaAsync exposes data but not channelId, so a caller cannot align them even knowing how. No test caught it because all four constructions of ExternalMediaActivity use the one-argument overload, leaving both server fields null so the polling block never runs — the same shape as the wire-format defect one level up. tasks.md carries three follow-ups this change does not fix, with links rather than the words "tracked separately": the WebSocketAudioServer URL-segment key, AudioStreamMetrics' ten uncalled instruments, and the five extensions that [test-functional] is dialed at but never defines. openspec validate --all --strict: 14 passed, 0 failed. Governance tests: 129 passed, 0 failed.
… broke it A five-agent review returned ship-with-changes with two blocking findings. Both were against the proposal's own evidence, and verifying them found a third thing neither the proposal nor the review had right. 1. The capture's RUN D was labelled "exactly what ExternalMediaActivity sends today". It was not: it carried transport=tcp, which the activity sends only when a caller sets Transport. The capture recorded parameters in prose and never the query string, which is how the mislabel survived. Measured now: the activity's real request returns 400 "transport must be 'tcp' for audiosocket encapsulation", and its DEFAULT configuration returns 200 with a UnicastRTP channel that no AudioSocket server ever sees. That is three defects, not two, and the review's own prediction for this case (501 not supported, read from resource_channels.c) was also wrong. 2. "Public API, additive, no CP0002" was false. The nine-parameter signature is shipped in two packages and PackageValidation runs against 2.5.3, so adding a parameter is CP0002 in both plus CP0006 on the interface; src/Verbara.Sdk has no CompatibilitySuppressions.xml today. Two NSubstitute setups stop compiling. The Impact section now prices this instead of denying it. 3. The capture explained an HTTP 500 as a reused channelId. Also wrong: crossing format against whether anything listens on the target port isolates it as a dead target port. A wrong "not a finding" note is worse than none. The smaller fix that avoids the break entirely — mint the identifier, pass it only as data — is now written down as a rejected alternative with the reason: it fixes the activity and leaves a consumer holding only an ARI channel id unable to find the stream, which is the capability itself. Also: the probe is committed beside its capture and prints every query string; A3 becomes an argument-capture test that can actually fail against unfixed code; C1 subscribes a Stasis app before the create; C2 reverts the fix instead of breaking the assertion; C3 applies the ci:functional label, which forces the functional lane on a pull request and would have caught #302 before the queue; F1's citation is corrected to :337 and :262; and F1-F3 become prose with a task to open their change, because unchecked boxes are the one form the archive rule says does not count. openspec validate --all --strict: 14 passed, 0 failed. Governance tests: 129 passed, 0 failed.
Harol-Reina
marked this pull request as ready for review
September 24, 2026 10:48
Harol-Reina
added
the
ci:functional
…ixed code
Phase A of externalmedia-returns-a-channel-id-that-finds-its-stream. The repo
rule is that a bug fix writes its failing test first and pastes the failure
verbatim, so this commit is deliberately red:
Failed StartAsync_ShouldSendDataAndTcpTransport_WhenEncapsulationIsAudioSocket
Expected sentData not to be <null> because an audiosocket create with no data
is HTTP 400 "data can not be empty" (probe-capture.txt RUN D), so the activity
must supply the identification uuid.
Build: 0 Warning(s), 0 Error(s). Failed: 1, Passed: 48.
It is an argument-capture test, not a "GetStream finds it" test: with a mocked
resource the test would choose both the returned Channel.Id and the uuid its own
client sends, and could be made to pass today. That closed loop is the defect
this change exists to correct.
Phase A also measured four things that were previously asserted:
- Asterisk 22.9.0 and 23.4.1 behave identically on every probed run. 23 had
never been probed; there is no version difference to carry into C1.
- The uppercase-identifier miss is real: HTTP 200 with Channel.Id in the
spelling sent, while the wire renders canonical lowercase into an ordinal
dictionary. B2's negative control has a fixture instead of an argument.
- probe-capture.txt's correction #2 was itself incomplete, the third time this
capture has been wrong. A malformed `data` also returns 500, with the
listener up: channelId is free-form, data must parse as a UUID. C2's third
reversion must therefore use two well-formed UUIDs or it proves nothing.
- RS0016 is globally suppressed by Directory.Build.props:15, so PublicAPI does
not mechanically catch a NEW api row; only RS0017 (a Shipped row whose symbol
vanished) is enforced. B1 cannot lean on the compiler for that half.
And three that change B1: it produces ~30 errors, not 3, of which 27 are false
NS1004 noise from NSubstitute's analyzer; AriResourceTests.cs:161 keeps passing
after the change while asserting nothing, and the `data=` appender it would
cover has never executed; and the default grep honours .gitignore, returning 14
hits where git grep returns 17.
Phase B of externalmedia-returns-a-channel-id-that-finds-its-stream.
ExternalMediaActivity could not reach a working AudioSocket stream by any
configuration: its defaults create an RTP channel nothing connects to, and
asking for audiosocket returned HTTP 400 because the activity sends neither the
transport that encapsulation requires nor the data Asterisk demands. Measured
against Asterisk 22.9.0 and 23.4.1, which behave identically.
CreateExternalMediaAsync gains channelId on IAriChannelsResource and
AriChannelsResource. The activity now derives its request from the server it was
handed: audiosocket implies transport=tcp, and one freshly minted UUID goes to
both channelId and data, so Channel.Id IS the identification UUID the stream
table is keyed by. An AudioSocketServer supplied against a non-AudioSocket
encapsulation is now refused instead of waiting out a 30-second timeout.
The mint is canonical lowercase and that is load-bearing, not incidental:
ParseUuid keys an ordinal dictionary with Guid.ToString(), and an uppercase
identifier returns HTTP 200 with Channel.Id in the spelling sent and a lookup
that misses. Measured, with a fixture.
BREAKING. The nine-parameter signature was shipped in two packages, so this is
CP0002 in both plus CP0006 on the interface, declared in CompatibilitySuppressions.xml
(a new file for Verbara.Sdk) with each entry read before it was kept, *REMOVED*
plus new rows in both PublicAPI.Unshipped.txt, and a migration guide per ADR-0028.
Callers that passed the cancellation token positionally get CS1503 — the parameter
was appended rather than placed first, because inserting it ahead of encapsulation
would have rebound every string? argument one slot over and gone on compiling.
Contract text on IAudioServer.GetStream, IAudioStream.ChannelId and both server
implementations now says what the key is and in what form, including that
WebSocketAudioServer keys on the last URL segment instead.
Verification, with the stale-green traps forced open:
build 0 Warning(s), 0 Error(s)
unit lane 3643 passed, 0 failed
dotnet pack exit 0, 29 packages — semaphores AND nupkgs deleted first,
because PackageValidation is incremental and a repeat pack
skips it entirely
negative ctl with CompatibilitySuppressions.xml removed, pack exits 1 with
exactly CP0002 + CP0006, proving validation actually ran
A3 regression was Failed: 1, now Passed: 1
…nd harvest the residuals
Phase C of externalmedia-returns-a-channel-id-that-finds-its-stream.
ExternalMediaChannelIdFunctionalTests originates a real externalMedia against
the container and asserts the stream resolves by the returned Channel.Id. It
subscribes a Stasis application first: externalMedia never checks that `app` is
registered, so the create returns 200 against a bare listener and Asterisk then
hangs the channel up within milliseconds of a 200 ms poll. Run on Asterisk
22.9.0 and 23.4.1, passing on both.
The functional project had no reference to Verbara.Sdk.Activities at all — the
class this change is about was unreachable from the only suite that can put a
real Asterisk on the other end.
Controlled by reverting the fix three ways, not by breaking the assertion.
Dropping `data` and dropping the tcp transport both fail at the create with the
400s the probe recorded; giving channelId and data two different well-formed
UUIDs returns 200, connects, and fails at the lookup. That third reversion first
produced "Asterisk did not connect to audio server" — a sentence that is false
about what happened and indistinguishable from Asterisk never connecting. The
assertion now prints the returned channel id beside the server's own table, so
the red shows the two identifiers having come apart.
C7 opens a-published-surface-is-one-something-measures for the three residuals,
and it corrected this change on three counts:
- F3 is ten undefined extensions, not five. The suite reaches [test-functional]
by two forms and both this change and the dialplan comment counted only
Local/N@test-functional. Re-verified independently: 160 161 162 163 300 700
750 999 9998 9999.
- F1's line numbers aged inside this phase — B3's doc edits moved the key from
:337 to :341 and its registration from :262 to :266.
- The unasserting early return has four spellings and covers all eight facts in
ConfBridgeAdvancedTests, not the five that failed.
And extensions.conf was carrying the exact ADR-0060 failure this change exists to
stop: it documented the hole, stated the wrong count, and ended "the hole is
filed separately" with no link. It now names the change.
CHANGELOG entry anchored on the #302 heading rather than on [Unreleased], since a
shared anchor is what ejected #300 from the queue with fourteen checks green.
build 0 Warning(s), 0 Error(s)
unit lane 3643 passed, 0 failed, 0 projects red
openspec 15 passed, 0 failed
…at failed on the wrong command All fourteen tasks of externalmedia-returns-a-channel-id-that-finds-its-stream are checked off with their measurements. C5 is the one worth reading. The first coverage run reported a red band — line 81.0% against a floor of 83.0, branch 62.51% against 64.0 — and it was not a regression. It was the wrong command: no --settings coverlet.runsettings and no -c Release. The tell was in its own output, 42125 lines measured against the 13322 that coverage-floor.json records as honest, three times the denominator. With the exact command from ci.yml:87-93 all three gates pass: line 84.37% in band, branch 68.81%, 13479 lines measured, patch coverage 100.0% at 22/22, zero exclusion markers. The 22 was checked against the diff rather than believed — the migration guide, CHANGELOG, suppression files, PublicAPI rows and openspec artifacts carry no executable lines, and the functional test is excluded from this lane, which is why the unit tests had to carry the activity's new lines. C6 records that the pack negative control is not ceremony: PackageValidation is incremental, and with the whole suppression file deleted a repeat pack still exits 0 and prints 29 successful packages. Any future "pack is green" claim in this repository has to say how it forced validation to run. #302's did not.
Harol-Reina
changed the title
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| activity.Channel?.Id ?? "no channel", | ||
| registered.Length == 0 ? "none" : string.Join(", ", registered)); | ||
|
|
||
| activity.Channel.Should().NotBeNull( |
This was referenced Sep 24, 2026
Merged
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
2 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.
ExternalMediaActivityaccepts anAudioSocketServerand then pollsGetStream(Channel.Id). It could never hit — by any configuration. Measured against a real Asterisk 22.9.0 and 23.4.1, which behave identically.200→ a UnicastRTP channel nothing connects to; the poll waits out 30 sEncapsulation = "audiosocket"400 transport must be 'tcp' for audiosocket encapsulation400 data can not be emptyADR-0060 predicted a timeout and recorded the residual as "tracked separately". Nothing tracked it. This change is that tracking, and the harvest the #302 close-out owed.
The two identifiers were different parameters
A probe with two distinct, byte-order-asymmetric UUIDs settles which one travels, and in which order:
databecomes the identification UUID the stream table is keyed by;channelIdbecomes the ARIChannel.Id. One value in both makesChannel.Idbe the key.What changed
CreateExternalMediaAsyncgainschannelId, appended rather than placed first — deliberately. Every slot fromencapsulationtodataisstring?, so inserting ahead of them would have rebound each positional argument one place over and gone on compiling. Appending producesCS1503, which is loud.AudioSocketServeragainst a non-AudioSocket encapsulation is now refused, not left to time out."Get an active stream by channel ID"sat on the interface and, verbatim, on both shipped implementations.Rejected alternative, recorded
Passing the identifier only as
datafixes the activity with zero API break — and leaves a consumer holding only an ARI channel id unable to find the stream, which is the capability itself. The break is a purchase, not an oversight.Verification
The functional test is controlled by reverting the fix three ways, not by breaking its assertion. The third reversion first read
Asterisk did not connect to audio server— a sentence that is false about what happened and indistinguishable from Asterisk never connecting. It now prints the returned channel id beside the server's own table.The pack negative control is not ceremony:
PackageValidationis incremental, and with the suppression file deleted a repeatdotnet packstill exits 0 and prints 29 successful packages.Residuals are harvested into
openspec/changes/a-published-surface-is-one-something-measures— with a link, which is what ADR-0060 did not do.🤖 Generated with Claude Code