fix(ci): close command migration governance gaps

2026-08-19 22:57:11 +08:00
1 parent 5ed7c3adce
commit d8a3d5d6fd
@@ -2,4 +2,4 @@
 category: Changed
 ---

- **CLI compatibility governance** — adds a reviewed two-stage path for hiding retained legacy commands or flags from Help and Schema when capability moves to a dedicated command.
+- **CLI compatibility governance** — adds a reviewed two-stage path for hiding retained legacy commands or optional flags from Help and Schema when capability moves to a dedicated command, with legacy-leaf and fail-closed required-parameter checks.
@@ -55,7 +55,13 @@ scripts/policy/interface-migrations/approved-command-migrations-v1.json
 | `command_move` | legacy 命令仍 runnable、由 visible 变 hidden；replacement 由 absent 变 visible runnable | 同一 stable tool identity 的 `primary_cli_path` 改到 replacement；只允许清单列出的参数改名，参数类型、property、requiredness、default 等必须等价 |
 | `flag_extraction` | legacy 命令保持 visible runnable；指定 legacy flag 仍可执行但由 visible 变 hidden；replacement 由 absent 变 visible runnable | source tool 只删除指定参数；replacement tool 必须位于精确的新路径，并保持 source 的 interface 与 safety identity |

-两种迁移都要求旧 argv 继续可执行。删除旧命令、删除旧 flag、把 legacy 改成 non-runnable、改变未登记参数、改变 interface / safety，或只完成部分 before → after 转换都会 fail closed。命令别名会先规范到 reference 的 canonical path，但清单本身仍只能记录精确 canonical 命令，不能用 alias 或前缀扩大授权。
+`command_move` 只能隐藏没有子命令的 legacy leaf，且 legacy 与 replacement
+不得互为祖先路径；整棵命令树的迁移需要单独设计逐叶治理，不能复用这一原语。
+稳定 Schema tool 可以继续接受普通的 optional 参数新增，但不得借路径迁移引入清单未登记的
+`required`、`cli_required` 或 `required_when` 参数。`flag_extraction` 只接受
+optional legacy flag，不能隐藏仍由 Cobra hard-required 的参数。
+
+两种迁移都要求旧 argv 继续可执行。删除旧命令、删除旧 flag、把 legacy 改成 non-runnable、改变未登记的历史参数、改变 interface / safety，或只完成部分 before → after 转换都会 fail closed。命令别名会先规范到 reference 的 canonical path，但清单本身仍只能记录精确 canonical 命令，不能用 alias 或前缀扩大授权。

 跨命令清单复用下文同一套 `pending → consumed → cleanup` 生命周期。治理 PR 只能新增 `pending` 且产品 surface 必须仍是 before；后续产品 PR 才能一次性切到 after 并改为 `consumed`。candidate 新增的 pending 记录不能批准自己的改动。

@@ -156,6 +156,10 @@ func (m CommandMigration) validate() error {
 	}
 	switch m.Kind {
 	case CommandMigrationMove:
+		if strings.HasPrefix(m.Replacement.Command, m.Legacy.Command+" ") ||
+			strings.HasPrefix(m.Legacy.Command, m.Replacement.Command+" ") {
+			return fmt.Errorf("command_move legacy and replacement command paths must not have an ancestor relationship")
+		}
 		if !m.Legacy.After.Hidden {
 			return fmt.Errorf("command_move legacy command must migrate exactly from visible to hidden")
 		}
@@ -208,6 +212,9 @@ func (f CommandMigrationFlag) validate() error {
 	if !f.Before.Present || !f.After.Present || f.Before.Hidden || !f.After.Hidden {
 		return fmt.Errorf("legacy_flag must migrate exactly from visible to hidden while remaining present")
 	}
+	if f.Before.Required || f.After.Required {
+		return fmt.Errorf("legacy_flag must remain optional before and after extraction")
+	}
 	before := f.Before
 	after := f.After
 	before.Hidden = false
@@ -297,9 +304,47 @@ func AuthorizeCommandMigrations(
 	if err := candidate.Validate(); err != nil {
 		return nil, fmt.Errorf("validate candidate command migrations: %w", err)
 	}
+	type commandMoveTopologyCheck struct {
+		label    string
+		snapshot Snapshot
+		manifest CommandMigrationManifest
+	}
+	topologyChecks := []commandMoveTopologyCheck{
+		{label: "approved", snapshot: current, manifest: authority},
+		{label: "candidate", snapshot: current, manifest: candidate},
+	}
+	if mergeBase, _, ok := flagMigrationAuthoritySnapshot(references); ok {
+		topologyChecks = append(topologyChecks,
+			commandMoveTopologyCheck{label: "approved base", snapshot: mergeBase, manifest: authority},
+			commandMoveTopologyCheck{label: "candidate base", snapshot: mergeBase, manifest: candidate},
+		)
+	}
+	for _, check := range topologyChecks {
+		if err := validateCommandMoveLegacyLeaves(check.snapshot, check.manifest); err != nil {
+			return nil, fmt.Errorf("validate %s command migration topology: %w", check.label, err)
+		}
+	}
 	return evaluateCommandMigrationLifecycle(current, references, authority, candidate)
 }

+func validateCommandMoveLegacyLeaves(snapshot Snapshot, manifest CommandMigrationManifest) error {
+	commands := commandIndex(snapshot)
+	for _, migration := range manifest.Migrations {
+		if migration.Kind != CommandMigrationMove {
+			continue
+		}
+		if _, present := commands[migration.Legacy.Command]; !present {
+			continue
+		}
+		for _, command := range snapshot.Commands {
+			if strings.HasPrefix(command.Path, migration.Legacy.Command+" ") {
+				return fmt.Errorf("command_move legacy command %q must be a leaf", migration.Legacy.Command)
+			}
+		}
+	}
+	return nil
+}
+
 func evaluateCommandMigrationLifecycle(
 	current Snapshot,
 	references map[string]Snapshot,
@@ -170,6 +170,110 @@ func TestCrossPlatformCoverageCommandMigrationValidationEdges(t *testing.T) {
 	}
 }

+func TestCrossPlatformCoverageFlagExtractionRejectsRequiredLegacyFlag(t *testing.T) {
+	migration := commandMigrationManifest(CommandMigrationPending).Migrations[1]
+	migration.LegacyFlag.Before.Required = true
+	migration.LegacyFlag.After.Required = true
+
+	if err := migration.validate(); err == nil || !strings.Contains(err.Error(), "optional") {
+		t.Fatalf("required legacy flag validation error=%v, want optional-only rejection", err)
+	}
+}
+
+func TestCrossPlatformCoverageCommandMoveRejectsRunnableParent(t *testing.T) {
+	migration := commandMigrationManifest(CommandMigrationPending).Migrations[0]
+	migration.Legacy.Command = "dws agoal"
+	migration.Replacement.Command = "dws goal"
+	migration.Schema = CommandMigrationSchema{
+		ProductID:         "agoal",
+		SourceToolID:      "agoal.root",
+		ReplacementToolID: "agoal.root",
+		Parameters:        []CommandParameterMigration{},
+	}
+	authority := CommandMigrationManifest{
+		Version:    CommandMigrationManifestVersion,
+		Migrations: []CommandMigration{migration},
+	}
+	consumed := authority
+	consumed.Migrations = append([]CommandMigration(nil), authority.Migrations...)
+	consumed.Migrations[0].State = CommandMigrationConsumed
+
+	before := testSnapshot(
+		testCommand("dws"),
+		testCommand("dws agoal"),
+		testCommand("dws agoal strategy"),
+		testCommand("dws agoal strategy list"),
+	)
+	after := testSnapshot(
+		testCommand("dws"),
+		Command{Path: "dws agoal", Runnable: true, Hidden: true},
+		testCommand("dws goal"),
+	)
+
+	_, err := AuthorizeCommandMigrations(
+		after,
+		map[string]Snapshot{"merge-base": before, "stable": before},
+		authority,
+		consumed,
+	)
+	if err == nil || !strings.Contains(err.Error(), "must be a leaf") {
+		t.Fatalf("runnable parent command_move error=%v, want leaf-only rejection", err)
+	}
+}
+
+func TestCrossPlatformCoverageCommandMoveAllowsReplacementParent(t *testing.T) {
+	before := commandMigrationSnapshot(false, false)
+	after := commandMigrationSnapshot(true, false)
+	after.Commands = append(after.Commands, testCommand("dws chat topic new detail"))
+	pending := singleCommandMigrationManifest(CommandMigrationPending)
+	consumed := singleCommandMigrationManifest(CommandMigrationConsumed)
+	if err := validateCommandMoveLegacyLeaves(testSnapshot(testCommand("dws")), pending); err != nil {
+		t.Fatalf("absent legacy topology should remain a lifecycle concern: %v", err)
+	}
+
+	got, err := AuthorizeCommandMigrations(
+		after,
+		map[string]Snapshot{"merge-base": before, "stable": before},
+		pending,
+		consumed,
+	)
+	if err != nil {
+		t.Fatalf("replacement parent command_move was rejected: %v", err)
+	}
+	if len(got) != 1 {
+		t.Fatalf("replacement parent command_move authorizations=%d, want 1", len(got))
+	}
+}
+
+func TestCrossPlatformCoverageCommandMoveRejectsAncestorOverlap(t *testing.T) {
+	for _, test := range []struct {
+		name        string
+		legacy      string
+		replacement string
+	}{
+		{
+			name:        "legacy ancestor",
+			legacy:      "dws chat message",
+			replacement: "dws chat message list",
+		},
+		{
+			name:        "replacement ancestor",
+			legacy:      "dws chat message list",
+			replacement: "dws chat message",
+		},
+	} {
+		t.Run(test.name, func(t *testing.T) {
+			migration := commandMigrationManifest(CommandMigrationPending).Migrations[0]
+			migration.Legacy.Command = test.legacy
+			migration.Replacement.Command = test.replacement
+
+			if err := migration.validate(); err == nil || !strings.Contains(err.Error(), "must not have an ancestor relationship") {
+				t.Fatalf("overlapping command_move error=%v, want ancestor-overlap rejection", err)
+			}
+		})
+	}
+}
+
 func TestCrossPlatformCoverageCommandMigrationLifecycleEdges(t *testing.T) {
 	before := commandMigrationSnapshot(false, false)
 	after := commandMigrationSnapshot(true, false)
@@ -75,6 +75,48 @@ func TestCrossPlatformCoverageSchemaCommandMigrationsFailClosedOnDrift(t *testin
 	}
 }

+func TestCrossPlatformCoverageSchemaCommandMigrationRejectsUnregisteredRequiredParameters(t *testing.T) {
+	baseline := schemaCommandMigrationContract(false)
+	migrations := schemaCommandMigrationAuthorizations()
+
+	for _, test := range []struct {
+		name      string
+		parameter parameterSchema
+	}{
+		{name: "required", parameter: parameterSchema{Type: `"string"`, Property: "must", Required: true}},
+		{name: "cli_required", parameter: parameterSchema{Type: `"string"`, Property: "must", CLIRequired: true}},
+		{name: "required_when", parameter: parameterSchema{Type: `"string"`, Property: "must", RequiredWhen: "mode=forced"}},
+	} {
+		t.Run(test.name, func(t *testing.T) {
+			current := schemaCommandMigrationContract(true)
+			product := current.Products["chat"]
+			tool := product.Tools["chat.move"]
+			tool.Parameters["must"] = test.parameter
+			product.Tools["chat.move"] = tool
+			current.Products["chat"] = product
+
+			if _, err := normalizeSchemaCommandMigrations(baseline, current, migrations); err == nil ||
+				!strings.Contains(err.Error(), `introduced unregistered required Schema parameter "must"`) {
+				t.Fatalf("unregistered required parameter error=%v", err)
+			}
+		})
+	}
+
+	current := schemaCommandMigrationContract(true)
+	product := current.Products["chat"]
+	tool := product.Tools["chat.move"]
+	tool.Parameters["optional"] = parameterSchema{Type: `"string"`, Property: "optional"}
+	product.Tools["chat.move"] = tool
+	current.Products["chat"] = product
+	normalized, err := normalizeSchemaCommandMigrations(baseline, current, migrations)
+	if err != nil {
+		t.Fatalf("optional addition should remain compatible: %v", err)
+	}
+	if failures := checkCompatibility(normalized, current); len(failures) != 0 {
+		t.Fatalf("optional addition should remain compatible: %v", failures)
+	}
+}
+
 func TestCrossPlatformCoverageSchemaCommandMigrationNormalizationEdges(t *testing.T) {
 	baseline := schemaCommandMigrationContract(false)
 	current := schemaCommandMigrationContract(true)
@@ -273,6 +315,19 @@ func TestCrossPlatformCoverageSchemaCommandMigrationLifecycleAndRun(t *testing.T
 	if code := run(args, &stdout, &stderr); code != 2 || !strings.Contains(stderr.String(), "normalize approved Schema command migrations") {
 		t.Fatalf("command normalization error code=%d stderr=%q", code, stderr.String())
 	}
+
+	requiredAddition := schemaCommandMigrationContract(true)
+	product = requiredAddition.Products["chat"]
+	tool = product.Tools["chat.move"]
+	tool.Parameters["must"] = parameterSchema{Type: `"string"`, Property: "must", Required: true, CLIRequired: true}
+	product.Tools["chat.move"] = tool
+	requiredAddition.Products["chat"] = product
+	writeRawSchemaContractFile(t, currentPath, requiredAddition)
+	stderr.Reset()
+	if code := run(args, &stdout, &stderr); code != 2 ||
+		!strings.Contains(stderr.String(), `introduced unregistered required Schema parameter "must"`) {
+		t.Fatalf("unregistered required addition code=%d stderr=%q", code, stderr.String())
+	}
 }

 func schemaCommandMigrationContract(after bool) schemaContract {
@@ -1415,6 +1415,7 @@ func normalizeSchemaCommandMigrations(
 				continue
 			}
 			renames := make(map[string]string, len(migration.Schema.Parameters))
+			renameTargets := make(map[string]struct{}, len(migration.Schema.Parameters))
 			for _, parameter := range migration.Schema.Parameters {
 				oldParameter, existed := oldTool.Parameters[parameter.From]
 				if !existed {
@@ -1445,6 +1446,29 @@ func normalizeSchemaCommandMigrations(
 				delete(normalizedTool.Parameters, parameter.From)
 				normalizedTool.Parameters[parameter.To] = newParameter
 				renames[parameter.From] = parameter.To
+				renameTargets[parameter.To] = struct{}{}
+			}
+			parameterNames := make([]string, 0, len(newSource.Parameters))
+			for name := range newSource.Parameters {
+				parameterNames = append(parameterNames, name)
+			}
+			sort.Strings(parameterNames)
+			for _, name := range parameterNames {
+				parameter := newSource.Parameters[name]
+				if _, existed := oldTool.Parameters[name]; existed {
+					continue
+				}
+				if _, approvedRename := renameTargets[name]; approvedRename {
+					continue
+				}
+				if parameter.Required || parameter.CLIRequired || parameter.RequiredWhen != "" {
+					return schemaContract{}, fmt.Errorf(
+						"approved command migration %q replacement Schema tool %q introduced unregistered required Schema parameter %q",
+						migration.Legacy.Command,
+						migration.Schema.SourceToolID,
+						name,
+					)
+				}
 			}
 			if oldTool.Constraints != newSource.Constraints {
 				oldConstraints, oldOK := canonicalizeMigratedConstraints(oldTool.Constraints, renames)