Skip to content

feat: add check-sprint-on-merge workflow#86

Merged
lml2468 merged 2 commits into
mainfrom
feat/check-sprint
May 25, 2026
Merged

feat: add check-sprint-on-merge workflow#86
lml2468 merged 2 commits into
mainfrom
feat/check-sprint

Conversation

@lml2468
Copy link
Copy Markdown
Contributor

@lml2468 lml2468 commented May 23, 2026

Summary

Adds a caller workflow that invokes the org-level reusable check-sprint workflow on every PR to main.

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.

Note for deployer: Enable the required status check check-sprint / check-sprint on the main branch protection after this PR is merged.

@lml2468 lml2468 requested a review from a team as a code owner May 23, 2026 13:23
@github-actions github-actions Bot added the size/S PR size: S label May 23, 2026
Copy link
Copy Markdown
Contributor Author

@lml2468 lml2468 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[COMMENT] (Cannot self-approve — gh account matches PR author)

Verdict: LGTM

Clean, minimal caller workflow — well structured.

What was reviewed

Single new file: .github/workflows/check-sprint.yml (+24 lines)
A caller workflow that invokes the org-level reusable check-sprint workflow to block merges when the PR Sprint field is unset or mismatched.

Security ✅

  • pull_request_target used correctly — runs in base-branch context, no PR code checkout.
  • permissions: {} — drops all default GITHUB_TOKEN scopes; API access is scoped to PROJECT_TOKEN PAT only.
  • Reusable workflow pinned to @main on the org .github repo — acceptable trust boundary for an internal org.

Correctness ✅

  • Trigger events (opened, synchronize, reopened) cover all relevant PR lifecycle moments.
  • Branch filter (branches: [main]) correctly scopes to the merge target.
  • Inputs (pr-number, repo-name) and secret (PROJECT_TOKEN) properly forwarded.

Sequencing note ℹ️

The referenced reusable workflow (.github #42) is still open. Merging this PR first is harmless (the check will simply not find the reusable workflow and fail-open or error), but the branch protection rule (check-sprint / check-sprint) should only be enabled after both PRs are merged — as noted in the PR description.

No blocking issues found.

Copy link
Copy Markdown

@Jerry-Xin Jerry-Xin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Project relevance gate passed; the workflow is in scope for this deployment repository, but it currently depends on a reusable workflow that is not available on the referenced branch.

🔴 Blocking

  • 🔴 Critical: .github/workflows/check-sprint.yml calls Mininglamp-OSS/.github/.github/workflows/reusable-check-sprint.yml@main, but that reusable workflow is not present on Mininglamp-OSS/.github@main at review time. It appears to exist only on the referenced external PR branch. If this PR merges before Mininglamp-OSS/.github#42, this required check will fail before the sprint validation runs, and branch protection would block all PRs once enabled. Merge the reusable workflow first, or point this caller at a ref that already contains it.

💬 Non-blocking

  • 🟡 Warning: .github/workflows/check-sprint.yml only runs on opened, synchronize, and reopened. Changes to the Project Sprint field, or a sprint rollover after a previous successful run, will not automatically re-evaluate the status. That means the required check can become stale unless maintainers manually rerun it. If “current at merge time” is a hard guarantee, consider adding an explicit rerun mechanism or companion automation.

✅ Highlights

  • The use of pull_request_target is consistent with existing metadata-only workflows in this repo.
  • permissions: {} and avoiding checkout/execution of PR code are appropriate for passing PROJECT_TOKEN to a trusted reusable workflow.

yujiawei
yujiawei previously approved these changes May 23, 2026
Copy link
Copy Markdown
Contributor

@yujiawei yujiawei left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review — PR #86 (octo-deployment)

Verdict

APPROVED — no P0/P1 blockers. A few P2 nits below; none of them prevent merge.

Scope

A single new file .github/workflows/check-sprint.yml (+24 / -0) that adds a caller workflow delegating to the org-level reusable Mininglamp-OSS/.github/.github/workflows/reusable-check-sprint.yml@main.

What I checked

Item Result Evidence
Trigger choice is appropriate (pull_request_target) The reusable workflow only performs read-only GraphQL queries against the GitHub Projects API and requires PROJECT_TOKEN. No PR ref is checked out or executed. This matches the long-standing repo pattern in .github/workflows/auto-add-to-project.yml and the security note in the reusable workflow itself.
GITHUB_TOKEN scope is minimal permissions: {} at the workflow level (check-sprint.yml:14). All authenticated API work is done by PROJECT_TOKEN, a dedicated PAT.
Secret is passed correctly to the reusable workflow secrets: PROJECT_TOKEN: ${{ secrets.PROJECT_TOKEN }} (check-sprint.yml:22-23). Matches the workflow_call contract declared in the reusable workflow.
Inputs align with reusable contract pr-number (number) and repo-name (string) match the inputs: block in the reusable workflow. ${{ github.event.pull_request.number }} and ${{ github.repository }} are correct under pull_request_target.
Trigger types and branch filter [opened, synchronize, reopened] covers the standard re-evaluation lifecycle; branches: [main] scopes correctly to the protected branch.
Will not regress existing CI The yamllint job currently failing on this PR was already failing on main (HEAD commit bad46740), caused by pre-existing Helm {{ ... }} templates and labeler.yml blank lines — not by this new file. actionlint, kubeconform, add-to-project, notify, label, welcome all pass.

Findings

P2 — Add zizmor suppression comment for consistency with repo convention

File: .github/workflows/check-sprint.yml:11

The sibling workflow uses pull_request_target with an inline suppression directive:

# auto-add-to-project.yml:13
pull_request_target: # zizmor: ignore[dangerous-triggers] metadata-only automation; no PR code executed

The new workflow does not carry the same marker. The repo's workflow-sanity check happens to pass (it's the org's actionlint-based job, not zizmor), so this is not blocking today, but adding the same comment keeps the convention consistent and pre-empts a regression if zizmor is wired into sanity later.

Suggested change:

on:
  pull_request_target: # zizmor: ignore[dangerous-triggers] metadata-only check; no PR code executed
    types: [opened, synchronize, reopened]
    branches: [main]

P2 — Operational sequencing: caller will fail if reusable workflow isn't on .github@main first

The PR pins the reusable workflow via @main. The companion PR Mininglamp-OSS/.github#42 that introduces reusable-check-sprint.yml is still OPEN at the time of this review (head 97a384a, not merged). If this PR merges before that one, every subsequent PR to main will fail with "workflow not found", which the new branch protection (once enabled) would then block on.

Recommendation: explicitly merge Mininglamp-OSS/.github#42 first, then this PR, then flip on the required status check as the PR body already notes. Worth calling out in the PR description so the deployer follows the right order.

P2 — @main vs SHA pin

uses: Mininglamp-OSS/.github/.github/workflows/reusable-check-sprint.yml@main is consistent with auto-add-to-project.yml's existing pattern, and the trust boundary is the same org. Acceptable as-is. For higher assurance you could pin to a commit SHA (e.g. @97a384a…) and bump it deliberately, but that's a broader convention change, not a blocker for this PR.

P2 — Consider ready_for_review in types:

If the team ever opens PRs as drafts, the check will not re-fire when the PR is marked ready for review (it would only re-evaluate on the next synchronize). Adding ready_for_review to types: is a small UX improvement and harmless otherwise.

Security review

  • pull_request_target is the correct trigger here because the workflow needs PROJECT_TOKEN (a secret unavailable in fork pull_request contexts) and never touches PR-supplied code. The well-known pwn-request shape (pull_request_target + actions/checkout of the PR ref + script execution) is not present.
  • permissions: {} zeroes out GITHUB_TOKEN, so even if the reusable workflow were compromised the blast radius is bounded by the PROJECT_TOKEN PAT's scopes (read:project, read:org per the reusable workflow's doc comment). Good defense-in-depth.
  • No untrusted inputs are interpolated into shell or run: steps in this caller. The reusable workflow does interpolate REPO_OWNER / REPO_SHORT into a GraphQL string literal — that's a finding for the .github PR review, not this one, but worth being aware of.

Summary

The caller workflow is minimal, correct, follows the established repo convention for pull_request_target-based reusable callers, and does not change any application code. Merge after .github#42 lands.

Copy link
Copy Markdown
Contributor Author

@lml2468 lml2468 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review — 📖 齐静春

Verdict: Clean, minimal caller workflow — but has a hard dependency that must be resolved before merge.

🔴 Blocking (agrees with @Jerry-Xin)

  1. Reusable workflow does not exist on main yet (.github repo)
    • Verified: gh api repos/Mininglamp-OSS/.github/contents/.github/workflows/reusable-check-sprint.yml404
    • Source is in .github#42 (state: OPEN, not merged)
    • If this caller merges first, every PR against main will trigger a failed Check Sprint run. If it's a required status check, all PRs blocked.
    • Fix: merge .github#42 first, then this PR.

💬 Non-blocking

  • Trigger types (opened/synchronize/reopened) won't re-check when the Sprint field is updated post-creation or on sprint rotation. Consider adding edited or a workflow_dispatch escape hatch.

✅ Highlights

  • pull_request_target + permissions: {} + no checkout = correct security posture.
  • Clean caller/reusable separation across repos.

(Cannot submit as CHANGES_REQUESTED — gh account matches PR author)

Copy link
Copy Markdown

@Jerry-Xin Jerry-Xin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adds a relevant repository governance check for PRs into main; I found no blocking bugs or security issues in the caller workflow.

🔴 Blocking: None.

💬 Non-blocking

🟡 Warning — .github/workflows/check-sprint.yml: The workflow only re-evaluates on opened, synchronize, and reopened. The comments cover stale green results, but stale red results are also likely: for example, a draft PR can fail before its Sprint field is set, then become ready without a new run because ready_for_review is not included. Consider adding ready_for_review if draft PRs are part of the normal flow.

🔵 Suggestion — .github/workflows/check-sprint.yml: Existing pull_request_target workflows such as auto-add-to-project.yml, labeler.yml, and pr-contributor-welcome.yml document the trusted metadata-only pattern inline with a zizmor suppression. Adding the same inline annotation here would keep workflow security documentation consistent.

✅ Highlights

The PR belongs in this repository because it extends PR merge policy for octo-deployment.

The workflow follows the repository’s reusable-workflow caller pattern, uses minimal top-level permissions, passes only metadata inputs, and does not check out or execute PR code in the caller.

Copy link
Copy Markdown
Contributor

@yujiawei yujiawei left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review — PR #86 (octo-deployment)

Verdict

APPROVED — no P0/P1 blockers. The previous blocking concern is resolved on this new head SHA. A couple of P2 nits noted; none prevent merge.

What changed since the prior review round

The new head 546b5053 updates the file from +24 to +49 lines. Two material deltas:

  1. A 30-line header comment explaining the pull_request_target rationale and the stale-required-status limitation, plus a recommended branch-protection mitigation ("Require branches to be up to date before merging").
  2. Adds enforce-freshness: true to the inputs forwarded to the reusable workflow.

Both directly address the non-blocking concern raised previously about sprint-rollover / Project-field-edit staleness.

Verified

Item Result Evidence
Reusable workflow exists on Mininglamp-OSS/.github@main .github#42 MERGED at 2026-05-24T16:06:44Z (merge commit 985d81f1). gh api repos/Mininglamp-OSS/.github/contents/.github/workflows/reusable-check-sprint.yml returns 200 (sha 20215840). This invalidates the prior P0 blocker.
enforce-freshness input is supported by the reusable workflow on main Reusable workflow declares enforce-freshness: { type: boolean, default: false } in its workflow_call.inputs. The caller's enforce-freshness: true is a valid input.
Other inputs/secrets contract pr-number (number), repo-name (string), PROJECT_TOKEN (secret) all match the reusable workflow's declared contract. ${{ github.event.pull_request.number }} and ${{ github.repository }} are correct under pull_request_target.
Trigger choice / scope pull_request_target is the correct trigger because the workflow needs PROJECT_TOKEN (a secret unavailable under fork pull_request) and never checks out or executes PR code. types: [opened, synchronize, reopened] and branches: [main] scope correctly.
GITHUB_TOKEN scope is minimal permissions: {} at the workflow level. All API work goes through the PROJECT_TOKEN PAT (read:project + read:org).
No regression in existing CI The failing yamllint check on this PR is pre-existing on main (Helm {{ ... }} templates produce 1:3 syntax error: expected the node content, but found '-' and labeler.yml blank-line warnings — none of the failures cite .github/workflows/check-sprint.yml). actionlint, kubeconform, label, sanity all pass.

Findings

P2 — Add zizmor suppression comment for repo-convention consistency

File: .github/workflows/check-sprint.yml:37

The sibling caller in this repo carries an inline suppression marker:

# .github/workflows/auto-add-to-project.yml
pull_request_target: # zizmor: ignore[dangerous-triggers] metadata-only automation; no PR code executed

The new file does not. Sanity (actionlint) currently passes without it, so this is not blocking, but adding the same comment keeps the convention consistent and pre-empts a future regression if zizmor is wired into sanity.

Suggested change:

on:
  pull_request_target: # zizmor: ignore[dangerous-triggers] metadata-only check; no PR code executed
    types: [opened, synchronize, reopened]
    branches: [main]

P2 — Consider ready_for_review in types:

If the team ever opens PRs as drafts, this check will not re-fire when a draft is marked ready for review (it would only re-evaluate on the next synchronize). Adding ready_for_review is a small UX improvement and harmless otherwise.

P2 — @main vs SHA pin (acceptable as-is)

uses: Mininglamp-OSS/.github/.github/workflows/reusable-check-sprint.yml@main is consistent with auto-add-to-project.yml's existing pattern, and the trust boundary is the same org. Pinning to a commit SHA would raise assurance but is a broader convention change, not a blocker for this PR.

Security review

  • pull_request_target is the correct trigger here because the workflow needs PROJECT_TOKEN (unavailable under fork pull_request) and never touches PR-supplied code. The classic pwn-request shape (pull_request_target + actions/checkout of the PR ref + script execution) is not present.
  • permissions: {} zeroes out GITHUB_TOKEN, bounding blast radius to the PROJECT_TOKEN PAT's documented scopes (read:project, read:org).
  • No untrusted inputs flow into shell or run: steps in this caller. The reusable workflow does interpolate REPO_OWNER / REPO_SHORT into a GraphQL string; that is a finding for the .github repo, not this one.

Operational note (not a code finding)

The PR body already calls this out, but worth repeating for the deployer:

  1. .github#42 is now merged on main, so this PR is safe to merge.
  2. After merge, enable the required status check check-sprint / check-sprint on main branch protection.
  3. Also enable "Require branches to be up to date before merging" to maximize the value of enforce-freshness: true against the documented residual stale-check risk.

Summary

The caller workflow is minimal, correct, addresses the prior blocking dependency (now merged) and the prior non-blocking staleness concern (enforce-freshness: true plus thorough documentation), follows the established repo convention for pull_request_target-based reusable callers, and does not change any application code. Ready to merge.

Copy link
Copy Markdown
Contributor Author

@lml2468 lml2468 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-review — PR #86 (546b505)

Incremental review based on changes since fe5951a.

Previous Blocking Issue — ✅ Resolved

P0: reusable-check-sprint.yml@main did not exist (.github#42 was still open).

.github#42 was merged on 2026-05-24. Verified the reusable workflow is now present on main and accepts the enforce-freshness input added in this commit.

Changes Since Last Review

  1. Expanded header documentation — clearly describes the two stale-check scenarios (sprint rollover + Projects UI field edit) and recommended mitigation ("Require branches to be up to date before merging"). Well-written, actionable.

  2. enforce-freshness: true — correctly wired to the reusable workflow's enforce-freshness boolean input (default: false). The reusable workflow implements this as a committed-date guard with appropriate caveats about GIT_COMMITTER_DATE spoofability. Good defense-in-depth choice for a deployment repo.

  3. Minor wording fixes — "doesn't" → "does not". Fine.

CI Note

yamllint is red, but the failures are from Helm template files (syntax error: expected the node content, but found '-'), not from check-sprint.yml. This is the pre-existing issue where CI runs yamllint with an inline config that doesn't exclude Helm templates — same problem Allen identified in PR #83. Not caused by this PR.

Verification

  • Confirmed Mininglamp-OSS/.github/.github/workflows/reusable-check-sprint.yml exists on main
  • Confirmed enforce-freshness input is defined as type: boolean, default: false in the reusable workflow ✅
  • pull_request_target + permissions: {} + no code checkout = correct security model ✅
  • PROJECT_TOKEN passed via secrets: (not with:) ✅

LGTM. All previous blocking issues resolved, new changes are well-documented improvements.

@lml2468 lml2468 merged commit 263ba64 into main May 25, 2026
5 of 6 checks passed
@lml2468 lml2468 deleted the feat/check-sprint branch May 25, 2026 01:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/S PR size: S

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants