Clear WordPress Plugin Check findings on the release ZIP (PRO-1196) - #31
Conversation
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
WordPress core has auto-loaded a plugin's translations by Text Domain
header match since 4.6 (_load_textdomain_just_in_time()), regardless
of wp.org hosting - manual load_plugin_textdomain() is redundant and
flagged by the official Plugin Check tool (plugin_repo category,
DiscouragedFunctions.load_plugin_textdomainFound). Safe here because
boardscribe.php's Text Domain header ("boardscribe") already matches
the plugin's slug/directory. Found running PRO-1196's Plugin Check
against the release ZIP.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Plugin Check flags a shipped vendor/ with no accompanying composer.json as missing_composer_json_file (plugin_repo category). The file has nothing sensitive to withhold - just the PHP version requirement, PSR-4 autoload map, and require-dev/scripts entries - so shipping it is free. composer.lock stays excluded; it only pins exact dev-tooling versions, which gives a site admin nothing actionable. Updated both plugins' recipes/manifests for consistency since rule 4 is shared, global guidance across the two. Found running PRO-1196's Plugin Check against the release ZIP. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
PRO-1196 Run WordPress Plugin Check against the packaged release
ProblemThe official Plugin Check has not yet been run against the exact release ZIP. Local PHPCS and CI do not cover every WordPress.org repository-specific check. Acceptance criteria
|
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
WalkthroughPackaging instructions document the Plugin Check warning for ChangesPlugin Packaging
Translation Loading
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
Reverts the previous commit's fix for missing_composer_json_file. Decided not to ship composer.json/composer.lock for a warning-level, non-blocking Plugin Check finding rather than add dev-tooling config to the release artifact. Documented as an explicitly accepted warning in the packaging skill so it isn't rediscovered and 're-fixed' by a future Plugin Check run. Re-verified: rebuilt zip still shows the warning (expected) with 0 errors, and the load_plugin_textdomain fix from the prior commit is unaffected. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.claude/skills/package-plugins/SKILL.md (1)
27-33: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winMake both packaging scripts fail closed before cleanup.
Neither block uses
set -e, so a failedcomposer install,cp, orzipcommand can be ignored while the script continues. More seriously, ifmktemp -dfails,STAGEcan become/boardscribeor/boardscribe-pro, causingrm -rf "$(dirname "$STAGE")"to resolve torm -rf /.Add
set -euo pipefail, validate the temporary directory, and use a guarded cleanup trap before performing recursive deletion.🛡️ Proposed hardening
+set -euo pipefail + -STAGE=$(mktemp -d)/boardscribe && mkdir -p "$STAGE/assets" +TMP_DIR=$(mktemp -d) +STAGE="$TMP_DIR/boardscribe" +mkdir -p "$STAGE/assets" +trap 'rm -rf -- "$TMP_DIR"' EXITApply the equivalent change to the Pro block.
Also applies to: 50-57
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.claude/skills/package-plugins/SKILL.md around lines 27 - 33, Harden both packaging script blocks by enabling set -euo pipefail, validating that mktemp -d successfully produces a nonempty temporary directory before assigning STAGE, and installing a guarded cleanup trap before any recursive deletion. Ensure cleanup only removes the expected temporary staging directory, then preserve the existing composer install, copy, zip, and cleanup flow in both the standard and Pro blocks.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In @.claude/skills/package-plugins/SKILL.md:
- Around line 27-33: Harden both packaging script blocks by enabling set -euo
pipefail, validating that mktemp -d successfully produces a nonempty temporary
directory before assigning STAGE, and installing a guarded cleanup trap before
any recursive deletion. Ensure cleanup only removes the expected temporary
staging directory, then preserve the existing composer install, copy, zip, and
cleanup flow in both the standard and Pro blocks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0197de26-1c3a-40d5-b0f8-d7f4f86a74bb
📒 Files selected for processing (2)
.claude/skills/package-plugins/SKILL.mdincludes/Plugin.php
…m -rf / Addresses CodeRabbit's Critical finding on #31. Neither recipe used set -e, so a failed composer install/cp/zip could be silently ignored while the script kept going and packaged a broken zip. Worse: if mktemp -d itself failed, STAGE=$(mktemp -d)/boardscribe degraded to the literal string /boardscribe with no error surfaced (bash doesn't propagate the failure through string concatenation), making the old cleanup line rm -rf "$(dirname "$STAGE")" resolve to rm -rf /. Fixed by separating the bare TMP_DIR=$(mktemp -d) assignment (which set -e DOES reliably catch, unlike the concatenated form - verified empirically) from the STAGE path, adding set -euo pipefail to both blocks, and replacing the manual rm -rf with a trap that only ever targets the one known $TMP_DIR path. Verified: ran both the free-plugin recipe end to end (correct zip, temp dir actually cleaned up post-exit, dev tooling restored) and a forced-failure case (bad cp path aborts immediately, never reaches later steps, trap still fires) and a forced mktemp-failure case (confirms set -e now catches it, where before it silently would not). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Fixed in 7f30fed — addresses CodeRabbit's Critical finding (posted outside the diff range, so replying here instead of inline). Confirmed the failure mode empirically before fixing: a bare |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.claude/skills/package-plugins/SKILL.md:
- Around line 24-37: Update both packaging recipes in the skill documentation so
zip writes the archive to a path under TMP_DIR, not directly under dist. After
zip succeeds, move the completed archive into dist; preserve the existing
cleanup and final artifact names while ensuring failed zip commands cannot leave
partial release archives in dist.
- Around line 24-38: The packaging script’s trailing composer install is skipped
when staging or archiving fails, leaving development dependencies uninstalled.
Update the packaging flow around the existing TMP_DIR EXIT trap and composer
install commands so that, after a successful composer install --no-dev, an
EXIT/finally cleanup path always runs composer install to restore dev tooling
while preserving the original command’s exit status.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0a9a7741-abfd-482b-bc07-8b1db65147a3
📒 Files selected for processing (1)
.claude/skills/package-plugins/SKILL.md
|
Following up now that this is merged (CodeRabbit's review didn't finish reviewing the last two pushes in time, so noting here for the record — it'll help future reviews on this repo learn the pattern): What changed and why, since your Critical finding on Fixed exactly as you suggested, in commit
Verified with three real test runs (not just reading the diff): the normal path still produces a correct zip and cleans up; a forced One related finding from the same PR that's since been reverted, for context if you re-review this file later: an earlier commit ( |
1 participant
What & why
Ran the official WP.org Plugin Check (v2.0.0) against
dist/boardscribe.zipon a clean, isolated WordPress 7.0.2 install, per PRO-1196.Before: 2 warnings, 0 errors, both in the
plugin_repocategory.general,security,performance, andaccessibilitywere already clean.After: 1 warning, 0 errors — one finding fixed, one deliberately accepted (see below).
Findings
missing_composer_json_file— the zip shipsvendor/(Composer's own autoloader machinery) but notcomposer.json. Accepted as-is, not fixed — decided not to ship dev-tooling config into the release artifact for a warning-level, non-blocking finding. Documented in the packaging skill (.claude/skills/package-plugins/SKILL.md) so it isn't rediscovered and "re-fixed" by a future Plugin Check run.DiscouragedFunctions.load_plugin_textdomainFound— fixed.Plugin::boot()calledload_plugin_textdomain( 'boardscribe', ... )manually. WordPress core has auto-loaded a plugin's translations by Text Domain header match since 4.6 (_load_textdomain_just_in_time()), regardless of wp.org hosting — the manual call is redundant. Safe here becauseboardscribe.php'sText Domain: boardscribeheader already matches the plugin's slug/directory exactly.Verification
--categories=general|plugin_repo|security|performance|accessibility) before any changes — confirms both original findings were exactly and only inplugin_repo.dist/boardscribe.zipwith the textdomain fix (composer.json still excluded), reinstalled it fresh in the same isolated environment, reran Plugin Check: themissing_composer_json_filewarning is present (expected/accepted),load_plugin_textdomainFoundis gone, 0 errors.es_ES, confirmed no fatals.npm run build(all three bundles),npm run lint:js,composer check-cs, PHPUnit 90/90 all clean.🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Documentation
vendor/is included withoutcomposer.json, WP.org Plugin Check will markmissing_composer_json_fileas a warning (not an error), while continuing to excludecomposer.json/composer.lockfrom release zips.