diff --git a/README.md b/README.md index 1951973..45e3aca 100644 --- a/README.md +++ b/README.md @@ -26,3 +26,19 @@ ENV: <環境名> メンション対象の通知(mention 系 reason)とアラートは、チャンネル全体に通知する。 Slack では `@channel`(``)、Discord では `@everyone` を使うため、 ユーザー ID の設定は不要。 + +ただし PR の通知は、CI などの自動チェックが失敗中・実行中のあいだメンションしない。 +レビューできる状態になっていない PR でチャンネル全体を叩かないため。 +通知そのものは従来どおり届く。 + +## 投稿の分割 + +メンション対象の通知とそれ以外は、別々の投稿に分けて送る。 +同じ投稿に混ざると、`@channel` が付いていても、どれが自分宛てなのかは投稿を開くまで +分からないため。 + +分けるのは投稿の区切りだけで、並べ替えはしない。 +通知は更新時刻の昇順のまま届き、メンション対象が無ければ投稿は 1 つになる。 + +区切りの判定には reason だけを使う。 +CI の状態でメンションが抑止された PR も、メンション対象の投稿に入る。 diff --git a/spec/github/usecase_spec.cr b/spec/github/usecase_spec.cr index 84b77ea..847b644 100644 --- a/spec/github/usecase_spec.cr +++ b/spec/github/usecase_spec.cr @@ -165,6 +165,26 @@ describe Github::Usecase do repo.checks_calls.should eq 0 end end + + # 投稿を分ける基準(issue #120)。メンション自体は CI の状態で抑止されるが、 + # 抑止されても通知の重要度は下がらないため、判定は reason だけで行う。 + describe "#build_message important" do + it "marks a mention reason as important" do + build(notification(reason: "review_requested"), comment).important?.should be_true + end + + it "keeps a non-mention reason unimportant" do + build(notification(reason: "subscribed"), comment).important?.should be_false + end + + it "keeps a pull request important while its checks are failing" do + message = build_with_checks(pull_request(reason: "review_requested"), Github::ChecksState::Failure) + + # チャンネル全体は叩かないが、メンション対象の投稿には入れる。 + message.mention?.should be_false + message.important?.should be_true + end + end end private def pull_request(reason = "review_requested") diff --git a/spec/notify/usecase_spec.cr b/spec/notify/usecase_spec.cr index ba46e71..5efebe4 100644 --- a/spec/notify/usecase_spec.cr +++ b/spec/notify/usecase_spec.cr @@ -5,10 +5,11 @@ require "../../src/notify/repository" require "../../src/notify/usecase" # updated_at だけを差し替えられる通知を組み立てる。 -private def notif(updated_at : String, reason = "subscribed") +# reason は投稿の区切り(issue #120)、title は送信順の確認に使う。 +private def notif(updated_at : String, reason = "subscribed", title = "title") Github::Notification.from_json({ reason: reason, - subject: {type: "Issue", title: "title"}, + subject: {type: "Issue", title: title}, repository: {owner: {login: "octocat"}}, updated_at: updated_at, }.to_json) @@ -65,6 +66,22 @@ private class ChunkPoster < Notify::PostRepository end end +# send_messages の呼び出しごとに、受け取ったメッセージを 1 投稿ぶんとして記録する。 +# 全件を 1 投稿で送る Slack と同じく、送信後に一度だけ累計を yield する。 +# fail_at 番目(0 始まり)の投稿で例外を投げて途中失敗を再現する。 +private class RecordingPoster < Notify::PostRepository + getter posts = [] of Array(Notify::Message) + + def initialize(@fail_at : Int32? = nil) + end + + def send_messages(messages : Array(Notify::Message), & : Int32 ->) + raise "send failed" if @fail_at == @posts.size + @posts << messages + yield messages.size + end +end + private def run(notifications, poster, &) repo = FakeNotificationRepo.new notifications usecase = Notify::Usecase.new repo, Github::Usecase.new(repo), poster @@ -125,6 +142,73 @@ describe Notify::Usecase do repo.read_calls.should be_empty end + # メンション対象とそれ以外を別々の投稿に分け、`@channel` の付く投稿に + # 自分宛て以外を混ぜない(issue #120)。 + + it "メンション対象とそれ以外を別々の投稿に分ける" do + notifications = [ + notif("2026-01-01T00:00:01Z"), + notif("2026-01-01T00:00:02Z", reason: "mention"), + notif("2026-01-01T00:00:03Z", reason: "review_requested"), + notif("2026-01-01T00:00:04Z"), + ] + poster = RecordingPoster.new + run(notifications, poster, &.check_notifications) + + poster.posts.map(&.map(&.important?)).should eq [[false], [true, true], [false]] + end + + it "投稿を分けても updated_at 昇順のまま送る" do + notifications = [ + notif("2026-01-01T00:00:01Z", title: "a"), + notif("2026-01-01T00:00:02Z", reason: "mention", title: "b"), + notif("2026-01-01T00:00:03Z", title: "c"), + ] + poster = RecordingPoster.new + run(notifications, poster, &.check_notifications) + + poster.posts.flat_map(&.map(&.title)).should eq ["a", "b", "c"] + end + + it "重要度が変わらなければ 1 投稿にまとめる" do + notifications = [notif("2026-01-01T00:00:01Z"), notif("2026-01-01T00:00:02Z")] + poster = RecordingPoster.new + run(notifications, poster, &.check_notifications) + + poster.posts.size.should eq 1 + end + + it "投稿を分けても既読化の境界は分割前と変わらない" do + # 投稿ごとの yield は区間内の累計だが、既読化には全体の累計が渡る。 + # 3 投稿に分かれても、境界は未送信先頭の updated_at と取得スナップショット。 + notifications = [ + notif("2026-01-01T00:00:01Z"), + notif("2026-01-01T00:00:02Z", reason: "mention"), + notif("2026-01-01T00:00:03Z", reason: "review_requested"), + notif("2026-01-01T00:00:04Z"), + ] + repo = run(notifications, RecordingPoster.new) do |usecase| + usecase.check_notifications + end + + repo.read_calls.should eq [t(2), t(4), repo.before_arg] + end + + it "投稿の途中で失敗したら送信済みの投稿までしか既読化しない" do + notifications = [ + notif("2026-01-01T00:00:01Z"), + notif("2026-01-01T00:00:02Z", reason: "mention"), + notif("2026-01-01T00:00:03Z"), + ] + repo = run(notifications, RecordingPoster.new(fail_at: 1)) do |usecase| + expect_raises(Exception, "send failed") { usecase.check_notifications } + end + + # 2 投稿目(t(2))は未送信。境界 t(2) は排他的なので t(2) 自身は既読化されず、 + # 次回 t(2) 以降だけが再取得される。 + repo.read_calls.should eq [t(2)] + end + it "既読化に失敗したら後続チャンクの送信を止める" do notifications = [notif("2026-01-01T00:00:01Z"), notif("2026-01-01T00:00:02Z"), notif("2026-01-01T00:00:03Z")] poster = ChunkPoster.new([1, 1, 1]) diff --git a/spec/slack/repository_spec.cr b/spec/slack/repository_spec.cr new file mode 100644 index 0000000..ad8eb43 --- /dev/null +++ b/spec/slack/repository_spec.cr @@ -0,0 +1,97 @@ +require "../spec_helper" +require "../../src/slack/repository" + +# HTTP を張らずに、あらかじめ用意した応答を順に返す送信先。 +# 応答を使い切ったあとは最後の応答を返し続ける。 +private class StubPostRepository < Slack::PostRepository + getter attempts = 0 + + def initialize(@responses : Array(HTTP::Client::Response)) + super("https://example.com/webhook") + end + + private def post_json(_body : String) : HTTP::Client::Response + @attempts += 1 + @responses[@attempts - 1]? || @responses.last + end +end + +# Retry-After を 0 にして、待機でテストが遅くならないようにする。 +private def response(status : Int32, retry_after : String? = "0") + headers = HTTP::Headers.new + headers["Retry-After"] = retry_after if retry_after + HTTP::Client::Response.new(status, body: "", headers: headers) +end + +private def messages(count = 1) + Array.new(count) { Notify::Message.new(pretext: "pre") } +end + +describe Slack::PostRepository do + describe "#send_messages" do + it "sends once and yields the total when the webhook succeeds" do + poster = StubPostRepository.new([response(200)]) + sent = [] of Int32 + poster.send_messages(messages(3)) { |count| sent << count } + + poster.attempts.should eq 1 + sent.should eq [3] + end + + # 応答を検査しないと、投稿されていない通知まで呼び出し側が既読化して + # 通知が消える。失敗時は yield させず例外にする(issue #120)。 + it "raises without yielding when the webhook keeps rate limiting" do + poster = StubPostRepository.new([response(429)]) + sent = [] of Int32 + + expect_raises(Exception, /slack webhook returned 429/) do + poster.send_messages(messages(2)) { |count| sent << count } + end + + poster.attempts.should eq Slack::PostRepository::MAX_SEND_ATTEMPTS + sent.should be_empty + end + + it "raises without yielding when the webhook returns a server error" do + poster = StubPostRepository.new([response(500)]) + sent = [] of Int32 + + expect_raises(Exception, /slack webhook returned 500/) do + poster.send_messages(messages(2)) { |count| sent << count } + end + + sent.should be_empty + end + + it "yields after a retried rate limit succeeds" do + poster = StubPostRepository.new([response(429), response(200)]) + sent = [] of Int32 + poster.send_messages(messages(2)) { |count| sent << count } + + poster.attempts.should eq 2 + sent.should eq [2] + end + + # 恒久的な失敗を再送しても通らないため、そのまま例外にする。 + it "does not retry a permanent client error" do + poster = StubPostRepository.new([response(404)]) + sent = [] of Int32 + + expect_raises(Exception, /slack webhook returned 404/) do + poster.send_messages(messages) { |count| sent << count } + end + + poster.attempts.should eq 1 + sent.should be_empty + end + + # Retry-After が無い応答でも待機上限を超えないこと。 + it "caps the wait when the webhook sends no Retry-After" do + poster = StubPostRepository.new([response(429, retry_after: nil), response(200)]) + started = Time.monotonic + poster.send_messages(messages) { } + + (Time.monotonic - started).should be < Slack::PostRepository::MAX_RETRY_WAIT + end + end +end diff --git a/src/github/usecase.cr b/src/github/usecase.cr index 7a9b861..9e0188d 100644 --- a/src/github/usecase.cr +++ b/src/github/usecase.cr @@ -15,6 +15,9 @@ module Github comment = @repo.find_comment_by_url notify.subject.comment_url Notify::Message.new( mention: mention?(notify), + # 投稿の区切りは reason だけで決め、CI によるメンション抑止は反映しない + # (issue #120。理由は Notify::Message#important? のコメント)。 + important: notify.mention?, author_name: comment.user.login, author_icon: comment.user.avatar_url, author_link: comment.user.html_url, diff --git a/src/notify/models.cr b/src/notify/models.cr index ce1e70c..9d21fa8 100644 --- a/src/notify/models.cr +++ b/src/notify/models.cr @@ -3,6 +3,14 @@ module Notify # Slack / Discord などのアダプタがそれぞれの形式へ変換する。 class Message getter? mention : Bool + # 重要度の高い(自分宛ての)通知か。メンション対象かどうかで投稿を区切り、 + # `@channel` 付きの投稿に自分宛て以外を混ぜないために使う(issue #120)。 + # + # mention? とは別に持つ。mention? は CI が赤い PR でチャンネル全体を叩かない + # ための抑止(issue #105)が掛かった後の値で、抑止されても通知の重要度自体は + # 下がらない。両者を同じフラグにすると、CI が赤いレビュー依頼が通常の通知に + # 混ざってしまう。 + getter? important : Bool getter author_name : String? getter author_icon : String? getter author_link : String? @@ -16,6 +24,7 @@ module Notify def initialize( @mention = false, + @important = false, @author_name = nil, @author_icon = nil, @author_link = nil, diff --git a/src/notify/usecase.cr b/src/notify/usecase.cr index bacaab3..2e2952b 100644 --- a/src/notify/usecase.cr +++ b/src/notify/usecase.cr @@ -34,13 +34,44 @@ module Notify # チャンク送信が成功するたび、そこまでに送信済みの通知だけを既読化する。 # 途中で失敗しても送信済み分は既読化済みなので、未送信分だけが次回 # 再取得され、前半チャンクの重複投稿が起きない。 - @poster.send_messages(notices) do |sent_count| + send_split notices do |sent_count| mark_read_through notifications, sent_count, fetched_at end {msg: "ok"} end + # 重要度の高い通知(メンション対象)とそれ以外を、別々の投稿に分けて送る + # (issue #120)。 + # + # 両者が同じ投稿に混ざると、`@channel` が付いていても、どれが自分宛てなのかは + # 投稿を開くまで分からない。important? が切り替わる位置で投稿を区切ると、 + # `@channel` の付く投稿には自分宛ての通知だけが入る。 + # + # 区切るのは並べ替えではなく分割なので、メッセージは updated_at 昇順のまま + # 送られる。既読化は「送信済みは常に先頭からのプレフィックス」であることに + # 依存しているため(mark_read_through 参照)、順序は保つ必要がある。 + # + # 送信先アダプタを包むデコレータにする案もあったが、委譲先の静的型に + # デコレータ自身が含まれ、yield するブロックのインライン展開が無限再帰して + # コンパイルできない。累計の正しさは既読化の境界と表裏なので、 + # mark_read_through と同じ場所に置く。 + private def send_split(notices : Array(Message), & : Int32 ->) + sent = 0 + + # important? が同じ値で連続する区間ごとに投稿する。 + notices.chunks(&.important?).each do |(_, run)| + # アダプタが yield するのは区間内の累計なので、直前までの区間の合計を + # 足して全体の累計に直す。呼び出し側から見える値は分割前と変わらない。 + @poster.send_messages(run) do |count| + yield sent + count + end + + # 送信の失敗は例外になる契約なので、正常に戻った時点で区間は全件送信済み。 + sent += run.size + end + end + # 昇順ソート済み notifications の先頭 sent_count 件(=送信済み)までを既読化する。 # # PUT /notifications の last_read_at は排他的境界で、updated_at < last_read_at diff --git a/src/slack/repository.cr b/src/slack/repository.cr index 048c1a4..841adbd 100644 --- a/src/slack/repository.cr +++ b/src/slack/repository.cr @@ -7,6 +7,9 @@ require "../notify/repository" module Slack class PostRepository < Notify::PostRepository + MAX_SEND_ATTEMPTS = 3 # 送信リトライ回数の上限 + MAX_RETRY_WAIT = 5.seconds # Retry-After の待機上限 + def initialize(url : String) @uri = URI.parse url @client = HTTP::Client.new @uri @@ -20,7 +23,38 @@ module Slack end private def send_post(post : Post) - @client.post(@uri.request_target, body: post.to_json) + body = post.to_json + + attempt = 0 + loop do + attempt += 1 + res = post_json body + return if res.success? + + # 重要度で投稿を分けるようになり、1 実行あたりの投稿数が増えてレート制限 + # (429)に当たりやすくなった(issue #120)。一時的な失敗でその実行を丸ごと + # 落とさずに済むよう、429 と 5xx は Retry-After に従って再送する。 + # 方針は Discord::PostRepository と揃えている。 + retryable = res.status.code == 429 || res.status.server_error? + if retryable && attempt < MAX_SEND_ATTEMPTS + sleep retry_after(res) + next + end + + # 応答を捨てると、投稿されていない通知まで呼び出し側が既読化してしまい、 + # その通知はどこにも表示されないまま消える。恒久的な失敗は例外にして + # 既読化を止め、次回実行に委ねる。 + raise "slack webhook returned #{res.status_code}: #{res.body}" + end + end + + private def post_json(body : String) : HTTP::Client::Response + @client.post(@uri.request_target, body: body) + end + + private def retry_after(res : HTTP::Client::Response) : Time::Span + seconds = res.headers["Retry-After"]?.try(&.to_f?) || 1.0 + seconds.seconds.clamp(Time::Span.zero, MAX_RETRY_WAIT) end end end