Atomic zip writes + restore composer tooling on packaging failure (follow-up to #31) - #32
Atomic zip writes + restore composer tooling on packaging failure (follow-up to #31)#32pattonwebz wants to merge 5 commits into
Conversation
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Addresses two findings from CodeRabbit's second review pass on #31 (landed after that PR was already merged, so following up here): 1. zip -r wrote directly to dist/*.zip. A mid-write failure (disk full, permissions) could leave a truncated file sitting at the release path, indistinguishable from a good one without re-checking. Now zips into $TMP_DIR and mv's into dist/ only after zip succeeds. 2. My own set -e fix in the prior commit introduced a regression: the trailing 'composer install' (restoring dev tooling) only ran on the happy path, since set -e now aborts the script before reaching it if an earlier cp/zip step fails - silently leaving vendor/ in --no-dev mode (no phpcs/phpunit) for the rest of the session, which is exactly the failure mode rule 2 already warned about. Folded the restore into the same cleanup trap (guarded with '|| true' so a restore hiccup can't mask the script's real exit code, which the trap captures via 'local exit_code=$?' before doing anything else and re-raises at the end). Verified with three real runs, capturing the actual exit code (not just glancing at the diff): happy path still produces a correct 54-file zip with dev tooling restored; a forced zip failure now propagates a real nonzero exit code (12), leaves no partial zip in dist/, and still restores phpcs/phpunit despite the failure. 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 documentation now standardizes cleanup for Free and Pro plugin archives, restores Composer tooling on all exits, removes temporary files, and moves completed ZIPs into ChangesPlugin packaging instructions
Estimated code review effort: 1 (Trivial) | ~5 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 31-37: Move the cleanup trap setup to immediately after strict
mode, before composer install --no-dev and any filesystem operations. Initialize
TMP_DIR to an empty value first, and update cleanup() to remove the temporary
directory only when TMP_DIR is non-empty while preserving the exit code and
restoring dev tooling.
- Around line 17-19: Update the packaging workflow documentation after the ZIP
build to require running the official WordPress Plugin Check against the exact
packaged ZIP, reviewing plugin_repo warnings, documenting actionable findings,
and rebuilding and retesting as needed. Require retaining the final Plugin Check
output, while preserving the existing manifest verification step.
- Around line 34-35: Update both cleanup trap blocks in the package plugin
workflow so the rm -rf operation is explicitly non-fatal, ensuring cleanup
failures cannot interrupt execution before the saved exit status is returned.
Preserve each block’s existing exit "$exit_code" behavior and apply the same
guard consistently to both traps.
🪄 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: 63160d8f-310f-4cbd-b86a-76504de64c26
📒 Files selected for processing (1)
.claude/skills/package-plugins/SKILL.md
| 6. **Run each plugin's whole bash block as a single script**, not line-by-line — `$STAGE`/`$TMP_DIR` are shell variables/state that don't persist across separate command invocations, and the `mv` into `dist/` after the zip subshell relies on still being in the repo root. | ||
| 7. **Both blocks open with `set -euo pipefail` and a `cleanup` trap — don't strip these when copy-pasting.** Without them, a failed `composer install`/`cp`/`zip` can be silently ignored and the script continues packaging a broken zip; worse, if `mktemp -d` itself fails, `STAGE=$(mktemp -d)/boardscribe` degrades to the literal string `/boardscribe` with **no error surfaced** (bash doesn't propagate the failure through the concatenation), making a naive cleanup line `rm -rf "$(dirname "$STAGE")"` resolve to `rm -rf /`. The current form avoids this entirely: `TMP_DIR=$(mktemp -d)` is a bare assignment (so `set -e` reliably aborts if it fails, unlike the concatenated form), `STAGE` is built from `$TMP_DIR` afterward, and the `cleanup` function only ever removes that one known path — it can't be redirected by a failure elsewhere. (Found via CodeRabbit review on boardscribe#31.) | ||
| 8. **The `cleanup` trap does two things, in order, and both matter under `set -e`:** (1) restores dev tooling with a plain `composer install` — this must happen on **every** exit path, not just the happy one, because with `set -e` active a `cp`/`zip` failure now aborts the script *before* reaching a trailing `composer install` line, which would silently leave `vendor/` in `--no-dev` mode (no phpcs/phpunit) for the rest of the session; (2) removes `$TMP_DIR`. The restore step is guarded with `|| true` so a Composer hiccup during cleanup can't itself mask the script's real exit code, which the trap captures up front (`local exit_code=$?`) and re-raises at the end via `exit "$exit_code"`. **The zip is built inside `$TMP_DIR` and `mv`'d into `dist/` only after `zip` succeeds** — writing `zip -r` directly to the final `dist/*.zip` path means a mid-write failure (disk full, permissions) can leave a truncated file sitting at the release path, indistinguishable from a good one without re-checking. (Also found via CodeRabbit review on boardscribe#31.) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Document the required Plugin Check release gate.
PRO-1196 requires running the official WordPress Plugin Check against the exact packaged ZIP, reviewing plugin_repo warnings, documenting actionable findings, rebuilding/retesting, and retaining the final output. The current workflow only verifies the manifest with unzip -l, so add this explicit post-build step.
🧰 Tools
🪛 SkillSpector (2.3.11)
[warning] 15: [EA2] Autonomous Decision Making: Skill enables autonomous high-impact decisions without human-in-the-loop verification. Critical operations (destructive commands, financial transactions, data deletion) should require explicit user confirmation.
Remediation: Add human-in-the-loop confirmation for destructive, irreversible, or high-impact operations. Never auto-execute commands that modify files, send data, or alter system state.
(Excessive Agency (EA2))
[error] 18: [TM1] Tool Parameter Abuse: Tool parameters are crafted to achieve unintended or unsafe behavior. Parameter abuse can bypass intended safety checks (e.g. shell=True, --force, dangerous glob patterns).
Remediation: Validate all tool parameters against an allowlist. Reject dangerous parameter values (shell=True, --force, -rf /) and use safe defaults.
(Tool Misuse (TM1))
[error] 18: [TM1] Tool Parameter Abuse: Tool parameters are crafted to achieve unintended or unsafe behavior. Parameter abuse can bypass intended safety checks (e.g. shell=True, --force, dangerous glob patterns).
Remediation: Validate all tool parameters against an allowlist. Reject dangerous parameter values (shell=True, --force, -rf /) and use safe defaults.
(Tool Misuse (TM1))
[error] 28: [TM1] Tool Parameter Abuse: Tool parameters are crafted to achieve unintended or unsafe behavior. Parameter abuse can bypass intended safety checks (e.g. shell=True, --force, dangerous glob patterns).
Remediation: Validate all tool parameters against an allowlist. Reject dangerous parameter values (shell=True, --force, -rf /) and use safe defaults.
(Tool Misuse (TM1))
[error] 60: [TM1] Tool Parameter Abuse: Tool parameters are crafted to achieve unintended or unsafe behavior. Parameter abuse can bypass intended safety checks (e.g. shell=True, --force, dangerous glob patterns).
Remediation: Validate all tool parameters against an allowlist. Reject dangerous parameter values (shell=True, --force, -rf /) and use safe defaults.
(Tool Misuse (TM1))
[error] 28: [TM2] Chaining Abuse: Tool calls are chained to bypass individual safety checks or escalate capabilities beyond what any single tool call would allow.
Remediation: Limit tool chaining depth and validate the output of each tool before passing it to the next. Require explicit user approval for multi-step chains.
(Tool Misuse (TM2))
[error] 60: [TM2] Chaining Abuse: Tool calls are chained to bypass individual safety checks or escalate capabilities beyond what any single tool call would allow.
Remediation: Limit tool chaining depth and validate the output of each tool before passing it to the next. Require explicit user approval for multi-step chains.
(Tool Misuse (TM2))
🤖 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 17 - 19, Update the
packaging workflow documentation after the ZIP build to require running the
official WordPress Plugin Check against the exact packaged ZIP, reviewing
plugin_repo warnings, documenting actionable findings, and rebuilding and
retesting as needed. Require retaining the final Plugin Check output, while
preserving the existing manifest verification step.
| cleanup() { | ||
| local exit_code=$? | ||
| composer install || true # restore dev tooling (phpcs/phpunit/etc.) - even on failure | ||
| rm -rf -- "$TMP_DIR" | ||
| exit "$exit_code" | ||
| } | ||
| trap cleanup EXIT |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Install the EXIT trap before composer install --no-dev.
The trap is registered only after Line 27/Line 59 and the dist cleanup. If either composer install --no-dev or the preceding filesystem operation fails under set -e, the script exits without restoring Composer development tooling. Initialize TMP_DIR to an empty value and register the trap immediately after strict mode, guarding the empty path.
Proposed ordering
set -euo pipefail
+TMP_DIR=""
+cleanup() {
+ local exit_code=$?
+ composer install || true
+ if [[ -n "$TMP_DIR" ]]; then
+ rm -rf -- "$TMP_DIR"
+ fi
+ exit "$exit_code"
+}
+trap cleanup EXIT
+
npm run build
composer install --no-dev --optimize-autoloader
...
-TMP_DIR=$(mktemp -d)
+TMP_DIR=$(mktemp -d)
STAGE="$TMP_DIR/boardscribe"
-cleanup() {
-...
-}
-trap cleanup EXITAlso applies to: 63-69
🧰 Tools
🪛 SkillSpector (2.3.11)
[warning] 15: [EA2] Autonomous Decision Making: Skill enables autonomous high-impact decisions without human-in-the-loop verification. Critical operations (destructive commands, financial transactions, data deletion) should require explicit user confirmation.
Remediation: Add human-in-the-loop confirmation for destructive, irreversible, or high-impact operations. Never auto-execute commands that modify files, send data, or alter system state.
(Excessive Agency (EA2))
[error] 18: [TM1] Tool Parameter Abuse: Tool parameters are crafted to achieve unintended or unsafe behavior. Parameter abuse can bypass intended safety checks (e.g. shell=True, --force, dangerous glob patterns).
Remediation: Validate all tool parameters against an allowlist. Reject dangerous parameter values (shell=True, --force, -rf /) and use safe defaults.
(Tool Misuse (TM1))
[error] 18: [TM1] Tool Parameter Abuse: Tool parameters are crafted to achieve unintended or unsafe behavior. Parameter abuse can bypass intended safety checks (e.g. shell=True, --force, dangerous glob patterns).
Remediation: Validate all tool parameters against an allowlist. Reject dangerous parameter values (shell=True, --force, -rf /) and use safe defaults.
(Tool Misuse (TM1))
[error] 28: [TM1] Tool Parameter Abuse: Tool parameters are crafted to achieve unintended or unsafe behavior. Parameter abuse can bypass intended safety checks (e.g. shell=True, --force, dangerous glob patterns).
Remediation: Validate all tool parameters against an allowlist. Reject dangerous parameter values (shell=True, --force, -rf /) and use safe defaults.
(Tool Misuse (TM1))
[error] 60: [TM1] Tool Parameter Abuse: Tool parameters are crafted to achieve unintended or unsafe behavior. Parameter abuse can bypass intended safety checks (e.g. shell=True, --force, dangerous glob patterns).
Remediation: Validate all tool parameters against an allowlist. Reject dangerous parameter values (shell=True, --force, -rf /) and use safe defaults.
(Tool Misuse (TM1))
[error] 28: [TM2] Chaining Abuse: Tool calls are chained to bypass individual safety checks or escalate capabilities beyond what any single tool call would allow.
Remediation: Limit tool chaining depth and validate the output of each tool before passing it to the next. Require explicit user approval for multi-step chains.
(Tool Misuse (TM2))
[error] 60: [TM2] Chaining Abuse: Tool calls are chained to bypass individual safety checks or escalate capabilities beyond what any single tool call would allow.
Remediation: Limit tool chaining depth and validate the output of each tool before passing it to the next. Require explicit user approval for multi-step chains.
(Tool Misuse (TM2))
🤖 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 31 - 37, Move the
cleanup trap setup to immediately after strict mode, before composer install
--no-dev and any filesystem operations. Initialize TMP_DIR to an empty value
first, and update cleanup() to remove the temporary directory only when TMP_DIR
is non-empty while preserving the exit code and restoring dev tooling.
| rm -rf -- "$TMP_DIR" | ||
| exit "$exit_code" |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
sed -n '1,120p' .claude/skills/package-plugins/SKILL.md | cat -nRepository: equalizedigital/boardscribe
Length of output: 10366
🏁 Script executed:
python3 - <<'PY'
import os, tempfile, subprocess, textwrap, pathlib, json, sys
script = r'''#!/usr/bin/env bash
set -euo pipefail
tmp=$(mktemp -d)
mkdir "$tmp/dir"
chmod 500 "$tmp"
exit_code=42
cleanup() {
local exit_code=$?
echo "trap-start:$exit_code"
rm -rf -- "$tmp/dir"
echo "trap-after-rm"
exit "$exit_code"
}
trap cleanup EXIT
false
'''
p = subprocess.run(["bash", "-lc", script], capture_output=True, text=True)
print("rc:", p.returncode)
print("stdout:")
print(p.stdout)
print("stderr:")
print(p.stderr)
PYRepository: equalizedigital/boardscribe
Length of output: 265
Guard both cleanup traps so rm can’t hide the real exit code.
rm -rf runs under set -e; if it fails, the trap exits before exit "$exit_code", so the original status is lost. Make the removal non-fatal and then re-raise the saved code in both cleanup blocks.
🧰 Tools
🪛 SkillSpector (2.3.11)
[warning] 15: [EA2] Autonomous Decision Making: Skill enables autonomous high-impact decisions without human-in-the-loop verification. Critical operations (destructive commands, financial transactions, data deletion) should require explicit user confirmation.
Remediation: Add human-in-the-loop confirmation for destructive, irreversible, or high-impact operations. Never auto-execute commands that modify files, send data, or alter system state.
(Excessive Agency (EA2))
[error] 18: [TM1] Tool Parameter Abuse: Tool parameters are crafted to achieve unintended or unsafe behavior. Parameter abuse can bypass intended safety checks (e.g. shell=True, --force, dangerous glob patterns).
Remediation: Validate all tool parameters against an allowlist. Reject dangerous parameter values (shell=True, --force, -rf /) and use safe defaults.
(Tool Misuse (TM1))
[error] 18: [TM1] Tool Parameter Abuse: Tool parameters are crafted to achieve unintended or unsafe behavior. Parameter abuse can bypass intended safety checks (e.g. shell=True, --force, dangerous glob patterns).
Remediation: Validate all tool parameters against an allowlist. Reject dangerous parameter values (shell=True, --force, -rf /) and use safe defaults.
(Tool Misuse (TM1))
[error] 28: [TM1] Tool Parameter Abuse: Tool parameters are crafted to achieve unintended or unsafe behavior. Parameter abuse can bypass intended safety checks (e.g. shell=True, --force, dangerous glob patterns).
Remediation: Validate all tool parameters against an allowlist. Reject dangerous parameter values (shell=True, --force, -rf /) and use safe defaults.
(Tool Misuse (TM1))
[error] 60: [TM1] Tool Parameter Abuse: Tool parameters are crafted to achieve unintended or unsafe behavior. Parameter abuse can bypass intended safety checks (e.g. shell=True, --force, dangerous glob patterns).
Remediation: Validate all tool parameters against an allowlist. Reject dangerous parameter values (shell=True, --force, -rf /) and use safe defaults.
(Tool Misuse (TM1))
[error] 28: [TM2] Chaining Abuse: Tool calls are chained to bypass individual safety checks or escalate capabilities beyond what any single tool call would allow.
Remediation: Limit tool chaining depth and validate the output of each tool before passing it to the next. Require explicit user approval for multi-step chains.
(Tool Misuse (TM2))
[error] 60: [TM2] Chaining Abuse: Tool calls are chained to bypass individual safety checks or escalate capabilities beyond what any single tool call would allow.
Remediation: Limit tool chaining depth and validate the output of each tool before passing it to the next. Require explicit user approval for multi-step chains.
(Tool Misuse (TM2))
🤖 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 34 - 35, Update both
cleanup trap blocks in the package plugin workflow so the rm -rf operation is
explicitly non-fatal, ensuring cleanup failures cannot interrupt execution
before the saved exit status is returned. Preserve each block’s existing exit
"$exit_code" behavior and apply the same guard consistently to both traps.
1 participant
What & why
Follow-up to CodeRabbit's second review pass on #31, which landed after that PR was already merged (per instruction, ahead of the slow-that-day review). Two real findings on the
set -ehardening from #31 itself:dist/—zip -rwrote straight to the final release path. A mid-write failure (disk full, permissions) could leave a truncated file sitting atdist/*.zip, indistinguishable from a good one without re-checking.set -efix regressed the composer-restore guarantee — the trailingcomposer install(restoring dev tooling) only ran on the happy path. Withset -eactive, acp/zipfailure now aborts before reaching it, silently leavingvendor/in--no-devmode (no phpcs/phpunit) for the rest of the session — exactly the failure mode rule 2 already warned about, just newly reachable because of the hardening itself.Fix
$TMP_DIR,mv'd intodist/only afterzipsucceeds.|| trueso a restore hiccup can't mask the script's real exit code (captured vialocal exit_code=$?at the top of the trap, re-raised at the end viaexit "$exit_code").Verification
Ran three real script executions, capturing actual exit codes (not just reading the diff):
zipfailure (pointed at a nonexistent staged directory): script now propagates a real nonzero exit code (12), no partial zip lands indist/, and phpcs/phpunit are still restored despite the failure.dist/boardscribe.zipwith the corrected recipe, confirmed manifest (0composer.json— still excluded per the earlier PRO-1196 decision, 54 files).composer check-cs, PHPUnit 90/90 clean.🤖 Generated with Claude Code
Summary by CodeRabbit