build: require sdk-ignore annotation for unimplemented gRPC queries#3213
Merged
build: require sdk-ignore annotation for unimplemented gRPC queries#3213
Conversation
Previously, the gRPC coverage check auto-accepted unimplemented queries by caching them as "not_implemented", allowing CI to pass silently on subsequent runs. This enabled accidental tech debt accumulation without explicit human decisions. Now unimplemented queries must be annotated with `// @sdk-ignore: <reason>` directly in platform.proto. Without the annotation, CI fails every time with clear instructions on how to add it. The cache only stores "implemented" entries (auto-managed). The hardcoded EXCLUDED_QUERIES set is removed — broadcastStateTransition now uses @sdk-ignore like everything else. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
❌ gRPC Query Coverage ReportTotal: 53 queries — 50 implemented, 2 ignored, 1 missing ❌ Missing
⏭️ Ignored (@sdk-ignore) (2)
✅ Implemented (50)
|
Contributor
📝 WalkthroughWalkthroughParses Changes
Sequence Diagram(s)sequenceDiagram
participant Proto as platform.proto
participant Script as check-grpc-coverage.py
participant SDK as rs-sdk
participant Cache as grpc-queries-cache.json
participant CI as PR comment / report
Proto->>Script: extract_grpc_queries(name, line)
Proto->>Script: extract_sdk_ignore_annotations(rpc -> reason)
Script->>SDK: index_sdk_sources(sdk_path)
Script->>SDK: check_query_implementation for each RPC
SDK-->>Script: implemented / not found (location)
Script->>Cache: prune & save implemented queries
Script->>CI: generate Markdown report (implemented, ignored, missing)
CI-->>Script: fail CI if missing or doc errors
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 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 |
Contributor
|
✅ DashSDKFFI.xcframework built for this PR.
SwiftPM (host the zip at a stable URL, then use): .binaryTarget(
name: "DashSDKFFI",
url: "https://your.cdn.example/DashSDKFFI.xcframework.zip",
checksum: "d26d8d0130ea4b9bbd32e1159ba1457d25e87076ea9b8bf0c924ab10d6dc5d55"
)Xcode manual integration:
|
lklimek
commented
Mar 9, 2026
Contributor
There was a problem hiding this comment.
Pull request overview
Updates the CI gRPC coverage check so new/changed platform.proto RPCs must be implemented in rs-sdk unless they’re explicitly and visibly skipped via a // @sdk-ignore: <reason> annotation in the proto.
Changes:
- Refactors
.github/scripts/check-grpc-coverage.pyto require@sdk-ignorefor unimplemented RPCs and to keep the cache as “implemented-only”. - Adds
@sdk-ignoreannotations inplatform.protofor existing exceptions. - Cleans
.github/grpc-queries-cache.jsonby removing non-implemented/excluded entries.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| packages/dapi-grpc/protos/platform/v0/platform.proto | Adds @sdk-ignore comments above selected RPCs. |
| .github/scripts/check-grpc-coverage.py | Implements ignore-annotation parsing, cache migration, and stricter CI failure behavior. |
| .github/grpc-queries-cache.json | Removes legacy excluded / not_implemented cache entries. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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)
packages/dapi-grpc/protos/platform/v0/platform.proto (1)
96-98:⚠️ Potential issue | 🟠 MajorMissing
@sdk-ignoreannotation forgetTokenPreProgrammedDistributionsThis RPC is missing the
@sdk-ignoreannotation. The query is not implemented in the SDK, so the annotation is required to prevent CI failure.Note: Only two RPCs in the proto file have
@sdk-ignoreannotations (broadcastStateTransition and getConsensusParams), not three as mentioned in the PR objectives.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/dapi-grpc/protos/platform/v0/platform.proto` around lines 96 - 98, Add the missing `@sdk-ignore` annotation to the RPC definition for getTokenPreProgrammedDistributions in platform.proto; locate the rpc getTokenPreProgrammedDistributions(GetTokenPreProgrammedDistributionsRequest) returns (GetTokenPreProgrammedDistributionsResponse) declaration and add the same `@sdk-ignore` marker used for the other ignored RPCs (e.g., broadcastStateTransition/getConsensusParams) immediately above that rpc so the codegen/CI will skip this unimplemented SDK query.
🧹 Nitpick comments (2)
.github/scripts/check-grpc-coverage.py (2)
324-326: Remove unused variablequery_line_map.This variable is assigned but never used in the code.
♻️ Proposed fix
query_names = [name for name, _ in all_queries] - query_line_map = {name: line for name, line in all_queries}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/scripts/check-grpc-coverage.py around lines 324 - 326, Remove the unused variable query_line_map: locate where all_queries is unpacked into query_names and query_line_map (the comprehension assigning query_line_map = {name: line for name, line in all_queries}) and delete the query_line_map assignment, keeping only query_names = [name for name, _ in all_queries]; ensure no other code references query_line_map (if any do, either use it or adjust those references accordingly).
364-366: Prefix unused variablelocationwith underscore.The
locationvariable returned fromcheck_query_implementationis unpacked but never used.♻️ Proposed fix
- implemented, location = check_query_implementation(query_name, sdk_path) + implemented, _location = check_query_implementation(query_name, sdk_path)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/scripts/check-grpc-coverage.py around lines 364 - 366, The variable returned by check_query_implementation is unpacked into implemented and location but location is unused; update the unpacking to ignore or underscore the variable (e.g., change "implemented, location = check_query_implementation(query_name, sdk_path)" to "implemented, _location = check_query_implementation(query_name, sdk_path)" or "implemented, _ = check_query_implementation(...)" so linters won't flag an unused variable and behavior remains the same, touching the call site where implemented and location are assigned.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@packages/dapi-grpc/protos/platform/v0/platform.proto`:
- Around line 96-98: Add the missing `@sdk-ignore` annotation to the RPC
definition for getTokenPreProgrammedDistributions in platform.proto; locate the
rpc
getTokenPreProgrammedDistributions(GetTokenPreProgrammedDistributionsRequest)
returns (GetTokenPreProgrammedDistributionsResponse) declaration and add the
same `@sdk-ignore` marker used for the other ignored RPCs (e.g.,
broadcastStateTransition/getConsensusParams) immediately above that rpc so the
codegen/CI will skip this unimplemented SDK query.
---
Nitpick comments:
In @.github/scripts/check-grpc-coverage.py:
- Around line 324-326: Remove the unused variable query_line_map: locate where
all_queries is unpacked into query_names and query_line_map (the comprehension
assigning query_line_map = {name: line for name, line in all_queries}) and
delete the query_line_map assignment, keeping only query_names = [name for name,
_ in all_queries]; ensure no other code references query_line_map (if any do,
either use it or adjust those references accordingly).
- Around line 364-366: The variable returned by check_query_implementation is
unpacked into implemented and location but location is unused; update the
unpacking to ignore or underscore the variable (e.g., change "implemented,
location = check_query_implementation(query_name, sdk_path)" to "implemented,
_location = check_query_implementation(query_name, sdk_path)" or "implemented, _
= check_query_implementation(...)" so linters won't flag an unused variable and
behavior remains the same, touching the call site where implemented and location
are assigned.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9fa57ca2-7abc-492b-a055-88bd0bac74ca
📒 Files selected for processing (3)
.github/grpc-queries-cache.json.github/scripts/check-grpc-coverage.pypackages/dapi-grpc/protos/platform/v0/platform.proto
- Replace 150-line QUERY_MAPPINGS with auto-derived pattern (capitalize + "Request"). Keep empty dict for optional overrides. - Format PR comment as Markdown with collapsible sections: Missing shown expanded, Ignored and Implemented collapsed by default. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
QuantumExplorer
previously approved these changes
Mar 9, 2026
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Remove scan window limit in @sdk-ignore parser (scan until next non-comment line instead of fixed 4-line window) - Remove unused variables: query_line_map, new_implemented - Index SDK sources once per run instead of N×os.walk Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
.github/scripts/check-grpc-coverage.py (1)
196-196: Drop the dead locals left by the refactor.
query_line_mapandlocationare both unused now, and Ruff is already flagging them.Suggested cleanup
- query_line_map = {name: line for name, line in all_queries} @@ - implemented, location = check_query_implementation(query_name, sdk_path) + implemented, _location = check_query_implementation(query_name, sdk_path)Also applies to: 236-236
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/scripts/check-grpc-coverage.py at line 196, Remove the dead local variables left by the refactor: delete the unused dict assignment to query_line_map and any unused variable named location (both referenced around the all_queries handling), and remove any now-unused imports or references that only existed to support those locals; ensure any logic that depended on all_queries still uses the remaining variables to avoid breaking flow (e.g., keep all_queries usage but drop the query_line_map = {name: line for name, line in all_queries} and any standalone location variable).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/grpc-queries-cache.json:
- Line 154: The RPC getTokenPreProgrammedDistributions must be annotated with
the exact comment marker before its declaration to avoid checker failures: open
the proto that defines rpc getTokenPreProgrammedDistributions (platform.proto)
and insert the line "// `@sdk-ignore`:" directly above the rpc
getTokenPreProgrammedDistributions(...) declaration so the checker recognizes
it; save and commit the proto update (and re-run the cache/generation step if
required).
In @.github/scripts/check-grpc-coverage.py:
- Around line 148-160: The subprocess invocation in check-grpc-coverage.py uses
a hardcoded "python3" and only inspects result.stdout for lines starting with
"ERROR:", causing stderr-only diagnostics or non-prefixed messages to be
ignored; replace the command with the current interpreter (use sys.executable)
when running ["sys.executable", str(check_script)] and preserve/inspect stderr
as well (use capture_output=True, text=True as already set) — update the logic
around result, errors, check_script, and wasm_sdk_path to append any non-empty
result.stderr lines to errors and/or include any non-ERROR-prefixed stdout lines
that look like diagnostics so failing check_documentation.py is not silently
reported as zero errors.
---
Nitpick comments:
In @.github/scripts/check-grpc-coverage.py:
- Line 196: Remove the dead local variables left by the refactor: delete the
unused dict assignment to query_line_map and any unused variable named location
(both referenced around the all_queries handling), and remove any now-unused
imports or references that only existed to support those locals; ensure any
logic that depended on all_queries still uses the remaining variables to avoid
breaking flow (e.g., keep all_queries usage but drop the query_line_map = {name:
line for name, line in all_queries} and any standalone location variable).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2de3d67c-8fd5-4ed0-b4f5-a5ee350385ba
📒 Files selected for processing (3)
.github/grpc-queries-cache.json.github/scripts/check-grpc-coverage.py.github/workflows/tests-rs-sdk-grpc-coverage.yml
Use sys.executable instead of hardcoded "python3" for subprocess call. Also capture stderr output when check_documentation.py fails. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
.github/scripts/check-grpc-coverage.py (1)
161-167:⚠️ Potential issue | 🟡 MinorKeep stdout diagnostics when the doc check fails.
This still drops non-
ERROR:messages fromstdout, so a failingcheck_documentation.pycan be reported as having zero errors even though it emitted diagnostics.Suggested fix
errors = [] if result.returncode != 0: - for line in result.stdout.strip().split("\n"): - if line.startswith("ERROR:"): - errors.append(line) - for line in result.stderr.strip().split("\n"): - if line.strip(): - errors.append(line) + stdout_lines = [line for line in result.stdout.splitlines() if line.strip()] + stderr_lines = [line for line in result.stderr.splitlines() if line.strip()] + + errors.extend(line for line in stdout_lines if line.startswith("ERROR:")) + errors.extend(stderr_lines) + + if not errors: + errors.extend(stdout_lines) + if not errors: + errors.append( + f"check_documentation.py exited with code {result.returncode}" + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/scripts/check-grpc-coverage.py around lines 161 - 167, The stdout-parsing logic only appends stdout lines that start with "ERROR:", which drops other diagnostics (so check_documentation.py messages can be missed); update the block that handles result.returncode != 0 to append all non-empty stdout lines (e.g., iterate result.stdout.strip().split("\n") and for each non-blank line append it to errors) instead of filtering by "ERROR:", while continuing to also append non-empty result.stderr lines; update references around the variables result, errors, stdout, and stderr accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/scripts/check-grpc-coverage.py:
- Around line 112-124: check_query_implementation currently treats any
occurrence of a "*Request" identifier as an implementation; change the heuristic
to detect actual SDK bindings instead: when building patterns from the
query_name (in the check_query_implementation function that uses QUERY_MAPPINGS
and patterns over sdk_sources), require that a matched pattern appears in a
binding context such as a "type Request =" alias, an actual fetch/handler
function/method declaration (e.g., "fn <queryName>" or case-insensitive variants
like "<queryName>_request" / "<queryName>Request" in function signatures), or an
impl that exposes the RPC, rather than any bare type reference; update the
search logic to verify one of these binding contexts around the match before
returning True and the file_path.
- Around line 148-149: The current check silently returns success when the WASM
doc checker file (check_script) is missing; change the behaviour so the script
fails fast: replace the branch that does "if not check_script.exists(): return
True, []" with a failing return or exception (e.g., "return False, [f'WASM doc
checker not found: {check_script}']" or raise SystemExit/RuntimeError) so CI
marks the job as failed; locate the check by the check_script variable in
.github/scripts/check-grpc-coverage.py and update the function that returns the
(bool, list) tuple accordingly.
---
Duplicate comments:
In @.github/scripts/check-grpc-coverage.py:
- Around line 161-167: The stdout-parsing logic only appends stdout lines that
start with "ERROR:", which drops other diagnostics (so check_documentation.py
messages can be missed); update the block that handles result.returncode != 0 to
append all non-empty stdout lines (e.g., iterate
result.stdout.strip().split("\n") and for each non-blank line append it to
errors) instead of filtering by "ERROR:", while continuing to also append
non-empty result.stderr lines; update references around the variables result,
errors, stdout, and stderr accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7a8f2765-772a-493d-9e2f-659bc1b367da
📒 Files selected for processing (2)
.github/grpc-queries-cache.json.github/scripts/check-grpc-coverage.py
lklimek
changed the title
lklimek
added
ready for final review
lklimek
changed the title
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
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.
Issue being fixed or feature implemented
The gRPC coverage check silently auto-accepted unimplemented queries by caching them as
not_implemented. After the first CI failure, subsequent runs passed without any human decision, enabling accidental tech debt accumulation.User Story
Imagine you are a developer adding a new
rpcmethod toplatform.proto. Previously, CI would fail once, auto-commit a cache entry marking the query as "known but not implemented", and then pass on all subsequent runs — silently accepting the missing SDK implementation. Now, CI will keep failing until you either implement the query inrs-sdkor deliberately add a// @sdk-ignore: <reason>annotation above therpcline in the proto file. This makes every skip a conscious, reviewable decision visible in code review.What was done?
check-grpc-coverage.py: Cache now only storesimplementedqueries (auto-managed). Removed thenot_implementedstatus and the hardcodedEXCLUDED_QUERIESset.@sdk-ignoreannotation support: The script parses// @sdk-ignore: <reason>comments aboverpclines inplatform.proto. Unimplemented queries without this annotation cause CI to fail with clear copy-paste instructions.platform.proto:broadcastStateTransition— Write-only endpoint, not a querygetConsensusParams— Consensus params fetched via Tenderdash RPCgetTokenPreProgrammedDistributions— Not yet implemented, tracking in backloggrpc-queries-cache.json: Removedexcludedandnot_implementedentries; onlyimplementedentries remain.Failure output example
When a new
rpcis added without implementation or annotation:How Has This Been Tested?
@sdk-ignore'd)rpcinto a temp proto copy — confirmed CI fails with correct error message and line numbernot_implemented/excludedentries are auto-cleaned on loadBreaking Changes
None. The workflow behavior changes but no APIs or interfaces are affected.
Checklist:
🤖 Co-authored by Claudius the Magnificent AI Agent
Summary by CodeRabbit