From 6a4744073c2d760f99a6496d6ce1c9304f93e5bd Mon Sep 17 00:00:00 2001 From: fengbai Date: Wed, 12 Aug 2026 21:07:59 +0800 Subject: [PATCH 01/32] feat(calendar): add event share-info command --- .changes/calendar-event-share-info.md | 5 ++ internal/helpers/calendar.go | 84 ++++++++++++++++++- skills/mono/references/products/calendar.md | 21 +++++ .../dingtalk-calendar/references/calendar.md | 21 +++++ 4 files changed, 129 insertions(+), 2 deletions(-) create mode 100644 .changes/calendar-event-share-info.md diff --git a/.changes/calendar-event-share-info.md b/.changes/calendar-event-share-info.md new file mode 100644 index 00000000..2ec9f1fa --- /dev/null +++ b/.changes/calendar-event-share-info.md @@ -0,0 +1,5 @@ +--- +category: Added +--- + +- **Calendar event share-info** — adds `dws calendar event share-info` to fetch a calendar event's share info (title, organizer, location, join info) for sharing with others; supports `--calendar-id` and `--language`. diff --git a/internal/helpers/calendar.go b/internal/helpers/calendar.go index 71eb3c6a..71d872f6 100644 --- a/internal/helpers/calendar.go +++ b/internal/helpers/calendar.go @@ -230,7 +230,7 @@ func newCalendarCommand() *cobra.Command { Long: `管理钉钉日历：日程、参会人、会议室、闲忙、附件、日历本、访问权限。调用前必须先使用 --help 查看参数结构。 命令结构: - dws calendar event [list|get|create|update|delete|suggest|respond|instances] 日程管理 + dws calendar event [list|get|create|update|delete|suggest|respond|instances|share-info] 日程管理 dws calendar attendee [list|add|delete] 参会人管理 dws calendar room [search|add|delete|list-groups] 会议室管理 dws calendar busy search 闲忙查询 (可查人、查会议室) @@ -2259,7 +2259,87 @@ func newCalendarCommand() *cobra.Command { eventInstancesCmd.Flags().Int("count", 0, "") _ = eventInstancesCmd.Flags().MarkHidden("count") - eventCmd.AddCommand(eventListCmd, eventGetCmd, eventCreateCmd, eventUpdateCmd, eventDeleteCmd, eventSuggestCmd, eventRespondCmd, eventInstancesCmd) + eventShareInfoCmd := &cobra.Command{ + Use: "share-info", + Aliases: []string{"share_info"}, + Short: "获取日程的分享信息", + Long: `根据日程 ID 获取日程的分享信息，展示日程主题、组织人、地点、入会信息等，用于向他人分享日程。`, + Example: ` dws calendar event share-info --id EVENT_ID # 查询 eventId: dws calendar event list + dws calendar event share-info --id EVENT_ID --language zh-CN + dws calendar event share-info --id EVENT_ID --calendar-id primary`, + RunE: func(cmd *cobra.Command, args []string) error { + eventID, err := mustFlagOrFallback(cmd, "id", "event", "event-id", "eventId") + if err != nil { + return err + } + toolArgs := map[string]any{"eventId": eventID} + if v := flagOrFallback(cmd, "calendar-id", "calendarId", "calendar"); v != "" { + toolArgs["calendarId"] = v + } + if v := flagOrFallback(cmd, "language", "lang"); v != "" { + toolArgs["language"] = v + } + return callMCPTool("get_event_share_info", toolArgs) + }, + } + DeclareLeafMetadata(eventShareInfoCmd, LeafSpec{ + Safety: contract.SafetySpec{ + Effect: "read", Risk: "low", + Confirmation: "not_required", Idempotency: "idempotent", + }, + Contract: LeafContract{ + Identity: contract.ToolIdentitySpec{ + ProductID: "calendar", + Name: "get_event_share_info", + CanonicalPath: "calendar.get_event_share_info", + CLIPath: "calendar event share-info", + PrimaryCLIPath: "calendar event share-info", + Aliases: []string{"calendar event share_info"}, + }, + Description: "获取日程的分享信息", + Interface: &contract.InterfaceSpec{ + Mode: "mcp", + Availability: "available", + Ref: &contract.InterfaceRefSpec{ProductID: "calendar", RPCName: "get_event_share_info"}, + }, + Selection: contract.SelectionSpec{ + AgentSummary: "获取日程的分享信息（主题、组织人、地点、入会信息等）", + UseWhen: []string{"已知 eventId，需要获取日程分享信息或入会信息用于分享给他人时"}, + AvoidWhen: []string{ + "要查看日程详情改用 dws calendar event get", + "未知 eventId 时先 dws calendar event list", + }, + Examples: []string{ + "dws calendar event share-info --id ", + "dws calendar event share-info --id --language zh-CN", + }, + }, + Parameters: []contract.ParamDecl{ + {Name: "id", Property: "eventId", Required: boolPtr(true)}, + {Name: "calendar-id", Property: "calendarId"}, + {Name: "language", Property: "language"}, + }, + }, + }) + + // EventShareInfo flags + eventShareInfoCmd.Flags().String("id", "", "日程 ID (必填)") + eventShareInfoCmd.Flags().String("event", "", "") + _ = eventShareInfoCmd.Flags().MarkHidden("event") + eventShareInfoCmd.Flags().String("event-id", "", "") + _ = eventShareInfoCmd.Flags().MarkHidden("event-id") + eventShareInfoCmd.Flags().String("eventId", "", "") + _ = eventShareInfoCmd.Flags().MarkHidden("eventId") + eventShareInfoCmd.Flags().String("calendar-id", "", "日历 ID (默认 primary 主日历)") + eventShareInfoCmd.Flags().String("calendarId", "", "") + _ = eventShareInfoCmd.Flags().MarkHidden("calendarId") + eventShareInfoCmd.Flags().String("calendar", "", "") + _ = eventShareInfoCmd.Flags().MarkHidden("calendar") + eventShareInfoCmd.Flags().String("language", "", "语言代码 (可选，默认 en-US，如 zh-CN)") + eventShareInfoCmd.Flags().String("lang", "", "") + _ = eventShareInfoCmd.Flags().MarkHidden("lang") + + eventCmd.AddCommand(eventListCmd, eventGetCmd, eventCreateCmd, eventUpdateCmd, eventDeleteCmd, eventSuggestCmd, eventRespondCmd, eventInstancesCmd, eventShareInfoCmd) // participant participantCmd.PersistentFlags().String("event", "", "日程 ID (必填)") diff --git a/skills/mono/references/products/calendar.md b/skills/mono/references/products/calendar.md index 70b3d2bc..4711854b 100644 --- a/skills/mono/references/products/calendar.md +++ b/skills/mono/references/products/calendar.md @@ -44,6 +44,8 @@ dws calendar event [create|update|get|delete|respond] [flags] dws calendar event list [flags] # 查询循环日程的实例列表（按时间范围展开重复日程） dws calendar event instances [flags] +# 获取日程的分享信息（日程主题、组织人、地点、入会信息等，用于向他人分享日程） +dws calendar event share-info [flags] # 对于非明确时间或一段时间范围的约会场景，可基于所有参会人的忙闲状态，推荐多个可用的时间块方案 dws calendar event suggest [flags] ``` @@ -160,6 +162,22 @@ Flags: > **默认行为**：不传 `--start` / `--end` 时，默认返回今天的实例（00:00:00 ~ 23:59:59）。 > **分页**：单次最多返回 `--limit` 指定的条数（默认/最大 100）；当结果超过 limit 时，返回体包含 `nextCursor` 字段。 +### 获取日程分享信息 +``` +Usage: + dws calendar event share-info [flags] +Example: + dws calendar event share-info --id + dws calendar event share-info --id --language zh-CN + dws calendar event share-info --id --calendar-id primary +Flags: + --id string 日程 ID (必填) + --calendar-id string 日历 ID (可选，默认 primary 主日历) + --language string 语言代码 (可选，默认 en-US，如 zh-CN) +``` + +> **说明**：根据日程 ID 获取日程的分享信息，展示日程主题、组织人、地点、入会信息等，用于向他人分享日程（如发送到群聊、邮件）。中文内容建议传 `--language zh-CN`。 + ### 创建日程 ``` Usage: @@ -585,6 +603,7 @@ Flags: - 推荐时间/什么时候有空/协调时间 → `event suggest` - 接受/拒绝/暂定日程 → `event respond` - 查询循环日程/重复日程的每次实例/展开循环日程 → `event instances` +- 分享日程/把日程发给别人/获取日程分享信息或入会信息 → `event share-info` 用户说"参会人/与会者": - 查看 → `attendee list` @@ -691,6 +710,7 @@ dws calendar event list --start "2026-03-10T14:00:00+08:00" --end "2026-03-10T15 | `event suggest` | 推荐的时间段 | event create 的 --start/--end | | `event respond` | 响应结果 | — | | `event instances` | `result.events[].id`, `nextCursor` | event get/update/delete/respond 的 --id；下一页 --cursor | +| `event share-info` | 日程分享信息（主题、组织人、地点、入会信息等） | 分享给他人（如 chat/mail 发送） | | `room search` | `rooms[].roomId` | room add 的 --rooms 或 event create 的 --rooms | | `room list-groups` | `groups[].groupId` | room search 的 --group-id | | `book list` | `id`（如 `primary`） | event list/get 的 --calendar-id, book get/update 的 --id | @@ -718,6 +738,7 @@ dws calendar event list --start "2026-03-10T14:00:00+08:00" --end "2026-03-10T15 - **日历本**：`book list` 返回的 `id` 才是合法 `calendarId`；如无明确说明，`event list` / `event get` 都不要带 `--calendar-id`，让接口默认走 primary 主日历 - **分页查询**：`event list` / `event instances` 均支持 `--limit`（控制每页条数，默认/最大 100）和 `--cursor`（翻页游标）；**首次查询无需传 `--cursor`**，仅当返回体中包含 `nextCursor` 时，将其作为 `--cursor` 传入可获取下一页 - **循环日程实例**：`event instances` 用于按时间范围展开重复日程（SeriesMaster）的每一个实例；**普通非循环日程调用该命令将查不到任何实例信息** +- **日程分享**：`event share-info` 获取日程的分享信息（主题、组织人、地点、入会信息等）；`--language` 控制文案语言（默认 en-US，中文场景传 zh-CN） - **日程提醒**：`event create` 支持 `--remind-minutes` 设置开始前提醒，逗号分隔多个分钟数（如 `--remind-minutes 5,10,15`），不传则默认15分钟提醒 - **会议室分页**：`room search` 支持 `--limit`（每页条数，默认100，最大100）和 `--page`（分页起始位置，默认0），与 `room list-groups` 分页风格一致 diff --git a/skills/multi/dingtalk-calendar/references/calendar.md b/skills/multi/dingtalk-calendar/references/calendar.md index 70b3d2bc..4711854b 100644 --- a/skills/multi/dingtalk-calendar/references/calendar.md +++ b/skills/multi/dingtalk-calendar/references/calendar.md @@ -44,6 +44,8 @@ dws calendar event [create|update|get|delete|respond] [flags] dws calendar event list [flags] # 查询循环日程的实例列表（按时间范围展开重复日程） dws calendar event instances [flags] +# 获取日程的分享信息（日程主题、组织人、地点、入会信息等，用于向他人分享日程） +dws calendar event share-info [flags] # 对于非明确时间或一段时间范围的约会场景，可基于所有参会人的忙闲状态，推荐多个可用的时间块方案 dws calendar event suggest [flags] ``` @@ -160,6 +162,22 @@ Flags: > **默认行为**：不传 `--start` / `--end` 时，默认返回今天的实例（00:00:00 ~ 23:59:59）。 > **分页**：单次最多返回 `--limit` 指定的条数（默认/最大 100）；当结果超过 limit 时，返回体包含 `nextCursor` 字段。 +### 获取日程分享信息 +``` +Usage: + dws calendar event share-info [flags] +Example: + dws calendar event share-info --id + dws calendar event share-info --id --language zh-CN + dws calendar event share-info --id --calendar-id primary +Flags: + --id string 日程 ID (必填) + --calendar-id string 日历 ID (可选，默认 primary 主日历) + --language string 语言代码 (可选，默认 en-US，如 zh-CN) +``` + +> **说明**：根据日程 ID 获取日程的分享信息，展示日程主题、组织人、地点、入会信息等，用于向他人分享日程（如发送到群聊、邮件）。中文内容建议传 `--language zh-CN`。 + ### 创建日程 ``` Usage: @@ -585,6 +603,7 @@ Flags: - 推荐时间/什么时候有空/协调时间 → `event suggest` - 接受/拒绝/暂定日程 → `event respond` - 查询循环日程/重复日程的每次实例/展开循环日程 → `event instances` +- 分享日程/把日程发给别人/获取日程分享信息或入会信息 → `event share-info` 用户说"参会人/与会者": - 查看 → `attendee list` @@ -691,6 +710,7 @@ dws calendar event list --start "2026-03-10T14:00:00+08:00" --end "2026-03-10T15 | `event suggest` | 推荐的时间段 | event create 的 --start/--end | | `event respond` | 响应结果 | — | | `event instances` | `result.events[].id`, `nextCursor` | event get/update/delete/respond 的 --id；下一页 --cursor | +| `event share-info` | 日程分享信息（主题、组织人、地点、入会信息等） | 分享给他人（如 chat/mail 发送） | | `room search` | `rooms[].roomId` | room add 的 --rooms 或 event create 的 --rooms | | `room list-groups` | `groups[].groupId` | room search 的 --group-id | | `book list` | `id`（如 `primary`） | event list/get 的 --calendar-id, book get/update 的 --id | @@ -718,6 +738,7 @@ dws calendar event list --start "2026-03-10T14:00:00+08:00" --end "2026-03-10T15 - **日历本**：`book list` 返回的 `id` 才是合法 `calendarId`；如无明确说明，`event list` / `event get` 都不要带 `--calendar-id`，让接口默认走 primary 主日历 - **分页查询**：`event list` / `event instances` 均支持 `--limit`（控制每页条数，默认/最大 100）和 `--cursor`（翻页游标）；**首次查询无需传 `--cursor`**，仅当返回体中包含 `nextCursor` 时，将其作为 `--cursor` 传入可获取下一页 - **循环日程实例**：`event instances` 用于按时间范围展开重复日程（SeriesMaster）的每一个实例；**普通非循环日程调用该命令将查不到任何实例信息** +- **日程分享**：`event share-info` 获取日程的分享信息（主题、组织人、地点、入会信息等）；`--language` 控制文案语言（默认 en-US，中文场景传 zh-CN） - **日程提醒**：`event create` 支持 `--remind-minutes` 设置开始前提醒，逗号分隔多个分钟数（如 `--remind-minutes 5,10,15`），不传则默认15分钟提醒 - **会议室分页**：`room search` 支持 `--limit`（每页条数，默认100，最大100）和 `--page`（分页起始位置，默认0），与 `room list-groups` 分页风格一致 From 8aee08268d38a8fc7bc8d5038ec0dfb12bde278d Mon Sep 17 00:00:00 2001 From: fengbai Date: Wed, 12 Aug 2026 21:17:32 +0800 Subject: [PATCH 02/32] test(calendar): add event share-info dry-run and required-flag tests --- internal/app/root_help_test.go | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/internal/app/root_help_test.go b/internal/app/root_help_test.go index eb370a7b..31949cd3 100644 --- a/internal/app/root_help_test.go +++ b/internal/app/root_help_test.go @@ -403,6 +403,35 @@ func TestCalendarEventListDryRunPreviewsOnly(t *testing.T) { } } +func TestCalendarEventShareInfoDryRunPreviewsOnly(t *testing.T) { + got, err := executeRootCaptureStdout(t, []string{ + "--dry-run", "calendar", "event", "share-info", + "--id", "EVT_001", + "--language", "zh-CN", + "--calendar-id", "primary", + }) + if err != nil { + t.Fatalf("calendar event share-info --dry-run error = %v\n%s", err, got) + } + for _, want := range []string{"get_event_share_info", "eventId", "EVT_001", "zh-CN", "primary"} { + if !strings.Contains(got, want) { + t.Fatalf("calendar event share-info dry-run output missing %q:\n%s", want, got) + } + } +} + +func TestCalendarEventShareInfoRequiresEventID(t *testing.T) { + got, err := executeRootCaptureStdout(t, []string{ + "--dry-run", "calendar", "event", "share-info", + }) + if err == nil { + t.Fatalf("calendar event share-info without --id: expected error, got nil\n%s", got) + } + if strings.Contains(got, "\"executed\": true") { + t.Fatalf("share-info without --id must not execute:\n%s", got) + } +} + func TestRootKeepsSVIPChatCompatibilityFlags(t *testing.T) { root := NewRootCommand() From 07aa2c883ad08822c96d49be2df55230caa759f1 Mon Sep 17 00:00:00 2001 From: fengbai Date: Fri, 14 Aug 2026 15:10:20 +0800 Subject: [PATCH 03/32] fix(calendar): address CR comments for share-info - Fix Example indentation (tab -> 2 spaces) - Remove unsubstantiated default en-US from --language help/docs - Add test asserting calendarId/language are omitted when only --id is passed --- .changes/calendar-event-share-info.md | 2 +- internal/app/root_help_test.go | 18 ++++++++++++++++++ internal/helpers/calendar.go | 6 +++--- skills/mono/references/products/calendar.md | 4 ++-- .../dingtalk-calendar/references/calendar.md | 4 ++-- 5 files changed, 26 insertions(+), 8 deletions(-) diff --git a/.changes/calendar-event-share-info.md b/.changes/calendar-event-share-info.md index 2ec9f1fa..b1cde315 100644 --- a/.changes/calendar-event-share-info.md +++ b/.changes/calendar-event-share-info.md @@ -2,4 +2,4 @@ category: Added --- -- **Calendar event share-info** — adds `dws calendar event share-info` to fetch a calendar event's share info (title, organizer, location, join info) for sharing with others; supports `--calendar-id` and `--language`. +- **Calendar event share-info** (#980) — adds `dws calendar event share-info` to fetch a calendar event's share info (title, organizer, location, join info) for sharing with others; supports `--calendar-id` and `--language`. diff --git a/internal/app/root_help_test.go b/internal/app/root_help_test.go index 31949cd3..c8bf72b8 100644 --- a/internal/app/root_help_test.go +++ b/internal/app/root_help_test.go @@ -432,6 +432,24 @@ func TestCalendarEventShareInfoRequiresEventID(t *testing.T) { } } +func TestCalendarEventShareInfoOmitsOptionalArgs(t *testing.T) { + got, err := executeRootCaptureStdout(t, []string{ + "--dry-run", "calendar", "event", "share-info", + "--id", "EVT_001", + }) + if err != nil { + t.Fatalf("calendar event share-info --dry-run with only --id error = %v\n%s", err, got) + } + if !strings.Contains(got, "\"eventId\"") { + t.Fatalf("calendar event share-info dry-run output missing eventId:\n%s", got) + } + for _, unwanted := range []string{"\"calendarId\"", "\"language\""} { + if strings.Contains(got, unwanted) { + t.Fatalf("calendar event share-info dry-run with only --id should not contain %q:\n%s", unwanted, got) + } + } +} + func TestRootKeepsSVIPChatCompatibilityFlags(t *testing.T) { root := NewRootCommand() diff --git a/internal/helpers/calendar.go b/internal/helpers/calendar.go index 71d872f6..b647b48e 100644 --- a/internal/helpers/calendar.go +++ b/internal/helpers/calendar.go @@ -2265,8 +2265,8 @@ func newCalendarCommand() *cobra.Command { Short: "获取日程的分享信息", Long: `根据日程 ID 获取日程的分享信息，展示日程主题、组织人、地点、入会信息等，用于向他人分享日程。`, Example: ` dws calendar event share-info --id EVENT_ID # 查询 eventId: dws calendar event list - dws calendar event share-info --id EVENT_ID --language zh-CN - dws calendar event share-info --id EVENT_ID --calendar-id primary`, + dws calendar event share-info --id EVENT_ID --language zh-CN + dws calendar event share-info --id EVENT_ID --calendar-id primary`, RunE: func(cmd *cobra.Command, args []string) error { eventID, err := mustFlagOrFallback(cmd, "id", "event", "event-id", "eventId") if err != nil { @@ -2335,7 +2335,7 @@ func newCalendarCommand() *cobra.Command { _ = eventShareInfoCmd.Flags().MarkHidden("calendarId") eventShareInfoCmd.Flags().String("calendar", "", "") _ = eventShareInfoCmd.Flags().MarkHidden("calendar") - eventShareInfoCmd.Flags().String("language", "", "语言代码 (可选，默认 en-US，如 zh-CN)") + eventShareInfoCmd.Flags().String("language", "", "语言代码 (可选，如 zh-CN)") eventShareInfoCmd.Flags().String("lang", "", "") _ = eventShareInfoCmd.Flags().MarkHidden("lang") diff --git a/skills/mono/references/products/calendar.md b/skills/mono/references/products/calendar.md index 4711854b..db2873f9 100644 --- a/skills/mono/references/products/calendar.md +++ b/skills/mono/references/products/calendar.md @@ -173,7 +173,7 @@ Example: Flags: --id string 日程 ID (必填) --calendar-id string 日历 ID (可选，默认 primary 主日历) - --language string 语言代码 (可选，默认 en-US，如 zh-CN) + --language string 语言代码 (可选，如 zh-CN) ``` > **说明**：根据日程 ID 获取日程的分享信息，展示日程主题、组织人、地点、入会信息等，用于向他人分享日程（如发送到群聊、邮件）。中文内容建议传 `--language zh-CN`。 @@ -738,7 +738,7 @@ dws calendar event list --start "2026-03-10T14:00:00+08:00" --end "2026-03-10T15 - **日历本**：`book list` 返回的 `id` 才是合法 `calendarId`；如无明确说明，`event list` / `event get` 都不要带 `--calendar-id`，让接口默认走 primary 主日历 - **分页查询**：`event list` / `event instances` 均支持 `--limit`（控制每页条数，默认/最大 100）和 `--cursor`（翻页游标）；**首次查询无需传 `--cursor`**，仅当返回体中包含 `nextCursor` 时，将其作为 `--cursor` 传入可获取下一页 - **循环日程实例**：`event instances` 用于按时间范围展开重复日程（SeriesMaster）的每一个实例；**普通非循环日程调用该命令将查不到任何实例信息** -- **日程分享**：`event share-info` 获取日程的分享信息（主题、组织人、地点、入会信息等）；`--language` 控制文案语言（默认 en-US，中文场景传 zh-CN） +- **日程分享**：`event share-info` 获取日程的分享信息（主题、组织人、地点、入会信息等）；`--language` 控制文案语言（中文场景传 zh-CN） - **日程提醒**：`event create` 支持 `--remind-minutes` 设置开始前提醒，逗号分隔多个分钟数（如 `--remind-minutes 5,10,15`），不传则默认15分钟提醒 - **会议室分页**：`room search` 支持 `--limit`（每页条数，默认100，最大100）和 `--page`（分页起始位置，默认0），与 `room list-groups` 分页风格一致 diff --git a/skills/multi/dingtalk-calendar/references/calendar.md b/skills/multi/dingtalk-calendar/references/calendar.md index 4711854b..db2873f9 100644 --- a/skills/multi/dingtalk-calendar/references/calendar.md +++ b/skills/multi/dingtalk-calendar/references/calendar.md @@ -173,7 +173,7 @@ Example: Flags: --id string 日程 ID (必填) --calendar-id string 日历 ID (可选，默认 primary 主日历) - --language string 语言代码 (可选，默认 en-US，如 zh-CN) + --language string 语言代码 (可选，如 zh-CN) ``` > **说明**：根据日程 ID 获取日程的分享信息，展示日程主题、组织人、地点、入会信息等，用于向他人分享日程（如发送到群聊、邮件）。中文内容建议传 `--language zh-CN`。 @@ -738,7 +738,7 @@ dws calendar event list --start "2026-03-10T14:00:00+08:00" --end "2026-03-10T15 - **日历本**：`book list` 返回的 `id` 才是合法 `calendarId`；如无明确说明，`event list` / `event get` 都不要带 `--calendar-id`，让接口默认走 primary 主日历 - **分页查询**：`event list` / `event instances` 均支持 `--limit`（控制每页条数，默认/最大 100）和 `--cursor`（翻页游标）；**首次查询无需传 `--cursor`**，仅当返回体中包含 `nextCursor` 时，将其作为 `--cursor` 传入可获取下一页 - **循环日程实例**：`event instances` 用于按时间范围展开重复日程（SeriesMaster）的每一个实例；**普通非循环日程调用该命令将查不到任何实例信息** -- **日程分享**：`event share-info` 获取日程的分享信息（主题、组织人、地点、入会信息等）；`--language` 控制文案语言（默认 en-US，中文场景传 zh-CN） +- **日程分享**：`event share-info` 获取日程的分享信息（主题、组织人、地点、入会信息等）；`--language` 控制文案语言（中文场景传 zh-CN） - **日程提醒**：`event create` 支持 `--remind-minutes` 设置开始前提醒，逗号分隔多个分钟数（如 `--remind-minutes 5,10,15`），不传则默认15分钟提醒 - **会议室分页**：`room search` 支持 `--limit`（每页条数，默认100，最大100）和 `--page`（分页起始位置，默认0），与 `room list-groups` 分页风格一致 From 90473284b84d3b2b42226b9b6c311c1bedb3da3a Mon Sep 17 00:00:00 2001 From: fengbai Date: Fri, 14 Aug 2026 15:51:20 +0800 Subject: [PATCH 04/32] fix(calendar): remove shell comment from share-info example - Move the eventId lookup hint into Long description - Keep example commands free of shell comments to pass example policy gate --- internal/helpers/calendar.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/helpers/calendar.go b/internal/helpers/calendar.go index b647b48e..5c71ea67 100644 --- a/internal/helpers/calendar.go +++ b/internal/helpers/calendar.go @@ -2263,8 +2263,8 @@ func newCalendarCommand() *cobra.Command { Use: "share-info", Aliases: []string{"share_info"}, Short: "获取日程的分享信息", - Long: `根据日程 ID 获取日程的分享信息，展示日程主题、组织人、地点、入会信息等，用于向他人分享日程。`, - Example: ` dws calendar event share-info --id EVENT_ID # 查询 eventId: dws calendar event list + Long: `根据日程 ID 获取日程的分享信息，展示日程主题、组织人、地点、入会信息等，用于向他人分享日程。eventId 可通过 dws calendar event list 获取。`, + Example: ` dws calendar event share-info --id EVENT_ID dws calendar event share-info --id EVENT_ID --language zh-CN dws calendar event share-info --id EVENT_ID --calendar-id primary`, RunE: func(cmd *cobra.Command, args []string) error { From d534ee242c87d5352b3d9a86876aaae5b9027478 Mon Sep 17 00:00:00 2001 From: ruigong Date: Fri, 14 Aug 2026 17:33:54 +0800 Subject: [PATCH 05/32] fix(skill): scope doc/drive descriptions to entity-content vs file management --- .changes/doc-drive-description-scope.md | 5 +++++ skills/multi/dingtalk-doc/SKILL.md | 2 +- skills/multi/dingtalk-drive/SKILL.md | 2 +- 3 files changed, 7 insertions(+), 2 deletions(-) create mode 100644 .changes/doc-drive-description-scope.md diff --git a/.changes/doc-drive-description-scope.md b/.changes/doc-drive-description-scope.md new file mode 100644 index 00000000..82d47917 --- /dev/null +++ b/.changes/doc-drive-description-scope.md @@ -0,0 +1,5 @@ +--- +category: Changed +--- + +- **Doc/drive description scope** — restates the `dingtalk-doc` description as document-entity-and-content operations with an explicit exclusion list, and narrows `dingtalk-drive` to file-level management of DingTalk documents, so first-round Agent selection separates content work from file management without changing CLI behavior. diff --git a/skills/multi/dingtalk-doc/SKILL.md b/skills/multi/dingtalk-doc/SKILL.md index 7c3b52c6..534cef40 100644 --- a/skills/multi/dingtalk-doc/SKILL.md +++ b/skills/multi/dingtalk-doc/SKILL.md @@ -1,6 +1,6 @@ --- name: dingtalk-doc -description: 钉钉在线文字文档（adoc，「文档空间」里的在线文档）的查找、读取、创建、编辑、块、评论、附件、导入、导出(docx/markdown/pdf)、模板、版本、权限协作及 Markdown/JSONML 写入。文档空间与钉盘的文件管理走 dingtalk-drive（doc 同名原子命令已弃用），知识库空间与空间内节点走 dingtalk-wiki，原生 .md 文件读写走 dingtalk-misc。命令前缀：dws doc。 +description: 钉钉在线文字文档（adoc）本体及其内容的操作：查找、创建、读取、文档信息、编辑、块、评论、附件与媒体、白板卡片、导入、导出(docx/markdown/pdf)、版本、模板、权限、分享及Markdown/JSONML写入。不包括：文档空间与钉盘的文件管理（归 dingtalk-drive，doc 同名原子命令已弃用）、知识库空间与节点管理（归 dingtalk-wiki）、原生 .md 文件读写（归 dingtalk-misc）、电子表格 axls（归 dingtalk-misc）、AI 表格 able（归 dingtalk-aitable）。命令前缀：dws doc。 metadata: cli_version: ">=0.2.14" category: product diff --git a/skills/multi/dingtalk-drive/SKILL.md b/skills/multi/dingtalk-drive/SKILL.md index fb0c3abd..7d2beba4 100644 --- a/skills/multi/dingtalk-drive/SKILL.md +++ b/skills/multi/dingtalk-drive/SKILL.md @@ -1,6 +1,6 @@ --- name: dingtalk-drive -description: 钉钉文件管理（存储层，覆盖钉盘与文档空间两个存储域）。Use when 用户说 钉盘/上传文件/下载文件/文件夹/查文件/找文件/全局搜索文件/复制/移动/重命名/删除/回收站/还原删除文件/权限管理/普通文件下载/本地与钉盘文件夹差异比较/整个文件夹拉到本地/整个文件夹推到钉盘/双向同步；也承接钉钉文档的这些管理动作（doc 侧同名原子命令已弃用）。文档正文编辑与导出 docx/markdown/pdf 走 dingtalk-doc，知识库空间与空间内节点组织走 dingtalk-wiki。命令前缀：dws drive。 +description: 钉钉文件管理（存储层，覆盖钉盘与文档空间两个存储域）。Use when 用户说 钉盘/上传文件/下载文件/文件夹/查文件/找文件/全局搜索文件/最近编辑的文档/复制/移动/重命名/删除/回收站/还原删除文件/文件权限管理/普通文件下载/本地与钉盘文件夹差异比较/整个文件夹拉到本地/整个文件夹推到钉盘/双向同步。对钉钉文档仅处理其作为文件的管理操作，文档本体及其内容操作归 dingtalk-doc；知识库空间与空间内节点组织归 dingtalk-wiki。命令前缀：dws drive。 metadata: cli_version: ">=0.2.14" category: product From abe512930649ea46a7a93076256674e4d896e6de Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E7=82=B3=E6=98=B1?= Date: Fri, 14 Aug 2026 17:56:51 +0800 Subject: [PATCH 06/32] fix(event): support Windows bus lifecycle --- .changes/windows-event-bus-lifecycle.md | 5 + internal/app/event_command.go | 3 +- internal/app/event_personal_command.go | 2 +- internal/event/bus/coverage_edges_test.go | 11 +- internal/event/bus/daemon.go | 21 ++-- internal/event/bus/daemon_test.go | 61 +++++++++++ internal/event/busctl/discover.go | 5 +- internal/event/busctl/discover_test.go | 5 +- internal/event/busctl/spawn.go | 29 +++--- internal/event/busctl/spawn_test.go | 25 ++++- internal/event/busctl/spawn_unix.go | 7 ++ internal/event/busctl/spawn_windows.go | 87 ++++++++++++++++ internal/event/busctl/spawn_windows_test.go | 108 ++++++++++++++++++++ internal/event/busctl/stop.go | 83 +++++++++++---- internal/event/busctl/stop_test.go | 84 +++++++++++++++ internal/event/busctl/stop_windows.go | 9 +- internal/event/busctl/stop_windows_test.go | 27 +++++ 17 files changed, 517 insertions(+), 55 deletions(-) create mode 100644 .changes/windows-event-bus-lifecycle.md create mode 100644 internal/event/busctl/spawn_windows_test.go create mode 100644 internal/event/busctl/stop_windows_test.go diff --git a/.changes/windows-event-bus-lifecycle.md b/.changes/windows-event-bus-lifecycle.md new file mode 100644 index 00000000..f158d0ad --- /dev/null +++ b/.changes/windows-event-bus-lifecycle.md @@ -0,0 +1,5 @@ +--- +category: Fixed +--- + +- **Windows event bus lifecycle** — start event consumers without unsupported inherited file descriptors, stop buses through local IPC with a termination fallback, and preserve subscription cleanup when startup fails. diff --git a/internal/app/event_command.go b/internal/app/event_command.go index 533c95a3..f0cc1e24 100644 --- a/internal/app/event_command.go +++ b/internal/app/event_command.go @@ -1220,7 +1220,8 @@ func newEventStopCommandWithFlags(globalFlags ...*GlobalFlags) *cobra.Command { editionName := editionNameOrDefault() clientIDHash := dwsevent.ClientIDHash(clientID) workDir := eventWorkDir(configDir, editionName, dwsevent.SourceKindAppStream, clientIDHash) - if err := eventStopBus(busctl.StopConfig{WorkDir: workDir}); err != nil { + ipcEndpoint := defaultIPCEndpoint(workDir, editionName, dwsevent.SourceKindAppStream, clientIDHash) + if err := eventStopBus(busctl.StopConfig{WorkDir: workDir, IPCEndpoint: ipcEndpoint}); err != nil { if errors.Is(err, busctl.ErrNotRunning) { fmt.Fprintln(c.OutOrStdout(), "bus is not running") return nil diff --git a/internal/app/event_personal_command.go b/internal/app/event_personal_command.go index b2b1dc03..20ff6207 100644 --- a/internal/app/event_personal_command.go +++ b/internal/app/event_personal_command.go @@ -1220,7 +1220,7 @@ func runPersonalEventStop(c *cobra.Command, opts personalStopOptions) error { } busState := "personal bus stopped" - if err := personalStopBus(busctl.StopConfig{WorkDir: workDir}); err != nil { + if err := personalStopBus(busctl.StopConfig{WorkDir: workDir, IPCEndpoint: ipcEndpoint}); err != nil { if errors.Is(err, busctl.ErrNotRunning) { busState = "personal bus is not running" } else { diff --git a/internal/event/bus/coverage_edges_test.go b/internal/event/bus/coverage_edges_test.go index 8086e4d7..ea0f9cbb 100644 --- a/internal/event/bus/coverage_edges_test.go +++ b/internal/event/bus/coverage_edges_test.go @@ -361,13 +361,18 @@ func TestCrossPlatformCoverageDaemonMethodEdges(t *testing.T) { } return nil, net.ErrClosed }} - d := &daemon{listener: l, log: logger, hub: NewHub(1), idleStop: make(chan struct{})} + d := &daemon{listener: l, log: logger, hub: NewHub(1), idleStop: make(chan struct{}), stopReq: make(chan string, 1)} d.acceptLoop(context.Background()) d.shuttingDown.Store(true) d.acceptLoop(context.Background()) d.triggerShutdown("test") - if !l.closed { - t.Fatal("trigger did not close listener") + select { + case reason := <-d.stopReq: + if reason != "test" { + t.Fatalf("stop reason = %q", reason) + } + default: + t.Fatal("trigger did not notify lifecycle loop") } d = &daemon{listener: &scriptedListener{accept: func() (net.Conn, error) { return nil, net.ErrClosed }}, log: logger, hub: NewHub(1), idleStop: make(chan struct{})} diff --git a/internal/event/bus/daemon.go b/internal/event/bus/daemon.go index 7fac9aff..0c43d908 100644 --- a/internal/event/bus/daemon.go +++ b/internal/event/bus/daemon.go @@ -186,6 +186,7 @@ func Run(ctx context.Context, cfg Config) error { dedup: dd, started: time.Now().UTC(), idleStop: make(chan struct{}), + stopReq: make(chan string, 1), } // 4. Signal ready BEFORE accepting consumers (avoids a slow-fork @@ -263,6 +264,9 @@ func Run(ctx context.Context, cfg Config) error { } case <-d.idleStop: log.Info("bus: idle timeout reached, shutting down") + case reason := <-d.stopReq: + shutdownReason = reason + log.Info("bus: shutdown requested via IPC", "reason", reason) } // 7. Graceful shutdown — cancel runCtx first so all background @@ -292,6 +296,7 @@ type daemon struct { shutdownMu sync.Mutex shuttingDown atomic.Bool idleStop chan struct{} + stopReq chan string credentialHandoffMu sync.Mutex terminalMu sync.RWMutex @@ -749,15 +754,17 @@ func (d *daemon) idleWatch(ctx context.Context) { } } -// triggerShutdown is called from RPC handlers that want to end the bus. -// It works by closing the listener (which unblocks Run's select via the -// source error path, indirectly). For v1 a full ctx-cancellation hook is -// out of scope; busctl/stop also sends SIGTERM which is the authoritative -// shutdown path. +// triggerShutdown is called from RPC handlers that want to end the bus. The +// buffered request wakes Run's lifecycle select, which then cancels the cloud +// source and performs the same graceful shutdown used for signals and idle +// expiry. It is intentionally non-blocking so repeated stop requests cannot +// strand connection handlers. func (d *daemon) triggerShutdown(reason string) { d.log.Info("bus: shutdown triggered via IPC", "reason", reason) - _ = d.listener.Close() // unblocks Accept(), but doesn't kill Source - // Best-effort: a future version wires a context.CancelFunc here. + select { + case d.stopReq <- reason: + default: + } } // shutdown performs the graceful tear-down sequence: diff --git a/internal/event/bus/daemon_test.go b/internal/event/bus/daemon_test.go index 49a46d20..d99e4aa7 100644 --- a/internal/event/bus/daemon_test.go +++ b/internal/event/bus/daemon_test.go @@ -141,6 +141,67 @@ func TestDaemon_RunStartsAndShutsDownCleanly(t *testing.T) { } } +func TestCrossPlatformCoverageDaemonIPCStopShutsDownCleanly(t *testing.T) { + workDir := shortTempDir(t) + identityHash := dwsevent.IdentityHash(workDir) + endpoint := dwsevent.IPCEndpoint(workDir, "open", dwsevent.SourceKindAppStream, identityHash) + runDone := make(chan error, 1) + go func() { + runDone <- Run(context.Background(), Config{ + WorkDir: workDir, + IPCEndpoint: endpoint, + ClientID: "ding_ipc_stop_test", + Edition: "open", + SourceKind: dwsevent.SourceKindAppStream, + IdentityHash: identityHash, + Source: &fakeSource{}, + }) + }() + + var conn net.Conn + deadline := time.Now().Add(3 * time.Second) + for time.Now().Before(deadline) { + var err error + conn, err = transport.Dial(endpoint) + if err == nil { + break + } + time.Sleep(10 * time.Millisecond) + } + if conn == nil { + t.Fatal("bus IPC endpoint did not become ready") + } + w := transport.NewWriter(conn) + r := transport.NewReader(conn) + if err := w.WriteJSON(transport.Hello{ + Type: transport.FrameTypeHello, + ConsumerPID: os.Getpid(), + Role: transport.HelloRoleStop, + }); err != nil { + t.Fatalf("write stop hello: %v", err) + } + var bye transport.Bye + if err := r.ReadJSON(&bye); err != nil { + t.Fatalf("read stop response: %v", err) + } + _ = conn.Close() + if bye.Type != transport.FrameTypeBye || bye.Reason != "stop_request" { + t.Fatalf("stop response = %#v", bye) + } + + select { + case err := <-runDone: + if err != nil { + t.Fatalf("Run returned after IPC stop: %v", err) + } + case <-time.After(3 * time.Second): + t.Fatal("Run did not return after IPC stop") + } + if pid := ReadHolderPID(filepath.Join(workDir, LockFileName)); pid != 0 { + t.Fatalf("bus lock retained pid %d after IPC stop", pid) + } +} + func TestDaemon_ConsumerReceivesEvents(t *testing.T) { skipOnWindows(t, "uses Unix socket dial") workDir := shortTempDir(t) diff --git a/internal/event/busctl/discover.go b/internal/event/busctl/discover.go index 2bbbb863..56fd1c64 100644 --- a/internal/event/busctl/discover.go +++ b/internal/event/busctl/discover.go @@ -112,8 +112,9 @@ func Discover(cfg DiscoverConfig) (net.Conn, error) { return nil, fmt.Errorf("busctl: mkdir workdir: %w", err) } _, spawnErr := cfg.Spawn(SpawnConfig{ - ClientID: cfg.ClientID, - ExtraArgs: cfg.SpawnExtraArgs, + ClientID: cfg.ClientID, + IPCEndpoint: cfg.IPCEndpoint, + ExtraArgs: cfg.SpawnExtraArgs, }) if spawnErr != nil && !errors.Is(spawnErr, ErrSpawnFailed) { // Hard error (couldn't even exec the child). Stop here — no bus diff --git a/internal/event/busctl/discover_test.go b/internal/event/busctl/discover_test.go index a4b42bcb..7e2e2dbc 100644 --- a/internal/event/busctl/discover_test.go +++ b/internal/event/busctl/discover_test.go @@ -122,7 +122,10 @@ func TestDiscover_NoBus_SpawnSucceeds(t *testing.T) { closer() } }) - fakeSpawn := func(SpawnConfig) (int, error) { + fakeSpawn := func(cfg SpawnConfig) (int, error) { + if cfg.IPCEndpoint != sock { + t.Fatalf("spawn IPC endpoint = %q, want %q", cfg.IPCEndpoint, sock) + } closer = startStubBus(t, sock) return 12345, nil } diff --git a/internal/event/busctl/spawn.go b/internal/event/busctl/spawn.go index 5c3c589f..f3b89165 100644 --- a/internal/event/busctl/spawn.go +++ b/internal/event/busctl/spawn.go @@ -43,18 +43,14 @@ var ( spawnPipe = os.Pipe ) -// ErrSpawnFailed is returned when the child reports startup failure via -// the ready pipe ('E' byte). The child's exit error / log file holds the -// actual cause; this sentinel just lets the caller distinguish "ready -// pipe said no" from "ready pipe timed out / closed early". -var ErrSpawnFailed = errors.New("busctl: bus child reported startup failure on ready pipe") +// ErrSpawnFailed is returned when the child reports startup failure through +// the Unix ready pipe or exits before binding the Windows named pipe. +var ErrSpawnFailed = errors.New("busctl: bus child reported startup failure") // ErrSpawnTimeout is returned when ReadyTimeout elapses without any signal. var ErrSpawnTimeout = errors.New("busctl: bus child did not signal readiness within deadline") -// SpawnConfig describes one spawn attempt. ClientID is the only field -// inspected by the child; the rest govern process attributes the parent -// applies before exec. +// SpawnConfig describes one spawn attempt. type SpawnConfig struct { // ExecPath is the dws binary to exec. Default os.Executable(). ExecPath string @@ -62,18 +58,23 @@ type SpawnConfig struct { // ClientID is passed as `--client-id` to `dws event _bus`. Required. ClientID string + // IPCEndpoint is the bus endpoint the child binds. Windows uses the + // existing named pipe as its readiness handshake because os/exec does not + // support ExtraFiles there. Unix keeps the inherited ready-pipe protocol. + IPCEndpoint string + // ExtraArgs are appended after `--client-id`. Empty for normal use; tests // pass `--extra-flag-for-test` etc. ExtraArgs []string - // Env to pass to the child. Defaults to os.Environ(). The ReadyFDEnv - // entry is appended automatically. + // Env to pass to the child. Defaults to os.Environ(). Unix appends the + // ReadyFDEnv entry automatically; Windows removes any inherited copy. Env []string } -// Spawn forks a detached `dws event _bus --client-id ` child process and -// waits for it to signal readiness via the ready pipe. Returns the child's -// PID on success — the caller can then dial the bus IPC endpoint. +// spawnWithReadyPipe is the Unix implementation behind Spawn. It forks a +// detached `dws event _bus --client-id ` child and waits for the inherited +// ready pipe. Windows has a named-pipe implementation in spawn_windows.go. // // stdio detach (plan invariant #7): // - cmd.Stdout / cmd.Stderr set to nil so the child's own writes don't @@ -89,7 +90,7 @@ type SpawnConfig struct { // Parent (this function): // - Holds the read end open until either 1 byte is read or ReadyTimeout // - Returns ErrSpawnFailed for 'E', ErrSpawnTimeout otherwise -func Spawn(cfg SpawnConfig) (pid int, err error) { +func spawnWithReadyPipe(cfg SpawnConfig) (pid int, err error) { if cfg.ClientID == "" { return 0, errors.New("busctl: SpawnConfig.ClientID is required") } diff --git a/internal/event/busctl/spawn_test.go b/internal/event/busctl/spawn_test.go index 42a1c532..dc931700 100644 --- a/internal/event/busctl/spawn_test.go +++ b/internal/event/busctl/spawn_test.go @@ -22,6 +22,8 @@ import ( "strings" "testing" "time" + + "github.com/DingTalk-Real-AI/dingtalk-workspace-cli/internal/event/transport" ) // Test child mode: when DWS_BUSCTL_TEST_CHILD is set, this test binary @@ -35,7 +37,8 @@ import ( // env-marker pattern is what Go's own os/exec tests use and stays // confined to this file. const ( - childEnvMarker = "DWS_BUSCTL_TEST_CHILD" + childEnvMarker = "DWS_BUSCTL_TEST_CHILD" + childEndpointEnv = "DWS_BUSCTL_TEST_ENDPOINT" // values: // "ready" — write 'R' then sleep 30s (parent should see ready) // "fail" — write 'E' then exit (parent should see ErrSpawnFailed) @@ -70,6 +73,26 @@ func TestMain(m *testing.M) { writeReady('R') time.Sleep(30 * time.Second) os.Exit(0) + case "windows-ready": + listener, err := transport.Listen(os.Getenv(childEndpointEnv)) + if err != nil { + os.Exit(3) + } + defer listener.Close() + conn, err := listener.Accept() + if err != nil { + os.Exit(4) + } + _ = conn.Close() + time.Sleep(30 * time.Second) + os.Exit(0) + case "windows-fail": + os.Exit(5) + case "windows-exit": + os.Exit(0) + case "windows-stall": + time.Sleep(30 * time.Second) + os.Exit(0) } os.Exit(m.Run()) } diff --git a/internal/event/busctl/spawn_unix.go b/internal/event/busctl/spawn_unix.go index b568c524..d882808c 100644 --- a/internal/event/busctl/spawn_unix.go +++ b/internal/event/busctl/spawn_unix.go @@ -20,6 +20,13 @@ import ( "syscall" ) +// Spawn starts the detached bus and waits for its inherited ready pipe. +// ExtraFiles is intentionally confined to Unix: Go does not support it on +// Windows. +func Spawn(cfg SpawnConfig) (pid int, err error) { + return spawnWithReadyPipe(cfg) +} + // applyDetach configures the child to live past parent death and not share // the parent's controlling terminal. Setsid puts the child in a new // session, so SIGHUP on the parent's controlling tty (e.g. SSH disconnect) diff --git a/internal/event/busctl/spawn_windows.go b/internal/event/busctl/spawn_windows.go index ee0174b1..1cdf6958 100644 --- a/internal/event/busctl/spawn_windows.go +++ b/internal/event/busctl/spawn_windows.go @@ -16,14 +16,101 @@ package busctl import ( + "fmt" + "os" "os/exec" + "strings" "syscall" + "time" + + "github.com/DingTalk-Real-AI/dingtalk-workspace-cli/internal/event/transport" ) // CREATE_NEW_PROCESS_GROUP (0x00000200) prevents the child from receiving // the parent's Ctrl+C signal, similar in spirit to Setsid on Unix. const createNewProcessGroup = 0x00000200 +var ( + spawnWindowsDial = transport.Dial + spawnWindowsPollInterval = 25 * time.Millisecond +) + +// Spawn starts a detached Windows bus without cmd.ExtraFiles (unsupported by +// Go on Windows). Readiness is confirmed by dialing the bus's existing named +// pipe. The child is reaped in the background after readiness, while an early +// process exit is surfaced as ErrSpawnFailed. +func Spawn(cfg SpawnConfig) (pid int, err error) { + if strings.TrimSpace(cfg.ClientID) == "" { + return 0, fmt.Errorf("busctl: SpawnConfig.ClientID is required") + } + if strings.TrimSpace(cfg.IPCEndpoint) == "" { + return 0, fmt.Errorf("busctl: SpawnConfig.IPCEndpoint is required on Windows") + } + if cfg.ExecPath == "" { + execPath, err := spawnExecutable() + if err != nil { + return 0, fmt.Errorf("busctl: locate executable: %w", err) + } + cfg.ExecPath = execPath + } + if cfg.Env == nil { + cfg.Env = os.Environ() + } + + args := append([]string{"event", "_bus", "--client-id", cfg.ClientID}, cfg.ExtraArgs...) + cmd := exec.Command(cfg.ExecPath, args...) + cmd.Env = withoutReadyFDEnv(cfg.Env) + cmd.Stdin = nil + cmd.Stdout = nil + cmd.Stderr = nil + applyDetach(cmd) + if err := cmd.Start(); err != nil { + return 0, fmt.Errorf("busctl: start %s: %w", cfg.ExecPath, err) + } + pid = cmd.Process.Pid + + waitDone := make(chan error, 1) + go func() { waitDone <- cmd.Wait() }() + timer := time.NewTimer(ReadyTimeout) + defer timer.Stop() + ticker := time.NewTicker(spawnWindowsPollInterval) + defer ticker.Stop() + + for { + if conn, dialErr := spawnWindowsDial(cfg.IPCEndpoint); dialErr == nil { + _ = conn.Close() + return pid, nil + } + select { + case waitErr := <-waitDone: + if waitErr == nil { + return pid, fmt.Errorf("%w: child exited before binding named pipe", ErrSpawnFailed) + } + return pid, fmt.Errorf("%w: %v", ErrSpawnFailed, waitErr) + case <-timer.C: + _ = cmd.Process.Kill() + select { + case <-waitDone: + case <-time.After(time.Second): + } + return pid, ErrSpawnTimeout + case <-ticker.C: + } + } +} + +func withoutReadyFDEnv(env []string) []string { + prefix := strings.ToUpper(ReadyFDEnv) + "=" + out := make([]string, 0, len(env)) + for _, entry := range env { + if strings.HasPrefix(strings.ToUpper(entry), prefix) { + continue + } + out = append(out, entry) + } + return out +} + func applyDetach(cmd *exec.Cmd) { cmd.SysProcAttr = &syscall.SysProcAttr{ CreationFlags: createNewProcessGroup, diff --git a/internal/event/busctl/spawn_windows_test.go b/internal/event/busctl/spawn_windows_test.go new file mode 100644 index 00000000..1eeafc4a --- /dev/null +++ b/internal/event/busctl/spawn_windows_test.go @@ -0,0 +1,108 @@ +// 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. + +//go:build windows + +package busctl + +import ( + "errors" + "fmt" + "os" + "strings" + "testing" + "time" + + "github.com/DingTalk-Real-AI/dingtalk-workspace-cli/internal/testseam" +) + +func TestCrossPlatformCoverageWindowsSpawnUsesNamedPipeReadiness(t *testing.T) { + endpoint := fmt.Sprintf(`\\.\pipe\dws-event-spawn-test-%d-%d`, os.Getpid(), time.Now().UnixNano()) + pid, err := spawnWithMarker(t, "windows-ready", func(cfg *SpawnConfig) { + cfg.IPCEndpoint = endpoint + cfg.Env = append(cfg.Env, childEndpointEnv+"="+endpoint) + }) + if err != nil { + t.Fatalf("Spawn() = %v", err) + } + if pid <= 0 { + t.Fatalf("Spawn() pid = %d", pid) + } + proc, err := os.FindProcess(pid) + if err != nil { + t.Fatal(err) + } + _ = proc.Kill() +} + +func TestCrossPlatformCoverageWindowsSpawnReportsEarlyExit(t *testing.T) { + endpoint := fmt.Sprintf(`\\.\pipe\dws-event-spawn-fail-%d-%d`, os.Getpid(), time.Now().UnixNano()) + _, err := spawnWithMarker(t, "windows-fail", func(cfg *SpawnConfig) { + cfg.IPCEndpoint = endpoint + }) + if !errors.Is(err, ErrSpawnFailed) { + t.Fatalf("Spawn() error = %v, want ErrSpawnFailed", err) + } +} + +func TestCrossPlatformCoverageWindowsSpawnReportsCleanEarlyExit(t *testing.T) { + endpoint := fmt.Sprintf(`\\.\pipe\dws-event-spawn-exit-%d-%d`, os.Getpid(), time.Now().UnixNano()) + _, err := spawnWithMarker(t, "windows-exit", func(cfg *SpawnConfig) { + cfg.IPCEndpoint = endpoint + }) + if !errors.Is(err, ErrSpawnFailed) { + t.Fatalf("Spawn() error = %v, want ErrSpawnFailed", err) + } +} + +func TestCrossPlatformCoverageWindowsSpawnValidationAndStartErrors(t *testing.T) { + endpoint := fmt.Sprintf(`\\.\pipe\dws-event-spawn-errors-%d-%d`, os.Getpid(), time.Now().UnixNano()) + if _, err := Spawn(SpawnConfig{ClientID: "client"}); err == nil { + t.Fatal("missing endpoint unexpectedly succeeded") + } + testseam.Swap(t, &spawnExecutable, func() (string, error) { + return "", errors.New("executable unavailable") + }) + if _, err := Spawn(SpawnConfig{ClientID: "client", IPCEndpoint: endpoint}); err == nil { + t.Fatal("executable lookup unexpectedly succeeded") + } + if _, err := Spawn(SpawnConfig{ + ClientID: "client", + IPCEndpoint: endpoint, + ExecPath: `C:\\definitely-missing-dws.exe`, + }); err == nil { + t.Fatal("missing executable unexpectedly started") + } +} + +func TestCrossPlatformCoverageWindowsSpawnReadyTimeoutTerminatesChild(t *testing.T) { + endpoint := fmt.Sprintf(`\\.\pipe\dws-event-spawn-stall-%d-%d`, os.Getpid(), time.Now().UnixNano()) + testseam.Swap(t, &ReadyTimeout, 100*time.Millisecond) + _, err := spawnWithMarker(t, "windows-stall", func(cfg *SpawnConfig) { + cfg.IPCEndpoint = endpoint + }) + if !errors.Is(err, ErrSpawnTimeout) { + t.Fatalf("Spawn() error = %v, want ErrSpawnTimeout", err) + } +} + +func TestCrossPlatformCoverageWindowsSpawnRemovesInheritedReadyFD(t *testing.T) { + env := withoutReadyFDEnv([]string{ + "PATH=C:\\Windows", + ReadyFDEnv + "=3", + strings.ToLower(ReadyFDEnv) + "=4", + }) + if len(env) != 1 || !strings.HasPrefix(env[0], "PATH=") { + t.Fatalf("withoutReadyFDEnv() = %#v", env) + } +} diff --git a/internal/event/busctl/stop.go b/internal/event/busctl/stop.go index 8b9bbdb1..c68ebfcd 100644 --- a/internal/event/busctl/stop.go +++ b/internal/event/busctl/stop.go @@ -17,15 +17,17 @@ import ( "errors" "fmt" "os" + "strings" "time" "github.com/DingTalk-Real-AI/dingtalk-workspace-cli/internal/event/bus" "github.com/DingTalk-Real-AI/dingtalk-workspace-cli/internal/event/process" + "github.com/DingTalk-Real-AI/dingtalk-workspace-cli/internal/event/transport" ) -// DefaultStopTimeout is the wall-clock budget Stop waits for the bus to -// exit after the signal is sent. 5s covers the bus's own graceful tear-down -// (broadcast Bye → consumer goroutines drain → cleanup) with margin. +// DefaultStopTimeout is the wall-clock budget Stop waits for each graceful or +// fallback termination phase. 5s covers the bus's own tear-down (broadcast +// Bye → consumer goroutines drain → cleanup) with margin. const DefaultStopTimeout = 5 * time.Second // ErrNotRunning indicates bus.lock either does not exist or its recorded @@ -38,15 +40,19 @@ var ( stopAlive = process.Alive stopFindProcess = os.FindProcess stopSignalProcess = func(proc *os.Process, signal os.Signal) error { return proc.Signal(signal) } + stopRequest = requestBusStop ) // StopConfig identifies the target bus and tunes timing. type StopConfig struct { // WorkDir holds bus.lock; Stop reads the PID from there. WorkDir string - // Timeout is the total wall-clock budget for graceful exit. After this, - // Stop returns an error; it does NOT escalate to SIGKILL — leave that - // to the operator. + // IPCEndpoint enables the cross-platform graceful stop RPC. Callers that + // know the bus identity should always provide it. Empty preserves the + // legacy signal-only fallback for compatibility and focused tests. + IPCEndpoint string + // Timeout is the wall-clock budget for graceful exit and, if required, + // the subsequent platform termination fallback. Timeout time.Duration } @@ -54,13 +60,12 @@ type StopConfig struct { // for the process to actually die. Returns ErrNotRunning if no bus is // running for that work dir. // -// Implementation note: on Unix we send SIGTERM. The bus daemon's Run loop -// watches its parent ctx for cancellation; the cobra `event _bus` command -// wires signal.NotifyContext so SIGTERM triggers ctx.Done() → graceful -// shutdown path. On Windows we use os.Process.Signal(os.Interrupt) which -// the Go runtime maps to TerminateProcess for processes outside our -// console group; for v1 that's acceptable (Windows graceful shutdown is -// future work — plan §16 v2). +// Stop first asks the bus to shut down through its owner-only IPC endpoint. +// This is the normal path on every platform and lets the daemon cancel its +// cloud source, notify consumers, and release its lock. If the endpoint is +// unavailable (for example an older bus) or graceful shutdown times out, the +// platform signal is used as a compatibility fallback: SIGTERM on Unix and +// TerminateProcess via os.Kill on Windows. func Stop(cfg StopConfig) error { if cfg.WorkDir == "" { return errors.New("busctl: StopConfig.WorkDir is required") @@ -81,6 +86,12 @@ func Stop(cfg StopConfig) error { if err != nil { return fmt.Errorf("busctl: find process %d: %w", pid, err) } + if strings.TrimSpace(cfg.IPCEndpoint) != "" { + if err := stopRequest(cfg.IPCEndpoint); err == nil && waitForBusExit(pid, cfg.Timeout) { + return nil + } + } + if err := stopSignalProcess(proc, stopSignal()); err != nil { // On many Unix platforms Signal returns "process already finished" // when the bus has just exited on its own — treat that as success. @@ -90,13 +101,45 @@ func Stop(cfg StopConfig) error { return fmt.Errorf("busctl: signal bus pid=%d: %w", pid, err) } - // Poll for actual exit. - deadline := time.Now().Add(cfg.Timeout) - for time.Now().Before(deadline) { - if !stopAlive(pid) { - return nil - } - time.Sleep(50 * time.Millisecond) + if waitForBusExit(pid, cfg.Timeout) { + return nil } return fmt.Errorf("busctl: bus pid=%d did not exit within %s", pid, cfg.Timeout) } + +func requestBusStop(endpoint string) error { + conn, err := transport.Dial(endpoint) + if err != nil { + return fmt.Errorf("busctl: dial bus for stop: %w", err) + } + defer conn.Close() + _ = conn.SetDeadline(time.Now().Add(DefaultStatusRPCTimeout)) + w := transport.NewWriter(conn) + r := transport.NewReader(conn) + if err := w.WriteJSON(transport.Hello{ + Type: transport.FrameTypeHello, + ConsumerPID: os.Getpid(), + Role: transport.HelloRoleStop, + }); err != nil { + return fmt.Errorf("busctl: write stop hello: %w", err) + } + var bye transport.Bye + if err := r.ReadJSON(&bye); err != nil { + return fmt.Errorf("busctl: read stop response: %w", err) + } + if bye.Type != transport.FrameTypeBye || bye.Reason != "stop_request" { + return fmt.Errorf("busctl: unexpected stop response type=%q reason=%q", bye.Type, bye.Reason) + } + return nil +} + +func waitForBusExit(pid int, timeout time.Duration) bool { + deadline := time.Now().Add(timeout) + for time.Now().Before(deadline) { + if !stopAlive(pid) { + return true + } + time.Sleep(50 * time.Millisecond) + } + return !stopAlive(pid) +} diff --git a/internal/event/busctl/stop_test.go b/internal/event/busctl/stop_test.go index e4cd99de..2a80d01e 100644 --- a/internal/event/busctl/stop_test.go +++ b/internal/event/busctl/stop_test.go @@ -23,7 +23,10 @@ import ( "testing" "time" + dwsevent "github.com/DingTalk-Real-AI/dingtalk-workspace-cli/internal/event" "github.com/DingTalk-Real-AI/dingtalk-workspace-cli/internal/event/bus" + "github.com/DingTalk-Real-AI/dingtalk-workspace-cli/internal/event/transport" + "github.com/DingTalk-Real-AI/dingtalk-workspace-cli/internal/testseam" ) func TestStop_NotRunningWhenLockMissing(t *testing.T) { @@ -46,6 +49,87 @@ func TestStop_NotRunningWhenPIDDead(t *testing.T) { } } +func TestCrossPlatformCoverageStopPrefersGracefulIPC(t *testing.T) { + const pid = 4242 + stopped := false + signals := 0 + testseam.Swap(t, &stopReadHolderPID, func(string) int { return pid }) + testseam.Swap(t, &stopAlive, func(int) bool { return !stopped }) + proc, err := os.FindProcess(os.Getpid()) + if err != nil { + t.Fatal(err) + } + testseam.Swap(t, &stopFindProcess, func(int) (*os.Process, error) { return proc, nil }) + testseam.Swap(t, &stopRequest, func(endpoint string) error { + if endpoint != "test-endpoint" { + t.Fatalf("stop endpoint = %q", endpoint) + } + stopped = true + return nil + }) + testseam.Swap(t, &stopSignalProcess, func(*os.Process, os.Signal) error { + signals++ + return nil + }) + + if err := Stop(StopConfig{ + WorkDir: "test-workdir", + IPCEndpoint: "test-endpoint", + Timeout: time.Second, + }); err != nil { + t.Fatalf("Stop() = %v", err) + } + if signals != 0 { + t.Fatalf("graceful IPC stop used %d process signals", signals) + } +} + +func TestCrossPlatformCoverageRequestBusStopProtocol(t *testing.T) { + dir := shortTempDir(t) + endpoint := dwsevent.IPCEndpoint( + dir, + "open", + dwsevent.SourceKindAppStream, + dwsevent.IdentityHash(dir), + ) + listener, err := transport.Listen(endpoint) + if err != nil { + t.Fatalf("Listen() = %v", err) + } + defer listener.Close() + serverDone := make(chan error, 1) + go func() { + conn, err := listener.Accept() + if err != nil { + serverDone <- err + return + } + defer conn.Close() + r := transport.NewReader(conn) + w := transport.NewWriter(conn) + var hello transport.Hello + if err := r.ReadJSON(&hello); err != nil { + serverDone <- err + return + } + if hello.Type != transport.FrameTypeHello || hello.Role != transport.HelloRoleStop { + serverDone <- errors.New("unexpected stop hello") + return + } + serverDone <- w.WriteJSON(transport.Bye{ + Type: transport.FrameTypeBye, + Reason: "stop_request", + }) + }() + + if err := requestBusStop(endpoint); err != nil { + t.Fatalf("requestBusStop() = %v", err) + } + if err := <-serverDone; err != nil { + t.Fatalf("stop protocol server = %v", err) + } +} + func TestStop_SignalsLiveProcess(t *testing.T) { skipOnWindows(t) dir := shortTempDir(t) diff --git a/internal/event/busctl/stop_windows.go b/internal/event/busctl/stop_windows.go index 2e7223f6..630ee330 100644 --- a/internal/event/busctl/stop_windows.go +++ b/internal/event/busctl/stop_windows.go @@ -17,8 +17,7 @@ package busctl import "os" -// stopSignal returns the graceful-shutdown signal for Windows. The Go -// runtime maps os.Interrupt to TerminateProcess for non-console-group -// processes — not truly graceful, but acceptable for v1 (Windows graceful -// shutdown via Ctrl+Break is in the v2 backlog, plan §16). -func stopSignal() os.Signal { return os.Interrupt } +// stopSignal is only the fallback after the graceful IPC stop path fails or +// times out. Go maps os.Kill to TerminateProcess on Windows; os.Interrupt is +// unsupported and returns syscall.EWINDOWS. +func stopSignal() os.Signal { return os.Kill } diff --git a/internal/event/busctl/stop_windows_test.go b/internal/event/busctl/stop_windows_test.go new file mode 100644 index 00000000..fcdc94aa --- /dev/null +++ b/internal/event/busctl/stop_windows_test.go @@ -0,0 +1,27 @@ +// 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. + +//go:build windows + +package busctl + +import ( + "os" + "testing" +) + +func TestCrossPlatformCoverageWindowsStopFallbackUsesKill(t *testing.T) { + if stopSignal() != os.Kill { + t.Fatalf("stopSignal() = %v, want os.Kill", stopSignal()) + } +} From f68a11f11db452d34ad8876e8b7e414c63632c6a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E7=82=B3=E6=98=B1?= Date: Fri, 14 Aug 2026 18:05:34 +0800 Subject: [PATCH 07/32] test(event): cover Windows lifecycle edges --- internal/event/busctl/spawn_windows_test.go | 18 ++++++ internal/event/busctl/stop.go | 3 +- internal/event/busctl/stop_test.go | 61 ++++++++++++++++++++- 3 files changed, 80 insertions(+), 2 deletions(-) diff --git a/internal/event/busctl/spawn_windows_test.go b/internal/event/busctl/spawn_windows_test.go index 1eeafc4a..22d38be2 100644 --- a/internal/event/busctl/spawn_windows_test.go +++ b/internal/event/busctl/spawn_windows_test.go @@ -65,6 +65,24 @@ func TestCrossPlatformCoverageWindowsSpawnReportsCleanEarlyExit(t *testing.T) { } } +func TestCrossPlatformCoverageWindowsSpawnResolvesExecutable(t *testing.T) { + endpoint := fmt.Sprintf(`\\.\pipe\dws-event-spawn-resolve-%d-%d`, os.Getpid(), time.Now().UnixNano()) + testseam.Swap(t, &spawnExecutable, func() (string, error) { return os.Args[0], nil }) + pid, err := spawnWithMarker(t, "windows-ready", func(cfg *SpawnConfig) { + cfg.ExecPath = "" + cfg.IPCEndpoint = endpoint + cfg.Env = append(cfg.Env, childEndpointEnv+"="+endpoint) + }) + if err != nil { + t.Fatalf("Spawn() = %v", err) + } + proc, err := os.FindProcess(pid) + if err != nil { + t.Fatal(err) + } + _ = proc.Kill() +} + func TestCrossPlatformCoverageWindowsSpawnValidationAndStartErrors(t *testing.T) { endpoint := fmt.Sprintf(`\\.\pipe\dws-event-spawn-errors-%d-%d`, os.Getpid(), time.Now().UnixNano()) if _, err := Spawn(SpawnConfig{ClientID: "client"}); err == nil { diff --git a/internal/event/busctl/stop.go b/internal/event/busctl/stop.go index c68ebfcd..c3349492 100644 --- a/internal/event/busctl/stop.go +++ b/internal/event/busctl/stop.go @@ -40,6 +40,7 @@ var ( stopAlive = process.Alive stopFindProcess = os.FindProcess stopSignalProcess = func(proc *os.Process, signal os.Signal) error { return proc.Signal(signal) } + stopDial = transport.Dial stopRequest = requestBusStop ) @@ -108,7 +109,7 @@ func Stop(cfg StopConfig) error { } func requestBusStop(endpoint string) error { - conn, err := transport.Dial(endpoint) + conn, err := stopDial(endpoint) if err != nil { return fmt.Errorf("busctl: dial bus for stop: %w", err) } diff --git a/internal/event/busctl/stop_test.go b/internal/event/busctl/stop_test.go index 2a80d01e..a3ae31f5 100644 --- a/internal/event/busctl/stop_test.go +++ b/internal/event/busctl/stop_test.go @@ -16,6 +16,7 @@ package busctl import ( "context" "errors" + "net" "os" "os/exec" "path/filepath" @@ -37,7 +38,7 @@ func TestStop_NotRunningWhenLockMissing(t *testing.T) { } } -func TestStop_NotRunningWhenPIDDead(t *testing.T) { +func TestCrossPlatformCoverageStopNotRunningWhenPIDDead(t *testing.T) { dir := shortTempDir(t) // Write a definitely-dead PID into bus.lock. if err := os.WriteFile(LockPath(dir), []byte("2147483646\n"), 0o600); err != nil { @@ -130,6 +131,64 @@ func TestCrossPlatformCoverageRequestBusStopProtocol(t *testing.T) { } } +func TestCrossPlatformCoverageRequestBusStopErrors(t *testing.T) { + t.Run("dial", func(t *testing.T) { + testseam.Swap(t, &stopDial, func(string) (net.Conn, error) { + return nil, errors.New("dial failed") + }) + if err := requestBusStop("test-endpoint"); err == nil { + t.Fatal("requestBusStop() unexpectedly succeeded") + } + }) + + t.Run("write", func(t *testing.T) { + testseam.Swap(t, &stopDial, func(string) (net.Conn, error) { + return &queryErrorConn{failAt: 1}, nil + }) + if err := requestBusStop("test-endpoint"); err == nil { + t.Fatal("requestBusStop() unexpectedly succeeded") + } + }) + + t.Run("read", func(t *testing.T) { + testseam.Swap(t, &stopDial, func(string) (net.Conn, error) { + return &queryErrorConn{}, nil + }) + if err := requestBusStop("test-endpoint"); err == nil { + t.Fatal("requestBusStop() unexpectedly succeeded") + } + }) + + t.Run("unexpected response", func(t *testing.T) { + client, server := net.Pipe() + t.Cleanup(func() { + _ = client.Close() + _ = server.Close() + }) + testseam.Swap(t, &stopDial, func(string) (net.Conn, error) { + return client, nil + }) + serverDone := make(chan error, 1) + go func() { + var hello transport.Hello + if err := transport.NewReader(server).ReadJSON(&hello); err != nil { + serverDone <- err + return + } + serverDone <- transport.NewWriter(server).WriteJSON(transport.Bye{ + Type: transport.FrameTypeBye, + Reason: "unexpected", + }) + }() + if err := requestBusStop("test-endpoint"); err == nil { + t.Fatal("requestBusStop() unexpectedly succeeded") + } + if err := <-serverDone; err != nil { + t.Fatalf("stop protocol server = %v", err) + } + }) +} + func TestStop_SignalsLiveProcess(t *testing.T) { skipOnWindows(t) dir := shortTempDir(t) From 22649e96ef45c7766b7b99c8fa7b2ddd32f47075 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E7=82=B3=E6=98=B1?= Date: Fri, 14 Aug 2026 18:11:42 +0800 Subject: [PATCH 08/32] test(event): cover Unix spawn validation on Windows --- internal/event/busctl/spawn_windows_test.go | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/internal/event/busctl/spawn_windows_test.go b/internal/event/busctl/spawn_windows_test.go index 22d38be2..c133f08b 100644 --- a/internal/event/busctl/spawn_windows_test.go +++ b/internal/event/busctl/spawn_windows_test.go @@ -124,3 +124,9 @@ func TestCrossPlatformCoverageWindowsSpawnRemovesInheritedReadyFD(t *testing.T) t.Fatalf("withoutReadyFDEnv() = %#v", env) } } + +func TestCrossPlatformCoverageWindowsUnixReadyPipeValidation(t *testing.T) { + if _, err := spawnWithReadyPipe(SpawnConfig{}); err == nil { + t.Fatal("spawnWithReadyPipe() unexpectedly accepted an empty client ID") + } +} From 353454abb28a0ed778ec709b3b6ab559328a316e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E7=82=B3=E6=98=B1?= Date: Mon, 17 Aug 2026 10:32:04 +0800 Subject: [PATCH 09/32] fix(event): verify bus owner before fallback stop --- internal/event/bus/lockfile.go | 41 ++++++++++++++++ internal/event/bus/lockfile_test.go | 51 ++++++++++++++++++++ internal/event/busctl/coverage_edges_test.go | 3 ++ internal/event/busctl/stop.go | 26 +++++++--- internal/event/busctl/stop_test.go | 37 ++++++++++++++ 5 files changed, 152 insertions(+), 6 deletions(-) diff --git a/internal/event/bus/lockfile.go b/internal/event/bus/lockfile.go index 0349a8c9..40dbaaee 100644 --- a/internal/event/bus/lockfile.go +++ b/internal/event/bus/lockfile.go @@ -114,6 +114,47 @@ func ReadHolderPID(path string) int { return parsePID(b) } +// ValidateHolderOwnership reports whether pid still owns the exclusive lock at +// path. If the lock can be acquired, no daemon owns it: the PID payload is +// stale and is cleared while the probe holds the lock. The PID is read again +// after a busy result so a daemon replacement cannot be mistaken for the +// original owner. +// +// Callers use this immediately before sending a process-level termination +// fallback. A live PID by itself is not proof of ownership because operating +// systems may reuse PIDs after an unclean daemon exit. +func ValidateHolderOwnership(path string, pid int) (bool, error) { + if pid <= 0 || ReadHolderPID(path) != pid { + return false, nil + } + + probe, err := busTryAcquire(path) + if err != nil { + if errors.Is(err, ErrBusy) { + return ReadHolderPID(path) == pid, nil + } + return false, fmt.Errorf("bus: verify lock owner: %w", err) + } + defer probe.Close() + + // We own the lock, so the recorded PID cannot own it. Clear only the PID + // we validated above; a changed payload is left untouched defensively. + f := probe.File() + if _, err := busSeek(f, 0, io.SeekStart); err != nil { + return false, fmt.Errorf("bus: seek stale lock: %w", err) + } + current, err := busReadAll(f) + if err != nil { + return false, fmt.Errorf("bus: read stale lock: %w", err) + } + if parsePID(current) == pid { + if err := truncateAndWritePID(f, 0); err != nil { + return false, fmt.Errorf("bus: clear stale PID: %w", err) + } + } + return false, nil +} + // Close releases the flock and best-effort blanks the PID body so a stale // reader (e.g. `event status` racing our shutdown) does not see our // long-dead PID and try to signal it. The lock file itself is NOT removed diff --git a/internal/event/bus/lockfile_test.go b/internal/event/bus/lockfile_test.go index 6ec4e86d..d36e7e8a 100644 --- a/internal/event/bus/lockfile_test.go +++ b/internal/event/bus/lockfile_test.go @@ -112,6 +112,57 @@ func TestReadHolderPID_MalformedReturnsZero(t *testing.T) { } } +func TestCrossPlatformCoverageValidateHolderOwnershipHeldLock(t *testing.T) { + path := filepath.Join(t.TempDir(), LockFileName) + held, err := busTryAcquire(path) + if err != nil { + t.Fatalf("hold lock: %v", err) + } + defer held.Close() + if err := truncateAndWritePID(held.File(), os.Getpid()); err != nil { + t.Fatalf("write holder PID: %v", err) + } + + owned, err := ValidateHolderOwnership(path, os.Getpid()) + if err != nil { + t.Fatalf("ValidateHolderOwnership: %v", err) + } + if !owned { + t.Fatal("held lock was not attributed to its recorded PID") + } +} + +func TestCrossPlatformCoverageValidateHolderOwnershipClearsReusedPID(t *testing.T) { + path := filepath.Join(t.TempDir(), LockFileName) + // The current PID is alive but deliberately does not hold this lock. This + // models a stale daemon PID that the OS has reassigned to another process. + if err := os.WriteFile(path, []byte(fmt.Sprintf("%d\n", os.Getpid())), 0o600); err != nil { + t.Fatalf("write stale PID: %v", err) + } + + owned, err := ValidateHolderOwnership(path, os.Getpid()) + if err != nil { + t.Fatalf("ValidateHolderOwnership: %v", err) + } + if owned { + t.Fatal("live reused PID without the bus lock was accepted as owner") + } + if got := ReadHolderPID(path); got != 0 { + t.Fatalf("stale PID after validation = %d, want cleared", got) + } +} + +func TestCrossPlatformCoverageValidateHolderOwnershipRejectsMismatch(t *testing.T) { + path := filepath.Join(t.TempDir(), LockFileName) + if err := os.WriteFile(path, []byte("123\n"), 0o600); err != nil { + t.Fatal(err) + } + owned, err := ValidateHolderOwnership(path, 456) + if err != nil || owned { + t.Fatalf("ValidateHolderOwnership mismatch = %v, %v", owned, err) + } +} + func TestAcquire_AfterReleaseReclaimable(t *testing.T) { path := filepath.Join(t.TempDir(), LockFileName) l1, err := Acquire(path) diff --git a/internal/event/busctl/coverage_edges_test.go b/internal/event/busctl/coverage_edges_test.go index f84ed54e..09de121c 100644 --- a/internal/event/busctl/coverage_edges_test.go +++ b/internal/event/busctl/coverage_edges_test.go @@ -284,11 +284,13 @@ func TestCrossPlatformCoverageQueryStatusAndEntryEdges(t *testing.T) { func TestCrossPlatformCoverageStopInjectedEdges(t *testing.T) { origRead := stopReadHolderPID + origValidateOwner := stopValidateHolderOwner origAlive := stopAlive origFind := stopFindProcess origSignal := stopSignalProcess t.Cleanup(func() { stopReadHolderPID = origRead + stopValidateHolderOwner = origValidateOwner stopAlive = origAlive stopFindProcess = origFind stopSignalProcess = origSignal @@ -307,6 +309,7 @@ func TestCrossPlatformCoverageStopInjectedEdges(t *testing.T) { t.Fatal(err) } stopFindProcess = func(int) (*os.Process, error) { return proc, nil } + stopValidateHolderOwner = func(string, int) (bool, error) { return true, nil } stopSignalProcess = func(*os.Process, os.Signal) error { return os.ErrProcessDone } if err := Stop(StopConfig{WorkDir: "x"}); err != nil { t.Fatalf("done signal = %v", err) diff --git a/internal/event/busctl/stop.go b/internal/event/busctl/stop.go index c3349492..6748b516 100644 --- a/internal/event/busctl/stop.go +++ b/internal/event/busctl/stop.go @@ -35,13 +35,19 @@ const DefaultStopTimeout = 5 * time.Second // distinguish "nothing to stop" from "failed to stop". var ErrNotRunning = errors.New("busctl: bus is not running") +// ErrOwnerUnverified indicates the PID from bus.lock is alive but no longer +// owns that lock. Stop refuses to send a process-level signal in this state +// because the operating system may have reused the stale PID. +var ErrOwnerUnverified = errors.New("busctl: bus process ownership could not be verified") + var ( - stopReadHolderPID = bus.ReadHolderPID - stopAlive = process.Alive - stopFindProcess = os.FindProcess - stopSignalProcess = func(proc *os.Process, signal os.Signal) error { return proc.Signal(signal) } - stopDial = transport.Dial - stopRequest = requestBusStop + stopReadHolderPID = bus.ReadHolderPID + stopValidateHolderOwner = bus.ValidateHolderOwnership + stopAlive = process.Alive + stopFindProcess = os.FindProcess + stopSignalProcess = func(proc *os.Process, signal os.Signal) error { return proc.Signal(signal) } + stopDial = transport.Dial + stopRequest = requestBusStop ) // StopConfig identifies the target bus and tunes timing. @@ -93,6 +99,14 @@ func Stop(cfg StopConfig) error { } } + owner, err := stopValidateHolderOwner(LockPath(cfg.WorkDir), pid) + if err != nil { + return fmt.Errorf("busctl: verify bus pid=%d ownership: %w", pid, err) + } + if !owner { + return fmt.Errorf("%w: pid=%d does not own %s; stale PID was not signalled", ErrOwnerUnverified, pid, LockPath(cfg.WorkDir)) + } + if err := stopSignalProcess(proc, stopSignal()); err != nil { // On many Unix platforms Signal returns "process already finished" // when the bus has just exited on its own — treat that as success. diff --git a/internal/event/busctl/stop_test.go b/internal/event/busctl/stop_test.go index a3ae31f5..10d481f2 100644 --- a/internal/event/busctl/stop_test.go +++ b/internal/event/busctl/stop_test.go @@ -85,6 +85,41 @@ func TestCrossPlatformCoverageStopPrefersGracefulIPC(t *testing.T) { } } +func TestCrossPlatformCoverageStopDoesNotSignalUnverifiedReusedPID(t *testing.T) { + const pid = 4242 + testseam.Swap(t, &stopReadHolderPID, func(string) int { return pid }) + testseam.Swap(t, &stopAlive, func(int) bool { return true }) + proc, err := os.FindProcess(os.Getpid()) + if err != nil { + t.Fatal(err) + } + testseam.Swap(t, &stopFindProcess, func(int) (*os.Process, error) { return proc, nil }) + testseam.Swap(t, &stopRequest, func(string) error { return errors.New("stale endpoint") }) + testseam.Swap(t, &stopValidateHolderOwner, func(path string, gotPID int) (bool, error) { + if path != LockPath("test-workdir") || gotPID != pid { + t.Fatalf("ownership check path=%q pid=%d", path, gotPID) + } + return false, nil + }) + signals := 0 + testseam.Swap(t, &stopSignalProcess, func(*os.Process, os.Signal) error { + signals++ + return nil + }) + + err = Stop(StopConfig{ + WorkDir: "test-workdir", + IPCEndpoint: "stale-endpoint", + Timeout: time.Millisecond, + }) + if !errors.Is(err, ErrOwnerUnverified) { + t.Fatalf("Stop() error = %v, want ErrOwnerUnverified", err) + } + if signals != 0 { + t.Fatalf("unverified reused PID received %d signals", signals) + } +} + func TestCrossPlatformCoverageRequestBusStopProtocol(t *testing.T) { dir := shortTempDir(t) endpoint := dwsevent.IPCEndpoint( @@ -206,6 +241,7 @@ func TestStop_SignalsLiveProcess(t *testing.T) { } }() pid := cmd.Process.Pid + testseam.Swap(t, &stopValidateHolderOwner, func(string, int) (bool, error) { return true, nil }) // Reap the child in background so Wait doesn't leave a zombie. waited := make(chan error, 1) @@ -254,6 +290,7 @@ func TestStop_TimeoutWhenChildIgnoresSignal(t *testing.T) { _, _ = cmd.Process.Wait() }() pid := cmd.Process.Pid + testseam.Swap(t, &stopValidateHolderOwner, func(string, int) (bool, error) { return true, nil }) if err := os.WriteFile(LockPath(dir), []byte(strconv.Itoa(pid)+"\n"), 0o600); err != nil { t.Fatal(err) From e3553fe7a5f0d659d5a8ee04f0689feeebe68ad7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E7=82=B3=E6=98=B1?= Date: Mon, 17 Aug 2026 10:40:21 +0800 Subject: [PATCH 10/32] test(event): cover bus ownership validation failures --- internal/event/bus/lockfile_test.go | 56 +++++++++++++++++++++++++++++ internal/event/busctl/stop_test.go | 28 +++++++++++++++ 2 files changed, 84 insertions(+) diff --git a/internal/event/bus/lockfile_test.go b/internal/event/bus/lockfile_test.go index d36e7e8a..f5b9a29a 100644 --- a/internal/event/bus/lockfile_test.go +++ b/internal/event/bus/lockfile_test.go @@ -16,9 +16,13 @@ package bus import ( "errors" "fmt" + "io" "os" "path/filepath" "testing" + + eventlock "github.com/DingTalk-Real-AI/dingtalk-workspace-cli/internal/event/lock" + "github.com/DingTalk-Real-AI/dingtalk-workspace-cli/internal/testseam" ) func TestAcquire_WritesOurPID(t *testing.T) { @@ -163,6 +167,58 @@ func TestCrossPlatformCoverageValidateHolderOwnershipRejectsMismatch(t *testing. } } +func TestCrossPlatformCoverageValidateHolderOwnershipErrors(t *testing.T) { + errInjected := errors.New("injected ownership validation failure") + writeLivePID := func(t *testing.T) string { + t.Helper() + path := filepath.Join(t.TempDir(), LockFileName) + if err := os.WriteFile(path, []byte(fmt.Sprintf("%d\n", os.Getpid())), 0o600); err != nil { + t.Fatal(err) + } + return path + } + + t.Run("probe", func(t *testing.T) { + path := writeLivePID(t) + testseam.Swap(t, &busTryAcquire, func(string) (*eventlock.File, error) { + return nil, errInjected + }) + if _, err := ValidateHolderOwnership(path, os.Getpid()); !errors.Is(err, errInjected) { + t.Fatalf("probe error = %v", err) + } + }) + + t.Run("seek", func(t *testing.T) { + path := writeLivePID(t) + testseam.Swap(t, &busSeek, func(*os.File, int64, int) (int64, error) { + return 0, errInjected + }) + if _, err := ValidateHolderOwnership(path, os.Getpid()); !errors.Is(err, errInjected) { + t.Fatalf("seek error = %v", err) + } + }) + + t.Run("read", func(t *testing.T) { + path := writeLivePID(t) + testseam.Swap(t, &busReadAll, func(io.Reader) ([]byte, error) { + return nil, errInjected + }) + if _, err := ValidateHolderOwnership(path, os.Getpid()); !errors.Is(err, errInjected) { + t.Fatalf("read error = %v", err) + } + }) + + t.Run("clear", func(t *testing.T) { + path := writeLivePID(t) + testseam.Swap(t, &busTruncate, func(*os.File, int64) error { + return errInjected + }) + if _, err := ValidateHolderOwnership(path, os.Getpid()); !errors.Is(err, errInjected) { + t.Fatalf("clear error = %v", err) + } + }) +} + func TestAcquire_AfterReleaseReclaimable(t *testing.T) { path := filepath.Join(t.TempDir(), LockFileName) l1, err := Acquire(path) diff --git a/internal/event/busctl/stop_test.go b/internal/event/busctl/stop_test.go index 10d481f2..5c086c86 100644 --- a/internal/event/busctl/stop_test.go +++ b/internal/event/busctl/stop_test.go @@ -120,6 +120,34 @@ func TestCrossPlatformCoverageStopDoesNotSignalUnverifiedReusedPID(t *testing.T) } } +func TestCrossPlatformCoverageStopReportsOwnershipValidationError(t *testing.T) { + const pid = 4242 + errInjected := errors.New("ownership validation failed") + testseam.Swap(t, &stopReadHolderPID, func(string) int { return pid }) + testseam.Swap(t, &stopAlive, func(int) bool { return true }) + proc, err := os.FindProcess(os.Getpid()) + if err != nil { + t.Fatal(err) + } + testseam.Swap(t, &stopFindProcess, func(int) (*os.Process, error) { return proc, nil }) + testseam.Swap(t, &stopValidateHolderOwner, func(string, int) (bool, error) { + return false, errInjected + }) + signals := 0 + testseam.Swap(t, &stopSignalProcess, func(*os.Process, os.Signal) error { + signals++ + return nil + }) + + err = Stop(StopConfig{WorkDir: "test-workdir"}) + if !errors.Is(err, errInjected) { + t.Fatalf("Stop() error = %v, want injected ownership error", err) + } + if signals != 0 { + t.Fatalf("ownership validation error sent %d signals", signals) + } +} + func TestCrossPlatformCoverageRequestBusStopProtocol(t *testing.T) { dir := shortTempDir(t) endpoint := dwsevent.IPCEndpoint( From 7ad82bbf0a72dac4937d2369ce82d853c98b3608 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E7=82=B3=E6=98=B1?= Date: Mon, 17 Aug 2026 13:00:07 +0800 Subject: [PATCH 11/32] fix(event): accept bus exit at stop timeout boundary --- internal/event/busctl/stop.go | 17 ++++++++- internal/event/busctl/stop_test.go | 61 ++++++++++++++++++++++++++++++ 2 files changed, 76 insertions(+), 2 deletions(-) diff --git a/internal/event/busctl/stop.go b/internal/event/busctl/stop.go index 6748b516..8779a94f 100644 --- a/internal/event/busctl/stop.go +++ b/internal/event/busctl/stop.go @@ -48,6 +48,7 @@ var ( stopSignalProcess = func(proc *os.Process, signal os.Signal) error { return proc.Signal(signal) } stopDial = transport.Dial stopRequest = requestBusStop + stopWaitForBusExit = waitForBusExit ) // StopConfig identifies the target bus and tunes timing. @@ -94,16 +95,28 @@ func Stop(cfg StopConfig) error { return fmt.Errorf("busctl: find process %d: %w", pid, err) } if strings.TrimSpace(cfg.IPCEndpoint) != "" { - if err := stopRequest(cfg.IPCEndpoint); err == nil && waitForBusExit(pid, cfg.Timeout) { + if err := stopRequest(cfg.IPCEndpoint); err == nil && stopWaitForBusExit(pid, cfg.Timeout) { return nil } } + // The bus may finish shutting down immediately after the graceful wait + // reaches its deadline (or after a failed IPC request). Recheck before + // inspecting ownership so a completed stop is not reported as stale. + if !stopAlive(pid) { + return nil + } owner, err := stopValidateHolderOwner(LockPath(cfg.WorkDir), pid) if err != nil { return fmt.Errorf("busctl: verify bus pid=%d ownership: %w", pid, err) } if !owner { + // ValidateHolderOwnership observes a released lock both for a stale, + // reused PID and for a bus that exits during the ownership probe. Only + // the still-alive case is unsafe to signal. + if !stopAlive(pid) { + return nil + } return fmt.Errorf("%w: pid=%d does not own %s; stale PID was not signalled", ErrOwnerUnverified, pid, LockPath(cfg.WorkDir)) } @@ -116,7 +129,7 @@ func Stop(cfg StopConfig) error { return fmt.Errorf("busctl: signal bus pid=%d: %w", pid, err) } - if waitForBusExit(pid, cfg.Timeout) { + if stopWaitForBusExit(pid, cfg.Timeout) { return nil } return fmt.Errorf("busctl: bus pid=%d did not exit within %s", pid, cfg.Timeout) diff --git a/internal/event/busctl/stop_test.go b/internal/event/busctl/stop_test.go index 5c086c86..8154dd64 100644 --- a/internal/event/busctl/stop_test.go +++ b/internal/event/busctl/stop_test.go @@ -120,6 +120,67 @@ func TestCrossPlatformCoverageStopDoesNotSignalUnverifiedReusedPID(t *testing.T) } } +func TestCrossPlatformCoverageStopAcceptsExitAtGracefulTimeoutBoundary(t *testing.T) { + const pid = 4242 + + t.Run("before ownership validation", func(t *testing.T) { + exited := false + validated := false + testseam.Swap(t, &stopReadHolderPID, func(string) int { return pid }) + testseam.Swap(t, &stopAlive, func(int) bool { return !exited }) + proc, err := os.FindProcess(os.Getpid()) + if err != nil { + t.Fatal(err) + } + testseam.Swap(t, &stopFindProcess, func(int) (*os.Process, error) { return proc, nil }) + testseam.Swap(t, &stopRequest, func(string) error { return nil }) + testseam.Swap(t, &stopWaitForBusExit, func(int, time.Duration) bool { + exited = true + return false + }) + testseam.Swap(t, &stopValidateHolderOwner, func(string, int) (bool, error) { + validated = true + return false, nil + }) + + if err := Stop(StopConfig{WorkDir: "test-workdir", IPCEndpoint: "test-endpoint"}); err != nil { + t.Fatalf("Stop() = %v, want success after bus exit", err) + } + if validated { + t.Fatal("ownership was validated after the bus had already exited") + } + }) + + t.Run("during ownership validation", func(t *testing.T) { + exited := false + signals := 0 + testseam.Swap(t, &stopReadHolderPID, func(string) int { return pid }) + testseam.Swap(t, &stopAlive, func(int) bool { return !exited }) + proc, err := os.FindProcess(os.Getpid()) + if err != nil { + t.Fatal(err) + } + testseam.Swap(t, &stopFindProcess, func(int) (*os.Process, error) { return proc, nil }) + testseam.Swap(t, &stopRequest, func(string) error { return nil }) + testseam.Swap(t, &stopWaitForBusExit, func(int, time.Duration) bool { return false }) + testseam.Swap(t, &stopValidateHolderOwner, func(string, int) (bool, error) { + exited = true + return false, nil + }) + testseam.Swap(t, &stopSignalProcess, func(*os.Process, os.Signal) error { + signals++ + return nil + }) + + if err := Stop(StopConfig{WorkDir: "test-workdir", IPCEndpoint: "test-endpoint"}); err != nil { + t.Fatalf("Stop() = %v, want success after exit during ownership validation", err) + } + if signals != 0 { + t.Fatalf("exited bus received %d fallback signals", signals) + } + }) +} + func TestCrossPlatformCoverageStopReportsOwnershipValidationError(t *testing.T) { const pid = 4242 errInjected := errors.New("ownership validation failed") From fe2f3ca92f92cda08453da4b7bf05e9f5430c4a2 Mon Sep 17 00:00:00 2001 From: Dennis Date: Fri, 14 Aug 2026 11:57:41 +0800 Subject: [PATCH 12/32] fix: harden aitable pagination and minutes unshare --- internal/app/param_alias_e2e_test.go | 2 + internal/shortcut/aitable/aitable.go | 2 +- internal/shortcut/aitable/record_batch.go | 45 +++-- internal/shortcut/aitable/record_query_all.go | 127 +++++++++++++- .../aitable/record_upsert_by_key_e2e_test.go | 164 +++++++++++++++++- internal/shortcut/minutes/workflows.go | 17 +- .../minutes/workflows_coverage_test.go | 21 ++- .../minutesdata/coverage_completion_test.go | 16 ++ internal/shortcut/minutesdata/workflow.go | 55 ++++++ 9 files changed, 432 insertions(+), 17 deletions(-) diff --git a/internal/app/param_alias_e2e_test.go b/internal/app/param_alias_e2e_test.go index 4717d2b1..71298e27 100644 --- a/internal/app/param_alias_e2e_test.go +++ b/internal/app/param_alias_e2e_test.go @@ -52,6 +52,8 @@ func (c *paramAliasCaptureCaller) paramAliasResponseForTool(tool string) string switch tool { case "list_calendar_events": return `{"result":{"events":[]}}` + case "query_records": + return `{"success":true,"status":"success","error":{},"data":{}}` case "search_mail_users": return `{"users":[{"name":"Fixture User","email":"fixture@example.com","id":"fixture-user"}]}` case "search_dept_by_keyword": diff --git a/internal/shortcut/aitable/aitable.go b/internal/shortcut/aitable/aitable.go index 0077dda9..2d1a9d64 100644 --- a/internal/shortcut/aitable/aitable.go +++ b/internal/shortcut/aitable/aitable.go @@ -733,7 +733,7 @@ var RecordQuery = shortcut.Shortcut{ if rt.Changed("cursor") { params["cursor"] = rt.Str("cursor") } - return rt.CallMCP("query_records", params) + return executeRecordQuery(rt, params) }, } diff --git a/internal/shortcut/aitable/record_batch.go b/internal/shortcut/aitable/record_batch.go index 8c26c3b1..05294ad3 100644 --- a/internal/shortcut/aitable/record_batch.go +++ b/internal/shortcut/aitable/record_batch.go @@ -98,7 +98,7 @@ func executeRecordDeleteBatches(rt *shortcut.RuntimeContext) error { step.Status = "unknown" step.Error = writeErr.Error() } - remaining, verifyErr := queryRecordsByIDs(rt, baseID, tableID, batch) + remaining, verifyErr := queryDeletedRecordsByIDs(rt, baseID, tableID, batch) if verifyErr == nil && len(remaining) > 0 { verifyErr = fmt.Errorf("read-back still contains deleted record IDs: %s", strings.Join(recordIDs(remaining), ",")) } @@ -303,20 +303,43 @@ func verifyUpsertBatch(rt *shortcut.RuntimeContext, baseID, tableID string, batc } func queryRecordsByIDs(rt *shortcut.RuntimeContext, baseID, tableID string, ids []string) ([]map[string]any, error) { - data, err := rt.CallMCPData(serverMain, "query_records", map[string]any{ - "baseId": baseID, "tableId": tableID, "recordIds": ids, "limit": len(ids), - }) + window, err := queryRecordWindow(rt, map[string]any{ + "baseId": baseID, "tableId": tableID, "recordIds": ids, + }, len(ids)) if err != nil { return nil, err } - records, found := findRecords(data) - if !found { - return nil, fmt.Errorf("query_records read-back is missing records") + // Exact-ID verification below compares every requested ID with the returned + // records. The service can publish a continuation even after all requested + // IDs are present, so hasMore is not evidence that this bounded read-back is + // incomplete. + return window.Records, nil +} + +func queryDeletedRecordsByIDs(rt *shortcut.RuntimeContext, baseID, tableID string, ids []string) ([]map[string]any, error) { + remaining := make([]map[string]any, 0) + for offset := 0; offset < len(ids); offset += recordQueryServicePageSize { + end := minInt(offset+recordQueryServicePageSize, len(ids)) + chunk := ids[offset:end] + data, err := rt.CallMCPData(serverMain, "query_records", map[string]any{ + "baseId": baseID, "tableId": tableID, "recordIds": chunk, "limit": len(chunk), + }) + if err != nil { + return nil, err + } + records, found := findRecords(data) + if !found { + if explicitEmptyRecordQuery(data) { + continue + } + return nil, fmt.Errorf("query_records deletion read-back chunk %d is missing records", offset/recordQueryServicePageSize+1) + } + if responseHasMore(data) { + return nil, fmt.Errorf("query_records deletion read-back chunk %d is incomplete", offset/recordQueryServicePageSize+1) + } + remaining = append(remaining, records...) } - if responseHasMore(data) { - return nil, fmt.Errorf("query_records read-back is incomplete") - } - return records, nil + return remaining, nil } func recordIDs(records []map[string]any) []string { diff --git a/internal/shortcut/aitable/record_query_all.go b/internal/shortcut/aitable/record_query_all.go index 0606abba..fa2c2368 100644 --- a/internal/shortcut/aitable/record_query_all.go +++ b/internal/shortcut/aitable/record_query_all.go @@ -10,13 +10,138 @@ import ( "github.com/DingTalk-Real-AI/dingtalk-workspace-cli/internal/shortcut" ) +// query_records is backed by a service pager whose stable typed-envelope +// boundary is 20 records. Asking that layer to aggregate more than one page can +// drop the envelope fields used by the record projector, turning a successful +// read into an empty collection. Keep every remote request on one service page +// and aggregate only after each page has passed the CLI's shape validation. +const recordQueryServicePageSize = 20 + +type recordQueryWindow struct { + Records []map[string]any + HasMore bool + NextCursor string + Pages int +} + +func queryRecordWindow(rt *shortcut.RuntimeContext, params map[string]any, limit int) (recordQueryWindow, error) { + if limit <= 0 { + return recordQueryWindow{}, fmt.Errorf("record query limit must be positive, got %d", limit) + } + request := cloneAnyMap(params) + cursor, _ := request["cursor"].(string) + cursor = strings.TrimSpace(cursor) + seen := map[string]bool{} + if cursor != "" { + seen[cursor] = true + } + window := recordQueryWindow{Records: make([]map[string]any, 0, limit)} + + for len(window.Records) < limit { + pageSize := minInt(recordQueryServicePageSize, limit-len(window.Records)) + request["limit"] = pageSize + if cursor == "" { + delete(request, "cursor") + } else { + request["cursor"] = cursor + } + data, err := rt.CallMCPData(serverMain, "query_records", request) + if err != nil { + return recordQueryWindow{}, err + } + records, found := findRecords(data) + if !found { + if explicitEmptyRecordQuery(data) { + window.Pages++ + window.HasMore = false + window.NextCursor = "" + return window, nil + } + return recordQueryWindow{}, fmt.Errorf("query_records page %d is missing the records collection", window.Pages+1) + } + window.Records = append(window.Records, records...) + window.Pages++ + + if !responseHasMore(data) { + window.HasMore = false + window.NextCursor = "" + return window, nil + } + next := responseCursor(data) + if next == "" { + return recordQueryWindow{}, fmt.Errorf("query_records page %d reports more data but no next cursor", window.Pages) + } + if seen[next] { + return recordQueryWindow{}, fmt.Errorf("query_records cursor cycle detected at %q", next) + } + seen[next] = true + cursor = next + window.HasMore = true + window.NextCursor = next + } + + return window, nil +} + +// explicitEmptyRecordQuery recognizes the service's reviewed zero-match wire +// shape. It is deliberately stricter than "records is absent": only a complete +// success envelope with an empty data object is accepted as an empty set. +func explicitEmptyRecordQuery(data map[string]any) bool { + if data == nil || data["success"] != true || data["status"] != "success" { + return false + } + payload, ok := data["data"].(map[string]any) + if !ok || len(payload) != 0 { + return false + } + if rawError, exists := data["error"]; exists && rawError != nil { + errorObject, ok := rawError.(map[string]any) + if !ok || len(errorObject) != 0 { + return false + } + } + return true +} + +func executeRecordQuery(rt *shortcut.RuntimeContext, params map[string]any) error { + limit := 100 + if rt.Changed("limit") { + limit = rt.Int("limit") + } + if limit < 1 || limit > recordBatchSize { + return fmt.Errorf("--limit must be in [1,%d], got %d", recordBatchSize, limit) + } + window, err := queryRecordWindow(rt, params, limit) + if err != nil { + return err + } + records := make([]any, 0, len(window.Records)) + for _, record := range window.Records { + records = append(records, record) + } + data := map[string]any{ + "records": records, + "hasMore": window.HasMore, + "page": window.Pages, + "size": len(window.Records), + } + if window.NextCursor != "" { + data["nextCursor"] = window.NextCursor + } + return rt.Output(map[string]any{ + "success": true, + "status": "success", + "data": data, + }) +} + func queryAllRecords(rt *shortcut.RuntimeContext, params map[string]any, maxRecords int) ([]map[string]any, error) { all := make([]map[string]any, 0) cursor := "" seen := map[string]bool{} for page := 0; ; page++ { request := cloneAnyMap(params) - request["limit"] = recordBatchSize + request["limit"] = recordQueryServicePageSize if cursor != "" { request["cursor"] = cursor } diff --git a/internal/shortcut/aitable/record_upsert_by_key_e2e_test.go b/internal/shortcut/aitable/record_upsert_by_key_e2e_test.go index 64bf759a..4744fbe1 100644 --- a/internal/shortcut/aitable/record_upsert_by_key_e2e_test.go +++ b/internal/shortcut/aitable/record_upsert_by_key_e2e_test.go @@ -265,6 +265,149 @@ func recordListJSON(t *testing.T, records []map[string]any) string { return string(raw) } +func pagedRecordQueryResponse(t *testing.T, records []map[string]any, args map[string]any) string { + t.Helper() + limit, ok := args["limit"].(int) + if !ok || limit < 1 || limit > recordQueryServicePageSize { + t.Fatalf("query_records limit = %#v, want 1..%d", args["limit"], recordQueryServicePageSize) + } + offset := 0 + if cursor, _ := args["cursor"].(string); cursor != "" { + if _, err := fmt.Sscanf(cursor, "offset-%d", &offset); err != nil { + t.Fatalf("invalid test cursor %q: %v", cursor, err) + } + } + end := minInt(offset+limit, len(records)) + pageRecords := records[offset:end] + data := map[string]any{"records": pageRecords, "hasMore": end < len(records)} + if end < len(records) { + data["nextCursor"] = fmt.Sprintf("offset-%d", end) + } + raw, err := json.Marshal(map[string]any{ + "success": true, + "hasMore": end < len(records), + "page": offset/recordQueryServicePageSize + 1, + "size": limit, + "data": data, + }) + if err != nil { + t.Fatal(err) + } + return string(raw) +} + +func runRecordQueryShortcutCLI(t *testing.T, caller *upsertByKeyCaller, limit int) (map[string]any, error) { + t.Helper() + helpers.InitDepsForTest(t, caller) + root := &cobra.Command{Use: "dws", SilenceErrors: true, SilenceUsage: true} + root.PersistentFlags().Bool("yes", false, "") + root.PersistentFlags().Bool("dry-run", false, "") + root.PersistentFlags().String("format", "json", "") + root.AddCommand(shortcut.Commands()...) + stdout := &bytes.Buffer{} + root.SetOut(stdout) + root.SetErr(&bytes.Buffer{}) + root.SetArgs([]string{"aitable", "+record-query", "--base-id", "base", "--table-id", "table", "--limit", fmt.Sprint(limit)}) + err := root.Execute() + if stdout.Len() == 0 { + return nil, err + } + var payload map[string]any + if decodeErr := json.Unmarshal(stdout.Bytes(), &payload); decodeErr != nil { + t.Fatalf("decode record query output %q: %v", stdout.String(), decodeErr) + } + return payload, err +} + +func TestCrossPlatformCoverageRecordQueryServicePageBoundariesE2E(t *testing.T) { + for _, size := range []int{20, 21, 22, 100} { + t.Run(fmt.Sprintf("size_%d", size), func(t *testing.T) { + records := updateFixtureRecords(0, size, "可见") + caller := &upsertByKeyCaller{} + caller.callFn = func(_ int, _, tool string, args map[string]any) (string, error) { + if tool != "query_records" { + return "", fmt.Errorf("unexpected tool %s", tool) + } + return pagedRecordQueryResponse(t, records, args), nil + } + payload, err := runRecordQueryShortcutCLI(t, caller, size) + if err != nil { + t.Fatalf("record query size %d: %v", size, err) + } + data, ok := payload["data"].(map[string]any) + if !ok || payload["success"] != true || data["hasMore"] != false || len(data["records"].([]any)) != size { + t.Fatalf("record query size %d payload = %#v", size, payload) + } + wantCalls := (size + recordQueryServicePageSize - 1) / recordQueryServicePageSize + if len(caller.calls) != wantCalls || data["page"] != float64(wantCalls) || data["size"] != float64(size) { + t.Fatalf("record query size %d calls=%d payload=%#v, want calls=%d", size, len(caller.calls), payload, wantCalls) + } + }) + } +} + +func TestCrossPlatformCoverageRecordWriteReadbackUsesStableServicePagesE2E(t *testing.T) { + for _, operation := range []string{"update", "upsert", "delete"} { + for _, size := range []int{21, 22, 100} { + t.Run(fmt.Sprintf("%s_%d", operation, size), func(t *testing.T) { + records := updateFixtureRecords(0, size, "完成") + caller := &upsertByKeyCaller{} + queryCalls := 0 + caller.callFn = func(_ int, _, tool string, args map[string]any) (string, error) { + if tool != "query_records" { + return `{"success":true}`, nil + } + queryCalls++ + if operation == "delete" { + ids := args["recordIds"].([]string) + empty := make([]map[string]any, len(ids)) + response := pagedRecordQueryResponse(t, empty, args) + var payload map[string]any + if err := json.Unmarshal([]byte(response), &payload); err != nil { + t.Fatal(err) + } + page := payload["data"].(map[string]any) + page["records"] = []any{} + raw, _ := json.Marshal(payload) + return string(raw), nil + } + response := pagedRecordQueryResponse(t, records, args) + var payload map[string]any + if err := json.Unmarshal([]byte(response), &payload); err != nil { + t.Fatal(err) + } + page := payload["data"].(map[string]any) + if len(page["records"].([]any))+recordQueryServicePageSize*(queryCalls-1) >= size { + // The live service can retain a continuation after it has + // returned every requested record ID. Exact-ID verification + // must use ID coverage, not this advisory bit. + page["hasMore"] = true + page["nextCursor"] = "ignored-after-complete-id-set" + payload["hasMore"] = true + } + raw, _ := json.Marshal(payload) + return string(raw), nil + } + + var out string + var err error + if operation == "delete" { + out, err = runRecordDeleteCLI(t, caller, recordIDs(records)) + } else { + out, err = runRecordBatchCLI(t, caller, "+record-"+operation, records) + } + if err != nil || out == "" { + t.Fatalf("%s size %d false negative: output=%q err=%v", operation, size, out, err) + } + wantQueries := (size + recordQueryServicePageSize - 1) / recordQueryServicePageSize + if queryCalls != wantQueries { + t.Fatalf("%s size %d query calls=%d, want %d", operation, size, queryCalls, wantQueries) + } + }) + } + } +} + func TestCrossPlatformCoverageRecordUpdateAutoChunksAndVerifiesE2E(t *testing.T) { records := updateFixtureRecords(0, 101, "完成") caller := &upsertByKeyCaller{steps: []upsertByKeyStep{ @@ -395,6 +538,10 @@ func TestCrossPlatformCoverageRecordDeleteAutoChunksAndProvesAbsenceE2E(t *testi caller := &upsertByKeyCaller{steps: []upsertByKeyStep{ {text: `{"deletedCount":100}`}, {text: `{"data":{"records":[]}}`}, + {text: `{"data":{"records":[]}}`}, + {text: `{"data":{"records":[]}}`}, + {text: `{"data":{"records":[]}}`}, + {text: `{"data":{"records":[]}}`}, {text: `{"deletedCount":1}`}, {text: `{"data":{"records":[]}}`}, }} @@ -407,7 +554,7 @@ func TestCrossPlatformCoverageRecordDeleteAutoChunksAndProvesAbsenceE2E(t *testi t.Fatalf("delete output missing %s: %s", want, out) } } - if len(caller.calls) != 4 || caller.calls[0].tool != "delete_records" || caller.calls[1].tool != "query_records" { + if len(caller.calls) != 8 || caller.calls[0].tool != "delete_records" || caller.calls[1].tool != "query_records" || caller.calls[6].tool != "delete_records" { t.Fatalf("delete call sequence = %#v", caller.calls) } } @@ -421,6 +568,17 @@ func TestCrossPlatformCoverageRecordDeleteEmptyReplyRecoveredOnlyByAbsenceE2E(t } }) + t.Run("explicit service success with empty data proves deletion", func(t *testing.T) { + caller := &upsertByKeyCaller{steps: []upsertByKeyStep{ + {text: `{"deletedCount":1}`}, + {text: `{"success":true,"status":"success","error":{},"data":{}}`}, + }} + out, err := runRecordDeleteCLI(t, caller, []string{"r1"}) + if err != nil || !strings.Contains(out, `"status": "verified_absent"`) { + t.Fatalf("delete explicit empty success = output:%q err:%v", out, err) + } + }) + t.Run("missing records contract is unknown", func(t *testing.T) { caller := &upsertByKeyCaller{steps: []upsertByKeyStep{{text: `{"deletedCount":1}`}, {text: `{"data":{}}`}}} out, err := runRecordDeleteCLI(t, caller, []string{"r1"}) @@ -450,6 +608,10 @@ func TestCrossPlatformCoverageRecordDeletePartialCheckpointE2E(t *testing.T) { caller := &upsertByKeyCaller{steps: []upsertByKeyStep{ {text: `{"deletedCount":100}`}, {text: `{"records":[]}`}, + {text: `{"records":[]}`}, + {text: `{"records":[]}`}, + {text: `{"records":[]}`}, + {text: `{"records":[]}`}, {text: `{"deletedCount":0}`}, {text: `{"records":[{"recordId":"r100","cells":{}}]}`}, }} diff --git a/internal/shortcut/minutes/workflows.go b/internal/shortcut/minutes/workflows.go index 68c11c8e..e9a44456 100644 --- a/internal/shortcut/minutes/workflows.go +++ b/internal/shortcut/minutes/workflows.go @@ -614,6 +614,17 @@ func executeMinutesShare(rt *shortcut.RuntimeContext) error { } func executeMinutesUnshare(rt *shortcut.RuntimeContext) error { + if !rt.DryRun() { + for _, id := range minutesIDs(rt) { + data, err := rt.CallMCPData("minutes", "get_minutes_basic_info", map[string]any{"taskUuid": id}) + if err != nil { + return fmt.Errorf("minutes unshare preflight for %s: %w", id, err) + } + if _, err := minutesdata.Basic(id, data); err != nil { + return fmt.Errorf("minutes unshare preflight for %s: %w", id, err) + } + } + } return executeMinutesPermissionLedger(rt, "unshare", "remove_member_permission", func(member string) map[string]any { return map[string]any{"uuids": minutesIDs(rt), "memberUids": []string{member}} }) @@ -631,7 +642,11 @@ func executeMinutesPermissionLedger(rt *shortcut.RuntimeContext, operation, tool for index, member := range members { data, err := rt.CallMCPWriteDataStrict("minutes", tool, params(member)) if err == nil { - err = minutesdata.RequireWriteAcknowledgement(operation, data) + if operation == "unshare" { + err = minutesdata.RequirePermissionMutationAcknowledgement(operation, minutesIDs(rt), []string{member}, data) + } else { + err = minutesdata.RequireWriteAcknowledgement(operation, data) + } } if err != nil { failures = append(failures, map[string]any{"memberUid": member, "error": err.Error()}) diff --git a/internal/shortcut/minutes/workflows_coverage_test.go b/internal/shortcut/minutes/workflows_coverage_test.go index aa05eb6d..f94f5e1a 100644 --- a/internal/shortcut/minutes/workflows_coverage_test.go +++ b/internal/shortcut/minutes/workflows_coverage_test.go @@ -323,7 +323,10 @@ func TestCrossPlatformCoverageMinutesPermissionLedgerBranchesE2E(t *testing.T) { if err := runMinutesAlignmentCLIWithWriter(t, &minutesE2ECaller{}, minutesFailWriter{}, "minutes", "+unshare", "--id", "u1", "--member-uids", "m1", "--dry-run"); err == nil { t.Fatal("unshare dry-run output failure accepted") } - unshare := &minutesE2ECaller{responses: map[string][]string{"minutes/remove_member_permission": {`{"success":true,"result":{}}`}}} + unshare := &minutesE2ECaller{responses: map[string][]string{ + "minutes/get_minutes_basic_info": {`{"success":true,"result":{"taskUuid":"u1"}}`}, + "minutes/remove_member_permission": {`{"success":true,"result":{"resultMap":{"u1":["m1"]}}}`}, + }} payload, _, err := runMinutesAlignmentCLI(t, unshare, "minutes", "+unshare", "--id", "u1", "--member-uids", "m1", "--yes") if err != nil || payload["complete"] != true || payload["succeeded"] != float64(1) { t.Fatalf("unshare payload=%#v err=%v", payload, err) @@ -337,11 +340,25 @@ func TestCrossPlatformCoverageMinutesPermissionLedgerBranchesE2E(t *testing.T) { if args["coverPermission"] != "true" || len(args["roleSubResourceIds"].([]string)) != 2 { t.Fatalf("share args=%#v", args) } - continueFailure := &minutesE2ECaller{responses: map[string][]string{"minutes/remove_member_permission": {`{"result":{}}`, `{"success":true,"result":{}}`}}} + continueFailure := &minutesE2ECaller{responses: map[string][]string{ + "minutes/get_minutes_basic_info": {`{"success":true,"result":{"taskUuid":"u1"}}`}, + "minutes/remove_member_permission": { + `{"result":{}}`, + `{"success":true,"result":{"resultMap":{"u1":["m2"]}}}`, + }, + }} payload, output, err := runMinutesAlignmentCLI(t, continueFailure, "minutes", "+unshare", "--id", "u1", "--member-uids", "m1,m2", "--failure-policy", "continue", "--yes") if err == nil || output == "" || payload["failed"] != float64(1) || payload["succeeded"] != float64(1) { t.Fatalf("continue ledger payload=%#v err=%v", payload, err) } + + missing := &minutesE2ECaller{responses: map[string][]string{ + "minutes/get_minutes_basic_info": {`{"success":true,"result":{}}`}, + }} + payload, output, err = runMinutesAlignmentCLI(t, missing, "minutes", "+unshare", "--id", "missing", "--member-uids", "m1", "--yes") + if err == nil || payload != nil || output != "" || missing.counts["minutes/remove_member_permission"] != 0 { + t.Fatalf("missing minutes reached unshare: payload=%#v output=%q err=%v calls=%#v", payload, output, err, missing.counts) + } } func TestCrossPlatformCoverageMinutesArtifactCollectorBranches(t *testing.T) { diff --git a/internal/shortcut/minutesdata/coverage_completion_test.go b/internal/shortcut/minutesdata/coverage_completion_test.go index e4d53737..d8b85dcb 100644 --- a/internal/shortcut/minutesdata/coverage_completion_test.go +++ b/internal/shortcut/minutesdata/coverage_completion_test.go @@ -224,6 +224,22 @@ func TestCrossPlatformCoverageMinutesWorkflowCompletion(t *testing.T) { if err := RequireWriteAcknowledgement("write", map[string]any{"result": map[string]any{"updated": true}}); err != nil { t.Fatal(err) } + if err := RequirePermissionMutationAcknowledgement("unshare", []string{"u1"}, []string{"202397"}, map[string]any{ + "success": true, + "result": map[string]any{"resultMap": map[string]any{"u1": []any{float64(202397)}}}, + }); err != nil { + t.Fatalf("valid permission acknowledgement rejected: %v", err) + } + for _, data := range []map[string]any{ + {"success": true, "result": map[string]any{}}, + {"success": true, "result": map[string]any{"resultMap": map[string]any{}}}, + {"success": true, "result": map[string]any{"resultMap": map[string]any{"other": []any{"202397"}}}}, + {"success": true, "result": map[string]any{"resultMap": map[string]any{"u1": []any{"other"}}}}, + } { + if err := RequirePermissionMutationAcknowledgement("unshare", []string{"u1"}, []string{"202397"}, data); err == nil { + t.Fatalf("invalid permission acknowledgement accepted: %#v", data) + } + } for _, tc := range []struct { cmd, id string data map[string]any diff --git a/internal/shortcut/minutesdata/workflow.go b/internal/shortcut/minutesdata/workflow.go index 6cfd1347..4ffc9a83 100644 --- a/internal/shortcut/minutesdata/workflow.go +++ b/internal/shortcut/minutesdata/workflow.go @@ -30,6 +30,61 @@ func RequireWriteAcknowledgement(operation string, data map[string]any) error { return fmt.Errorf("minutes %s response has no explicit successful acknowledgement", operation) } +// RequirePermissionMutationAcknowledgement validates the target-level payload +// returned by add/remove_member_permission. The backend's top-level +// success=true is not sufficient: historically it also accompanied a +// resultMap that merely echoed nonexistent task UUIDs. +func RequirePermissionMutationAcknowledgement(operation string, taskUUIDs, memberUIDs []string, data map[string]any) error { + if err := validateEnvelope(data); err != nil { + return err + } + if success, ok := data["success"].(bool); !ok || !success { + return fmt.Errorf("minutes %s response has no explicit success=true", operation) + } + result, ok := data["result"].(map[string]any) + if !ok { + return fmt.Errorf("minutes %s response has no result object", operation) + } + resultMap, ok := result["resultMap"].(map[string]any) + if !ok { + return fmt.Errorf("minutes %s response has no result.resultMap object", operation) + } + if len(resultMap) != len(taskUUIDs) { + return fmt.Errorf("minutes %s resultMap covers %d task UUIDs, want %d", operation, len(resultMap), len(taskUUIDs)) + } + wantMembers := make(map[string]bool, len(memberUIDs)) + for _, member := range memberUIDs { + wantMembers[strings.TrimSpace(member)] = true + } + for _, taskUUID := range taskUUIDs { + rawMembers, exists := resultMap[taskUUID] + if !exists { + return fmt.Errorf("minutes %s resultMap is missing task UUID %s", operation, taskUUID) + } + members, ok := rawMembers.([]any) + if !ok { + return fmt.Errorf("minutes %s resultMap[%s] has type %T, want array", operation, taskUUID, rawMembers) + } + gotMembers := make(map[string]bool, len(members)) + for index, member := range members { + value := stringField(map[string]any{"member": member}, "member") + if value == "" { + return fmt.Errorf("minutes %s resultMap[%s][%d] is not a member UID", operation, taskUUID, index) + } + gotMembers[value] = true + } + if len(gotMembers) != len(wantMembers) { + return fmt.Errorf("minutes %s resultMap[%s] member coverage mismatch", operation, taskUUID) + } + for member := range wantMembers { + if !gotMembers[member] { + return fmt.Errorf("minutes %s resultMap[%s] is missing member UID %s", operation, taskUUID, member) + } + } + } + return nil +} + // RecordResult validates the gateway's observed listening-note command result. func RecordResult(expectedCmd, taskUUID string, data map[string]any) (map[string]any, error) { if err := RequireWriteAcknowledgement("record "+expectedCmd, data); err != nil { From e14de2b4c20ad4f9fc1b0a8f566e4d3b34c88395 Mon Sep 17 00:00:00 2001 From: Dennis Date: Fri, 14 Aug 2026 15:41:42 +0800 Subject: [PATCH 13/32] test: close shortcut fix review gates --- ...1006-aitable-pagination-minutes-unshare.md | 9 ++ .../aitable/record_upsert_by_key_e2e_test.go | 111 ++++++++++++++++++ .../minutes/workflows_coverage_test.go | 6 + .../minutesdata/coverage_completion_test.go | 10 ++ 4 files changed, 136 insertions(+) create mode 100644 .changes/1006-aitable-pagination-minutes-unshare.md diff --git a/.changes/1006-aitable-pagination-minutes-unshare.md b/.changes/1006-aitable-pagination-minutes-unshare.md new file mode 100644 index 00000000..8ca97c30 --- /dev/null +++ b/.changes/1006-aitable-pagination-minutes-unshare.md @@ -0,0 +1,9 @@ +--- +category: Fixed +--- + +- **Aitable pagination and Minutes unshare verification** (#1006) — keeps + record queries on the service's 20-record page boundary so multi-page reads + and mutation readbacks no longer report false retryable failures, and rejects + Minutes unshare success until the listening note exists and the service + acknowledges the exact task and member targets. diff --git a/internal/shortcut/aitable/record_upsert_by_key_e2e_test.go b/internal/shortcut/aitable/record_upsert_by_key_e2e_test.go index 4744fbe1..3a639b6e 100644 --- a/internal/shortcut/aitable/record_upsert_by_key_e2e_test.go +++ b/internal/shortcut/aitable/record_upsert_by_key_e2e_test.go @@ -346,6 +346,117 @@ func TestCrossPlatformCoverageRecordQueryServicePageBoundariesE2E(t *testing.T) } } +func TestCrossPlatformCoverageRecordQueryFailureAndContinuationBranches(t *testing.T) { + t.Run("non-positive window limit", func(t *testing.T) { + caller := &upsertByKeyCaller{} + helpers.InitDepsForTest(t, caller) + rt := shortcut.RuntimeContextForTest(&cobra.Command{Use: "query"}, RecordQuery) + if _, err := queryRecordWindow(rt, nil, 0); err == nil { + t.Fatal("non-positive record query window limit accepted") + } + }) + + t.Run("initial cursor cycle", func(t *testing.T) { + caller := &upsertByKeyCaller{callFn: func(_ int, _, tool string, _ map[string]any) (string, error) { + if tool != "query_records" { + return "", fmt.Errorf("unexpected tool %s", tool) + } + return `{"success":true,"data":{"records":[{"recordId":"r1"}],"hasMore":true,"nextCursor":"seed"}}`, nil + }} + helpers.InitDepsForTest(t, caller) + rt := shortcut.RuntimeContextForTest(&cobra.Command{Use: "query"}, RecordQuery) + if _, err := queryRecordWindow(rt, map[string]any{"cursor": " seed "}, 2); err == nil || !strings.Contains(err.Error(), "cursor cycle") { + t.Fatalf("initial cursor cycle error = %v", err) + } + }) + + t.Run("strict explicit empty shape", func(t *testing.T) { + for _, data := range []map[string]any{ + {"success": true, "status": "success", "data": map[string]any{"unexpected": true}}, + {"success": true, "status": "success", "data": map[string]any{}, "error": "bad"}, + } { + if explicitEmptyRecordQuery(data) { + t.Fatalf("invalid empty-query envelope accepted: %#v", data) + } + } + }) + + t.Run("explicit empty page", func(t *testing.T) { + caller := &upsertByKeyCaller{callFn: func(_ int, _, _ string, _ map[string]any) (string, error) { + return `{"success":true,"status":"success","error":{},"data":{}}`, nil + }} + helpers.InitDepsForTest(t, caller) + rt := shortcut.RuntimeContextForTest(&cobra.Command{Use: "query"}, RecordQuery) + window, err := queryRecordWindow(rt, map[string]any{}, 1) + if err != nil || len(window.Records) != 0 || window.Pages != 1 || window.HasMore || window.NextCursor != "" { + t.Fatalf("explicit empty window=%#v err=%v", window, err) + } + }) + + t.Run("missing records collection", func(t *testing.T) { + caller := &upsertByKeyCaller{callFn: func(_ int, _, _ string, _ map[string]any) (string, error) { + return `{}`, nil + }} + helpers.InitDepsForTest(t, caller) + rt := shortcut.RuntimeContextForTest(&cobra.Command{Use: "query"}, RecordQuery) + if _, err := queryRecordWindow(rt, map[string]any{}, 1); err == nil || !strings.Contains(err.Error(), "missing the records collection") { + t.Fatalf("missing records error = %v", err) + } + }) + + t.Run("invalid shortcut limit", func(t *testing.T) { + caller := &upsertByKeyCaller{} + helpers.InitDepsForTest(t, caller) + cmd := &cobra.Command{Use: "query"} + cmd.Flags().Int("limit", 0, "") + if err := cmd.Flags().Set("limit", "0"); err != nil { + t.Fatal(err) + } + rt := shortcut.RuntimeContextForTest(cmd, RecordQuery) + if err := executeRecordQuery(rt, map[string]any{}); err == nil { + t.Fatal("invalid shortcut limit accepted") + } + }) + + t.Run("shortcut query failure", func(t *testing.T) { + caller := &upsertByKeyCaller{callFn: func(_ int, _, _ string, _ map[string]any) (string, error) { + return "", errors.New("query unavailable") + }} + helpers.InitDepsForTest(t, caller) + rt := shortcut.RuntimeContextForTest(&cobra.Command{Use: "query"}, RecordQuery) + if err := executeRecordQuery(rt, map[string]any{}); err == nil || !strings.Contains(err.Error(), "query unavailable") { + t.Fatalf("shortcut query failure = %v", err) + } + }) + + t.Run("bounded continuation is published", func(t *testing.T) { + records := updateFixtureRecords(0, 21, "visible") + caller := &upsertByKeyCaller{callFn: func(_ int, _, tool string, args map[string]any) (string, error) { + if tool != "query_records" { + return "", fmt.Errorf("unexpected tool %s", tool) + } + return pagedRecordQueryResponse(t, records, args), nil + }} + payload, err := runRecordQueryShortcutCLI(t, caller, 20) + data, _ := payload["data"].(map[string]any) + if err != nil || data["hasMore"] != true || data["nextCursor"] != "offset-20" { + t.Fatalf("bounded continuation payload=%#v err=%v", payload, err) + } + }) + + t.Run("delete readback rejects continuation", func(t *testing.T) { + caller := &upsertByKeyCaller{callFn: func(_ int, _, tool string, _ map[string]any) (string, error) { + if tool == "query_records" { + return `{"success":true,"data":{"records":[],"hasMore":true,"nextCursor":"unexpected"}}`, nil + } + return `{"deletedCount":1}`, nil + }} + if _, err := runRecordDeleteCLI(t, caller, []string{"r1"}); err == nil || len(caller.calls) != 2 || caller.calls[1].tool != "query_records" { + t.Fatalf("delete continuation error = %v calls=%#v", err, caller.calls) + } + }) +} + func TestCrossPlatformCoverageRecordWriteReadbackUsesStableServicePagesE2E(t *testing.T) { for _, operation := range []string{"update", "upsert", "delete"} { for _, size := range []int{21, 22, 100} { diff --git a/internal/shortcut/minutes/workflows_coverage_test.go b/internal/shortcut/minutes/workflows_coverage_test.go index f94f5e1a..c2d7bf43 100644 --- a/internal/shortcut/minutes/workflows_coverage_test.go +++ b/internal/shortcut/minutes/workflows_coverage_test.go @@ -359,6 +359,12 @@ func TestCrossPlatformCoverageMinutesPermissionLedgerBranchesE2E(t *testing.T) { if err == nil || payload != nil || output != "" || missing.counts["minutes/remove_member_permission"] != 0 { t.Fatalf("missing minutes reached unshare: payload=%#v output=%q err=%v calls=%#v", payload, output, err, missing.counts) } + + preflightFailure := &minutesE2ECaller{failAt: map[string]int{"minutes/get_minutes_basic_info": 1}} + payload, output, err = runMinutesAlignmentCLI(t, preflightFailure, "minutes", "+unshare", "--id", "unavailable", "--member-uids", "m1", "--yes") + if err == nil || payload != nil || output != "" || preflightFailure.counts["minutes/remove_member_permission"] != 0 { + t.Fatalf("failed preflight reached unshare: payload=%#v output=%q err=%v calls=%#v", payload, output, err, preflightFailure.counts) + } } func TestCrossPlatformCoverageMinutesArtifactCollectorBranches(t *testing.T) { diff --git a/internal/shortcut/minutesdata/coverage_completion_test.go b/internal/shortcut/minutesdata/coverage_completion_test.go index d8b85dcb..aa2e3cb4 100644 --- a/internal/shortcut/minutesdata/coverage_completion_test.go +++ b/internal/shortcut/minutesdata/coverage_completion_test.go @@ -231,15 +231,25 @@ func TestCrossPlatformCoverageMinutesWorkflowCompletion(t *testing.T) { t.Fatalf("valid permission acknowledgement rejected: %v", err) } for _, data := range []map[string]any{ + {"success": false, "errorMsg": "denied"}, + {"success": true, "result": "bad"}, {"success": true, "result": map[string]any{}}, {"success": true, "result": map[string]any{"resultMap": map[string]any{}}}, {"success": true, "result": map[string]any{"resultMap": map[string]any{"other": []any{"202397"}}}}, + {"success": true, "result": map[string]any{"resultMap": map[string]any{"u1": "bad"}}}, + {"success": true, "result": map[string]any{"resultMap": map[string]any{"u1": []any{nil}}}}, {"success": true, "result": map[string]any{"resultMap": map[string]any{"u1": []any{"other"}}}}, } { if err := RequirePermissionMutationAcknowledgement("unshare", []string{"u1"}, []string{"202397"}, data); err == nil { t.Fatalf("invalid permission acknowledgement accepted: %#v", data) } } + if err := RequirePermissionMutationAcknowledgement("unshare", []string{"u1"}, []string{"202397", "other"}, map[string]any{ + "success": true, + "result": map[string]any{"resultMap": map[string]any{"u1": []any{"202397"}}}, + }); err == nil { + t.Fatal("incomplete member coverage accepted") + } for _, tc := range []struct { cmd, id string data map[string]any From f486105836fdbbc69163123ec9d6e8edfbf225d2 Mon Sep 17 00:00:00 2001 From: Dennis Date: Fri, 14 Aug 2026 16:39:46 +0800 Subject: [PATCH 14/32] fix: bound empty aitable pagination --- internal/shortcut/aitable/record_query_all.go | 17 +++++++++++++++- .../aitable/record_upsert_by_key_e2e_test.go | 20 +++++++++++++++++++ .../minutesdata/coverage_completion_test.go | 1 + 3 files changed, 37 insertions(+), 1 deletion(-) diff --git a/internal/shortcut/aitable/record_query_all.go b/internal/shortcut/aitable/record_query_all.go index fa2c2368..bdaffb41 100644 --- a/internal/shortcut/aitable/record_query_all.go +++ b/internal/shortcut/aitable/record_query_all.go @@ -15,7 +15,10 @@ import ( // drop the envelope fields used by the record projector, turning a successful // read into an empty collection. Keep every remote request on one service page // and aggregate only after each page has passed the CLI's shape validation. -const recordQueryServicePageSize = 20 +const ( + recordQueryServicePageSize = 20 + recordQueryMaxConsecutiveEmptyPages = 3 +) type recordQueryWindow struct { Records []map[string]any @@ -36,6 +39,7 @@ func queryRecordWindow(rt *shortcut.RuntimeContext, params map[string]any, limit seen[cursor] = true } window := recordQueryWindow{Records: make([]map[string]any, 0, limit)} + consecutiveEmptyPages := 0 for len(window.Records) < limit { pageSize := minInt(recordQueryServicePageSize, limit-len(window.Records)) @@ -67,6 +71,17 @@ func queryRecordWindow(rt *shortcut.RuntimeContext, params map[string]any, limit window.NextCursor = "" return window, nil } + if len(records) == 0 { + consecutiveEmptyPages++ + if consecutiveEmptyPages >= recordQueryMaxConsecutiveEmptyPages { + return recordQueryWindow{}, fmt.Errorf( + "query_records made no progress for %d consecutive pages", + consecutiveEmptyPages, + ) + } + } else { + consecutiveEmptyPages = 0 + } next := responseCursor(data) if next == "" { return recordQueryWindow{}, fmt.Errorf("query_records page %d reports more data but no next cursor", window.Pages) diff --git a/internal/shortcut/aitable/record_upsert_by_key_e2e_test.go b/internal/shortcut/aitable/record_upsert_by_key_e2e_test.go index 3a639b6e..d468901a 100644 --- a/internal/shortcut/aitable/record_upsert_by_key_e2e_test.go +++ b/internal/shortcut/aitable/record_upsert_by_key_e2e_test.go @@ -404,6 +404,26 @@ func TestCrossPlatformCoverageRecordQueryFailureAndContinuationBranches(t *testi } }) + t.Run("changing cursors cannot hide sustained empty pages", func(t *testing.T) { + caller := &upsertByKeyCaller{callFn: func(call int, _, tool string, _ map[string]any) (string, error) { + if tool != "query_records" { + return "", fmt.Errorf("unexpected tool %s", tool) + } + return fmt.Sprintf( + `{"success":true,"data":{"records":[],"hasMore":true,"nextCursor":"empty-%d"}}`, + call, + ), nil + }} + helpers.InitDepsForTest(t, caller) + rt := shortcut.RuntimeContextForTest(&cobra.Command{Use: "query"}, RecordQuery) + if _, err := queryRecordWindow(rt, map[string]any{}, 1); err == nil || !strings.Contains(err.Error(), "no progress") { + t.Fatalf("sustained empty pages error = %v", err) + } + if len(caller.calls) != recordQueryMaxConsecutiveEmptyPages { + t.Fatalf("query calls = %d, want %d", len(caller.calls), recordQueryMaxConsecutiveEmptyPages) + } + }) + t.Run("invalid shortcut limit", func(t *testing.T) { caller := &upsertByKeyCaller{} helpers.InitDepsForTest(t, caller) diff --git a/internal/shortcut/minutesdata/coverage_completion_test.go b/internal/shortcut/minutesdata/coverage_completion_test.go index aa2e3cb4..b0bbcfbc 100644 --- a/internal/shortcut/minutesdata/coverage_completion_test.go +++ b/internal/shortcut/minutesdata/coverage_completion_test.go @@ -232,6 +232,7 @@ func TestCrossPlatformCoverageMinutesWorkflowCompletion(t *testing.T) { } for _, data := range []map[string]any{ {"success": false, "errorMsg": "denied"}, + {"success": "yes"}, {"success": true, "result": "bad"}, {"success": true, "result": map[string]any{}}, {"success": true, "result": map[string]any{"resultMap": map[string]any{}}}, From f0412758118e5abf3e1d8eb43212c85733e7e859 Mon Sep 17 00:00:00 2001 From: Dennis Date: Fri, 14 Aug 2026 22:55:22 +0800 Subject: [PATCH 15/32] fix: make minutes polling portable --- internal/shortcut/minutes/alignment.go | 10 +++------- internal/shortcut/minutes/workflows.go | 19 ++++++++++++++----- .../minutes/workflows_coverage_test.go | 3 +-- 3 files changed, 18 insertions(+), 14 deletions(-) diff --git a/internal/shortcut/minutes/alignment.go b/internal/shortcut/minutes/alignment.go index 21d35464..7a3d85f1 100644 --- a/internal/shortcut/minutes/alignment.go +++ b/internal/shortcut/minutes/alignment.go @@ -618,15 +618,11 @@ func completeMinutesUpload(rt *shortcut.RuntimeContext, sessionID string, timeou } message := strings.ToLower(err.Error()) retryable := strings.Contains(message, "still uploading") || strings.Contains(message, "wait and retry") || strings.Contains(message, "正在上传") - if !retryable || time.Now().Add(interval).After(deadline) { + if !retryable || minutesPollDeadlineReached(deadline, interval) { return nil, attempts, err } - timer := time.NewTimer(interval) - select { - case <-rt.Command().Context().Done(): - timer.Stop() - return nil, attempts, rt.Command().Context().Err() - case <-timer.C: + if err := waitMinutesInterval(rt, interval); err != nil { + return nil, attempts, err } } } diff --git a/internal/shortcut/minutes/workflows.go b/internal/shortcut/minutes/workflows.go index e9a44456..7ba980d3 100644 --- a/internal/shortcut/minutes/workflows.go +++ b/internal/shortcut/minutes/workflows.go @@ -4,6 +4,7 @@ package minutes import ( + "context" "encoding/json" "fmt" "os" @@ -372,7 +373,7 @@ func runMinutesMindmap(rt *shortcut.RuntimeContext, id string, timeout, interval payload := map[string]any{"operation": "minutes.mindmap", "complete": false, "taskUuid": id, "taskStatus": status, "attempts": attempts, "result": result, "recovery": map[string]any{"taskUuid": id, "nextAction": "inspect source transcript; do not assume an empty mind map"}} return payload, minutesCompositeError("minutes_mindmap_failed", "poll", payload) } - if time.Now().Add(interval).After(deadline) { + if minutesPollDeadlineReached(deadline, interval) { payload := map[string]any{"operation": "minutes.mindmap", "complete": false, "taskUuid": id, "taskStatus": status, "attempts": attempts, "recovery": map[string]any{"taskUuid": id, "nextAction": "dws minutes mind-graph status --id "}} return payload, minutesCompositeError("minutes_mindmap_timeout", "poll", payload) } @@ -422,7 +423,7 @@ func runMinutesSpeakerInsights(rt *shortcut.RuntimeContext, id string, timeout, payload := map[string]any{"operation": "minutes.speaker_insights", "complete": false, "taskUuid": id, "taskId": taskID, "attempts": attempts, "stage": "poll", "recovery": map[string]any{"taskUuid": id, "taskId": taskID, "nextAction": "dws minutes speaker summary get --ids "}} return payload, callErr } - if time.Now().Add(interval).After(deadline) { + if minutesPollDeadlineReached(deadline, interval) { payload := map[string]any{"operation": "minutes.speaker_insights", "complete": false, "taskUuid": id, "taskId": taskID, "attempts": attempts, "stage": "poll", "recovery": map[string]any{"taskUuid": id, "taskId": taskID, "nextAction": "dws minutes speaker summary get --ids "}} return payload, minutesCompositeError("minutes_speaker_insights_timeout", "poll", payload) } @@ -723,7 +724,7 @@ func waitMinutesArtifacts(rt *shortcut.RuntimeContext, id string, artifacts []st for { attempts++ bundle, failures := collectMinutesArtifactsOnce(rt, id, artifacts, pageLimit) - if len(failures) == 0 || time.Now().Add(interval).After(deadline) { + if len(failures) == 0 || minutesPollDeadlineReached(deadline, interval) { return bundle, failures, attempts } if err := waitMinutesInterval(rt, interval); err != nil { @@ -735,14 +736,22 @@ func waitMinutesArtifacts(rt *shortcut.RuntimeContext, id string, artifacts []st func waitMinutesInterval(rt *shortcut.RuntimeContext, interval time.Duration) error { timer := time.NewTimer(interval) defer timer.Stop() + commandContext := rt.Command().Context() + if commandContext == nil { + commandContext = context.Background() + } select { - case <-rt.Command().Context().Done(): - return rt.Command().Context().Err() + case <-commandContext.Done(): + return commandContext.Err() case <-timer.C: return nil } } +func minutesPollDeadlineReached(deadline time.Time, interval time.Duration) bool { + return !time.Now().Add(interval).Before(deadline) +} + func outputWorkflowResult(rt *shortcut.RuntimeContext, payload map[string]any, failed bool, reason, stage string) error { if err := rt.Output(payload); err != nil { return err diff --git a/internal/shortcut/minutes/workflows_coverage_test.go b/internal/shortcut/minutes/workflows_coverage_test.go index c2d7bf43..36ff3ac8 100644 --- a/internal/shortcut/minutes/workflows_coverage_test.go +++ b/internal/shortcut/minutes/workflows_coverage_test.go @@ -442,10 +442,9 @@ func TestCrossPlatformCoverageMinutesArtifactWaitAndOutput(t *testing.T) { t.Fatalf("cancel failures=%#v", failures) } cmd = &cobra.Command{Use: "wait"} - cmd.SetContext(context.Background()) rt = shortcut.RuntimeContextForTest(cmd, ExportPack) if err := waitMinutesInterval(rt, 0); err != nil { - t.Fatalf("timer wait: %v", err) + t.Fatalf("timer wait with unset command context: %v", err) } cmd.SetOut(minutesFailWriter{}) From 2ab8748c4d1a9310841c178562544dc0410009ec Mon Sep 17 00:00:00 2001 From: Dennis Date: Fri, 14 Aug 2026 23:01:31 +0800 Subject: [PATCH 16/32] test: use native minutes path separators --- internal/shortcut/minutes/workflows_coverage_test.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/internal/shortcut/minutes/workflows_coverage_test.go b/internal/shortcut/minutes/workflows_coverage_test.go index 36ff3ac8..34c47c74 100644 --- a/internal/shortcut/minutes/workflows_coverage_test.go +++ b/internal/shortcut/minutes/workflows_coverage_test.go @@ -586,7 +586,7 @@ func TestCrossPlatformCoverageMinutesExportPackBranchesE2E(t *testing.T) { } func TestCrossPlatformCoverageMinutesExportPathAndJSONFaults(t *testing.T) { - if _, _, _, err := prepareExportTarget("../escape"); err == nil { + if _, _, _, err := prepareExportTarget(filepath.Join("..", "escape")); err == nil { t.Fatal("unsafe target accepted") } for _, test := range []struct { @@ -612,7 +612,7 @@ func TestCrossPlatformCoverageMinutesExportPathAndJSONFaults(t *testing.T) { } }}, {name: "escape", run: func(t *testing.T) { - testseam.Swap(t, &minutesRel, func(string, string) (string, error) { return "../escape", nil }) + testseam.Swap(t, &minutesRel, func(string, string) (string, error) { return filepath.Join("..", "escape"), nil }) if _, _, _, err := prepareExportTarget("pack"); err == nil { t.Fatal("rel escape accepted") } @@ -624,7 +624,7 @@ func TestCrossPlatformCoverageMinutesExportPathAndJSONFaults(t *testing.T) { } }}, {name: "parent", run: func(t *testing.T) { - testseam.Swap(t, &minutesRel, func(string, string) (string, error) { return "parent/pack", nil }) + testseam.Swap(t, &minutesRel, func(string, string) (string, error) { return filepath.Join("parent", "pack"), nil }) testseam.Swap(t, &minutesLstat, func(string) (os.FileInfo, error) { return nil, errors.New("parent") }) if _, _, _, err := prepareExportTarget("pack"); err == nil { t.Fatal("unsafe parent accepted") From 37b9a1dc3166eecdb1cfa743108389c03212ef65 Mon Sep 17 00:00:00 2001 From: Dennis Date: Fri, 14 Aug 2026 23:26:31 +0800 Subject: [PATCH 17/32] fix: bound exact aitable record queries --- internal/shortcut/aitable/record_query_all.go | 63 +++++++++++++++ .../aitable/record_upsert_by_key_e2e_test.go | 78 ++++++++++++++++++- 2 files changed, 139 insertions(+), 2 deletions(-) diff --git a/internal/shortcut/aitable/record_query_all.go b/internal/shortcut/aitable/record_query_all.go index bdaffb41..3e079384 100644 --- a/internal/shortcut/aitable/record_query_all.go +++ b/internal/shortcut/aitable/record_query_all.go @@ -126,10 +126,35 @@ func executeRecordQuery(rt *shortcut.RuntimeContext, params map[string]any) erro if limit < 1 || limit > recordBatchSize { return fmt.Errorf("--limit must be in [1,%d], got %d", recordBatchSize, limit) } + requestedIDs, exactIDQuery, err := recordQueryRequestedIDs(params) + if err != nil { + return err + } + if exactIDQuery { + if len(requestedIDs) > recordBatchSize { + return fmt.Errorf("--record-ids accepts at most %d unique IDs, got %d", recordBatchSize, len(requestedIDs)) + } + params = cloneAnyMap(params) + params["recordIds"] = requestedIDs + limit = minInt(limit, len(requestedIDs)) + } window, err := queryRecordWindow(rt, params, limit) if err != nil { return err } + if exactIDQuery { + complete, err := validateExactRecordQuery(window.Records, requestedIDs) + if err != nil { + return err + } + if complete { + // The service may advertise unrelated continuation after every + // requested ID is already present. Exact-ID completion is stronger + // evidence than that residual pager state. + window.HasMore = false + window.NextCursor = "" + } + } records := make([]any, 0, len(window.Records)) for _, record := range window.Records { records = append(records, record) @@ -150,6 +175,44 @@ func executeRecordQuery(rt *shortcut.RuntimeContext, params map[string]any) erro }) } +func recordQueryRequestedIDs(params map[string]any) ([]string, bool, error) { + raw, exists := params["recordIds"] + if !exists { + return nil, false, nil + } + values, ok := raw.([]string) + if !ok { + return nil, false, fmt.Errorf("recordIds must be a string list, got %T", raw) + } + ids, err := parseRecordIDs(values) + if err != nil { + return nil, false, err + } + return ids, true, nil +} + +func validateExactRecordQuery(records []map[string]any, requestedIDs []string) (bool, error) { + wanted := make(map[string]bool, len(requestedIDs)) + for _, id := range requestedIDs { + wanted[id] = true + } + seen := make(map[string]bool, len(records)) + for index, record := range records { + id := recordID(record) + if id == "" { + return false, fmt.Errorf("query_records exact-ID result %d is missing recordId", index) + } + if !wanted[id] { + return false, fmt.Errorf("query_records exact-ID result contains unexpected recordId %q", id) + } + if seen[id] { + return false, fmt.Errorf("query_records exact-ID result contains duplicate recordId %q", id) + } + seen[id] = true + } + return len(seen) == len(wanted), nil +} + func queryAllRecords(rt *shortcut.RuntimeContext, params map[string]any, maxRecords int) ([]map[string]any, error) { all := make([]map[string]any, 0) cursor := "" diff --git a/internal/shortcut/aitable/record_upsert_by_key_e2e_test.go b/internal/shortcut/aitable/record_upsert_by_key_e2e_test.go index d468901a..3a3800d2 100644 --- a/internal/shortcut/aitable/record_upsert_by_key_e2e_test.go +++ b/internal/shortcut/aitable/record_upsert_by_key_e2e_test.go @@ -9,6 +9,7 @@ import ( "encoding/json" "errors" "fmt" + "slices" "strings" "testing" @@ -296,7 +297,7 @@ func pagedRecordQueryResponse(t *testing.T, records []map[string]any, args map[s return string(raw) } -func runRecordQueryShortcutCLI(t *testing.T, caller *upsertByKeyCaller, limit int) (map[string]any, error) { +func runRecordQueryShortcutCLI(t *testing.T, caller *upsertByKeyCaller, limit int, extra ...string) (map[string]any, error) { t.Helper() helpers.InitDepsForTest(t, caller) root := &cobra.Command{Use: "dws", SilenceErrors: true, SilenceUsage: true} @@ -307,7 +308,8 @@ func runRecordQueryShortcutCLI(t *testing.T, caller *upsertByKeyCaller, limit in stdout := &bytes.Buffer{} root.SetOut(stdout) root.SetErr(&bytes.Buffer{}) - root.SetArgs([]string{"aitable", "+record-query", "--base-id", "base", "--table-id", "table", "--limit", fmt.Sprint(limit)}) + args := []string{"aitable", "+record-query", "--base-id", "base", "--table-id", "table", "--limit", fmt.Sprint(limit)} + root.SetArgs(append(args, extra...)) err := root.Execute() if stdout.Len() == 0 { return nil, err @@ -346,6 +348,33 @@ func TestCrossPlatformCoverageRecordQueryServicePageBoundariesE2E(t *testing.T) } } +func TestCrossPlatformCoverageRecordQueryExactIDsIgnoreResidualContinuationE2E(t *testing.T) { + caller := &upsertByKeyCaller{callFn: func(call int, _, tool string, args map[string]any) (string, error) { + if tool != "query_records" { + return "", fmt.Errorf("unexpected tool %s", tool) + } + if call != 0 { + return "", errors.New("exact-ID query followed residual continuation") + } + if args["limit"] != 2 { + t.Fatalf("exact-ID service limit = %#v, want 2", args["limit"]) + } + ids, ok := args["recordIds"].([]string) + if !ok || !slices.Equal(ids, []string{"r1", "r2"}) { + t.Fatalf("exact-ID request IDs = %#v", args["recordIds"]) + } + return `{"success":true,"data":{"records":[{"recordId":"r1"},{"recordId":"r2"}],"hasMore":true,"nextCursor":"residual"}}`, nil + }} + payload, err := runRecordQueryShortcutCLI(t, caller, 100, "--record-ids", "r2,r1,r2") + data, _ := payload["data"].(map[string]any) + if err != nil || len(caller.calls) != 1 || data["hasMore"] != false || data["size"] != float64(2) { + t.Fatalf("exact-ID payload=%#v calls=%d err=%v", payload, len(caller.calls), err) + } + if _, exists := data["nextCursor"]; exists { + t.Fatalf("exact-ID payload retained residual cursor: %#v", payload) + } +} + func TestCrossPlatformCoverageRecordQueryFailureAndContinuationBranches(t *testing.T) { t.Run("non-positive window limit", func(t *testing.T) { caller := &upsertByKeyCaller{} @@ -438,6 +467,51 @@ func TestCrossPlatformCoverageRecordQueryFailureAndContinuationBranches(t *testi } }) + t.Run("invalid exact-ID request", func(t *testing.T) { + for _, testCase := range []struct { + name string + params map[string]any + want string + }{ + {name: "wrong type", params: map[string]any{"recordIds": 1}, want: "string list"}, + {name: "empty", params: map[string]any{"recordIds": []string{" "}}, want: "至少包含一个非空 recordId"}, + {name: "over service limit", params: map[string]any{"recordIds": recordIDFixtures(recordBatchSize + 1)}, want: "at most 100 unique IDs"}, + } { + t.Run(testCase.name, func(t *testing.T) { + caller := &upsertByKeyCaller{} + helpers.InitDepsForTest(t, caller) + rt := shortcut.RuntimeContextForTest(&cobra.Command{Use: "query"}, RecordQuery) + err := executeRecordQuery(rt, testCase.params) + if err == nil || !strings.Contains(err.Error(), testCase.want) { + t.Fatalf("exact-ID request error = %v, want %q", err, testCase.want) + } + if len(caller.calls) != 0 { + t.Fatalf("invalid exact-ID request made %d service calls", len(caller.calls)) + } + }) + } + }) + + t.Run("invalid exact-ID response", func(t *testing.T) { + caller := &upsertByKeyCaller{callFn: func(_ int, _, tool string, _ map[string]any) (string, error) { + if tool != "query_records" { + return "", fmt.Errorf("unexpected tool %s", tool) + } + return `{"success":true,"data":{"records":[{"recordId":"other"}]}}`, nil + }} + if _, err := runRecordQueryShortcutCLI(t, caller, 100, "--record-ids", "r1"); err == nil || !strings.Contains(err.Error(), "unexpected recordId") { + t.Fatalf("unexpected exact-ID response error = %v", err) + } + + if _, err := validateExactRecordQuery([]map[string]any{{}}, []string{"r1"}); err == nil || !strings.Contains(err.Error(), "missing recordId") { + t.Fatalf("missing exact-ID response error = %v", err) + } + duplicate := []map[string]any{{"recordId": "r1"}, {"recordId": "r1"}} + if _, err := validateExactRecordQuery(duplicate, []string{"r1", "r2"}); err == nil || !strings.Contains(err.Error(), "duplicate recordId") { + t.Fatalf("duplicate exact-ID response error = %v", err) + } + }) + t.Run("shortcut query failure", func(t *testing.T) { caller := &upsertByKeyCaller{callFn: func(_ int, _, _ string, _ map[string]any) (string, error) { return "", errors.New("query unavailable") From 7aa8240629b49ea383638501db26e07c55b72432 Mon Sep 17 00:00:00 2001 From: Dennis Date: Mon, 17 Aug 2026 09:49:52 +0800 Subject: [PATCH 18/32] fix: preserve record query preview contract --- ...1006-aitable-pagination-minutes-unshare.md | 3 +- internal/shortcut/aitable/record_batch.go | 5 +-- internal/shortcut/aitable/record_query_all.go | 37 +++++++++++++++++-- .../aitable/record_upsert_by_key_e2e_test.go | 27 +++++++++++--- 4 files changed, 59 insertions(+), 13 deletions(-) diff --git a/.changes/1006-aitable-pagination-minutes-unshare.md b/.changes/1006-aitable-pagination-minutes-unshare.md index 8ca97c30..9607d987 100644 --- a/.changes/1006-aitable-pagination-minutes-unshare.md +++ b/.changes/1006-aitable-pagination-minutes-unshare.md @@ -4,6 +4,7 @@ category: Fixed - **Aitable pagination and Minutes unshare verification** (#1006) — keeps record queries on the service's 20-record page boundary so multi-page reads - and mutation readbacks no longer report false retryable failures, and rejects + and mutation readbacks no longer report false retryable failures, preserves + `totalCount` when supplied, keeps `--dry-run` transport-free, and rejects Minutes unshare success until the listening note exists and the service acknowledges the exact task and member targets. diff --git a/internal/shortcut/aitable/record_batch.go b/internal/shortcut/aitable/record_batch.go index 05294ad3..f779b32f 100644 --- a/internal/shortcut/aitable/record_batch.go +++ b/internal/shortcut/aitable/record_batch.go @@ -334,9 +334,8 @@ func queryDeletedRecordsByIDs(rt *shortcut.RuntimeContext, baseID, tableID strin } return nil, fmt.Errorf("query_records deletion read-back chunk %d is missing records", offset/recordQueryServicePageSize+1) } - if responseHasMore(data) { - return nil, fmt.Errorf("query_records deletion read-back chunk %d is incomplete", offset/recordQueryServicePageSize+1) - } + // Exact-ID absence is complete evidence for deletion even when the + // service retains an unrelated continuation marker. remaining = append(remaining, records...) } return remaining, nil diff --git a/internal/shortcut/aitable/record_query_all.go b/internal/shortcut/aitable/record_query_all.go index 3e079384..eadec9f6 100644 --- a/internal/shortcut/aitable/record_query_all.go +++ b/internal/shortcut/aitable/record_query_all.go @@ -21,10 +21,12 @@ const ( ) type recordQueryWindow struct { - Records []map[string]any - HasMore bool - NextCursor string - Pages int + Records []map[string]any + HasMore bool + NextCursor string + Pages int + TotalCount any + HasTotalCount bool } func queryRecordWindow(rt *shortcut.RuntimeContext, params map[string]any, limit int) (recordQueryWindow, error) { @@ -53,6 +55,9 @@ func queryRecordWindow(rt *shortcut.RuntimeContext, params map[string]any, limit if err != nil { return recordQueryWindow{}, err } + if !window.HasTotalCount { + window.TotalCount, window.HasTotalCount = responseTotalCount(data) + } records, found := findRecords(data) if !found { if explicitEmptyRecordQuery(data) { @@ -119,6 +124,14 @@ func explicitEmptyRecordQuery(data map[string]any) bool { } func executeRecordQuery(rt *shortcut.RuntimeContext, params map[string]any) error { + if rt.DryRun() { + return rt.Output(map[string]any{ + "dry_run": true, + "executed": false, + "tool": "query_records", + "arguments": params, + }) + } limit := 100 if rt.Changed("limit") { limit = rt.Int("limit") @@ -168,6 +181,9 @@ func executeRecordQuery(rt *shortcut.RuntimeContext, params map[string]any) erro if window.NextCursor != "" { data["nextCursor"] = window.NextCursor } + if window.HasTotalCount { + data["totalCount"] = window.TotalCount + } return rt.Output(map[string]any{ "success": true, "status": "success", @@ -175,6 +191,19 @@ func executeRecordQuery(rt *shortcut.RuntimeContext, params map[string]any) erro }) } +func responseTotalCount(data map[string]any) (any, bool) { + for { + if totalCount, exists := data["totalCount"]; exists { + return totalCount, true + } + nested, ok := data["data"].(map[string]any) + if !ok { + return nil, false + } + data = nested + } +} + func recordQueryRequestedIDs(params map[string]any) ([]string, bool, error) { raw, exists := params["recordIds"] if !exists { diff --git a/internal/shortcut/aitable/record_upsert_by_key_e2e_test.go b/internal/shortcut/aitable/record_upsert_by_key_e2e_test.go index 3a3800d2..6af79e58 100644 --- a/internal/shortcut/aitable/record_upsert_by_key_e2e_test.go +++ b/internal/shortcut/aitable/record_upsert_by_key_e2e_test.go @@ -280,7 +280,7 @@ func pagedRecordQueryResponse(t *testing.T, records []map[string]any, args map[s } end := minInt(offset+limit, len(records)) pageRecords := records[offset:end] - data := map[string]any{"records": pageRecords, "hasMore": end < len(records)} + data := map[string]any{"records": pageRecords, "hasMore": end < len(records), "totalCount": len(records)} if end < len(records) { data["nextCursor"] = fmt.Sprintf("offset-%d", end) } @@ -337,7 +337,7 @@ func TestCrossPlatformCoverageRecordQueryServicePageBoundariesE2E(t *testing.T) t.Fatalf("record query size %d: %v", size, err) } data, ok := payload["data"].(map[string]any) - if !ok || payload["success"] != true || data["hasMore"] != false || len(data["records"].([]any)) != size { + if !ok || payload["success"] != true || data["hasMore"] != false || data["totalCount"] != float64(size) || len(data["records"].([]any)) != size { t.Fatalf("record query size %d payload = %#v", size, payload) } wantCalls := (size + recordQueryServicePageSize - 1) / recordQueryServicePageSize @@ -348,6 +348,22 @@ func TestCrossPlatformCoverageRecordQueryServicePageBoundariesE2E(t *testing.T) } } +func TestCrossPlatformCoverageRecordQueryDryRunStopsBeforeTransportE2E(t *testing.T) { + caller := &upsertByKeyCaller{} + payload, err := runRecordQueryShortcutCLI(t, caller, 5, "--query", "needle", "--dry-run") + if err != nil { + t.Fatalf("record query dry-run error = %v", err) + } + if len(caller.calls) != 0 { + t.Fatalf("record query dry-run crossed transport: %#v", caller.calls) + } + arguments, _ := payload["arguments"].(map[string]any) + if payload["dry_run"] != true || payload["executed"] != false || payload["tool"] != "query_records" || + arguments["baseId"] != "base" || arguments["tableId"] != "table" || arguments["keyword"] != "needle" || arguments["limit"] != float64(5) { + t.Fatalf("record query dry-run payload = %#v", payload) + } +} + func TestCrossPlatformCoverageRecordQueryExactIDsIgnoreResidualContinuationE2E(t *testing.T) { caller := &upsertByKeyCaller{callFn: func(call int, _, tool string, args map[string]any) (string, error) { if tool != "query_records" { @@ -538,15 +554,16 @@ func TestCrossPlatformCoverageRecordQueryFailureAndContinuationBranches(t *testi } }) - t.Run("delete readback rejects continuation", func(t *testing.T) { + t.Run("delete readback ignores residual continuation", func(t *testing.T) { caller := &upsertByKeyCaller{callFn: func(_ int, _, tool string, _ map[string]any) (string, error) { if tool == "query_records" { return `{"success":true,"data":{"records":[],"hasMore":true,"nextCursor":"unexpected"}}`, nil } return `{"deletedCount":1}`, nil }} - if _, err := runRecordDeleteCLI(t, caller, []string{"r1"}); err == nil || len(caller.calls) != 2 || caller.calls[1].tool != "query_records" { - t.Fatalf("delete continuation error = %v calls=%#v", err, caller.calls) + out, err := runRecordDeleteCLI(t, caller, []string{"r1"}) + if err != nil || out == "" || len(caller.calls) != 2 || caller.calls[1].tool != "query_records" { + t.Fatalf("delete residual continuation output=%q error=%v calls=%#v", out, err, caller.calls) } }) } From 5a414999efc37945609687b400ebbc7b848904eb Mon Sep 17 00:00:00 2001 From: Dennis Date: Mon, 17 Aug 2026 11:34:28 +0800 Subject: [PATCH 19/32] fix: validate record query previews --- internal/shortcut/aitable/record_query_all.go | 16 +++++----- .../aitable/record_upsert_by_key_e2e_test.go | 30 +++++++++++++++++++ 2 files changed, 38 insertions(+), 8 deletions(-) diff --git a/internal/shortcut/aitable/record_query_all.go b/internal/shortcut/aitable/record_query_all.go index eadec9f6..95ef48b4 100644 --- a/internal/shortcut/aitable/record_query_all.go +++ b/internal/shortcut/aitable/record_query_all.go @@ -124,14 +124,6 @@ func explicitEmptyRecordQuery(data map[string]any) bool { } func executeRecordQuery(rt *shortcut.RuntimeContext, params map[string]any) error { - if rt.DryRun() { - return rt.Output(map[string]any{ - "dry_run": true, - "executed": false, - "tool": "query_records", - "arguments": params, - }) - } limit := 100 if rt.Changed("limit") { limit = rt.Int("limit") @@ -151,6 +143,14 @@ func executeRecordQuery(rt *shortcut.RuntimeContext, params map[string]any) erro params["recordIds"] = requestedIDs limit = minInt(limit, len(requestedIDs)) } + if rt.DryRun() { + return rt.Output(map[string]any{ + "dry_run": true, + "executed": false, + "tool": "query_records", + "arguments": params, + }) + } window, err := queryRecordWindow(rt, params, limit) if err != nil { return err diff --git a/internal/shortcut/aitable/record_upsert_by_key_e2e_test.go b/internal/shortcut/aitable/record_upsert_by_key_e2e_test.go index 6af79e58..41872a71 100644 --- a/internal/shortcut/aitable/record_upsert_by_key_e2e_test.go +++ b/internal/shortcut/aitable/record_upsert_by_key_e2e_test.go @@ -364,6 +364,36 @@ func TestCrossPlatformCoverageRecordQueryDryRunStopsBeforeTransportE2E(t *testin } } +func TestCrossPlatformCoverageRecordQueryDryRunRejectsInvalidLocalPlanE2E(t *testing.T) { + for _, testCase := range []struct { + name string + limit int + extra []string + want string + }{ + {name: "zero limit", limit: 0, extra: []string{"--dry-run"}, want: "--limit must be in [1,100]"}, + {name: "excessive limit", limit: recordBatchSize + 1, extra: []string{"--dry-run"}, want: "--limit must be in [1,100]"}, + {name: "empty record IDs", limit: 100, extra: []string{"--record-ids", " ", "--dry-run"}, want: "至少包含一个非空 recordId"}, + { + name: "excessive record IDs", + limit: 100, + extra: []string{"--record-ids", strings.Join(recordIDFixtures(recordBatchSize+1), ","), "--dry-run"}, + want: "at most 100 unique IDs", + }, + } { + t.Run(testCase.name, func(t *testing.T) { + caller := &upsertByKeyCaller{} + payload, err := runRecordQueryShortcutCLI(t, caller, testCase.limit, testCase.extra...) + if err == nil || !strings.Contains(err.Error(), testCase.want) { + t.Fatalf("record query dry-run error = %v, want %q (payload=%#v)", err, testCase.want, payload) + } + if len(caller.calls) != 0 { + t.Fatalf("invalid record query dry-run crossed transport: %#v", caller.calls) + } + }) + } +} + func TestCrossPlatformCoverageRecordQueryExactIDsIgnoreResidualContinuationE2E(t *testing.T) { caller := &upsertByKeyCaller{callFn: func(call int, _, tool string, args map[string]any) (string, error) { if tool != "query_records" { From c1a549cd64b83f4ae312c342d652b0125fba2547 Mon Sep 17 00:00:00 2001 From: Dennis Date: Mon, 17 Aug 2026 13:46:33 +0800 Subject: [PATCH 20/32] fix: close delete readback continuations --- ...1006-aitable-pagination-minutes-unshare.md | 7 ++- internal/shortcut/aitable/record_batch.go | 17 ++---- .../aitable/record_upsert_by_key_e2e_test.go | 60 ++++++++++++++++--- 3 files changed, 61 insertions(+), 23 deletions(-) diff --git a/.changes/1006-aitable-pagination-minutes-unshare.md b/.changes/1006-aitable-pagination-minutes-unshare.md index 9607d987..3275bd7a 100644 --- a/.changes/1006-aitable-pagination-minutes-unshare.md +++ b/.changes/1006-aitable-pagination-minutes-unshare.md @@ -5,6 +5,7 @@ category: Fixed - **Aitable pagination and Minutes unshare verification** (#1006) — keeps record queries on the service's 20-record page boundary so multi-page reads and mutation readbacks no longer report false retryable failures, preserves - `totalCount` when supplied, keeps `--dry-run` transport-free, and rejects - Minutes unshare success until the listening note exists and the service - acknowledges the exact task and member targets. + `totalCount` when supplied, validates `--dry-run` plans before transport, + follows active deletion readback continuations before proving absence, and + rejects Minutes unshare success until the listening note exists and the + service acknowledges the exact task and member targets. diff --git a/internal/shortcut/aitable/record_batch.go b/internal/shortcut/aitable/record_batch.go index f779b32f..63f7d92f 100644 --- a/internal/shortcut/aitable/record_batch.go +++ b/internal/shortcut/aitable/record_batch.go @@ -321,22 +321,13 @@ func queryDeletedRecordsByIDs(rt *shortcut.RuntimeContext, baseID, tableID strin for offset := 0; offset < len(ids); offset += recordQueryServicePageSize { end := minInt(offset+recordQueryServicePageSize, len(ids)) chunk := ids[offset:end] - data, err := rt.CallMCPData(serverMain, "query_records", map[string]any{ - "baseId": baseID, "tableId": tableID, "recordIds": chunk, "limit": len(chunk), - }) + window, err := queryRecordWindow(rt, map[string]any{ + "baseId": baseID, "tableId": tableID, "recordIds": chunk, + }, len(chunk)) if err != nil { return nil, err } - records, found := findRecords(data) - if !found { - if explicitEmptyRecordQuery(data) { - continue - } - return nil, fmt.Errorf("query_records deletion read-back chunk %d is missing records", offset/recordQueryServicePageSize+1) - } - // Exact-ID absence is complete evidence for deletion even when the - // service retains an unrelated continuation marker. - remaining = append(remaining, records...) + remaining = append(remaining, window.Records...) } return remaining, nil } diff --git a/internal/shortcut/aitable/record_upsert_by_key_e2e_test.go b/internal/shortcut/aitable/record_upsert_by_key_e2e_test.go index 41872a71..2fc9333f 100644 --- a/internal/shortcut/aitable/record_upsert_by_key_e2e_test.go +++ b/internal/shortcut/aitable/record_upsert_by_key_e2e_test.go @@ -584,16 +584,62 @@ func TestCrossPlatformCoverageRecordQueryFailureAndContinuationBranches(t *testi } }) - t.Run("delete readback ignores residual continuation", func(t *testing.T) { - caller := &upsertByKeyCaller{callFn: func(_ int, _, tool string, _ map[string]any) (string, error) { - if tool == "query_records" { - return `{"success":true,"data":{"records":[],"hasMore":true,"nextCursor":"unexpected"}}`, nil + t.Run("delete readback follows active continuation to absence", func(t *testing.T) { + caller := &upsertByKeyCaller{callFn: func(call int, _, tool string, args map[string]any) (string, error) { + if tool != "query_records" { + return `{"deletedCount":1}`, nil } - return `{"deletedCount":1}`, nil + if call == 1 { + if _, exists := args["cursor"]; exists { + t.Fatalf("first deletion readback unexpectedly has cursor: %#v", args) + } + return `{"success":true,"data":{"records":[],"hasMore":true,"nextCursor":"c1"}}`, nil + } + if args["cursor"] != "c1" { + t.Fatalf("continued deletion readback cursor = %#v", args["cursor"]) + } + return `{"success":true,"data":{"records":[],"hasMore":false}}`, nil }} out, err := runRecordDeleteCLI(t, caller, []string{"r1"}) - if err != nil || out == "" || len(caller.calls) != 2 || caller.calls[1].tool != "query_records" { - t.Fatalf("delete residual continuation output=%q error=%v calls=%#v", out, err, caller.calls) + if err != nil || out == "" || len(caller.calls) != 3 { + t.Fatalf("delete continued absence output=%q error=%v calls=%#v", out, err, caller.calls) + } + }) + + t.Run("delete readback follows active continuation to remaining record", func(t *testing.T) { + caller := &upsertByKeyCaller{callFn: func(call int, _, tool string, _ map[string]any) (string, error) { + if tool != "query_records" { + return `{"deletedCount":1}`, nil + } + if call == 1 { + return `{"success":true,"data":{"records":[],"hasMore":true,"nextCursor":"c1"}}`, nil + } + return `{"success":true,"data":{"records":[{"recordId":"r1"}],"hasMore":false}}`, nil + }} + out, err := runRecordDeleteCLI(t, caller, []string{"r1"}) + if err == nil || out != "" || len(caller.calls) != 3 { + t.Fatalf("delete continued remaining output=%q error=%v calls=%#v", out, err, caller.calls) + } + var typed *apperrors.Error + if !errors.As(err, &typed) || typed.Reason != "aitable_composite_unknown" { + t.Fatalf("delete continued remaining error = %#v", err) + } + }) + + t.Run("delete readback fails closed on empty continuation stall", func(t *testing.T) { + caller := &upsertByKeyCaller{callFn: func(call int, _, tool string, _ map[string]any) (string, error) { + if tool != "query_records" { + return `{"deletedCount":1}`, nil + } + return fmt.Sprintf(`{"success":true,"data":{"records":[],"hasMore":true,"nextCursor":"c%d"}}`, call), nil + }} + out, err := runRecordDeleteCLI(t, caller, []string{"r1"}) + if err == nil || out != "" || len(caller.calls) != recordQueryMaxConsecutiveEmptyPages+1 { + t.Fatalf("delete stalled continuation output=%q error=%v calls=%#v", out, err, caller.calls) + } + var typed *apperrors.Error + if !errors.As(err, &typed) || typed.Reason != "aitable_composite_unknown" { + t.Fatalf("delete stalled continuation error = %#v", err) } }) } From 93dbd768f70ef93b4015e8c01233996c8364fd98 Mon Sep 17 00:00:00 2001 From: ruigong Date: Mon, 17 Aug 2026 14:18:03 +0800 Subject: [PATCH 21/32] fix(skill): add explicit recent-edited route to drive SOP-1 --- skills/multi/dingtalk-drive/SKILL.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/skills/multi/dingtalk-drive/SKILL.md b/skills/multi/dingtalk-drive/SKILL.md index 7d2beba4..d08e94b5 100644 --- a/skills/multi/dingtalk-drive/SKILL.md +++ b/skills/multi/dingtalk-drive/SKILL.md @@ -49,9 +49,9 @@ metadata: ### SOP-1 找文件（find-file） -**触发**：找文件/搜文件/我的文件/最近文件/某文档在哪。 +**触发**：找文件/搜文件/我的文件/最近文件/最近编辑的文档/某文档在哪。 -1. **选源（必须）**：最近访问 → `dws drive +recent --limit --format json`（翻页用上次返回的 `nextCursor` 传 `--cursor`）；按内容/名称全局搜 → `dws drive +search --query "<关键词>" --format json`；浏览某目录 → `dws drive +list --folder --format json`。 +1. **选源（必须）**：最近访问 → `dws drive +recent --limit --format json`（翻页用上次返回的 `nextCursor` 传 `--cursor`）；最近编辑 → `dws drive +recent --operate-type 1 --limit --format json`（不传 `--operate-type` 默认为 0 最近访问，会混入仅打开过的文档）；按内容/名称全局搜 → `dws drive +search --query "<关键词>" --format json`；浏览某目录 → `dws drive +list --folder --format json`。 2. **解析（必须）**：取真实 `dentryUuid`（= `id`/`nodeId`）；多候选让用户确认，**禁止**默认取第一个。 3. **下钻（必须）**：根目录没命中时，进入最相关文件夹继续 `drive +list --folder`，必要时 `python scripts/drive_tree_list.py --depth 2` 递归，**禁止**只看根目录就放弃。 4. **回读元数据（必须）**：命中后 `dws drive +inspect --node --format json`，按 `extension` 确认类型。 From 3ee66d4373797ec17c4a2191b2eae3ee4d5b2513 Mon Sep 17 00:00:00 2001 From: Dennis Date: Wed, 12 Aug 2026 10:33:00 +0800 Subject: [PATCH 22/32] fix(doc): harden mutation readback verification --- go.mod | 1 + go.sum | 2 + internal/helpers/doc.go | 184 +++++++++- internal/helpers/doc_command_edges_test.go | 116 +++++- internal/shortcut/doc/content_shortcuts.go | 334 ++++++++++++++++-- .../doc/doc_readback_verification_test.go | 275 ++++++++++++++ internal/shortcut/doc/doc_shortcuts_test.go | 15 +- .../doc/history_template_shortcuts.go | 34 +- 8 files changed, 918 insertions(+), 43 deletions(-) create mode 100644 internal/shortcut/doc/doc_readback_verification_test.go diff --git a/go.mod b/go.mod index eb74fd55..62bec438 100644 --- a/go.mod +++ b/go.mod @@ -18,6 +18,7 @@ require ( github.com/muesli/termenv v0.16.0 github.com/open-dingtalk/dingtalk-stream-sdk-go v0.9.2-beta.1 github.com/spf13/cobra v1.10.2 + github.com/yuin/goldmark v1.8.5 github.com/zalando/go-keyring v0.2.8 gitlab.alibaba-inc.com/aes/aem-go-sdk v0.3.0 golang.org/x/crypto v0.49.0 diff --git a/go.sum b/go.sum index 99e029e0..06a5b621 100644 --- a/go.sum +++ b/go.sum @@ -105,6 +105,8 @@ github.com/stretchr/testify v1.11.1 h1:7s2iGBzp5EwR7/aIZr8ao5+dra3wiQyKjjFuvgVKu github.com/stretchr/testify v1.11.1/go.mod h1:wZwfW3scLgRK+23gO65QZefKpKQRnfz6sD981Nm4B6U= github.com/xo/terminfo v0.0.0-20220910002029-abceb7e1c41e h1:JVG44RsyaB9T2KIHavMF/ppJZNG9ZpyihvCd0w101no= github.com/xo/terminfo v0.0.0-20220910002029-abceb7e1c41e/go.mod h1:RbqR21r5mrJuqunuUZ/Dhy/avygyECGrLceyNeo4LiM= +github.com/yuin/goldmark v1.8.5 h1:r6N5afV5qj/5S4UTch8agZHJ8UxNCMwX7WjkkJam2NA= +github.com/yuin/goldmark v1.8.5/go.mod h1:ip/1k0VRfGynBgxOz0yCqHrbZXhcjxyuS66Brc7iBKg= github.com/zalando/go-keyring v0.2.8 h1:6sD/Ucpl7jNq10rM2pgqTs0sZ9V3qMrqfIIy5YPccHs= github.com/zalando/go-keyring v0.2.8/go.mod h1:tsMo+VpRq5NGyKfxoBVjCuMrG47yj8cmakZDO5QGii0= go.yaml.in/yaml/v3 v3.0.4/go.mod h1:DhzuOOF2ATzADvBadXxruRBLzYTpT36CKvDb3+aBEFg= diff --git a/internal/helpers/doc.go b/internal/helpers/doc.go index 1897aa18..d1c9ce4c 100644 --- a/internal/helpers/doc.go +++ b/internal/helpers/doc.go @@ -492,10 +492,11 @@ func defaultHTTPGetFile(ctx context.Context, url string, headers map[string]stri return nil } -// runMediaInsert implements the three-step flow for inserting an attachment into a document: +// runMediaInsert implements the four-step flow for inserting an attachment into a document: // 1. get_doc_attachment_upload_info → obtain uploadUrl + resourceId // 2. HTTP PUT file content to OSS // 3. insert_document_block with attachment element +// 4. list_document_blocks → prove the uploaded resource is visible in the document func runMediaInsert(cmd *cobra.Command, _ []string) error { nodeID, err := mustFlagOrFallback(cmd, "node", "url", "id", "node-id", "doc-id", "file-id") if err != nil { @@ -551,7 +552,7 @@ func runMediaInsert(cmd *cobra.Command, _ []string) error { ctx := cmd.Context() // Step 1: get upload credentials (uploadUrl + resourceId) - deps.Out.PrintInfo(fmt.Sprintf("[1/3] 获取附件上传凭证 (%s, %d bytes)...", fileName, fileSize)) + deps.Out.PrintInfo(fmt.Sprintf("[1/4] 获取附件上传凭证 (%s, %d bytes)...", fileName, fileSize)) credText, err := callMCPToolReturnText(ctx, "get_doc_attachment_upload_info", map[string]any{ "nodeId": nodeID, @@ -569,7 +570,7 @@ func runMediaInsert(cmd *cobra.Command, _ []string) error { } // Step 2: HTTP PUT file to OSS - deps.Out.PrintInfo("[2/3] 上传文件到 OSS...") + deps.Out.PrintInfo("[2/4] 上传文件到 OSS...") ossHeaders := map[string]string{ "Content-Type": mimeType, @@ -592,7 +593,7 @@ func runMediaInsert(cmd *cobra.Command, _ []string) error { } // Step 3: insert block into document - deps.Out.PrintInfo("[3/3] 插入块到文档...") + deps.Out.PrintInfo("[3/4] 插入块到文档...") const maxInlineImageSize = 20 * 1024 * 1024 // 20MB @@ -644,7 +645,8 @@ func runMediaInsert(cmd *cobra.Command, _ []string) error { insertArgs["referenceBlockId"] = v } - if err := callMCPTool("insert_document_block", insertArgs); err != nil { + insertText, err := callMCPToolReturnText(ctx, "insert_document_block", insertArgs) + if err != nil { return apperrors.NewAPI( "附件已上传，但正文 block 插入结果未知；请先检查媒体列表，不要重复上传或插入", apperrors.WithOperation("doc.media_insert"), @@ -665,6 +667,23 @@ func runMediaInsert(cmd *cobra.Command, _ []string) error { apperrors.WithCause(err), ) } + insertResult := map[string]any{} + if strings.TrimSpace(insertText) != "" { + if err := json.Unmarshal([]byte(insertText), &insertResult); err != nil { + return docMediaInsertVerificationError(nodeID, resourceID, resourceURL, fileName, + fmt.Errorf("解析 insert_document_block 响应失败: %w", err)) + } + } + insertedBlockID := nestedDocString(insertResult, "blockId", "elementId", "id") + + deps.Out.PrintInfo("[4/4] 回读验证媒体块...") + verifiedBlockID, verifyErr := verifyInsertedDocMedia(ctx, nodeID, insertedBlockID, resourceID, resourceURL) + if verifyErr != nil { + return docMediaInsertVerificationError(nodeID, resourceID, resourceURL, fileName, verifyErr) + } + if insertedBlockID == "" { + insertedBlockID = verifiedBlockID + } return deps.Out.PrintJSON(map[string]any{ "contractVersion": "doc.operation.v1", @@ -674,16 +693,169 @@ func runMediaInsert(cmd *cobra.Command, _ []string) error { "operation": "doc.media_insert", "data": map[string]any{ "nodeId": nodeID, "resourceId": resourceID, "resourceUrl": resourceURL, - "fileName": fileName, "mimeType": mimeType, "sizeBytes": fileSize, "inserted": true, + "blockId": insertedBlockID, "fileName": fileName, "mimeType": mimeType, "sizeBytes": fileSize, + "inserted": true, "verified": true, }, "steps": []map[string]any{ {"name": "resolve_upload", "status": "success"}, {"name": "upload_oss", "status": "success"}, {"name": "insert_block", "status": "success"}, + {"name": "verify", "status": "success"}, }, }) } +func docMediaInsertVerificationError(nodeID, resourceID, resourceURL, fileName string, cause error) error { + return apperrors.NewAPI( + "附件已上传且插块请求已执行，但回读未能证明媒体块落库；不要直接重试上传或插入", + apperrors.WithOperation("doc.media_insert"), + apperrors.WithReason("doc_media_insert_verification_failed"), + apperrors.WithFailureStage("verify"), + apperrors.WithExecutionStarted(true), + apperrors.WithRetryable(false), + apperrors.WithActions("运行 dws doc +media-list 检查 resourceId", "确认媒体不存在后再决定是否重新执行"), + apperrors.WithDetails(map[string]any{ + "contractVersion": "doc.operation.v1", "status": "partial_success", "nodeId": nodeID, + "resourceId": resourceID, "resourceUrl": resourceURL, "fileName": fileName, "verified": false, + "steps": []map[string]any{ + {"name": "resolve_upload", "status": "success"}, + {"name": "upload_oss", "status": "success"}, + {"name": "insert_block", "status": "success"}, + {"name": "verify", "status": "failed"}, + }, + }), + apperrors.WithCause(cause), + ) +} + +func verifyInsertedDocMedia(ctx context.Context, nodeID, blockID, resourceID, resourceURL string) (string, error) { + delays := []time.Duration{250 * time.Millisecond, 500 * time.Millisecond, time.Second, 2 * time.Second, 4 * time.Second, 8 * time.Second} + var lastErr error + for attempt := 0; attempt <= len(delays); attempt++ { + blocks, err := readAllDocBlocksForVerification(ctx, nodeID) + if err != nil { + lastErr = err + } else { + lastErr = nil + if found := findVerifiedMediaBlock(blocks, blockID, resourceID, resourceURL); found != "" { + return found, nil + } + } + if attempt < len(delays) { + helperSleep(delays[attempt]) + } + } + if lastErr != nil { + return "", fmt.Errorf("媒体资源在有界回读窗口内仍无法读取: %w", lastErr) + } + return "", fmt.Errorf("媒体资源在有界回读窗口内仍不可见") +} + +func readAllDocBlocksForVerification(ctx context.Context, nodeID string) ([]any, error) { + const pageSize = 50 + const maxItems = 5000 + all := make([]any, 0, pageSize) + seen := map[string]bool{} + for start := 0; start < maxItems; start += pageSize { + text, err := callMCPToolReturnTextOnServer(ctx, "doc", "list_document_blocks", map[string]any{ + "nodeId": nodeID, "format": "element", "startIndex": start, "endIndex": start + pageSize - 1, + }) + if err != nil { + return nil, err + } + var payload map[string]any + if err := json.Unmarshal([]byte(text), &payload); err != nil { + return nil, fmt.Errorf("解析 list_document_blocks 回读失败: %w", err) + } + payload = nestedDocMap(payload) + blocks, ok := payload["blocks"].([]any) + if !ok { + return nil, fmt.Errorf("list_document_blocks 回读缺少 blocks 数组") + } + encoded, _ := json.Marshal(blocks) + key := string(encoded) + if key != "[]" && seen[key] { + return nil, fmt.Errorf("list_document_blocks 分页停滞") + } + seen[key] = true + all = append(all, blocks...) + hasMore, hasMoreKnown := payload["hasMore"].(bool) + if hasMoreKnown && !hasMore { + return all, nil + } + if total, ok := docNumberAsInt(payload["totalCount"]); ok && len(all) >= total { + return all, nil + } + if !hasMoreKnown && len(blocks) < pageSize { + return all, nil + } + if len(blocks) == 0 { + return nil, fmt.Errorf("list_document_blocks 声明仍有下一页但当前页为空") + } + } + return nil, fmt.Errorf("文档块超过安全回读上限") +} + +func nestedDocMap(data map[string]any) map[string]any { + for _, key := range []string{"result", "data"} { + if nested, ok := data[key].(map[string]any); ok { + return nestedDocMap(nested) + } + } + return data +} + +func nestedDocString(value any, keys ...string) string { + switch typed := value.(type) { + case map[string]any: + for _, key := range keys { + if text, ok := typed[key].(string); ok && strings.TrimSpace(text) != "" { + return strings.TrimSpace(text) + } + } + for _, child := range typed { + if text := nestedDocString(child, keys...); text != "" { + return text + } + } + case []any: + for _, child := range typed { + if text := nestedDocString(child, keys...); text != "" { + return text + } + } + } + return "" +} + +func findVerifiedMediaBlock(blocks []any, blockID, resourceID, resourceURL string) string { + for _, value := range blocks { + candidateID := nestedDocString(value, "blockId", "id", "uuid") + if candidateID == "" || (blockID != "" && candidateID != blockID) { + continue + } + if resourceID != "" && nestedDocString(value, "resourceId") == resourceID { + return candidateID + } + if resourceID == "" && resourceURL != "" && nestedDocString(value, "resourceUrl") == resourceURL { + return candidateID + } + } + return "" +} + +func docNumberAsInt(value any) (int, bool) { + switch typed := value.(type) { + case float64: + if typed >= 0 && typed == float64(int(typed)) { + return int(typed), true + } + case int: + return typed, typed >= 0 + } + return 0, false +} + // parseAttachmentUploadInfo extracts uploadUrl, resourceId and resourceUrl from the MCP tool response. func parseAttachmentUploadInfo(text string) (uploadURL, resourceID, resourceURL string, err error) { var data map[string]any diff --git a/internal/helpers/doc_command_edges_test.go b/internal/helpers/doc_command_edges_test.go index d781efed..17ab60ca 100644 --- a/internal/helpers/doc_command_edges_test.go +++ b/internal/helpers/doc_command_edges_test.go @@ -4,6 +4,7 @@ import ( "bytes" "context" "errors" + "fmt" "io" "net/http" "net/http/httptest" @@ -13,6 +14,7 @@ import ( "testing" "time" + "github.com/DingTalk-Real-AI/dingtalk-workspace-cli/internal/testseam" "github.com/DingTalk-Real-AI/dingtalk-workspace-cli/pkg/edition" "github.com/spf13/cobra" ) @@ -74,6 +76,7 @@ func TestCrossPlatformCoverageDocUploadAndMediaErrorEdges(t *testing.T) { t.Cleanup(func() { os.Args = oldArgs }) oldPut, oldGet := httpPutFile, httpGetFile t.Cleanup(func() { httpPutFile, httpGetFile = oldPut, oldGet }) + testseam.Swap(t, &helperSleep, func(time.Duration) {}) file := filepath.Join(t.TempDir(), "file.txt") if err := os.WriteFile(file, []byte("content"), 0o600); err != nil { t.Fatal(err) @@ -193,6 +196,16 @@ func TestCrossPlatformCoverageDocUploadAndMediaErrorEdges(t *testing.T) { t.Fatal("media insert failure returned nil") } }) + t.Run("media insert response parse failure", func(t *testing.T) { + httpPutFile = func(context.Context, string, map[string]string, string, int64) error { return nil } + caller := &scriptedToolCaller{steps: []scriptedToolStep{ + {text: `{"uploadUrl":"https://upload","resourceId":"resource"}`}, + {text: `{`}, + }} + if err := mediaCommand(t, caller, file, "text/plain"); err == nil { + t.Fatal("invalid media insert response returned nil") + } + }) t.Run("large image becomes attachment", func(t *testing.T) { httpPutFile = func(context.Context, string, map[string]string, string, int64) error { return nil } large := filepath.Join(t.TempDir(), "large.png") @@ -202,7 +215,11 @@ func TestCrossPlatformCoverageDocUploadAndMediaErrorEdges(t *testing.T) { if err := os.Truncate(large, 21*1024*1024); err != nil { t.Fatal(err) } - caller := &scriptedToolCaller{steps: []scriptedToolStep{{text: `{"uploadUrl":"https://upload","resourceId":"resource","resourceUrl":"https://image"}`}, {text: `{}`}}} + caller := &scriptedToolCaller{steps: []scriptedToolStep{ + {text: `{"uploadUrl":"https://upload","resourceId":"resource","resourceUrl":"https://image"}`}, + {text: `{}`}, + {text: `{"blocks":[{"blockId":"media-block","element":{"attachment":{"resourceId":"resource"}}}],"hasMore":false}`}, + }} if err := mediaCommand(t, caller, large, "image/png"); err != nil { t.Fatal(err) } @@ -240,6 +257,103 @@ func TestCrossPlatformCoverageDefaultDocHTTPTransportEdges(t *testing.T) { } } +func TestCrossPlatformCoverageDocMediaReadbackDefensiveEdges(t *testing.T) { + testseam.Swap(t, &helperSleep, func(time.Duration) {}) + ctx := context.Background() + + for _, tc := range []struct { + name string + steps []scriptedToolStep + }{ + {"call failure", []scriptedToolStep{{err: errors.New("read")}}}, + {"invalid json", []scriptedToolStep{{text: `{`}}}, + {"missing blocks", []scriptedToolStep{{text: `{}`}}}, + {"stalled page", []scriptedToolStep{{text: `{"blocks":[{"id":"a"}],"hasMore":true}`}, {text: `{"blocks":[{"id":"a"}],"hasMore":true}`}}}, + {"empty continued page", []scriptedToolStep{{text: `{"blocks":[],"hasMore":true}`}}}, + } { + t.Run(tc.name, func(t *testing.T) { + installScriptedCaller(t, &scriptedToolCaller{steps: tc.steps}) + if _, err := readAllDocBlocksForVerification(ctx, "node"); err == nil { + t.Fatal("defensive readback returned nil") + } + }) + } + + t.Run("total count and nested payload", func(t *testing.T) { + installScriptedCaller(t, &scriptedToolCaller{steps: []scriptedToolStep{{text: `{"data":{"blocks":[{"id":"a"}],"totalCount":1}}`}}}) + blocks, err := readAllDocBlocksForVerification(ctx, "node") + if err != nil || len(blocks) != 1 { + t.Fatalf("blocks=%#v err=%v", blocks, err) + } + }) + + t.Run("unknown pagination short page", func(t *testing.T) { + installScriptedCaller(t, &scriptedToolCaller{steps: []scriptedToolStep{{text: `{"blocks":[{"id":"a"}]}`}}}) + if blocks, err := readAllDocBlocksForVerification(ctx, "node"); err != nil || len(blocks) != 1 { + t.Fatalf("blocks=%#v err=%v", blocks, err) + } + }) + + t.Run("bounded retry failure", func(t *testing.T) { + installScriptedCaller(t, &scriptedToolCaller{steps: []scriptedToolStep{{text: `{"blocks":[],"hasMore":false}`}}}) + if _, err := verifyInsertedDocMedia(ctx, "node", "", "missing", ""); err == nil { + t.Fatal("missing media unexpectedly verified") + } + }) + + t.Run("transient read failure retries", func(t *testing.T) { + caller := &scriptedToolCaller{steps: []scriptedToolStep{ + {err: errors.New("temporary read failure")}, + {text: `{"blocks":[{"blockId":"media-block","element":{"attachment":{"resourceId":"resource"}}}],"hasMore":false}`}, + }} + installScriptedCaller(t, caller) + blockID, err := verifyInsertedDocMedia(ctx, "node", "media-block", "resource", "") + if err != nil || blockID != "media-block" || caller.calls != 2 { + t.Fatalf("blockID=%q calls=%d err=%v", blockID, caller.calls, err) + } + }) + + t.Run("block identity alone does not verify media", func(t *testing.T) { + installScriptedCaller(t, &scriptedToolCaller{steps: []scriptedToolStep{{text: `{"blocks":[{"blockId":"media-block","element":{"attachment":{"resourceId":"other-resource"}}}],"hasMore":false}`}}}) + if _, err := verifyInsertedDocMedia(ctx, "node", "media-block", "resource", ""); err == nil { + t.Fatal("matching block ID with a different resource unexpectedly verified") + } + }) + + t.Run("block read safety limit", func(t *testing.T) { + steps := make([]scriptedToolStep, 100) + for index := range steps { + steps[index] = scriptedToolStep{text: fmt.Sprintf(`{"blocks":[{"id":"block-%d"}],"hasMore":true}`, index)} + } + installScriptedCaller(t, &scriptedToolCaller{steps: steps}) + if _, err := readAllDocBlocksForVerification(ctx, "node"); err == nil { + t.Fatal("oversized block read returned nil") + } + }) + + if got := nestedDocMap(map[string]any{"result": map[string]any{"data": map[string]any{"ok": true}}}); got["ok"] != true { + t.Fatalf("nested map=%#v", got) + } + if nestedDocString(map[string]any{"x": []any{map[string]any{"id": " nested "}}}, "id") != "nested" || nestedDocString(3, "id") != "" { + t.Fatal("nested string traversal failed") + } + blocks := []any{map[string]any{"id": "block", "resourceId": "rid"}, map[string]any{"id": "url-block", "resourceUrl": "https://media"}} + if findVerifiedMediaBlock(blocks, "block", "rid", "") != "block" || findVerifiedMediaBlock(blocks, "", "rid", "") != "block" || findVerifiedMediaBlock(blocks, "", "", "https://media") != "url-block" || findVerifiedMediaBlock(blocks, "block", "wrong", "") != "" || findVerifiedMediaBlock(blocks, "", "missing", "") != "" { + t.Fatal("media block matching failed") + } + for _, tc := range []struct { + value any + want bool + }{ + {float64(3), true}, {float64(-1), false}, {1.5, false}, {3, true}, {-1, false}, {"3", false}, + } { + _, ok := docNumberAsInt(tc.value) + if ok != tc.want { + t.Fatalf("docNumberAsInt(%#v) ok=%v want=%v", tc.value, ok, tc.want) + } + } +} + func TestCrossPlatformCoverageDocCreateUpdateAndBlockCommandEdges(t *testing.T) { oldDeps, oldArgs, oldPut, oldGet := deps, os.Args, httpPutFile, httpGetFile t.Cleanup(func() { diff --git a/internal/shortcut/doc/content_shortcuts.go b/internal/shortcut/doc/content_shortcuts.go index 6d3d781d..1232088e 100644 --- a/internal/shortcut/doc/content_shortcuts.go +++ b/internal/shortcut/doc/content_shortcuts.go @@ -4,8 +4,10 @@ package doc import ( + "bytes" "encoding/json" "fmt" + "io" "os" "path/filepath" "strings" @@ -18,6 +20,9 @@ import ( "github.com/DingTalk-Real-AI/dingtalk-workspace-cli/internal/localio" "github.com/DingTalk-Real-AI/dingtalk-workspace-cli/internal/shortcut" "github.com/DingTalk-Real-AI/dingtalk-workspace-cli/internal/shortcut/docresolver" + "github.com/yuin/goldmark" + "github.com/yuin/goldmark/extension" + "github.com/yuin/goldmark/renderer/html" ) var ( @@ -28,6 +33,21 @@ var ( docMkdirTemp = os.MkdirTemp docRemoveAll = os.RemoveAll docDownload = localio.Download + docVerifySleep = time.Sleep + docVerifyDelays = []time.Duration{250 * time.Millisecond, 500 * time.Millisecond, time.Second, 2 * time.Second, 4 * time.Second, 8 * time.Second} + docMarkdown = goldmark.New( + goldmark.WithExtensions(extension.Table), + goldmark.WithRendererOptions(html.WithUnsafe()), + ) + docMarkdownConvert = func(source []byte, writer io.Writer) error { + return docMarkdown.Convert(source, writer) + } +) + +const ( + docBlockReadPageSize = 50 + docBlockReadMaxItems = 5000 + docMarkdownVerifyMax = 2 * 1024 * 1024 ) var Create = shortcut.Shortcut{ @@ -133,7 +153,9 @@ var Create = shortcut.Shortcut{ verifyTool = "get_document_content" verifyParams["format"] = format } - verification, err := rt.CallMCPData(productDoc, verifyTool, verifyParams) + verification, err := readDocVerification(rt, verifyTool, verifyParams, func(data map[string]any) bool { + return content == "" || verifyUpdatedDocumentContent(data, content, "overwrite", format) + }) if err != nil { return docVerificationError("doc.create", "verify", nodeID, err, append(steps, map[string]any{"name": "verify", "status": "failed"})) } @@ -422,7 +444,9 @@ var CheckpointUpdate = shortcut.Shortcut{ append(steps, map[string]any{"name": "update", "status": "failed"}, map[string]any{"name": "verify", "status": "not_started"})) } steps = append(steps, map[string]any{"name": "update", "status": "success"}) - verification, err := rt.CallMCPData(productDoc, "get_document_content", map[string]any{"nodeId": rt.Str("node"), "format": "markdown"}) + verification, err := readDocVerification(rt, "get_document_content", map[string]any{"nodeId": rt.Str("node"), "format": "markdown"}, func(data map[string]any) bool { + return verifyUpdatedDocumentContent(data, content, rt.Str("mode"), "markdown") + }) if err != nil { return checkpointPartialWriteError(rt.Str("node"), checkpoint, "verify", "doc_checkpoint_verification_failed", err, append(steps, map[string]any{"name": "verify", "status": "failed"})) @@ -539,7 +563,7 @@ func executeUpdate(rt *shortcut.RuntimeContext) error { } referenceBlockID := rt.Str("after-block-id") return executeVerifiedDocMutation(rt, "doc.update", "insert_document_block", params, node, - "list_document_blocks", map[string]any{"nodeId": node, "format": verificationFormat}, + "list_document_blocks", map[string]any{"nodeId": node, "format": verificationFormat, "__allBlocks": true}, func(result, data map[string]any) bool { return verifyInsertedBlock(result, data, referenceBlockID, content, rt.Str("doc-format")) }) @@ -553,14 +577,14 @@ func executeUpdate(rt *shortcut.RuntimeContext) error { params["element"] = map[string]any{"blockType": "paragraph", "paragraph": map[string]any{"text": content}} } return executeVerifiedDocMutation(rt, "doc.update", "update_document_block", params, node, - "list_document_blocks", map[string]any{"nodeId": node, "blockId": blockID, "format": verificationFormat}, + "list_document_blocks", map[string]any{"nodeId": node, "format": verificationFormat, "__allBlocks": true}, func(_, data map[string]any) bool { return blockContentEquals(data, blockID, content, rt.Str("doc-format")) }) case "block_delete": blockID := rt.Str("block-id") return executeVerifiedDocMutation(rt, "doc.update", "delete_document_block", map[string]any{"nodeId": node, "blockId": blockID}, node, - "list_document_blocks", map[string]any{"nodeId": node, "format": "element"}, + "list_document_blocks", map[string]any{"nodeId": node, "format": "element", "__allBlocks": true}, func(_, data map[string]any) bool { return findBlock(data, blockID) == nil }) case "str_replace": return executePlainTextReplace(rt, node) @@ -609,7 +633,7 @@ func nestedRevision(value any) (int, bool) { } func executePlainTextReplace(rt *shortcut.RuntimeContext, nodeID string) error { - data, err := rt.CallMCPData(productDoc, "list_document_blocks", map[string]any{"nodeId": nodeID, "format": "element"}) + data, err := readAllDocumentBlocks(rt, map[string]any{"nodeId": nodeID, "format": "element"}) if err != nil { return err } @@ -643,12 +667,12 @@ func executePlainTextReplace(rt *shortcut.RuntimeContext, nodeID string) error { blockID := matches[0].blockID return executeVerifiedDocMutation(rt, "doc.update", "update_document_block", map[string]any{"nodeId": nodeID, "blockId": blockID, "element": map[string]any{"blockType": "paragraph", "paragraph": map[string]any{"text": updated}}}, nodeID, - "list_document_blocks", map[string]any{"nodeId": nodeID, "blockId": blockID, "format": "element"}, + "list_document_blocks", map[string]any{"nodeId": nodeID, "format": "element", "__allBlocks": true}, func(_, data map[string]any) bool { return blockContentEquals(data, blockID, updated, "markdown") }) } func executeBlockCopy(rt *shortcut.RuntimeContext, nodeID string) error { - data, err := rt.CallMCPData(productDoc, "list_document_blocks", map[string]any{"nodeId": nodeID, "blockId": rt.Str("block-id"), "format": "element"}) + data, err := readAllDocumentBlocks(rt, map[string]any{"nodeId": nodeID, "format": "element"}) if err != nil { return err } @@ -664,7 +688,7 @@ func executeBlockCopy(rt *shortcut.RuntimeContext, nodeID string) error { referenceBlockID := rt.Str("after-block-id") return executeVerifiedDocMutation(rt, "doc.update", "insert_document_block", map[string]any{"nodeId": nodeID, "referenceBlockId": referenceBlockID, "where": "after", "element": block}, nodeID, - "list_document_blocks", map[string]any{"nodeId": nodeID, "format": "element"}, + "list_document_blocks", map[string]any{"nodeId": nodeID, "format": "element", "__allBlocks": true}, func(result, data map[string]any) bool { return verifyInsertedCanonicalBlock(result, data, referenceBlockID, expectedContent, "markdown") }) @@ -684,7 +708,9 @@ func executeVerifiedDocMutation( return docUnknownWriteError(operation, tool, nodeID, err) } steps[0]["status"] = "success" - verification, err := rt.CallMCPData(productDoc, verifyTool, verifyParams) + verification, err := readDocVerification(rt, verifyTool, verifyParams, func(data map[string]any) bool { + return verify == nil || verify(result, data) + }) if err != nil { return docVerificationError(operation, "verify", nodeID, err, append(steps, map[string]any{"name": "verify", "status": "failed"})) } @@ -732,7 +758,9 @@ func executeVerifiedDocContentMutation(rt *shortcut.RuntimeContext, firstParams } steps = append(steps, map[string]any{"name": stepName, "status": "success"}) } - verification, err := rt.CallMCPData(productDoc, "get_document_content", map[string]any{"nodeId": nodeID, "format": format}) + verification, err := readDocVerification(rt, "get_document_content", map[string]any{"nodeId": nodeID, "format": format}, func(data map[string]any) bool { + return verifyUpdatedDocumentContent(data, content, mode, format) + }) if err != nil { return docVerificationError("doc.update", "verify", nodeID, err, append(steps, map[string]any{"name": "verify", "status": "failed"})) } @@ -745,6 +773,134 @@ func executeVerifiedDocContentMutation(rt *shortcut.RuntimeContext, firstParams }, steps...)) } +func readDocVerification(rt *shortcut.RuntimeContext, tool string, rawParams map[string]any, verify func(map[string]any) bool) (map[string]any, error) { + params := cloneMap(rawParams) + allBlocks, _ := params["__allBlocks"].(bool) + delete(params, "__allBlocks") + var last map[string]any + var lastErr error + for attempt := 0; attempt <= len(docVerifyDelays); attempt++ { + var data map[string]any + var err error + if allBlocks && tool == "list_document_blocks" { + data, err = readAllDocumentBlocks(rt, params) + } else { + data, err = rt.CallMCPData(productDoc, tool, params) + } + if err != nil { + lastErr = err + } else { + last = data + lastErr = nil + if verify == nil || verify(data) { + return data, nil + } + } + if attempt < len(docVerifyDelays) { + docVerifySleep(docVerifyDelays[attempt]) + } + } + if lastErr != nil { + return nil, lastErr + } + return last, nil +} + +func readAllDocumentBlocks(rt *shortcut.RuntimeContext, base map[string]any) (map[string]any, error) { + all := make([]any, 0, docBlockReadPageSize) + seenPages := map[string]bool{} + for start := 0; start < docBlockReadMaxItems; start += docBlockReadPageSize { + params := cloneMap(base) + params["startIndex"] = start + params["endIndex"] = start + docBlockReadPageSize - 1 + page, err := rt.CallMCPData(productDoc, "list_document_blocks", params) + if err != nil { + return nil, err + } + blocks, ok := documentBlockEntries(page) + if !ok { + return nil, fmt.Errorf("list_document_blocks 回读缺少 blocks 数组") + } + encoded, _ := json.Marshal(blocks) + pageKey := string(encoded) + if pageKey != "[]" && seenPages[pageKey] { + return nil, fmt.Errorf("list_document_blocks 分页停滞，无法证明回读完整") + } + seenPages[pageKey] = true + all = append(all, blocks...) + hasMore, known, _ := docPageState(page) + if known && !hasMore { + return map[string]any{"blocks": all, "hasMore": false, "totalCount": len(all)}, nil + } + if total, ok := nestedNonNegativeInt(page, "totalCount", "total_count"); ok && len(all) >= total { + return map[string]any{"blocks": all, "hasMore": false, "totalCount": total}, nil + } + if !known && len(blocks) < docBlockReadPageSize { + return map[string]any{"blocks": all, "hasMore": false, "totalCount": len(all)}, nil + } + if len(blocks) == 0 { + return nil, fmt.Errorf("list_document_blocks 声明仍有下一页但当前页为空，无法证明回读完整") + } + } + return nil, fmt.Errorf("list_document_blocks 超过 %d 个块，无法在安全上限内完成回读", docBlockReadMaxItems) +} + +func documentBlockEntries(value any) ([]any, bool) { + switch typed := value.(type) { + case map[string]any: + for _, key := range []string{"blocks", "items"} { + if blocks, ok := typed[key].([]any); ok { + return blocks, true + } + } + if encoded, ok := typed["jsonml"].(string); ok { + var decoded any + if json.Unmarshal([]byte(encoded), &decoded) == nil { + blocks := orderedJSONMLBlocks(decoded) + values := make([]any, len(blocks)) + for index := range blocks { + values[index] = blocks[index] + } + return values, true + } + } + for _, key := range []string{"result", "data"} { + if nested, ok := typed[key]; ok { + if blocks, found := documentBlockEntries(nested); found { + return blocks, true + } + } + } + } + return nil, false +} + +func nestedNonNegativeInt(value any, keys ...string) (int, bool) { + switch typed := value.(type) { + case map[string]any: + for _, key := range keys { + if raw, ok := typed[key]; ok { + switch number := raw.(type) { + case float64: + if number >= 0 && number == float64(int(number)) { + return int(number), true + } + case int: + if number >= 0 { + return number, true + } + } + } + } + for _, key := range []string{"result", "data"} { + if result, ok := nestedNonNegativeInt(typed[key], keys...); ok { + return result, true + } + } + } + return 0, false +} + func splitDocMarkdown(content string, maxRunes int) []string { if maxRunes <= 0 { return []string{content} @@ -796,11 +952,17 @@ func containsText(value any, needle string) bool { } func verifyUpdatedDocumentContent(value any, expected, mode, format string) bool { - expected = normalizeDocumentContentForVerification(expected, format) + expectedRaw := expected + expected = normalizeDocumentContentForVerification(expectedRaw, format) for _, candidate := range documentContentCandidates(value, format) { - actual := normalizeDocumentContentForVerification(candidate, format) + actualRaw := candidate + actual := normalizeDocumentContentForVerification(actualRaw, format) if mode == "overwrite" { - if actual == expected { + if actual == expected || (format == "markdown" && stripReadbackDocumentTitle(actual) == expected) { + return true + } + if format == "markdown" && (markdownSemanticallyEquivalent(actualRaw, expectedRaw) || + markdownSemanticallyEquivalent(stripReadbackDocumentTitle(actualRaw), expectedRaw)) { return true } continue @@ -808,10 +970,48 @@ func verifyUpdatedDocumentContent(value any, expected, mode, format string) bool if actual == expected || strings.HasSuffix(actual, "\n"+expected) { return true } + if format == "markdown" && markdownSemanticallyEndsWith(actualRaw, expectedRaw) { + return true + } } return false } +func markdownSemanticallyEquivalent(left, right string) bool { + leftFingerprint, leftOK := markdownSemanticFingerprint(left) + rightFingerprint, rightOK := markdownSemanticFingerprint(right) + return leftOK && rightOK && leftFingerprint == rightFingerprint +} + +func markdownSemanticallyEndsWith(content, suffix string) bool { + contentFingerprint, contentOK := markdownSemanticFingerprint(content) + suffixFingerprint, suffixOK := markdownSemanticFingerprint(suffix) + return contentOK && suffixOK && strings.HasSuffix(contentFingerprint, suffixFingerprint) +} + +func markdownSemanticFingerprint(source string) (string, bool) { + if len(source) > docMarkdownVerifyMax { + return "", false + } + var rendered bytes.Buffer + if err := docMarkdownConvert([]byte(source), &rendered); err != nil { + return "", false + } + return rendered.String(), true +} + +func stripReadbackDocumentTitle(content string) string { + lines := strings.Split(content, "\n") + if len(lines) == 0 || !strings.HasPrefix(strings.TrimSpace(lines[0]), "# ") { + return content + } + lines = lines[1:] + for len(lines) > 0 && strings.TrimSpace(lines[0]) == "" { + lines = lines[1:] + } + return strings.Join(lines, "\n") +} + func verifyInsertedBlock(result, data map[string]any, referenceBlockID, expected, format string) bool { return verifyInsertedCanonicalBlock(result, data, referenceBlockID, normalizeDocumentContentForVerification(expected, format), format) } @@ -848,6 +1048,11 @@ func blockContentEquals(data map[string]any, blockID, expected, format string) b } func canonicalBlockContent(value any, format string) string { + if values, ok := value.(map[string]any); ok { + if element, ok := values["element"].(map[string]any); ok { + value = element + } + } if format == "jsonml" { if values, ok := value.(map[string]any); ok { if encoded, ok := values["jsonml"].(string); ok { @@ -871,7 +1076,7 @@ func canonicalBlockContent(value any, format string) string { return } for key, child := range typed { - if key == "id" || key == "blockId" || key == "uuid" { + if key == "id" || key == "blockId" || key == "uuid" || key == "blockType" { continue } walk(child) @@ -1005,6 +1210,10 @@ func orderedDocumentBlocks(value any) []map[string]any { walk = func(current any) { switch typed := current.(type) { case map[string]any: + if element, ok := typed["element"].(map[string]any); ok && blockIdentity(element, "") != "" { + blocks = append(blocks, element) + return + } if blockIdentity(typed, "") != "" { blocks = append(blocks, typed) return @@ -1065,9 +1274,23 @@ func normalizeDocumentContentForVerification(raw, format string) string { func normalizeMarkdownForVerification(raw string) string { raw = strings.ReplaceAll(strings.ReplaceAll(raw, "\r\n", "\n"), "\r", "\n") lines := make([]string, 0, strings.Count(raw, "\n")+1) + inFence := false for _, line := range strings.Split(raw, "\n") { - line = strings.TrimSpace(line) + trimmed := strings.TrimSpace(line) + if strings.HasPrefix(trimmed, "```") || strings.HasPrefix(trimmed, "~~~") { + inFence = !inFence + lines = append(lines, trimmed) + continue + } + if inFence { + lines = append(lines, strings.TrimRight(line, " \t")) + continue + } + line = trimmed if line == "" { + if len(lines) > 0 && lines[len(lines)-1] != "" { + lines = append(lines, "") + } continue } if strings.Contains(line, "|") { @@ -1081,6 +1304,9 @@ func normalizeMarkdownForVerification(raw string) string { } lines = append(lines, line) } + for len(lines) > 0 && lines[len(lines)-1] == "" { + lines = lines[:len(lines)-1] + } return strings.Join(lines, "\n") } @@ -1089,33 +1315,75 @@ func normalizeJSONMLForVerification(raw string) string { if err := json.Unmarshal([]byte(raw), &value); err != nil { return normalizeMarkdownForVerification(raw) } - var tokens []string - var walk func(any) - walk = func(current any) { + var normalize func(any) any + normalize = func(current any) any { switch typed := current.(type) { case []any: - start := 0 - if len(typed) > 0 { - if tag, ok := typed[0].(string); ok { - tokens = append(tokens, "<"+strings.ToLower(strings.TrimSpace(tag))+">") - start = 1 + if len(typed) == 0 { + return []any{} + } + tag, isElement := typed[0].(string) + if !isElement { + out := make([]any, 0, len(typed)) + for _, child := range typed { + out = append(out, normalize(child)) + } + return out + } + start := 1 + attrs := map[string]any{} + if len(typed) > 1 { + if declared, ok := typed[1].(map[string]any); ok { + attrs, _ = normalize(declared).(map[string]any) + start = 2 } } + children := make([]any, 0, len(typed)-start) for _, child := range typed[start:] { - walk(child) + normalized := normalize(child) + if normalized != nil { + children = append(children, normalized) + } } + if strings.EqualFold(tag, "span") && isGeneratedTextSpan(attrs) { + if len(children) == 1 { + return children[0] + } + return children + } + out := []any{strings.ToLower(tag), attrs} + out = append(out, children...) + return out case map[string]any: - // JSONML maps contain element attributes. Server-generated UUIDs and - // default attributes do not change the authored document content. - return - case string: - if text := normalizeMarkdownForVerification(typed); text != "" { - tokens = append(tokens, text) + out := make(map[string]any, len(typed)) + for key, child := range typed { + normalizedKey := strings.ToLower(strings.NewReplacer("_", "", "-", "").Replace(key)) + if normalizedKey == "uuid" || normalizedKey == "blockid" || normalizedKey == "elementid" || normalizedKey == "index" { + continue + } + out[key] = normalize(child) } + return out + case string: + return strings.ReplaceAll(strings.ReplaceAll(typed, "\r\n", "\n"), "\r", "\n") } + return current } - walk(value) - return strings.Join(tokens, "\n") + // normalize only receives values decoded by encoding/json, so the resulting + // tree is always JSON-marshalable. + encoded, _ := json.Marshal(normalize(value)) + return string(encoded) +} + +func isGeneratedTextSpan(attrs map[string]any) bool { + if len(attrs) == 0 { + return true + } + if len(attrs) != 1 { + return false + } + value, ok := attrs["data-type"].(string) + return ok && (value == "text" || value == "leaf") } func executeExport(rt *shortcut.RuntimeContext) error { diff --git a/internal/shortcut/doc/doc_readback_verification_test.go b/internal/shortcut/doc/doc_readback_verification_test.go new file mode 100644 index 00000000..0c5dc10c --- /dev/null +++ b/internal/shortcut/doc/doc_readback_verification_test.go @@ -0,0 +1,275 @@ +// Copyright 2026 Alibaba Group +// Licensed under the Apache License, Version 2.0 + +package doc + +import ( + "errors" + "fmt" + "io" + "strings" + "testing" + "time" + + "github.com/DingTalk-Real-AI/dingtalk-workspace-cli/internal/testseam" +) + +func TestCrossPlatformCoverageDocReadbackRetriesStaleContent(t *testing.T) { + testseam.Swap(t, &docVerifySleep, func(time.Duration) {}) + testseam.Swap(t, &docVerifyDelays, []time.Duration{time.Millisecond}) + caller := &docCoverageCaller{responses: map[string][]map[string]any{ + "get_document_content": {{"markdown": "old"}, {"markdown": "old\nnew"}}, + }} + if err := runDocCoverage(t, Update, caller, "--node", "n", "--command", "append", "--content", "new", "--yes"); err != nil { + t.Fatal(err) + } + reads := 0 + for _, call := range caller.history { + if call.tool == "get_document_content" { + reads++ + } + } + if reads != 2 { + t.Fatalf("readback calls = %d, want 2; history=%#v", reads, caller.history) + } +} + +func TestCrossPlatformCoverageDocDeleteReadbackConsumesEveryPage(t *testing.T) { + testseam.Swap(t, &docVerifySleep, func(time.Duration) {}) + testseam.Swap(t, &docVerifyDelays, []time.Duration{time.Millisecond}) + firstPage := make([]any, 50) + for index := range firstPage { + firstPage[index] = map[string]any{"id": fmt.Sprintf("block-%d", index), "text": "body"} + } + caller := &docCoverageCaller{responses: map[string][]map[string]any{ + "list_document_blocks": { + {"blocks": firstPage, "hasMore": true, "totalCount": 51}, + {"blocks": []any{map[string]any{"id": "target", "text": "stale"}}, "hasMore": false, "totalCount": 51}, + {"blocks": firstPage, "hasMore": false, "totalCount": 50}, + }, + }} + if err := runDocCoverage(t, Update, caller, "--node", "n", "--command", "block_delete", "--block-id", "target", "--yes"); err != nil { + t.Fatal(err) + } + starts := []int{} + for _, call := range caller.history { + if call.tool == "list_document_blocks" { + starts = append(starts, call.params["startIndex"].(int)) + } + } + if fmt.Sprint(starts) != "[0 50 0]" { + t.Fatalf("pagination starts = %v, want [0 50 0]", starts) + } +} + +func TestCrossPlatformCoverageDocReplacePreflightIsGloballyUnique(t *testing.T) { + firstPage := make([]any, 50) + for index := range firstPage { + text := "body" + if index == 0 { + text = "unique needle" + } + firstPage[index] = map[string]any{"id": fmt.Sprintf("block-%d", index), "text": text} + } + caller := &docCoverageCaller{responses: map[string][]map[string]any{ + "list_document_blocks": { + {"blocks": firstPage, "hasMore": true, "totalCount": 51}, + {"blocks": []any{map[string]any{"id": "block-50", "text": "another needle"}}, "hasMore": false, "totalCount": 51}, + }, + }} + err := runDocCoverage(t, Update, caller, "--node", "n", "--command", "str_replace", "--old", "needle", "--new", "changed", "--yes") + if err == nil { + t.Fatal("str_replace accepted a second match on a later page") + } + for _, call := range caller.history { + if call.tool == "update_document_block" { + t.Fatalf("ambiguous replace executed a write: %#v", caller.history) + } + } +} + +func TestCrossPlatformCoverageDocVerificationPreservesMeaning(t *testing.T) { + expected := "[\"p\",{},\"text\"]" + serverExpanded := "[\"p\",{\"uuid\":\"generated\"},[\"span\",{\"data-type\":\"text\"},[\"span\",{\"data-type\":\"leaf\"},\"text\"]]]" + if normalizeJSONMLForVerification(expected) != normalizeJSONMLForVerification(serverExpanded) { + t.Fatal("generated JSONML text wrappers should not change document meaning") + } + linkA := "[\"a\",{\"href\":\"https://example.com/a\"},\"text\"]" + linkB := "[\"a\",{\"href\":\"https://example.com/b\"},\"text\"]" + if normalizeJSONMLForVerification(linkA) == normalizeJSONMLForVerification(linkB) { + t.Fatal("semantic JSONML attributes were ignored") + } + codeA := "~~~go\n return nil\n~~~" + codeB := "~~~go\nreturn nil\n~~~" + if normalizeMarkdownForVerification(codeA) == normalizeMarkdownForVerification(codeB) { + t.Fatal("fenced code indentation was ignored") + } + if !verifyUpdatedDocumentContent(map[string]any{"markdown": "# Server title\n\nbody"}, "body", "overwrite", "markdown") { + t.Fatal("server-generated document title prevented body verification") + } +} + +func TestCrossPlatformCoverageMarkdownSemanticRoundTrip(t *testing.T) { + input := strings.Join([]string{ + "sales_data.xlsx", + "### 1.", + "+10.22%", + "| name | value |", + "| -------- | -------- |", + "| sales_data.xlsx | +10.22% |", + }, "\n") + server := strings.Join([]string{ + `sales\_data.xlsx`, + `### 1\.`, + `\+10.22%`, + "|name|value|", + "|---|---|", + `|sales\_data.xlsx|\+10.22%|`, + }, "\n") + if !markdownSemanticallyEquivalent(input, server) { + t.Fatal("server Markdown escaping and table delimiter normalization changed the semantic fingerprint") + } + if !verifyUpdatedDocumentContent(map[string]any{"markdown": server}, input, "overwrite", "markdown") { + t.Fatal("equivalent server Markdown failed overwrite verification") + } + if !verifyUpdatedDocumentContent(map[string]any{"markdown": "existing\n\n" + server}, input, "append", "markdown") { + t.Fatal("equivalent server Markdown failed append verification") + } +} + +func TestCrossPlatformCoverageMarkdownSemanticDifferencesRemainStrict(t *testing.T) { + for _, test := range []struct { + name string + left string + right string + }{ + {name: "emphasis", left: `*important*`, right: `\*important\*`}, + {name: "inline code", left: "`sales_data`", right: "`sales\\_data`"}, + {name: "fenced code", left: "```\nsales_data\n```", right: "```\nsales\\_data\n```"}, + {name: "table alignment", left: "|a|\n|---|\n|x|", right: "|a|\n|:---|\n|x|"}, + {name: "table columns", left: "|a|b|\n|---|---|\n|x|y|", right: "|a|\n|---|\n|x|"}, + {name: "table content", left: "|a|\n|---|\n|x|", right: "|a|\n|---|\n|y|"}, + } { + t.Run(test.name, func(t *testing.T) { + if markdownSemanticallyEquivalent(test.left, test.right) { + t.Fatal("meaningful Markdown difference was ignored") + } + }) + } + oversized := strings.Repeat("x", docMarkdownVerifyMax+1) + if _, ok := markdownSemanticFingerprint(oversized); ok { + t.Fatal("oversized Markdown entered semantic verification") + } + testseam.Swap(t, &docMarkdownConvert, func([]byte, io.Writer) error { return errors.New("render") }) + if _, ok := markdownSemanticFingerprint("body"); ok { + t.Fatal("failed Markdown render produced a semantic fingerprint") + } +} + +func TestCrossPlatformCoverageDocElementReadbackUsesNestedElement(t *testing.T) { + wrapper := map[string]any{ + "blockType": "paragraph", + "element": map[string]any{"id": "inserted", "blockType": "paragraph", "paragraph": map[string]any{"text": "body"}}, + } + if got := canonicalBlockContent(wrapper, "markdown"); got != "body" { + t.Fatalf("nested element content = %q, want body", got) + } + blocks := orderedDocumentBlocks(map[string]any{"blocks": []any{wrapper}}) + if len(blocks) != 1 || blockIdentity(blocks[0], "") != "inserted" { + t.Fatalf("nested element blocks = %#v", blocks) + } +} + +func TestCrossPlatformCoverageVersionRevertRequiresTargetEvidence(t *testing.T) { + if revertResultMatchesVersion(map[string]any{"ok": true}, 3) || currentDocumentMatchesRestoredVersion(map[string]any{"version": 99}, 3) { + t.Fatal("readability or an unrelated current version must not prove a revert") + } + if !revertResultMatchesVersion(map[string]any{"revertedToVersion": 3}, 3) { + t.Fatal("explicit target-version acknowledgement was not accepted") + } +} + +func TestCrossPlatformCoverageDocReadbackDefensiveEdges(t *testing.T) { + testseam.Swap(t, &docVerifySleep, func(time.Duration) {}) + for _, tc := range []struct { + name string + responses []map[string]any + failAt int + }{ + {"call failure", nil, 1}, + {"missing blocks", []map[string]any{{"ok": true}}, 0}, + {"stalled page", []map[string]any{{"blocks": []any{map[string]any{"id": "a"}}, "hasMore": true}, {"blocks": []any{map[string]any{"id": "a"}}, "hasMore": true}}, 0}, + {"empty continued page", []map[string]any{{"blocks": []any{}, "hasMore": true}}, 0}, + } { + t.Run(tc.name, func(t *testing.T) { + testseam.Swap(t, &docVerifyDelays, []time.Duration{}) + caller := &docCoverageCaller{failAt: tc.failAt, responses: map[string][]map[string]any{"list_document_blocks": tc.responses}} + err := runDocCoverage(t, Update, caller, "--node", "n", "--command", "block_delete", "--block-id", "target", "--yes") + if err == nil { + t.Fatal("defensive readback unexpectedly succeeded") + } + }) + } + + t.Run("total count terminates pagination", func(t *testing.T) { + testseam.Swap(t, &docVerifyDelays, []time.Duration{}) + caller := &docCoverageCaller{responses: map[string][]map[string]any{"list_document_blocks": {{"blocks": []any{map[string]any{"id": "other"}}, "totalCount": 1}}}} + if err := runDocCoverage(t, Update, caller, "--node", "n", "--command", "block_delete", "--block-id", "target", "--yes"); err != nil { + t.Fatal(err) + } + }) + + t.Run("block read safety limit", func(t *testing.T) { + testseam.Swap(t, &docVerifyDelays, []time.Duration{}) + pages := make([]map[string]any, docBlockReadMaxItems/docBlockReadPageSize) + for index := range pages { + pages[index] = map[string]any{"blocks": []any{map[string]any{"id": fmt.Sprintf("block-%d", index)}}, "hasMore": true} + } + caller := &docCoverageCaller{responses: map[string][]map[string]any{"list_document_blocks": pages}} + if err := runDocCoverage(t, Update, caller, "--node", "n", "--command", "block_delete", "--block-id", "target", "--yes"); err == nil { + t.Fatal("oversized block read returned nil") + } + }) + + if blocks, ok := documentBlockEntries(map[string]any{"jsonml": `["root",{},["p",{"uuid":"a"},"x"]]`}); !ok || len(blocks) == 0 { + t.Fatalf("jsonml blocks=%#v ok=%v", blocks, ok) + } + if _, ok := documentBlockEntries(map[string]any{"jsonml": `{`}); ok { + t.Fatal("invalid jsonml produced blocks") + } + if blocks, ok := documentBlockEntries(map[string]any{"data": map[string]any{"items": []any{"x"}}}); !ok || len(blocks) != 1 { + t.Fatalf("nested items=%#v ok=%v", blocks, ok) + } + if _, ok := documentBlockEntries(nil); ok { + t.Fatal("nil produced blocks") + } + + for _, tc := range []struct { + value any + want bool + }{ + {map[string]any{"totalCount": float64(2)}, true}, + {map[string]any{"totalCount": float64(-1)}, false}, + {map[string]any{"totalCount": 2.5}, false}, + {map[string]any{"data": map[string]any{"total_count": 2}}, true}, + {map[string]any{"totalCount": -1}, false}, + {nil, false}, + } { + _, ok := nestedNonNegativeInt(tc.value, "totalCount", "total_count") + if ok != tc.want { + t.Fatalf("nestedNonNegativeInt(%#v) ok=%v want=%v", tc.value, ok, tc.want) + } + } + + for _, raw := range []string{ + `[]`, `[1,["p",{},"x"]]`, `["span",{},"a","b"]`, + `["p",{"block_id":"x","custom":true},"x"]`, `true`, + } { + if normalizeJSONMLForVerification(raw) == "" { + t.Fatalf("empty normalized JSONML for %s", raw) + } + } + if !isGeneratedTextSpan(nil) || isGeneratedTextSpan(map[string]any{"a": 1, "b": 2}) || isGeneratedTextSpan(map[string]any{"data-type": 3}) || isGeneratedTextSpan(map[string]any{"data-type": "other"}) { + t.Fatal("generated span classification failed") + } +} diff --git a/internal/shortcut/doc/doc_shortcuts_test.go b/internal/shortcut/doc/doc_shortcuts_test.go index 0ef06fab..81e2e238 100644 --- a/internal/shortcut/doc/doc_shortcuts_test.go +++ b/internal/shortcut/doc/doc_shortcuts_test.go @@ -15,6 +15,7 @@ import ( "strings" "sync" "testing" + "time" "unicode/utf8" "github.com/DingTalk-Real-AI/dingtalk-workspace-cli/internal/corecmd" @@ -112,6 +113,7 @@ func runDocCoverageInput(t *testing.T, declaration shortcut.Shortcut, caller *do func runDocCoveragePath(t *testing.T, declaration shortcut.Shortcut, caller *docCoverageCaller, input io.Reader, commandPath string, args ...string) error { t.Helper() + testseam.Swap(t, &docVerifySleep, func(time.Duration) {}) helpers.InitDeps(caller) root := &cobra.Command{Use: "dws", SilenceErrors: true, SilenceUsage: true} root.PersistentFlags().Bool("yes", false, "") @@ -491,6 +493,7 @@ func TestCrossPlatformCoverageDocUpdateAliasReachesNestedBranches(t *testing.T) } func TestCrossPlatformCoverageDocWritesStopOnUnknownCommitAndRequireVerification(t *testing.T) { + testseam.Swap(t, &docVerifyDelays, []time.Duration{}) unknown := &docCoverageCaller{failAt: 1, responses: map[string][]map[string]any{}} err := runDocCoverage(t, Update, unknown, "--node", "n", "--command", "append", "--content", "x", "--yes") var typed *apperrors.Error @@ -512,6 +515,7 @@ func TestCrossPlatformCoverageDocWritesStopOnUnknownCommitAndRequireVerification } func TestCrossPlatformCoverageDocCreateRejectsSuccessfulMismatchedReadback(t *testing.T) { + testseam.Swap(t, &docVerifyDelays, []time.Duration{}) caller := &docCoverageCaller{responses: map[string][]map[string]any{ "get_document_content": {{"markdown": "truncated"}}, }} @@ -577,8 +581,15 @@ func TestCrossPlatformCoverageDocVersionRevertPaginationAndVerification(t *testi "revert_doc_version": {{}}, "get_document_info": {{"nodeId": "n", "revision": 99.0}}, }} - if err := runDocCoverage(t, VersionRevert, caller, "--node", "n", "--version", "3", "--yes"); err != nil { - t.Fatal(err) + err := runDocCoverage(t, VersionRevert, caller, "--node", "n", "--version", "3", "--yes") + var typed *apperrors.Error + if !errors.As(err, &typed) || typed.Reason != "doc_history_revert_target_unproven" || typed.FailureStage != "verify" || typed.Details["status"] != "partial_success" { + t.Fatalf("unproven revert error = %#v", err) + } + data, _ := typed.Details["data"].(map[string]any) + steps, _ := typed.Details["steps"].([]map[string]any) + if data["verified"] != false || len(steps) != 3 || steps[2]["status"] != "failed" { + t.Fatalf("unproven revert details = %#v", typed.Details) } }) diff --git a/internal/shortcut/doc/history_template_shortcuts.go b/internal/shortcut/doc/history_template_shortcuts.go index ea9ac94d..048ae9bd 100644 --- a/internal/shortcut/doc/history_template_shortcuts.go +++ b/internal/shortcut/doc/history_template_shortcuts.go @@ -178,15 +178,47 @@ func executeHistoryRevert(rt *shortcut.RuntimeContext) error { map[string]any{"available": false, "reason": "the requested revert completed; verify the current document before any further write"}, ) } + verified := revertResultMatchesVersion(reverted, target) || currentDocumentMatchesRestoredVersion(current, target) + if !verified { + return docPartialWriteError( + "doc.history_revert", "doc_history_revert_target_unproven", "verify", + fmt.Sprintf("版本 %d 的回滚请求已执行且文档可读，但响应没有提供目标版本证据；不要直接重试回滚", target), + fmt.Errorf("回读缺少目标版本 %d 的明确证据", target), + map[string]any{ + "nodeId": nodeID, "version": target, "reverted": true, "verified": false, + "revertResult": reverted, "current": current, + }, + []map[string]any{ + {"name": "preflight", "status": "success"}, + {"name": "revert", "status": "success"}, + {"name": "verify", "status": "failed"}, + }, + map[string]any{"available": false, "reason": "the revert may have completed; inspect version history before any further revert"}, + ) + } return rt.Output(docEnvelope("doc.history_revert", map[string]any{ "version": target, "revertResult": reverted, "current": current, "verified": true, - "verification": "revert_acknowledged_and_document_readable", + "verification": "target_version_proven", }, map[string]any{"name": "preflight", "status": "success"}, map[string]any{"name": "revert", "status": "success"}, map[string]any{"name": "verify", "status": "success"})) } +func revertResultMatchesVersion(value map[string]any, target int) bool { + return versionEvidenceMatches(value, target, map[string]bool{ + "version": true, "targetversion": true, "appliedversion": true, + "restoredversion": true, "revertedversion": true, "revertedtoversion": true, "sourceversion": true, + }) +} + +func currentDocumentMatchesRestoredVersion(value map[string]any, target int) bool { + return versionEvidenceMatches(value, target, map[string]bool{ + "restoredfromversion": true, "revertedfromversion": true, + "sourceversion": true, "appliedversion": true, "targetversion": true, + }) +} + func findHistoryVersion(rt *shortcut.RuntimeContext, nodeID string, target int) (bool, error) { const maxPages = 20 cursor := "" From c6ebe307cd843cefda01448fc3417389cfa62ccd Mon Sep 17 00:00:00 2001 From: Dennis Date: Wed, 12 Aug 2026 19:46:52 +0800 Subject: [PATCH 23/32] fix(doc): verify inline media from jsonml readback --- internal/helpers/doc.go | 23 ++++++++++++++++++--- internal/helpers/doc_command_edges_test.go | 24 ++++++++++++++++++++-- 2 files changed, 42 insertions(+), 5 deletions(-) diff --git a/internal/helpers/doc.go b/internal/helpers/doc.go index d1c9ce4c..9a86e5e5 100644 --- a/internal/helpers/doc.go +++ b/internal/helpers/doc.go @@ -758,7 +758,7 @@ func readAllDocBlocksForVerification(ctx context.Context, nodeID string) ([]any, seen := map[string]bool{} for start := 0; start < maxItems; start += pageSize { text, err := callMCPToolReturnTextOnServer(ctx, "doc", "list_document_blocks", map[string]any{ - "nodeId": nodeID, "format": "element", "startIndex": start, "endIndex": start + pageSize - 1, + "nodeId": nodeID, "format": "jsonml", "startIndex": start, "endIndex": start + pageSize - 1, }) if err != nil { return nil, err @@ -834,16 +834,33 @@ func findVerifiedMediaBlock(blocks []any, blockID, resourceID, resourceURL strin if candidateID == "" || (blockID != "" && candidateID != blockID) { continue } - if resourceID != "" && nestedDocString(value, "resourceId") == resourceID { + mediaValue := docMediaReadbackValue(value) + if resourceID != "" && nestedDocString(mediaValue, "resourceId") == resourceID { return candidateID } - if resourceID == "" && resourceURL != "" && nestedDocString(value, "resourceUrl") == resourceURL { + if resourceURL != "" && nestedDocString(mediaValue, "resourceUrl", "src") == resourceURL { return candidateID } } return "" } +func docMediaReadbackValue(value any) any { + block, ok := value.(map[string]any) + if !ok { + return value + } + encoded, ok := block["jsonml"].(string) + if !ok || strings.TrimSpace(encoded) == "" { + return value + } + var decoded any + if json.Unmarshal([]byte(encoded), &decoded) != nil { + return value + } + return decoded +} + func docNumberAsInt(value any) (int, bool) { switch typed := value.(type) { case float64: diff --git a/internal/helpers/doc_command_edges_test.go b/internal/helpers/doc_command_edges_test.go index 17ab60ca..e4c21f25 100644 --- a/internal/helpers/doc_command_edges_test.go +++ b/internal/helpers/doc_command_edges_test.go @@ -206,6 +206,23 @@ func TestCrossPlatformCoverageDocUploadAndMediaErrorEdges(t *testing.T) { t.Fatal("invalid media insert response returned nil") } }) + t.Run("small image verifies src readback when upload also returns resource ID", func(t *testing.T) { + httpPutFile = func(context.Context, string, map[string]string, string, int64) error { return nil } + caller := &scriptedToolCaller{steps: []scriptedToolStep{ + {text: `{"uploadUrl":"https://upload","resourceId":"resource","resourceUrl":"https://image"}`}, + {text: `{"blockId":"media-block"}`}, + {text: `{"blocks":[{"blockId":"media-block","jsonml":"[\"p\",{\"uuid\":\"media-block\"},[\"img\",{\"src\":\"https://image\"}]]"}],"hasMore":false}`}, + }} + if err := mediaCommand(t, caller, file, "image/png"); err != nil { + t.Fatal(err) + } + if caller.calls != 3 { + t.Fatalf("media insert calls = %d, want credential, insert, and one readback", caller.calls) + } + if caller.args["format"] != "jsonml" { + t.Fatalf("media readback format = %#v, want jsonml", caller.args["format"]) + } + }) t.Run("large image becomes attachment", func(t *testing.T) { httpPutFile = func(context.Context, string, map[string]string, string, int64) error { return nil } large := filepath.Join(t.TempDir(), "large.png") @@ -337,10 +354,13 @@ func TestCrossPlatformCoverageDocMediaReadbackDefensiveEdges(t *testing.T) { if nestedDocString(map[string]any{"x": []any{map[string]any{"id": " nested "}}}, "id") != "nested" || nestedDocString(3, "id") != "" { t.Fatal("nested string traversal failed") } - blocks := []any{map[string]any{"id": "block", "resourceId": "rid"}, map[string]any{"id": "url-block", "resourceUrl": "https://media"}} - if findVerifiedMediaBlock(blocks, "block", "rid", "") != "block" || findVerifiedMediaBlock(blocks, "", "rid", "") != "block" || findVerifiedMediaBlock(blocks, "", "", "https://media") != "url-block" || findVerifiedMediaBlock(blocks, "block", "wrong", "") != "" || findVerifiedMediaBlock(blocks, "", "missing", "") != "" { + blocks := []any{map[string]any{"id": "block", "resourceId": "rid"}, map[string]any{"id": "url-block", "resourceUrl": "https://media"}, map[string]any{"id": "src-block", "jsonml": `["p",{},["img",{"src":"https://image"}]]`}} + if findVerifiedMediaBlock(blocks, "block", "rid", "") != "block" || findVerifiedMediaBlock(blocks, "", "rid", "") != "block" || findVerifiedMediaBlock(blocks, "", "", "https://media") != "url-block" || findVerifiedMediaBlock(blocks, "src-block", "upload-resource", "https://image") != "src-block" || findVerifiedMediaBlock(blocks, "other-block", "upload-resource", "https://image") != "" || findVerifiedMediaBlock(blocks, "block", "wrong", "") != "" || findVerifiedMediaBlock(blocks, "", "missing", "") != "" { t.Fatal("media block matching failed") } + if findVerifiedMediaBlock([]any{map[string]any{"id": "bad-jsonml", "jsonml": `[`}}, "bad-jsonml", "", "https://image") != "" { + t.Fatal("invalid JSONML unexpectedly verified") + } for _, tc := range []struct { value any want bool From 1083093cbce1cd3647341e22fa37be7c14ad13c7 Mon Sep 17 00:00:00 2001 From: Dennis Date: Thu, 13 Aug 2026 00:13:27 +0800 Subject: [PATCH 24/32] test(doc): complete readback coverage evidence --- .changes/960-doc-readback-verification.md | 5 +++++ internal/helpers/doc_command_edges_test.go | 3 +++ 2 files changed, 8 insertions(+) create mode 100644 .changes/960-doc-readback-verification.md diff --git a/.changes/960-doc-readback-verification.md b/.changes/960-doc-readback-verification.md new file mode 100644 index 00000000..06467959 --- /dev/null +++ b/.changes/960-doc-readback-verification.md @@ -0,0 +1,5 @@ +--- +category: Fixed +--- + +- **Document write verification** (#960) — avoids false partial-success results when normalized Markdown, paginated blocks, inline images, or version reverts are confirmed by server readback. diff --git a/internal/helpers/doc_command_edges_test.go b/internal/helpers/doc_command_edges_test.go index e4c21f25..a9e775d6 100644 --- a/internal/helpers/doc_command_edges_test.go +++ b/internal/helpers/doc_command_edges_test.go @@ -361,6 +361,9 @@ func TestCrossPlatformCoverageDocMediaReadbackDefensiveEdges(t *testing.T) { if findVerifiedMediaBlock([]any{map[string]any{"id": "bad-jsonml", "jsonml": `[`}}, "bad-jsonml", "", "https://image") != "" { t.Fatal("invalid JSONML unexpectedly verified") } + if got := docMediaReadbackValue("plain"); got != "plain" { + t.Fatalf("non-object media readback = %#v, want unchanged value", got) + } for _, tc := range []struct { value any want bool From a62332be9312b467c790b79c6477f0fc189f1c9d Mon Sep 17 00:00:00 2001 From: Dennis Date: Fri, 14 Aug 2026 10:13:54 +0800 Subject: [PATCH 25/32] fix(doc): trust only explicit inserted block IDs --- internal/helpers/doc.go | 21 ++++++++++++++++++++- internal/helpers/doc_command_edges_test.go | 20 ++++++++++++++++++++ 2 files changed, 40 insertions(+), 1 deletion(-) diff --git a/internal/helpers/doc.go b/internal/helpers/doc.go index 9a86e5e5..815a432d 100644 --- a/internal/helpers/doc.go +++ b/internal/helpers/doc.go @@ -674,7 +674,7 @@ func runMediaInsert(cmd *cobra.Command, _ []string) error { fmt.Errorf("解析 insert_document_block 响应失败: %w", err)) } } - insertedBlockID := nestedDocString(insertResult, "blockId", "elementId", "id") + insertedBlockID := insertedDocBlockID(insertResult) deps.Out.PrintInfo("[4/4] 回读验证媒体块...") verifiedBlockID, verifyErr := verifyInsertedDocMedia(ctx, nodeID, insertedBlockID, resourceID, resourceURL) @@ -828,6 +828,25 @@ func nestedDocString(value any, keys ...string) string { return "" } +// insertedDocBlockID only accepts explicit block IDs from the insert result or +// known response wrappers. Arbitrary IDs may belong to the document, operator, +// or request and must not become a hard constraint for the media readback. +func insertedDocBlockID(data map[string]any) string { + for _, key := range []string{"blockId", "elementId"} { + if text, ok := data[key].(string); ok && strings.TrimSpace(text) != "" { + return strings.TrimSpace(text) + } + } + for _, wrapper := range []string{"result", "data", "content"} { + if inner, ok := data[wrapper].(map[string]any); ok { + if text := insertedDocBlockID(inner); text != "" { + return text + } + } + } + return "" +} + func findVerifiedMediaBlock(blocks []any, blockID, resourceID, resourceURL string) string { for _, value := range blocks { candidateID := nestedDocString(value, "blockId", "id", "uuid") diff --git a/internal/helpers/doc_command_edges_test.go b/internal/helpers/doc_command_edges_test.go index a9e775d6..ef894c38 100644 --- a/internal/helpers/doc_command_edges_test.go +++ b/internal/helpers/doc_command_edges_test.go @@ -223,6 +223,20 @@ func TestCrossPlatformCoverageDocUploadAndMediaErrorEdges(t *testing.T) { t.Fatalf("media readback format = %#v, want jsonml", caller.args["format"]) } }) + t.Run("unrelated insert response IDs do not constrain media readback", func(t *testing.T) { + httpPutFile = func(context.Context, string, map[string]string, string, int64) error { return nil } + caller := &scriptedToolCaller{steps: []scriptedToolStep{ + {text: `{"uploadUrl":"https://upload","resourceId":"resource","resourceUrl":"https://image"}`}, + {text: `{"id":"document-id","operator":{"id":"operator-id"},"data":{"request":{"id":"request-id"}}}`}, + {text: `{"blocks":[{"blockId":"media-block","jsonml":"[\"p\",{\"uuid\":\"media-block\"},[\"img\",{\"src\":\"https://image\"}]]"}],"hasMore":false}`}, + }} + if err := mediaCommand(t, caller, file, "image/png"); err != nil { + t.Fatal(err) + } + if caller.calls != 3 { + t.Fatalf("media insert calls = %d, want credential, insert, and one readback", caller.calls) + } + }) t.Run("large image becomes attachment", func(t *testing.T) { httpPutFile = func(context.Context, string, map[string]string, string, int64) error { return nil } large := filepath.Join(t.TempDir(), "large.png") @@ -354,6 +368,12 @@ func TestCrossPlatformCoverageDocMediaReadbackDefensiveEdges(t *testing.T) { if nestedDocString(map[string]any{"x": []any{map[string]any{"id": " nested "}}}, "id") != "nested" || nestedDocString(3, "id") != "" { t.Fatal("nested string traversal failed") } + if got := insertedDocBlockID(map[string]any{"id": "document", "result": map[string]any{"data": map[string]any{"blockId": " block "}}}); got != "block" { + t.Fatalf("trusted inserted block ID = %q, want block", got) + } + if got := insertedDocBlockID(map[string]any{"id": "document", "operator": map[string]any{"id": "operator"}, "data": map[string]any{"request": map[string]any{"id": "request"}}}); got != "" { + t.Fatalf("untrusted inserted block ID = %q, want empty", got) + } blocks := []any{map[string]any{"id": "block", "resourceId": "rid"}, map[string]any{"id": "url-block", "resourceUrl": "https://media"}, map[string]any{"id": "src-block", "jsonml": `["p",{},["img",{"src":"https://image"}]]`}} if findVerifiedMediaBlock(blocks, "block", "rid", "") != "block" || findVerifiedMediaBlock(blocks, "", "rid", "") != "block" || findVerifiedMediaBlock(blocks, "", "", "https://media") != "url-block" || findVerifiedMediaBlock(blocks, "src-block", "upload-resource", "https://image") != "src-block" || findVerifiedMediaBlock(blocks, "other-block", "upload-resource", "https://image") != "" || findVerifiedMediaBlock(blocks, "block", "wrong", "") != "" || findVerifiedMediaBlock(blocks, "", "missing", "") != "" { t.Fatal("media block matching failed") From 2db73a818565845fef0e5deedb5dd997fabcc7ac Mon Sep 17 00:00:00 2001 From: Dennis Date: Fri, 14 Aug 2026 10:55:00 +0800 Subject: [PATCH 26/32] fix(doc): distinguish identical pagination pages --- internal/helpers/doc.go | 51 +++++++++++++++++-- internal/helpers/doc_command_edges_test.go | 22 ++++++++ internal/shortcut/doc/content_shortcuts.go | 38 ++++++++++++-- .../doc/doc_readback_verification_test.go | 24 +++++++++ 4 files changed, 125 insertions(+), 10 deletions(-) diff --git a/internal/helpers/doc.go b/internal/helpers/doc.go index 815a432d..ecad5e5a 100644 --- a/internal/helpers/doc.go +++ b/internal/helpers/doc.go @@ -755,7 +755,7 @@ func readAllDocBlocksForVerification(ctx context.Context, nodeID string) ([]any, const pageSize = 50 const maxItems = 5000 all := make([]any, 0, pageSize) - seen := map[string]bool{} + seenPageIdentities := map[string]bool{} for start := 0; start < maxItems; start += pageSize { text, err := callMCPToolReturnTextOnServer(ctx, "doc", "list_document_blocks", map[string]any{ "nodeId": nodeID, "format": "jsonml", "startIndex": start, "endIndex": start + pageSize - 1, @@ -772,12 +772,13 @@ func readAllDocBlocksForVerification(ctx context.Context, nodeID string) ([]any, if !ok { return nil, fmt.Errorf("list_document_blocks 回读缺少 blocks 数组") } - encoded, _ := json.Marshal(blocks) - key := string(encoded) - if key != "[]" && seen[key] { + pageIdentity := docBlockPageIdentity(blocks) + if pageIdentity != "" && seenPageIdentities[pageIdentity] { return nil, fmt.Errorf("list_document_blocks 分页停滞") } - seen[key] = true + if pageIdentity != "" { + seenPageIdentities[pageIdentity] = true + } all = append(all, blocks...) hasMore, hasMoreKnown := payload["hasMore"].(bool) if hasMoreKnown && !hasMore { @@ -796,6 +797,46 @@ func readAllDocBlocksForVerification(ctx context.Context, nodeID string) ([]any, return nil, fmt.Errorf("文档块超过安全回读上限") } +func docBlockPageIdentity(blocks []any) string { + if len(blocks) == 0 { + return "" + } + ids := make([]string, 0, len(blocks)) + for _, value := range blocks { + id := "" + switch block := value.(type) { + case map[string]any: + id = directDocBlockIdentity(block) + if id == "" { + if element, ok := block["element"].(map[string]any); ok { + id = directDocBlockIdentity(element) + } + } + case []any: + if len(block) > 1 { + if attributes, ok := block[1].(map[string]any); ok { + id = directDocBlockIdentity(attributes) + } + } + } + if id == "" { + return "" + } + ids = append(ids, id) + } + encoded, _ := json.Marshal(ids) + return string(encoded) +} + +func directDocBlockIdentity(block map[string]any) string { + for _, key := range []string{"blockId", "id", "uuid", "elementId"} { + if text, ok := block[key].(string); ok && strings.TrimSpace(text) != "" { + return strings.TrimSpace(text) + } + } + return "" +} + func nestedDocMap(data map[string]any) map[string]any { for _, key := range []string{"result", "data"} { if nested, ok := data[key].(map[string]any); ok { diff --git a/internal/helpers/doc_command_edges_test.go b/internal/helpers/doc_command_edges_test.go index ef894c38..bfd7aec2 100644 --- a/internal/helpers/doc_command_edges_test.go +++ b/internal/helpers/doc_command_edges_test.go @@ -3,6 +3,7 @@ package helpers import ( "bytes" "context" + "encoding/json" "errors" "fmt" "io" @@ -318,6 +319,27 @@ func TestCrossPlatformCoverageDocMediaReadbackDefensiveEdges(t *testing.T) { } }) + t.Run("identical adjacent pages advance by requested indexes", func(t *testing.T) { + blocks := make([]any, 50) + for index := range blocks { + blocks[index] = map[string]any{"blockType": "paragraph"} + } + first, err := json.Marshal(map[string]any{"blocks": blocks, "hasMore": true, "totalCount": 100}) + if err != nil { + t.Fatal(err) + } + second, err := json.Marshal(map[string]any{"blocks": blocks, "hasMore": false, "totalCount": 100}) + if err != nil { + t.Fatal(err) + } + caller := &scriptedToolCaller{steps: []scriptedToolStep{{text: string(first)}, {text: string(second)}}} + installScriptedCaller(t, caller) + got, err := readAllDocBlocksForVerification(ctx, "node") + if err != nil || len(got) != 100 || caller.calls != 2 { + t.Fatalf("blocks=%d calls=%d err=%v, want 100 blocks from two pages", len(got), caller.calls, err) + } + }) + t.Run("unknown pagination short page", func(t *testing.T) { installScriptedCaller(t, &scriptedToolCaller{steps: []scriptedToolStep{{text: `{"blocks":[{"id":"a"}]}`}}}) if blocks, err := readAllDocBlocksForVerification(ctx, "node"); err != nil || len(blocks) != 1 { diff --git a/internal/shortcut/doc/content_shortcuts.go b/internal/shortcut/doc/content_shortcuts.go index 1232088e..74ae46bd 100644 --- a/internal/shortcut/doc/content_shortcuts.go +++ b/internal/shortcut/doc/content_shortcuts.go @@ -808,7 +808,7 @@ func readDocVerification(rt *shortcut.RuntimeContext, tool string, rawParams map func readAllDocumentBlocks(rt *shortcut.RuntimeContext, base map[string]any) (map[string]any, error) { all := make([]any, 0, docBlockReadPageSize) - seenPages := map[string]bool{} + seenPageIdentities := map[string]bool{} for start := 0; start < docBlockReadMaxItems; start += docBlockReadPageSize { params := cloneMap(base) params["startIndex"] = start @@ -821,12 +821,13 @@ func readAllDocumentBlocks(rt *shortcut.RuntimeContext, base map[string]any) (ma if !ok { return nil, fmt.Errorf("list_document_blocks 回读缺少 blocks 数组") } - encoded, _ := json.Marshal(blocks) - pageKey := string(encoded) - if pageKey != "[]" && seenPages[pageKey] { + pageIdentity := documentBlockPageIdentity(blocks) + if pageIdentity != "" && seenPageIdentities[pageIdentity] { return nil, fmt.Errorf("list_document_blocks 分页停滞，无法证明回读完整") } - seenPages[pageKey] = true + if pageIdentity != "" { + seenPageIdentities[pageIdentity] = true + } all = append(all, blocks...) hasMore, known, _ := docPageState(page) if known && !hasMore { @@ -845,6 +846,33 @@ func readAllDocumentBlocks(rt *shortcut.RuntimeContext, base map[string]any) (ma return nil, fmt.Errorf("list_document_blocks 超过 %d 个块，无法在安全上限内完成回读", docBlockReadMaxItems) } +func documentBlockPageIdentity(blocks []any) string { + if len(blocks) == 0 { + return "" + } + ids := make([]string, 0, len(blocks)) + for _, value := range blocks { + id := "" + switch block := value.(type) { + case map[string]any: + id = blockIdentity(block, "") + if id == "" { + if element, ok := block["element"].(map[string]any); ok { + id = blockIdentity(element, "") + } + } + case []any: + id = jsonMLBlockIdentity(block) + } + if id == "" { + return "" + } + ids = append(ids, id) + } + encoded, _ := json.Marshal(ids) + return string(encoded) +} + func documentBlockEntries(value any) ([]any, bool) { switch typed := value.(type) { case map[string]any: diff --git a/internal/shortcut/doc/doc_readback_verification_test.go b/internal/shortcut/doc/doc_readback_verification_test.go index 0c5dc10c..6f852b4d 100644 --- a/internal/shortcut/doc/doc_readback_verification_test.go +++ b/internal/shortcut/doc/doc_readback_verification_test.go @@ -211,6 +211,30 @@ func TestCrossPlatformCoverageDocReadbackDefensiveEdges(t *testing.T) { }) } + t.Run("identical adjacent pages advance by requested indexes", func(t *testing.T) { + testseam.Swap(t, &docVerifyDelays, []time.Duration{}) + blocks := make([]any, docBlockReadPageSize) + for index := range blocks { + blocks[index] = map[string]any{"blockType": "paragraph"} + } + caller := &docCoverageCaller{responses: map[string][]map[string]any{"list_document_blocks": { + {"blocks": blocks, "hasMore": true, "totalCount": 2 * docBlockReadPageSize}, + {"blocks": blocks, "hasMore": false, "totalCount": 2 * docBlockReadPageSize}, + }}} + if err := runDocCoverage(t, Update, caller, "--node", "n", "--command", "block_delete", "--block-id", "target", "--yes"); err != nil { + t.Fatal(err) + } + starts := []int{} + for _, call := range caller.history { + if call.tool == "list_document_blocks" { + starts = append(starts, call.params["startIndex"].(int)) + } + } + if fmt.Sprint(starts) != "[0 50]" { + t.Fatalf("pagination starts = %v, want [0 50]", starts) + } + }) + t.Run("total count terminates pagination", func(t *testing.T) { testseam.Swap(t, &docVerifyDelays, []time.Duration{}) caller := &docCoverageCaller{responses: map[string][]map[string]any{"list_document_blocks": {{"blocks": []any{map[string]any{"id": "other"}}, "totalCount": 1}}}} From 7ffbbc4a51cad222ddf676fcf2accf568d8c71c8 Mon Sep 17 00:00:00 2001 From: Dennis Date: Fri, 14 Aug 2026 11:21:43 +0800 Subject: [PATCH 27/32] test(doc): cover stable pagination identities --- internal/helpers/doc_command_edges_test.go | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/internal/helpers/doc_command_edges_test.go b/internal/helpers/doc_command_edges_test.go index bfd7aec2..e20d4151 100644 --- a/internal/helpers/doc_command_edges_test.go +++ b/internal/helpers/doc_command_edges_test.go @@ -340,6 +340,15 @@ func TestCrossPlatformCoverageDocMediaReadbackDefensiveEdges(t *testing.T) { } }) + t.Run("page identity accepts nested and JSONML block IDs", func(t *testing.T) { + if got := docBlockPageIdentity([]any{map[string]any{"element": map[string]any{"blockId": "nested-block"}}}); got != `["nested-block"]` { + t.Fatalf("nested block page identity = %q", got) + } + if got := docBlockPageIdentity([]any{[]any{"p", map[string]any{"uuid": "jsonml-block"}}}); got != `["jsonml-block"]` { + t.Fatalf("JSONML block page identity = %q", got) + } + }) + t.Run("unknown pagination short page", func(t *testing.T) { installScriptedCaller(t, &scriptedToolCaller{steps: []scriptedToolStep{{text: `{"blocks":[{"id":"a"}]}`}}}) if blocks, err := readAllDocBlocksForVerification(ctx, "node"); err != nil || len(blocks) != 1 { From 9ed55978d9c4751b3a738984ec37a6c20ba1892a Mon Sep 17 00:00:00 2001 From: Dennis Date: Fri, 14 Aug 2026 12:10:17 +0800 Subject: [PATCH 28/32] fix(doc): cancel readback retry waits --- internal/helpers/doc.go | 20 ++++++++++- internal/helpers/doc_command_edges_test.go | 22 ++++++++++++ internal/helpers/doc_coverage_more_test.go | 2 ++ internal/shortcut/doc/content_shortcuts.go | 21 +++++++++-- .../doc/doc_readback_verification_test.go | 36 +++++++++++++++++-- internal/shortcut/doc/doc_shortcuts_test.go | 2 +- 6 files changed, 96 insertions(+), 7 deletions(-) diff --git a/internal/helpers/doc.go b/internal/helpers/doc.go index ecad5e5a..4c1380e4 100644 --- a/internal/helpers/doc.go +++ b/internal/helpers/doc.go @@ -728,6 +728,8 @@ func docMediaInsertVerificationError(nodeID, resourceID, resourceURL, fileName s ) } +var docMediaVerifyWait = waitForDocVerification + func verifyInsertedDocMedia(ctx context.Context, nodeID, blockID, resourceID, resourceURL string) (string, error) { delays := []time.Duration{250 * time.Millisecond, 500 * time.Millisecond, time.Second, 2 * time.Second, 4 * time.Second, 8 * time.Second} var lastErr error @@ -742,7 +744,9 @@ func verifyInsertedDocMedia(ctx context.Context, nodeID, blockID, resourceID, re } } if attempt < len(delays) { - helperSleep(delays[attempt]) + if err := docMediaVerifyWait(ctx, delays[attempt]); err != nil { + return "", err + } } } if lastErr != nil { @@ -751,6 +755,20 @@ func verifyInsertedDocMedia(ctx context.Context, nodeID, blockID, resourceID, re return "", fmt.Errorf("媒体资源在有界回读窗口内仍不可见") } +func waitForDocVerification(ctx context.Context, delay time.Duration) error { + if ctx == nil { + ctx = context.Background() + } + timer := time.NewTimer(delay) + defer timer.Stop() + select { + case <-ctx.Done(): + return ctx.Err() + case <-timer.C: + return nil + } +} + func readAllDocBlocksForVerification(ctx context.Context, nodeID string) ([]any, error) { const pageSize = 50 const maxItems = 5000 diff --git a/internal/helpers/doc_command_edges_test.go b/internal/helpers/doc_command_edges_test.go index e20d4151..7f478518 100644 --- a/internal/helpers/doc_command_edges_test.go +++ b/internal/helpers/doc_command_edges_test.go @@ -78,6 +78,7 @@ func TestCrossPlatformCoverageDocUploadAndMediaErrorEdges(t *testing.T) { oldPut, oldGet := httpPutFile, httpGetFile t.Cleanup(func() { httpPutFile, httpGetFile = oldPut, oldGet }) testseam.Swap(t, &helperSleep, func(time.Duration) {}) + testseam.Swap(t, &docMediaVerifyWait, func(context.Context, time.Duration) error { return nil }) file := filepath.Join(t.TempDir(), "file.txt") if err := os.WriteFile(file, []byte("content"), 0o600); err != nil { t.Fatal(err) @@ -291,6 +292,7 @@ func TestCrossPlatformCoverageDefaultDocHTTPTransportEdges(t *testing.T) { func TestCrossPlatformCoverageDocMediaReadbackDefensiveEdges(t *testing.T) { testseam.Swap(t, &helperSleep, func(time.Duration) {}) + testseam.Swap(t, &docMediaVerifyWait, func(context.Context, time.Duration) error { return nil }) ctx := context.Background() for _, tc := range []struct { @@ -428,6 +430,26 @@ func TestCrossPlatformCoverageDocMediaReadbackDefensiveEdges(t *testing.T) { } } +func TestCrossPlatformCoverageDocMediaReadbackStopsOnCancellation(t *testing.T) { + if err := waitForDocVerification(nil, time.Nanosecond); err != nil { + t.Fatalf("completed verification wait = %v", err) + } + cancelled, cancel := context.WithCancel(context.Background()) + cancel() + if err := waitForDocVerification(cancelled, time.Hour); !errors.Is(err, context.Canceled) { + t.Fatalf("cancelled verification wait = %v, want context.Canceled", err) + } + + caller := &scriptedToolCaller{steps: []scriptedToolStep{{text: `{"blocks":[],"hasMore":false}`}}} + installScriptedCaller(t, caller) + if _, err := verifyInsertedDocMedia(cancelled, "node", "", "missing", ""); !errors.Is(err, context.Canceled) { + t.Fatalf("cancelled media verification = %v, want context.Canceled", err) + } + if caller.calls != 1 { + t.Fatalf("cancelled media verification calls = %d, want 1", caller.calls) + } +} + func TestCrossPlatformCoverageDocCreateUpdateAndBlockCommandEdges(t *testing.T) { oldDeps, oldArgs, oldPut, oldGet := deps, os.Args, httpPutFile, httpGetFile t.Cleanup(func() { diff --git a/internal/helpers/doc_coverage_more_test.go b/internal/helpers/doc_coverage_more_test.go index c375ac40..674d74f1 100644 --- a/internal/helpers/doc_coverage_more_test.go +++ b/internal/helpers/doc_coverage_more_test.go @@ -9,6 +9,7 @@ import ( "path/filepath" "strings" "testing" + "time" "github.com/DingTalk-Real-AI/dingtalk-workspace-cli/internal/testseam" "github.com/spf13/cobra" @@ -194,6 +195,7 @@ func TestCrossPlatformCoverageDocDeprecationWrappersCoverage(t *testing.T) { } func TestCrossPlatformCoverageRunDocUploadDownloadAndMediaCoverage(t *testing.T) { + testseam.Swap(t, &docMediaVerifyWait, func(context.Context, time.Duration) error { return nil }) oldArgs := os.Args os.Args = []string{"dws", "doc"} t.Cleanup(func() { os.Args = oldArgs }) diff --git a/internal/shortcut/doc/content_shortcuts.go b/internal/shortcut/doc/content_shortcuts.go index 74ae46bd..4a054928 100644 --- a/internal/shortcut/doc/content_shortcuts.go +++ b/internal/shortcut/doc/content_shortcuts.go @@ -5,6 +5,7 @@ package doc import ( "bytes" + "context" "encoding/json" "fmt" "io" @@ -33,7 +34,7 @@ var ( docMkdirTemp = os.MkdirTemp docRemoveAll = os.RemoveAll docDownload = localio.Download - docVerifySleep = time.Sleep + docVerifyWait = waitForDocVerification docVerifyDelays = []time.Duration{250 * time.Millisecond, 500 * time.Millisecond, time.Second, 2 * time.Second, 4 * time.Second, 8 * time.Second} docMarkdown = goldmark.New( goldmark.WithExtensions(extension.Table), @@ -797,7 +798,9 @@ func readDocVerification(rt *shortcut.RuntimeContext, tool string, rawParams map } } if attempt < len(docVerifyDelays) { - docVerifySleep(docVerifyDelays[attempt]) + if err := docVerifyWait(rt.Command().Context(), docVerifyDelays[attempt]); err != nil { + return nil, err + } } } if lastErr != nil { @@ -806,6 +809,20 @@ func readDocVerification(rt *shortcut.RuntimeContext, tool string, rawParams map return last, nil } +func waitForDocVerification(ctx context.Context, delay time.Duration) error { + if ctx == nil { + ctx = context.Background() + } + timer := time.NewTimer(delay) + defer timer.Stop() + select { + case <-ctx.Done(): + return ctx.Err() + case <-timer.C: + return nil + } +} + func readAllDocumentBlocks(rt *shortcut.RuntimeContext, base map[string]any) (map[string]any, error) { all := make([]any, 0, docBlockReadPageSize) seenPageIdentities := map[string]bool{} diff --git a/internal/shortcut/doc/doc_readback_verification_test.go b/internal/shortcut/doc/doc_readback_verification_test.go index 6f852b4d..f1f83361 100644 --- a/internal/shortcut/doc/doc_readback_verification_test.go +++ b/internal/shortcut/doc/doc_readback_verification_test.go @@ -4,6 +4,7 @@ package doc import ( + "context" "errors" "fmt" "io" @@ -11,11 +12,14 @@ import ( "testing" "time" + "github.com/DingTalk-Real-AI/dingtalk-workspace-cli/internal/helpers" + "github.com/DingTalk-Real-AI/dingtalk-workspace-cli/internal/shortcut" "github.com/DingTalk-Real-AI/dingtalk-workspace-cli/internal/testseam" + "github.com/spf13/cobra" ) func TestCrossPlatformCoverageDocReadbackRetriesStaleContent(t *testing.T) { - testseam.Swap(t, &docVerifySleep, func(time.Duration) {}) + testseam.Swap(t, &docVerifyWait, func(context.Context, time.Duration) error { return nil }) testseam.Swap(t, &docVerifyDelays, []time.Duration{time.Millisecond}) caller := &docCoverageCaller{responses: map[string][]map[string]any{ "get_document_content": {{"markdown": "old"}, {"markdown": "old\nnew"}}, @@ -34,8 +38,34 @@ func TestCrossPlatformCoverageDocReadbackRetriesStaleContent(t *testing.T) { } } +func TestCrossPlatformCoverageDocReadbackStopsOnCancellation(t *testing.T) { + if err := waitForDocVerification(nil, time.Nanosecond); err != nil { + t.Fatalf("completed verification wait = %v", err) + } + cancelled, cancel := context.WithCancel(context.Background()) + cancel() + if err := waitForDocVerification(cancelled, time.Hour); !errors.Is(err, context.Canceled) { + t.Fatalf("cancelled verification wait = %v, want context.Canceled", err) + } + + cmd := &cobra.Command{Use: "verify"} + cmd.SetContext(cancelled) + caller := &docCoverageCaller{responses: map[string][]map[string]any{ + "get_document_content": {{"markdown": "stale"}}, + }} + helpers.InitDeps(caller) + rt := shortcut.RuntimeContextForTest(cmd, Update) + _, err := readDocVerification(rt, "get_document_content", map[string]any{"nodeId": "n"}, func(map[string]any) bool { return false }) + if !errors.Is(err, context.Canceled) { + t.Fatalf("cancelled document verification = %v, want context.Canceled", err) + } + if len(caller.history) != 1 { + t.Fatalf("cancelled document verification calls = %d, want 1", len(caller.history)) + } +} + func TestCrossPlatformCoverageDocDeleteReadbackConsumesEveryPage(t *testing.T) { - testseam.Swap(t, &docVerifySleep, func(time.Duration) {}) + testseam.Swap(t, &docVerifyWait, func(context.Context, time.Duration) error { return nil }) testseam.Swap(t, &docVerifyDelays, []time.Duration{time.Millisecond}) firstPage := make([]any, 50) for index := range firstPage { @@ -190,7 +220,7 @@ func TestCrossPlatformCoverageVersionRevertRequiresTargetEvidence(t *testing.T) } func TestCrossPlatformCoverageDocReadbackDefensiveEdges(t *testing.T) { - testseam.Swap(t, &docVerifySleep, func(time.Duration) {}) + testseam.Swap(t, &docVerifyWait, func(context.Context, time.Duration) error { return nil }) for _, tc := range []struct { name string responses []map[string]any diff --git a/internal/shortcut/doc/doc_shortcuts_test.go b/internal/shortcut/doc/doc_shortcuts_test.go index 81e2e238..7f62ed3a 100644 --- a/internal/shortcut/doc/doc_shortcuts_test.go +++ b/internal/shortcut/doc/doc_shortcuts_test.go @@ -113,7 +113,7 @@ func runDocCoverageInput(t *testing.T, declaration shortcut.Shortcut, caller *do func runDocCoveragePath(t *testing.T, declaration shortcut.Shortcut, caller *docCoverageCaller, input io.Reader, commandPath string, args ...string) error { t.Helper() - testseam.Swap(t, &docVerifySleep, func(time.Duration) {}) + testseam.Swap(t, &docVerifyWait, func(context.Context, time.Duration) error { return nil }) helpers.InitDeps(caller) root := &cobra.Command{Use: "dws", SilenceErrors: true, SilenceUsage: true} root.PersistentFlags().Bool("yes", false, "") From e5bf332b051a36b25de6141bd9958d5d3aa037ce Mon Sep 17 00:00:00 2001 From: Dennis Date: Fri, 14 Aug 2026 15:06:44 +0800 Subject: [PATCH 29/32] fix(doc): address readback review findings --- .changes/960-doc-readback-verification.md | 2 +- internal/helpers/doc.go | 16 +++++++++++---- internal/helpers/doc_command_edges_test.go | 15 ++++++++++++++ internal/shortcut/doc/content_shortcuts.go | 6 ++++-- .../doc/doc_readback_verification_test.go | 20 +++++++++++++++++++ .../references/best_practices/04-document.md | 2 +- 6 files changed, 53 insertions(+), 8 deletions(-) diff --git a/.changes/960-doc-readback-verification.md b/.changes/960-doc-readback-verification.md index 06467959..3dd1f89c 100644 --- a/.changes/960-doc-readback-verification.md +++ b/.changes/960-doc-readback-verification.md @@ -2,4 +2,4 @@ category: Fixed --- -- **Document write verification** (#960) — avoids false partial-success results when normalized Markdown, paginated blocks, inline images, or version reverts are confirmed by server readback. +- **Document write verification** (#960) — avoids false partial-success results when normalized Markdown, paginated blocks, inline images, or version reverts are confirmed by server readback. Document reverts and media inserts now require explicit readback evidence and report partial success when the server cannot prove the requested result. diff --git a/internal/helpers/doc.go b/internal/helpers/doc.go index 4c1380e4..19c0241e 100644 --- a/internal/helpers/doc.go +++ b/internal/helpers/doc.go @@ -9,6 +9,7 @@ import ( "net/url" "os" "path/filepath" + "sort" "strconv" "strings" "time" @@ -802,8 +803,10 @@ func readAllDocBlocksForVerification(ctx context.Context, nodeID string) ([]any, if hasMoreKnown && !hasMore { return all, nil } - if total, ok := docNumberAsInt(payload["totalCount"]); ok && len(all) >= total { - return all, nil + if !hasMoreKnown { + if total, ok := docNumberAsInt(payload["totalCount"]); ok && len(all) >= total { + return all, nil + } } if !hasMoreKnown && len(blocks) < pageSize { return all, nil @@ -872,8 +875,13 @@ func nestedDocString(value any, keys ...string) string { return strings.TrimSpace(text) } } - for _, child := range typed { - if text := nestedDocString(child, keys...); text != "" { + orderedKeys := make([]string, 0, len(typed)) + for key := range typed { + orderedKeys = append(orderedKeys, key) + } + sort.Strings(orderedKeys) + for _, key := range orderedKeys { + if text := nestedDocString(typed[key], keys...); text != "" { return text } } diff --git a/internal/helpers/doc_command_edges_test.go b/internal/helpers/doc_command_edges_test.go index 7f478518..1d2918c2 100644 --- a/internal/helpers/doc_command_edges_test.go +++ b/internal/helpers/doc_command_edges_test.go @@ -342,6 +342,18 @@ func TestCrossPlatformCoverageDocMediaReadbackDefensiveEdges(t *testing.T) { } }) + t.Run("explicit has more overrides inconsistent total count", func(t *testing.T) { + caller := &scriptedToolCaller{steps: []scriptedToolStep{ + {text: `{"blocks":[{"id":"first"}],"hasMore":true,"totalCount":1}`}, + {text: `{"blocks":[{"id":"second"}],"hasMore":false,"totalCount":2}`}, + }} + installScriptedCaller(t, caller) + got, err := readAllDocBlocksForVerification(ctx, "node") + if err != nil || len(got) != 2 || caller.calls != 2 { + t.Fatalf("blocks=%d calls=%d err=%v, want both explicitly advertised pages", len(got), caller.calls, err) + } + }) + t.Run("page identity accepts nested and JSONML block IDs", func(t *testing.T) { if got := docBlockPageIdentity([]any{map[string]any{"element": map[string]any{"blockId": "nested-block"}}}); got != `["nested-block"]` { t.Fatalf("nested block page identity = %q", got) @@ -401,6 +413,9 @@ func TestCrossPlatformCoverageDocMediaReadbackDefensiveEdges(t *testing.T) { if nestedDocString(map[string]any{"x": []any{map[string]any{"id": " nested "}}}, "id") != "nested" || nestedDocString(3, "id") != "" { t.Fatal("nested string traversal failed") } + if got := nestedDocString(map[string]any{"z": map[string]any{"id": "last"}, "a": map[string]any{"id": "first"}}, "id"); got != "first" { + t.Fatalf("nested string traversal = %q, want deterministic key order", got) + } if got := insertedDocBlockID(map[string]any{"id": "document", "result": map[string]any{"data": map[string]any{"blockId": " block "}}}); got != "block" { t.Fatalf("trusted inserted block ID = %q, want block", got) } diff --git a/internal/shortcut/doc/content_shortcuts.go b/internal/shortcut/doc/content_shortcuts.go index 4a054928..8eb9d007 100644 --- a/internal/shortcut/doc/content_shortcuts.go +++ b/internal/shortcut/doc/content_shortcuts.go @@ -850,8 +850,10 @@ func readAllDocumentBlocks(rt *shortcut.RuntimeContext, base map[string]any) (ma if known && !hasMore { return map[string]any{"blocks": all, "hasMore": false, "totalCount": len(all)}, nil } - if total, ok := nestedNonNegativeInt(page, "totalCount", "total_count"); ok && len(all) >= total { - return map[string]any{"blocks": all, "hasMore": false, "totalCount": total}, nil + if !known { + if total, ok := nestedNonNegativeInt(page, "totalCount", "total_count"); ok && len(all) >= total { + return map[string]any{"blocks": all, "hasMore": false, "totalCount": total}, nil + } } if !known && len(blocks) < docBlockReadPageSize { return map[string]any{"blocks": all, "hasMore": false, "totalCount": len(all)}, nil diff --git a/internal/shortcut/doc/doc_readback_verification_test.go b/internal/shortcut/doc/doc_readback_verification_test.go index f1f83361..3d4e6775 100644 --- a/internal/shortcut/doc/doc_readback_verification_test.go +++ b/internal/shortcut/doc/doc_readback_verification_test.go @@ -273,6 +273,26 @@ func TestCrossPlatformCoverageDocReadbackDefensiveEdges(t *testing.T) { } }) + t.Run("explicit has more overrides inconsistent total count", func(t *testing.T) { + testseam.Swap(t, &docVerifyDelays, []time.Duration{}) + caller := &docCoverageCaller{responses: map[string][]map[string]any{"list_document_blocks": { + {"blocks": []any{map[string]any{"id": "first"}}, "hasMore": true, "totalCount": 1}, + {"blocks": []any{map[string]any{"id": "second"}}, "hasMore": false, "totalCount": 2}, + }}} + if err := runDocCoverage(t, Update, caller, "--node", "n", "--command", "block_delete", "--block-id", "target", "--yes"); err != nil { + t.Fatal(err) + } + reads := 0 + for _, call := range caller.history { + if call.tool == "list_document_blocks" { + reads++ + } + } + if reads != 2 { + t.Fatalf("read calls = %d, want both explicitly advertised pages", reads) + } + }) + t.Run("block read safety limit", func(t *testing.T) { testseam.Swap(t, &docVerifyDelays, []time.Duration{}) pages := make([]map[string]any, docBlockReadMaxItems/docBlockReadPageSize) diff --git a/skills/mono/references/best_practices/04-document.md b/skills/mono/references/best_practices/04-document.md index c884341e..9985c60d 100644 --- a/skills/mono/references/best_practices/04-document.md +++ b/skills/mono/references/best_practices/04-document.md @@ -14,7 +14,7 @@ | delete-old-doc | 1. `doc search --query "<关键词>"` 或 `doc list --folder ` 取 `nodeId`
2. **必须先向用户展示要删除的文档标题/路径**，等用户确认
3. `doc delete --node --yes`（注意：是 `doc delete`，不是 `doc block delete`；前者删整篇，后者删块）
4. 验证：`doc info --node ` 应返回 not-found | | export-doc-as-docx | 1. `doc search --query "<关键词>"` → 取 `nodeId`
2. `doc export --node --output /tmp/.docx --timeout-sec 600`（CLI 内置渐进式退避轮询，**不要自己拼 GET downloadUrl**）
3. 落盘失败兜底：`doc export get --job-id --output /tmp/.docx` 续等
4. （可选）`doc-to-message` 把 docx 路径作为附件发给用户 | | grant-doc-access | 1. `doc search --query "<关键词>"` → 取 `nodeId`
2. `contact user search --query "<姓名>"` → 取 `userId`（**注意**: doc permission 用 `userId`，不是 `openDingTalkId`）
3. **节点级**授权：`doc permission add --node --user --role EDITOR`
　　role 取值: MANAGER / EDITOR / DOWNLOADER / READER（**不要传 OWNER**）
　　单次最多 30 个 userId
4. 查权限：`doc permission list --node ` 确认
5. **不要混淆**: 给"知识库"加成员用 `wiki member add --workspace`（容器级）；给"单个文档"加权限用 `doc permission add --node`（节点级）| -| insert-image-to-doc | 1. `doc search --query "<关键词>"` → 取 `nodeId`
2. `doc media insert --node --file ./image.png`（3 步一体化：获取凭证 → PUT OSS → 插入块；图片 ≤20MB 自动作为内联图片，其他作为附件块）
3. （可选）`doc read --node ` 回读确认
**禁止**: 自己拿 uploadUrl 写 PUT —— 90%+ 会因 Content-Type 未清空触发 SignatureDoesNotMatch | +| insert-image-to-doc | 1. `doc search --query "<关键词>"` → 取 `nodeId`
2. `doc media insert --node --file ./image.png`（4 步一体化：获取凭证 → PUT OSS → 插入块 → 自动回读验证；图片 ≤20MB 自动作为内联图片，其他作为附件块）
3. 检查结果中的 `verified=true`；若服务端无法提供明确回读证据，命令会返回 partial success
**禁止**: 自己拿 uploadUrl 写 PUT —— 90%+ 会因 Content-Type 未清空触发 SignatureDoesNotMatch | | download-doc-attachment | 1. `doc block list --node ` → 找到 `blockType=attachment` 的块取 `resourceId`
2. `doc media download --node --resource-id ` → 返回 OSS 临时 URL（**注意 expirationSeconds**）
3. 调用方自行 GET 该 URL 落盘 | --- From 54c2054a5cdb7844bacea530ddad69f9752ec420 Mon Sep 17 00:00:00 2001 From: Dennis Date: Fri, 14 Aug 2026 16:40:15 +0800 Subject: [PATCH 30/32] fix(doc): ignore generated JSONML defaults --- internal/shortcut/doc/content_shortcuts.go | 38 ++++++++++++++++++- .../doc/doc_readback_verification_test.go | 12 +++++- 2 files changed, 47 insertions(+), 3 deletions(-) diff --git a/internal/shortcut/doc/content_shortcuts.go b/internal/shortcut/doc/content_shortcuts.go index 8eb9d007..df6fd28e 100644 --- a/internal/shortcut/doc/content_shortcuts.go +++ b/internal/shortcut/doc/content_shortcuts.go @@ -11,6 +11,7 @@ import ( "io" "os" "path/filepath" + "reflect" "strings" "time" "unicode" @@ -1382,6 +1383,7 @@ func normalizeJSONMLForVerification(raw string) string { if len(typed) > 1 { if declared, ok := typed[1].(map[string]any); ok { attrs, _ = normalize(declared).(map[string]any) + attrs = removeGeneratedJSONMLDefaults(tag, attrs) start = 2 } } @@ -1408,7 +1410,7 @@ func normalizeJSONMLForVerification(raw string) string { if normalizedKey == "uuid" || normalizedKey == "blockid" || normalizedKey == "elementid" || normalizedKey == "index" { continue } - out[key] = normalize(child) + out[normalizedKey] = normalize(child) } return out case string: @@ -1422,6 +1424,38 @@ func normalizeJSONMLForVerification(raw string) string { return string(encoded) } +var generatedJSONMLAttributeDefaults = map[string]map[string]any{ + "hr": { + "sz": float64(1), + }, + "tc": { + "colspan": float64(1), "rowspan": float64(1), "valign": "middle", + }, + "code": { + "code": "", "syntax": "plaintext", "theme": "default", + "wrap": true, "showlinenumber": true, "fold": false, + }, +} + +// removeGeneratedJSONMLDefaults drops only defaults declared by the reviewed +// JSONML schema, plus empty server style objects. Other attributes remain part +// of the semantic fingerprint so links, formatting, and table layout stay +// strict. +func removeGeneratedJSONMLDefaults(tag string, attrs map[string]any) map[string]any { + defaults := generatedJSONMLAttributeDefaults[strings.ToLower(tag)] + out := make(map[string]any, len(attrs)) + for key, value := range attrs { + if object, ok := value.(map[string]any); ok && len(object) == 0 { + continue + } + if defaultValue, ok := defaults[key]; ok && reflect.DeepEqual(value, defaultValue) { + continue + } + out[key] = value + } + return out +} + func isGeneratedTextSpan(attrs map[string]any) bool { if len(attrs) == 0 { return true @@ -1429,7 +1463,7 @@ func isGeneratedTextSpan(attrs map[string]any) bool { if len(attrs) != 1 { return false } - value, ok := attrs["data-type"].(string) + value, ok := attrs["datatype"].(string) return ok && (value == "text" || value == "leaf") } diff --git a/internal/shortcut/doc/doc_readback_verification_test.go b/internal/shortcut/doc/doc_readback_verification_test.go index 3d4e6775..262771bf 100644 --- a/internal/shortcut/doc/doc_readback_verification_test.go +++ b/internal/shortcut/doc/doc_readback_verification_test.go @@ -120,15 +120,25 @@ func TestCrossPlatformCoverageDocReplacePreflightIsGloballyUnique(t *testing.T) func TestCrossPlatformCoverageDocVerificationPreservesMeaning(t *testing.T) { expected := "[\"p\",{},\"text\"]" - serverExpanded := "[\"p\",{\"uuid\":\"generated\"},[\"span\",{\"data-type\":\"text\"},[\"span\",{\"data-type\":\"leaf\"},\"text\"]]]" + serverExpanded := "[\"p\",{\"uuid\":\"generated\",\"style\":{}},[\"span\",{\"data-type\":\"text\"},[\"span\",{\"data-type\":\"leaf\"},\"text\"]]]" if normalizeJSONMLForVerification(expected) != normalizeJSONMLForVerification(serverExpanded) { t.Fatal("generated JSONML text wrappers should not change document meaning") } + defaultFree := `[["hr",{}],["code",{}],["table",{},["tr",{},["tc",{},["p",{},"cell"]]]]]` + serverDefaulted := `[["hr",{"sz":1}],["code",{"code":"","syntax":"plaintext","theme":"default","wrap":true,"showLineNumber":true,"fold":false}],["table",{},["tr",{},["tc",{"colSpan":1,"rowSpan":1,"vAlign":"middle"},["p",{},"cell"]]]]]` + if !verifyUpdatedDocumentContent(map[string]any{"jsonml": serverDefaulted}, defaultFree, "overwrite", "jsonml") { + t.Fatal("server-generated JSONML schema defaults should not fail readback verification") + } linkA := "[\"a\",{\"href\":\"https://example.com/a\"},\"text\"]" linkB := "[\"a\",{\"href\":\"https://example.com/b\"},\"text\"]" if normalizeJSONMLForVerification(linkA) == normalizeJSONMLForVerification(linkB) { t.Fatal("semantic JSONML attributes were ignored") } + tableA := `[["table",{"jc":"center"},["tr",{},["tc",{},["p",{},"cell"]]]]]` + tableB := `[["table",{"jc":"right"},["tr",{},["tc",{},["p",{},"cell"]]]]]` + if normalizeJSONMLForVerification(tableA) == normalizeJSONMLForVerification(tableB) { + t.Fatal("semantic JSONML table alignment was ignored") + } codeA := "~~~go\n return nil\n~~~" codeB := "~~~go\nreturn nil\n~~~" if normalizeMarkdownForVerification(codeA) == normalizeMarkdownForVerification(codeB) { From 68463264455ac3da33a3904c8c08d3834d168500 Mon Sep 17 00:00:00 2001 From: Dennis Date: Sat, 15 Aug 2026 23:28:00 +0800 Subject: [PATCH 31/32] fix(doc): reject revert request echo evidence --- internal/app/param_alias_e2e_test.go | 2 +- .../doc/doc_readback_verification_test.go | 21 ++++++ internal/shortcut/doc/doc_shortcuts_test.go | 41 ++++++++++- .../doc/history_template_shortcuts.go | 68 ++++++++++++++++++- 4 files changed, 128 insertions(+), 4 deletions(-) diff --git a/internal/app/param_alias_e2e_test.go b/internal/app/param_alias_e2e_test.go index 71298e27..d1f2df97 100644 --- a/internal/app/param_alias_e2e_test.go +++ b/internal/app/param_alias_e2e_test.go @@ -65,7 +65,7 @@ func (c *paramAliasCaptureCaller) paramAliasResponseForTool(tool string) string case "list_doc_versions": return `{"result":{"items":[{"version":3}]}}` case "revert_doc_version": - return `{"version":3}` + return `{"revertedToVersion":3}` case "search_doc_templates": return `{"result":[{"templateId":"fixture-template-id"}]}` case "create_document": diff --git a/internal/shortcut/doc/doc_readback_verification_test.go b/internal/shortcut/doc/doc_readback_verification_test.go index 262771bf..9252727a 100644 --- a/internal/shortcut/doc/doc_readback_verification_test.go +++ b/internal/shortcut/doc/doc_readback_verification_test.go @@ -224,9 +224,30 @@ func TestCrossPlatformCoverageVersionRevertRequiresTargetEvidence(t *testing.T) if revertResultMatchesVersion(map[string]any{"ok": true}, 3) || currentDocumentMatchesRestoredVersion(map[string]any{"version": 99}, 3) { t.Fatal("readability or an unrelated current version must not prove a revert") } + if revertResultMatchesVersion(map[string]any{"version": 3}, 3) { + t.Fatal("the request version parameter must not prove its own revert") + } + if revertResultMatchesVersion(map[string]any{"data": map[string]any{"request": map[string]any{"targetVersion": 3}}}, 3) { + t.Fatal("request echo containers must not provide version evidence") + } + for _, failed := range []map[string]any{ + {"data": map[string]any{"success": "false", "revertedToVersion": 3}}, + {"data": []any{map[string]any{"error_code": "REVERT_FAILED", "revertedToVersion": 3}}}, + {"data": map[string]any{"errorCode": 500.0, "revertedToVersion": 3}}, + } { + if revertResultMatchesVersion(failed, 3) { + t.Fatalf("explicit failure %#v must override target-version evidence", failed) + } + } if !revertResultMatchesVersion(map[string]any{"revertedToVersion": 3}, 3) { t.Fatal("explicit target-version acknowledgement was not accepted") } + if !revertResultMatchesVersion(map[string]any{"errorCode": "0", "revertedToVersion": 3}, 3) { + t.Fatal("a success error-code sentinel must not suppress target-version evidence") + } + if revertErrorCodeIsFailure(nil) { + t.Fatal("an absent error code must not be treated as an explicit failure") + } } func TestCrossPlatformCoverageDocReadbackDefensiveEdges(t *testing.T) { diff --git a/internal/shortcut/doc/doc_shortcuts_test.go b/internal/shortcut/doc/doc_shortcuts_test.go index 7f62ed3a..aae46f3f 100644 --- a/internal/shortcut/doc/doc_shortcuts_test.go +++ b/internal/shortcut/doc/doc_shortcuts_test.go @@ -58,7 +58,7 @@ func (f *docCoverageCaller) CallTool(_ context.Context, _, tool string, params m } value := docCoveragePayload(tool) if tool == "revert_doc_version" { - value = map[string]any{"version": params["version"]} + value = map[string]any{"revertedToVersion": params["version"]} } if queue := f.responses[tool]; len(queue) > 0 { value = queue[0] @@ -593,6 +593,45 @@ func TestCrossPlatformCoverageDocVersionRevertPaginationAndVerification(t *testi } }) + for _, test := range []struct { + name string + response map[string]any + current map[string]any + wantOK bool + }{ + {name: "explicit server evidence", response: map[string]any{"data": map[string]any{"revertResult": map[string]any{"revertedToVersion": 3}}}, wantOK: true}, + {name: "bare request parameter echo", response: map[string]any{"version": 3}}, + {name: "nested request parameter echo", response: map[string]any{"data": map[string]any{"request": map[string]any{"nodeId": "n", "version": 3}}}}, + {name: "accepted field inside request echo", response: map[string]any{"data": map[string]any{"request": map[string]any{"targetVersion": 3}}}}, + {name: "nested business failure with request echo", response: map[string]any{"data": map[string]any{"success": false, "errorCode": "REVERT_FAILED", "request": map[string]any{"version": 3}}}}, + { + name: "nested business failure overrides all evidence", + response: map[string]any{"data": map[string]any{"success": false, "revertResult": map[string]any{"revertedToVersion": 3}}}, + current: map[string]any{"targetVersion": 3}, + }, + } { + t.Run(test.name, func(t *testing.T) { + responses := map[string][]map[string]any{ + "revert_doc_version": {test.response}, + } + if test.current != nil { + responses["get_document_info"] = []map[string]any{test.current} + } + caller := &docCoverageCaller{responses: responses} + err := runDocCoverage(t, VersionRevert, caller, "--node", "n", "--version", "3", "--yes") + if test.wantOK { + if err != nil { + t.Fatal(err) + } + return + } + var typed *apperrors.Error + if !errors.As(err, &typed) || typed.Reason != "doc_history_revert_target_unproven" || typed.Details["status"] != "partial_success" { + t.Fatalf("request echo response %#v produced %#v", test.response, err) + } + }) + } + for _, test := range []struct { name string responses []map[string]any diff --git a/internal/shortcut/doc/history_template_shortcuts.go b/internal/shortcut/doc/history_template_shortcuts.go index 048ae9bd..e5086dc2 100644 --- a/internal/shortcut/doc/history_template_shortcuts.go +++ b/internal/shortcut/doc/history_template_shortcuts.go @@ -178,7 +178,8 @@ func executeHistoryRevert(rt *shortcut.RuntimeContext) error { map[string]any{"available": false, "reason": "the requested revert completed; verify the current document before any further write"}, ) } - verified := revertResultMatchesVersion(reverted, target) || currentDocumentMatchesRestoredVersion(current, target) + verified := !revertResponseHasExplicitFailure(reverted) && + (revertResultMatchesVersion(reverted, target) || currentDocumentMatchesRestoredVersion(current, target)) if !verified { return docPartialWriteError( "doc.history_revert", "doc_history_revert_target_unproven", "verify", @@ -206,8 +207,11 @@ func executeHistoryRevert(rt *shortcut.RuntimeContext) error { } func revertResultMatchesVersion(value map[string]any, target int) bool { + if revertResponseHasExplicitFailure(value) { + return false + } return versionEvidenceMatches(value, target, map[string]bool{ - "version": true, "targetversion": true, "appliedversion": true, + "targetversion": true, "appliedversion": true, "restoredversion": true, "revertedversion": true, "revertedtoversion": true, "sourceversion": true, }) } @@ -272,6 +276,9 @@ func versionEvidenceMatches(value any, target int, acceptedKeys map[string]bool) case map[string]any: for key, child := range typed { normalized := strings.ToLower(strings.NewReplacer("_", "", "-", "").Replace(key)) + if versionEvidenceRequestEchoKeys[normalized] { + continue + } if acceptedKeys[normalized] { if versionNumberMatches(child, target) { return true @@ -291,6 +298,63 @@ func versionEvidenceMatches(value any, target int, acceptedKeys map[string]bool) return false } +var versionEvidenceRequestEchoKeys = map[string]bool{ + "args": true, "arguments": true, "input": true, "inputs": true, + "params": true, "parameters": true, "request": true, "requestbody": true, + "requestparams": true, "toolargs": true, "toolarguments": true, +} + +func revertResponseHasExplicitFailure(value any) bool { + switch typed := value.(type) { + case map[string]any: + for key, child := range typed { + normalized := strings.ToLower(strings.NewReplacer("_", "", "-", "").Replace(key)) + if versionEvidenceRequestEchoKeys[normalized] { + continue + } + if normalized == "success" { + if success, ok := child.(bool); ok && !success { + return true + } + if success, ok := child.(string); ok && strings.EqualFold(strings.TrimSpace(success), "false") { + return true + } + } + if normalized == "errorcode" { + if revertErrorCodeIsFailure(child) { + return true + } + } + if revertResponseHasExplicitFailure(child) { + return true + } + } + case []any: + for _, child := range typed { + if revertResponseHasExplicitFailure(child) { + return true + } + } + } + return false +} + +func revertErrorCodeIsFailure(value any) bool { + switch typed := value.(type) { + case string: + switch strings.ToLower(strings.TrimSpace(typed)) { + case "", "0", "ok", "success", "succeed": + return false + default: + return true + } + case float64: + return typed != 0 + default: + return false + } +} + func versionNumberMatches(value any, target int) bool { switch number := value.(type) { case float64: From a82d945f54abdd64bdb5a1611cf57b8889ffaef5 Mon Sep 17 00:00:00 2001 From: Dennis Date: Mon, 17 Aug 2026 14:14:44 +0800 Subject: [PATCH 32/32] fix(doc): reject explicit revert failure states --- .../doc/doc_readback_verification_test.go | 33 +++++++++++++--- internal/shortcut/doc/doc_shortcuts_test.go | 4 +- .../doc/history_template_shortcuts.go | 38 +++++++++++++++---- 3 files changed, 61 insertions(+), 14 deletions(-) diff --git a/internal/shortcut/doc/doc_readback_verification_test.go b/internal/shortcut/doc/doc_readback_verification_test.go index 9252727a..63be4455 100644 --- a/internal/shortcut/doc/doc_readback_verification_test.go +++ b/internal/shortcut/doc/doc_readback_verification_test.go @@ -5,6 +5,7 @@ package doc import ( "context" + "encoding/json" "errors" "fmt" "io" @@ -232,21 +233,41 @@ func TestCrossPlatformCoverageVersionRevertRequiresTargetEvidence(t *testing.T) } for _, failed := range []map[string]any{ {"data": map[string]any{"success": "false", "revertedToVersion": 3}}, + {"data": map[string]any{"ok": false, "revertedToVersion": 3}}, + {"data": map[string]any{"status": "FAILED", "revertedToVersion": 3}}, + {"data": map[string]any{"state": "failure", "revertedToVersion": 3}}, {"data": []any{map[string]any{"error_code": "REVERT_FAILED", "revertedToVersion": 3}}}, {"data": map[string]any{"errorCode": 500.0, "revertedToVersion": 3}}, + {"data": map[string]any{"code": json.Number("500"), "revertedToVersion": 3}}, } { if revertResultMatchesVersion(failed, 3) { t.Fatalf("explicit failure %#v must override target-version evidence", failed) } } - if !revertResultMatchesVersion(map[string]any{"revertedToVersion": 3}, 3) { - t.Fatal("explicit target-version acknowledgement was not accepted") + for _, succeeded := range []map[string]any{ + {"revertedToVersion": 3}, + {"status": "SUCCESS", "revertedToVersion": 3}, + {"state": "succeeded", "revertedToVersion": 3}, + {"errorCode": "0", "revertedToVersion": 3}, + {"code": 200, "revertedToVersion": 3}, + {"code": "204", "revertedToVersion": 3}, + } { + if !revertResultMatchesVersion(succeeded, 3) { + t.Fatalf("explicit success %#v suppressed target-version evidence", succeeded) + } } - if !revertResultMatchesVersion(map[string]any{"errorCode": "0", "revertedToVersion": 3}, 3) { - t.Fatal("a success error-code sentinel must not suppress target-version evidence") + for _, absentOrSuccess := range []any{nil, "", "OK", "SUCCESS", json.Number("0"), json.Number("200"), 0.0, 201.0, 0, 202} { + if revertErrorCodeIsFailure(absentOrSuccess) { + t.Fatalf("success code %#v was treated as an explicit failure", absentOrSuccess) + } } - if revertErrorCodeIsFailure(nil) { - t.Fatal("an absent error code must not be treated as an explicit failure") + for _, failedCode := range []any{json.Number("bad"), json.Number("500"), 1.0, 500.0, 1, 500} { + if !revertErrorCodeIsFailure(failedCode) { + t.Fatalf("failure code %#v was not treated as an explicit failure", failedCode) + } + } + if revertStatusIsFailure(500) || revertStatusIsFailure("PROCESSING") { + t.Fatal("non-failure status was treated as an explicit failure") } } diff --git a/internal/shortcut/doc/doc_shortcuts_test.go b/internal/shortcut/doc/doc_shortcuts_test.go index aae46f3f..8b3da279 100644 --- a/internal/shortcut/doc/doc_shortcuts_test.go +++ b/internal/shortcut/doc/doc_shortcuts_test.go @@ -604,9 +604,11 @@ func TestCrossPlatformCoverageDocVersionRevertPaginationAndVerification(t *testi {name: "nested request parameter echo", response: map[string]any{"data": map[string]any{"request": map[string]any{"nodeId": "n", "version": 3}}}}, {name: "accepted field inside request echo", response: map[string]any{"data": map[string]any{"request": map[string]any{"targetVersion": 3}}}}, {name: "nested business failure with request echo", response: map[string]any{"data": map[string]any{"success": false, "errorCode": "REVERT_FAILED", "request": map[string]any{"version": 3}}}}, + {name: "failed status overrides target evidence", response: map[string]any{"data": map[string]any{"status": "FAILED", "revertResult": map[string]any{"revertedToVersion": 3}}}}, + {name: "failed code overrides target evidence", response: map[string]any{"data": map[string]any{"code": "REVERT_FAILED", "revertResult": map[string]any{"revertedToVersion": 3}}}}, { name: "nested business failure overrides all evidence", - response: map[string]any{"data": map[string]any{"success": false, "revertResult": map[string]any{"revertedToVersion": 3}}}, + response: map[string]any{"data": map[string]any{"state": "FAILURE", "revertResult": map[string]any{"revertedToVersion": 3}}}, current: map[string]any{"targetVersion": 3}, }, } { diff --git a/internal/shortcut/doc/history_template_shortcuts.go b/internal/shortcut/doc/history_template_shortcuts.go index e5086dc2..10a88eb7 100644 --- a/internal/shortcut/doc/history_template_shortcuts.go +++ b/internal/shortcut/doc/history_template_shortcuts.go @@ -312,7 +312,7 @@ func revertResponseHasExplicitFailure(value any) bool { if versionEvidenceRequestEchoKeys[normalized] { continue } - if normalized == "success" { + if normalized == "success" || normalized == "ok" { if success, ok := child.(bool); ok && !success { return true } @@ -320,7 +320,10 @@ func revertResponseHasExplicitFailure(value any) bool { return true } } - if normalized == "errorcode" { + if (normalized == "status" || normalized == "state") && revertStatusIsFailure(child) { + return true + } + if normalized == "errorcode" || normalized == "code" { if revertErrorCodeIsFailure(child) { return true } @@ -339,17 +342,38 @@ func revertResponseHasExplicitFailure(value any) bool { return false } +func revertStatusIsFailure(value any) bool { + status, ok := value.(string) + if !ok { + return false + } + switch strings.ToLower(strings.TrimSpace(status)) { + case "fail", "failed", "failure", "error", "errored", "reject", "rejected", "deny", "denied", "cancel", "cancelled", "canceled", "abort", "aborted": + return true + default: + return false + } +} + func revertErrorCodeIsFailure(value any) bool { switch typed := value.(type) { case string: - switch strings.ToLower(strings.TrimSpace(typed)) { - case "", "0", "ok", "success", "succeed": + normalized := strings.ToLower(strings.TrimSpace(typed)) + switch normalized { + case "", "ok", "success", "succeed", "succeeded": return false - default: - return true } + if parsed, err := strconv.Atoi(normalized); err == nil { + return parsed != 0 && (parsed < 200 || parsed >= 300) + } + return true case float64: - return typed != 0 + return typed != 0 && (typed < 200 || typed >= 300) + case json.Number: + parsed, err := typed.Int64() + return err != nil || (parsed != 0 && (parsed < 200 || parsed >= 300)) + case int: + return typed != 0 && (typed < 200 || typed >= 300) default: return false }