Merge remote-tracking branch 'origin/main' into pr-922

scripts/policy/check-mono-multi-skill-content.sh

@@ -1,5 +1,5 @@
 #!/bin/sh
-# Mono↔multi skill content QA (G1–G4). See docs/skill-mono-multi-qa.md
+# Mono↔multi skill content QA (G1–G5). See docs/skill-mono-multi-qa.md
 set -eu
 
 ROOT="$(CDPATH= cd -- "$(dirname -- "$0")/../.." && pwd)"

scripts/policy/interface-baseline/contract.go

@@ -312,10 +312,13 @@ func checkCompatibility(root *cobra.Command, baseline interfaceContract) []strin
 				continue
 			}
 			if actual.Value.Type() != expectedFlag.Type {
-				failures = append(failures, fmt.Sprintf(
-					"%q flag --%s changed type from %s to %s",
-					displayPath(path), expectedFlag.Name, expectedFlag.Type, actual.Value.Type(),
-				))
+				if !reviewedFlagTypeChange(displayPath(path), expectedFlag.Name, expectedFlag.Type, actual.Value.Type()) ||
+					flagContractOtherwiseChanged(expectedFlag, actual, actualScope) {
+					failures = append(failures, fmt.Sprintf(
+						"%q flag --%s changed type from %s to %s",
+						displayPath(path), expectedFlag.Name, expectedFlag.Type, actual.Value.Type(),
+					))
+				}
 			}
 			if expectedFlag.Shorthand != "" && actual.Shorthand != expectedFlag.Shorthand {
 				failures = append(failures, fmt.Sprintf(
@@ -443,11 +446,20 @@ func mergeContracts(historical, current interfaceContract) (interfaceContract, [
 				continue
 			}
 			if oldFlag.Type != newFlag.Type {
-				failures = append(failures, fmt.Sprintf(
-					"%q flag --%s changed type from %s to %s",
-					displayPath(path), name, oldFlag.Type, newFlag.Type,
-				))
-				continue
+				if !reviewedFlagTypeChange(displayPath(path), name, oldFlag.Type, newFlag.Type) ||
+					mergedFlagContractOtherwiseChanged(oldFlag, newFlag) {
+					failures = append(failures, fmt.Sprintf(
+						"%q flag --%s changed type from %s to %s",
+						displayPath(path), name, oldFlag.Type, newFlag.Type,
+					))
+					continue
+				}
+				// A reviewed migration keeps merging, and the merged entry keeps
+				// the historical type below, exactly as this function keeps the
+				// historical shorthand, required and hidden values. The baseline
+				// records what was promised, so a later --check still resolves
+				// this flag through the reviewed table rather than silently
+				// adopting the new type as history.
 			}
 			if oldFlag.Shorthand != "" && oldFlag.Shorthand != newFlag.Shorthand {
 				failures = append(failures, fmt.Sprintf(

scripts/policy/interface-baseline/contract_test.go

@@ -10,6 +10,7 @@ import (
 	"strings"
 	"testing"
 
+	"github.com/DingTalk-Real-AI/dingtalk-workspace-cli/internal/app"
 	"github.com/spf13/cobra"
 )
 
@@ -239,6 +240,286 @@ func assertFailureContains(t *testing.T, failures []string, want string) {
 	t.Fatalf("failures %v do not contain %q", failures, want)
 }
 
+// policyRoot builds "dws old" with a single --policy flag declared by declare,
+// so a baseline and a candidate can differ only in how that flag is typed.
+func policyRoot(declare func(*cobra.Command)) *cobra.Command {
+	root := &cobra.Command{Use: "dws"}
+	old := &cobra.Command{Use: "old", Run: func(*cobra.Command, []string) {}}
+	declare(old)
+	root.AddCommand(old)
+	root.InitDefaultHelpCmd()
+	return root
+}
+
+// registerReviewedFixture puts a fixture command in the reviewed table for the
+// duration of one test, so the behaviour tests exercise the real lookup instead
+// of depending on whichever production entries happen to exist.
+func registerReviewedFixture(t *testing.T, change flagTypeChange) {
+	t.Helper()
+	if _, exists := reviewedFlagTypeChanges[change]; exists {
+		t.Fatalf("fixture %+v collides with a production entry", change)
+	}
+	reviewedFlagTypeChanges[change] = struct{}{}
+	t.Cleanup(func() { delete(reviewedFlagTypeChanges, change) })
+}
+
+const typeChangedFailure = "changed type from string to int"
+
+func TestReviewedFlagTypeChangeAcceptsRegisteredMigration(t *testing.T) {
+	baseline := snapshot(policyRoot(func(command *cobra.Command) {
+		command.Flags().String("policy", "", "policy")
+	}))
+	current := policyRoot(func(command *cobra.Command) {
+		command.Flags().Int("policy", 0, "policy")
+	})
+
+	// Without an entry the migration is still a blocking change.
+	assertFailureContains(t, checkCompatibility(current, baseline), typeChangedFailure)
+
+	registerReviewedFixture(t, flagTypeChange{CommandPath: "dws old", Flag: "policy", From: "string", To: "int"})
+	if failures := checkCompatibility(current, baseline); len(failures) != 0 {
+		t.Fatalf("a reviewed string->int migration should pass: %v", failures)
+	}
+}
+
+func TestReviewedFlagTypeChangeIsDirectionSensitive(t *testing.T) {
+	registerReviewedFixture(t, flagTypeChange{CommandPath: "dws old", Flag: "policy", From: "string", To: "int"})
+
+	// The reverse migration shares command and flag but not direction, so the
+	// string->int entry must not admit it.
+	baseline := snapshot(policyRoot(func(command *cobra.Command) {
+		command.Flags().Int("policy", 0, "policy")
+	}))
+	current := policyRoot(func(command *cobra.Command) {
+		command.Flags().String("policy", "", "policy")
+	})
+	assertFailureContains(t, checkCompatibility(current, baseline), "changed type from int to string")
+}
+
+func TestReviewedFlagTypeChangeRejectsOtherCommandsAndFlags(t *testing.T) {
+	registerReviewedFixture(t, flagTypeChange{CommandPath: "dws other", Flag: "policy", From: "string", To: "int"})
+	baseline := snapshot(policyRoot(func(command *cobra.Command) {
+		command.Flags().String("policy", "", "policy")
+	}))
+	current := policyRoot(func(command *cobra.Command) {
+		command.Flags().Int("policy", 0, "policy")
+	})
+	// Same flag and direction, different command.
+	assertFailureContains(t, checkCompatibility(current, baseline), typeChangedFailure)
+
+	registerReviewedFixture(t, flagTypeChange{CommandPath: "dws old", Flag: "elsewhere", From: "string", To: "int"})
+	// Same command and direction, different flag.
+	assertFailureContains(t, checkCompatibility(current, baseline), typeChangedFailure)
+}
+
+// A reviewed migration is only accepted when the rest of the flag's contract
+// held still. Each case bundles one unrelated regression with the reviewed type
+// change and requires the type failure to reappear, which also anchors
+// flagContractOtherwiseChanged against the checks it mirrors: a condition that
+// drifts out of sync fails here instead of silently widening the exemption.
+func TestReviewedFlagTypeChangeRejectsBundledRegression(t *testing.T) {
+	registerReviewedFixture(t, flagTypeChange{CommandPath: "dws old", Flag: "policy", From: "string", To: "int"})
+
+	tests := []struct {
+		name     string
+		baseline func(*cobra.Command)
+		current  func(*cobra.Command)
+		want     string
+	}{
+		{
+			name:     "became required",
+			baseline: func(command *cobra.Command) { command.Flags().String("policy", "", "policy") },
+			current: func(command *cobra.Command) {
+				command.Flags().Int("policy", 0, "policy")
+				_ = command.MarkFlagRequired("policy")
+			},
+			want: "became required",
+		},
+		{
+			name:     "became hidden",
+			baseline: func(command *cobra.Command) { command.Flags().String("policy", "", "policy") },
+			current: func(command *cobra.Command) {
+				command.Flags().Int("policy", 0, "policy")
+				_ = command.Flags().MarkHidden("policy")
+			},
+			want: "became hidden",
+		},
+		{
+			name:     "lost shorthand",
+			baseline: func(command *cobra.Command) { command.Flags().StringP("policy", "p", "", "policy") },
+			current:  func(command *cobra.Command) { command.Flags().Int("policy", 0, "policy") },
+			want:     "lost shorthand",
+		},
+		{
+			name:     "changed no-opt value",
+			baseline: func(command *cobra.Command) { command.Flags().String("policy", "", "policy") },
+			current: func(command *cobra.Command) {
+				command.Flags().Int("policy", 0, "policy")
+				command.Flags().Lookup("policy").NoOptDefVal = "4"
+			},
+			want: "changed no-opt value",
+		},
+		{
+			name:     "narrowed persistent scope",
+			baseline: func(command *cobra.Command) { command.PersistentFlags().String("policy", "", "policy") },
+			current:  func(command *cobra.Command) { command.Flags().Int("policy", 0, "policy") },
+			want:     "narrowed persistent scope",
+		},
+	}
+
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			failures := checkCompatibility(policyRoot(test.current), snapshot(policyRoot(test.baseline)))
+			assertFailureContains(t, failures, test.want)
+			assertFailureContains(t, failures, typeChangedFailure)
+		})
+	}
+}
+
+// mergeContracts runs the same admission decision on --merge. A reviewed
+// migration keeps merging and the merged entry keeps the historical type, so a
+// later --check still resolves the flag through the reviewed table.
+func TestMergeAcceptsReviewedFlagTypeChangeAndKeepsHistoricalType(t *testing.T) {
+	historical := snapshot(policyRoot(func(command *cobra.Command) {
+		command.Flags().String("policy", "", "policy")
+	}))
+	current := snapshot(policyRoot(func(command *cobra.Command) {
+		command.Flags().Int("policy", 0, "policy")
+	}))
+
+	if _, failures := mergeContracts(historical, current); len(failures) == 0 {
+		t.Fatal("an unreviewed merge should report the type change")
+	}
+
+	registerReviewedFixture(t, flagTypeChange{CommandPath: "dws old", Flag: "policy", From: "string", To: "int"})
+	merged, failures := mergeContracts(historical, current)
+	if len(failures) != 0 {
+		t.Fatalf("a reviewed merge should pass: %v", failures)
+	}
+	if got := merged.Commands["old"].Flags["policy"].Type; got != "string" {
+		t.Fatalf("merged type = %q, want the historical %q", got, "string")
+	}
+}
+
+// mergeContracts applies the same "nothing else moved" condition as
+// checkCompatibility, but against two recorded contracts rather than a live
+// pflag definition. Each case bundles one unrelated regression with the reviewed
+// type change and requires the type failure to reappear, anchoring
+// mergedFlagContractOtherwiseChanged against the checks it mirrors.
+func TestMergeRejectsReviewedFlagTypeChangeWithBundledRegression(t *testing.T) {
+	registerReviewedFixture(t, flagTypeChange{CommandPath: "dws old", Flag: "policy", From: "string", To: "int"})
+
+	tests := []struct {
+		name       string
+		historical func(*cobra.Command)
+		current    func(*cobra.Command)
+	}{
+		{
+			name:       "lost shorthand",
+			historical: func(command *cobra.Command) { command.Flags().StringP("policy", "p", "", "policy") },
+			current:    func(command *cobra.Command) { command.Flags().Int("policy", 0, "policy") },
+		},
+		{
+			name:       "became required",
+			historical: func(command *cobra.Command) { command.Flags().String("policy", "", "policy") },
+			current: func(command *cobra.Command) {
+				command.Flags().Int("policy", 0, "policy")
+				_ = command.MarkFlagRequired("policy")
+			},
+		},
+		{
+			name:       "became hidden",
+			historical: func(command *cobra.Command) { command.Flags().String("policy", "", "policy") },
+			current: func(command *cobra.Command) {
+				command.Flags().Int("policy", 0, "policy")
+				_ = command.Flags().MarkHidden("policy")
+			},
+		},
+		{
+			name:       "changed no-opt value",
+			historical: func(command *cobra.Command) { command.Flags().String("policy", "", "policy") },
+			current: func(command *cobra.Command) {
+				command.Flags().Int("policy", 0, "policy")
+				command.Flags().Lookup("policy").NoOptDefVal = "4"
+			},
+		},
+		{
+			name:       "narrowed persistent scope",
+			historical: func(command *cobra.Command) { command.PersistentFlags().String("policy", "", "policy") },
+			current:    func(command *cobra.Command) { command.Flags().Int("policy", 0, "policy") },
+		},
+	}
+
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			_, failures := mergeContracts(
+				snapshot(policyRoot(test.historical)),
+				snapshot(policyRoot(test.current)),
+			)
+			assertFailureContains(t, failures, typeChangedFailure)
+		})
+	}
+}
+
+// The behaviour tests above register their own fixtures, so they cannot catch a
+// production entry whose key is spelled in the wrong form — that mistake
+// disables the exemption silently and only a real gate run reports it. Recompute
+// every key through displayPath instead of trusting the spelling in the table.
+func TestReviewedFlagTypeChangeKeysUseDisplayPaths(t *testing.T) {
+	if len(reviewedFlagTypeChanges) == 0 {
+		t.Fatal("豁免表为空：若确已清空，请连这条守卫一起删除")
+	}
+	for change := range reviewedFlagTypeChanges {
+		if got := displayPath(internalCommandPath(change.CommandPath)); got != change.CommandPath {
+			t.Errorf("命令路径不是 displayPath 的规范形态\n  登记: %s\n  规范: %s", change.CommandPath, got)
+		}
+		if change.Flag == "" || strings.HasPrefix(change.Flag, "-") {
+			t.Errorf("%s: flag 名应是裸名（lookupFlag 用裸名查找），登记为 %q", change.CommandPath, change.Flag)
+		}
+		if change.From == "" || change.To == "" || change.From == change.To {
+			t.Errorf("%s --%s: %q -> %q 不构成类型迁移", change.CommandPath, change.Flag, change.From, change.To)
+		}
+	}
+}
+
+// Anchor every production entry to the real command tree. A misspelled command
+// or flag name would leave the exemption permanently inert, and the behaviour
+// tests cannot see that because they register their own paths.
+func TestReviewedFlagTypeChangeEntriesResolveInTheRealCommandTree(t *testing.T) {
+	if len(reviewedFlagTypeChanges) == 0 {
+		t.Fatal("豁免表为空：若确已清空，请连这条守卫一起删除")
+	}
+	root := app.NewRootCommand()
+	for change := range reviewedFlagTypeChanges {
+		command, resolved := resolveCommand(root, internalCommandPath(change.CommandPath))
+		if !resolved {
+			t.Errorf("%q 在真实命令树中不存在", change.CommandPath)
+			continue
+		}
+		flag, _ := lookupFlag(command, change.Flag)
+		if flag == nil {
+			t.Errorf("%q 上不存在 --%s", change.CommandPath, change.Flag)
+			continue
+		}
+		// Exactly one side holds depending on whether the migration has landed:
+		// From before it does, To afterwards. Asserting membership catches a
+		// misspelled type name without failing while the migration is pending.
+		if actual := flag.Value.Type(); actual != change.From && actual != change.To {
+			t.Errorf("%q --%s 当前类型 %q 既不是 %q 也不是 %q",
+				change.CommandPath, change.Flag, actual, change.From, change.To)
+		}
+	}
+}
+
+// internalCommandPath inverts displayPath so a table key can be fed back into
+// resolveCommand and displayPath.
+func internalCommandPath(displayed string) string {
+	if displayed == "dws" {
+		return "root"
+	}
+	return strings.ReplaceAll(strings.TrimPrefix(displayed, "dws "), " ", ".")
+}
+
 func testRoot() *cobra.Command {
 	root := &cobra.Command{Use: "dws"}
 	old := &cobra.Command{Use: "old"}

scripts/policy/interface-baseline/reviewed.go

@@ -0,0 +1,140 @@
+// Copyright 2026 Alibaba Group
+// Licensed under the Apache License, Version 2.0 (the "License");
+
+package main
+
+import "github.com/spf13/pflag"
+
+// flagTypeChange identifies one flag type migration exactly: which command,
+// which flag, and in which direction. Nothing is matched by wildcard — an entry
+// that differs in any of the four fields does not apply.
+//
+// CommandPath is the display form produced by displayPath ("dws minutes
+// permission apply"), not the dot-separated internal path. Writing the internal
+// form here would silently disable the exemption instead of failing loudly, so
+// TestReviewedFlagTypeChangeKeysUseDisplayPaths recomputes every key through
+// displayPath rather than trusting the spelling below.
+//
+// This table is duplicated in internal/interfacesnapshot/reviewed.go and the two
+// must stay identical; the interfacesnapshot package has a test that fails if
+// they drift. The duplication is forced rather than chosen:
+// check-authoritative-interface-baselines.sh copies this whole directory into a
+// worktree checked out at a historical revision and builds it there, so nothing
+// here may import a package this branch adds — that build would fail with an
+// unresolved import instead of reporting a compatibility result.
+type flagTypeChange struct {
+	CommandPath string
+	Flag        string
+	From        string
+	To          string
+}
+
+// reviewedFlagTypeChanges enumerates the individually reviewed flag type
+// migrations this gate accepts. A flag type is part of the published CLI
+// contract, and a type swap cannot be proven safe from the type names alone —
+// whether old invocations still parse depends on the concrete value domain the
+// command enforces. So a migration is accepted only when this exact command,
+// flag, and old→new pair appear here. Adding an entry is a contract decision
+// and belongs in review, not in a feature change.
+//
+// Entries are direction-sensitive by construction: "string" → "int" is a
+// separate key from "int" → "string" and only the reviewed direction is
+// accepted. The reverse is not a candidate for this table — widening an int
+// flag back to string lets values the parser used to reject reach RunE, which
+// is a genuine loosening of validation rather than a type migration.
+//
+// The table alone does not admit a change: reviewedFlagTypeChange is consulted
+// only when nothing else about the flag moved. See the callers in
+// checkCompatibility and mergeContracts.
+var reviewedFlagTypeChanges = map[flagTypeChange]struct{}{
+	// "dws minutes permission apply --policy" moved from a String flag parsed
+	// with strconv.ParseInt(v, 10, 64) in RunE to a native Int flag, keeping the
+	// same [2,4] domain check in RunE.
+	//
+	// This is admissible because the historical set of *successful* invocations
+	// is a subset of the new one, not merely similar to it:
+	//
+	//   - Old: the value had to parse as base-10 and land in [2,4]. That is
+	//     exactly "2", "3", "4" plus sign/leading-zero spellings of them
+	//     ("+3", "03", "003", ...).
+	//   - New: pflag's intValue.Set parses with strconv.ParseInt(s, 0, 64) —
+	//     base 0 — and RunE still enforces [2,4]. Every base-10 spelling above
+	//     resolves to the same number under base 0, so no historical success
+	//     becomes a failure.
+	//   - Base 0 additionally accepts "0x3" and friends, which the old parser
+	//     rejected. That widens the accepted set; it cannot break a caller who
+	//     was already succeeding.
+	//   - Values outside [2,4] and non-numeric values still fail. Only the
+	//     failure *message and timing* move, from RunE to flag parsing.
+	//
+	// The flag's default value changes from "" to "0" as an unavoidable
+	// consequence of the type. Neither gate compares defaults, and a default is
+	// not reachable by a caller here because RunE requires the flag to be
+	// explicitly set, so this is not a contract change.
+	{CommandPath: "dws minutes permission apply", Flag: "policy", From: "string", To: "int"}: {},
+}
+
+// reviewedFlagTypeChange reports whether this exact command, flag and direction
+// is a reviewed migration. Callers must additionally establish that the rest of
+// the flag's contract is unchanged before skipping the failure.
+func reviewedFlagTypeChange(commandPath, flag, from, to string) bool {
+	_, reviewed := reviewedFlagTypeChanges[flagTypeChange{
+		CommandPath: commandPath,
+		Flag:        flag,
+		From:        from,
+		To:          to,
+	}]
+	return reviewed
+}
+
+// flagContractOtherwiseChanged reports whether anything checkCompatibility
+// enforces about a flag, other than its type, regressed against the baseline. A
+// reviewed type migration is only accepted when this returns false, so the
+// exemption cannot carry an unrelated regression in behind it.
+//
+// Every condition here mirrors one of the checks that follow the type check in
+// checkCompatibility and must stay in step with them. The pairing is anchored by
+// tests that bundle each regression with the reviewed type change and require
+// the type failure to reappear, so a condition that drifts out of sync fails
+// loudly instead of silently widening the exemption.
+func flagContractOtherwiseChanged(expected flagContract, actual *pflag.Flag, actualScope string) bool {
+	if expected.Shorthand != "" && actual.Shorthand != expected.Shorthand {
+		return true
+	}
+	if expected.RequiredSet && !expected.Required && isRequiredFlag(actual) {
+		return true
+	}
+	if expected.HiddenSet && !expected.Hidden && actual.Hidden {
+		return true
+	}
+	if expected.NoOptSet && actual.NoOptDefVal != expected.NoOpt {
+		return true
+	}
+	if expected.ScopeSet && expected.Scope == "persistent" && actualScope == "local" {
+		return true
+	}
+	return false
+}
+
+// mergedFlagContractOtherwiseChanged is the mergeContracts counterpart of
+// flagContractOtherwiseChanged, comparing two recorded contracts rather than a
+// contract against a live pflag definition. It mirrors the checks that follow
+// the type check in mergeContracts.
+func mergedFlagContractOtherwiseChanged(oldFlag, newFlag flagContract) bool {
+	if oldFlag.Shorthand != "" && oldFlag.Shorthand != newFlag.Shorthand {
+		return true
+	}
+	if oldFlag.RequiredSet && !oldFlag.Required && newFlag.Required {
+		return true
+	}
+	if oldFlag.HiddenSet && !oldFlag.Hidden && newFlag.Hidden {
+		return true
+	}
+	if oldFlag.NoOptSet && newFlag.NoOptSet && oldFlag.NoOpt != newFlag.NoOpt {
+		return true
+	}
+	if oldFlag.ScopeSet && oldFlag.Scope == "persistent" && newFlag.Scope == "local" {
+		return true
+	}
+	return false
+}