ci: fix registry notification workflow (PLT-846) - #331
Conversation
Remove the unreachable push path and update repository dispatch so registry notifications only follow successful releases. Co-authored-by: Cursor <cursoragent@cursor.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #331 +/- ##
=======================================
Coverage 83.82% 83.82%
=======================================
Files 64 64
Lines 4390 4390
=======================================
Hits 3680 3680
Misses 710 710
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Clean, well-scoped CI hygiene change to .github/workflows/notify.yml: the removed push trigger was provably unreachable, the if simplification is correct, the v1→v4 action bump is input-compatible, and contents: write is the right minimum permission. No blockers; the main open question is whether the dispatch this workflow sends actually reaches a listener.
Findings: 0 blocking | 6 non-blocking | 2 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Verified the removed
pushtrigger was genuinely dead: the oldif: github.event_name == 'workflow_run' && ...guard meant push-triggered runs always skipped the job, so this is behavior-preserving cleanup, not a lost code path.workflows: ["Release"]also matches the actualname: Releaseinrelease.yml. - Sibling workflows touched by #327 (
checks.yml,release.yml,uci-stale-check.yml) all declare aconcurrencygroup;notify.ymlstill does not. With a 90s sleep plus a dispatch, overlapping Release runs could stack duplicate notifications. Low impact, but adding one would finish the PLT-846 hygiene pass consistently. - The Cursor second-opinion review file (
cursor-review.md) was empty — that pass produced no output. Codex reported no material issues, which matches my read. - No changeset needed here: this is CI-only and touches no published
@sei-js/*package (REVIEW_GUIDELINES.md §3). - 2 suggestion(s)/nit(s) flagged inline on specific lines.
|
|
||
| - name: Update registry API | ||
| uses: peter-evans/repository-dispatch@v1 | ||
| uses: peter-evans/repository-dispatch@v4 |
There was a problem hiding this comment.
[suggestion] The v1→v4 bump itself is safe — token, event-type, and client-payload kept the same names across those majors, and this moves off the deprecated Node runtime.
Worth confirming while you're in here, though, whether this step actually notifies anything today. Two things stack up:
- No
repositoryinput is set, so the dispatch defaults tosei-protocol/sei-jsitself — and nothing in.github/workflows/subscribes torepository_dispatch(types: [workflow_completed]). - GitHub does not start workflow runs from
repository_dispatchevents created withsecrets.GITHUB_TOKEN(line 25). Reaching an external repo would need a PAT anyway, sinceGITHUB_TOKENis scoped to this repo.
Both predate this PR, so not a blocker on the diff. But given the PR title is "fix registry notification workflow," it seems worth checking that the registry API is really receiving these — otherwise the fix is polishing a no-op.
| - 'packages/registry/**' | ||
|
|
||
| permissions: | ||
| contents: write |
There was a problem hiding this comment.
[nit] contents: write is the correct minimum for the repository-dispatch REST endpoint — good call declaring it explicitly. Since there's only one job, you could scope it under jobs.notify.permissions instead, so any future job added to this file starts from the default read-only token rather than inheriting write.
Summary
peter-evans/repository-dispatchfrom v1 to v4, declare its required permission, and remove the unused checkoutTest plan
go run github.com/rhysd/actionlint/cmd/actionlint@latest .github/workflows/*.ymlgit diff --checkMade with Cursor