fix: stop retrying after successful payment signing is rejected by merchant by rajuans · Pull Request #492 · aws/bedrock-agentcore-sdk-python

rajuans · 2026-05-21T18:52:16Z

Previously, when a signed payment was rejected (e.g., insufficient balance), the plugin would store failure state and trigger interrupt-based retries. This was unnecessary since a server-side rejection after successful signing is not recoverable by retrying.

Changes:

Add payment_signed_{toolUseId} flag to track successful signing
After successful signing + retry, if server returns 402 again, stop immediately
Signing failures still retry up to MAX_PAYMENT_RETRIES
Move retry count increment to after successful signing attempt
Remove _is_post_payment_failure (replaced by _has_successful_signing)

Issue #, if available:

Description of changes:

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

…rver Previously, when a signed payment was rejected (e.g., insufficient balance), the plugin would store failure state and trigger interrupt-based retries. This was unnecessary since a server-side rejection after successful signing is not recoverable by retrying. Changes: - Add payment_signed_{toolUseId} flag to track successful signing - After successful signing + retry, if server returns 402 again, stop immediately - Signing failures still retry up to MAX_PAYMENT_RETRIES - Move retry count increment to after successful signing attempt - Remove _is_post_payment_failure (replaced by _has_successful_signing)

- Preserve interrupt behavior: post-payment failure stores state and triggers interrupt so downstream consumers are notified - Remove dead code: _extract_payment_error_message (unreachable) - Remove TestExtractPaymentErrorMessage (tested removed method) - Clarify comment on retry counter increment - Add test: signing flag takes precedence over retry limit - Remove emojis from new integration test logger output

codecov-commenter · 2026-05-21T18:54:08Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
⚠️ Please upload report for BASE (main@c311682). Learn more about missing BASE report.

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #492   +/-   ##
=======================================
  Coverage        ?   89.40%           
=======================================
  Files           ?       83           
  Lines           ?     7633           
  Branches        ?     1143           
=======================================
  Hits            ?     6824           
  Misses          ?      514           
  Partials        ?      295

Flag	Coverage Δ
unittests	`89.40% <100.00%> (?)`

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

aidandaly24 · 2026-05-21T20:08:31Z

+            self._increment_payment_retry_count(event)
+
            # Validate tool input before processing payment
            tool_input = event.tool_use.get("input", {})


Nit: tool_input is already fetched at line 177 (tool_input = event.tool_use.get("input", {})) and is unchanged since. The fetch on line 221 is redundant.

aidandaly24 · 2026-05-21T20:09:45Z

+    def _mark_successful_signing(self, event: AfterToolCallEvent) -> None:
+        """Mark that signing succeeded for this tool use.

-        return False
-
-    @staticmethod
-    def _extract_payment_error_message(body: Optional[Dict[str, Any]]) -> str:
-        """Extract a human-readable error message from a 402 response body.
+        Called after generate_payment_header and apply_payment_header both succeed,
+        right before setting event.retry. If a subsequent 402 is received,
+        _has_successful_signing will return True indicating the failure is server-side.

        Args:
-            body: The extracted response body (may be None)
-
-        Returns:
-            Error message string, or "unknown error" if not extractable
+            event: The after tool call event
        """
-        if body and isinstance(body, dict):
-            error = body.get("error")
-            if isinstance(error, str) and error:
-                return error
-        return "unknown error"
+        tool_use_id = event.tool_use.get("toolUseId", "unknown")
+        signed_key = f"payment_signed_{tool_use_id}"
+        event.invocation_state[signed_key] = True


Nit: payment_signed_* flag is never cleared.

Once _mark_successful_signing sets the flag, it lives in invocation_state for the rest of the invocation. Same applies to payment_retry_count_* and payment_failure_* (pre-existing). For long-lived agent sessions with many paid tool calls, invocation_state accumulates keys without bound. Either clear the flag once the failure is delivered via before_tool_call's interrupt path, or add a comment confirming invocation_state is intentionally per-invocation and cleanup is not required.

aidandaly24 · 2026-05-21T20:10:12Z

@@ -710,3 +710,300 @@ def test_plugin_with_agent_name_initializes_payment_manager(self):
        assert agent is not None


Can you clean these up please?
Inline imports inside test methods violate project convention.

CLAUDE.md: "Always place imports at the top of files — never use inline imports within functions or methods."

Inline imports to hoist:

Line 731: from unittest.mock import MagicMock

Line 763: from unittest.mock import MagicMock, patch

Line 828: from unittest.mock import MagicMock, patch

Line 830: from bedrock_agentcore.payments.manager import PaymentError

Line 894: from unittest.mock import MagicMock, patch

Line 954: from unittest.mock import MagicMock, patch

Move all of these to the module's top-level imports.

aidandaly24 · 2026-05-21T20:10:49Z

+@pytest.mark.integration
+class TestPostPaymentFailureFlow:
+    """Hook-level integration tests for post-payment failure behavior.
+
+    Exercises the plugin's after_tool_call hook directly to verify the
+    retry logic end-to-end without requiring an LLM.
+    """


Mislabeled "integration" tests.

These tests live in tests_integ/, are tagged @pytest.mark.integration, and the docstring says "integration tests" — but they fully mock PaymentManager and never hit AWS. CI runners that filter -m "not integration" will skip them, and developers may assume they need credentials to run them.

Either:

Move the class to tests/ as unit tests, or

Update the class docstring (lines 717–721) to make explicit that these are mock-driven hook-flow scenarios that require no AWS credentials.

- plugin.py: drop redundant tool_input re-fetch (already bound at line 177) - plugin.py: document that invocation_state markers are intentionally per-invocation and do not need explicit cleanup - test_plugin_integration.py: hoist unittest.mock and PaymentError imports to module top, per project convention - test_plugin_integration.py: clarify TestPostPaymentFailureFlow docstring to flag it as mock-driven and credential-free

Raju Ansari added 2 commits May 21, 2026 10:52

rajuans requested a review from a team May 21, 2026 18:52

rajuans requested a deployment to manual-approval May 21, 2026 18:52 — with GitHub Actions Waiting

aidandaly24 reviewed May 21, 2026

View reviewed changes

aidandaly24 previously approved these changes May 21, 2026

View reviewed changes

aidandaly24 dismissed their stale review via 743b1a0 May 21, 2026 20:37

aidandaly24 requested a deployment to manual-approval May 21, 2026 20:38 — with GitHub Actions Waiting

Merge branch 'main' into retry-improvement

8c92119

aidandaly24 requested a deployment to manual-approval May 21, 2026 20:38 — with GitHub Actions Waiting

aidandaly24 approved these changes May 21, 2026

View reviewed changes

jariy17 approved these changes May 21, 2026

View reviewed changes

aidandaly24 merged commit 0b2b34f into aws:main May 21, 2026
23 of 33 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: stop retrying after successful payment signing is rejected by merchant#492

fix: stop retrying after successful payment signing is rejected by merchant#492
aidandaly24 merged 4 commits into
aws:mainfrom
rajuans:retry-improvement

rajuans commented May 21, 2026

Uh oh!

codecov-commenter commented May 21, 2026 •

edited

Loading

Uh oh!

aidandaly24 May 21, 2026

Uh oh!

aidandaly24 May 21, 2026

Uh oh!

aidandaly24 May 21, 2026

Uh oh!

aidandaly24 May 21, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

		@@ -710,3 +710,300 @@ def test_plugin_with_agent_name_initializes_payment_manager(self):
		assert agent is not None

Conversation

rajuans commented May 21, 2026

Uh oh!

codecov-commenter commented May 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

aidandaly24 May 21, 2026

Choose a reason for hiding this comment

Uh oh!

aidandaly24 May 21, 2026

Choose a reason for hiding this comment

Uh oh!

aidandaly24 May 21, 2026

Choose a reason for hiding this comment

Uh oh!

aidandaly24 May 21, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov-commenter commented May 21, 2026 •

edited

Loading