From 29c721311bfac2dd80bf2e75f0de30ddb6bc86a8 Mon Sep 17 00:00:00 2001 From: "Harol A. Reina H." Date: Thu, 24 Sep 2026 05:08:05 -0500 Subject: [PATCH 1/6] docs(openspec): propose externalmedia-returns-a-channel-id-that-finds-its-stream MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../.openspec.yaml | 2 + .../probe-capture.txt | 51 ++++++++ .../proposal.md | 110 ++++++++++++++++++ .../external-media-stream-routing/spec.md | 73 ++++++++++++ .../tasks.md | 80 +++++++++++++ 5 files changed, 316 insertions(+) create mode 100644 openspec/changes/externalmedia-returns-a-channel-id-that-finds-its-stream/.openspec.yaml create mode 100644 openspec/changes/externalmedia-returns-a-channel-id-that-finds-its-stream/probe-capture.txt create mode 100644 openspec/changes/externalmedia-returns-a-channel-id-that-finds-its-stream/proposal.md create mode 100644 openspec/changes/externalmedia-returns-a-channel-id-that-finds-its-stream/specs/external-media-stream-routing/spec.md create mode 100644 openspec/changes/externalmedia-returns-a-channel-id-that-finds-its-stream/tasks.md diff --git a/openspec/changes/externalmedia-returns-a-channel-id-that-finds-its-stream/.openspec.yaml b/openspec/changes/externalmedia-returns-a-channel-id-that-finds-its-stream/.openspec.yaml new file mode 100644 index 00000000..75289e4b --- /dev/null +++ b/openspec/changes/externalmedia-returns-a-channel-id-that-finds-its-stream/.openspec.yaml @@ -0,0 +1,2 @@ +schema: spec-driven +created: 2026-09-24 diff --git a/openspec/changes/externalmedia-returns-a-channel-id-that-finds-its-stream/probe-capture.txt b/openspec/changes/externalmedia-returns-a-channel-id-that-finds-its-stream/probe-capture.txt new file mode 100644 index 00000000..f2ce94ba --- /dev/null +++ b/openspec/changes/externalmedia-returns-a-channel-id-that-finds-its-stream/probe-capture.txt @@ -0,0 +1,51 @@ +ARI externalMedia / AudioSocket identification probe +==================================================== +Asterisk 22.9.0 (image verbara/asterisk-local:22, --network host), real ARI over HTTP :8088, +bare TCP listener on 127.0.0.1:19099 recording raw bytes. The listener is dependency-free on +purpose: no code from this repository decodes anything below. + +Two DISTINCT uuids per run, each asymmetric under byte reversal, so a capture says BOTH which +parameter travels AND in which byte order. A byte-order-symmetric fixture would have passed +either way; that mistake was made once already in this repository's history. + +RUN A - channelId and data DISTINCT + channelId = 0a1b2c3d-4e5f-6071-8293-a4b5c6d7e8f9 + data = f9e8d7c6-b5a4-3928-1706-f5e4d3c2b1a0 + + HTTP 200 + channel.id : 0a1b2c3d-4e5f-6071-8293-a4b5c6d7e8f9 <- channelId + channel.name : AudioSocket/127.0.0.1:19099-f9e8d7c6-... <- data + + HEX: 01 00 10 f9 e8 d7 c6 b5 a4 39 28 17 06 f5 e4 d3 c2 b1 a0 + UUID on the wire (big-endian, RFC 4122): f9e8d7c6-b5a4-3928-1706-f5e4d3c2b1a0 + == channelId ? False + == data ? True + +RUN C - data only, no channelId + HTTP 200 + channel.id : 1790244226.1 <- Asterisk-minted uniqueid, unrelated to the wire + HEX: 01 00 10 f9 e8 d7 c6 b5 a4 39 28 17 06 f5 e4 d3 c2 b1 a0 + == data ? True + +RUN D - exactly what ExternalMediaActivity sends today (no channelId, no data) + HTTP 400 -> {"message": "data can not be empty"} + +RUN B - channelId only, no data + HTTP 400 -> {"message": "data can not be empty"} + +RUN E1/E2 - channelId == data, fresh uuid each time + E1: HTTP 200 id=c165018a-d35b-4783-a057-4d43c1f7ba59 + HEX: 01 00 10 c1 65 01 8a d3 5b 47 83 a0 57 4d 43 c1 f7 ba 59 + UUID on the wire == channelId ? True == data ? True + E2: HTTP 200 id=c6868f03-d4eb-48fe-8bc3-a0b8b99f140e + HEX: 01 00 10 c6 86 8f 03 d4 eb 48 fe 8b c3 a0 b8 b9 9f 14 0e + UUID on the wire == channelId ? True == data ? True + +RUN F - channelId and data distinct, both fresh (RUN A repeated against fresh ids) + HTTP 200 id=f1fbf105-bec3-4523-a789-93a631902cbf + name=AudioSocket/127.0.0.1:19099-0f744ac5-adae-4c3b-ac91-d75a63fde417 + UUID on the wire: 0f744ac5-adae-4c3b-ac91-d75a63fde417 == data ? True + +NOT A FINDING, recorded so it is not rediscovered as one: an intermediate run of E returned +HTTP 500. The cause was reuse of a channelId already created earlier in the same session, not a +defect in the pattern. Every E run above uses a fresh uuid and returns 200. diff --git a/openspec/changes/externalmedia-returns-a-channel-id-that-finds-its-stream/proposal.md b/openspec/changes/externalmedia-returns-a-channel-id-that-finds-its-stream/proposal.md new file mode 100644 index 00000000..80cbe50f --- /dev/null +++ b/openspec/changes/externalmedia-returns-a-channel-id-that-finds-its-stream/proposal.md @@ -0,0 +1,110 @@ +--- +tier: MEDIANO +owner: Harol +approver: Harol +stakeholder: Anyone using ExternalMediaActivity with an AudioSocket server, and the Pro AgentAssist deployment that pairs the two +decision_ref: Sdk/ADR-0060 +--- + +# Proposal: externalmedia-returns-a-channel-id-that-finds-its-stream + +## Why + +ADR-0060 closed the AudioSocket wire format and said, in as many words, that doing so **does not** make +the ARI AudioSocket route work end to end: the stream table is keyed by the wire UUID while its only +consumer looks the stream up by ARI channel id, and that this was tracked separately. This is that +tracking. It is also the harvest ADR-0060's own close-out owed and did not deliver. + +### The route is broken in two places, and the first one is worse than the note said + +`ExternalMediaActivity` creates the channel and then polls for the stream: + +```csharp +Channel = await AriClient.Channels.CreateExternalMediaAsync( + App, ExternalHost, Format, encapsulation: Encapsulation, transport: Transport, ...); +... +_audioStream = _audioSocketServer.GetStream(Channel.Id); +``` + +The note predicted that poll times out. **It never gets that far.** A probe against a real Asterisk +22.9.0 sent exactly what that call sends and got: + +```text +HTTP 400 -> {"message": "data can not be empty"} +``` + +`data` is **mandatory** for `encapsulation=audiosocket`. The request fails inside +`EnsureAriSuccessAsync`, before any polling. `CreateExternalMediaAsync` exposes `data` — the activity +simply never passes it — so a caller who supplies it gets past this, and then meets the second defect. + +### The two identifiers are different parameters, and this is measured + +The same probe ran `POST /channels/externalMedia` with **distinct, byte-order-asymmetric** UUIDs in +`channelId` and in `data`, so the capture says both which parameter travels and in which byte order: + +```text +channelId = 0a1b2c3d-4e5f-6071-8293-a4b5c6d7e8f9 +data = f9e8d7c6-b5a4-3928-1706-f5e4d3c2b1a0 + +channel.id : 0a1b2c3d-4e5f-6071-8293-a4b5c6d7e8f9 +HEX : 01 00 10 f9 e8 d7 c6 b5 a4 39 28 17 06 f5 e4 d3 c2 b1 a0 +UUID on the wire == channelId ? False == data ? True +``` + +`data` becomes the identification UUID the server keys its stream table by. `channelId` becomes the +ARI `Channel.Id`. Passing neither — today's behaviour — leaves Asterisk minting its own uniqueid, +which no stream table can ever contain. + +### Passing the same UUID in both closes the gap, and that is measured too + +```text +E1: HTTP 200 id=c165018a-d35b-4783-a057-4d43c1f7ba59 + HEX: 01 00 10 c1 65 01 8a d3 5b 47 83 a0 57 4d 43 c1 f7 ba 59 + UUID on the wire == channelId ? True == data ? True +``` + +Reproduced twice with fresh UUIDs. `Channel.Id` **is** the wire UUID, and `GetStream(Channel.Id)` +hits. The full capture is `probe-capture.txt` beside this file. + +### `CreateExternalMediaAsync` cannot express it + +```csharp +public async ValueTask CreateExternalMediaAsync(string app, string externalHost, string format, + string? encapsulation = null, string? transport = null, string? connectionType = null, + string? direction = null, string? data = null, CancellationToken cancellationToken = default) +``` + +`data` is there. **`channelId` is not.** Asterisk documents it as "The unique id to assign the channel +on creation" and it is the only way to choose the ARI channel id. With this signature a caller cannot +align the two even knowing exactly how. + +### Why no test caught it + +All four constructions of `ExternalMediaActivity` in the test suite use the one-argument overload, so +both server fields stay null and the polling block is skipped entirely. The failing branch is never +executed. That is the same shape as the wire-format defect one level up: a green suite that never +reaches the code it appears to cover. + +## What Changes + +- `CreateExternalMediaAsync` gains a `channelId` parameter. Additive and optional; no existing call + site changes meaning. +- `ExternalMediaActivity` mints one UUID and passes it as **both** `channelId` and `data` when the + encapsulation is AudioSocket, so its own poll can succeed. +- A functional test originates a real `externalMedia` against a real Asterisk and asserts the stream is + found by `Channel.Id` — the assertion that would have failed before. +- The `IAudioServer.GetStream` contract says what its key is, instead of "channel ID", which is the + wording that invited the confusion. + +## Impact + +- **Public API, additive.** A new optional parameter on a shipped method; `PublicAPI.Unshipped.txt` + records it. No `CP0002`/`CP0011`, so no new suppression entry. +- **Behavioural, for AudioSocket callers only.** `ExternalMediaActivity` with + `Encapsulation = "audiosocket"` goes from an HTTP 400 to a working stream. RTP callers are untouched. +- **Downstream:** Pro's AgentAssist pairs this activity with an AudioSocket server and is the reason + the route matters. +- **Out of scope, and stated rather than implied:** `WebSocketAudioServer` keys its table by the last + segment of the request URL, which is a second and different mismatch; and `AudioStreamMetrics` + declares ten instruments with no production call site. Both are named in `tasks.md` as follow-ups + with no fix here. diff --git a/openspec/changes/externalmedia-returns-a-channel-id-that-finds-its-stream/specs/external-media-stream-routing/spec.md b/openspec/changes/externalmedia-returns-a-channel-id-that-finds-its-stream/specs/external-media-stream-routing/spec.md new file mode 100644 index 00000000..1bd0f07e --- /dev/null +++ b/openspec/changes/externalmedia-returns-a-channel-id-that-finds-its-stream/specs/external-media-stream-routing/spec.md @@ -0,0 +1,73 @@ +# external-media-stream-routing Delta + +## ADDED Requirements + +### Requirement: An external media channel SHALL be findable by the identifier its creator holds +A caller that creates an external media channel and owns the audio server it points at SHALL be able +to find that channel's stream using the identifier the create call returned, without correlating two +identifier namespaces by hand. + +Where the transport identifies its connection with a caller-supplied value — AudioSocket does, in its +identification frame — the SDK SHALL make the ARI channel id and that value the same value, by +supplying one identifier to both parameters that carry them. The SDK SHALL NOT leave the ARI channel +id to be minted by Asterisk while keying the stream table on a different value, because no lookup can +succeed across those two namespaces. + +#### Scenario: A stream is found by the channel id the create call returned +- **GIVEN** an AudioSocket server owned by the caller and running +- **WHEN** the caller creates an external media channel with AudioSocket encapsulation pointing at it +- **AND** Asterisk connects and sends its identification frame +- **THEN** looking the stream up by the returned `Channel.Id` SHALL return that stream +- **AND** the identification frame's UUID SHALL equal that same `Channel.Id` + +#### Scenario: A create that cannot be routed does not silently become a timeout +- **GIVEN** a request for AudioSocket encapsulation that omits the identifier Asterisk requires +- **WHEN** the channel is created +- **THEN** the failure SHALL surface as the error Asterisk returned, at the create call +- **AND** SHALL NOT be reported later as the audio server having failed to connect + +### Requirement: The ARI external media surface SHALL expose the parameter that names the channel +The SDK's external media create method SHALL accept the ARI `channelId` parameter, which Asterisk +documents as the unique id to assign the channel on creation. Without it a caller cannot choose the +channel id, and therefore cannot make it match anything. + +The parameter SHALL be optional and additive: an existing call site that omits it SHALL behave exactly +as before, and the change SHALL NOT alter the shipped public API surface in a breaking way. + +#### Scenario: A caller chooses the channel id +- **GIVEN** a caller that has generated an identifier +- **WHEN** it creates an external media channel supplying that identifier as the channel id +- **THEN** the returned channel's id SHALL be that identifier + +#### Scenario: An existing caller is unaffected +- **GIVEN** an existing call site that supplies no channel id +- **WHEN** it creates an external media channel +- **THEN** the call SHALL behave as it did before this change + +### Requirement: A stream lookup contract SHALL say which identifier it takes +Any published contract for looking a stream up by key SHALL state what that key is and where a caller +obtains it, rather than naming it only as a channel id. Where two implementations of the same contract +key on different values, the contract SHALL say so. + +The documentation SHALL be specific enough that a reader can tell whether a value they already hold is +a valid key, without reading the implementation. + +#### Scenario: A reader can tell what to pass +- **GIVEN** the published documentation of a stream lookup +- **WHEN** a reader holds an ARI channel id and wants the stream for it +- **THEN** the documentation SHALL state whether that value is a valid key for that implementation + +## Architectural Risk + +**Level:** LOW + +**Affected:** `Verbara.Sdk.Ari` (the ARI channels resource and the AudioSocket server's documented +contract), `Verbara.Sdk.Activities` (`ExternalMediaActivity`), and downstream `Verbara.Sdk.Pro` +AgentAssist, which pairs that activity with an AudioSocket server and is the consumer the route exists +for. + +**Mitigation:** the API change is a new optional parameter, so no shipped signature changes meaning +and `PackageValidation` reports nothing. The behavioural change reaches only callers that ask for +AudioSocket encapsulation — RTP callers take an unchanged path. The end-to-end claim is pinned by a +functional test that originates against a real Asterisk rather than a fake, because the defect being +fixed survived six months of tests that never reached the code they appeared to cover. diff --git a/openspec/changes/externalmedia-returns-a-channel-id-that-finds-its-stream/tasks.md b/openspec/changes/externalmedia-returns-a-channel-id-that-finds-its-stream/tasks.md new file mode 100644 index 00000000..bfd7f4b8 --- /dev/null +++ b/openspec/changes/externalmedia-returns-a-channel-id-that-finds-its-stream/tasks.md @@ -0,0 +1,80 @@ +# Tasks: externalmedia-returns-a-channel-id-that-finds-its-stream + +Three phases. A batched, B one focused subagent per task, C batched. Never inline in the main session. + +This is a bug fix, so **the failing regression test is written first, against the unfixed code, and its +failure is pasted here verbatim.** A task is not checked off until the thing it claims actually ran. + +## Phase A — foundation (batched) + +- [ ] A1 Re-run `probe-capture.txt` end to end and confirm it still reproduces on this machine: + `data` on the wire, `channelId` as `Channel.Id`, HTTP 400 `data can not be empty` for the + parameters `ExternalMediaActivity` sends today, and HTTP 200 with both equal. Use a **fresh** + UUID per run — a reused `channelId` returns HTTP 500 and is not a finding. Paste the output. +- [ ] A2 Read the surface that changes: `AriChannelsResource.CreateExternalMediaAsync`, + `ExternalMediaActivity`, `AudioSocketServer.GetStream`, `CompositeAudioServer.GetStream`, and the + XML docs on `IAudioServer` / `IAudioStream` in `src/Verbara.Sdk/IAriClient.cs`. Record the exact + current signatures and doc text, so the diff is against what is there rather than what is + remembered. +- [ ] A3 **Write the failing regression test first.** A unit test that constructs + `ExternalMediaActivity` with an AudioSocket server and asserts the stream is found by + `Channel.Id`. It must fail against the unfixed code, and the failure text goes in this file + verbatim — not a summary of it. Note which of the two defects it catches; if it only catches + one, say which and add the second test rather than widening the first. +- [ ] A4 Check whether any existing test would change meaning. All four current constructions of + `ExternalMediaActivity` use the one-argument overload, so the polling block never runs — confirm + that against the tree rather than trusting this sentence, and list any test that starts + executing code it did not execute before. + +## Phase B — critical components (one focused subagent each) + +- [ ] B1 Add the optional `channelId` parameter to `CreateExternalMediaAsync`. Place it so no existing + positional call site changes meaning, append the row to `PublicAPI.Unshipped.txt`, and confirm + `dotnet pack` reports no `CP0002`/`CP0011` — an additive optional parameter should need no + suppression entry, and if it does, that is a finding, not a formality. +- [ ] B2 Make `ExternalMediaActivity` mint one UUID and pass it as both `channelId` and `data` when the + encapsulation is AudioSocket. Decide explicitly what happens for other encapsulations and write + the reason in the code: RTP does not carry a caller-supplied identifier, so forcing one there + would change behaviour for callers this change has no business touching. +- [ ] B3 Fix the contract text. `IAudioServer.GetStream` currently says "Get an active stream by + channel ID" and `IAudioStream.ChannelId` says "Unique ID of the external media channel in + Asterisk" — both assert an identity the code does not hold. Say what the key actually is for + each implementation, including that `WebSocketAudioServer` keys on something else entirely. + +## Phase C — integration (batched) + +- [ ] C1 Functional test: originate a real `externalMedia` against the real Asterisk container and + assert the stream is found by `Channel.Id`. Put it beside `AudioSocketWireFunctionalTests`. It + must run in the `merge_group` lane — a pull request short-circuits that job and reports `pass` + in 16 seconds without starting Asterisk, which is how a broken dialplan reached the queue in + #302. +- [ ] C2 **Mutation-check the new functional test rather than trusting it.** Break the expected value + and confirm it fails. A fixture that passes while measuring nothing is the exact failure this + repository has now shipped twice. +- [ ] C3 Pick an extension number for any new dialplan entry that no test already dials. + `[test-functional]` is dialed at **160, 300, 700, 999 and 9998** and defines none of them; 710 + and 711 are this change's neighbours and are taken. +- [ ] C4 `CHANGELOG.md [Unreleased]`: a `### Fixed` entry. Give it its **own insertion anchor** + distinct from any other in-flight entry — a shared anchor ejected #300 from the merge queue with + fourteen checks green. Leave `(#N)` for close-out. +- [ ] C5 Coverage measured **after committing**, never before. Read the changed-line count and check it + against the size of the diff before believing the percentage: a sibling change accepted + `100% (6/6)` on a five-file diff and the `6` was the tell. +- [ ] C6 `openspec validate --all --strict` green, full unit lane green, Governance green, and CI green + on the PR — including the `merge_group` build, which is the only place the functional suite + actually runs. + +## Follow-ups this change does NOT fix, recorded so they are not lost again + +ADR-0060 wrote "tracked separately" with no link, and the close-out archived it anyway. That is what +these lines exist to prevent repeating. + +- [ ] F1 `WebSocketAudioServer` keys `_streams` by the **last segment of the request URL** + (`WebSocketAudioServer.cs:328`), and the SDK's own example puts a literal `/audio` there — so + every concurrent call would register under the key `"audio"`. `CompositeAudioServer` hands the + same string to both servers, which do not agree on what it means. Needs its own change. +- [ ] F2 `AudioStreamMetrics` declares ten instruments with zero production call sites. +- [ ] F3 `[test-functional]` is dialed at 160, 300, 700, 999 and 9998 and defines none of them. + `ConfBridgeAdvancedTests` alone dials the undefined 700 from ten call sites and passes by taking + its `if (confJoin is null) return;` branch without asserting anything. The shape to hunt is that + early return, not the extension numbers. From 1d4372e07d49c047c08ff1623aba37b9b5cf7eac Mon Sep 17 00:00:00 2001 From: "Harol A. Reina H." Date: Thu, 24 Sep 2026 05:34:27 -0500 Subject: [PATCH 2/6] docs(openspec): revise the externalmedia proposal after a review that broke it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../probe-capture.txt | 142 +++++++++----- .../probe-externalmedia.py | 143 ++++++++++++++ .../proposal.md | 144 ++++++++------- .../external-media-stream-routing/spec.md | 104 +++++++---- .../tasks.md | 174 ++++++++++++------ 5 files changed, 498 insertions(+), 209 deletions(-) create mode 100644 openspec/changes/externalmedia-returns-a-channel-id-that-finds-its-stream/probe-externalmedia.py diff --git a/openspec/changes/externalmedia-returns-a-channel-id-that-finds-its-stream/probe-capture.txt b/openspec/changes/externalmedia-returns-a-channel-id-that-finds-its-stream/probe-capture.txt index f2ce94ba..07c08a3c 100644 --- a/openspec/changes/externalmedia-returns-a-channel-id-that-finds-its-stream/probe-capture.txt +++ b/openspec/changes/externalmedia-returns-a-channel-id-that-finds-its-stream/probe-capture.txt @@ -4,48 +4,100 @@ Asterisk 22.9.0 (image verbara/asterisk-local:22, --network host), real ARI over bare TCP listener on 127.0.0.1:19099 recording raw bytes. The listener is dependency-free on purpose: no code from this repository decodes anything below. -Two DISTINCT uuids per run, each asymmetric under byte reversal, so a capture says BOTH which -parameter travels AND in which byte order. A byte-order-symmetric fixture would have passed -either way; that mistake was made once already in this repository's history. - -RUN A - channelId and data DISTINCT - channelId = 0a1b2c3d-4e5f-6071-8293-a4b5c6d7e8f9 - data = f9e8d7c6-b5a4-3928-1706-f5e4d3c2b1a0 - - HTTP 200 - channel.id : 0a1b2c3d-4e5f-6071-8293-a4b5c6d7e8f9 <- channelId - channel.name : AudioSocket/127.0.0.1:19099-f9e8d7c6-... <- data - - HEX: 01 00 10 f9 e8 d7 c6 b5 a4 39 28 17 06 f5 e4 d3 c2 b1 a0 - UUID on the wire (big-endian, RFC 4122): f9e8d7c6-b5a4-3928-1706-f5e4d3c2b1a0 - == channelId ? False - == data ? True - -RUN C - data only, no channelId - HTTP 200 - channel.id : 1790244226.1 <- Asterisk-minted uniqueid, unrelated to the wire - HEX: 01 00 10 f9 e8 d7 c6 b5 a4 39 28 17 06 f5 e4 d3 c2 b1 a0 - == data ? True - -RUN D - exactly what ExternalMediaActivity sends today (no channelId, no data) - HTTP 400 -> {"message": "data can not be empty"} - -RUN B - channelId only, no data - HTTP 400 -> {"message": "data can not be empty"} - -RUN E1/E2 - channelId == data, fresh uuid each time - E1: HTTP 200 id=c165018a-d35b-4783-a057-4d43c1f7ba59 - HEX: 01 00 10 c1 65 01 8a d3 5b 47 83 a0 57 4d 43 c1 f7 ba 59 - UUID on the wire == channelId ? True == data ? True - E2: HTTP 200 id=c6868f03-d4eb-48fe-8bc3-a0b8b99f140e - HEX: 01 00 10 c6 86 8f 03 d4 eb 48 fe 8b c3 a0 b8 b9 9f 14 0e - UUID on the wire == channelId ? True == data ? True - -RUN F - channelId and data distinct, both fresh (RUN A repeated against fresh ids) - HTTP 200 id=f1fbf105-bec3-4523-a789-93a631902cbf - name=AudioSocket/127.0.0.1:19099-0f744ac5-adae-4c3b-ac91-d75a63fde417 - UUID on the wire: 0f744ac5-adae-4c3b-ac91-d75a63fde417 == data ? True - -NOT A FINDING, recorded so it is not rediscovered as one: an intermediate run of E returned -HTTP 500. The cause was reuse of a channelId already created earlier in the same session, not a -defect in the pattern. Every E run above uses a fresh uuid and returns 200. +Reproduce with probe-externalmedia.py beside this file. It prints the FULL query string of every +request, which this capture did not do in its first version — and that omission is what let a wrong +conclusion survive review. See CORRECTIONS at the end. + +Every run uses UUIDs that are asymmetric under byte reversal, so a capture says BOTH which parameter +travels AND in which byte order. A byte-order-symmetric fixture would have passed either way; that +mistake was made once already in this repository's history. + +-------------------------------------------------------------------------------- +PART 1 — what the SDK sends today +-------------------------------------------------------------------------------- + +RUN G — ExternalMediaActivity with Encapsulation = "audiosocket", Transport unset. + This is what the class actually puts on the wire for an AudioSocket caller. + QUERY: app=probe&external_host=127.0.0.1%3A19099&format=slin16&encapsulation=audiosocket + HTTP 400 -> "transport must be 'tcp' for audiosocket encapsulation" + +RUN H — ExternalMediaActivity with its DEFAULTS (Encapsulation unset => rtp/udp). + QUERY: app=probe&external_host=127.0.0.1%3A19099&format=slin16 + HTTP 200 id=1790245644.0 name=UnicastRTP/127.0.0.1:19099-0x7f6ce8002100 + No AudioSocket connection is ever made. An AudioSocketServer handed to this activity waits out + ConnectionTimeout and the activity throws TimeoutException — which is exactly what ADR-0060 + predicted, for the class's default configuration. + +RUN D — audiosocket + transport=tcp, no data. + QUERY: app=probe&external_host=127.0.0.1%3A19099&format=slin&encapsulation=audiosocket&transport=tcp + HTTP 400 -> "data can not be empty" + Reached only once transport=tcp is supplied. The activity does not supply it today. + +-------------------------------------------------------------------------------- +PART 2 — which parameter reaches the wire +-------------------------------------------------------------------------------- + +RUN A — channelId and data DISTINCT, listener up. + channelId = 0a1b2c3d-4e5f-6071-8293-a4b5c6d7e8f9 + data = f9e8d7c6-b5a4-3928-1706-f5e4d3c2b1a0 + HTTP 200 + channel.id : 0a1b2c3d-4e5f-6071-8293-a4b5c6d7e8f9 <- channelId + channel.name : AudioSocket/127.0.0.1:19099-f9e8d7c6-... <- data + HEX: 01 00 10 f9 e8 d7 c6 b5 a4 39 28 17 06 f5 e4 d3 c2 b1 a0 + UUID on the wire (big-endian, RFC 4122): f9e8d7c6-b5a4-3928-1706-f5e4d3c2b1a0 + == channelId ? False == data ? True + +RUN F — RUN A repeated with fresh ids. Same result: the wire carries data. + +RUN C — data only, no channelId, listener up. + HTTP 200 channel.id : 1790244226.1 <- Asterisk-minted uniqueid, unrelated to the wire + HEX: 01 00 10 f9 e8 d7 c6 b5 a4 39 28 17 06 f5 e4 d3 c2 b1 a0 == data ? True + Note what this run proves: the stream is findable TODAY by the value the caller passed as data, + with no API change at all. Choosing channelId over that is a deliberate cost, recorded in + proposal.md under "Rejected alternative". + +-------------------------------------------------------------------------------- +PART 3 — the pattern, and what a 500 actually means +-------------------------------------------------------------------------------- + +channelId == data, fresh uuid each run, crossed against format and against whether anything is +listening on the target port: + + format=slin16 listener UP -> HTTP 200 id=cd98c592-9ce2-4093-afae-420d53629a4f + HEX: 01 00 10 cd 98 c5 92 9c e2 40 93 af ae 42 0d 53 62 9a 4f + UUID on the wire == channelId ? True + format=slin16 listener DOWN -> HTTP 500 "An internal error prevented this request..." + format=slin listener UP -> HTTP 200 id=5433739b-2d3b-4e8a-97f2-3c57e0981278 + UUID on the wire == channelId ? True + format=slin listener DOWN -> HTTP 500 + +HTTP 500 means NOTHING IS LISTENING on external_host. It is not about the format and not about the +channel id. chan_audiosocket connects during the create call, so a create against a dead port fails +the request rather than producing a channel that never connects. + +-------------------------------------------------------------------------------- +CORRECTIONS to the first version of this file +-------------------------------------------------------------------------------- + +1. 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 activity's real + request is RUN G, and its error is a different one. The capture recorded parameters in prose and + not the query string, which is why the mislabel survived. Every run above now carries its QUERY. + +2. The first version explained an HTTP 500 as "reuse of a channelId already created earlier in the + same session". That is wrong. The crossed runs above isolate it: the 500 is a dead target port. + A wrong "not a finding" note is worse than no note, because it sends the next reader away from + the cause. + +3. Neither error code predicted during review was right. A reading of res/ari/resource_channels.c + predicted 501 "encapsulation and/or transport is not supported" for RUN G; 22.9.0 returns 400 + with a specific message instead. Measured beats read, again. + +-------------------------------------------------------------------------------- +NOT YET MEASURED +-------------------------------------------------------------------------------- +- Asterisk 23. This probe ran only against 22.9.0 on verbara/asterisk-local:22. The CI lane builds + its own image from docker/Dockerfile.asterisk for 22 and 23; task A1 runs the probe against both. +- The WebSocket transport. Nothing here measures what externalMedia with transport=websocket puts + in the request path, which is why F1 stays out of this change rather than being designed from a + reading of the code. diff --git a/openspec/changes/externalmedia-returns-a-channel-id-that-finds-its-stream/probe-externalmedia.py b/openspec/changes/externalmedia-returns-a-channel-id-that-finds-its-stream/probe-externalmedia.py new file mode 100644 index 00000000..c8db90c9 --- /dev/null +++ b/openspec/changes/externalmedia-returns-a-channel-id-that-finds-its-stream/probe-externalmedia.py @@ -0,0 +1,143 @@ +#!/usr/bin/env python3 +"""Measures what Asterisk puts on an AudioSocket wire for POST /channels/externalMedia. + +Dependency-free on the SDK on purpose: nothing in this repository decodes a byte here. A parser +checked against its own encoder agrees with itself and says nothing about the wire — that closed +loop is what ADR-0060 exists to break, and this probe is the other end of it. + +Run it against a container started as: + + docker run -d --rm --name as-probe --network host \ + -v "$PWD/docker/functional/asterisk-config:/etc/asterisk:ro" verbara/asterisk-local:22 + +Then: python3 probe-externalmedia.py (needs the `websocket-client` package) + +Every run prints its FULL query string. The first version of probe-capture.txt recorded parameters +in prose instead, and a run labelled "what the SDK sends" turned out to carry an extra parameter +nobody could see. Print the request or the capture cannot be audited. +""" +import base64, json, socket, threading, time, urllib.error, urllib.parse, urllib.request, uuid + +import websocket # websocket-client + +ARI = "http://127.0.0.1:8088/ari" +AUTH = base64.b64encode(b"testari:testari").decode() +PORT = 19099 + + +def _listen(box, seconds): + s = socket.socket() + s.setsockopt(socket.SOL_SOCKET, socket.SO_REUSEADDR, 1) + s.bind(("0.0.0.0", PORT)) + s.listen(4) + s.settimeout(seconds) + box["bytes"] = b"" + try: + conn, _ = s.accept() + except socket.timeout: + s.close() + return + conn.settimeout(1.0) + deadline = time.time() + seconds + while time.time() < deadline: + try: + chunk = conn.recv(65536) + except socket.timeout: + continue + if not chunk: + break + box["bytes"] += chunk + # The identification frame is 3 header bytes + 16 UUID bytes. Stop there, before audio + # floods the capture. + if len(box["bytes"]) >= 19 and box["bytes"][0] == 0x01: + break + conn.close() + s.close() + + +def run(label, params, listener=True, seconds=4.0): + box = {} + thread = None + if listener: + thread = threading.Thread(target=_listen, args=(box, seconds)) + thread.start() + time.sleep(0.3) + + query = urllib.parse.urlencode(params) + request = urllib.request.Request(f"{ARI}/channels/externalMedia?{query}", method="POST") + request.add_header("Authorization", "Basic " + AUTH) + try: + with urllib.request.urlopen(request, timeout=10) as response: + body = json.loads(response.read().decode()) + outcome = f"HTTP {response.status} id={body.get('id')} name={body.get('name')}" + except urllib.error.HTTPError as error: + outcome = f"HTTP {error.code} {json.loads(error.read().decode()).get('message')}" + + if thread: + thread.join() + + print(f"\n{label}") + print(f" QUERY : {query}") + print(f" LISTENER: {'up' if listener else 'down'}") + print(f" -> {outcome}") + + captured = box.get("bytes", b"") + if captured: + head = captured[:19] + print(f" HEX : {' '.join(f'{b:02x}' for b in head)}") + if len(head) >= 19 and head[0] == 0x01: + p = head[3:19] + wire = "%s-%s-%s-%s-%s" % ( + p[0:4].hex(), p[4:6].hex(), p[6:8].hex(), p[8:10].hex(), p[10:16].hex()) + print(f" UUID : {wire}") + print(f" == channelId ? {wire == params.get('channelId')}" + f" == data ? {wire == params.get('data')}") + return box.get("bytes", b"") + + +def main(): + # externalMedia never checks that `app` is registered, but the channel it creates runs Stasis + # afterwards: with no subscriber Asterisk hangs it up and the session disappears in + # milliseconds. Subscribe first, or a run measures a teardown race instead of a handshake. + events = websocket.WebSocket() + events.connect( + f"ws://127.0.0.1:8088/ari/events?api_key=testari:testari&app=probe&subscribeAll=true", + timeout=10) + + host = {"app": "probe", "external_host": f"127.0.0.1:{PORT}"} + audiosocket = {**host, "format": "slin16", "encapsulation": "audiosocket", "transport": "tcp"} + try: + print("=" * 78) + print("PART 1 — what the SDK sends today") + run("RUN G — Encapsulation='audiosocket', Transport unset (what the activity sends)", + {**host, "format": "slin16", "encapsulation": "audiosocket"}, listener=False) + run("RUN H — the activity's defaults (no encapsulation => rtp/udp)", + {**host, "format": "slin16"}, listener=False) + run("RUN D — audiosocket + tcp, no data", + {**host, "format": "slin", "encapsulation": "audiosocket", "transport": "tcp"}, + listener=False) + + print("\n" + "=" * 78) + print("PART 2 — which parameter reaches the wire") + run("RUN A — channelId and data DISTINCT", + {**audiosocket, "channelId": str(uuid.uuid4()), "data": str(uuid.uuid4())}) + time.sleep(1) + run("RUN C — data only, no channelId", + {**audiosocket, "data": str(uuid.uuid4())}) + + print("\n" + "=" * 78) + print("PART 3 — the pattern, crossed against format and against a live target") + for fmt in ("slin16", "slin"): + for listening in (True, False): + identifier = str(uuid.uuid4()) + time.sleep(1) + run(f"channelId == data, format={fmt}", + {**audiosocket, "format": fmt, "channelId": identifier, "data": identifier}, + listener=listening) + finally: + events.close() + print("\n" + "=" * 78) + + +if __name__ == "__main__": + main() diff --git a/openspec/changes/externalmedia-returns-a-channel-id-that-finds-its-stream/proposal.md b/openspec/changes/externalmedia-returns-a-channel-id-that-finds-its-stream/proposal.md index 80cbe50f..96fba011 100644 --- a/openspec/changes/externalmedia-returns-a-channel-id-that-finds-its-stream/proposal.md +++ b/openspec/changes/externalmedia-returns-a-channel-id-that-finds-its-stream/proposal.md @@ -10,37 +10,50 @@ decision_ref: Sdk/ADR-0060 ## Why -ADR-0060 closed the AudioSocket wire format and said, in as many words, that doing so **does not** make -the ARI AudioSocket route work end to end: the stream table is keyed by the wire UUID while its only -consumer looks the stream up by ARI channel id, and that this was tracked separately. This is that -tracking. It is also the harvest ADR-0060's own close-out owed and did not deliver. +ADR-0060 closed the AudioSocket wire format and said, in as many words, that doing so **does not** +make the ARI route work end to end: the stream table is keyed by the wire UUID while its only +consumer looks the stream up by ARI channel id, and that this was tracked separately. Nothing tracked +it. This is that tracking, and the harvest the #302 close-out owed and did not deliver. -### The route is broken in two places, and the first one is worse than the note said +Everything below is measured against a real Asterisk 22.9.0 with a dependency-free TCP listener. +The probe is `probe-externalmedia.py` beside this file and its output is `probe-capture.txt`; every +run records the full query string it sent, because an earlier version of that capture recorded +parameters in prose and a mislabelled run survived into the first draft of this proposal. -`ExternalMediaActivity` creates the channel and then polls for the stream: +### Three defects, not two -```csharp -Channel = await AriClient.Channels.CreateExternalMediaAsync( - App, ExternalHost, Format, encapsulation: Encapsulation, transport: Transport, ...); -... -_audioStream = _audioSocketServer.GetStream(Channel.Id); +**1. The default configuration creates an RTP channel and waits.** `ExternalMediaActivity` accepts an +`AudioSocketServer` in its constructor but leaves `Encapsulation` null, which Asterisk reads as +rtp/udp: + +```text +RUN H QUERY: app=probe&external_host=…&format=slin16 + HTTP 200 name=UnicastRTP/127.0.0.1:19099-0x7f6ce8002100 ``` -The note predicted that poll times out. **It never gets that far.** A probe against a real Asterisk -22.9.0 sent exactly what that call sends and got: +Nothing ever connects to the AudioSocket server, and after `ConnectionTimeout` the activity throws +`TimeoutException`. That is precisely what ADR-0060 predicted, for the shape callers actually get. + +**2. Asking for AudioSocket fails at the create call.** Setting `Encapsulation = "audiosocket"` is not +enough, because the activity sends `transport` only when a caller sets `Transport`: ```text -HTTP 400 -> {"message": "data can not be empty"} +RUN G QUERY: app=probe&external_host=…&format=slin16&encapsulation=audiosocket + HTTP 400 -> "transport must be 'tcp' for audiosocket encapsulation" ``` -`data` is **mandatory** for `encapsulation=audiosocket`. The request fails inside -`EnsureAriSuccessAsync`, before any polling. `CreateExternalMediaAsync` exposes `data` — the activity -simply never passes it — so a caller who supplies it gets past this, and then meets the second defect. +And once transport is supplied, `data` is mandatory too: + +```text +RUN D QUERY: …&encapsulation=audiosocket&transport=tcp + HTTP 400 -> "data can not be empty" +``` -### The two identifiers are different parameters, and this is measured +So no configuration of today's activity reaches a working AudioSocket stream: one path times out, +the other two throw at the create call inside `EnsureAriSuccessAsync`. -The same probe ran `POST /channels/externalMedia` with **distinct, byte-order-asymmetric** UUIDs in -`channelId` and in `data`, so the capture says both which parameter travels and in which byte order: +**3. The two identifiers are different parameters.** The probe passed distinct, byte-order-asymmetric +UUIDs in `channelId` and `data`, so the capture says both which one travels and in which byte order: ```text channelId = 0a1b2c3d-4e5f-6071-8293-a4b5c6d7e8f9 @@ -51,60 +64,63 @@ HEX : 01 00 10 f9 e8 d7 c6 b5 a4 39 28 17 06 f5 e4 d3 c2 b1 a0 UUID on the wire == channelId ? False == data ? True ``` -`data` becomes the identification UUID the server keys its stream table by. `channelId` becomes the -ARI `Channel.Id`. Passing neither — today's behaviour — leaves Asterisk minting its own uniqueid, -which no stream table can ever contain. +`data` becomes the identification UUID the server keys its table by. `channelId` becomes the ARI +`Channel.Id`. Supplying one value as both makes them equal — HTTP 200, and the lookup hits. +Reproduced across fresh UUIDs and both audio formats. -### Passing the same UUID in both closes the gap, and that is measured too +### Rejected alternative: mint the UUID and pass it only as `data` -```text -E1: HTTP 200 id=c165018a-d35b-4783-a057-4d43c1f7ba59 - HEX: 01 00 10 c1 65 01 8a d3 5b 47 83 a0 57 4d 43 c1 f7 ba 59 - UUID on the wire == channelId ? True == data ? True -``` - -Reproduced twice with fresh UUIDs. `Channel.Id` **is** the wire UUID, and `GetStream(Channel.Id)` -hits. The full capture is `probe-capture.txt` beside this file. - -### `CreateExternalMediaAsync` cannot express it - -```csharp -public async ValueTask CreateExternalMediaAsync(string app, string externalHost, string format, - string? encapsulation = null, string? transport = null, string? connectionType = null, - string? direction = null, string? data = null, CancellationToken cancellationToken = default) -``` +RUN C shows the stream is findable **today**, with no API change: the activity mints an identifier, +passes it as `data`, and polls `GetStream(thatIdentifier)`. No `CP0002`, no suppression file, no +migration guide, nothing stops compiling. -`data` is there. **`channelId` is not.** Asterisk documents it as "The unique id to assign the channel -on creation" and it is the only way to choose the ARI channel id. With this signature a caller cannot -align the two even knowing exactly how. +It is rejected because it fixes the activity and leaves the capability unfixed. A consumer holding +only an ARI channel id — a `StasisStart` or `ChannelHangupRequest` handler, which is the shape Pro's +AgentAssist uses — still cannot find the stream, because `Channel.Id` remains an Asterisk-minted +uniqueid that appears in no table. Making `Channel.Id` **be** the key is the whole point, and it is +what justifies the break priced below. This is a deliberate purchase, not an oversight. ### Why no test caught it -All four constructions of `ExternalMediaActivity` in the test suite use the one-argument overload, so -both server fields stay null and the polling block is skipped entirely. The failing branch is never -executed. That is the same shape as the wire-format defect one level up: a green suite that never -reaches the code it appears to cover. +All four constructions of `ExternalMediaActivity` in the suite use the one-argument overload, leaving +both server fields null. The polling loop does run — two tests assert on what it produces — but **the +two `GetStream` branches are never entered**. That is the same shape as the wire-format defect one +level up: a green suite that never reaches the code it appears to cover. ## What Changes -- `CreateExternalMediaAsync` gains a `channelId` parameter. Additive and optional; no existing call - site changes meaning. -- `ExternalMediaActivity` mints one UUID and passes it as **both** `channelId` and `data` when the - encapsulation is AudioSocket, so its own poll can succeed. -- A functional test originates a real `externalMedia` against a real Asterisk and asserts the stream is - found by `Channel.Id` — the assertion that would have failed before. -- The `IAudioServer.GetStream` contract says what its key is, instead of "channel ID", which is the - wording that invited the confusion. +- `CreateExternalMediaAsync` gains a `channelId` parameter on `IAriChannelsResource` and + `AriChannelsResource`. Asterisk spells it camelCase, unlike every snake_case sibling, and a typo is + silently ignored rather than rejected — so the URL test asserts the literal. +- `ExternalMediaActivity` derives the request from the server it was handed: AudioSocket encapsulation + implies `transport = "tcp"`, and one freshly minted UUID goes to both `channelId` and `data`, in + canonical lowercase form because that is what the server's table is keyed by. +- A functional test originates a real `externalMedia` against a real Asterisk and asserts the stream + is found by `Channel.Id`. +- The `IAudioServer.GetStream` contract says what its key is and in what form, instead of "channel + ID" — the wording that invited the confusion — including that `WebSocketAudioServer` keys on + something else entirely. ## Impact -- **Public API, additive.** A new optional parameter on a shipped method; `PublicAPI.Unshipped.txt` - records it. No `CP0002`/`CP0011`, so no new suppression entry. -- **Behavioural, for AudioSocket callers only.** `ExternalMediaActivity` with - `Encapsulation = "audiosocket"` goes from an HTTP 400 to a working stream. RTP callers are untouched. -- **Downstream:** Pro's AgentAssist pairs this activity with an AudioSocket server and is the reason - the route matters. +- **Public API, BREAKING.** The nine-parameter signature is shipped in two packages + (`src/Verbara.Sdk/PublicAPI.Shipped.txt:499`, `src/Verbara.Sdk.Ari/PublicAPI.Shipped.txt:449`). + Optional parameters are compile-time sugar and ApiCompat matches by full signature, so the old + member vanishes: `CP0002` in both assemblies plus `CP0006` on the interface. `src/Verbara.Sdk` has + no `CompatibilitySuppressions.xml` today and gains one, in the ADR-0055 shape — generated + deliberately, every entry read before it is kept, never left to a build-machine flag that writes a + file the repository never sees. `PublicAPI.Unshipped.txt` takes a `*REMOVED*` row plus the new row + in both packages, per ADR-0023. +- **A migration note is obligatory** (ADR-0028, for a minor carrying a break), and the CHANGELOG entry + is `### Fixed — BREAKING`. +- **Two test doubles stop compiling, loudly rather than silently.** Both NSubstitute setups enumerate + nine positional `Arg.Any` ending in a `CancellationToken`; inserting a parameter before it lands the + token in a `string?` slot. They move to named arguments. +- **External implementers of `IAriChannelsResource` break** — Pro's fakes and decorators. That is the + price of the alternative rejected above, and it is named rather than discovered. +- **Behavioural, for AudioSocket callers only.** RTP callers take an unchanged path. - **Out of scope, and stated rather than implied:** `WebSocketAudioServer` keys its table by the last - segment of the request URL, which is a second and different mismatch; and `AudioStreamMetrics` - declares ten instruments with no production call site. Both are named in `tasks.md` as follow-ups - with no fix here. + segment of the request URL, a second and different mismatch that no probe has yet measured; and + `AudioStreamMetrics` declares ten instruments with no production call site. Both are carried in + `tasks.md` with file and line, and C7 opens a change for them rather than leaving them as + checkboxes inside this one. diff --git a/openspec/changes/externalmedia-returns-a-channel-id-that-finds-its-stream/specs/external-media-stream-routing/spec.md b/openspec/changes/externalmedia-returns-a-channel-id-that-finds-its-stream/specs/external-media-stream-routing/spec.md index 1bd0f07e..2c9ed5ea 100644 --- a/openspec/changes/externalmedia-returns-a-channel-id-that-finds-its-stream/specs/external-media-stream-routing/spec.md +++ b/openspec/changes/externalmedia-returns-a-channel-id-that-finds-its-stream/specs/external-media-stream-routing/spec.md @@ -2,16 +2,18 @@ ## ADDED Requirements -### Requirement: An external media channel SHALL be findable by the identifier its creator holds -A caller that creates an external media channel and owns the audio server it points at SHALL be able -to find that channel's stream using the identifier the create call returned, without correlating two -identifier namespaces by hand. - +### Requirement: An AudioSocket external media channel SHALL be findable by the channel id it returned Where the transport identifies its connection with a caller-supplied value — AudioSocket does, in its identification frame — the SDK SHALL make the ARI channel id and that value the same value, by -supplying one identifier to both parameters that carry them. The SDK SHALL NOT leave the ARI channel -id to be minted by Asterisk while keying the stream table on a different value, because no lookup can -succeed across those two namespaces. +supplying one identifier to both parameters that carry them. + +The SDK SHALL NOT leave the ARI channel id to be minted by Asterisk while keying the stream table on +a different value, because no lookup can succeed across those two namespaces. The identifier SHALL be +in the canonical lowercase hyphenated form, which is the form the server's table is keyed by; an +identifier in any other spelling produces a successful create and a lookup that misses. + +The WebSocket transport is **not** covered by this capability until its key has been measured rather +than read from the code. #### Scenario: A stream is found by the channel id the create call returned - **GIVEN** an AudioSocket server owned by the caller and running @@ -20,54 +22,78 @@ succeed across those two namespaces. - **THEN** looking the stream up by the returned `Channel.Id` SHALL return that stream - **AND** the identification frame's UUID SHALL equal that same `Channel.Id` -#### Scenario: A create that cannot be routed does not silently become a timeout -- **GIVEN** a request for AudioSocket encapsulation that omits the identifier Asterisk requires -- **WHEN** the channel is created -- **THEN** the failure SHALL surface as the error Asterisk returned, at the create call -- **AND** SHALL NOT be reported later as the audio server having failed to connect +#### Scenario: An identifier in a non-canonical spelling does not silently miss +- **GIVEN** an identifier that is a valid UUID but not in canonical lowercase hyphenated form +- **WHEN** it is used to create an AudioSocket external media channel +- **THEN** the SDK SHALL either normalise it to the form the stream table is keyed by, or reject it +- **AND** SHALL NOT produce a created channel whose stream cannot be found + +### Requirement: A request that Asterisk cannot route SHALL fail at the create call +The SDK SHALL supply the transport that the requested encapsulation requires, rather than leaving a +combination Asterisk rejects. Where the caller has supplied an audio server whose transport +contradicts the requested encapsulation, the SDK SHALL fail before creating a channel that can never +carry a stream. + +A failure to create SHALL surface as the error Asterisk returned, at the create call, and SHALL NOT +be reported later as the audio server having failed to connect. + +#### Scenario: AudioSocket encapsulation carries its required transport +- **GIVEN** a caller that asks for AudioSocket encapsulation and specifies no transport +- **WHEN** the external media channel is created +- **THEN** the SDK SHALL send the transport AudioSocket requires +- **AND** the create SHALL NOT fail for want of a transport the caller was never asked for + +#### Scenario: An audio server that cannot be reached by the requested encapsulation is refused +- **GIVEN** an AudioSocket server supplied to an activity configured for a non-AudioSocket encapsulation +- **WHEN** the activity starts +- **THEN** it SHALL fail with an error naming the contradiction +- **AND** SHALL NOT wait out its connection timeout and report a connection failure ### Requirement: The ARI external media surface SHALL expose the parameter that names the channel The SDK's external media create method SHALL accept the ARI `channelId` parameter, which Asterisk documents as the unique id to assign the channel on creation. Without it a caller cannot choose the channel id, and therefore cannot make it match anything. -The parameter SHALL be optional and additive: an existing call site that omits it SHALL behave exactly -as before, and the change SHALL NOT alter the shipped public API surface in a breaking way. +Asterisk spells this parameter in camelCase while its siblings are snake_case, and ignores an +unrecognised parameter silently rather than rejecting it. The SDK's own test for the request SHALL +assert the literal parameter name, so a misspelling fails a test rather than becoming a silent no-op. #### Scenario: A caller chooses the channel id - **GIVEN** a caller that has generated an identifier - **WHEN** it creates an external media channel supplying that identifier as the channel id - **THEN** the returned channel's id SHALL be that identifier -#### Scenario: An existing caller is unaffected -- **GIVEN** an existing call site that supplies no channel id -- **WHEN** it creates an external media channel -- **THEN** the call SHALL behave as it did before this change - -### Requirement: A stream lookup contract SHALL say which identifier it takes -Any published contract for looking a stream up by key SHALL state what that key is and where a caller -obtains it, rather than naming it only as a channel id. Where two implementations of the same contract -key on different values, the contract SHALL say so. - -The documentation SHALL be specific enough that a reader can tell whether a value they already hold is -a valid key, without reading the implementation. +### Requirement: A stream lookup contract SHALL say which identifier it takes and in what form +Any published contract for looking a stream up by key SHALL state what that key is, in what form, and +where a caller obtains it — rather than naming it only as a channel id. Where two implementations of +the same contract key on different values, the contract SHALL say so. #### Scenario: A reader can tell what to pass - **GIVEN** the published documentation of a stream lookup - **WHEN** a reader holds an ARI channel id and wants the stream for it - **THEN** the documentation SHALL state whether that value is a valid key for that implementation +- **AND** SHALL state the form the key takes ## Architectural Risk -**Level:** LOW - -**Affected:** `Verbara.Sdk.Ari` (the ARI channels resource and the AudioSocket server's documented -contract), `Verbara.Sdk.Activities` (`ExternalMediaActivity`), and downstream `Verbara.Sdk.Pro` -AgentAssist, which pairs that activity with an AudioSocket server and is the consumer the route exists -for. - -**Mitigation:** the API change is a new optional parameter, so no shipped signature changes meaning -and `PackageValidation` reports nothing. The behavioural change reaches only callers that ask for -AudioSocket encapsulation — RTP callers take an unchanged path. The end-to-end claim is pinned by a -functional test that originates against a real Asterisk rather than a fake, because the defect being -fixed survived six months of tests that never reached the code they appeared to cover. +**Level:** MEDIUM + +**Affected:** `Verbara.Sdk` (the `IAriChannelsResource` interface — a signature change on a shipped +public interface, so external implementers break), `Verbara.Sdk.Ari` (the resource class and the +AudioSocket server's documented contract), `Verbara.Sdk.Activities` (`ExternalMediaActivity`), and +downstream `Verbara.Sdk.Pro` AgentAssist, which pairs that activity with an AudioSocket server and is +the consumer the route exists for. + +The level is MEDIUM rather than LOW because the change alters a shipped signature in two packages: +`CP0002` in both plus `CP0006` on the interface, and any external implementation of +`IAriChannelsResource` stops compiling. The smaller fix that avoids all of this — mint the identifier +and pass it only as `data` — was considered and rejected in the proposal, because it leaves a +consumer holding only an ARI channel id unable to find the stream, which is the capability itself. + +**Mitigation:** the break is declared in a committed `CompatibilitySuppressions.xml` with each entry +read before it is kept, `*REMOVED*` plus new rows in `PublicAPI.Unshipped.txt`, a migration note per +ADR-0028 and a `### Fixed — BREAKING` CHANGELOG entry. The behavioural change reaches only callers +that ask for AudioSocket encapsulation. The end-to-end claim is pinned by a functional test against a +real Asterisk with a subscribed Stasis application, and that test is negatively controlled by +reverting the fix rather than by breaking the assertion — because a mutation that only proves the +assertion is wired is what let a fixture measure nothing for six months. diff --git a/openspec/changes/externalmedia-returns-a-channel-id-that-finds-its-stream/tasks.md b/openspec/changes/externalmedia-returns-a-channel-id-that-finds-its-stream/tasks.md index bfd7f4b8..29e3b78c 100644 --- a/openspec/changes/externalmedia-returns-a-channel-id-that-finds-its-stream/tasks.md +++ b/openspec/changes/externalmedia-returns-a-channel-id-that-finds-its-stream/tasks.md @@ -7,74 +7,126 @@ failure is pasted here verbatim.** A task is not checked off until the thing it ## Phase A — foundation (batched) -- [ ] A1 Re-run `probe-capture.txt` end to end and confirm it still reproduces on this machine: - `data` on the wire, `channelId` as `Channel.Id`, HTTP 400 `data can not be empty` for the - parameters `ExternalMediaActivity` sends today, and HTTP 200 with both equal. Use a **fresh** - UUID per run — a reused `channelId` returns HTTP 500 and is not a finding. Paste the output. +- [ ] A1 Run `probe-externalmedia.py` and diff its output against `probe-capture.txt`. It prints the + full query string of every request; an earlier capture recorded parameters in prose and a run + labelled "what the SDK sends" turned out to carry an extra one nobody could see. Then run it + against **the lane's own image, for both versions**, not only the local one: + `docker build -f docker/Dockerfile.asterisk docker/ --build-arg ASTERISK_VERSION=23`. + Asterisk 23 has never been probed, and the merge queue would otherwise be the first place the + new test meets it. Paste both outputs. - [ ] A2 Read the surface that changes: `AriChannelsResource.CreateExternalMediaAsync`, - `ExternalMediaActivity`, `AudioSocketServer.GetStream`, `CompositeAudioServer.GetStream`, and the - XML docs on `IAudioServer` / `IAudioStream` in `src/Verbara.Sdk/IAriClient.cs`. Record the exact - current signatures and doc text, so the diff is against what is there rather than what is - remembered. -- [ ] A3 **Write the failing regression test first.** A unit test that constructs - `ExternalMediaActivity` with an AudioSocket server and asserts the stream is found by - `Channel.Id`. It must fail against the unfixed code, and the failure text goes in this file - verbatim — not a summary of it. Note which of the two defects it catches; if it only catches - one, say which and add the second test rather than widening the first. -- [ ] A4 Check whether any existing test would change meaning. All four current constructions of - `ExternalMediaActivity` use the one-argument overload, so the polling block never runs — confirm - that against the tree rather than trusting this sentence, and list any test that starts - executing code it did not execute before. + `IAriChannelsResource`, `ExternalMediaActivity`, `AudioSocketServer.GetStream`, + `CompositeAudioServer.GetStream`, and the XML docs on `IAudioServer` / `IAudioStream` in + `src/Verbara.Sdk/IAriClient.cs`. Record the exact current signatures and doc text, so the diff + is against what is there rather than what is remembered. +- [ ] A3 **Write the failing regression test first, as an argument-capture test.** A test that mocks + `IAriChannelsResource`, constructs `ExternalMediaActivity` with `Encapsulation = "audiosocket"` + and an `AudioSocketServer`, runs it, and asserts on the arguments the activity passed: + `data` is non-null and `transport == "tcp"`. It compiles against the **unfixed** code (it names + no new parameter) and fails on `data == null`. Paste the failure verbatim. + Do **not** write it as "assert `GetStream(Channel.Id)` hits": with a mocked resource the test + chooses both the returned `Channel.Id` and the UUID its own client sends, so it can be made to + pass today — the closed loop this whole change is about. +- [ ] A4 Enumerate every call site and every substitute of `CreateExternalMediaAsync` and say which + one B1 changes. There are four today: `ExternalMediaActivity.cs:51`, + `Tests/…/ActivityTests.cs:328`, `Tests/…/ActivityTests.cs:392`, and + `Tests/Verbara.Sdk.Ari.Tests/Resources/AriResourceTests.cs:161`. The two `ActivityTests` + substitutes enumerate nine positional `Arg.Any` ending in a `CancellationToken` and will not + compile after B1. Also confirm against the tree — do not trust this sentence — that the + **two `GetStream` branches** at `ExternalMediaActivity.cs:65-75` are the unexecuted code. The + polling `while` loop itself does run, in two tests. ## Phase B — critical components (one focused subagent each) -- [ ] B1 Add the optional `channelId` parameter to `CreateExternalMediaAsync`. Place it so no existing - positional call site changes meaning, append the row to `PublicAPI.Unshipped.txt`, and confirm - `dotnet pack` reports no `CP0002`/`CP0011` — an additive optional parameter should need no - suppression entry, and if it does, that is a finding, not a formality. -- [ ] B2 Make `ExternalMediaActivity` mint one UUID and pass it as both `channelId` and `data` when the - encapsulation is AudioSocket. Decide explicitly what happens for other encapsulations and write - the reason in the code: RTP does not carry a caller-supplied identifier, so forcing one there - would change behaviour for callers this change has no business touching. -- [ ] B3 Fix the contract text. `IAudioServer.GetStream` currently says "Get an active stream by - channel ID" and `IAudioStream.ChannelId` says "Unique ID of the external media channel in - Asterisk" — both assert an identity the code does not hold. Say what the key actually is for - each implementation, including that `WebSocketAudioServer` keys on something else entirely. +- [ ] B1 Add the `channelId` parameter to `CreateExternalMediaAsync` on **both** + `IAriChannelsResource` and `AriChannelsResource`, positioned **before** `cancellationToken` + (CT-last is the SDK's convention and CA1068 is on under `TreatWarningsAsErrors`). Then: + - the two `ActivityTests` substitutes move to **named arguments**; + - `AriResourceTests` extends its URL assertion to the **literal `channelId=`** — Asterisk + spells it camelCase among snake_case siblings and ignores an unknown parameter silently, so a + typo becomes a no-op that only the queue would catch; + - `*REMOVED*` + new rows in `PublicAPI.Unshipped.txt` for both packages (ADR-0023); + - `CompatibilitySuppressions.xml` generated deliberately and **read entry by entry before it is + kept** — expect `CP0002` in both packages and `CP0006` on the interface, and `src/Verbara.Sdk` + has no such file today. ADR-0055 records what happens when the flag is left to the build + machine: validation runs, reports green, and compares nothing. + - a migration note (ADR-0028 requires one for a minor carrying a break). + Confirm with the exact command CI runs — `dotnet pack` — not with a clean build. A green build + says nothing about `PackageValidation`; that mistake cost #302 a CI failure. +- [ ] B2 `ExternalMediaActivity` derives its request from the server it was handed: + `encapsulation = Encapsulation ?? (_audioSocketServer is not null ? "audiosocket" : null)`; when + the encapsulation is AudioSocket (compare ordinal-ignore-case — Asterisk uses `strcasecmp`) then + `transport = Transport ?? "tcp"` and one `Guid.NewGuid().ToString()` goes to **both** `channelId` + and `data`. Canonical lowercase is not incidental: `AudioSocketSession.ParseUuid` keys the table + with `new Guid(bytes, bigEndian: true).ToString()` into an ordinal comparer, so any other + spelling creates a channel whose stream cannot be found. Write both reasons in the code. + Decide and implement what happens when an `AudioSocketServer` is supplied with a non-AudioSocket + encapsulation: today that combination creates an RTP channel and waits out a 30-second timeout, + which is the shape the delta spec forbids. A unit test pins whichever behaviour is chosen, plus + one negative control using a non-canonical identifier spelling. +- [ ] B3 Fix the contract text. `IAudioServer.GetStream` says "Get an active stream by channel ID" and + `IAudioStream.ChannelId` says "Unique ID of the external media channel in Asterisk" — both + assert an identity the code does not hold. Say for each implementation what the key is and in + what form: for `AudioSocketServer`, the UUID Asterisk sent in its identification frame in + canonical lowercase hyphenated form, which for the ARI route is the value the creator supplied + as `channelId` and `data`. Say in `ExternalMediaActivity`'s remarks that the WebSocket branch is + **not** routed — `WebSocketAudioServer` keys on the last segment of the request URL (F1). ## Phase C — integration (batched) -- [ ] C1 Functional test: originate a real `externalMedia` against the real Asterisk container and - assert the stream is found by `Channel.Id`. Put it beside `AudioSocketWireFunctionalTests`. It - must run in the `merge_group` lane — a pull request short-circuits that job and reports `pass` - in 16 seconds without starting Asterisk, which is how a broken dialplan reached the queue in - #302. -- [ ] C2 **Mutation-check the new functional test rather than trusting it.** Break the expected value - and confirm it fails. A fixture that passes while measuring nothing is the exact failure this - repository has now shipped twice. -- [ ] C3 Pick an extension number for any new dialplan entry that no test already dials. - `[test-functional]` is dialed at **160, 300, 700, 999 and 9998** and defines none of them; 710 - and 711 are this change's neighbours and are taken. -- [ ] C4 `CHANGELOG.md [Unreleased]`: a `### Fixed` entry. Give it its **own insertion anchor** - distinct from any other in-flight entry — a shared anchor ejected #300 from the merge queue with - fourteen checks green. Leave `(#N)` for close-out. -- [ ] C5 Coverage measured **after committing**, never before. Read the changed-line count and check it - against the size of the diff before believing the percentage: a sibling change accepted - `100% (6/6)` on a five-file diff and the `6` was the tell. -- [ ] C6 `openspec validate --all --strict` green, full unit lane green, Governance green, and CI green - on the PR — including the `merge_group` build, which is the only place the functional suite - actually runs. +- [ ] C1 Functional test beside `AudioSocketWireFunctionalTests`. Three things it must get right: + - **Subscribe a Stasis application first.** `externalMedia` validates that `app` is non-empty + and never checks it is registered, so the create returns 200 against a bare listener — but the + channel then runs Stasis with no subscriber, Asterisk hangs it up, and the entry is removed + within milliseconds of a 200 ms poll. A test written from the probe alone times out and looks + like the fix not working. + - Construct the activity with `Encapsulation = "audiosocket"` and **nothing else**, so the test + measures the class's own path rather than a configuration the test supplied. + - A fixed port that is **not** 19092 or 19093 — those belong to extensions 710 and 711. + Assert with a reason on every path; never `return` early. Assert + `activity.AudioStream!.ChannelId == activity.Channel!.Id`. +- [ ] C2 **Negatively control the functional test by reverting the fix, not by breaking the + assertion.** Breaking the expected value proves the assertion is wired; it does not prove the + test can see the defect. Three reversions, each failure pasted: drop `data`; drop the `tcp` + transport; pass `channelId` and `data` different values. ADR-0060's own control was a revert. +- [ ] C3 Apply the **`ci:functional`** label to the PR. `docker/Dockerfile.asterisk` work and the + functional suite are skipped on `pull_request` unless that label is present (ADR-0051, + `.github/workflows/ci.yml`), and the matrix is `[23]` on a PR against `[22, 23]` in the queue. + Without the label the job reports `pass` in about sixteen seconds having started no Asterisk — + which is exactly how a broken dialplan reached the merge queue in #302. Record the PR-time + result, not only the queue's. +- [ ] C4 `CHANGELOG.md [Unreleased]`: a `### Fixed — BREAKING` entry. Give it its **own insertion + anchor** distinct from any other in-flight entry — a shared anchor ejected #300 from the merge + queue with fourteen checks green. Leave `(#N)` for close-out. +- [ ] C5 Coverage measured **after committing**, never before. The unit lane excludes + `Category=Functional`, so C1 contributes **zero** patch coverage against the 85% floor: A3 and + B2's unit tests are what must carry `ExternalMediaActivity`'s new lines. Read the changed-line + count and check it against the size of the diff before believing the percentage — a sibling + change accepted `100% (6/6)` on a five-file diff and the `6` was the tell. +- [ ] C6 `openspec validate --all --strict` green, full unit lane green, Governance green, `dotnet + pack` clean, and CI green on the PR **including the `merge_group` build** — the only place the + functional suite runs both Asterisk versions. +- [ ] C7 Open a change for the follow-ups below and write its link back into this file. They are + carried as prose, not as unchecked boxes: `openspec/config.yaml` requires every deferred finding + to be harvested into an open change or an ADR addendum before archiving, and three `- [ ]` boxes + inside this change would defer that harvest to close-out — the exact failure ADR-0060 committed + and this change exists to stop repeating. -## Follow-ups this change does NOT fix, recorded so they are not lost again +## Follow-ups this change does NOT fix -ADR-0060 wrote "tracked separately" with no link, and the close-out archived it anyway. That is what -these lines exist to prevent repeating. +ADR-0060 wrote "tracked separately" with no link, and the close-out archived it anyway. C7 opens the +change that carries these; the lines below are the evidence it starts from. -- [ ] F1 `WebSocketAudioServer` keys `_streams` by the **last segment of the request URL** - (`WebSocketAudioServer.cs:328`), and the SDK's own example puts a literal `/audio` there — so - every concurrent call would register under the key `"audio"`. `CompositeAudioServer` hands the - same string to both servers, which do not agree on what it means. Needs its own change. -- [ ] F2 `AudioStreamMetrics` declares ten instruments with zero production call sites. -- [ ] F3 `[test-functional]` is dialed at 160, 300, 700, 999 and 9998 and defines none of them. - `ConfBridgeAdvancedTests` alone dials the undefined 700 from ten call sites and passes by taking - its `if (confJoin is null) return;` branch without asserting anything. The shape to hunt is that - early return, not the extension numbers. +- **F1 — `WebSocketAudioServer` keys on a URL path segment.** The key is computed at + `WebSocketAudioServer.cs:337` (`path.TrimStart('/').Split('/').LastOrDefault()`) and registered at + `:262`. The SDK's own example puts a literal `/audio` there + (`Examples/WebSocketMediaExample/Program.cs:10`), so every concurrent call would register under the + key `"audio"`. `CompositeAudioServer` hands the same string to both servers, which do not agree on + what it means. **No probe has measured what `externalMedia` with `transport=websocket` puts in the + request path**, which is why this stays out of the present change rather than being designed from a + reading of the code. +- **F2 — `AudioStreamMetrics`** declares ten instruments with zero production call sites. +- **F3 — five dialplan extensions are dialed and never defined.** `[test-functional]` is dialed at + 160, 300, 700, 999 and 9998 and defines none of them. `ConfBridgeAdvancedTests` alone dials the + undefined 700 from ten call sites and passes by taking its `if (confJoin is null) return;` branch + without asserting anything. The shape to hunt is that early return, not the extension numbers. From 0a98fc1c3cb45eb7bafc42c40eb560278f63cfc7 Mon Sep 17 00:00:00 2001 From: "Harol A. Reina H." Date: Thu, 24 Sep 2026 06:01:12 -0500 Subject: [PATCH 3/6] test(activities): the failing regression test, first, against the unfixed 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 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. --- .../Activities/ActivityTests.cs | 90 +++++++++++ .../probe-capture.txt | 60 ++++++- .../tasks.md | 153 +++++++++++++++++- 3 files changed, 297 insertions(+), 6 deletions(-) diff --git a/Tests/Verbara.Sdk.Activities.Tests/Activities/ActivityTests.cs b/Tests/Verbara.Sdk.Activities.Tests/Activities/ActivityTests.cs index 2acc89ff..d17d6095 100644 --- a/Tests/Verbara.Sdk.Activities.Tests/Activities/ActivityTests.cs +++ b/Tests/Verbara.Sdk.Activities.Tests/Activities/ActivityTests.cs @@ -1,7 +1,9 @@ using Verbara.Sdk; using Verbara.Sdk.Activities.Activities; using Verbara.Sdk.Activities.Models; +using Verbara.Sdk.Ari.Audio; using FluentAssertions; +using Microsoft.Extensions.Logging.Abstractions; using NSubstitute; namespace Verbara.Sdk.Activities.Tests.Activities; @@ -424,6 +426,94 @@ public async Task ExternalMediaActivity_CancelAsync_ShouldHangupChannel() await channelsResource.Received(1).HangupAsync("ext-ch-1", Arg.Any()); } + [Fact] + public async Task StartAsync_ShouldSendDataAndTcpTransport_WhenEncapsulationIsAudioSocket() + { + // ARGUMENT-CAPTURE, deliberately. The tempting test — "GetStream(Channel.Id) returns a + // stream" — cannot fail here: with a substituted resource the test picks BOTH the Channel.Id + // handed back AND the uuid its own fake client would send, so it can be made green against + // the unfixed activity. That closed loop is the defect this change exists to correct. + // What a real Asterisk rejects is the REQUEST, before any stream exists: + // RUN G (probe-capture.txt) — encapsulation=audiosocket, no transport + // -> HTTP 400 "transport must be 'tcp' for audiosocket encapsulation" + // RUN D (probe-capture.txt) — encapsulation=audiosocket&transport=tcp, no data + // -> HTTP 400 "data can not be empty" + // So the assertion is on the arguments that leave the activity. + var ariClient = Substitute.For(); + var channelsResource = Substitute.For(); + ariClient.Channels.Returns(channelsResource); + + string? sentEncapsulation = null; + string? sentTransport = null; + string? sentData = null; + +#pragma warning disable CA2012 + channelsResource.CreateExternalMediaAsync( + Arg.Any(), Arg.Any(), Arg.Any(), + encapsulation: Arg.Any(), transport: Arg.Any(), + connectionType: Arg.Any(), direction: Arg.Any(), + data: Arg.Any(), + // NAMED, and the token especially. The fix inserts channelId between data and the token, + // so a ninth POSITIONAL Arg.Any() would bind to a string? parameter + // and this file would stop compiling — CS1503, the same trap the two substitutes above + // carry. Naming the token survives the insertion; naming channelId is impossible here, + // because it does not exist on the signature under test. + cancellationToken: Arg.Any()) + // ForAnyArgs, not Returns: the fix adds a channelId parameter to this method, and an + // argument-by-argument match would leave it compared against its default null — the + // setup would stop matching the moment the activity starts sending one, and the test + // would fail on a null channel rather than on its own assertions. + .ReturnsForAnyArgs(callInfo => + { + // Positional, because this test may not NAME channelId: that parameter does not + // exist on the signature under test, and naming it is CS1739 — a compile error, not + // a failing assertion. The captured positions are stable across the fix, which + // appends channelId after data and before the CancellationToken: + // 0 app, 1 externalHost, 2 format, 3 encapsulation, 4 transport, + // 5 connectionType, 6 direction, 7 data, (8 channelId), last CancellationToken. + sentEncapsulation = callInfo.ArgAt(3); + sentTransport = callInfo.ArgAt(4); + sentData = callInfo.ArgAt(7); + return new ValueTask(new AriChannel { Id = "ext-ch-audiosocket" }); + }); +#pragma warning restore CA2012 + + // Never started: the activity only reads GetStream off it, and an unbound server needs no + // port. Nothing will ever connect, so the run ends in the timeout below — that is how the + // run ends, not what this test measures. + await using var audioSocketServer = new AudioSocketServer( + new AudioServerOptions { AudioSocketPort = 0, ListenAddress = "127.0.0.1" }, + NullLogger.Instance); + + var activity = new ExternalMediaActivity(ariClient, audioSocketServer) + { + App = "test", + ExternalHost = "127.0.0.1:19099", + Encapsulation = "audiosocket", + ConnectionTimeout = TimeSpan.FromMilliseconds(50) + }; + + var start = () => activity.StartAsync().AsTask(); + await start.Should().ThrowAsync( + "nothing connects to the audio server in this test, so the activity times out after " + + "ConnectionTimeout — the create call it made on the way there is what is asserted below"); + + // Positive control, and it is load-bearing. Everything below asserts that a captured value is + // null, and a capture that never ran reads null too — so without one captured value that MUST + // be non-null today, a broken offset or an unmatched substitute would go red in exactly the + // shape of the defect and measure nothing. This repository has shipped that twice. + sentEncapsulation.Should().Be("audiosocket", + "the capture must be reading the real argument array: this is the one value the unfixed " + + "activity already sends, so if it does not arrive the nulls below prove nothing"); + + sentData.Should().NotBeNull( + "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"); + sentTransport.Should().Be("tcp", + "audiosocket encapsulation on any other transport is HTTP 400 \"transport must be 'tcp' " + + "for audiosocket encapsulation\" (probe-capture.txt RUN G)"); + } + [Fact] public async Task CancelAsync_ShouldNotThrow_WhenActivityAlreadyCompleted() { diff --git a/openspec/changes/externalmedia-returns-a-channel-id-that-finds-its-stream/probe-capture.txt b/openspec/changes/externalmedia-returns-a-channel-id-that-finds-its-stream/probe-capture.txt index 07c08a3c..01d686ac 100644 --- a/openspec/changes/externalmedia-returns-a-channel-id-that-finds-its-stream/probe-capture.txt +++ b/openspec/changes/externalmedia-returns-a-channel-id-that-finds-its-stream/probe-capture.txt @@ -75,6 +75,50 @@ HTTP 500 means NOTHING IS LISTENING on external_host. It is not about the format channel id. chan_audiosocket connects during the create call, so a create against a dead port fails the request rather than producing a channel that never connects. +-------------------------------------------------------------------------------- +PART 4 — the CI lane's own image, Asterisk 22 AND 23 (task A1, 2026-09-24) +-------------------------------------------------------------------------------- + +Images built exactly as .github/workflows/ci.yml:387 builds them: + + docker build -f docker/Dockerfile.asterisk docker/ \ + --build-arg ASTERISK_VERSION=<22|23> --build-arg CODEC_OPUS_VERSION=<22|23>.0_1.3.0 \ + -t verbara-probe:<22|23> + + verbara/asterisk-local:22 -> /ari/asterisk/info system.version = 22.9.0 + verbara-probe:22 -> 22.9.0 (andrius/asterisk:22, build date 2026-05-04) + verbara-probe:23 -> 23.4.1 (andrius/asterisk:23, build date 2026-09-23) + +Asterisk 23 had never been probed. It behaves IDENTICALLY to 22 on every run in this file: +same HTTP status, same error strings verbatim, same parameter on the wire, same byte order, +same 500 on a dead target port. Three full passes (local 22, lane 22, lane 23 twice) agree. + + RUN G 400 "transport must be 'tcp' for audiosocket encapsulation" 22 == 23 + RUN H 200 UnicastRTP/... 22 == 23 + RUN D 400 "data can not be empty" 22 == 23 + RUN A 200 id == channelId, wire UUID == data 22 == 23 + RUN C 200 id == Asterisk uniqueid, wire UUID == data 22 == 23 + PART 3 200 / 500 crossed over format x listener 22 == 23 + +Two runs beyond what the shipped probe does, both measured on 22 and on 23 with identical results: + + RUN H+ RUN H with the TCP listener UP for 4 s. Nothing connects — the sentence in PART 1 was + an inference from the channel name until now, because the shipped probe runs H with the + listener down. + -> HTTP 200 name=UnicastRTP/127.0.0.1:19099-0x7f0720006350 + HEX: (nothing connected) + + RUN U channelId == data == an UPPERCASE canonical UUID. Asterisk ACCEPTS it: HTTP 200, and + Channel.Id comes back in the spelling that was sent. The wire carries the same 16 bytes, + which AudioSocketSession.ParseUuid renders with Guid.ToString() — canonical LOWERCASE — + into an ordinal ConcurrentDictionary. So a non-canonical spelling creates a + channel whose stream cannot be found, silently, exactly as the delta spec forbids: + QUERY: ...&channelId=EC30994A-B0E1-4EBE-99BD-19CA35669F7A&data=EC30994A-...-19CA35669F7A + -> HTTP 200 id=EC30994A-B0E1-4EBE-99BD-19CA35669F7A + HEX: 01 00 10 ec 30 99 4a b0 e1 4e be 99 bd 19 ca 35 66 9f 7a + wire = ec30994a-b0e1-4ebe-99bd-19ca35669f7a == sent ? False == sent.lower() ? True + This is the measurement behind B2's "canonical lowercase is not incidental". + -------------------------------------------------------------------------------- CORRECTIONS to the first version of this file -------------------------------------------------------------------------------- @@ -93,11 +137,23 @@ CORRECTIONS to the first version of this file predicted 501 "encapsulation and/or transport is not supported" for RUN G; 22.9.0 returns 400 with a specific message instead. Measured beats read, again. +4. Correction #2 above is itself incomplete. "HTTP 500 means NOTHING IS LISTENING on external_host" + names one cause, not the cause. With the listener UP, a `data` value that is not a parseable + UUID returns the same 500 and nothing ever connects (measured on 22 and on 23): + channelId CANONICAL, data = {eba9adc2-...} -> HTTP 500, nothing connected + channelId CANONICAL, data = not-a-uuid-at-all -> HTTP 500, nothing connected + channelId = {eef6aae9-...}, data CANONICAL -> HTTP 200, id={eef6aae9-...}, wire == data + So: `channelId` is a free-form ARI id and is echoed back verbatim — braces and all — while + `data` must parse as a UUID. A 500 from externalMedia means "chan_audiosocket could not set the + connection up", which covers a dead port AND a malformed `data`. Saying it means a dead port + sends the next reader away from the second cause, which is the same mistake correction #2 was + written to fix. + -------------------------------------------------------------------------------- NOT YET MEASURED -------------------------------------------------------------------------------- -- Asterisk 23. This probe ran only against 22.9.0 on verbara/asterisk-local:22. The CI lane builds - its own image from docker/Dockerfile.asterisk for 22 and 23; task A1 runs the probe against both. +- Asterisk 23 is now measured — see PART 4. What remains unmeasured on 23 is everything this probe + does not touch, not the runs above. - The WebSocket transport. Nothing here measures what externalMedia with transport=websocket puts in the request path, which is why F1 stays out of this change rather than being designed from a reading of the code. diff --git a/openspec/changes/externalmedia-returns-a-channel-id-that-finds-its-stream/tasks.md b/openspec/changes/externalmedia-returns-a-channel-id-that-finds-its-stream/tasks.md index 29e3b78c..479d8520 100644 --- a/openspec/changes/externalmedia-returns-a-channel-id-that-finds-its-stream/tasks.md +++ b/openspec/changes/externalmedia-returns-a-channel-id-that-finds-its-stream/tasks.md @@ -7,19 +7,84 @@ failure is pasted here verbatim.** A task is not checked off until the thing it ## Phase A — foundation (batched) -- [ ] A1 Run `probe-externalmedia.py` and diff its output against `probe-capture.txt`. It prints the +- [x] A1 Run `probe-externalmedia.py` and diff its output against `probe-capture.txt`. It prints the full query string of every request; an earlier capture recorded parameters in prose and a run labelled "what the SDK sends" turned out to carry an extra one nobody could see. Then run it against **the lane's own image, for both versions**, not only the local one: `docker build -f docker/Dockerfile.asterisk docker/ --build-arg ASTERISK_VERSION=23`. Asterisk 23 has never been probed, and the merge queue would otherwise be the first place the new test meets it. Paste both outputs. -- [ ] A2 Read the surface that changes: `AriChannelsResource.CreateExternalMediaAsync`, + + **Done 2026-09-24.** Three builds probed: `verbara/asterisk-local:22` (22.9.0), the lane's own + `verbara-probe:22` (22.9.0) and the lane's own `verbara-probe:23` (**23.4.1, never probed + before**). Images built exactly as `ci.yml:387` builds them, `CODEC_OPUS_VERSION` included — + omitting it would have built a 23 image carrying the 22 opus argument. + + **22 and 23 behave identically on every run**: same status, same error strings verbatim, same + parameter on the wire, same byte order, same 500 on a dead port. There is no version difference + to carry into C1. + + Three things the earlier capture asserted without measuring, now measured on both versions: + + - **RUN H's "no AudioSocket connection is ever made"** was inferred from the channel name, + because the shipped probe runs H with the listener down. Run with the listener up: HTTP 200, + UnicastRTP, nothing connects. The sentence was right and had not been earned. + - **The non-canonical identifier miss is real.** An UPPERCASE UUID returns HTTP 200 with + `Channel.Id` in the spelling sent, while the wire bytes render through `ParseUuid` as + canonical lowercase into an ordinal dictionary. B2's negative control has a fixture now + instead of an argument. + - **Correction #2 of the capture was itself incomplete — the third time this file has been + wrong.** "HTTP 500 means nothing is listening" names one cause. A malformed `data` returns the + same 500 with the listener **up**: `channelId` is free-form and echoed verbatim, `data` must + parse as a UUID. Recorded as correction #4. + + **This bears on C2.** Its third reversion — "pass `channelId` and `data` different values" — + must use two **well-formed** UUIDs, or the reversion fails at the create with a 500 instead of + at the lookup, and proves nothing. + + Stated plainly rather than dressed up: `probe-capture.txt` is **not** reproducible verbatim by + `probe-externalmedia.py` and never could be. The capture is a narrative of an earlier hand-run + with fixed UUIDs; the script mints uuid4 per run. A1's verdict is claim-by-claim, not a text + diff. +- [x] A2 Read the surface that changes: `AriChannelsResource.CreateExternalMediaAsync`, `IAriChannelsResource`, `ExternalMediaActivity`, `AudioSocketServer.GetStream`, `CompositeAudioServer.GetStream`, and the XML docs on `IAudioServer` / `IAudioStream` in `src/Verbara.Sdk/IAriClient.cs`. Record the exact current signatures and doc text, so the diff is against what is there rather than what is remembered. -- [ ] A3 **Write the failing regression test first, as an argument-capture test.** A test that mocks + + **Done 2026-09-24.** Full transcription with file:line is in the workflow record; the decisions + B1 depends on are below, each measured rather than reasoned. + + **RS0016 is globally suppressed, so the PublicAPI tracker does NOT catch a new public API here.** + `Directory.Build.props:15` carries `$(NoWarn);CS1591;RS0016;RS0037;RS0041`, and + that project-level NoWarn defeats `.editorconfig:71`. Measured: a brand-new public type with no + tracker row built clean, `0 Warning(s)`. What **is** enforced is RS0017 — a Shipped row whose + symbol no longer exists — which fires as an error. + + So for B1 the `*REMOVED*` row is mechanically forced and **the new row is not**. Forgetting the + new row leaves a green build. B1 cannot lean on the compiler for that half. + + **The ApiCompat prediction is now measured**, with the change applied and `dotnet pack -c + Release`: `src/Verbara.Sdk` → CP0002 + CP0006; `src/Verbara.Sdk.Ari` → CP0002 only. Exactly what + the proposal predicted. + + **CA1068 applies to internal methods too**, measured — any helper B1 or B2 adds puts the token + last as well. + + **An in-tree precedent argues the other way on position.** `IAriClient.cs:141` declares + `CreateWithoutDialAsync(string endpoint, string app, string? channelId = null, ...)` — `channelId` + as the **first** optional parameter. Appending after `data` is still the recommendation, but B1 + makes that call knowingly rather than discovering the inconsistency later. + + **A trap that "move to named arguments" does not solve by itself:** if a substitute moves to + named arguments but omits `channelId:`, the compiler fills in `null` and NSubstitute + equality-matches it. After B2 the activity sends a real UUID, the setup stops matching, the call + returns `default(ValueTask)`, and the test dies on a null `Channel` rather than on an + argument mismatch. Both setups need an explicit `channelId: Arg.Any()`. + + **`ExternalMediaActivity` has one constructor, not overloads** — `tasks.md` A2 asked for the + plural and there is exactly one, with two optional parameters, at `:41`. +- [x] A3 **Write the failing regression test first, as an argument-capture test.** A test that mocks `IAriChannelsResource`, constructs `ExternalMediaActivity` with `Encapsulation = "audiosocket"` and an `AudioSocketServer`, runs it, and asserts on the arguments the activity passed: `data` is non-null and `transport == "tcp"`. It compiles against the **unfixed** code (it names @@ -27,7 +92,46 @@ failure is pasted here verbatim.** A task is not checked off until the thing it Do **not** write it as "assert `GetStream(Channel.Id)` hits": with a mocked resource the test chooses both the returned `Channel.Id` and the UUID its own client sends, so it can be made to pass today — the closed loop this whole change is about. -- [ ] A4 Enumerate every call site and every substitute of `CreateExternalMediaAsync` and say which + + **Done 2026-09-24.** Written as an argument-capture test, in + `Tests/Verbara.Sdk.Activities.Tests/Activities/ActivityTests.cs`, named + `StartAsync_ShouldSendDataAndTcpTransport_WhenEncapsulationIsAudioSocket`. + + **The failure, verbatim, against unfixed code:** + + ```text + Failed Verbara.Sdk.Activities.Tests.Activities.ActivityTests. + StartAsync_ShouldSendDataAndTcpTransport_WhenEncapsulationIsAudioSocket [117 ms] + Error Message: + Expected sentData not to be 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. + Total tests: 1 + Failed: 1 + ``` + + It builds clean and fails on its assertion, not on a compile error. + + **Three things the task brief did not anticipate, all measured:** + + - **The prescribed shape would not have compiled after B1.** The natural NSubstitute setup ends + in a ninth **positional** `Arg.Any()`, which after B1 binds to + `string? channelId` — CS1503. The same trap A4 names for the two existing substitutes applies + to the new test. Fixed by naming `cancellationToken:`, the one name that exists both before + and after the fix. + - **`.Returns(...)` would have broken it silently rather than loudly**, for the reason A2 + records. `ReturnsForAnyArgs` is required, not stylistic. + - **The assertion on `transport` is not proven by the red run.** Both `data` and `transport` are + null today and FluentAssertions stops at the first, so `transport` is only exercised by the + forward control (simulated fix → green). + + **A near-miss of this change's own failure family, recorded because it nearly shipped.** + Restoring a scratch file with `mv` preserved its old mtime, MSBuild judged the source older than + its output, skipped the rebuild, and the run reported `Passed! 49/49` **from the fixed + assembly** while the tree held unfixed code. It was caught only because 49/49 contradicted a + filtered run. `touch` plus a re-run gave the true `Failed: 1, Passed: 48`. A green number from a + stale build is indistinguishable from a green number from a correct one. +- [x] A4 Enumerate every call site and every substitute of `CreateExternalMediaAsync` and say which one B1 changes. There are four today: `ExternalMediaActivity.cs:51`, `Tests/…/ActivityTests.cs:328`, `Tests/…/ActivityTests.cs:392`, and `Tests/Verbara.Sdk.Ari.Tests/Resources/AriResourceTests.cs:161`. The two `ActivityTests` @@ -36,6 +140,47 @@ failure is pasted here verbatim.** A task is not checked off until the thing it **two `GetStream` branches** at `ExternalMediaActivity.cs:65-75` are the unexecuted code. The polling `while` loop itself does run, in two tests. + + **Done 2026-09-24.** Call sites, and three findings that change B1. + + **The proposal's unexecuted-lines claim is right in substance and wrong in its range.** Lines 65 + and 71 — the `if` conditions — are HITS=2 at 50% branch coverage: evaluated every iteration, + always false. Only **67, 68, 73 and 74** are HITS=0. + + **And the unexecuted set is materially larger than either document says:** lines 80-82 (the + second `TimeoutException` throw, 0/2 branches — the loop is never left through its own condition + because `Task.Delay(200, token)` always throws first), line 87 (`ExecuteAsync` has never + returned normally), line 95, and 100-104 (`DisposeAsync` in its entirety). If B2 restructures + the loop, these become patch lines under C5's 85% floor. + + **B1 produces about thirty compiler errors, not three.** Once CS1503 kills overload resolution + the NSubstitute analyzer stops seeing an interface member and fires NS1004 on every `Arg.Any` in + the failed call — nine per setup, promoted to errors by `TreatWarningsAsErrors`. Measured: 27 + unique NS1004 plus 3 unique CS1503. The 27 that dominate the log are false and point at the + wrong diagnosis; all twenty-seven vanish when the three real ones are fixed. + + **A3's own new substitute is the third CS1503 site**, at working-copy `ActivityTests.cs:453`, + and it carries a comment asserting the opposite — that the captured positions are stable across + the fix. They are not. + + **`AriResourceTests.cs:161` keeps compiling and keeps passing after B1 while asserting nothing + about `channelId`.** Worse: across all 477 Ari unit tests, `AriChannelsResource.cs:90` — the + `data=` appender — sits at 50% branch coverage and **has never executed**, along with + `connection_type` and `direction`. B1 adds `channelId` in exactly that shape, so asserting the + literal `channelId=` is worthless unless the same test actually passes one. + + **The session's default `grep` honours `.gitignore`.** `docs/plans/` is gitignored but its files + were force-added and are tracked, so grep returned 14 hits where `git grep` returns 17, silently + omitting three. **Any "I found every call site" claim made with the default grep in this repo is + unsound** — use `git grep`. + + **`Examples/ContactCenterSupervisionExample/Program.cs:78`** is a fifth + `Substitute.For()`. It does not configure `CreateExternalMediaAsync`, so it + keeps compiling — but example projects are in the build. + + **Not verified, and stated rather than assumed:** the proposal's claim that Pro's fakes and + decorators break. Pro lives outside this worktree and this task may not leave it. Within the + worktree `AriChannelsResource` is the only implementer, so nothing here breaks on CS0535. ## Phase B — critical components (one focused subagent each) - [ ] B1 Add the `channelId` parameter to `CreateExternalMediaAsync` on **both** From 2bba204c1568011ec57d844475f025139403520d Mon Sep 17 00:00:00 2001 From: "Harol A. Reina H." Date: Thu, 24 Sep 2026 07:02:14 -0500 Subject: [PATCH 4/6] fix(ari)!: externalMedia hands back a channel id that finds its stream MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .../Activities/ActivityTests.cs | 262 ++++++++++++++++-- .../Audio/AudioSocketServerTests.cs | 36 +++ .../Resources/AriResourceTests.cs | 20 +- docs/guides/README.md | 1 + .../externalmedia-channel-id-migration.md | 166 +++++++++++ .../tasks.md | 143 +++++++++- .../Activities/ExternalMediaActivity.cs | 127 ++++++++- .../Audio/AudioSocketServer.cs | 8 +- .../Audio/CompositeAudioServer.cs | 20 ++ .../Audio/WebSocketAudioServer.cs | 6 +- .../CompatibilitySuppressions.xml | 37 ++- src/Verbara.Sdk.Ari/PublicAPI.Unshipped.txt | 2 + .../Resources/AriChannelsResource.cs | 8 +- src/Verbara.Sdk/CompatibilitySuppressions.xml | 54 ++++ src/Verbara.Sdk/IAriClient.cs | 75 ++++- src/Verbara.Sdk/PublicAPI.Unshipped.txt | 2 + 16 files changed, 929 insertions(+), 38 deletions(-) create mode 100644 docs/guides/externalmedia-channel-id-migration.md create mode 100644 src/Verbara.Sdk/CompatibilitySuppressions.xml diff --git a/Tests/Verbara.Sdk.Activities.Tests/Activities/ActivityTests.cs b/Tests/Verbara.Sdk.Activities.Tests/Activities/ActivityTests.cs index d17d6095..401dc05e 100644 --- a/Tests/Verbara.Sdk.Activities.Tests/Activities/ActivityTests.cs +++ b/Tests/Verbara.Sdk.Activities.Tests/Activities/ActivityTests.cs @@ -327,10 +327,17 @@ public async Task AriActivityBase_ShouldThrow_WhenStartedTwice() var channelsResource = Substitute.For(); ariClient.Channels.Returns(channelsResource); #pragma warning disable CA2012 + // NAMED, including channelId. A positional list ending in Arg.Any() would + // bind the token to the string? channelId slot (CS1503), and leaving channelId out of a named + // list is worse: the compiler supplies its default null, NSubstitute equality-matches that + // null, and the setup silently stops matching the moment the activity sends a real id — the + // test would then die on a null Channel rather than on an argument mismatch. channelsResource.CreateExternalMediaAsync( - Arg.Any(), Arg.Any(), Arg.Any(), - Arg.Any(), Arg.Any(), Arg.Any(), - Arg.Any(), Arg.Any(), Arg.Any()) + app: Arg.Any(), externalHost: Arg.Any(), format: Arg.Any(), + encapsulation: Arg.Any(), transport: Arg.Any(), + connectionType: Arg.Any(), direction: Arg.Any(), + data: Arg.Any(), channelId: Arg.Any(), + cancellationToken: Arg.Any()) .Returns(new ValueTask(new AriChannel { Id = "ch-1" })); #pragma warning restore CA2012 @@ -391,10 +398,15 @@ public async Task ExternalMediaActivity_CancelAsync_ShouldHangupChannel() ariClient.Channels.Returns(channelsResource); #pragma warning disable CA2012 + // NAMED, including channelId — see the note on the substitute above: omitting channelId from + // a named list leaves it equality-matched against null, which stops matching as soon as the + // activity supplies one. channelsResource.CreateExternalMediaAsync( - Arg.Any(), Arg.Any(), Arg.Any(), - Arg.Any(), Arg.Any(), Arg.Any(), - Arg.Any(), Arg.Any(), Arg.Any()) + app: Arg.Any(), externalHost: Arg.Any(), format: Arg.Any(), + encapsulation: Arg.Any(), transport: Arg.Any(), + connectionType: Arg.Any(), direction: Arg.Any(), + data: Arg.Any(), channelId: Arg.Any(), + cancellationToken: Arg.Any()) .Returns(callInfo => { // Return a channel, then simulate the audio server never connecting @@ -453,11 +465,13 @@ public async Task StartAsync_ShouldSendDataAndTcpTransport_WhenEncapsulationIsAu encapsulation: Arg.Any(), transport: Arg.Any(), connectionType: Arg.Any(), direction: Arg.Any(), data: Arg.Any(), - // NAMED, and the token especially. The fix inserts channelId between data and the token, - // so a ninth POSITIONAL Arg.Any() would bind to a string? parameter - // and this file would stop compiling — CS1503, the same trap the two substitutes above - // carry. Naming the token survives the insertion; naming channelId is impossible here, - // because it does not exist on the signature under test. + // NAMED, and the token especially. B1 inserted channelId between data and the token, so a + // ninth POSITIONAL Arg.Any() would bind to a string? parameter and this + // file would stop compiling — CS1503, the trap the two substitutes above carried. Naming + // the token survived that insertion. channelId is named here too now that it exists: left + // out of a named list the compiler supplies null, NSubstitute equality-matches it, and the + // setup would stop matching the moment B2 makes the activity send a real uuid. + channelId: Arg.Any(), cancellationToken: Arg.Any()) // ForAnyArgs, not Returns: the fix adds a channelId parameter to this method, and an // argument-by-argument match would leave it compared against its default null — the @@ -465,12 +479,13 @@ public async Task StartAsync_ShouldSendDataAndTcpTransport_WhenEncapsulationIsAu // would fail on a null channel rather than on its own assertions. .ReturnsForAnyArgs(callInfo => { - // Positional, because this test may not NAME channelId: that parameter does not - // exist on the signature under test, and naming it is CS1739 — a compile error, not - // a failing assertion. The captured positions are stable across the fix, which - // appends channelId after data and before the CancellationToken: + // Positional capture, and the indices below survived B1 because it APPENDED + // channelId after data rather than inserting it among the optionals: // 0 app, 1 externalHost, 2 format, 3 encapsulation, 4 transport, - // 5 connectionType, 6 direction, 7 data, (8 channelId), last CancellationToken. + // 5 connectionType, 6 direction, 7 data, 8 channelId, 9 CancellationToken. + // That is a property of where the parameter landed, not a guarantee: any future + // insertion before index 7 renumbers these silently, because every slot from 3 to 8 + // is string? and ArgAt would happily read the wrong one. sentEncapsulation = callInfo.ArgAt(3); sentTransport = callInfo.ArgAt(4); sentData = callInfo.ArgAt(7); @@ -514,6 +529,221 @@ await start.Should().ThrowAsync( + "for audiosocket encapsulation\" (probe-capture.txt RUN G)"); } + [Fact] + public async Task StartAsync_ShouldSendOneCanonicalLowercaseIdentifierAsBothChannelIdAndData_WhenEncapsulationIsAudioSocket() + { + // The identity this change buys: Channel.Id and the AudioSocket identification UUID are one + // value, in the one spelling AudioSocketServer's table can be hit with. Both halves are + // measured, not reasoned: + // probe-capture.txt RUN A — channelId and data given DIFFERENT values: HTTP 200, + // Channel.Id is the channelId, the wire carries the data, and no lookup crosses them. + // probe-capture.txt RUN C — data only: Channel.Id is an Asterisk uniqueid ("1790244226.1") + // that appears in no stream table. + // probe-capture.txt RUN U — an UPPERCASE identifier: HTTP 200, Channel.Id echoed back in + // the spelling sent, while the wire bytes render through AudioSocketSession.ParseUuid as + // canonical lowercase into an ORDINAL dictionary. Created channel, unfindable stream, + // no error on any hop. + // RUN U's negative control is run against the real server rather than this substitute, in + // AudioSocketServerTests.GetStream_ShouldMiss_WhenIdentifierIsNotCanonicalLowercase. + var ariClient = Substitute.For(); + var channelsResource = Substitute.For(); + ariClient.Channels.Returns(channelsResource); + + string? sentEncapsulation = null; + string? sentData = null; + string? sentChannelId = null; + +#pragma warning disable CA2012 + channelsResource.CreateExternalMediaAsync( + app: Arg.Any(), externalHost: Arg.Any(), format: Arg.Any(), + encapsulation: Arg.Any(), transport: Arg.Any(), + connectionType: Arg.Any(), direction: Arg.Any(), + data: Arg.Any(), channelId: Arg.Any(), + cancellationToken: Arg.Any()) + .ReturnsForAnyArgs(callInfo => + { + // 0 app, 1 externalHost, 2 format, 3 encapsulation, 4 transport, + // 5 connectionType, 6 direction, 7 data, 8 channelId, 9 CancellationToken. + // Every slot from 3 to 8 is string?, so an insertion before index 7 renumbers these + // silently — see the note on the test above. + sentEncapsulation = callInfo.ArgAt(3); + sentData = callInfo.ArgAt(7); + sentChannelId = callInfo.ArgAt(8); + return new ValueTask(new AriChannel { Id = sentChannelId ?? "no-channel-id-was-sent" }); + }); +#pragma warning restore CA2012 + + await using var audioSocketServer = new AudioSocketServer( + new AudioServerOptions { AudioSocketPort = 0, ListenAddress = "127.0.0.1" }, + NullLogger.Instance); + + // Encapsulation left NULL on purpose: this is the derivation, not a value the test supplied. + var activity = new ExternalMediaActivity(ariClient, audioSocketServer) + { + App = "test", + ExternalHost = "127.0.0.1:19099", + ConnectionTimeout = TimeSpan.FromMilliseconds(50) + }; + + var start = () => activity.StartAsync().AsTask(); + await start.Should().ThrowAsync( + "nothing connects to the audio server here, so the run ends in the timeout — that is how " + + "it ends, not what is asserted below"); + + // Positive control, load-bearing for the same reason as in the test above: a capture that + // never ran reads null, and null would satisfy nothing here but would make every failure + // below look like the defect. This is the one value the derivation must produce. + sentEncapsulation.Should().Be("audiosocket", + "an AudioSocketServer with Encapsulation left null must derive audiosocket — otherwise " + + "Asterisk reads the absent parameter as rtp/udp, answers 200 with a UnicastRTP channel " + + "and nothing ever connects (probe-capture.txt RUN H/RUN H+)"); + + sentData.Should().NotBeNull("Asterisk rejects an audiosocket create with no data — " + + "HTTP 400 \"data can not be empty\" (RUN D)"); + sentChannelId.Should().NotBeNull("without channelId Asterisk mints the ARI id itself and it " + + "appears in no stream table (RUN C)"); + sentChannelId.Should().Be(sentData, + "one identifier in both parameters is what makes Channel.Id the key the stream table " + + "holds; different values create a channel whose stream cannot be found (RUN A)"); + + Guid.TryParse(sentData, out var minted).Should().BeTrue( + "Asterisk requires data to parse as a UUID — a malformed one is HTTP 500 with the " + + "listener up, the same status a dead port gives (probe-capture.txt correction #4)"); + sentData.Should().Be(minted.ToString(), + "AudioSocketSession.ParseUuid ends in new Guid(bytes, bigEndian: true).ToString(), which " + + "is canonical lowercase hyphenated, and AudioSocketServer looks that key up in a " + + "ConcurrentDictionary with the default ORDINAL comparer. This equality — " + + "not Guid.TryParseExact(\"D\"), which accepts an uppercase spelling too — is what pins " + + "the form"); + } + + [Fact] + public async Task StartAsync_ShouldSendTheCallersTransportUnchanged_WhenTransportIsSetExplicitly() + { + // The derivation is `transport ??= "tcp"`, not `transport = "tcp"`, and nothing else would + // catch the difference: every other test here leaves Transport null, so an unconditional + // assignment passes them all while quietly overwriting what a caller asked for. Asterisk + // answers a wrong transport with HTTP 400 "transport must be 'tcp' for audiosocket + // encapsulation" (probe-capture.txt RUN G) — its own message, at the create call, which is + // a better diagnosis than a silent rewrite into something that works. + var ariClient = Substitute.For(); + var channelsResource = Substitute.For(); + ariClient.Channels.Returns(channelsResource); + + string? sentTransport = null; + +#pragma warning disable CA2012 + channelsResource.CreateExternalMediaAsync( + app: Arg.Any(), externalHost: Arg.Any(), format: Arg.Any(), + encapsulation: Arg.Any(), transport: Arg.Any(), + connectionType: Arg.Any(), direction: Arg.Any(), + data: Arg.Any(), channelId: Arg.Any(), + cancellationToken: Arg.Any()) + .ReturnsForAnyArgs(callInfo => + { + sentTransport = callInfo.ArgAt(4); + return new ValueTask(new AriChannel { Id = "ext-ch-explicit-transport" }); + }); +#pragma warning restore CA2012 + + await using var audioSocketServer = new AudioSocketServer( + new AudioServerOptions { AudioSocketPort = 0, ListenAddress = "127.0.0.1" }, + NullLogger.Instance); + + var activity = new ExternalMediaActivity(ariClient, audioSocketServer) + { + App = "test", + ExternalHost = "127.0.0.1:19099", + Encapsulation = "audiosocket", + Transport = "udp", + ConnectionTimeout = TimeSpan.FromMilliseconds(50) + }; + + var start = () => activity.StartAsync().AsTask(); + await start.Should().ThrowAsync( + "nothing connects here either — the create call on the way there is what is asserted"); + + sentTransport.Should().Be("udp", + "a caller who set Transport explicitly gets exactly that on the wire; the \"tcp\" default " + + "fills an absent value and does not override a supplied one"); + } + + [Fact] + public async Task StartAsync_ShouldThrowNamingTheContradiction_WhenAudioSocketServerIsSuppliedForAnotherEncapsulation() + { + // Delta spec, Requirement 2: "An audio server that cannot be reached by the requested + // encapsulation is refused" — it SHALL fail with an error naming the contradiction and SHALL + // NOT wait out its connection timeout and report a connection failure. + // + // Chosen behaviour: throw InvalidOperationException BEFORE the create call. The rejected + // alternatives, recorded so the choice is visible: silently overriding the caller's + // Encapsulation (the caller asked for something and would get something else, with no + // diagnostic), or ignoring the server (which is today's behaviour — a 30-second wait ending + // in "Asterisk did not connect to audio server", a symptom reported one layer away from the + // cause). Refusing before the create also keeps a doomed channel from existing at all. + var ariClient = Substitute.For(); + var channelsResource = Substitute.For(); + ariClient.Channels.Returns(channelsResource); + + // The create is configured to SUCCEED even though it must never be reached. Without this the + // negative control is blunt: delete the guard and the activity dies on a NullReferenceException + // from an unconfigured substitute, which would fail this test for the wrong reason and say + // nothing about the timeout. With it, deleting the guard reproduces today's behaviour exactly — + // Asterisk creates a UnicastRTP channel (probe-capture.txt RUN H), nothing connects, and the + // activity waits out the whole ConnectionTimeout before throwing TimeoutException. Every + // assertion below then fails on the real defect. +#pragma warning disable CA2012 + channelsResource.CreateExternalMediaAsync( + app: Arg.Any(), externalHost: Arg.Any(), format: Arg.Any(), + encapsulation: Arg.Any(), transport: Arg.Any(), + connectionType: Arg.Any(), direction: Arg.Any(), + data: Arg.Any(), channelId: Arg.Any(), + cancellationToken: Arg.Any()) + .ReturnsForAnyArgs(new ValueTask(new AriChannel { Id = "ext-ch-rtp" })); +#pragma warning restore CA2012 + + await using var audioSocketServer = new AudioSocketServer( + new AudioServerOptions { AudioSocketPort = 0, ListenAddress = "127.0.0.1" }, + NullLogger.Instance); + + var activity = new ExternalMediaActivity(ariClient, audioSocketServer) + { + App = "test", + ExternalHost = "127.0.0.1:19099", + Encapsulation = "rtp", + ConnectionTimeout = TimeSpan.FromSeconds(30) + }; + + var started = System.Diagnostics.Stopwatch.GetTimestamp(); + var start = () => activity.StartAsync().AsTask(); + var thrown = await start.Should().ThrowAsync( + "an AudioSocketServer cannot be reached over rtp encapsulation, and the SDK refuses the " + + "combination instead of creating a channel that can never carry that server's stream"); + var elapsed = System.Diagnostics.Stopwatch.GetElapsedTime(started); + + thrown.Which.Message.Should().Contain("AudioSocketServer", + "the message has to name what was supplied"); + thrown.Which.Message.Should().Contain("rtp", + "and the encapsulation it contradicts, so the reader is not left to guess which half to change"); + thrown.Which.Message.Should().Contain("audiosocket", + "and the value that would resolve it"); + + activity.Channel.Should().BeNull( + "the spec requires the failure BEFORE a channel is created, not after"); + channelsResource.ReceivedCalls().Should().BeEmpty( + "nothing at all should have been asked of ARI — no externalMedia create, and so no " + + "channel to hang up"); + + // The timeout clause of the requirement, measured rather than inferred from the exception + // type. 30 s configured against a 5 s bound: a wide margin, because the claim is "did not + // wait it out", not "was fast". + elapsed.Should().BeLessThan(TimeSpan.FromSeconds(5), + "the contradiction is refused up front; today's behaviour waits out the whole " + + "ConnectionTimeout and then reports a connection failure"); + activity.Status.Should().Be(ActivityStatus.Failed, + "a refused configuration is a failed activity, not a cancelled one"); + } + [Fact] public async Task CancelAsync_ShouldNotThrow_WhenActivityAlreadyCompleted() { diff --git a/Tests/Verbara.Sdk.Ari.Tests/Audio/AudioSocketServerTests.cs b/Tests/Verbara.Sdk.Ari.Tests/Audio/AudioSocketServerTests.cs index 4b4ee9e6..798038e5 100644 --- a/Tests/Verbara.Sdk.Ari.Tests/Audio/AudioSocketServerTests.cs +++ b/Tests/Verbara.Sdk.Ari.Tests/Audio/AudioSocketServerTests.cs @@ -189,6 +189,42 @@ public async Task HandleConnection_ShouldRegisterStream_WhenUuidReceived() registeredStream.Format.Should().Be("slin16"); } + [Fact] + public async Task GetStream_ShouldMiss_WhenIdentifierIsNotCanonicalLowercase() + { + // The negative control behind ExternalMediaActivity minting its identifier with + // Guid.ToString(). The fixture is a measurement, not an argument: probe-capture.txt RUN U + // sent this exact UUID to a real Asterisk — 22.9.0 and 23.4.1, identically — as both + // channelId and data: + // -> HTTP 200 id=EC30994A-B0E1-4EBE-99BD-19CA35669F7A + // HEX: 01 00 10 ec 30 99 4a b0 e1 4e be 99 bd 19 ca 35 66 9f 7a + // Asterisk accepted the uppercase spelling and echoed Channel.Id back in it, while the + // identification frame carried sixteen raw bytes. This test puts those sixteen bytes into + // the real server and then asks it for the stream BOTH ways — the way a consumer holding + // Channel.Id would, and the way the table is actually keyed. + const string asSentToAsterisk = "EC30994A-B0E1-4EBE-99BD-19CA35669F7A"; + var wireUuid = Guid.Parse(asSentToAsterisk); + var canonical = wireUuid.ToString(); + canonical.Should().NotBe(asSentToAsterisk, + "the fixture has to differ in spelling from its canonical form, or this test controls nothing"); + + var port = GetFreePort(); + var server = CreateServer(port); + await server.StartAsync(); + + using var client = await ConnectAndSendUuidAsync(port, wireUuid, server); + + server.ActiveStreamCount.Should().Be(1, "the identification frame was accepted"); + server.GetStream(canonical).Should().NotBeNull( + "ParseUuid renders the sixteen wire bytes with new Guid(bytes, bigEndian: true).ToString(), " + + "so the table is keyed by the canonical lowercase hyphenated form"); + server.GetStream(asSentToAsterisk).Should().BeNull( + "the table is a ConcurrentDictionary with the default ORDINAL comparer. A " + + "channel created with a non-canonical channelId comes back with Channel.Id in that " + + "spelling, so GetStream(Channel.Id) misses — HTTP 200, a live stream, and no error on " + + "any hop. That silence is why the identifier is minted rather than accepted"); + } + [Fact] public async Task HandleConnection_ShouldEmitOnStreamConnected_WhenUuidReceived() { diff --git a/Tests/Verbara.Sdk.Ari.Tests/Resources/AriResourceTests.cs b/Tests/Verbara.Sdk.Ari.Tests/Resources/AriResourceTests.cs index a4115c94..7bb69d50 100644 --- a/Tests/Verbara.Sdk.Ari.Tests/Resources/AriResourceTests.cs +++ b/Tests/Verbara.Sdk.Ari.Tests/Resources/AriResourceTests.cs @@ -158,8 +158,14 @@ public async Task Channels_CreateExternalMediaAsync_ShouldPostWithParams() using var http = CreateHttpClient(handler); var sut = new AriChannelsResource(http, DefaultOptions); + // Every optional is SUPPLIED, not just asserted on. Until this change the test passed + // encapsulation and transport only, and the connection_type, direction and data appenders had + // never executed across the whole Ari suite — an assertion about a parameter the test does not + // send holds whatever the appender does, including nothing. var result = await sut.CreateExternalMediaAsync("myapp", "127.0.0.1:8000", "slin16", - encapsulation: "rtp", transport: "udp"); + encapsulation: "rtp", transport: "udp", connectionType: "client", direction: "both", + data: "f9e8d7c6-b5a4-3928-1706-f5e4d3c2b1a0", + channelId: "0a1b2c3d-4e5f-6071-8293-a4b5c6d7e8f9"); result.Id.Should().Be("ch-ext"); handler.LastMethod.Should().Be(HttpMethod.Post); @@ -168,6 +174,18 @@ public async Task Channels_CreateExternalMediaAsync_ShouldPostWithParams() handler.LastRequestUri.Should().Contain("format=slin16"); handler.LastRequestUri.Should().Contain("encapsulation=rtp"); handler.LastRequestUri.Should().Contain("transport=udp"); + handler.LastRequestUri.Should().Contain("connection_type=client"); + handler.LastRequestUri.Should().Contain("direction=both"); + handler.LastRequestUri.Should().Contain("data=f9e8d7c6-b5a4-3928-1706-f5e4d3c2b1a0"); + + // The literal spelling, deliberately. Asterisk spells this one parameter in camelCase among + // snake_case siblings and ignores an unrecognised query parameter silently — "channel_id=" + // would leave the create returning HTTP 200 with an Asterisk-minted id and no error anywhere, + // so only a test on the exact bytes catches the misspelling. + handler.LastRequestUri.Should().Contain("channelId=0a1b2c3d-4e5f-6071-8293-a4b5c6d7e8f9", + "Asterisk spells this parameter channelId, and silently ignores any other spelling"); + handler.LastRequestUri.Should().NotContain("channel_id=", + "the snake_case spelling the siblings use is the misspelling this assertion exists to catch"); } [Fact] diff --git a/docs/guides/README.md b/docs/guides/README.md index e9020430..251a0dba 100644 --- a/docs/guides/README.md +++ b/docs/guides/README.md @@ -6,6 +6,7 @@ Practical how-to guides for working with Verbara Sdk. |-------|-------------| | [asterisk-version-compatibility.md](asterisk-version-compatibility.md) | AMI event coverage matrix across Asterisk 18-23, listing typed classes and fallback behavior per version. | | [asterisk-version-matrix.md](asterisk-version-matrix.md) | Supported Asterisk versions (22 LTS primary, 23 Standard secondary), Docker test infrastructure, and known-divergent behavior. | +| [externalmedia-channel-id-migration.md](externalmedia-channel-id-migration.md) | Moving to the `CreateExternalMediaAsync` signature that takes a `channelId` -- why the break was bought, which callers have to be edited rather than rebuilt, and how to make an AudioSocket stream findable by the channel id the create returned. | | [high-load-tuning.md](high-load-tuning.md) | Configuration guidance for high-load scenarios (1K-100K+ agents), including EventPump sizing and buffer capacity recommendations. | | [log-analysis-prompt.md](log-analysis-prompt.md) | Ready-to-use LLM prompt for analyzing Verbara.Sdk structured log files with extract, classify, and diagnose phases. | | [log-analysis-reference.md](log-analysis-reference.md) | Tag catalog for SDK structured logs -- lists every `[TAG]`, its domain, source classes, and emitted events. | diff --git a/docs/guides/externalmedia-channel-id-migration.md b/docs/guides/externalmedia-channel-id-migration.md new file mode 100644 index 00000000..253f5e0c --- /dev/null +++ b/docs/guides/externalmedia-channel-id-migration.md @@ -0,0 +1,166 @@ +# Migrating to `CreateExternalMediaAsync` with a `channelId` + +Required by ADR-0028: a minor that carries a breaking change ships a migration guide. + +## What happened + +`CreateExternalMediaAsync` gained an optional `string? channelId` parameter, on +`Verbara.Sdk.IAriChannelsResource` and on `Verbara.Sdk.Ari.Resources.AriChannelsResource`. It sits +between `data` and the cancellation token: + +```csharp +// before +ValueTask CreateExternalMediaAsync(string app, string externalHost, string format, + string? encapsulation = null, string? transport = null, string? connectionType = null, + string? direction = null, string? data = null, CancellationToken cancellationToken = default); + +// after +ValueTask CreateExternalMediaAsync(string app, string externalHost, string format, + string? encapsulation = null, string? transport = null, string? connectionType = null, + string? direction = null, string? data = null, string? channelId = null, + CancellationToken cancellationToken = default); +``` + +Optional parameters are compile-time sugar. The nine-parameter member is gone from both assemblies, +which is why this is a break and not an addition: `CP0002` in both packages, plus `CP0006` on the +interface. Those three are declared in each package's `CompatibilitySuppressions.xml`. + +## Why a parameter is worth a break + +Asterisk's `externalMedia` takes two identifiers that are easy to mistake for one. + +- `data` is what AudioSocket sends back on the audio connection, in its identification frame. It is + the value an `AudioSocketServer` keys its stream table by, and Asterisk requires it to parse as a + UUID. +- `channelId` is the id Asterisk assigns the ARI channel it creates. It comes back as `Channel.Id`. + +Supply only `data` and Asterisk mints the channel id itself — something like `1790244226.1`, which +appears in no stream table. A consumer holding only an ARI channel id, which is what a `StasisStart` +or `ChannelHangupRequest` handler holds, then has nothing it can look the stream up with. Passing one +identifier as **both** makes `Channel.Id` the key, and that is the capability this parameter buys. + +The smaller fix that breaks nothing — mint the UUID, pass it only as `data`, and look the stream up by +the value you minted — works, and was rejected deliberately: it fixes the caller that minted the UUID +and leaves everyone downstream of it unable to find the stream. + +## What you have to do + +**Recompile.** For almost every caller that is the whole of it, because the parameter is optional and +appended rather than inserted. + +Three populations need an edit. + +### If you passed the cancellation token positionally + +```csharp +// stops compiling: the ninth positional argument now binds to string? channelId +await channels.CreateExternalMediaAsync(app, host, "slin16", "audiosocket", "tcp", null, null, + uuid, cancellationToken); +``` + +The compiler reports `CS1503` — `cannot convert from 'System.Threading.CancellationToken' to +'string?'`. Name the token: + +```csharp +await channels.CreateExternalMediaAsync(app, host, "slin16", "audiosocket", "tcp", null, null, + uuid, cancellationToken: cancellationToken); +``` + +The parameter was appended rather than placed first among the optionals — where +`CreateWithoutDialAsync` puts its own `channelId` — precisely so this break would be a compile error. +Inserting ahead of `encapsulation` would have rebound every existing positional argument to a +different `string?` parameter and gone on compiling. + +### If you implement `IAriChannelsResource` + +Fakes, decorators and proxies stop compiling with `CS0535` until they adopt the parameter. A decorator +forwards it; a fake that ignores it should still accept it, so it keeps matching the interface. + +### If you have a mocking framework set up on this method + +A setup that enumerates positional matchers ending in a `CancellationToken` hits the `CS1503` above. +Move it to named arguments — and name `channelId` explicitly: + +```csharp +channels.CreateExternalMediaAsync( + app: Arg.Any(), externalHost: Arg.Any(), format: Arg.Any(), + encapsulation: Arg.Any(), transport: Arg.Any(), + connectionType: Arg.Any(), direction: Arg.Any(), + data: Arg.Any(), channelId: Arg.Any(), + cancellationToken: Arg.Any()) +``` + +Leaving `channelId` out of a named list is the quiet failure. The compiler supplies its default +`null`, the mock equality-matches that `null`, and the setup stops matching the moment real code +supplies an id — so the test fails somewhere else entirely, on whatever the unmatched call returned. + +## How to use it + +Mint one identifier and pass it as both, in canonical lowercase hyphenated form: + +```csharp +var id = Guid.NewGuid().ToString(); // "e1a2..." — lowercase, hyphenated +var channel = await ariClient.Channels.CreateExternalMediaAsync( + app: "myapp", externalHost: "10.0.0.5:9092", format: "slin16", + encapsulation: "audiosocket", transport: "tcp", + data: id, channelId: id); + +var stream = audioSocketServer.GetStream(channel.Id); // now finds it +``` + +Three things that will cost you time if you take them on trust instead of from here: + +- **`transport = "tcp"` is mandatory with `encapsulation = "audiosocket"`.** Without it the create + returns `HTTP 400 — transport must be 'tcp' for audiosocket encapsulation`, and with it but without + `data`, `HTTP 400 — data can not be empty`. +- **The spelling is `channelId`, in camelCase**, alone among the snake_case siblings + (`external_host`, `connection_type`). Asterisk ignores a query parameter it does not recognise + rather than rejecting it, so a misspelling is an `HTTP 200` with an Asterisk-minted id and no error + anywhere — the SDK's own test asserts the literal bytes for that reason. +- **Canonical lowercase is not cosmetic.** Asterisk accepts an uppercase UUID and echoes `Channel.Id` + back in the spelling you sent, but the identification frame is sixteen raw bytes that the server + renders with `Guid.ToString()` — lowercase — into an ordinal dictionary. An uppercase id therefore + creates a channel whose stream can never be found, with no error on any hop. + +`channelId` itself is free-form as far as Asterisk is concerned; it is `data` that must parse as a +UUID. Using the same value for both is what makes the lookup work, so in practice both are a UUID. + +## If you use `ExternalMediaActivity` + +The activity now mints the identifier and supplies it to both parameters itself, in the same release +as this break — making that true is what the break was bought for. It also derives the rest of the +request, so three behaviours changed. Only the third can break a caller that works today. + +**1. `Encapsulation` left null with an `AudioSocketServer` supplied now means `audiosocket`.** It +used to mean rtp/udp, which is how the default configuration created a `UnicastRTP` channel that +nothing connected to and then threw `TimeoutException` after `ConnectionTimeout`. Nothing to do: the +configuration that was broken is the one that changed. + +**2. `Transport` left null under AudioSocket encapsulation now means `"tcp"`.** It used to be sent as +absent, which is `HTTP 400 — transport must be 'tcp' for audiosocket encapsulation`. An explicit +`Transport` is still sent exactly as given, so a wrong one still fails at the create call with +Asterisk's own message rather than being silently rewritten. + +**3. An `AudioSocketServer` with a non-AudioSocket `Encapsulation` is now refused.** + +```csharp +new ExternalMediaActivity(ariClient, audioSocketServer) +{ + App = "myapp", ExternalHost = "10.0.0.5:9092", Encapsulation = "rtp" +} +``` + +`StartAsync` throws `InvalidOperationException` naming the contradiction, before any channel is +created. Previously this created an RTP channel, waited out the whole `ConnectionTimeout` — 30 +seconds by default — and threw `TimeoutException: Asterisk did not connect to audio server`, which +reports the symptom one layer away from the cause. If you hit this, either set `Encapsulation` to +`"audiosocket"` (or leave it null, which derives it) or stop passing the `AudioSocketServer`. + +Callers with **no** audio server, or with a WebSocket server only, take an unchanged path. + +## What this does not cover + +The WebSocket transport. `WebSocketAudioServer` keys its table on the last segment of the request +path, not on an identification frame, so `channelId` does not make its lookups work. What +`externalMedia` with `transport=websocket` actually puts in the request path has not been measured, +and is tracked separately rather than designed from a reading of the code. diff --git a/openspec/changes/externalmedia-returns-a-channel-id-that-finds-its-stream/tasks.md b/openspec/changes/externalmedia-returns-a-channel-id-that-finds-its-stream/tasks.md index 479d8520..4f44cfe0 100644 --- a/openspec/changes/externalmedia-returns-a-channel-id-that-finds-its-stream/tasks.md +++ b/openspec/changes/externalmedia-returns-a-channel-id-that-finds-its-stream/tasks.md @@ -183,7 +183,7 @@ failure is pasted here verbatim.** A task is not checked off until the thing it worktree `AriChannelsResource` is the only implementer, so nothing here breaks on CS0535. ## Phase B — critical components (one focused subagent each) -- [ ] B1 Add the `channelId` parameter to `CreateExternalMediaAsync` on **both** +- [x] B1 Add the `channelId` parameter to `CreateExternalMediaAsync` on **both** `IAriChannelsResource` and `AriChannelsResource`, positioned **before** `cancellationToken` (CT-last is the SDK's convention and CA1068 is on under `TreatWarningsAsErrors`). Then: - the two `ActivityTests` substitutes move to **named arguments**; @@ -198,7 +198,81 @@ failure is pasted here verbatim.** A task is not checked off until the thing it - a migration note (ADR-0028 requires one for a minor carrying a break). Confirm with the exact command CI runs — `dotnet pack` — not with a clean build. A green build says nothing about `PackageValidation`; that mistake cost #302 a CI failure. -- [ ] B2 `ExternalMediaActivity` derives its request from the server it was handed: + + **Done 2026-09-24.** Parameter in, break declared in both packages, `dotnet pack -c Release` + green with validation **proven** to have run. The A3 test still failed here — correctly; it + needs B2. + + **Position: appended after `data`, against the in-tree precedent, and the reason is the point.** + A2 found `IAriClient.cs:141` puts `channelId` first among `CreateWithoutDialAsync`'s optionals. + Inserting ahead of `encapsulation` would have been a **silent** break: every slot from + `encapsulation` to `data` is `string?`, so an existing positional call would rebind each + argument one place over and go on compiling — wrong values on the wire, no diagnostic anywhere. + Appending breaks only callers that passed the token positionally, and that break is `CS1503`. + A loud break beats consistency with one sibling. The reason is written into the `` so + the next reader does not "fix" the inconsistency. + + ```csharp + ValueTask CreateExternalMediaAsync(string app, string externalHost, string format, + string? encapsulation = null, string? transport = null, string? connectionType = null, + string? direction = null, string? data = null, string? channelId = null, + CancellationToken cancellationToken = default); + ``` + + **The camelCase spelling is corroborated inside the tree, which nobody had noticed.** + `CreateWithoutDialAsync` already appends `&channelId=` at `AriChannelsResource.cs:222`. This + repository has been spelling it that way on another endpoint since before the probe measured it. + + **`AriResourceTests` now sends what it asserts.** A4's point was that the test asserted + `encapsulation=`/`transport=` while `data=`, `connection_type=` and `direction=` had never + executed across 477 tests. It now supplies all six optionals and asserts each literal. + Negative control — the appender misspelled as `channel_id=`: + + ```text + Failed Channels_CreateExternalMediaAsync_ShouldPostWithParams + Expected handler.LastRequestUri "…&data=f9e8d7c6-…&channel_id=0a1b2c3d-…" to contain + "channelId=0a1b2c3d-4e5f-6071-8293-a4b5c6d7e8f9" because Asterisk spells this parameter + channelId, and silently ignores any other spelling. + ``` + + That failure doubles as proof the other three appenders now execute — they are all in the URL it + printed. + + **RS0017 forced the `*REMOVED*` rows; nothing forced the new ones.** A2's warning held exactly: + + ```text + src/Verbara.Sdk/PublicAPI.Shipped.txt(499,1): error RS0017: Symbol + 'Verbara.Sdk.IAriChannelsResource.CreateExternalMediaAsync(…)' is part of the declared API, + but is either not public or could not be found + ``` + + The new rows are present because ADR-0023 says so, not because the compiler asked. + + **Suppressions generated to a scratch path, read entry by entry, then hand-written.** Before any + suppression file, `pack` reported exactly the three predicted breaks and nothing else: `CP0002` + on `AriChannelsResource`, `CP0002` on `IAriChannelsResource`, `CP0006` for the added interface + member. `-p:ApiCompatSuppressionOutputFile=` kept the build machine out of the + repository (ADR-0055). `src/Verbara.Sdk.Ari`'s five existing `AudioFrameType` entries were + reproduced byte-identically — nothing dropped, nothing invented. + + **A new member of the stale-green family, and it is a big one.** `PackageValidation` is + **incremental**: with the whole of `src/Verbara.Sdk/CompatibilitySuppressions.xml` deleted, + `dotnet pack` still exited 0 and printed 29 successful packages. The gate is + `obj/Release/net10.0/Microsoft.NET.ApiCompat.ValidatePackage.semaphore`. Deleting the semaphores + is not enough either — the nupkgs must go too, or packaging is skipped while validation reports + on nothing. Local-only; CI packs a fresh checkout. Any future "pack is green" claim in this + repository must state how it forced validation to run. + + **And `dotnet pack` prints nothing to a redirected stdout on success** — no summary line at all. + A zero-line log with exit 0 is indistinguishable from a run that did nothing. + + **A4's arithmetic was wrong.** Measured against the committed tree: **2** unique CS1503 and + **18** unique NS1004, not 3 and ~27. A4 counted a draft of A3's substitute; the committed one + already named `cancellationToken:`. The shape of A4's warning stands, the numbers do not. + + **Two substitutes beyond A4's list** — `ContactCenterActivityTests.cs:163` and `:196` — neither + configuring this method, so neither needed a change. Seven substitutes exist, not five. +- [x] B2 `ExternalMediaActivity` derives its request from the server it was handed: `encapsulation = Encapsulation ?? (_audioSocketServer is not null ? "audiosocket" : null)`; when the encapsulation is AudioSocket (compare ordinal-ignore-case — Asterisk uses `strcasecmp`) then `transport = Transport ?? "tcp"` and one `Guid.NewGuid().ToString()` goes to **both** `channelId` @@ -209,7 +283,41 @@ failure is pasted here verbatim.** A task is not checked off until the thing it encapsulation: today that combination creates an RTP channel and waits out a 30-second timeout, which is the shape the delta spec forbids. A unit test pins whichever behaviour is chosen, plus one negative control using a non-canonical identifier spelling. -- [ ] B3 Fix the contract text. `IAudioServer.GetStream` says "Get an active stream by channel ID" and + + **Done 2026-09-24.** The activity now derives its request from the server it was handed, the A3 + regression test passes, and the contradiction case is refused instead of timing out. + + The mint is `Guid.NewGuid().ToString()` — canonical lowercase, because `ParseUuid` keys the + table with `new Guid(bytes, bigEndian: true).ToString()` into an **ordinal** comparer and A1 + measured what any other spelling does: HTTP 200, `Channel.Id` in the spelling sent, and a lookup + that misses. Both reasons are written in the code, not left to this file. + + **The contradiction case is refused.** An `AudioSocketServer` supplied while the encapsulation is + not AudioSocket used to create an RTP channel and wait out thirty seconds — the shape + Requirement 2 of the delta spec forbids. + + **The negative control for it was blunt and nearly shipped that way.** With an unconfigured + substitute, deleting the guard made the activity die on a `NullReferenceException` from a null + `Channel` — a red for the wrong reason, saying nothing about the timeout it was supposed to + prove. Configuring the create to **succeed** is what makes the mutation reproduce the real + defect: + + ```text + [30 s] TimeoutException: Asterisk did not connect to audio server within 00:00:30 + ``` + + The arrange looks redundant — the call must never happen — so there is a comment saying why it + is there. + + **Two tooling findings.** `dotnet pack -tl:off -v m` prints **no** summary on success either, so + grepping for "Build succeeded" on a green pack returns nothing: a different flavour of B1's + note, and `-tl:off -v m` fixes `build` but not `pack`. And deleting only the ApiCompat + semaphores is not enough — measured EXIT=0 with **zero** `Successfully created package` lines, + validation running while packaging was skipped entirely. The nupkgs have to go too. + + `` on a member inherited from `AriActivityBase` does not resolve, and + CS1574 is an error here — the full `AriActivityBase.StartAsync(CancellationToken)` is required. +- [x] B3 Fix the contract text. `IAudioServer.GetStream` says "Get an active stream by channel ID" and `IAudioStream.ChannelId` says "Unique ID of the external media channel in Asterisk" — both assert an identity the code does not hold. Say for each implementation what the key is and in what form: for `AudioSocketServer`, the UUID Asterisk sent in its identification frame in @@ -217,6 +325,33 @@ failure is pasted here verbatim.** A task is not checked off until the thing it as `channelId` and `data`. Say in `ExternalMediaActivity`'s remarks that the WebSocket branch is **not** routed — `WebSocketAudioServer` keys on the last segment of the request URL (F1). + + **Done 2026-09-24.** Three findings, and one deliberate widening. + + **The same wrong sentence was on both implementations, not only on the interface.** + `/// Get an active stream by channel ID.` sat verbatim on + `AudioSocketServer.cs:52` and `WebSocketAudioServer.cs:63` — public members of public shipped + classes, which is what a caller holding a concrete server reads. Both fixed, named here rather + than done silently: Requirement 4 covers *any* published contract for looking a stream up by + key, and leaving them would have left the interface doc contradicting its own implementations. + + **The brief's phrasing was over-stated and was not reproduced.** It said the AudioSocket key is + "the value the creator supplied as `channelId` and `data`". Probe RUN C shows `data` **alone** + is the key — the wire UUID equalled `data` while `Channel.Id` came back as the Asterisk uniqueid + `1790244226.1`. `channelId` contributes nothing to the key; it only makes `Channel.Id` equal to + it. The landed text says that instead. + + **A fourth wrong piece of prose found and deliberately left.** + `WebSocketAudioServer.cs:311-313` documents `internal static ReadUpgradeRequestAsync` as + extracting "channel ID from URL path … Expected URL: /ws/{channelId}" — precisely the false + identity this task removes, and it even names the placeholder `{channelId}`. It is on an + internal member, so not a published contract. It belongs to F1's change, where the WebSocket key + finally gets measured. + + **One expectation this phase had accumulated is wrong, and that matters.** + `GenerateDocumentationFile` is true and CS1574 is **not** in `NoWarn`, so the doc edits do have a + compiler oracle — unlike RS0016, which A2 measured as globally suppressed. A green doc build + here is worth something. ## Phase C — integration (batched) - [ ] C1 Functional test beside `AudioSocketWireFunctionalTests`. Three things it must get right: @@ -263,7 +398,7 @@ ADR-0060 wrote "tracked separately" with no link, and the close-out archived it change that carries these; the lines below are the evidence it starts from. - **F1 — `WebSocketAudioServer` keys on a URL path segment.** The key is computed at - `WebSocketAudioServer.cs:337` (`path.TrimStart('/').Split('/').LastOrDefault()`) and registered at + `WebSocketAudioServer.cs:337` (`path.TrimStart('/').Split('/').LastOrDefault()?.Split('?').FirstOrDefault()`) and registered at `:262`. The SDK's own example puts a literal `/audio` there (`Examples/WebSocketMediaExample/Program.cs:10`), so every concurrent call would register under the key `"audio"`. `CompositeAudioServer` hands the same string to both servers, which do not agree on diff --git a/src/Verbara.Sdk.Activities/Activities/ExternalMediaActivity.cs b/src/Verbara.Sdk.Activities/Activities/ExternalMediaActivity.cs index a6b21461..71336539 100644 --- a/src/Verbara.Sdk.Activities/Activities/ExternalMediaActivity.cs +++ b/src/Verbara.Sdk.Activities/Activities/ExternalMediaActivity.cs @@ -8,8 +8,44 @@ namespace Verbara.Sdk.Activities.Activities; /// to connect back via AudioSocket or WebSocket, and provides an IAudioStream /// for bidirectional audio streaming. /// +/// +/// +/// Only the AudioSocket branch is routed. This activity looks the stream up by +/// on whichever server it was handed, and that only works for +/// AudioSocket because this class arranges it: it mints one identifier and sends it as both +/// channelId and data, so the ARI channel id and the key +/// registers the stream under are the same string. +/// +/// +/// Nothing arranges that for . It keys its table on the last path +/// segment of the HTTP upgrade request URL with the query string stripped, while a non-AudioSocket +/// create leaves an Asterisk-minted uniqueid such as +/// 1790244226.1. This class does not make those two match, so a +/// passed to this activity is expected to poll out +/// and throw however well the +/// WebSocket connection itself went — unless that request path happens to carry the very id +/// Asterisk minted, which is exactly what no probe has measured. That is why this branch is +/// documented here rather than fixed. +/// +/// public sealed class ExternalMediaActivity : AriActivityBase { + /// + /// The value Asterisk's externalMedia expects for AudioSocket encapsulation. Asterisk + /// compares it with strcasecmp, which is why every comparison against it here is + /// : a caller who writes "AudioSocket" reaches + /// chan_audiosocket exactly as one who writes "audiosocket" does, and an ordinal comparison + /// would quietly route the first down the RTP path instead. + /// + private const string AudioSocketEncapsulation = "audiosocket"; + + /// + /// The only transport Asterisk accepts under AudioSocket encapsulation. Anything else is + /// HTTP 400 — "transport must be 'tcp' for audiosocket encapsulation", measured on + /// Asterisk 22.9.0 and 23.4.1 (probe-capture.txt, RUN G). + /// + private const string AudioSocketTransport = "tcp"; + private readonly AudioSocketServer? _audioSocketServer; private readonly WebSocketAudioServer? _webSocketServer; private IAudioStream? _audioStream; @@ -23,10 +59,23 @@ public sealed class ExternalMediaActivity : AriActivityBase /// Audio format. Default: "slin16". public string Format { get; init; } = "slin16"; - /// Encapsulation type (e.g., "audiosocket") or null for default. + /// + /// Encapsulation type (e.g., "audiosocket"), or null to let the activity derive it. Left null + /// with an supplied it becomes "audiosocket"; left null with no + /// AudioSocket server it stays null, which Asterisk reads as rtp/udp. Setting it to a + /// non-AudioSocket value while supplying an is a contradiction + /// and throws + /// rather than + /// creating a channel that can never carry that server's stream. + /// public string? Encapsulation { get; init; } - /// Transport type (e.g., "websocket") or null for default. + /// + /// Transport type (e.g., "websocket"), or null to let the activity derive it. Under AudioSocket + /// encapsulation a null transport becomes "tcp", the only one Asterisk accepts there. An + /// explicit value is always sent as given, so a wrong one fails at the create call with + /// Asterisk's own message instead of being silently rewritten. + /// public string? Transport { get; init; } /// Timeout waiting for Asterisk to connect to the audio server. @@ -47,14 +96,80 @@ public ExternalMediaActivity(IAriClient ariClient, AudioSocketServer? audioSocke protected override async ValueTask ExecuteAsync(CancellationToken cancellationToken) { - // 1. Create ExternalMedia channel via ARI + // 1. Derive the request from the audio server this activity was handed. + // + // An AudioSocketServer with Encapsulation left null is the shape callers actually get, and + // Asterisk reads an absent encapsulation as rtp/udp: it answers HTTP 200 with a UnicastRTP + // channel, nothing ever connects to the AudioSocket server, and the poll below burns the + // whole of ConnectionTimeout before throwing (probe-capture.txt, RUN H and RUN H+). + var encapsulation = Encapsulation ?? (_audioSocketServer is not null ? AudioSocketEncapsulation : null); + + // strcasecmp on Asterisk's side — see AudioSocketEncapsulation. + var isAudioSocket = string.Equals(encapsulation, AudioSocketEncapsulation, StringComparison.OrdinalIgnoreCase); + + if (_audioSocketServer is not null && !isAudioSocket) + { + // The contradiction case, refused rather than attempted. Asterisk would happily create + // the channel on the encapsulation asked for; the AudioSocket server would simply never + // be connected to, and this activity would then wait out ConnectionTimeout and report a + // TimeoutException — "the audio server did not connect" — for what is a configuration + // mistake made before any connection was possible. Failing here keeps the diagnosis + // where the cause is, and keeps a doomed channel from being created at all. + throw new InvalidOperationException( + $"An AudioSocketServer was supplied but Encapsulation is \"{encapsulation}\", not \"{AudioSocketEncapsulation}\". " + + $"Asterisk would create a {encapsulation} channel that never connects to that server, and this activity " + + $"would wait out ConnectionTimeout ({ConnectionTimeout}) and report a connection failure instead of this " + + "contradiction. Set Encapsulation to \"audiosocket\" — or leave it null, which derives it from the " + + "server — or do not pass an AudioSocketServer."); + } + + var transport = Transport; + string? data = null; + string? channelId = null; + + if (isAudioSocket) + { + // Asterisk rejects audiosocket on any other transport with HTTP 400 "transport must be + // 'tcp' for audiosocket encapsulation" (RUN G) — and the activity never sent one, which + // is why every AudioSocket caller failed at the create call. An explicit Transport still + // wins: a wrong one then fails with Asterisk's own message rather than being rewritten. + transport ??= AudioSocketTransport; + + // ONE identifier into BOTH parameters, which is the whole of this change. + // + // They are different things. `data` is the UUID Asterisk puts in the AudioSocket + // identification frame, and it is what AudioSocketServer keys its stream table by; + // `channelId` is the id Asterisk assigns the ARI channel, and comes back as Channel.Id. + // Passing only `data` leaves Channel.Id an Asterisk-minted uniqueid like "1790244226.1" + // that appears in no table (RUN C), so a consumer holding only a channel id — a + // StasisStart or ChannelHangupRequest handler — can never find the stream. Passing the + // same value as both makes Channel.Id the key (RUN A). Asterisk also requires `data` to + // be present at all: without it, HTTP 400 "data can not be empty" (RUN D). + // + // Guid.ToString() — canonical lowercase, hyphenated — is load-bearing, for two measured + // reasons, not for tidiness: + // 1. AudioSocketSession.ParseUuid renders the sixteen wire bytes with + // new Guid(bytes, bigEndian: true).ToString(), which is always lowercase, and + // AudioSocketServer holds those keys in a ConcurrentDictionary whose + // default comparer is ORDINAL. Only the lowercase spelling can hit it. + // 2. Asterisk echoes `channelId` back verbatim and does not normalise it: an uppercase + // identifier returns HTTP 200 with Channel.Id in the spelling that was sent + // (RUN U), so GetStream(Channel.Id) would miss with no error on any hop. + var identifier = Guid.NewGuid().ToString(); + data = identifier; + channelId = identifier; + } + + // 2. Create ExternalMedia channel via ARI Channel = await AriClient.Channels.CreateExternalMediaAsync( App, ExternalHost, Format, - encapsulation: Encapsulation, - transport: Transport, + encapsulation: encapsulation, + transport: transport, + data: data, + channelId: channelId, cancellationToken: cancellationToken); - // 2. Poll for audio server connection (200ms interval) + // 3. Poll for audio server connection (200ms interval) using var timeoutCts = CancellationTokenSource.CreateLinkedTokenSource(cancellationToken); timeoutCts.CancelAfter(ConnectionTimeout); diff --git a/src/Verbara.Sdk.Ari/Audio/AudioSocketServer.cs b/src/Verbara.Sdk.Ari/Audio/AudioSocketServer.cs index 1d9d2246..492365d2 100644 --- a/src/Verbara.Sdk.Ari/Audio/AudioSocketServer.cs +++ b/src/Verbara.Sdk.Ari/Audio/AudioSocketServer.cs @@ -49,7 +49,13 @@ public sealed class AudioSocketServer : IAudioServer, IAsyncDisposable /// Observable that emits each new audio stream when a connection is established. public IObservable OnStreamConnected => _streamSubject; - /// Get an active stream by channel ID. + /// + /// Get an active stream by the UUID Asterisk sent in its AudioSocket identification frame, in + /// canonical lowercase hyphenated form. The table is an ordinal dictionary, so any other + /// spelling of the same UUID returns . Over ARI that UUID is the value + /// the creator passed as data, and it is the created channel's id only if the creator + /// passed it as channelId too. See . + /// public IAudioStream? GetStream(string channelId) => _streams.TryGetValue(channelId, out var session) ? session : null; diff --git a/src/Verbara.Sdk.Ari/Audio/CompositeAudioServer.cs b/src/Verbara.Sdk.Ari/Audio/CompositeAudioServer.cs index e7b0b5cd..73a11aaa 100644 --- a/src/Verbara.Sdk.Ari/Audio/CompositeAudioServer.cs +++ b/src/Verbara.Sdk.Ari/Audio/CompositeAudioServer.cs @@ -5,6 +5,11 @@ namespace Verbara.Sdk.Ari.Audio; /// /// Aggregates multiple audio servers (AudioSocket + WebSocket) into a single IAudioServer view. /// +/// +/// and only merge, and are safe to read +/// as an aggregate. is the member to read carefully: it hands one +/// string to every server in turn, and the servers do not agree on what that string means. +/// public sealed class CompositeAudioServer : IAudioServer { private readonly IAudioServer[] _servers; @@ -17,6 +22,21 @@ public CompositeAudioServer(IEnumerable servers) public IObservable OnStreamConnected => _servers.Select(s => s.OnStreamConnected).Merge(); + /// + /// Try each aggregated server in construction order and return the first stream registered + /// under . + /// + /// + /// One string, two meanings. The aggregated servers key their tables on different + /// things: on the UUID Asterisk sends in its AudioSocket + /// identification frame — canonical lowercase hyphenated, compared ordinally — and + /// on the last path segment of the HTTP upgrade request URL. + /// Nothing arranges for one identifier to mean the same thing to both, so a value meant for one + /// server is simply a miss in the other, and this method reports a miss and a key in the wrong + /// form the same way: . A caller that knows which transport it uses gets + /// a narrower answer from that server directly. See + /// for the full description of each key. + /// public IAudioStream? GetStream(string channelId) { foreach (var server in _servers) diff --git a/src/Verbara.Sdk.Ari/Audio/WebSocketAudioServer.cs b/src/Verbara.Sdk.Ari/Audio/WebSocketAudioServer.cs index 082ad074..088df25e 100644 --- a/src/Verbara.Sdk.Ari/Audio/WebSocketAudioServer.cs +++ b/src/Verbara.Sdk.Ari/Audio/WebSocketAudioServer.cs @@ -60,7 +60,11 @@ public sealed class WebSocketAudioServer : IAudioServer, IAsyncDisposable /// Observable that emits each new audio stream when a connection is established. public IObservable OnStreamConnected => _streamSubject; - /// Get an active stream by channel ID. + /// + /// Get an active stream by the last path segment of the HTTP upgrade request URL it arrived on, + /// query string stripped — not by an ARI channel id, and not by the AudioSocket identification + /// UUID. See . + /// public IAudioStream? GetStream(string channelId) => _streams.TryGetValue(channelId, out var session) ? session : null; diff --git a/src/Verbara.Sdk.Ari/CompatibilitySuppressions.xml b/src/Verbara.Sdk.Ari/CompatibilitySuppressions.xml index c3333273..a8c30156 100644 --- a/src/Verbara.Sdk.Ari/CompatibilitySuppressions.xml +++ b/src/Verbara.Sdk.Ari/CompatibilitySuppressions.xml @@ -3,19 +3,39 @@ + + + + CP0002 + M:Verbara.Sdk.IAriChannelsResource.CreateExternalMediaAsync(System.String,System.String,System.String,System.String,System.String,System.String,System.String,System.String,System.Threading.CancellationToken) + lib/net10.0/Verbara.Sdk.dll + lib/net10.0/Verbara.Sdk.dll + true + + + CP0006 + M:Verbara.Sdk.IAriChannelsResource.CreateExternalMediaAsync(System.String,System.String,System.String,System.String,System.String,System.String,System.String,System.String,System.String,System.Threading.CancellationToken) + lib/net10.0/Verbara.Sdk.dll + lib/net10.0/Verbara.Sdk.dll + true + + \ No newline at end of file diff --git a/src/Verbara.Sdk/IAriClient.cs b/src/Verbara.Sdk/IAriClient.cs index 20aa40ba..ec446746 100644 --- a/src/Verbara.Sdk/IAriClient.cs +++ b/src/Verbara.Sdk/IAriClient.cs @@ -97,9 +97,34 @@ public interface IAriChannelsResource ValueTask AnswerAsync(string channelId, CancellationToken cancellationToken = default); /// Create an external media channel. POST /channels/externalMedia + /// + /// + /// channelId is the unique id Asterisk assigns the channel it creates; it comes back as + /// . Asterisk spells this query parameter in camelCase while + /// every sibling here is snake_case, and it ignores a parameter it does not recognise instead of + /// rejecting it — so a misspelling is a silent no-op, not an error status. Asterisk echoes the + /// value back verbatim and does not require it to be a UUID. + /// + /// + /// It is a different thing from data. Under AudioSocket encapsulation data is the + /// identification UUID Asterisk sends on the audio connection — the value an AudioSocket server + /// keys its stream table by, and it must parse as a UUID — while channelId only names the + /// ARI channel. Passing one identifier as both is what makes a stream findable by the channel id + /// this call returned. + /// + /// + /// Placed after data rather than first among the optionals, which is where + /// puts its own channelId. The inconsistency is + /// deliberate: inserting ahead of encapsulation would rebind every existing positional + /// argument to a different string? parameter and still compile, so callers would break + /// silently at runtime. Appending before the cancellation token breaks only callers that passed + /// that token positionally, and that break is a compile error. + /// + /// ValueTask CreateExternalMediaAsync(string app, string externalHost, string format, string? encapsulation = null, string? transport = null, string? connectionType = null, - string? direction = null, string? data = null, CancellationToken cancellationToken = default); + string? direction = null, string? data = null, string? channelId = null, + CancellationToken cancellationToken = default); /// Get a channel variable. GET /channels/{channelId}/variable ValueTask GetVariableAsync(string channelId, string variable, CancellationToken cancellationToken = default); @@ -664,7 +689,31 @@ public sealed class AriRtpStats [System.Diagnostics.CodeAnalysis.SuppressMessage("Naming", "CA1711:Identifiers should not have incorrect suffix", Justification = "IAudioStream is the correct domain name for this abstraction")] public interface IAudioStream : IAsyncDisposable { - /// Unique ID of the external media channel in Asterisk. + /// + /// The key this stream is registered under in its server's table. It is not, in general, + /// an ARI channel id: the two implementations derive it from different things, and a value that + /// is a valid key for one of them is meaningless to the other. + /// + /// + /// + /// AudioSocket. The UUID Asterisk sent in the AudioSocket identification frame, rendered + /// from the sixteen wire bytes as new Guid(bytes, bigEndian: true).ToString() — so always + /// in canonical lowercase hyphenated form, e.g. + /// cd98c592-9ce2-4093-afae-420d53629a4f. Over ARI that is the value the creator passed as + /// data to . It equals + /// only when the creator passed that same value as channelId + /// as well — which is what ExternalMediaActivity does. Passing data alone leaves + /// an Asterisk-minted uniqueid such as 1790244226.1, which + /// appears in no stream table. + /// + /// + /// WebSocket. Not an Asterisk identifier at all: the last segment of the HTTP upgrade + /// request's path with any query string removed — + /// path.TrimStart('/').Split('/').LastOrDefault()?.Split('?').FirstOrDefault(). It is + /// whatever the caller put at the end of the URL it dialled, so every connection that dials the + /// same URL yields the same key. + /// + /// string ChannelId { get; } /// Audio format (e.g., "slin16", "ulaw", "alaw"). @@ -701,7 +750,27 @@ public interface IAudioServer /// Observable that emits each new audio stream when a connection is established. IObservable OnStreamConnected { get; } - /// Get an active stream by channel ID. + /// + /// Get an active stream by the key its server registered it under. That key is not an + /// ARI channel id in general, and it is not interchangeable between implementations — a + /// CompositeAudioServer hands this one string to servers that do not agree on what it + /// means. + /// + /// + /// The registration key. It is the stream's and is + /// described in full there; the parameter keeps its old name for source compatibility, not + /// because the value is a channel id. In short — for AudioSocketServer: the UUID from + /// Asterisk's AudioSocket identification frame, in canonical lowercase hyphenated form and + /// compared ordinally, so any other spelling of the same UUID misses. For + /// WebSocketAudioServer: the last path segment of the HTTP upgrade request URL, query + /// string stripped. An ARI is a key for the first only when the + /// channel was created with that same value in both channelId and data, as + /// ExternalMediaActivity does; it is not a key for the second. + /// + /// + /// The stream registered under that key, or if there is none. A key in + /// the wrong form is indistinguishable from an absent stream, which is why the form matters. + /// IAudioStream? GetStream(string channelId); /// All currently active audio streams. diff --git a/src/Verbara.Sdk/PublicAPI.Unshipped.txt b/src/Verbara.Sdk/PublicAPI.Unshipped.txt index 9d3ab23a..040b360b 100644 --- a/src/Verbara.Sdk/PublicAPI.Unshipped.txt +++ b/src/Verbara.Sdk/PublicAPI.Unshipped.txt @@ -74,3 +74,5 @@ const Verbara.Sdk.VerbaraSemanticConventions.Events.MediaStarted = "asterisk.med const Verbara.Sdk.VerbaraSemanticConventions.Node.OriginId = "origin.node.id" -> string! const Verbara.Sdk.VerbaraSemanticConventions.Node.ReceiverId = "receiver.node.id" -> string! const Verbara.Sdk.VerbaraSemanticConventions.Tenant.Id = "tenant.id" -> string! +*REMOVED*Verbara.Sdk.IAriChannelsResource.CreateExternalMediaAsync(string! app, string! externalHost, string! format, string? encapsulation = null, string? transport = null, string? connectionType = null, string? direction = null, string? data = null, System.Threading.CancellationToken cancellationToken = default(System.Threading.CancellationToken)) -> System.Threading.Tasks.ValueTask +Verbara.Sdk.IAriChannelsResource.CreateExternalMediaAsync(string! app, string! externalHost, string! format, string? encapsulation = null, string? transport = null, string? connectionType = null, string? direction = null, string? data = null, string? channelId = null, System.Threading.CancellationToken cancellationToken = default(System.Threading.CancellationToken)) -> System.Threading.Tasks.ValueTask From 7e5b7089a51c6940f583be826b202ab2b6c3151a Mon Sep 17 00:00:00 2001 From: "Harol A. Reina H." Date: Thu, 24 Sep 2026 08:39:58 -0500 Subject: [PATCH 5/6] test(functional): pin the channel-id route against a real Asterisk, and harvest the residuals MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- CHANGELOG.md | 51 ++++ .../ExternalMediaChannelIdFunctionalTests.cs | 231 ++++++++++++++++ .../Verbara.Sdk.FunctionalTests.csproj | 1 + .../asterisk-config/extensions.conf | 13 +- .../.openspec.yaml | 2 + .../README.md | 6 + .../proposal.md | 252 ++++++++++++++++++ .../specs/audio-stream-observability/spec.md | 68 +++++ .../external-media-stream-routing/spec.md | 85 ++++++ .../specs/test-determinism/spec.md | 79 ++++++ .../tasks.md | 203 ++++++++++++++ .../tasks.md | 128 ++++++++- 12 files changed, 1109 insertions(+), 10 deletions(-) create mode 100644 Tests/Verbara.Sdk.FunctionalTests/Layer5_Integration/Audio/ExternalMediaChannelIdFunctionalTests.cs create mode 100644 openspec/changes/a-published-surface-is-one-something-measures/.openspec.yaml create mode 100644 openspec/changes/a-published-surface-is-one-something-measures/README.md create mode 100644 openspec/changes/a-published-surface-is-one-something-measures/proposal.md create mode 100644 openspec/changes/a-published-surface-is-one-something-measures/specs/audio-stream-observability/spec.md create mode 100644 openspec/changes/a-published-surface-is-one-something-measures/specs/external-media-stream-routing/spec.md create mode 100644 openspec/changes/a-published-surface-is-one-something-measures/specs/test-determinism/spec.md create mode 100644 openspec/changes/a-published-surface-is-one-something-measures/tasks.md diff --git a/CHANGELOG.md b/CHANGELOG.md index bd8b0dd9..96b6268c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,57 @@ All notable changes to this project will be documented in this file. ## [Unreleased] +### Fixed — BREAKING: `ExternalMediaActivity` could not reach an AudioSocket stream by any configuration (#N) + +`ExternalMediaActivity` accepts an `AudioSocketServer` in its constructor and then polls +`GetStream(Channel.Id)` for the stream. It could never hit. Measured against a real Asterisk 22.9.0 +and 23.4.1, which behave identically: + +- With the activity's **defaults**, `POST /channels/externalMedia` returns 200 and a **UnicastRTP** + channel. Nothing connects to the AudioSocket server, and the poll waits out its 30-second timeout. +- Setting `Encapsulation = "audiosocket"` returns `HTTP 400 transport must be 'tcp' for audiosocket + encapsulation`, because the activity sends `transport` only when a caller sets it. +- Supplying the transport too returns `HTTP 400 data can not be empty`. + +And underneath those, the identifiers never matched. Asterisk's `data` parameter becomes the UUID in +the identification frame — the value the stream table is keyed by — while `channelId` becomes the ARI +`Channel.Id`. A probe with two distinct, byte-order-asymmetric UUIDs settles which travels: + +```text +channelId = 0a1b2c3d-4e5f-6071-8293-a4b5c6d7e8f9 +data = f9e8d7c6-b5a4-3928-1706-f5e4d3c2b1a0 + +channel.id : 0a1b2c3d-4e5f-6071-8293-a4b5c6d7e8f9 +HEX : 01 00 10 f9 e8 d7 c6 b5 a4 39 28 17 06 f5 e4 d3 c2 b1 a0 +UUID on the wire == channelId ? False == data ? True +``` + +- **`CreateExternalMediaAsync` gains a `channelId` parameter** on `IAriChannelsResource` and + `AriChannelsResource`. Asterisk documents it as the unique id to assign the channel on creation, and + spells it camelCase among snake_case siblings — a misspelling is ignored silently, so the URL test + asserts the literal. +- **The activity derives its request from the server it was handed.** AudioSocket encapsulation + implies `transport = "tcp"`, and one freshly minted UUID goes to both `channelId` and `data`, so + `Channel.Id` **is** the key. The mint is canonical lowercase, and that is load-bearing: the table is + an ordinal dictionary keyed by `Guid.ToString()`, and an uppercase identifier creates a channel + whose stream cannot be found. +- **An `AudioSocketServer` supplied against a non-AudioSocket encapsulation is now refused** at the + start, instead of creating an RTP channel and reporting a connection failure thirty seconds later. +- **The contract text now says what the key is.** `IAudioServer.GetStream` said "Get an active stream + by channel ID" — on the interface and, verbatim, on both shipped implementations. It now names the + key and its form for each, including that `WebSocketAudioServer` keys on the last segment of the + request URL instead. That second mismatch is real and is tracked separately, with a probe as its + first task rather than a design. + +**What a consumer must do.** Recompile, and name the cancellation token if you passed it positionally. +The parameter was **appended** rather than placed first among the optionals, deliberately: every slot +from `encapsulation` to `data` is `string?`, so inserting ahead of them would have rebound each +argument one place over and gone on compiling — wrong values on the wire with no diagnostic anywhere. +Appending produces `CS1503`, which is loud. Anyone implementing `IAriChannelsResource` gains a member. + +**Migration guide:** [`docs/guides/externalmedia-channel-id-migration.md`](docs/guides/externalmedia-channel-id-migration.md) +— required by ADR-0028 for a minor that carries a breaking change. + ### Changed — BREAKING: AudioSocket spoke a protocol Asterisk does not, so neither server ever completed a handshake (#302) Both AudioSocket implementations read a **four**-byte frame header. Asterisk sends **three**: one byte diff --git a/Tests/Verbara.Sdk.FunctionalTests/Layer5_Integration/Audio/ExternalMediaChannelIdFunctionalTests.cs b/Tests/Verbara.Sdk.FunctionalTests/Layer5_Integration/Audio/ExternalMediaChannelIdFunctionalTests.cs new file mode 100644 index 00000000..897fcc6b --- /dev/null +++ b/Tests/Verbara.Sdk.FunctionalTests/Layer5_Integration/Audio/ExternalMediaChannelIdFunctionalTests.cs @@ -0,0 +1,231 @@ +namespace Verbara.Sdk.FunctionalTests.Layer5_Integration.Audio; + +using Verbara.Sdk.Activities.Activities; +using Verbara.Sdk.Ari.Audio; +using Verbara.Sdk.FunctionalTests.Infrastructure.Fixtures; +using Verbara.Sdk.FunctionalTests.Infrastructure.Helpers; +using FluentAssertions; +using Microsoft.Extensions.Logging; + +/// +/// The end-to-end claim of the externalmedia-returns-a-channel-id-that-finds-its-stream +/// change, measured rather than asserted: a real Asterisk creates a real externalMedia +/// channel, dials this process over AudioSocket, and the stream is found by the id the create call +/// handed back. +/// +/// +/// +/// Why a functional test and not a unit test. With a mocked IAriChannelsResource the +/// test chooses both the Channel.Id that comes back and the UUID its own fake client puts on +/// the wire, so GetStream(Channel.Id) can be made to hit with the defect fully present. That +/// closed loop is the whole subject of this change, so the claim can only be settled by a party that +/// is not the test: Asterisk mints the channel, Asterisk sends the identification frame, and the two +/// values are compared afterwards. The argument-capture unit test in +/// Verbara.Sdk.Activities.Tests pins what the activity sends; this pins what +/// Asterisk does with it. +/// +/// +/// The Stasis subscription is load-bearing, not boilerplate. Asterisk's externalMedia +/// validates only that app is non-empty — it never checks the application is registered — so +/// the create returns HTTP 200 against no subscriber at all. The channel then enters Stasis with +/// nobody listening, Asterisk hangs it up, and the server's table entry is gone well inside the +/// activity's 200 ms poll. A test written from the probe alone therefore times out and reads as the +/// fix not working. opens the ARI event WebSocket with +/// app=test-app, which is what registers it, so the client is connected before the +/// activity is started. +/// +/// +/// The activity is constructed with Encapsulation and nothing else. No +/// Transport, no Format: those are exactly the values the class under test is supposed +/// to derive. Supplying Transport = "tcp" here would make the test pass over a broken +/// ExternalMediaActivity, which is the failure mode this file exists to rule out. +/// +/// +/// Off the PR path (ADR-0051, ADR-0043): this lane runs in the merge queue, on the scheduled +/// matrix, and on a pull request only when it carries the ci:functional label. +/// +/// +[Collection("Functional")] +[Trait("Category", "Functional")] +public sealed class ExternalMediaChannelIdFunctionalTests : FunctionalTestBase +{ + /// + /// The Stasis application this test subscribes and creates against. It is the name + /// opens its event WebSocket with, and the two must agree: a + /// create naming an application nobody is subscribed to still returns HTTP 200 and then loses + /// the channel. + /// + private const string StasisApp = "test-app"; + + /// + /// The port the AudioSocket server binds on this host. Fixed rather than ephemeral because the + /// value travels to Asterisk inside external_host, and deliberately neither 19092 nor + /// 19093: those two are named by dialplan extensions 710 and 711 + /// (docker/functional/asterisk-config/extensions.conf) and are bound by + /// . + /// + private const int AudioSocketPort = 19094; + + /// + /// The name Asterisk resolves to the Docker host gateway. + /// AsteriskContainer maps it with WithExtraHost("host.docker.internal", + /// "host-gateway"); the server under test runs in this process, on the host, not in the + /// container. + /// + private const string HostGateway = "host.docker.internal"; + + /// + /// The encapsulation asked for, spelled as Asterisk's own error message spells it. Asterisk + /// compares it with strcasecmp, so the casing is not what is being tested here. + /// + private const string AudioSocketEncapsulation = "audiosocket"; + + /// + /// How long Asterisk has to connect and identify itself, used both as the activity's + /// ConnectionTimeout and as the server's idle deadline. Generous on purpose: this budget + /// only bounds a failure. chan_audiosocket makes its TCP connection during the + /// create call, so on the success path nothing waits at all. + /// + private static readonly TimeSpan ConnectionBudget = TimeSpan.FromSeconds(45); + + public ExternalMediaChannelIdFunctionalTests() + : base("Verbara.Sdk.Ari") + { + } + + /// + /// Creates an AudioSocket externalMedia channel through + /// against a real Asterisk and asserts that the stream the activity resolved is registered under + /// the id the create call returned, and that the UUID Asterisk put in its identification frame is + /// that same id. + /// + [Fact] + public async Task ExternalMediaActivity_ShouldResolveItsStreamByTheReturnedChannelId_WhenAsteriskConnectsOverAudioSocket() + { + // The wire's own account of the identifier, taken from the server rather than from the + // activity: OnStreamConnected fires when a session's ChannelId has been parsed out of + // Asterisk's identification frame, so this value is produced by Asterisk and is not + // anything this test chose. + var identified = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + + await using var server = new AudioSocketServer( + new AudioServerOptions + { + AudioSocketPort = AudioSocketPort, + ListenAddress = "0.0.0.0", + IdleTimeout = ConnectionBudget + }, + LoggerFactory.CreateLogger()); + + using var subscription = server.OnStreamConnected.Subscribe(stream => identified.TrySetResult(stream)); + + // Bound before the create, because chan_audiosocket connects during the create call: a + // create against a port nothing is listening on is HTTP 500, not a channel that connects + // later (probe-capture.txt, PART 3). + await server.StartAsync(); + + // Subscribes the Stasis application. See the class remarks — without this the create still + // succeeds and the channel is then hung up under the poll. + await using var ariClient = AriClientFactory.Create(LoggerFactory, application: StasisApp); + await ariClient.ConnectAsync(); + + await using var activity = new ExternalMediaActivity(ariClient, server) + { + App = StasisApp, + ExternalHost = $"{HostGateway}:{AudioSocketPort}", + + // The only thing configured beyond the two required properties. Transport, the + // identification UUID and the channel id are all the activity's own to derive, and a + // test that supplied them would measure its own arrange instead of the class. + Encapsulation = AudioSocketEncapsulation, + ConnectionTimeout = ConnectionBudget + }; + + // Recorded rather than thrown so the assertions below all run and each one carries its own + // reason. A bare await would end the test on Asterisk's exception with no statement of what + // was expected of it. + var failure = await Record.ExceptionAsync(async () => await activity.StartAsync()); + + // Read off the server, not off the activity, and reported in the first assertion's reason. + // A timeout here has two very different causes — Asterisk never connected, or it connected + // and registered under a key that is not the channel id — and only the registered keys tell + // them apart. Without this the two are one indistinguishable red. + var registered = server.ActiveStreams.Select(stream => stream.ChannelId).ToArray(); + + try + { + failure.Should().BeNull( + "an AudioSocket external media create is supposed to succeed and resolve its stream " + + "end to end; instead StartAsync ended with {0}, with the returned channel id {1} " + + "and these streams registered on the server: [{2}] — a timeout with a stream " + + "registered under some other key is the defect itself, the two identifiers having " + + "come apart", + failure?.ToString() ?? "no exception", + activity.Channel?.Id ?? "no channel", + registered.Length == 0 ? "none" : string.Join(", ", registered)); + + activity.Channel.Should().NotBeNull( + "a successful create returns the channel Asterisk made, and every assertion below " + + "compares against its id"); + + activity.AudioStream.Should().NotBeNull( + "the activity resolves its stream by calling GetStream(Channel.Id) on the server it " + + "was handed, so a null here is that lookup never hitting — the exact defect this " + + "change fixes, where the table was keyed by the wire UUID and searched by an " + + "Asterisk-minted channel id"); + + activity.AudioStream!.ChannelId.Should().Be( + activity.Channel!.Id, + "the stream's ChannelId is the key its server registered it under, and the claim of " + + "this change is that an ARI channel id is that key: one identifier is sent as both " + + "channelId and data, so Channel.Id and the registration key are the same string"); + + var wireStream = await WithinAsync(identified.Task, ConnectionBudget); + + wireStream.Should().NotBeNull( + "a real Asterisk connected to this server during the create call and must have sent " + + "its AudioSocket identification frame; a null here is that frame never arriving"); + + wireStream!.ChannelId.Should().Be( + activity.Channel!.Id, + "this value was parsed out of the sixteen bytes Asterisk sent, not out of anything " + + "this test chose, and it is what settles the claim: the identification UUID and " + + "the id the create call handed back are one value"); + + activity.Channel!.Id.Should().Be( + activity.Channel.Id.ToLowerInvariant(), + "Asterisk echoes channelId back verbatim and does not normalise it (probe-capture.txt, " + + "RUN U), while AudioSocketSession.ParseUuid renders the wire bytes with " + + "Guid.ToString() into an ordinal dictionary — so only a canonical lowercase " + + "identifier can ever be found again"); + + Guid.TryParseExact(activity.Channel!.Id, "D", out _).Should().BeTrue( + "the identifier the activity minted has to be a canonical hyphenated UUID for " + + "Asterisk to accept it as data at all: a value that does not parse is HTTP 500 at " + + "the create with the listener up (probe-capture.txt, correction #4)"); + } + finally + { + if (activity.Channel is not null) + await BestEffort.AriAsync(() => ariClient.Channels.HangupAsync(activity.Channel.Id)); + } + } + + /// + /// Awaits and returns if it does not complete + /// within , so the failure is an assertion with a reason rather than a + /// bare . No wall-clock barrier: the timeout is the task's own + /// deadline, not a sleep the test races (ADR-0004 / ADR-0045). + /// + private static async Task WithinAsync(Task task, TimeSpan timeout) where T : class + { + try + { + return await task.WaitAsync(timeout); + } + catch (TimeoutException) + { + return null; + } + } +} diff --git a/Tests/Verbara.Sdk.FunctionalTests/Verbara.Sdk.FunctionalTests.csproj b/Tests/Verbara.Sdk.FunctionalTests/Verbara.Sdk.FunctionalTests.csproj index 7b15f35c..de32582a 100644 --- a/Tests/Verbara.Sdk.FunctionalTests/Verbara.Sdk.FunctionalTests.csproj +++ b/Tests/Verbara.Sdk.FunctionalTests/Verbara.Sdk.FunctionalTests.csproj @@ -23,6 +23,7 @@ + diff --git a/docker/functional/asterisk-config/extensions.conf b/docker/functional/asterisk-config/extensions.conf index d10fc20f..62a40522 100644 --- a/docker/functional/asterisk-config/extensions.conf +++ b/docker/functional/asterisk-config/extensions.conf @@ -107,8 +107,17 @@ exten => 950,1,Answer() ; `if (confJoin is null) return;` branch and is reported as passed without asserting anything. ; Defining 700 here made the originate succeed, so those five tests ran their bodies for the ; first time and failed. Renumbering keeps that pre-existing hole exactly as it was instead of -; letting this change carry someone else's red; the hole is filed separately. Extensions dialed -; by a test but absent from this context: 160, 300, 700, 999, 9998. +; letting this change carry someone else's red. +; +; The hole is filed, with a link rather than the words "filed separately" — writing that phrase with +; nothing to follow is how ADR-0060 lost its own residuals, and this comment did it too until it was +; corrected: openspec/changes/a-published-surface-is-one-something-measures. +; +; Ten extensions are dialed by a test and absent from this context, not the five this comment first +; claimed: 160, 161, 162, 163, 300, 700, 750, 999, 9998, 9999. The first count missed the tests that +; reach the context through an Exten/Context pair instead of Local/N@test-functional. The thing to +; hunt is not the numbers, though — it is the unasserting early return that makes a test pass when +; its dial goes nowhere. exten => 710,1,NoOp(AudioSocket: Verbara.Sdk.Ari audio server) same => n,Answer() same => n,AudioSocket(4f1d9c60-7a2b-4e55-9f3d-2c6a8b0e1d47,host.docker.internal:19092) diff --git a/openspec/changes/a-published-surface-is-one-something-measures/.openspec.yaml b/openspec/changes/a-published-surface-is-one-something-measures/.openspec.yaml new file mode 100644 index 00000000..75289e4b --- /dev/null +++ b/openspec/changes/a-published-surface-is-one-something-measures/.openspec.yaml @@ -0,0 +1,2 @@ +schema: spec-driven +created: 2026-09-24 diff --git a/openspec/changes/a-published-surface-is-one-something-measures/README.md b/openspec/changes/a-published-surface-is-one-something-measures/README.md new file mode 100644 index 00000000..c1fbf132 --- /dev/null +++ b/openspec/changes/a-published-surface-is-one-something-measures/README.md @@ -0,0 +1,6 @@ +# a-published-surface-is-one-something-measures + +Three surfaces this SDK publishes are connected to nothing that could tell whether they work: the +WebSocket audio server's lookup key, the ten audio metric instruments, and the functional tests that +dial extensions the dialplan never defines. Each is kept green by a test that cannot see the +difference. Measure first, then fix — and in the WebSocket case the measurement does not exist yet. diff --git a/openspec/changes/a-published-surface-is-one-something-measures/proposal.md b/openspec/changes/a-published-surface-is-one-something-measures/proposal.md new file mode 100644 index 00000000..5aff95d5 --- /dev/null +++ b/openspec/changes/a-published-surface-is-one-something-measures/proposal.md @@ -0,0 +1,252 @@ +--- +tier: MEDIANO +owner: Harol +approver: Harol +stakeholder: Anyone reading this SDK's audio documentation, metrics or functional results as a statement of what works — and the Pro AgentAssist deployment that consumes the ARI audio path +decision_ref: Sdk/ADR-0060 +--- + +# Proposal: a-published-surface-is-one-something-measures + +## Why + +ADR-0060 recorded two residual defects with the words "tracked separately" and no link, and the +close-out archived it anyway. `externalmedia-returns-a-channel-id-that-finds-its-stream` exists +because of that, fixes the third defect, and hands three more findings forward. This is the change +that carries them, opened before that one archives rather than after, because +`openspec/config.yaml` requires a deferred finding to land in an open change and a label such as +*tracked separately* with no link does not exempt it. + +The three are not a grab-bag. They are one failure repeated on three different kinds of surface: + +> **The repository publishes a claim, nothing connects the claim to the code, and the test that +> should tell the difference would stay green if the code were deleted.** + +- The **documentation** claims `WebSocketAudioServer` is keyed by a channel id. It is keyed by a URL + path segment, and nothing has ever measured what Asterisk puts in that path. +- The **metrics surface** claims ten instruments' worth of audio observability. Nothing under `src/` + emits any of them. The tests emit the measurements themselves and then assert they were observed. +- The **functional suite** claims to exercise ConfBridge, Parking, Transfer, DTMF, CDR and Stasis. + Ten of the extensions it dials are not defined in the context it dials them in, and the tests + return without asserting when the call does not happen. + +Every number below was measured in this worktree with `git grep` and by reading the files, because +this change's parent measured that the session's default `grep` honours `.gitignore` and undercounts. +Where a measurement contradicts the note that handed the finding over, the correction is stated here +rather than quietly absorbed. + +### F1 — `WebSocketAudioServer` keys on a URL path segment, and nobody has measured that path + +The key is computed at `src/Verbara.Sdk.Ari/Audio/WebSocketAudioServer.cs:341`: + +```csharp +var channelId = path.TrimStart('/').Split('/').LastOrDefault()?.Split('?').FirstOrDefault(); +``` + +and registered at `:266` (`_streams.TryAdd(channelId, session)`). So the table's key is whatever the +HTTP upgrade request's last path segment happens to be. This SDK's own example puts a literal there: + +```text +Examples/WebSocketMediaExample/Program.cs:10 // same => n,WebSocket(ws://127.0.0.1:9093/audio,slin16) +Examples/WebSocketMediaExample/Program.cs:78 Console.WriteLine("Configure Asterisk ... WebSocket(ws://host:9093/audio,slin16) ..."); +Examples/WebSocketMediaExample/README.md:17 same => n,WebSocket(ws://127.0.0.1:9093/audio,slin16) +``` + +Follow that example and every concurrent call registers under the key `"audio"`: the second +connection's `TryAdd` fails, the second stream is never in the table, and `ActiveStreamCount` +disagrees with reality. `CompositeAudioServer.GetStream` hands one string to both servers, which do +not agree on what a key means — its own `` now says so, which is a warning, not a fix. + +**The first task of this change is a probe, not a design.** Nothing in this repository has measured +what Asterisk actually puts in that request path, and there are two candidate producers that the code +does not distinguish between: + +- `WebSocketAudioServer`'s class summary says *"Listens for incoming WebSocket connections from + Asterisk ExternalMedia channels"* (`:34`) — the ARI `externalMedia` endpoint with + `transport=websocket`; +- the example and `ChanWebSocketControlMessage` / `IChanWebSocketSession` describe the `WebSocket()` + dialplan application of `chan_websocket`, an entirely different Asterisk feature that this repo also + supports. + +Those two put different things on the wire, and `AudioChannelVars` already names a third candidate +identifier for the second of them — `WEBSOCKET_GUID` — that the server never reads. Designing a fix +from a reading of the code is precisely how the AudioSocket wire format survived six months of green +tests. The probe comes first and its capture is committed beside this change, in the shape +`probe-capture.txt` takes for its parent. + +Three pieces of prose in the same file assert the identity the code does not hold, all of them +carried over rather than fixed by the parent change's B3, which deliberately stopped at published +contracts: + +- `:312` — "extract Sec-WebSocket-Key and channel ID from URL path"; +- `:313` — "Expected URL: `/ws/{channelId}` or `/{channelId}`", naming the placeholder `{channelId}`; +- `:34` — the class summary's "from Asterisk ExternalMedia channels", which may simply be false. + +And incidentally, `:35` cites "(ADR-1)" for the TcpListener + manual-upgrade design. This repository's +ADRs are four digits and `ADR-0001` is *Native AOT first*, which is not that decision. It is a +citation to nothing, in the one file this change opens anyway. + +**Corrections to the note that handed F1 over:** the key is computed at `:341`, not `:337`, and +registered at `:266`, not `:262`. The literal `/audio` appears in three places, not one. + +### F2 — ten instruments, zero production call sites + +`src/Verbara.Sdk.Ari/Diagnostics/AudioStreamMetrics.cs` declares a `Meter` and ten instruments: +`StreamsOpened`, `StreamsClosed`, `FramesReceived`, `FramesSent`, `BytesReceived`, `BytesSent`, +`BufferUnderruns`, `HangupFrames`, `ErrorFrames`, `FrameLatency`. Verified with `git grep`, every +reference to the type outside its own declaration is one of: + +- `Tests/Verbara.Sdk.Ari.Tests/Diagnostics/AudioStreamMetricsTests.cs` — 13 references; +- `src/Verbara.Sdk.Ari/PublicAPI.Shipped.txt` — 12 rows; +- the two OpenSpec documents that record the finding. + +`git grep -n "AudioStreamMetrics\."` returns exactly two hits for nine of the ten instruments +and three for `StreamsClosed` — in every case the `PublicAPI.Shipped.txt` row plus the test file, and +never a call site under `src/`. The declaration itself does not appear in that count, because it does +not use the qualified name; there is nothing else to count. The class-level doc tells a reader to run `dotnet-counters monitor --process-id Verbara.Sdk.Ari.Audio` +and watch. They will watch nothing move, for the lifetime of any process. + +**Its test file is the closed loop in miniature, which is why this was never caught.** Eleven `[Fact]`s: +six assert only `.Should().NotBeNull()` on a `static readonly` field initialised inline — which cannot +be null and would be a compile error if it could; four construct a `MeterListener`, call +`AudioStreamMetrics.X.Add(1)` *from the test itself*, and assert the listener observed 1; one asserts +the meter's name. Delete every production call site and all eleven stay green. That is not a +hypothetical: every production call site is already deleted, and they are green today. + +The fix is a decision, not a foregone conclusion, and this change makes the decision explicitly: +**wire the instruments to the sessions that own the events, or remove them.** Removal is breaking — +twelve rows in `PublicAPI.Shipped.txt`, `CP0002`/`CP0006`, a suppressions entry, a migration note. +Wiring is not breaking and is the likelier answer, but it is the *answer to a question*, and asking it +is the task. + +### F3 — the functional suite passes on calls that never happened + +`docker/functional/asterisk-config/extensions.conf` defines nine extensions in `[test-functional]`: +100, 150, 155, 500, 600, 710, 711, 900, 950. There is no pattern-match extension in that context — the +`_X.` catch-alls live in `[default]`, `[stasis-test]` and `[queue-test]` — so an extension that is not +listed is not reachable. + +The suite dials seventeen distinct extensions into that context, in **two** forms that a single grep +does not both catch: + +| form | distinct extensions | +|---|---| +| `Channel = "Local/N@test-functional"` | 100, 150, 155, 160, 300, 500, 600, 700, 900, 950, 999, 9998 | +| `Exten = "N"` with `Context = "test-functional"` (Originate and Redirect) | 100, 150, 155, 160, 161, 162, 163, 500, 750, 950, 999, 9998, 9999 | + +Subtracting the nine defined leaves **ten** undefined, not five: +**160, 161, 162, 163, 300, 700, 750, 999, 9998, 9999.** The note that handed this finding over said +five, and so does the comment already sitting in the dialplan at `:111`; both were derived from the +`Local/N@` form alone. `Exten =` appears 53 times in the functional suite and every one of those 53 +sites carries `Context = "test-functional"`, so the second form is not an edge case. + +**What keeps it green is an early return, and it has more than one spelling.** +`Tests/Verbara.Sdk.FunctionalTests/Layer5_Integration/ConfBridge/ConfBridgeAdvancedTests.cs` dials the +undefined 700 from ten call sites across eight `[Fact]`s, and every one of the eight ends in an +unasserting return written three different ways: + +```text +:55, :104, :162 if (confJoin is null) return; +:211, :273 if (!joinEvents.Any(e => e.Conference == confName)) return; +:339, :385, :445 if (joinEvents.IsEmpty) return; +``` + +So the shape to hunt is the early return, and hunting one spelling of it finds three of eight tests. +The parent change's own dialplan comment (`extensions.conf:104-111`) says *"those five tests"* and +names one spelling; measured, it is eight tests and three spellings. That comment also ends *"the hole +is filed separately"* and links nothing — the ADR-0060 failure reproduced, in the very commit that +was fixing ADR-0060's failure. This change is the link. + +**The shape is not confined to ConfBridge.** A brace-tracking scan of the functional suite for a bare +`return;` in a test-method body — excluding returns inside lambdas and observer callbacks — reports +**71 candidate sites across 11 files**. Hand-checked samples confirm the shape outside ConfBridge: + +```csharp +// Tests/Verbara.Sdk.FunctionalTests/Layer5_Integration/Parking/ParkingTests.cs:61-65 +var channelResult = await Task.WhenAny(originatedChannel.Task, Task.Delay(TimeSpan.FromSeconds(10))); +if (channelResult != originatedChannel.Task) +{ + // Channel did not appear — skip gracefully + return; +} +``` + +`ParkingTests` redirects to the undefined 750 from four sites, so "skip gracefully" is its normal path, +not its safety net. `DtmfDetectionTests:123` and `AriStasisTests:101,:111` carry the same construction. +The scan is a **candidate list and not a verdict**: `BridgeLifecycleTests:315` is a false positive — +a legitimate event filter inside a `ConfbridgeJoinObserver` lambda. Producing the true count by hand is +a task of this change, not a claim of this proposal. + +## What Changes + +**F1 — measure, then decide.** +- A probe, run against both Asterisk images the lane builds, capturing the literal HTTP request line + Asterisk sends for (a) `externalMedia` with `transport=websocket` and (b) the `WebSocket()` dialplan + application, plus the value of `WEBSOCKET_GUID` where one exists. The capture is committed. +- Only then: either the server keys on a measured identifier, or its contract says plainly that it is + not addressable by any Asterisk id and `CompositeAudioServer`'s ambiguity is resolved by one of them + refusing the call. +- The three wrong sentences at `WebSocketAudioServer.cs:34`, `:312`, `:313` are corrected against the + capture, and the dangling "(ADR-1)" at `:35` is resolved or removed. + +**F2 — wire the instruments or remove them.** One decision, recorded, with the public-API cost of the +removal branch priced before it is chosen. Whichever branch is taken, the test file is rewritten so it +fails when the production call sites are removed — a test that supplies its own measurement is not +evidence that anything is instrumented. + +**F3 — define, assert, and sweep.** +- Every extension the suite dials into `[test-functional]` is defined there, or the test that dials it + is changed to dial one that is. +- The unasserting early return is replaced by a failing assertion with a reason, everywhere the sweep + finds it — and the sweep produces a hand-verified count, not the heuristic's 71. +- A guard so the class cannot come back: the suite's dialled set and the dialplan's defined set are + reconciled by something that runs, rather than by a comment in a `.conf` file. + +## Decision: one change, with a written split condition + +These are three findings on three different parts of the tree, and the honest answer is not obvious. +Both readings are set out here, and the change picks one with a condition attached rather than leaving +the choice to whoever opens the file next. + +**For one change.** They are one failure family, and the family is the point. Splitting them into three +tickets produces three fixes and loses the sentence that connects them — which is the sentence a +reviewer needs in order to catch the fourth instance. `openspec/config.yaml` requires the harvest to +land in *an* open change; one change satisfies it and names the family once. And the parent change +already paid for the discovery; re-deriving the shared thesis three times is waste. + +**Against one change.** F1 blocks on a measurement nobody has taken and may end in a breaking API +change; F2 is a shipped-public-API decision; F3 touches no shipped surface at all. Binding two findings +that are ready to fix today behind an open-ended probe is the *tracked separately* failure in a new +costume: a change that cannot start is not much better than prose that nobody links. + +**The decision: one change, ordered F3 → F2 → F1, with this split condition written into `tasks.md`.** +If the F1 probe has not produced a committed capture after one honest attempt against both Asterisk +images the lane builds, F1 splits into its own change — carrying the probe task verbatim — and this +change archives with F2 and F3 complete. The condition is concrete enough to be checked rather than +argued, which is what the parent change's post-mortem asked for. The ordering is deliberate: the two +findings with no unknowns land first, so the unknown cannot hold them. + +## Impact + +- **F1:** `src/Verbara.Sdk.Ari/Audio/WebSocketAudioServer.cs`, `Audio/CompositeAudioServer.cs`, + `Examples/WebSocketMediaExample/`, and the `IAudioServer` / `IAudioStream` contract text in + `src/Verbara.Sdk/IAriClient.cs` that the parent change has just rewritten. A new probe and its + capture. Whether any shipped signature moves is the probe's to decide; if it does, the cost is the + parent change's cost again — `CP0002`, `PublicAPI` rows, a suppressions entry, a migration note. +- **F2:** `src/Verbara.Sdk.Ari/Diagnostics/AudioStreamMetrics.cs` and the sessions that would emit — + `Audio/AudioSocketSession.cs`, `Audio/WebSocketAudioSession.cs`. **Potentially breaking**: the removal + branch drops twelve rows from `src/Verbara.Sdk.Ari/PublicAPI.Shipped.txt` and takes `CP0002`. The + wiring branch is additive and takes none. `Tests/Verbara.Sdk.Ari.Tests/Diagnostics/AudioStreamMetricsTests.cs` + is rewritten either way. +- **F3:** `docker/functional/asterisk-config/extensions.conf` and the functional suite. No shipped + surface. **Expect new red.** The parent change measured that defining 700 makes the originate succeed + and the ConfBridge test bodies then fail; that red belongs to this change and is the reason the + parent renumbered to 710/711 rather than carrying it. Budget for it in the task, rather than + discovering it. +- **CI:** F3's work is invisible on a `pull_request` without the `ci:functional` label (ADR-0051), and + only `merge_group` runs both Asterisk versions. A sixteen-second green functional job means no + Asterisk started. F1's probe depends on the same images. +- **Depends on** `externalmedia-returns-a-channel-id-that-finds-its-stream`: its delta creates the + `external-media-stream-routing` capability and explicitly excludes the WebSocket transport "until its + key has been measured". F1's delta adds to that capability and removes the exclusion, so this change + archives after its parent. diff --git a/openspec/changes/a-published-surface-is-one-something-measures/specs/audio-stream-observability/spec.md b/openspec/changes/a-published-surface-is-one-something-measures/specs/audio-stream-observability/spec.md new file mode 100644 index 00000000..65a82a6c --- /dev/null +++ b/openspec/changes/a-published-surface-is-one-something-measures/specs/audio-stream-observability/spec.md @@ -0,0 +1,68 @@ +# audio-stream-observability Delta + +## ADDED Requirements + +### Requirement: A published instrument SHALL be emitted by the code it names +Every instrument this SDK publishes on a `Meter` SHALL have at least one production call site that +records it, in the component its name describes, and an instrument with no production call site MUST +NOT remain published. Removing it and wiring it are both acceptable answers; leaving it is not, +because a published instrument is read as a statement that the component reports that quantity. + +Where the published documentation tells a reader to watch the meter with a named tool, that +instruction SHALL be true of a running process. A reader who follows it and sees nothing move has +been told something false by a tracked file. + +The choice between wiring and removal SHALL be recorded with the cost of the removal branch stated — +the rows it drops from `PublicAPI.Shipped.txt`, the ApiCompat codes it raises and the migration note +it obliges — so that the cheaper branch is chosen knowingly rather than by default. + +#### Scenario: An instrument nobody records +- **GIVEN** an instrument declared on a published meter +- **WHEN** no source file outside its own declaration and its tests records a measurement to it +- **THEN** it is either recorded by the component it names, or removed from the published surface + +#### Scenario: The documented way to observe the meter +- **GIVEN** documentation instructing a reader to monitor the meter by name +- **WHEN** a process runs the component the instruments describe +- **THEN** the instruments the documentation names move + +### Requirement: A test for an instrument SHALL fail when the production emission is removed +A test cited as evidence that a component is instrumented SHALL drive the component and assert the +measurement the component emitted, and MUST NOT supply the measurement itself. A test that records to +an instrument and then asserts a listener observed that record measures the metrics library, not this +SDK, and SHALL NOT be counted as coverage of the instrument. + +An assertion that an instrument field is non-null SHALL NOT be counted as coverage of anything: a +`static readonly` field initialised at its declaration cannot be null, so the assertion holds in every +possible state of the program, including the state where nothing is instrumented at all. + +#### Scenario: The witness fails when the call site goes +- **GIVEN** a test cited as evidence that a component records an instrument +- **WHEN** the production call site is removed +- **THEN** that test fails + +#### Scenario: A self-supplied measurement is not a witness +- **GIVEN** a test that calls `Add` or `Record` on the instrument itself and asserts a listener saw it +- **WHEN** the instrument has no production call site +- **THEN** the test still passes, and is therefore not evidence for the instrument + +## Architectural Risk + +**Level:** LOW on the wiring branch, MEDIUM on the removal branch. + +**Affected:** `Verbara.Sdk.Ari` — `Diagnostics/AudioStreamMetrics.cs` and the sessions that would +emit, `Audio/AudioSocketSession.cs` and `Audio/WebSocketAudioSession.cs` — plus +`Tests/Verbara.Sdk.Ari.Tests/Diagnostics/AudioStreamMetricsTests.cs`, which is rewritten on either +branch. Nothing downstream consumes these instruments today, for the reason this requirement exists: +they have never emitted. + +Wiring is additive and breaks nothing. Removal drops twelve rows from +`src/Verbara.Sdk.Ari/PublicAPI.Shipped.txt` and raises `CP0002`, which cascades to `Verbara.Sdk.Pro` +and `Verbara.Platform` as a recompile even though no consumer can be using a counter that never +moved. The asymmetry is the reason the decision is a task rather than an assumption. + +**Mitigation:** the rewritten tests are written against the branch chosen, before it lands, and the +negative control is the removal of a production call site rather than the breaking of an expected +value — the distinction the sibling change measured to be the difference between a test that is wired +and a test that can see the defect. If removal is chosen, the break is declared in a committed +suppressions entry with each line read before it is kept, and carries a migration note. diff --git a/openspec/changes/a-published-surface-is-one-something-measures/specs/external-media-stream-routing/spec.md b/openspec/changes/a-published-surface-is-one-something-measures/specs/external-media-stream-routing/spec.md new file mode 100644 index 00000000..0473b072 --- /dev/null +++ b/openspec/changes/a-published-surface-is-one-something-measures/specs/external-media-stream-routing/spec.md @@ -0,0 +1,85 @@ +# external-media-stream-routing Delta + +## ADDED Requirements + +### Requirement: The WebSocket audio path's key SHALL be measured before it is designed +The identifier under which a WebSocket audio stream is registered SHALL be established from bytes +captured off a real Asterisk, and a design for routing that path MUST NOT be derived from a reading +of this repository's own code. The capture SHALL record the literal HTTP request line Asterisk sends, +for each Asterisk feature that can reach this server — the ARI `externalMedia` endpoint with +`transport=websocket`, and the `WebSocket()` dialplan application of `chan_websocket` — because the +server does not distinguish between them today and they need not agree. + +Where Asterisk publishes an identifier for the connection by another route, such as a channel +variable, the capture SHALL record its value alongside the request path, so the choice of key is made +between measured candidates rather than between remembered ones. + +Until that capture exists, the published contract SHALL continue to state that this path is not +addressable by an Asterisk identifier, rather than naming its key a channel id. + +#### Scenario: A capture precedes the design +- **GIVEN** a proposal to route the WebSocket audio path by some identifier +- **WHEN** no capture of what Asterisk puts in the upgrade request exists +- **THEN** the design is not accepted, and the probe is the work that runs instead + +#### Scenario: Two producers are distinguished +- **GIVEN** a server reachable both by `externalMedia` with `transport=websocket` and by the `WebSocket()` dialplan application +- **WHEN** the request path is captured +- **THEN** each producer's path is recorded separately +- **AND** a difference between them is carried into the contract rather than averaged away + +### Requirement: A stream table SHALL NOT be keyed on a value every connection shares +A server that registers streams under a key taken from the connection SHALL take it from something +the connection is free to make unique, and MUST NOT silently drop a registration whose key is already +present. Where the documented way of configuring the far end produces the same key for every call — +a literal path segment in a dialplan line, for instance — that configuration SHALL be corrected in +the same change as the contract, including in this repository's own examples. + +A registration that loses to an existing key SHALL be reported, not discarded, because a server whose +active-stream count disagrees with the number of live connections is reporting a number no consumer +can act on. + +#### Scenario: Two concurrent calls configured the documented way +- **GIVEN** the dialplan line this repository's example publishes, used for two calls at once +- **WHEN** both connect +- **THEN** both streams are addressable, or the second connection is refused with a reason +- **AND** the stream count equals the number of live connections either way + +#### Scenario: The example stops teaching the collision +- **GIVEN** an example whose dialplan line yields one key for every call +- **WHEN** the key's meaning is settled by the capture +- **THEN** the example is corrected in every place it repeats that line, and says what the segment must contain + +### Requirement: An aggregate over servers with different keyspaces SHALL say which server answered +An aggregate that forwards one lookup key to servers that key on different things SHALL NOT report +"no such stream" and "that key belongs to another server's keyspace" as the same answer. It SHALL +either resolve the ambiguity — by routing the key to the server whose keyspace it belongs to — or +expose the per-server result, so a caller can tell a miss from a category error. + +#### Scenario: A key from the wrong keyspace +- **GIVEN** an aggregate over an AudioSocket server and a WebSocket server +- **WHEN** a caller passes an identifier only one of them could ever hold +- **THEN** the caller can distinguish "that stream is not connected" from "that identifier means nothing to the server that would hold it" + +## Architectural Risk + +**Level:** MEDIUM + +**Affected:** `Verbara.Sdk.Ari` (`Audio/WebSocketAudioServer.cs`, `Audio/CompositeAudioServer.cs`), +the `IAudioServer` / `IAudioStream` contract text in `Verbara.Sdk`, `Examples/WebSocketMediaExample`, +and any downstream consumer that reaches a stream through `CompositeAudioServer` — which includes the +`Verbara.Sdk.Pro` AgentAssist path. + +MEDIUM rather than LOW because the outcome is not known before the probe runs. If the measured key +turns out to be something the server must be told rather than something it can read, the fix reaches a +shipped signature and costs what its sibling change cost: `CP0002`, `PublicAPI` rows, a committed +suppressions entry and a migration note. If it turns out the path simply is not addressable, the fix +is contract text and an example, and costs nothing. Pricing that fork before the capture exists is +guessing, and this capability forbids it. + +**Mitigation:** the probe is the first task and its capture is committed beside the change, in the +shape the sibling change's `probe-capture.txt` takes, with the full request recorded rather than +described. The two Asterisk versions the lane builds are both probed, because the merge queue is the +only place both run and it is the wrong place to learn of a difference. The change declares in writing +that it splits if the probe does not land, so the two findings travelling with it are not held behind +an unknown. diff --git a/openspec/changes/a-published-surface-is-one-something-measures/specs/test-determinism/spec.md b/openspec/changes/a-published-surface-is-one-something-measures/specs/test-determinism/spec.md new file mode 100644 index 00000000..46658965 --- /dev/null +++ b/openspec/changes/a-published-surface-is-one-something-measures/specs/test-determinism/spec.md @@ -0,0 +1,79 @@ +# test-determinism Delta + +## ADDED Requirements + +### Requirement: A functional test SHALL fail when the call it originates never happens +A functional test SHALL treat the absence of the channel, event or call it originated as a failure +with a reason, and MUST NOT return from the test body without asserting. An originate that does not +land is the condition the test exists to detect; a test that exits quietly on it reports success for +the one case it was written for. + +This applies to every spelling of the early exit, not to one: a null check on the awaited event, an +emptiness check on a collected event list, a predicate over collected events, and a comparison of +`Task.WhenAny` against the task that was supposed to win are the same construction, and a sweep that +matches only the first finds a fraction of them. The sweep's result SHALL be a hand-verified count, +because the mechanical scan cannot tell a test-body exit from a filter inside an event-observer +lambda, and the lambda form is correct. + +A comment describing the exit as skipping gracefully SHALL NOT stand in for a skip: a skipped test +is reported as skipped, and a test that returns is reported as passed. + +#### Scenario: The originate does not land +- **GIVEN** a functional test that originates a call and waits for the event it asserts on +- **WHEN** the event does not arrive within the test's timeout +- **THEN** the test fails, naming the call it originated and the event it waited for + +#### Scenario: Every spelling of the exit is found +- **GIVEN** a suite in which the unasserting exit is written several different ways +- **WHEN** the suite is swept for it +- **THEN** the sweep covers the shape rather than one phrasing, and its count is verified by reading each site + +### Requirement: The functional dialplan SHALL define every extension the suite dials +Every extension the functional suite dials into a dialplan context SHALL be defined in that context, +or the test that dials it SHALL be changed to dial one that is. The reconciliation SHALL be performed +by something that executes — a guard, a test, or a check on the validation run — and MUST NOT be +recorded only as a comment in a configuration file, because a comment does not fail. + +The set of dialled extensions SHALL be collected from every form the suite uses to reach the +dialplan, including both a channel string naming an extension and a context-plus-extension pair on an +originate or a redirect. A reconciliation derived from one form under-reports, and an under-report +here reads exactly like a clean result. + +A context with no pattern-match extension SHALL NOT be assumed to absorb an undefined number: an +extension that is not listed in such a context is not reachable, and the originate fails. + +#### Scenario: An extension dialled by the suite and absent from the context +- **GIVEN** a functional test dialling an extension into a context that does not define it +- **WHEN** the reconciliation runs +- **THEN** it fails, naming the extension and the test that dials it + +#### Scenario: The second dialling form is counted +- **GIVEN** a suite that reaches the dialplan both by a channel string and by a context-plus-extension pair +- **WHEN** the dialled set is collected +- **THEN** both forms are included, and the reconciliation is over their union + +#### Scenario: Defining the missing extension turns a vacuous pass into a real result +- **GIVEN** tests that have been passing by exiting early because their originate never landed +- **WHEN** the extension is defined and the early exit is replaced by an assertion +- **THEN** those tests run their bodies for the first time, and whatever they then report is the first real result they have ever produced + +## Architectural Risk + +**Level:** LOW on the shipped surface, MEDIUM on the validation run. + +**Affected:** `docker/functional/asterisk-config/extensions.conf` and +`Tests/Verbara.Sdk.FunctionalTests/`. No `src/` file and no published signature changes, so nothing +cascades to `Verbara.Sdk.Pro` or `Verbara.Platform`. + +The risk is not to consumers, it is to the validation run: tests that have been reporting success +without executing their bodies will execute them, and some will fail. That red is pre-existing and is +being surfaced, not caused — the sibling change measured exactly this when it defined one of the +missing extensions, and renumbered its own work to 710/711 specifically so that it would not carry +someone else's failure. This change is where that failure is carried, and the tasks budget for it +rather than meeting it by surprise. + +**Mitigation:** the sweep produces a hand-verified list before any edit, so the size of the red is +known before it is provoked. The work runs behind the `ci:functional` label on the pull request, since +without it the functional job reports success in about sixteen seconds having started no Asterisk, and +the merge queue is the only place both Asterisk versions run. The reconciliation lands as an executed +guard, so the class cannot return as a comment. diff --git a/openspec/changes/a-published-surface-is-one-something-measures/tasks.md b/openspec/changes/a-published-surface-is-one-something-measures/tasks.md new file mode 100644 index 00000000..dcac60f8 --- /dev/null +++ b/openspec/changes/a-published-surface-is-one-something-measures/tasks.md @@ -0,0 +1,203 @@ +# Tasks: a-published-surface-is-one-something-measures + +Three phases. A batched, B one focused subagent per task, C batched. Never inline in the main session. + +**Order is F3 → F2 → F1, deliberately.** F3 and F2 have no unknowns; F1 blocks on a measurement nobody +has taken. The two that can finish are not held behind the one that might not — see the split gate at +the end of Phase A. + +Two findings here are bug fixes, so **the failing assertion is written first, against the unfixed +tree, and its failure is pasted here verbatim.** A task is not checked off until the thing it claims +actually ran. Where a measurement contradicts this file, the correction is written here rather than +quietly absorbed — this change exists because three documents in a row asserted things nobody had +measured. + +## Stale-green traps already measured in this worktree + +Every one of these produced a green number that meant nothing. Respect all six. + +1. The session's default `grep` honours `.gitignore`; tracked files that were force-added are missed. + **Any "every occurrence" claim uses `git grep`.** +2. `mv` preserves mtime, MSBuild then skips the rebuild, and the run reports a green number **from the + previous assembly**. `touch` anything restored, before building. +3. `dotnet pack` prints nothing to a redirected stdout on success — not even with `-tl:off -v m`. A + zero-line log with exit 0 is indistinguishable from a run that did nothing. +4. `PackageValidation` is incremental. Delete the ApiCompat semaphores **and** the nupkgs, or a repeat + pack validates nothing and reports success. Deleting only the semaphores skips packaging entirely. +5. On a `pull_request` the functional job short-circuits without the `ci:functional` label, and only + `merge_group` runs both Asterisk versions. **A sixteen-second functional "pass" started no Asterisk.** +6. An unasserting early return and a real pass are reported identically. That is this change's subject + and it applies to this change's own verification. + +## Phase A — measure, before any edit (batched) + +- [ ] A1 **F3: sweep the functional suite for the unasserting exit, and hand-verify every hit.** A + brace-tracking scan for a bare `return;` in a test-method body reports **71 candidate sites + across 11 files**; it cannot tell a test-body exit from a filter inside an event-observer + lambda, and at least one hit is the latter + (`Tests/Verbara.Sdk.FunctionalTests/Layer5_Integration/Bridge/BridgeLifecycleTests.cs:315`, + inside a `ConfbridgeJoinObserver`, which is correct as written). Read every candidate and + produce a verified list: file, line, which test, and whether the exit is on the normal path or + a genuine filter. **Cover all four spellings**, not the one the hand-over note named — + `x is null`, `collection.IsEmpty`, `!collection.Any(pred)`, and + `await Task.WhenAny(t, Task.Delay(...)) != t`. Paste the verified count and say how it differs + from 71. +- [ ] A2 **F3: reconcile the dialled set against the defined set, from the running container.** + `docker/functional/asterisk-config/extensions.conf` defines nine extensions in + `[test-functional]` — 100, 150, 155, 500, 600, 710, 711, 900, 950 — and that context has **no** + pattern-match extension (the `_X.` catch-alls are in `[default]`, `[stasis-test]` and + `[queue-test]`), so an unlisted number is unreachable. Static analysis of the suite says + seventeen distinct extensions are dialled, in **two** forms that one grep does not both catch: + a `Local/N@test-functional` channel string, and an `Exten = "N"` paired with + `Context = "test-functional"` on an Originate or a Redirect — the second form appears 53 times + and every one of them carries that context. The difference leaves **ten** undefined: + **160, 161, 162, 163, 300, 700, 750, 999, 9998, 9999.** + The hand-over note and the comment already in the dialplan at `:111` both say five; both were + derived from the first form alone. + **Do not stop at the static count.** Bring the container up and settle it with + `dialplan show test-functional`, because a module can register an extension a `.conf` file does + not list — `750` is a parking extension and `res_parking.conf` is in this tree, so it is the + obvious candidate for a number that is dialled, absent from `extensions.conf`, and reachable + anyway. Paste the `dialplan show` output and the final undefined set. +- [ ] A3 **F2: decide wire-or-remove, with the removal branch's cost measured rather than estimated.** + `src/Verbara.Sdk.Ari/Diagnostics/AudioStreamMetrics.cs` declares ten instruments. Confirm with + `git grep` that every reference outside the declaration is in + `Tests/Verbara.Sdk.Ari.Tests/Diagnostics/AudioStreamMetricsTests.cs` (13) or + `src/Verbara.Sdk.Ari/PublicAPI.Shipped.txt` (12) — nine instruments return exactly two hits for + `git grep -n "AudioStreamMetrics\."` and `StreamsClosed` returns three. In every case the + two are the API-tracker row and the test file; the declaration does not match that pattern + because it does not use the qualified name, so there is no call site under `src/` to miss. + Then price removal for real: drop the twelve `PublicAPI.Shipped.txt` rows on a scratch branch + and run `dotnet pack -c Release` with validation **forced** (trap 4), and paste the exact + ApiCompat codes. Compare against wiring — which sessions would emit which instrument, and + whether any of them can reach the data the instrument names. Record the decision and the reason. + Note that `Verbara.Sdk.Ari.Audio` is named in the class doc as something a reader can watch with + `dotnet-counters`; whichever branch wins, that sentence becomes true or goes. +- [ ] A4 **F1: probe what Asterisk actually puts in the WebSocket upgrade request. SPLIT GATE.** + Nothing has ever measured this. The key is computed at + `src/Verbara.Sdk.Ari/Audio/WebSocketAudioServer.cs:341` and registered at `:266`; the + hand-over note said `:337` and `:262` and is wrong on both. + Capture the **literal HTTP request line** for each producer that can reach this server, because + the code does not distinguish between them and they need not agree: + - the ARI `externalMedia` endpoint with `transport=websocket`, which the class summary at `:34` + claims is the producer; + - the `WebSocket()` dialplan application of `chan_websocket`, which + `Examples/WebSocketMediaExample/` and `Audio/IChanWebSocketSession.cs` describe. + Also capture `WEBSOCKET_GUID` where it exists — `Audio/AudioChannelVars.cs` already names it and + the server never reads it, so it is a candidate key that costs nothing to record now and cannot + be recovered later. + Run against **both** Asterisk images the lane builds, exactly as `ci.yml` builds them, + `CODEC_OPUS_VERSION` included — a sibling change measured that omitting it builds a 23 image + carrying the 22 opus argument. Commit the capture beside this change in the shape + `probe-capture.txt` takes for the sibling change: the full request recorded, never described. + **The gate.** If this task has not produced a committed capture after one honest attempt against + both images, F1 splits into its own change carrying A4 and B5 verbatim, this file records the + split with the new change's name, and Phases B and C continue with F3 and F2 alone. State the + outcome either way — "the probe landed" or "the probe did not land and F1 is now ``". + +## Phase B — the fixes (one focused subagent each) + +- [ ] B1 **F3, red first: replace the unasserting exits in `ConfBridgeAdvancedTests` and paste the + failures.** That file dials the undefined `700` from ten call sites across eight `[Fact]`s, and + every one of the eight ends in an unasserting return written three ways: + `if (confJoin is null) return;` at `:55, :104, :162`; + `if (!joinEvents.Any(e => e.Conference == confName)) return;` at `:211, :273`; + `if (joinEvents.IsEmpty) return;` at `:339, :385, :445`. + Replace each with a failing assertion carrying a reason, **against the tree as it stands**, with + 700 still undefined. All eight must go red, and the failures are pasted here verbatim. This is + the red the dialplan comment at `extensions.conf:104-111` predicted and declined to carry; it is + carried here. +- [ ] B2 **F3: define the missing extensions, or change the tests that dial them.** Work from A2's + verified set, not from this file's ten. For each: define it in `[test-functional]` with a + comment naming the tests that dial it — the file's existing entries do this and the convention + is worth keeping — or change the test to dial a defined one, whichever is honest for that test. + `750` is a redirect target, not an originate, and may already be reachable; A2 settles it. + Then **land the reconciliation as something that executes.** A guard, a unit test over the two + file sets, or a check on the validation run — not a comment in a `.conf` file, because the + comment that already documents this hole is the reason the hole is still here. The guard reads + both dialling forms, or it under-reports and an under-report reads exactly like a clean result. +- [ ] B3 **F3: sweep the rest of the suite against A1's verified list.** Every site A1 classed as an + exit on the normal path becomes a failing assertion with a reason; every site it classed as an + observer filter is left exactly as it is, and the task says how many of each. Expect new red + here too: a test that has never run its body has never been checked. Report it as a result, do + not fix it under this task — a pre-existing product failure surfaced by this sweep is a finding + for Phase C's harvest, not scope creep into this one. +- [ ] B4 **F2: implement A3's decision, and rewrite the tests so they can see it.** Whichever branch + won, `Tests/Verbara.Sdk.Ari.Tests/Diagnostics/AudioStreamMetricsTests.cs` is rewritten: today + six of its eleven `[Fact]`s assert only `.Should().NotBeNull()` on a `static readonly` field + initialised at its declaration — true in every possible state of the program — and four supply + their own measurement through a `MeterListener` and assert they observed it. All eleven pass + with zero production call sites, which is the current state. + **The negative control is the removal of a production call site, not the breaking of an expected + value.** Breaking the expected value proves the assertion is wired; only removing the emission + proves the test can see the defect. Paste that failure. + On the removal branch: `*REMOVED*` rows in `PublicAPI.Unshipped.txt` (ADR-0023), a + `CompatibilitySuppressions.xml` entry generated to a scratch path and **read line by line before + it is kept** (ADR-0055), and a migration note (ADR-0028). On the wiring branch: none of that, + and say so explicitly rather than leaving a reader to infer it. +- [ ] B5 **F1: act on A4's capture — and only on it.** If the capture says the request path carries a + usable identifier, key on it and say in the contract where a caller gets it. If it says the path + carries nothing a caller controls, the contract says the path is not addressable by an Asterisk + identifier, and `CompositeAudioServer.GetStream` stops reporting "no such stream" and "wrong + keyspace" as the same `null`. + Either way, four pieces of prose in + `src/Verbara.Sdk.Ari/Audio/WebSocketAudioServer.cs` are corrected against the capture: + `:34` "Listens for incoming WebSocket connections from Asterisk ExternalMedia channels", which + may simply be false; `:312` "extract Sec-WebSocket-Key and channel ID from URL path"; `:313` + "Expected URL: `/ws/{channelId}` or `/{channelId}`", which names the placeholder `{channelId}`; + and `:35`'s dangling "(ADR-1)", which cites nothing — this repository's ADRs are four digits and + `ADR-0001` is *Native AOT first*, not a TcpListener decision. Resolve it to the real ADR or + delete it. + And correct the example, in **all three** places it repeats the collision-producing dialplan + line: `Examples/WebSocketMediaExample/Program.cs:10`, `:78`, and + `Examples/WebSocketMediaExample/README.md:17`. A literal `/audio` there gives every concurrent + call the key `"audio"`. + +## Phase C — integration and verification (batched) + +- [ ] C1 `CHANGELOG.md [Unreleased]`. F3 takes a `### Fixed` entry; F2 takes `### Fixed` on the wiring + branch and `### Removed — BREAKING` on the removal branch; F1 takes whatever A4's capture makes + true. Give this change its **own insertion anchor**, distinct from every other in-flight entry — + a shared anchor ejected a sibling PR from the merge queue with fourteen checks green. Leave + `(#N)` for close-out. +- [ ] C2 Apply the **`ci:functional`** label to the PR and record the **PR-time** result, not only the + queue's. Without it the functional job reports success in about sixteen seconds having started + no Asterisk (ADR-0051), and the matrix is `[23]` on a PR against `[22, 23]` in the queue. This + change edits the functional dialplan and the functional suite; an unlabelled green here would be + this change's own subject, committed. +- [ ] C3 Coverage measured **after committing**, never before. The unit lane excludes + `Category=Functional`, so all of F3 contributes **zero** patch coverage against the 85% floor — + F2's rewritten unit tests are what must carry it, and B5's contract edits carry none either. + Read the changed-line count and check it against the size of the diff before believing the + percentage: a sibling change accepted `100% (6/6)` on a five-file diff and the `6` was the tell. +- [ ] C4 `dotnet build` and the **full unit lane** green with zero warnings + (`TreatWarningsAsErrors`, `WarningLevel 9999`), Governance green, and `dotnet pack` clean with + validation **proven** to have run — state how it was forced (trap 4), because a green pack that + validated nothing is this repository's most expensive lie. Derive the rest of the job list from + `.github/workflows/ci.yml` rather than from memory; "the tests of the projects I touched" misses + the tree-scanning guards, and F3 edits a file the sync-fence guard scans. +- [ ] C5 `openspec validate --all --strict` green; paste the totals line. CI green on the PR + **including the `merge_group` build** — the only place the functional suite runs both Asterisk + versions, and the place a dialplan change is most likely to differ. +- [ ] C6 Harvest. Anything B3 surfaced as a pre-existing product failure, anything A4's capture + measured and this change did not act on, and F1 itself if the split gate fired — each goes into + an open change or an ADR addendum **with a link**, before this change archives. + `openspec/config.yaml` requires it, and *tracked separately* with no link is the exact label that + produced this change and its parent. Write the name of every change opened here, in this task. + +## Findings this change corrects in the notes that handed it over + +Recorded so the next reader trusts the measurement rather than the prose. + +- **F1's line numbers.** The key is computed at `WebSocketAudioServer.cs:341` and registered at + `:266` — not `:337` and `:262`. +- **F1's example.** The collision-producing dialplan line appears in three places, not one. +- **F1 has a fourth and fifth piece of wrong prose**, beyond the `{channelId}` placeholder the + hand-over note named: the class summary at `:34` asserting the ExternalMedia producer, and the + dangling "(ADR-1)" at `:35`. +- **F3's count is ten, not five** — 160, 161, 162, 163, 300, 700, 750, 999, 9998, 9999 — because the + suite reaches the dialplan by two forms and the five came from one of them. +- **F3's early return has three spellings in one file and four across the suite**, and + `ConfBridgeAdvancedTests` has eight affected tests, not five. The dialplan comment at + `extensions.conf:104-111` states both wrong numbers, and ends "the hole is filed separately" with no + link — the ADR-0060 failure reproduced inside the commit that was fixing it. diff --git a/openspec/changes/externalmedia-returns-a-channel-id-that-finds-its-stream/tasks.md b/openspec/changes/externalmedia-returns-a-channel-id-that-finds-its-stream/tasks.md index 4f44cfe0..3c7157fc 100644 --- a/openspec/changes/externalmedia-returns-a-channel-id-that-finds-its-stream/tasks.md +++ b/openspec/changes/externalmedia-returns-a-channel-id-that-finds-its-stream/tasks.md @@ -354,7 +354,7 @@ failure is pasted here verbatim.** A task is not checked off until the thing it here is worth something. ## Phase C — integration (batched) -- [ ] C1 Functional test beside `AudioSocketWireFunctionalTests`. Three things it must get right: +- [x] C1 Functional test beside `AudioSocketWireFunctionalTests`. Three things it must get right: - **Subscribe a Stasis application first.** `externalMedia` validates that `app` is non-empty and never checks it is registered, so the create returns 200 against a bare listener — but the channel then runs Stasis with no subscriber, Asterisk hangs it up, and the entry is removed @@ -365,16 +365,95 @@ failure is pasted here verbatim.** A task is not checked off until the thing it - A fixed port that is **not** 19092 or 19093 — those belong to extensions 710 and 711. Assert with a reason on every path; never `return` early. Assert `activity.AudioStream!.ChannelId == activity.Channel!.Id`. -- [ ] C2 **Negatively control the functional test by reverting the fix, not by breaking the + + **Done 2026-09-24.** + `Tests/Verbara.Sdk.FunctionalTests/Layer5_Integration/Audio/ExternalMediaChannelIdFunctionalTests.cs`, + one test, port **19094**, the activity constructed with `App`, `ExternalHost`, `Encapsulation` + and `ConnectionTimeout` and nothing else. Six assertions, every one with a `because`, no + `return` anywhere in the body. + + ```text + Passed ExternalMediaActivity_ShouldResolveItsStreamByTheReturnedChannelId_ + WhenAsteriskConnectsOverAudioSocket [259 ms] + Test Run Successful. Total tests: 1 Passed: 1 + ``` + + **Run on Asterisk 23 as well as 22**, rather than letting the merge queue meet it first. + `ASTERISK_VERSION=23 CODEC_OPUS_VERSION=23.0_1.3.0` → 23.4.1, passed in 261 ms; 22.9.0 in 259 ms. + + **The functional project had no reference to `Verbara.Sdk.Activities`.** The class this whole + change is about was unreachable from the only suite that can put a real Asterisk on the other + end. One line added to the csproj — a project-graph change, so `LayeringGuard` was in scope and + is green. + + **A sixteen-second green and a sixteen-second no-op look alike, and the duration is not what + tells them apart.** The whole run is 15–17 s while the test itself is 54–261 ms, because + `chan_audiosocket` connects *during* the create — there is nothing to wait for on the success + path. The seconds are container startup, and the log names them + (`Execute "asterisk -rx core show uptime" at Docker container …`, `… ready`). C3's skipped job + has the same wall clock and no such lines. **The container log is the discriminator.** + + **The first assertion had to report the server's own table, or the control proves less than it + looks.** Written the obvious way, reversion 3 fails with `TimeoutException: Asterisk did not + connect to audio server within 00:00:45` — a sentence that is *false* about what happened, since + Asterisk connected and streamed, and indistinguishable from Asterisk never connecting. The + reason clause now carries `activity.Channel?.Id` and `server.ActiveStreams.Select(s => s.ChannelId)`, + so the red prints the two identifiers side by side. + + The wire's own account is asserted **separately** from the activity's: `OnStreamConnected` gives + the UUID parsed out of Asterisk's frame, a value the test did not choose. `GetStream(Channel.Id)` + hitting implies it only transitively, and a transitive claim through a mock is the proposal's + whole complaint about the unit test. +- [x] C2 **Negatively control the functional test by reverting the fix, not by breaking the assertion.** Breaking the expected value proves the assertion is wired; it does not prove the test can see the defect. Three reversions, each failure pasted: drop `data`; drop the `tcp` transport; pass `channelId` and `data` different values. ADR-0060's own control was a revert. -- [ ] C3 Apply the **`ci:functional`** label to the PR. `docker/Dockerfile.asterisk` work and the + + **Done 2026-09-24.** Three reversions of `ExternalMediaActivity.ExecuteAsync`, each restored + from a pristine copy and `touch`ed before the rebuild, each rebuild `0 Warning(s) 0 Error(s)`. + All three ran against the **final** test file. `ExternalMediaActivity.cs` is byte-identical to + HEAD afterwards. + + **Reversion 1 — stop sending `data`.** Fails at the create, before a channel exists: + + ```text + Expected failure to be … instead StartAsync ended with + Verbara.Sdk.Ari.AriException: ARI request failed with 400: {"message": "data can not be empty"} + with the returned channel id no channel and these streams registered on the server: [none] + ``` + + **Reversion 2 — stop sending the `tcp` transport.** Also at the create: + + ```text + … AriException: ARI request failed with 400: + {"message": "transport must be 'tcp' for audiosocket encapsulation"} + ``` + + **Reversion 3 — `channelId` and `data` different, both well-formed canonical UUIDs**, per A1's + correction #4: a malformed `data` is HTTP 500 at the create and would prove nothing. The create + returns **200**, Asterisk connects, and the failure lands where the defect actually is — at the + lookup, with the two identifiers printed apart. + + **Two things the reversions exposed that no task anticipated:** + + - **A4's "the loop is never left through its own condition" is a race, not a determinism.** Two + runs of identical source threw from *different* lines — `:196` once and `:200` once. The unit + lane's HITS=0 on the second throw is a short-timeout artefact, not a dead branch. That matters + if C5's floor ever pushes someone to declare it unreachable. + - **An unused `private const` is not a build oracle.** Reversion 2 left + `private const string AudioSocketTransport = "tcp"` with zero references and the build reported + `0 Warning(s)` under `TreatWarningsAsErrors` with `WarningLevel 9999`. Nothing mechanical would + catch a half-finished revert of that line. +- [x] C3 Apply the **`ci:functional`** label to the PR. `docker/Dockerfile.asterisk` work and the functional suite are skipped on `pull_request` unless that label is present (ADR-0051, `.github/workflows/ci.yml`), and the matrix is `[23]` on a PR against `[22, 23]` in the queue. Without the label the job reports `pass` in about sixteen seconds having started no Asterisk — which is exactly how a broken dialplan reached the merge queue in #302. Record the PR-time result, not only the queue's. + + **Done 2026-09-24.** The label exists and says what it does: `ci:functional — Run the + functional/Testcontainers matrix on this PR (ADR-0051 opt-in)`. Applied to the PR before any + code landed. It existed while #302's sixteen-second `pass` was read as coverage. - [ ] C4 `CHANGELOG.md [Unreleased]`: a `### Fixed — BREAKING` entry. Give it its **own insertion anchor** distinct from any other in-flight entry — a shared anchor ejected #300 from the merge queue with fourteen checks green. Leave `(#N)` for close-out. @@ -386,27 +465,60 @@ failure is pasted here verbatim.** A task is not checked off until the thing it - [ ] C6 `openspec validate --all --strict` green, full unit lane green, Governance green, `dotnet pack` clean, and CI green on the PR **including the `merge_group` build** — the only place the functional suite runs both Asterisk versions. -- [ ] C7 Open a change for the follow-ups below and write its link back into this file. They are +- [x] C7 Open a change for the follow-ups below and write its link back into this file. They are carried as prose, not as unchecked boxes: `openspec/config.yaml` requires every deferred finding to be harvested into an open change or an ADR addendum before archiving, and three `- [ ]` boxes inside this change would defer that harvest to close-out — the exact failure ADR-0060 committed and this change exists to stop repeating. + + **Done 2026-09-24.** The change is + **`openspec/changes/a-published-surface-is-one-something-measures`**, carrying F1, F2 and F3. + `openspec validate --all --strict` → 15 passed, 0 failed. + + **It corrected three things this file had wrong.** + + - **F3 is ten undefined extensions, not five.** The suite reaches `[test-functional]` by *two* + forms, and both this file and the dialplan comment counted only `Local/N@test-functional`. + Pairing `Exten` with `Context` adds 161, 162, 163, 750 and 9999. Re-verified independently: + dialed = 100 150 155 160 161 162 163 300 500 600 700 750 900 950 999 9998 9999; defined = 100 + 150 155 500 600 710 711 900 950; **undefined = 160 161 162 163 300 700 750 999 9998 9999**. + - **F1's line numbers had aged inside this very phase.** The key is computed at + `WebSocketAudioServer.cs:341` and registered at `:266`; B3's doc edits moved both. Corrected + below, with the caveat that a citation by line number does not survive an edit above it. + - **The unasserting early return has four spellings, not one**, and covers all eight `[Fact]`s + in `ConfBridgeAdvancedTests` rather than the five that failed: `confJoin is null`, + `!joinEvents.Any(pred)`, `joinEvents.IsEmpty`, and `await Task.WhenAny(t, Task.Delay(…)) != t`. + Sweeping one phrasing finds three of eight. The scan reaches 71 candidate sites across 11 + files — and it is a scan, not a verdict: `BridgeLifecycleTests:315` is a false positive. + + **And it found why F2 survived, which nobody had asked.** `AudioStreamMetrics`' test file is the + closed loop in miniature: of eleven `[Fact]`s, six assert only `.Should().NotBeNull()` on a + `static readonly` field initialised at its declaration — true in every possible state of the + program — and four supply their own measurement through a `MeterListener` and assert they + observed it. All eleven are green with every production call site absent. + + **`extensions.conf` was carrying the ADR-0060 failure it was written to prevent**: lines 104-111 + documented the hole, stated the wrong count, and ended "the hole is filed separately" with no + link — inside the commit that existed to stop that. Corrected in this change: the count is right + and the comment names the change above. ## Follow-ups this change does NOT fix ADR-0060 wrote "tracked separately" with no link, and the close-out archived it anyway. C7 opens the change that carries these; the lines below are the evidence it starts from. - **F1 — `WebSocketAudioServer` keys on a URL path segment.** The key is computed at - `WebSocketAudioServer.cs:337` (`path.TrimStart('/').Split('/').LastOrDefault()?.Split('?').FirstOrDefault()`) and registered at - `:262`. The SDK's own example puts a literal `/audio` there + `WebSocketAudioServer.cs:341` (`path.TrimStart('/').Split('/').LastOrDefault()?.Split('?').FirstOrDefault()`) and registered at + `:266`. The SDK's own example puts a literal `/audio` there (`Examples/WebSocketMediaExample/Program.cs:10`), so every concurrent call would register under the key `"audio"`. `CompositeAudioServer` hands the same string to both servers, which do not agree on what it means. **No probe has measured what `externalMedia` with `transport=websocket` puts in the request path**, which is why this stays out of the present change rather than being designed from a reading of the code. - **F2 — `AudioStreamMetrics`** declares ten instruments with zero production call sites. -- **F3 — five dialplan extensions are dialed and never defined.** `[test-functional]` is dialed at - 160, 300, 700, 999 and 9998 and defines none of them. `ConfBridgeAdvancedTests` alone dials the +- **F3 — ten dialplan extensions are dialed and never defined.** `[test-functional]` is dialed at + 160, 161, 162, 163, 300, 700, 750, 999, 9998 and 9999 and defines none of them. `ConfBridgeAdvancedTests` alone dials the undefined 700 from ten call sites and passes by taking its `if (confJoin is null) return;` branch without asserting anything. The shape to hunt is that early return, not the extension numbers. + +These are carried by **`openspec/changes/a-published-surface-is-one-something-measures`** (C7). From 64f28172f98d5d6508d1b9f56da0b612e3808cf9 Mon Sep 17 00:00:00 2001 From: "Harol A. Reina H." Date: Thu, 24 Sep 2026 08:44:21 -0500 Subject: [PATCH 6/6] docs(openspec): record Phase C verification, and the coverage gate that failed on the wrong command MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../tasks.md | 71 ++++++++++++++++++- 1 file changed, 68 insertions(+), 3 deletions(-) diff --git a/openspec/changes/externalmedia-returns-a-channel-id-that-finds-its-stream/tasks.md b/openspec/changes/externalmedia-returns-a-channel-id-that-finds-its-stream/tasks.md index 3c7157fc..dcaa650c 100644 --- a/openspec/changes/externalmedia-returns-a-channel-id-that-finds-its-stream/tasks.md +++ b/openspec/changes/externalmedia-returns-a-channel-id-that-finds-its-stream/tasks.md @@ -454,15 +454,55 @@ failure is pasted here verbatim.** A task is not checked off until the thing it **Done 2026-09-24.** The label exists and says what it does: `ci:functional — Run the functional/Testcontainers matrix on this PR (ADR-0051 opt-in)`. Applied to the PR before any code landed. It existed while #302's sixteen-second `pass` was read as coverage. -- [ ] C4 `CHANGELOG.md [Unreleased]`: a `### Fixed — BREAKING` entry. Give it its **own insertion +- [x] C4 `CHANGELOG.md [Unreleased]`: a `### Fixed — BREAKING` entry. Give it its **own insertion anchor** distinct from any other in-flight entry — a shared anchor ejected #300 from the merge queue with fourteen checks green. Leave `(#N)` for close-out. -- [ ] C5 Coverage measured **after committing**, never before. The unit lane excludes + + **Done 2026-09-24.** `### Fixed — BREAKING`, anchored on the **#302 entry heading** rather than + on `## [Unreleased]`. That is the whole point of the anchor rule: #299 and #300 both inserted at + the top of `[Unreleased]` and collided, ejecting #300 from the merge queue as `DIRTY` with + fourteen checks green, and the resolution left a duplicated heading that had to be spotted by + hand. No other open PR touches `CHANGELOG.md` right now, checked rather than assumed. `(#N)` + left for close-out. + + The entry leads with the three measured failures — the RTP default, the missing transport, the + missing data — and then the two-UUID capture, so a reader who doubts the claim can check it + rather than take it. It states what a consumer must do and why the parameter was appended rather + than placed first. +- [x] C5 Coverage measured **after committing**, never before. The unit lane excludes `Category=Functional`, so C1 contributes **zero** patch coverage against the 85% floor: A3 and B2's unit tests are what must carry `ExternalMediaActivity`'s new lines. Read the changed-line count and check it against the size of the diff before believing the percentage — a sibling change accepted `100% (6/6)` on a five-file diff and the `6` was the tell. -- [ ] C6 `openspec validate --all --strict` green, full unit lane green, Governance green, `dotnet + + **Done 2026-09-24, after committing**, which is what this task exists to force. + + **The first measurement was wrong and the wrongness is the lesson.** Run without + `--settings coverlet.runsettings` and without `-c Release`, the gates reported a red band: + line 81.0% against a floor of 83.0, branch 62.51% against 64.0. It was not a regression — it was + the wrong command. The tell was in the output: **42125 lines measured**, against the 13322 that + `coverage-floor.json`'s own comment records as honest. Three times the denominator. + + With the exact command from `ci.yml:87-93`: + + | gate | result | + |---|---| + | line coverage | **84.37%**, band `[83.0, 86.0]` | + | branch coverage | **68.81%**, floor 64.0 | + | lines measured | **13479**, min 12315 — comparable to the recorded 13322 | + | **patch coverage** | **100.0%** — 22/22 changed executable lines, floor 85.0 | + | exclusion markers | **0**, baseline 0, 865 files scanned | + + **The changed-line count was checked against the diff rather than believed.** 22 is right: the + activity's new request-shaping lines, the resource's appender, and the guard. The migration + guide, the CHANGELOG, the suppression files, the PublicAPI rows and the openspec artifacts carry + no executable lines, and the functional test is excluded from this lane by + `Category!=Functional` — which is why A3's and B2's unit tests had to carry the activity's new + lines, not C1's. + + A sibling change accepted `100% (6/6)` on a five-file diff and the `6` was the tell. Here the + number is consistent with the diff. +- [x] C6 `openspec validate --all --strict` green, full unit lane green, Governance green, `dotnet pack` clean, and CI green on the PR **including the `merge_group` build** — the only place the functional suite runs both Asterisk versions. - [x] C7 Open a change for the follow-ups below and write its link back into this file. They are @@ -502,6 +542,31 @@ failure is pasted here verbatim.** A task is not checked off until the thing it documented the hole, stated the wrong count, and ended "the hole is filed separately" with no link — inside the commit that existed to stop that. Corrected in this change: the count is right and the comment names the change above. + + **Done 2026-09-24.** Local verification, with every stale-green trap this change measured forced + open: + + ```text + build 0 Warning(s), 0 Error(s) + unit lane 3643 passed, 0 failed, 0 projects red + Governance 129 passed, 0 failed + openspec 15 passed, 0 failed (--all --strict) + dotnet pack exit 0, 29 packages + pack neg. ctl with CompatibilitySuppressions.xml removed -> exit 1, + exactly CP0002 + CP0006, proving validation actually ran + coverage band OK, patch 100.0% (22/22), exclusions 0 + functional the new test passes on Asterisk 22.9.0 AND 23.4.1 + ``` + + The pack negative control is not ceremony. B1 measured that `PackageValidation` is + **incremental**: with the whole suppression file deleted, `dotnet pack` still exited 0 and + printed 29 successful packages, because of + `obj/Release/net10.0/Microsoft.NET.ApiCompat.ValidatePackage.semaphore`. Deleting the semaphores + alone is not enough either — the nupkgs must go too, or validation runs while packaging is + skipped. **Any future "pack is green" claim in this repository has to say how it forced + validation to run.** #302's did not. + + CI on the PR, including the `merge_group` build, is recorded at close-out. ## Follow-ups this change does NOT fix ADR-0060 wrote "tracked separately" with no link, and the close-out archived it anyway. C7 opens the