Provide a more structured result log for Herald conditions

Summary:
Ref T13586. Currently, Herald condition logs encode "pass" or "fail" robustly, "forbidden" through a sort of awkward side channel, and can not properly encode "invalid" or "exception" outcomes.

Structure the condition log so results are represented unambiguously and all possible outcomes (pass, fail, forbidden, invalid, exception) are clearly encoded.

Test Plan:
{F8446102}

{F8446103}

Maniphest Tasks: T13586

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

resources/celerity/map.php

@@ -78,7 +78,7 @@ return array(
     'rsrc/css/application/files/global-drag-and-drop.css' => '1d2713a4',
     'rsrc/css/application/flag/flag.css' => '2b77be8d',
     'rsrc/css/application/harbormaster/harbormaster.css' => '8dfe16b2',
-    'rsrc/css/application/herald/herald-test.css' => 'e004176f',
+    'rsrc/css/application/herald/herald-test.css' => '7e7bbdae',
     'rsrc/css/application/herald/herald.css' => '648d39e2',
     'rsrc/css/application/maniphest/report.css' => '3d53188b',
     'rsrc/css/application/maniphest/task-edit.css' => '272daa84',
@@ -587,7 +587,7 @@ return array(
     'harbormaster-css' => '8dfe16b2',
     'herald-css' => '648d39e2',
     'herald-rule-editor' => '2633bef7',
-    'herald-test-css' => 'e004176f',
+    'herald-test-css' => '7e7bbdae',
     'inline-comment-summary-css' => '81eb368d',
     'javelin-aphlict' => '022516b4',
     'javelin-behavior' => '1b6acc2a',

src/__phutil_library_map__.php

@@ -1566,6 +1566,7 @@ phutil_register_library_map(array(
     'HeraldCommentContentField' => 'applications/herald/field/HeraldCommentContentField.php',
     'HeraldCommitAdapter' => 'applications/diffusion/herald/HeraldCommitAdapter.php',
     'HeraldCondition' => 'applications/herald/storage/HeraldCondition.php',
+    'HeraldConditionResult' => 'applications/herald/storage/transcript/HeraldConditionResult.php',
     'HeraldConditionTranscript' => 'applications/herald/storage/transcript/HeraldConditionTranscript.php',
     'HeraldContentSourceField' => 'applications/herald/field/HeraldContentSourceField.php',
     'HeraldController' => 'applications/herald/controller/HeraldController.php',
@@ -7793,6 +7794,7 @@ phutil_register_library_map(array(
       'HarbormasterBuildableAdapterInterface',
     ),
     'HeraldCondition' => 'HeraldDAO',
+    'HeraldConditionResult' => 'Phobject',
     'HeraldConditionTranscript' => 'Phobject',
     'HeraldContentSourceField' => 'HeraldField',
     'HeraldController' => 'PhabricatorController',

src/applications/herald/adapter/HeraldAdapter.php

@@ -520,21 +520,29 @@ abstract class HeraldAdapter extends Phobject {
       case self::CONDITION_NOT_REGEXP:
         $result_if_match = ($condition_type == self::CONDITION_REGEXP);
 
-        foreach ((array)$field_value as $value) {
         // We add the 'S' flag because we use the regexp multiple times.
         // It shouldn't cause any troubles if the flag is already there
         // - /.*/S is evaluated same as /.*/SS.
-          $result = @preg_match($condition_value.'S', $value);
+        $condition_pattern = $condition_value.'S';
-          if ($result === false) {
+
-            throw new HeraldInvalidConditionException(
+        foreach ((array)$field_value as $value) {
-              pht(
+          try {
+            $result = phutil_preg_match($condition_pattern, $value);
+          } catch (PhutilRegexException $ex) {
+            $message = array();
+            $message[] = pht(
               'Regular expression "%s" in Herald rule "%s" is not valid, '.
               'or exceeded backtracking or recursion limits while '.
               'executing. Verify the expression and correct it or rewrite '.
               'it with less backtracking.',
               $condition_value,
-                $rule->getMonogram()));
+              $rule->getMonogram());
+            $message[] = $ex->getMessage();
+            $message = implode("\n\n", $message);
+
+            throw new HeraldInvalidConditionException($message);
           }
+
           if ($result) {
             return $result_if_match;
           }

src/applications/herald/controller/HeraldTranscriptController.php

@@ -269,34 +269,20 @@ final class HeraldTranscriptController extends HeraldController {
           ->setTarget(phutil_tag('strong', array(), pht('Conditions'))));
 
       foreach ($cond_xscripts as $cond_xscript) {
-        if ($cond_xscript->isForbidden()) {
+        $result = $cond_xscript->getResult();
-          $icon = 'fa-ban';
-          $color = 'indigo';
-          $result = pht('Forbidden');
-        } else if ($cond_xscript->getResult()) {
-          $icon = 'fa-check';
-          $color = 'green';
-          $result = pht('Passed');
-        } else {
-          $icon = 'fa-times';
-          $color = 'red';
-          $result = pht('Failed');
-        }
 
-        if ($cond_xscript->getNote()) {
+        $icon = $result->getIconIcon();
-          $note_text = $cond_xscript->getNote();
+        $color = $result->getIconColor();
-          if ($cond_xscript->isForbidden()) {
+        $name = $result->getName();
-            $note_text = HeraldStateReasons::getExplanation($note_text);
-          }
 
-          $note = phutil_tag(
+        $result_details = $result->newDetailsView();
+        if ($result_details !== null) {
+          $result_details = phutil_tag(
             'div',
             array(
               'class' => 'herald-condition-note',
             ),
-            $note_text);
+            $result_details);
-        } else {
-          $note = null;
         }
 
         // TODO: This is not really translatable and should be driven through
@@ -309,8 +295,8 @@ final class HeraldTranscriptController extends HeraldController {
 
         $cond_item = id(new PHUIStatusItemView())
           ->setIcon($icon, $color)
-          ->setTarget($result)
+          ->setTarget($name)
-          ->setNote(array($explanation, $note));
+          ->setNote(array($explanation, $result_details));
 
         $cond_list->addItem($cond_item);
       }

src/applications/herald/engine/HeraldEngine.php

@@ -8,7 +8,8 @@ final class HeraldEngine extends Phobject {
   protected $activeRule;
   protected $transcript;
 
-  protected $fieldCache = array();
+  private $fieldCache = array();
+  private $fieldExceptions = array();
   protected $object;
   private $dryRun;
 
@@ -64,6 +65,7 @@ final class HeraldEngine extends Phobject {
     $this->transcript = new HeraldTranscript();
     $this->transcript->setObjectPHID((string)$object->getPHID());
     $this->fieldCache = array();
+    $this->fieldExceptions = array();
     $this->results = array();
     $this->rules   = $rules;
     $this->object  = $object;
@@ -74,6 +76,7 @@ final class HeraldEngine extends Phobject {
 
       $is_first_only = $rule->isRepeatFirst();
 
+      $caught = null;
       try {
         if (!$this->getDryRun() &&
             $is_first_only &&
@@ -119,7 +122,21 @@ final class HeraldEngine extends Phobject {
                 $names));
         }
         $rule_matches = false;
+      } catch (Exception $ex) {
+        $caught = $ex;
+      } catch (Throwable $ex) {
+        $caught = $ex;
       }
+
+      if ($caught) {
+        $this->newRuleTranscript($rules[$phid])
+          ->setResult(false)
+          ->setReason(
+            pht(
+              'Rule encountered an exception while evaluting.'));
+        $rule_matches = false;
+      }
+
       $this->results[$phid] = $rule_matches;
 
       if ($rule_matches) {
@@ -323,42 +340,18 @@ final class HeraldEngine extends Phobject {
       $result = false;
     } else {
       foreach ($conditions as $condition) {
-        try {
-          $this->getConditionObjectValue($condition, $object);
-        } catch (Exception $ex) {
-          $reason = pht(
-            'Field "%s" does not exist!',
-            $condition->getFieldName());
-          $result = false;
-          break;
-        }
-
-        // Here, we're profiling the cost to match the condition value against
-        // whatever test is configured. Normally, this cost should be very
-        // small (<<1ms) since it amounts to a single comparison:
-        //
-        //   [ Task author ][ is any of ][ alice ]
-        //
-        // However, it may be expensive in some cases, particularly if you
-        // write a rule with a very creative regular expression that backtracks
-        // explosively.
-        //
-        // At time of writing, the "Another Herald Rule" field is also
-        // evaluated inside the matching function. This may be arbitrarily
-        // expensive (it can prompt us to execute any finite number of other
-        // Herald rules), although we'll push the profiler stack appropriately
-        // so we don't count the evaluation time against this rule in the final
-        // profile.
-
         $caught = null;
 
-        $this->pushProfilerRule($rule);
         try {
-          $match = $this->doesConditionMatch($rule, $condition, $object);
+          $match = $this->doesConditionMatch(
+            $rule,
+            $condition,
+            $object);
         } catch (Exception $ex) {
           $caught = $ex;
+        } catch (Throwable $ex) {
+          $caught = $ex;
         }
-        $this->popProfilerRule($rule);
 
         if ($caught) {
           throw $ex;
@@ -419,63 +412,176 @@ final class HeraldEngine extends Phobject {
     return $result;
   }
 
-  protected function doesConditionMatch(
+  private function doesConditionMatch(
     HeraldRule $rule,
     HeraldCondition $condition,
-    HeraldAdapter $object) {
+    HeraldAdapter $adapter) {
 
-    $object_value = $this->getConditionObjectValue($condition, $object);
     $transcript = $this->newConditionTranscript($rule, $condition);
 
+    $caught = null;
+    $result_data = array();
+
     try {
-      $result = $object->doesConditionMatch(
+      $field_key = $condition->getFieldName();
-        $this,
+
+      $field_value = $this->getProfiledObjectFieldValue(
+        $adapter,
+        $field_key);
+
+      $is_match = $this->getProfiledConditionMatch(
+        $adapter,
         $rule,
         $condition,
-        $object_value);
+        $field_value);
+      if ($is_match) {
+        $result_code = HeraldConditionResult::RESULT_MATCHED;
+      } else {
+        $result_code = HeraldConditionResult::RESULT_FAILED;
+      }
     } catch (HeraldInvalidConditionException $ex) {
-      $result = false;
+      $result_code = HeraldConditionResult::RESULT_INVALID;
-      $transcript->setNote($ex->getMessage());
+      $caught = $ex;
-    }
-
-    $transcript->setResult($result);
-
-    return $result;
-  }
-
-  protected function getConditionObjectValue(
-    HeraldCondition $condition,
-    HeraldAdapter $object) {
-
-    $field = $condition->getFieldName();
-
-    return $this->getObjectFieldValue($field);
-  }
-
-  public function getObjectFieldValue($field) {
-    if (!array_key_exists($field, $this->fieldCache)) {
-      $adapter = $this->object;
-
-      $adapter->willGetHeraldField($field);
-
-      $caught = null;
-
-      $this->pushProfilerField($field);
-      try {
-        $value = $adapter->getHeraldField($field);
     } catch (Exception $ex) {
+      $result_code = HeraldConditionResult::RESULT_EXCEPTION;
+      $caught = $ex;
+    } catch (Throwable $ex) {
+      $result_code = HeraldConditionResult::RESULT_EXCEPTION;
       $caught = $ex;
     }
-      $this->popProfilerField($field);
+
+    if ($caught) {
+      $result_data = array(
+        'exception.class' => get_class($caught),
+        'exception.message' => $ex->getMessage(),
+      );
+    }
+
+    $result = HeraldConditionResult::newFromResultCode($result_code)
+      ->setResultData($result_data);
+
+    $transcript->setResult($result);
 
     if ($caught) {
       throw $caught;
     }
 
-      $this->fieldCache[$field] = $value;
+    return $result->getIsMatch();
   }
 
-    return $this->fieldCache[$field];
+  private function getProfiledConditionMatch(
+    HeraldAdapter $adapter,
+    HeraldRule $rule,
+    HeraldCondition $condition,
+    $field_value) {
+
+    // Here, we're profiling the cost to match the condition value against
+    // whatever test is configured. Normally, this cost should be very
+    // small (<<1ms) since it amounts to a single comparison:
+    //
+    //   [ Task author ][ is any of ][ alice ]
+    //
+    // However, it may be expensive in some cases, particularly if you
+    // write a rule with a very creative regular expression that backtracks
+    // explosively.
+    //
+    // At time of writing, the "Another Herald Rule" field is also
+    // evaluated inside the matching function. This may be arbitrarily
+    // expensive (it can prompt us to execute any finite number of other
+    // Herald rules), although we'll push the profiler stack appropriately
+    // so we don't count the evaluation time against this rule in the final
+    // profile.
+
+    $this->pushProfilerRule($rule);
+
+    $caught = null;
+    try {
+      $is_match = $adapter->doesConditionMatch(
+        $this,
+        $rule,
+        $condition,
+        $field_value);
+    } catch (Exception $ex) {
+      $caught = $ex;
+    } catch (Throwable $ex) {
+      $caught = $ex;
+    }
+
+    $this->popProfilerRule($rule);
+
+    if ($caught) {
+      throw $caught;
+    }
+
+    return $is_match;
+  }
+
+  private function getProfiledObjectFieldValue(
+    HeraldAdapter $adapter,
+    $field_key) {
+
+    // Before engaging the profiler, make sure the field class is loaded.
+
+    $adapter->willGetHeraldField($field_key);
+
+    // The first time we read a field value, we'll actually generate it, which
+    // may be slow.
+
+    // After it is generated for the first time, this will just read it from a
+    // cache, which should be very fast.
+
+    // We still want to profile the request even if it goes to cache so we can
+    // get an accurate count of how many times we access the field value: when
+    // trying to improve the performance of Herald rules, it's helpful to know
+    // how many rules rely on the value of a field which is slow to generate.
+
+    $caught = null;
+
+    $this->pushProfilerField($field_key);
+    try {
+      $value = $this->getObjectFieldValue($field_key);
+    } catch (Exception $ex) {
+      $caught = $ex;
+    } catch (Throwable $ex) {
+      $caught = $ex;
+    }
+    $this->popProfilerField($field_key);
+
+    if ($caught) {
+      throw $caught;
+    }
+
+    return $value;
+  }
+
+  private function getObjectFieldValue($field_key) {
+    if (array_key_exists($field_key, $this->fieldExceptions)) {
+      throw $this->fieldExceptions[$field_key];
+    }
+
+    if (array_key_exists($field_key, $this->fieldCache)) {
+      return $this->fieldCache[$field_key];
+    }
+
+    $adapter = $this->object;
+
+    $caught = null;
+    try {
+      $value = $adapter->getHeraldField($field_key);
+    } catch (Exception $ex) {
+      $caught = $ex;
+    } catch (Throwable $ex) {
+      $caught = $ex;
+    }
+
+    if ($caught) {
+      $this->fieldExceptions[$field_key] = $caught;
+      throw $caught;
+    }
+
+    $this->fieldCache[$field_key] = $value;
+
+    return $value;
   }
 
   protected function getRuleEffects(
@@ -639,9 +745,16 @@ final class HeraldEngine extends Phobject {
 
       $forbidden_reason = $this->forbiddenFields[$field_key];
       if ($forbidden_reason !== null) {
+        $result_code = HeraldConditionResult::RESULT_OBJECT_STATE;
+        $result_data = array(
+          'reason' => $forbidden_reason,
+        );
+
+        $result = HeraldConditionResult::newFromResultCode($result_code)
+          ->setResultData($result_data);
+
         $this->newConditionTranscript($rule, $condition)
-          ->setResult(HeraldConditionTranscript::RESULT_FORBIDDEN)
+          ->setResult($result);
-          ->setNote($forbidden_reason);
 
         $is_forbidden = true;
       }

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

@@ -0,0 +1,177 @@
+<?php
+
+final class HeraldConditionResult
+  extends Phobject {
+
+  const RESULT_MATCHED = 'matched';
+  const RESULT_FAILED = 'failed';
+  const RESULT_OBJECT_STATE = 'object-state';
+  const RESULT_INVALID = 'invalid';
+  const RESULT_EXCEPTION = 'exception';
+  const RESULT_UNKNOWN = 'unknown';
+
+  private $resultCode;
+  private $resultData = array();
+
+  public function toMap() {
+    return array(
+      'code' => $this->getResultCode(),
+      'data' => $this->getResultData(),
+    );
+  }
+
+  public static function newFromMap(array $map) {
+    $result_code = idx($map, 'code');
+    $result = self::newFromResultCode($result_code);
+
+    $result_data = idx($map, 'data', array());
+    $result->setResultData($result_data);
+
+    return $result;
+  }
+
+  public static function newFromResultCode($result_code) {
+    $map = self::getResultSpecification($result_code);
+
+    $result = new self();
+    $result->resultCode = $result_code;
+
+    return $result;
+  }
+
+  public function getResultCode() {
+    return $this->resultCode;
+  }
+
+  private function getResultData() {
+    return $this->resultData;
+  }
+
+  public function getIconIcon() {
+    return $this->getSpecificationProperty('icon');
+  }
+
+  public function getIconColor() {
+    return $this->getSpecificationProperty('color.icon');
+  }
+
+  public function getIsMatch() {
+    return ($this->getSpecificationProperty('match') === true);
+  }
+
+  public function getName() {
+    return $this->getSpecificationProperty('name');
+  }
+
+  public function newDetailsView() {
+    switch ($this->resultCode) {
+      case self::RESULT_OBJECT_STATE:
+        $reason = $this->getDataProperty('reason');
+        $details = HeraldStateReasons::getExplanation($reason);
+        break;
+      case self::RESULT_INVALID:
+      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');
+        }
+
+        switch ($error_class) {
+          case 'HeraldInvalidConditionException':
+            $error_class = pht('Invalid Condition');
+            break;
+        }
+
+        if (!strlen($error_message)) {
+          $error_message = pht(
+            'An unknown error occurred while evaluating this condition. No '.
+            'additional information is available.');
+        }
+
+        $details = pht(
+          '%s: %s',
+          phutil_tag('strong', array(), $error_class),
+          phutil_escape_html_newlines($error_message));
+        break;
+        $details = 'exception';
+        break;
+      default:
+        $details = null;
+        break;
+    }
+
+    return $details;
+  }
+
+  public function setResultData(array $result_data) {
+    $this->resultData = $result_data;
+    return $this;
+  }
+
+  private function getDataProperty($key) {
+    $data = $this->getResultData();
+    return idx($data, $key);
+  }
+
+  private function getSpecificationProperty($key) {
+    $map = self::getResultSpecification($this->resultCode);
+    return $map[$key];
+  }
+
+  private static function getResultSpecification($result_code) {
+    $map = self::getResultSpecificationMap();
+
+    if (!isset($map[$result_code])) {
+      throw new Exception(
+        pht(
+          'Condition result "%s" is unknown.',
+          $result_code));
+    }
+
+    return $map[$result_code];
+  }
+
+  private static function getResultSpecificationMap() {
+    return array(
+      self::RESULT_MATCHED => array(
+        'match' => true,
+        'icon' => 'fa-check',
+        'color.icon' => 'green',
+        'name' => pht('Passed'),
+      ),
+      self::RESULT_FAILED => array(
+        'match' => false,
+        'icon' => 'fa-times',
+        'color.icon' => 'red',
+        'name' => pht('Failed'),
+      ),
+      self::RESULT_OBJECT_STATE => array(
+        'match' => null,
+        'icon' => 'fa-ban',
+        'color.icon' => 'indigo',
+        'name' => pht('Forbidden'),
+      ),
+      self::RESULT_INVALID => array(
+        'match' => null,
+        'icon' => 'fa-exclamation-triangle',
+        'color.icon' => 'yellow',
+        'name' => pht('Invalid'),
+      ),
+      self::RESULT_EXCEPTION => array(
+        'match' => null,
+        'icon' => 'fa-exclamation-triangle',
+        'color.icon' => 'red',
+        'name' => pht('Exception'),
+      ),
+      self::RESULT_UNKNOWN => array(
+        'match' => null,
+        'icon' => 'fa-question',
+        'color.icon' => 'grey',
+        'name' => pht('Unknown'),
+      ),
+    );
+  }
+
+}

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

@@ -7,10 +7,17 @@ final class HeraldConditionTranscript extends Phobject {
   protected $fieldName;
   protected $condition;
   protected $testValue;
-  protected $note;
+  protected $resultMap;
-  protected $result;
 
-  const RESULT_FORBIDDEN = 'forbidden';
+  // See T13586. Older versions of this record stored a boolean true, boolean
+  // false, or the string "forbidden" in the "$result" field. They stored a
+  // human-readable English-language message or a state code in the "$note"
+  // field.
+
+  // The modern record does not use either field.
+
+  protected $result;
+  protected $note;
 
   public function setRuleID($rule_id) {
     $this->ruleID = $rule_id;
@@ -57,26 +64,39 @@ final class HeraldConditionTranscript extends Phobject {
     return $this->testValue;
   }
 
-  public function setNote($note) {
+  public function setResult(HeraldConditionResult $result) {
-    $this->note = $note;
+    $this->resultMap = $result->toMap();
-    return $this;
-  }
-
-  public function getNote() {
-    return $this->note;
-  }
-
-  public function setResult($result) {
-    $this->result = $result;
     return $this;
   }
 
   public function getResult() {
-    return $this->result;
+    $map = $this->resultMap;
+
+    if (is_array($map)) {
+      $result = HeraldConditionResult::newFromMap($map);
+    } else {
+      $legacy_result = $this->result;
+
+      $result_data = array();
+
+      if ($legacy_result === 'forbidden') {
+        $result_code = HeraldConditionResult::RESULT_OBJECT_STATE;
+        $result_data = array(
+          'reason' => $this->note,
+        );
+      } else if ($legacy_result === true) {
+        $result_code = HeraldConditionResult::RESULT_MATCHED;
+      } else if ($legacy_result === false) {
+        $result_code = HeraldConditionResult::RESULT_FAILED;
+      } else {
+        $result_code = HeraldConditionResult::RESULT_UNKNOWN;
       }
 
-  public function isForbidden() {
+      $result = HeraldConditionResult::newFromResultCode($result_code)
-    return ($this->getResult() === self::RESULT_FORBIDDEN);
+        ->setResultData($result_data);
+    }
+
+    return $result;
   }
 
 }

webroot/rsrc/css/application/herald/herald-test.css

@@ -4,6 +4,8 @@
 
 .herald-condition-note {
   color: {$red};
+  padding: 4px 0;
+  margin: 4px 0 8px;
 }
 
 textarea.herald-field-value-transcript {