chore(ci): add edited trigger to check-sprint and auto-add workflows#49
chore(ci): add edited trigger to check-sprint and auto-add workflows#49lml2468 wants to merge 1 commit into
Conversation
lml2468
left a comment
There was a problem hiding this comment.
[APPROVE]
Verdict: APPROVED ✅ (submitted as COMMENT — cannot approve own PR)
Clean workflow trigger update. Two files, +3 -2.
Analysis:
-
editedtrigger is correct — when a developer addsCloses #<issue>to the PR body after opening, the sprint check and auto-add-to-project workflows need to re-run.pull_request_targetwitheditedfires on title/body/base-branch changes, which covers this case. -
Security: no concern — both workflows only pass metadata (
github.event.pull_request.number,github.repository) to reusable workflows. No PR code checkout, sopull_request_target+editedis safe. -
enforce-freshness: trueincluded — subsumes PR #48 (now closed). No merge conflict risk. -
Idempotency — sprint check and project-add are idempotent; re-triggering on description edits is harmless.
LGTM 🚢
yujiawei
left a comment
There was a problem hiding this comment.
Code Review — PR #49 (octo-lib)
1. Verification Summary
| Item | Status | Evidence |
|---|---|---|
edited trigger added to check-sprint.yml |
✅ | .github/workflows/check-sprint.yml:12 |
edited trigger added to auto-add-to-project.yml |
✅ | .github/workflows/auto-add-to-project.yml:8 |
enforce-freshness: true is a valid input on the upstream reusable workflow |
✅ | Mininglamp-OSS/.github/.github/workflows/reusable-check-sprint.yml declares enforce-freshness (boolean, default false) under workflow_call.inputs |
| Matches documented usage from upstream reusable workflow | ✅ | Upstream comment block explicitly recommends including edited in pull_request_target.types so the check re-runs when developers add Closes #<issue> after opening |
| YAML syntax valid | ✅ | Only adds an enum value to an existing list and one new input key under with: |
2. Findings
No P0 / P1 issues found.
P2 / Nits (non-blocking)
editedfires on more than description changes. Theeditedactivity type fires for any PR metadata change — title, body, base branch, etc. Both workflows will therefore run on title-only edits. Theauto-add-to-projectreusable is idempotent so this is safe but slightly wasteful. The check-sprint reusable does a read-only GraphQL query and exits cheaply, so the extra runs are inexpensive. Worth being aware of, not worth changing.- Scope creep —
enforce-freshness: trueis bundled with the trigger change. The PR title and description only mention theeditedtrigger; theenforce-freshness: trueflip incheck-sprint.yml:23is a separate behavioral change (it now fails the check when the PR head commit predates the current sprint start date). It's a one-line, well-aligned change so I'm not blocking on it, but the PR description should ideally call it out so future archaeology doesn't need to read the diff to discover the new merge gate. Consider amending the PR body or splitting if the project values strict commit hygiene. enforce-freshnessis a one-way ratchet on long-lived branches. Onceenforce-freshness: trueis in effect, any PR whose head commit predates the current sprint start will fail and need a fresh push to recover. This is the intended behavior per the upstream docstring, and it's the right call for an issue-first sprint flow, but it's worth communicating to contributors so a stale-branch failure doesn't read as a false positive.
3. Security Review
pull_request_targetis the right trigger here: the workflow needssecrets.PROJECT_TOKENand must run on PRs from forks. Addingediteddoes not change the security posture because:permissions: {}is preserved on the caller workflows (check-sprint.yml:15,auto-add-to-project.yml:10) —GITHUB_TOKENcarries zero scopes.- No PR code is checked out or executed in either reusable; the only network calls are read-only GraphQL queries against the Projects API.
- No new attack surface from
edited. A malicious PR author cannot leverage description edits to gain code execution because no PR-controlled content is interpolated into a shell context —inputs.pr-numberis typednumberupstream and the body is never read.
4. Suggestions
- (Optional) Update the PR description to mention
enforce-freshness: trueso the merge gate change is discoverable. - (Optional, follow-up) Consider whether
auto-add-to-projectactually benefits fromedited— the project add is idempotent on first call, so this only matters if the upstream reusable does something Sprint-related on edit. If not, theeditedaddition there is dead weight.
5. Verdict
APPROVED. Three lines of YAML, all well-motivated, matches documented upstream usage, no security or correctness concerns. The bundled enforce-freshness: true is a separate concern but is correct on its own merits and the file is small enough that calling it out in the PR body is sufficient.
Jerry-Xin
left a comment
There was a problem hiding this comment.
The PR is in scope for Mininglamp-OSS/octo-lib because it updates repository CI/project automation, and the changes are ready to merge.
💬 Non-blocking
- 🔵 Suggestion:
.github/workflows/check-sprint.yml:23addsenforce-freshness: true, which is a broader behavior change than the PR description emphasizes. It is supported by the reusable workflow and appears intentional, but the PR description could mention that stale head commits may now fail until the contributor pushes or rebases.
✅ Highlights
.github/workflows/check-sprint.yml:12correctly addsedited, allowing the sprint check to re-run when a PR description gains aCloses #<issue>reference..github/workflows/auto-add-to-project.yml:8matches the same trigger behavior so project automation can re-run after PR body edits.- No checkout or PR code execution is introduced under
pull_request_target, so the existing security model remains intact.
Verification: git diff --check passed. actionlint was not installed in the environment.
## Summary Add two new caller workflows per the PR notification split design: - `octo-pr-review-feed.yml` — reviewer action needed (opened/ready_for_review/review_requested/synchronize) → this repo's project group - `octo-pr-result-notify.yml` — status broadcast + review results (closed/reopened/approved/changes_requested) → global pr-feed group Reusable workflows live in `.github` repo (PR #49). The old `octo-pr-feed.yml` is kept — cleanup is a separate step. --------- Co-authored-by: Octo Bot <bot@mininglamp.com>
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.