Surface low PRO API credit balance via advisory note#402
Open
akolotov wants to merge 9 commits into
Open
Conversation
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Wondering what really moved? Review this PR in Change Stack to inspect semantic changes, definitions, and references. WalkthroughThis PR implements PRO API credit-balance tracking and advisory notes. It captures the ChangesPRO API Credit Tracking
Possibly related issues
Possibly related PRs
🚥 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 |
A non-finite `x-credits-remaining` header value (e.g. `-Infinity`) parsed by `_capture_credits_remaining` reached `CreditSink` and caused two defects: - `-inf` later hit `int(remaining)` in the low-credits display branch of `build_tool_response`, raising `OverflowError` and turning a successful tool call into a hard error. - A `nan`/`-inf` recorded first poisoned the running minimum: `value < self.remaining` is `False` for every later observation, so a genuine low balance was dropped and the advisory note silently suppressed for the whole invocation. Guard at the single chokepoint `CreditSink.record`: non-finite values are now ignored, enforcing the invariant "remaining is None or a finite float". This fixes both the crash and the suppression without touching `build_tool_response`. Add unit tests (non-finite ignored, minimum not poisoned) and an end-to-end regression test that a non-finite captured header neither crashes nor emits a note. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
akolotov
force-pushed
the
claude/gifted-heisenberg-21b7f7
branch
from
396363b to
a3bd14c
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/tools/test_credit_tracking.py (1)
1-982: 🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy liftSplit this test module; it exceeds the project’s unit-test size limit.
This file is ~982 LOC, which is well past the 500 LOC cap for unit test modules. Please split it into focused modules (e.g.,
test_credit_sink.py,test_credit_capture_helpers.py,test_credit_scope_decorator.py,test_build_tool_response_credit_note.py,test_credit_note_e2e_mcp.py) to keep maintenance and failure triage manageable.As per coding guidelines,
tests/**/*.py: “Unit test files must not exceed 500 LOC; split into multiple focused modules when approaching the limit.”🤖 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 `@tests/tools/test_credit_tracking.py` around lines 1 - 982, Large test module exceeds 500 LOC limit; split into several focused test modules grouped by concern (e.g., CreditSink behavior, HTTP capture helpers, helper-surface routing, pro_api_credit_scope decorator behavior, build_tool_response note logic, and end-to-end MCP flow). For each new module, move the relevant tests and any small shared fixtures/helpers they need (e.g., _set_sink, _MockResponse, _SimpleClient/_PostClient/_GetClient/_AlternatingClient, HELPER_IDS) or extract truly shared pieces into a new tests/helpers module and import them; ensure imports reference the same symbols used in the tests (CreditSink, _credit_sink, make_blockscout_request, make_blockscout_post_request, make_metadata_request, _capture_credits_remaining, build_tool_response, pro_api_credit_scope, get_block_number) so tests keep their assertions intact; run/adjust patches and pytest markers after splitting to preserve test isolation and names.Source: Coding guidelines
🤖 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 `@tests/api/test_routes.py`:
- Line 873: A function-local import of "from blockscout_mcp_server.config import
config as bms_config" was added inside a test; move that import to the
module-level import block at the top of tests/api/test_routes.py (so bms_config
is imported with the other top-level imports) and remove the local import; if
you encounter circular-import issues after moving it, resolve them by using a
module-level import guard (e.g., import in TYPE_CHECKING or refactor the
dependency) so the import remains at the top.
In `@tests/test_config.py`:
- Around line 147-154: The test function
test_pro_api_low_credits_threshold_negative_rejected currently performs
function-local imports of ValidationError and pytest; hoist "from pydantic
import ValidationError" and "import pytest" to module scope (file header) and
remove the in-test imports so the test simply uses ValidationError and pytest
when calling ServerConfig in the with pytest.raises(...) block.
In `@tests/tools/test_credit_tracking.py`:
- Around line 433-460: In test_cross_task_visibility_via_periodic_progress add
assertions that mock_ctx.report_progress was called the expected number of times
to enforce progress-contract checks: after awaiting
make_request_with_periodic_progress (which uses make_blockscout_request and the
shared CreditSink), assert mock_ctx.report_progress.call_count (or specific
call_args if needed) matches the expected calls (e.g., at least one or a
specific count based on total_duration_hint); locate the test function
test_cross_task_visibility_via_periodic_progress and update its teardown
assertions to include mock_ctx.report_progress assertions so progress reporting
regressions are caught.
---
Outside diff comments:
In `@tests/tools/test_credit_tracking.py`:
- Around line 1-982: Large test module exceeds 500 LOC limit; split into several
focused test modules grouped by concern (e.g., CreditSink behavior, HTTP capture
helpers, helper-surface routing, pro_api_credit_scope decorator behavior,
build_tool_response note logic, and end-to-end MCP flow). For each new module,
move the relevant tests and any small shared fixtures/helpers they need (e.g.,
_set_sink, _MockResponse,
_SimpleClient/_PostClient/_GetClient/_AlternatingClient, HELPER_IDS) or extract
truly shared pieces into a new tests/helpers module and import them; ensure
imports reference the same symbols used in the tests (CreditSink, _credit_sink,
make_blockscout_request, make_blockscout_post_request, make_metadata_request,
_capture_credits_remaining, build_tool_response, pro_api_credit_scope,
get_block_number) so tests keep their assertions intact; run/adjust patches and
pytest markers after splitting to preserve test isolation and names.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 9a124c4a-8953-474e-ba62-bb82d82b71c2
📒 Files selected for processing (31)
.env.exampleAGENTS.mdAPI.mdREADME.mdSPEC.mdblockscout_mcp_server/__init__.pyblockscout_mcp_server/config.pyblockscout_mcp_server/pro_api_key_context.pyblockscout_mcp_server/tools/address/get_address_info.pyblockscout_mcp_server/tools/address/get_tokens_by_address.pyblockscout_mcp_server/tools/address/nft_tokens_by_address.pyblockscout_mcp_server/tools/block/get_block_info.pyblockscout_mcp_server/tools/block/get_block_number.pyblockscout_mcp_server/tools/chains/get_chains_list.pyblockscout_mcp_server/tools/common.pyblockscout_mcp_server/tools/contract/get_contract_abi.pyblockscout_mcp_server/tools/contract/inspect_contract_code.pyblockscout_mcp_server/tools/contract/read_contract.pyblockscout_mcp_server/tools/direct_api/direct_api_call.pyblockscout_mcp_server/tools/ens/get_address_by_ens_name.pyblockscout_mcp_server/tools/initialization/unlock_blockchain_analysis.pyblockscout_mcp_server/tools/search/lookup_token_by_symbol.pyblockscout_mcp_server/tools/transaction/get_token_transfers_by_address.pyblockscout_mcp_server/tools/transaction/get_transaction_info.pyblockscout_mcp_server/tools/transaction/get_transactions_by_address.pypyproject.tomlserver.jsontests/api/test_routes.pytests/integration/test_common_helpers.pytests/test_config.pytests/tools/test_credit_tracking.py
| itself (with AsyncMock returning a pre-built ToolResponse) would bypass the | ||
| decorator/capture path and produce no advisory note. | ||
| """ | ||
| from blockscout_mcp_server.config import config as bms_config |
There was a problem hiding this comment.
Move the function-local import to the module import block.
Line 873 introduces a local import, which breaks this repository’s import-placement rule for Python modules. Please move from blockscout_mcp_server.config import config as bms_config to the top import section.
As per coding guidelines: “ALL import statements must be placed at the top of the Python module, immediately after the module docstring (if present) and before any other code.”
🤖 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 `@tests/api/test_routes.py` at line 873, A function-local import of "from
blockscout_mcp_server.config import config as bms_config" was added inside a
test; move that import to the module-level import block at the top of
tests/api/test_routes.py (so bms_config is imported with the other top-level
imports) and remove the local import; if you encounter circular-import issues
after moving it, resolve them by using a module-level import guard (e.g., import
in TYPE_CHECKING or refactor the dependency) so the import remains at the top.
Source: Coding guidelines
Comment on lines
+147
to
+154
| def test_pro_api_low_credits_threshold_negative_rejected(monkeypatch): | ||
| from pydantic import ValidationError | ||
|
|
||
| monkeypatch.setenv("BLOCKSCOUT_PRO_API_LOW_CREDITS_THRESHOLD", "-1") | ||
| import pytest | ||
|
|
||
| with pytest.raises(ValidationError): | ||
| ServerConfig(_env_file=None) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
python - <<'PY'
import ast
from pathlib import Path
path = Path("tests/test_config.py")
code = path.read_text()
tree = ast.parse(code)
violations = []
def walk(node, in_local_scope=False):
for child in ast.iter_child_nodes(node):
child_local = in_local_scope or isinstance(node, (ast.FunctionDef, ast.AsyncFunctionDef, ast.ClassDef))
if isinstance(child, (ast.Import, ast.ImportFrom)) and child_local:
violations.append(child.lineno)
walk(child, child_local)
walk(tree)
print("Nested import lines:", violations)
PY
ruff check tests/test_config.pyRepository: blockscout/mcp-server
Length of output: 127
Move nested imports in tests/test_config.py to module scope (lines 148 and 151).
Function-local imports violate the repo import-placement rule; hoist ValidationError and pytest to the file header.
♻️ Proposed fix
+import pytest
+from pydantic import ValidationError
+
from blockscout_mcp_server.config import ServerConfig
@@
def test_pro_api_low_credits_threshold_negative_rejected(monkeypatch):
- from pydantic import ValidationError
-
monkeypatch.setenv("BLOCKSCOUT_PRO_API_LOW_CREDITS_THRESHOLD", "-1")
- import pytest
-
with pytest.raises(ValidationError):
ServerConfig(_env_file=None)🤖 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 `@tests/test_config.py` around lines 147 - 154, The test function
test_pro_api_low_credits_threshold_negative_rejected currently performs
function-local imports of ValidationError and pytest; hoist "from pydantic
import ValidationError" and "import pytest" to module scope (file header) and
remove the in-test imports so the test simply uses ValidationError and pytest
when calling ServerConfig in the with pytest.raises(...) block.
Source: Coding guidelines
Comment on lines
+433
to
+460
| async def test_cross_task_visibility_via_periodic_progress(mock_ctx): | ||
| """Credit captured in a child task spawned by make_request_with_periodic_progress is visible to the parent. | ||
|
|
||
| make_request_with_periodic_progress uses an anyio task group internally. | ||
| The child task runs in a copied context, but shares the same CreditSink | ||
| object reference, so mutations are visible to the parent. | ||
| """ | ||
| from blockscout_mcp_server.tools.common import make_blockscout_request, make_request_with_periodic_progress | ||
|
|
||
| sink = CreditSink() | ||
| response = _MockResponse({"data": "ok"}, headers={"x-credits-remaining": "3333"}) | ||
|
|
||
| with _set_sink(sink): | ||
| with ( | ||
| patch("blockscout_mcp_server.tools.common._create_httpx_client", return_value=_SimpleClient(response)), | ||
| patch("blockscout_mcp_server.tools.common.ensure_chain_supported", AsyncMock()), | ||
| patch.object(config, "pro_api_key", "test_key"), | ||
| ): | ||
| result = await make_request_with_periodic_progress( | ||
| ctx=mock_ctx, | ||
| request_function=make_blockscout_request, | ||
| request_args={"chain_id": "1", "api_path": "/api/v2/blocks/1"}, | ||
| total_duration_hint=30.0, | ||
| ) | ||
|
|
||
| assert result == {"data": "ok"} | ||
| # The value written by the child task must be visible on the parent's sink. | ||
| assert sink.remaining == 3333.0 |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Add explicit progress-report assertions in tests that use mock_ctx.
These tests validate response/note behavior but don’t assert mock_ctx.report_progress calls, so progress-contract regressions can slip through unnoticed. Add call-count assertions in these mock_ctx-based tests.
Suggested assertions
@@
async def test_cross_task_visibility_via_periodic_progress(mock_ctx):
@@
assert result == {"data": "ok"}
# The value written by the child task must be visible on the parent's sink.
assert sink.remaining == 3333.0
+ assert mock_ctx.report_progress.await_count > 0
@@
async def test_mcp_mode_low_credits_note_in_tool_response(mock_ctx):
@@
assert result.notes is not None
assert any("4200" in note for note in result.notes)
assert any("https://dev.blockscout.com" in note for note in result.notes)
+ assert mock_ctx.report_progress.await_count > 0
@@
async def test_mcp_mode_healthy_credits_no_note_in_tool_response(mock_ctx):
@@
# notes should be None (no advisory, no caller-supplied notes)
assert result.notes is None
+ assert mock_ctx.report_progress.await_count > 0As per coding guidelines, tests/**/*.py: “Assert progress tracking by verifying mock_ctx.report_progress call counts.”
Also applies to: 934-981
🤖 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 `@tests/tools/test_credit_tracking.py` around lines 433 - 460, In
test_cross_task_visibility_via_periodic_progress add assertions that
mock_ctx.report_progress was called the expected number of times to enforce
progress-contract checks: after awaiting make_request_with_periodic_progress
(which uses make_blockscout_request and the shared CreditSink), assert
mock_ctx.report_progress.call_count (or specific call_args if needed) matches
the expected calls (e.g., at least one or a specific count based on
total_duration_hint); locate the test function
test_cross_task_visibility_via_periodic_progress and update its teardown
assertions to include mock_ctx.report_progress assertions so progress reporting
regressions are caught.
Source: Coding guidelines
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
1 participant
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
Closes #394.
Surfaces a low PRO API credit balance to MCP clients as an advisory note on tool responses, so users learn they are running low on credits before requests start failing with HTTP 402.
When a Blockscout PRO API response carries an
x-credits-remainingheader and the captured balance falls below the configured threshold (default5000,0disables), every affected tool response gains a note pointing at https://dev.blockscout.com. The mechanism is transport-agnostic and works identically in MCP (stdio) and REST (HTTP) modes.How it works
pro_api_low_credits_thresholdfield (BLOCKSCOUT_PRO_API_LOW_CREDITS_THRESHOLD, default5000,ge=0).CreditSink(mutable box in aContextVar) records the minimumx-credits-remainingseen on the success path of the shared HTTP core. Capture is defensive: missing/unparseable/non-string headers are silently ignored; negative values are kept.@pro_api_credit_scopedecorator allocates a fresh sink per tool call and resets it infinally, applied innermost on all 16 tools.build_tool_responsereads the sink and, when below a positive threshold, appends the advisory note (integer display for whole numbers) without mutating the caller's notes list.Tests
tests/tools/test_credit_tracking.pycovers the sink, capture defensiveness, the decorator (lifecycle, isolation, transport-agnosticism,functools.wraps), and note emission across thresholds/zero/negative. Cross-mode end-to-end coverage intest_credit_tracking.py(MCP) andtests/api/test_routes.py(REST) exercises the real decorator → capture →build_tool_responsechain (only the HTTP transport is mocked).751 passed.tests/integration/test_common_helpers.py::test_credit_capture_on_real_pro_api_responsemakes a real PRO API GET and asserts a numeric balance is captured; skip-gated when no PRO key is configured. Ran live via the timeout runner:9 passed, 0 skipped.ruff check .andruff format --check .both clean.Notes for reviewers
0.16.0.dev17inpyproject.toml,blockscout_mcp_server/__init__.py, andserver.json;mcpb/manifest.jsonintentionally unchanged.SPEC.md,AGENTS.md,API.md,README.md,.env.example.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Configuration
BLOCKSCOUT_PRO_API_LOW_CREDITS_THRESHOLDenvironment variable (default: 5000; set to 0 to disable warnings)Documentation