facad78d09bbeddf4645d52f2b5f3a5a41e097db
881
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 QuickandClaude Fable 5
|
facad78d09 |
feat(settings): serve admin user-settings through the canonical API
Replace the ten string-registry /admin/users/{id}/settings* and
device-settings* routes with the canonical contract surface: one list of
every explicit value the target user has stored across all scopes, and
set/delete at an explicit scope named in the query string.
The admin handlers live on SettingValuesHandler and share the session
routes' implementation rather than duplicating it — the same key/scope
parsing, identity validation, contract scope allowance, value
normalization and mutation-receipt idempotency, factored into
keyedScopeFromRequest/completeIdentity and setValueAt/deleteValueAt.
The only admin-specific parts are the target user coming from the path,
profile and device ids coming from the query (an admin holds no session
claim to the user being inspected, so its named profile is checked to
exist), and change events attributed to the target user so their
clients refresh.
The list is a new UserStore read, ListAllSettingValues, implemented in
both backends and pinned by the shared storetest conformance suite:
the admin surface wants the stored truth (which overrides exist, for a
per-row reset affordance), which no resolution-shaped read answers.
The ten removed routes are recorded in the pre-lock removals table in
docs/architecture/v1-scope.md per the v1 API rules; the web admin
device-overrides page moves onto the new surface in the Phase B
rewrite inside this same unmerged PR.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
||
|
QuickandClaude Fable 5
|
440a75b189 |
refactor(jellycompat): give DisplayPreferences its own table
The Jellyfin DisplayPreferences blobs rode the legacy user_settings key/value table under synthetic jellycompat:* keys, which forced the legacy settings API to carry a prefix carve-out in its otherwise-closed unknown-key gate. They are the compat subsystem's storage, not user settings: the contract neither validates nor resolves them. Move them to a dedicated jellycompat_displayprefs table in both backends, keyed by (prefs id, client) per user, with the blob stored as opaque text served back byte-for-byte (deliberately not jsonb, which would re-serialize it). The data-copy migrations — per-user SQLite V16 and a paired SQL + Go goose migration for Postgres — are transactional and harmless to re-run, and both drive their key parsing and row classification from the new internal/jellycompat/displayprefs package so the backends cannot diverge, following the internal/settingsmigrate precedent. A jellycompat:* row that does not parse as a DisplayPrefs key (only ever writable through the removed carve-out) is recorded in user_setting_migration_rejects rather than silently deleted. With the last non-settings tenant gone, the jellycompatSettingPrefix carve-out is deleted: the legacy settings endpoints now refuse jellycompat:* keys like any other unknown key and never surface them. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> |
||
|
QuickandClaude Fable 5
|
22e9d7f11d |
refactor(settings): resolve metadata language canonically in access and policy
Repoint the last legacy column readers onto canonical contract resolution (settings cutover task A4a): - access.Resolver and policy.ViewerResolver now resolve catalog.metadata_language through settingsresolve (profile scope -> contract default) via a shared access.PreferredMetadataLanguage helper, instead of reading user_profiles.preferred_metadata_language. Resolution is deliberately unconstrained: the policy input this preference feeds is the one a constraint would have to reference, which is circular — see the key's manifest notes. - playback start now resolves playback.audio_language canonically for the profile default instead of reading user_profiles.language, matching the catalog detail path. Series and library override handling is unchanged. - items.go needed no change: it already consumes the resolver-produced scope.PreferredMetadataLanguage. The legacy columns keep their values but are no longer read on these paths; a profile with only a column value now resolves to the contract default, and a stored canonical value wins. Tests pin both directions in access, policy (including scope parity, where the column is now a decoy), and the playback handler. Read cost is one batched store read per resolution, same as the profile-row read it replaces. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> |
||
|
QuickandClaude Fable 5
|
4fe417ce27 |
feat(settings): sweep expired mutation receipts daily
Setting-mutation idempotency receipts were written with an expires_at that nothing enforced, so the table grew forever. Add a hidden daily system task (05:00) that walks every login account, opens its user store, and calls DeleteExpiredSettingMutations. A user whose store fails to open or sweep is logged and skipped so one broken store cannot stall retention for everyone else; the delete is idempotent, so the next run repairs anything missed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> |
||
|
QuickandClaude Fable 5
|
2d74be984f |
feat(settings): publish user_settings change events
Add a user_settings realtime channel so clients learn when a setting
changed on another device without polling. The channel is modeled on
user_state: non-admin subscribable, per-user addressed envelopes, null
snapshot.
SettingValuesHandler gains an EventsHub and publishes
user_settings.changed after every successful PUT and DELETE on
/settings/values/{key}. The payload carries only key, scope and
profile_id — never the value. Admins receive every user's user-scoped
events, so a value in the payload would leak private settings to
admins; interested clients re-fetch over the scoped REST API instead.
The payload is always non-empty because an empty Data falls back to a
null snapshot in the hub. A nil hub (tests) skips publishing.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
||
|
QuickandClaude Fable 5
|
40e0f77a74 |
feat(settings): enforce viewer quality caps through resolver constraints
constraintsFor was the unwired half of the preferences-versus-restrictions
seam: it returned nil, so a profile capped at 1080p by policy still resolved
its stored 2160p preference at face value through the effective endpoint.
The settings routes are mounted inside RequireViewerAccess, so the resolved
access scope is already on the request context. Scope.MaxPlaybackQuality
holds a literal member of the contract's quality enum ("1080p"/"2160p"),
which is exactly what the manifest binds playback.preferred_quality's
ceiling to under policy_input "max_playback_quality" — so the wiring is a
direct map with no translation table. An empty value means the policy sets
no cap, expressed by returning nil so the resolver leaves the preference
alone.
catalog.metadata_language deliberately stays unconstrained: the manifest
notes record that the allowlist draft was circular (the policy input it
would bind to is populated from the very preference it would narrow).
The handler test covers both halves of the seam: a 2160p preference under a
1080p cap resolves to the cap with constrained:true/ceiling and the authored
value reported in stored_value, the stored row itself is not rewritten, and
an uncapped viewer gets the preference unchanged with no constraint noise.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
||
|
QuickandClaude Opus 5
|
9ed0aefd66 |
feat(settings): close the unknown-key extension bag
keyUsesUserScope returned true for any key the registry did not know, so a client could invent a production setting unilaterally and the server stored it as an unvalidated string. That is how six ui.* settings and five orphan keys reached production untyped, and it is the root enabler the design names. An unknown key is no longer a user setting, so the legacy write path rejects it and the canonical API — which validates every value against its own definition — is the only way to store something new. jellycompat's DisplayPreferences blobs ride the same table under synthetic keys and keep working: they are that subsystem's storage rather than user settings, and they move to dedicated storage in the follow-up rather than being dropped here. Also repoints the DisplayPreferences seed at the canonical resolver. Resolved at profile scope with no device on purpose — Jellyfin clients do not carry Silo's device identity, so a device override leaking into the seed would hand one device's settings to every Jellyfin client on the account. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
QuickandClaude Opus 5
|
3d039e163c |
feat(settings): resolve catalog playback preferences through the contract
catalog/detail.go held the two hardest ladders in the codebase: subtitles resolved across four levels by hand, audio across three, each partially overriding the last through Has* flags. Both now call the canonical resolver, so the precedence lives in the manifest and this file cannot disagree with the contract about which override wins. Adding a scope is a manifest change rather than another branch here. The subtitle track signature stays on its specialized table — it identifies a concrete track rather than expressing a preference, so it is not a setting. Resolution keeps the memoization the old lookups had: the audio resolver still reads once per profile and once per library rather than once per file, which is what kept a many-track audiobook detail page fast. The test that guards it now counts resolver reads instead of GetProfile calls, since the guarantee is about scaling with file count rather than about which method does the reading. Four tests seeded the profile column directly. That column is a migration source now, not a read path, so they seed the canonical value instead — they were passing against storage nothing reads. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
QuickandClaude Opus 5
|
0ed699953f |
feat(web): add the two-axis quality picker and typed settings hooks
Quality becomes one picker over two stored values.
The server holds a resolution cap and a bandwidth cap independently, which
is what the player has always sent on the wire — useTranscodeQuality.ts has
decomposed 1080p-high into {resolution, bitrate} for as long as it has
existed. Presets live in the client rather than the contract so retuning
what "High" means is a one-line edit here instead of a contract change four
codebases have to agree on, and an older server keeps working because it
only ever sees the two axes it already understands.
A combination no preset covers still gets a truthful label rather than a
picker showing the wrong entry: reachable by setting the axes separately
through the API, or from a legacy value whose bitrate is off this ladder.
Choosing an uncapped preset clears the bitrate rather than storing a
sentinel, so "no cap" stays the absence of a value at every layer.
Adds hooks over the canonical API alongside the legacy ones rather than
replacing them wholesale — a key that is not in the manifest cannot be
expressed, because SettingKey is generated from it, and the default for an
unset value comes from the generated table rather than a literal at the
call site. That last part is what stops the flip-off bug the Apple client
carries a hand-written guard for.
A test asserts every preset composes values the contract actually accepts,
so a preset naming a resolution outside the enum or a bitrate outside the
declared bounds fails here rather than 400ing when a user picks it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
QuickandClaude Opus 5
|
24ee99523d |
feat(settings): generate typed bindings for all four languages
One generator rather than one per repo. The point of the contract is that four codebases agree on keys, types, scopes and defaults, and four independently written generators would be four chances to disagree. Go and TypeScript land in this repo; Kotlin and Swift are written into the sibling client checkouts, skipped with a note when they are not present so a server-only developer can still run it. Output is sorted by key so an unrelated manifest edit does not produce spurious diffs. The Kotlin output is the interesting one: it generates the DeviceSettings allowlist Android maintained by hand, plus the BOOLEAN_KEYS/INT_KEYS/ DOUBLE_KEYS classification it kept as a *second* hand-maintained table that had to agree with the first. Both are manifest questions now, so the whole class of "wrote a local key to the server" and "flushed a value the store could not parse" bugs stops being possible by construction. The TypeScript output carries the full definition table — labels, controls, enum members, bounds — so web/src/lib/settingsManifest.ts can be deleted rather than kept in sync: it declared 17 definitions against the contract's 49, with its own two-scope model that does not match the contract's five. make verify-settings-bindings fails when the committed output disagrees with the manifest, wired into CI, so a manifest change cannot merge leaving every client reading stale keys. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
QuickandClaude Opus 5
|
cdbb2997fd |
feat(settings): add the canonical settings API
The routes that make the typed storage reachable. Until now the manifest,
the resolver and the migration all existed with nothing able to call them.
GET /settings/contract serves the public manifest behind an ETag — clients
vendor a pinned copy and generate bindings from it, so the common request
asks "still the same contract?" rather than transferring it. Its
capabilities sibling reports revision and supported scopes for feature
detection instead of version sniffing.
/settings/values/{key} reads, writes and clears an explicit value at one
named scope, which is what a reset affordance needs: "did I set this here"
is a different question from "what applies", and the old endpoint could
only answer a blurred version of both. Scope comes from the query while
profile and device come from session headers, so one profile cannot address
another's settings by naming it.
/settings/values/effective resolves any number of keys in one request, with
the resolution ladder and the source of each answer reported so a client can
offer "reset this device's override" against the exact row holding it.
Asking for no keys returns every remote setting, which is what a settings
screen wants.
Writes are idempotent when a client sends X-Silo-Mutation-Id: a retry after
a dropped response replays the receipt, and reusing an id with different
content is a conflict rather than a silent overwrite of the wrong thing.
Three things the string-only endpoint could not do, each pinned by a test:
an unknown key is refused rather than stored in the extension bag, values
are checked against their declared type and range, and a write to a scope
the definition does not allow is rejected.
Registered before the catch-all /{key} routes, which would otherwise
swallow "contract" and "values" as setting names. The legacy endpoints stay
live for now; deleting them is the next commit, once their consumers move.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
QuickandClaude Opus 5
|
cfbd69b240 |
feat(settings): run the one-time migration on the Postgres backend
The mirror of userdb V15, registered with goose as a Go migration rather
than SQL: the conversion validates every value against its own definition
and re-encodes it as typed JSON, and one legacy quality string becomes two
rows — neither is expressible in SQL without duplicating the manifest. The
rules stay in internal/settingsmigrate, so the two backends cannot disagree.
RunTx, so the whole backfill lands in goose's transaction. The down
migration empties the canonical tables; the legacy ones are never touched
by the up, which is what keeps the cutover reversible until the follow-up
migration drops the superseded columns.
preferred_metadata_language is read here and only here — the column exists
in this schema and not in SQLite's, so this is the sole source for
catalog.metadata_language.
Verified against a real Postgres: the full goose chain runs, 1080p-high
decomposes to ("1080p", 10000), values land as typed jsonb rather than
strings (jsonb_typeof reports number), rejects carry a queryable jsonb
identity, and the composite profile foreign key refuses a row naming a
profile that does not exist.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
QuickandClaude Opus 5
|
1f40e25438 |
feat(settings): run the one-time migration on the SQLite backend
Wires the planner to real storage as userdb migration V15. V14 created the tables; this fills them. It runs inside runMigrations' existing transaction, so a database either comes out fully migrated or untouched — a partial migration is the one state neither the operator's backup nor a rollback covers. Pinned by a test that rolls back and asserts nothing was left behind. Two things the wiring had to get right that the planner could not see: Reject identities are JSON. Postgres declares that column jsonb NOT NULL and SQLite guards it with a json_valid CHECK, so the free-form "profile=p1 device=d1" the planner emitted would have failed to insert — on exactly the rows the table exists to record. They are structured documents now, which is also queryable. Subtitle and audio preferences are two tables keyed the same way, so they merge into one per-series record before planning. Converting them independently would have produced two rows racing for the same identity. Every legacy read tolerates a missing table, since this runs against databases created at any schema version, and preferred_metadata_language is deliberately absent: that column exists only in the Postgres schema. Tested end to end against a real database rather than only through the planner — the rows land, satisfy the scope CHECK and the partial unique indexes, and hold valid JSON. Also covers the empty-install case and asserts a second run fails rather than silently doubling every value. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
QuickandClaude Opus 5
|
7a29fcd0e6 |
feat(settings): add the one-time migration planner
The conversion rules from legacy settings storage to canonical values, as ordinary Go rather than twice in two SQL dialects. Both backends read their own rows, hand them to Plan, and write what comes back — so the decisions are testable without a database and SQLite and Postgres cannot drift apart in what they decide. The rules that needed care, each pinned by a test: Column defaults are not choices. quality_preference is NOT NULL DEFAULT '1080p' while the contract defaults to auto, so migrating the column unconditionally would pin every profile in the install to 1080p having never chosen it — and that stored value would then outrank the contract default forever. Same for language 'en', subtitle_mode 'auto', and show_forced_subtitles true. The empty string is unset, not a value. The legacy string API had no way to send null, so both Android and web spell "clear my choice" as "". Storing that would make a cleared setting outrank the default. Legacy quality decomposes rather than rejects. Every compound value maps to a resolution and a bitrate from the ladder in useTranscodeQuality.ts, so nothing lands in the rejects table. Account rows fan out to every profile, which is the account-to-profile move the contract makes for appearance and search scope: a household that shared one theme each end up owning theirs. Legacy strings become typed JSON — "true" to true, "30" to 30 — or every generated binding would fail to decode what the migration wrote. Nullability differs per backend, so profile columns arrive as pointers and the caller resolves "chose the default" versus "never written" when it reads. jellycompat's DisplayPreferences blobs ride the same table under synthetic keys and are left alone; they are that subsystem's storage. Everything that cannot convert is recorded with a reason rather than dropped, and a final test asserts every planned row would be accepted by the mutation endpoint's own validation. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
QuickandClaude Opus 5
|
9fa57653b8 |
feat(settings): add the canonical resolution engine
One answer to "what is this setting, for this profile, on this device, for this content". Before this, each caller carried its own ladder: catalog/detail.go resolved subtitles across four levels by hand and audio across three, handlers/settings.go had a two-level device/user resolution with a lazy write-back inside a GET, and jellycompat read profile columns directly. Those disagreed about precedence, which is the drift the contract exists to remove. Resolution is one batched read regardless of how many keys, libraries, or series are in play — ranking happens in Go against each definition's declared resolution_order. Five sequential index lookups per key per item is the implementation the design rejects, and a season view is exactly where it would have shown up. An absent identity drops its scope rather than erroring, so one code path serves an identified client, an anonymous jellycompat seed, and a batch spanning many series. Rows for a foreign profile, device, library or series are ignored even though the batched read returns them. Constraints narrow without destroying: a capped 4K preference resolves to the cap, reports itself constrained, and keeps the authored value so it takes effect the day the cap lifts. Two cases needed care — null on a nullable numeric means unbounded, so a ceiling must cap it rather than rank it equal and let the value that most needs capping slip past; and an allowlist falls back to a permitted member rather than the definition's default, which may itself be outside the list. Adds ValueSchema.CompareValues to the contract package, since ordering values is what makes a ceiling or floor mean anything and value semantics belong with the schema that declares them. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
QuickandClaude Opus 5
|
f44240e5e5 |
feat(settings): split quality into two axes and register the orphan keys
Two manifest changes the cutover needs.
**Quality becomes resolution + bitrate.** The legacy ladder values
(1080p-high, 720p-medium, 1080p-8, 420p, 328p) were never a third dimension
— they are a bitrate spelled into the resolution string. The web player
already decomposes them: useTranscodeQuality.ts defines 1080p-high as
{resolution: 1080p, bitrate: 10000} and sends the two separately, so the
compound form never reached the wire. Downloads went further and kept only
a bitrate ladder.
So playback.preferred_quality keeps the six clean resolutions and
playback.max_bitrate_kbps becomes the second axis, nullable because
"uncapped" is a real answer and a numeric sentinel would need widening
every time hardware improves. Clients compose their own presets from the
pair, which means retuning what "High" means is a client release rather
than a contract break. Migration decomposes each legacy value losslessly,
so none of them lands in the rejects table.
**The five extension-bag keys are now definitions.** card_overlays,
next_up_mode, sidebar_pins, disabled_library_ids and library_order reached
the server only through the unknown-key path, stored as unvalidated
strings. Two of them the server reads back — next_up_mode decides home
section assembly and card_overlays falls back to an admin default — so
they cannot be demoted to client-local. Registering them is what lets the
extension bag close.
Adds three schemas for their shapes and a test that exercises every
schema_ref against a real value: each of these is nullable with a null
default, so the existing default-validation test returns at the null branch
without ever compiling the reference.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
QuickandClaude Opus 5
|
1291316c34 |
fix(settings): size the library page state bound to what the web client writes
ui.library_page_state's `search` was bounded at 256 characters. The web client serializes an advanced library view as URLSearchParams, encoding each filter rule as three groups[i][rules][j][field|op|value] keys — measured at 216 characters for one rule, 518 for three, 820 for five. The current endpoint validates this key by checking only that it parses, so those oversized values are already stored in production. Typing them at the declared bound would have failed the migration for anyone who had saved a view with more than one filter rule, and rejected the equivalent write afterwards. Raised to 4096, which clears ten rules with room to spare while staying a real bound. The test pins it against the key shapes libraryPageSearchParams.ts actually emits rather than a round number. Reported by the Codex review bot; the lengths above were measured by calling serializeLibraryPageSearchParams, not estimated. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
QuickandClaude Opus 5
|
8e0d310a03 |
fix(settings): reject raw invalid UTF-8, not just escaped surrogates
The previous commit hoisted the lone-surrogate check to cover every value type, but that only closes the escaped path. A raw 0xff byte inside a quoted string — what an HTTP body carries when a client encodes text in the wrong charset — is not an escape, so the surrogate scan never sees it, while encoding/json still substitutes U+FFFD and reports success. NormalizeValue then stores the original bytes, which SQLite's json_valid accepts and Postgres jsonb refuses: the same backend divergence, reached the other way. Found by the Codex review bot on the previous commit's own diff. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
QuickandClaude Opus 5
|
fa03333888 |
test(auth): stop TestJWT_TamperedToken passing a valid signature
The test overwrote the last character of the signature with "X". An HMAC-SHA256 signature is 32 bytes, so its base64url encoding is 43 characters and the final one carries only four significant bits — U, V, W and X all decode to the same trailing byte. Roughly one token in sixteen was therefore left byte-identical and validly signed, and the test failed because ValidateToken correctly accepted it. Measured at 3098/50000 (6.2%) over distinct signatures; it just failed the Go job on this branch for reasons unrelated to the branch. Flipping a character in the middle of the signature is 0/50000. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
QuickandClaude Opus 5
|
dcedbbdd88 |
fix(settings): close the review findings in the validator and the theme cache
Four defects the existing tests did not reach. The web theme resolver compared the server's value against the appearance cache and fell back when they agreed, but the mirroring effect writes the server's value into that same cache — so the comparison held on the first render and stopped holding on the second, reverting an explicitly chosen theme to the default. The server's value is this account's own stored choice, so it now simply wins. The regression test re-renders rather than asserting on the first paint, which is why the original one passed. golangci-lint's exclusions.paths is a path regex, not a directory list, so a bare `web` also excluded internal/jellycompat/web_component.go, internal/webhooksync/, internal/notifications/webhook*.go and eleven other non-test files that were being linted before. Anchored. json.Number is a string kind, so `"1.5"` unmarshalled into it happily and Float64 parsed the quoted digits: a numeric setting validated as a JSON string and NormalizeValue stored the quoted form into jsonb. Rejected. The lone-surrogate check ran only on the object branch, so a lone surrogate in ui.custom_css decoded to U+FFFD on SQLite and was refused outright by Postgres jsonb — the two backends disagreeing about whether the same value could be stored. Hoisted to cover every type. The strict language-tag validation this branch added is correct, but it rejects what the shipped Android client sends; the companion fix is silo-android 4aeb78b4. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Quick
|
1f9b300add |
test(settings): pin the settings-value schema constraints in both backends
Completes the storage track. The conformance suite exercises the store API, which validates identities in Go before any SQL runs — so nothing noticed whether the CHECK constraints and partial unique indexes actually existed. The one-time migration writes these rows in bulk without going through the per-request path, so the schema is the only thing guarding it. Adds constraint tests to both backends covering every scope's column requirements, rejection of an unknown scope, a profile that does not exist, non-JSON values, and each of the five partial unique indexes. Also clears the lint the storage commit did not get to: sql.ErrNoRows and pgx.ErrNoRows compared with == rather than errors.Is (which fails on a wrapped error), an unchecked rows.Close, and repeated fixture literals in the shared suite now named so a backend that confuses two scope columns fails on the assertion rather than on a typo. |
||
|
QuickandClaude Opus 5
|
05af63ea2b |
feat(settings): add canonical typed storage for the settings contract
The cross-platform settings contract needs one typed store behind it before a resolver, routes or a migration can exist. This adds that storage to both user-store backends and holds them to identical behavior. PostgreSQL gets user_setting_values with the scope CHECK constraints, the five partial unique indexes that enforce one explicit value per identity, and the covering indexes the one-query read path needs, plus user_setting_mutations for mutation_id idempotency and the inert user_setting_migration_rejects audit table. The per-user SQLite store gets the same shape minus user_id, since that database is already user-scoped. The UserStore interface grows the typed operations: read one explicit value at one scope, collect every candidate row for a resolution request in a single query, upsert with a revision increment, unset, and the idempotency receipt operations. The resolution read deliberately returns unranked candidates so the resolver can rank in Go — one query per request, never one per scope, which the pgx query-count test pins. Delete behavior is application-enforced. Neither backend can inherit it from constraints: the SQLite store declares no foreign keys, and library, series and device columns are not FK targets in Postgres either. Profile deletion cascades to profile-anchored values while account scope survives, forgetting a device clears its profile_device values alongside the legacy overrides, and the library/series purges remove only what is scoped to that entity. The shared conformance suite covers all of it, including the set-versus-unset distinction for false, 0, "" and null, so a divergence between the two backends fails a test rather than reaching a client. Part of #376 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Quick
|
90224c583f |
fix(settings): clear the remaining review findings
Small, unrelated except that each was raised on #479. compileObjectSchemas parsed every non-directory file under schemas/ as a JSON Schema, so a stray editor backup or .DS_Store would panic the server at startup through MustLoad. schema_ref can only name a .json file; anything else is skipped. cmd/silo used MustLoad while the ETag check beside it and every other startup failure use log.Fatalf. It now fails the same way, so a bad contract prints an error instead of a stack trace. TestRegistryDefaultsMatchTheContract called scalarDefault before handling null, and scalarDefault rejects null as non-scalar — so the subtest skipped and the comparison after it was unreachable. A nullable contract default could disagree with a non-empty registry default and nothing failed. Confirmed by injecting that drift, which now reports it. The three appearance providers each adapted the auth context to AppearanceAuth with identical code, putting the shape of auth back in three places that widening cache ownership would have to find. useAppearanceCacheOwner now does it once. useTheme.test.ts cleared storage.KEYS between cases, but appearanceCache writes namespaced keys and an owner pointer that are not in that list, so both survived and the suite was order-dependent. It clears the store, as storage.test.ts already did. The abs_smart_collection_store comment is reworded rather than given back its SQL quotes: gofmt folds a pair of apostrophes in a doc comment into a typographic quote, which is how it became one in the first place. Reported by CodeRabbit review on #479. |
||
|
Quick
|
5df181f733 |
docs(v1): record the settings removal as a pre-lock exception
The design removes the legacy /api/v1/settings routes and the profile DTO preference fields, while AGENTS.md states /api/v1 is additive-only and removals go through Deprecation/Sunset. Read together those contradict. They do not actually conflict: v1-scope.md scopes the additive-only rule to "when the scope locks", and the scope is still open, so a removal taken now is in scope and there is no amendment process to invoke yet. But that reasoning lived only in the settings design, where nobody checking the API policy would find it. v1-scope.md now carries a pre-lock removals table naming what goes and why waiting is worse, and states the deadline the argument depends on: a removal listed there must ship before lock or fall back to Deprecation/Sunset. AGENTS.md points at the table and says to treat an unlisted removal as a mistake. Reported by CodeRabbit review on #479. |
||
|
Quick
|
6535203791 |
ci: gate Go lint on the lines a branch changes
AGENTS.md told contributors CI ran the same checks as `make lint`, and the Go job ran only gofmt and vet. A change failing the documented Go lint gate passed all three jobs. Running the linter as-is is not an option: the tree has ~296 findings today, which is why this half of `make lint` was never enforced. Blocking every PR on a cleanup nobody has scheduled gets the gate deleted again, so CI runs with --new-from-merge-base and only the lines a branch touches have to be clean. The count can then only fall. golangci-lint is built from source at a pinned version rather than downloaded. A released binary refuses to run against a Go newer than the one it was built with, and go.mod here tracks Go closely enough that the current release already fails that way on 1.26.4. .golangci.yml declared version 2 while still using v1's issues.exclude-rules key. Current golangci-lint ignores it, so the "allow repeated strings and unchecked cleanup errors in tests" exclusions silently did not apply — 16 findings in test files that the config says to skip. Moved to linters.exclusions, which `golangci-lint config verify` accepts. The four lines this surfaced in scantrigger are fixed rather than excluded: its repeated status codes and messages are now named constants, so one condition cannot end up worded two ways. Also drops the workflow token to contents:read and stops persisting credentials in the three checkouts, neither of which any job needs. Reported by CodeRabbit and Codex review on #479. |
||
|
Quick
|
f7b72a4191 |
fix(settings): reject JSON the decoder would otherwise rewrite
Two cases where encoding/json accepts input by quietly changing it, which is
the one thing a contract promising byte-identical agreement between peers
cannot tolerate.
Duplicate object properties. jsonschema.UnmarshalJSON keeps the last
occurrence, so {"fontSize":"small","fontSize":"large"} validated and stored
"large". Which one wins is a property of the parser, not of the contract: a
client generated against a different JSON library can disagree about what it
just sent, and the canonical form cannot represent the duplicate at all.
Lone surrogates. An unpaired \ud800 became U+FFFD and canonicalization
reported success, so the server would issue canonical bytes and an ETag for
an artifact a conforming implementation must refuse — RFC 8785 requires
terminating here. Substitution also means the value read back is not the
value written.
Both checks run before the decode that would hide them, on the shared
decodeJSON path that the manifest, its public projection and every value
schema go through, and again on the object branch of ValidateValue, which
uses a different decoder.
Reported by Codex review on #479.
|
||
|
Quick
|
d7452a8207 |
fix(jellycompat): scan the parent directory when a sidecar changes
Autoscan matched scantrigger rejections by comparing RequestError.Message against literal strings. One of those messages became "Unsupported media file extension for library type" and the copy in handlers_autoscan.go did not, so the comparison silently stopped matching. The effect is user-visible: a Jellyfin client posting a change for Movie.nfo or poster.jpg gets a 400 and the batch is abandoned, when the sidecar should have resolved to a scan of the directory containing it. Three tests covered exactly this and had been excluded rather than read. RequestError now carries a Reason the caller can switch on. Message stays prose for the client reading the response — it is meant to be reworded, and nothing should break when it is. Also makes two tests honest about asynchronous work. The Jellyfin Web teardown deleted its install root while the operation goroutine was still writing to it, where a late write recreates a path RemoveAll already walked past; it now waits for the operation's terminal state, which required exporting CurrentWebOperation. And the direct-play If-Range test pinned size and mtime so ctime was the only remaining validator, then read it back inside a single coarse-clock tick — it failed about 85% of the time on main for a reason unrelated to what it tests, and now rewrites until the stamp moves. With those fixed, GOTEST_KNOWN_FAILURES is empty and gone: make test-go runs the whole Go suite. The one test that cannot pass yet — TestHandleReplanPlaybackV3SeekFailureRecoveryNeverChangesMediaVersion, which has failed since the commit that introduced it and describes unimplemented v3 planner behavior — carries a t.Skip explaining that where the test is, rather than a regex in the Makefile. Reported by CodeRabbit review on #479. |
||
|
Quick
|
181bb93333 |
fix(settings): accept the partial subtitle appearance objects already stored
The schema required all nine properties, but the current API accepts and
round-trips sparse objects — settings_device_test.go stores
{"fontSize":"xxlarge"} and reads it back — and the web client has always
merged whatever it gets over DEFAULT_SUBTITLE_APPEARANCE. Requiring the full
object would have made the cutover migration quarantine preferences users
really set, or block on them.
Every property is now optional and a stored value is documented as a sparse
override merged over the definition's complete default. An empty object is
still rejected: an override that overrides nothing is the same state as no
override, which the contract represents as unset.
Cross-scope resolution is deliberately unchanged. A device override still
replaces the profile's object rather than merging into it, because a device
override means "draw subtitles this way on this screen", not "amend the
profile" — and that is what the server does today.
Reported by Codex review on #479.
|
||
|
Quick
|
61448d6161 |
fix(settings): keep widened numeric bounds resolvable at older revisions
A bound was one scalar plus the revision that introduced it, which discards the value it replaced. Widening a maximum from 240 to 480 at revision 3 left a revision-3 client with no correct answer against a revision-1 server: honoring 480 offers values that server rejects, and filtering the tagged bound out leaves the setting unbounded. Since clients are specified to filter their pinned contract against the server's advertised revision, the bound has to carry what it used to be. Bounds now hold their full history, oldest first, and AtRevision hands back the limit a given peer actually enforces. A bound nobody has widened still serializes as a bare number, so the manifest reads the same and untouched entries do not churn the ETag. Validation gains the rules the representation makes checkable: a maximum may only grow and a minimum may only shrink, history is strictly ordered, later entries must say when they arrived, and the first entry cannot predate the definition. That last rule is the lower bound allowed_scopes already enforced; the same gap is closed for enum members, which could previously claim to predate the definition containing them. Reported by Codex review on #479. |
||
|
QuickandClaude Opus 5
|
a1bf79c5f7 |
fix(settings): enforce the language-tag and step constraints the manifest declares
A sweep of all 43 manifest definitions against the running server (160 checks: declared default, both boundaries, and deliberate violations for each remote key) found two places where the live registry accepts what the contract forbids. Both are fixed by calling the contract's own validators rather than adding a second implementation. playback.audio_language was checked as "32 characters or fewer", so the server stored "!!!" for a field the manifest declares as language_tag — a value track matching would then silently never match. It now requires a well-formed tag via settingscontract.NormalizeLanguageTag. The empty string is still accepted: the string-only endpoint has no way to send null, and both Android and web send "" to clear the choice, so rejecting it would break clearing the preference. player.playback_speed declared step 0.05 and nothing enforced it, so 0.26 was stored — a value no client's stepper can represent and that every client would silently snap on the next write. settingscontract.StepAligned is now exported and used by both the contract validator and the registry, so there is one definition of "on step" rather than two that can drift. This gives the contract its first production consumer beyond the startup load, which is the direction Phase 2 continues in. Also fixes a genuinely flaky test that the new CI gate would have hit intermittently: TestRemoveJellyfinCompatWebDisablesWebSetting used t.TempDir as the install root, but the endpoint returns 202 and its goroutine keeps writing there after the test body returns, so cleanup tripped "directory not empty" roughly one run in four. Confirmed pre-existing and unrelated to settings; the suite now passes six consecutive full-package runs. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
QuickandClaude Opus 5
|
044f159b79 |
fix(build): copy the settings contract into the container build context
Both Dockerfiles copy cmd/, internal/, migrations/ and web/embed.go, but the manifest lives in contracts/settings/v1 — an embedded Go package that sits outside internal/ because clients vendor those files. The image build therefore fails with "no required module provides package .../contracts/settings/v1". Caught deploying to the dev box. Nothing had built an image since the manifest landed: the Docker workflow only runs on pushes to main and workflow_dispatch, and CI's go build runs against a full checkout, so neither gate covers the container context. This would have broken the published image on merge. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
QuickandClaude Opus 5
|
45ca765450 |
test(web): stop the diagnostics download test depending on the Node version
new Response(blob) reads the body through blob.stream(), which jsdom's Blob does not implement on Node 22 — the version the Dockerfile builds with. The test passed locally on Node 24 and threw "object.stream is not a function" in CI. Nothing in it asserts on the body, only that the object URL and filename reach the anchor, so a string body is equivalent and works on both. Surfaced by the CI workflow added in this branch, which is the first thing in this repo to run the frontend suite anywhere but a developer's machine. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
QuickandClaude Opus 5
|
e1e16020f6 |
ci: give the new workflow the deps it actually needs
The first run exposed two gaps in the workflow itself. go build ./... fails without libvips headers, because h2non/bimg binds libvips through cgo and pkg-config; the Dockerfile installs the same package. And pnpm/action-setup resolves its version from package.json, but there is no package.json at the repo root — the packageManager field lives in web/package.json, and a job's defaults.run.working-directory does not apply to an action's inputs. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
QuickandClaude Opus 5
|
790250d87f |
fix(settings): align the sleep timer default and range with the shipped client
Android is the only client that implements this setting. It clamps to 0..240 and defaults to 30. The manifest said 0..480 with a default of 0, so a manifest-driven UI would have offered durations no client can store, and every user who never opened the picker would have had the preset silently turned off at cutover. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
QuickandClaude Opus 5
|
d8faf83c6e |
fix(settings): make the settings contract enforceable and fix the appearance cache
The contract manifest landed as a document nothing checked. This makes it a mechanism, and fixes the one defect in the change set that hurt users on merge rather than at cutover. Web appearance cache. useTheme cleared the cache for any account whose stamp did not match and never repopulated it — the only writers were the four user-action setters — so every upgrading user lost their warm start on every load, not once, and x-large-text and high-contrast users lost theirs too. The owner-stamp protocol is replaced with per-account key namespacing (`silo-theme:7`): a foreign value is absent rather than present-and-distrusted, so nothing has to be deleted, the first account keeps its warm start, and there is no shared stamp for a second tab, a stale debounce timer, or an out-of-order effect to race on. Widening ownership to profile scope, which this manifest requires, is now a change to appearanceCacheOwner alone. Adds the API-to-cache mirror useTheme was missing, cancels pending debounced writes across an account change, and re-seeds provider state during render so no frame paints the previous account's look. Canonicalization. writeCanonical used json.Marshal, which HTML-escapes < > and &, and canonicalNumber used Go's 'g' format — both diverge from RFC 8785, so the first label containing an ampersand or bound below 1e-4 would have forked the server's ETag from every conforming client. Output is now byte-identical to ECMAScript String() across the edge cases, verified against node. The ETag also covers the value schemas, which decide what the server accepts and previously could change while the tag stood still. All four derived representations are memoized; a conditional GET no longer costs a full parse and re-serialize. Validation. strictUnmarshal's decoder.More() answered false for a stray ] or }, so `true]` validated as a boolean. Enum matching compared fmt.Sprintf tokens, so the string "3" satisfied an integer member. Declared steps were never enforced. The language pattern rejected tags both mobile platforms emit unprompted (en_US, ca-ES-valencia, ar-EG-u-nu-latn) and never normalized case, so en-US and en-us were two rows for one preference; NormalizeValue now canonicalizes on the shared path. Manifest. show_forced_subtitles defaulted false where the server column is NOT NULL DEFAULT true, which would have turned forced subtitles off for every profile that never touched it. preferred_quality declared 13 members where the planner speaks 6 and collapses the rest to auto. metadata_language's allowlist was bound to the very column it migrates from. subtitle-appearance pinned fontFamily to three families while Apple stores any installed system font. Registers five user-facing settings the clients already ship, and corrects three notes that described Android behaviour that was not true. Enforcement. The package had no non-test callers, so MustLoad never ran; it now loads and logs at startup. The inventory test compared the manifest against a hand-copied map and could not see the drift it named; it now iterates settingsRegistry and checks defaults too — both verified to fail on injected drift. Adds .github/workflows/ci.yml, the repo's first CI that runs go test, go vet, gofmt, and the frontend suite. Known pre-existing failures are named individually in the Makefile so everything else stays gated and the list can only shrink. Part of #135 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
QuickandClaude Opus 5
|
4f59356cf0 |
fix(web): tag local appearance caches with their owning account
The theme, text scale, text weight, high contrast, custom theme variable and custom CSS caches in localStorage were untagged, so on a shared browser a second account inherited the first account's appearance: with no server value of its own, every fallback resolved to whatever the previous account had stored, and the leftover `silo-theme` key also suppressed the admin-configured default theme for the new account. DateTimeFormatProvider already solved this by stamping its cache with the authenticated user id and refusing another account's values. Extract that mechanism into `createOwnedCache` in utils/storage.ts (where key namespacing lives) and put all three groups behind it, so appearance and custom theme get the same protection instead of a third copy of the rule. - Each group carries its own owner stamp. A shared stamp would be unsafe: the groups are written by hooks nested inside each other, and effects run inner-first, so whichever hook stamped first would vouch for the other's still-stale values. - A null owner (auth bootstrapping, or signed out) still trusts the cache, which keeps the warm start and the login screen's last look. - An unstamped cache is not trusted once an account is known, so existing users take a one-time appearance reset on first load rather than a chance of seeing someone else's settings. - When a foreign cache is detected the values are dropped and the empty cache is handed to the new account, so a later single save cannot re-trust the rest of the previous account's state. Owner is the user id because /settings is user-scoped server side; it lives in one helper (`appearanceCacheOwner`) so it can be widened if appearance moves to profile scope. `shouldLoadApiTheme` is gone: it had become a synonym for `appearanceCacheOwner(...) !== null` with no callers left. Part of #376 AI-use disclosure: implemented with Claude Code. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
QuickandClaude Opus 5
|
f4f7cbd8d6 |
feat(settings): give ui.theme a device override
Theme joins text scale, text weight, and high contrast as a profile default with an optional per-device override, resolving profile_device -> profile -> default. The right theme is partly a function of the screen and the room — a light theme on a phone in daylight, a dark one on a TV at night — which is the same reasoning the other three appearance keys already used. All four appearance settings now cascade consistently, which also means one rule to explain in the UI rather than "these three follow the device, that one does not". ui.custom_theme_vars and ui.custom_css stay profile-wide. They are authored styling rather than a contextual preference, so a profile's custom tokens still apply on top of whichever theme a device resolves to. Recorded in the definition notes because it is a visible consequence: vars tuned against a dark theme will sit on top of a light one if a device overrides the theme. Widening those to profile_device later is an additive revision bump if it turns out to matter. Part of #376. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
QuickandClaude Opus 5
|
5933ac0bf2 |
feat(settings): add the canonical settings contract manifest
First implementation step for the cross-platform settings contract (#376). Adds the artifact everything else depends on: the manifest, its JSON Schema, the object value schemas, and a Go loader that validates the whole thing at load time. No routes, no storage, no behavior change — nothing reads this yet. contracts/settings/v1/ holds the artifact at a stable path because clients vendor it and generate bindings from it. The embed directive has to sit beside it (go:embed cannot reach outside its own directory), so that directory is a tiny Go package containing nothing else; loading and validation live in internal/settingscontract. 38 definitions: 35 remote, 3 contract-known client_local. That covers every key the legacy registry accepts, every unregistered key the extension bag was silently accepting from the web client, every unregistered device key Android writes, and the profile preference columns that become settings. Registering the previously-unregistered keys is where the drift shows up, and the manifest records each case in a notes field: - ui_theme, ui_text_scale, ui_text_weight, ui_high_contrast, ui_custom_theme_vars, and ui_custom_css reached the server only because keyUsesUserScope returns true for any unregistered key. They are now typed, renamed to the dotted convention every other key uses, and moved to profile scope per the design. - player.match_frame_rate and player.sleep_timer_default_minutes are written by Android against a server that does not register them, so every write and reset is currently rejected. Registered. - player.next_up_prompt_seconds is Android's alias for playback.next_up_prompt_seconds and does not become a definition; the test matrix pins it as a migration alias. - player.playback_speed is capped at 3.0, matching the server rather than Android's 4.0. - subtitle_appearance becomes playback.subtitle_appearance. Every other canonical key carries a domain prefix, and preserving accidental key names is an explicit non-goal of the design. Validation is deliberately stricter than the schema can express. Beyond shape, it enforces that a resolution order ends in "default", that it only resolves scopes the definition allows, and — the one most likely to bite — that every writable scope is actually read, so a setting cannot accept writes at a scope it will never honor. Defaults are validated against their own value schema, so a default that violates its own range or enum fails at load. Revision tags are checked to never run ahead of the manifest revision, which is what makes revision-aware client filtering trustworthy. Ceiling and floor policy constraints are rejected on unordered types, where capping would silently do nothing; playback.preferred_quality's enum is therefore ordered ascending. ValidateValue is the single validation path, so the mutation endpoint, the migration, and the manifest's own default checks cannot diverge later. Numbers decode through json.Number so an integer setting rejects 30.5 rather than truncating, and object values validate against their referenced JSON Schema instead of accepting arbitrary JSON the way validateJSONSetting does today. Canonicalization implements RFC 8785 over the value domain the contract uses: sorted keys, no insignificant whitespace, ECMAScript number formatting. The digest is the ETag, and PublicBytes strips maintainer notes so the served manifest never carries internal commentary. Promotes santhosh-tekuri/jsonschema/v6 from indirect to direct. Verification: 124 tests pass across 16 cases; golangci-lint clean; make verify-local-paths passes. Two failures in internal/api/handlers (TestRemoveJellyfinCompatWebDisablesWebSetting, the playback v3 seek recovery test) reproduce unchanged on main and are unrelated. Part of #376. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
QuickandClaude Opus 5
|
35b84cca0f |
docs(settings): define the cross-platform settings contract
Turns the audit in #376 into a decision-complete design for how user settings work across the server, bundled web client, Apple clients, and Android clients. Today there are three partial contracts - the server registry, the web client's own manifest, and independently owned key constants in each native client - and they have measurably drifted. The root enabler is that keyUsesUserScope returns true for any unregistered key, so a client can invent a production setting unilaterally and the server stores it as an unvalidated string. The design decides: Ownership. Every production user-facing setting needs a server-owned manifest entry, even when the value is stored only on one client. The single exception is private local.<client>.* diagnostics, bounded by five conditions. Types and scopes. Native JSON values instead of strings. Five remote scopes plus client_local, and each definition declares its own resolution order rather than inheriting a global precedence. Preferences versus restrictions. internal/policy already resolves max_playback_quality and metadata-language limits over the same controls this contract resolves preferences for. Definitions declare constrained_by, the effective response reports the permitted value alongside the user's stored one, and a mutation exceeding a restriction is stored rather than rejected - a capped 4K preference should take effect the day the cap lifts, not be destroyed by it. Compatibility. Widening a scope, adding an enum member, or widening a range is additive and revision-tagged; narrowing anything needs a new key. introduced_in is a manifest revision attached to individual enum members and scopes, not just whole definitions, so a newer client never offers a choice an older server will reject. Rollout. One coordinated breaking release, with no compatibility shim, projection, or client fallback. After the cutover no future setting requires coordination. No settings version check goes in the authenticated middleware and nothing returns 426: deleting the old routes already produces the break, and a gate would be more code in four repos for the same outcome while permanently coupling every endpoint to one subsystem's versioning. Scope placement. Appearance and date/time move from account to profile scope. Account scope was an artifact of pre-profile storage; leaving it there means a household shares one theme and text size, and any non-child profile can restyle everyone else. Read path. Batched context resolution, index requirements, a session-snapshot rule, and a no-regression benchmark gating storage consolidation - profile_series resolution is per-item, so a season view would otherwise issue one request per episode. Verified against the current server, Apple, and Android implementations. Two findings shape it: the unknown-key extension bag is real, and v1 scope reads NOT LOCKED, so removing the legacy surface needs no amendment if it lands before lock. Related to #376. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
|
99d205676f |
fix(metadata): prevent stale cross-provider IDs (#480)
* fix(metadata): prevent stale cross-provider IDs * fix(metadata): address stale ID review findings * fix(migrations): build the stale-ID primary key concurrently ALTER TABLE ... ADD PRIMARY KEY builds the index under ACCESS EXCLUSIVE, blocking reads and writes on stale_media_ids for the whole build. Create the wider unique index with CREATE UNIQUE INDEX CONCURRENTLY and attach it with ADD CONSTRAINT ... PRIMARY KEY USING INDEX instead; all three key columns are already NOT NULL, so the attach is metadata-only. Same treatment on the rollback path, plus the repo's INVALID-remnant cleanup so a failed concurrent build is not silently accepted by IF NOT EXISTS. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Quick <31828688+Quick104@users.noreply.github.com> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
|
02203d9e40 |
fix(playback): trust the server's media runtime end to end (#482)
* fix(scanner): reject durations that imply an impossible bitrate The duration-plausibility rule only rejected videos of 10 seconds or less, so a feature film that probed as 61 seconds passed untouched and persisted. Clients then had nothing trustworthy to anchor on: Android's grow-only duration ratchet has no floor to hold when the catalog value is wrong, so the playback engine's growing-HLS-window duration won and a 90-minute movie displayed as ~1 minute. Size and duration together pin an implied bitrate, which separates the two cases the absolute floor conflates. A genuine short clip has an ordinary bitrate; a 100 GB file claiming 61 seconds implies ~13 Gbps. The ceiling sits far above any real medium, so legitimate content cannot trip it — and unlike the absolute floor, it does not false-positive on a genuine high-bitrate short. Also bump the repair-rule revision marker so rows judged by the previous, weaker rule are re-checked once under this one. Without that bump an improved rule never reaches the rows it was written for. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(playback): publish source runtime in v3 plans and stop faking the copy seek window Two defects with one root: a v3 plan described where playback sits without ever stating how long the media is. Add source.duration_seconds. It is the file's full runtime, never `total - source_start` and never adjusted by timeline_offset_seconds, and it is omitted rather than null when unknown — clients that coerce null to a numeric default would read it as zero, the exact value this field exists to stop them inventing. It is set in SourceDescriptorFromFileV3, the single place every delivery already flows through, so direct play, progressive remux, HLS remux and HLS transcode all carry it. Until now the v3 plan omitted duration entirely, so clients fell back to the playback engine. On an HLS copy remux the server intentionally serves FFmpeg's still-growing playlist, so the engine reports the length produced so far. With no server-supplied runtime to anchor on, a feature film played back as a couple of minutes. The legacy protocol already answered this correctly via fileDurationSeconds; this restores parity. Separately, the copy branch published seek_window_end_seconds as the media runtime. That made the window look *complete*, which clients read as proof that any target inside it is locally seekable, so they native-seek past the produced head of a growing playlist instead of asking for a reanchor. Leave the end open: an incomplete window plus can_seek_anywhere=false routes every seek through the server, which is what legacy did before v3 added the bound. Advertise plan_source_duration_v1 so a client can distinguish "this server does not populate the field" from "this server knows the runtime is genuinely unknown" — without it, both look like an absent field and a client cannot tell whether its own catalog fallback is still required. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(web): pair the exit position with the media runtime, not the element duration The player's exit state converts its position to media time but took the duration from the video element, which is player-local. On a remux or transcode stream the element only covers the window produced so far, so the two values live in different coordinate systems. Resuming a movie 50 minutes in makes that concrete: the exit position is ~3060s of media time while the element reports ~120s. The progress cache then evaluates `position >= duration`, marks the item completed, latches the watched badge, and — because completion clears the resume point — resets position to 0. Exiting a resumed movie destroyed the resume point and claimed it had been watched. The server's runtime is authoritative and already expressed in media time, so prefer it and fall back to the element only when no server value exists. The rule moves into mediaTimeline.ts next to the coordinate conversions it depends on, which is also what makes it testable — VideoPlayer itself has no test harness. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
QuickandGitHub
|
10394b0a05 |
fix(scanner): stop hiding media when a library root is offline (#472)
* fix(scanner): stop hiding media when a library root is offline
A scan that cannot read a library root found no files there, so every
cataloged file under it was marked missing. Catalog reads all filter on
missing_since IS NULL, so marking is equivalent to deletion from a user's
point of view: the title leaves browse, search and next-up, and playback
answers "Source media file is missing" for media that is intact on disk.
The dead-root protection already existed but only guarded the destructive
operations. protectedConfiguredRoots was computed *after* the marking loop
in scanPaths, and applyScopedScan received the protected set but applied it
only to its force-delete branch. So an unreachable root could not lose its
rows, but could still have its entire catalog hidden until the next
successful scan.
On a CephFS deployment whose per-library subvolume mounts flap, this marked
190 present files missing in a single day — 15% of all missing-flagged rows
were files sitting untouched on disk, some flagged more than 20 hours after
their last write.
Hoist the probe above the marking loop and skip files under an unreachable
or suspect-empty root in both the folder and scoped paths. Pass the
unreachable set to the walked-scope call too: a nested child mount can die
under a healthy parent, and its rows are inside the parent's scope.
An offline root tells us nothing about whether its files exist. The only
safe reading is to leave them alone and let the next good scan decide.
Genuine deletions under a reachable root are unaffected and still marked
and swept on the same schedule as before.
Report the count as ScanResult.MissingSkippedProtected and log it, so an
operator can tell "my library shrank" from "my mount dropped".
Known gap: a suspect-empty *nested child* root under a healthy parent is
still marked missing, because the suspect set is not resolved until after
the walk loop. Unreachable roots — the case observed in production — are
covered.
* fix(scanner): protect suspect-empty and partially-walked roots too
Addresses review findings on #472. The original change guarded missing-marking
against probe-unreachable roots, but left three ways for a storage fault to
still hide a healthy library.
Suspect-empty detection was reactive. suspectEmptyRoots asked
ListRootsWithOnlyMissingFiles, which returns a root only once it has NO live
rows left. On the first scan after a mount drops — the moment that matters —
the rows are still live, so the root was not classified suspect and the scan
marked everything missing. The protection then engaged on the next scan, in
time to protect the wreckage. Ask ListRootsWithCatalogedFiles instead: any
cataloged row under an empty-but-reachable root is the lost-mount signature.
Intentional emptying is still reachable through the operator's one-time
cleanup allowance, which is the deliberate path for it.
Nested suspect-empty children were unprotected. Root compaction sends only the
populated parent through the walked-scope branch, which received only
unreachableRoots, so an empty child mountpoint had its rows marked missing on
its parent scanning cleanly. Pass the suspect set as well.
Partial walks were treated as authoritative. walkLogicalTree deliberately
swallows per-entry Lstat/ReadDir failures so one bad file cannot abort a scan
of a million, and collectLogicalFilePaths passed nil for the failure counter —
so the video path had no signal at all. A mount dying partway through
traversal produced a short file list indistinguishable from a large deletion.
Thread the counter through, and exclude a scope whose walk came back
incomplete from missing reconciliation, mirroring what the ebook scanner
already does via ebookRootScan.failed.
Also extract the duplicated mark-missing loop into markMissingExcludingProtected
so the folder and scoped paths cannot drift, and correct two comments that
still described the pre-fix "files are marked missing" behaviour — the exact
text a future reader would have trusted when reintroducing this bug.
TestScanFolderNestedSuspectEmptyChildRootProtection asserted the old
behaviour and is updated accordingly.
* fix(scanner): scope walk-failure protection and stop pruning on partial walks
Addresses the second Codex review round on #472. The previous commit's
incomplete-walk protection was too blunt in one direction and applied too late
in another.
Walk failures were counted, not located, and any non-zero count protected the
whole library root. A dangling symlink is both common and permanent, so that
would have suppressed missing-file reconciliation for its entire root on every
future scan — genuinely deleted titles would stay live indefinitely. That is
the same class of bug as the one this PR fixes, pointing the other way.
recordWalkFailure now records the logical path of each unreadable entry, and
only those paths are protected. Per-entry failures record the child path, so a
dangling symlink protects itself and nothing else, while a directory that
cannot be read protects its subtree.
Snapshot and group pruning ran before the protection. reconcileScannedRoots
and reconcileScannedGroups delete whatever the walk did not see, and both run
ahead of the missing-file guard, so a partial walk still dropped root
snapshots, observed locations and group locations for the unread portion —
corrupting later metadata matching even though the media_files rows survived.
Upserting what was seen is always safe; pruning now waits for a scan that read
the whole tree.
The confirmed-cleanup allowance was consumed to no effect for nested suspect
children. The walked-parent branch protected them unconditionally and runs
before the allowance is consumed, and an already-reconciled scope cannot be
revisited — so arming the allowance burned the confirmation while the child's
rows stayed live forever. Read the allowance without consuming it before the
walk loop, and honour it there. Unreachable roots stay protected either way:
an outage is never a confirmation to erase a catalog.
Two new regression tests, plus signature updates in the ebook pipeline, which
already tracked walk failures and now shares the path-based representation.
* fix(scanner): re-probe nested roots and gate group pruning on walk completeness
Third Codex review round on #472; both findings confirmed.
Group pruning ignored walk completeness in the subtree path. scanPaths passed
the completeness decision to reconcileScannedRoots but left
reconcileScannedGroups on !allowEmptyRootGuard, which is always true for
ScanSubtree — so a subtree scan that hit an unreadable directory still replaced
group snapshots and locations from a partial inventory. Same rule now applies
to both.
Nested roots were not re-probed before their parent was reconciled. Root
compaction folds a child mount into its parent for traversal, so a child that
is healthy at the initial probe but drops before the parent is walked leaves no
scope of its own, and the post-walk re-probe only revisits scopes that walked
empty. The parent walks files, looks healthy, and the child's rows are marked
missing on its success. reprobeNestedRoots re-checks this root's configured
children immediately before reconciling, protecting any that have since become
unreachable — or suspect-empty, unless the operator has confirmed cleanup.
Also guard suspectEmptyRoots against a nil file repository, matching
emptyCleanupArmed: without a catalog there is nothing to protect.
* fix(scanner): keep re-probed outages protected through folder-wide cleanup
Fourth Codex review round on #472; both findings confirmed. The first could
destroy data.
reprobeNestedRoots protected a root it found offline only for the scope being
reconciled, then discarded the result. The folder-wide membership reconcile and
the trash sweep afterwards rebuilt their protected set from the initial probe
alone, so rows under a child that dropped mid-scan — already marked missing and
past the removal grace — were hard-deleted by the very scan that noticed the
outage. Accumulate those roots in reprobedRoots, fold them into
protectedScanRoots, and reuse that set for the membership reconcile and sweep
instead of rebuilding. They now also land in ScanResult.UnreachableRoots so the
folder warning reflects the outage rather than presenting a partial scan as
clean.
Snapshot and group pruning was enabled for scopes that were never walked. The
gate was len(walkFailures) == 0, but an unreachable root gets nil walkRoots, so
it has no walk and therefore no failures — and pruning then deleted its
snapshots, observed locations and group locations even though its media rows
were protected. The same held for a suspect-empty child compacted into a
populated parent. Pruning now additionally requires that the scope was actually
walked and contains no protected path.
The new test pins that the sweep honours the protected set it is given. It does
not reproduce the mid-scan race itself: staging that needs the drop to land
between the probe and the walk, which a test cannot reach without hooks. That
path is covered by inspection, and the test comment says so rather than
implying coverage it does not have.
* fix(scanner): route every protection source through one folder-wide set
Fifth Codex review round on #472. Two P1s, one of them the second data-loss
path in this area — and the direct sibling of the one fixed in
|
||
|
QuickandClaude Opus 5
|
482efdf140 |
chore: add T3 Code project icon
T3 Code resolves a repository icon from t3.json's iconPath, falling back to well-known locations such as assets/icon.png. Neither resolved in this repo, so it showed the default folder icon in the T3 Code sidebar. The icon reuses the canonical Silo mark unchanged over a navy backdrop, so the Silo repos stay distinguishable at the ~14px the sidebar renders. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
QuickandClaude Opus 5
|
9e7fe79590 |
docs: record live TV, IPTV, and .strm as permanent non-goals
Live TV, OTA/DVB tuners, IPTV, EPG/XMLTV guide sync, DVR, and .strm remote-URL shortcuts are permanently out of scope for Silo. The deciding factor is app store distribution: the first-party iOS, tvOS, macOS, and Android clients ship through Apple and Google, and a server that plays arbitrary remote stream URLs puts the entire client suite at risk of rejection or takedown, not just the feature. Secondarily, live TV is a separate product surface whose reliability burden competes with the core playback path. This was an undocumented boundary until now, and contributors spent real effort against it (#419, #420, #474). Write it down in docs/non-goals.md and summarize it in AGENTS.md so both humans and agents see it before proposing or implementing in this area. Refs #474, #419, #295 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
|
355508f6e7 |
fix(requests): retire stalled targets when presence confirms the media (#470)
reconcileRequest completes a presence-confirmed request only when it has no live targets, so a quality-agnostic TMDB hit cannot orphan an in-flight download. That gate assumes the router eventually moves every target to a terminal state; when it does not, the request is pinned open forever even though the media is in the library. Retire targets stuck in queued past a 24h horizon with no status transition when presence confirms the media, and let the existing target aggregate drive request status. Targets actively downloading are never retired. Each retirement logs at WARN, since reaching this path means a router is misbehaving. Part of #469 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Quick
|
a102630365 |
feat(web): allow tailnet and hostname access to dev server
- Extend Vite `allowedHosts` with `.ts.net`, the machine hostname, and a `VITE_ALLOWED_HOSTS` override - Document reaching the dev UI over Tailscale in the web-ui-testing skill |
||
|
Quick
|
51ef906025 |
docs: add agent skills and silo-dev helper script
- Add skills for dev-environment debugging, jellycompat diagnosis, Discord triage, and web UI testing under .claude/skills (symlinked as .agents/skills) - Add scripts/silo-dev plus .silo-dev.env.example for driving local or remote Silo deployments - Ignore .silo-dev.env and reference the shared skill config in AGENTS.md |
||
|
|
ece3578e91 |
docs: rightsize repo context for Claude 5 context engineering (#467)
Applies the practices from Anthropic's "new rules of context engineering for Claude 5 generation models" to this repo's always-on context. AGENTS.md (symlinked as CLAUDE.md) drops content derivable from the filesystem — the module-by-module structure tour, the Makefile target list, and most of the style section, all of which Claude reads directly from internal/, the Makefile, .golangci and web/.prettierrc. What stays is the part that isn't derivable: the Goose migration rules, the v1 additive-only API contract, multi-repo boundaries, and the workspace gotchas. Also fixes a dead pointer to .claude/skills/deployment-debugging, which does not exist; the runbook is the dev-environment-debugging skill. The external-contributor AI disclosure block moves to docs/ai-contributions.md, reached by a one-line pointer, so it costs nothing in the common case where no external PR is being prepared. issue-to-pr sheds the generic agent hygiene now covered by the harness system prompt and keeps the Silo-specific gates. Its commit trailer no longer pins a stale model name. scripts/jellycompat-diff.sh replaces the hand-typed curl/python one-liners the jellycompat-diagnosis skill used to carry as prose. It unions item keys across all returned items rather than reading item[0], which was hiding fields present on only some items. Verified: make verify-local-paths, bash -n, shellcheck, and a mock two-server run of the diff script. Co-authored-by: Claude Opus 5 <noreply@anthropic.com> |
||
|
|
ee31a1f0e2 |
feat(playback): formalize resumable direct streams and stall observability (#464)
* feat(playback): formalize resumable direct streams and stall observability Implements #443: strong stat-based ETag + If-Range on original-file direct play (via http.ServeContent), stream-end outcome classification in RollingDeadlineWriter (stalled_reap vs client_gone vs completed) with a structured log event and Prometheus counters, the direct_stream_resume_v1 protocol-v3 capability, and a contract doc. Progressive remux is explicitly excluded from the resume contract. Code written by OpenAI Codex CLI (gpt-5.6-sol) from a Claude-authored spec; reviewed and verified by Claude. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(playback): harden direct stream resume contract * test(playback): cover resume platform contracts --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com> |
||
|
|
22fec4ed2d |
feat(metadata): add resilient match queue diagnostics (#463)
* feat(metadata): add resilient match queue diagnostics * fix(metadata): harden match queue lifecycle --------- Co-authored-by: Quick104 <31828688+Quick104@users.noreply.github.com> |