diff --git a/internal/app/schema_shortcut_contract_test.go b/internal/app/schema_shortcut_contract_test.go index b7a304c6..20cc7066 100644 --- a/internal/app/schema_shortcut_contract_test.go +++ b/internal/app/schema_shortcut_contract_test.go @@ -16,8 +16,12 @@ import ( ) const ( - publicShortcutCount = 266 - schemaPublishedShortcutCount = 215 + publicShortcutCount = 266 + // schemaPublishedShortcutCount counts every delivered *.shortcut_* tool, + // including hidden leaves such as minutes.shortcut_minutes_search. + schemaPublishedShortcutCount = 216 + // publiclyDeliveredShortcutCount is the public-catalog subset of that surface. + publiclyDeliveredShortcutCount = 215 ) func TestDeliverySchemaCoversOrExactlyExcludesEveryPublicShortcutContract(t *testing.T) { @@ -75,7 +79,7 @@ func TestDeliverySchemaCoversOrExactlyExcludesEveryPublicShortcutContract(t *tes assertDeliveryShortcutConstraints(t, tool, declared, canonical) }) } - if got, want := excludedShortcuts, publicShortcutCount-schemaPublishedShortcutCount; got != want { + if got, want := excludedShortcuts, publicShortcutCount-publiclyDeliveredShortcutCount; got != want { t.Fatalf("exactly excluded public shortcuts = %d, want %d", got, want) } } @@ -89,11 +93,15 @@ func TestDeliveryShortcutProgressiveQueriesReturnCompleteContracts(t *testing.T) t.Fatalf("shortcut leaf confirmation = %q, want %q", got, want) } conversationID := schemaContractMap(leaf["parameters"])["conversation-id"] - if required, _ := conversationID["required"].(bool); !required { - t.Fatal("public --conversation-id must become required after hidden compatibility aliases are removed from Schema") + if required, _ := conversationID["required"].(bool); required { + t.Fatal("public --conversation-id must stay optional when hidden siblings still satisfy the declared exactly_one group") } - if got := leaf["constraints"]; got != nil { - t.Fatalf("shortcut leaf constraints = %#v, want omitted after hidden compatibility aliases collapse", got) + wantMessagesConstraints := map[string]any{ + "require_one_of": [][]string{{"conversation-id", "group", "id"}}, + "mutually_exclusive": [][]string{{"conversation-id", "group", "id"}}, + } + if got := leaf["constraints"]; !schemaContractJSONEqual(got, wantMessagesConstraints) { + t.Fatalf("shortcut leaf constraints = %#v, want %#v", got, wantMessagesConstraints) } constrainedLeaf := executeShortcutSchemaQuery(t, "--cli-path", "calendar +freebusy") @@ -308,7 +316,13 @@ func shortcutSchemaRequired(declared shortcut.Shortcut, flagName string) bool { visible = append(visible, constrained) } } - if len(visible) == 1 && visible[0] == flagName { + // Match AnnotateConstraints: only collapse to required when the projected + // group has a single member (no remaining hidden siblings). + flags := visible + if len(visible) < len(constraint.Flags) { + flags = append([]string(nil), constraint.Flags...) + } + if len(flags) == 1 && flags[0] == flagName { return true } } @@ -330,12 +344,18 @@ func assertDeliveryShortcutConstraints( } want := map[string][][]string{} for _, constraint := range declared.Constraints { - flags := make([]string, 0, len(constraint.Flags)) + visible := make([]string, 0, len(constraint.Flags)) for _, flagName := range constraint.Flags { if public[flagName] { - flags = append(flags, flagName) + visible = append(visible, flagName) } } + // Match AnnotateConstraints declare≡execute projection: keep the full + // declared group when any hidden sibling remains. + flags := visible + if len(visible) < len(constraint.Flags) { + flags = append([]string(nil), constraint.Flags...) + } switch constraint.Kind { case shortcut.ConstraintAtLeastOne: if len(flags) > 1 { diff --git a/internal/shortcut/shortcut_test.go b/internal/shortcut/shortcut_test.go index 4c55756f..d5458065 100644 --- a/internal/shortcut/shortcut_test.go +++ b/internal/shortcut/shortcut_test.go @@ -144,7 +144,7 @@ func TestCrossPlatformCoverageMountRegistersFlagsAndUse(t *testing.T) { } } -func TestCrossPlatformCoverageSchemaConstraintCollapsesHiddenAliases(t *testing.T) { +func TestCrossPlatformCoverageSchemaConstraintKeepsHiddenSiblingGroups(t *testing.T) { cmd := mount(Shortcut{ Service: "chat", Command: "+search", @@ -165,16 +165,19 @@ func TestCrossPlatformCoverageSchemaConstraintCollapsesHiddenAliases(t *testing. }, }, }) + // declare≡execute: hidden siblings still satisfy ValidateConstraints, so Schema + // must project the full group instead of collapsing the visible flag to required. query := cmd.Flags().Lookup("query") - if got := query.Annotations["dws.schema.required"]; len(got) != 1 || got[0] != "true" { - t.Fatalf("collapsed public requirement annotation = %#v", got) + if got := query.Annotations["dws.schema.required"]; len(got) > 0 { + t.Fatalf("visible flag must not collapse to required with hidden sibling: %#v", got) } id := cmd.Flags().Lookup("id") - if got := id.Annotations["dws.schema.required"]; len(got) != 1 || got[0] != "true" { - t.Fatalf("collapsed exact-one requirement annotation = %#v", got) + if got := id.Annotations["dws.schema.required"]; len(got) > 0 { + t.Fatalf("visible exact-one flag must not collapse to required with hidden sibling: %#v", got) } - if raw := cmd.Annotations["dws.schema.constraints"]; raw != "" { - t.Fatalf("hidden compatibility alias leaked into public Schema constraints: %q", raw) + want := `{"mutually_exclusive":[["id","legacy-id"]],"require_one_of":[["query","keyword"],["id","legacy-id"]]}` + if raw := cmd.Annotations["dws.schema.constraints"]; raw != want { + t.Fatalf("constraints annotation = %q, want %q", raw, want) } } diff --git a/scripts/policy/check-schema-catalog.sh b/scripts/policy/check-schema-catalog.sh index e422fbe2..8504b811 100755 --- a/scripts/policy/check-schema-catalog.sh +++ b/scripts/policy/check-schema-catalog.sh @@ -161,14 +161,21 @@ if ! jq -e ' exit 1 fi +# declare≡execute may project hidden execute-side siblings into constraint +# groups. Require every group to touch at least one published parameter/positional; +# unpublished members are allowed as hidden companions. if ! jq -e ' [.tools[] | select(.constraints != null)] as $tools | ($tools | length) >= 21 and all($tools[]; (((.parameters // {}) | keys) + ((.positionals // []) | map(.name))) as $names | - all((.constraints.mutually_exclusive // [])[]; length > 1 and all(.[]; IN($names[]))) and - all((.constraints.require_one_of // [])[]; length > 1 and all(.[]; IN($names[]))) and - all((.constraints.require_together // [])[]; length > 1 and all(.[]; IN($names[]))) + def ok_group: + length > 1 and + all(.[]; type == "string" and length > 0) and + any(.[]; IN($names[])); + all((.constraints.mutually_exclusive // [])[]; ok_group) and + all((.constraints.require_one_of // [])[]; ok_group) and + all((.constraints.require_together // [])[]; ok_group) ) ' "$catalog" >/dev/null; then printf '%s\n' 'schema command constraints are incomplete or reference unknown parameters' >&2 diff --git a/scripts/policy/schema-compat/main.go b/scripts/policy/schema-compat/main.go index 13cbacfe..382d46b2 100644 --- a/scripts/policy/schema-compat/main.go +++ b/scripts/policy/schema-compat/main.go @@ -481,7 +481,6 @@ func checkToolCompatibility(toolPath string, oldTool, newTool toolSchema) []stri {name: "interface_mode", old: oldTool.InterfaceMode, new: newTool.InterfaceMode}, {name: "interface_ref", old: oldTool.InterfaceRef, new: newTool.InterfaceRef}, {name: "availability", old: oldTool.Availability, new: newTool.Availability}, - {name: "constraints", old: oldTool.Constraints, new: newTool.Constraints}, {name: "effect", old: oldTool.Effect, new: newTool.Effect}, {name: "risk", old: oldTool.Risk, new: newTool.Risk}, {name: "confirmation", old: oldTool.Confirmation, new: newTool.Confirmation}, @@ -491,6 +490,10 @@ func checkToolCompatibility(toolPath string, oldTool, newTool toolSchema) []stri failures = append(failures, fmt.Sprintf("schema tool %q changed %s", toolPath, field.name)) } } + if oldTool.Constraints != newTool.Constraints && + !compatibleHiddenSiblingConstraintExpansion(oldTool, newTool) { + failures = append(failures, fmt.Sprintf("schema tool %q changed constraints", toolPath)) + } if !compatiblePositionals(oldTool.Positionals, newTool.Positionals) { failures = append(failures, fmt.Sprintf("schema tool %q changed positionals", toolPath)) } @@ -510,6 +513,66 @@ func checkToolCompatibility(toolPath string, oldTool, newTool toolSchema) []stri return failures } +// compatibleHiddenSiblingConstraintExpansion allows declare≡execute repairs: +// Schema may start projecting full constraint groups that include unpublished +// (hidden) execute-side siblings when the previous contract collapsed the sole +// published member to required and omitted constraints. +func compatibleHiddenSiblingConstraintExpansion(oldTool, newTool toolSchema) bool { + if strings.TrimSpace(oldTool.Constraints) != "" { + return false + } + var projected struct { + MutuallyExclusive [][]string `json:"mutually_exclusive"` + RequireOneOf [][]string `json:"require_one_of"` + RequireTogether [][]string `json:"require_together"` + } + if err := json.Unmarshal([]byte(newTool.Constraints), &projected); err != nil { + return false + } + if len(projected.RequireTogether) > 0 || len(projected.RequireOneOf) == 0 { + return false + } + groups := append([][]string(nil), projected.RequireOneOf...) + groups = append(groups, projected.MutuallyExclusive...) + for _, group := range groups { + if len(group) < 2 { + return false + } + published := 0 + hidden := 0 + for _, name := range group { + name = strings.TrimSpace(name) + if name == "" { + return false + } + if _, ok := newTool.Parameters[name]; ok { + published++ + } else { + hidden++ + } + } + if published == 0 || hidden == 0 { + return false + } + // Former collapse artifact: exactly one published member was required. + if published == 1 { + var sole string + for _, name := range group { + if _, ok := newTool.Parameters[name]; ok { + sole = name + break + } + } + oldParam, ok := oldTool.Parameters[sole] + newParam, okNew := newTool.Parameters[sole] + if !ok || !okNew || !oldParam.Required || newParam.Required { + return false + } + } + } + return true +} + func compatiblePositionals(oldPositionals, newPositionals []positionalSchema) bool { if len(newPositionals) < len(oldPositionals) { return false