feat: add check-sprint-on-merge workflow#48
Conversation
yujiawei
left a comment
There was a problem hiding this comment.
Code Review — PR #48 (octo-admin)
Summary
Adds a single workflow file .github/workflows/check-sprint.yml (24 lines) that calls the org-level reusable workflow Mininglamp-OSS/.github/.github/workflows/reusable-check-sprint.yml@main on every PR to main. The workflow blocks merges when the PR's Sprint field on the Octo Board is missing or doesn't match the current sprint iteration.
Verdict
No P0/P1 blockers. The change is small, focused, and follows the security-aware pattern recommended for pull_request_target callers. Approving with a few non-blocking suggestions.
What's correct
pull_request_targetis the right trigger. Secrets (PROJECT_TOKEN) are needed for the Projects GraphQL query, and forks need to be supported. Usingpull_request_targetis the documented pattern.- No PR code is executed. The caller never checks out the head ref and the reusable workflow only performs a read-only GraphQL query, so the well-known
pull_request_targetprivilege-escalation risk does not apply here. permissions: {}at the caller level is correct. The defaultGITHUB_TOKENcarries zero scopes; all authenticated calls go through the explicitPROJECT_TOKENPAT. This matches the security model documented in the reusable workflow.- Trigger types are appropriate.
[opened, synchronize, reopened]covers the cases the gate needs to enforce (initial PR open, push of new commits, reopening a closed PR).closedwould be wasteful;editedwould needlessly re-run when only the description is touched. - Branch filter limits blast radius.
branches: [main]ensures the gate only fires for PRs targeting main, not internal feature-branch PRs. - Inputs and secrets match the reusable workflow signature. Verified against PR #42 in
Mininglamp-OSS/.github:pr-number(number),repo-name(string), and thePROJECT_TOKENsecret all line up.
Suggestions (non-blocking)
S1 — Coordinate merge order with Mininglamp-OSS/.github PR #42
The reusable workflow at Mininglamp-OSS/.github/.github/workflows/reusable-check-sprint.yml@main does not yet exist on main — its PR (#42) is still open. If this PR (#48) is merged before #42, every subsequent PR to main will trigger a workflow that fails to resolve its uses: reference until #42 lands.
Practical impact is small because the deployer note already says the required status check should only be enabled after #48 merges, and presumably #42 merges first in the rollout sequence. Worth confirming explicitly:
- Recommend merging
Mininglamp-OSS/.github#42first, then this PR, then enabling the required status check.
S2 — Consider pinning the reusable workflow to a commit SHA
uses: Mininglamp-OSS/.github/.github/workflows/reusable-check-sprint.yml@main resolves the reusable workflow at run time from the main branch. For an internal, trusted org-level repo this is acceptable, but pinning to a commit SHA (with a comment naming the version) would:
- Prevent silent behavior changes when the reusable workflow is updated.
- Make rollbacks of the gate logic decoupled from the caller repo.
This is a maintainability preference, not a security fix in this case (since .github is a trusted internal repo). Feel free to keep @main if the team prefers fast propagation.
S3 — Optional: add a concurrency block
A long-lived PR can accumulate many synchronize events. A simple concurrency group would cancel in-flight runs when a new push arrives:
concurrency:
group: check-sprint-${{ github.event.pull_request.number }}
cancel-in-progress: trueThis is purely a cost/latency optimization and not required.
S4 — Confirm "deployer" note tracking
The PR body includes a note to enable the required status check check-sprint / check-sprint on the main branch protection after merge. Worth ensuring there's a tracking issue or runbook entry so this doesn't get forgotten — without that follow-up, the workflow will run but won't actually gate merges.
Out of scope (acknowledged)
- The substantive logic (sprint-matching, GraphQL query, error messaging) lives in the reusable workflow in PR #42 and should be reviewed there, not here.
- Branch-protection configuration is intentionally a manual step post-merge.
Jerry-Xin
left a comment
There was a problem hiding this comment.
This PR is relevant to octo-admin, but the workflow currently depends on a reusable workflow that is not available on the referenced @main ref.
🔴 Blocking
- 🔴 Critical: .github/workflows/check-sprint.yml:19 calls
Mininglamp-OSS/.github/.github/workflows/reusable-check-sprint.yml@main, but at review time the referenced reusable workflow is still only inMininglamp-OSS/.github#42and is not present onMininglamp-OSS/.githubmain. GitHub reusable workflow calls resolve the workflow file from the specified{owner}/{repo}/.github/workflows/{file}@{ref}, so this caller will fail to load until the upstream workflow is merged tomain. Merge the reusable workflow first, or otherwise ensure the referenced ref exists before this PR lands. (docs.github.com)
💬 Non-blocking
- 🟡 Warning: .github/workflows/check-sprint.yml:12 only runs on
opened,synchronize, andreopened. That is probably sufficient for required status checks, but PRs retargeted tomainviaeditedwould not get an immediate actionable run; they would likely be blocked by a missing required status instead. Consider addingeditedif you want clearer feedback.
✅ Highlights
- Uses
permissions: {}and passes only PR metadata plusPROJECT_TOKEN. - The
pull_request_targetusage matches existing metadata-only workflow patterns in this repository and does not check out PR code.
lml2468
left a comment
There was a problem hiding this comment.
📖 Review — No blocking issues found
(Cannot submit APPROVE — same GitHub account as PR author. Findings below.)
✅ pull_request_target correctly chosen — needs secrets for Projects API, no PR code checked out (safe pattern)
✅ permissions: {} — least privilege, actual permissions delegated to the reusable workflow
✅ Inputs (github.event.pull_request.number, github.repository) are safe contexts — no injection risk
✅ Secret passing via secrets: block is correct
🔵 Minor: reusable workflow pinned to @main rather than SHA — acceptable for org-internal .github repo, worth noting for future hardening.
LGTM from architecture perspective.
lml2468
left a comment
There was a problem hiding this comment.
Re-review — 87f1e5349dc8
Previous round context: Allen flagged that the reusable workflow only existed in PR #42 of .github repo, not on main.
Resolution check
✅ Blocking issue resolved: reusable-check-sprint.yml now exists on main of Mininglamp-OSS/.github. Verified via API — the workflow will resolve at runtime.
New changes in this commit
- Thorough KNOWN LIMITATION documentation — two stale-check scenarios (sprint rollover, field edit via Projects UI) are explicitly called out with partial/recommended mitigations.
enforce-freshness: true— passes the freshness input to the reusable workflow, which rejects PRs whose head commit predates the current sprint start date. This addresses the sprint-rollover stale-check scenario.- Minor copy edits in the header comments.
Assessment
| Aspect | Verdict |
|---|---|
| Security | ✅ permissions: {}, no PR code checkout, secrets scoped to PROJECT_TOKEN only |
| Correctness | ✅ Matches the reusable workflow's documented usage example exactly |
| Org consistency | ✅ @main pinning acceptable for internal org-level policy workflows |
Known gap (no edited trigger) |
ℹ️ Non-blocking — explicitly documented as a limitation with no automated mitigation possible (GitHub Projects has no webhook events for field changes). Branch-protection "up to date" setting is the recommended mitigation. |
Clean, well-documented, secure. Ship it.
Jerry-Xin
left a comment
There was a problem hiding this comment.
The workflow is relevant to octo-admin, but its trigger set leaves a stale-pass path for draft PRs that can bypass the intended sprint gate.
🔴 Blocking
🔴 Critical — Missing ready_for_review can allow stale sprint approval
File: .github/workflows/check-sprint.yml:35-37
The workflow only reruns on opened, synchronize, and reopened. If a draft PR is opened during sprint A, passes this check, remains draft through a sprint rollover, and is later marked ready for review without a new commit, this workflow will not rerun. Since draft PRs cannot be merged while draft, ready_for_review is the first point where the PR becomes mergeable, and the stale green check may still satisfy branch protection.
Add ready_for_review to the trigger list:
types: [opened, synchronize, reopened, ready_for_review]💬 Non-blocking
🟡 Warning — The limitation comment discusses stale green results, but not the draft-to-ready case
File: .github/workflows/check-sprint.yml:8-27
After adding ready_for_review, consider documenting that transition explicitly because it is a common lifecycle event for PRs that may span sprint boundaries.
✅ Highlights
The PR passes the project relevance gate: this is repository-level merge policy for octo-admin.
The workflow follows existing repository patterns for org-level reusable workflows, uses permissions: {}, avoids checking out PR code under pull_request_target, and passes the expected inputs/secrets to the reusable check-sprint workflow.
yujiawei
left a comment
There was a problem hiding this comment.
Code Review — PR #48 (octo-admin)
Summary
Re-review at head 87f1e53. The current diff is a single new file .github/workflows/check-sprint.yml (49 lines) that calls the org-level reusable workflow Mininglamp-OSS/.github/.github/workflows/reusable-check-sprint.yml@main on every PR to main. The merge will be blocked when the PR's Sprint field on the Octo Board is missing or doesn't match the current sprint iteration.
Since the previous round of review, two things changed:
- The upstream reusable workflow PR (
Mininglamp-OSS/.github#42) merged on 2026-05-24 (commit985d81f1). The@mainref now resolves — the prior blocker is no longer a blocker. - A new commit
87f1e53added a thoroughKNOWN LIMITATIONdocumentation block and enabledenforce-freshness: true, which is the documented partial mitigation for sprint-rollover staleness.
Verdict
No P0/P1 issues. APPROVED with one optional follow-up.
Verification (✅ / ⚠️ / ❌ + evidence)
- ✅ Upstream
uses:ref resolves.gh api repos/Mininglamp-OSS/.github/contents/.github/workflows/reusable-check-sprint.yml?ref=mainreturns SHA202158405fca…(size 17023). The reusable workflow'sworkflow_callsignature (pr-number: number,repo-name: string,enforce-freshness: boolean, secretPROJECT_TOKEN) matches what the caller passes at.github/workflows/check-sprint.yml:43-48. - ✅
pull_request_targetusage is safe here..github/workflows/check-sprint.yml:36declarespull_request_targetandpermissions: {}at line 40 strips the defaultGITHUB_TOKENto zero scopes. Noactions/checkoutof the PR head occurs in the caller, and the reusable workflow only performs read-only GraphQL queries against the Projects API usingPROJECT_TOKEN. This matches the documented security model in the reusable workflow header. - ✅ Trigger types and branch filter are correct.
types: [opened, synchronize, reopened](line 37) covers the events that need to (re-)evaluate the gate;branches: [main](line 38) limits firing to PRs targetingmain, which matches the gating intent. - ✅
enforce-freshness: trueis wired correctly (line 46). Per the reusable workflow's documented contract, this rejects PRs whose head commit predates the current sprint start — the partial mitigation for the sprint-rollover staleness scenario described in the file's header comment. - ✅ Status check name will be
check-sprint / check-sprintas documented. The caller's job keycheck-sprint(line 42) plus the reusable's job namecheck-sprintproduces the expected branch-protection identifier the deployer note references. - ✅ The documentation block at lines 1-29 is accurate. Both stale-check scenarios (sprint rollover, Projects-UI field edit) are real GitHub limitations with no first-class webhook fix, and the recommended "Require branches to be up to date before merging" mitigation is the conventional workaround.
Findings
None at P0/P1.
Suggestions (non-blocking)
S1 — Optional: pin the reusable workflow to a SHA instead of @main
.github/workflows/check-sprint.yml:43 uses @main. For a trusted internal org-level repo this is acceptable, but pinning to a commit SHA (e.g. @985d81f1c56e31d115c6f669175e9f3e5b02faf5) plus a comment naming the version would:
- Prevent silent behavior changes when the reusable workflow is updated.
- Make rollbacks of the gate logic decoupled from this caller repo.
- Avoid a class of supply-chain risk if the trust boundary of
.githubever changes.
Recommend either keeping @main for fast propagation (current choice, fine), or moving to a SHA-pin convention org-wide. Either is defensible; flagging it as a deliberate decision rather than a fix.
S2 — Optional: confirm the deployer-note follow-up has an owner
The PR body asks the deployer to enable the required status check check-sprint / check-sprint on main branch protection after merge. Without that follow-up, the workflow will run on every PR but won't actually gate merges. Worth ensuring there's a tracking issue or runbook line item so it doesn't slip.
Out of scope (acknowledged)
- The substantive sprint-matching / GraphQL / freshness-enforcement logic lives in the reusable workflow and was reviewed there in
Mininglamp-OSS/.github#42. - Branch-protection configuration is intentionally a manual post-merge step.
Jerry-Xin
left a comment
There was a problem hiding this comment.
Blocking withdrawn after reading full diff.
enforce-freshness: true + the detailed KNOWN LIMITATION documentation adequately cover the stale-pass scenario I raised. The authors clearly considered draft-to-ready and sprint-rollover edge cases and provided layered mitigations (freshness check + recommended branch protection setting).
Adding ready_for_review to triggers would still be a nice improvement, but the current approach is a reasonable engineering tradeoff — not a blocker.
Previous concerns resolved ✅
lml2468
left a comment
There was a problem hiding this comment.
[COMMENT]
Clean, well-structured caller workflow. A few observations:
What looks good:
pull_request_targetis the correct trigger here since the workflow needsPROJECT_TOKENfor the Projects API, and importantly no PR code is checked out or executed — only metadata (pr-number,repo-name) is passed to the reusable workflow. This avoids the typicalpull_request_targetcode-injection risk.permissions: {}drops all default permissions at the caller level — good practice; the reusable workflow declares its own.enforce-freshness: trueis a solid mitigation for the sprint-rollover staleness scenario.- The header comment block is excellent — clearly documents known limitations and recommended branch protection settings.
Minor note (non-blocking):
- The reusable workflow is pinned to
@mainrather than a SHA or tag. This is fine for an internal org-level workflow where the org controls the.githubrepo, but worth keeping in mind if the org ever opens.githubto external contributors.
LGTM. Remember to enable the check-sprint / check-sprint required status check on main branch protection (as noted in the PR body).
Note: Cannot submit as APPROVE since the PR author (lml2468) is also the gh CLI authenticated user.
Summary
Adds a caller workflow that invokes the org-level reusable
check-sprintworkflow on every PR tomain.Merges will be blocked if the PR's Sprint field on the Octo Board is not set to the current sprint iteration.
See Mininglamp-OSS/.github #42 for the reusable workflow implementation.