Fix lazy commands losing function signature defaults#35
Conversation
…arse function_to_schema() tracked which params had defaults but never stored the actual values. _add_arguments_from_schema() only read defaults via inspect.signature() for eager commands. This caused lazy command parameters to resolve to None instead of their declared defaults. Store default values in the JSON schema and fall back to schema defaults when the function signature is unavailable. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Coverage Report✅ 87.6% overall coverage
|
There was a problem hiding this comment.
Pull request overview
This PR fixes lazy-command argument parsing so omitted optional parameters use the handler’s declared defaults (instead of becoming None) by propagating defaults through the JSON schema and using schema defaults when the function signature isn’t available.
Changes:
- Store function parameter defaults in
function_to_schema()as JSON Schema"default"values. - Teach
_add_arguments_from_schema()to fall back to schema defaults when signature inspection is unavailable (lazy/schema-only mode). - Add tests validating lazy default propagation for ints and booleans, including precomputed schemas.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/milo/schema.py |
Adds "default" to generated JSON schema properties based on function signature defaults. |
src/milo/commands.py |
Applies schema-provided defaults when signature defaults can’t be read (lazy/schema-only mode). |
tests/test_lazy.py |
Adds test coverage for default propagation in lazy commands (signature + precomputed schema). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| has_default = param.default is not inspect.Parameter.empty | ||
| if has_default: | ||
| prop["default"] = param.default | ||
| if not has_default and not is_optional: |
There was a problem hiding this comment.
function_to_schema() now copies param.default into the JSON schema. Function defaults can be arbitrary Python objects (e.g., Enum members, Path objects, dataclass instances), which can make cmd.schema non-JSON-serializable and cause --mcp (e.g. tools/list) to crash when responses are serialized with json.dumps (no default= handler). Consider normalizing defaults to JSON-compatible values (Enum -> .value, Path -> str, dataclass -> asdict), or omitting/encoding non-serializable defaults in the schema while still keeping Python defaults for argparse/runtime dispatch.
| # Set default from signature or schema | ||
| if param and param.default is not inspect.Parameter.empty and json_type != "boolean": | ||
| kwargs["default"] = param.default | ||
| elif "default" not in kwargs and "default" in param_schema: | ||
| kwargs["default"] = param_schema["default"] |
There was a problem hiding this comment.
Schema defaults are now applied for non-boolean params, but boolean params still compute default as False when the signature is unavailable (lazy / schema-only mode) because the boolean branch sets kwargs["default"] before the new schema-default fallback. This means a precomputed schema like {type: "boolean", default: true} (or a handler defaulting to True captured into schema) will still parse as False when omitted. Consider using param_schema.get("default", False) for booleans when param is missing, and ensure the chosen argparse action is compatible with the intended default.
Summary
host,port) to beNoneinstead of their declared defaultsfunction_to_schema()now stores"default"values in JSON schema properties_add_arguments_from_schema()falls back to schema defaults when the function signature is unavailable (lazy commands)Test plan
🤖 Generated with Claude Code