-
-
Notifications
You must be signed in to change notification settings - Fork 174
MCP output schemas and schema/runtime conformance tests (Bounty #946) #1180
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -103,9 +103,9 @@ def optional_bounty_search_query_arg() -> str | None: | |
| return query | ||
|
|
||
| def output_format_arg() -> str: | ||
| value = args.get("format", "text") | ||
| if value is None: | ||
| if "format" not in args: | ||
| return "text" | ||
| value = args["format"] | ||
|
Comment on lines
105
to
+108
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift Reject explicit This fixes Suggested direction def optional_clean_str_arg(field: str) -> str | None:
- value = args.get(field)
- if value is None:
+ if field not in args:
return None
+ value = args[field]
+ if value is None:
+ raise ValueError(f"{field} must be a string")
if not isinstance(value, str):
raise ValueError(f"{field} must be a string")
@@
def list_limit_arg(default: int = 25) -> int:
- if "limit" not in args or args.get("limit") is None:
+ if "limit" not in args:
return default
value = positive_int_arg("limit")
@@
def optional_bool_arg(field: str, default: bool = False) -> bool:
- value = args.get(field, default)
- if value is None:
+ if field not in args:
return default
+ value = args[field]
if not isinstance(value, bool):
raise ValueError(f"{field} must be a boolean") |
||
| if not isinstance(value, str): | ||
| raise ValueError("format must be a string") | ||
| if contains_control_character(value): | ||
|
|
@@ -142,7 +142,7 @@ def optional_bool_arg(field: str, default: bool = False) -> bool: | |
| def require_known_fields(*allowed_fields: str) -> None: | ||
| unknown_fields = set(args) - set(allowed_fields) | ||
| if unknown_fields: | ||
| raise ValueError(f"unknown argument: {sorted(unknown_fields)[0]}") | ||
| raise ValueError("unknown argument") | ||
|
|
||
| def bounty_by_issue_number(repo_selector: str | None) -> Bounty | None: | ||
| issue_query = select(Bounty).where(Bounty.issue_number == positive_int_arg("issue_number")) | ||
|
|
@@ -245,6 +245,7 @@ def reject_unexpected_args(tool_name: str, allowed: set[str]) -> None: | |
| ) | ||
| return json.dumps(sorted_bounties[:limit]) | ||
| if name == "get_bounty": | ||
| require_known_fields("id", "bounty_id", "issue_number", "repo", "include_awards") | ||
| bounty = selected_bounty("id", internal_id_aliases=("bounty_id",)) | ||
| if bounty is None: | ||
| return "bounty not found" | ||
|
|
@@ -253,6 +254,14 @@ def reject_unexpected_args(tool_name: str, allowed: set[str]) -> None: | |
| bounty_data["awards"] = bounty_awards_to_dict(session, bounty.id) | ||
| return json.dumps(bounty_data) | ||
| if name == "list_bounty_attempts": | ||
| require_known_fields( | ||
| "id", | ||
| "bounty_id", | ||
| "issue_number", | ||
| "repo", | ||
| "include_expired", | ||
| "limit", | ||
| ) | ||
| bounty = selected_bounty("bounty_id", internal_id_aliases=("id",)) | ||
| if bounty is None: | ||
| return "bounty not found" | ||
|
|
@@ -270,6 +279,7 @@ def reject_unexpected_args(tool_name: str, allowed: set[str]) -> None: | |
| "attempts": attempt_listing["attempts"], | ||
| } | ||
| if name == "get_balance": | ||
| require_known_fields("account") | ||
| account = normalized_account(str_arg("account")) | ||
| balance_microunits = get_balance(session, account) | ||
| balance_mrwk = format_mrwk(balance_microunits) | ||
|
|
@@ -290,6 +300,7 @@ def reject_unexpected_args(tool_name: str, allowed: set[str]) -> None: | |
| ) | ||
| return json.dumps(wallet_to_dict(session, wallet)) | ||
| if name == "get_wallet": | ||
| require_known_fields("address") | ||
| wallet_row = session.get(Wallet, normalized_wallet_address(str_arg("address"))) | ||
| if wallet_row is None: | ||
| return "wallet not found" | ||
|
|
@@ -317,11 +328,13 @@ def reject_unexpected_args(tool_name: str, allowed: set[str]) -> None: | |
| ) | ||
| return json.dumps(wallet_transfer_to_dict(transfer)) | ||
| if name == "get_ledger_entry": | ||
| require_known_fields("sequence") | ||
| entry = ledger_entry_to_dict(session, positive_int_arg("sequence")) | ||
| if entry is None: | ||
| return "ledger entry not found" | ||
| return json.dumps(entry) | ||
| if name == "get_proof": | ||
| require_known_fields("hash") | ||
| proof = session.get(Proof, proof_hash_from_path(str_arg("hash"))) | ||
| if proof is None: | ||
| return "proof not found" | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Close the wallet output schema.
register_wallet/get_walletadvertise a fixed field set here, andtests/test_api_mcp.py:2941-2960already asserts those schema keys exactly match the serialized wallet payload. LeavingadditionalPropertiesopen means undeclared wallet fields can slip intostructuredContentwithout breaking the published contract.Suggested fix
MCP_WALLET_OUTPUT_SCHEMA: dict[str, Any] = { @@ - "additionalProperties": True, + "additionalProperties": False, }📝 Committable suggestion