From d3087d170bc7e8c38cddeec04451b0bf29ced39d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E4=BF=AE=E9=9B=A8?= Date: Mon, 29 Jun 2026 10:19:45 +0800 Subject: [PATCH] feat(compat): native @file / --contents-file input for structured JSON flags `dws report entry submit --contents-file ` (and `--contents -` stdin) silently submitted `contents:[null]`. Root cause: the `--contents-file` flag had no transform, so its value mapped to an unused param while `--contents` stayed empty. - file_read_json transform + a build-time report hook resolve --contents-file / --contents - / @file natively in Go (priority: file > stdin > inline) and declare a contents / contents-file one-of group so a file-only invocation is no longer rejected at parse time. - generalizes to @file / @- input for any structured JSON-array flag. --- internal/compat/report_hooks.go | 186 +++++++++++++++++++++++++ internal/compat/report_hooks_test.go | 198 +++++++++++++++++++++++++++ internal/compat/transform.go | 47 ++++++- internal/compat/transform_test.go | 59 ++++++++ 4 files changed, 488 insertions(+), 2 deletions(-) create mode 100644 internal/compat/report_hooks.go create mode 100644 internal/compat/report_hooks_test.go diff --git a/internal/compat/report_hooks.go b/internal/compat/report_hooks.go new file mode 100644 index 00000000..3db46ffc --- /dev/null +++ b/internal/compat/report_hooks.go @@ -0,0 +1,186 @@ +// Copyright 2026 Alibaba Group +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +// report_hooks.go — CLI-side input resolution for the `report` product. +// +// The envelope publishes `report entry submit` (MCP tool create_report) with a +// `--contents` flag (json_parse, required) and a sibling `--contents-file` +// flag (omitWhen empty, no transform/mapsTo). On its own, `--contents-file` +// therefore goes nowhere: its value maps to the unused `contentsFile` param and +// the real `contents` param stays empty, so a `--contents-file`-only (or +// `--contents -` stdin) submit silently sends `contents: [null]` and the report +// fails. The literal-only `--contents` path works, which is why +// `report create` (the helper, inline-only) succeeds while +// `report entry submit --contents-file` does not. +// +// The wukong reference implementation reads the file/stdin natively inside its +// hand-written cobra RunE (dws-wukong/wukong/products/report.go +// resolveReportContentsFromFlags, priority: --contents-file > --contents - +// (stdin) > --contents ''). The open-source CLI is envelope-driven, so we +// attach the equivalent native resolution as a build-time hook here, mirroring +// AttachReportListReadableEnrichment (which layers wukong-equivalent list +// enrichment onto the same envelope leaves). No discovery-config change is +// needed: the hook populates the real `--contents` flag before the envelope's +// json_parse transform runs, and the broken `contentsFile` override is left +// inert. +// +// Two build-time adjustments make `--contents-file`-only valid: +// +// 1. The envelope marks `--contents` individually required (cobra +// MarkFlagRequired, enforced at parse time, before PreRunE). We clear that +// annotation and instead declare a `contents` / `contents-file` one-of +// group (MarkFlagsOneRequired, validated by ValidateFlagGroups — also +// before PreRunE, but satisfied when either flag is set). Supplying +// neither still errors, now naming both flags. +// 2. A chained PreRunE resolves the chosen source into `--contents` so the +// downstream json_parse transform sees inline JSON regardless of origin. + +package compat + +import ( + "fmt" + "io" + "os" + "strings" + "unicode/utf8" + + apperrors "github.com/DingTalk-Real-AI/dingtalk-workspace-cli/internal/errors" + "github.com/spf13/cobra" +) + +// reportContentsMaxBytes caps the contents payload at 10MB, matching the +// wukong upstream limit (dws-wukong/wukong/products/report.go +// reportContentsMaxBytes). Oversized input is rejected rather than truncated. +const reportContentsMaxBytes = 10 * 1024 * 1024 + +// reportToolsWithContentsFile lists every report toolName whose `--contents` / +// `--contents-file` pair needs native file/stdin resolution. Today only +// create_report (the `report entry submit` leaf) carries the pair. +var reportToolsWithContentsFile = map[string]bool{ + "create_report": true, +} + +// installReportHook wires report-specific input resolution onto leaf commands +// emitted by BuildDynamicCommands. It is a no-op for non-report products and +// for report tools that do not expose the contents/contents-file pair. +// +// The hook chain preserves the cmd.PreRunE that NewDirectCommand already +// installed (currently validateRequireTogether) by invoking it first. +func installReportHook(cmd *cobra.Command, canonicalProduct, toolName string) { + if cmd == nil { + return + } + if strings.TrimSpace(canonicalProduct) != "report" { + return + } + if !reportToolsWithContentsFile[toolName] { + return + } + contents := cmd.Flags().Lookup("contents") + file := cmd.Flags().Lookup("contents-file") + if contents == nil || file == nil { + // Envelope shape changed (renamed/removed flags) — do not block the + // command; leave whatever the envelope declared untouched. + return + } + + // (1) Relax the individually-required `--contents` into a one-of group so + // `--contents-file`-only (or `--contents -`) is accepted. Clearing the + // required annotation must happen before parse-time ValidateRequiredFlags; + // this hook runs at build time, so it does. + if contents.Annotations != nil { + delete(contents.Annotations, cobra.BashCompOneRequiredFlag) + } + cmd.MarkFlagsOneRequired("contents", "contents-file") + + // (2) Resolve the chosen source into --contents before the RunE transform. + original := cmd.PreRunE + cmd.PreRunE = func(c *cobra.Command, args []string) error { + if original != nil { + if err := original(c, args); err != nil { + return err + } + } + return resolveReportContents(c) + } +} + +// resolveReportContents applies the wukong source priority — `--contents-file` +// (file) > `--contents -` (stdin) > `--contents ''` (literal) — and +// writes the resolved JSON string back into the `--contents` flag so the +// downstream json_parse transform decodes it uniformly. When a file or stdin +// source is used, `--contents-file` is cleared so the envelope's omitWhen:empty +// drops the now-redundant param. +func resolveReportContents(cmd *cobra.Command) error { + filePath, _ := cmd.Flags().GetString("contents-file") + if strings.TrimSpace(filePath) != "" { + data, err := readReportContentsFile(filePath) + if err != nil { + return err + } + if err := cmd.Flags().Set("contents", data); err != nil { + return apperrors.NewInternal("failed to set --contents from --contents-file") + } + _ = cmd.Flags().Set("contents-file", "") + return nil + } + + raw, _ := cmd.Flags().GetString("contents") + if strings.TrimSpace(raw) == "-" { + data, err := readReportContentsLimited(cmd.InOrStdin(), "--contents -") + if err != nil { + return err + } + if err := cmd.Flags().Set("contents", data); err != nil { + return apperrors.NewInternal("failed to set --contents from stdin") + } + } + return nil +} + +// readReportContentsFile opens a file path and reads its contents under the +// 10MB cap and UTF-8 check. Error wording mirrors wukong so agents and humans +// see a stable message across both editions. +func readReportContentsFile(path string) (string, error) { + file, err := os.Open(path) + if err != nil { + if os.IsNotExist(err) { + return "", apperrors.NewValidation( + fmt.Sprintf("--contents-file: file not found: %s", path), + apperrors.WithHint("确认路径存在，且指向一个 JSON 文件"), + ) + } + return "", apperrors.NewValidation(fmt.Sprintf("--contents-file: cannot read %s: %v", path, err)) + } + defer file.Close() + return readReportContentsLimited(file, fmt.Sprintf("--contents-file %s", path)) +} + +// readReportContentsLimited reads from r enforcing the 10MB cap and UTF-8 +// validity. A LimitReader at cap+1 detects overflow without reading unbounded. +func readReportContentsLimited(r io.Reader, source string) (string, error) { + data, err := io.ReadAll(io.LimitReader(r, int64(reportContentsMaxBytes)+1)) + if err != nil { + return "", apperrors.NewValidation(fmt.Sprintf("%s: read failed: %v", source, err)) + } + if len(data) > reportContentsMaxBytes { + return "", apperrors.NewValidation( + fmt.Sprintf("%s: contents exceed maximum size of 10MB", source), + apperrors.WithHint("精简内容或拆分为多份日志提交"), + ) + } + if !utf8.Valid(data) { + return "", apperrors.NewValidation(fmt.Sprintf("%s: not valid UTF-8", source)) + } + return string(data), nil +} diff --git a/internal/compat/report_hooks_test.go b/internal/compat/report_hooks_test.go new file mode 100644 index 00000000..43335362 --- /dev/null +++ b/internal/compat/report_hooks_test.go @@ -0,0 +1,198 @@ +// Copyright 2026 Alibaba Group +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package compat + +import ( + "errors" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/spf13/cobra" +) + +// newReportSubmitStub mirrors the leaf command shape emitted by +// BuildDynamicCommands for `report entry submit` (envelope: create_report). +// Only the flags the hook touches are registered. --contents is marked +// required to reproduce the envelope's MarkFlagRequired so the relaxation +// behaviour can be asserted. +func newReportSubmitStub() *cobra.Command { + cmd := &cobra.Command{Use: "submit", RunE: func(*cobra.Command, []string) error { return nil }} + cmd.Flags().String("contents", "", "contents JSON array") + cmd.Flags().String("contents-file", "", "contents JSON file") + cmd.Flags().String("template-id", "", "template id") + _ = cmd.MarkFlagRequired("contents") + return cmd +} + +const reportContentsPayload = `[{"key":"今日完成工作","sort":"0","content":"done","contentType":"markdown","type":"1"}]` + +func TestResolveReportContents_FromFile(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "contents.json") + if err := os.WriteFile(path, []byte(reportContentsPayload), 0o600); err != nil { + t.Fatalf("write temp file: %v", err) + } + + cmd := newReportSubmitStub() + if err := cmd.Flags().Set("contents-file", path); err != nil { + t.Fatal(err) + } + if err := resolveReportContents(cmd); err != nil { + t.Fatalf("resolveReportContents(file): %v", err) + } + got, _ := cmd.Flags().GetString("contents") + if got != reportContentsPayload { + t.Fatalf("--contents not populated from file: %q", got) + } + // contents-file must be cleared so omitWhen:empty drops the dead param. + if cf, _ := cmd.Flags().GetString("contents-file"); cf != "" { + t.Fatalf("--contents-file should be cleared after resolution, got %q", cf) + } +} + +func TestResolveReportContents_FromStdin(t *testing.T) { + cmd := newReportSubmitStub() + if err := cmd.Flags().Set("contents", "-"); err != nil { + t.Fatal(err) + } + cmd.SetIn(strings.NewReader(reportContentsPayload)) + if err := resolveReportContents(cmd); err != nil { + t.Fatalf("resolveReportContents(stdin): %v", err) + } + got, _ := cmd.Flags().GetString("contents") + if got != reportContentsPayload { + t.Fatalf("--contents not populated from stdin: %q", got) + } +} + +func TestResolveReportContents_InlineUntouched(t *testing.T) { + cmd := newReportSubmitStub() + if err := cmd.Flags().Set("contents", reportContentsPayload); err != nil { + t.Fatal(err) + } + if err := resolveReportContents(cmd); err != nil { + t.Fatalf("resolveReportContents(inline): %v", err) + } + got, _ := cmd.Flags().GetString("contents") + if got != reportContentsPayload { + t.Fatalf("inline --contents must be left untouched, got %q", got) + } +} + +func TestResolveReportContents_FilePriorityOverInline(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "contents.json") + if err := os.WriteFile(path, []byte(reportContentsPayload), 0o600); err != nil { + t.Fatal(err) + } + cmd := newReportSubmitStub() + if err := cmd.Flags().Set("contents", `[{"stale":"inline"}]`); err != nil { + t.Fatal(err) + } + if err := cmd.Flags().Set("contents-file", path); err != nil { + t.Fatal(err) + } + if err := resolveReportContents(cmd); err != nil { + t.Fatalf("resolveReportContents: %v", err) + } + got, _ := cmd.Flags().GetString("contents") + if got != reportContentsPayload { + t.Fatalf("--contents-file must win over inline --contents, got %q", got) + } +} + +func TestResolveReportContents_MissingFileErrors(t *testing.T) { + cmd := newReportSubmitStub() + if err := cmd.Flags().Set("contents-file", filepath.Join(t.TempDir(), "nope.json")); err != nil { + t.Fatal(err) + } + err := resolveReportContents(cmd) + if err == nil { + t.Fatal("expected error for missing --contents-file path") + } + if !strings.Contains(err.Error(), "file not found") { + t.Fatalf("unexpected error: %v", err) + } +} + +// ── installReportHook composition ────────────────────────────── + +func TestInstallReportHook_RelaxesRequiredToOneOf(t *testing.T) { + cmd := newReportSubmitStub() + // Before the hook, --contents carries the cobra required annotation. + if cmd.Flags().Lookup("contents").Annotations[cobra.BashCompOneRequiredFlag] == nil { + t.Fatal("precondition: --contents should start out required") + } + installReportHook(cmd, "report", "create_report") + // After the hook, the individual required annotation must be cleared so a + // --contents-file-only invocation is not rejected at parse time. + if cmd.Flags().Lookup("contents").Annotations[cobra.BashCompOneRequiredFlag] != nil { + t.Fatal("installReportHook should clear the individual required on --contents") + } + // And a PreRunE must now be installed to resolve the source. + if cmd.PreRunE == nil { + t.Fatal("installReportHook should install a PreRunE") + } +} + +func TestInstallReportHook_NoOpForOtherProduct(t *testing.T) { + cmd := newReportSubmitStub() + installReportHook(cmd, "chat", "create_report") + if cmd.Flags().Lookup("contents").Annotations[cobra.BashCompOneRequiredFlag] == nil { + t.Fatal("non-report product must not touch required annotation") + } + if cmd.PreRunE != nil { + t.Fatal("non-report product must not install a PreRunE") + } +} + +func TestInstallReportHook_NoOpForOtherReportTool(t *testing.T) { + cmd := newReportSubmitStub() + installReportHook(cmd, "report", "get_received_report_list") + if cmd.PreRunE != nil { + t.Fatal("non-target report tool must not install a PreRunE") + } +} + +func TestInstallReportHook_ChainsExistingPreRunE(t *testing.T) { + cmd := newReportSubmitStub() + originalCalled := false + cmd.PreRunE = func(*cobra.Command, []string) error { originalCalled = true; return nil } + installReportHook(cmd, "report", "create_report") + if err := cmd.Flags().Set("contents", reportContentsPayload); err != nil { + t.Fatal(err) + } + if err := cmd.PreRunE(cmd, nil); err != nil { + t.Fatalf("unexpected err: %v", err) + } + if !originalCalled { + t.Fatal("original PreRunE was dropped") + } +} + +func TestInstallReportHook_BailsIfChainedPreRunEFails(t *testing.T) { + cmd := newReportSubmitStub() + cmd.PreRunE = func(*cobra.Command, []string) error { return errors.New("original boom") } + installReportHook(cmd, "report", "create_report") + err := cmd.PreRunE(cmd, nil) + if err == nil || !strings.Contains(err.Error(), "original boom") { + t.Fatalf("expected original PreRunE error to bubble, got %v", err) + } +} + +func TestInstallReportHook_NilCmdSafe(t *testing.T) { + installReportHook(nil, "report", "create_report") +} diff --git a/internal/compat/transform.go b/internal/compat/transform.go index 0b893d73..c700ba87 100644 --- a/internal/compat/transform.go +++ b/internal/compat/transform.go @@ -243,6 +243,18 @@ func transformJSONParse(value any) (any, error) { if s == "" { return value, nil } + // @file / @- expansion — read the JSON/YAML payload from a file or stdin + // before parsing. A leading "@" is an unambiguous file sentinel because a + // JSON/YAML value never starts with "@"; this is what the error hint below + // promises and lets long/complex payloads (many records, big cell ranges) + // avoid shell-quoting hell. + s, err := resolveJSONSource(s) + if err != nil { + return nil, err + } + if s == "" { + return value, nil + } // Strict JSON first — fast path and unambiguous type promotion (numbers // stay numbers, etc.). var parsed any @@ -257,10 +269,32 @@ func transformJSONParse(value any) (any, error) { return nil, apperrors.NewValidation( "json_parse: input is not valid JSON or YAML; " + "quote the whole value and use `[{key: value, ...}]` for ad-hoc input, " + - "or pass `@path/to/file.json` to read from a file", + "or pass `@path/to/file.json` (or `@-` for stdin) to read from a file", ) } +// resolveJSONSource expands an @file / @- reference used by the json_parse +// transforms. A leading "@" is the file sentinel: "@-" reads stdin, "@" +// reads the file (UTF-8, via transformFileRead). Any value not starting with +// "@" is returned unchanged. JSON/YAML payloads never start with "@", so this +// is unambiguous for structured flags. +func resolveJSONSource(s string) (string, error) { + if !strings.HasPrefix(s, "@") { + return s, nil + } + ref := strings.TrimSpace(s[1:]) + if ref == "" { + return "", apperrors.NewValidation( + "json_parse: `@` must be followed by a file path, or `@-` to read from stdin") + } + out, err := transformFileRead(ref) + if err != nil { + return "", err + } + loaded, _ := out.(string) + return strings.TrimSpace(loaded), nil +} + // transformJSONParseStrict is the strict variant of json_parse: only accepts // well-formed JSON, rejecting input that the YAML fallback would otherwise // silently coerce to a scalar string. Use when the upstream tool requires a @@ -274,12 +308,21 @@ func transformJSONParseStrict(value any) (any, error) { if s == "" { return value, nil } + // @file / @- expansion — same sentinel as json_parse (see resolveJSONSource). + s, err := resolveJSONSource(s) + if err != nil { + return nil, err + } + if s == "" { + return value, nil + } var parsed any if err := json.Unmarshal([]byte(s), &parsed); err != nil { return nil, apperrors.NewValidation( "json_parse_strict: input is not valid JSON; " + "this transform rejects YAML-style ad-hoc input — quote the whole value " + - "as strict JSON (e.g. '[{\"key\":\"value\"}]') or use `json_parse` for YAML-tolerant parsing", + "as strict JSON (e.g. '[{\"key\":\"value\"}]'), pass `@path/to/file.json` " + + "(or `@-` for stdin), or use `json_parse` for YAML-tolerant parsing", ) } return parsed, nil diff --git a/internal/compat/transform_test.go b/internal/compat/transform_test.go index 3302d8a8..8b0965d2 100644 --- a/internal/compat/transform_test.go +++ b/internal/compat/transform_test.go @@ -127,6 +127,65 @@ func TestJSONParse_InvalidInput(t *testing.T) { } } +func TestJSONParse_AtFile(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "payload.json") + if err := os.WriteFile(path, []byte(`[{"k":"长内容\n多行","n":1}]`), 0o600); err != nil { + t.Fatalf("write temp file: %v", err) + } + got, err := ApplyTransform("@"+path, "json_parse", nil) + if err != nil { + t.Fatalf("json_parse @file: %v", err) + } + arr, ok := got.([]any) + if !ok || len(arr) != 1 { + t.Fatalf("expected 1-element array from @file, got %T %v", got, got) + } + item := arr[0].(map[string]any) + if item["k"] != "长内容\n多行" { + t.Fatalf("@file content mismatch: %v", item) + } +} + +func TestJSONParse_AtFileMissing(t *testing.T) { + if _, err := ApplyTransform("@"+filepath.Join(t.TempDir(), "nope.json"), "json_parse", nil); err == nil { + t.Fatal("json_parse @missing-file should error") + } +} + +func TestJSONParse_BareAtErrors(t *testing.T) { + _, err := ApplyTransform("@", "json_parse", nil) + if err == nil { + t.Fatal("bare @ should error (needs a path or -)") + } +} + +func TestJSONParseStrict_AtFile(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "p.json") + if err := os.WriteFile(path, []byte(`{"a":[1,2,3]}`), 0o600); err != nil { + t.Fatalf("write: %v", err) + } + got, err := ApplyTransform("@"+path, "json_parse_strict", nil) + if err != nil { + t.Fatalf("json_parse_strict @file: %v", err) + } + if _, ok := got.(map[string]any); !ok { + t.Fatalf("expected object, got %T", got) + } +} + +func TestJSONParse_AtPassthroughNonAt(t *testing.T) { + // A value not starting with "@" must be parsed inline, untouched. + got, err := ApplyTransform(`[{"x":1}]`, "json_parse", nil) + if err != nil { + t.Fatalf("inline json_parse: %v", err) + } + if arr, ok := got.([]any); !ok || len(arr) != 1 { + t.Fatalf("inline parse regressed: %T %v", got, got) + } +} + // TestFileRead_BasicFile exercises the happy path: a UTF-8 file on disk is // read in full and surfaced as a string value. This is the contract the // `--content-file ./a.md` flag relies on so the upstream MCP tool sees the