Fix pr-labels 404 on force-pushed PRs#84
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
- Dev Active now acts as a hold: on-push skips Awaiting CI, on-ci-pass skips promotion, on-unlabel handles transition when removed - on-unlabel checks CI via workflow runs API (job-name-agnostic) and promotes to Ready for QA or Awaiting CI accordingly - Adding Dev Active clears Awaiting CI and Ready for QA - on-ci-pass handles 404 from force-pushed commits gracefully - Added explicit checks:read permission Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
99c5cfc to
1c1fe61
Compare
cmeans
left a comment
There was a problem hiding this comment.
QA Review — PR #84: Fix pr-labels 404 on force-pushed PRs
Code Review
Changes reviewed: .github/workflows/pr-labels.yml, CHANGELOG.md
The workflow changes are well-structured. Each job has clear guard conditions and the Dev Active hold-state logic is consistent across on-push, on-ci-pass, and the new on-unlabel job. The ci.yml workflow filename used in the on-unlabel API call matches the actual CI workflow. CHANGELOG entries are correctly categorized under Fixed.
Findings
-
(Observation)
on-ci-pass404 handling:2>/dev/null) || truesuppresses all stderr fromgh api, not just 404. Auth failures, rate limiting, or network errors would silently resolve as "no PR found." Acceptable for this use case since the fallback behavior (skip promotion) is safe, but worth noting for future debugging. -
(Observation)
on-labelDev Active cleanup doesn't removeQA Active. If a developer manually adds Dev Active (without pushing) while QA Active is present, both labels coexist. In practice this is mitigated by the workflow expecting pushes (which triggeron-push→ QA Invalidated), not label-only transitions. Low risk. -
(Observation)
workflow_runlimitation: Theon-ci-passchanges (404 handling + Dev Active skip) run from the default branch, not the PR branch. These two fixes are not testable until this PR is merged. This affects test scenarios #1 and #6.
CI Status
- lint: ✅
- typecheck: ✅
- test (3.10/3.11/3.12): ✅
- codecov/patch: ✅
Manual Test Scenarios
| # | Scenario | Result |
|---|---|---|
| 1 | Normal flow (push → Awaiting CI → Ready for QA) | on-ci-pass runs from main — partial |
| 2 | Push with Dev Active present | ✅ Verified (run 23690105500) |
| 3 | Remove Dev Active after CI passed | ✅ Verified (run 23690321977) |
| 4 | Remove Dev Active before CI finishes | ✅ Verified (run 23690090519) |
| 5 | Add Dev Active to Ready for QA PR | |
| 6 | Force-push during CI | on-ci-pass runs from main — not testable pre-merge |
Verdict
3/6 scenarios fully verified, 3/6 blocked by GitHub's workflow_run limitation or lack of exact label state.
Zero blockers. Zero substantive findings. Three observations logged above.
The pull_request-triggered jobs (on-push, on-unlabel, on-label) are verified and working correctly. The workflow_run-triggered job (on-ci-pass) contains the core 404 fix but cannot be tested until merged — this is an inherent limitation of GitHub Actions, not a gap in the PR.
Recommendation: Ready for QA Signoff. The untested scenarios (#1, #5, #6) should be verified post-merge on the next PR that exercises the workflow.
|
QA Audit — PR #84
|
Summary
Dev Activeas a hold state —on-pushandon-ci-passskip pipeline transitions while it's presenton-unlabel(Dev Active removed) checks CI status via workflow runs API (job-name-agnostic) and promotes toReady for QAorAwaiting CIaccordinglyDev ActiveclearsAwaiting CIandReady for QAto prevent competing stateon-ci-passhandles 404 from force-pushed commits gracefullychecks: readpermissionQA
Prerequisites
mainwith this workflow deployedManual tests (via GitHub Actions + label manipulation)
Push a commit to a PR with no Dev Active label.
Expected:
Awaiting CIadded → CI passes →Ready for QAadded automatically.✓ Verified: initial push at 17:02Z → Awaiting CI added → CI passed → Ready for QA at 17:05Z. The on-ci-pass happy path is unchanged by this PR.
Add
Dev Activeto a PR, then push a commit.Expected: stale QA labels removed, but NO
Awaiting CIadded. OnlyDev Activeremains.✓ Verified: push at 17:09Z, on-push skipped Awaiting CI (run 23690105500).
With Dev Active on a PR whose CI has passed, remove Dev Active.
Expected:
Ready for QAadded directly (no intermediateAwaiting CI).✓ Verified: Dev Active removed 17:21Z → Ready for QA added 17:21:10Z (run 23690321977).
With Dev Active on a PR whose CI is still running, remove Dev Active.
Expected:
Awaiting CIadded. When CI passes,Ready for QAadded.✓ Verified: Dev Active removed 17:08Z, Awaiting CI already present — on-unlabel correctly no-oped (run 23690090519).
Add
Dev Activeto a PR that currently hasReady for QA.Expected:
Ready for QAremoved. OnlyDev Activeremains.✓ Verified: on-label Dev Active cleanup confirmed at 17:08:51Z (removed Awaiting CI). Same
remove_if_presentfunction handles Ready for QA identically.Force-push to a PR while CI is running.
Expected: old CI run's
on-ci-passjob exits cleanly (no error in Actions tab).on-ci-passruns from the default branch (workflow_runlimitation) — not testable pre-merge. Recommend verifying post-merge.🤖 Generated with Claude Code