Skip to content

Fix delete_transaction_rule reporting successful deletions as failures - #86

Open
ahnj wants to merge 1 commit into
robcerda:mainfrom
ahnj:fix/delete-rule-false-negative
Open

Fix delete_transaction_rule reporting successful deletions as failures#86
ahnj wants to merge 1 commit into
robcerda:mainfrom
ahnj:fix/delete-rule-false-negative

Conversation

@ahnj

@ahnj ahnj commented Aug 7, 2026

Copy link
Copy Markdown

Problem

delete_transaction_rule returns {"success": false, "message": "Rule was not deleted"} for deletions that actually succeed.

Monarch's deleteTransactionRule mutation returns deleted: false with no errors even when the rule was removed. The current guard treats that flag as authoritative:

if delete_result.get("deleted") is False:
    return json_success({"success": False, "message": "Rule was not deleted"})

This is more than cosmetic — the tool reports a completed destructive operation as not having happened, which invites the caller to retry or to act on state they believe is still present.

Verification against the live API

Deleted four real rules. Every call returned deleted: false; all four were genuinely gone, confirmed by re-querying the rule list (the IDs were absent) and by the total count dropping from 322 to 318.

Genuine failures never reach that branch. Deleting a non-existent rule raises TransportQueryError from the GraphQL layer:

gql.transport.exceptions.TransportQueryError:
{'message': 'Not found', 'path': ['deleteTransactionRule']}

which the surrounding except Exception already turns into an error result.

Change

Drop the deleted check. The explicit errors payload check is left in place as the only trustworthy in-payload failure signal.

Tests

test_delete_rule_not_found passed before and after because it mocks deleted: false together with an errors object, so the errors branch catches it first — the real-world shape was never covered.

Added test_delete_rule_success_with_false_deleted_flag for deleted: false + errors: None. It fails before this change and passes after. Full suite: 223 passed.

🤖 Generated with Claude Code

https://claude.ai/code/session_01KTJGjJh4qNHBGWdZW3UDPo

Monarch's deleteTransactionRule mutation returns `deleted: false` with no
errors even when the rule was successfully removed. Verified against the
live API: deleting four real rules returned `deleted: false` every time,
while the rules were in fact gone (confirmed by re-querying the rule list
and by the total count dropping from 322 to 318).

The existing guard treated that flag as authoritative, so every successful
deletion reported `{"success": false, "message": "Rule was not deleted"}`.
That is worse than a cosmetic bug: the tool reports a destructive operation
as having not happened, inviting the caller to retry or to believe stale
state is still present.

Genuine failures never reach that branch. Deleting a non-existent rule
raises TransportQueryError ("Not found") from the GraphQL layer, which the
surrounding except block already converts into an error result. The only
in-payload failure signal worth honouring is an explicit `errors` object,
which is left in place.

The existing "not found" test passed because it mocked `deleted: false`
*together with* an errors object, so the errors branch caught it first.
Added a test covering the real-world shape (`deleted: false`, `errors: None`);
it fails before this change and passes after.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KTJGjJh4qNHBGWdZW3UDPo
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant