Test for unusual task data (#3770)

* Test for unusual task data

A task might have any combination of keys and values, but Taskwarrior
often assumes that only valid values can occur, and crashes otherwise.
This is highly inconvenient, as it's often impossible to do anything
with the invalid task -- Taskwarrior just fails without modifying it.

So, this is the beginning of some testing for such invalid tasks, with
the goal of making Taskwarrior due something reasonable. In general, an
invalid attribute value is treated as if it was not set. This is not
exhaustive, and there are likely still bugs of this sort, but as we find
them we can fix and add regression tests to this script.

This introduces a new test-only binary that creates a "bare" task using
TaskChampion, avoiding Taskwarrior's efforts to not create "unusual"
tasks.

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

---------

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>

Author: djmitche

Diff for: src/TDB2.cpp

@@ -57,10 +57,6 @@ void TDB2::open_replica(const std::string& location, bool create_if_missing) {
 ////////////////////////////////////////////////////////////////////////////////
 // Add the new task to the replica.
 void TDB2::add(Task& task) {
-  // Validate a task for addition. This is stricter than `task.validate`, as any
-  // inconsistency is probably user error.
-  task.validate_add();
-
   // Ensure the task is consistent, and provide defaults if necessary.
   // bool argument to validate() is "applyDefault", to apply default values for
   // properties not otherwise given.

Diff for: src/Task.cpp

@@ -321,8 +321,8 @@ void Task::setStatus(Task::status status) {
 ////////////////////////////////////////////////////////////////////////////////
 // Determines status of a date attribute.
 Task::dateState Task::getDateState(const std::string& name) const {
-  std::string value = get(name);
-  if (value.length()) {
+  time_t value = get_date(name);
+  if (value > 0) {
     Datetime reference(value);
     Datetime now;
     Datetime today("today");
@@ -804,13 +804,16 @@ std::string Task::composeJSON(bool decorate /*= false*/) const {
 
     // Date fields are written as ISO 8601.
     if (type == "date") {
-      Datetime d(i.second);
-      out << '"' << (i.first == "modification" ? "modified" : i.first)
-          << "\":\""
-          // Date was deleted, do not export parsed empty string
-          << (i.second == "" ? "" : d.toISO()) << '"';
-
-      ++attributes_written;
+      time_t epoch = get_date(i.first);
+      if (epoch != 0) {
+        Datetime d(i.second);
+        out << '"' << (i.first == "modification" ? "modified" : i.first)
+            << "\":\""
+            // Date was deleted, do not export parsed empty string
+            << (i.second == "" ? "" : d.toISO()) << '"';
+
+        ++attributes_written;
+      }
     }
 
     /*

Diff for: src/commands/CmdAdd.cpp

@@ -56,6 +56,11 @@ int CmdAdd::execute(std::string& output) {
   // the task is empty, but DOM references can refer to earlier parts of the
   // command line, e.g., `task add due:20110101 wait:due`.
   task.modify(Task::modReplace, true);
+
+  // Validate a task for addition. This is stricter than `task.validate`, as any
+  // inconsistency is probably user error.
+  task.validate_add();
+
   Context::getContext().tdb2.add(task);
 
   // Do not display ID 0, users cannot query by that

Diff for: src/commands/CmdInfo.cpp

@@ -120,9 +120,11 @@ int CmdInfo::execute(std::string& output) {
       description += '\n' + std::string(indent, ' ') +
                      Datetime(anno.first.substr(11)).toString(dateformatanno) + ' ' + anno.second;
 
-    row = view.addRow();
-    view.set(row, 0, "Description");
-    view.set(row, 1, description, c);
+    if (task.has("description")) {
+      row = view.addRow();
+      view.set(row, 0, "Description");
+      view.set(row, 1, description, c);
+    }
 
     // status
     row = view.addRow();
@@ -215,64 +217,76 @@ int CmdInfo::execute(std::string& output) {
     }
 
     // entry
-    row = view.addRow();
-    view.set(row, 0, "Entered");
-    Datetime dt(task.get_date("entry"));
-    std::string entry = dt.toString(dateformat);
-
-    std::string age;
-    auto created = task.get("entry");
-    if (created.length()) {
-      Datetime dt(strtoll(created.c_str(), nullptr, 10));
-      age = Duration(now - dt).formatVague();
+    if (task.has("entry") && task.get_date("entry")) {
+      row = view.addRow();
+      view.set(row, 0, "Entered");
+      Datetime dt(task.get_date("entry"));
+      std::string entry = dt.toString(dateformat);
+
+      std::string age;
+      auto created = task.get("entry");
+      if (created.length()) {
+        Datetime dt(strtoll(created.c_str(), nullptr, 10));
+        age = Duration(now - dt).formatVague();
+      }
+
+      view.set(row, 1, entry + " (" + age + ')');
     }
 
-    view.set(row, 1, entry + " (" + age + ')');
+    auto validDate = [&](const char* prop) {
+      if (!task.has(prop)) {
+        return false;
+      }
+      if (task.get_date(prop) == 0) {
+        return false;
+      }
+      return true;
+    };
 
     // wait
-    if (task.has("wait")) {
+    if (validDate("wait")) {
       row = view.addRow();
       view.set(row, 0, "Waiting until");
       view.set(row, 1, Datetime(task.get_date("wait")).toString(dateformat));
     }
 
     // scheduled
-    if (task.has("scheduled")) {
+    if (validDate("scheduled")) {
       row = view.addRow();
       view.set(row, 0, "Scheduled");
       view.set(row, 1, Datetime(task.get_date("scheduled")).toString(dateformat));
     }
 
     // start
-    if (task.has("start")) {
+    if (validDate("start")) {
       row = view.addRow();
       view.set(row, 0, "Start");
       view.set(row, 1, Datetime(task.get_date("start")).toString(dateformat));
     }
 
     // due (colored)
-    if (task.has("due")) {
+    if (validDate("due")) {
       row = view.addRow();
       view.set(row, 0, "Due");
       view.set(row, 1, Datetime(task.get_date("due")).toString(dateformat));
     }
 
     // end
-    if (task.has("end")) {
+    if (validDate("end")) {
       row = view.addRow();
       view.set(row, 0, "End");
       view.set(row, 1, Datetime(task.get_date("end")).toString(dateformat));
     }
 
     // until
-    if (task.has("until")) {
+    if (validDate("until")) {
       row = view.addRow();
       view.set(row, 0, "Until");
       view.set(row, 1, Datetime(task.get_date("until")).toString(dateformat));
     }
 
     // modified
-    if (task.has("modified")) {
+    if (validDate("modified")) {
       row = view.addRow();
       view.set(row, 0, "Last modified");
 
@@ -632,7 +646,11 @@ std::optional<std::string> CmdInfo::formatForInfo(const std::vector<Operation>&
       } else {
         // Record the last start time for later duration calculation.
         if (prop == "start") {
-          last_start = Datetime(value.value()).toEpoch();
+          try {
+            last_start = Datetime(value.value()).toEpoch();
+          } catch (std::string) {
+            // ignore invalid dates
+          }
         }
 
         out << format("{1} set to '{2}'.", Lexer::ucFirst(prop),

Diff for: src/commands/CmdModify.cpp

@@ -119,7 +119,7 @@ void CmdModify::checkConsistency(Task &before, Task &after) {
     throw std::string("You cannot remove the recurrence from a recurring task.");
 
   if ((before.getStatus() == Task::pending) && (after.getStatus() == Task::pending) &&
-      (after.get("end") != ""))
+      (before.get("end") == "") && (after.get("end") != ""))
     throw format("Could not modify task {1}. You cannot set an end date on a pending task.",
                  before.identifier(true));
 }

Diff for: src/feedback.cpp

@@ -51,7 +51,12 @@ std::string renderAttribute(const std::string& name, const std::string& value,
   if (Context::getContext().columns.find(name) != Context::getContext().columns.end()) {
     Column* col = Context::getContext().columns[name];
     if (col && col->type() == "date" && value != "") {
-      Datetime d((time_t)strtoll(value.c_str(), nullptr, 10));
+      int64_t epoch = strtoll(value.c_str(), nullptr, 10);
+      // Do not try to render an un-parseable value.
+      if (epoch == 0) {
+        return value;
+      }
+      Datetime d((time_t)epoch);
       if (format == "") return d.toString(Context::getContext().config.get("dateformat"));
 
       return d.toString(format);

Diff for: src/recur.cpp

@@ -288,9 +288,11 @@ std::optional<Datetime> getNextRecurrence(Datetime& current, std::string& period
   else if (unicodeLatinDigit(period[0]) && period[period.length() - 1] == 'q') {
     int increment = strtol(period.substr(0, period.length() - 1).c_str(), nullptr, 10);
 
-    if (increment <= 0)
-      throw format("Recurrence period '{1}' is equivalent to {2} and hence invalid.", period,
-                   increment);
+    if (increment <= 0) {
+      Context::getContext().footnote(format(
+          "Recurrence period '{1}' is equivalent to {2} and hence invalid.", period, increment));
+      return std::nullopt;
+    }
 
     m += 3 * increment;
     while (m > 12) {
@@ -346,8 +348,11 @@ std::optional<Datetime> getNextRecurrence(Datetime& current, std::string& period
   // Add the period to current, and we're done.
   std::string::size_type idx = 0;
   Duration p;
-  if (!p.parse(period, idx))
-    throw std::string(format("The recurrence value '{1}' is not valid.", period));
+  if (!p.parse(period, idx)) {
+    Context::getContext().footnote(
+        format("Warning: The recurrence value '{1}' is not valid.", period));
+    return std::nullopt;
+  }
 
   return checked_add_datetime(current, p.toTime_t());
 }

Diff for: test/CMakeLists.txt

@@ -188,6 +188,7 @@ set (pythonTests
   undo.test.py
   unicode.test.py
   unique.test.py
+  unusual_task.test.py
   upgrade.test.py
   urgency.test.py
   urgency_inherit.test.py
@@ -207,6 +208,15 @@ foreach (python_Test ${pythonTests})
     )
 endforeach(python_Test)
 
+# Create a `make_tc_task` binary, used for unusual_task.test.py. In order to build this
+# for the tests, it's added as a dependency of `test_runner`.
+add_executable (make_tc_task make_tc_task.cpp)
+target_link_libraries (make_tc_task task commands columns libshared task commands columns libshared task commands columns libshared ${TASK_LIBRARIES})
+if (DARWIN)
+  target_link_libraries (make_tc_task "-framework CoreFoundation -framework Security -framework SystemConfiguration")
+endif (DARWIN)
+add_dependencies(test_runner make_tc_task)
+
 # -- Shell tests
 
 set (shell_SRCS

Diff for: test/basetest/utils.py

@@ -28,8 +28,11 @@
 # Directory relative to basetest module location
 CURRENT_DIR = os.path.dirname(os.path.abspath(__file__))
 
+# From the CMAKE value of the same name. This is substituted at configure.
+CMAKE_BINARY_DIR = os.path.abspath("${CMAKE_BINARY_DIR}")
+
 # Location of binary files (usually the src/ folder)
-BIN_PREFIX = os.path.abspath(os.path.join("${CMAKE_BINARY_DIR}", "src"))
+BIN_PREFIX = os.path.abspath(os.path.join(CMAKE_BINARY_DIR, "src"))
 
 # Default location of test hooks
 DEFAULT_HOOK_PATH = os.path.abspath(

Diff for: test/make_tc_task.cpp

@@ -0,0 +1,80 @@
+////////////////////////////////////////////////////////////////////////////////
+//
+// Copyright 2025, Dustin J. Mitchell
+//
+// Permission is hereby granted, free of charge, to any person obtaining a copy
+// of this software and associated documentation files (the "Software"), to deal
+// in the Software without restriction, including without limitation the rights
+// to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+// copies of the Software, and to permit persons to whom the Software is
+// furnished to do so, subject to the following conditions:
+//
+// The above copyright notice and this permission notice shall be included
+// in all copies or substantial portions of the Software.
+//
+// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS
+// OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+// THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
+// SOFTWARE.
+//
+// https://www.opensource.org/licenses/mit-license.php
+//
+////////////////////////////////////////////////////////////////////////////////
+
+#include <cmake.h>
+// cmake.h include header must come first
+
+#include <CmdInfo.h>
+#include <main.h>
+#include <stdlib.h>
+#include <taskchampion-cpp/lib.h>
+#include <test.h>
+#include <util.h>
+
+#include <iostream>
+#include <limits>
+
+#include "format.h"
+
+namespace {
+
+////////////////////////////////////////////////////////////////////////////////
+int usage() {
+  std::cerr << "USAGE: make_tc_task DATADIR KEY=VALUE ..\n";
+  return 1;
+}
+
+}  // namespace
+
+////////////////////////////////////////////////////////////////////////////////
+int main(int argc, char **argv) {
+  if (!--argc) {
+    return usage();
+  }
+  char *datadir = *++argv;
+
+  auto replica = tc::new_replica_on_disk(datadir, true);
+  auto uuid = tc::uuid_v4();
+  auto operations = tc::new_operations();
+  auto task = tc::create_task(uuid, operations);
+
+  while (--argc) {
+    std::string arg = *++argv;
+    size_t eq_idx = arg.find('=');
+    if (eq_idx == std::string::npos) {
+      return usage();
+    }
+    std::string property = arg.substr(0, eq_idx);
+    std::string value = arg.substr(eq_idx + 1);
+    task->update(property, value, operations);
+  }
+  replica->commit_operations(std::move(operations));
+
+  std::cout << static_cast<std::string>(uuid.to_string()) << "\n";
+  return 0;
+}
+
+////////////////////////////////////////////////////////////////////////////////