feat(wizard): support context-aware wizard default suggestions in Opt… - #307
Conversation
|
@jeqcho have a look |
jeqcho
left a comment
There was a problem hiding this comment.
Thanks for picking this up, @Adityakk9031 — this lands exactly the shape proposed in #303, and the implementation gets the important things right: the suggest result is computed before the carried-value check, so an explicit stored bool keeps top precedence, and the three tests in tests/test_setup.py (no-context fallback, context-aware flip to [Y/n], explicit-config override) cover the precedence matrix nicely, including asserting on the rendered prompt suffix. Good call amending the existing unreleased OptionSlot changelog bullet rather than adding a new one.
A few things before this is ready to merge:
-
Plugin-facing docs are missing —
docs/guide/adapters.md(theOPTION_SLOTSsection, ~lines 103–117) documents the plugin contract that this PR extends, but it isn't updated. Sincesuggestis exactly the kind of thing an adapter author discovers through that guide, please add a short mention (signature plus a one-line example, e.g. thecollision_guardrail-from-geometry case from the issue). -
OptionSlotdocstring not updated — the class docstring insrc/inspect_robots/conformance.pydescribes each field (arg,label,default) but says nothing aboutsuggest. A sentence noting it computes the suggestion from the carried[embodiment.args]and that its failures/absence fall back todefaultwould keep the dataclass self-documenting, matching the style used elsewhere in this module. -
Non-bool returns from
suggestflow through unvalidated — in_options_section(src/inspect_robots/_setup.py),contextlib.suppress(Exception)guards against a raising callback (good, and consistent with this codebase's "a plugin bug must never crash the wizard" posture), but a callback returning a non-bool (say the string"false", which is truthy) is assigned tosuggestedas-is and drives_ask_yes_novia truthiness — Enter would then write the wrong value. The sibling carried-value branch right below already guards withisinstance(parsed, bool), andoption_slots/missing_runtime_requirementsboth type-filter plugin data the same way. Suggest mirroring that:if option.suggest is not None: try: computed = option.suggest(existing_args) except Exception: computed = None if isinstance(computed, bool): suggested = computed
As a small optional hardening while you're there:
existing_argsis the live carried dict, so a buggy callback could mutate it; wrapping it intypes.MappingProxyType(the declared type is already read-onlyMapping[str, str]) would close that off cheaply.
None of these touch the core design, which looks right. Happy to re-review once the docs and the bool guard are in — thanks again for the clean tests!
Summary
Resolves #303.
This PR updates the embodiment setup wizard's
OptionSlotschema to support context-aware default suggestions. Instead of relying solely on a static fallback boolean when a configuration key is absent, an embodiment plugin can now provide a dynamicsuggestcallback:This enables plugins (such as
inspect-robots-yaml) to recommend sensible defaults based on the existing configuration. For example, a plugin can automatically suggest enablingcollision_guardrailwhencollision_left/right_base_posis already configured in[embodiment.args].Changes
conformance.pysuggestcallback field to theOptionSlotdataclass._setup.py_options_sectionto invokesuggest, passing the current[embodiment.args]mapping.test_setup.py/test_conformance.pyCHANGELOG.mdVerification
Ran the setup and conformance test suites successfully: