From 08c9c8a923749375450848a65078664aae766cfd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E7=8E=89=E6=BE=9C?= Date: Thu, 30 Jul 2026 21:12:56 +0800 Subject: [PATCH] fix(cmdcore): close review findings on safety tier inference MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - schemaSafetyFromDecl: drop the now-unreachable nil return; the tier fill always produces a complete block for a declared Schema - validateDispatchDecl: panic when ConfirmFirst is set without Risk — it orders a confirmation that does not exist, and for declared-Schema writes an empty Risk would silently publish the read safety tier - RFC: document the boundary that write commands must declare Risk Co-Authored-By: Claude Opus 4.6 Co-authored-by: Cursor --- docs/rfc-command-framework-convergence.md | 2 +- internal/cmdcore/cmdcore.go | 20 +++++++++++++------- internal/cmdcore/cmdcore_test.go | 9 +++++++++ internal/cmdcore/schema_decl.go | 6 +++++- 4 files changed, 28 insertions(+), 9 deletions(-) diff --git a/docs/rfc-command-framework-convergence.md b/docs/rfc-command-framework-convergence.md index 44263932..8d681904 100644 --- a/docs/rfc-command-framework-convergence.md +++ b/docs/rfc-command-framework-convergence.md @@ -1146,7 +1146,7 @@ cmd.RunE = func(cmd *cobra.Command, args []string) error { | 非 Shortcut 受管定义的 Cobra 命令 Hidden | 可执行 Contract | 挂载的命令可见性与声明匹配 | | Shortcut 列表成员资格与语义 disposition | 经评审的 Shortcut 可见性解析器 | public/all 列表成员资格与经评审决策匹配 | | Runtime Schema / Agent 暴露 | 经评审的 CommandRegistry 加精确排除 | 每个暴露叶子解析到活 Contract；排除显式且不重叠 | -| 运行时 Risk 与确认 | 可执行 Contract（非空 `Risk`）或显式 annotate（如 `runtime_gate`）；见 §5.0 | 不得靠推断；空 Risk 不嵌入 schema risk；写命令须 Risk 或 gate；Safety 组装优先级 **`Schema.Safety`（Final 显式字段）> `CommandSpec.Safety` 枚举 tier > `Risk.SafetyDefault()` > `runtime_gate`**，高优先命中即忽略低优先；tier 填充含 idempotency（读 idempotent / 写 unknown），故仅声明 Risk/Safety 枚举即自足 | +| 运行时 Risk 与确认 | 可执行 Contract（非空 `Risk`）或显式 annotate（如 `runtime_gate`）；见 §5.0 | 不得靠推断；空 Risk 不嵌入 schema risk；写命令须 Risk 或 gate；Safety 组装优先级 **`Schema.Safety`（Final 显式字段）> `CommandSpec.Safety` 枚举 tier > `Risk.SafetyDefault()` > `runtime_gate`**，高优先命中即忽略低优先；tier 填充含 idempotency（读 idempotent / 写 unknown），故仅声明 Risk/Safety 枚举即自足。**边界**：tier 恒可推导意味着写命令忘声明 `Risk` 会静默发布 read tier（且失去运行时确认）——写命令必须声明 `Risk`；`ConfirmFirst` 无 `Risk` 属 authoring 错误，构造期 panic | | 后端 product/tool/载荷绑定 | mcpbind + 后端元数据 | 每个绑定引用真实的 flag/属性 | | Agent 选择文案（`use_when`、`avoid_when`、摘要） | 经评审的 hints/catalog | 身份解析到活契约 | diff --git a/internal/cmdcore/cmdcore.go b/internal/cmdcore/cmdcore.go index 97f9c613..9f000946 100644 --- a/internal/cmdcore/cmdcore.go +++ b/internal/cmdcore/cmdcore.go @@ -469,6 +469,15 @@ func validateDispatchDecl(spec CommandSpec) { "command %q must declare exactly one of RunE/Invoke/Orchestrate, got %d", spec.Use, declared)) } + // ConfirmFirst only makes sense on a Risk-driven confirmation flow. A + // command with ConfirmFirst but no Risk is always an authoring mistake — + // and for a declared-Schema write it would also silently publish the read + // safety tier (empty Risk infers SafetyRead). + if spec.ConfirmFirst && strings.TrimSpace(string(spec.Risk)) == "" { + panic(fmt.Sprintf( + "command %q sets ConfirmFirst but declares no Risk: ConfirmFirst orders the Risk-driven confirmation and is meaningless without one", + spec.Use)) + } } // RegisterFlags registers every flag (plus hidden aliases and MarkFlagRequired) @@ -1073,13 +1082,10 @@ func schemaSafetyFromDecl(risk Risk, safety Safety, decl SafetyDecl) *cli.Safety } // effect_source is assembly-derived for every Contract-declared safety, // whether it came from the Risk shorthand or an explicit SchemaDecl; the - // declaration in code is the source of truth. - if out.Effect != "" && out.EffectSource == "" { - out.EffectSource = "cmdcore.contract" - } - if out.Effect == "" && out.Risk == "" && out.Confirmation == "" && out.Idempotency == "" { - return nil - } + // declaration in code is the source of truth. The tier fill above always + // produces a complete block (read tier by default), so the payload is + // never nil for a declared Schema. + out.EffectSource = "cmdcore.contract" return &out } diff --git a/internal/cmdcore/cmdcore_test.go b/internal/cmdcore/cmdcore_test.go index bf775e9d..6d20ce13 100644 --- a/internal/cmdcore/cmdcore_test.go +++ b/internal/cmdcore/cmdcore_test.go @@ -899,6 +899,15 @@ func TestCrossPlatformCoverageNewCommandRequiresExactlyOneDispatcher(t *testing. RunE: func(*cobra.Command, []string) error { return nil }, Invoke: func(*Ctx, map[string]any) error { return nil }, }, "got 2") + + // ConfirmFirst without Risk orders a confirmation that does not exist — and + // for declared-Schema writes it would also publish the read safety tier. + mustPanic("confirmFirst without risk", CommandSpec{ + Use: "guarded", + Flags: flags, + ConfirmFirst: true, + Invoke: func(*Ctx, map[string]any) error { return nil }, + }, "ConfirmFirst but declares no Risk") } func TestCrossPlatformCoverageNewCommandOrchestrateDispatch(t *testing.T) { diff --git a/internal/cmdcore/schema_decl.go b/internal/cmdcore/schema_decl.go index 2b2a041b..fbdd330c 100644 --- a/internal/cmdcore/schema_decl.go +++ b/internal/cmdcore/schema_decl.go @@ -131,7 +131,11 @@ func validateSchemaDecl(spec CommandSpec) { // (no hints fallback). Safety needs no completeness check: the enum tier // (CommandSpec.Safety, falling back to Risk.SafetyDefault()) fills every // Safety field including Idempotency, so any declared Schema projects a - // complete safety block by construction. + // complete safety block by construction. The flip side of always-on + // inference: a WRITE command must declare Risk (which also drives runtime + // confirmation) — an empty Risk publishes the read tier. The detectable + // half of that mistake (ConfirmFirst without Risk) is caught by + // validateDispatchDecl. if iface := spec.Schema.Interface; iface == nil || strings.TrimSpace(iface.Mode) == "" || strings.TrimSpace(iface.Availability) == "" { missing = append(missing, "Schema.Interface (mode/availability)")