From d8a3d5d6fd0fa0f6e8fd50bbd6eec60dadb87d3b Mon Sep 17 00:00:00 2001 From: chichuan Date: Wed, 19 Aug 2026 22:57:11 +0800 Subject: [PATCH] fix(ci): close command migration governance gaps --- .changes/command-path-migration-governance.md | 2 +- docs/cli-interface-flag-migrations.md | 8 +- .../interfacesnapshot/command_migrations.go | 45 ++++++++ .../command_migrations_test.go | 104 ++++++++++++++++++ .../schema-compat/command_migrations_test.go | 55 +++++++++ scripts/policy/schema-compat/main.go | 24 ++++ 6 files changed, 236 insertions(+), 2 deletions(-) diff --git a/.changes/command-path-migration-governance.md b/.changes/command-path-migration-governance.md index b78a839d..fac7e8aa 100644 --- a/.changes/command-path-migration-governance.md +++ b/.changes/command-path-migration-governance.md @@ -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. diff --git a/docs/cli-interface-flag-migrations.md b/docs/cli-interface-flag-migrations.md index ef2fa9e8..e7e71b3e 100644 --- a/docs/cli-interface-flag-migrations.md +++ b/docs/cli-interface-flag-migrations.md @@ -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 记录不能批准自己的改动。 diff --git a/internal/interfacesnapshot/command_migrations.go b/internal/interfacesnapshot/command_migrations.go index 785c02b1..e9cdac4e 100644 --- a/internal/interfacesnapshot/command_migrations.go +++ b/internal/interfacesnapshot/command_migrations.go @@ -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, diff --git a/internal/interfacesnapshot/command_migrations_test.go b/internal/interfacesnapshot/command_migrations_test.go index 7b511f29..8172ebcc 100644 --- a/internal/interfacesnapshot/command_migrations_test.go +++ b/internal/interfacesnapshot/command_migrations_test.go @@ -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) diff --git a/scripts/policy/schema-compat/command_migrations_test.go b/scripts/policy/schema-compat/command_migrations_test.go index af1636bc..5b3212f8 100644 --- a/scripts/policy/schema-compat/command_migrations_test.go +++ b/scripts/policy/schema-compat/command_migrations_test.go @@ -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 { diff --git a/scripts/policy/schema-compat/main.go b/scripts/policy/schema-compat/main.go index 2d19c0e2..44319c65 100644 --- a/scripts/policy/schema-compat/main.go +++ b/scripts/policy/schema-compat/main.go @@ -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)