diff --git a/spec/github/models_spec.cr b/spec/github/models_spec.cr index ad916ca..c75cc84 100644 --- a/spec/github/models_spec.cr +++ b/spec/github/models_spec.cr @@ -210,6 +210,28 @@ describe Github::Notification do end end + describe "#checks_gated?" do + it "is true for every pull request notification regardless of reason" do + # reason は購読理由であってイベント種別ではないため、mention 系も + # 「今回の更新がメンションだった」ことを意味しない。よって例外にしない。 + [ + "review_requested", + "assign", + "author", + "mention", + "team_mention", + ].each do |reason| + notification_from(reason, type: "PullRequest").checks_gated?.should be_true + end + end + + it "is false for non pull request subjects" do + notification_from("review_requested", type: "Issue").checks_gated?.should be_false + notification_from("mention", type: "Issue").checks_gated?.should be_false + notification_from("author", type: "Commit").checks_gated?.should be_false + end + end + it "parses a GitHub notifications API payload" do notifications = Array(Github::Notification).from_json(NOTIFICATIONS_FIXTURE) notifications.size.should eq 1 @@ -224,10 +246,10 @@ describe Github::Notification do end end -private def notification_from(reason : String, url = "", latest_comment_url = "") +private def notification_from(reason : String, type = "Issue", url = "", latest_comment_url = "") Github::Notification.from_json({ reason: reason, - subject: {type: "Issue", title: "title", url: url, latest_comment_url: latest_comment_url}, + subject: {type: type, title: "title", url: url, latest_comment_url: latest_comment_url}, repository: {owner: {login: "octocat"}}, updated_at: "2026-07-14T00:00:00Z", }.to_json) @@ -242,6 +264,96 @@ private def notification_with(url = "", repo_html_url : String? = nil) }.to_json) end +describe Github::ChecksState do + describe "#merge" do + it "takes the stricter state of the two" do + Github::ChecksState::Success.merge(Github::ChecksState::Failure).should eq Github::ChecksState::Failure + Github::ChecksState::Pending.merge(Github::ChecksState::Failure).should eq Github::ChecksState::Failure + Github::ChecksState::Success.merge(Github::ChecksState::Pending).should eq Github::ChecksState::Pending + Github::ChecksState::NoChecks.merge(Github::ChecksState::Success).should eq Github::ChecksState::Success + end + + it "keeps the other state when one side could not be fetched" do + Github::ChecksState::Unknown.merge(Github::ChecksState::Success).should eq Github::ChecksState::Success + Github::ChecksState::Failure.merge(Github::ChecksState::Unknown).should eq Github::ChecksState::Failure + end + + it "stays unknown when neither side could be fetched" do + Github::ChecksState::Unknown.merge(Github::ChecksState::Unknown).should eq Github::ChecksState::Unknown + end + + it "stays no-checks when neither side has any check" do + Github::ChecksState::NoChecks.merge(Github::ChecksState::NoChecks).should eq Github::ChecksState::NoChecks + end + end + + describe "#blocks_mention?" do + it "blocks while checks are failing or still running" do + Github::ChecksState::Failure.blocks_mention?.should be_true + Github::ChecksState::Pending.blocks_mention?.should be_true + end + + it "does not block on success, no checks, or a failed lookup" do + Github::ChecksState::Success.blocks_mention?.should be_false + Github::ChecksState::NoChecks.blocks_mention?.should be_false + Github::ChecksState::Unknown.blocks_mention?.should be_false + end + end +end + +describe Github::CheckRuns do + describe "#checks_state" do + it "is success when every run completed without a blocking conclusion" do + check_runs_from([ + {status: "completed", conclusion: "success"}, + {status: "completed", conclusion: "skipped"}, + {status: "completed", conclusion: "neutral"}, + ]).checks_state.should eq Github::ChecksState::Success + end + + it "is failure when a completed run has a blocking conclusion" do + check_runs_from([ + {status: "completed", conclusion: "success"}, + {status: "completed", conclusion: "failure"}, + ]).checks_state.should eq Github::ChecksState::Failure + end + + it "is pending while a run has not completed" do + check_runs_from([ + {status: "completed", conclusion: "success"}, + {status: "in_progress", conclusion: nil}, + ]).checks_state.should eq Github::ChecksState::Pending + end + + it "is no-checks when the commit has no check run" do + check_runs_from([] of NamedTuple(status: String, conclusion: String?)).checks_state.should eq Github::ChecksState::NoChecks + end + end +end + +describe Github::CombinedStatus do + describe "#checks_state" do + it "maps the combined state" do + combined_status_from("success", 2).checks_state.should eq Github::ChecksState::Success + combined_status_from("failure", 2).checks_state.should eq Github::ChecksState::Failure + combined_status_from("error", 2).checks_state.should eq Github::ChecksState::Failure + combined_status_from("pending", 2).checks_state.should eq Github::ChecksState::Pending + end + + it "is no-checks when the commit has no status, even though the api reports pending" do + combined_status_from("pending", 0).checks_state.should eq Github::ChecksState::NoChecks + end + end +end + +private def check_runs_from(runs) + Github::CheckRuns.from_json({check_runs: runs}.to_json) +end + +private def combined_status_from(state : String, total_count : Int32) + Github::CombinedStatus.from_json({state: state, total_count: total_count}.to_json) +end + NOTIFICATIONS_FIXTURE = <<-JSON [ { diff --git a/spec/github/usecase_spec.cr b/spec/github/usecase_spec.cr index 796b8a3..419d364 100644 --- a/spec/github/usecase_spec.cr +++ b/spec/github/usecase_spec.cr @@ -2,26 +2,34 @@ require "../spec_helper" require "../../src/github/repository" require "../../src/github/usecase" -# HTTP を張らずに、あらかじめ用意した Comment を返すリポジトリ。 +# HTTP を張らずに、あらかじめ用意した Comment とチェック状態を返すリポジトリ。 private class StubRepo < Github::NotificationRepository - def initialize(@comment : Github::Comment) + getter checks_calls = 0 + + def initialize(@comment : Github::Comment, @checks_state : Github::ChecksState = Github::ChecksState::NoChecks) super("token") end def find_comment_by_url(url : String) : Github::Comment @comment end + + def find_checks_state(notify : Github::Notification) : Github::ChecksState + @checks_calls += 1 + @checks_state + end end private def notification( url = "https://api.github.com/repos/octocat/Hello-World/issues/42", reason = "review_requested", repo_html_url : String? = "https://github.com/octocat/Hello-World", + type = "Issue", latest_comment_url = "", ) Github::Notification.from_json({ reason: reason, - subject: {type: "Issue", title: "Spurious failure", url: url, latest_comment_url: latest_comment_url}, + subject: {type: type, title: "Spurious failure", url: url, latest_comment_url: latest_comment_url}, repository: {full_name: "octocat/Hello-World", html_url: repo_html_url, owner: {login: "octocat"}}, updated_at: "2026-07-14T00:00:00Z", }.to_json) @@ -76,4 +84,73 @@ describe Github::Usecase do build(notification, comment(body: nil)).text.should be_nil end end + + describe "#build_message mention" do + it "mentions on a pull request whose checks all succeeded" do + message = build_with_checks(pull_request, Github::ChecksState::Success) + message.mention?.should be_true + end + + it "does not mention while a pull request has failing checks" do + build_with_checks(pull_request, Github::ChecksState::Failure).mention?.should be_false + end + + it "does not mention while a pull request still has running checks" do + build_with_checks(pull_request, Github::ChecksState::Pending).mention?.should be_false + end + + it "mentions when the pull request has no checks configured" do + build_with_checks(pull_request, Github::ChecksState::NoChecks).mention?.should be_true + end + + it "mentions when the checks state could not be fetched" do + build_with_checks(pull_request, Github::ChecksState::Unknown).mention?.should be_true + end + + it "does not mention on a failing pull request even for a mention reason" do + # reason=mention は一度メンションされた PR に永続するため、その後の + # push やコメントでも維持される。例外にすると赤い PR でチャンネル全体を + # 叩いてしまうので、mention 系もチェック状態で制御する。 + notify = pull_request(reason: "mention") + build_with_checks(notify, Github::ChecksState::Failure).mention?.should be_false + end + + it "mentions on a passing pull request for a mention reason" do + notify = pull_request(reason: "mention") + build_with_checks(notify, Github::ChecksState::Success).mention?.should be_true + end + + it "keeps non-mention reasons unmentioned regardless of the checks state" do + notify = pull_request(reason: "subscribed") + build_with_checks(notify, Github::ChecksState::Success).mention?.should be_false + end + + it "does not look up checks for non pull request subjects" do + repo = StubRepo.new(comment, Github::ChecksState::Failure) + message = Github::Usecase.new(repo).build_message(notification(reason: "review_requested")) + + message.mention?.should be_true + repo.checks_calls.should eq 0 + end + + it "does not look up checks for reasons that never mention" do + repo = StubRepo.new(comment, Github::ChecksState::Failure) + message = Github::Usecase.new(repo).build_message(pull_request(reason: "subscribed")) + + message.mention?.should be_false + repo.checks_calls.should eq 0 + end + end +end + +private def pull_request(reason = "review_requested") + notification( + url: "https://api.github.com/repos/octocat/Hello-World/pulls/42", + reason: reason, + type: "PullRequest", + ) +end + +private def build_with_checks(notify, checks_state) + Github::Usecase.new(StubRepo.new(comment, checks_state)).build_message(notify) end diff --git a/src/github/models.cr b/src/github/models.cr index d6bd7ac..712c1af 100644 --- a/src/github/models.cr +++ b/src/github/models.cr @@ -1,6 +1,35 @@ require "json" module Github + # PR の CI・自動チェックの集計状態(issue #105)。 + # check runs(GitHub Actions 等)と commit status の 2 系統をまとめて表す。 + enum ChecksState + Success # 全て完了し、ブロックする結果が無い + Pending # 未完了のチェックがある + Failure # 失敗したチェックがある + NoChecks # チェックが 1 つも設定されていない + Unknown # 取得できなかった + + # 2 系統の結果を 1 つに畳む。厳しい方(Failure > Pending > Success > + # NoChecks)を採る。Unknown は「情報が無い」だけなので、もう片方が + # 取得できていればそちらの結果を活かす。 + def merge(other : ChecksState) : ChecksState + return other if unknown? + return self if other.unknown? + return Failure if failure? || other.failure? + return Pending if pending? || other.pending? + return Success if success? || other.success? + NoChecks + end + + # メンションを抑止すべき状態か。 + # 成功・チェック未設定・取得失敗ではメンションする。取得できなかった場合に + # 抑止すると通知の見逃しにつながるため、安全側(誤メンションを許容)に倒す。 + def blocks_mention? : Bool + failure? || pending? + end + end + class Notification include JSON::Serializable @@ -71,6 +100,23 @@ module Github REASON_MESSAGES[reason]? || GENERIC_MESSAGE end + # CI・自動チェックの状態でメンションを抑止する対象か(issue #105)。 + # レビューできる状態になっていない PR で `@channel` / `@everyone` を撃たない + # ことが目的なので、PR の通知はすべて対象にする。 + # + # 当初は mention / team_mention を「人が明示的に呼んだ」ものとして対象外に + # していたが、reason は購読理由であってイベント種別ではないため(FOLLOWUP_MESSAGES + # のコメント参照)、一度メンションされた PR はその後の push やコメントでも + # reason=mention のまま届く。これを対象外にすると、最も関与している PR でこそ + # チェックが赤いままチャンネル全体を叩いてしまい本末転倒なので、reason による + # 例外は設けない(PR #107 レビュー指摘)。 + # + # 実際に今回の更新がメンションだったかは通知 payload からは判別できない。 + # メンションされた通知自体は従来どおり届き、`@channel` が付かなくなるだけ。 + def checks_gated? : Bool + subject.type == Subject::Type::PULL_REQUEST + end + # 通知の pretext(botのセリフ)。`[] ` 形式。 def pretext : String "[#{subject.type}] #{reason_message}" @@ -217,6 +263,79 @@ module Github end end + # チェック状態の判定に使う PR 情報。head の SHA だけ参照する(issue #105)。 + class PullRequest + include JSON::Serializable + + getter head : Head + + class Head + include JSON::Serializable + + getter sha : String + end + end + + # GET /repos/:owner/:repo/commits/:ref/check-runs のレスポンス(issue #105)。 + class CheckRuns + include JSON::Serializable + + # ブロックしない conclusion。neutral / skipped は「実行された上で + # 通していい」結果なので成功側に含める。 + PASSING_CONCLUSIONS = { + "success", + "neutral", + "skipped", + } + + getter check_runs : Array(CheckRun) = [] of CheckRun + + def checks_state : ChecksState + return ChecksState::NoChecks if check_runs.empty? + return ChecksState::Pending unless check_runs.all?(&.completed?) + + check_runs.all?(&.passing?) ? ChecksState::Success : ChecksState::Failure + end + + class CheckRun + include JSON::Serializable + + getter status : String = "" + getter conclusion : String? + + def completed? : Bool + status == "completed" + end + + def passing? : Bool + conclusion.in?(PASSING_CONCLUSIONS) + end + end + end + + # GET /repos/:owner/:repo/commits/:ref/status のレスポンス(issue #105)。 + # check runs とは別系統の commit status(外部 CI 等)を表す。 + class CombinedStatus + include JSON::Serializable + + getter state : String = "" + getter total_count : Int32 = 0 + + def checks_state : ChecksState + # status が 1 件も無いと state は "pending" で返るため、件数で先に弾く。 + return ChecksState::NoChecks if total_count.zero? + + case state + when "success" + ChecksState::Success + when "failure", "error" + ChecksState::Failure + else + ChecksState::Pending + end + end + end + class Error include JSON::Serializable diff --git a/src/github/repository.cr b/src/github/repository.cr index c621c05..f7cd4ef 100644 --- a/src/github/repository.cr +++ b/src/github/repository.cr @@ -132,6 +132,79 @@ module Github end end + # チェック状態を見るときに一度に取得する check runs の件数。 + # これを超える数のチェックがある PR は一部しか見えないが、GitHub の上限 + # 100 を超えるチェックは想定しにくいためページングはしない(issue #105)。 + CHECKS_PER_PAGE = 100 + + # PR の CI・自動チェックの集計状態を返す(issue #105)。 + # + # check runs(GitHub Actions 等)と commit status(外部 CI 等)は別系統で、 + # 片方にしか結果が出ないことがあるため両方を見て厳しい方を採る。 + # + # 取得できなかった場合は Unknown を返し、呼び出し側でメンションを維持させる。 + # 通知の見逃しより誤メンションの方が軽い、という判断(安全側に倒す)。 + def find_checks_state(notify : Notification) : ChecksState + return ChecksState::Unknown unless repo = notify.repository.full_name.try(&.presence) + return ChecksState::Unknown unless number = notify.subject.number + return ChecksState::Unknown unless sha = find_head_sha "/repos/#{repo}/pulls/#{number}" + + runs = find_check_runs_state "/repos/#{repo}/commits/#{sha}/check-runs?per_page=#{CHECKS_PER_PAGE}" + runs.merge find_combined_status_state("/repos/#{repo}/commits/#{sha}/status?per_page=#{CHECKS_PER_PAGE}") + end + + private def find_head_sha(path : String) : String? + return unless body = get_body path + + begin + PullRequest.from_json(body).head.sha.presence + rescue + Serverless::Lambda.print_log "failed parse pull request data" + nil + end + end + + private def find_check_runs_state(path : String) : ChecksState + return ChecksState::Unknown unless body = get_body path + + begin + CheckRuns.from_json(body).checks_state + rescue + Serverless::Lambda.print_log "failed parse check runs data" + ChecksState::Unknown + end + end + + private def find_combined_status_state(path : String) : ChecksState + return ChecksState::Unknown unless body = get_body path + + begin + CombinedStatus.from_json(body).checks_state + rescue + Serverless::Lambda.print_log "failed parse combined status data" + ChecksState::Unknown + end + end + + # チェック状態の取得用 GET。1 件の取得失敗で通知全体を巻き添えにしないよう、 + # 例外・エラー応答はログだけ残して nil を返す(issue #105)。 + private def get_body(path : String) : String? + res = + begin + @github.get path + rescue ex + Serverless::Lambda.print_log "failed to get #{path}: #{ex.message}" + return + end + + unless res.success? + Serverless::Lambda.print_log "return #{res.status_code} from #{path}" + return + end + + res.body + end + # 通知を既読化する。last_read_at は排他的境界で、その時刻より前 # (updated_at < last_read_at)に更新された通知だけが既読化される。 # 等値(updated_at == last_read_at)は未読のまま残るため、送信済み通知を diff --git a/src/github/usecase.cr b/src/github/usecase.cr index 28f2094..78052a9 100644 --- a/src/github/usecase.cr +++ b/src/github/usecase.cr @@ -14,7 +14,7 @@ module Github def build_message(notify : Notification) : Notify::Message comment = @repo.find_comment_by_url notify.subject.comment_url Notify::Message.new( - mention: notify.mention?, + mention: mention?(notify), author_name: comment.user.login, author_icon: comment.user.avatar_url, author_link: comment.user.html_url, @@ -28,6 +28,18 @@ module Github ) end + # メンション(`@channel` / `@everyone`)を付けるか。 + # + # mention 系 reason であることに加え、PR は CI・自動チェックが失敗中・実行中 + # でないことを条件にする。まだレビューできる状態ではない PR でチャンネル全体を + # 叩かないため(issue #105)。通知そのものは抑止しない。 + private def mention?(notify : Notification) : Bool + return false unless notify.mention? + return true unless notify.checks_gated? + + !@repo.find_checks_state(notify).blocks_mention? + end + private def truncate_body(body : String?) : String? return unless text = body.try(&.presence) text.size > BODY_LIMIT ? "#{text[0, BODY_LIMIT]}…" : text