Fix Dependabot workflow guard and add missing test coverage#325
Fix Dependabot workflow guard and add missing test coverage#325umair-ably merged 1 commit intomainfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughThis PR fixes a crash in the Changes
Review Notes
|
There was a problem hiding this comment.
Review
This PR fixes a real crash in the fix-dependabot workflow (using gh pr view / gh run list before checkout). The overall approach is correct — using event payload values and gh api instead of git-dependent CLI calls. Most of the changes are straightforward improvements, but there's one bug.
Bug: Removing set -o pipefail breaks test failure detection
The PR removes set -o pipefail on the grounds that || failed=1 already handles exit codes. This reasoning is incorrect — the two interact, and removing pipefail silently breaks test failure detection.
The pipeline:
pnpm test:unit 2>&1 | tee /tmp/test-output.txt || failed=1In bash, the exit status of a pipeline is the exit status of the last command (tee) unless pipefail is set. tee exits 0 as long as the file write succeeds — regardless of whether pnpm test:unit failed. So without pipefail, failed stays 0 even when tests fail.
With pipefail (the original), the pipeline fails if pnpm test:unit fails, || failed=1 fires, and the fix step is triggered correctly.
Impact: After this change, if pnpm test:unit or pnpm --filter @ably/react-web-cli test returns a nonzero exit code, failed will remain 0. The workflow will reach the conclusion that no fix is needed and quietly do nothing, even though tests are broken. This defeats the entire purpose of the test step.
Fix: keep set -o pipefail, or capture test exit codes explicitly:
pnpm test:unit 2>&1 | tee /tmp/test-output.txt; [[ ${PIPESTATUS[0]} -ne 0 ]] && failed=1Other observations (no action needed)
app/dependabot→dependabot[bot]: The old string never matched anything (Dependabot's GitHub login isdependabot[bot]), so the non-Dependabot guard was always silently broken. The new string is correct.gh apifor run count: Usingtotal_countfrom the paginated API response is correct — it counts all matching runs, not just the first page, which is actually an improvement over the previousgh run list(which defaulted to the last 20 runs).- Event payload injection: Using
${{ github.event.pull_request.user.login }}and${{ github.event.pull_request.head.ref }}as env vars before checkout is the right pattern. actions: readpermission: Required for thegh apiworkflows endpoint — good catch.- Timeout bump: Reasonable given the build+test+Claude workload.
- Fix guard step: use event payload (github.event.pull_request.user.login) instead of gh pr view which fails before checkout (no git repo) - Fix loop guard: use gh api instead of gh run list (also needs no repo) - Add actions:read permission for workflow runs API query - Add react-web-cli tests to catch web CLI package failures - Remove dead set -o pipefail from test step (counter pattern handles it) - Increase timeout to 30min to accommodate Claude's 30 turns - Include web CLI test command in Claude's verification instructions
6981dcf to
fd18df6
Compare
fixed... asked for re-review
Summary
Fixes multiple issues causing the Dependabot auto-fix workflow to fail silently.
Fixes
direct_promptis not a valid input forclaude-code-action@v1. Changed toprompt, which triggers automation mode (no@claudemention needed)gh pr viewandgh run listneed a git repo but run before checkout. Replaced withgithub.event.pull_request.user.login(event payload) andgh api(no repo needed)pnpm test:unitonly runs root tests, notpackages/react-web-cli. Addedpnpm --filter @ably/react-web-cli testactions: readfor the workflow runs API querypnpm --filter @ably/react-web-cli testin verification instructionsTest plan
@dependabot recreateon fix(deps): bump the all-dependencies group in /packages/react-web-cli with 9 updates #326 to test the full flow