Collapse phantom progress beats across MCP tools#391
Merged
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>
|
Warning Review limit reached
More reviews will be available in 36 minutes and 22 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
WalkthroughThis PR refactors MCP tool progress reporting to adopt an honest beat model: the number of progress steps now reflects actual distinct awaited operations, eliminating phantom intermediate steps introduced during the PRO API migration. Single-fetch tools report only start/completion on total=1.0; multi-step tools adjust their totals and remove unnecessary beats. The contract helper loses Context dependency, direct_api_call gains path validation and cursor messaging, and all tests align accordingly. ChangesProgress Model Honest Reporting
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 unit tests (beta)
Warning Review ran into problems🔥 ProblemsStopped waiting for pipeline failures after 30000ms. One of your pipelines takes longer than our 30000ms fetch window to run, so review may not consider pipeline-failure results for inline comments if any failures occurred after the fetch window. Increase the timeout if you want to wait longer or run a 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 |
akolotov
changed the title
get_transactions_by_address and get_token_transfers_by_address still emitted an instant pre-fetch "Fetching..." beat (phantom since #384 moved URL resolution into the request helper, leaving two report calls back to back). Drop that beat and rescale step counts to the real operations: - transactions: total 12 -> 11, progress_start_step 2 -> 1 (10 pages + final completion); the periodic mechanism's first beat now legitimately occupies the ~1.0 region. - token transfers: total 2 -> 1, current_step_number 2 -> 1. - smart-pagination helper defaults updated to match, with a docstring describing the step model. inspect_contract_code: completion message "Successfully fetched contract data." -> "Contract data ready." so it stays truthful on a cache hit, where no network fetch occurs. SPEC.md: split the dense progress-convention bullet into a readable nested list (no semantic change). Tests updated to the new honest beat counts and step numbers. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
get_transaction_info does concurrent fetches (gather) followed by
processing, which the SPEC's progress convention describes as a
start -> watershed -> completion (total=2.0) flow. It was reported as
a single-fetch (total=1.0) tool, diverging from get_block_info which
uses the watershed model for the same shape.
Convert it to total=2.0:
- introduce total_steps (no hardcoded literals)
- reposition the post-gather beat as a neutral watershed
("... requests completed; processing results.") so it stays truthful
when the optional user-operations request fails
- add the completion beat before building TransactionInfoData
Tests: success paths now expect 3 beats; failure paths that raise
before the watershed stay at 1; ops-failure path asserts the neutral
watershed message and the completion message.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
blockscout_mcp_server/tools/transaction/get_token_transfers_by_address.py (1)
71-121:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMissing final progress completion beat.
After
make_request_with_periodic_progresscompletes (line 80-90), the tool performs additional processing (transforming items, creating pagination, building the response) but never reports a final completion beat before returning. Other single-fetch tools in this PR explicitly report completion (e.g.,get_block_infoat line 46-51,get_block_numberat line 65-70).For consistency with the honest progress model, add a final
report_and_log_progresscall before the return statement.📊 Proposed fix to add completion beat
transformed_items = [ AdvancedFilterItem.model_validate(_transform_advanced_filter_item(item, fields_to_remove)) for item in sliced_items ] + await report_and_log_progress( + ctx, + progress=1.0, + total=tool_overall_total_steps, + message="Successfully fetched and processed token transfers.", + ) + range_text = f"from {age_from}" if age_to is None else f"from {age_from} to {age_to}" content_text = f"Found {len(transformed_items)} token transfers for {address} on chain {chain_id} {range_text}."🤖 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 `@blockscout_mcp_server/tools/transaction/get_token_transfers_by_address.py` around lines 71 - 121, After processing the Blockscout response but before returning, add a final report_and_log_progress call to mark completion: call report_and_log_progress(ctx, progress=tool_overall_total_steps, total=tool_overall_total_steps, message="Completed fetching token transfers for {address} on chain {chain_id}.") (use the same ctx, tool_overall_total_steps variable and include address/chain_id in the message) and place it just before the build_tool_response(...) return so the tool emits the final completion beat after transformation/pagination.
🤖 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.
Outside diff comments:
In `@blockscout_mcp_server/tools/transaction/get_token_transfers_by_address.py`:
- Around line 71-121: After processing the Blockscout response but before
returning, add a final report_and_log_progress call to mark completion: call
report_and_log_progress(ctx, progress=tool_overall_total_steps,
total=tool_overall_total_steps, message="Completed fetching token transfers for
{address} on chain {chain_id}.") (use the same ctx, tool_overall_total_steps
variable and include address/chain_id in the message) and place it just before
the build_tool_response(...) return so the tool emits the final completion beat
after transformation/pagination.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: a643aed8-611b-458b-9860-8557f59d6dcf
📒 Files selected for processing (35)
.cursor/rules/110-new-mcp-tool.mdcSPEC.mdblockscout_mcp_server/__init__.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/contract/_shared.pyblockscout_mcp_server/tools/contract/get_contract_abi.pyblockscout_mcp_server/tools/contract/inspect_contract_code.pyblockscout_mcp_server/tools/direct_api/direct_api_call.pyblockscout_mcp_server/tools/search/lookup_token_by_symbol.pyblockscout_mcp_server/tools/transaction/_shared.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/tools/address/test_get_address_info.pytests/tools/address/test_get_address_info_metadata.pytests/tools/address/test_get_tokens_by_address.pytests/tools/address/test_nft_tokens_by_address.pytests/tools/block/test_get_block_info.pytests/tools/block/test_get_block_number.pytests/tools/contract/test_fetch_and_process_contract.pytests/tools/contract/test_get_contract_abi.pytests/tools/contract/test_inspect_contract_code.pytests/tools/direct_api/test_direct_api_call.pytests/tools/direct_api/test_direct_api_call_progress.pytests/tools/search/test_lookup_token_by_symbol.pytests/tools/transaction/test_get_token_transfers_by_address.pytests/tools/transaction/test_get_transaction_info.pytests/tools/transaction/test_get_transactions_by_address.pytests/tools/transaction/test_get_transactions_by_address_pagination.py
main already shipped 0.16.0.dev12 via #389 after this branch diverged, so re-bump to dev13 to keep the dev version monotonic on merge. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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.
Closes #385
Summary
Implements honest progress reporting across the MCP tools. Several tools were emitting phantom progress beats — e.g. a "Fetching data…" beat that fired instantly with no real work behind it, and
totalscales that overstated the number of operations. This made progress fractions misleading and, in some branches, produced messages that were outright false (e.g. claiming data was "fetched" on a path where the fetch had failed).The fix collapses each tool's progress to a count that reflects the real operations performed: single-fetch tools report
total=1.0(start → completion), and tools with a genuine fetch-to-process watershed reporttotal=2.0(start → watershed → completion). Watershed messages on multi-request paths were reworded to be neutral ("… requests completed; processing results.") so they stay truthful regardless of which underlying request failed.What changed
total=1.0(phantom beat removed):get_tokens_by_address,nft_tokens_by_address,lookup_token_by_symbol,get_transaction_info,get_contract_abi,inspect_contract_code(DD-02),direct_api_call(DD-01, with an enriched start message naming the method/endpoint/chain and a(next page)suffix).get_transactions_by_addressandget_token_transfers_by_addressdropped the instant "Fetching…" beat that lingered after Switch to PRO API #384 moved URL resolution into the request helper (leaving tworeport_and_log_progresscalls back-to-back). Step counts were rescaled to the real operations — transactionstotal12→11 with the first page now step 1 (10 pages + final completion); token transferstotal2→1 — and the smart-pagination helper defaults (progress_start_step,total_steps) updated to match. The periodic-progress mechanism already emits a real beat in the ~1.0 region, so no gap is introduced. These two were the remaining gap behind the "across MCP tools" claim.inspect_contract_codecompletion message neutralized: "Successfully fetched contract data." → "Contract data ready.", so the completion beat stays truthful on a cache hit (where_fetch_and_process_contractreturns cached data and no network fetch occurs).get_block_numberandget_block_infonow scaletotalper branch (latest vs. by-datetime; with/without transactions) and emit honest counts on each path.get_address_infoslimmed while keeping its real fetch-to-process watershed (DD-03), with a neutralized watershed message._fetch_and_process_contractis now progress-free (the calling tool owns the beats); inline imports hoisted to module top.SPEC.mdprogress convention rewritten to describe the honest beat model and split into a readable nested list;.cursor/rules/110-new-mcp-tool.mdcexample updated..cursor/AGENTS.mdandmcpb/manifest.jsonintentionally left unchanged.0.16.0.dev13(pyproject.toml,blockscout_mcp_server/__init__.py,server.json).Reviewer notes
total, and watershed/completion message indices) — no tests were skipped, xfailed, deleted, or loosened.inspect_contract_codemessage, and the SPEC list reformat were added during review (commit662e7e2) to close the "across MCP tools" gap.Verification
ruff check .andruff format --check .: clean.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Chores