feat(ci): PR test plugins ship a patch so they stack on shared files (OS-469)#2674
Conversation
The per-PR test plugin packaged changed files as whole-file copies and overwrote them on install, aborting if another webgui-pr-* plugin already managed any of the same files. Two PRs touching one file could not be tested together. Now the build emits a unified diff (pr.patch, system-relative paths) for text files plus whole copies of any binary files, and the plugin applies the patch with `patch -p1` (dry-run first; abort only on a real reject) and reverses it on removal. Non-overlapping edits to the same file from different PR plugins now stack; only truly overlapping hunks conflict. Binaries keep the whole-file replace with a per-binary conflict guard. Upload/R2 plumbing is unchanged (still one tarball). Closes OS-469 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
WalkthroughThe CI workflow's packaging step is rewritten to classify PR-changed files as text or binary, produce a unified ChangesPatch-based PR plugin build and lifecycle
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.github/scripts/generate-pr-plugin.sh (1)
177-177: 💤 Low valueConsider adding
--batchflag for consistency with revert operations.The patch revert (line 96 and line 284) uses
--batchto prevent interactive prompts, but the forward-apply here doesn't. While--forwardshould prevent most prompts, adding--batchwould ensure the install never hangs waiting for input if unexpected conditions arise.- patch -p1 -d / --forward < "$PAYLOAD/pr.patch" + patch -p1 -d / --forward --batch < "$PAYLOAD/pr.patch"🤖 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 @.github/scripts/generate-pr-plugin.sh at line 177, The patch command used to apply the forward patch is missing the `--batch` flag that is already used in the patch revert operations elsewhere in the script. Add the `--batch` flag to the patch command (the one with `-p1 -d / --forward` flags) to ensure consistency and prevent the script from hanging on unexpected interactive prompts, matching the pattern already established in the revert operations.
🤖 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.
Nitpick comments:
In @.github/scripts/generate-pr-plugin.sh:
- Line 177: The patch command used to apply the forward patch is missing the
`--batch` flag that is already used in the patch revert operations elsewhere in
the script. Add the `--batch` flag to the patch command (the one with `-p1 -d /
--forward` flags) to ensure consistency and prevent the script from hanging on
unexpected interactive prompts, matching the pattern already established in the
revert operations.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: e2be2f1e-f0c9-4009-a6c2-aad5835e35bd
📒 Files selected for processing (2)
.github/scripts/generate-pr-plugin.sh.github/workflows/pr-plugin-build.yml
…se-patch Reverse-applying a plugin's patch on removal can fail when another stacked PR plugin edited nearby lines (disturbed reverse context), leaving a file half-reverted. Instead, ship each text file's original (base) version in the payload and, on removal/update, restore those originals then re-apply the other still-installed PR plugins' patches. This is deterministic and independent of reverse-patch context, so removing one of several plugins that share a file cleanly drops only that plugin's changes. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Update — removal is now deterministic (rebuild from originals) (commit Addressed a real weakness in the first cut: removal used Now the payload also ships each text file's original (base) version (
so it cleanly drops only its own changes regardless of overlap/order — no dependence on reverse-patch context. Verified by simulation, including the hard case: two plugins editing adjacent lines of the same file. Old reverse-apply fails; the new rebuild removes one cleanly, keeps the other intact, and returns the file to stock when the last is removed. (Files live in the RAM rootfs, so a reboot is always the ultimate fallback — but removal no longer needs it.) |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.github/scripts/generate-pr-plugin.sh (1)
93-116: 💤 Low valueRestore-then-reapply strategy is sound.
The deterministic rollback approach (restore shipped originals, then re-apply other plugins' patches) correctly addresses the reverse-patch context problem described in PR objectives.
One observation: line 110 silently suppresses patch errors during re-application. This is reasonable since all patches must have passed dry-run during installation, but consider logging a warning (not to stderr, which is also suppressed) so operators can diagnose rare edge cases:
for other in /boot/config/plugins/webgui-pr-*; do [ -d "$other" ] || continue [ "$other" == "$PLUGIN_DIR" ] && continue - [ -f "$other/applied.patch" ] && patch -p1 -d / --forward < "$other/applied.patch" >/dev/null 2>&1 || true + if [ -f "$other/applied.patch" ]; then + if ! patch -p1 -d / --forward < "$other/applied.patch" >/dev/null 2>&1; then + echo "⚠️ Warning: failed to re-apply patch from $(basename "$other")" + fi + fi done🤖 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 @.github/scripts/generate-pr-plugin.sh around lines 93 - 116, In the loop that re-applies patches from other installed PR plugins (the `for other in /boot/config/plugins/webgui-pr-*` loop), the current patch command silently suppresses all errors using `>/dev/null 2>&1 || true`. To aid diagnostic capabilities, modify the patch re-application logic to capture when the patch fails to apply and log a warning message to a diagnostic log file (not stderr, which is also redirected to /dev/null), so operators can track which patches failed to re-apply in rare edge cases, while still maintaining the non-blocking behavior of the `|| true` continuation.
🤖 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.
Nitpick comments:
In @.github/scripts/generate-pr-plugin.sh:
- Around line 93-116: In the loop that re-applies patches from other installed
PR plugins (the `for other in /boot/config/plugins/webgui-pr-*` loop), the
current patch command silently suppresses all errors using `>/dev/null 2>&1 ||
true`. To aid diagnostic capabilities, modify the patch re-application logic to
capture when the patch fails to apply and log a warning message to a diagnostic
log file (not stderr, which is also redirected to /dev/null), so operators can
track which patches failed to re-apply in rare edge cases, while still
maintaining the non-blocking behavior of the `|| true` continuation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: a06908e0-43dc-447e-beff-64e29cbe4aa4
📒 Files selected for processing (2)
.github/scripts/generate-pr-plugin.sh.github/workflows/pr-plugin-build.yml
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/pr-plugin-build.yml
- Add --batch to all forward patch applies (dry-run, install, re-apply loops) for consistency with the reverse operations and to avoid any interactive hang on unexpected input. - Re-apply loop now surfaces a warning (stdout + logger) when another PR plugin's patch fails to re-apply during rollback, instead of silently suppressing it; behavior stays non-blocking. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Thanks @coderabbitai — both nitpicks addressed in
|
|
Tip For best results, initiate chat on the files or code changes.
🐇✨ |
Summary
Implements OS-469. The per-PR test plugin used to package changed files as whole-file copies, overwrite them on install, and abort if another
webgui-pr-*plugin already managed any of the same files — so two PRs touching one file (e.g. #2672 + #2673, both editingCreateDocker.php) couldn't be tested together.Now it ships a unified diff and
patch-applies it, so non-overlapping edits to the same file stack.How it works
pr-plugin-build.yml): for changed text files, stage base + head versions in system layout and emitpr.patch(diff -ruN, paths apply withpatch -p1at/). Changed binary files are copied whole intobinary/with abinary_files.txtlist. Both go in the same tarball.generate-pr-plugin.sh): on install,patch -p1 --dry-run --forwardfirst — abort with a clear message on a real reject (overlapping change), otherwise apply and saveapplied.patch. Binaries are whole-file replaced with a per-binary conflict guard. On remove (and before update), reverse the patch (patch -R) and restore binary backups.Validation (local simulation)
.plgis well-formed XML (all placeholders substituted);bash -n+ YAML parse clean.Notes / still to verify
.github/**, so it will itself build a PR test plugin with the new code — a live smoke test of the generator.patchis present in the Unraid base; uses--forward/--batchfor non-interactive apply with fuzz.🤖 Generated with Claude Code
Summary by CodeRabbit