From 49ad5fc7a9cc71d28e6b99d9424fa82e6c2e3c7e Mon Sep 17 00:00:00 2001
From: Claude <noreply@anthropic.com>
Date: Sun, 14 Dec 2025 12:50:08 +0000
Subject: [PATCH] Fix duplicate violations when reusing that() for multiple
 should() calls

This fixes issue #303 where reusing the result of that() for multiple
should() calls would cause duplicate violations due to shared mutable
state in the RuleBuilder.

The fix makes should() and andThat() clone the RuleBuilder before
adding constraints/specs, ensuring each rule branch gets an isolated
copy of the builder state.

Added RuleBuilder::__clone() to deep clone Specs and Constraints objects.
---
 src/Rules/AndThatShould.php          | 10 ++--
 src/Rules/RuleBuilder.php            |  6 +++
 tests/Unit/Rules/RuleBuilderTest.php | 75 ++++++++++++++++++++++++++++
 3 files changed, 87 insertions(+), 4 deletions(-)
 create mode 100644 tests/Unit/Rules/RuleBuilderTest.php

diff --git a/src/Rules/AndThatShould.php b/src/Rules/AndThatShould.php
index c6848f53..03030be1 100644
--- a/src/Rules/AndThatShould.php
+++ b/src/Rules/AndThatShould.php
@@ -19,15 +19,17 @@ public function __construct(RuleBuilder $expressionBuilder)
 
     public function andThat(Expression $expression): AndThatShouldParser
     {
-        $this->ruleBuilder->addThat($expression);
+        $ruleBuilder = clone $this->ruleBuilder;
+        $ruleBuilder->addThat($expression);
 
-        return $this;
+        return new self($ruleBuilder);
     }
 
     public function should(Expression $expression): BecauseParser
     {
-        $this->ruleBuilder->addShould($expression);
+        $ruleBuilder = clone $this->ruleBuilder;
+        $ruleBuilder->addShould($expression);
 
-        return new Because($this->ruleBuilder);
+        return new Because($ruleBuilder);
     }
 }
diff --git a/src/Rules/RuleBuilder.php b/src/Rules/RuleBuilder.php
index e6f12bf2..e56eb41d 100644
--- a/src/Rules/RuleBuilder.php
+++ b/src/Rules/RuleBuilder.php
@@ -30,6 +30,12 @@ public function __construct()
         $this->runOnlyThis = false;
     }
 
+    public function __clone()
+    {
+        $this->thats = clone $this->thats;
+        $this->shoulds = clone $this->shoulds;
+    }
+
     public function addThat(Expression $that): self
     {
         $this->thats->add($that);
diff --git a/tests/Unit/Rules/RuleBuilderTest.php b/tests/Unit/Rules/RuleBuilderTest.php
new file mode 100644
index 00000000..5c4dad85
--- /dev/null
+++ b/tests/Unit/Rules/RuleBuilderTest.php
@@ -0,0 +1,75 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Arkitect\Tests\Unit\Rules;
+
+use Arkitect\Analyzer\ClassDescription;
+use Arkitect\Expression\ForClasses\Extend;
+use Arkitect\Expression\ForClasses\HaveNameMatching;
+use Arkitect\Expression\ForClasses\ResideInOneOfTheseNamespaces;
+use Arkitect\Rules\Rule;
+use Arkitect\Rules\Violations;
+use PHPUnit\Framework\TestCase;
+
+/**
+ * @see https://github.com/phparkitect/arkitect/issues/303
+ */
+class RuleBuilderTest extends TestCase
+{
+    public function test_reusing_that_for_multiple_should_produces_independent_rules(): void
+    {
+        $classViolatingBothRules = ClassDescription::getBuilder('App\Rector\MyClass', 'src/Rector/MyClass.php')
+            ->build();
+
+        $rectors = Rule::allClasses()
+            ->that(new ResideInOneOfTheseNamespaces('App\Rector'));
+
+        $nameMatchingRule = $rectors
+            ->should(new HaveNameMatching('*Rector'))
+            ->because('rector classes should have Rector suffix');
+
+        $extendsRule = $rectors
+            ->should(new Extend('Rector\Core\Rector\AbstractRector'))
+            ->because('rector classes should extend AbstractRector');
+
+        $nameMatchingViolations = new Violations();
+        $nameMatchingRule->check($classViolatingBothRules, $nameMatchingViolations);
+
+        $extendsViolations = new Violations();
+        $extendsRule->check($classViolatingBothRules, $extendsViolations);
+
+        self::assertCount(1, $nameMatchingViolations);
+        self::assertCount(1, $extendsViolations);
+        self::assertStringContainsString('should have a name that matches', $nameMatchingViolations->get(0)->getError());
+        self::assertStringContainsString('should extend', $extendsViolations->get(0)->getError());
+    }
+
+    public function test_reusing_that_for_multiple_and_that_produces_independent_rules(): void
+    {
+        $classInBaseNamespaceOnly = ClassDescription::getBuilder('App\Service\MyService', 'src/Service/MyService.php')
+            ->build();
+
+        $services = Rule::allClasses()
+            ->that(new ResideInOneOfTheseNamespaces('App\Service'));
+
+        $internalRule = $services
+            ->andThat(new ResideInOneOfTheseNamespaces('App\Service\Internal'))
+            ->should(new HaveNameMatching('*Service'))
+            ->because('internal services should have Service suffix');
+
+        $externalRule = $services
+            ->andThat(new ResideInOneOfTheseNamespaces('App\Service\External'))
+            ->should(new HaveNameMatching('*Client'))
+            ->because('external services should have Client suffix');
+
+        $internalViolations = new Violations();
+        $internalRule->check($classInBaseNamespaceOnly, $internalViolations);
+
+        $externalViolations = new Violations();
+        $externalRule->check($classInBaseNamespaceOnly, $externalViolations);
+
+        self::assertCount(0, $internalViolations);
+        self::assertCount(0, $externalViolations);
+    }
+}