Repository navigation
feat(conformance): fail server-initiated-pcm-24bit when the stream was not 24-bit - #147
Merged
chrisuthe merged 2 commits intoOct 7, 2026
Conversation
…d-pcm-24bit The adapter listed 16-bit PCM in every scenario, so the server streamed 16-bit and the SDK's 24-bit decode path never ran in the scenario that exists to test it. It now lists 24-bit as the only depth there, as the scenario describes.
…s not 24-bit The canonical PCM hash is taken over float samples, so a 16-bit stream of the clip matched it and the cell passed without the 24-bit path being exercised. The scenario now also reads the negotiated bit depth from the server's recorded stream, and from the client's where it reports one. RC1 requires no bit depth of a player, so the reason names a harness gap or the server's selection, never a client defect. scenario_revision goes from 6 to 7.
There was a problem hiding this comment.
🟢 Approval recommended
The adapter fix, scoped verdict, revision bump, documentation, and tests consistently address the false-positive scenario.
0 open findings
What changed in this PR
Ensures the 24-bit PCM scenario genuinely negotiates and validates a 24-bit stream.
Changes:
- Advertises only 24-bit PCM from the .NET adapter for this scenario.
- Adds negotiated bit-depth validation and increments the scenario revision.
- Adds comprehensive tests and updates documentation.
| File | Description |
|---|---|
adapters/sendspin-dotnet/client/Program.cs |
Advertises 24-bit PCM for the targeted scenario. |
src/conformance/models.py |
Adds scenario bit-depth verification metadata. |
src/conformance/scenarios.py |
Enables validation and bumps revision to 7. |
src/conformance/declared_formats.py |
Implements the negotiated-depth verdict. |
src/conformance/runner.py |
Applies the verdict after successful PCM comparison. |
tests/test_stream_bit_depth.py |
Covers evidence and scenario outcomes. |
README.md |
Documents the strengthened assertion. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
chrisuthe
marked this pull request as ready for review
October 7, 2026 20:21
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
server-initiated-pcm-24bitpassed foraiosendspin -> sendspin-dotneton a 16-bit stream. The scenario compares canonical PCM hashes, which are taken over float samples, so a 16-bit stream of the clip matches as well as a 24-bit one does. Nothing read the negotiated depth.Two commits:
fix(sendspin-dotnet adapter)— the adapter listed 16-bit PCM in every scenario. It now lists 24-bit as the only depth in this one, as the scenario describes and as the other client adapters already do (fix(sendspin-cpp adapter): negotiate 24-bit PCM and read the negotiated depth #102 did the same for sendspin-cpp).feat(conformance)— the scenario now also fails when the negotiated stream was not 24-bit.ScenarioSpec.verifies_stream_bit_depthnames the depth andstream_bit_depth_violationjudges it, next toformat_priority_violation.Does RC1 require 24-bit of a player?
No.
roles/player/v1.md:20(spec671a34d,1.0.0-rc1): "Players MUST list eitherflacorpcm".bit_depthis "integer ... (e.g., 16, 24)" with no required value, and the PCM encoding convention only says how 24-bit is packed when it is used. A 16-bit-only player is conformant.The premise turned out different
sendspin-dotnet is not a 16-bit-only player. Its
PcmDecoderdecodes 16, 24 and 32-bit; the 16-bit declaration came from the harness adapter'sBuildCapabilities. So the false green was a harness gap, not a capability limit, and the fix for the known cell is the adapter.Outcome chosen for a pair that did not negotiate 24-bit
Fail the case as a scenario-scope mismatch, with a reason that names the side the evidence shows. The verdict reads the server's recorded
stream(what went on the wire) and the client'sstreamwhere it reports one:streamHarness gap: the server adapter recorded no stream formatHarness gap: the client adapter advertised no 24-bit formatHarness gap: the stream was ...(names neither side)Server streamed ... although the client listed a 24-bit formatClient observed a ... stream although the server sent ...No reason says a client "does not support" 24-bit, and none cites a spec MUST, because there is none. A client summary without a
streamis not judged.Rejected:
supports_flacshape). That flag excludes a case that genuinely does not apply. No client in the tree is shown to be 16-bit-only, so asupports_pcm_24bitflag would have no honest user today, and setting it for sendspin-dotnet would stampunsupportedon what is a harness gap. It is the right tool the day a real 16-bit-only client is added.This follows #127 and #131 in naming the side at fault, and
format_priority_violationin failing a case whose stimulus was not what the scenario needs.scenario_revision
server-initiated-pcm-24bitgoes from 6 (read frommainat5726f98) to 7. Nothing else is bumped.Expected cell movement
Against the live baseline (240 cases, 47 passing): no cell changes status; 47 stay passing.
aiosendspin -> aiosendspin,-> SendspinKitand-> sendspin-dotnet. The first two already negotiate 24-bit on both sides.sendspin-dotnetis the only client that passed without a 24-bit negotiation. With the adapter fix it negotiatespcm/8000/1ch/24-bit(120000 wire bytes instead of 80000) and passes for real. Applying the new verdict to the baseline's published 16-bit summaries for that cell fails it with theclient adapter advertised no 24-bit formatreason.#146
Not needed, and it has merged; this is rebased on top of it. With it, the .NET client reports the 24-bit
streamit observed, so the client-side branch of the verdict is exercised by that cell too.Verification
python -m unittest discover -s tests: 358 pass (14 new intests/test_stream_bit_depth.py).scripts/run_all.py --from aiosendspin --to aiosendspin,sendspin-dotnet,SendspinKiton fresh clones of every upstream default branch (sendspin-dotnetf891067, aiosendspin90cecf2, SendspinKit68d2676): 48 cases, every one keeps its baseline status;scripts/detect_regressions.pyreports no regressions.aiosendspin -> sendspin-dotnetcase page for this scenario.Known limit
The verdict compares bit depth only, not codec. No adapter declares a non-PCM 24-bit entry, and
undeclared_format_violationalready fails a stream in a codec the client did not list.