You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
This PR is the design-first record for the source-scoped offline-media work in docs/task.md, P3.1. The current exact head 998305926d7782071177d4c282f3a47fa1661d3d must not merge because its
normative contract cannot yet be implemented safely against Tributary's current
identity, HTTP, and persistence model.
Exact-head review found these blockers:
Cached playback is still conditioned on live session/generation authority,
while disconnect behavior is described both as retiring and preserving the
row. That defeats the core offline-use contract. Durable saved-source/cache
authority must be separated from transient adapter authority.
An opaque remote TrackId can contain path separators, so it cannot be
interpolated directly into a cache path.
The proposed resume path truncates the destination prefix and has no stable
ETag/Last-Modified validator or If-Range contract, allowing revision
splicing instead of a valid resume.
Cross-filesystem copy/sync/delete is described as atomic, and publication by
rename occurs before integrity verification.
Mandatory server-advertised SHA-256 has no defined source for protocols that
do not provide one.
Disconnect, refresh, predecessor retention, and Denied authority semantics
contradict one another.
The proposed foreign keys target the local-only tracks table or an
in-memory source registry; neither can represent durable remote media/jobs
across restart.
Dependent work already uses a different offline_capability surface, so the
contracts must be reconciled before releasing it.
Quota order contradicts itself, filesystem deletion is incorrectly described
as atomic with a database transaction, and the proposed worker buffers an
unbounded whole-media Vec<u8> instead of a streamed body.
The retained PR should be corrected to define an encoded/collision-safe cache
identity, validator-bound resume, verify-before-publish semantics, truthful
cross-filesystem durability, obtainable integrity evidence, a single-valued
live-versus-durable authority model, streamed bounded I/O, durable
remote-media/download-job/reconciliation tables, and truthful quota/eviction
semantics matching the actual repository and adapters.
This is documentation-only, but green compilation/CI is not evidence that a
normative design is coherent. Dependents remain blocked until a corrected exact
head passes independent source/design review.
Understand the request (code review of docs-only PR)
Review docs/offline-media.md for correctness, consistency, security implications
Review docs/task.md change
Post review feedback
This is a docs-only change (docs/offline-media.md, +479, new; docs/task.md, +2/-1). Since there's no runtime code, the review focused on internal consistency, accuracy of cross-references, and whether the contract is implementable as written.
What holds up well:
Cross-references resolve correctly: task.md#p31--offline-remote-media, #credential-boundary, and all sibling docs (source-scoped-playlists.md, subsonic-playlist-sync.md, architecture/source-lifecycle.md, lastfm-scrobbling.md, task-remediation-2026-07.md) exist with matching content.
The PR Compose Last.fm application owner #165 reference (Last.fm application owner) and the TRIBUTARY_* naming (matches existing usage in src/panic_reporting.rs, src/lastfm/client.rs, etc.) both check out against the actual repo state.
The "reversible like migration 13" claim matches m20260719_000013_source_scoped_playlist_entries.rs, which does wrap schema changes in a transaction that rolls back and preserves the original error on failure.
docs/task.md diff is minimal and correctly adds the offline-media.md link to the P3.1 checkbox without altering its unchecked state, consistent with the design-first delivery-boundary claims in the PR body.
Referenced code symbols (MediaBackend trait, ServerPlaylist, chromecast_output.rs, http_security.rs) all exist in src/.
Two internal inconsistencies filed as inline comments:
Misattributed "verbatim" quote (docs/offline-media.md:101-102) — "no token in MPD/Chromecast/AirPlay ticket" is cited as a verbatim P1.4 rule, but that string doesn't appear anywhere in task-remediation-2026-07.md. The MPD/Chromecast/AirPlay credential-free ticket behavior is actually P1.6's contribution ("Stop handing backend credentials to receivers"), not P1.4's (exact-origin redirects). The same P1.4-only attribution recurs at lines 72, 264, and 416 for content that appears to be P1.6's.
Undefined Denied error variant (docs/offline-media.md:128-130, repeated at :386) — offline_snapshot() is typed -> Result<Option<OfflineSnapshot>, OfflineError> and the doc says sources return Err(Denied) to refuse, but the OfflineError variant list enumerated at lines 180-184 has no Denied variant (it has a distinct LicenceDenied for a different concept). As written, this isn't constructible against the declared type.
Minor / non-blocking observation: The eviction policy in "Cancellation, quota, and eviction" (docs/offline-media.md:344-346) — "newest-first within source, oldest-first across sources" — is an unusual ordering (evicting a source's newest downloads before its oldest). It reads intentional rather than accidental, but may be worth a one-line rationale so implementers don't second-guess it.
No security, performance, or testing concerns apply — this slice deliberately introduces no schema, runtime code, or credential-handling implementation (correctly documented as such in "Status and delivery boundary"), so those review dimensions don't have surface area here yet.
The PR adds a design contract for source-scoped offline media, covering identity, capabilities, downloads, storage, licensing, reconciliation, UI behavior, migration, validation, and compatibility. It also links the contract from the task checklist.
Changes
Offline media contract
Layer / File(s)
Summary
Identity, capability, and download contract docs/offline-media.md
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR establishes the normative design contract for source-scoped offline media. While the design-first approach is sound, the current documentation contains high-risk security flaws and logical inconsistencies that must be resolved before this serves as a reference for implementation. Most notably, using un-normalized TrackIds in file paths poses a path traversal risk, and the mandatory SHA-256 integrity requirement is incompatible with several target backends (Subsonic, Plex, Jellyfin).
Codacy analysis indicates the PR is not up to standards, primarily due to systemic markdown linting violations (MD004). These style issues, along with the identified design gaps in eviction and cache retirement policies, should be addressed to provide a stable foundation for the upcoming download engine.
About this PR
This PR is a documentation-only design contract. While this aligns with the design-first requirement, ensure that the functional implementation following this PR strictly validates these boundaries, as no automated validation of these contracts is possible within this documentation-only slice.
Test suggestions
Verify identity mapping uses (SourceId, TrackId) without minting new types
Verify resumable download job correctly handles partial bytes and range requests
The reason will be displayed to describe this comment to others. Learn more.
🔴 HIGH RISK
Using an opaque and un-normalized track_id directly in a filesystem path is a security and stability risk. Since TrackId is provided by remote sources, it could contain path traversal characters (e.g., .., /) or characters invalid on specific operating systems. Specify that the track_id must be hashed (e.g. SHA-256) before being used as a directory name on disk to ensure filesystem safety and cross-platform compatibility.
| Plex | `GET /library/parts/<partId>` authenticated through the exact-origin proxy; uses `X-Plex-Token` only inside the proxy boundary. | Identical. | Same. |
| DAAP | `DAAP.song` request, authenticated through the DAAP protocol-specific lane already retired to the source lifecycle. | Identical. | DAAP connection still has exactly-once logout; cache rows must retire on disconnect. |
| Radio-Browser | Disallowed. | — | Streams are public and not licensable for offline by default; deny hard. |
| Built-in local | Disallowed. | — | Local files are already local; the cache is the filesystem. |
The reason will be displayed to describe this comment to others. Learn more.
🟡 MEDIUM RISK
There is a contradiction in the design regarding the lifecycle of DAAP cache rows. This line states DAAP cache rows must retire on disconnect, but line 282 states that session revocation retires only the in-flight lease and NOT the cache row. These requirements should be reconciled.
The reason will be displayed to describe this comment to others. Learn more.
🟡 MEDIUM RISK
Most target backends (Subsonic, Jellyfin, Plex) do not typically provide SHA-256 digests for media streams in their HTTP responses. If the contract mandates a match against a 'server-advertised digest' as a terminal requirement for playability, these sources will be unable to satisfy the contract. Update the 'Integrity' and 'Atomic storage' sections to allow for optional integrity verification (e.g. falling back to size-only check) when a SHA-256 digest is not advertised.
The reason will be displayed to describe this comment to others. Learn more.
Citation doesn't check out. The quoted phrase "no token in MPD/Chromecast/AirPlay ticket" is attributed to task-remediation-2026-07.md P1.4 "verbatim," but that exact string doesn't appear anywhere in task-remediation-2026-07.md. Looking at that file, P1.4 ("Enforce exact-origin authenticated redirects") covers redirect-matrix/Referer/HTTPS-downgrade behavior; the credential-free MPD/Chromecast/AirPlay ticket behavior this line describes is actually documented under P1.6 ("Stop handing backend credentials to receivers", task-remediation-2026-07.md:633).
Same misattribution recurs elsewhere in this doc — lines 72, 264, and 416 all cite "P1.4" for behavior (credential-free GTK rows, receiver ticket boundary) that P1.6 actually established. Worth either fixing the citations to point at P1.6 where appropriate, or dropping the "verbatim" framing since no such literal sentence exists in the cited file.
The reason will be displayed to describe this comment to others. Learn more.
Denied isn't a defined OfflineError variant. This adapter is typed as offline_snapshot() -> Result<Option<OfflineSnapshot>, OfflineError>, and the text says a source returns Err(Denied) to explicitly refuse. But the OfflineError enum is later enumerated (lines 180–184) as exactly Network, AuthExpired, LeaseRevoked, IntegrityMismatch, LicenceDenied, QuotaExceeded, StorageUnavailable, UnsupportedSource — no Denied variant appears. The failure-modes table at line 386 repeats Err(Denied) the same way.
As written, an implementer following this contract literally can't construct Err(Denied) against the declared OfflineError type. Either Denied needs to be added to the enumerated variant list, or the two Err(Denied) call sites should be reconciled with an existing variant (e.g. is this meant to be distinct from LicenceDenied, which is a separate concept in the Licensing section?).
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 10
🤖 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/offline-media.md`:
- Around line 207-213: Update the cross-filesystem fallback described in the
“Atomic rename” step to copy into a temporary file on the destination
filesystem, fsync that temporary file, atomically rename it to the final cache
path, and delete the source only after the destination rename succeeds. Preserve
the existing single-step rename path and prevent direct writes to the final
destination.
- Around line 457-460: Update the offline subsystem retirement guidance to
define how on-disk cache files are handled before dropping the offline tables,
indexes, and triggers: specify that cache files are unlinked or reconciled while
their metadata remains available, or explicitly document intentional retention
and ownership. Keep the migration’s single-transaction behavior clear.
- Around line 227-230: The cache refresh flow must keep the predecessor snapshot
available until the replacement is fully committed, rather than exposing only
the live endpoint during steps 1–5. Update the lookup and reconciliation rules
described in the offline-media contract so valid offline items continue
resolving through the previous snapshot until step 5 succeeds, while preserving
the prohibition on exposing partial or half-promoted cache rows.
- Around line 451-455: Update the offline compatibility description near the
OfflineSnapshot::None discussion: replace the byte-level database identity claim
with the invariant that databases have the same logical application data and
contain no offline cache rows. Keep the statements about runtime machinery and
opt-out semantics aligned with this logical-data definition.
- Around line 119-120: Update the offline media documentation’s TrackId path
layout to define a collision-safe, path-safe filesystem encoding for opaque IDs,
such as an explicit reversible encoding, while retaining the original TrackId
only as metadata. Clarify that the download engine must not place the raw ID
directly in paths or parse, normalize, or hash it, and apply the same definition
to the additional TrackId layout described later in the document.
- Line 252: Resolve the conflicting DAAP disconnect lifecycle by choosing a
single rule for committed cache rows versus in-flight leases, then apply it
consistently across the capability matrix, credential/session revocation
section, logout behavior, failure table, and related tests in
docs/offline-media.md. Update the DAAP-specific statements and assertions so
disconnect and revocation semantics match the selected rule.
- Around line 173-176: The offline media documentation must distinguish
fresh-download and resume file-opening modes: preserve the committed prefix
during resumption without truncating or reopening in append mode, validate the
existing temporary-file prefix byte-for-byte, and seek to the committed offset
before writing the next range. Update both the resumption section and the
corresponding later guidance around the storage contract.
- Around line 342-346: Align the normative eviction policy and the
validation/regression expectation to one ordering: oldest-cache-first across
sources, with newest-first eviction within each source. Update the contradictory
newest-source-first wording wherever it appears, including the additional
referenced occurrence, without changing the documented quota behavior.
- Around line 26-29: Clarify the documentation’s contract for Denied entries:
state explicitly whether Denied is only an ephemeral admission/job state or
whether it creates a stored catalogue row, and make the central rule and
licence-table wording consistent with that decision. Update the related
references to Denied so they describe the same persistence and playability
behavior.
- Around line 214-216: Update the “Post-write integrity check” section to
identify the exact trusted digest source for each adapter, such as the relevant
response header or manifest field, and state that downloads are rejected when
the server-provided digest is missing. Ensure current_sha256 is compared only
with that explicitly trusted value.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: deaf82cb-4765-45a4-a749-faf688bcf168
📥 Commits
Reviewing files that changed from the base of the PR and between 6afb427 and 126125c.
The reason will be displayed to describe this comment to others. Learn more.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Define a path-safe encoding for TrackId.
TrackId is opaque and must not be parsed, normalized, or hashed, but the layout places it in a filesystem path. IDs containing separators, traversal components, or filesystem-invalid characters could escape the source directory or fail on disk. Define a collision-safe filesystem representation and retain the original ID only as metadata.
Also applies to: 234-240
🤖 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 `@docs/offline-media.md` around lines 119 - 120, Update the offline media
documentation’s TrackId path layout to define a collision-safe, path-safe
filesystem encoding for opaque IDs, such as an explicit reversible encoding,
while retaining the original TrackId only as metadata. Clarify that the download
engine must not place the raw ID directly in paths or parse, normalize, or hash
it, and apply the same definition to the additional TrackId layout described
later in the document.
The reason will be displayed to describe this comment to others. Learn more.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Do not truncate the temp file when resuming.
The job contract reuses the previously committed prefix, but the storage contract mandates truncate(true) and forbids reopening for append. Following the latter destroys the prefix and invalidates current_bytes. Specify separate fresh-download and resume open modes, including prefix validation and seeking before the next range.
Also applies to: 203-206
🤖 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 `@docs/offline-media.md` around lines 173 - 176, The offline media
documentation must distinguish fresh-download and resume file-opening modes:
preserve the committed prefix during resumption without truncating or reopening
in append mode, validate the existing temporary-file prefix byte-for-byte, and
seek to the committed offset before writing the next range. Update both the
resumption section and the corresponding later guidance around the storage
contract.
The reason will be displayed to describe this comment to others. Learn more.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bashset -euo pipefail
# Inspect the relevant doc section and related digest terminology.
git ls-files docs/offline-media.md
echo"----"
cat -n docs/offline-media.md | sed -n '180,250p'echo"---- SEARCH current_sha256 / digest terms ----"
rg -n "current_sha256|server-advertised digest|digest|sha256|checksum|manifest|header" docs offline-media.md . -g '!**/node_modules/**'
Repository: jm2/tributary
Length of output: 50370
🏁 Script executed:
#!/bin/bashset -euo pipefail
# Narrow search to likely source/docs files for the digest contract.
rg -n "current_sha256|server-advertised digest|digest|sha256|checksum|manifest|header". \
-g '!**/node_modules/**' -g '!**/dist/**' -g '!**/build/**' -g '!**/.git/**'
Repository: jm2/tributary
Length of output: 50370
🏁 Script executed:
#!/bin/bashset -euo pipefail
# If the document references adapters or download flows, inspect nearby sections too.
rg -n "adapter|download|integrity check|current_sha256|digest" docs/offline-media.md
Repository: jm2/tributary
Length of output: 5249
🏁 Script executed:
#!/bin/bashset -euo pipefail
# Map the doc structure first if the file is small enough.
wc -l docs/offline-media.md
ast-grep outline docs/offline-media.md --view expanded
#!/bin/bashset -euo pipefail
# Read the precise sections that define the download contract and adapter matrix.
cat -n docs/offline-media.md | sed -n '60,90p'echo"----"
cat -n docs/offline-media.md | sed -n '149,170p'echo"----"
cat -n docs/offline-media.md | sed -n '214,260p'echo"----"
cat -n docs/offline-media.md | sed -n '372,405p'
Repository: jm2/tributary
Length of output: 9174
🏁 Script executed:
#!/bin/bashset -euo pipefail
# Find any explicit missing-digest handling in the offline-media spec.
rg -n "missing|absent|without one|reject downloads|digest source|trusted digest|advertised digest|current_sha256" docs/offline-media.md README.md CHANGELOG.md docs/task.md docs/task-remediation-2026-07.md
Repository: jm2/tributary
Length of output: 43895
Define the digest source and missing-digest behavior.current_sha256 still compares against a “server-advertised digest,” but this contract never names the header/manifest field or says whether missing digests fail closed. Spell out the trusted field per adapter, or reject downloads without one. docs/offline-media.md:214-216
🤖 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 `@docs/offline-media.md` around lines 214 - 216, Update the “Post-write
integrity check” section to identify the exact trusted digest source for each
adapter, such as the relevant response header or manifest field, and state that
downloads are rejected when the server-provided digest is missing. Ensure
current_sha256 is compared only with that explicitly trusted value.
Keep the predecessor available until refresh commits.
The lookup rule returns only the live endpoint during steps 1–5, while reconciliation requires the previous snapshot to remain available until the replacement is committed. A refresh can therefore make a valid offline item unavailable before the new snapshot is ready.
Also applies to: 322-324
🤖 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 `@docs/offline-media.md` around lines 227 - 230, The cache refresh flow must
keep the predecessor snapshot available until the replacement is fully
committed, rather than exposing only the live endpoint during steps 1–5. Update
the lookup and reconciliation rules described in the offline-media contract so
valid offline items continue resolving through the previous snapshot until step
5 succeeds, while preserving the prohibition on exposing partial or
half-promoted cache rows.
| Subsonic | `GET .../download?view=...&id=<trackId>` authenticated through the exact-origin proxy. | Per-source byte total bounded at the source-adapter-declared cap; offline rows are still capped by the per-track quota. | Bearer URL handling per `task-remediation-2026-07.md` P1.6 — only the proxy ticket ever reaches GTK. |
| Jellyfin | `GET /Items/<id>/Download` authenticated through the exact-origin proxy. | Identical. | Same. |
| Plex | `GET /library/parts/<partId>` authenticated through the exact-origin proxy; uses `X-Plex-Token` only inside the proxy boundary. | Identical. | Same. |
| DAAP | `DAAP.song` request, authenticated through the DAAP protocol-specific lane already retired to the source lifecycle. | Identical. | DAAP connection still has exactly-once logout; cache rows must retire on disconnect. |
The reason will be displayed to describe this comment to others. Learn more.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Resolve DAAP disconnect semantics.
The capability matrix says cache rows retire on DAAP disconnect, while the credential section says session revocation retires only the in-flight lease and not the committed cache row. Choose one lifecycle rule and align the matrix, logout behavior, failure table, and tests.
Also applies to: 281-283
🤖 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 `@docs/offline-media.md` at line 252, Resolve the conflicting DAAP disconnect
lifecycle by choosing a single rule for committed cache rows versus in-flight
leases, then apply it consistently across the capability matrix,
credential/session revocation section, logout behavior, failure table, and
related tests in docs/offline-media.md. Update the DAAP-specific statements and
assertions so disconnect and revocation semantics match the selected rule.
The reason will be displayed to describe this comment to others. Learn more.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Use one eviction ordering.
The policy specifies oldest-cache-first across sources, but the validation table says newest-source-first eviction. These produce different deletion results; make the normative policy and regression expectation identical.
Also applies to: 421-421
🤖 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 `@docs/offline-media.md` around lines 342 - 346, Align the normative eviction
policy and the validation/regression expectation to one ordering:
oldest-cache-first across sources, with newest-first eviction within each
source. Update the contradictory newest-source-first wording wherever it
appears, including the additional referenced occurrence, without changing the
documented quota behavior.
The reason will be displayed to describe this comment to others. Learn more.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Replace the byte-level compatibility claim.
Adding an offline table necessarily changes database bytes even when it is empty. The invariant should be “same logical application data and no offline rows,” otherwise this statement cannot be true or tested.
🤖 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 `@docs/offline-media.md` around lines 451 - 455, Update the offline
compatibility description near the OfflineSnapshot::None discussion: replace the
byte-level database identity claim with the invariant that databases have the
same logical application data and contain no offline cache rows. Keep the
statements about runtime machinery and opt-out semantics aligned with this
logical-data definition.
The reason will be displayed to describe this comment to others. Learn more.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Define cache-file cleanup before dropping offline tables.
Dropping the metadata tables without unlinking or reconciling on-disk cache files leaves unreachable media consuming storage. Specify cleanup ordering, or explicitly document intentional retention and ownership.
🤖 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 `@docs/offline-media.md` around lines 457 - 460, Update the offline subsystem
retirement guidance to define how on-disk cache files are handled before
dropping the offline tables, indexes, and triggers: specify that cache files are
unlinked or reconciled while their metadata remains available, or explicitly
document intentional retention and ownership. Keep the migration’s
single-transaction behavior clear.
Adds the design-first contract required by task.md:21-22 before the P3.1
issue #11 implementation lands. The document fixes the persistent
source-scoped identity shape, the authenticated/resumable download job
model, atomic temp-then-rename storage, the per-source capability matrix,
the credential boundary (matching P1.4 exact-origin / no-Referer policy
and the 2026-07-13 authenticated redirect policy), the licensing row
types, and the reconciliation rules. It deliberately introduces no
schema, migration, runtime worker, GTK widget, or HTTP client; those
follow implementation slices after this contract is accepted.
The design reuses the source-scoped (SourceId, TrackId) identity shape
already established in architecture/source-lifecycle.md and
source-scoped-playlists.md, so the offline cache does not introduce a
parallel identity kind. The download engine is not a SourceKind; it is a
state change for an existing source-owned media row.
Links the design document into the P3.1 checkbox in task.md and keeps
that checkbox unchecked — the design is a fleet-completable artifact but
the implementation record remains open until the download/cache engine
and storage UI land.
…view)
Review-blocking fixes for tr-92a:
- docs/offline-media.md:170-172: drop mol-polecat-work application-owner
reference; point at the Last.fm application owner composed in #165 as the
reference shape for the offline-job supervisor.
- docs/offline-media.md:285-287: replace GC_* build-time prefix with
TRIBUTARY_* (the real project prefix used by src/lastfm/client.rs for
TRIBUTARY_LASTFM_API_KEY / TRIBUTARY_SHARED_SECRET).
- Qualify every P1.4/P1.5/P1.6/P2.3 anchor to task-remediation-2026-07.md
(these resolved against task.md which has different P-codes for the same
labels) and add task-remediation-2026-07.md to the See also list with the
P-code map.
- Fix the 'Path 2.3' typo at line 326 to 'P2.3'.
The contract itself is unchanged; only surface vocabulary and citation
disambiguation. Docs-only diff; no Rust changes.
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🤖 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/offline-media.md`:
- Around line 127-135: Resolve the offline_snapshot() unsupported-source
contract consistently: choose either None or Err(Denied) for Radio-Browser and
other disallowed adapters, then update the adapter rules, failure table, and
related tests to match that choice while preserving the documented distinction
between undeclared and explicitly refused capabilities.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 329df48b-ce1a-4281-bcff-be482626a0a3
📥 Commits
Reviewing files that changed from the base of the PR and between 126125c and 9983059.
📒 Files selected for processing (2)
docs/offline-media.md
docs/task.md
🚧 Files skipped from review as they are similar to previous changes (1)
The reason will be displayed to describe this comment to others. Learn more.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Align unsupported-source return semantics.
The capability contract requires Radio-Browser and other disallowed adapters to return None, but the failure table says Radio-Browser returns Err(Denied). Since the document explicitly distinguishes these outcomes, choose one behavior and update the adapter rules, failure table, and tests consistently.
Also applies to: 386-387
🤖 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 `@docs/offline-media.md` around lines 127 - 135, Resolve the offline_snapshot()
unsupported-source contract consistently: choose either None or Err(Denied) for
Radio-Browser and other disallowed adapters, then update the adapter rules,
failure table, and related tests to match that choice while preserving the
documented distinction between undeclared and explicitly refused capabilities.
What commit 9983059 actually fixed (matches the PR body's "Resumed from rejection" note): dropped the mol-polecat-work reference in favor of the real Last.fm application-owner PR (#165), replaced GC_* with TRIBUTARY_*, added a filename qualifier (task-remediation-2026-07.md) to every bare P-code citation, added that file to "See also," and fixed the "Path 2.3" → "P2.3" typo. This is a narrow, correctly-scoped fix for exactly what it claims.
Two prior findings are marked "✅ Addressed" by CodeRabbit but are not actually fixed. I checked: the Denied-persistence contradiction (lines 26-29, 295-299) and the cross-filesystem atomic-rename fallback (lines 210-213) are byte-identical to the versions CodeRabbit originally flagged. CodeRabbit's tracker cites the range "commits 5f9dd50 to 9983059" — I inspected 5f9dd50 directly and it's an unrelated commit (test: await Last.fm failure publication (tr-d3f), touching only src/lastfm/production.rs on main) that never touches docs/offline-media.md. The "addressed" annotation appears to be tracking the base branch moving forward between review passes, not a real fix in this PR — left inline comments re-opening both.
The P1.4/P1.6 misattribution I raised last time is only half-fixed. Line 101-102 now cites task-remediation-2026-07.md P1.4 for the "no token in MPD/Chromecast/AirPlay ticket" claim — same milestone number as before, just with the filename added. I read the source file directly this time: task-remediation-2026-07.md:652 (P1.6, "Output boundary") is what actually establishes credential-free tickets for MPD/Chromecast/AirPlay/local GStreamer; P1.4 (:606-622) is the exact-origin/redirect policy and doesn't mention MPD or Chromecast at all. Left an inline comment with the citation.
Everything else CodeRabbit and Codacy already flagged remains open (this commit didn't touch it, and the PR body doesn't claim otherwise): Err(Denied) isn't a defined OfflineError variant (128-130, 386); Radio-Browser's None vs. Err(Denied) contradiction (133-135 vs. 386, re-flagged by CodeRabbit's own 2026-07-27 pass on this same commit); the temp-file truncate-on-resume contradiction (173-176 vs. 203-206); undefined digest source for integrity checks (214-216); the DAAP disconnect contradiction (252 vs. 281-283, cache row retires on disconnect and survives revocation); the eviction-ordering contradiction between the normative policy and the validation table (344-346 vs. 421); the byte-level database-identity claim (451-455); and TrackId-in-path-safety (119-120, 234-240). None of these are new findings — flagging so the open count is accurate against what's actually in the diff versus what the bot trackers claim.
Docs-only change; no runtime/security/test surface beyond what's already covered above.
The reason will be displayed to describe this comment to others. Learn more.
Still misattributed to P1.4 — this is P1.6's territory. Commit 9983059 added the task-remediation-2026-07.md filename qualifier here but didn't correct the milestone number. I checked the cited file directly: task-remediation-2026-07.md:652 (P1.6, "Output boundary" row) states "Chromecast, MPD, local GStreamer, and AirPlay exchange it for their existing opaque, receiver-reachable tickets; none can fall back to the clean endpoint or serialized credential state." — that's the actual source of the "no token in MPD/Chromecast/AirPlay ticket" claim. P1.4 (task-remediation-2026-07.md:606-622) is the exact-origin/redirect policy; it does touch local/AirPlay routing, but the credential-free-ticket guarantee for all four receivers (including MPD/Chromecast, which P1.4 doesn't mention at all) is P1.6's contribution.
This is the same issue flagged in the original review (also recurring at lines 264 and 416) — worth fixing for real this time rather than just adding the filename.
The reason will be displayed to describe this comment to others. Learn more.
CodeRabbit's tracker marked this "✅ Addressed in commits 5f9dd50 to 9983059," but the text is unchanged — Denied still says "The cached row exists structurally" while Persistent is marked No in the same row, and the central-rules text at lines 26-29 is byte-identical to the prior review. I checked commit 5f9dd50 directly: it's test: await Last.fm failure publication (tr-d3f), touching only src/lastfm/production.rs on main — it never touches this file. The "addressed" marker looks like it's tracking the base branch moving forward between reviews, not an actual fix in this PR. This finding (and the atomic-rename one at lines 210-213, marked the same way) is still open.
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
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.
![codacy-production[bot]](https://avatars.githubusercontent.com/in/56611?s=60&v=4){kind=small}
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Draft — contract correction required
This PR is the design-first record for the source-scoped offline-media work in
docs/task.md, P3.1. The current exact head998305926d7782071177d4c282f3a47fa1661d3dmust not merge because itsnormative contract cannot yet be implemented safely against Tributary's current
identity, HTTP, and persistence model.
Exact-head review found these blockers:
while disconnect behavior is described both as retiring and preserving the
row. That defeats the core offline-use contract. Durable saved-source/cache
authority must be separated from transient adapter authority.
TrackIdcan contain path separators, so it cannot beinterpolated directly into a cache path.
ETag/Last-Modified validator or
If-Rangecontract, allowing revisionsplicing instead of a valid resume.
rename occurs before integrity verification.
do not provide one.
Deniedauthority semanticscontradict one another.
trackstable or anin-memory source registry; neither can represent durable remote media/jobs
across restart.
offline_capabilitysurface, so thecontracts must be reconciled before releasing it.
as atomic with a database transaction, and the proposed worker buffers an
unbounded whole-media
Vec<u8>instead of a streamed body.The retained PR should be corrected to define an encoded/collision-safe cache
identity, validator-bound resume, verify-before-publish semantics, truthful
cross-filesystem durability, obtainable integrity evidence, a single-valued
live-versus-durable authority model, streamed bounded I/O, durable
remote-media/download-job/reconciliation tables, and truthful quota/eviction
semantics matching the actual repository and adapters.
This is documentation-only, but green compilation/CI is not evidence that a
normative design is coherent. Dependents remain blocked until a corrected exact
head passes independent source/design review.
Issue:
tr-92a