Merge pull request #931 from DingTalk-Real-AI/fix/schema-compat-policy-int

ci(schema): allow reviewed parameter type migrations

CHANGELOG.md

@@ -39,6 +39,7 @@ The format is inspired by [Keep a Changelog](https://keepachangelog.com/) and th
 - **`sheet range batch-clear` / `batch-set-style` 的 `--ranges` 拒绝空白工作表前缀**（用户可见行为变更）— 此前只按原始串里 `!` 的位置判断，`" !A1:B2"` 修剪后工作表名成了空串，操作却照样带着 `sheetId: ""` 提交：服务端要么让整批 `batch_update` 失败，要么更糟——落到默认工作表而不是用户指定的那张表，且命令报成功。现在工作表名与范围都必须在修剪之后仍非空，否则在发起任何请求之前报错。`batch-set-style --batch` 的纯空白 `sheetId` / `range` 同样拒绝（此前只挡空字符串）；`--batch` 下发仍用原值不替用户修剪，因为 `sheetId` 可以是允许带首尾空格的工作表**名**。两条 `--ranges` 路径现在共用同一个拆分器。
 - **`sheet insert-dimension` / `delete-dimension` / `update-dimension` 的 `--length` 严格校验**（用户可见行为变更）— 解析由 `fmt.Sscanf("%d")` 改为 `strconv.Atoi`。此前只消费前缀数字，`--length 2x` / `3foo` 会被静默当成 `2` / `3` 并对错误的行列数执行操作（删除方向不可回滚）；现在整个值必须是合法正整数，否则报错「`--length` 必须为正整数（>= 1）」且不发起任何请求。**升级影响**：原先依赖这种宽松解析、在传畸形 `--length` 的脚本会开始报错，请把参数修正为纯数字。合法数字值行为不变，上限仍为 5000。`add-dimension` 的 `--length` 是 `Int` 类型 flag，一直由 cobra 严格校验，不受影响。
 - **CLI 接口兼容门禁支持 reviewed flag 类型豁免**（无用户可见变更）— `authoritative-interface-integrity` 与 `check-command-compatibility.sh` 此前一律拒绝历史命令的 flag 类型变更，即使新类型只是把同一套校验从 RunE 前移到解析期，也没有任何评审通道。现在两道门禁各带一张精确豁免表：命令路径 + flag 名 + 旧类型 → 新类型四元组全等才命中、方向敏感（`string`→`int` 与 `int`→`string` 是两个不同的键，只有被评审的方向可用），且仅当该 flag 的其他契约（shorthand / required / hidden / no-opt / scope）纹丝不动时才放行，因此豁免夹带不了别的破坏。首条也是目前唯一一条登记的是 `dws minutes permission apply --policy` 的 `string` → `int`（配合 #912）：旧实现在 RunE 里做 `strconv.ParseInt(v, 10, 64)` 再校验 `[2,4]`，新实现由 pflag 以 `strconv.ParseInt(s, 0, 64)` 解析后仍校验 `[2,4]`，**历史上能成功的调用集是新调用集的子集**（base 0 额外接受 `0x3` 这类写法，只放宽不收紧），非法值依然失败、只是报错文案与时机前移；flag 默认值由 `""` 变 `"0"` 是类型的必然结果，两道门禁都不比较默认值，且该 flag 必须显式给出、默认值不可达。两张表必须逐字一致并有守卫测试锚定漂移——重复是被迫的而非选择：`check-authoritative-interface-baselines.sh` 会把整个 `scripts/policy/interface-baseline` 目录复制进检出历史版本的 worktree 再编译，那份拷贝不能 import 本分支新增的包。
+- **Schema 兼容门禁支持 reviewed 参数类型豁免**（无用户可见变更）— 接上一条。`schema-compatibility` 是同一个 `Interface Integrity` job 里排在两道 CLI 接口门禁之后的第三道检查，此前也一律拒绝已发布参数的 `type` 变更。由于前两道先失败、`set -e` 让它从未在 CI 上暴露，上一条豁免只解决了三分之二。现在 `checkParameterCompatibility` 也带一张精确豁免表：`<product>/<tool id>` + 参数名 + 旧类型 + 新类型四元组全等才命中、方向敏感，且仅当该参数**除 `type` 外的全部已发布字段逐字段相等**时才放行。这里刻意用相等性比较而非「没有产生其他兼容性错误」：放宽 `required` / `cli_required`、清空 `required_when`、扩宽 `enum`、清空 `interface_type`、经 reviewed mapping exclusion 清空 `property`——这些变化单独看都是兼容的、根本不产生错误，若以错误列表代替相等性检查，它们就能搭着一次已评审的类型迁移一起蒙混过关。结构体整体比较还意味着将来给 `parameterSchema` 新增字段时会自动纳入守卫，而不是悄悄放宽每一条既有条目。唯一条目是 `minutes/minutes.apply_minutes_permission` 的 `policy` 由 `"string"` 迁移到 `"integer"`（配合 #912）：该 `type` 由 Cobra flag 类型投影而来（provenance `cobra_flag_type`），描述的是 CLI 如何接受取值；消费方据此拼装的是命令行，而 `--policy 4` 在两种声明下是同一个 argv，加引号的 `--policy "4"` 到 pflag 仍是 4，RunE 也仍校验 `[2,4]`——而且该参数映射的 property `policyId` 一直以数字上报，新声明比旧声明更贴近真实请求。表里的类型值必须是 `schemaType` 实际产出的带引号形态（`"string"` 而非裸 `string`），守卫测试用 `schemaType` 复算并校验类型名属于 JSON Schema 的封闭取值集合——`reviewedInterfaceRefRedirect` 曾因键的书写形态错误两次静默失效，这里不重犯。
 
 ## [1.0.58-beta.1] - 2026-08-07
 

scripts/policy/schema-compat/main.go

@@ -12,6 +12,7 @@ import (
 	"fmt"
 	"io"
 	"os"
+	"reflect"
 	"sort"
 	"strings"
 )
@@ -840,14 +841,120 @@ func compatiblePositionals(oldPositionals, newPositionals []positionalSchema) bo
 	return true
 }
 
+// parameterTypeChange identifies one published parameter type migration
+// exactly: which tool, which parameter, and in which direction. Nothing is
+// matched by wildcard — an entry differing in any of the four fields does not
+// apply.
+//
+// ToolPath is "<product id>/<tool id>" exactly as the comparison builds it (see
+// where toolPath is assembled), e.g.
+// "minutes/minutes.apply_minutes_permission". From and To are the canonical
+// type values schemaType produces — the JSON encoding of the "type" keyword,
+// so `"string"` **with** its quotes, not a bare string. Spelling either one any
+// other way silently disables the exemption instead of failing loudly (exactly
+// how reviewedInterfaceRefRedirect was broken twice), so
+// TestCrossPlatformCoverageReviewedParameterTypeChangeKeysAreCanonical
+// recomputes both through schemaType rather than trusting the table.
+type parameterTypeChange struct {
+	ToolPath  string
+	Parameter string
+	From      string
+	To        string
+}
+
+// reviewedParameterTypeChanges enumerates the individually reviewed parameter
+// type migrations this gate accepts. A published type is part of the Schema
+// contract and a swap cannot be proven safe from the type names alone, so a
+// migration is accepted only when this exact tool, parameter 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" → "integer" is a
+// separate key from "integer" → "string", and only the reviewed direction is
+// accepted.
+//
+// The table alone does not admit a change: the migration is rejected unless
+// every other published field of the parameter is identical — not merely free
+// of compatibility failures. See compatibleReviewedParameterTypeChange.
+var reviewedParameterTypeChanges = map[parameterTypeChange]struct{}{
+	// "dws minutes permission apply --policy" moved from a String flag to a
+	// native Int flag, so the published type follows it from "string" to
+	// "integer". A parameter's type is projected from the Cobra flag type
+	// (provenance source cobra_flag_type), so it describes how the CLI accepts
+	// the value, not how the backing RPC receives it.
+	//
+	// Accepted because no historical caller is invalidated:
+	//
+	//   - Consumers read this Schema to construct CLI invocations, and a command
+	//     line is text either way: "--policy 4" is the same argv under both
+	//     declarations, and a quoted "--policy \"4\"" still reaches pflag as 4.
+	//   - The flag keeps enforcing the same [2,4] domain in RunE, and pflag's
+	//     base-0 parse accepts every base-10 spelling the previous
+	//     strconv.ParseInt(v, 10, 64) accepted, so the set of successful
+	//     invocations only widens.
+	//   - "integer" also matches what actually goes on the wire: this parameter
+	//     maps to property "policyId", which the command has always sent as a
+	//     number, so the new declaration is closer to the request than the old.
+	//
+	// The parameter's default stays absent on both sides here (a zero default is
+	// not published) and every other published field is identical, which is why
+	// the equality guard admits this entry. A migration that also moved the
+	// default — or relaxed required, cleared required_when, widened enum, cleared
+	// interface_type — would be rejected even though several of those are
+	// individually compatible, because none of them were reviewed under this
+	// entry.
+	{
+		ToolPath:  "minutes/minutes.apply_minutes_permission",
+		Parameter: "policy",
+		From:      `"string"`,
+		To:        `"integer"`,
+	}: {},
+}
+
+// parameterContractUnchangedExceptType reports whether every published field of
+// the parameter other than its type is identical.
+//
+// This is a full equality check rather than "the gate recorded no other
+// failure", because several field changes are individually *compatible* and so
+// produce no failure at all: relaxing required or cli_required, clearing
+// required_when, widening enum, clearing interface_type, and clearing property
+// through a reviewed mapping exclusion. Keying the carve-out on the failure list
+// would let any of those ride along with a reviewed type migration without ever
+// having been reviewed under that entry — the exemption would be wider than what
+// it documents.
+//
+// Comparing the struct also means a field added to parameterSchema later is
+// covered here automatically, instead of silently widening every existing entry.
+func parameterContractUnchangedExceptType(oldParameter, newParameter parameterSchema) bool {
+	oldParameter.Type = ""
+	newParameter.Type = ""
+	return reflect.DeepEqual(oldParameter, newParameter)
+}
+
+// compatibleReviewedParameterTypeChange accepts a published parameter type
+// migration when it is an explicitly reviewed entry **and** the rest of the
+// parameter's published contract is unchanged.
+func compatibleReviewedParameterTypeChange(toolPath, name string, oldParameter, newParameter parameterSchema) bool {
+	_, reviewed := reviewedParameterTypeChanges[parameterTypeChange{
+		ToolPath:  toolPath,
+		Parameter: name,
+		From:      oldParameter.Type,
+		To:        newParameter.Type,
+	}]
+	return reviewed && parameterContractUnchangedExceptType(oldParameter, newParameter)
+}
+
 func checkParameterCompatibility(toolPath, name string, oldParameter, newParameter parameterSchema) []string {
 	var failures []string
+	if oldParameter.Type != newParameter.Type &&
+		!compatibleReviewedParameterTypeChange(toolPath, name, oldParameter, newParameter) {
+		failures = append(failures, fmt.Sprintf("schema tool %q parameter %q changed type", toolPath, name))
+	}
 	for _, field := range []struct {
 		name string
 		old  string
 		new  string
 	}{
-		{name: "type", old: oldParameter.Type, new: newParameter.Type},
 		{name: "default", old: oldParameter.Default, new: newParameter.Default},
 		{name: "interface_default", old: oldParameter.InterfaceDefault, new: newParameter.InterfaceDefault},
 		{name: "format", old: oldParameter.Format, new: newParameter.Format},

scripts/policy/schema-compat/main_test.go

@@ -430,6 +430,239 @@ func TestMergeContracts(t *testing.T) {
 	}
 }
 
+// registerReviewedParameterTypeFixture puts a fixture entry 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 registerReviewedParameterTypeFixture(t *testing.T, change parameterTypeChange) {
+	t.Helper()
+	if _, exists := reviewedParameterTypeChanges[change]; exists {
+		t.Fatalf("fixture %+v collides with a production entry", change)
+	}
+	reviewedParameterTypeChanges[change] = struct{}{}
+	t.Cleanup(func() { delete(reviewedParameterTypeChanges, change) })
+}
+
+func reviewedFormatTypeFixture() parameterTypeChange {
+	return parameterTypeChange{
+		ToolPath:  "doc/doc.create",
+		Parameter: "format",
+		From:      `"string"`,
+		To:        `"integer"`,
+	}
+}
+
+const docCreateFormatTypeFailure = `schema tool "doc/doc.create" parameter "format" changed type`
+
+func assertSchemaFailureContains(t *testing.T, failures []string, want string) {
+	t.Helper()
+	for _, failure := range failures {
+		if strings.Contains(failure, want) {
+			return
+		}
+	}
+	t.Fatalf("failures %v do not contain %q", failures, want)
+}
+
+func TestCrossPlatformCoverageSchemaCompatReviewedParameterTypeChange(t *testing.T) {
+	baseline := cloneContract(baselineContract())
+	current := cloneContract(baseline)
+	mutateParameter(&current, func(parameter *parameterSchema) { parameter.Type = `"integer"` })
+
+	// Without an entry the migration is still a blocking change.
+	assertSchemaFailureContains(t, checkCompatibility(baseline, current), docCreateFormatTypeFailure)
+
+	registerReviewedParameterTypeFixture(t, reviewedFormatTypeFixture())
+	if failures := checkCompatibility(baseline, current); len(failures) != 0 {
+		t.Fatalf("a reviewed parameter type migration should pass: %v", failures)
+	}
+}
+
+func TestCrossPlatformCoverageSchemaCompatReviewedParameterTypeChangeIsDirectionSensitive(t *testing.T) {
+	registerReviewedParameterTypeFixture(t, reviewedFormatTypeFixture())
+
+	// The reverse migration shares tool and parameter but not direction, so the
+	// string->integer entry must not admit it.
+	baseline := cloneContract(baselineContract())
+	mutateParameter(&baseline, func(parameter *parameterSchema) { parameter.Type = `"integer"` })
+	current := cloneContract(baselineContract())
+	assertSchemaFailureContains(t, checkCompatibility(baseline, current), docCreateFormatTypeFailure)
+}
+
+func TestCrossPlatformCoverageSchemaCompatReviewedParameterTypeChangeRejectsOtherToolsAndParameters(t *testing.T) {
+	baseline := cloneContract(baselineContract())
+	current := cloneContract(baseline)
+	mutateParameter(&current, func(parameter *parameterSchema) { parameter.Type = `"integer"` })
+
+	// Same parameter and direction, different tool.
+	other := reviewedFormatTypeFixture()
+	other.ToolPath = "doc/doc.elsewhere"
+	registerReviewedParameterTypeFixture(t, other)
+	assertSchemaFailureContains(t, checkCompatibility(baseline, current), docCreateFormatTypeFailure)
+
+	// Same tool and direction, different parameter.
+	elsewhere := reviewedFormatTypeFixture()
+	elsewhere.Parameter = "title"
+	registerReviewedParameterTypeFixture(t, elsewhere)
+	assertSchemaFailureContains(t, checkCompatibility(baseline, current), docCreateFormatTypeFailure)
+}
+
+// A reviewed migration is only accepted when the rest of the parameter's
+// contract held still. Each case bundles one unrelated regression with the
+// reviewed type change and requires the type failure to reappear, so the
+// exemption cannot carry a second change in behind it.
+func TestCrossPlatformCoverageSchemaCompatReviewedParameterTypeChangeRejectsBundledRegression(t *testing.T) {
+	registerReviewedParameterTypeFixture(t, reviewedFormatTypeFixture())
+
+	tests := []struct {
+		name   string
+		mutate func(*parameterSchema)
+	}{
+		{name: "changed default", mutate: func(p *parameterSchema) { p.Default = `"text"` }},
+		{name: "changed interface_default", mutate: func(p *parameterSchema) { p.InterfaceDefault = `"text"` }},
+		{name: "changed format", mutate: func(p *parameterSchema) { p.Format = "uri" }},
+		{name: "redirected property", mutate: func(p *parameterSchema) { p.Property = "renamed" }},
+		{name: "redirected interface_type", mutate: func(p *parameterSchema) { p.InterfaceType = "integer" }},
+		{name: "newly required", mutate: func(p *parameterSchema) { p.Required = true }},
+		{name: "newly cli_required", mutate: func(p *parameterSchema) { p.CLIRequired = true }},
+		{name: "changed required_when", mutate: func(p *parameterSchema) { p.RequiredWhen = "always" }},
+		{name: "narrowed enum", mutate: func(p *parameterSchema) { p.Enum = []string{"markdown"} }},
+	}
+
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			baseline := cloneContract(baselineContract())
+			current := cloneContract(baseline)
+			mutateParameter(&current, func(parameter *parameterSchema) {
+				parameter.Type = `"integer"`
+				test.mutate(parameter)
+			})
+			assertSchemaFailureContains(t, checkCompatibility(baseline, current), docCreateFormatTypeFailure)
+		})
+	}
+}
+
+// Every drift below is individually *compatible* — on its own it produces no
+// failure at all — which is precisely why the carve-out cannot be keyed on "the
+// gate recorded no other failure for this parameter". Bundled with a reviewed
+// type migration they must still be rejected: none of them were reviewed under
+// that entry, and admitting them would make the exemption wider than what the
+// table documents.
+//
+// Each case first asserts that the drift alone really is compatible, so a future
+// rule change that starts reporting it turns into an explicit failure here
+// rather than quietly turning this test into a duplicate of the bundled-
+// regression cases above.
+func TestCrossPlatformCoverageSchemaCompatReviewedParameterTypeChangeRejectsIndividuallyCompatibleDrift(t *testing.T) {
+	registerReviewedParameterTypeFixture(t, reviewedFormatTypeFixture())
+
+	tests := []struct {
+		name     string
+		baseline func(*parameterSchema)
+		current  func(*parameterSchema)
+	}{
+		{
+			name:     "relaxed required",
+			baseline: func(p *parameterSchema) { p.Required = true },
+			current:  func(p *parameterSchema) { p.Required = false },
+		},
+		{
+			name:     "relaxed cli_required",
+			baseline: func(p *parameterSchema) { p.CLIRequired = true },
+			current:  func(p *parameterSchema) { p.CLIRequired = false },
+		},
+		{
+			name:     "cleared required_when",
+			baseline: func(p *parameterSchema) { p.RequiredWhen = "always" },
+			current:  func(p *parameterSchema) { p.RequiredWhen = "" },
+		},
+		{
+			name:    "widened enum",
+			current: func(p *parameterSchema) { p.Enum = []string{"html", "markdown", "text"} },
+		},
+		{
+			name:     "cleared interface_type",
+			baseline: func(p *parameterSchema) { p.InterfaceType = "string" },
+			current:  func(p *parameterSchema) { p.InterfaceType = "" },
+		},
+		{
+			name: "cleared property through reviewed mapping exclusion",
+			current: func(p *parameterSchema) {
+				p.Property = ""
+				p.PropertySource = propertySourceReviewedMappingExclusion
+			},
+		},
+	}
+
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			baseline := cloneContract(baselineContract())
+			if test.baseline != nil {
+				mutateParameter(&baseline, test.baseline)
+			}
+
+			driftOnly := cloneContract(baseline)
+			mutateParameter(&driftOnly, test.current)
+			if failures := checkCompatibility(baseline, driftOnly); len(failures) != 0 {
+				t.Fatalf("前提不成立：该变化本身已不兼容，用例测不到相等性守卫: %v", failures)
+			}
+
+			bundled := cloneContract(baseline)
+			mutateParameter(&bundled, func(parameter *parameterSchema) {
+				parameter.Type = `"integer"`
+				test.current(parameter)
+			})
+			assertSchemaFailureContains(t, checkCompatibility(baseline, bundled), docCreateFormatTypeFailure)
+		})
+	}
+}
+
+// The behaviour tests above register their own fixtures, so they cannot catch a
+// production entry whose type values are spelled in the wrong form. That mistake
+// disables the exemption silently and only a real gate run reports it — exactly
+// how reviewedInterfaceRefRedirect was broken twice. Recompute both values
+// through schemaType instead of trusting the spelling in the table.
+func TestCrossPlatformCoverageReviewedParameterTypeChangeKeysAreCanonical(t *testing.T) {
+	if len(reviewedParameterTypeChanges) == 0 {
+		t.Fatal("豁免表为空：若确已清空，请连这条守卫一起删除")
+	}
+	for change := range reviewedParameterTypeChanges {
+		if strings.Count(change.ToolPath, "/") != 1 ||
+			strings.HasPrefix(change.ToolPath, "/") || strings.HasSuffix(change.ToolPath, "/") {
+			t.Errorf("tool path %q 不是 \"<product id>/<tool id>\" 形态", change.ToolPath)
+		}
+		if strings.TrimSpace(change.Parameter) != change.Parameter || change.Parameter == "" ||
+			strings.HasPrefix(change.Parameter, "-") {
+			t.Errorf("%s: 参数名应是 Schema 里的裸名，登记为 %q", change.ToolPath, change.Parameter)
+		}
+		if change.From == change.To {
+			t.Errorf("%s %s: From 与 To 相同，不构成迁移", change.ToolPath, change.Parameter)
+		}
+		for _, side := range []struct {
+			label string
+			value string
+		}{{"From", change.From}, {"To", change.To}} {
+			var name string
+			if err := json.Unmarshal([]byte(side.value), &name); err != nil {
+				t.Errorf("%s %s 的 %s=%q 不是合法的 JSON type 值（schemaType 产出带引号的形态，如 `\"string\"`）: %v",
+					change.ToolPath, change.Parameter, side.label, side.value, err)
+				continue
+			}
+			if got := schemaType(map[string]any{"type": name}); got != side.value {
+				t.Errorf("%s %s 的 %s 不是规范形态\n  登记: %s\n  规范: %s",
+					change.ToolPath, change.Parameter, side.label, side.value, got)
+			}
+			// 复算只能证明引号形态对，证明不了类型名本身没拼错——"integar" 同样
+			// 能通过复算。JSON Schema 的 type 取值是封闭集合，直接校验。
+			switch name {
+			case "string", "integer", "number", "boolean", "array", "object", "null":
+			default:
+				t.Errorf("%s %s 的 %s=%q 不是 JSON Schema 的合法 type 名",
+					change.ToolPath, change.Parameter, side.label, name)
+			}
+		}
+	}
+}
+
 func baselineContract() schemaContract {
 	return schemaContract{Version: schemaContractVersion, Products: map[string]productSchema{
 		"doc": {Tools: map[string]toolSchema{