Refuse workflows that move cards in a loop

Every enabled workflow is an edge "source stack -> target stack";
LoopDetector finds circles in that graph, a workflow whose source is its
own target being the shortest. Without a check, two workflows A -> B and
B -> A move the same cards back and forth on every run, even within a
single run, with a notice for each move. The trigger date is ignored on
purpose: by start date one way and by due date the other, a card with
both dates passed still circles. Chains stay allowed.

The controller rejects saving an enabled workflow that would close a loop
and names the workflows involved. Workflows stored before this check are
skipped by the runner (logged, not disabled) and flagged in the settings
list via a new `loop` field from index().

lib/Controller/WorkflowController.php

@@ -8,6 +8,7 @@ use OCA\WorkflowDeckAutomation\Db\Workflow;
 use OCA\WorkflowDeckAutomation\Db\WorkflowMapper;
 use OCA\WorkflowDeckAutomation\Service\DeckIntegrationService;
 use OCA\WorkflowDeckAutomation\Service\DeckUnavailableException;
+use OCA\WorkflowDeckAutomation\Service\LoopDetector;
 use OCA\WorkflowDeckAutomation\Service\NotificationPolicy;
 use OCA\WorkflowDeckAutomation\Service\TriggerDate;
 use OCP\AppFramework\Db\DoesNotExistException;
@@ -48,7 +49,17 @@ class WorkflowController extends OCSController {
 	#[NoAdminRequired]
 	public function index(): DataResponse {
 		$workflows = $this->workflowMapper->findAllForBoards($this->ownedBoardIds());
-		return new DataResponse(array_map(static fn (Workflow $w) => $w->jsonSerialize(), $workflows));
+
+		// validate() keeps new loops out, but workflows stored before that
+		// check can still form one; the runner skips them, the list says why.
+		$looping = LoopDetector::findLoopingWorkflows(LoopDetector::edgesOf(
+			array_filter($workflows, static fn (Workflow $w) => (bool)$w->getEnabled()),
+		));
+
+		return new DataResponse(array_map(
+			static fn (Workflow $w) => $w->jsonSerialize() + ['loop' => in_array((int)$w->getId(), $looping, true)],
+			$workflows,
+		));
 	}
 
 	#[NoAdminRequired]
@@ -62,7 +73,7 @@ class WorkflowController extends OCSController {
 		string $notifyChannel = NotificationPolicy::CHANNEL_MAIL,
 		bool $enabled = true,
 	): DataResponse {
-		$this->validate($title, $boardId, $sourceStackId, $targetStackId, $triggerDate, $notifyMode, $notifyChannel);
+		$this->validate(null, $title, $boardId, $sourceStackId, $targetStackId, $triggerDate, $notifyMode, $notifyChannel, $enabled);
 
 		$workflow = new Workflow();
 		$workflow->setUserId($this->currentUserId());
@@ -85,7 +96,7 @@ class WorkflowController extends OCSController {
 		bool $enabled = true,
 	): DataResponse {
 		$workflow = $this->findOwnWorkflow($id);
-		$this->validate($title, $boardId, $sourceStackId, $targetStackId, $triggerDate, $notifyMode, $notifyChannel);
+		$this->validate($id, $title, $boardId, $sourceStackId, $targetStackId, $triggerDate, $notifyMode, $notifyChannel, $enabled);
 
 		// Kept in step with the board owner so the job runs as, and mails,
 		// the right user; validate() made sure the editor owns the board.
@@ -161,7 +172,10 @@ class WorkflowController extends OCSController {
 		}
 	}
 
-	private function validate(string $title, int $boardId, int $sourceStackId, int $targetStackId, string $triggerDate, string $notifyMode, string $notifyChannel): void {
+	/**
+	 * @param ?int $id the workflow being changed, `null` for a new one
+	 */
+	private function validate(?int $id, string $title, int $boardId, int $sourceStackId, int $targetStackId, string $triggerDate, string $notifyMode, string $notifyChannel, bool $enabled): void {
 		if (trim($title) === '') {
 			throw new OCSBadRequestException('Bitte einen Titel angeben.');
 		}
@@ -208,6 +222,37 @@ class WorkflowController extends OCSController {
 		if (!in_array($sourceStackId, $stackIds, true) || !in_array($targetStackId, $stackIds, true)) {
 			throw new OCSBadRequestException('Der gewählte Stapel existiert auf diesem Board nicht.');
 		}
+
+		// A disabled workflow moves nothing and can close no loop; it is
+		// checked again when it gets switched on, which also goes through here.
+		if ($enabled) {
+			$this->assertNoLoop($id, $boardId, $sourceStackId, $targetStackId);
+		}
+	}
+
+	/**
+	 * Rejects a workflow whose target stack leads back to its source through
+	 * the board's other enabled workflows — cards would circle forever. See
+	 * LoopDetector.
+	 */
+	private function assertNoLoop(?int $id, int $boardId, int $sourceStackId, int $targetStackId): void {
+		$others = [];
+		foreach ($this->workflowMapper->findAllForBoards([$boardId]) as $other) {
+			if ($other->getId() !== $id && $other->getEnabled()) {
+				$others[(int)$other->getId()] = $other;
+			}
+		}
+
+		$path = LoopDetector::findPath(LoopDetector::edgesOf($others), $targetStackId, $sourceStackId);
+		if ($path === null) {
+			return;
+		}
+
+		$titles = array_map(static fn (int $otherId) => '„' . $others[$otherId]->getTitle() . '“', $path);
+		throw new OCSBadRequestException(sprintf(
+			'Zusammen mit %s würde dieser Workflow eine Schleife bilden, in der Karten immer wieder hin- und hergeschoben werden. Wähle einen anderen Ziel-Stapel oder deaktiviere einen der Workflows.',
+			implode(', ', $titles),
+		));
 	}
 
 	private function applyFields(

lib/Service/LoopDetector.php

@@ -0,0 +1,91 @@
+<?php
+
+declare(strict_types=1);
+
+namespace OCA\WorkflowDeckAutomation\Service;
+
+/**
+ * Finds workflows that would hand cards around in a circle — pure logic, no
+ * Nextcloud or Deck dependencies, so it is unit-testable like
+ * NotificationPolicy.
+ *
+ * Every enabled workflow is an edge "source stack → target stack". Two
+ * workflows S1 → S2 and S2 → S1 move a card back and forth on every run,
+ * and since the runner reads each source stack afresh, even within a single
+ * run — with a notice each time. The trigger date is deliberately ignored:
+ * with S1 → S2 by start date and S2 → S1 by due date, every card whose two
+ * dates have both passed still circles. A workflow whose source is its own
+ * target is the shortest such loop.
+ *
+ * Deck's stack ids are unique across boards, so edges from several boards
+ * can be passed in together without mixing anything up.
+ */
+class LoopDetector {
+	/**
+	 * The edges for findPath() and findLoopingWorkflows(). Typed only in the
+	 * docblock, so the tests need no Nextcloud classes.
+	 *
+	 * @param iterable<\OCA\WorkflowDeckAutomation\Db\Workflow> $workflows
+	 * @return array<int, array{0: int, 1: int}>
+	 */
+	public static function edgesOf(iterable $workflows): array {
+		$edges = [];
+		foreach ($workflows as $workflow) {
+			$edges[(int)$workflow->getId()] = [(int)$workflow->getSourceStackId(), (int)$workflow->getTargetStackId()];
+		}
+		return $edges;
+	}
+
+	/**
+	 * The workflows leading from stack $from to stack $to, in order, or
+	 * `null` if there is no way. `[]` if $from already is $to.
+	 *
+	 * @param array<int, array{0: int, 1: int}> $edges workflow id => [source stack id, target stack id]
+	 * @return list<int>|null
+	 */
+	public static function findPath(array $edges, int $from, int $to): ?array {
+		if ($from === $to) {
+			return [];
+		}
+
+		// Breadth-first, remembering how each stack was reached.
+		$reachedBy = [$from => null];
+		$queue = [$from];
+		while ($queue !== []) {
+			$stack = array_shift($queue);
+			foreach ($edges as $id => [$source, $target]) {
+				if ($source !== $stack || array_key_exists($target, $reachedBy)) {
+					continue;
+				}
+				$reachedBy[$target] = [$id, $stack];
+				if ($target === $to) {
+					$path = [];
+					for ($at = $to; $reachedBy[$at] !== null; $at = $reachedBy[$at][1]) {
+						array_unshift($path, $reachedBy[$at][0]);
+					}
+					return $path;
+				}
+				$queue[] = $target;
+			}
+		}
+
+		return null;
+	}
+
+	/**
+	 * Ids of the workflows that lie on a loop: those whose target leads back
+	 * to their own source.
+	 *
+	 * @param array<int, array{0: int, 1: int}> $edges workflow id => [source stack id, target stack id]
+	 * @return list<int>
+	 */
+	public static function findLoopingWorkflows(array $edges): array {
+		$looping = [];
+		foreach ($edges as $id => [$source, $target]) {
+			if (self::findPath($edges, $target, $source) !== null) {
+				$looping[] = $id;
+			}
+		}
+		return $looping;
+	}
+}

lib/Service/WorkflowRunner.php

@@ -67,11 +67,25 @@ class WorkflowRunner {
 			return;
 		}
 
+		// The controller refuses to store a loop, but workflows saved before
+		// that check — including ones whose source is their own target — can
+		// still form one. They would move the same cards on every run, with a
+		// notice each time, so they are left out until the user fixes them;
+		// the settings page marks them.
+		$looping = LoopDetector::findLoopingWorkflows(LoopDetector::edgesOf($workflows));
+
 		$this->pendingNotices = [];
 		$byUser = [];
 		$owners = [];
 		foreach ($workflows as $workflow) {
 			$this->followBoardOwner($workflow, $owners);
+			if (in_array((int)$workflow->getId(), $looping, true)) {
+				$this->logger->warning('Skipping workflow {id}: it moves cards in a loop together with other workflows', [
+					'app' => 'workflow_deck_automation',
+					'id' => $workflow->getId(),
+				]);
+				continue;
+			}
 			$byUser[$workflow->getUserId()][] = $workflow;
 		}