From 7517aeb416dc0a7fb76145812be8f53fa874af9d Mon Sep 17 00:00:00 2001
From: Yasunari Watanabe <yasunari.watanabe@ahrefs.com>
Date: Tue, 22 Dec 2020 13:21:39 +0800
Subject: [PATCH] make status rule handling fall back on sensible defaults if
 no match

---
 documentation/config_docs.md |  6 +++++-
 lib/rule.atd                 |  9 ++++++++-
 lib/rule.ml                  | 14 +++++++++++---
 test/notabot.json            |  5 +----
 4 files changed, 25 insertions(+), 9 deletions(-)

diff --git a/documentation/config_docs.md b/documentation/config_docs.md
index d93d04b3..a029c407 100644
--- a/documentation/config_docs.md
+++ b/documentation/config_docs.md
@@ -143,7 +143,11 @@ Monorobot supports additional behavior for GitHub status notifications, which ar
 
 The following takes place when a status notification is received.
 
-1. Check whether a status notification should be *allowed* for further processing or *ignored*, according to a list of **status rules**. The bot will check the list *in order*, and use the policy defined by the first rule that the notification satisfies. If no rule matches, the default behavior is to allow the notification.
+1. Check whether a status notification should be *allowed* for further processing or *ignored*, according to a list of **status rules**. The bot will check the list *in order*, and use the policy defined by the first rule that the notification satisfies. If no rule matches, the default behavior of the status state is used:
+   - `pending`: `ignore`
+   - `failure`: `allow`
+   - `error`: `allow`
+   - `success`: `allow_once`
 1. For those payloads allowed by step 1, if it isn't a main branch build notification, route to the default channel to reduce spam in topic channels. Otherwise, check the notification commit's files according to the prefix rules.
 
 Internally, the bot keeps track of the status of the last allowed payload, for a given pipeline and branch. This information is used to evaluate the status rules (see below).
diff --git a/lib/rule.atd b/lib/rule.atd
index d3bc1fd5..501643f1 100644
--- a/lib/rule.atd
+++ b/lib/rule.atd
@@ -30,7 +30,14 @@ type status_policy = [
 type build_status <ocaml from="Github" t="status_state"> = abstract
 
 (* A status matches a status rule with the policy with the build status is in
-   the list, and the condition is true *)
+   the list, and the condition is true.
+
+   The default behavior for each status state is:
+   - pending: ignore
+   - failure: allow
+   - error: allow
+   - success: allow_once
+*)
 type status_rule = {
   trigger <json name="on"> : build_status list;
   ?condition <json name="when"> : status_condition nullable;
diff --git a/lib/rule.ml b/lib/rule.ml
index 29451a82..e1645af5 100644
--- a/lib/rule.ml
+++ b/lib/rule.ml
@@ -2,9 +2,17 @@ open Base
 open Rule_t
 
 module Status = struct
+  let default_rules =
+    [
+      { trigger = [ Pending ]; condition = None; policy = Ignore };
+      { trigger = [ Failure; Error ]; condition = None; policy = Allow };
+      { trigger = [ Success ]; condition = None; policy = Allow_once };
+    ]
+
   (** `match_rules n rs` returns the policy declared by the first rule in `rs`
-      to match status notification `n`, if one exists. A rule `r` matches `n`
-      if `n.state` is in `r.trigger` and `n` meets `r.condition`. *)
+      to match status notification `n` if one exists, falling back to
+      the default rules otherwise. A rule `r` matches `n` if `n.state` is in
+      `r.trigger` and `n` meets `r.condition`. *)
   let match_rules (notification : Github_t.status_notification) ~rules =
     let match_rule rule =
       let value_of_field = function
@@ -27,7 +35,7 @@ module Status = struct
       then Some rule.policy
       else None
     in
-    List.find_map rules ~f:match_rule
+    List.find_map (List.append rules default_rules) ~f:match_rule
 end
 
 module Prefix = struct
diff --git a/test/notabot.json b/test/notabot.json
index 13aacaa8..49dce207 100644
--- a/test/notabot.json
+++ b/test/notabot.json
@@ -15,10 +15,7 @@
                     }
                 },
                 "policy": "ignore"
-            },
-            { "on": ["pending"], "policy": "ignore"},
-            { "on": ["failure", "error"], "policy": "allow"},
-            { "on": ["success"], "policy": "allow_once"}
+            }
         ]
     },
     "prefix_rules": {