test(contracts): add provider compatibility baseline - #82
Conversation
📝 WalkthroughWalkthroughAdds a Jellyfin/Silo provider compatibility contract baseline, a machine-readable contract matrix, offline schema validation, live MediaBrowser probes, and CTest/Nix wiring for automated validation. ChangesProvider compatibility validation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ContractMatrix
participant MediaBrowserV1Probe
participant HttpTransport
participant ProviderServer
ContractMatrix->>MediaBrowserV1Probe: provide deployment expectations
MediaBrowserV1Probe->>HttpTransport: send contract probes
HttpTransport->>ProviderServer: issue HTTP requests
ProviderServer-->>HttpTransport: return responses and headers
HttpTransport-->>MediaBrowserV1Probe: return response data
MediaBrowserV1Probe-->>ContractMatrix: record pass/fail/inconclusive results
Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
859ee0c to
e7dd699
Compare
Greptile SummaryThis PR adds a provider-neutral contract baseline for Bloom's MediaBrowser surface, covering 29 contracts across pinned Jellyfin and Silo compatibility deployments. It includes an offline validator, an opt-in live probe harness, an 8-test unit suite, documentation, and the Nix/CMake wiring to run the validator as part of
Confidence Score: 4/5No production code is changed; all changes are test infrastructure and documentation. The offline validator and unit tests are correct. The live probe harness has a logic flaw that can produce incorrect results for one contract when the library fixture returns a Movie before any Episode. The episode-specific tests/contracts/run_live_contracts.py — specifically the
|
| Filename | Overview |
|---|---|
| tests/contracts/run_live_contracts.py | New live probe harness for the MediaBrowser contract matrix; contains a logic flaw where the episode-specific segments.plugin-intro-skipper probe uses item_id (potentially a Movie) instead of the tracked episodeId, causing false FAIL results against Jellyfin when a Movie sorts first in the library. |
| tests/contracts/validate_contracts.py | New offline validator for the contract JSON; enforces immutable image pins, deployment coverage, HTTP-status-only rejection, and native Silo contract shape — logic is sound and all checks match the test suite expectations. |
| tests/contracts/provider_contracts_test.py | Eight-test unittest suite covering validator rejection cases, driver registration, response parsing, and same-origin transport logic; all test cases are correct and cover the key invariants. |
| tests/contracts/provider-contracts.json | New machine-readable contract matrix with 29 contracts across two deployments; image pins use immutable sha256 digests, all coverage requirements are satisfied, and every contract carries payload-level evidence and required semantics beyond HTTP status. |
| tests/CMakeLists.txt | Adds ProviderContractValidationTest CTest target using the Python interpreter; path resolution relies on Path(__file__) inside the test script, making it working-directory-independent. |
| nix/tests.nix | Correctly adds python3 to nativeBuildInputs so the new contract test is available during the Nix test derivation build. |
| docs/provider-compatibility.md | New documentation page covering Bloom's MediaBrowser wire assumptions, reproducible Jellyfin/Silo deployment pins, compatibility results, native Silo contract decisions, and open upstream questions; thorough and consistent with the JSON matrix. |
| AGENTS.md | One-line addition linking docs/provider-compatibility.md from the See also section, consistent with the AGENTS.md update policy. |
Sequence Diagram
%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant Op as Operator
participant Runner as run_live_contracts.py
participant V as validate_contracts.py
participant S as MediaBrowser Server
Op->>Runner: --deployment --base-url --allow-mutations
Runner->>V: load_and_validate(provider-contracts.json)
V-->>Runner: validated data + expected outcomes per contract
Runner->>S: POST /Users/AuthenticateByName
S-->>Runner: AccessToken + User.Id
Runner->>S: "GET /Users/{id}/Views"
Runner->>S: "GET /Users/{id}/Items (paged)"
Runner->>S: "GET /Users/{id}/Items/{id} (details)"
Runner->>S: "GET /Items/{id}/Images/Primary"
Runner->>S: "POST /Items/{id}/PlaybackInfo"
S-->>Runner: PlaySessionId + MediaSource URL
Runner->>S: "GET /Videos/{id}/stream (Range: bytes=0-31)"
S-->>Runner: 206 + Content-Range
Note over Runner,S: Mutating probes (--allow-mutations only)
Runner->>S: POST /Sessions/Playing (start/progress/stop)
Runner->>S: "POST/DELETE /Users/{id}/PlayedItems/{id}"
Runner->>S: "POST/DELETE /Users/{id}/FavoriteItems/{id}"
Runner-->>Op: PASS/SKIP/FAIL per contract + JSON report
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant Op as Operator
participant Runner as run_live_contracts.py
participant V as validate_contracts.py
participant S as MediaBrowser Server
Op->>Runner: --deployment --base-url --allow-mutations
Runner->>V: load_and_validate(provider-contracts.json)
V-->>Runner: validated data + expected outcomes per contract
Runner->>S: POST /Users/AuthenticateByName
S-->>Runner: AccessToken + User.Id
Runner->>S: "GET /Users/{id}/Views"
Runner->>S: "GET /Users/{id}/Items (paged)"
Runner->>S: "GET /Users/{id}/Items/{id} (details)"
Runner->>S: "GET /Items/{id}/Images/Primary"
Runner->>S: "POST /Items/{id}/PlaybackInfo"
S-->>Runner: PlaySessionId + MediaSource URL
Runner->>S: "GET /Videos/{id}/stream (Range: bytes=0-31)"
S-->>Runner: 206 + Content-Range
Note over Runner,S: Mutating probes (--allow-mutations only)
Runner->>S: POST /Sessions/Playing (start/progress/stop)
Runner->>S: "POST/DELETE /Users/{id}/PlayedItems/{id}"
Runner->>S: "POST/DELETE /Users/{id}/FavoriteItems/{id}"
Runner-->>Op: PASS/SKIP/FAIL per contract + JSON report
Comments Outside Diff (3)
-
tests/contracts/run_live_contracts.py, line 1206-1212 (link)Episode-specific probe uses generic
item_idthat may be a MovieThe
segments.plugin-intro-skipperprobe hits/Episode/{item_id}/IntroSkipperSegments, butitem_idis the first result from aMovie,Episodelibrary query sorted bySortName. If a Movie title sorts before any Episode,item_idis a Movie ID. Jellyfin then returns404, the probe records"missing", but the expected outcome is"partial"— sopassedisFalseand the run fails for the Jellyfin deployment.The
self.variables["episodeId"]key is populated at line 1117 whenever the first item is an episode, but is never used here. The probe should preferself.variables.get("episodeId", item_id)to ensure it targets an episode when one is available.Prompt To Fix With AI
This is a comment left during a code review. Path: tests/contracts/run_live_contracts.py Line: 1206-1212 Comment: **Episode-specific probe uses generic `item_id` that may be a Movie** The `segments.plugin-intro-skipper` probe hits `/Episode/{item_id}/IntroSkipperSegments`, but `item_id` is the first result from a `Movie,Episode` library query sorted by `SortName`. If a Movie title sorts before any Episode, `item_id` is a Movie ID. Jellyfin then returns `404`, the probe records `"missing"`, but the expected outcome is `"partial"` — so `passed` is `False` and the run fails for the Jellyfin deployment. The `self.variables["episodeId"]` key is populated at line 1117 whenever the first item is an episode, but is never used here. The probe should prefer `self.variables.get("episodeId", item_id)` to ensure it targets an episode when one is available. How can I resolve this? If you propose a fix, please make it concise.
-
tests/contracts/run_live_contracts.py, line 1194-1195 (link)additional_observedvacuously passes on an empty Items listIf the fixture item has no multipart parts, Jellyfin returns
{"Items": [], "TotalRecordCount": 0}with status200.all(...)over an empty iterable returnsTruevacuously, so the probe records"supported"without having seen a single part with a validId. This silently validates the route shape rather than its semantics. At minimum, the probe should fall through to"inconclusive"whenadditional_itemsis empty.Prompt To Fix With AI
This is a comment left during a code review. Path: tests/contracts/run_live_contracts.py Line: 1194-1195 Comment: **`additional_observed` vacuously passes on an empty Items list** If the fixture item has no multipart parts, Jellyfin returns `{"Items": [], "TotalRecordCount": 0}` with status `200`. `all(...)` over an empty iterable returns `True` vacuously, so the probe records `"supported"` without having seen a single part with a valid `Id`. This silently validates the route shape rather than its semantics. At minimum, the probe should fall through to `"inconclusive"` when `additional_items` is empty. How can I resolve this? If you propose a fix, please make it concise.
-
tests/contracts/run_live_contracts.py, line 1138-1139 (link)Noneaccepted as a validTypefor Next Up itemsentry.get("Type") in {None, "Episode"}allows items that are entirely missing theTypefield to pass the shape check. A server that omitsTypewould still be counted as producing "usable episode items", masking an incomplete response. Next Up items from Jellyfin always carryType: "Episode"; the check should require it explicitly.Prompt To Fix With AI
This is a comment left during a code review. Path: tests/contracts/run_live_contracts.py Line: 1138-1139 Comment: **`None` accepted as a valid `Type` for Next Up items** `entry.get("Type") in {None, "Episode"}` allows items that are entirely missing the `Type` field to pass the shape check. A server that omits `Type` would still be counted as producing "usable episode items", masking an incomplete response. Next Up items from Jellyfin always carry `Type: "Episode"`; the check should require it explicitly. How can I resolve this? If you propose a fix, please make it concise.
Prompt To Fix All With AI
Fix the following 3 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 3
tests/contracts/run_live_contracts.py:1206-1212
**Episode-specific probe uses generic `item_id` that may be a Movie**
The `segments.plugin-intro-skipper` probe hits `/Episode/{item_id}/IntroSkipperSegments`, but `item_id` is the first result from a `Movie,Episode` library query sorted by `SortName`. If a Movie title sorts before any Episode, `item_id` is a Movie ID. Jellyfin then returns `404`, the probe records `"missing"`, but the expected outcome is `"partial"` — so `passed` is `False` and the run fails for the Jellyfin deployment.
The `self.variables["episodeId"]` key is populated at line 1117 whenever the first item is an episode, but is never used here. The probe should prefer `self.variables.get("episodeId", item_id)` to ensure it targets an episode when one is available.
### Issue 2 of 3
tests/contracts/run_live_contracts.py:1194-1195
**`additional_observed` vacuously passes on an empty Items list**
If the fixture item has no multipart parts, Jellyfin returns `{"Items": [], "TotalRecordCount": 0}` with status `200`. `all(...)` over an empty iterable returns `True` vacuously, so the probe records `"supported"` without having seen a single part with a valid `Id`. This silently validates the route shape rather than its semantics. At minimum, the probe should fall through to `"inconclusive"` when `additional_items` is empty.
```suggestion
additional_observed = (
"supported" if additional.status == 200 and additional_items and all(isinstance(entry, dict) and entry.get("Id") for entry in additional_items)
else "inconclusive" if additional.status == 200 and not additional_items
else self._outcome_for_missing(additional)
)
```
### Issue 3 of 3
tests/contracts/run_live_contracts.py:1138-1139
**`None` accepted as a valid `Type` for Next Up items**
`entry.get("Type") in {None, "Episode"}` allows items that are entirely missing the `Type` field to pass the shape check. A server that omits `Type` would still be counted as producing "usable episode items", masking an incomplete response. Next Up items from Jellyfin always carry `Type: "Episode"`; the check should require it explicitly.
```suggestion
next_shape_ok = bool(next_items) and next_up.status == 200 and all(isinstance(entry, dict) and entry.get("Id") and entry.get("Type") == "Episode" for entry in next_items)
```
Reviews (1): Last reviewed commit: "test(contracts): add provider compatibil..." | Re-trigger Greptile
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
tests/contracts/validate_contracts.py (2)
104-109: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low value
native["detection"]is indexed without anisinstancedict check.Every other section (
outcomes,surfaces/deployments/contracts,snapshot,nativeitself) validatesisinstance(x, dict)before calling.get().detectiononly gets a truthiness check viabool(native.get(field))at line 107, so a malformed truthy non-dict value (e.g. a string) would raise an uncaughtAttributeErrorat line 108 instead of a cleanContractValidationError.♻️ Suggested refactor
for field in ("detection", "authenticationRoutes", "profileRoutes", "catalogRoutes", "playbackRoutes", "requiredHeaders", "identityRules", "playbackDecision"): _require(bool(native.get(field)), f"nativeSiloContract needs {field}") + _require(isinstance(native.get("detection"), dict), "nativeSiloContract detection must be an object") _require(native["detection"].get("path") == "/api/v1/health", "native detection must use /api/v1/health")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/contracts/validate_contracts.py` around lines 104 - 109, Update the native contract validation around native["detection"] to require that detection is a dict before calling .get() or indexing its fields. Preserve the existing required-field, path, and server_id validations while ensuring malformed truthy values raise the established ContractValidationError through _require.
72-79: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winStatus-only heuristic can be bypassed by other status codes.
The check only screens for the literal
"HTTP 200"string or the word"status"; arequiredSemanticsrule that only asserts a different status code (e.g. "Returns HTTP 404") without those substrings would pass as "behavior semantics" even though it's still just a bare status assertion, undermining the intent of the guard.♻️ Suggested refactor
+STATUS_ONLY_RE = re.compile(r"\bHTTP\s+\d{3}\b", re.IGNORECASE) + _require( - any("HTTP 200" not in rule and "status" not in rule.lower() for rule in required_semantics), + any(not STATUS_ONLY_RE.search(rule) and "status" not in rule.lower() for rule in required_semantics), f"{contract_id} must assert payload or behavior semantics, not only an HTTP status", )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/contracts/validate_contracts.py` around lines 72 - 79, Update the required_semantics validation in the contract-checking logic to recognize bare HTTP status assertions for any status code, not only rules containing “HTTP 200” or “status”. Preserve the existing requirement that at least one requiredSemantics rule asserts payload or behavior semantics, while allowing status-related text only when accompanied by substantive behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/provider-compatibility.md`:
- Around line 96-99: Update the BLOOM_CONTRACT_PASSWORD setup in the documented
probe so the value read by the silent prompt remains assigned and exported;
remove the no-value reset behavior from the final set -x command, while
preserving the existing BLOOM_CONTRACT_USERNAME configuration.
In `@tests/contracts/run_live_contracts.py`:
- Around line 26-29: Update the response helper’s json() method to catch
json.JSONDecodeError from json.loads(self.body) and return None for malformed
bodies, while preserving the existing None result for empty bodies and normal
decoded values for valid JSON. Do not change the individual call sites.
- Around line 82-86: Update the request() method to catch urllib.error.URLError
and TimeoutError alongside HTTPError, returning a sentinel Response for
transport failures so subsequent contract checks continue. Preserve the existing
HTTPError response conversion and use the established Response shape/status
convention for failures.
---
Nitpick comments:
In `@tests/contracts/validate_contracts.py`:
- Around line 104-109: Update the native contract validation around
native["detection"] to require that detection is a dict before calling .get() or
indexing its fields. Preserve the existing required-field, path, and server_id
validations while ensuring malformed truthy values raise the established
ContractValidationError through _require.
- Around line 72-79: Update the required_semantics validation in the
contract-checking logic to recognize bare HTTP status assertions for any status
code, not only rules containing “HTTP 200” or “status”. Preserve the existing
requirement that at least one requiredSemantics rule asserts payload or behavior
semantics, while allowing status-related text only when accompanied by
substantive behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 087b9a3b-bf3d-472c-9464-8b22a35b82ef
📒 Files selected for processing (8)
AGENTS.mddocs/provider-compatibility.mdnix/tests.nixtests/CMakeLists.txttests/contracts/provider-contracts.jsontests/contracts/provider_contracts_test.pytests/contracts/run_live_contracts.pytests/contracts/validate_contracts.py
e7dd699 to
9b6afad
Compare
|
Addressed the latest Greptile/CodeRabbit findings in
Re-ran the 10 offline tests and the pinned live Silo baseline: 27 matched, 2 explicitly inconclusive, 0 failed. |
1 participant
Summary
/api/v1assumptionsThe matrix is keyed by protocol surface and deployment rather than application-wide Jellyfin/Silo conditionals, so future sources can add a driver and deployment without changing existing contracts.
Closes #74
Parent: #73
Live baseline
Pinned Silo
8044eb84/ image digestsha256:944ee9…:/MediaSegments/{id}206byte-range streaming, observable progress reporting, stop, and reversible played/favorite changesUpstream follow-ups:
Test plan
python3 tests/contracts/validate_contracts.pypython3 tests/contracts/provider_contracts_test.py(8 tests)--allow-mutations(27 matched, 2 inconclusive, 0 failed)./scripts/dev-build.shnixfmt --check nix/tests.nixnix flake check --no-write-lock-filenix build --no-write-lock-fileDocumentation
See
docs/provider-compatibility.md. A maintainer review of the compatibility classifications and native contract assumptions is requested.Note
Add provider compatibility baseline and contract validation test suite
ProviderContractValidationTestin tests/CMakeLists.txt so CTest runs the Python-based contract tests; addspython3to the Nix test environment in nix/tests.nix.Macroscope summarized 9b6afad.
Summary by CodeRabbit
Documentation
Tests