From ceca98c5733ca2b06ba3e1fa7cd1f8316a24985a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E7=8E=89=E6=BE=9C?= Date: Thu, 30 Jul 2026 10:30:05 +0800 Subject: [PATCH] refactor(cmdcore): layer dispatch into Invoke/Orchestrate behind a shared Ctx The single Dispatch hook assumed every command is one MCP call, so the Shortcut projection could only ever describe a command, never run it. Split dispatch into Invoke (assembled toolArgs) and Orchestrate (multi-step), keep RunE as the escape hatch, and reject specs that declare anything other than exactly one at construction time. Ctx gives both hooks the same typed flag accessors so orchestration no longer reaches for framework-specific plumbing. Catalog output stays byte-identical. --- docs/rfc-command-framework-convergence.md | 422 ++++++++++++++++++++++ internal/cmdcore/cmdcore.go | 161 +++++++-- internal/cmdcore/cmdcore_test.go | 180 +++++++-- internal/helpers/leaf.go | 4 +- internal/shortcut/adapter.go | 21 +- internal/shortcut/adapter_test.go | 7 +- 6 files changed, 734 insertions(+), 61 deletions(-) create mode 100644 docs/rfc-command-framework-convergence.md 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 {