diff --git a/CHANGELOG.md b/CHANGELOG.md index 6d9494c0..ba478a89 100644 --- a/CHANGELOG.md +++ b/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` 也带一张精确豁免表:`/` + 参数名 + 旧类型 + 新类型四元组全等才命中、方向敏感,且仅当该参数**除 `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 f79dbccf..9f2dc082 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" ) @@ -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 "/" 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}, diff --git a/scripts/policy/schema-compat/main_test.go b/scripts/policy/schema-compat/main_test.go index f1948f53..72bc6462 100644 --- a/scripts/policy/schema-compat/main_test.go +++ b/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(¤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) + }) + } +} + +// 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 不是 \"/\" 形态", 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{