fix(SK-819, SK-821): provider error differentiation, blind retry fix, and upsert credentials#174
Open
ravibits wants to merge 3 commits into
Open
fix(SK-819, SK-821): provider error differentiation, blind retry fix, and upsert credentials#174ravibits wants to merge 3 commits into
ravibits wants to merge 3 commits into
Conversation
…s for provider errors - Add ScalekitToolException base class with tool_error_code, tool_error_message, execution_id properties - Add ScalekitToolRateLimitException (multiple-inherits ScalekitToolException + ScalekitTooManyRequestsException) - Add ScalekitToolUnauthorizedException (multiple-inherits ScalekitToolException + ScalekitUnauthorizedException) - Add ScalekitToolForbiddenException (multiple-inherits ScalekitToolException + ScalekitForbiddenException) - Update promote() to detect error_code == TOOL_ERROR and raise the appropriate tool exception - Fix grpc_exec: TOOL_ERROR errors raise immediately (no retry, no M2M refresh) - Fix grpc_exec: plain RESOURCE_EXHAUSTED (Scalekit 429) now surfaces immediately (no retry) - Add _extract_error_code() helper to read error_code from gRPC trailing metadata without full exception construction - Add 21-test suite covering Phase 1 baseline and Phase 2 new behavior Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
WalkthroughThe SDK adds a tool-error exception hierarchy ( ChangesTool Error Exception Hierarchy and gRPC Routing
Connected Account Upsert Behavior
Sequence Diagram(s)sequenceDiagram
participant Caller
participant CoreClient
participant ScalekitServerException
participant ToolExceptions
Caller->>CoreClient: grpc_exec(fn, *args)
CoreClient->>CoreClient: fn(*args) raises grpc.RpcError
CoreClient->>ScalekitServerException: _extract_error_code(exp)
alt error_code == "TOOL_ERROR"
ScalekitServerException-->>CoreClient: "TOOL_ERROR"
CoreClient->>ScalekitServerException: promote(exp)
ScalekitServerException->>ToolExceptions: ScalekitToolRateLimitException / ScalekitToolUnauthorizedException / ScalekitToolForbiddenException
ToolExceptions-->>Caller: raises immediately (no retry)
else StatusCode.RESOURCE_EXHAUSTED
CoreClient->>ScalekitServerException: promote(exp)
ScalekitServerException-->>Caller: raises ScalekitTooManyRequestsException immediately
else StatusCode.UNAUTHENTICATED (no TOOL_ERROR)
CoreClient->>CoreClient: __authenticate_client() refresh
CoreClient->>CoreClient: grpc_exec(fn, *args, retry-1)
CoreClient-->>Caller: result
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
…when account exists When an account already exists, the "get" path previously returned it as-is and silently dropped any authorization_details provided by the caller. Fix: if authorization_details is provided and the account already exists, call update_connected_account to apply the credentials regardless of the account's current status (PENDING_AUTH, ACTIVE, EXPIRED, DISCONNECTED). Also adds upsert_connected_account as an alias on ActionClient — identical signature, preferred name going forward. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
ravibits
changed the title
New ScalekitTool* exception hierarchy, upsert_connected_account alias, and changed grpc_exec retry behavior for provider errors. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
scalekit/common/exceptions.py (1)
106-121: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueAvoid silently swallowing parse failures in
_extract_error_code.The blind
except Exception: passmeans any failure while reading the trailing metadata makes this returnNone, which causesgrpc_execto fall through to its retry/refresh path — i.e. the precise behavior SK-819 is trying to eliminate. At minimum log the swallowed exception so a parsing regression is debuggable. The localfrom grpc_status import rpc_statusis also redundant sincerpc_statusis already used at module scope (Line 95).♻️ Suggested change
`@staticmethod` def _extract_error_code(error: grpc.RpcError) -> str | None: """ Extract error_code from gRPC trailing metadata without constructing a full exception """ - from grpc_status import rpc_status try: status = rpc_status.from_call(error) if status is None: return None for detail in status.details: info = ErrorInfo() detail.Unpack(info) if info.error_code: return info.error_code - except Exception: - pass + except Exception: + logger.debug("Failed to extract error_code from gRPC trailing metadata", exc_info=True) return NoneAs per static analysis hints (Ruff S110
try-except-passand BLE001 blindexcept).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scalekit/common/exceptions.py` around lines 106 - 121, Replace the bare `except Exception: pass` block in the `_extract_error_code` method with proper exception handling that logs the swallowed exception to aid debugging of parsing regressions. Additionally, remove the local `from grpc_status import rpc_status` import statement inside the method since `rpc_status` is already imported at the module scope and can be reused directly.Source: Linters/SAST tools
scalekit/core.py (1)
166-170: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueEarly-exit routing is correct; consider
raise ... from expfor cleaner tracebacks.The TOOL_ERROR pre-check and explicit
RESOURCE_EXHAUSTEDbranch correctly bypass retry/refresh. Minor: re-raising inside theexceptblock withoutfromchains an implicit__context__; Ruff flags this on Lines 169 and 181.♻️ Suggested change
if error_code == "TOOL_ERROR": - raise ScalekitServerException.promote(exp) + raise ScalekitServerException.promote(exp) from exp ... elif exp.code() == grpc.StatusCode.RESOURCE_EXHAUSTED: # Surface Scalekit rate-limits immediately — retrying triples the damage - raise ScalekitServerException.promote(exp) + raise ScalekitServerException.promote(exp) from expAs per static analysis hints (Ruff B904).
Also applies to: 179-181
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scalekit/core.py` around lines 166 - 170, When re-raising exceptions in the except block, use explicit exception chaining with the `from` keyword to provide clearer tracebacks. In the TOOL_ERROR handling block where ScalekitServerException.promote(exp) is raised, change the raise statement to use `raise ScalekitServerException.promote(exp) from exp` instead of the current implicit chaining. Apply the same fix to the similar exception re-raise pattern at lines 179-181 as indicated by the review comment. This makes the exception chain explicit and follows Ruff B904 guidelines.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scalekit/actions/actions.py`:
- Around line 794-809: The docstring for the api_config parameter (around line
775) currently states it is "only used when creating", but the new upsert logic
now forwards api_config to update_connected_account when authorization_details
are supplied, meaning it also applies to existing accounts. Update the
api_config parameter docstring to clarify that it is used for both creating new
connected accounts and updating existing ones through the upsert mechanism, so
callers understand that providing api_config could modify an existing account's
configuration.
- Around line 794-809: The docstring for the `api_config` parameter (around line
775) currently states it is "only used when creating," but the upsert path now
passes `api_config` to the `update_connected_account` method when
`authorization_details` are supplied. Update the docstring to clarify that
`api_config` is used both when creating and when updating connected accounts in
the upsert scenario.
---
Nitpick comments:
In `@scalekit/common/exceptions.py`:
- Around line 106-121: Replace the bare `except Exception: pass` block in the
`_extract_error_code` method with proper exception handling that logs the
swallowed exception to aid debugging of parsing regressions. Additionally,
remove the local `from grpc_status import rpc_status` import statement inside
the method since `rpc_status` is already imported at the module scope and can be
reused directly.
In `@scalekit/core.py`:
- Around line 166-170: When re-raising exceptions in the except block, use
explicit exception chaining with the `from` keyword to provide clearer
tracebacks. In the TOOL_ERROR handling block where
ScalekitServerException.promote(exp) is raised, change the raise statement to
use `raise ScalekitServerException.promote(exp) from exp` instead of the current
implicit chaining. Apply the same fix to the similar exception re-raise pattern
at lines 179-181 as indicated by the review comment. This makes the exception
chain explicit and follows Ruff B904 guidelines.
🪄 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: cc52783d-a7b7-46ce-8d64-cdbd423393d9
📒 Files selected for processing (5)
scalekit/_version.pyscalekit/actions/actions.pyscalekit/common/exceptions.pyscalekit/core.pytests/test_sk819_retry_behavior.py
Comment on lines
+794
to
+809
|
|
||
| # True upsert: if credentials were supplied, apply them regardless of | ||
| # the account's current status (PENDING_AUTH, ACTIVE, EXPIRED, DISCONNECTED). | ||
| if authorization_details: | ||
| update_response = self.update_connected_account( | ||
| connection_name=connection_name, | ||
| identifier=identifier, | ||
| authorization_details=authorization_details, | ||
| organization_id=organization_id, | ||
| user_id=user_id, | ||
| api_config=api_config | ||
| ) | ||
| return CreateConnectedAccountResponse(connected_account=update_response.connected_account) | ||
|
|
||
| return CreateConnectedAccountResponse(connected_account=existing_response.connected_account) | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Update the api_config docstring — it is no longer "only used when creating".
The new upsert branch forwards api_config to update_connected_account (Line 804), so api_config now also applies to existing accounts. The parameter doc at Line 775 (only used when creating) is now misleading and could cause callers to unknowingly mutate an existing account's API config.
📝 Suggested docstring update (Line 775)
- :param api_config: Optional API configuration for the connected account (optional, only used when creating)
+ :param api_config: Optional API configuration for the connected account. Applied when
+ creating a new account, and also when updating an existing account if
+ ``authorization_details`` are supplied. (optional)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scalekit/actions/actions.py` around lines 794 - 809, The docstring for the
api_config parameter (around line 775) currently states it is "only used when
creating", but the new upsert logic now forwards api_config to
update_connected_account when authorization_details are supplied, meaning it
also applies to existing accounts. Update the api_config parameter docstring to
clarify that it is used for both creating new connected accounts and updating
existing ones through the upsert mechanism, so callers understand that providing
api_config could modify an existing account's configuration.
🩺 Stability & Availability | 🟡 Minor
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect the three response models' constructors and connected_account fields.
fd -t f -e py | xargs rg -nP -C3 '\bclass\s+(CreateConnectedAccountResponse|GetConnectedAccountAuthResponse|UpdateConnectedAccountResponse)\b'
echo '--- connected_account references in these classes ---'
fd -t f -e py | xargs rg -nP -C2 '\bconnected_account\b'Repository: scalekit-inc/scalekit-sdk-python
Length of output: 50390
🏁 Script executed:
sed -n '770,810p' scalekit/actions/actions.pyRepository: scalekit-inc/scalekit-sdk-python
Length of output: 1989
🏁 Script executed:
# Check if update_connected_account accepts api_config parameter
rg -A 15 'def update_connected_account' scalekit/actions/actions.py | head -25Repository: scalekit-inc/scalekit-sdk-python
Length of output: 727
Update docstring to reflect api_config usage in upsert path.
The docstring at line 775 states that api_config is "only used when creating," but the upsert branch (lines 801–806) now passes api_config to update_connected_account when authorization_details are supplied. Update the docstring to clarify that api_config is used when creating or updating.
The response object contracts are sound: CreateConnectedAccountResponse, GetConnectedAccountAuthResponse, and UpdateConnectedAccountResponse all expose a .connected_account attribute of type ConnectedAccount, and all constructors accept it as a keyword argument.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scalekit/actions/actions.py` around lines 794 - 809, The docstring for the
`api_config` parameter (around line 775) currently states it is "only used when
creating," but the upsert path now passes `api_config` to the
`update_connected_account` method when `authorization_details` are supplied.
Update the docstring to clarify that `api_config` is used both when creating and
when updating connected accounts in the upsert scenario.
Avinash-Kamath
approved these changes
Jun 25, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes SK-819 — blind retries and missing provider error differentiation for tool execution errors.
grpc_execnow checkserror_code == "TOOL_ERROR"from gRPCErrorInfodetails before any retry decision. Provider errors raise immediately — no more 3x retry amplification on provider 429s.ScalekitToolExceptionbase (extendsScalekitServerException) with typed subclasses via multiple inheritance for full backward + forward compat:ScalekitToolRateLimitException(ScalekitToolException, ScalekitTooManyRequestsException)— provider 429ScalekitToolUnauthorizedException(ScalekitToolException, ScalekitUnauthorizedException)— provider 401ScalekitToolForbiddenException(ScalekitToolException, ScalekitForbiddenException)— provider 403ScalekitToolExceptiondirectly — any other provider errorRESOURCE_EXHAUSTEDwithoutTOOL_ERRORno longer retried — caller owns retry strategy.UNAUTHENTICATEDwithoutTOOL_ERRORstill refreshes token and retries. Provider 401 raisesScalekitToolUnauthorizedExceptionimmediately.Backward compatibility
Existing
except ScalekitTooManyRequestsExceptionblocks still catchScalekitToolRateLimitExceptionvia inheritance. No customer code changes required unless they want the new granular exception types.Behavior change to note in release notes: Provider errors previously retried up to 3 times now surface immediately. Scalekit 429s also surface immediately instead of retrying.
Test plan
tests/test_sk819_retry_behavior.py— all passing🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes