[betinacosta]

Disclaimer: Do not include SAP-internal or customer-specific information in this PR (e.g. internal system URLs, customer names, tenant IDs, or confidential configurations). This is a public repository.

Description

_fetch_auth_token() in agentgateway/_lob.py previously failed on the first transient error from the BTP Destination Service — e.g. an OAuth2 token endpoint blip or an unparseable destination response — with no retry. This PR wraps the function body in a retry loop of up to 3 attempts with exponential backoff (1 s, 2 s). MCPServerNotFoundError is excluded from retries: it signals a genuine missing resource (no destination configured, no auth token returned, or empty auth header), not a transient failure, so it is re-raised immediately.

Type of Change

• Bug fix (non-breaking change that fixes an issue)
• New feature (non-breaking change that adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)
• Documentation update
• Code refactoring
• Dependency update

How to Test

1. Run the new unit test class: pytest tests/agentgateway/unit/test_lob.py::TestFetchAuthTokenRetry -v
2. Confirm all 6 tests pass and that time.sleep is never called in production paths (only in retry scenarios).
3. Confirm the existing TestFetchAuthToken suite still passes (no regressions in the non-retry path).
4. Optional: trigger a transient Destination Service failure (e.g. by temporarily misconfiguring credentials in a dev landscape) and verify retry warnings appear in the logs before a successful resolution.

Checklist

• I have read the Contributing Guidelines
• I have verified that my changes solve the issue
• I have added/updated automated tests to cover my changes
• All tests pass locally
• I have verified that my code follows the Code Guidelines
• I have updated documentation (if applicable)
• I have added type hints for all public APIs
• My code does not contain sensitive information (credentials, tokens, etc.)
• I have followed Conventional Commits for commit messages

Additional Notes

Code Review

• [A1] PR template — This template has been filled in by /prep-pr; confirm checklist items marked [ ] before merging.
• [A3] Issue linked — No issue number was found in the branch name or commits. Please replace Closes #<issue_number> with the real issue number, or remove the line if there is no tracked issue.
• [E1] Non-asserting test check — tests/agentgateway/unit/test_lob.py:283: mock_client.return_value.get_destination.call_count == 1 is a bare expression (not an assert), so it is silently discarded. Change it to assert mock_client.return_value.get_destination.call_count == 1 to actually enforce the no-retry behaviour.

Security Review

• [S3] Exception logging may expose OAuth2 error details — logger.warning(..., exc, ...) logs the full exception message string. If the BTP Destination Service OAuth2 endpoint returns error responses that include token fragments, tenant identifiers, or other sensitive strings, those may appear in application logs. Consider logging type(exc).__name__ only, or truncating/sanitising the message before logging.

[betinacosta]

PR Review: #433: fix(agent-gateway): add retry strategy

Author: betinacosta Branch: fix/agw-user-auth-retry → main
Verdict: ⚠️ Needs Minor Work
Summary: Solid, well-tested retry implementation with two minor issues — an unfilled PR template and a non-asserting call count check in the test suite.

---

A: Process & Compliance

#	Criterion	Status	Finding
A1	PR template complete	❌	PR body still has the raw template with no checkboxes ticked, no description of the change, no actual issue linked, no test steps, and "Type of Change" unchecked.
A2	Conventional Commits	✅	PR title fix(agent-gateway): add retry strategy is valid conventional-commits format; commit-validation CI job passes.
A3	Issue linked	⚠️	Body still contains the placeholder Closes #<issue_number> — no real issue number provided.
A4	AI-generated code disclosure	➖	Code does not appear AI-generated in style or structure.

B: Security & Sensitive Data

#	Criterion	Status	Finding
B1	No sensitive data in code	✅	No credentials, tokens, internal URLs, tenant IDs, or customer data in the diff.
B2	No sensitive data in PR body	✅	PR body contains only the unfilled template placeholder text — nothing sensitive.

C: Code Quality

#	Criterion	Status	Finding
C1	CI checks passing	⚠️	Integration Tests is still pending; all other required jobs pass (Code Quality, Tests & Coverage, Build & Package, Commit Validation, Check Version Bump, Analyze (python)). "SDK review label present" is a gate check that will clear once this review completes.
C2	Version bump	➖	No manual version bump in pyproject.toml. The uv.lock change bumps ruff (dev tool) and widens setuptools upper bound — both are legitimate lock-file drift, not a version-bump violation.
C3	Type hints	✅	_fetch_auth_token already had full annotations; the retry refactor preserves them. `last_exc: Exception
C4	No hardcoded values	✅	Magic numbers extracted into module-level constants _FETCH_AUTH_MAX_ATTEMPTS = 3 and _FETCH_AUTH_BACKOFF_BASE = 1.0 at _lob.py:66-67.
C5	Import organization	✅	import time added at the top-level alongside existing stdlib imports (_lob.py:11). No lazy imports introduced.
C6	Naming conventions	✅	New constants follow SCREAMING_SNAKE_CASE; no new enums or files.
C7	No unused code	✅	time import is used at _lob.py:169. No dead code introduced.
C8	No unjustified new dependencies	✅	time is stdlib. The ruff and setuptools changes in uv.lock are minor version bumps, not new runtime deps.
C9	Proto code freshness	➖	No proto files changed.

D: API & Design

#	Criterion	Status	Finding
D1	API future-proofing	✅	Constants allow future callers to adjust retry policy; no new public API surface introduced.
D2	Public API hygiene	✅	_fetch_auth_token is a private helper (leading underscore); no public __all__ changes.
D3	Breaking changes marked	➖	No breaking changes.
D4	Pagination & tenant filtering	➖	Not applicable to this change.
D5	Telemetry instrumentation	➖	No new client methods or module added.
D6	Multi-tenancy support	➖	Retry logic is transparent to the multi-tenancy layer already handled by _fetch_auth_token's tenant_subdomain param.

E: Tests & Documentation

#	Criterion	Status	Finding
E1	Tests added/updated	✅	New TestFetchAuthTokenRetry class with 6 tests covering: success on first attempt, transient error → retry → success, full exhaustion, MCPServerNotFoundError bypass, backoff sequence, and exact delay values. time.sleep is patched to avoid real delays.
E2	Documentation quality	✅	Docstring for _fetch_auth_token updated with retry semantics (_lob.py:108-123).
E2a	Multi-tenancy documentation	➖	Not a new module; no user-guide changes required.
E3	Module structure compliance	➖	No new module added.

---

❌ Blocking Issues

• [A1]: PR body is the unfilled template — description, type of change, checklist, and test steps are all empty placeholders. Please fill in at minimum: what the bug was and when it occurs, the type of change (Bug fix), and the checklist items.

⚠️ Non-Blocking Suggestions

• [A3]: Replace the Closes #<issue_number> placeholder with the actual issue number, or remove the line if no issue tracks this bug.
• [E1]: tests/agentgateway/unit/test_lob.py:283 — mock_client.return_value.get_destination.call_count == 1 is a bare expression, not an assertion. It evaluates to True or False and is silently discarded. Change it to assert mock_client.return_value.get_destination.call_count == 1 to actually verify the no-retry behaviour.
• [C1]: Integration Tests is pending — confirm it passes before merging.

✅ Things Done Well

• The retry logic correctly exempts MCPServerNotFoundError from retry (it is a client-side "resource not found", not a transient failure) — this is the right semantic distinction.
• Module-level constants (_FETCH_AUTH_MAX_ATTEMPTS, _FETCH_AUTH_BACKOFF_BASE) with an inline comment documenting the resulting delay sequence (1s, 2s) make the policy easy to audit and change.
• time.sleep is properly patched in all retry tests, so the test suite runs in milliseconds regardless of the configured backoff.
• The type: ignore[misc] annotation on raise last_exc is correct — mypy/ty cannot prove last_exc is non-None at that point in the control flow (the loop always executes at least once, so it is safe).

[betinacosta]

PR Review: #433: fix(agent-gateway): add retry strategy

Author: betinacosta Branch: fix/agw-user-auth-retry → main
Verdict: ✅ Ready to Merge
Summary: Clean, well-tested retry implementation — all CI passes, PR body is fully filled in, and the previously flagged bare-expression test issue has been corrected.

---

A: Process & Compliance

#	Criterion	Status	Finding
A1	PR template complete	✅	Description, Type of Change (Bug fix ✅), How to Test (4 numbered steps), and all 9 checklist items filled in. Two conditional items (new module e2e evidence, security-review) are N/A and not present.
A2	Conventional Commits	✅	PR title fix(agent-gateway): add retry strategy is valid conventional-commits format; Commit Validation CI job passes.
A3	Issue linked	⚠️	No Closes #N in the PR body — the "Related Issue" section is omitted entirely. If a GitHub issue tracks this bug, link it.
A4	AI-generated code disclosure	➖	Code does not appear AI-generated in style or structure.

B: Security & Sensitive Data

#	Criterion	Status	Finding
B1	No sensitive data in code	✅	No credentials, tokens, internal URLs, tenant IDs, or customer data in the diff.
B2	No sensitive data in PR body	✅	PR body contains only public implementation details — no sensitive or SAP-internal content.

C: Code Quality

#	Criterion	Status	Finding
C1	CI checks passing	✅	All required jobs pass: Code Quality Checks, Tests & Coverage, Build & Package, Commit Validation, Check Version Bump, Analyze (python), Integration Tests.
C2	Version bump	➖	No manual version bump in pyproject.toml (still 0.62.0). The uv.lock changes bump ruff from 0.16.0 → 0.17.1 (dev-only) and widen the setuptools upper bound <84 → <85 — both are legitimate lock-file drift, not version-bump violations.
C3	Type hints	✅	_fetch_auth_token had full annotations before; the refactor preserves them. New variable last_exc: Exception | None = None at _lob.py:125 is properly annotated.
C4	No hardcoded values	✅	Retry policy extracted into module-level constants _FETCH_AUTH_MAX_ATTEMPTS = 3 (_lob.py:66) and _FETCH_AUTH_BACKOFF_BASE = 1.0 (_lob.py:67).
C5	Import organization	✅	import time added at the top-level alongside existing stdlib imports (_lob.py:11). No lazy imports.
C6	Naming conventions	✅	New constants follow SCREAMING_SNAKE_CASE. No new enums, files, or workflow files.
C7	No unused code	✅	time is used at _lob.py:169. No dead code introduced.
C8	No unjustified new dependencies	✅	time is stdlib. ruff and setuptools changes in uv.lock are minor version updates to existing dev/build deps — no new runtime dependency added.
C9	Proto code freshness	➖	No proto files changed.

D: API & Design

#	Criterion	Status	Finding
D1	API future-proofing	✅	Module-level constants make the retry policy easy to audit and change without touching the logic. No new public API surface.
D2	Public API hygiene	✅	_fetch_auth_token is a private helper (leading underscore). No __all__ changes.
D3	Breaking changes marked	➖	No breaking changes.
D4	Pagination & tenant filtering	➖	Not applicable — retry is transparent to callers.
D5	Telemetry instrumentation	➖	No new client methods or module added.
D6	Multi-tenancy support	➖	Retry logic is transparent to the multi-tenancy layer already handled by _fetch_auth_token's tenant_subdomain param.

E: Tests & Documentation

#	Criterion	Status	Finding
E1	Tests added/updated	✅	New TestFetchAuthTokenRetry class (6 tests, test_lob.py:203-298) covers: first-attempt success, transient-error-then-success (1 retry), two failures-then-success (2 retries), full exhaustion, MCPServerNotFoundError bypass (line 283 correctly uses assert mock_client.return_value.get_destination.call_count == 1), and exact exponential delay sequence [1.0, 2.0]. time.sleep is patched in all retry tests.
E2	Documentation quality	✅	_fetch_auth_token docstring updated with retry semantics at _lob.py:108-123, including what is and isn't retried and the updated Raises: section.
E2a	Multi-tenancy documentation	➖	Not a new module; no user-guide changes required.
E3	Module structure compliance	➖	No new module added.

---

⚠️ Non-Blocking Suggestions

• [A3]: No issue linked — if a GitHub issue tracks this transient-auth bug, add Closes #N to the PR body.
• [B1 / Security note]: logger.warning("... %s ...", exc, ...) at _lob.py:163 logs the full exception message. BTP Destination Service OAuth2 errors can occasionally embed partial token fragments or tenant identifiers in their error bodies. Consider logging type(exc).__name__ only, or sanitising the message, to avoid leaking sensitive strings into application logs. This is advisory — it is not a definite information-disclosure risk, just a hardening recommendation.

✅ Things Done Well

• MCPServerNotFoundError is correctly excluded from retries — it signals a genuine missing resource, not a transient failure. This is the semantically correct distinction and prevents silent retry-loops on misconfigured destinations.
• Module-level constants (_FETCH_AUTH_MAX_ATTEMPTS, _FETCH_AUTH_BACKOFF_BASE) with an inline comment documenting the resulting delay sequence make the policy transparent and easy to tune.
• time.sleep is patched in every test that exercises retry paths, so the suite runs at full speed with no real waits.
• The type: ignore[misc] annotation on raise last_exc (_lob.py:171) is correct — mypy/ty cannot prove last_exc is non-None at that point (the loop always executes at least once), so the suppression is justified.
• The bare-expression issue flagged in the earlier review (call_count == 1 without assert) has been corrected — test_lob.py:283 now uses a proper assert.
• All CI checks pass, including Integration Tests.