fix: refresh published Discord member roles daily#94
Conversation
steipete
left a comment
There was a problem hiding this comment.
--with-members still does not guarantee a member crawl. refreshGuildMembers exits early through shouldRefreshMembers when the snapshot is fresh, or when an imported snapshot has rows but no marker. With the 24-hour freshness interval and GitHub schedule jitter, the daily job can therefore skip a day and publish stale roles without exercising the required failure path.
Please make RequireMembers bypass the freshness/import-seeding gate (or add an explicit force parameter at this ownership boundary), and add a regression test where a fresh marker exists but RequireMembers: true still calls GuildMembers. Full go test ./... passes on the current head; this is the remaining correctness blocker.
|
Codex review: needs maintainer review before merge. Reviewed June 19, 2026, 4:28 AM ET / 08:28 UTC. Summary Reproducibility: yes. from source inspection: current main's scheduled Discord publisher always uses Review metrics: 3 noteworthy metrics.
Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Risk before merge
Maintainer options:
Next step before merge
Security Review detailsBest possible solution: Land after a maintainer clears the requested-changes state and confirms the production backup bot can perform the daily required member crawl while the 15-minute path stays message-only. Do we have a high-confidence way to reproduce the issue? Yes, from source inspection: current main's scheduled Discord publisher always uses Is this the best way to solve the issue? Yes, the latest PR head is a narrow maintainable implementation of the requested direction: it adds an explicit CLI mode, forces required member refreshes past the freshness gate, and keeps the frequent publisher path message-only. The remaining question is maintainer acceptance of the fail-closed daily workflow behavior. AGENTS.md: not found in the target repository. Codex review notes: model internal, reasoning high; reviewed against e6f719b7fa9b. Label changesLabel justifications:
Evidence reviewedWhat I checked:
Likely related people:
What the crustacean ranks mean
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics. How this review workflow works
|
05e7887 to
76f258a
Compare
|
Rebased onto current Only remaining gate: a real required-member sync needs Discord bot credentials, and |
steipete
left a comment
There was a problem hiding this comment.
Live gate passed on the exact PR head: bot authentication and guild access succeeded; sync --with-members --latest-only --no-update forced a member refresh despite the existing freshness marker, returned a nonzero member set, advanced the member sync timestamp, and completed. Full tests, autoreview, and exact-head CI are green.
|
Landed as Live proof on the exact PR head: bot authentication and guild access passed; a required member sync bypassed the prior freshness marker, returned a nonzero member set, advanced the member-refresh state, and completed latest-message sync. Full tests, autoreview, and exact-head CI passed. Thanks @hannesrudolph. |
Summary
sync --with-membersto opt into member crawling during a latest-only syncValidation
go test ./internal/syncer ./internal/cligit diff --check