diff --git a/.agents/skills/jetpack-review-pr.md b/.agents/skills/jetpack-review-pr.md index e56fe6a6fee..fe70d323007 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