feat(resizer): scope the cache key by the source's upload path

[parisek]

Stacked on #152. Base is fix/cleanup-cached-images-shared-file, not main. Merge #152 first; GitHub retargets this one automatically. Do not delete the parent branch until both have landed (AGENTS.md § Stacked PRs).

From project

sloneek. A homepage hero rendered with a black background. The cause turned out to be two independent defects, and this PR is the second: Resizer names a derivative after its source's basename alone, so two uploads that share a name share one cached file.

The measurement

On that site's staging database:

names mapping to more than one source file : 496

The cache namespace is flatter than the namespace it caches, twice over — the derivative is named for the source without its extension, so 11.png and 11.jpg collide as surely as two 11.png in different months do:

900x0-center/11.avif   <-  2022/03/11.png
                       <-  2022/04/11.png
                       <-  2022/08/11.png
                       <-  2022/10/11.png

Whichever renders first writes the file; the rest read it and get a picture of something else. Nothing errors, and the result is a plausible image, so it stays invisible until someone recognises the wrong photograph on a page.

What changed

The source's own upload directory now sits between the size directory and the filename:

<W>x<H>-<style>[-q<N>]/<source-upload-dir>/<name>.<fmt>

Behind StarterBase::$resizer_source_path_in_cache_key, default false, mirroring the $resizer_quality_in_cache_key flag it sits beside. It is derived from the source URL — not the resolved file path — because resize() needs the value before it knows whether the file exists locally: a missing source is handed to DevMediaProxy, which addresses the same cache path on another host.

The segment is written into $variant['cache_key'], not into the target path. cache_key is already the one value both the local writer and DevMediaProxy::build_remote_resizer_variant() derive from, and the comment at DevMediaProxy.php:456-460 names that as deliberate. Appending in processVariant() would have left the remote path in the old shape.

wp timber-kit migrate-image-cache moves an existing cache into the new shape rather than re-encoding it. Dry-run by default; --apply writes. It moves rather than copies (no transient doubling), re-checks the target immediately before every rename(), never deletes, never overwrites, and is idempotent so an interrupted run is resumed by running it again.

Derivatives whose name maps to more than one source cannot be placed — recovering that association is exactly what the flat layout destroyed. They are reported and left alone, and re-encoded on first view.

With the flag on, cleanup_cached_images() addresses derivatives by computed path instead of matching basenames across the tree. That closes the delete half of the same bug. The sibling-attachment guard from #152 stays in force — it answers a different question and is independent of the layout.

A site with year/month folders switched off gets byte-identical paths and needs no migration at all. The suite pins that.

Review

Every task was reviewed independently before it landed. Three Critical defects were caught this way and are fixed in the branch, all of the same shape — a check performed at one moment and relied on at another, where the gap is silent:

1. apply() re-used plan()'s conflict check, and POSIX rename() silently replaces its destination. A target written between plan and apply was destroyed with no error and no entry in failed.
2. The delete path took the filename raw from _wp_attached_file and used it as a glob() pattern, while the writer sanitizes it. Deleting a[1].png removed unrelated a1.png's derivative, because [1] is a glob character class. Names carrying stripped characters silently no-opped and leaked instead.
3. buildNameToDirs() dropped a guard-rejected attachment from the map entirely, which could make a genuine collision read as unambiguous — and move one attachment's derivative into another's directory. The reopened swapped-image bug, inside the tool operators run to make the flag-flip safe.

Each has a regression test that fails against the code as it was.

Three components now implement "is this path component safe to use as a directory" — the writer, the migrator and the deleter. They are deliberately not coupled; the final review compared all three against each other, since any input they disagree on is a bug.

Deliberately not done

• Helpers::resizeImage() — the legacy path predating Resizer — keeps the flat layout. Not migrated, not flagged.
• Symlinks inside the cache tree are not handled. The theme owns and regenerates this directory, so it is outside the threat model. Stated rather than silently skipped.
• The flag defaults off, so the bug stays live until a project opts in. That is the cost of the change invalidating every cached derivative — it has to be the project's decision, not a consequence of composer update. This differs from fix(media): keep cached derivatives a sibling attachment still uses #152, where I argued against a flag because nothing was at stake.

Decision, rejected alternatives and consequences: docs/adr/0007-resizer-source-path-cache-key.md, which lands in this PR.

[parisek]

Agent review — Claude (opus) and Codex (gpt-5-codex), 2026-08-27

Two independent reviewers, different models, neither able to see the other's output. Recorded here because the rejected and deferred findings are the ones the diff cannot explain.

They converged on nothing. That is worth stating plainly rather than hiding: it describes the lenses, not the findings. Claude was asked to verify the fix works; Codex was asked to find where it still does not. Both did their job and returned disjoint sets.

What Codex found that seven Claude review rounds did not

The branch originally added only the source's directory to the cache key. Codex found that closes half the problem: the derivative is named for the source without its extension, so 2026/08/hero.jpg and 2026/08/hero.png still wrote one file, and deleting one removed the derivative the other served.

Measured on production before acting:

colliding names total                  : 496
of those, colliding inside one directory : 152

So the first version fixed 344 of 496 while the ADR, CHANGELOG and README all claimed the job was done — and the ADR's own opening example (11.png vs 11.jpg) was in the broken half.

The key now carries the source's whole sanitized identity, directory and full filename: 900x0-center/2026/08/hero.webp.avif. ADR-0007 was amended rather than superseded, since it had not merged.

Also found and fixed

• Filename came from the encoded URL while everything else came from the decoded database value. sourcePathSegment() already stripped the query string and decoded each directory component; the filename path did neither, so writer and deleter could compute different names. Measured: 230 _wp_attached_file values contain a space — not theoretical.
• Root uploads broke the migration's idempotence claim. A migrated root-upload derivative stays directly inside the size directory, so a second run rescanned it and reported it orphaned. Production has zero root uploads; fixed because the library serves other sites and the spec claims idempotence.
• apply() retained a TOCTOU window between is_file() and rename(). Now link() + unlink() — atomic create-if-absent, so a target appearing mid-run fails the move instead of being overwritten.
• The CLI reported success when moves had failed. WP_CLI::success() ran before the warning loop, so automation got exit code 0 and no detail.
• The ADR contained six statements the code did not implement, including a claim that sites without year/month folders need no migration. Adding the extension made that false and I missed it while amending. Corrected.

Rejected

Codex rated "sanitization can collapse two distinct source names" as High — hero[1].jpg and hero1.jpg sanitize to the same value, so the key is the sanitized identity, not the source identity. Its suggested fix was to append a hash of the canonical path.

Rejected, and Codex was asked to challenge the rejection and agreed with it. Measured zero occurrences of [ or ] in _wp_attached_file, and WordPress sanitizes filenames at upload, so the second sanitisation is normally a no-op. A hash would permanently damage path readability and complicate migration for every site, to guard a case the data does not contain. The limit is now stated in the ADR's Consequences instead — an accepted, documented constraint rather than an accidental gap. Worth revisiting only if unsanitized _wp_attached_file values ever surface in a supported install.

Deferred

• strtok($src, '?') does not strip a # fragment, where a sibling helper uses '?#'. Fails closed; image URLs do not carry fragments.
• guardSourceDir() omits the rawurldecode() step its sibling performs. Different input domain (_wp_attached_file is never percent-encoded); same outcome.
• Symlinks inside the cache tree are not handled — GLOB_ONLYDIR descends into a symlinked directory. Outside the stated threat model: the theme owns and regenerates this directory. Stated rather than silently skipped.
• No test covers link() succeeding and unlink() then failing. The path is handled (recorded as failed, both copies left on disk) but unproven.

A reviewer error worth recording

Codex's first pass called the %i placeholder a silent-delete path on WordPress < 6.2. The premise was right and the consequence was not: the accompanying fail-closed fix means a failed query now keeps the files, so what remains is cache retention, not data loss. On re-examination Codex withdrew the objection. Recorded because the next reader is calibrating how far to trust the rest of that report.

Tests 1750 + 18 green, PHPStan clean, ADR index in sync, CI 6/6.

[parisek]

Rollout notes — measured scope, and what to do when you come back to this

Written for whoever picks this up later, including me. Everything below is measured on sloneek production data, not estimated.

Merging this changes nothing

StarterBase::$resizer_source_path_in_cache_key defaults to false, and the filter behind it defaults to false too. No project sets it. Until one does, the derivative name still drops the source extension, no directory segment is added, and cleanup_cached_images() keeps using the original basename scan — a verbatim extraction, not a rewrite.

So this can sit merged and dormant. Nothing reaches a site until someone opts in and bumps the package.

Contrast with #152, already merged: that one is not behind a flag, deliberately, because gating a data-loss fix would leave the loss live by default. It applies on the next version bump.

How much is actually broken today

The bug this PR fixes is not hypothetical — it is live right now. But the blast radius is smaller than the raw collision count suggests, and the difference matters for scheduling.

Measure	Value
Colliding names in the database	497
Of those, colliding inside one directory	152
Unique names present in cache/image	1376
Colliding names that actually have a derivative	90

90 is the ceiling on visible damage. A collision in the database harms nothing by itself; a wrong image can only be served where the shared derivative physically exists. Ninety cache files are currently standing in for two or more different source images each.

What those 90 are

Almost all of them are auto-generated names — pasted screenshots and import artefacts:

10   ->  2022/03/10.png | 2022/08/10.png | 2023/06/10.png | 2024/03/10.webp
11   ->  2022/03/11.png | 2022/04/11.png | 2022/08/11.png | 2022/10/11.png
1634240049156  ->  2023/01/1634240049156.jpg | 2023/03/1634240049156.jpg

No hero, no logo, nothing a human named. Of six colliding sources sampled, one appears in published post_content, and once.

A looser check over ACF placements (attachment IDs in postmeta) returned 500 of 5286 attachment rows — but read both numbers with suspicion: 5286 is inflated by WPML writing one row per language (≈1000 distinct files), and 500 is an upper bound because the test matches any numeric meta value equal to the ID. Neither number raises the ceiling of 90; they only indicate how the affected images are placed.

Conclusion: real, not acute. The risk concentrates in inline article images, not in page furniture. That is also why nobody has reported it in three years — one screenshot looks like another, and nothing here gets compared side by side. It is the dangerous category: a failure that renders as a plausible success.

When you turn it on

Prerequisite: the Cloudways AVIF encoder must be fixed first. Its ImageMagick 6.9.11 (2021) drops the alpha channel, so anything regenerated there comes out with a black background — see docs/migration/cloudways-imagemagick/README.md in the sloneek repo. Migrating before that trades a silent wrong-image bug for a loud black-background one.

Then, in order:

1. Set $resizer_source_path_in_cache_key = true in the project's Base.
2. Run wp timber-kit migrate-image-cache without --apply and read the numbers. Dry-run is the default for this reason.
3. Expect a non-trivial ambiguous count. Those are the derivatives whose source cannot be recovered — precisely what the flat layout destroyed. They are left alone and re-encoded on first view, which is correct and requires a working encoder.
4. Only then --apply.

Unproven, and worth knowing before you rely on it

The migration command has never run against real data. It has unit tests over synthetic directory trees and two independent reviews, and three Critical defects were caught and fixed in it — but zero production mileage. The dry-run in step 2 is its first real exercise; treat the first run as an experiment, not a formality.

Still open on this branch

Three Minor findings from the Codex review of #152 were recorded there and not fixed: the SQL contract test does not pin the table's position in the query, seedFile() owns paths rather than files, and two adjacent docblocks sit above one test method. None blocks anything; all are on #152.