S96 — make a music scan tell the truth about itself#568
Merged
Conversation
A music scan could not be diagnosed from outside the process, which is why the
empty Music library survived four wrong diagnoses. Every fact in the final
production investigation had to be reverse-engineered from
`music_artists.created_at` timestamps.
(a) `MusicLibraryScanner::createLogger()` mkdir'd a `sys_get_temp_dir()`
directory per construction and logged into it, so every line landed inside
the systemd unit's `PrivateTmp` — unreadable without nsenter, destroyed on
restart — and one directory leaked per instantiation (66 on prod, 6,346 on
the dev box). It also accepted ONLY a `StructuredLogger` and discarded any
other PSR-3 logger, while `MediaServicesProvider` passed none at all, so the
"fallback" was the only live path. The logger property is now PSR-3, the
fallback is the shared MEDIA channel (`.logs/app.log`), and the container
names the `logger.media` parameter.
(b) `items_added` was never written for a scan/rescan job: the sink wrote only
`items_found`/`items_updated`/`current_path`, and `markCompleted()`'s
`$finalCounts` parameter had no caller. The scanner now passes its live
`added`/`updated`/`failed` snapshot as the progress sink's 4th argument
(folded into the UPDATE the sink already throttled — no extra statement),
`LibraryManager::scanLibrary()` returns a `ScanResult`, `MediaScanner::scan()`
returns its added count, and the worker stamps the final values. A rescan
also stops discarding its prune count. `items_updated` deliberately keeps
meaning "processed files" — it is the UI's progress numerator.
(c) `reapStaleJobs()` fails EVERY `running` row. Documented rather than
age-guarded: this is the only call site and it runs once at boot, so an age
guard would leave a young orphan `running` forever (the very hang it exists
to fix), and a periodic reaper would kill legitimate multi-hour scans (prod
took 4 h 09 m to its first durable write). `config/process.php`'s claim that
running both spawn paths at once is "SAFE" was true of `claimNext()` and
false of the reaper; it is corrected, and a config test now fails if
`library-scan` count leaves 1.
(d) NOT de-duplicated. `countAudioFiles()` is only free to drop if the progress
denominator or S95's bounded memory is given up; see the worklog.
(e) A `music_artists`/`music_albums` row with a NULL `media_item_id` (one failed
`createMediaItem()`, which swallows its Throwable and returns '') stayed NULL
forever — the natural-key branch returned the stored NULL and short-circuited
before the adoption lookup, measured still NULL after two clean rescans, and
`NULL UNIQUE` means nothing fails loudly. It is now backfilled, adopting a
committed orphan when one exists, which is also the only route by which that
residue case was ever reclaimable.
(f) `ScanResult` gains `failed`, and `library_scan_jobs` gains `items_failed`
(migration 095). A partially-failed scan reported clean success everywhere:
S95's `finally` made the counts self-consistent, removing the last DB-visible
trace. Policy skips (the unknown-artist rule) are NOT counted as failures —
they are tallied in the scan-completion log line instead, once per scan.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… IS NULL guard (S96) The outer catch in flushAlbum() must charge only the files the track loop never reached; the existing log-write-failure fixture is the only shape where some files are already accounted for when it fires, so the discriminating assertions belong there. Also pins that the S96(e) backfill UPDATE carries `AND media_item_id IS NULL` — the column is UNIQUE, so clobbering a concurrent writer's id would point two rows at one media_item and lose an album later. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…er (S96a) has() cannot catch a missing constructorParameter — the entry exists either way — so this asserts the PHP-DI definition itself. PHP-DI skips defaulted optional constructor params, which is exactly what sent every music-scan log line into the unit's PrivateTmp. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…alk (S96d) countAudioFiles() costs 0.0069 ms/file (median of 5 runs, warm 10,000-file tree, PHP 8.3.6) = ~0.4 s for the 61,135-file production path, against a scan that took 4 h 09 m to its first durable write. Every way to remove the second walk costs more than it saves: materialising the file list undoes S95's bounded memory (1,463 B/entry, ~89 MB there), dropping the denominator removes the admin progress percentage, and caching a previous count is stale and useless on a first scan. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…er header (S96) Fix round r1 for S96: all 11 review findings (1 HIGH, 2 MED, 4 LOW, 4 INFO). HIGH-1 — scripts/run-library-scan-worker.php still told the operator that running it alongside the start.php-embedded worker "is safe", the same false claim the step corrected in three other places, in the one document read immediately before doing the unsafe thing. (c)'s entire mitigation is documentation, so this defeated it. Corrected, the reaper named as the reason, and pinned by a new ManagedWorkersConfigTest guard over BOTH spawner documents. MED-2 — severity inversion: losing a whole album logged at `warning` while losing one file logged at `error`, and config/logger.php routes only error-and-above into .logs/error.log, so the quietest signal got a clean file and the loudest three were buried. Level now tracks data loss: the artist/album upsert failures, the per-path summary and LibraryManager's per-library summary all log at `error`; policy-only unknown-artist skips stay `warning`. The test doubles now record the level (they discarded it), so the ladder is pinned. MED-3 — the (a) fix made per-entity debug lines the dominant content of app.log. Dropped the per-TRACK "Upserted track" line: ~89% of the volume (measured 61,135 of 68,247 lines/scan), and its only question — "is the scan writing?" — is now answered authoritatively by items_added. Per-artist/album lines stay: bounded by album count, and the album is S95's flush unit. Prod projection 68,247 lines/11.25 MiB -> 7,112 lines/1.08 MiB per scan. The suite also stopped writing into the working tree's .logs/ (MusicLibraryScannerTest injects a NullLogger; the production fallback is pinned by its own identity test): 27,094 lines/4.33 MiB -> 39 lines/6.6 KiB per run. LOW-4 — a rescan job row carried two definitions of items_added over its lifetime and could move backwards. markCompleted() now writes the three cumulative tallies through GREATEST(), so the row is a high-water mark; the progress denominator/numerator stay absolute. Real-MySQL test, since a mock cannot evaluate GREATEST. LOW-5 — MusicLibraryService::scanDirectory()'s docblock still documented the 3-argument sink the 4th argument crosses. LOW-6 — AudiobookLibraryManager::rescanLibrary() counted per-file failures and discarded the number, reporting a false clean `failed: 0`. LOW-7 — documented why a job that dies mid-scan stamps no final counters, and what closing it for the non-music scanners would take. INFO-8 — migration 095's semicolon caveat described a runner limitation that no longer exists. INFO-9 — ScanResult::toArray()'s consumer list claimed the Arr sync response, which serialises a different result object. INFO-10 — `failed` reached the JSON but nothing rendered it; `library:scan` had the whole ScanResult in hand and discarded it, so it now prints the counters. The phlix-ui ScanJob interface stays a follow-up (S110 is live in that repo). INFO-11 — was the orchestrator's stray worklog, already removed; tree is clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… MED-3) Without this the MED-3 fix is a comment: re-adding a per-track debug line restores ~89% of the app.log volume, and deleting the per-album lines would leave a successful scan with no write trace at any level. Both halves asserted. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…review r2) Fix round r2 for S96: all actionable review r2 findings (1 HIGH, 1 MED, 4 LOW, 2 INFO; F8/F10/F11 are record-only and filed as steps). F1 HIGH — a lost track was charged to nothing and the scan closed at INFO. upsertTrack() returned 'skipped' both for a LOST file (createMediaItem() swallowed its own Throwable and returned '', or the INSERT reported writing nothing) and for a BENIGN unchanged one, so flushAlbum() charged neither. A scan that lost five files was byte-identical to a rescan of an unchanged library: `scanned=5 added=0 updated=0 failed=0`, summary INFO, items_failed 0, and on the "wrote nothing" shape no log line at any level — which is precisely what migration 095 exists to make impossible. The return is now split 'skipped' vs 'failed'; a 'failed' track costs exactly one file and logs its PATH at error (createMediaItem()'s own line carries no path). Re-measured, all six of the reviewer's scenarios: S2 0→1 charged, S3 0→5 charged with 5 error lines, S4/S5 unchanged, S1/S6 still clean. Resolved the self-contradiction this exposed. Measured contract on real MySQL 8.0.46: an INSERT returns lastInsertId() as a string, returns NULL when it wrote 0 rows, and THROWS PDOException on any real error. So flushAlbum()'s "the DB layer throws on error" was right, and the three `=== false` branches were wrong twice over — unreachable for this client, and blind to the one falsy value it does return. All four sites now go through statementWroteNothing(), which also documents why `if (!$result)` would be a bug: a successful media_items INSERT returns the string '0' (UUID PK, no AUTO_INCREMENT). F2 MED — MusicLibraryManager::createDefaultLogger() was a SECOND, unfixed copy of the S96(a) defect: mkdir(sys_get_temp_dir() . '/phlix_music_' . uniqid()) per construction, matching the step's own acceptance-criterion glob, so "no new /tmp/phlix_music_* dirs" was met only for the copy S96(a) looked at. Measured +11 dirs per suite run before, +0 after. Both halves fixed: the shared MEDIA logger, and ?LoggerInterface so MusicLibraryType::getLibraryManager() stops discarding any other PSR-3 logger. getScanner()'s narrowing stays and says why (MediaScanner's ctor). Pre-existing leaked dirs left in place deliberately. F3 LOW — the r1 fix for the toArray() consumer list was also wrong: library:scan reads PROPERTIES and never calls toArray(). Third version names the single real consumer and the grep that establishes it. F4 LOW — MED-3's rationale cited items_added/current_path, which the two sink-less entry points do not have; scoped to the paths where it holds and stated what the other two leave behind (one album ≈ 8.6 files). F5 LOW — items_removed dropped from MONOTONIC_FINAL_COLUMNS: no path ever gives it a prior value, so its clamp could not fire. items_added stays, with the arithmetic showing the final delta EQUALS newRows and the live value is a lower bound on it, so GREATEST is a retraction guard and not a max of rival metrics. NULL cannot bite (all five columns int unsigned NOT NULL DEFAULT 0, verified). F6 LOW — microtime(true) → hrtime(true) for every elapsed interval feeding ScanResult::durationMs (audiobook, music, book, photo, base manager). F7 INFO — no caller of library:scan exists (no unit, cron, docker entrypoint, supervisord or CI job), so a lossy scan now exits 2 and warns on STDERR. F9 INFO — the branch's 2 new phpcs errors in tests/ are gone: the snake_case names renamed, and the bespoke PSR-3 double replaced by an interface mock. Every touched test file is back to its master error count. Test delta against master ffc4173 (7480): 7520, i.e. +40, reconciling to 40 added test* methods. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The hrtime() switch had no coverage at all: mutating one of the two timing lines back to microtime() left the whole audiobook suite green. A clock going backwards cannot be tested without moving the system clock, but the realistic regression (editing one line and not the other, mixing nanoseconds with seconds) can — the mutant now fails with durationMs = -88274223122786272. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…96 r2 F9) Review r2 F9 named 2 new errors; a per-file sweep against ffc4173 found 4, and characterised them slightly wrong (only 2 of the 4 are "Method name"): - ManagedWorkersConfigTest 11 -> 9 two snake_case names renamed - MediaServicesProviderTest 10 -> 9 one snake_case name renamed (not in the finding; same class of defect) - MusicScanMediaItemBackfillIntegrationTest 1 -> 0 BackfillTaggedScanner moved to its own file, following the existing precedent for helper classes under tests/ - MusicLibraryScannerTest 8 -> 7 the bespoke RecordingPsrLogger double dropped for a LoggerInterface mock (done in the previous commit) Every file the branch touches is now at its master error count, and both files the branch adds are at 0. The 7 remaining in MusicLibraryScannerTest and the 9 each in the two config/provider tests are pre-existing and belong to S128. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…s (S96 review r3) Review r3 measured two fixes that could not fail, plus five doc/contract defects. MED-1 — the HIGH fix's `null` arm was pinned by nothing. `statementWroteNothing()` is strict, but every test that made an INSERT report "wrote nothing" went through `MusicSchemaConnection::returnFalseFor()`, which returns literal `false`, while `runInsert()` returned `int 1`. Deleting `|| $result === null` left scanner+library+command+integration at `OK (1113 tests, 10122 assertions)` — byte-identical to baseline. The doubles still modelled the OLD, unreachable `=== false` contract the fix had replaced. So the double now models the measured one: `returnNullFor()` for "wrote nothing", and `runInsert()` returns the insert id as a STRING — `'0'` for `media_items` (CHAR(36) UUID PK, no AUTO_INCREMENT), which is FALSY, so the docblock's "do not simplify to `!$result`" warning is now falsifiable off a real-MySQL box too. Both falsy arms and the `'0'` success are asserted individually via reflection, and the production consequence is driven end-to-end on all four INSERTs the scanner branches on. MED-2 — F6 changed the `hrtime()` pair in five managers and guarded ONE. Mutating the end timestamp back to `microtime(true)` in `BookLibraryManager`, `MusicLibraryManager`, `LibraryManager` and `PhotoLibraryManager` left the FULL suite with zero `durationMs` failures, and `MusicLibraryManagerTest`'s `assertIsInt($result->durationMs)` could not fail at all (`public int` + `(int)` cast). All four now pin BOTH bounds — a one-sided `assertLessThan()` passes for the negative number the mixed-unit bug actually produces. Also: - `backfillMusicMediaItemId()` used `$affected === false`, so a `null` fell through to `$referenced = true` and logged "Backfilled a NULL media_item_id" at `info` for a row still NULL — the inverse of the r2 HIGH, in the same file. Now on `statementWroteNothing()`, pinned by a `returnNullFor()` scenario. - `EXIT_FILES_LOST` 2 -> 3: 2 is `Symfony\...\Command::INVALID`, the framework's "invalid usage", i.e. exactly the meaning this code exists to be distinguished from. The test now asserts the value AND the non-collision. No consumer existed. - `LibraryScanWorker`'s "these three counters through GREATEST()" was made false by this branch's own F5 (`items_removed` left MONOTONIC_FINAL_COLUMNS); corrected. - `ScanJobRepository::COUNTER_COLUMNS`, the single authority for `items_added`: "new TRACK rows" -> "new track `media_items` rows", with the one shape where those differ spelled out. - `MusicLibraryType`'s "~10 copies … recorded as a follow-up" pointed at a step that did not exist; it is 23 classes and the step is S132. Gates on dev PHP 8.3.6 (prod is 8.5; CI pins 8.3 — no gate here measures 8.5): phpunit `Tests: 7523` = 7480 (master ffc4173) + 43 added test methods, 0 failures, own throwaway mysql:8.0.46 on port 34433 (--network host, 3306 never bound); phpstan L9 `[OK] No errors`; phpcs PSR12 src/ 0 errors + only the 2 pre-existing `LibraryMetadataMatcher` warnings; every touched test file at its base phpcs count. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… five (S96 r3 INFO-6)
`statementWroteNothing()`'s docblock said the pre-fix file had "three call sites"
branching on `=== false`. Measured at master `ffc41739` there were FIVE, in FOUR
methods: `upsertArtist()` (:1083), `upsertAlbum()` (:1206), `upsertTrack()` twice
(:1324, :1341) and `createMediaItem()` (:1712) — and all five are on the helper now,
so the sweep is complete and only the count was wrong. Review r3 finding 6 raised
the same miscount in the Fix r2 worklog prose ("all four"); this is the copy a
reader actually meets, and it is this file's third doc-accuracy finding, so the
number is now the measured one with the sites named.
Comment-only. Gates on dev PHP 8.3.6: php -l clean, phpcs PSR12 0 issues on the
file, phpstan L9 [OK] No errors, MusicLibraryScannerTest OK (49 tests, 419
assertions).
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Review r4 (MED, single finding): the r3 round's own new test armed the scanner double with `null` for the backfill UPDATE and claimed in a comment that this is "the way the real client does" it. That comment was false, and the state it modelled cannot occur. Measured again here against real MySQL 8.0.46 through PhlixMySQLConnection, using the scanner's verbatim statement `UPDATE music_artists SET media_item_id = ? WHERE id = ? AND media_item_id IS NULL`: guard-excluded row -> int 0 re-run of the same statement -> int 0 healable row -> int 1 matched but value unchanged -> int 0 unknown column -> THROWS null -> never `Connection::query()` returns `rowCount()` whenever the leading keyword is `update` (vendor/workerman/mysql/src/Connection.php:1859-1860, after trim() at :1835), so `null` is unreachable for this statement. Consequence: the only production-reachable arm of the guard at MusicLibraryScanner.php:1644, `is_int($affected) && $affected < 1`, was pinned by nothing — dropping it left the FULL suite byte-identical to baseline, assertion count included. The production predicate is CORRECT and unchanged. What changes: * MusicSchemaConnection::returnAffectedRowsFor($needle, $affected) — the measured UPDATE/DELETE/REPLACE arm, returning before the table handlers so the in-memory row is not mutated either. It REFUSES an INSERT/SELECT/SHOW needle, so the same defect cannot be written in the opposite direction. * testABackfillUpdateThatWroteNothingIsNotReportedAsHealed() now has two scenarios: (A) the measured `int 0`, which is what production takes, and (B) the `null` arm kept and relabelled honestly as defence-in-depth against a bare createMock(Connection::class) and the keyword-miss branch at Connection.php:1866 — not as real-client behaviour. * testTheSchemaDoubleModelsTheClientsPerKeywordReturnDomain() asserts the per-keyword mapping (select/show -> list, insert -> string|null, update/delete/replace -> int) directly, because the return domain belongs to the STATEMENT KEYWORD and not to the client. That over-generalisation is what produced this finding. * The guard site and statementWroteNothing()'s docblock now record which arm is reachable, which test pins each one, and that five of the helper's six call sites are inserts while the sixth is this UPDATE. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…t documents
Review r5 returned 3 LOW + 4 INFO on S96, all of them claims that overstated
what was verified. This closes them.
LOW-1 — the double contradicted two rows of the four-row table fix r4 cited it
as pinning: `SHOW` returned `int 1` (no `show` arm, fell through to
`runUpdate()`) where the client returns an array, and `TRUNCATE`/`SET` returned
`int 1` where the client returns `null`. `MusicSchemaConnection` now derives the
keyword exactly as the driver does (`keywordOf()` mirrors
`trim()`/`explode(" ")`/`strtolower()`) and dispatches on all four rows,
`default` included. The fidelity test asserts every row it claims to pin —
select/show/insert/update/delete/replace/anything-else — which also closes the
`DELETE`-regresses-to-null and `SHOW` holes r5 drove through the meta-pin.
LOW-2 — the arm-time refusal read only the first `" \t\n"`-delimited token while
the needle is matched with `str_contains()`, so r5 measured four bypasses, one
of which armed an INSERT with `int 0`. The refusal now matches the way the
dispatcher matches: the needle must not mention insert/select/show and must
mention update/delete/replace. Its honest residue (a keyword inside an
identifier, `INTO metadata_updates`) is closed one layer down by
`keywordFaithful()`, a dispatch-time check that sees the statement rather than
the needle — so the unproducible shape cannot be returned however it was armed.
`query()` now has exactly one `return`, funnelling every arm through that check,
and the single-return structure is itself pinned, which closes r5's
new-arm-outside-the-guarded-helper defeat.
LOW-3 — comment-only, and the src half of this commit is comment-only
(token-stream verified): "`null` is unreachable for an UPDATE" was false.
`Connection.php:1854` splits on spaces only, so a reformatted UPDATE
(`"UPDATE\nmusic_artists …"`, a tab, a leading block comment) returns `null` on
real MySQL. Both guard halves are load-bearing and both comments now say why.
INFO — the unexercised `?: ''` fallback is gone with the rewrite; both refusal
messages are now distinguishable and pinned; the audit's "13 arms" is corrected
to 19 (13 return-value + 6 fault arms) where `faultOnNth()` is declared.
Production predicate at the backfill guard unchanged.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…null arm
The scenario labels and the assertion message inside
testABackfillUpdateThatWroteNothingIsNotReportedAsHealed() still carried the
claim review r5 disproved — "the ONLY half a real UPDATE can trip" and "`null`
is unreachable for this statement". `Connection.php:1854` splits with
`explode(" ", …)`, so the real client returns `null` for an UPDATE whose keyword
is followed by a newline, a tab, or a leading block comment (measured on real
MySQL 8.0.46 at r5). Both arms are load-bearing; the messages a future round
will read now say so, and the stale `MusicLibraryScanner.php:1644` citation is
replaced by the method name.
Assertions and behaviour unchanged — message text only.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…-time check Review r6 LOW-1: the arm-time refusal `returnAffectedRowsFor()` grew in r4 and r5 was refusing legitimate needles — `'SET a.total_tracks = (SELECT COUNT(*)'` (lifted verbatim from `refreshAlbumTrackTotal()`'s production UPDATE, whose real return is an `int`), `'UPDATE settings SET selected'` and `'DELETE FROM tv_shows'` — because a needle is a substring and `str_contains($needle, 'select')` fires on a subquery, a column name and a table name. Worse, its message told the reader to use `returnNullFor()` instead, i.e. to model `null` for an UPDATE: the exact r3/r4 defect the whole thread exists to prevent. Removed rather than patched a fourth time. `keywordFaithful()` classifies the STATEMENT, which is what the driver does, and for this arm the check is total: the method can only arm an `int`, and an `int` is faithful for update/delete/replace and nothing else. Every needle r4 and r5 measured is now re-pinned at dispatch (10 rows, distinguishable per-keyword reasons), plus the six needles that must arm AND take effect, plus an inert leading-whitespace needle. Review r6 PRIORITY 2: pin the eighth defeat shape. Dropping `keywordOf()`'s inner `trim()` left the file green while the double answered `null` for `"UPDATE\t music_artists …"`, which the real client reports as an `int`. Asserted, together with the two other rows r6 found documented-but-unasserted (tab-with-no-space, leading block comment). Review r6 LOW-2: `keywordOf()`'s table now marks each row asserted or unreachable, says which part of the derivation each asserted row pins, and records that the outer `trim()` cannot be reached because `query()` `ltrim()`s first. Review r6 LOW-3: `keywordFaithful()`'s `default => 'null'` arm is now evaluated by three real statements, so `default => 'int'` is RED. `src/` stays COMMENT-ONLY (4899 tokens, md5 e430dd0553304c3c62d56d495d60d00d, identical at 1ed09c7, 372a162 and here): two comments narrowed r5's "followed by a single space", which r6 measured as not the rule. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…docblock Prose wrap only — no new claim. Re-verified COMMENT-ONLY by token stream: 4899 tokens, md5 e430dd0553304c3c62d56d495d60d00d, identical to 1ed09c7, 372a162 and 77150dd, and `git diff HEAD -- src/` has 0 non-comment changed lines. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…rage gaps
Review r7 (1 LOW + 1 INFO, neither blocking):
- LOW-1: the funnel's "totality" rested on an UNSTATED, UNENFORCED precondition —
resolveQueryReturn() being the SOLE reader of $this->affectedOn. r7 measured a real
loss: a second, non-funnelling public reader hands `int 0` back for a SELECT and every
layer stays GREEN. Closed MECHANICALLY, not in prose: the existing meta-pin
testEveryQueryReturnFunnelsThroughTheKeywordFidelityCheck() now also asserts
`->affectedOn` is referenced EXACTLY twice (the write in returnAffectedRowsFor(), the
one read in resolveQueryReturn()), via a token-based countPropertyReads() helper scoped
to the class's own line range. A sentence would have to be re-read to work; a count
fails when someone adds the third reference. r7's exact 9A shape goes from
`OK (53 tests, 453 assertions)` to `Tests: 53, Assertions: 453, Failures: 1`.
Both of r7's spellings are caught — including 'INTO metadata_updates', which the
DELETED arm-time layer never covered — so the new guard is strictly stronger than the
layer fix r6 removed. The KNOWN-LIMIT block gains the missing limit and the new guard's
own measured residue (a dynamic `$this->{'affectedOn'}` access evades the count: GREEN).
- INFO-1: src signpost at MusicLibraryScanner.php restored the qualifier its own target
docblock states — the fidelity test asserts every whitespace layout the keywordOf()
table names AS REACHABLE; row 6 (the driver's outer trim()) is asserted by nothing and
the table says so. COMMENT-ONLY: 4899 tokens / md5 e430dd0553304c3c62d56d495d60d00d,
unchanged at 1ed09c7 / 372a162 / 77150dd / 3bfb2b0 / this tree.
Coverage (S96's first measurement, PCOV 1.0.11 / PHP 8.3.6). New-code coverage on the
branch's changed src files was 87.86% (246/280 added executable lines). Six tests added
for genuinely reachable, AC-load-bearing gaps, each mutation-proven:
- LibraryManager::scanLibrary() routing for photo/book/audiobook + all three
scan*Library() helpers were at count=0 — 22 added lines, so `return 0;` in any of them
was a GREEN mutation and a fully successful scan of those three types would have gone
on reporting items_added = 0, i.e. AC (b) unpinned in three library types. The scanner
LABEL is asserted too ('image', not the 'photo' ENUM member).
- the music arm of that same routing block.
- MediaScanner::scan() `return 0` for a missing path (feeds items_added).
- MusicLibraryScanner: the album backfill's adoption closure (the artist twin was
covered, the album one was not); backfillMusicMediaItemId()'s failed-mint block
($mayAdopt = true; return null) — AC (e); and upsertTrack()'s repair branch returning
'failed' vs 'updated' — AC (f), r2's HIGH finding, both arms distinguishable.
Two 0.00% files are tooling artifacts, NOT missing tests, and are left alone (S141 is out
of scope): ScanResult.php measures 0/13 with @Covers and 13/13 with it stripped;
MusicLibraryType.php 0/11 -> 7/11. Both are executed and asserted by existing tests.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…dOn meta-pin Review r8 defeated the `->affectedOn` reference count three ways, each handing an `int 0` back for a `SELECT` — r7's own 9A defeat — while the count still read exactly 2, and none of the three was named in the KNOWN-LIMIT block. It also measured a fourth bypass (a public wrapper on `resolveQueryReturn()`) that skips `keywordFaithful()` with zero new `affectedOn` references, and showed that one residue the block DID name (a rename) is not a residue at all: it fails loud. Closed rather than documented: - widen `private` -> `public` and read the store from a test body: closed by asserting the property stays private, which also closes the subclass route independently of `final`. - relocate the single read into a delegate method: closed by counting per-METHOD as well as per-class, so "the ONE read in resolveQueryReturn()" is now a pinned fact rather than a claim a token counter cannot see. - `get_object_vars($this)['affectedOn']`: closed by counting the NAME in every spelling (`->affectedOn`, `$affectedOn`, `'affectedOn'`), which also closes r7's two disclosed residues — the dynamic access and an in-class reflection read. - r8 finding 2, the public wrapper on `resolveQueryReturn()`: closed by asserting the method stays private AND that its name occurs exactly twice in the class (declaration + the one call from `query()`). `countPropertyReads()` becomes `countNameTokens()`: spelling-agnostic, and optionally scoped to one method's line range. Comments still cannot move the number — a whole comment is one token that matches none of the three spellings. The KNOWN-LIMIT block now records the measured residue instead of a guessed one: a test that defines its own double class, a name assembled at runtime (`'affected' . 'On'`), a `ReflectionProperty` read from OUTSIDE the class, duplicating `resolveQueryReturn()`'s body instead of calling it, and `nullOn`/`falseOn`. The false "rename" claim is named only to un-name it. No production file touched. No new test methods: `Tests: 7534` unchanged, MusicLibraryScannerTest 55 tests / 468 -> 473 assertions (+5). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…rong Mutation MUT-H measured the rename against the NEW pin: because the isPrivate() assertion runs first, a renamed property never reaches a count and the test ERRORS with `ReflectionException: Property …MusicSchemaConnection::$affectedOn does not exist` — it does not fail `"0 is identical to 3"` as the block claimed. Still RED, but a block written to stop prose over-claiming must not over-claim its own failure mode. Docblock only; `OK (55 tests, 473 assertions)` unchanged. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… limits
Review r9 measured two GREEN defeats of the affectedOn meta-pin that the
KNOWN-LIMIT block said could not happen, both exploiting the same root
cause: countNameTokens() is scoped to the class's LINE RANGE, and a trait
is inside the class semantically while sitting outside that range.
- MUT-TRAIT: a `use`d trait holding a public non-funnelling reader handed
an int 0 back for a SELECT with all seven assertions green.
- MUT-SPARE: the "total must be 3" pin attributed only 2 of its 3
occurrences, so moving the declaration into a trait freed a slot for an
in-class reader at the same total.
Closed at the root rather than documented:
- assertion 3 pins `getTraitNames() === []`;
- assertion 5 pins the sum over every method the class DECLARES at 2, so
all three occurrences are attributed to a location and no slot is free.
A flattened trait method reports as declared by the using class while
its line range resolves to the trait, so this catches MUT-TRAIT too.
- countNameTokens() now also matches T_ENCAPSED_AND_WHITESPACE, closing
the nowdoc/heredoc-spelled name (r9 finding 4).
The KNOWN-LIMIT block is restructured to stop generating findings by
making a completeness claim it cannot keep: it now states the invariant
the nine assertions enforce, then states explicitly that the evasion list
is NOT exhaustive and why, then lists the known evasion CLASSES rather
than spellings, each with a measured GREEN behind it. The false entry
r9 found ("duplicating resolveQueryReturn()'s body", RED in its literal
form) is recorded as RED, not as a residue.
No production file is touched.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Measured, not assumed. A trait fires assertion 3 FIRST; a token probe shows assertion 5 sees both trait mutants independently (method sum 2 -> 3) while assertion 4 does NOT (the class-range total stays 3), so "closed by assertions 3 and 5" was true but imprecise about ordering. The no-trait assertion's own message said the trait-held reader kept "all seven other assertions" green — that was true of the SEVEN-assertion version review r9 attacked, not of this nine-assertion one, where the method-sum assertion also sees it. Reworded to say so. Message text and docblock only: the token count is unchanged at 16046 and no assertion, expected value or helper is touched. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…prose Review r10's three findings are all stated claims that its own measurements falsify. Text only — no assertion, helper body, charlist or token-type list is touched, so there is NO behaviour change: the file still runs OK (55 tests, 475 assertions), and MUT-TRAIT / MUT-PROMO stay RED at the same assertions with their messages intact. 1. LOW-MED — the file asserted both A and not-A about scoping, and the false version sat in assertion 3's own FAILURE MESSAGE, i.e. the text an engineer reads at the moment a trait is added. "Every count below is scoped to the class's own line range" is false of assertion 5: it sums over the methods the class DECLARES, and a flattened trait method's range resolves into the TRAIT's file and lines, which is precisely why it catches trait placement independently of assertion 3 (r10 measured A5_methodSum 2 -> 3 under MUT-TRAIT/MUT-SPARE while A4 stayed 3). All three sites now state the split — 4/6/7 class-range, 5 per declared method — consistent with what countNameTokensInMethods()'s docblock and the "WHAT IS CLOSED" r9 bullet already said correctly. 2. LOW — countNameTokens() OVER-counts and its docblock described something narrower than what it does. It counts four token types, and the trim() charlist includes whitespace, which is LOAD-BEARING (a nowdoc token's text is "affectedOn\n"), so a padded literal and an interpolation fragment count too; summing per-method also double-counts two declarations sharing one physical line. Both are disclosed, with r10's measurements and the reason the direction is safe: an over-count can only produce a spurious RED, it can never hide an evasion, since a reader that is present cannot subtract from a count. The charlist is deliberately left alone — narrowing it would re-open the nowdoc evasion. 3. LOW, cosmetic — an out-of-class private read is a RUNTIME Error: Cannot access private property, not a compile failure; php -l exits 0 and the script prints before dying. The KNOWN-LIMIT block is untouched: it still declares itself non-exhaustive and still lists evasions by class, not by spelling. No evasion was added to it and no completeness claim was restored. 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 |
|---|---|
| Compatibility | 1 high |
| UnusedCode | 1 medium |
| BestPractice | 3 medium |
| Documentation | 17 minor |
| ErrorProne | 4 medium |
| CodeStyle | 66 minor |
| Complexity | 7 medium |
| Comprehensibility | 1 minor |
🟢 Metrics 122 complexity · 0 duplication
Metric Results Complexity 122 Duplication 0
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.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #568 +/- ##
============================================
+ Coverage 64.95% 65.30% +0.34%
- Complexity 20931 20957 +26
============================================
Files 656 656
Lines 65856 66023 +167
============================================
+ Hits 42775 43114 +339
+ Misses 23081 22909 -172 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
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
2 participants
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.
S96 — make a music scan tell the truth about itself
Observability for the music scan path: severity, log volume, the stale worker header, lost-track
accounting, and the durable-progress fields a caller can actually trust. Builds on S95's two-phase
deferred write (
project_music_scan_hang_fixedcontext:a scan completed end-to-end for the first time on 2026-07-25, 61135/61135).
33 files, +5518/−312. Production changes are confined to
src/Media/{Library,Music}; the rest is tests.Highlights
MusicLibraryScanner(+745) — truthful per-run accounting:$handledremainder, charging losttracks rather than silently dropping them, per-album (not per-track) log volume, and one
temp-dir logger instead of two.
ScanResult(+73) /ScanJobRepository(+141) — durable progress fields and the backfill'sIS NULLguard pinned on the arm real MySQL actually returns.MusicLibraryType/MusicLibraryService/MediaScanner/ the library managers — severity anddurationMscorrections, including adurationMspin against mixed-unit timing.Verification
Tests: 7534, Assertions: 57601, Skipped: 7— exit 0phpstan.neon.dist)[OK] No errors— exit 0src/)LibraryMetadataMatcher.phpwarningsphp -lsrc/lines@covers-discarded lines)Tests: 7534= masterffc41739's 7480 + 54 added.merge-tree --write-treeagainst master is clean(0 conflicts).
Review history
Ten review→fix rounds (r1–r10); every finding fixed, including low and informational. Two lessons
recorded from the sequence rather than left implicit:
make accidental drift and plausible refactors loud, not to be tamper-proof. Rounds 7–9 each found a
new way to defeat a token-level check, which is unbounded; the block now states the invariant it
enforces and lists known evasions by class rather than by spelling.
assertion 5 sums over the methods the class declares, and a flattened trait method's range resolves
into the trait — which is why it catches trait placement independently of assertion 3.
getMethods()reporting a trait-flattened method as declared by the using class was verified empirically on 8.3.6.
A3
:2865→:2873, A4:2876→:2888, A5:2888→:2900. A4's new number equals A5's old one, socite the assertion by name, not just by line.
🤖 Generated with Claude Code