Provide a more structured result log for Herald rules

Summary: Ref T13586. In the footsteps of D21563, make Herald rule results more formal and structured to support meaningful exception reporting.

Test Plan:
Ran various Herald rules and viewed transcripts, including rules with recursive dependencies and condition exceptions.

{F8447894}

Maniphest Tasks: T13586

Differential Revision: https://secure.phabricator.com/D21565

src/__phutil_library_map__.php

@@ -1621,6 +1621,7 @@ phutil_register_library_map(array(
     'HeraldRuleDisableTransaction' => 'applications/herald/xaction/HeraldRuleDisableTransaction.php',
     'HeraldRuleEditTransaction' => 'applications/herald/xaction/HeraldRuleEditTransaction.php',
     'HeraldRuleEditor' => 'applications/herald/editor/HeraldRuleEditor.php',
+    'HeraldRuleEvaluationException' => 'applications/herald/engine/exception/HeraldRuleEvaluationException.php',
     'HeraldRuleField' => 'applications/herald/field/rule/HeraldRuleField.php',
     'HeraldRuleFieldGroup' => 'applications/herald/field/rule/HeraldRuleFieldGroup.php',
     'HeraldRuleIndexEngineExtension' => 'applications/herald/engineextension/HeraldRuleIndexEngineExtension.php',
@@ -1631,6 +1632,7 @@ phutil_register_library_map(array(
     'HeraldRulePHIDType' => 'applications/herald/phid/HeraldRulePHIDType.php',
     'HeraldRuleQuery' => 'applications/herald/query/HeraldRuleQuery.php',
     'HeraldRuleReplyHandler' => 'applications/herald/mail/HeraldRuleReplyHandler.php',
+    'HeraldRuleResult' => 'applications/herald/storage/transcript/HeraldRuleResult.php',
     'HeraldRuleSearchEngine' => 'applications/herald/query/HeraldRuleSearchEngine.php',
     'HeraldRuleSerializer' => 'applications/herald/editor/HeraldRuleSerializer.php',
     'HeraldRuleTestCase' => 'applications/herald/storage/__tests__/HeraldRuleTestCase.php',
@@ -7861,6 +7863,7 @@ phutil_register_library_map(array(
     'HeraldRuleDisableTransaction' => 'HeraldRuleTransactionType',
     'HeraldRuleEditTransaction' => 'HeraldRuleTransactionType',
     'HeraldRuleEditor' => 'PhabricatorApplicationTransactionEditor',
+    'HeraldRuleEvaluationException' => 'Exception',
     'HeraldRuleField' => 'HeraldField',
     'HeraldRuleFieldGroup' => 'HeraldFieldGroup',
     'HeraldRuleIndexEngineExtension' => 'PhabricatorEdgeIndexEngineExtension',
@@ -7871,6 +7874,7 @@ phutil_register_library_map(array(
     'HeraldRulePHIDType' => 'PhabricatorPHIDType',
     'HeraldRuleQuery' => 'PhabricatorCursorPagedPolicyAwareQuery',
     'HeraldRuleReplyHandler' => 'PhabricatorApplicationTransactionReplyHandler',
+    'HeraldRuleResult' => 'HeraldTranscriptResult',
     'HeraldRuleSearchEngine' => 'PhabricatorApplicationSearchEngine',
     'HeraldRuleSerializer' => 'Phobject',
     'HeraldRuleTestCase' => 'PhabricatorTestCase',

src/applications/herald/controller/HeraldTranscriptController.php

@@ -224,6 +224,7 @@ final class HeraldTranscriptController extends HeraldController {
   }
 
   private function buildActionTranscriptPanel(HeraldTranscript $xscript) {
+    $viewer = $this->getViewer();
     $action_xscript = mgroup($xscript->getApplyTranscripts(), 'getRuleID');
 
     $adapter = $this->getAdapter();
@@ -253,7 +254,9 @@ final class HeraldTranscriptController extends HeraldController {
         ->setHeader($rule_xscript->getRuleName())
         ->setHref($rule_uri);
 
-      if (!$rule_xscript->getResult()) {
+      $rule_result = $rule_xscript->getRuleResult();
+
+      if (!$rule_result->getShouldApplyActions()) {
         $rule_item->setDisabled(true);
       }
 
@@ -275,7 +278,7 @@ final class HeraldTranscriptController extends HeraldController {
         $color = $result->getIconColor();
         $name = $result->getName();
 
-        $result_details = $result->newDetailsView();
+        $result_details = $result->newDetailsView($viewer);
         if ($result_details !== null) {
           $result_details = phutil_tag(
             'div',
@@ -301,27 +304,27 @@ final class HeraldTranscriptController extends HeraldController {
         $cond_list->addItem($cond_item);
       }
 
-      if ($rule_xscript->isForbidden()) {
+      $rule_result = $rule_xscript->getRuleResult();
-        $last_icon = 'fa-ban';
+
-        $last_color = 'indigo';
+      $last_icon = $rule_result->getIconIcon();
-        $last_result = pht('Forbidden');
+      $last_color = $rule_result->getIconColor();
-        $last_note = pht('Object state prevented rule evaluation.');
+      $last_result = $rule_result->getName();
-      } else if ($rule_xscript->getResult()) {
+      $last_note = $rule_result->getDescription();
-        $last_icon = 'fa-check-circle';
+
-        $last_color = 'green';
+      $last_details = $rule_result->newDetailsView($viewer);
-        $last_result = pht('Passed');
+      if ($last_details !== null) {
-        $last_note = pht('Rule passed.');
+        $last_details = phutil_tag(
-      } else {
+          'div',
-        $last_icon = 'fa-times-circle';
+          array(
-        $last_color = 'red';
+            'class' => 'herald-condition-note',
-        $last_result = pht('Failed');
+          ),
-        $last_note = pht('Rule failed.');
+          $last_details);
       }
 
       $cond_last = id(new PHUIStatusItemView())
         ->setIcon($last_icon, $last_color)
         ->setTarget(phutil_tag('strong', array(), $last_result))
-        ->setNote($last_note);
+        ->setNote(array($last_note, $last_details));
       $cond_list->addItem($cond_last);
 
       $cond_box = id(new PHUIBoxView())
@@ -330,11 +333,10 @@ final class HeraldTranscriptController extends HeraldController {
 
       $rule_item->appendChild($cond_box);
 
-      if (!$rule_xscript->getResult()) {
+      // Not all rules will have any action transcripts, but we show them
-        // If the rule didn't pass, don't generate an action transcript since
+      // in general because they may have relevant information even when
-        // actions didn't apply.
+      // rules did not take actions. In particular, state-based actions may
-        continue;
+      // forbid rules from matching.
-      }
 
       $cond_box->addMargin(PHUI::MARGIN_MEDIUM_BOTTOM);
 

src/applications/herald/engine/HeraldEngine.php

@@ -3,8 +3,6 @@
 final class HeraldEngine extends Phobject {
 
   protected $rules = array();
-  protected $results = array();
-  protected $stack = array();
   protected $activeRule;
   protected $transcript;
 
@@ -20,6 +18,9 @@ final class HeraldEngine extends Phobject {
   private $profilerStack = array();
   private $profilerFrames = array();
 
+  private $ruleResults;
+  private $ruleStack;
+
   public function setDryRun($dry_run) {
     $this->dryRun = $dry_run;
     return $this;
@@ -54,6 +55,74 @@ final class HeraldEngine extends Phobject {
     return $engine->getTranscript();
   }
 
+/* -(  Rule Stack  )--------------------------------------------------------- */
+
+  private function resetRuleStack() {
+    $this->ruleStack = array();
+    return $this;
+  }
+
+  private function hasRuleOnStack(HeraldRule $rule) {
+    $phid = $rule->getPHID();
+    return isset($this->ruleStack[$phid]);
+  }
+
+  private function pushRuleStack(HeraldRule $rule) {
+    $phid = $rule->getPHID();
+    $this->ruleStack[$phid] = $rule;
+    return $this;
+  }
+
+  private function getRuleStack() {
+    return array_values($this->ruleStack);
+  }
+
+/* -(  Rule Results  )------------------------------------------------------- */
+
+  private function resetRuleResults() {
+    $this->ruleResults = array();
+    return $this;
+  }
+
+  private function setRuleResult(
+    HeraldRule $rule,
+    HeraldRuleResult $result) {
+
+    $phid = $rule->getPHID();
+
+    if ($this->hasRuleResult($rule)) {
+      throw new Exception(
+        pht(
+          'Herald rule "%s" already has an evaluation result.',
+          $phid));
+    }
+
+    $this->ruleResults[$phid] = $result;
+
+    $this->newRuleTranscript($rule)
+      ->setRuleResult($result);
+
+    return $this;
+  }
+
+  private function hasRuleResult(HeraldRule $rule) {
+    $phid = $rule->getPHID();
+    return isset($this->ruleResults[$phid]);
+  }
+
+  private function getRuleResult(HeraldRule $rule) {
+    $phid = $rule->getPHID();
+
+    if (!$this->hasRuleResult($rule)) {
+      throw new Exception(
+        pht(
+          'Herald rule "%s" does not have an evaluation result.',
+          $phid));
+    }
+
+    return $this->ruleResults[$phid];
+  }
+
   public function applyRules(array $rules, HeraldAdapter $object) {
     assert_instances_of($rules, 'HeraldRule');
     $t_start = microtime(true);
@@ -66,62 +135,70 @@ final class HeraldEngine extends Phobject {
     $this->transcript->setObjectPHID((string)$object->getPHID());
     $this->fieldCache = array();
     $this->fieldExceptions = array();
-    $this->results = array();
     $this->rules   = $rules;
     $this->object  = $object;
 
+    $this->resetRuleResults();
+
     $effects = array();
     foreach ($rules as $phid => $rule) {
-      $this->stack = array();
+      $this->resetRuleStack();
-
-      $is_first_only = $rule->isRepeatFirst();
 
       $caught = null;
+      $result = null;
       try {
+        $is_first_only = $rule->isRepeatFirst();
+
         if (!$this->getDryRun() &&
             $is_first_only &&
             $rule->getRuleApplied($object->getPHID())) {
+
           // This is not a dry run, and this rule is only supposed to be
-          // applied a single time, and it's already been applied...
+          // applied a single time, and it has already been applied.
           // That means automatic failure.
-          $this->newRuleTranscript($rule)
-            ->setResult(false)
-            ->setReason(
-              pht(
-                'This rule is only supposed to be repeated a single time, '.
-                'and it has already been applied.'));
 
-          $rule_matches = false;
+          $result_code = HeraldRuleResult::RESULT_ALREADY_APPLIED;
+          $result = HeraldRuleResult::newFromResultCode($result_code);
+        } else if ($this->isForbidden($rule, $object)) {
+          $result_code = HeraldRuleResult::RESULT_OBJECT_STATE;
+          $result = HeraldRuleResult::newFromResultCode($result_code);
         } else {
-          if ($this->isForbidden($rule, $object)) {
+          $result = $this->getRuleMatchResult($rule, $object);
-            $this->newRuleTranscript($rule)
-              ->setResult(HeraldRuleTranscript::RESULT_FORBIDDEN)
-              ->setReason(
-                pht(
-                  'Object state is not compatible with rule.'));
-
-            $rule_matches = false;
-          } else {
-            $rule_matches = $this->doesRuleMatch($rule, $object);
-          }
         }
       } catch (HeraldRecursiveConditionsException $ex) {
-        $names = array();
+        $cycle_phids = array();
-        foreach ($this->stack as $rule_phid => $ignored) {
+
-          $names[] = '"'.$rules[$rule_phid]->getName().'"';
+        $stack = $this->getRuleStack();
+        foreach ($stack as $stack_rule) {
+          $cycle_phids[] = $stack_rule->getPHID();
         }
-        $names = implode(', ', $names);
+        // Add the rule which actually cycled to the list to make the
-        foreach ($this->stack as $rule_phid => $ignored) {
+        // result more clear when we show it to the user.
-          $this->newRuleTranscript($rules[$rule_phid])
+        $cycle_phids[] = $phid;
-            ->setResult(false)
+
-            ->setReason(
+        foreach ($stack as $stack_rule) {
-              pht(
+          if ($this->hasRuleResult($stack_rule)) {
-                "Rules %s are recursively dependent upon one another! ".
+            continue;
-                "Don't do this! You have formed an unresolvable cycle in the ".
-                "dependency graph!",
-                $names));
           }
-        $rule_matches = false;
+
+          $result_code = HeraldRuleResult::RESULT_RECURSION;
+          $result_data = array(
+            'cyclePHIDs' => $cycle_phids,
+          );
+
+          $result = HeraldRuleResult::newFromResultCode($result_code)
+            ->setResultData($result_data);
+          $this->setRuleResult($stack_rule, $result);
+        }
+
+        $result = $this->getRuleResult($rule);
+      } catch (HeraldRuleEvaluationException $ex) {
+        // When we encounter an evaluation exception, the condition which
+        // failed to evaluate is responsible for logging the details of the
+        // error.
+
+        $result_code = HeraldRuleResult::RESULT_EVALUATION_EXCEPTION;
+        $result = HeraldRuleResult::newFromResultCode($result_code);
       } catch (Exception $ex) {
         $caught = $ex;
       } catch (Throwable $ex) {
@@ -129,17 +206,26 @@ final class HeraldEngine extends Phobject {
       }
 
       if ($caught) {
-        $this->newRuleTranscript($rules[$phid])
+        // These exceptions are unexpected, and did not arise during rule
-          ->setResult(false)
+        // evaluation, so we're responsible for handling the details.
-          ->setReason(
+
-            pht(
+        $result_code = HeraldRuleResult::RESULT_EXCEPTION;
-              'Rule encountered an exception while evaluting.'));
+
-        $rule_matches = false;
+        $result_data = array(
+          'exception.class' => get_class($caught),
+          'exception.message' => $ex->getMessage(),
+        );
+
+        $result = HeraldRuleResult::newFromResultCode($result_code)
+          ->setResultData($result_data);
       }
 
-      $this->results[$phid] = $rule_matches;
+      if (!$this->hasRuleResult($rule)) {
+        $this->setRuleResult($rule, $result);
+      }
+      $result = $this->getRuleResult($rule);
 
-      if ($rule_matches) {
+      if ($result->getShouldApplyActions()) {
         foreach ($this->getRuleEffects($rule, $object) as $effect) {
           $effects[] = $effect;
         }
@@ -286,58 +372,46 @@ final class HeraldEngine extends Phobject {
   public function doesRuleMatch(
     HeraldRule $rule,
     HeraldAdapter $object) {
-
+    $result = $this->getRuleMatchResult($rule, $object);
-    $phid = $rule->getPHID();
+    return $result->getShouldApplyActions();
-
-    if (isset($this->results[$phid])) {
-      // If we've already evaluated this rule because another rule depends
-      // on it, we don't need to reevaluate it.
-      return $this->results[$phid];
   }
 
-    if (isset($this->stack[$phid])) {
+  private function getRuleMatchResult(
+    HeraldRule $rule,
+    HeraldAdapter $object) {
+
+    if ($this->hasRuleResult($rule)) {
+      // If we've already evaluated this rule because another rule depends
+      // on it, we don't need to reevaluate it.
+      return $this->getRuleResult($rule);
+    }
+
+    if ($this->hasRuleOnStack($rule)) {
       // We've recursed, fail all of the rules on the stack. This happens when
       // there's a dependency cycle with "Rule conditions match for rule ..."
       // conditions.
-      foreach ($this->stack as $rule_phid => $ignored) {
-        $this->results[$rule_phid] = false;
-      }
       throw new HeraldRecursiveConditionsException();
     }
-
+    $this->pushRuleStack($rule);
-    $this->stack[$phid] = true;
 
     $all = $rule->getMustMatchAll();
 
     $conditions = $rule->getConditions();
 
-    $result = null;
+    $result_code = null;
+    $result_data = array();
 
     $local_version = id(new HeraldRule())->getConfigVersion();
     if ($rule->getConfigVersion() > $local_version) {
-      $reason = pht(
+      $result_code = HeraldRuleResult::RESULT_VERSION;
-        'Rule could not be processed, it was created with a newer version '.
-        'of Herald.');
-      $result = false;
     } else if (!$conditions) {
-      $reason = pht(
+      $result_code = HeraldRuleResult::RESULT_EMPTY;
-        'Rule failed automatically because it has no conditions.');
-      $result = false;
     } else if (!$rule->hasValidAuthor()) {
-      $reason = pht(
+      $result_code = HeraldRuleResult::RESULT_OWNER;
-        'Rule failed automatically because its owner is invalid '.
-        'or disabled.');
-      $result = false;
     } else if (!$this->canAuthorViewObject($rule, $object)) {
-      $reason = pht(
+      $result_code = HeraldRuleResult::RESULT_VIEW_POLICY;
-        'Rule failed automatically because it is a personal rule and its '.
-        'owner can not see the object.');
-      $result = false;
     } else if (!$this->canRuleApplyToObject($rule, $object)) {
-      $reason = pht(
+      $result_code = HeraldRuleResult::RESULT_OBJECT_RULE;
-        'Rule failed automatically because it is an object rule which is '.
-        'not relevant for this object.');
-      $result = false;
     } else {
       foreach ($conditions as $condition) {
         $caught = null;
@@ -347,6 +421,10 @@ final class HeraldEngine extends Phobject {
             $rule,
             $condition,
             $object);
+        } catch (HeraldRuleEvaluationException $ex) {
+          throw $ex;
+        } catch (HeraldRecursiveConditionsException $ex) {
+          throw $ex;
         } catch (Exception $ex) {
           $caught = $ex;
         } catch (Throwable $ex) {
@@ -354,60 +432,59 @@ final class HeraldEngine extends Phobject {
         }
 
         if ($caught) {
-          throw $ex;
+          throw new HeraldRuleEvaluationException();
         }
 
         if (!$all && $match) {
-          $reason = pht('Any condition matched.');
+          $result_code = HeraldRuleResult::RESULT_ANY_MATCHED;
-          $result = true;
           break;
         }
 
         if ($all && !$match) {
-          $reason = pht('Not all conditions matched.');
+          $result_code = HeraldRuleResult::RESULT_ANY_FAILED;
-          $result = false;
           break;
         }
       }
 
-      if ($result === null) {
+      if ($result_code === null) {
         if ($all) {
-          $reason = pht('All conditions matched.');
+          $result_code = HeraldRuleResult::RESULT_ALL_MATCHED;
-          $result = true;
         } else {
-          $reason = pht('No conditions matched.');
+          $result_code = HeraldRuleResult::RESULT_ALL_FAILED;
-          $result = false;
         }
       }
     }
 
     // If this rule matched, and is set to run "if it did not match the last
-    // time", and we matched the last time, we're going to return a match in
+    // time", and we matched the last time, we're going to return a special
-    // the transcript but set a flag so we don't actually apply any effects.
+    // result code which records a match but doesn't actually apply effects.
 
     // We need the rule to match so that storage gets updated properly. If we
     // just pretend the rule didn't match it won't cause any effects (which
     // is correct), but it also won't set the "it matched" flag in storage,
     // so the next run after this one would incorrectly trigger again.
 
+    $result = HeraldRuleResult::newFromResultCode($result_code)
+      ->setResultData($result_data);
+
+    $should_apply = $result->getShouldApplyActions();
+
     $is_dry_run = $this->getDryRun();
-    if ($result && !$is_dry_run) {
+    if ($should_apply && !$is_dry_run) {
       $is_on_change = $rule->isRepeatOnChange();
       if ($is_on_change) {
         $did_apply = $rule->getRuleApplied($object->getPHID());
         if ($did_apply) {
-          $reason = pht(
+          // Replace the result with our modified result.
-            'This rule matched, but did not take any actions because it '.
+          $result_code = HeraldRuleResult::RESULT_LAST_MATCHED;
-            'is configured to act only if it did not match the last time.');
+          $result = HeraldRuleResult::newFromResultCode($result_code);
 
           $this->skipEffects[$rule->getID()] = true;
         }
       }
     }
 
-    $this->newRuleTranscript($rule)
+    $this->setRuleResult($rule, $result);
-      ->setResult($result)
-      ->setReason($reason);
 
     return $result;
   }
@@ -439,6 +516,9 @@ final class HeraldEngine extends Phobject {
       } else {
         $result_code = HeraldConditionResult::RESULT_FAILED;
       }
+    } catch (HeraldRecursiveConditionsException $ex) {
+      $result_code = HeraldConditionResult::RESULT_RECURSION;
+      $caught = $ex;
     } catch (HeraldInvalidConditionException $ex) {
       $result_code = HeraldConditionResult::RESULT_INVALID;
       $caught = $ex;

src/applications/herald/engine/exception/HeraldRuleEvaluationException.php

@@ -0,0 +1,3 @@
+<?php
+
+final class HeraldRuleEvaluationException extends Exception {}

src/applications/herald/storage/transcript/HeraldConditionResult.php

@@ -7,6 +7,7 @@ final class HeraldConditionResult
   const RESULT_FAILED = 'failed';
   const RESULT_OBJECT_STATE = 'object-state';
   const RESULT_INVALID = 'invalid';
+  const RESULT_RECURSION = 'recursion';
   const RESULT_EXCEPTION = 'exception';
   const RESULT_UNKNOWN = 'unknown';
 
@@ -22,7 +23,7 @@ final class HeraldConditionResult
     return ($this->getSpecificationProperty('match') === true);
   }
 
-  public function newDetailsView() {
+  public function newDetailsView(PhabricatorUser $viewer) {
     switch ($this->getResultCode()) {
       case self::RESULT_OBJECT_STATE:
         $reason = $this->getDataProperty('reason');
@@ -54,8 +55,6 @@ final class HeraldConditionResult
           phutil_tag('strong', array(), $error_class),
           phutil_escape_html_newlines($error_message));
         break;
-        $details = 'exception';
-        break;
       default:
         $details = null;
         break;
@@ -90,6 +89,12 @@ final class HeraldConditionResult
         'color.icon' => 'yellow',
         'name' => pht('Invalid'),
       ),
+      self::RESULT_RECURSION => array(
+        'match' => null,
+        'icon' => 'fa-exclamation-triangle',
+        'color.icon' => 'red',
+        'name' => pht('Recursion'),
+      ),
       self::RESULT_EXCEPTION => array(
         'match' => null,
         'icon' => 'fa-exclamation-triangle',

src/applications/herald/storage/transcript/HeraldRuleResult.php

@@ -0,0 +1,238 @@
+<?php
+
+final class HeraldRuleResult
+  extends HeraldTranscriptResult {
+
+  const RESULT_ANY_MATCHED = 'any-match';
+  const RESULT_ALL_MATCHED = 'all-match';
+  const RESULT_ANY_FAILED = 'any-failed';
+  const RESULT_ALL_FAILED = 'all-failed';
+  const RESULT_LAST_MATCHED = 'last-match';
+  const RESULT_VERSION = 'version';
+  const RESULT_EMPTY = 'empty';
+  const RESULT_OWNER = 'owner';
+  const RESULT_VIEW_POLICY = 'view-policy';
+  const RESULT_OBJECT_RULE = 'object-rule';
+  const RESULT_EXCEPTION = 'exception';
+  const RESULT_EVALUATION_EXCEPTION = 'evaluation-exception';
+  const RESULT_UNKNOWN = 'unknown';
+  const RESULT_ALREADY_APPLIED = 'already-applied';
+  const RESULT_OBJECT_STATE = 'object-state';
+  const RESULT_RECURSION = 'recursion';
+
+  public static function newFromResultCode($result_code) {
+    return id(new self())->setResultCode($result_code);
+  }
+
+  public static function newFromResultMap(array $map) {
+    return id(new self())->loadFromResultMap($map);
+  }
+
+  public function getShouldRecordMatch() {
+    return ($this->getSpecificationProperty('match') === true);
+  }
+
+  public function getShouldApplyActions() {
+    return ($this->getSpecificationProperty('apply') === true);
+  }
+
+  public function getDescription() {
+    return $this->getSpecificationProperty('description');
+  }
+
+  public function newDetailsView(PhabricatorUser $viewer) {
+    switch ($this->getResultCode()) {
+      case self::RESULT_EXCEPTION:
+        $error_class = $this->getDataProperty('exception.class');
+        $error_message = $this->getDataProperty('exception.message');
+
+        if (!strlen($error_class)) {
+          $error_class = pht('Unknown Error');
+        }
+
+        if (!strlen($error_message)) {
+          $error_message = pht(
+            'An unknown error occurred while evaluating this condition. No '.
+            'additional information is available.');
+        }
+
+        $details = $this->newErrorView($error_class, $error_message);
+        break;
+      case self::RESULT_RECURSION:
+        $rule_phids = $this->getDataProperty('cyclePHIDs', array());
+        $handles = $viewer->loadHandles($rule_phids);
+
+        $links = array();
+        foreach ($rule_phids as $rule_phid) {
+          $links[] = $handles[$rule_phid]->renderLink();
+        }
+
+        $links = phutil_implode_html(' > ', $links);
+
+        $details = array(
+          pht('This rule has a dependency cycle and can not be evaluated:'),
+          ' ',
+          $links,
+        );
+        break;
+      default:
+        $details = null;
+        break;
+    }
+
+    return $details;
+  }
+
+  protected function newResultSpecificationMap() {
+    return array(
+      self::RESULT_ANY_MATCHED => array(
+        'match' => true,
+        'apply' => true,
+        'name' => pht('Matched'),
+        'description' => pht('Any condition matched.'),
+        'icon' => 'fa-check-circle',
+        'color.icon' => 'green',
+      ),
+      self::RESULT_ALL_MATCHED => array(
+        'match' => true,
+        'apply' => true,
+        'name' => pht('Matched'),
+        'description' => pht('All conditions matched.'),
+        'icon' => 'fa-check-circle',
+        'color.icon' => 'green',
+      ),
+      self::RESULT_ANY_FAILED => array(
+        'match' => false,
+        'apply' => false,
+        'name' => pht('Failed'),
+        'description' => pht('Not all conditions matched.'),
+        'icon' => 'fa-times-circle',
+        'color.icon' => 'red',
+      ),
+      self::RESULT_ALL_FAILED => array(
+        'match' => false,
+        'apply' => false,
+        'name' => pht('Failed'),
+        'description' => pht('No conditions matched.'),
+        'icon' => 'fa-times-circle',
+        'color.icon' => 'red',
+      ),
+      self::RESULT_LAST_MATCHED => array(
+        'match' => true,
+        'apply' => false,
+        'name' => pht('Failed'),
+        'description' => pht(
+          'This rule matched, but did not take any actions because it '.
+          'is configured to act only if it did not match the last time.'),
+        'icon' => 'fa-times-circle',
+        'color.icon' => 'red',
+      ),
+      self::RESULT_VERSION => array(
+        'match' => null,
+        'apply' => false,
+        'name' => pht('Version Issue'),
+        'description' => pht(
+          'Rule could not be processed because it was created with a newer '.
+          'version of Herald.'),
+        'icon' => 'fa-times-circle',
+        'color.icon' => 'red',
+      ),
+      self::RESULT_EMPTY => array(
+        'match' => null,
+        'apply' => false,
+        'name' => pht('Empty'),
+        'description' => pht(
+          'Rule failed automatically because it has no conditions.'),
+        'icon' => 'fa-times-circle',
+        'color.icon' => 'red',
+      ),
+      self::RESULT_OWNER => array(
+        'match' => null,
+        'apply' => false,
+        'name' => pht('Rule Owner'),
+        'description' => pht(
+          'Rule failed automatically because it is a personal rule and '.
+          'its owner is invalid or disabled.'),
+        'icon' => 'fa-times-circle',
+        'color.icon' => 'red',
+      ),
+      self::RESULT_VIEW_POLICY => array(
+        'match' => null,
+        'apply' => false,
+        'name' => pht('View Policy'),
+        'description' => pht(
+          'Rule failed automatically because it is a personal rule and '.
+          'its owner does not have permission to view the object.'),
+        'icon' => 'fa-times-circle',
+        'color.icon' => 'red',
+      ),
+      self::RESULT_OBJECT_RULE => array(
+        'match' => null,
+        'apply' => false,
+        'name' => pht('Object Rule'),
+        'description' => pht(
+          'Rule failed automatically because it is an object rule which is '.
+          'not relevant for this object.'),
+        'icon' => 'fa-times-circle',
+        'color.icon' => 'red',
+      ),
+      self::RESULT_EXCEPTION => array(
+        'match' => null,
+        'apply' => false,
+        'name' => pht('Exception'),
+        'description' => pht(
+          'Rule failed because an exception occurred.'),
+        'icon' => 'fa-times-circle',
+        'color.icon' => 'red',
+      ),
+      self::RESULT_EVALUATION_EXCEPTION => array(
+        'match' => null,
+        'apply' => false,
+        'name' => pht('Exception'),
+        'description' => pht(
+          'Rule failed because an exception occurred while evaluating it.'),
+        'icon' => 'fa-times-circle',
+        'color.icon' => 'red',
+      ),
+      self::RESULT_UNKNOWN => array(
+        'match' => null,
+        'apply' => false,
+        'name' => pht('Unknown'),
+        'description' => pht(
+          'Rule evaluation result is unknown.'),
+        'icon' => 'fa-times-circle',
+        'color.icon' => 'red',
+      ),
+      self::RESULT_ALREADY_APPLIED => array(
+        'match' => null,
+        'apply' => false,
+        'name' => pht('Already Applied'),
+        'description' => pht(
+          'This rule is only supposed to be repeated a single time, '.
+          'and it has already been applied.'),
+        'icon' => 'fa-times-circle',
+        'color.icon' => 'red',
+      ),
+      self::RESULT_OBJECT_STATE => array(
+        'match' => null,
+        'apply' => false,
+        'name' => pht('Forbidden'),
+        'description' => pht(
+          'Object state prevented rule evaluation.'),
+        'icon' => 'fa-ban',
+        'color.icon' => 'indigo',
+      ),
+      self::RESULT_RECURSION => array(
+        'match' => null,
+        'apply' => false,
+        'name' => pht('Recursion'),
+        'description' => pht(
+          'This rule has a recursive dependency on itself and can not '.
+          'be evaluated.'),
+        'icon' => 'fa-times-circle',
+        'color.icon' => 'red',
+      ),
+    );
+  }
+
+}

src/applications/herald/storage/transcript/HeraldRuleTranscript.php

@@ -3,35 +3,18 @@
 final class HeraldRuleTranscript extends Phobject {
 
   protected $ruleID;
-  protected $result;
+  protected $ruleResultMap;
-  protected $reason;
-
   protected $ruleName;
   protected $ruleOwner;
 
-  const RESULT_FORBIDDEN = 'forbidden';
+  // See T13586. This no longer has readers, but was written by older versions
+  // of Herald. It contained a human readable English-language description of
+  // the outcome of rule evaluation and was superseded by "HeraldRuleResult".
+  protected $reason;
 
-  public function isForbidden() {
+  // See T13586. Older transcripts store a boolean "true", a boolean "false",
-    return ($this->getResult() === self::RESULT_FORBIDDEN);
+  // or the string "forbidden" here.
-  }
+  protected $result;
-
-  public function setResult($result) {
-    $this->result = $result;
-    return $this;
-  }
-
-  public function getResult() {
-    return $this->result;
-  }
-
-  public function setReason($reason) {
-    $this->reason = $reason;
-    return $this;
-  }
-
-  public function getReason() {
-    return $this->reason;
-  }
 
   public function setRuleID($rule_id) {
     $this->ruleID = $rule_id;
@@ -59,4 +42,40 @@ final class HeraldRuleTranscript extends Phobject {
   public function getRuleOwner() {
     return $this->ruleOwner;
   }
+
+  public function setRuleResult(HeraldRuleResult $result) {
+    $this->ruleResultMap = $result->newResultMap();
+    return $this;
+  }
+
+  public function getRuleResult() {
+    $map = $this->ruleResultMap;
+
+    if (is_array($map)) {
+      $result = HeraldRuleResult::newFromResultMap($map);
+    } else {
+      $legacy_result = $this->result;
+
+      $result_data = array();
+
+      if ($legacy_result === 'forbidden') {
+        $result_code = HeraldRuleResult::RESULT_OBJECT_STATE;
+        $result_data = array(
+          'reason' => $this->reason,
+        );
+      } else if ($legacy_result === true) {
+        $result_code = HeraldRuleResult::RESULT_ANY_MATCHED;
+      } else if ($legacy_result === false) {
+        $result_code = HeraldRuleResult::RESULT_ANY_FAILED;
+      } else {
+        $result_code = HeraldRuleResult::RESULT_UNKNOWN;
+      }
+
+      $result = HeraldRuleResult::newFromResultCode($result_code)
+        ->setResultData($result_data);
+    }
+
+    return $result;
+  }
+
 }

src/applications/herald/storage/transcript/HeraldTranscriptResult.php

@@ -47,9 +47,11 @@ abstract class HeraldTranscriptResult
     return $this->getSpecificationProperty('name');
   }
 
-  final protected function getDataProperty($key) {
+  abstract public function newDetailsView(PhabricatorUser $viewer);
+
+  final protected function getDataProperty($key, $default = null) {
     $data = $this->getResultData();
-    return idx($data, $key);
+    return idx($data, $key, $default);
   }
 
   final public function newResultMap() {
@@ -79,4 +81,11 @@ abstract class HeraldTranscriptResult
 
   abstract protected function newResultSpecificationMap();
 
+  final protected function newErrorView($error_class, $error_message) {
+    return pht(
+      '%s: %s',
+      phutil_tag('strong', array(), $error_class),
+      phutil_escape_html_newlines($error_message));
+  }
+
 }