From 7e74cfdedc6d403c93b60b6d0ce01f2d2edb6095 Mon Sep 17 00:00:00 2001 From: Douglas Date: Fri, 26 Jun 2026 19:59:01 -0300 Subject: [PATCH] Review PR skill: catch cross-package version-skew fatals Add a "cross-package version skew" check to the jetpack-review-pr skill. When a package calls into a sibling it does not declare in composer.json require, guarding with class_exists() is insufficient: standalone plugins bundle their own jetpack_vendor/ copies and the autoloader may load an older version, so a newly-added method/constant is absent at runtime and fatals with "Call to undefined method". The guard must check the exact symbol (method_exists/is_callable/defined/function_exists/property_exists). This is the class of bug behind SOCIAL-515 (SEO\Initializer::is_optin_available() fatal on Social 9.0.2 standalone) and MYJP-308, which both human and Claude reviewers missed because the introducing PR added the new method and its class_exists-guarded caller in the same self-consistent diff. Changes: a new check in step 4 (all depths) with a grep detection recipe, a checks-table row, an upstream-direction note in the cross-project step, and a report heading. Validated RED 0/3 -> GREEN 3/3 against the real #49672 (introducer) and #49988 (fix) diffs. Co-Authored-By: Claude Opus 4.8 (1M context) --- .agents/skills/jetpack-review-pr.md | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/.agents/skills/jetpack-review-pr.md b/.agents/skills/jetpack-review-pr.md index e56fe6a6fee1..fe70d323007e 100644 --- a/.agents/skills/jetpack-review-pr.md +++ b/.agents/skills/jetpack-review-pr.md @@ -127,6 +127,7 @@ Note: `AGENTS.md` is already loaded in your context via the CLAUDE.md `@AGENTS.m | Security patterns | yes | yes | yes | | Security threat model | no | if security files | yes | | Backward compat | yes | yes | yes | +| Cross-package version skew (optional-sibling calls) | yes | yes | yes | | Error handling | no | yes | yes | | Feature gating | no | yes | yes | | HTML / a11y / RTL | no | if has_html/has_css | yes | @@ -170,6 +171,26 @@ Note: `AGENTS.md` is already loaded in your context via the CLAUDE.md `@AGENTS.m - **Removed public constants/properties**: Fatal errors in consumers - Scan diff for removed lines with `public function`, `function jetpack_`, `do_action(`, `apply_filters(`, `register_rest_route` +*Cross-package version skew — calls into optional sibling packages* (all depths — this catches fatal errors): + +A monorepo package may call into another package it does NOT list in its own `composer.json` `require` (e.g. my-jetpack → `SEO\Initializer`, `Search\*`, `VideoPress\*`). On trunk every package is at the same version, so the call resolves and CI passes. But standalone plugins (Social, Boost, VideoPress, Protect, …) each bundle their own `jetpack_vendor/` copies, and jetpack-autoloader loads ONE copy of each shared package across all active plugins — frequently an OLDER version than trunk. The symbol you call may not exist at runtime. + +- **`class_exists()` is NOT a sufficient guard** when you then call a method, read a constant, or access a property on that class. The class can load from an older bundled copy that predates the member → `Call to undefined method` fatal. Guard the EXACT symbol you use: + - method → `method_exists( $class, 'method' )` or `is_callable()` + - constant → `defined( "$class::CONST" )` + - function → `function_exists()` + - property → `property_exists()` +- `[blocker]`: a newly-added call into a non-`require`d sibling package guarded only by `class_exists()` (or unguarded). The same PR often adds both the new symbol (in package A) and its caller (in package B), so the diff looks self-consistent — the skew only appears across release trains. Check the guard, not the definition; an internally-consistent diff does not prove the symbol ships together everywhere. +- Detection: + ```bash + # Cross-package class references added in this diff: + gh pr diff | grep -nE '^\+.*\\Automattic\\Jetpack\\[A-Z][A-Za-z]+\\' + # For each referenced package, confirm it is in THIS project's composer.json "require": + grep '"automattic/jetpack-"' projects///composer.json \ + || echo "OPTIONAL sibling — any method/constant/property access needs a symbol-level guard" + ``` +- Precedent: SOCIAL-515 (`SEO\Initializer::is_optin_available()` fatal on Social 9.0.2 standalone), MYJP-308 (Search product fatal on plugins not bundling jetpack-search). Both: `class_exists()` passed, the method was absent from the bundled copy. + *Feature gating* (standard + thorough): - New user-facing features (admin pages, blocks, endpoints) should be gated behind feature flags for staged rollout. Flag if ungated. @@ -254,6 +275,8 @@ grep -r "@automattic/jetpack-" projects/*/*/package.json Flag changes that could break consumers (changed signatures, removed methods, altered return types). List affected downstream projects. +**Upstream direction too:** if the diff *adds* a call into a monorepo package this project does NOT `require`, that call is subject to version skew on standalone installs — apply the *Cross-package version skew* check from step 4. + ### 6. Test and static analysis (thorough only) Identify the test runner for each project and **always report it**, even when skipped: @@ -366,6 +389,7 @@ Review depth: **** ( lines, projects) ### Feature Gating ### Data / Privacy ### Backward Compatibility / Removed Public API +### Cross-Package Version Skew ### Cross-Project Impact ### Test Results ### Test Coverage Gaps