fix(cli): keep legacy hidden selectors valid for advertise-required inputs - #2607
Open
nolanchic wants to merge 1 commit into
Open
fix(cli): keep legacy hidden selectors valid for advertise-required inputs#2607nolanchic wants to merge 1 commit into
nolanchic wants to merge 1 commit into
Conversation
…nputs Since tutti-os#2004, Registry.Invoke validates builtin commands against their advertised schema before the handler runs. Three agentcontext commands mark agent-id as advertise-required, so the schema lists it in required while the daemon binder deliberately treats it as optional: resolveAgentSelector accepts exactly one of agent-id or the hidden deprecated provider. The invocation validator therefore rejected the supported legacy spelling outright, and passing both selectors tripped the binder's exactly-one guard — no valid spelling was left. AdvertisedRequired keeps the advertised contract strict while allowing a hidden compatibility selector to satisfy runtime validation, so say so in the schema itself: a new advertise-alt tag names the hidden fields that can stand in, and Schema renders one anyOf branch per accepted spelling (typed object branches, which the invocation validator already supports) instead of a hard required entry. One branch per name matters: a single branch listing both names would require both at once. Fields without an alternate keep rendering exactly as before. The advertised capability contract for the three commands now states the real constraint — one of agent-id or the deprecated provider — which is also what external schema consumers should see. Fixes tutti-os#2193 Signed-off-by: Nolan <nolanchic@gmail.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
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.
Summary
Since #2004,
Registry.Invokevalidates builtin commands against their advertised schema before the handler runs. Threeagentcontextcommands markagent-idasadvertise-required, so the schema lists it inrequired— but the daemon binder deliberately treats it as optional:resolveAgentSelectoraccepts exactly one of--agent-idor the hidden deprecated--provider(#2193). The pre-binding validator therefore rejected the supported legacy spelling outright, and passing both selectors tripped the binder's exactly-one guard — no valid spelling was left for that path.The
AdvertisedRequireddoc comment already states the intent — keep the advertised contract strict while allowing a hidden compatibility selector to satisfy runtime validation — so the fix says exactly that in the schema itself:advertise-altstruct tag names the hidden fields that can stand in for an advertise-required field (the three commands declareadvertise-alt:"provider"onagent-id)Schema()emits one anyOf branch per accepted spelling ({type:"object", required:["agent-id"]},{type:"object", required:["provider"]}) instead of a hardrequiredentry. One branch per name matters — a single branch listing both names would require both at once. Typed object branches are required because the invocation validator only appliesrequiredto branches declaring an object type.Notes
command_catalog.go) reads the top-levelrequiredlist, soagent-idnow shows as optional in generated help. The anyOf constraint is not reflected there; happy to follow up if the help generator should learn about alternates.Test plan
TestStartCommandSchemaPassesInvocationValidationForLegacySelectore2e — the realstartcommand throughNewRegistryFromProviders→Invoke: canonical and legacy spellings pass the pre-binding validator, neither present is rejectedTestSchemaRendersAnyOfForAdvertisedRequiredWithAlternate— one branch per spelling, hidden field stays out of propertiesTestSchemaKeepsHardRequiredForAdvertisedRequiredWithoutAlternate— no-alternate fields unchangedTestRegistryInvokeAcceptsLegacyAlternateForAdvertisedRequired— validator-level coverageTestStartCommandRequiresOneSelectorAndPrompt/TestAgentStartCommandAllowsOmittedModelto the new advertised shapego test ./service/cli/...— 11 packages ok; full./service/...— 42 packages ok;gofmtclean;ValidateCapabilityInputSchemaaccepts the new shapeFixes #2193