Align StarCache implementation with technical specification hardening requirements

[Copilot]

Summary

Align StarCache with TECHNICAL-SPECIFICATION.md as a deterministic cache orchestration engine (not a backend caching layer), while preserving stored-file asset minification behavior.

Changes

• Added adapter hardening and backend capability introspection in StarCacheAdapter.
• Added Predis detection/connection path alongside PhpRedis.
• Made Redis/Memcached/Memcache group handling explicit and safe via backend-internal namespaced storage keys.
• Guarded dangerous global flush behavior behind STARCACHE_ALLOW_DANGEROUS_FLUSH.
• Added adapter add() primitive to support lock semantics without moving orchestration logic into adapter.
• Added cache stampede protection in StarCache::star_remember() with lock keys, soft TTL/hard TTL handling, and optional stale-while-revalidate behavior.
• Added final eligibility gate in StarPageCache::capturePageOutput() before persisting HTML.
• Hardened minified asset writes with atomic temp-file rename flow and fallback handling.
• Added stale hashed asset cleanup sampling strategy in StarAssetMinifier.
• Added/updated documentation in README.md for backend capability detection and dangerous flush guard.
• Extended test/bootstrap stubs for scheduling, object-cache-add capability, Memcached constants/classes, and flush-call counting.
• Added tests for:
  • first request serving original asset + subsequent request serving stored minified asset,
  • Redis/Memcached round-trip correctness for false/array/string/null values,
  • version-bump key invalidation without backend flush,
  • multisite blog-ID isolation across page/object/query/asset cache spaces.

Validation

• Performed syntax validation with php -l on all modified PHP files.
• parallel_validation (Code Review + CodeQL) was run repeatedly and actionable feedback was addressed.
• Repository scripts composer test, composer lint, and composer analyze could not run in this environment because Composer dependency installation was blocked by GitHub authentication requirements (Could not authenticate against github.com).

[Copilot]

Pull request overview

This PR hardens StarCache’s adapter layer to better align with the technical specification as a deterministic cache orchestration engine, while preserving the stored-file asset minification behavior and adding stampede protection to star_remember().

Changes:

• Added backend capability introspection plus a Predis probing path alongside PhpRedis in StarCacheAdapter.
• Introduced StarCache::star_remember() stampede protection (lock key + soft/hard TTL envelope + optional stale-while-revalidate).
• Hardened asset minifier writes (unique temp file + atomic rename) and added sampled stale hashed-asset cleanup.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
tests/UnitTests.php	Adds test doubles and new unit tests for async asset minification, backend round-trips, version invalidation, and multisite isolation.
tests/bootstrap.php	Extends WP stubs for scheduling, wp_cache_add(), and flush-call counting used by new tests.
StarPageCache.php	Adds a final eligibility check immediately before persisting the page cache payload.
StarCacheAdapter.php	Adds capability introspection, Predis probing, group-namespaced storage keys, guarded flush, and add() primitive for lock semantics.
StarCache.php	Implements star_remember() stampede protection with lock coordination and soft/hard TTL envelope handling.
StarAssetMinifier.php	Hardens atomic writes and adds sampled cleanup of stale hashed minified assets.
README.md	Documents Predis probing, capability introspection, and the dangerous flush guard constant.
phpstan-bootstrap.php	Adds stubs for Redis/Memcached/Memcache and missing WP functions referenced by the new adapter logic.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[MaximillianGroup]

@copilot - Main improvement

This now looks more like the intended cache orchestration engine, not just a simple WordPress cache helper.

The strongest additions are:

Predis support added after PhpRedis.
Backend capability reporting added.
Dangerous flush guard added with STARCACHE_ALLOW_DANGEROUS_FLUSH.
Redis/Memcached group namespacing added through internal storage keys.
add() support added for lock keys / stampede protection.
Stored-file asset minification is preserved.
Atomic asset writes added with unique temp files.
Stale hashed asset cleanup added.
Final page-cache eligibility gate added before storing HTML.
Tests added for asset build flow, backend round-trips, version invalidation, and multisite isolation.

That is the correct direction.

My merge opinion

Do not merge yet. Approve the direction, request revisions.

The PR fixed several big architectural concerns, but there are still production details I would tighten.

Remaining issues

1. isset() is still present

This violates your code preference.

Example:

if (!isset(self::$groupHashCache[$normalizedGroup])) {

Use:

if (!array_key_exists($normalizedGroup, self::$groupHashCache)) {

There is also ?? usage in tests and payload normalization. That may or may not bother you, but the direct isset() should be removed.

2. Flush is safer, but still not ideal

The guard is good:

STARCACHE_ALLOW_DANGEROUS_FLUSH

But even guarded flushDB() can still be dangerous if Redis DBs are shared by multiple apps or environments. For your architecture, production invalidation should be version bump / tag purge only.

My preference:

StarCacheAdapter::flush() should be dev/test only.
Production code should not rely on it.
3. Predis detection needs hardening

This is a good start:

$client = new \Predis\Client($params, ['exceptions' => false]);
$pong = $client->ping();

But Predis responses can vary depending on version/config. I would make detection tolerant of 'PONG', status response objects, and false/null failure.

4. Stampede protection is good, but not true background SWR

The new star_remember() logic is much better. It has soft TTL, hard TTL, lock key, and stale return behavior.

But the “revalidate” part is still mostly synchronous. If the caller gets the lock, that request regenerates the value. That is fine, but I would describe it as:

soft TTL + lock-based stale protection

not full background stale-while-revalidate yet.

5. Asset cleanup should probably be cron-based later

The sampled cleanup is acceptable:

1 / 20 requests

But long-term, frontend requests should not be doing filesystem cleanup. I would eventually move stale asset cleanup to a scheduled task.

6. Query cache is still hanging around

The tests still reference StarQueryCache. If this class is deprecated, I would not keep building new production confidence around it.

I would either:

A) remove StarQueryCache before production

or

B) mark it legacy and exclude it from new architecture tests
Best Copilot response

Use this:

Good direction. Please revise before merge.

Required changes:

1. Remove direct isset() usage and replace with explicit array_key_exists(), is_object(), or strict checks as appropriate.
2. Keep StarCacheAdapter::flush() blocked by default, but document it as dev/test only. Production invalidation must use version bumping and edge purge, not backend flush.
3. Harden Predis ping detection so it accepts valid Predis status responses, not only non-null/non-false.
4. Rename or document the star_remember() behavior as lock-based soft TTL protection unless true background stale-while-revalidate is implemented.
5. Add tests proving only one callback executes during lock contention where possible.
6. Move stale asset cleanup toward a cron-driven cleanup path or at least add a TODO noting frontend sampled cleanup is temporary.
7. Do not expand StarQueryCache. If deprecated, isolate it from new production architecture confidence tests.
8. Confirm all new test stubs are complete and syntactically valid; the pasted PR view suggests some stub sections may be partial.

Do not change the stored-file asset minification model. The first request should continue serving the original asset while the stored minified file is built for later requests.
Bottom line

This PR is meaningfully better than the earlier state.

I would call it:

Direction: correct
Spec alignment: improved
Production safety: better, not finished
Merge status: request revisions

The most important part: the repo is now moving toward the right identity — StarCache as the deterministic cache brain for Redis/Memcached/Varnish/Cloudflare coordination, not a confused all-in-one page-cache plugin.

[Copilot]

@copilot - Main improvement

This now looks more like the intended cache orchestration engine, not just a simple WordPress cache helper....

Implemented the requested revisions in commits 404535b and f29ca40.

Key updates:

• Replaced direct adapter isset() usage with array_key_exists().
• Hardened Predis probing: tryPredis() now catches probe exceptions and validates ping as real PONG (string/status payload), rejecting error-like responses.
• Clarified star_remember() docs as lock-based soft-TTL stale protection (not full background SWR).
• Documented StarCacheAdapter::flush() as dev/test-only and reinforced production invalidation via version bump + edge purge.
• Added TODO for moving sampled stale-asset cleanup toward cron.
• Added lock-contention test coverage to ensure callback is not executed again while refresh lock is held.
• Added Predis ping helper tests for valid and invalid response variants.
• Kept StarQueryCache coverage explicitly scoped as deprecated query-cache space in the multisite architecture test naming/context.

[MaximillianGroup]

@copilot - What improved

The PR now addresses the main issues I called out:

README now correctly describes star_remember() as lock-based soft-TTL stale protection, not full background SWR.
Dangerous flush is documented as dev/test only, with production invalidation expected through version bumping and edge purge.
Predis ping handling now has tests for valid and invalid responses.
A lock-contention test was added to prove stale values are returned without running the callback again.
Asset cleanup now has a TODO to move cleanup into cron.
The deprecated query cache test name now explicitly calls it deprecated.
The stored-file asset minification model remains intact.
Final page-cache eligibility gate is still there before saving HTML.

That is real progress.

My concern now

The pasted GitHub diff still looks like it may be showing duplicated removed/added lines mixed together, so I cannot fully confirm whether the actual file is clean without seeing the full StarCache.php and StarCacheAdapter.php.

For example, the diff display still shows this suspicious sequence in StarCache.php:

$found = false;
$cached = $this->star_getCachedData($reference, $userId, $found);
if ($found) {
return $cached;
$hardTtl = $ttl > 0 ? $ttl : self::CACHE_EXPIRATION_DYNAMIC;

That may just be GitHub’s compact diff view blending old and new code, but if that is literally in the file, it is a syntax error. Same issue in StarAssetMinifier.php, where the diff view appears to show both old and new temp-file write logic.

So my merge advice is:

Do not merge from the pasted diff alone.
Run tests / PHP syntax / PHPStan first.
Final pre-merge checklist

Have Copilot run or confirm:

find . -name '*.php' -print0 | xargs -0 -n1 php -l
composer test
composer phpstan
composer phpcs

Then ask for these final revisions:

Final pre-merge cleanup:

1. Confirm StarCache.php and StarAssetMinifier.php contain only the new logic, not duplicated old/new diff fragments.
2. Remove remaining direct isset() usage in production code.
3. Confirm StarCacheAdapter.php large diff renders cleanly and includes no unreachable duplicate return paths from the old key handling.
4. Confirm StarCacheAdapter::flush() remains blocked by default and is documented as dev/test only.
5. Confirm the deprecated StarQueryCache remains isolated and is not expanded as part of the new production architecture.
6. Add or confirm index.php files in every folder, including tests/, if this repo must follow that standard.
   My updated rating
   Direction: correct
   Spec alignment: strong
   Redis/Memcached work: much improved
   Asset minifier model: correct
   Production readiness: close, pending syntax/static checks
   Merge status: almost, but verify actual files first

My opinion: this is now a serious caching orchestration repo. One more verification pass, then it can probably merge.

[MaximillianGroup]

@copilot - #3 (review)

[Copilot]

@copilot - Main improvement

This now looks more like the intended cache orchestration engine, not just a simple WordPress cache helper....

Addressed in eaa4a05.

Implemented the requested follow-ups:

• removed direct isset() usage in adapter group-hash caching
• hardened Predis ping probing/error handling and kept backend probing resilient
• documented flush as dev/test-only with production invalidation guidance
• clarified star_remember() as lock-based soft-TTL protection (not full background SWR)
• added/clarified lock-contention test coverage to ensure no second callback execution during contention
• kept stale asset cleanup marked as temporary frontend behavior pending cron migration
• kept StarQueryCache references clearly labeled as legacy in tests/docs
• verified syntax and ran targeted/full PHPUnit with the available runner (full-suite failures are due to missing PHPUnit 10 assertion APIs in the environment runner)

[MaximillianGroup]

@copilot - there are outstanding fixes

[Copilot]

@copilot - there are outstanding fixes

The outstanding docblock fixes for set() and delete() methods were already completed in commit 66ed5b9. Both $group parameter docblocks now correctly state: "Used for namespacing across all cache backends; maps to a WP cache group for the WP object cache backend." All PHP files pass syntax validation (php -l).

[MaximillianGroup]

@copilot - we still have open fixs with agents working

[Copilot]

@copilot - we still have open fixs with agents working

I checked the current PR state: the previously flagged set()/delete() $group docblocks are already updated in 66ed5b9, and the latest completed workflow run has no failed jobs. I still see some older unresolved review threads marked outdated; if you want additional code changes, please point me to the exact open comment/thread IDs to address next.