S270b: the docs sweep followups — 13 verified-open items, rewritten against the code - #103
Merged
Conversation
Nine of the sweep's fourteen in-repo items, each re-verified against the code at a named file:line before any edit. Claims that verified TRUE were left byte-identical. S158 MOVED-FILE BLOCK (sweep item 8) — the largest correction. S158 shipped (`3378bb2c`, reachable from origin/master, 22 integration tests): the scan records an adoption candidate and `LibraryManager::pruneRemovedItems()` calls `adoptRecordedPath($id)` immediately before the DELETE, re-pointing the row instead. The docs still told operators a plain move destroys watch state, and told Radarr users not to change root folders. The sweep listed three places; there were SEVENTEEN saying the same wrong thing, so all seventeen moved together — fixing three would have left them contradicting fourteen. Five sub-clauses stayed because they are still TRUE, and a blanket deletion would have lost them: a plain `scan` job records nothing (it never prunes) so the stale row survives and will not play; the standalone `prune` job still deletes; a move that also RENAMES is a different canonical key and still lands a new UUID; the 20,000-candidate cap; and the 2,000-entry probe budget. like_level (item 9) — documented `0-3`, actually `-2..2` (`MIN_LIKE`/`MAX_LIKE`), a signed thumbs axis. Wrong in TWO places, not the one the sweep found. Both 400 bodies differ and are now quoted exactly. Catalog source duplicate (item 10) — documented as a 200 no-op; the service throws `InvalidArgumentException(..., 409)` twice and the controller maps it to `plugin.catalog.url.duplicate`. Added the 409 response; URLs are compared after normalisation. No prose page claimed otherwise, so this is spec-only. "Immutable default catalog URL" (item 11) — OVERSTATED, not false: "cannot be removed" is TRUE (`removeSource()` only filters extras, `sources()` always prepends the default) but the value resolves from the `plugins.catalog.default_source` setting. `docs/plugins/plugin-catalog.md:64` already said so — spec-vs-prose drift again. Also recorded that no endpoint can change it: the key is absent from `server-settings.schema.json`, so `PUT /admin/settings` rejects it. Federation port (item 12) — 8804 -> 8805. FIVE occurrences, not the four the sweep listed. 8804 is SyncPlay. Added the disambiguation, since `FederationWorker`'s own docblock carried this exact error until S66. MCP scopes (item 5) — the fail-open is wider than "omitting `scopes`": the guard is `is_array($rawScopes)`, so null, a bare string, a number or a boolean all grant all four scopes including the write scope. A typo INSIDE an array fails closed; a wrong TYPE fails open. Also recorded that any authenticated user can mint one. media-index offsets (item 13) — the claimed "server<->rail integration tests" do not exist; the real tests use hand-written counts and a mocked ItemRepository. Also documents that the offset invariant does not hold for `rating` or `date_added`, for rows with no value, or when `minRating`/tag filtering diverge. Plugin env vars (item 14) — `PHLIX_PLUGINS_ALLOW_UNVERIFIED` was undocumented in both places `dev/plugin-sdk.md:489` requires. Four more were undocumented anywhere: `PHLIX_GITHUB_TOKEN`, `GITHUB_TOKEN`, `GH_TOKEN`, `PHLIX_ANIDB_CACHE_DIR`. The rule's own preamble said "three env vars"; `PluginsProvider` reads two. Pairing protocol (item 3) — rotation is test-only (`rotate()` has no production caller and no `scripts/rotate-hub-key.php` exists); claim codes are 8 chars not 6 (three places); the user-session `aud` is `phlix-server` not `server`, and §8 is unimplemented hub-side; claim submission is NOT rate-limited; the `kid` is a SHA-256 fingerprint not a timestamp; enrollment state is one file, not two; the per-server `heartbeat_interval` column is inert; and the keypair snippet split at byte 32 instead of 64, which would produce a truncated key if copied. NOT changed: the enrollment JWT's `aud: server` at :356 — the sweep flagged it, but minting and validation both use `server`. It is correct. Backup (item 4) — restore is NOT atomic: no transaction, no snapshot, and a config failure after the mysql import leaves the DB replaced and config/ half-written. Auto-backup requires an explicit `enabled` the sample config omitted, and the schedule is only read at boot. Retention deletes the S3 object OR the local file, never both — and uploading rewrites `file_path`, so an uploaded backup's local archive can never be cleaned up. Five wrong response literals corrected. Gates, each exit code read from its own `$?` on an unpiped command: `npm test` exit 0 (5 files, 85 tests); `npm run docs:build` exit 0, anchor gate 158 page(s), 3456 unique fragment links, 0 dead. +2 vs the origin/master control (3454), both from the outline links of the two new `###` headings (`#retention` in admin/backup.md, `#the-17-endpoints` in admin/remote-access.md); both confirmed present as real ids in the emitted HTML. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…the real model
Sweep item P0.1. The access-schedule section the previous sweep already fixed is
left byte-identical; so are the day/time fields, the "first active match refuses"
rule, overnight ranges, and the fail-open note. Everything else was wrong.
The three security-relevant corrections:
1. "All three layers are evaluated together on every playback request" — FALSE, and
the drawn pipeline does not exist. The access-schedule middleware is the only
GLOBAL middleware, and direct play (`GET /media/{id}/stream`) is served BEFORE
the router, so no schedule check applies to it at all. A profile inside a
blackout window can still direct-play. Said plainly, because an operator relying
on this for a bedtime cutoff does not have one.
2. Tag blocking was described as a playback gate that denies direct URLs. It is a
row filter inside `ItemRepository` and never blocks a stream. It is also NOT
applied to Continue Watching, Next Up or Search — those are built from playback
state or take only the rating gate. And it is inert on any pre-router path,
because the only writer of the request-context profile id is the access-schedule
middleware.
3. The `allowed` tag list is subtractive, not additive: one allowed tag drops every
item that lacks it. The page said user rules were "additive to — never
subtractive from" admin rules; there is one store, not two tiers.
Also fictional, now removed: subscription tiers (Individual/Starter/Family/Premium
— no plan concept exists; the cap is one integer per profile, defaulting to 1),
household/family stream budgets, the 18 predefined tag categories (tags are
free-text `VARCHAR(100)` with no seed data), a per-schedule Timezone field, group
targeting, and the allow/deny Action column — which the page's own "Schedule
Priority" section already contradicted.
Both error bodies were invented. `ACCESS_SCHEDULED_DENIED`, `STREAM_LIMIT_REACHED`
and `next_available` have ZERO occurrences repo-wide. The real bodies are flat,
`403 {"error":"AccessScheduled",...}` and `429 {"error":"StreamLimitExceeded",...}`,
and neither carries the active-stream list a client was told to render — that needs
a separate call to `GET /api/v1/profiles/{id}/active-streams`.
One behaviour worth knowing that the page never mentioned: stream slots are
reclaimed on a 60-second heartbeat timeout, not when playback stops, so with the
default cap of 1 a viewer can be locked out of their own next title for a minute.
Anchor gate: 158 page(s), 3455 unique fragment links, 0 dead — net -1 against the
previous commit's 3456, from the tag section losing three `###` headings and gaining
two. `npm test` exit 0 (85 tests); `npm run docs:build` exit 0. Both read from `$?`.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… against the code
Sweep item P0.2. These two pages are not invention: phlix-hub's own CHANGELOG (H.2)
describes exactly the UI they document — a "Share Library" modal with server,
library, collaborator email, permission and expiry; a "Libraries I've Shared" table
with inline permission editing; a "Shared With Me" page with a "Browse Library"
link. That UI was built on the server-rendered template stack, deleted with it, and
the Vue replacement re-implemented **list and revoke only**. The pages were left
describing the old one.
Security-relevant, and inverted:
* "The invite is tied to the exact email address it was sent to." The OPPOSITE is
true. Redeeming a link creates the share for whoever redeems it, under their own
account email; a link is bounded only by max-uses and expiry, never by identity.
Now documented as a bearer token, because a reader who believed the old sentence
would forward a link thinking it was safe.
* `permission_level` is stored, validated, returned and rendered as a badge, but
**nothing enforces it** — `LibraryShare::canWrite()` and
`SharedLibraryDto::canWrite()` have zero production callers and are pinned only
by their own unit tests. Both pages now say the level is a label, not a control.
* "DLNA casting requires View + Playback + Download." No permission gates DLNA,
and that tier does not exist. The old troubleshooting entry told a user to ask
for a permission upgrade that would have changed nothing.
Removed as non-existent: the entire email-invite flow (phlix-hub has **no mailer**
— `mail(` has zero hits repo-wide including vendor/, and the one `symfony/mailer`
reference in composer.lock is inside monolog's require-dev and is not installed);
three permission tiers (there are two, `read` and `readwrite`); folder-level and
item-level scopes (a share carries one `library_id`; there is no sub-library
plumbing); and the "Restrict to G-rated content" filter (zero occurrences — the
rating cap is a media-server profile feature with no wiring to hub shares).
Corrected: Shares and Shared With Me are two pages, not one; the shares table has
six columns, not five; "Shared with" shows a display name, not an email, because
the API never returns one; the button is "Open Server" and opens the media server's
own web UI, because the hub has no `/browse/:server/:library` route; invite expiry
has five options, not four; the audit-log greps named a file that does not exist
(`.logs/hub-audit.log`) and event strings that do not exist.
Left byte-identical because they verified TRUE: the legacy-redirect note, the
six-row API endpoint table, "collaborator must have a hub account", the
authentication note, "revoke takes effect immediately", "you cannot re-share",
"your access can be revoked at any time", and "there is no server-side CLI for
sharing". Dropped `**Since:** 0.19.0` from library-sharing.md — no repo has ever
been at 0.19.0 (hub is 0.5.0) and the real origin is hub milestone C.9.
Gates, each from its own `$?`: `npm test` exit 0 (85 tests); `npm run docs:build`
exit 0; anchor gate 158 page(s), 3452 unique fragment links, 0 dead. -3 against the
previous commit, verified rather than assumed: `share-with-friends.md` goes 17 -> 14
headings and `library-sharing.md` stays at 17 (one added, one removed). "Without a
fragment" +2, the two new cross-page links.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Not up to standards ⛔🔴 Issues
|
| Category | Results |
|---|---|
| ErrorProne | 1 medium |
| CodeStyle | 1 minor |
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Rebased onto
f1f942b(S270 Job 1) so its green is measured against the true base — the two branches overlap on 10 files and Job 2 alone still showed:32400in two pages.Denominator
14 in-repo items, 13 worked, 0 deferred (item 6 was Job 1). ~132 claims re-verified; 58 verified TRUE and left byte-identical.
The sweep's own list was incomplete in five places
Found only by going back to the code, not by trusting the list:
audflag was wrong — the enrollment JWT is correct, so it was left aloneWorst finds
share-with-friends.mdsaid an invite link "is tied to the exact email address it was sent to". The opposite is true — the share goes to whoever redeems it.permission_levelis never enforced —canWrite()has zero production callers.parental-controls.mdclaimed all three layers run on every playback request. Direct play is served before the router, so there is no bedtime cutoff on that path.arr-integration.mdtold Radarr users to avoid root-folder moves over a defect S158 already fixed.Gates
Clean detached worktree, every exit code from its own
$?:npm test0 (85 tests),docs:build0.ca79f7aThe −2 is measured, not assumed: heading changes in
share-with-friends.md(17→14),library-sharing.mdflat, plus two new###anchors confirmed present as real ids in the emitted HTML. Local and CI counts agree, sosrcExcludeis holding.23 files.
🤖 Generated with Claude Code