feat: add rules management with 5 new MCP tools#75
feat: add rules management with 5 new MCP tools#75edmundhighcock wants to merge 3 commits intoRadCod3:mainfrom
Conversation
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis PR introduces comprehensive rule-management functionality to the Firefly III client, extending the FireflyClient with five new API methods, defining request/response models, implementing a RuleService layer with search filtering and business logic, registering MCP tools for rule operations, and providing unit test coverage for the service layer. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as MCP Client
participant Tools as Rules Server
participant Service as RuleService
participant Firefly as FireflyClient
Client->>Tools: call test_rule(rule_id, dates, accounts)
Tools->>Service: test_rule(TestRuleRequest)
Service->>Firefly: get_rule(rule_id)
Firefly-->>Service: RuleSingle (title, details)
Service->>Firefly: test_rule(rule_id, start, end, accounts)
Firefly-->>Service: TransactionArray (matched transactions)
Service->>Service: Convert transactions to simplified format
Service->>Service: Build RuleTestResult with count & matches
Service-->>Tools: RuleTestResult
Tools-->>Client: Preview results with matched transactions
sequenceDiagram
participant Client as MCP Client
participant Tools as Rules Server
participant Service as RuleService
participant Firefly as FireflyClient
Client->>Tools: call execute_rule(rule_id, dates, confirm=true)
Tools->>Service: execute_rule(ExecuteRuleRequest)
Service->>Service: Validate confirm=true (safety check)
Service->>Firefly: get_rule(rule_id)
Firefly-->>Service: RuleSingle (title, details)
Service->>Firefly: trigger_rule(rule_id, start, end, accounts)
Firefly-->>Service: boolean (status 204 = success)
Service->>Service: Build RuleExecuteResult with success flag
Service-->>Tools: RuleExecuteResult
Tools-->>Client: Execution confirmation with status message
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@src/lampyrid/clients/firefly.py`:
- Around line 503-532: The rule-related APIs and their request models currently
use account_ids: Optional[list[str]] (e.g., test_rule and
ExecuteRuleRequest/TestRuleRequest) while insight methods use
Optional[list[int]]; unify them by changing all rule method signatures, query
param handling, and request models to use Optional[list[int]] (or pick int
consistently) so the shared query key 'accounts[]' always receives integers;
update test_rule, trigger_rule, TestRuleRequest, ExecuteRuleRequest and any
other rule-insight callers to accept/serialize list[int] and ensure
params['accounts[]'] passes ints (or converts incoming ints to strings only at
the HTTP layer if necessary).
In `@src/lampyrid/services/rules.py`:
- Around line 243-245: The current assignments use the faulty "or True"/"or
False" pattern which coerces falsy values like False to the defaults; update the
assignments to use explicit None-coalescing so an explicit False is preserved.
For example, replace the active and stop_processing assignments that reference
rule_attrs.active and rule_attrs.stop_processing with logic that checks "is
None" (e.g., set active = True if rule_attrs.active is None else
rule_attrs.active and set stop_processing = False if rule_attrs.stop_processing
is None else rule_attrs.stop_processing), leaving strict as rule_attrs.strict;
target the active/stop_processing assignments in the function/class where these
lines appear.
- Around line 135-137: The current assignment uses boolean "or" which forces
True for active and may mis-handle stop_processing; update the mappings where
you set active, strict, and stop_processing from rule_read.attributes to
explicitly check for None and only use the default when the attribute is None
(e.g., use a conditional that returns rule_read.attributes.active if it is not
None else True, and similarly for stop_processing defaulting to False), keeping
the field names (rule_read, attributes, active, strict, stop_processing) to
locate and modify the code.
- Around line 175-177: The assignment active=rule_attrs.active or True
incorrectly forces active to True even when rule_attrs.active is False; change
it to preserve explicit False by using a None-check such as active =
rule_attrs.active if rule_attrs.active is not None else True (or equivalent like
getattr(rule_attrs, "active", True) with an explicit None check) in the code
that constructs the rule (the lines referencing rule_attrs.active,
rule_attrs.strict, rule_attrs.stop_processing) so inactive rules remain
inactive.
In `@tests/unit/test_rules_service.py`:
- Around line 148-163: The test test_search_rules_by_trigger_type uses an
invalid trigger_type value 'amount_greater_than' when creating rule2 via
_make_rule_read; update the trigger_type to a valid RuleTriggerKeyword such as
'amount_more' so the mock rule matches the service's expected enums and the
SearchRulesRequest(trigger_type='description') test logic remains correct.
🧹 Nitpick comments (2)
src/lampyrid/models/lampyrid_models.py (1)
1190-1206: Consider adding a validator for trigger/action structure.The
triggersandactionsfields useList[dict[str, Any]]which defers validation to the service layer. While this works, adding a basic validator to check for required keys (liketype) 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 selfsrc/lampyrid/services/rules.py (1)
228-233: Move inline import to module level.The
RuleActionUpdateimport is done inline within the function. While functional, this is inconsistent withRuleTriggerUpdatewhich is imported at the module level.♻️ Proposed refactor
At line 13, add
RuleActionUpdateto the existing import:-from ..models.firefly_models import RuleTriggerUpdate, RuleUpdate +from ..models.firefly_models import RuleActionUpdate, RuleTriggerUpdate, RuleUpdateThen remove the inline import at line 229.
| async def test_rule( | ||
| self, | ||
| rule_id: str, | ||
| start_date: date, | ||
| end_date: date, | ||
| account_ids: Optional[list[str]] = None, | ||
| ) -> TransactionArray: | ||
| """Test a rule in preview mode (shows matches without changes). | ||
|
|
||
| Args: | ||
| rule_id: ID of the rule to test | ||
| start_date: Start date for matching transactions | ||
| end_date: End date for matching transactions | ||
| account_ids: Optional list of account IDs to filter | ||
|
|
||
| Returns: | ||
| TransactionArray with matching transactions | ||
|
|
||
| """ | ||
| params: Dict[str, Any] = { | ||
| 'start': start_date.strftime('%Y-%m-%d'), | ||
| 'end': end_date.strftime('%Y-%m-%d'), | ||
| } | ||
| if account_ids: | ||
| params['accounts[]'] = account_ids | ||
|
|
||
| r = await self._client.get(f'/api/v1/rules/{rule_id}/test', params=params) | ||
| self._handle_api_error(r) | ||
| r.raise_for_status() | ||
| return TransactionArray.model_validate(r.json()) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's find the trigger_rule method and examine its signature
rg -n "def trigger_rule" src/lampyrid/clients/firefly.py -A 10Repository: RadCod3/LamPyrid
Length of output: 413
🏁 Script executed:
# Search for insight methods and their account_ids parameter types
rg -n "def.*insight" src/lampyrid/clients/firefly.py -A 10Repository: RadCod3/LamPyrid
Length of output: 482
🏁 Script executed:
# Search for _build_insight_params method mentioned in the scratchpad
rg -n "_build_insight_params" src/lampyrid/clients/firefly.py -B 2 -A 15Repository: RadCod3/LamPyrid
Length of output: 6951
🏁 Script executed:
# Get overall structure of the file to understand parameter patterns
wc -l src/lampyrid/clients/firefly.pyRepository: RadCod3/LamPyrid
Length of output: 95
🏁 Script executed:
# Let's check the context around how params['accounts[]'] is used
# and if there's any type conversion happening
rg -n "accounts\[\]" src/lampyrid/clients/firefly.py -B 2 -A 2Repository: RadCod3/LamPyrid
Length of output: 563
🏁 Script executed:
# Check the imports to understand what types are expected
head -30 src/lampyrid/clients/firefly.pyRepository: RadCod3/LamPyrid
Length of output: 657
🏁 Script executed:
# Let's examine the test_rule and trigger_rule methods more closely
sed -n '503,550p' src/lampyrid/clients/firefly.pyRepository: RadCod3/LamPyrid
Length of output: 1655
🏁 Script executed:
# Let's search for other uses of account_ids to see the pattern
rg -n "account_id" src/lampyrid/clients/firefly.py | head -20Repository: RadCod3/LamPyrid
Length of output: 1334
🏁 Script executed:
# Check if there's any documentation or comments about account ID types
rg -n "account" src/lampyrid/clients/firefly.py | grep -i "id" | head -15Repository: RadCod3/LamPyrid
Length of output: 972
🏁 Script executed:
# Let's check if there are any test files or examples showing how account_ids are used
find . -type f -name "*.py" | xargs grep -l "test_rule\|trigger_rule" | head -5Repository: RadCod3/LamPyrid
Length of output: 232
🏁 Script executed:
# Let's also check if there's any documentation about the Firefly III API in the repo
find . -type f \( -name "*.md" -o -name "*.txt" -o -name "*.rst" \) | xargs grep -l "account" 2>/dev/null | head -5Repository: RadCod3/LamPyrid
Length of output: 71
🏁 Script executed:
# Check if there's type validation or conversion happening in the _client.get or _client.post methods
rg -n "class.*Client" src/ -A 5 | head -30Repository: RadCod3/LamPyrid
Length of output: 526
🏁 Script executed:
# Let's check the test_rules_service.py to see how these methods are called
cat tests/unit/test_rules_service.py | head -100Repository: RadCod3/LamPyrid
Length of output: 2496
🏁 Script executed:
# Let's check the rules.py service to see how it uses test_rule and trigger_rule
rg -n "test_rule\|trigger_rule" src/lampyrid/services/rules.py -B 3 -A 5Repository: RadCod3/LamPyrid
Length of output: 42
🏁 Script executed:
# Let's check the README for any API documentation references
cat README.md | grep -i -A 5 -B 5 "account\|rule"Repository: RadCod3/LamPyrid
Length of output: 2511
🏁 Script executed:
# Let's check how the RuleService uses test_rule and trigger_rule
cat src/lampyrid/services/rules.pyRepository: RadCod3/LamPyrid
Length of output: 11440
🏁 Script executed:
# Let's look for any API documentation or comments about account IDs
rg -n "accounts\[\]|account.*id|rule.*id" src/lampyrid/clients/firefly.py | grep -i "str\|int" | head -20Repository: RadCod3/LamPyrid
Length of output: 1052
🏁 Script executed:
# Let's check the TestRuleRequest and ExecuteRuleRequest to see what account_ids type they expect
rg -n "class.*Request" src/lampyrid/models/lampyrid_models.py -A 15 | grep -A 15 "TestRuleRequest\|ExecuteRuleRequest"Repository: RadCod3/LamPyrid
Length of output: 1222
🏁 Script executed:
# Let's also verify if there are any type conversion issues when passing account_ids to the API
rg -n "Optional\[list" src/lampyrid/ -A 1Repository: RadCod3/LamPyrid
Length of output: 2052
🏁 Script executed:
# Let's see the full definition of TestRuleRequest and ExecuteRuleRequest including account_ids field
sed -n '1209,1237p' src/lampyrid/models/lampyrid_models.pyRepository: RadCod3/LamPyrid
Length of output: 964
🏁 Script executed:
# And ExecuteRuleRequest
sed -n '1238,1270p' src/lampyrid/models/lampyrid_models.pyRepository: RadCod3/LamPyrid
Length of output: 1242
Standardize account_ids parameter types across rule and insight methods.
The test_rule and trigger_rule methods use account_ids: Optional[list[str]], while all insight methods use account_ids: Optional[list[int]]. Both pass these to the same query parameter key (accounts[]), so the types should be consistent. Additionally, the corresponding request models (TestRuleRequest and ExecuteRuleRequest) also specify List[str], creating an inconsistency throughout the interface. Either unify all account filtering to list[int] (matching insight methods) or clarify why rule endpoints require different types.
🤖 Prompt for AI Agents
In `@src/lampyrid/clients/firefly.py` around lines 503 - 532, The rule-related
APIs and their request models currently use account_ids: Optional[list[str]]
(e.g., test_rule and ExecuteRuleRequest/TestRuleRequest) while insight methods
use Optional[list[int]]; unify them by changing all rule method signatures,
query param handling, and request models to use Optional[list[int]] (or pick int
consistently) so the shared query key 'accounts[]' always receives integers;
update test_rule, trigger_rule, TestRuleRequest, ExecuteRuleRequest and any
other rule-insight callers to accept/serialize list[int] and ensure
params['accounts[]'] passes ints (or converts incoming ints to strings only at
the HTTP layer if necessary).
src/lampyrid/services/rules.py
Outdated
| active=rule_read.attributes.active or True, | ||
| strict=rule_read.attributes.strict, | ||
| stop_processing=rule_read.attributes.stop_processing or False, |
There was a problem hiding this comment.
Logic error: or True always evaluates to True.
The expression rule_attrs.active or True will always be True because when active is False, False or True evaluates to True. This means inactive rules will incorrectly appear as active.
🐛 Proposed fix
- active=rule_read.attributes.active or True,
+ active=rule_read.attributes.active if rule_read.attributes.active is not None else True,
strict=rule_read.attributes.strict,
- stop_processing=rule_read.attributes.stop_processing or False,
+ stop_processing=rule_read.attributes.stop_processing if rule_read.attributes.stop_processing is not None else False,Or more concisely using Python 3.8+ walrus operator isn't needed here, but a simple ternary or ??-style pattern:
active=rule_read.attributes.active if rule_read.attributes.active is not None else True,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| active=rule_read.attributes.active or True, | |
| strict=rule_read.attributes.strict, | |
| stop_processing=rule_read.attributes.stop_processing or False, | |
| active=rule_read.attributes.active if rule_read.attributes.active is not None else True, | |
| strict=rule_read.attributes.strict, | |
| stop_processing=rule_read.attributes.stop_processing if rule_read.attributes.stop_processing is not None else False, |
🤖 Prompt for AI Agents
In `@src/lampyrid/services/rules.py` around lines 135 - 137, The current
assignment uses boolean "or" which forces True for active and may mis-handle
stop_processing; update the mappings where you set active, strict, and
stop_processing from rule_read.attributes to explicitly check for None and only
use the default when the attribute is None (e.g., use a conditional that returns
rule_read.attributes.active if it is not None else True, and similarly for
stop_processing defaulting to False), keeping the field names (rule_read,
attributes, active, strict, stop_processing) to locate and modify the code.
src/lampyrid/services/rules.py
Outdated
| active=rule_attrs.active or True, | ||
| strict=rule_attrs.strict, | ||
| stop_processing=rule_attrs.stop_processing or False, |
There was a problem hiding this comment.
Same logic error with or True pattern.
This has the same issue as search_rules - inactive rules will appear as active.
🐛 Proposed fix
- active=rule_attrs.active or True,
+ active=rule_attrs.active if rule_attrs.active is not None else True,
strict=rule_attrs.strict,
- stop_processing=rule_attrs.stop_processing or False,
+ stop_processing=rule_attrs.stop_processing if rule_attrs.stop_processing is not None else False,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| active=rule_attrs.active or True, | |
| strict=rule_attrs.strict, | |
| stop_processing=rule_attrs.stop_processing or False, | |
| active=rule_attrs.active if rule_attrs.active is not None else True, | |
| strict=rule_attrs.strict, | |
| stop_processing=rule_attrs.stop_processing if rule_attrs.stop_processing is not None else False, |
🤖 Prompt for AI Agents
In `@src/lampyrid/services/rules.py` around lines 175 - 177, The assignment
active=rule_attrs.active or True incorrectly forces active to True even when
rule_attrs.active is False; change it to preserve explicit False by using a
None-check such as active = rule_attrs.active if rule_attrs.active is not None
else True (or equivalent like getattr(rule_attrs, "active", True) with an
explicit None check) in the code that constructs the rule (the lines referencing
rule_attrs.active, rule_attrs.strict, rule_attrs.stop_processing) so inactive
rules remain inactive.
src/lampyrid/services/rules.py
Outdated
| active=rule_attrs.active or True, | ||
| strict=rule_attrs.strict, | ||
| stop_processing=rule_attrs.stop_processing or False, |
There was a problem hiding this comment.
Same logic error with or True pattern.
Same issue as the other methods - needs proper None-coalescing.
🤖 Prompt for AI Agents
In `@src/lampyrid/services/rules.py` around lines 243 - 245, The current
assignments use the faulty "or True"/"or False" pattern which coerces falsy
values like False to the defaults; update the assignments to use explicit
None-coalescing so an explicit False is preserved. For example, replace the
active and stop_processing assignments that reference rule_attrs.active and
rule_attrs.stop_processing with logic that checks "is None" (e.g., set active =
True if rule_attrs.active is None else rule_attrs.active and set stop_processing
= False if rule_attrs.stop_processing is None else rule_attrs.stop_processing),
leaving strict as rule_attrs.strict; target the active/stop_processing
assignments in the function/class where these lines appear.
| @pytest.mark.asyncio | ||
| async def test_search_rules_by_trigger_type(self, service, mock_client): | ||
| """Test searching rules by trigger type keyword.""" | ||
| rule1 = _make_rule_read( | ||
| '1', | ||
| 'Description Trigger', | ||
| trigger_type='description_contains', | ||
| ) | ||
| rule2 = _make_rule_read('2', 'Amount Trigger', trigger_type='amount_greater_than') | ||
| mock_client.get_rules.return_value = _make_rule_array([rule1, rule2]) | ||
|
|
||
| req = SearchRulesRequest(trigger_type='description') | ||
| result = await service.search_rules(req) | ||
|
|
||
| assert len(result) == 1 | ||
| assert result[0].title == 'Description Trigger' |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for amount_greater_than in the codebase
rg -n "amount_greater" --type py
# Also check the RuleTriggerKeyword enum definition
rg -n -A 30 "class RuleTriggerKeyword" --type py
# Additionally search for firefly_models file
fd firefly_models --type fRepository: RadCod3/LamPyrid
Length of output: 2734
Fix invalid trigger type in test: replace amount_greater_than with amount_more.
The test uses trigger_type='amount_greater_than' at line 156, but this is not a valid RuleTriggerKeyword enum value. The valid amount-related triggers are amount_less, amount_exactly, and amount_more. Replace the invalid value with amount_more.
🤖 Prompt for AI Agents
In `@tests/unit/test_rules_service.py` around lines 148 - 163, The test
test_search_rules_by_trigger_type uses an invalid trigger_type value
'amount_greater_than' when creating rule2 via _make_rule_read; update the
trigger_type to a valid RuleTriggerKeyword such as 'amount_more' so the mock
rule matches the service's expected enums and the
SearchRulesRequest(trigger_type='description') test logic remains correct.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/lampyrid/models/lampyrid_models.py`:
- Around line 1223-1244: Add a Pydantic validation to both TestRuleRequest and
ExecuteRuleRequest to ensure start_date <= end_date: implement a `@root_validator`
(or classmethod validator) in each model that checks the two fields and raises a
ValueError (or ValidationError) with a clear message when start_date is after
end_date so API returns a validation error instead of empty results; locate the
models by name (TestRuleRequest, ExecuteRuleRequest) in lampyrid_models.py and
add the validator logic to each class.
🧹 Nitpick comments (2)
src/lampyrid/models/lampyrid_models.py (2)
1191-1207: Consider using typed models fortriggersandactionsinstead ofdict[str, Any].Using untyped dicts forgoes compile-time and validation-time safety. Invalid payloads (e.g. missing
typekey, wrong value types) will only surface as downstream API errors rather than clear Pydantic validation failures. A lightweight typed model (even aTypedDictor a small Pydantic model withtype: strrequired andvalue: 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_inputvalidator acrossTestRuleRequestandExecuteRuleRequest.Both classes define identical
mode='before'validators. If more request models withdatefields 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
| rule_id: str = Field(..., description='Unique identifier of the rule to test') | ||
| start_date: date = Field( | ||
| ..., | ||
| description=( | ||
| 'Start date for matching (YYYY-MM-DD). ' | ||
| 'Only transactions on or after this date will be checked.' | ||
| ), | ||
| ) | ||
| end_date: date = Field( | ||
| ..., | ||
| description=( | ||
| 'End date for matching (YYYY-MM-DD). ' | ||
| 'Only transactions on or before this date will be checked.' | ||
| ), | ||
| ) | ||
| account_ids: Optional[List[str]] = Field( | ||
| None, | ||
| description=( | ||
| 'Optional list of account IDs to limit the test to specific accounts. ' | ||
| 'When provided, only transactions involving these accounts are tested.' | ||
| ), | ||
| ) |
There was a problem hiding this comment.
Missing start_date <= end_date validation in both TestRuleRequest and ExecuteRuleRequest.
If start_date is after end_date, the API will silently return no results (for test) or apply to no transactions (for execute). Adding a quick validator would surface the mistake immediately with a clear error rather than a confusing empty result — especially important for ExecuteRuleRequest where a user might think the rule executed successfully on zero matches.
🛡️ Proposed validation
class TestRuleRequest(BaseModel):
...
account_ids: Optional[List[str]] = Field(...)
+
+ `@model_validator`(mode='after')
+ def validate_date_range(self):
+ if self.start_date > self.end_date:
+ raise ValueError('start_date must be on or before end_date')
+ return selfSame for ExecuteRuleRequest.
Also applies to: 1260-1289
🤖 Prompt for AI Agents
In `@src/lampyrid/models/lampyrid_models.py` around lines 1223 - 1244, Add a
Pydantic validation to both TestRuleRequest and ExecuteRuleRequest to ensure
start_date <= end_date: implement a `@root_validator` (or classmethod validator)
in each model that checks the two fields and raises a ValueError (or
ValidationError) with a clear message when start_date is after end_date so API
returns a validation error instead of empty results; locate the models by name
(TestRuleRequest, ExecuteRuleRequest) in lampyrid_models.py and add the
validator logic to each class.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@src/lampyrid/models/lampyrid_models.py`:
- Around line 1215-1221: The pre-validator _parse_string_input currently calls
json.loads(data) unguarded; wrap that call in a try/except that catches
json.JSONDecodeError and re-raises a plain ValueError (or TypeError) with a
clear message so Pydantic will convert it into a ValidationError; update this
logic in both classes that define _parse_string_input (TestRuleRequest and
ExecuteRuleRequest) so malformed JSON strings produce a validation-friendly
error instead of an uncaught JSONDecodeError.
In `@src/lampyrid/services/rules.py`:
- Around line 84-107: Extract the duplicated mapping logic into a single private
helper method (e.g., _to_rule) that accepts the Firefly response object (handle
both rule_single.data and rule_read.attributes cases) and returns a Rule; inside
the helper read rule_attrs = ...attributes, map
id/title/description/trigger/actions/triggers to Rule, replacing brittle "or
True"/"or False" with explicit None checks (e.g., active = rule_attrs.active if
rule_attrs.active is not None else True; prohibited = t.prohibited if
t.prohibited is not None else False; stop_processing likewise) and construct
nested RuleTriggerSimple and RuleActionSimple; then update create_rule,
search_rules, get_rule, and update_rule to call self._to_rule(...) (for
search_rules map each result through the helper).
- Around line 67-72: Change RuleStore.rule_group_id to be Optional[str] in the
Firefly model (update its type annotation and any serializers/deserializers) and
then modify the code that builds the RuleStore instance in the rules service so
it sets rule_group_id to '1' only when rule_group_title is not provided (i.e.,
rule_group_id = '1' if not req.rule_group_title else None). Update any
imports/uses of RuleStore and ensure RuleUpdate typing remains compatible.
🧹 Nitpick comments (7)
src/lampyrid/models/lampyrid_models.py (3)
1148-1163:SearchRulesRequestvalidator treats empty strings as "no criterion" — reasonable but worth noting.The truthiness-based check (
any([self.trigger_type, ...])) means passingtrigger_type=""would not count as a criterion. This is acceptable behavior since an empty string isn't a meaningful filter, but it differs from theSearchTransactionsRequestvalidator (lines 476-503) which explicitly handles this withisinstance(field, str) and field.strip() != ''. The inconsistency is minor but could confuse future maintainers.
1191-1207:triggersandactionstyped asList[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). ForUpdateRuleRequestandCreateRuleRequest, consider usingList[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: UseRuleTriggerTypeenum instead ofstrfor thetriggerfield to improve type safety.The
triggerfield accepts only'store-journal','update-journal', or'manual-activation', but is typed asstr. Invalid values fail later at the service layer whenRuleTriggerType(req.trigger)is called, producing less informative errors. SinceRuleTriggerTypealready exists infirefly_modelsand 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
RuleTriggerTypeto the imports fromfirefly_modelsat the top of the file.src/lampyrid/services/rules.py (4)
295-301: Inline import ofRuleActionUpdateis inconsistent with top-level imports.
RuleTriggerUpdateis imported at the top of the file (line 13), butRuleActionUpdateis 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, RuleUpdateAt 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_pagenever reachingtotal_pagesdue 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_rulemakes 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_patternandaction_value_patternare 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.
| @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 |
There was a problem hiding this comment.
json.loads may raise JSONDecodeError that won't be wrapped as a ValidationError.
If the MCP client sends a malformed (non-JSON) string, json.loads will raise json.JSONDecodeError which bypasses Pydantic's validation error handling. Wrapping it gives a cleaner error message to callers.
🛡️ Proposed fix
`@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)
+ try:
+ return json.loads(data)
+ except json.JSONDecodeError as e:
+ raise ValueError(f'Invalid JSON input: {e}') from e
return dataApply to both TestRuleRequest and ExecuteRuleRequest.
Also applies to: 1252-1258
🤖 Prompt for AI Agents
In `@src/lampyrid/models/lampyrid_models.py` around lines 1215 - 1221, The
pre-validator _parse_string_input currently calls json.loads(data) unguarded;
wrap that call in a try/except that catches json.JSONDecodeError and re-raises a
plain ValueError (or TypeError) with a clear message so Pydantic will convert it
into a ValidationError; update this logic in both classes that define
_parse_string_input (TestRuleRequest and ExecuteRuleRequest) so malformed JSON
strings produce a validation-friendly error instead of an uncaught
JSONDecodeError.
src/lampyrid/services/rules.py
Outdated
| rule_store = RuleStore( | ||
| title=req.title, | ||
| description=req.description, | ||
| rule_group_id='1', | ||
| rule_group_title=req.rule_group_title, | ||
| trigger=RuleTriggerType(req.trigger), |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the file at the specified lines
head -n 80 src/lampyrid/services/rules.py | tail -n 20Repository: RadCod3/LamPyrid
Length of output: 734
🏁 Script executed:
# Find the RuleStore class definition
rg "class RuleStore" -A 10 -B 2Repository: RadCod3/LamPyrid
Length of output: 1120
🏁 Script executed:
# Check imports in rules.py to understand where RuleStore comes from
head -n 30 src/lampyrid/services/rules.pyRepository: RadCod3/LamPyrid
Length of output: 780
🏁 Script executed:
# Look at the full RuleStore definition
rg "class RuleStore" -A 30 src/lampyrid/models/firefly_models.pyRepository: RadCod3/LamPyrid
Length of output: 1424
🏁 Script executed:
# Check if rule_group_id is used elsewhere or if there are any examples
rg "rule_group_id" src/lampyrid/Repository: RadCod3/LamPyrid
Length of output: 891
🏁 Script executed:
# Let's check the CreateRuleRequest to see what it expects
rg "class CreateRuleRequest" -A 20 src/lampyrid/models/lampyrid_models.pyRepository: RadCod3/LamPyrid
Length of output: 833
🏁 Script executed:
# Check the context of rule_group_id=req.rule_group_id usage
rg "rule_group_id=req.rule_group_id" -B 5 -A 5 src/lampyrid/services/rules.pyRepository: RadCod3/LamPyrid
Length of output: 397
🏁 Script executed:
# Check if RuleStore ever has rule_group_id set to None or Optional
rg "rule_group_id.*None\|Optional.*rule_group_id" src/lampyrid/models/firefly_models.pyRepository: RadCod3/LamPyrid
Length of output: 42
🏁 Script executed:
# Check RuleUpdate to see if it has rule_group_id as Optional
rg "class RuleUpdate" -A 15 src/lampyrid/models/firefly_models.pyRepository: RadCod3/LamPyrid
Length of output: 736
🏁 Script executed:
# Check if there are any tests that show both fields being sent
rg "rule_group_id.*rule_group_title\|rule_group_title.*rule_group_id" -A 2 -B 2 src/Repository: RadCod3/LamPyrid
Length of output: 42
🏁 Script executed:
# Let's check the full RuleStore including the end
rg "class RuleStore" -A 50 src/lampyrid/models/firefly_models.py | head -60Repository: RadCod3/LamPyrid
Length of output: 2152
🏁 Script executed:
# Check if there are any validator or model_config settings in RuleStore
rg "class RuleStore" -B 5 -A 80 src/lampyrid/models/firefly_models.pyRepository: RadCod3/LamPyrid
Length of output: 3511
Hardcoded rule_group_id='1' with mutual exclusivity concern — fix requires type and logic changes.
The field descriptions state "Either this field or rule_group_id is mandatory," but the code always sends both rule_group_id='1' and rule_group_title. This violates the mutual exclusivity expectation and assumes a rule group with ID 1 always exists.
The proposed fix to conditionally set rule_group_id won't work as-is: RuleStore.rule_group_id is typed as required str, not Optional[str]. (Note: RuleUpdate.rule_group_id is optional, suggesting the API supports it.)
To fix this properly:
- Change
RuleStore.rule_group_idtoOptional[str]in the Firefly models - Update the code to set
rule_group_id='1' if not req.rule_group_title else None
♻️ Proposed changes
In src/lampyrid/models/firefly_models.py:
rule_group_id: str = Field(
+rule_group_id: str | None = Field(
...,In src/lampyrid/services/rules.py:
- rule_group_id='1',
+ rule_group_id='1' if not req.rule_group_title else None,🤖 Prompt for AI Agents
In `@src/lampyrid/services/rules.py` around lines 67 - 72, Change
RuleStore.rule_group_id to be Optional[str] in the Firefly model (update its
type annotation and any serializers/deserializers) and then modify the code that
builds the RuleStore instance in the rules service so it sets rule_group_id to
'1' only when rule_group_title is not provided (i.e., rule_group_id = '1' if not
req.rule_group_title else None). Update any imports/uses of RuleStore and ensure
RuleUpdate typing remains compatible.
src/lampyrid/services/rules.py
Outdated
| return Rule( | ||
| id=rule_single.data.id, | ||
| title=rule_attrs.title, | ||
| description=rule_attrs.description, | ||
| active=rule_attrs.active or True, | ||
| strict=rule_attrs.strict, | ||
| stop_processing=rule_attrs.stop_processing or False, | ||
| trigger=rule_attrs.trigger.value, | ||
| triggers=[ | ||
| RuleTriggerSimple( | ||
| type=t.type, | ||
| value=t.value, | ||
| prohibited=t.prohibited or False, | ||
| ) | ||
| for t in rule_attrs.triggers | ||
| ], | ||
| actions=[ | ||
| RuleActionSimple( | ||
| type=a.type, | ||
| value=a.value, | ||
| ) | ||
| for a in rule_attrs.actions | ||
| ], | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Duplicated Rule construction logic across four methods.
The mapping from rule_attrs → Rule (with nested RuleTriggerSimple / RuleActionSimple lists) is copy-pasted across create_rule, search_rules, get_rule, and update_rule. Extracting a private helper like _to_rule(rule_single) would eliminate the duplication and ensure the or True fix (and any future changes) is applied in one place.
♻️ Proposed helper
def _to_rule(self, rule_single) -> Rule:
"""Convert a Firefly RuleSingle response to a simplified Rule model."""
rule_attrs = rule_single.data.attributes
return Rule(
id=rule_single.data.id,
title=rule_attrs.title,
description=rule_attrs.description,
active=rule_attrs.active if rule_attrs.active is not None else True,
strict=rule_attrs.strict,
stop_processing=rule_attrs.stop_processing if rule_attrs.stop_processing is not None else False,
trigger=rule_attrs.trigger.value,
triggers=[
RuleTriggerSimple(
type=t.type,
value=t.value,
prohibited=t.prohibited if t.prohibited is not None else False,
)
for t in rule_attrs.triggers
],
actions=[
RuleActionSimple(type=a.type, value=a.value)
for a in rule_attrs.actions
],
)Then each method becomes e.g. return self._to_rule(rule_single).
For search_rules, wrap each rule_read in a lightweight object or adjust the helper to accept a RuleRead directly (since rule_read.attributes vs rule_single.data.attributes differ slightly).
Also applies to: 198-224, 239-262, 307-330
🤖 Prompt for AI Agents
In `@src/lampyrid/services/rules.py` around lines 84 - 107, Extract the duplicated
mapping logic into a single private helper method (e.g., _to_rule) that accepts
the Firefly response object (handle both rule_single.data and
rule_read.attributes cases) and returns a Rule; inside the helper read
rule_attrs = ...attributes, map id/title/description/trigger/actions/triggers to
Rule, replacing brittle "or True"/"or False" with explicit None checks (e.g.,
active = rule_attrs.active if rule_attrs.active is not None else True;
prohibited = t.prohibited if t.prohibited is not None else False;
stop_processing likewise) and construct nested RuleTriggerSimple and
RuleActionSimple; then update create_rule, search_rules, get_rule, and
update_rule to call self._to_rule(...) (for search_rules map each result through
the helper).
|
Hi @edmundhighcock, Apologies for the late reply and thank you for your contribution! Thanks! |
Add comprehensive rules functionality to manage Firefly III rules: - search_rules: Client-side filtering with regex pattern support - get_rule: Retrieve single rule details - update_rule: Modify rule configuration (triggers/actions) - test_rule: Preview rule matches without changes (read-only) - execute_rule: Apply changes with explicit confirmation requirement Implementation includes: - RuleService in services layer with client-side search/filtering - 11 new request/response models for type safety - 5 new HTTP client methods for Firefly III rule APIs - Comprehensive unit tests (19 test cases) - Safety-first design: test before execute, confirmation required This brings the total tool count from 22 to 27. Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
edmundhighcock
force-pushed
the
feat/add-rules-management
branch
from
ffd5f40 to
470f5e9
Compare
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #75 +/- ##
==========================================
- Coverage 97.66% 96.64% -1.02%
==========================================
Files 19 21 +2
Lines 3121 3338 +217
==========================================
+ Hits 3048 3226 +178
- Misses 73 112 +39
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
- Fix `active or True` logic error that made inactive rules appear active - Replace all `or <default>` patterns with explicit None-coalescing - Extract Rule.from_rule_read() factory method (DRY: 3x duplication → 1) - Fix test helper using wrong enum (RuleTriggerType vs RuleTriggerKeyword) - Use Pydantic's Field(deprecated=...) for the trigger field - Move RuleActionUpdate to top-level import Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
src/lampyrid/models/lampyrid_models.py (1)
1249-1269:⚠️ Potential issue | 🟡 MinorReject inverted date ranges up front.
start_date > end_dateis still accepted for both preview and execute requests, so a caller typo turns into an empty preview or a no-op execution instead of a validation error. Add anaftervalidator on both models before these requests reach the service.Also applies to: 1278-1306
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lampyrid/models/lampyrid_models.py` around lines 1249 - 1269, Add a post-validation check to the request models that declare start_date and end_date: implement a `@root_validator` (or an `@validator` on end_date that accesses start_date) that raises a ValueError when start_date > end_date so inverted date ranges are rejected; apply this to both the preview and execute request models that define the start_date and end_date fields (validate the start_date and end_date attributes and raise a clear error message like "start_date must be on or before end_date").
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/lampyrid/models/lampyrid_models.py`:
- Around line 1243-1246: The TestRuleRequest Pydantic model is being collected
by pytest due to the "Test*" class pattern; add a class attribute __test__ =
False inside the TestRuleRequest class (e.g., directly under the docstring and
above model_config) so pytest will skip collecting it, leaving the class name
unchanged and keeping model_config = ConfigDict(extra='forbid') intact.
- Around line 1224-1240: The triggers and actions fields currently use
Optional[List[dict[str, Any]]] which allows malformed payloads and silent
dropping of unknown fields; change these to use the dedicated nested request
models (e.g., replace types with Optional[List[RuleTriggerUpdate]] and
Optional[List[RuleActionUpdate]]), ensure the RuleTriggerUpdate and
RuleActionUpdate classes mark the type field as required and set Config.extra =
'forbid' (or equivalent Pydantic config) so unknown fields are rejected, and
update any validators/usage in lampyrid_models.py to reflect the new types so
invalid payloads like {} or {"prohibited": true} fail validation immediately.
In `@src/lampyrid/services/rules.py`:
- Around line 106-121: The code currently excludes empty-string trigger/action
values by using truthy checks (e.g., list comprehensions in the trigger_values
and action_values where you use "if t.value" and "if a.value"), which prevents
regexes like ^$ or .* from matching intentionally-blank values; update the
filters to only exclude None (use explicit None checks such as "if t.value is
not None" and "if a.value is not None") so empty strings remain in
trigger_values and action_values for pattern matching while still filtering out
missing values in rule_attrs.triggers and rule_attrs.actions.
---
Duplicate comments:
In `@src/lampyrid/models/lampyrid_models.py`:
- Around line 1249-1269: Add a post-validation check to the request models that
declare start_date and end_date: implement a `@root_validator` (or an `@validator`
on end_date that accesses start_date) that raises a ValueError when start_date >
end_date so inverted date ranges are rejected; apply this to both the preview
and execute request models that define the start_date and end_date fields
(validate the start_date and end_date attributes and raise a clear error message
like "start_date must be on or before end_date").
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3be0f6a3-291a-41ea-9167-504801ffd06e
📒 Files selected for processing (6)
src/lampyrid/clients/firefly.pysrc/lampyrid/models/lampyrid_models.pysrc/lampyrid/services/rules.pysrc/lampyrid/tools/__init__.pysrc/lampyrid/tools/rules.pytests/unit/test_rules_service.py
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/unit/test_rules_service.py
- src/lampyrid/clients/firefly.py
| triggers: Optional[List[dict[str, Any]]] = Field( | ||
| None, | ||
| description=( | ||
| 'Array of trigger objects to update. Each object should have: ' | ||
| 'type (required), value (optional), active (optional), ' | ||
| 'order (optional), stop_processing (optional). ' | ||
| 'Note: prohibited field cannot be modified through the API.' | ||
| ), | ||
| ) | ||
| actions: Optional[List[dict[str, Any]]] = Field( | ||
| None, | ||
| description=( | ||
| 'Array of action objects to update. Each object should have: ' | ||
| 'type (required), value (optional), active (optional), ' | ||
| 'order (optional), stop_processing (optional).' | ||
| ), | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate and inspect the UpdateRuleRequest model
cd /repo && wc -l src/lampyrid/models/lampyrid_models.pyRepository: RadCod3/LamPyrid
Length of output: 115
🏁 Script executed:
# Get the context around the mentioned lines to see the full UpdateRuleRequest definition
sed -n '1200,1260p' src/lampyrid/models/lampyrid_models.pyRepository: RadCod3/LamPyrid
Length of output: 2403
🏁 Script executed:
# Check the firefly_models.py file for RuleTriggerUpdate and RuleActionUpdate
sed -n '3500,3600p' src/lampyrid/models/firefly_models.pyRepository: RadCod3/LamPyrid
Length of output: 4421
🏁 Script executed:
# Search for the TestRuleRequest class definition
rg 'class TestRuleRequest' -A 20 src/lampyrid/models/lampyrid_models.pyRepository: RadCod3/LamPyrid
Length of output: 741
🏁 Script executed:
# Search for RuleTriggerUpdate and RuleActionUpdate to verify the type field
rg 'class RuleTriggerUpdate|class RuleActionUpdate' -A 15 src/lampyrid/models/firefly_models.pyRepository: RadCod3/LamPyrid
Length of output: 1479
🏁 Script executed:
# Check RuleTriggerUpdate and RuleActionUpdate model_config
rg 'class RuleTriggerUpdate|class RuleActionUpdate' -A 30 src/lampyrid/models/firefly_models.py | grep -E '(class|model_config|extra)'Repository: RadCod3/LamPyrid
Length of output: 195
🏁 Script executed:
# Search for how UpdateRuleRequest is used - where triggers are converted/validated
rg 'UpdateRuleRequest' src/ -A 5 -B 2 | head -100Repository: RadCod3/LamPyrid
Length of output: 2395
🏁 Script executed:
# Check if there's any validation model in between
fd -e py src/ | xargs grep -l 'RuleTriggerUpdate\|RuleActionUpdate' | head -5Repository: RadCod3/LamPyrid
Length of output: 381
🏁 Script executed:
# Check if RuleTriggerUpdate and RuleActionUpdate are used in parsing logic
rg 'model_validate|parse_obj' src/ -B 3 -A 3 | grep -E '(RuleTriggerUpdate|RuleActionUpdate|model_validate|parse_obj)'Repository: RadCod3/LamPyrid
Length of output: 2630
🏁 Script executed:
# Get the full implementation of update_rule in RuleService
sed -n '/async def update_rule/,/^ async def\|^class /p' src/lampyrid/services/rules.py | head -80Repository: RadCod3/LamPyrid
Length of output: 1595
🏁 Script executed:
# Check the update_rule implementation in tools/rules.py too
sed -n '/async def update_rule/,/^ async def\|^ `@/p`' src/lampyrid/tools/rules.py | head -80Repository: RadCod3/LamPyrid
Length of output: 610
🏁 Script executed:
# Look for where the triggers/actions are handled - check if they're passed as dicts or converted
rg 'triggers|actions' src/lampyrid/services/rules.py -B 2 -A 2Repository: RadCod3/LamPyrid
Length of output: 2994
🏁 Script executed:
# Get full definition of RuleTriggerUpdate
rg 'class RuleTriggerUpdate' -A 25 src/lampyrid/models/firefly_models.pyRepository: RadCod3/LamPyrid
Length of output: 1365
🏁 Script executed:
# Get full definition of RuleActionUpdate
rg 'class RuleActionUpdate' -A 25 src/lampyrid/models/firefly_models.pyRepository: RadCod3/LamPyrid
Length of output: 1228
🏁 Script executed:
# Check if these models inherit model_config from a base class or parent
rg 'class RuleTriggerUpdate|class RuleActionUpdate' -B 5 src/lampyrid/models/firefly_models.py | grep -E '(class|BaseModel|Config)'Repository: RadCod3/LamPyrid
Length of output: 130
🏁 Script executed:
# Verify Pydantic default behavior: when extra fields are provided, does it ignore or forbid?
# Check if BaseModel in firefly_models has any base config
head -50 src/lampyrid/models/firefly_models.py | grep -E '(import|Config|BaseModel|ConfigDict)'Repository: RadCod3/LamPyrid
Length of output: 301
🏁 Script executed:
# Check what happens when RuleTriggerUpdate is given extra fields
# by examining any existing tests or examples
fd -type f -name '*.py' src/ | xargs grep -l 'RuleTriggerUpdate\|RuleActionUpdate' | grep testRepository: RadCod3/LamPyrid
Length of output: 230
🏁 Script executed:
# Look for actual usage of these models to see if extra fields would be problematic
rg 'RuleTriggerUpdate\|RuleActionUpdate' -B 5 -A 5 src/ --type py | head -80Repository: RadCod3/LamPyrid
Length of output: 42
🏁 Script executed:
# Check Pydantic's default behavior for extra fields without explicit config
# Let's verify by looking at test examples or how the models are documented
rg 'model_config|ConfigDict.*extra' src/lampyrid/models/firefly_models.py | head -20Repository: RadCod3/LamPyrid
Length of output: 42
🏁 Script executed:
# Search for any validation error handling related to triggers/actions
rg 'prohibited|trigger.*extra|action.*extra' src/lampyrid/ -A 2 -B 2Repository: RadCod3/LamPyrid
Length of output: 2811
🏁 Script executed:
# Check if there are any validators on RuleTriggerUpdate or RuleActionUpdate
rg '@field_validator|@model_validator' src/lampyrid/models/firefly_models.py -A 3 | grep -A 3 'RuleTriggerUpdate\|RuleActionUpdate'Repository: RadCod3/LamPyrid
Length of output: 42
🏁 Script executed:
# Check the actual Pydantic behavior: does Pydantic default to ignoring extra fields?
# Look for how firefly_models uses BaseModel
python3 << 'EOF'
from pydantic import BaseModel, ValidationError
# Test default Pydantic behavior
class TestModel(BaseModel):
type: str | None = None
value: str | None = None
# Test with extra field
try:
obj = TestModel(**{"type": "test", "prohibited": True})
print(f"Extra field allowed: {obj}")
print(f"prohibited field in model_dump: {'prohibited' in obj.model_dump()}")
print(f"model_dump output: {obj.model_dump()}")
except ValidationError as e:
print(f"Validation error: {e}")
# Test with empty dict
try:
obj2 = TestModel(**{})
print(f"Empty dict allowed: {obj2}")
except ValidationError as e:
print(f"Validation error on empty dict: {e}")
EOFRepository: RadCod3/LamPyrid
Length of output: 233
🏁 Script executed:
# Verify that extra fields like "prohibited" are silently dropped in actual update scenario
python3 << 'EOF'
from pydantic import BaseModel, ValidationError
from enum import Enum
class RuleTriggerKeyword(str, Enum):
DESCRIPTION = "description"
TAG = "tag"
class RuleTriggerUpdate(BaseModel):
type: RuleTriggerKeyword | None = None
value: str | None = None
# Simulate what happens in the update_rule function
test_payload = [
{"prohibited": True}, # Only prohibited, no type - should this fail?
{"type": "description", "prohibited": True}, # Extra field that should be forbidden
{}, # Empty object - no type required?
]
for item in test_payload:
try:
update = RuleTriggerUpdate(**item)
print(f"Input: {item}")
print(f" Accepted: {update}")
print(f" Has type: {update.type is not None}")
print()
except ValidationError as e:
print(f"Input: {item}")
print(f" Rejected: {e}")
print()
EOFRepository: RadCod3/LamPyrid
Length of output: 346
🏁 Script executed:
# Check if the Firefly API itself would reject these malformed payloads
# Look at the RuleUpdate model that gets sent to Firefly
rg 'class RuleUpdate' -A 30 src/lampyrid/models/firefly_models.pyRepository: RadCod3/LamPyrid
Length of output: 1412
🏁 Script executed:
# Check what the client.update_rule does with the RuleUpdate object
sed -n '/async def update_rule/,/^ async def /p' src/lampyrid/clients/firefly.py | head -50Repository: RadCod3/LamPyrid
Length of output: 483
Tighten the trigger/action payload schema to prevent silent field dropping.
The triggers and actions fields accept raw dict[str, Any], and the downstream RuleTriggerUpdate / RuleActionUpdate models keep type optional without forbidding extra fields. This allows malformed payloads like {"triggers":[{}]} or {"triggers":[{"prohibited": true}]} to pass validation here; extra fields are silently dropped rather than rejected. Switch to dedicated nested request models with required type and extra='forbid' to fail fast and clearly communicate API constraints.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lampyrid/models/lampyrid_models.py` around lines 1224 - 1240, The
triggers and actions fields currently use Optional[List[dict[str, Any]]] which
allows malformed payloads and silent dropping of unknown fields; change these to
use the dedicated nested request models (e.g., replace types with
Optional[List[RuleTriggerUpdate]] and Optional[List[RuleActionUpdate]]), ensure
the RuleTriggerUpdate and RuleActionUpdate classes mark the type field as
required and set Config.extra = 'forbid' (or equivalent Pydantic config) so
unknown fields are rejected, and update any validators/usage in
lampyrid_models.py to reflect the new types so invalid payloads like {} or
{"prohibited": true} fail validation immediately.
| class TestRuleRequest(BaseModel): | ||
| """Request model for testing a rule (preview matches).""" | ||
|
|
||
| model_config = ConfigDict(extra='forbid') |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, verify the file exists and examine the lines in question
head -1250 src/lampyrid/models/lampyrid_models.py | tail -20 | cat -nRepository: RadCod3/LamPyrid
Length of output: 798
🏁 Script executed:
# Check if there are any existing __test__ = False patterns in the codebase
rg '__test__\s*=\s*False' --type pyRepository: RadCod3/LamPyrid
Length of output: 42
🏁 Script executed:
# Search for any pytest configuration or warnings related to TestRuleRequest
rg 'TestRuleRequest|PytestCollectionWarning' --type pyRepository: RadCod3/LamPyrid
Length of output: 546
🏁 Script executed:
# Check for other classes with "Test" prefix in the models file to see if they have the same issue
rg 'class Test\w+\(' src/lampyrid/models/lampyrid_models.pyRepository: RadCod3/LamPyrid
Length of output: 93
🏁 Script executed:
# Look for pytest configuration files
fd -e 'pytest.ini' -o -e 'pyproject.toml' -o -e 'setup.cfg' | head -5 | xargs -I {} sh -c 'echo "=== {} ===" && cat {}'Repository: RadCod3/LamPyrid
Length of output: 176
🏁 Script executed:
# Check if there are any references to pytest collection warnings in the codebase or CI config
find . -name '.github' -o -name 'pytest.ini' -o -name 'pyproject.toml' | xargs grep -l 'pytest\|collection' 2>/dev/null | head -5Repository: RadCod3/LamPyrid
Length of output: 76
🏁 Script executed:
# Search for any CI configuration files that might mention the warning
find . -type f \( -name '*.yml' -o -name '*.yaml' \) | xargs grep -l 'pytest\|test' 2>/dev/null | head -5Repository: RadCod3/LamPyrid
Length of output: 204
🏁 Script executed:
# Check pyproject.toml for pytest configuration
cat pyproject.toml | grep -A 20 '\[tool.pytest'Repository: RadCod3/LamPyrid
Length of output: 698
Add __test__ = False to prevent pytest collection.
Pytest is configured with python_classes = ["Test*"] (see pyproject.toml), which causes it to collect TestRuleRequest as a test class, triggering a PytestCollectionWarning in CI. Adding __test__ = False prevents this collection without renaming the public model.
Suggested fix
class TestRuleRequest(BaseModel):
"""Request model for testing a rule (preview matches)."""
+ __test__ = False
model_config = ConfigDict(extra='forbid')📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| class TestRuleRequest(BaseModel): | |
| """Request model for testing a rule (preview matches).""" | |
| model_config = ConfigDict(extra='forbid') | |
| class TestRuleRequest(BaseModel): | |
| """Request model for testing a rule (preview matches).""" | |
| __test__ = False | |
| model_config = ConfigDict(extra='forbid') |
🧰 Tools
🪛 GitHub Actions: CI
[warning] 1243-1243: PytestCollectionWarning: cannot collect test class 'TestRuleRequest' because it has a init constructor (from: tests/unit/test_rules_service.py).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lampyrid/models/lampyrid_models.py` around lines 1243 - 1246, The
TestRuleRequest Pydantic model is being collected by pytest due to the "Test*"
class pattern; add a class attribute __test__ = False inside the TestRuleRequest
class (e.g., directly under the docstring and above model_config) so pytest will
skip collecting it, leaving the class name unchanged and keeping model_config =
ConfigDict(extra='forbid') intact.
| # Filter by trigger value pattern if specified | ||
| if trigger_pattern: | ||
| trigger_values = [t.value for t in rule_attrs.triggers if t.value] | ||
| if not any(trigger_pattern.search(v) for v in trigger_values): | ||
| continue | ||
|
|
||
| # Filter by action type keyword if specified | ||
| if req.action_type: | ||
| action_keywords = [a.type.value for a in rule_attrs.actions] | ||
| if not any(req.action_type.lower() in kw.lower() for kw in action_keywords): | ||
| continue | ||
|
|
||
| # Filter by action value pattern if specified | ||
| if action_pattern: | ||
| action_values = [a.value for a in rule_attrs.actions if a.value] | ||
| if not any(action_pattern.search(v) for v in action_values): |
There was a problem hiding this comment.
Don’t drop empty values before regex matching.
if t.value / if a.value excludes empty strings, so patterns like ^$ or .* can never match rules whose trigger/action value is intentionally blank. Filter only None here.
Suggested fix
- trigger_values = [t.value for t in rule_attrs.triggers if t.value]
+ trigger_values = [
+ t.value for t in rule_attrs.triggers if t.value is not None
+ ]
if not any(trigger_pattern.search(v) for v in trigger_values):
continue
...
- action_values = [a.value for a in rule_attrs.actions if a.value]
+ action_values = [
+ a.value for a in rule_attrs.actions if a.value is not None
+ ]
if not any(action_pattern.search(v) for v in action_values):
continue🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lampyrid/services/rules.py` around lines 106 - 121, The code currently
excludes empty-string trigger/action values by using truthy checks (e.g., list
comprehensions in the trigger_values and action_values where you use "if
t.value" and "if a.value"), which prevents regexes like ^$ or .* from matching
intentionally-blank values; update the filters to only exclude None (use
explicit None checks such as "if t.value is not None" and "if a.value is not
None") so empty strings remain in trigger_values and action_values for pattern
matching while still filtering out missing values in rule_attrs.triggers and
rule_attrs.actions.
3 participants
Hi! Thanks for a great tool. I really needed rules functionality so I've added it to my fork. Hoping you might be happy to add it to the main branch.
Note: I've also added a one-click install to add this to home assistant, which many people use to host firefly iii. (See https://github.com/edmundhighcock/hassio-repository/tree/main/fireflyiii-mcp).
Claude summary
Add comprehensive rules functionality to manage Firefly III rules:
Implementation includes:
This brings the total tool count from 22 to 27.