Add crit threshold modifier tests and fix effect application

- Add comprehensive Quench tests for crit threshold system:
  - Skill crit thresholds modified by Active Effects
  - Attack crit thresholds (melee, ranged, brawl, finesse)
  - Effect stacking, disabled effects, minimum threshold enforcement
  - Helper function validation

- Fix crit threshold clamping in CharacterData.prepareDerivedData():
  - Skills: clamp in _calculateSkillDifficulties() after effects apply
  - Attacks: new _clampAttackCritThresholds() method
  - Ensures thresholds stay within 1-20 range after Active Effects

- Note: Foundry v13 auto-prepares data after createEmbeddedDocuments,
  manual prepareData() calls cause double effect application

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

module/data/actor/character.mjs

@@ -650,8 +650,11 @@ export default class CharacterData extends VagabondActorBase {
     this.saves.will.difficulty =
       20 - (stats.reason.value + stats.presence.value) - this.saves.will.bonus;
 
-    // Calculate Skill Difficulties
+    // Calculate Skill Difficulties (also clamps skill crit thresholds)
     this._calculateSkillDifficulties();
+
+    // Clamp attack crit thresholds after Active Effects
+    this._clampAttackCritThresholds();
   }
 
   /**
@@ -674,6 +677,21 @@ export default class CharacterData extends VagabondActorBase {
 
       // Calculate difficulty: 20 - stat (untrained) or 20 - stat×2 (trained)
       skillData.difficulty = trained ? 20 - statValue * 2 : 20 - statValue;
+
+      // Clamp crit threshold after Active Effects (schema min doesn't apply to effect-modified data)
+      skillData.critThreshold = Math.max(1, Math.min(20, skillData.critThreshold));
+    }
+  }
+
+  /**
+   * Clamp attack crit thresholds after Active Effects are applied.
+   * Schema constraints don't apply to effect-modified data.
+   *
+   * @private
+   */
+  _clampAttackCritThresholds() {
+    for (const attackData of Object.values(this.attacks)) {
+      attackData.critThreshold = Math.max(1, Math.min(20, attackData.critThreshold));
     }
   }
 

module/tests/crit-threshold.test.mjs

@@ -0,0 +1,182 @@
+/**
+ * Crit Threshold Modifier System Tests
+ *
+ * Tests that Active Effects can modify crit thresholds for:
+ * - Individual skills (system.skills.<id>.critThreshold)
+ * - Attack types (system.attacks.<type>.critThreshold)
+ *
+ * Use cases:
+ * - Fighter's Valor: reduces melee crit threshold by 1/2/3
+ * - Gunslinger's Deadeye: reduces ranged crit threshold dynamically
+ *
+ * @module tests/crit-threshold
+ */
+
+import { createCritReductionEffect, EFFECT_MODES } from "../helpers/effects.mjs";
+
+/**
+ * Register crit threshold tests with Quench
+ * @param {Quench} quenchRunner - The Quench test runner
+ */
+export function registerCritThresholdTests(quenchRunner) {
+  quenchRunner.registerBatch(
+    "vagabond.critThreshold.skills",
+    (context) => {
+      const { describe, it, expect, beforeEach, afterEach } = context;
+
+      describe("Skill Crit Threshold Modifiers", () => {
+        let actor;
+
+        beforeEach(async () => {
+          actor = await Actor.create({
+            name: "Test Fighter",
+            type: "character",
+          });
+        });
+
+        afterEach(async () => {
+          await actor?.delete();
+        });
+
+        it("should have default crit threshold of 20 for all skills", () => {
+          expect(actor.system.skills.arcana.critThreshold).to.equal(20);
+          expect(actor.system.skills.brawl.critThreshold).to.equal(20);
+          expect(actor.system.skills.finesse.critThreshold).to.equal(20);
+        });
+
+        it("should reduce skill crit threshold with Active Effect", async () => {
+          // Create an effect that reduces brawl crit by 2 (like Fighter's Valor at level 4)
+          const effectData = createCritReductionEffect("brawl", 2, "Valor (Brawl)");
+          await actor.createEmbeddedDocuments("ActiveEffect", [effectData]);
+
+          // Foundry automatically prepares data after embedded document creation
+          // Crit threshold should now be 18
+          expect(actor.system.skills.brawl.critThreshold).to.equal(18);
+          // Other skills should be unaffected
+          expect(actor.system.skills.arcana.critThreshold).to.equal(20);
+        });
+
+        it("should stack multiple crit reductions on same skill", async () => {
+          // Create two effects reducing brawl crit
+          const effect1 = createCritReductionEffect("brawl", 1, "Effect 1");
+          const effect2 = createCritReductionEffect("brawl", 2, "Effect 2");
+          await actor.createEmbeddedDocuments("ActiveEffect", [effect1, effect2]);
+
+          // Total reduction: 1 + 2 = 3, so threshold is 17
+          expect(actor.system.skills.brawl.critThreshold).to.equal(17);
+        });
+
+        it("should allow different skills to have different thresholds", async () => {
+          // Reduce brawl by 2, finesse by 1
+          const brawlEffect = createCritReductionEffect("brawl", 2, "Brawl Crit");
+          const finesseEffect = createCritReductionEffect("finesse", 1, "Finesse Crit");
+          await actor.createEmbeddedDocuments("ActiveEffect", [brawlEffect, finesseEffect]);
+
+          expect(actor.system.skills.brawl.critThreshold).to.equal(18);
+          expect(actor.system.skills.finesse.critThreshold).to.equal(19);
+          expect(actor.system.skills.arcana.critThreshold).to.equal(20);
+        });
+
+        it("should not reduce crit threshold below 1", async () => {
+          // Create an absurd reduction
+          const effectData = createCritReductionEffect("brawl", 25, "Overpowered");
+          await actor.createEmbeddedDocuments("ActiveEffect", [effectData]);
+
+          // Crit threshold should be clamped to minimum of 1
+          expect(actor.system.skills.brawl.critThreshold).to.be.at.least(1);
+        });
+
+        it("should not affect threshold when effect is disabled", async () => {
+          const effectData = createCritReductionEffect("brawl", 2, "Disabled Effect");
+          effectData.disabled = true;
+          await actor.createEmbeddedDocuments("ActiveEffect", [effectData]);
+
+          // Should remain at default since effect is disabled
+          expect(actor.system.skills.brawl.critThreshold).to.equal(20);
+        });
+      });
+    },
+    { displayName: "Vagabond: Skill Crit Thresholds" }
+  );
+
+  quenchRunner.registerBatch(
+    "vagabond.critThreshold.attacks",
+    (context) => {
+      const { describe, it, expect, beforeEach, afterEach } = context;
+
+      describe("Attack Crit Threshold Modifiers", () => {
+        let actor;
+
+        beforeEach(async () => {
+          actor = await Actor.create({
+            name: "Test Fighter",
+            type: "character",
+          });
+        });
+
+        afterEach(async () => {
+          await actor?.delete();
+        });
+
+        it("should have default crit threshold of 20 for all attack types", () => {
+          expect(actor.system.attacks.melee.critThreshold).to.equal(20);
+          expect(actor.system.attacks.brawl.critThreshold).to.equal(20);
+          expect(actor.system.attacks.ranged.critThreshold).to.equal(20);
+          expect(actor.system.attacks.finesse.critThreshold).to.equal(20);
+        });
+
+        it("should reduce attack crit threshold with Active Effect", async () => {
+          // Create effect reducing melee crit by 1 (Fighter's Valor level 1)
+          const effectData = createCritReductionEffect("attack.melee", 1, "Valor (Melee)");
+          await actor.createEmbeddedDocuments("ActiveEffect", [effectData]);
+
+          expect(actor.system.attacks.melee.critThreshold).to.equal(19);
+          expect(actor.system.attacks.ranged.critThreshold).to.equal(20);
+        });
+
+        it("should reduce ranged crit threshold (Gunslinger style)", async () => {
+          // Simulate Gunslinger's Deadeye reducing ranged crit
+          const effectData = createCritReductionEffect("attack.ranged", 3, "Deadeye");
+          await actor.createEmbeddedDocuments("ActiveEffect", [effectData]);
+
+          expect(actor.system.attacks.ranged.critThreshold).to.equal(17);
+        });
+      });
+    },
+    { displayName: "Vagabond: Attack Crit Thresholds" }
+  );
+
+  quenchRunner.registerBatch(
+    "vagabond.critThreshold.effectHelpers",
+    (context) => {
+      const { describe, it, expect } = context;
+
+      describe("Crit Effect Helper Functions", () => {
+        it("should create valid crit reduction effect data", () => {
+          const effectData = createCritReductionEffect("brawl", 2, "Test Effect");
+
+          expect(effectData.name).to.equal("Test Effect");
+          expect(effectData.changes).to.have.length(1);
+          expect(effectData.changes[0].key).to.equal("system.skills.brawl.critThreshold");
+          expect(effectData.changes[0].value).to.equal("-2");
+          expect(effectData.changes[0].mode).to.equal(EFFECT_MODES.ADD);
+        });
+
+        it("should create attack crit reduction with correct key", () => {
+          const effectData = createCritReductionEffect("attack.melee", 1, "Valor");
+
+          expect(effectData.changes[0].key).to.equal("system.attacks.melee.critThreshold");
+          expect(effectData.changes[0].value).to.equal("-1");
+        });
+
+        it("should always use negative value for reduction", () => {
+          // Even if positive is passed, should be negative
+          const effectData = createCritReductionEffect("brawl", 3, "Test");
+
+          expect(effectData.changes[0].value).to.equal("-3");
+        });
+      });
+    },
+    { displayName: "Vagabond: Crit Effect Helpers" }
+  );
+}

module/tests/quench-init.mjs

@@ -11,6 +11,7 @@
 import { registerActorTests } from "./actor.test.mjs";
 import { registerDiceTests } from "./dice.test.mjs";
 import { registerSpellTests } from "./spell.test.mjs";
+import { registerCritThresholdTests } from "./crit-threshold.test.mjs";
 // import { registerItemTests } from "./item.test.mjs";
 // import { registerEffectTests } from "./effects.test.mjs";
 
@@ -68,6 +69,7 @@ export function registerQuenchTests(quenchRunner) {
   registerActorTests(quenchRunner);
   registerDiceTests(quenchRunner);
   registerSpellTests(quenchRunner);
+  registerCritThresholdTests(quenchRunner);
   // registerItemTests(quenchRunner);
   // registerEffectTests(quenchRunner);