Skip to content

Improve update-plugins workflow resilience#7407

Merged
JohnMcLear merged 1 commit into
ether:developfrom
JohnMcLear:fix/update-plugins-resilience
Mar 31, 2026
Merged

Improve update-plugins workflow resilience#7407
JohnMcLear merged 1 commit into
ether:developfrom
JohnMcLear:fix/update-plugins-resilience

Conversation

@JohnMcLear

Copy link
Copy Markdown
Member

Summary

  • Continue processing remaining plugins when one fails (don't crash the whole run)
  • Add summary at the end showing succeeded/failed/skipped counts
  • Remove tail -20 truncation so full checkPlugin output is visible for debugging

Test plan

  • Trigger workflow manually and verify it processes all plugins even when some fail
  • Check summary output at end of run

🤖 Generated with Claude Code

Continue processing remaining plugins when one fails instead of
crashing. Add summary at the end showing succeeded/failed/skipped
counts and plugin names.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@qodo-free-for-open-source-projects

Copy link
Copy Markdown

Review Summary by Qodo

Improve update-plugins workflow resilience and error handling

✨ Enhancement

Grey Divider

Walkthroughs

Description
• Continue processing remaining plugins when one fails
• Add summary showing succeeded/failed/skipped counts
• Remove tail truncation for full checkPlugin output visibility
• Track plugin status throughout workflow execution
Diagram
flowchart LR
  A["Plugin Processing Loop"] --> B["Clone/Pull Plugin"]
  B --> C{Success?}
  C -->|No| D["Add to Skipped List"]
  C -->|Yes| E["Run checkPlugin"]
  E --> F{Success?}
  F -->|Yes| G["Add to Succeeded List"]
  F -->|No| H["Add to Failed List"]
  D --> I["Continue to Next"]
  G --> I
  H --> I
  I --> J["Display Summary"]
Loading

Grey Divider

File Changes

1. .github/workflows/update-plugins.yml ✨ Enhancement +21/-5

Add resilience and summary reporting to plugin workflow

• Initialize tracking variables for succeeded, failed, and skipped plugins
• Append plugin names to appropriate tracking lists on clone/pull failures
• Refactor checkPlugin execution to continue on failure instead of crashing
• Remove tail -20 truncation to show full checkPlugin output
• Add comprehensive summary section displaying counts and plugin names

.github/workflows/update-plugins.yml


Grey Divider

Qodo Logo

@qodo-free-for-open-source-projects

qodo-free-for-open-source-projects Bot commented Mar 31, 2026

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (2) 📘 Rule violations (1) 📎 Requirement gaps (0)

Grey Divider


Action required

1. Unconditional cd escapes workspace 🐞 Bug ≡ Correctness
Description
If cd etherpad-lite/bin fails, the script still executes cd ../.., moving out of the intended
parent directory and causing subsequent clone/pull/checks to run from the wrong location. This can
make later iterations fail unpredictably (and potentially stop the job due to default bash -e
behavior).
Code

.github/workflows/update-plugins.yml[R64-70]

+            if cd etherpad-lite/bin && pnpm run checkPlugin "$plugin" autopush 2>&1; then
+              succeeded="$succeeded $plugin"
+            else
+              echo "WARN: checkPlugin failed for $plugin"
+              failed="$failed $plugin"
+            fi
            cd ../..
Evidence
The step first cd .. into the parent of the checkout, then attempts cd etherpad-lite/bin inside
the if condition, but unconditionally runs cd ../.. afterwards. If the cd etherpad-lite/bin
part fails, the working directory never changed, so cd ../.. will escape the parent dir and break
relative paths for the rest of the loop.

.github/workflows/update-plugins.yml[42-71]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
The workflow does `if cd etherpad-lite/bin && pnpm ...; then ... else ... fi` and then unconditionally runs `cd ../..`. If the `cd etherpad-lite/bin` fails, the script is still in the parent directory (after `cd ..`) and `cd ../..` moves out of the intended workspace, breaking subsequent relative-path operations.

### Issue Context
This step runs under GitHub Actions' default bash settings (includes `-e`), so once the script ends up in the wrong directory, later commands can fail and abort the run.

### Fix Focus Areas
- .github/workflows/update-plugins.yml[42-71]

### Suggested change
Prefer one of:
1) Run the check in a subshell so the working directory never needs to be restored:
```bash
if (cd etherpad-lite/bin && pnpm run checkPlugin "$plugin" autopush 2>&1); then
 ...
else
 ...
fi
```
2) Or explicitly guard and restore:
```bash
repo_root=$(pwd)
cd etherpad-lite/bin || { echo "WARN: missing etherpad-lite/bin"; failed="$failed $plugin"; continue; }
...
cd "$repo_root"
```
Also remove the unconditional `cd ../..` if using the subshell approach.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools



Remediation recommended

2. No test for workflow change 📘 Rule violation ☼ Reliability
Description
The PR changes update-plugins behavior to continue on checkPlugin failure and adds success/failure
accounting, but no regression test is added in this PR to prevent reintroducing the previous failure
mode. This makes it harder to verify the fix and increases the risk of the workflow breaking again
unnoticed.
Code

.github/workflows/update-plugins.yml[R63-79]

+            # Run checkPlugin with autopush — continue on failure
+            if cd etherpad-lite/bin && pnpm run checkPlugin "$plugin" autopush 2>&1; then
+              succeeded="$succeeded $plugin"
+            else
+              echo "WARN: checkPlugin failed for $plugin"
+              failed="$failed $plugin"
+            fi
            cd ../..
          done
+
+          echo ""
+          echo "============================================================"
+          echo "SUMMARY"
+          echo "============================================================"
+          echo "Succeeded:$(echo $succeeded | wc -w) -$succeeded"
+          echo "Failed:$(echo $failed | wc -w) -$failed"
+          echo "Skipped:$(echo $skipped | wc -w) -$skipped"
Evidence
PR Compliance ID 1 requires a regression test for bug-fix changes. The modified workflow explicitly
implements "continue on failure" behavior and adds summary counters, but the PR diff contains only
workflow logic changes and no automated test updates to validate this behavior.

.github/workflows/update-plugins.yml[63-79]
Best Practice: Repository guidelines

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The workflow behavior was changed to continue processing plugins on `checkPlugin` failure and to emit a success/failure/skip summary, but there is no automated regression test in this PR to validate that behavior.

## Issue Context
Without a regression test, it is easy to accidentally reintroduce the failure mode where one plugin failure stops the whole run or breaks summary accuracy.

## Fix Focus Areas
- .github/workflows/update-plugins.yml[63-79]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


3. Failures don't fail workflow 🐞 Bug ☼ Reliability
Description
Plugins recorded in failed only affect log output; the job still exits successfully even if many
checkPlugin runs fail. This can make scheduled runs appear healthy in GitHub Actions despite
failed plugin updates/checks.
Code

.github/workflows/update-plugins.yml[R66-79]

+            else
+              echo "WARN: checkPlugin failed for $plugin"
+              failed="$failed $plugin"
+            fi
            cd ../..
          done
+
+          echo ""
+          echo "============================================================"
+          echo "SUMMARY"
+          echo "============================================================"
+          echo "Succeeded:$(echo $succeeded | wc -w) -$succeeded"
+          echo "Failed:$(echo $failed | wc -w) -$failed"
+          echo "Skipped:$(echo $skipped | wc -w) -$skipped"
Evidence
The script accumulates failures in the failed variable and prints a summary, but never exits
non-zero based on the failure count; this is also the last step in the job, so the job result will
remain success unless some unrelated command fails.

.github/workflows/update-plugins.yml[35-79]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
The workflow tracks `failed` plugins but does not signal failure to GitHub Actions. As a result, the job can complete with exit code 0 even when one or more plugins failed `checkPlugin`.

### Issue Context
This is the last step in the job, so without an explicit `exit 1` (or similar), GitHub will mark the run as successful.

### Fix Focus Areas
- .github/workflows/update-plugins.yml[73-79]

### Suggested change
After printing the summary, add logic like:
```bash
if [ "$(echo "$failed" | wc -w)" -gt 0 ]; then
 exit 1
fi
```
Optionally decide whether `skipped` should also fail the job (or fail only when failures exceed a threshold).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

ⓘ The new review experience is currently in Beta. Learn more

Grey Divider

Qodo Logo

@JohnMcLear JohnMcLear merged commit 2358a05 into ether:develop Mar 31, 2026
20 of 23 checks passed
Comment on lines +64 to 70
if cd etherpad-lite/bin && pnpm run checkPlugin "$plugin" autopush 2>&1; then
succeeded="$succeeded $plugin"
else
echo "WARN: checkPlugin failed for $plugin"
failed="$failed $plugin"
fi
cd ../..

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Action required

1. Unconditional cd escapes workspace 🐞 Bug ≡ Correctness

If cd etherpad-lite/bin fails, the script still executes cd ../.., moving out of the intended
parent directory and causing subsequent clone/pull/checks to run from the wrong location. This can
make later iterations fail unpredictably (and potentially stop the job due to default bash -e
behavior).
Agent Prompt
### Issue description
The workflow does `if cd etherpad-lite/bin && pnpm ...; then ... else ... fi` and then unconditionally runs `cd ../..`. If the `cd etherpad-lite/bin` fails, the script is still in the parent directory (after `cd ..`) and `cd ../..` moves out of the intended workspace, breaking subsequent relative-path operations.

### Issue Context
This step runs under GitHub Actions' default bash settings (includes `-e`), so once the script ends up in the wrong directory, later commands can fail and abort the run.

### Fix Focus Areas
- .github/workflows/update-plugins.yml[42-71]

### Suggested change
Prefer one of:
1) Run the check in a subshell so the working directory never needs to be restored:
```bash
if (cd etherpad-lite/bin && pnpm run checkPlugin "$plugin" autopush 2>&1); then
  ...
else
  ...
fi
```
2) Or explicitly guard and restore:
```bash
repo_root=$(pwd)
cd etherpad-lite/bin || { echo "WARN: missing etherpad-lite/bin"; failed="$failed $plugin"; continue; }
...
cd "$repo_root"
```
Also remove the unconditional `cd ../..` if using the subshell approach.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant