diff --git a/docs/rfc-command-framework-convergence.md b/docs/rfc-command-framework-convergence.md new file mode 100644 index 00000000..6a3fcfe7 --- /dev/null +++ b/docs/rfc-command-framework-convergence.md @@ -0,0 +1,422 @@ +# RFC: Converging the DWS command frameworks onto one typed base + +**Status**: draft, seeking review +**Scope**: `internal/cmdcore`, `internal/helpers` (LeafSpec), `internal/shortcut` +**Related PR**: #830 (landed the shared base; supersedes #827 and #829) + +This document is self-contained: a reviewer without repository access should be +able to judge the design from it. + +--- + +## 1. Context + +`dws` (DingTalk Workspace CLI) is a Go/Cobra CLI whose commands are thin wrappers +that dispatch a JSON-RPC `tools/call` to DingTalk MCP servers. It targets both +humans and AI agents, so every command also publishes an Agent-visible "Runtime +Schema" (parameters, constraints, safety/confirmation semantics). + +### 1.1 Three command-definition systems (measured) + +| System | Count | Shape | Notes | +|---|---:|---|---| +| Handwritten `cobra.Command` | **1195** | `dws ` | 96% of commands; `internal/helpers/*.go`, 7.3万 lines | +| Shortcut | **376** | `dws +` | Declarative + **multi-step orchestration** | +| LeafSpec | **27** | `dws ` | Declarative, single-step; only `devapp` | + +Duplication measured inside the handwritten commands: `mustGetFlag` ×500, +`callMCPTool*` ×469, `flagOrFallback` ×306, required validation ×262, +`AttachRuntimeSchema`/`AnnotateRuntimeConstraints` ×165 each. + +A static AST census classified the 1195 handwritten commands by how they could +migrate: + +| Class | Count | Criterion | Disposition | +|---|---:|---|---| +| GROUP | 594 | no MCP call, no loop/IO (pure parent) | not migrated | +| **A** pure CRUD | **141** | exactly 1 MCP call, no loop/IO/confirm | 1:1 migratable | +| **B** confirm/fallback | **92** | 1 MCP call + confirm/dry-run/alias fallback | migratable | +| **C** multi-step / special IO | **368** | 0 or ≥2 MCP calls, loops, upload/poll | needs multi-step support | + +So 233/601 (38.8%) of commands with a body are migratable **today**; the 368 +C-class ones require the base to model multi-step orchestration. + +### 1.2 What already landed (PR #830) + +1. `LeafSpec` gained a declarative contract: `at_least_one` / `exactly_one` / + `mutually_exclusive` constraints, `bool` + `string-slice` flag kinds, and + `Risk`-driven write confirmation honoring `--dry-run` / `--yes` (including + root-injected globals, via a 3-level flag lookup). +2. `internal/cmdcore` — a dispatch-agnostic shared base extracted **verbatim** + from `leaf.go`: flag registration, the `explicit → alias → env → default` + value chain, required validation, constraint declaration checks + runtime + enforcement, Risk confirmation, `toolArgs` assembly, Runtime Schema + projection, help rendering. +3. `cmdcore.CommandSpec` + `cmdcore.NewCommand` — one typed definition and one + orchestration path; dispatch became a spec field. +4. `helpers.FromLeafSpec`; `NewLeafCommand` is now a one-line mapping. All + LeafSpec commands (incl. all 27 devapp leaves) flow through the unified spec. +5. `shortcut.FromShortcut` — a typed seam projecting a Shortcut's shared base + into a `CommandSpec`, deliberately **not** wired into the live `mount()`. + +### 1.3 Verification discipline (already enforced, non-negotiable) + +- **`check-generated-drift`**: regenerate the Agent metadata + command catalog + and byte-compare against the committed artifacts. For a refactor, a + byte-identical catalog (840 tools, identical hashes) is the equivalence proof + for the **build-time projection** — identity, flags, help, annotations. +- **Runtime semantics are NOT covered by drift.** Required validation, args + assembly, confirmation and dispatch order are evidenced only by unit tests. + (An earlier version of this work overstated drift as proof of runtime + behavior; that claim was corrected.) +- **100% changed-code coverage** gate. Important subtlety: CI runs + `go test -coverprofile` **without `-coverpkg`**, so each package is measured + only by its *own* tests. A new package whose logic is exercised only from a + consumer package will fail the gate (this actually happened: `cmdcore` + self-coverage was 31.5%, CI changed-code coverage 40.5%, plus an overall + regression — fixed by giving `cmdcore` its own exhaustive tests). +- **Interface compatibility**: the public Cobra command surface is diffed + against both the PR merge-base and the latest stable GA tag. + +--- + +## 2. Design review findings + +An adversarial review found **no BLOCKERs**: the Phase-1 extraction was +mechanically verified as a verbatim move, and the unified `BoolFlag` was proven +equivalent to the two readers it replaced. The following are the structural +issues that motivate this plan. (Items marked ✅ were already fixed in #830.) + +### Structural + +**F1. `Dispatch` is single-step biased; the type permits an unrunnable value.** +```go +Dispatch func(cmd *cobra.Command, args []string, toolArgs map[string]any) error +``` +This presumes "assemble `toolArgs` → make one call". A Shortcut's +`Execute(rt *RuntimeContext)` is arbitrary multi-step orchestration and **cannot +be expressed**. Consequently `FromShortcut` returned a spec that could not run. +✅ Partially mitigated in #830 by making `NewCommand` reject such a spec — but +that is a patch, not a model fix. **A type that can be constructed into a +permanently unusable state means the model is missing a dimension.** + +**F2. The base leaks the MCP payload shape.** +`BuildArgs → map[string]any` is the MCP `tools/call` argument shape, yet it lives +in a package documented as dispatch-agnostic. `devapp` dispatches through +`executor.Runner` and is still handed a `toolArgs` map. + +**F3. The same field name carries two semantics.** +- `Required`: cmdcore accepts *any effective value* (including registration + default and env fallback); Shortcut requires the flag to be `Changed`. +- Decline behavior: LeafSpec/cmdcore return a typed validation error (**exit ≠ + 0**); Shortcut's `mount()` returns `nil` (**exit 0**). + +These are semantic splits inside a "shared" base — documentation cannot fix them. + +**F4. The constraint model is incomplete and asymmetric with the Schema.** + +| Capability | Runtime Schema | `cmdcore.Constraint` | +|---|---|---| +| `mutually_exclusive` | ✅ | ✅ | +| `require_one_of` | ✅ | ✅ (`at_least_one` / `exactly_one`) | +| **`require_together`** | ✅ | ❌ | +| `enum` | ✅ (`AnnotateRuntimeFlagEnum`) | ❌ | + +Anything unrepresentable falls back to the imperative `Validate` hook, which +**loses the Schema projection and help rendering** — destroying the main value of +the design ("declare once, take effect in three places"). + +**F5. Too many hooks; `PostMount` is a back door.** +Extension points: `Validate` + `PostMount` + `RunE` + dispatch. `PostMount(cmd)` +can rewrite anything on the built Cobra command (args, annotations, flags), so +any declarative guarantee can be bypassed — which is precisely why drift became +the only safety net. Measured real usage is narrow: `devAppLeafMeta` ×8 (attach +schema identity) and `registerDevAppCursorFlags` ×4 (pagination flags). + +### Secondary + +**F6. `LeafSpec` is now vestigial** — 179 lines that are mostly comments and type +aliases; the only substantive own fields are the three dispatch fields +(`Server`/`Tool`/`Call`). It is a pure forwarding layer kept for 27 call sites. + +**F7. `FlagSpec` has 14 fields with three "two ways to do one thing" pairs**: +`Default` vs `ArgDefault`, `OmitEmpty` vs `Transform`-returning-nil, `Required` +vs `MarkRequired`. This accreted from per-command special cases rather than a +clean model. + +**F8. "Single registry" currently covers ~2%** of commands. The 368 C-class +handwritten commands are exactly the ones the current `CommandSpec` cannot +express, so F1 is a prerequisite for coverage, not an optional polish. + +**F9. The Schema still has two sources.** `AnnotateConstraints` projects only two +constraint kinds; `required` / `enum` / `require_together` still come from the +separately reviewed registry + hint files. The declarative spec is not yet the +single source of parameter truth. + +### What is worth preserving + +- **Declare once → runtime validation + Schema projection + `--help`** (the core + value). +- **Drift-as-equivalence-proof** — makes a 400+ command migration verifiable. +- **Type aliases for zero-churn migration** (`type LeafFlag = cmdcore.FlagSpec`). +- **Dispatch as a spec property** rather than a framework identity. + +--- + +## 3. Plan + +### Dependency graph +``` +S1 dispatch layering + Ctx ──┬─→ S3 constraints/enum ──┐ + │ ├─→ S8 Shortcut live-wire → S9 Schema single source +S2 semantics unification ────┘ │ + (needs a product decision) │ +S4 MCP shape push-down → S5 FlagSpec orthogonalization → S6 narrow PostMount → S7 delete LeafSpec +``` +S1/S2/S3 are **hard prerequisites** for S8. S4→S7 is an independent cleanup +chain that can proceed in parallel. + +--- + +### S1 — Dispatch layering + unified `Ctx` (biggest lever) + +Fixes **F1**; unblocks the 368 C-class commands and S8. + +```go +// cmdcore: framework-neutral execution context (knows nothing about MCP) +type Ctx struct { /* cmd, args, declared flags */ } +func (c *Ctx) Command() *cobra.Command +func (c *Ctx) Args() []string +func (c *Ctx) Str(name string) string // reuses alias → env → default chain +func (c *Ctx) Int(name string) int +func (c *Ctx) Bool(name string) bool +func (c *Ctx) StrSlice(name string) []string +func (c *Ctx) Changed(name string) bool +func (c *Ctx) DryRun() bool +func (c *Ctx) Yes() bool + +// CommandSpec: exactly one of the three, validated at CONSTRUCTION time +RunE func(cmd *cobra.Command, args []string) error // escape hatch +Invoke func(c *Ctx, toolArgs map[string]any) error // single-step +Orchestrate func(c *Ctx) error // multi-step +``` + +Notes: +- `Ctx` accessors honor the declared fallback chain, so a body reads exactly the + value the required/constraint checks saw. This is **stronger** than Shortcut's + `RuntimeContext`, which reads flags bare. +- **Layering rule**: MCP capability stays out of `cmdcore`. + `shortcut.RuntimeContext` keeps `CallMCPData`/`CallMCPWriteData`/`Output`; the + `FromShortcut` adapter builds it from `Ctx.Command()` inside `Orchestrate`. + (Embedding `*cmdcore.Ctx` into `RuntimeContext` is deferred to S8 to avoid + changing 376 shortcuts' flag-reading behavior in this step.) +- Construction-time `panic` replaces the run-time error added in #830, + consistent with the existing `ValidateConstraintDecls` philosophy: a spec with + no runnable body (or two competing ones) is a programming error that every + test and startup path trips immediately. + +**Gates**: drift zero; leaf suites green; three construction-panic cases; +`Orchestrate` confirmation/decline cases; `Ctx` accessor cases. +**Risk**: low (existing dispatch shifts to `Invoke`). +**Effort**: 1 PR (medium). *Status: in progress on the branch.* + +--- + +### S2 — Semantics unification (contains a **decision that needs sign-off**) + +Fixes **F3**. + +**S2a. `Required`** — make the divergence explicit instead of implicit: +```go +type RequiredMode int +const ( + RequiredAnyEffectiveValue RequiredMode = iota // = today's LeafSpec behavior (default) + RequiredMustBePassed // = today's Shortcut behavior (Changed) +) +``` +During migration both modes coexist; at S8 each shortcut flag maps to +`MustBePassed`, so behavior is unchanged. + +**S2b. Decline behavior — ⚠️ user-visible contract change.** +LeafSpec returns an error (exit ≠ 0); Shortcut returns `nil` (exit 0). Options: +1. Converge on **error** (declining a destructive operation should exit non-zero). + Changes the exit code of 376 shortcuts → needs product sign-off + CHANGELOG. +2. Carry a `DeclineBehavior` field through migration and converge later. +3. Converge on `nil` (weakens the atomic-command contract; not recommended). + +**S8 must not start before this decision is made.** + +**Gates**: cases for both modes; Shortcut's existing suites unchanged. +**Effort**: 1 PR (small) + 1 decision. + +--- + +### S3 — Complete the constraint model and add `enum` + +Fixes **F4**; restores "declare once → three effects". + +| Addition | Runtime validation | Schema projection | `--help` | +|---|---|---|---| +| `RequireTogether` constraint | ✅ | `RuntimeSchemaConstraints.RequireTogether` (exists) | ✅ | +| `FlagSpec.Enum` | ✅ | `cli.AnnotateRuntimeFlagEnum` (exists) | `(可选值: a, b)` | +| `Required` projection | — | `cli.AnnotateRuntimeRequiredFlags` (exists) | `(必填)` | + +All three annotation APIs already exist in `internal/cli`, so this is wiring, not +new Schema surface. + +**By-product**: the base can derive the `必填 / 可选值` help decoration itself, so +`FromShortcut` no longer needs to borrow Shortcut's `flagHelp` (removing a +coupling that caused a real bug — see §4). + +**Gates**: drift zero (**new annotations must appear only on commands that +declare them; all existing commands unchanged**) + runtime/help/schema cases for +each addition. +**Risk**: medium (touches Schema annotations; drift is the guard). +**Effort**: 1 PR (medium). + +--- + +### S4 — Push the MCP shape out of the base + +Fixes **F2**. Do it while only 27 call sites exist. + +```go +// base: CLI-facing contract +FlagSpec{ Name, Usage, Kind, Default, Aliases, EnvVar, Required, RequiredMode, Enum, Trim } +// MCP binding layer: payload shaping +ArgBinding{ Flag, Bind string; Transform func(string)(any,error); OmitEmpty bool; ArgDefault string } +``` +`BuildArgs` moves to the MCP binding layer (`helpers` or `cmdcore/mcpbind`). + +**Gates**: drift zero + per-key comparison of devapp's 27 commands' `toolArgs`. +**Effort**: 1 PR (medium-large; rewrites 27 call sites). + +--- + +### S5 — Orthogonalize `FlagSpec` + +Fixes **F7**. + +| Redundancy | Resolution | +|---|---| +| `Default` vs `ArgDefault` | S4 moves `ArgDefault` into `ArgBinding` — resolved naturally | +| `OmitEmpty` vs `Transform`-nil | keep `OmitEmpty` (declarative wins); document `Transform`-nil as the general escape | +| `Required` vs `MarkRequired` | merge into `Required` + `EnforceBy{Framework,Cobra}`; `Cobra` preserves the catalog `required` hard floor | + +**Gates**: drift zero (the `MarkRequired` → catalog `required` projection must be +byte-identical). +**Effort**: 1 PR (small-medium). + +--- + +### S6 — Narrow `PostMount` + +Fixes **F5**. Measured usage is only two kinds: +- `devAppLeafMeta(cmd, tool)` ×8 → named field `SchemaIdentity{Product, Tool, Source}` +- `registerDevAppCursorFlags(cmd)` ×4 → named `ExtraFlags []FlagSpec` / `Pagination` + +`PostMount` survives as an explicitly labelled escape hatch with a call-site +inventory (migration-era asset). + +**Gates**: drift zero. **Effort**: 1 PR (small). + +--- + +### S7 — Delete `LeafSpec` + +Fixes **F6**. Convert devapp's 27 sites to `CommandSpec` + an MCP dispatch +helper; delete `LeafSpec`, the type aliases, `FromLeafSpec`, `NewLeafCommand`. + +**Gates**: drift zero + interface-compatibility dual baseline (the command +surface must not change). +**Effort**: 1 PR (medium). + +--- + +### S8 — Live-wire Shortcut (prerequisites: S1 + S2 + S3) + +Migrate per service package (20 packages, 1 PR each), ordered +`usage` (smallest) → `todo`/`contact` → … → `chat`/`aitable` (largest). + +**Per-PR gates** +1. **Generalize the existing mount-equivalence test into a table over + `allShortcuts`**: for every registered shortcut, assert + `cmdcore.NewCommand(FromShortcut(s))` and `mount(s)` agree byte-for-byte on + flag set / types / usage / `Long` / annotations. ← the most important guard. +2. `shortcut list` output byte-identical (265 entries). +3. `dws schema --all` byte-identical. +4. drift zero + 100% changed-code coverage. + +**Still unmodeled before S8** (must be represented or explicitly rejected): +`Flag.Hidden` (no `CommandSpec` field), `Shortcut.Tips → Example`, +`ConstraintCustom`. + +**Effort**: ~20 PRs. + +--- + +### S9 — Schema single source (endgame) + +Fixes **F9**. Make the declarative spec the only source of parameter facts +(`required` / `enum` / `require_together` / constraints); `schema_hints` retains +only Agent selection prose (`agent_summary` / `use_when` / `avoid_when`). + +**Risk**: high (touches the Schema generation pipeline's division of +responsibility with the reviewed registry). **Effort**: separate project. + +--- + +## 4. Bugs this design review already caught (evidence the gates matter) + +Fixed in #830 after review: +- A spec with neither `RunE` nor `Dispatch` ran the whole pipeline — **including + the write-confirmation prompt** — then silently exited 0 having done nothing. +- `FromShortcut` **double-rendered** the `参数约束` help section (`shortcutLongHelp` + already appends it; `NewCommand` appended `ConstraintHelp` again) and **dropped** + the `必填 / 可选值` usage decoration. Both would have silently changed help and + catalog output at S8. +- A new **mount-equivalence test** (projected flag set/types/usage + rendered + `Long` vs live `mount(s)`) now catches this whole class; it is the seed of the + S8 gate. + +--- + +## 5. Handwritten commands (1195) + +- After **S1**, the 368 C-class commands become expressible for the first time + (via `Orchestrate`). +- The 233 A/B-class commands are migratable today, but should be scheduled + **after S4/S5**, otherwise they get migrated onto the old `FlagSpec` shape and + then rewritten again. +- The 594 GROUP nodes need no migration. + +--- + +## 6. Global discipline (per PR, non-negotiable) + +1. `check-generated-drift` zero drift = build-time projection equivalence proof. +2. Runtime semantics can only be proven by unit tests — drift cannot see them. +3. 100% changed-code coverage, and **a new package must have its own direct + tests** (never rely on cross-package `coverpkg`; this was a real CI failure). +4. Anything touching a user-visible contract (exit codes, flag names, help text) + requires a CHANGELOG entry and an explicit human decision. + +--- + +## 7. Questions for the reviewer + +1. **S2b**: should declining a risky operation exit non-zero (unify on error), or + should Shortcut's exit-0 behavior be preserved? This is the only user-visible + contract change in the plan. +2. **S1**: is `Invoke` / `Orchestrate` / `RunE` with a construction-time + "exactly one" rule the right factoring, or should dispatch be an interface + (e.g. `Dispatcher` with two implementations) instead of three nullable + function fields? +3. **S4**: is splitting `FlagSpec` (CLI-facing) from `ArgBinding` (payload-facing) + worth the churn on 27 call sites now, versus keeping one struct and accepting + the leak? +4. **S8 ordering**: is per-service migration with a byte-identity gate the right + risk posture for 376 shipped shortcuts, or is a parallel "shadow mode" + (build both, compare at run time) warranted? +5. Is **S9** worth attempting at all, given the reviewed-registry design + intentionally separates identity/safety review from code? diff --git a/internal/cmdcore/cmdcore.go b/internal/cmdcore/cmdcore.go index 482a02c3..0cfe1201 100644 --- a/internal/cmdcore/cmdcore.go +++ b/internal/cmdcore/cmdcore.go @@ -158,16 +158,20 @@ type Constraint struct { // CommandSpec is the single typed definition of a leaf command, shared by the // LeafSpec and (via FromShortcut) Shortcut frameworks. It carries the shared -// base (flags, constraints, risk) plus the orchestration hooks; dispatch is a -// property of the spec, not a separate framework: +// base (flags, constraints, risk) plus the orchestration hooks. Dispatch is a +// property of the spec, not a separate framework, and is layered so that both +// single-step and multi-step commands are expressible: // -// - RunE, when set, is the full escape hatch: the framework only registers -// flags/constraints/help and hands control to RunE. -// - otherwise Dispatch runs after required/constraint/Validate checks, args -// assembly, and the Risk write-confirmation, receiving the assembled -// toolArgs. The adapter (FromLeafSpec) supplies the concrete dispatch -// (single-step MCP call / executor.Runner / etc.); cmdcore stays -// dispatch-agnostic and never calls an MCP tool itself. +// - RunE — full escape hatch: the framework only registers flags/constraints/ +// help and hands control over. +// - Invoke — single-step: runs after required/constraint/Validate checks, args +// assembly and the Risk write-confirmation, receiving the assembled toolArgs. +// - Orchestrate — multi-step: runs after the same checks and confirmation but +// receives only the Ctx, so it can chain several backend calls itself. +// +// Exactly one of the three must be set; NewCommand validates this at +// construction time. cmdcore itself stays dispatch-agnostic and never calls a +// backend: the adapters (FromLeafSpec / FromShortcut) supply the concrete body. type CommandSpec struct { Use string Short string @@ -184,26 +188,102 @@ type CommandSpec struct { // PostMount adjusts the built command after flag registration and before // RunE is set (Args/DisableAutoGenTag/annotate/…); always runs. PostMount func(cmd *cobra.Command) - // RunE, when set, fully replaces the generated body (escape hatch). + // RunE fully replaces the generated body (escape hatch). RunE func(cmd *cobra.Command, args []string) error - // Dispatch executes the command with the assembled toolArgs. Used only when - // RunE is nil. - Dispatch func(cmd *cobra.Command, args []string, toolArgs map[string]any) error + // Invoke executes a single-step command with the assembled toolArgs. + Invoke func(c *Ctx, toolArgs map[string]any) error + // Orchestrate executes a multi-step command; it assembles whatever payloads + // it needs from the Ctx. + Orchestrate func(c *Ctx) error } +// Ctx is the framework-neutral execution context handed to Invoke/Orchestrate. +// It deliberately knows nothing about MCP or any other backend: it exposes the +// command, its positional args, and typed flag access that reuses the declared +// alias → env → default fallback chain, so a consumer reading a flag through Ctx +// gets exactly the value the required/constraint checks saw. +type Ctx struct { + cmd *cobra.Command + args []string + flags map[string]FlagSpec +} + +// newCtx builds the execution context for one command invocation. +func newCtx(cmd *cobra.Command, args []string, flags []FlagSpec) *Ctx { + byName := make(map[string]FlagSpec, len(flags)) + for _, flag := range flags { + byName[flag.Name] = flag + } + return &Ctx{cmd: cmd, args: args, flags: byName} +} + +// Command returns the running cobra command. +func (c *Ctx) Command() *cobra.Command { return c.cmd } + +// Args returns the positional arguments. +func (c *Ctx) Args() []string { return c.args } + +// Str returns a flag's effective string value (explicit → alias → env → +// default). An undeclared name yields "". +func (c *Ctx) Str(name string) string { + flag, ok := c.flags[name] + if !ok { + return "" + } + return EffectiveValue(c.cmd, flag) +} + +// Int returns a flag's effective integer value; an unparseable or undeclared +// value yields 0 (BuildArgs reports the precise parse error for Invoke specs). +func (c *Ctx) Int(name string) int { + flag, ok := c.flags[name] + if !ok { + return 0 + } + v, err := integerValue(c.cmd, flag) + if err != nil { + return 0 + } + return int(v) +} + +// Bool returns a boolean flag's value. +func (c *Ctx) Bool(name string) bool { + v, _ := c.cmd.Flags().GetBool(name) + return v +} + +// StrSlice returns a list flag's effective elements (trimmed, empties dropped). +func (c *Ctx) StrSlice(name string) []string { + flag, ok := c.flags[name] + if !ok { + return nil + } + return sliceValue(c.cmd, flag) +} + +// Changed reports whether the user explicitly passed the flag. +func (c *Ctx) Changed(name string) bool { return c.cmd.Flags().Changed(name) } + +// DryRun reports the effective global --dry-run. +func (c *Ctx) DryRun() bool { return BoolFlag(c.cmd, "dry-run") } + +// Yes reports the effective global --yes. +func (c *Ctx) Yes() bool { return BoolFlag(c.cmd, "yes") } + // NewCommand builds a cobra command from a CommandSpec. It is the single -// orchestration path: flag registration → constraint declaration checks → -// Runtime Schema projection → constraint help → PostMount → (RunE escape) → -// generated RunE{ required → constraints → Validate → BuildArgs → ConfirmRisk → -// Dispatch }. +// orchestration path: dispatch declaration check → flag registration → +// constraint declaration checks → Runtime Schema projection → constraint help → +// PostMount → (RunE escape) → generated RunE{ required → constraints → Validate +// → BuildArgs → ConfirmRisk → Invoke/Orchestrate }. // -// Behavior matches the former helpers.NewLeafCommand with one deliberate -// addition: that function always dispatched (Call → Server → callMCPTool), so a -// spec without a dispatcher was impossible. Here a spec declaring neither RunE -// nor Dispatch is a programming error and fails loudly, rather than running the -// whole pipeline — write-confirmation prompt included — and then silently -// exiting 0 having done nothing. +// Behavior matches the former helpers.NewLeafCommand, which always dispatched +// (Call → Server → callMCPTool) and therefore could not express a dispatcher-less +// spec. Here that is a programming error caught at construction time, so a +// malformed spec can never run the pipeline — write-confirmation prompt +// included — and then silently exit 0 having done nothing. func NewCommand(spec CommandSpec) *cobra.Command { + validateDispatchDecl(spec) cmd := &cobra.Command{ Use: spec.Use, Short: spec.Short, @@ -224,10 +304,6 @@ func NewCommand(spec CommandSpec) *cobra.Command { return cmd } cmd.RunE = func(cmd *cobra.Command, args []string) error { - if spec.Dispatch == nil { - return apperrors.NewInternal(fmt.Sprintf( - "command %q declares neither RunE nor Dispatch", spec.Use)) - } if err := ValidateRequired(cmd, spec.Flags); err != nil { return err } @@ -239,6 +315,13 @@ func NewCommand(spec CommandSpec) *cobra.Command { return err } } + ctx := newCtx(cmd, args, spec.Flags) + if spec.Orchestrate != nil { + if !ConfirmRisk(cmd, spec.Risk) { + return apperrors.NewValidation("用户取消了操作") + } + return spec.Orchestrate(ctx) + } toolArgs, err := BuildArgs(cmd, spec.Flags) if err != nil { return err @@ -246,11 +329,33 @@ func NewCommand(spec CommandSpec) *cobra.Command { if !ConfirmRisk(cmd, spec.Risk) { return apperrors.NewValidation("用户取消了操作") } - return spec.Dispatch(cmd, args, toolArgs) + return spec.Invoke(ctx, toolArgs) } return cmd } +// validateDispatchDecl enforces "exactly one dispatcher" at build time. Like +// ValidateConstraintDecls this panics: a spec with no runnable body (or with two +// competing ones) is a programming error that every test and startup path should +// trip immediately, not a condition to surface at user run time. +func validateDispatchDecl(spec CommandSpec) { + declared := 0 + if spec.RunE != nil { + declared++ + } + if spec.Invoke != nil { + declared++ + } + if spec.Orchestrate != nil { + declared++ + } + if declared != 1 { + panic(fmt.Sprintf( + "command %q must declare exactly one of RunE/Invoke/Orchestrate, got %d", + spec.Use, declared)) + } +} + // RegisterFlags registers every flag (plus hidden aliases and MarkFlagRequired) // declared by the spec set onto cmd. func RegisterFlags(cmd *cobra.Command, flags []FlagSpec) { diff --git a/internal/cmdcore/cmdcore_test.go b/internal/cmdcore/cmdcore_test.go index 9a001caf..101f1b63 100644 --- a/internal/cmdcore/cmdcore_test.go +++ b/internal/cmdcore/cmdcore_test.go @@ -693,7 +693,7 @@ func TestCrossPlatformCoverageNewCommandOrchestration(t *testing.T) { t.Error("PostMount must run before RunE is assigned") } }, - Dispatch: func(_ *cobra.Command, _ []string, toolArgs map[string]any) error { + Invoke: func(_ *Ctx, toolArgs map[string]any) error { order = append(order, "dispatch") gotArgs = toolArgs return nil @@ -727,26 +727,25 @@ func TestCrossPlatformCoverageNewCommandOrchestration(t *testing.T) { } func TestCrossPlatformCoverageNewCommandRunEEscapeHatch(t *testing.T) { + // RunE bypasses the whole generated pipeline: the declared Required flag is + // left unset, so had the framework body run it would have failed validation. ran := false - dispatched := false cmd := NewCommand(CommandSpec{ Use: "escape", - Flags: []FlagSpec{{Name: "x", Usage: "X"}}, + Flags: []FlagSpec{{Name: "x", Usage: "X", Required: true}}, RunE: func(*cobra.Command, []string) error { ran = true return nil }, - Dispatch: func(*cobra.Command, []string, map[string]any) error { - dispatched = true - return nil - }, }) + cmd.SilenceErrors = true + cmd.SilenceUsage = true cmd.SetArgs(nil) if err := cmd.Execute(); err != nil { - t.Fatal(err) + t.Fatalf("RunE escape hatch must bypass validation, got %v", err) } - if !ran || dispatched { - t.Fatalf("RunE escape hatch must win: ran=%v dispatched=%v", ran, dispatched) + if !ran { + t.Fatal("RunE escape hatch did not run") } } @@ -764,7 +763,7 @@ func TestCrossPlatformCoverageNewCommandStopsOnFailures(t *testing.T) { validated = true return nil }, - Dispatch: func(*cobra.Command, []string, map[string]any) error { + Invoke: func(*Ctx, map[string]any) error { dispatched = true return nil }, @@ -796,7 +795,7 @@ func TestCrossPlatformCoverageNewCommandStopsOnFailures(t *testing.T) { Use: "hook", Flags: []FlagSpec{{Name: "x", Usage: "X"}}, Validate: func(*cobra.Command, []string) error { return boom }, - Dispatch: func(*cobra.Command, []string, map[string]any) error { + Invoke: func(*Ctx, map[string]any) error { dispatched = true return nil }, @@ -814,7 +813,7 @@ func TestCrossPlatformCoverageNewCommandStopsOnFailures(t *testing.T) { cmd = NewCommand(CommandSpec{ Use: "args", Flags: []FlagSpec{{Name: "y", Usage: "Y", Transform: func(string) (any, error) { return nil, boom }}}, - Dispatch: func(*cobra.Command, []string, map[string]any) error { + Invoke: func(*Ctx, map[string]any) error { dispatched = true return nil }, @@ -834,7 +833,7 @@ func TestCrossPlatformCoverageNewCommandDeclineCancels(t *testing.T) { Use: "risky", Risk: RiskHighWrite, Flags: []FlagSpec{{Name: "x", Usage: "X"}}, - Dispatch: func(*cobra.Command, []string, map[string]any) error { + Invoke: func(*Ctx, map[string]any) error { dispatched = true return nil }, @@ -854,20 +853,151 @@ func TestCrossPlatformCoverageNewCommandDeclineCancels(t *testing.T) { } } -func TestCrossPlatformCoverageNewCommandRejectsMissingDispatch(t *testing.T) { - // Neither RunE nor Dispatch is a programming error: it must fail loudly - // rather than run the pipeline (prompt included) and silently exit 0. +func TestCrossPlatformCoverageNewCommandRequiresExactlyOneDispatcher(t *testing.T) { + mustPanic := func(name string, spec CommandSpec, needle string) { + t.Helper() + defer func() { + r := recover() + if r == nil { + t.Fatalf("%s: expected panic", name) + } + if msg, _ := r.(string); !strings.Contains(msg, needle) { + t.Fatalf("%s: panic = %v, want %q", name, r, needle) + } + }() + NewCommand(spec) + } + flags := []FlagSpec{{Name: "x", Usage: "X"}} + + // Zero dispatchers: a spec with no runnable body must never reach run time, + // where it would have prompted for confirmation and then exited 0 doing nothing. + mustPanic("no dispatcher", CommandSpec{Use: "bare", Risk: RiskHighWrite, Flags: flags}, + "must declare exactly one of RunE/Invoke/Orchestrate, got 0") + + // Two competing dispatchers are equally a programming error. + mustPanic("two dispatchers", CommandSpec{ + Use: "both", + Flags: flags, + Invoke: func(*Ctx, map[string]any) error { return nil }, + Orchestrate: func(*Ctx) error { return nil }, + }, "got 2") + mustPanic("runE plus invoke", CommandSpec{ + Use: "both2", + Flags: flags, + RunE: func(*cobra.Command, []string) error { return nil }, + Invoke: func(*Ctx, map[string]any) error { return nil }, + }, "got 2") +} + +func TestCrossPlatformCoverageNewCommandOrchestrateDispatch(t *testing.T) { + var seen struct { + str string + n int + flag bool + slice []string + args []string + dry bool + yes bool + chg bool + } + t.Setenv("DWS_CMDCORE_ORCH_ENV", "from-env") + t.Setenv("DWS_CMDCORE_ORCH_BADINT", "not-a-number") cmd := NewCommand(CommandSpec{ - Use: "bare", - Risk: RiskHighWrite, - Flags: []FlagSpec{{Name: "x", Usage: "X"}}, + Use: "orch", + Flags: []FlagSpec{ + {Name: "name", Usage: "N", Aliases: []string{"n-alias"}}, + {Name: "env", Usage: "E", EnvVar: "DWS_CMDCORE_ORCH_ENV"}, + {Name: "count", Usage: "C", Kind: KindInt}, + {Name: "bad", Usage: "B", Kind: KindInt, EnvVar: "DWS_CMDCORE_ORCH_BADINT"}, + {Name: "on", Usage: "O", Kind: KindBool}, + {Name: "ids", Usage: "I", Kind: KindStringSlice}, + }, + Orchestrate: func(c *Ctx) error { + // Ctx accessors must honor the declared alias/env fallback chain. + seen.str = c.Str("name") + seen.n = c.Int("count") + seen.flag = c.Bool("on") + seen.slice = c.StrSlice("ids") + seen.args = c.Args() + seen.dry = c.DryRun() + seen.yes = c.Yes() + seen.chg = c.Changed("count") + if got := c.Str("env"); got != "from-env" { + t.Errorf("Ctx.Str(env) = %q, want env fallback", got) + } + if c.Command() == nil || c.Command().Use != "orch" { + t.Errorf("Ctx.Command() = %v", c.Command()) + } + // Undeclared names resolve to zero values rather than panicking. + if c.Str("nope") != "" || c.Int("nope") != 0 || c.StrSlice("nope") != nil { + t.Error("undeclared flag must yield zero values") + } + // An unparseable effective value degrades to 0 instead of erroring. + if got := c.Int("bad"); got != 0 { + t.Errorf("Ctx.Int(bad) = %d, want 0", got) + } + return nil + }, }) + cmd.Args = cobra.ArbitraryArgs + cmd.PersistentFlags().Bool("dry-run", false, "") + cmd.PersistentFlags().Bool("yes", false, "") cmd.SilenceErrors = true cmd.SilenceUsage = true - // No stdin: if the pipeline reached ConfirmRisk it would block/consume input. - cmd.SetArgs([]string{"--x", "v"}) - err := cmd.Execute() - if err == nil || !strings.Contains(err.Error(), "declares neither RunE nor Dispatch") { - t.Fatalf("missing-dispatch err = %v, want typed internal error", err) + cmd.SetArgs([]string{"--n-alias", "via-alias", "--count", "4", "--on", "--ids", " a , b ", "--yes", "pos1"}) + if err := cmd.Execute(); err != nil { + t.Fatal(err) + } + if seen.str != "via-alias" { + t.Fatalf("Ctx.Str(name) = %q, want alias value", seen.str) + } + if seen.n != 4 || !seen.flag || !seen.chg { + t.Fatalf("typed accessors = %+v", seen) + } + if !reflect.DeepEqual(seen.slice, []string{"a", "b"}) { + t.Fatalf("Ctx.StrSlice = %#v", seen.slice) + } + if !reflect.DeepEqual(seen.args, []string{"pos1"}) { + t.Fatalf("Ctx.Args = %#v", seen.args) + } + if seen.dry || !seen.yes { + t.Fatalf("global flags: dry=%v yes=%v", seen.dry, seen.yes) + } +} + +func TestCrossPlatformCoverageNewCommandOrchestrateHonorsConfirmation(t *testing.T) { + ran := false + build := func() *cobra.Command { + cmd := NewCommand(CommandSpec{ + Use: "risky-orch", + Risk: RiskHighWrite, + Flags: []FlagSpec{{Name: "x", Usage: "X"}}, + Orchestrate: func(*Ctx) error { ran = true; return nil }, + }) + cmd.PersistentFlags().Bool("dry-run", false, "") + cmd.PersistentFlags().Bool("yes", false, "") + cmd.SilenceErrors = true + cmd.SilenceUsage = true + cmd.SetErr(&strings.Builder{}) + return cmd + } + // Declining must abort before the orchestration body runs. + declined := build() + declined.SetIn(strings.NewReader("no\n")) + declined.SetArgs([]string{"--x", "v"}) + if err := declined.Execute(); err == nil || !strings.Contains(err.Error(), "用户取消了操作") { + t.Fatalf("declined orchestrate err = %v", err) + } + if ran { + t.Fatal("declined orchestrate must not run the body") + } + // --yes bypasses the prompt and runs it. + confirmed := build() + confirmed.SetArgs([]string{"--x", "v", "--yes"}) + if err := confirmed.Execute(); err != nil { + t.Fatal(err) + } + if !ran { + t.Fatal("confirmed orchestrate did not run") } } diff --git a/internal/helpers/leaf.go b/internal/helpers/leaf.go index 125a46a2..982f9b93 100644 --- a/internal/helpers/leaf.go +++ b/internal/helpers/leaf.go @@ -165,9 +165,9 @@ func FromLeafSpec(spec LeafSpec) cmdcore.CommandSpec { RunE: spec.RunE, } if spec.RunE == nil { - cs.Dispatch = func(cmd *cobra.Command, _ []string, toolArgs map[string]any) error { + cs.Invoke = func(c *cmdcore.Ctx, toolArgs map[string]any) error { if spec.Call != nil { - return spec.Call(cmd, spec.Tool, toolArgs) + return spec.Call(c.Command(), spec.Tool, toolArgs) } if spec.Server != "" { return callMCPToolOnServer(spec.Server, spec.Tool, toolArgs) diff --git a/internal/shortcut/adapter.go b/internal/shortcut/adapter.go index 2e3a0a68..5f3e377d 100644 --- a/internal/shortcut/adapter.go +++ b/internal/shortcut/adapter.go @@ -14,8 +14,11 @@ package shortcut import ( + "fmt" "strings" + apperrors "github.com/DingTalk-Real-AI/dingtalk-workspace-cli/internal/errors" + "github.com/DingTalk-Real-AI/dingtalk-workspace-cli/internal/cmdcore" ) @@ -31,10 +34,10 @@ import ( // are therefore dropped by this projection — Phase 3 must extend CommandSpec (or // reject such specs) before wiring it in: // -// - multi-step orchestration: Shortcut.Execute(rt) uses RuntimeContext with -// CallMCPData/CallMCPWriteData; cmdcore's single-step Dispatch(toolArgs) -// cannot express it. FromShortcut therefore leaves Dispatch/RunE nil, and -// cmdcore.NewCommand rejects such a spec at run time rather than no-oping. +// - multi-step orchestration is now expressible: Shortcut.Execute(rt) is +// wrapped into CommandSpec.Orchestrate, so the projection is runnable. The +// wrapper still builds the shortcut's own RuntimeContext (which owns the MCP +// calls), because cmdcore stays backend-agnostic. // - decline behavior: mount() returns nil when the user declines a risky // shortcut, whereas cmdcore.NewCommand returns a typed validation error. // - Required semantics: mount() requires the flag to be Changed (message @@ -65,6 +68,16 @@ func FromShortcut(s Shortcut) cmdcore.CommandSpec { Flags: fromShortcutFlags(s.Flags), Constraints: fromShortcutConstraints(s.Constraints), Risk: cmdcore.Risk(s.risk()), + // Multi-step body: cmdcore stays backend-agnostic, so the shortcut's own + // RuntimeContext — which owns CallMCPData/CallMCPWriteData/Output — is + // built here from the Ctx's command. + Orchestrate: func(c *cmdcore.Ctx) error { + if s.Execute == nil { + return apperrors.NewInternal(fmt.Sprintf( + "shortcut %s %s 未实现 Execute", s.Service, s.Command)) + } + return s.Execute(&RuntimeContext{cmd: c.Command(), shortcut: s}) + }, } } diff --git a/internal/shortcut/adapter_test.go b/internal/shortcut/adapter_test.go index a3d6b126..31ca0258 100644 --- a/internal/shortcut/adapter_test.go +++ b/internal/shortcut/adapter_test.go @@ -61,8 +61,11 @@ func TestCrossPlatformCoverageFromShortcutMapsSharedBase(t *testing.T) { if cs.Risk != cmdcore.RiskHighWrite { t.Fatalf("risk = %q, want high-risk-write", cs.Risk) } - if cs.Dispatch != nil || cs.RunE != nil { - t.Fatal("dispatch/RunE must stay nil (multi-step Execute not modeled)") + if cs.Orchestrate == nil { + t.Fatal("multi-step Execute must project into Orchestrate") + } + if cs.Invoke != nil || cs.RunE != nil { + t.Fatal("only Orchestrate may be set for a shortcut projection") } if len(cs.Flags) != 5 {