chore(ci): add edited trigger to check-sprint and auto-add workflows#57
chore(ci): add edited trigger to check-sprint and auto-add workflows#57lml2468 wants to merge 1 commit into
Conversation
lml2468
left a comment
There was a problem hiding this comment.
[COMMENT] Correct and safe β adding edited trigger to metadata-only workflows.
β
Both workflows use pull_request_target without checking out PR code β no security concern with the edited trigger.
β
actionlint passes β trigger syntax is valid.
β
Logic is sound: re-running sprint check and auto-add when PR description is edited enables the issue-first workflow (adding Closes #N after initial open).
π¬ CI notes (not blocking):
check-sprintfails because Sprint is not set on the project board (unrelated to code).welcomefails due to the permissions bug fixed in PR #55 (pending merge).
lml2468). Needs another maintainer's approval.
yujiawei
left a comment
There was a problem hiding this comment.
Code Review β PR #57 (octo-admin)
1. Verdict Summary
This PR adds the edited event type to pull_request_target triggers in two CI workflows so that the Issue-first Sprint check re-evaluates when a developer fills in Closes #<issue> after opening a PR. The change is small (2 additions / 2 deletions), well-scoped, and addresses a real gap in the existing trigger set.
| Item | Status | Note |
|---|---|---|
| Correctness β solves stated problem | β | edited fires on PR title/body changes, which is exactly when a Closes #N reference would be added post-open |
Security under pull_request_target |
β | Top-level permissions: {}; no PR-head checkout; delegates to org reusable workflow with explicit secret |
| Branch / scope filters preserved | β | branches: [main] in check-sprint.yml retained; only base-branch=main PRs are affected |
| YAML syntax / formatting | β | Valid; only the types: array expanded |
| Doc consistency with new trigger | The KNOWN LIMITATION comment in check-sprint.yml still lists (opened, synchronize, reopened) as the trigger set; now stale post-merge |
2. Findings
P0 / P1 β none
No correctness, security, or data-loss issues identified.
P2 β Stale documentation comment in check-sprint.yml
.github/workflows/check-sprint.yml:9 contains a multi-line KNOWN LIMITATION block that explicitly enumerates the old trigger set:
This check re-evaluates only on pull_request_target events (opened, synchronize, reopened).
After this PR, the trigger list at line 36 is [opened, synchronize, reopened, edited], so the comment is no longer accurate. Worth updating in the same PR (or a quick follow-up) to mention edited and to note that PR-body edits (e.g. adding Closes #N) now also re-trigger the check. Not a merge blocker β purely a documentation drift nit.
Suggested edit (illustrative):
# This check re-evaluates on pull_request_target events
# (opened, synchronize, reopened, edited). The `edited` trigger
# covers post-open PR description changes such as adding a
# `Closes #<issue>` reference.Maintainability / suggestion (non-blocking)
auto-add-to-project.ymladdseditedalongsideopened. The PR description focuses on theCloses #Nflow for the sprint check; for the auto-add workflow,editedtypically only matters if the reusable workflow does additional work beyond initial project membership (e.g. re-syncing fields based on body content). If it just adds-to-project once and is a no-op on subsequent calls, the trigger is harmless but adds noise. A one-liner in the PR description clarifying the intent forauto-add-to-projectwould be helpful for future readers.
3. Notes
pull_request_targetsemantics are correct here: it runs with base-branch workflow definitions and base-branch secrets, no PR-fork code is checked out, so adding event types does not widen the attack surface.- Increased fire frequency from
editedis bounded β PR title/body edits are infrequent compared to pushes β and the downstream reusable workflows are short. - Head SHA verified:
72b094df253e4604e4928009fa8c6eba2cd4634e. Files at this SHA match the diff posted on the PR.
4. Recommendation
Approving. The doc-comment drift in check-sprint.yml is worth fixing but does not need to block merge; it can be done in this PR or a trivial follow-up.
Jerry-Xin
left a comment
There was a problem hiding this comment.
Summary: This PR is relevant to the repository and the trigger change correctly supports re-running the sprint/project automation when a PR body is edited.
π¬ Non-blocking
- π΅ Suggestion: .github/workflows/check-sprint.yml:8 The header comment still says the workflow re-evaluates only on
opened, synchronize, reopened; it should includeeditedto stay accurate after this change.
β Highlights
- π΅ The PR is in scope: it updates repo-owned CI automation for PR/project workflow behavior.
- π΅ The
pull_request_targetusage remains consistent with the existing security posture: no PR code is checked out or executed in these wrapper workflows. - π΅ No tests are necessary for this small workflow trigger-only change.
This PR adds the
editedevent to thepull_request_target.typestrigger in bothcheck-sprint.ymlandauto-add-to-project.yml, enabling the Issue-first Sprint check flow to work correctly when a developer adds aCloses #<issue>reference to a PR description after opening.