From fd0c3350f373dbdac9749eb0fa25a52174397ccc Mon Sep 17 00:00:00 2001 From: chichuan Date: Mon, 10 Aug 2026 15:56:59 +0800 Subject: [PATCH 1/2] ci(schema): allow reviewed parameter type migrations MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit schema-compatibility is the third check in the same Interface Integrity job, after the two CLI interface gates. It also rejected every published parameter type change outright. Because the earlier gates failed first and `set -e` stopped the step from ever running, this one never surfaced in CI, so the previous exemption only covered two thirds of the problem. checkParameterCompatibility now consults a precise allowlist: the tool path, parameter name and both type values must match exactly, making it direction-sensitive by construction, and it applies only when nothing else the gate checks about the parameter moved (default, interface_default, format, property, interface_type, required, cli_required, required_when, enum). The type check moved to the end of the function so the carve-out can see those findings; ordering is unobservable because the result is sorted. The only entry is "minutes/minutes.apply_minutes_permission" parameter "policy" migrating from "string" to "integer" (for #912). That type is projected from the Cobra flag type (provenance cobra_flag_type), so it describes how the CLI accepts a value. Consumers build a command line from it, and "--policy 4" is the same argv under either declaration — a quoted "--policy \"4\"" still reaches pflag as 4 — while RunE keeps enforcing the same [2,4] domain. The parameter maps to property "policyId", which the command has always sent as a number, so "integer" is closer to the actual request than "string" was. Table values must be the canonical form schemaType emits: the JSON encoding of the type keyword, so `"string"` with its quotes rather than a bare string. The guard test recomputes both through schemaType and checks the decoded name against the closed JSON Schema type set — reviewedInterfaceRefRedirect was silently disabled twice by exactly this class of spelling mistake. --- CHANGELOG.md | 1 + scripts/policy/schema-compat/main.go | 96 ++++++++++++- scripts/policy/schema-compat/main_test.go | 158 ++++++++++++++++++++++ 3 files changed, 254 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6afb3a99..fe7cbf90 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -26,6 +26,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` 也带一张精确豁免表：`/` + 参数名 + 旧类型 + 新类型四元组全等才命中、方向敏感，且仅当该参数的其余契约（`default` / `interface_default` / `format` / `property` / `interface_type` / `required` / `cli_required` / `required_when` / `enum`）全部未变时才放行。唯一条目是 `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 diff --git a/scripts/policy/schema-compat/main.go b/scripts/policy/schema-compat/main.go index d186a6c1..9bf19990 100644 --- a/scripts/policy/schema-compat/main.go +++ b/scripts/policy/schema-compat/main.go @@ -814,6 +814,93 @@ 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 "/" 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 +// everything else this gate checks about the parameter is unchanged. 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), which is why the "nothing else changed" guard does not veto + // this entry. A migration that did move the default would be reported, and + // that is the intended behaviour. + { + ToolPath: "minutes/minutes.apply_minutes_permission", + Parameter: "policy", + From: `"string"`, + To: `"integer"`, + }: {}, +} + +// compatibleReviewedParameterTypeChange accepts a published parameter type +// migration when it is an explicitly reviewed entry **and** nothing else this +// gate checks about the parameter moved. +// +// otherFailures is every other finding already recorded for this parameter. The +// scope is deliberately the parameter rather than the whole tool: an unrelated +// break elsewhere in the tool is reported on its own and does not need to +// suppress this decision, whereas a change to this parameter's property, +// interface_type, default, format, required, required_when or enum means the +// contract being migrated is not the contract that was reviewed. +func compatibleReviewedParameterTypeChange(toolPath, name string, oldParameter, newParameter parameterSchema, otherFailures []string) bool { + _, reviewed := reviewedParameterTypeChanges[parameterTypeChange{ + ToolPath: toolPath, + Parameter: name, + From: oldParameter.Type, + To: newParameter.Type, + }] + return reviewed && len(otherFailures) == 0 +} + func checkParameterCompatibility(toolPath, name string, oldParameter, newParameter parameterSchema) []string { var failures []string for _, field := range []struct { @@ -821,7 +908,6 @@ func checkParameterCompatibility(toolPath, name string, oldParameter, newParamet 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}, @@ -884,6 +970,14 @@ func checkParameterCompatibility(toolPath, name string, oldParameter, newParamet if enumNarrowed(oldParameter.Enum, newParameter.Enum) { failures = append(failures, fmt.Sprintf("schema tool %q parameter %q narrowed enum", toolPath, name)) } + // The published type is checked last so the reviewed carve-out can see every + // other finding for this parameter: a reviewed migration is only accepted + // when the rest of the parameter's contract held still. Ordering is not + // observable because the result is sorted below. + if oldParameter.Type != newParameter.Type && + !compatibleReviewedParameterTypeChange(toolPath, name, oldParameter, newParameter, failures) { + failures = append(failures, fmt.Sprintf("schema tool %q parameter %q changed type", toolPath, name)) + } sort.Strings(failures) return failures } diff --git a/scripts/policy/schema-compat/main_test.go b/scripts/policy/schema-compat/main_test.go index 2d10fa53..904f6681 100644 --- a/scripts/policy/schema-compat/main_test.go +++ b/scripts/policy/schema-compat/main_test.go @@ -430,6 +430,164 @@ 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(¤t, 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(¤t, 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(¤t, func(parameter *parameterSchema) { + parameter.Type = `"integer"` + test.mutate(parameter) + }) + assertSchemaFailureContains(t, checkCompatibility(baseline, current), 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 不是 \"/\" 形态", 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{ From 7e8b216e071846b805de02fb2556c75876013c41 Mon Sep 17 00:00:00 2001 From: chichuan Date: Mon, 10 Aug 2026 21:49:11 +0800 Subject: [PATCH 2/2] ci(schema): compare the full parameter contract for reviewed type migrations Auto-CR (P1) correctly flagged that the carve-out's "nothing else changed" guard was keyed on len(otherFailures) == 0, which only observes changes the gate already judges incompatible. Several parameter contract 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. Any of those could have ridden along with a reviewed type migration, leaving the exemption wider than both its documentation and what the entry actually reviewed. Replace the failure-list heuristic with a real equality check over every published field except Type. Comparing the struct also means a field added to parameterSchema later is covered automatically, instead of silently widening every existing entry. The type check moves back ahead of the field loop because it no longer needs to observe the other findings. Add a rejection case for each individually-compatible direction. Each case first asserts that the drift alone really is compatible, so it keeps exercising the equality guard instead of quietly duplicating one of the incompatible-bundle cases. Verified against the previous implementation: with the old guard all six new cases fail while the nine incompatible-bundle cases still pass, which is exactly the gap that was reported. --- CHANGELOG.md | 2 +- scripts/policy/schema-compat/main.go | 61 ++++++++++-------- scripts/policy/schema-compat/main_test.go | 75 +++++++++++++++++++++++ 3 files changed, 113 insertions(+), 25 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b4c4e5ed..ba478a89 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -39,7 +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` 也带一张精确豁免表：`/` + 参数名 + 旧类型 + 新类型四元组全等才命中、方向敏感，且仅当该参数的其余契约（`default` / `interface_default` / `format` / `property` / `interface_type` / `required` / `cli_required` / `required_when` / `enum`）全部未变时才放行。唯一条目是 `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` 曾因键的书写形态错误两次静默失效，这里不重犯。 +- **Schema 兼容门禁支持 reviewed 参数类型豁免**（无用户可见变更）— 接上一条。`schema-compatibility` 是同一个 `Interface Integrity` job 里排在两道 CLI 接口门禁之后的第三道检查，此前也一律拒绝已发布参数的 `type` 变更。由于前两道先失败、`set -e` 让它从未在 CI 上暴露，上一条豁免只解决了三分之二。现在 `checkParameterCompatibility` 也带一张精确豁免表：`/` + 参数名 + 旧类型 + 新类型四元组全等才命中、方向敏感，且仅当该参数**除 `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 diff --git a/scripts/policy/schema-compat/main.go b/scripts/policy/schema-compat/main.go index 9bf19990..eb4163ee 100644 --- a/scripts/policy/schema-compat/main.go +++ b/scripts/policy/schema-compat/main.go @@ -12,6 +12,7 @@ import ( "fmt" "io" "os" + "reflect" "sort" "strings" ) @@ -847,8 +848,8 @@ type parameterTypeChange struct { // accepted. // // The table alone does not admit a change: the migration is rejected unless -// everything else this gate checks about the parameter is unchanged. See -// compatibleReviewedParameterTypeChange. +// 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 @@ -870,9 +871,12 @@ var reviewedParameterTypeChanges = map[parameterTypeChange]struct{}{ // 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), which is why the "nothing else changed" guard does not veto - // this entry. A migration that did move the default would be reported, and - // that is the intended behaviour. + // 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", @@ -881,28 +885,45 @@ var reviewedParameterTypeChanges = map[parameterTypeChange]struct{}{ }: {}, } -// compatibleReviewedParameterTypeChange accepts a published parameter type -// migration when it is an explicitly reviewed entry **and** nothing else this -// gate checks about the parameter moved. +// parameterContractUnchangedExceptType reports whether every published field of +// the parameter other than its type is identical. // -// otherFailures is every other finding already recorded for this parameter. The -// scope is deliberately the parameter rather than the whole tool: an unrelated -// break elsewhere in the tool is reported on its own and does not need to -// suppress this decision, whereas a change to this parameter's property, -// interface_type, default, format, required, required_when or enum means the -// contract being migrated is not the contract that was reviewed. -func compatibleReviewedParameterTypeChange(toolPath, name string, oldParameter, newParameter parameterSchema, otherFailures []string) bool { +// 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 && len(otherFailures) == 0 + 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 @@ -970,14 +991,6 @@ func checkParameterCompatibility(toolPath, name string, oldParameter, newParamet if enumNarrowed(oldParameter.Enum, newParameter.Enum) { failures = append(failures, fmt.Sprintf("schema tool %q parameter %q narrowed enum", toolPath, name)) } - // The published type is checked last so the reviewed carve-out can see every - // other finding for this parameter: a reviewed migration is only accepted - // when the rest of the parameter's contract held still. Ordering is not - // observable because the result is sorted below. - if oldParameter.Type != newParameter.Type && - !compatibleReviewedParameterTypeChange(toolPath, name, oldParameter, newParameter, failures) { - failures = append(failures, fmt.Sprintf("schema tool %q parameter %q changed type", toolPath, name)) - } sort.Strings(failures) return failures } diff --git a/scripts/policy/schema-compat/main_test.go b/scripts/policy/schema-compat/main_test.go index 904f6681..c00d3317 100644 --- a/scripts/policy/schema-compat/main_test.go +++ b/scripts/policy/schema-compat/main_test.go @@ -541,6 +541,81 @@ func TestCrossPlatformCoverageSchemaCompatReviewedParameterTypeChangeRejectsBund } } +// 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