src/lampyrid/models/lampyrid_models.py (1)

1190-1206: Consider adding a validator for trigger/action structure.

The triggers and actions fields use List[dict[str, Any]] which defers validation to the service layer. While this works, adding a basic validator to check for required keys (like type) at the model level would provide earlier feedback to users.

♻️ Optional: Add basic structure validation

+    `@model_validator`(mode='after')
+    def validate_trigger_action_structure(self):
+        """Validate that triggers/actions have required 'type' field."""
+        if self.triggers:
+            for i, t in enumerate(self.triggers):
+                if 'type' not in t:
+                    raise ValueError(f"triggers[{i}] missing required 'type' field")
+        if self.actions:
+            for i, a in enumerate(self.actions):
+                if 'type' not in a:
+                    raise ValueError(f"actions[{i}] missing required 'type' field")
+        return self

src/lampyrid/services/rules.py (1)

228-233: Move inline import to module level.

The RuleActionUpdate import is done inline within the function. While functional, this is inconsistent with RuleTriggerUpdate which is imported at the module level.

♻️ Proposed refactor

At line 13, add RuleActionUpdate to the existing import:

-from ..models.firefly_models import RuleTriggerUpdate, RuleUpdate
+from ..models.firefly_models import RuleActionUpdate, RuleTriggerUpdate, RuleUpdate

Then remove the inline import at line 229.

src/lampyrid/models/lampyrid_models.py (2)

1191-1207: Consider using typed models for triggers and actions instead of dict[str, Any].

Using untyped dicts forgoes compile-time and validation-time safety. Invalid payloads (e.g. missing type key, wrong value types) will only surface as downstream API errors rather than clear Pydantic validation failures. A lightweight typed model (even a TypedDict or a small Pydantic model with type: str required and value: Optional[str]) would catch malformed input earlier and provide better error messages to MCP clients.

That said, the current approach does offer flexibility for passing API-specific fields that may not all be modeled yet, so this is a tradeoff.

---

1215-1221: Duplicated _parse_string_input validator across TestRuleRequest and ExecuteRuleRequest.

Both classes define identical mode='before' validators. If more request models with date fields are added, this pattern will spread further. Consider extracting a shared mixin or base class:

♻️ Example DRY refactor

+class _JsonStringParseMixin(BaseModel):
+    """Mixin for models that need to handle JSON-string input from MCP clients."""
+
+    `@model_validator`(mode='before')
+    `@classmethod`
+    def _parse_string_input(cls, data: Any) -> Any:
+        if isinstance(data, str):
+            return json.loads(data)
+        return data
+
+
-class TestRuleRequest(BaseModel):
+class TestRuleRequest(_JsonStringParseMixin):
     """Request model for testing a rule (preview matches)."""
 
     model_config = ConfigDict(extra='forbid')
 
-    `@model_validator`(mode='before')
-    `@classmethod`
-    def _parse_string_input(cls, data: Any) -> Any:
-        """Handle MCP clients that serialize the request as a JSON string."""
-        if isinstance(data, str):
-            return json.loads(data)
-        return data
-
     rule_id: str = Field(...)
     ...

-class ExecuteRuleRequest(BaseModel):
+class ExecuteRuleRequest(_JsonStringParseMixin):
     """Request model for executing a rule (apply changes)."""
 
     model_config = ConfigDict(extra='forbid')
 
-    `@model_validator`(mode='before')
-    `@classmethod`
-    def _parse_string_input(cls, data: Any) -> Any:
-        """Handle MCP clients that serialize the request as a JSON string."""
-        if isinstance(data, str):
-            return json.loads(data)
-        return data
-
     rule_id: str = Field(...)
     ...

Also applies to: 1252-1258

src/lampyrid/models/lampyrid_models.py (3)

1148-1163: SearchRulesRequest validator treats empty strings as "no criterion" — reasonable but worth noting.

The truthiness-based check (any([self.trigger_type, ...])) means passing trigger_type="" would not count as a criterion. This is acceptable behavior since an empty string isn't a meaningful filter, but it differs from the SearchTransactionsRequest validator (lines 476-503) which explicitly handles this with isinstance(field, str) and field.strip() != ''. The inconsistency is minor but could confuse future maintainers.

---

1191-1207: triggers and actions typed as List[dict[str, Any]] — validation deferred to service layer.

This is a deliberate loose-typing choice, but it means invalid trigger/action structures won't be caught until the service layer calls RuleTriggerStore(**t) / RuleActionStore(**a). For UpdateRuleRequest and CreateRuleRequest, consider using List[RuleTriggerStore] / List[RuleActionStore] (or their update variants) to shift validation left to the model boundary. This is a tradeoff with MCP tool flexibility, so flagging as optional.

Also applies to: 1340-1357

---

1324-1331: Use RuleTriggerType enum instead of str for the trigger field to improve type safety.

The trigger field accepts only 'store-journal', 'update-journal', or 'manual-activation', but is typed as str. Invalid values fail later at the service layer when RuleTriggerType(req.trigger) is called, producing less informative errors. Since RuleTriggerType already exists in firefly_models and other enum types from that module are already used in this file, constraining the field to the enum would catch invalid values at the model boundary and align with existing patterns.

♻️ Proposed fix

-    trigger: str = Field(
-        'store-journal',
+    trigger: RuleTriggerType = Field(
+        RuleTriggerType.store_journal,
         description=(
             'When the rule should fire. '
             '"store-journal" = on new transactions (most common), '
             '"update-journal" = on transaction updates.'
         ),
     )

Add RuleTriggerType to the imports from firefly_models at the top of the file.

src/lampyrid/services/rules.py (4)

295-301: Inline import of RuleActionUpdate is inconsistent with top-level imports.

RuleTriggerUpdate is imported at the top of the file (line 13), but RuleActionUpdate is imported inline here. Move it to the top-level import for consistency.

♻️ Proposed fix

At line 13:

-from ..models.firefly_models import RuleActionStore, RuleStore, RuleTriggerStore, RuleTriggerType, RuleTriggerUpdate, RuleUpdate
+from ..models.firefly_models import RuleActionStore, RuleActionUpdate, RuleStore, RuleTriggerStore, RuleTriggerType, RuleTriggerUpdate, RuleUpdate

At lines 295-301:

         if req.actions is not None:
             try:
-                from ..models.firefly_models import RuleActionUpdate
-
                 rule_update.actions = [RuleActionUpdate(**a) for a in req.actions]
             except ValidationError as e:
                 raise ValueError(f'Invalid action format: {e}')

---

141-154: Unbounded pagination loop has no safety limit.

If the API returns inconsistent pagination metadata (e.g., current_page never reaching total_pages due to concurrent rule creation), this loop runs indefinitely. Consider adding a max page limit.

🛡️ Proposed fix

         all_rules = []
         page = 1
-        while True:
+        max_pages = 100  # Safety limit
+        while page <= max_pages:
             rule_array = await self._client.get_rules(page)
             all_rules.extend(rule_array.data)
 
             # Check pagination safely (can be None)
             if (
                 not rule_array.meta.pagination
                 or rule_array.meta.pagination.current_page >= rule_array.meta.pagination.total_pages
             ):
                 break
             page += 1

---

332-361: test_rule makes two sequential API calls; the first (get_rule) is only for the title.

The extra round-trip to fetch the rule title is unnecessary latency. Consider whether the title is required in the result, or if it could be populated from the test response itself. If the title is essential, this is acceptable — just noting the tradeoff.

---

125-139: User-supplied regex compiled without bounds — low ReDoS risk but worth noting.

User-provided trigger_value_pattern and action_value_pattern are compiled directly. In an MCP context, the user is typically the LLM agent or a trusted client, so the risk is minimal. If this were ever exposed to untrusted input, consider adding a timeout or pattern complexity limit.