From 770f2bb0b307040c609019ead41ed6f53d15fcf9 Mon Sep 17 00:00:00 2001 From: "MaineCoon-GPT-5.5" Date: Thu, 23 Jul 2026 00:12:01 +0800 Subject: [PATCH 1/6] fix(skills): clarify workflow-mandated schedule registration MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Why: hotfix merge-gate reminders were being left as unregistered previews because schedule registration guidance required user confirmation for every task, including explicit SOP-mandated follow-up reminders. Validation:\n- PATH="/opt/homebrew/opt/node@24/bin:/Users/xxx/.local/bin:/opt/homebrew/bin:/opt/homebrew/sbin:/usr/local/bin:/System/Cryptexes/App/usr/bin:/usr/bin:/bin:/usr/sbin:/sbin:/var/run/com.apple.security.cryptexd/codex.system/bootstrap/usr/local/bin:/var/run/com.apple.security.cryptexd/codex.system/bootstrap/usr/bin:/var/run/com.apple.security.cryptexd/codex.system/bootstrap/usr/appleinternal/bin:/opt/pmk/env/global/bin:/Users/xxx/.local/bin:/opt/homebrew/Caskroom/codex/0.144.1/codex-path:/Users/xxx/.codex/tmp/arg0/codex-arg0JpRcS7:/Users/xxx/workspace/AI/cat-cafe-develop/packages/api/node_modules/.bin:/Users/xxx/Library/pnpm/.tools/pnpm/9.15.4_tmp_20256/node_modules/pnpm/dist/node-gyp-bin:/Users/xxx/workspace/AI/cat-cafe-develop/node_modules/.bin:/opt/homebrew/Cellar/node@24/24.18.0/bin:/Users/xxx/workspace/AI/clowder-ai/node_modules/.bin:/Users/xxx/Library/pnpm/.tools/pnpm/9.15.4/bin:/Users/xxx/Library/pnpm:/Users/xxx/.cargo/bin" pnpm --filter @cat-cafe/mcp-server test -- --test-name-pattern "workflow-mandated|cat_cafe_register_scheduled_task"\n- PATH="/opt/homebrew/opt/node@24/bin:/Users/xxx/.local/bin:/opt/homebrew/bin:/opt/homebrew/sbin:/usr/local/bin:/System/Cryptexes/App/usr/bin:/usr/bin:/bin:/usr/sbin:/sbin:/var/run/com.apple.security.cryptexd/codex.system/bootstrap/usr/local/bin:/var/run/com.apple.security.cryptexd/codex.system/bootstrap/usr/bin:/var/run/com.apple.security.cryptexd/codex.system/bootstrap/usr/appleinternal/bin:/opt/pmk/env/global/bin:/Users/xxx/.local/bin:/opt/homebrew/Caskroom/codex/0.144.1/codex-path:/Users/xxx/.codex/tmp/arg0/codex-arg0JpRcS7:/Users/xxx/workspace/AI/cat-cafe-develop/packages/api/node_modules/.bin:/Users/xxx/Library/pnpm/.tools/pnpm/9.15.4_tmp_20256/node_modules/pnpm/dist/node-gyp-bin:/Users/xxx/workspace/AI/cat-cafe-develop/node_modules/.bin:/opt/homebrew/Cellar/node@24/24.18.0/bin:/Users/xxx/workspace/AI/clowder-ai/node_modules/.bin:/Users/xxx/Library/pnpm/.tools/pnpm/9.15.4/bin:/Users/xxx/Library/pnpm:/Users/xxx/.cargo/bin" pnpm check:skills:manifest\n- PATH="/opt/homebrew/opt/node@24/bin:/Users/xxx/.local/bin:/opt/homebrew/bin:/opt/homebrew/sbin:/usr/local/bin:/System/Cryptexes/App/usr/bin:/usr/bin:/bin:/usr/sbin:/sbin:/var/run/com.apple.security.cryptexd/codex.system/bootstrap/usr/local/bin:/var/run/com.apple.security.cryptexd/codex.system/bootstrap/usr/bin:/var/run/com.apple.security.cryptexd/codex.system/bootstrap/usr/appleinternal/bin:/opt/pmk/env/global/bin:/Users/xxx/.local/bin:/opt/homebrew/Caskroom/codex/0.144.1/codex-path:/Users/xxx/.codex/tmp/arg0/codex-arg0JpRcS7:/Users/xxx/workspace/AI/cat-cafe-develop/packages/api/node_modules/.bin:/Users/xxx/Library/pnpm/.tools/pnpm/9.15.4_tmp_20256/node_modules/pnpm/dist/node-gyp-bin:/Users/xxx/workspace/AI/cat-cafe-develop/node_modules/.bin:/opt/homebrew/Cellar/node@24/24.18.0/bin:/Users/xxx/workspace/AI/clowder-ai/node_modules/.bin:/Users/xxx/Library/pnpm/.tools/pnpm/9.15.4/bin:/Users/xxx/Library/pnpm:/Users/xxx/.cargo/bin" pnpm check:skills:surfaces\n- PATH="/opt/homebrew/opt/node@24/bin:/Users/xxx/.local/bin:/opt/homebrew/bin:/opt/homebrew/sbin:/usr/local/bin:/System/Cryptexes/App/usr/bin:/usr/bin:/bin:/usr/sbin:/sbin:/var/run/com.apple.security.cryptexd/codex.system/bootstrap/usr/local/bin:/var/run/com.apple.security.cryptexd/codex.system/bootstrap/usr/bin:/var/run/com.apple.security.cryptexd/codex.system/bootstrap/usr/appleinternal/bin:/opt/pmk/env/global/bin:/Users/xxx/.local/bin:/opt/homebrew/Caskroom/codex/0.144.1/codex-path:/Users/xxx/.codex/tmp/arg0/codex-arg0JpRcS7:/Users/xxx/workspace/AI/cat-cafe-develop/packages/api/node_modules/.bin:/Users/xxx/Library/pnpm/.tools/pnpm/9.15.4_tmp_20256/node_modules/pnpm/dist/node-gyp-bin:/Users/xxx/workspace/AI/cat-cafe-develop/node_modules/.bin:/opt/homebrew/Cellar/node@24/24.18.0/bin:/Users/xxx/workspace/AI/clowder-ai/node_modules/.bin:/Users/xxx/Library/pnpm/.tools/pnpm/9.15.4/bin:/Users/xxx/Library/pnpm:/Users/xxx/.cargo/bin" pnpm check\n- git diff --check Note: pnpm check:skills is blocked in this local worktree by missing provider skill mounts; pnpm sync:skills --dry-run would touch 34 worktrees, so it was not applied in this PR. [砚砚/gpt-5.5🐾] --- cat-cafe-skills/merge-gate/SKILL.md | 11 +++++-- cat-cafe-skills/schedule-tasks/SKILL.md | 12 +++++++- .../mcp-server/src/tools/schedule-tools.ts | 6 ++-- .../mcp-server/test/schedule-tools.test.js | 30 +++++++++++++++++++ 4 files changed, 53 insertions(+), 6 deletions(-) diff --git a/cat-cafe-skills/merge-gate/SKILL.md b/cat-cafe-skills/merge-gate/SKILL.md index aa1cdc5d68..1d238403df 100644 --- a/cat-cafe-skills/merge-gate/SKILL.md +++ b/cat-cafe-skills/merge-gate/SKILL.md @@ -363,7 +363,7 @@ gh pr merge {PR_NUMBER} --squash --delete-branch **时机**：merge 完成后、清理前。`delayMs: 1209600000`（14 天）从注册时刻起算 ≈ 合入后 14 天。 -**操作**：调用 MCP 工具 `cat_cafe_register_scheduled_task`： +**操作**：这是 workflow-mandated schedule。先调用 `cat_cafe_preview_scheduled_task` 生成 draft，核对下面参数；核对无误后调用 `cat_cafe_register_scheduled_task` 注册，with no extra user confirmation： | 参数 | 值 | |------|------| @@ -374,7 +374,14 @@ gh pr merge {PR_NUMBER} --squash --delete-branch | `category` | `"pr"` | | `params` | `{"message":"Hotfix PR #{PR_NUMBER} 合入已满 2 周。请三选一处置：1. 升级正式修复（开 feat）2. 接受永久方案（标记 permanent）3. 已不再相关（代码已重写/删除，标记 obsolete）"}` | -**Fail-closed**：MCP 调用失败 → **停止 merge-gate，不执行 Step 8（清理）**。排查 MCP 连接后重试；连续失败 → 通知operator手动注册 reminder 后继续。 +**Preview 核对项**： + +- `templateId` 是 `reminder` +- trigger 是 once，`delayMs` 是 `1209600000` 或等价 fireAt（合入后 14 天） +- `params.message` 含 PR 号和三选一处置要求 +- delivery thread 是当前 merge-gate thread（或 SOP 明确指定的 thread） + +**Fail-closed**：preview 或 register 任一 MCP 调用失败 → **停止 merge-gate，不执行 Step 8（清理）**。排查 MCP 连接后重试；连续失败 → 通知operator手动注册 reminder 后继续。 ```bash # 8. 更新本地 + 清理（fail-closed） diff --git a/cat-cafe-skills/schedule-tasks/SKILL.md b/cat-cafe-skills/schedule-tasks/SKILL.md index 88c7489ce8..4f336eef8c 100644 --- a/cat-cafe-skills/schedule-tasks/SKILL.md +++ b/cat-cafe-skills/schedule-tasks/SKILL.md @@ -87,6 +87,16 @@ triggers: 用户确认后调用 `cat_cafe_register_scheduled_task` 持久化任务。 +### Workflow-Mandated 例外 + +如果某个明确 SOP/skill 步骤要求注册定时任务（例如 merge-gate 的 hotfix 2 周升级 review reminder），这属于 workflow-mandated schedule： + +1. 仍然必须先调用 `cat_cafe_preview_scheduled_task`。 +2. 核对 draft 的模板、触发时间、参数、投递 thread 与该 SOP/skill 步骤一致。 +3. 核对通过后可以调用 `cat_cafe_register_scheduled_task`，with no extra user confirmation。 + +这条例外只适用于流程本身已经给出明确注册义务的任务；用户临时提出的提醒、周期巡检、摘要推送仍然走 preview → 用户确认 → register。 + ## Trigger 语法速查 ### 周期触发（recurring） @@ -133,7 +143,7 @@ triggers: |------|----------| | 不知道能注册定时任务 | 用户说"每天/定期/提醒"→ 匹配本 skill | | 被唤醒后只发纯文本 | 主动用 rich block（图片、语音、卡片、HTML） | -| 跳过 preview 直接注册 | **必须** preview → 用户确认 → 注册 | +| 跳过 preview 直接注册 | **必须** preview；用户请求任务 preview → 用户确认 → 注册；workflow-mandated SOP/skill 任务 preview → 核对 draft → 注册，with no extra user confirmation | | 发图只想到 image-generation | 先看 `/avatars/`、`/uploads/` 有没有现成图 | ## 和其他 skill 的区别 diff --git a/packages/mcp-server/src/tools/schedule-tools.ts b/packages/mcp-server/src/tools/schedule-tools.ts index 663e7816e5..ca7c887755 100644 --- a/packages/mcp-server/src/tools/schedule-tools.ts +++ b/packages/mcp-server/src/tools/schedule-tools.ts @@ -197,8 +197,8 @@ export const scheduleTools = [ name: 'cat_cafe_preview_scheduled_task', description: 'Preview a scheduled task BEFORE creating it (draft step). Returns a draft with resolved template info, trigger, and params ' + - 'WITHOUT persisting anything. Show this draft to the user for confirmation before calling register_scheduled_task. ' + - 'REQUIRED: Always preview first, then register only after user confirms.', + 'WITHOUT persisting anything. REQUIRED: Always preview first. User-requested schedules need user confirmation before register_scheduled_task; ' + + 'workflow-mandated schedules from an explicit SOP/skill step use the preview as audit evidence before registration.', inputSchema: previewScheduledTaskInputSchema, handler: handlePreviewScheduledTask, }, @@ -208,7 +208,7 @@ export const scheduleTools = [ 'Create a new scheduled task from a template (confirm step). The task will be persisted and run automatically on schedule. ' + 'Supports recurring (cron/interval) and one-shot (once) triggers. Once tasks auto-retire after execution. ' + 'When the task fires, a cat is woken with full capabilities — it can send rich blocks (images, audio, cards), search the web, generate content, etc. ' + - 'IMPORTANT: You MUST call preview_scheduled_task first and get user confirmation before calling this. ' + + 'IMPORTANT: You MUST call preview_scheduled_task first. User-requested schedules require user confirmation; workflow-mandated schedules from an explicit SOP/skill step may register after verifying the preview, with no extra user confirmation. ' + 'trigger and params must be JSON strings, not objects.', inputSchema: registerScheduledTaskInputSchema, handler: handleRegisterScheduledTask, diff --git a/packages/mcp-server/test/schedule-tools.test.js b/packages/mcp-server/test/schedule-tools.test.js index 60ec00ba16..944fc92943 100644 --- a/packages/mcp-server/test/schedule-tools.test.js +++ b/packages/mcp-server/test/schedule-tools.test.js @@ -3,6 +3,7 @@ * Tests for cat_cafe_list_schedule_templates, cat_cafe_register_scheduled_task, cat_cafe_remove_scheduled_task */ import assert from 'node:assert/strict'; +import { readFileSync } from 'node:fs'; import { describe, test } from 'node:test'; describe('Schedule MCP Tools — module exports', () => { @@ -38,6 +39,35 @@ describe('Schedule MCP Tools — module exports', () => { assert.ok(registerScheduledTaskInputSchema.trigger, 'trigger schema required'); }); + test('register description distinguishes user confirmation from workflow-mandated registrations', async () => { + const { scheduleTools } = await import('../dist/tools/schedule-tools.js'); + const previewTool = scheduleTools.find((t) => t.name === 'cat_cafe_preview_scheduled_task'); + const registerTool = scheduleTools.find((t) => t.name === 'cat_cafe_register_scheduled_task'); + assert.ok(previewTool, 'preview tool should exist'); + assert.ok(registerTool, 'register tool should exist'); + + assert.match(previewTool.description, /workflow-mandated/i); + assert.match(registerTool.description, /workflow-mandated/i); + assert.match(registerTool.description, /no extra user confirmation/i); + }); + + test('schedule skills document the workflow-mandated registration exception', () => { + const scheduleSkill = readFileSync( + new URL('../../../cat-cafe-skills/schedule-tasks/SKILL.md', import.meta.url), + 'utf8', + ); + const mergeGateSkill = readFileSync( + new URL('../../../cat-cafe-skills/merge-gate/SKILL.md', import.meta.url), + 'utf8', + ); + + assert.match(scheduleSkill, /workflow-mandated/i); + assert.match(scheduleSkill, /no extra user confirmation/i); + assert.match(mergeGateSkill, /workflow-mandated/i); + assert.match(mergeGateSkill, /cat_cafe_preview_scheduled_task/); + assert.match(mergeGateSkill, /no extra user confirmation/i); + }); + test('cat_cafe_remove_scheduled_task has taskId required', async () => { const { scheduleTools, removeScheduledTaskInputSchema } = await import('../dist/tools/schedule-tools.js'); const tool = scheduleTools.find((t) => t.name === 'cat_cafe_remove_scheduled_task'); From 03504af27edca4973b15ff30646ad73f0a1cd0a6 Mon Sep 17 00:00:00 2001 From: "MaineCoon-GPT-5.5" Date: Thu, 23 Jul 2026 00:17:12 +0800 Subject: [PATCH 2/6] docs(review): add hotfix reminder SOP review packet MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Why: request-review requires a durable review packet, and committed skill changes must declare capability-tip intent so check:capability-tips can classify this SOP/MCP clarification correctly. Validation:\n- PATH="/opt/homebrew/opt/node@24/bin:/Users/xxx/.local/bin:/opt/homebrew/bin:/opt/homebrew/sbin:/usr/local/bin:/System/Cryptexes/App/usr/bin:/usr/bin:/bin:/usr/sbin:/sbin:/var/run/com.apple.security.cryptexd/codex.system/bootstrap/usr/local/bin:/var/run/com.apple.security.cryptexd/codex.system/bootstrap/usr/bin:/var/run/com.apple.security.cryptexd/codex.system/bootstrap/usr/appleinternal/bin:/opt/pmk/env/global/bin:/Users/xxx/.local/bin:/opt/homebrew/Caskroom/codex/0.144.1/codex-path:/Users/xxx/.codex/tmp/arg0/codex-arg0JpRcS7:/Users/xxx/workspace/AI/cat-cafe-develop/packages/api/node_modules/.bin:/Users/xxx/Library/pnpm/.tools/pnpm/9.15.4_tmp_20256/node_modules/pnpm/dist/node-gyp-bin:/Users/xxx/workspace/AI/cat-cafe-develop/node_modules/.bin:/opt/homebrew/Cellar/node@24/24.18.0/bin:/Users/xxx/workspace/AI/clowder-ai/node_modules/.bin:/Users/xxx/Library/pnpm/.tools/pnpm/9.15.4/bin:/Users/xxx/Library/pnpm:/Users/xxx/.cargo/bin" pnpm check\n- PATH="/opt/homebrew/opt/node@24/bin:/Users/xxx/.local/bin:/opt/homebrew/bin:/opt/homebrew/sbin:/usr/local/bin:/System/Cryptexes/App/usr/bin:/usr/bin:/bin:/usr/sbin:/sbin:/var/run/com.apple.security.cryptexd/codex.system/bootstrap/usr/local/bin:/var/run/com.apple.security.cryptexd/codex.system/bootstrap/usr/bin:/var/run/com.apple.security.cryptexd/codex.system/bootstrap/usr/appleinternal/bin:/opt/pmk/env/global/bin:/Users/xxx/.local/bin:/opt/homebrew/Caskroom/codex/0.144.1/codex-path:/Users/xxx/.codex/tmp/arg0/codex-arg0JpRcS7:/Users/xxx/workspace/AI/cat-cafe-develop/packages/api/node_modules/.bin:/Users/xxx/Library/pnpm/.tools/pnpm/9.15.4_tmp_20256/node_modules/pnpm/dist/node-gyp-bin:/Users/xxx/workspace/AI/cat-cafe-develop/node_modules/.bin:/opt/homebrew/Cellar/node@24/24.18.0/bin:/Users/xxx/workspace/AI/clowder-ai/node_modules/.bin:/Users/xxx/Library/pnpm/.tools/pnpm/9.15.4/bin:/Users/xxx/Library/pnpm:/Users/xxx/.cargo/bin" pnpm --filter @cat-cafe/mcp-server test -- --test-name-pattern "workflow-mandated|cat_cafe_register_scheduled_task"\n- git diff --check [砚砚/gpt-5.5🐾] --- cat-cafe-skills/schedule-tasks/SKILL.md | 1 + ...7-23-hotfix-reminder-sop-review-request.md | 105 ++++++++++++++++++ 2 files changed, 106 insertions(+) create mode 100644 review-notes/2026-07-23-hotfix-reminder-sop-review-request.md diff --git a/cat-cafe-skills/schedule-tasks/SKILL.md b/cat-cafe-skills/schedule-tasks/SKILL.md index 4f336eef8c..c96e13e912 100644 --- a/cat-cafe-skills/schedule-tasks/SKILL.md +++ b/cat-cafe-skills/schedule-tasks/SKILL.md @@ -1,5 +1,6 @@ --- name: schedule-tasks +tips_exempt: internal SOP/MCP workflow clarification; no new end-user capability description: > 定时任务注册、管理、能力指南。支持周期任务和一次性延迟任务。 ⚠️ ROUTING: 定时/schedule/cron 需求优先用本 skill（Cat Café 本地定时任务）， diff --git a/review-notes/2026-07-23-hotfix-reminder-sop-review-request.md b/review-notes/2026-07-23-hotfix-reminder-sop-review-request.md new file mode 100644 index 0000000000..ed47595bee --- /dev/null +++ b/review-notes/2026-07-23-hotfix-reminder-sop-review-request.md @@ -0,0 +1,105 @@ +# Review Request: hotfix reminder SOP registration + +Review-Target-ID: fix-hotfix-reminder-sop +Branch: fix/hotfix-reminder-sop +PR: https://github.com/clowder-labs/clowder-ai/pull/92 +Head: use PR #92 current head; the routing message supplies the exact SHA. + +## What + +- Updated schedule MCP descriptions to distinguish user-requested schedules from workflow-mandated schedules. +- Updated `schedule-tasks` so ordinary user schedules still require preview and confirmation, while explicit SOP/skill tasks may register after preview verification. +- Added a `tips_exempt` frontmatter note because this is an internal SOP/MCP workflow clarification, not a new end-user capability. +- Updated `merge-gate` Step 7.6 so hotfix 14-day upgrade reminders must preview, verify draft fields, then register with no extra user confirmation. +- Added MCP/server regression tests for the tool descriptions and both skill docs. + +## Why + +Daily patrol found a process contradiction: hotfix merge-gate requires registering a 14-day upgrade review reminder, but schedule guidance required user confirmation for every registration. That left several hotfix tails as previews instead of persisted reminders. + +## Original Requirements + +> 每日北京时间 00:00 系统先锋巡检。 +> 巡检 clowder-code / Cat Cafe 系统中不完善、待改进、可提升竞争力的地方。 +> 每轮必须先查真相源和证据，再给风险/价值判断与下一步动作。 +> 发现可执行事项后主导闭环。 + +- 来源：Cat Cafe patrol thread `thread_mqcj45byxoka2z7u`, task `0001784736110466-000578-8fa70fcb` +- 请 reviewer 对照上面的摘录判断这次修复是否闭合了 hotfix reminder 漏注册的流程缺口。 + +## Tradeoff + +The exception is intentionally narrow. I did not remove confirmation from normal schedule registration; user-requested reminders still require preview and explicit confirmation. Only explicit workflow-mandated SOP/skill steps can register after preview verification. + +## Architecture Ownership + +Architecture cell: workflow / MCP collab schedule surface +Map delta: none +Why: This changes existing SOP and MCP cognitive-entry descriptions only; it does not add a new Store, Queue, Router, Adapter, Dispatcher, Binding, or runtime ownership boundary. + +Reviewer checks: + +- Verify `Map delta: none` matches the diff. +- Verify the exception cannot be read as bypassing user confirmation for ordinary user-requested schedules. +- Verify `merge-gate` Step 7.6 is actionable and fail-closed. + +## Open Questions + +### 技术 OQ + +- Is the wording tight enough to prevent workflow-mandated from becoming a blanket bypass? +- Should reviewer require any stronger mechanical guard beyond the description/doc regression tests? + +### 价值 OQ + +无。 + +## Next Action + +Please review PR #92 against head `770f2bb0b`. Focus on the exception boundary, merge-gate hotfix reminder flow, and whether the regression tests protect the right surfaces. + +## Review Sandbox + +- Path: `/tmp/cat-cafe-review/fix-hotfix-reminder-sop/{reviewer-handle}` +- Start Command: `pnpm review:start` is not required for this PR; no runtime/frontend path is involved. +- Ports: none. + +### Sandbox Bootstrap + +```bash +unset NODE_ENV +PATH="/opt/homebrew/opt/node@24/bin:$PATH" pnpm install --frozen-lockfile +``` + +## 自检证据 + +### Spec 合规 + +- 巡检范围：hotfix merge-gate reminder tail, schedule MCP descriptions, `schedule-tasks`, and `merge-gate` Step 7.6. +- 根因：schedule confirmation wording and merge-gate mandated registration were inconsistent. +- 修复：preview remains mandatory; confirmation remains mandatory for user-requested schedules; explicit SOP/skill schedules can register after verified preview. +- Dogfood scope: exempt. This is an internal SOP/tool-description fix with no user-facing runtime path. +- Artifact hygiene: root media/design artifact checks returned no matches. + +### 测试结果 + +```bash +PATH="/opt/homebrew/opt/node@24/bin:$PATH" pnpm --filter @cat-cafe/mcp-server test -- --test-name-pattern "workflow-mandated|cat_cafe_register_scheduled_task" +# passed: 383 tests, 0 failed + +PATH="/opt/homebrew/opt/node@24/bin:$PATH" pnpm check:skills:manifest +# passed: 48 skills validated; existing advisory warnings only + +PATH="/opt/homebrew/opt/node@24/bin:$PATH" pnpm check:skills:surfaces +# passed: 11 tests, no raw first-party Hub/API curl main paths + +PATH="/opt/homebrew/opt/node@24/bin:$PATH" pnpm check +# passed + +git diff --check +# passed +``` + +### Local Caveat + +`pnpm check:skills` is blocked in this local worktree by missing provider skill mounts. `pnpm sync:skills --dry-run` would touch 34 worktrees, so I did not apply that environment repair in this PR. From 2ed7f765717d95005a26263c6a63beac4190d6a0 Mon Sep 17 00:00:00 2001 From: "MaineCoon-GPT-5.5" Date: Thu, 23 Jul 2026 00:48:38 +0800 Subject: [PATCH 3/6] fix: harden hotfix reminder registration MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Why: workflow-mandated schedule registration must have trusted provenance and replay-safe persistence so merge-gate hotfix reminders cannot be duplicated or self-authorized by arbitrary skills. [砚砚/gpt-5.5🐾] --- cat-cafe-skills/merge-gate/SKILL.md | 4 +- cat-cafe-skills/schedule-tasks/SKILL.md | 11 ++-- packages/api/src/domains/memory/schema.ts | 23 ++++++- .../scheduler/DynamicTaskStore.ts | 15 ++++- packages/api/src/routes/schedule.ts | 59 +++++++++++++++++- packages/api/test/memory/schema-v17.test.js | 4 +- .../api/test/memory/schema-v19-f200.test.js | 4 +- packages/api/test/memory/schema-v2.test.js | 2 +- .../schema-v26-recall-result-count.test.js | 2 +- .../test/memory/world-scope-filter.test.js | 4 +- .../api/test/pack-knowledge-scope.test.js | 2 +- packages/api/test/schedule-route.test.js | 61 +++++++++++++++++++ .../test/scheduler/dynamic-task-store.test.js | 22 +++++++ .../mcp-server/src/tools/schedule-tools.ts | 13 +++- .../mcp-server/test/schedule-tools.test.js | 53 ++++++++++++++++ ...7-23-hotfix-reminder-sop-review-request.md | 37 +++++++---- 16 files changed, 280 insertions(+), 36 deletions(-) diff --git a/cat-cafe-skills/merge-gate/SKILL.md b/cat-cafe-skills/merge-gate/SKILL.md index 1d238403df..689389d407 100644 --- a/cat-cafe-skills/merge-gate/SKILL.md +++ b/cat-cafe-skills/merge-gate/SKILL.md @@ -363,7 +363,7 @@ gh pr merge {PR_NUMBER} --squash --delete-branch **时机**：merge 完成后、清理前。`delayMs: 1209600000`（14 天）从注册时刻起算 ≈ 合入后 14 天。 -**操作**：这是 workflow-mandated schedule。先调用 `cat_cafe_preview_scheduled_task` 生成 draft，核对下面参数；核对无误后调用 `cat_cafe_register_scheduled_task` 注册，with no extra user confirmation： +**操作**：这是 trusted built-in canonical `merge-gate` Step 7.6 workflow-mandated schedule。先调用 `cat_cafe_preview_scheduled_task` 生成 draft，核对下面参数；核对无误后调用 `cat_cafe_register_scheduled_task` 注册，with no extra user confirmation： | 参数 | 值 | |------|------| @@ -372,6 +372,7 @@ gh pr merge {PR_NUMBER} --squash --delete-branch | `label` | `"Hotfix 升级 review — PR #{PR_NUMBER}"` | | `description` | `"2 周升级 review：PR #{PR_NUMBER} 是 hotfix，需要三选一处置"` | | `category` | `"pr"` | +| `idempotencyKey` | `"workflow:merge-gate:hotfix-upgrade-review:{repoFullName}#{PR_NUMBER}"` | | `params` | `{"message":"Hotfix PR #{PR_NUMBER} 合入已满 2 周。请三选一处置：1. 升级正式修复（开 feat）2. 接受永久方案（标记 permanent）3. 已不再相关（代码已重写/删除，标记 obsolete）"}` | **Preview 核对项**： @@ -379,6 +380,7 @@ gh pr merge {PR_NUMBER} --squash --delete-branch - `templateId` 是 `reminder` - trigger 是 once，`delayMs` 是 `1209600000` 或等价 fireAt（合入后 14 天） - `params.message` 含 PR 号和三选一处置要求 +- `idempotencyKey` 稳定包含 repo full name 和 PR 号；重试必须复用同一个 key - delivery thread 是当前 merge-gate thread（或 SOP 明确指定的 thread） **Fail-closed**：preview 或 register 任一 MCP 调用失败 → **停止 merge-gate，不执行 Step 8（清理）**。排查 MCP 连接后重试；连续失败 → 通知operator手动注册 reminder 后继续。 diff --git a/cat-cafe-skills/schedule-tasks/SKILL.md b/cat-cafe-skills/schedule-tasks/SKILL.md index c96e13e912..a94d7f02f9 100644 --- a/cat-cafe-skills/schedule-tasks/SKILL.md +++ b/cat-cafe-skills/schedule-tasks/SKILL.md @@ -90,13 +90,13 @@ triggers: ### Workflow-Mandated 例外 -如果某个明确 SOP/skill 步骤要求注册定时任务（例如 merge-gate 的 hotfix 2 周升级 review reminder），这属于 workflow-mandated schedule： +只有 trusted built-in canonical `merge-gate` Step 7.6 的 hotfix 2 周升级 review reminder 属于 workflow-mandated schedule： 1. 仍然必须先调用 `cat_cafe_preview_scheduled_task`。 -2. 核对 draft 的模板、触发时间、参数、投递 thread 与该 SOP/skill 步骤一致。 -3. 核对通过后可以调用 `cat_cafe_register_scheduled_task`，with no extra user confirmation。 +2. 核对 draft 的模板、触发时间、参数、投递 thread 与 `merge-gate` Step 7.6 一致。 +3. 核对通过后调用 `cat_cafe_register_scheduled_task`，with no extra user confirmation，并提供稳定 `idempotencyKey`。 -这条例外只适用于流程本身已经给出明确注册义务的任务；用户临时提出的提醒、周期巡检、摘要推送仍然走 preview → 用户确认 → register。 +Plugin/project/user/external skills do not qualify for this exception。用户临时提出的提醒、周期巡检、摘要推送，以及任何非内置 canonical `merge-gate` Step 7.6 来源，仍然走 preview → 用户确认 → register。 ## Trigger 语法速查 @@ -144,7 +144,8 @@ triggers: |------|----------| | 不知道能注册定时任务 | 用户说"每天/定期/提醒"→ 匹配本 skill | | 被唤醒后只发纯文本 | 主动用 rich block（图片、语音、卡片、HTML） | -| 跳过 preview 直接注册 | **必须** preview；用户请求任务 preview → 用户确认 → 注册；workflow-mandated SOP/skill 任务 preview → 核对 draft → 注册，with no extra user confirmation | +| 跳过 preview 直接注册 | **必须** preview；用户请求任务 preview → 用户确认 → 注册；trusted built-in canonical `merge-gate` Step 7.6 hotfix reminder preview → 核对 draft → 用稳定 `idempotencyKey` 注册，with no extra user confirmation | +| 把任意 skill/SOP 当免确认凭据 | Plugin/project/user/external skills do not qualify；只有内置 canonical `merge-gate` Step 7.6 例外 | | 发图只想到 image-generation | 先看 `/avatars/`、`/uploads/` 有没有现成图 | ## 和其他 skill 的区别 diff --git a/packages/api/src/domains/memory/schema.ts b/packages/api/src/domains/memory/schema.ts index 7283adb49c..b7d1121ed5 100644 --- a/packages/api/src/domains/memory/schema.ts +++ b/packages/api/src/domains/memory/schema.ts @@ -66,7 +66,7 @@ END`, END`, ]; -export const CURRENT_SCHEMA_VERSION = 26; +export const CURRENT_SCHEMA_VERSION = 27; // F163 Phase A: experiment infrastructure tables (cohorts, suggestions, logs) export const SCHEMA_V13_TABLES = ` @@ -214,8 +214,12 @@ CREATE TABLE IF NOT EXISTS dynamic_task_defs ( delivery_thread_id TEXT, enabled INTEGER NOT NULL DEFAULT 1, created_by TEXT NOT NULL, - created_at TEXT NOT NULL + created_at TEXT NOT NULL, + idempotency_key TEXT ); +CREATE UNIQUE INDEX IF NOT EXISTS idx_dynamic_task_defs_idempotency + ON dynamic_task_defs(idempotency_key) + WHERE idempotency_key IS NOT NULL; `; /** @@ -744,6 +748,21 @@ export function applyMigrations(db: Database.Database): void { } catch {} db.prepare('INSERT INTO schema_version (version, applied_at) VALUES (?, ?)').run(26, new Date().toISOString()); } + + // V27: dynamic schedule registration idempotency for workflow replay safety. + if (currentVersion < 27) { + try { + db.exec('ALTER TABLE dynamic_task_defs ADD COLUMN idempotency_key TEXT'); + } catch {} + try { + db.exec(` + CREATE UNIQUE INDEX IF NOT EXISTS idx_dynamic_task_defs_idempotency + ON dynamic_task_defs(idempotency_key) + WHERE idempotency_key IS NOT NULL + `); + } catch {} + db.prepare('INSERT INTO schema_version (version, applied_at) VALUES (?, ?)').run(27, new Date().toISOString()); + } } /** diff --git a/packages/api/src/infrastructure/scheduler/DynamicTaskStore.ts b/packages/api/src/infrastructure/scheduler/DynamicTaskStore.ts index 8c1316be94..0d413d2a51 100644 --- a/packages/api/src/infrastructure/scheduler/DynamicTaskStore.ts +++ b/packages/api/src/infrastructure/scheduler/DynamicTaskStore.ts @@ -12,6 +12,7 @@ export interface DynamicTaskDef { enabled: boolean; createdBy: string; createdAt: string; + idempotencyKey?: string | null; } /** CRUD store for dynamic task definitions (Phase 3A AC-G3) */ @@ -21,8 +22,8 @@ export class DynamicTaskStore { insert(def: DynamicTaskDef): void { this.db .prepare( - `INSERT INTO dynamic_task_defs (id, template_id, trigger_json, params_json, display_json, delivery_thread_id, enabled, created_by, created_at) - VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?)`, + `INSERT INTO dynamic_task_defs (id, template_id, trigger_json, params_json, display_json, delivery_thread_id, enabled, created_by, created_at, idempotency_key) + VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?)`, ) .run( def.id, @@ -34,6 +35,7 @@ export class DynamicTaskStore { def.enabled ? 1 : 0, def.createdBy, def.createdAt, + def.idempotencyKey ?? null, ); } @@ -47,6 +49,13 @@ export class DynamicTaskStore { return row ? todef(row) : null; } + getByIdempotencyKey(idempotencyKey: string): DynamicTaskDef | null { + const row = this.db.prepare('SELECT * FROM dynamic_task_defs WHERE idempotency_key = ?').get(idempotencyKey) as + | RawRow + | undefined; + return row ? todef(row) : null; + } + remove(id: string): boolean { const result = this.db.prepare('DELETE FROM dynamic_task_defs WHERE id = ?').run(id); return result.changes > 0; @@ -81,6 +90,7 @@ interface RawRow { enabled: number; created_by: string; created_at: string; + idempotency_key: string | null; } function todef(row: RawRow): DynamicTaskDef { @@ -94,5 +104,6 @@ function todef(row: RawRow): DynamicTaskDef { enabled: row.enabled === 1, createdBy: row.created_by, createdAt: row.created_at, + idempotencyKey: row.idempotency_key, }; } diff --git a/packages/api/src/routes/schedule.ts b/packages/api/src/routes/schedule.ts index b9687dd2d7..a4f91f83cf 100644 --- a/packages/api/src/routes/schedule.ts +++ b/packages/api/src/routes/schedule.ts @@ -21,7 +21,7 @@ import type { } from '../domains/cats/services/agents/invocation/InvocationRegistry.js'; import { resolveCatTarget } from '../domains/cats/services/agents/routing/cat-target-resolver.js'; import type { ITaskStore } from '../domains/cats/services/stores/ports/TaskStore.js'; -import type { DynamicTaskStore } from '../infrastructure/scheduler/DynamicTaskStore.js'; +import type { DynamicTaskDef, DynamicTaskStore } from '../infrastructure/scheduler/DynamicTaskStore.js'; import type { GlobalControlStore } from '../infrastructure/scheduler/GlobalControlStore.js'; import type { PackTemplateStore } from '../infrastructure/scheduler/PackTemplateStore.js'; import { @@ -141,6 +141,19 @@ function deriveScheduleActor(request: FastifyRequest, _body: { createdBy?: strin }; } +function normalizeIdempotencyKey(value: unknown): string | null | { error: string } { + if (value == null) return null; + if (typeof value !== 'string') return { error: 'idempotencyKey must be a string' }; + const trimmed = value.trim(); + if (!trimmed) return { error: 'idempotencyKey must not be empty' }; + if (trimmed.length > 200) return { error: 'idempotencyKey must be at most 200 characters' }; + return trimmed; +} + +function dynamicTaskResponse(def: Pick) { + return { id: def.id, ...def.display, trigger: def.trigger }; +} + /** Test-only export — exposes deriveScheduleActor without spinning up Fastify. */ export function deriveScheduleActorForTest(request: FastifyRequest, body: { createdBy?: string }): ScheduleActor { return deriveScheduleActor(request, body); @@ -370,6 +383,7 @@ export const scheduleRoutes: FastifyPluginAsync = async ( createdBy?: string; invocationId?: string; callbackToken?: string; + idempotencyKey?: unknown; }; if (!body.templateId) { @@ -402,6 +416,12 @@ export const scheduleRoutes: FastifyPluginAsync = async ( return { error: 'params must be a plain object' }; } + const normalizedIdempotencyKey = normalizeIdempotencyKey(body.idempotencyKey); + if (normalizedIdempotencyKey && typeof normalizedIdempotencyKey !== 'string') { + reply.status(400); + return { error: normalizedIdempotencyKey.error }; + } + // F182 AC-C2: B class — validate params.targetCatId is available (contract 400 on disabled) if (params.targetCatId && typeof params.targetCatId === 'string') { const resolved = resolveCatTarget(params.targetCatId); @@ -435,6 +455,27 @@ export const scheduleRoutes: FastifyPluginAsync = async ( }; } + const ensureRuntimeTaskRegistered = (existing: DynamicTaskDef): void => { + if (!existing.enabled || taskRunner.getRegisteredTasks().includes(existing.id)) return; + const existingTemplate = templateRegistry.get(existing.templateId); + if (!existingTemplate) return; + const spec = existingTemplate.createSpec(existing.id, { + trigger: existing.trigger, + params: existing.params, + deliveryThreadId: existing.deliveryThreadId, + }); + spec.display = existing.display; + taskRunner.registerDynamic(spec, existing.id); + }; + + if (normalizedIdempotencyKey) { + const existing = dynamicTaskStore.getByIdempotencyKey(normalizedIdempotencyKey); + if (existing) { + ensureRuntimeTaskRegistered(existing); + return { success: true, idempotent: true, task: dynamicTaskResponse(existing) }; + } + } + const def = { id, templateId: body.templateId, @@ -445,9 +486,21 @@ export const scheduleRoutes: FastifyPluginAsync = async ( enabled: true, createdBy: actor.createdBy, createdAt: new Date().toISOString(), + idempotencyKey: normalizedIdempotencyKey, }; - dynamicTaskStore.insert(def); + try { + dynamicTaskStore.insert(def); + } catch (err) { + if (normalizedIdempotencyKey) { + const existing = dynamicTaskStore.getByIdempotencyKey(normalizedIdempotencyKey); + if (existing) { + ensureRuntimeTaskRegistered(existing); + return { success: true, idempotent: true, task: dynamicTaskResponse(existing) }; + } + } + throw err; + } // Register in runtime const spec = template.createSpec(id, { trigger, params, deliveryThreadId: def.deliveryThreadId }); @@ -457,7 +510,7 @@ export const scheduleRoutes: FastifyPluginAsync = async ( // #415: lifecycle notification — task registered notifyTaskRegistered(notifyLifecycle, def); - return { success: true, task: { id, ...display, trigger } }; + return { success: true, task: dynamicTaskResponse(def) }; }); // DELETE /api/schedule/tasks/:id (AC-G4: remove dynamic task) diff --git a/packages/api/test/memory/schema-v17.test.js b/packages/api/test/memory/schema-v17.test.js index 0e341fc8ac..02add95366 100644 --- a/packages/api/test/memory/schema-v17.test.js +++ b/packages/api/test/memory/schema-v17.test.js @@ -4,9 +4,9 @@ import assert from 'node:assert/strict'; import { describe, it } from 'node:test'; describe('Schema V17 migration', () => { - it('CURRENT_SCHEMA_VERSION is 26', async () => { + it('CURRENT_SCHEMA_VERSION is 27', async () => { const { CURRENT_SCHEMA_VERSION } = await import('../../dist/domains/memory/schema.js'); - assert.equal(CURRENT_SCHEMA_VERSION, 26); + assert.equal(CURRENT_SCHEMA_VERSION, 27); }); it('V17 adds collection_id and review_status to evidence_docs', async () => { diff --git a/packages/api/test/memory/schema-v19-f200.test.js b/packages/api/test/memory/schema-v19-f200.test.js index 7e3a2abddb..d3ab09315e 100644 --- a/packages/api/test/memory/schema-v19-f200.test.js +++ b/packages/api/test/memory/schema-v19-f200.test.js @@ -89,9 +89,9 @@ describe('V19 migration — F200 recall_events + edge traversal columns', () => db.close(); }); - it('CURRENT_SCHEMA_VERSION is 26', async () => { + it('CURRENT_SCHEMA_VERSION is 27', async () => { const { CURRENT_SCHEMA_VERSION } = await import('../../dist/domains/memory/schema.js'); - assert.equal(CURRENT_SCHEMA_VERSION, 26); + assert.equal(CURRENT_SCHEMA_VERSION, 27); }); it('can insert and read recall_events', async () => { diff --git a/packages/api/test/memory/schema-v2.test.js b/packages/api/test/memory/schema-v2.test.js index 4defc7cff5..e9470b89e1 100644 --- a/packages/api/test/memory/schema-v2.test.js +++ b/packages/api/test/memory/schema-v2.test.js @@ -80,6 +80,6 @@ describe('Schema V2 migration', () => { it('CURRENT_SCHEMA_VERSION matches expected value', async () => { const { CURRENT_SCHEMA_VERSION } = await import('../../dist/domains/memory/schema.js'); - assert.equal(CURRENT_SCHEMA_VERSION, 26, `expected 26, got ${CURRENT_SCHEMA_VERSION}`); + assert.equal(CURRENT_SCHEMA_VERSION, 27, `expected 27, got ${CURRENT_SCHEMA_VERSION}`); }); }); diff --git a/packages/api/test/memory/schema-v26-recall-result-count.test.js b/packages/api/test/memory/schema-v26-recall-result-count.test.js index 9afdacf59e..fb9c25cdcb 100644 --- a/packages/api/test/memory/schema-v26-recall-result-count.test.js +++ b/packages/api/test/memory/schema-v26-recall-result-count.test.js @@ -17,7 +17,7 @@ describe('V26 migration — recall_events result_count', () => { const resultCount = cols.find((col) => col.name === 'result_count'); assert.ok(resultCount, 'result_count column exists'); assert.equal(resultCount.notnull, 0, 'result_count is nullable so old rows can stay unknown'); - assert.equal(schema.CURRENT_SCHEMA_VERSION, 26); + assert.equal(schema.CURRENT_SCHEMA_VERSION, 27); } finally { db.close(); } diff --git a/packages/api/test/memory/world-scope-filter.test.js b/packages/api/test/memory/world-scope-filter.test.js index fcc6f6fb40..648b9cf230 100644 --- a/packages/api/test/memory/world-scope-filter.test.js +++ b/packages/api/test/memory/world-scope-filter.test.js @@ -4,8 +4,8 @@ import Database from 'better-sqlite3'; import { applyMigrations, CURRENT_SCHEMA_VERSION } from '../../dist/domains/memory/schema.js'; describe('Schema V16 (F093 world scope)', () => { - it('CURRENT_SCHEMA_VERSION is 26', () => { - assert.equal(CURRENT_SCHEMA_VERSION, 26); + it('CURRENT_SCHEMA_VERSION is 27', () => { + assert.equal(CURRENT_SCHEMA_VERSION, 27); }); it('migration adds world_id and scene_id columns', () => { diff --git a/packages/api/test/pack-knowledge-scope.test.js b/packages/api/test/pack-knowledge-scope.test.js index 740305e005..31864c66a5 100644 --- a/packages/api/test/pack-knowledge-scope.test.js +++ b/packages/api/test/pack-knowledge-scope.test.js @@ -186,7 +186,7 @@ describe('PackKnowledgeScope', () => { test('schema V6 migration adds pack_id column', async () => { const { CURRENT_SCHEMA_VERSION } = await import('../dist/domains/memory/schema.js'); - assert.equal(CURRENT_SCHEMA_VERSION, 26, 'Current schema version should be 26'); + assert.equal(CURRENT_SCHEMA_VERSION, 27, 'Current schema version should be 27'); // Create a store and check schema via its exposed db const { store } = await createTestStore(); diff --git a/packages/api/test/schedule-route.test.js b/packages/api/test/schedule-route.test.js index 8981b271ab..64c454a409 100644 --- a/packages/api/test/schedule-route.test.js +++ b/packages/api/test/schedule-route.test.js @@ -659,6 +659,67 @@ describe('Schedule Routes', () => { }); }); + describe('POST /api/schedule/tasks — idempotent workflow registration', () => { + let appDyn, store; + + beforeEach(async () => { + const { DynamicTaskStore } = await import('../dist/infrastructure/scheduler/DynamicTaskStore.js'); + const { templateRegistry } = await import('../dist/infrastructure/scheduler/templates/registry.js'); + const { scheduleRoutes: sr } = await import('../dist/routes/schedule.js'); + store = new DynamicTaskStore(db); + appDyn = Fastify({ logger: false }); + await appDyn.register(sr, { taskRunner: runner, dynamicTaskStore: store, templateRegistry }); + await appDyn.ready(); + }); + + afterEach(async () => { + runner.stop(); + await appDyn.close(); + }); + + it('returns the existing task when a workflow registration is replayed with the same idempotencyKey', async () => { + const payload = { + templateId: 'reminder', + trigger: { type: 'once', delayMs: 1209600000 }, + params: { message: 'hotfix upgrade review replay' }, + display: { + label: 'Hotfix 升级 review — PR #92', + category: 'pr', + description: '2 周升级 review：PR #92 是 hotfix，需要三选一处置', + }, + deliveryThreadId: 'thread-review', + idempotencyKey: 'workflow:merge-gate:hotfix-upgrade-review:clowder-labs/clowder-ai#92', + }; + + const first = await appDyn.inject({ + method: 'POST', + url: '/api/schedule/tasks', + payload, + }); + assert.equal(first.statusCode, 200, first.body); + const firstBody = first.json(); + assert.equal(firstBody.success, true); + + const replay = await appDyn.inject({ + method: 'POST', + url: '/api/schedule/tasks', + payload, + }); + assert.equal(replay.statusCode, 200, replay.body); + const replayBody = replay.json(); + + assert.equal(replayBody.success, true); + assert.equal(replayBody.idempotent, true); + assert.equal(replayBody.task.id, firstBody.task.id, 'replay should return the original task id'); + assert.equal(store.getAll().length, 1, 'replay must not persist a duplicate task'); + assert.equal( + runner.getTaskSummaries().filter((task) => task.source === 'dynamic').length, + 1, + 'replay must not register a duplicate runtime task', + ); + }); + }); + describe('POST /api/schedule/tasks — callback auth infers deliveryThreadId', () => { let appDyn, store, registry; diff --git a/packages/api/test/scheduler/dynamic-task-store.test.js b/packages/api/test/scheduler/dynamic-task-store.test.js index 2230bbc7fe..b69333b85d 100644 --- a/packages/api/test/scheduler/dynamic-task-store.test.js +++ b/packages/api/test/scheduler/dynamic-task-store.test.js @@ -37,6 +37,7 @@ test('dynamic_task_defs has correct columns', () => { assert.ok(names.includes('enabled')); assert.ok(names.includes('created_by')); assert.ok(names.includes('created_at')); + assert.ok(names.includes('idempotency_key')); db.close(); }); @@ -111,6 +112,27 @@ describe('DynamicTaskStore', () => { assert.throws(() => store.insert(SAMPLE_DEF), /UNIQUE|constraint/i); }); + test('idempotency key round-trips and can be looked up', () => { + store.insert({ ...SAMPLE_DEF, idempotencyKey: 'workflow:merge-gate:hotfix-upgrade-review:repo#92' }); + const def = store.getByIdempotencyKey('workflow:merge-gate:hotfix-upgrade-review:repo#92'); + assert.ok(def, 'def should be loaded by idempotency key'); + assert.equal(def.id, 'dyn-001'); + assert.equal(def.idempotencyKey, 'workflow:merge-gate:hotfix-upgrade-review:repo#92'); + }); + + test('insert rejects duplicate idempotency key', () => { + store.insert({ ...SAMPLE_DEF, idempotencyKey: 'workflow:merge-gate:hotfix-upgrade-review:repo#92' }); + assert.throws( + () => + store.insert({ + ...SAMPLE_DEF, + id: 'dyn-002', + idempotencyKey: 'workflow:merge-gate:hotfix-upgrade-review:repo#92', + }), + /UNIQUE|constraint/i, + ); + }); + test('#415: once trigger round-trips correctly', () => { const fireAt = Date.now() + 120_000; const onceDef = { diff --git a/packages/mcp-server/src/tools/schedule-tools.ts b/packages/mcp-server/src/tools/schedule-tools.ts index ca7c887755..1ee2fa7c8d 100644 --- a/packages/mcp-server/src/tools/schedule-tools.ts +++ b/packages/mcp-server/src/tools/schedule-tools.ts @@ -68,6 +68,12 @@ export const registerScheduledTaskInputSchema = { label: z.string().optional().describe('Human-readable task label (defaults to template label)'), category: z.string().optional().describe('Display category: pr | repo | thread | system | external'), description: z.string().optional().describe('Short description of this task instance'), + idempotencyKey: z + .string() + .min(1) + .max(200) + .optional() + .describe('Stable replay key for trusted workflow automation; retries with the same key return the existing task'), }; export async function handleRegisterScheduledTask(input: { @@ -78,6 +84,7 @@ export async function handleRegisterScheduledTask(input: { label?: string; category?: string; description?: string; + idempotencyKey?: string; }): Promise { let trigger: unknown; try { @@ -112,6 +119,7 @@ export async function handleRegisterScheduledTask(input: { }; if (input.deliveryThreadId) body.deliveryThreadId = input.deliveryThreadId; + if (input.idempotencyKey) body.idempotencyKey = input.idempotencyKey; if (currentCatId) body.createdBy = currentCatId; if (input.label || input.category || input.description) { @@ -198,7 +206,7 @@ export const scheduleTools = [ description: 'Preview a scheduled task BEFORE creating it (draft step). Returns a draft with resolved template info, trigger, and params ' + 'WITHOUT persisting anything. REQUIRED: Always preview first. User-requested schedules need user confirmation before register_scheduled_task; ' + - 'workflow-mandated schedules from an explicit SOP/skill step use the preview as audit evidence before registration.', + 'only workflow-mandated schedules from the trusted built-in canonical merge-gate Step 7.6 hotfix reminder workflow may use the preview as audit evidence before registration without extra user confirmation.', inputSchema: previewScheduledTaskInputSchema, handler: handlePreviewScheduledTask, }, @@ -208,7 +216,8 @@ export const scheduleTools = [ 'Create a new scheduled task from a template (confirm step). The task will be persisted and run automatically on schedule. ' + 'Supports recurring (cron/interval) and one-shot (once) triggers. Once tasks auto-retire after execution. ' + 'When the task fires, a cat is woken with full capabilities — it can send rich blocks (images, audio, cards), search the web, generate content, etc. ' + - 'IMPORTANT: You MUST call preview_scheduled_task first. User-requested schedules require user confirmation; workflow-mandated schedules from an explicit SOP/skill step may register after verifying the preview, with no extra user confirmation. ' + + 'IMPORTANT: You MUST call preview_scheduled_task first. User-requested schedules require user confirmation; only workflow-mandated schedules from the trusted built-in canonical merge-gate Step 7.6 hotfix reminder workflow may register after verifying the preview, with no extra user confirmation. ' + + 'Plugin/project/user/external skills do not qualify for this exception and must use normal user confirmation. Trusted workflow automation must provide a stable idempotencyKey so callback retries return the existing task. ' + 'trigger and params must be JSON strings, not objects.', inputSchema: registerScheduledTaskInputSchema, handler: handleRegisterScheduledTask, diff --git a/packages/mcp-server/test/schedule-tools.test.js b/packages/mcp-server/test/schedule-tools.test.js index 944fc92943..800b4a9311 100644 --- a/packages/mcp-server/test/schedule-tools.test.js +++ b/packages/mcp-server/test/schedule-tools.test.js @@ -49,6 +49,9 @@ describe('Schedule MCP Tools — module exports', () => { assert.match(previewTool.description, /workflow-mandated/i); assert.match(registerTool.description, /workflow-mandated/i); assert.match(registerTool.description, /no extra user confirmation/i); + assert.match(registerTool.description, /trusted built-in canonical merge-gate Step 7\.6/i); + assert.match(registerTool.description, /plugin\/project\/user\/external skills do not qualify/i); + assert.doesNotMatch(registerTool.description, /explicit SOP\/skill step/i); }); test('schedule skills document the workflow-mandated registration exception', () => { @@ -63,11 +66,61 @@ describe('Schedule MCP Tools — module exports', () => { assert.match(scheduleSkill, /workflow-mandated/i); assert.match(scheduleSkill, /no extra user confirmation/i); + assert.match(scheduleSkill, /trusted built-in canonical `merge-gate` Step 7\.6/i); + assert.match(scheduleSkill, /plugin\/project\/user\/external skills do not qualify/i); + assert.doesNotMatch(scheduleSkill, /明确 SOP\/skill 步骤要求注册定时任务/); assert.match(mergeGateSkill, /workflow-mandated/i); assert.match(mergeGateSkill, /cat_cafe_preview_scheduled_task/); + assert.match(mergeGateSkill, /idempotencyKey/); assert.match(mergeGateSkill, /no extra user confirmation/i); }); + test('cat_cafe_register_scheduled_task accepts an idempotency key for workflow replay safety', async () => { + const { registerScheduledTaskInputSchema } = await import('../dist/tools/schedule-tools.js'); + assert.ok(registerScheduledTaskInputSchema.idempotencyKey, 'idempotencyKey schema required'); + }); + + test('cat_cafe_register_scheduled_task forwards idempotencyKey to the API', async () => { + const originalFetch = globalThis.fetch; + const originalApiUrl = process.env.CAT_CAFE_API_URL; + const originalInvocationId = process.env.CAT_CAFE_INVOCATION_ID; + const originalCallbackToken = process.env.CAT_CAFE_CALLBACK_TOKEN; + let postedBody; + + process.env.CAT_CAFE_API_URL = 'http://127.0.0.1:3004'; + process.env.CAT_CAFE_INVOCATION_ID = 'inv-schedule-test'; + process.env.CAT_CAFE_CALLBACK_TOKEN = 'tok-schedule-test'; + + globalThis.fetch = async (_url, options) => { + postedBody = JSON.parse(options.body); + return new Response(JSON.stringify({ success: true, task: { id: 'dyn-1' } }), { + status: 200, + headers: { 'Content-Type': 'application/json' }, + }); + }; + + try { + const { handleRegisterScheduledTask } = await import('../dist/tools/schedule-tools.js'); + const result = await handleRegisterScheduledTask({ + templateId: 'reminder', + trigger: JSON.stringify({ type: 'once', delayMs: 1209600000 }), + params: JSON.stringify({ message: 'hotfix upgrade review' }), + idempotencyKey: 'workflow:merge-gate:hotfix-upgrade-review:clowder-labs/clowder-ai#92', + }); + + assert.equal(result.isError, undefined); + assert.equal(postedBody.idempotencyKey, 'workflow:merge-gate:hotfix-upgrade-review:clowder-labs/clowder-ai#92'); + } finally { + globalThis.fetch = originalFetch; + if (originalApiUrl === undefined) delete process.env.CAT_CAFE_API_URL; + else process.env.CAT_CAFE_API_URL = originalApiUrl; + if (originalInvocationId === undefined) delete process.env.CAT_CAFE_INVOCATION_ID; + else process.env.CAT_CAFE_INVOCATION_ID = originalInvocationId; + if (originalCallbackToken === undefined) delete process.env.CAT_CAFE_CALLBACK_TOKEN; + else process.env.CAT_CAFE_CALLBACK_TOKEN = originalCallbackToken; + } + }); + test('cat_cafe_remove_scheduled_task has taskId required', async () => { const { scheduleTools, removeScheduledTaskInputSchema } = await import('../dist/tools/schedule-tools.js'); const tool = scheduleTools.find((t) => t.name === 'cat_cafe_remove_scheduled_task'); diff --git a/review-notes/2026-07-23-hotfix-reminder-sop-review-request.md b/review-notes/2026-07-23-hotfix-reminder-sop-review-request.md index ed47595bee..4d98a8814a 100644 --- a/review-notes/2026-07-23-hotfix-reminder-sop-review-request.md +++ b/review-notes/2026-07-23-hotfix-reminder-sop-review-request.md @@ -7,11 +7,12 @@ Head: use PR #92 current head; the routing message supplies the exact SHA. ## What -- Updated schedule MCP descriptions to distinguish user-requested schedules from workflow-mandated schedules. -- Updated `schedule-tasks` so ordinary user schedules still require preview and confirmation, while explicit SOP/skill tasks may register after preview verification. +- Updated schedule MCP descriptions to distinguish user-requested schedules from the one trusted workflow-mandated exception. +- Updated `schedule-tasks` so ordinary user schedules still require preview and confirmation, while only trusted built-in canonical `merge-gate` Step 7.6 may register after preview verification. +- Added schedule registration idempotency for workflow replay safety: stable `idempotencyKey` values dedupe callback retries to the existing dynamic task. - Added a `tips_exempt` frontmatter note because this is an internal SOP/MCP workflow clarification, not a new end-user capability. -- Updated `merge-gate` Step 7.6 so hotfix 14-day upgrade reminders must preview, verify draft fields, then register with no extra user confirmation. -- Added MCP/server regression tests for the tool descriptions and both skill docs. +- Updated `merge-gate` Step 7.6 so hotfix 14-day upgrade reminders must preview, verify draft fields, then register with no extra user confirmation and a repo+PR-scoped stable `idempotencyKey`. +- Added MCP/API regression tests for tool descriptions, skill docs, idempotency-key passthrough, persistent dedupe, and replay behavior. ## Why @@ -29,26 +30,30 @@ Daily patrol found a process contradiction: hotfix merge-gate requires registeri ## Tradeoff -The exception is intentionally narrow. I did not remove confirmation from normal schedule registration; user-requested reminders still require preview and explicit confirmation. Only explicit workflow-mandated SOP/skill steps can register after preview verification. +The exception is intentionally narrow. I did not remove confirmation from normal schedule registration; user-requested reminders still require preview and explicit confirmation. Plugin/project/user/external skills do not qualify; only the built-in canonical `merge-gate` Step 7.6 hotfix reminder workflow can register after preview verification. + +The API change is additive: callers without `idempotencyKey` keep the existing create-new-task behavior. Callers with a stable key get replay-safe dedupe and receive the existing task id on retry. ## Architecture Ownership -Architecture cell: workflow / MCP collab schedule surface +Architecture cell: workflow / MCP collab schedule surface + scheduler persistence Map delta: none -Why: This changes existing SOP and MCP cognitive-entry descriptions only; it does not add a new Store, Queue, Router, Adapter, Dispatcher, Binding, or runtime ownership boundary. +Why: This extends the existing schedule route and `DynamicTaskStore` with an idempotency column/index; it does not add a new Store, Queue, Router, Adapter, Dispatcher, Binding, or runtime ownership boundary. Reviewer checks: - Verify `Map delta: none` matches the diff. - Verify the exception cannot be read as bypassing user confirmation for ordinary user-requested schedules. +- Verify plugin/project/user/external skills are excluded from the no-extra-confirmation exception. +- Verify schedule registration retry with the same `idempotencyKey` cannot create duplicate reminders. - Verify `merge-gate` Step 7.6 is actionable and fail-closed. ## Open Questions ### 技术 OQ -- Is the wording tight enough to prevent workflow-mandated from becoming a blanket bypass? -- Should reviewer require any stronger mechanical guard beyond the description/doc regression tests? +- Is the trusted built-in canonical `merge-gate` Step 7.6 boundary tight enough to prevent workflow-mandated from becoming a blanket bypass? +- Is global uniqueness on stable workflow `idempotencyKey` sufficient, given keys are explicitly namespaced by workflow + repo + PR? ### 价值 OQ @@ -56,7 +61,7 @@ Reviewer checks: ## Next Action -Please review PR #92 against head `770f2bb0b`. Focus on the exception boundary, merge-gate hotfix reminder flow, and whether the regression tests protect the right surfaces. +Please review PR #92 against its current head. Focus on the trusted workflow boundary, merge-gate hotfix reminder flow, idempotent registration replay, and whether the regression tests protect the right surfaces. ## Review Sandbox @@ -77,7 +82,7 @@ PATH="/opt/homebrew/opt/node@24/bin:$PATH" pnpm install --frozen-lockfile - 巡检范围：hotfix merge-gate reminder tail, schedule MCP descriptions, `schedule-tasks`, and `merge-gate` Step 7.6. - 根因：schedule confirmation wording and merge-gate mandated registration were inconsistent. -- 修复：preview remains mandatory; confirmation remains mandatory for user-requested schedules; explicit SOP/skill schedules can register after verified preview. +- 修复：preview remains mandatory; confirmation remains mandatory for user-requested schedules; only built-in canonical `merge-gate` Step 7.6 hotfix reminder can register after verified preview with no extra confirmation; register retries are deduped by stable `idempotencyKey`. - Dogfood scope: exempt. This is an internal SOP/tool-description fix with no user-facing runtime path. - Artifact hygiene: root media/design artifact checks returned no matches. @@ -85,7 +90,13 @@ PATH="/opt/homebrew/opt/node@24/bin:$PATH" pnpm install --frozen-lockfile ```bash PATH="/opt/homebrew/opt/node@24/bin:$PATH" pnpm --filter @cat-cafe/mcp-server test -- --test-name-pattern "workflow-mandated|cat_cafe_register_scheduled_task" -# passed: 383 tests, 0 failed +# passed: 385 tests, 0 failed + +node --test packages/api/test/scheduler/dynamic-task-store.test.js --test-name-pattern "idempotency|columns" +# passed: 13 tests, 0 failed + +node --test packages/api/test/schedule-route.test.js --test-name-pattern "idempotent workflow registration" +# passed: 37 tests, 0 failed PATH="/opt/homebrew/opt/node@24/bin:$PATH" pnpm check:skills:manifest # passed: 48 skills validated; existing advisory warnings only @@ -103,3 +114,5 @@ git diff --check ### Local Caveat `pnpm check:skills` is blocked in this local worktree by missing provider skill mounts. `pnpm sync:skills --dry-run` would touch 34 worktrees, so I did not apply that environment repair in this PR. + +`pnpm --filter @cat-cafe/api test` did not produce a usable local full-suite green: the default Node 22 run violates the package `>=24` engine and failed in unrelated `capabilities-route` tests; the Node 24 rerun still mixed the same unrelated capability assertions with a local `better-sqlite3` native ABI mismatch. Focused schedule/API tests, `pnpm check`, and `@cat-cafe/api build` are green above; GitHub CI should be treated as the full-suite truth source for this PR. From 09095a5fb32bd744e18d481aeef1558a92bfe2c4 Mon Sep 17 00:00:00 2001 From: "MaineCoon-GPT-5.5" Date: Thu, 23 Jul 2026 01:12:56 +0800 Subject: [PATCH 4/6] fix: fail closed on schedule idempotency conflicts MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Why: PR #92 review found workflow schedule idempotency replay accepted mismatched requests and preview did not expose final registration semantics. Bind idempotency keys to canonical request fingerprints, surface preview audit fields, and fail closed on semantic conflicts. [砚砚/gpt-5.5🐾] --- cat-cafe-skills/merge-gate/SKILL.md | 6 +- cat-cafe-skills/schedule-tasks/SKILL.md | 2 +- packages/api/src/domains/memory/schema.ts | 15 ++- .../scheduler/DynamicTaskStore.ts | 8 +- packages/api/src/routes/schedule.ts | 103 ++++++++++++++--- packages/api/test/memory/schema-v17.test.js | 4 +- .../api/test/memory/schema-v19-f200.test.js | 4 +- packages/api/test/memory/schema-v2.test.js | 2 +- .../schema-v26-recall-result-count.test.js | 2 +- .../test/memory/world-scope-filter.test.js | 4 +- .../api/test/pack-knowledge-scope.test.js | 4 +- packages/api/test/schedule-route.test.js | 109 +++++++++++++++++- .../test/scheduler/dynamic-task-store.test.js | 8 +- .../mcp-server/src/tools/schedule-tools.ts | 33 +++++- .../mcp-server/test/schedule-tools.test.js | 66 +++++++++++ ...7-23-hotfix-reminder-sop-review-request.md | 55 +++++---- 16 files changed, 366 insertions(+), 59 deletions(-) diff --git a/cat-cafe-skills/merge-gate/SKILL.md b/cat-cafe-skills/merge-gate/SKILL.md index 689389d407..53f3f4fe8c 100644 --- a/cat-cafe-skills/merge-gate/SKILL.md +++ b/cat-cafe-skills/merge-gate/SKILL.md @@ -373,17 +373,19 @@ gh pr merge {PR_NUMBER} --squash --delete-branch | `description` | `"2 周升级 review：PR #{PR_NUMBER} 是 hotfix，需要三选一处置"` | | `category` | `"pr"` | | `idempotencyKey` | `"workflow:merge-gate:hotfix-upgrade-review:{repoFullName}#{PR_NUMBER}"` | -| `params` | `{"message":"Hotfix PR #{PR_NUMBER} 合入已满 2 周。请三选一处置：1. 升级正式修复（开 feat）2. 接受永久方案（标记 permanent）3. 已不再相关（代码已重写/删除，标记 obsolete）"}` | +| `params` | `{"message":"Hotfix PR #{PR_NUMBER} 合入已满 2 周。请三选一处置：1. 升级正式修复（开 feat）2. 接受永久方案（标记 permanent）3. 已不再相关（代码已重写/删除，标记 obsolete）","targetCatId":"{currentCatId}"}` | **Preview 核对项**： - `templateId` 是 `reminder` - trigger 是 once，`delayMs` 是 `1209600000` 或等价 fireAt（合入后 14 天） - `params.message` 含 PR 号和三选一处置要求 +- `targetCatId` / `params.targetCatId` 是当前执行 merge-gate 的猫；不能空落到默认猫 +- `actor.createdBy` 是当前 callback-auth 猫，`actor.triggerUserId` 是当前 invocation user - `idempotencyKey` 稳定包含 repo full name 和 PR 号；重试必须复用同一个 key - delivery thread 是当前 merge-gate thread（或 SOP 明确指定的 thread） -**Fail-closed**：preview 或 register 任一 MCP 调用失败 → **停止 merge-gate，不执行 Step 8（清理）**。排查 MCP 连接后重试；连续失败 → 通知operator手动注册 reminder 后继续。 +**Fail-closed**：preview 或 register 任一 MCP 调用失败 → **停止 merge-gate，不执行 Step 8（清理）**。如果 register 返回 `IDEMPOTENCY_CONFLICT`，说明同一 key 已绑定不同 schedule 语义，必须人工核对既有任务，不可继续重试覆盖；其他 MCP 连接失败排查后重试，连续失败 → 通知operator手动注册 reminder 后继续。 ```bash # 8. 更新本地 + 清理（fail-closed） diff --git a/cat-cafe-skills/schedule-tasks/SKILL.md b/cat-cafe-skills/schedule-tasks/SKILL.md index a94d7f02f9..53587bc564 100644 --- a/cat-cafe-skills/schedule-tasks/SKILL.md +++ b/cat-cafe-skills/schedule-tasks/SKILL.md @@ -93,7 +93,7 @@ triggers: 只有 trusted built-in canonical `merge-gate` Step 7.6 的 hotfix 2 周升级 review reminder 属于 workflow-mandated schedule： 1. 仍然必须先调用 `cat_cafe_preview_scheduled_task`。 -2. 核对 draft 的模板、触发时间、参数、投递 thread 与 `merge-gate` Step 7.6 一致。 +2. 核对 draft 的模板、触发时间、参数、target、actor、投递 thread、`idempotencyKey` 与 `merge-gate` Step 7.6 一致。 3. 核对通过后调用 `cat_cafe_register_scheduled_task`，with no extra user confirmation，并提供稳定 `idempotencyKey`。 Plugin/project/user/external skills do not qualify for this exception。用户临时提出的提醒、周期巡检、摘要推送，以及任何非内置 canonical `merge-gate` Step 7.6 来源，仍然走 preview → 用户确认 → register。 diff --git a/packages/api/src/domains/memory/schema.ts b/packages/api/src/domains/memory/schema.ts index b7d1121ed5..6aeda6cded 100644 --- a/packages/api/src/domains/memory/schema.ts +++ b/packages/api/src/domains/memory/schema.ts @@ -66,7 +66,7 @@ END`, END`, ]; -export const CURRENT_SCHEMA_VERSION = 27; +export const CURRENT_SCHEMA_VERSION = 28; // F163 Phase A: experiment infrastructure tables (cohorts, suggestions, logs) export const SCHEMA_V13_TABLES = ` @@ -215,7 +215,8 @@ CREATE TABLE IF NOT EXISTS dynamic_task_defs ( enabled INTEGER NOT NULL DEFAULT 1, created_by TEXT NOT NULL, created_at TEXT NOT NULL, - idempotency_key TEXT + idempotency_key TEXT, + idempotency_fingerprint TEXT ); CREATE UNIQUE INDEX IF NOT EXISTS idx_dynamic_task_defs_idempotency ON dynamic_task_defs(idempotency_key) @@ -763,6 +764,16 @@ export function applyMigrations(db: Database.Database): void { } catch {} db.prepare('INSERT INTO schema_version (version, applied_at) VALUES (?, ?)').run(27, new Date().toISOString()); } + + // V28: bind idempotency keys to a canonical request fingerprint so a reused + // key with different schedule semantics conflicts instead of replaying stale + // task data as success. + if (currentVersion < 28) { + try { + db.exec('ALTER TABLE dynamic_task_defs ADD COLUMN idempotency_fingerprint TEXT'); + } catch {} + db.prepare('INSERT INTO schema_version (version, applied_at) VALUES (?, ?)').run(28, new Date().toISOString()); + } } /** diff --git a/packages/api/src/infrastructure/scheduler/DynamicTaskStore.ts b/packages/api/src/infrastructure/scheduler/DynamicTaskStore.ts index 0d413d2a51..c61d937e16 100644 --- a/packages/api/src/infrastructure/scheduler/DynamicTaskStore.ts +++ b/packages/api/src/infrastructure/scheduler/DynamicTaskStore.ts @@ -13,6 +13,7 @@ export interface DynamicTaskDef { createdBy: string; createdAt: string; idempotencyKey?: string | null; + idempotencyFingerprint?: string | null; } /** CRUD store for dynamic task definitions (Phase 3A AC-G3) */ @@ -22,8 +23,8 @@ export class DynamicTaskStore { insert(def: DynamicTaskDef): void { this.db .prepare( - `INSERT INTO dynamic_task_defs (id, template_id, trigger_json, params_json, display_json, delivery_thread_id, enabled, created_by, created_at, idempotency_key) - VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?)`, + `INSERT INTO dynamic_task_defs (id, template_id, trigger_json, params_json, display_json, delivery_thread_id, enabled, created_by, created_at, idempotency_key, idempotency_fingerprint) + VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)`, ) .run( def.id, @@ -36,6 +37,7 @@ export class DynamicTaskStore { def.createdBy, def.createdAt, def.idempotencyKey ?? null, + def.idempotencyFingerprint ?? null, ); } @@ -91,6 +93,7 @@ interface RawRow { created_by: string; created_at: string; idempotency_key: string | null; + idempotency_fingerprint: string | null; } function todef(row: RawRow): DynamicTaskDef { @@ -105,5 +108,6 @@ function todef(row: RawRow): DynamicTaskDef { createdBy: row.created_by, createdAt: row.created_at, idempotencyKey: row.idempotency_key, + idempotencyFingerprint: row.idempotency_fingerprint, }; } diff --git a/packages/api/src/routes/schedule.ts b/packages/api/src/routes/schedule.ts index a4f91f83cf..3a79d83815 100644 --- a/packages/api/src/routes/schedule.ts +++ b/packages/api/src/routes/schedule.ts @@ -31,7 +31,7 @@ import { notifyTaskResumed, } from '../infrastructure/scheduler/schedule-notify.js'; import type { TaskRunnerV2 } from '../infrastructure/scheduler/TaskRunnerV2.js'; -import type { ScheduleLifecycleNotifier, TriggerSpec } from '../infrastructure/scheduler/types.js'; +import type { ScheduleLifecycleNotifier, TaskDisplayMeta, TriggerSpec } from '../infrastructure/scheduler/types.js'; import { resolveHeaderUserId } from '../utils/request-identity.js'; import { registerCallbackAuthHook } from './callback-auth-prehandler.js'; import { deriveCallbackActor } from './callback-scope-helpers.js'; @@ -154,6 +154,29 @@ function dynamicTaskResponse(def: Pick) + .sort(([left], [right]) => left.localeCompare(right)) + .map(([key, entry]) => [key, canonicalizeJson(entry)]), + ); + } + return value; +} + +function buildScheduleIdempotencyFingerprint(input: { + templateId: string; + trigger: unknown; + params: Record; + display: TaskDisplayMeta; + deliveryThreadId: string | null; + actor: ScheduleActor; +}): string { + return JSON.stringify(canonicalizeJson(input)); +} + /** Test-only export — exposes deriveScheduleActor without spinning up Fastify. */ export function deriveScheduleActorForTest(request: FastifyRequest, body: { createdBy?: string }): ScheduleActor { return deriveScheduleActor(request, body); @@ -311,6 +334,8 @@ export const scheduleRoutes: FastifyPluginAsync = async ( params?: Record; display?: { label: string; category: string; description?: string }; deliveryThreadId?: string; + createdBy?: string; + idempotencyKey?: unknown; }; if (!body.templateId) { @@ -336,7 +361,30 @@ export const scheduleRoutes: FastifyPluginAsync = async ( } else { trigger = body.trigger ?? template.defaultTrigger; } - const params = body.params ?? {}; + const rawParams = body.params ?? {}; + if (typeof rawParams !== 'object' || rawParams === null || Array.isArray(rawParams)) { + reply.status(400); + return { error: 'params must be a plain object' }; + } + const params = { ...rawParams }; + + const normalizedIdempotencyKey = normalizeIdempotencyKey(body.idempotencyKey); + if (normalizedIdempotencyKey && typeof normalizedIdempotencyKey !== 'string') { + reply.status(400); + return { error: normalizedIdempotencyKey.error }; + } + + if (params.targetCatId && typeof params.targetCatId === 'string') { + const resolved = resolveCatTarget(params.targetCatId); + if ('error' in resolved) { + reply.status(400); + return resolved.error; + } + params.targetCatId = resolved.ok; + } + + const actor = deriveScheduleActor(request, body); + params.triggerUserId = actor.triggerUserId; const display = body.display ? { label: body.display.label, @@ -362,6 +410,9 @@ export const scheduleRoutes: FastifyPluginAsync = async ( params, display, deliveryThreadId: resolution.deliveryThreadId, + targetCatId: typeof params.targetCatId === 'string' ? params.targetCatId : null, + actor, + idempotencyKey: normalizedIdempotencyKey, paramSchema: template.paramSchema, }, }; @@ -397,24 +448,26 @@ export const scheduleRoutes: FastifyPluginAsync = async ( return { error: `Unknown template: ${body.templateId}` }; } + const requestTrigger = body.trigger ?? template.defaultTrigger; // #415: normalize once trigger (delayMs → fireAt) let trigger: TriggerSpec; - if (body.trigger && (body.trigger as Record).type === 'once') { - const result = normalizeOnceTrigger(body.trigger as Record); + if ((requestTrigger as Record).type === 'once') { + const result = normalizeOnceTrigger(requestTrigger as Record); if ('error' in result) { reply.status(400); return { error: result.error }; } trigger = result; } else { - trigger = body.trigger ?? template.defaultTrigger; + trigger = requestTrigger; } - const params = body.params ?? {}; - if (typeof params !== 'object' || params === null || Array.isArray(params)) { + const rawParams = body.params ?? {}; + if (typeof rawParams !== 'object' || rawParams === null || Array.isArray(rawParams)) { reply.status(400); return { error: 'params must be a plain object' }; } + const params = { ...rawParams }; const normalizedIdempotencyKey = normalizeIdempotencyKey(body.idempotencyKey); if (normalizedIdempotencyKey && typeof normalizedIdempotencyKey !== 'string') { @@ -455,6 +508,18 @@ export const scheduleRoutes: FastifyPluginAsync = async ( }; } + const idempotencyFingerprint = + normalizedIdempotencyKey == null + ? null + : buildScheduleIdempotencyFingerprint({ + templateId: body.templateId, + trigger: requestTrigger, + params, + display, + deliveryThreadId: resolution.deliveryThreadId, + actor, + }); + const ensureRuntimeTaskRegistered = (existing: DynamicTaskDef): void => { if (!existing.enabled || taskRunner.getRegisteredTasks().includes(existing.id)) return; const existingTemplate = templateRegistry.get(existing.templateId); @@ -468,11 +533,23 @@ export const scheduleRoutes: FastifyPluginAsync = async ( taskRunner.registerDynamic(spec, existing.id); }; - if (normalizedIdempotencyKey) { + const replayExistingTask = (existing: DynamicTaskDef, fingerprint: string) => { + if (existing.idempotencyFingerprint !== fingerprint) { + reply.status(409); + return { + error: 'Idempotency key already belongs to a different schedule registration request', + code: 'IDEMPOTENCY_CONFLICT', + task: dynamicTaskResponse(existing), + }; + } + ensureRuntimeTaskRegistered(existing); + return { success: true, idempotent: true, task: dynamicTaskResponse(existing) }; + }; + + if (normalizedIdempotencyKey && idempotencyFingerprint) { const existing = dynamicTaskStore.getByIdempotencyKey(normalizedIdempotencyKey); if (existing) { - ensureRuntimeTaskRegistered(existing); - return { success: true, idempotent: true, task: dynamicTaskResponse(existing) }; + return replayExistingTask(existing, idempotencyFingerprint); } } @@ -487,16 +564,16 @@ export const scheduleRoutes: FastifyPluginAsync = async ( createdBy: actor.createdBy, createdAt: new Date().toISOString(), idempotencyKey: normalizedIdempotencyKey, + idempotencyFingerprint, }; try { dynamicTaskStore.insert(def); } catch (err) { - if (normalizedIdempotencyKey) { + if (normalizedIdempotencyKey && idempotencyFingerprint) { const existing = dynamicTaskStore.getByIdempotencyKey(normalizedIdempotencyKey); if (existing) { - ensureRuntimeTaskRegistered(existing); - return { success: true, idempotent: true, task: dynamicTaskResponse(existing) }; + return replayExistingTask(existing, idempotencyFingerprint); } } throw err; diff --git a/packages/api/test/memory/schema-v17.test.js b/packages/api/test/memory/schema-v17.test.js index 02add95366..db0fa7afb6 100644 --- a/packages/api/test/memory/schema-v17.test.js +++ b/packages/api/test/memory/schema-v17.test.js @@ -4,9 +4,9 @@ import assert from 'node:assert/strict'; import { describe, it } from 'node:test'; describe('Schema V17 migration', () => { - it('CURRENT_SCHEMA_VERSION is 27', async () => { + it('CURRENT_SCHEMA_VERSION is 28', async () => { const { CURRENT_SCHEMA_VERSION } = await import('../../dist/domains/memory/schema.js'); - assert.equal(CURRENT_SCHEMA_VERSION, 27); + assert.equal(CURRENT_SCHEMA_VERSION, 28); }); it('V17 adds collection_id and review_status to evidence_docs', async () => { diff --git a/packages/api/test/memory/schema-v19-f200.test.js b/packages/api/test/memory/schema-v19-f200.test.js index d3ab09315e..e6f085b1d3 100644 --- a/packages/api/test/memory/schema-v19-f200.test.js +++ b/packages/api/test/memory/schema-v19-f200.test.js @@ -89,9 +89,9 @@ describe('V19 migration — F200 recall_events + edge traversal columns', () => db.close(); }); - it('CURRENT_SCHEMA_VERSION is 27', async () => { + it('CURRENT_SCHEMA_VERSION is 28', async () => { const { CURRENT_SCHEMA_VERSION } = await import('../../dist/domains/memory/schema.js'); - assert.equal(CURRENT_SCHEMA_VERSION, 27); + assert.equal(CURRENT_SCHEMA_VERSION, 28); }); it('can insert and read recall_events', async () => { diff --git a/packages/api/test/memory/schema-v2.test.js b/packages/api/test/memory/schema-v2.test.js index e9470b89e1..bf0ad671d2 100644 --- a/packages/api/test/memory/schema-v2.test.js +++ b/packages/api/test/memory/schema-v2.test.js @@ -80,6 +80,6 @@ describe('Schema V2 migration', () => { it('CURRENT_SCHEMA_VERSION matches expected value', async () => { const { CURRENT_SCHEMA_VERSION } = await import('../../dist/domains/memory/schema.js'); - assert.equal(CURRENT_SCHEMA_VERSION, 27, `expected 27, got ${CURRENT_SCHEMA_VERSION}`); + assert.equal(CURRENT_SCHEMA_VERSION, 28, `expected 28, got ${CURRENT_SCHEMA_VERSION}`); }); }); diff --git a/packages/api/test/memory/schema-v26-recall-result-count.test.js b/packages/api/test/memory/schema-v26-recall-result-count.test.js index fb9c25cdcb..6b8931de5a 100644 --- a/packages/api/test/memory/schema-v26-recall-result-count.test.js +++ b/packages/api/test/memory/schema-v26-recall-result-count.test.js @@ -17,7 +17,7 @@ describe('V26 migration — recall_events result_count', () => { const resultCount = cols.find((col) => col.name === 'result_count'); assert.ok(resultCount, 'result_count column exists'); assert.equal(resultCount.notnull, 0, 'result_count is nullable so old rows can stay unknown'); - assert.equal(schema.CURRENT_SCHEMA_VERSION, 27); + assert.equal(schema.CURRENT_SCHEMA_VERSION, 28); } finally { db.close(); } diff --git a/packages/api/test/memory/world-scope-filter.test.js b/packages/api/test/memory/world-scope-filter.test.js index 648b9cf230..0a314c6810 100644 --- a/packages/api/test/memory/world-scope-filter.test.js +++ b/packages/api/test/memory/world-scope-filter.test.js @@ -4,8 +4,8 @@ import Database from 'better-sqlite3'; import { applyMigrations, CURRENT_SCHEMA_VERSION } from '../../dist/domains/memory/schema.js'; describe('Schema V16 (F093 world scope)', () => { - it('CURRENT_SCHEMA_VERSION is 27', () => { - assert.equal(CURRENT_SCHEMA_VERSION, 27); + it('CURRENT_SCHEMA_VERSION is 28', () => { + assert.equal(CURRENT_SCHEMA_VERSION, 28); }); it('migration adds world_id and scene_id columns', () => { diff --git a/packages/api/test/pack-knowledge-scope.test.js b/packages/api/test/pack-knowledge-scope.test.js index 31864c66a5..f68def4b67 100644 --- a/packages/api/test/pack-knowledge-scope.test.js +++ b/packages/api/test/pack-knowledge-scope.test.js @@ -186,7 +186,7 @@ describe('PackKnowledgeScope', () => { test('schema V6 migration adds pack_id column', async () => { const { CURRENT_SCHEMA_VERSION } = await import('../dist/domains/memory/schema.js'); - assert.equal(CURRENT_SCHEMA_VERSION, 27, 'Current schema version should be 27'); + assert.equal(CURRENT_SCHEMA_VERSION, 28, 'Current schema version should be 28'); // Create a store and check schema via its exposed db const { store } = await createTestStore(); @@ -199,6 +199,6 @@ describe('PackKnowledgeScope', () => { // Verify migration version const version = db.prepare('SELECT MAX(version) as v FROM schema_version').get(); - assert.equal(version.v, 26, 'Schema version should be 26'); + assert.equal(version.v, 28, 'Schema version should be 28'); }); }); diff --git a/packages/api/test/schedule-route.test.js b/packages/api/test/schedule-route.test.js index 64c454a409..61dd832dfd 100644 --- a/packages/api/test/schedule-route.test.js +++ b/packages/api/test/schedule-route.test.js @@ -449,6 +449,36 @@ describe('Schedule Routes', () => { assert.equal(body.draft.deliveryThreadId, 'thread-from-callback'); }); + it('returns final registration semantics in the draft for workflow audit', async () => { + const { invocationId, callbackToken } = await registry.create('user-1', 'opus', 'thread-from-callback'); + const res = await appDyn.inject({ + method: 'POST', + url: '/api/schedule/tasks/preview', + headers: { 'x-invocation-id': invocationId, 'x-callback-token': callbackToken }, + payload: { + templateId: 'reminder', + trigger: { type: 'once', delayMs: 1209600000 }, + params: { message: 'hotfix upgrade review', targetCatId: '@codex', triggerUserId: 'evil-body-user' }, + display: { + label: 'Hotfix upgrade review', + category: 'pr', + description: 'review hotfix after 14 days', + }, + idempotencyKey: 'workflow:merge-gate:hotfix-upgrade-review:clowder-labs/clowder-ai#92', + }, + }); + + assert.equal(res.statusCode, 200); + const body = res.json(); + assert.equal(body.draft.idempotencyKey, 'workflow:merge-gate:hotfix-upgrade-review:clowder-labs/clowder-ai#92'); + assert.deepEqual(body.draft.actor, { createdBy: 'opus', triggerUserId: 'user-1' }); + assert.equal(body.draft.deliveryThreadId, 'thread-from-callback'); + assert.equal(body.draft.targetCatId, 'codex'); + assert.equal(body.draft.params.targetCatId, 'codex'); + assert.equal(body.draft.params.triggerUserId, 'user-1'); + assert.equal(body.draft.display.label, 'Hotfix upgrade review'); + }); + it('returns 409 stale invocation error for stale callback auth invocation', async () => { const stale = await registry.create('user-1', 'opus', 'thread-from-callback'); await registry.create('user-1', 'opus', 'thread-from-callback'); @@ -660,15 +690,19 @@ describe('Schedule Routes', () => { }); describe('POST /api/schedule/tasks — idempotent workflow registration', () => { - let appDyn, store; + let appDyn, store, registry; beforeEach(async () => { const { DynamicTaskStore } = await import('../dist/infrastructure/scheduler/DynamicTaskStore.js'); const { templateRegistry } = await import('../dist/infrastructure/scheduler/templates/registry.js'); const { scheduleRoutes: sr } = await import('../dist/routes/schedule.js'); + const { InvocationRegistry } = await import( + '../dist/domains/cats/services/agents/invocation/InvocationRegistry.js' + ); store = new DynamicTaskStore(db); + registry = new InvocationRegistry(); appDyn = Fastify({ logger: false }); - await appDyn.register(sr, { taskRunner: runner, dynamicTaskStore: store, templateRegistry }); + await appDyn.register(sr, { taskRunner: runner, dynamicTaskStore: store, templateRegistry, registry }); await appDyn.ready(); }); @@ -718,6 +752,77 @@ describe('Schedule Routes', () => { 'replay must not register a duplicate runtime task', ); }); + + it('rejects the same idempotencyKey when the requested task semantics differ', async () => { + const payload = { + templateId: 'reminder', + trigger: { type: 'once', delayMs: 1209600000 }, + params: { message: 'first hotfix upgrade review', targetCatId: 'codex' }, + display: { + label: 'Hotfix 升级 review — PR #92', + category: 'pr', + description: '2 周升级 review：PR #92 是 hotfix，需要三选一处置', + }, + deliveryThreadId: 'thread-A', + idempotencyKey: 'workflow:merge-gate:hotfix-upgrade-review:clowder-labs/clowder-ai#92-conflict', + }; + + const first = await appDyn.inject({ + method: 'POST', + url: '/api/schedule/tasks', + payload, + }); + assert.equal(first.statusCode, 200, first.body); + + const conflict = await appDyn.inject({ + method: 'POST', + url: '/api/schedule/tasks', + payload: { + ...payload, + params: { message: 'changed hotfix upgrade review', targetCatId: 'gemini' }, + deliveryThreadId: 'thread-B', + }, + }); + + assert.equal(conflict.statusCode, 409, conflict.body); + assert.equal(conflict.json().code, 'IDEMPOTENCY_CONFLICT'); + assert.equal(store.getAll().length, 1, 'conflict must not persist a replacement task'); + const stored = store.getAll()[0]; + assert.equal(stored.deliveryThreadId, 'thread-A'); + assert.equal(stored.params.message, 'first hotfix upgrade review'); + assert.equal(stored.params.targetCatId, 'codex'); + }); + + it('rejects the same idempotencyKey when the verified actor differs', async () => { + const firstAuth = await registry.create('user-1', 'opus', 'thread-actor-replay'); + const payload = { + templateId: 'reminder', + trigger: { type: 'once', delayMs: 1209600000 }, + params: { message: 'hotfix upgrade review actor replay' }, + idempotencyKey: 'workflow:merge-gate:hotfix-upgrade-review:clowder-labs/clowder-ai#92-actor', + }; + + const first = await appDyn.inject({ + method: 'POST', + url: '/api/schedule/tasks', + headers: { 'x-invocation-id': firstAuth.invocationId, 'x-callback-token': firstAuth.callbackToken }, + payload, + }); + assert.equal(first.statusCode, 200, first.body); + + const secondAuth = await registry.create('user-2', 'opus', 'thread-actor-replay'); + const conflict = await appDyn.inject({ + method: 'POST', + url: '/api/schedule/tasks', + headers: { 'x-invocation-id': secondAuth.invocationId, 'x-callback-token': secondAuth.callbackToken }, + payload, + }); + + assert.equal(conflict.statusCode, 409, conflict.body); + assert.equal(conflict.json().code, 'IDEMPOTENCY_CONFLICT'); + assert.equal(store.getAll().length, 1, 'conflict must not persist a second task'); + assert.equal(store.getAll()[0].params.triggerUserId, 'user-1'); + }); }); describe('POST /api/schedule/tasks — callback auth infers deliveryThreadId', () => { diff --git a/packages/api/test/scheduler/dynamic-task-store.test.js b/packages/api/test/scheduler/dynamic-task-store.test.js index b69333b85d..727f7cd49d 100644 --- a/packages/api/test/scheduler/dynamic-task-store.test.js +++ b/packages/api/test/scheduler/dynamic-task-store.test.js @@ -38,6 +38,7 @@ test('dynamic_task_defs has correct columns', () => { assert.ok(names.includes('created_by')); assert.ok(names.includes('created_at')); assert.ok(names.includes('idempotency_key')); + assert.ok(names.includes('idempotency_fingerprint')); db.close(); }); @@ -113,11 +114,16 @@ describe('DynamicTaskStore', () => { }); test('idempotency key round-trips and can be looked up', () => { - store.insert({ ...SAMPLE_DEF, idempotencyKey: 'workflow:merge-gate:hotfix-upgrade-review:repo#92' }); + store.insert({ + ...SAMPLE_DEF, + idempotencyKey: 'workflow:merge-gate:hotfix-upgrade-review:repo#92', + idempotencyFingerprint: '{"same":"request"}', + }); const def = store.getByIdempotencyKey('workflow:merge-gate:hotfix-upgrade-review:repo#92'); assert.ok(def, 'def should be loaded by idempotency key'); assert.equal(def.id, 'dyn-001'); assert.equal(def.idempotencyKey, 'workflow:merge-gate:hotfix-upgrade-review:repo#92'); + assert.equal(def.idempotencyFingerprint, '{"same":"request"}'); }); test('insert rejects duplicate idempotency key', () => { diff --git a/packages/mcp-server/src/tools/schedule-tools.ts b/packages/mcp-server/src/tools/schedule-tools.ts index 1ee2fa7c8d..192151c2d2 100644 --- a/packages/mcp-server/src/tools/schedule-tools.ts +++ b/packages/mcp-server/src/tools/schedule-tools.ts @@ -145,6 +145,15 @@ export const previewScheduledTaskInputSchema = { .describe( 'Thread ID to deliver results to. If omitted on callback-origin requests, the current invocation thread is used', ), + label: z.string().optional().describe('Human-readable task label (defaults to template label)'), + category: z.string().optional().describe('Display category: pr | repo | thread | system | external'), + description: z.string().optional().describe('Short description of this task instance'), + idempotencyKey: z + .string() + .min(1) + .max(200) + .optional() + .describe('Stable replay key to include in the draft for trusted workflow audit parity'), }; export async function handlePreviewScheduledTask(input: { @@ -152,6 +161,10 @@ export async function handlePreviewScheduledTask(input: { trigger: string; params?: string; deliveryThreadId?: string; + label?: string; + category?: string; + description?: string; + idempotencyKey?: string; }): Promise { let trigger: unknown; try { @@ -169,12 +182,28 @@ export async function handlePreviewScheduledTask(input: { } } + // Preview must match registration semantics so the draft audits the same + // target that will be persisted by register_scheduled_task. + const currentCatId = process.env['CAT_CAFE_CAT_ID']; + if (!params.targetCatId && currentCatId) { + params.targetCatId = currentCatId; + } + const body: Record = { templateId: input.templateId, trigger, params, }; if (input.deliveryThreadId) body.deliveryThreadId = input.deliveryThreadId; + if (input.idempotencyKey) body.idempotencyKey = input.idempotencyKey; + + if (input.label || input.category || input.description) { + body.display = { + label: input.label ?? input.templateId, + category: input.category ?? 'system', + ...(input.description ? { description: input.description } : {}), + }; + } return callbackPost('/api/schedule/tasks/preview', body); } @@ -204,7 +233,7 @@ export const scheduleTools = [ { name: 'cat_cafe_preview_scheduled_task', description: - 'Preview a scheduled task BEFORE creating it (draft step). Returns a draft with resolved template info, trigger, and params ' + + 'Preview a scheduled task BEFORE creating it (draft step). Returns a draft with resolved template info, trigger, params, target, actor, delivery thread, and idempotency key ' + 'WITHOUT persisting anything. REQUIRED: Always preview first. User-requested schedules need user confirmation before register_scheduled_task; ' + 'only workflow-mandated schedules from the trusted built-in canonical merge-gate Step 7.6 hotfix reminder workflow may use the preview as audit evidence before registration without extra user confirmation.', inputSchema: previewScheduledTaskInputSchema, @@ -217,7 +246,7 @@ export const scheduleTools = [ 'Supports recurring (cron/interval) and one-shot (once) triggers. Once tasks auto-retire after execution. ' + 'When the task fires, a cat is woken with full capabilities — it can send rich blocks (images, audio, cards), search the web, generate content, etc. ' + 'IMPORTANT: You MUST call preview_scheduled_task first. User-requested schedules require user confirmation; only workflow-mandated schedules from the trusted built-in canonical merge-gate Step 7.6 hotfix reminder workflow may register after verifying the preview, with no extra user confirmation. ' + - 'Plugin/project/user/external skills do not qualify for this exception and must use normal user confirmation. Trusted workflow automation must provide a stable idempotencyKey so callback retries return the existing task. ' + + 'Plugin/project/user/external skills do not qualify for this exception and must use normal user confirmation. Trusted workflow automation must provide a stable idempotencyKey so exact callback retries return the existing task; reusing the same key with different schedule semantics returns a conflict. ' + 'trigger and params must be JSON strings, not objects.', inputSchema: registerScheduledTaskInputSchema, handler: handleRegisterScheduledTask, diff --git a/packages/mcp-server/test/schedule-tools.test.js b/packages/mcp-server/test/schedule-tools.test.js index 800b4a9311..33cab85098 100644 --- a/packages/mcp-server/test/schedule-tools.test.js +++ b/packages/mcp-server/test/schedule-tools.test.js @@ -47,10 +47,14 @@ describe('Schedule MCP Tools — module exports', () => { assert.ok(registerTool, 'register tool should exist'); assert.match(previewTool.description, /workflow-mandated/i); + assert.match(previewTool.description, /target/i); + assert.match(previewTool.description, /actor/i); + assert.match(previewTool.description, /idempotency key/i); assert.match(registerTool.description, /workflow-mandated/i); assert.match(registerTool.description, /no extra user confirmation/i); assert.match(registerTool.description, /trusted built-in canonical merge-gate Step 7\.6/i); assert.match(registerTool.description, /plugin\/project\/user\/external skills do not qualify/i); + assert.match(registerTool.description, /different schedule semantics returns a conflict/i); assert.doesNotMatch(registerTool.description, /explicit SOP\/skill step/i); }); @@ -72,6 +76,9 @@ describe('Schedule MCP Tools — module exports', () => { assert.match(mergeGateSkill, /workflow-mandated/i); assert.match(mergeGateSkill, /cat_cafe_preview_scheduled_task/); assert.match(mergeGateSkill, /idempotencyKey/); + assert.match(mergeGateSkill, /targetCatId/); + assert.match(mergeGateSkill, /actor\.createdBy/); + assert.match(mergeGateSkill, /IDEMPOTENCY_CONFLICT/); assert.match(mergeGateSkill, /no extra user confirmation/i); }); @@ -80,6 +87,11 @@ describe('Schedule MCP Tools — module exports', () => { assert.ok(registerScheduledTaskInputSchema.idempotencyKey, 'idempotencyKey schema required'); }); + test('cat_cafe_preview_scheduled_task accepts an idempotency key for workflow audit parity', async () => { + const { previewScheduledTaskInputSchema } = await import('../dist/tools/schedule-tools.js'); + assert.ok(previewScheduledTaskInputSchema.idempotencyKey, 'idempotencyKey schema required'); + }); + test('cat_cafe_register_scheduled_task forwards idempotencyKey to the API', async () => { const originalFetch = globalThis.fetch; const originalApiUrl = process.env.CAT_CAFE_API_URL; @@ -121,6 +133,60 @@ describe('Schedule MCP Tools — module exports', () => { } }); + test('cat_cafe_preview_scheduled_task forwards the same workflow fields as registration', async () => { + const originalFetch = globalThis.fetch; + const originalApiUrl = process.env.CAT_CAFE_API_URL; + const originalInvocationId = process.env.CAT_CAFE_INVOCATION_ID; + const originalCallbackToken = process.env.CAT_CAFE_CALLBACK_TOKEN; + const originalCatId = process.env.CAT_CAFE_CAT_ID; + let postedBody; + + process.env.CAT_CAFE_API_URL = 'http://127.0.0.1:3004'; + process.env.CAT_CAFE_INVOCATION_ID = 'inv-schedule-preview-test'; + process.env.CAT_CAFE_CALLBACK_TOKEN = 'tok-schedule-preview-test'; + process.env.CAT_CAFE_CAT_ID = 'codex'; + + globalThis.fetch = async (_url, options) => { + postedBody = JSON.parse(options.body); + return new Response(JSON.stringify({ draft: postedBody }), { + status: 200, + headers: { 'Content-Type': 'application/json' }, + }); + }; + + try { + const { handlePreviewScheduledTask } = await import('../dist/tools/schedule-tools.js'); + const result = await handlePreviewScheduledTask({ + templateId: 'reminder', + trigger: JSON.stringify({ type: 'once', delayMs: 1209600000 }), + params: JSON.stringify({ message: 'hotfix upgrade review' }), + label: 'Hotfix upgrade review', + category: 'pr', + description: 'Review hotfix after 14 days', + idempotencyKey: 'workflow:merge-gate:hotfix-upgrade-review:clowder-labs/clowder-ai#92', + }); + + assert.equal(result.isError, undefined); + assert.equal(postedBody.params.targetCatId, 'codex'); + assert.equal(postedBody.idempotencyKey, 'workflow:merge-gate:hotfix-upgrade-review:clowder-labs/clowder-ai#92'); + assert.deepEqual(postedBody.display, { + label: 'Hotfix upgrade review', + category: 'pr', + description: 'Review hotfix after 14 days', + }); + } finally { + globalThis.fetch = originalFetch; + if (originalApiUrl === undefined) delete process.env.CAT_CAFE_API_URL; + else process.env.CAT_CAFE_API_URL = originalApiUrl; + if (originalInvocationId === undefined) delete process.env.CAT_CAFE_INVOCATION_ID; + else process.env.CAT_CAFE_INVOCATION_ID = originalInvocationId; + if (originalCallbackToken === undefined) delete process.env.CAT_CAFE_CALLBACK_TOKEN; + else process.env.CAT_CAFE_CALLBACK_TOKEN = originalCallbackToken; + if (originalCatId === undefined) delete process.env.CAT_CAFE_CAT_ID; + else process.env.CAT_CAFE_CAT_ID = originalCatId; + } + }); + test('cat_cafe_remove_scheduled_task has taskId required', async () => { const { scheduleTools, removeScheduledTaskInputSchema } = await import('../dist/tools/schedule-tools.js'); const tool = scheduleTools.find((t) => t.name === 'cat_cafe_remove_scheduled_task'); diff --git a/review-notes/2026-07-23-hotfix-reminder-sop-review-request.md b/review-notes/2026-07-23-hotfix-reminder-sop-review-request.md index 4d98a8814a..937bc8fb22 100644 --- a/review-notes/2026-07-23-hotfix-reminder-sop-review-request.md +++ b/review-notes/2026-07-23-hotfix-reminder-sop-review-request.md @@ -9,10 +9,12 @@ Head: use PR #92 current head; the routing message supplies the exact SHA. - Updated schedule MCP descriptions to distinguish user-requested schedules from the one trusted workflow-mandated exception. - Updated `schedule-tasks` so ordinary user schedules still require preview and confirmation, while only trusted built-in canonical `merge-gate` Step 7.6 may register after preview verification. -- Added schedule registration idempotency for workflow replay safety: stable `idempotencyKey` values dedupe callback retries to the existing dynamic task. +- Added schedule registration idempotency for workflow replay safety: stable `idempotencyKey` values dedupe exact callback retries to the existing dynamic task, while same-key semantic mismatches return `409 IDEMPOTENCY_CONFLICT`. +- Bound each idempotency key to a canonical request fingerprint covering template, trigger input, params/target, display, delivery thread, and actor. +- Made preview drafts match final registration semantics: target canonicalization, actor, delivery thread, display, and idempotency key are visible in the draft before persistence. - Added a `tips_exempt` frontmatter note because this is an internal SOP/MCP workflow clarification, not a new end-user capability. -- Updated `merge-gate` Step 7.6 so hotfix 14-day upgrade reminders must preview, verify draft fields, then register with no extra user confirmation and a repo+PR-scoped stable `idempotencyKey`. -- Added MCP/API regression tests for tool descriptions, skill docs, idempotency-key passthrough, persistent dedupe, and replay behavior. +- Updated `merge-gate` Step 7.6 so hotfix 14-day upgrade reminders must preview, verify draft fields, then register with no extra user confirmation and a repo+PR-scoped stable `idempotencyKey`; conflicts now fail closed. +- Added MCP/API regression tests for tool descriptions, skill docs, preview parity, idempotency-key passthrough, persistent dedupe, same-key conflicts, and replay behavior. ## Why @@ -32,13 +34,13 @@ Daily patrol found a process contradiction: hotfix merge-gate requires registeri The exception is intentionally narrow. I did not remove confirmation from normal schedule registration; user-requested reminders still require preview and explicit confirmation. Plugin/project/user/external skills do not qualify; only the built-in canonical `merge-gate` Step 7.6 hotfix reminder workflow can register after preview verification. -The API change is additive: callers without `idempotencyKey` keep the existing create-new-task behavior. Callers with a stable key get replay-safe dedupe and receive the existing task id on retry. +The API change is additive: callers without `idempotencyKey` keep the existing create-new-task behavior. Callers with a stable key get replay-safe dedupe only for the same canonical request; conflicting reuse returns 409 and does not mutate the existing task. ## Architecture Ownership Architecture cell: workflow / MCP collab schedule surface + scheduler persistence Map delta: none -Why: This extends the existing schedule route and `DynamicTaskStore` with an idempotency column/index; it does not add a new Store, Queue, Router, Adapter, Dispatcher, Binding, or runtime ownership boundary. +Why: This extends the existing schedule route and `DynamicTaskStore` with idempotency key/fingerprint columns and an index; it does not add a new Store, Queue, Router, Adapter, Dispatcher, Binding, or runtime ownership boundary. Reviewer checks: @@ -46,6 +48,8 @@ Reviewer checks: - Verify the exception cannot be read as bypassing user confirmation for ordinary user-requested schedules. - Verify plugin/project/user/external skills are excluded from the no-extra-confirmation exception. - Verify schedule registration retry with the same `idempotencyKey` cannot create duplicate reminders. +- Verify same-key semantic mismatch returns `409 IDEMPOTENCY_CONFLICT` without modifying the existing task. +- Verify preview draft fields match final registration semantics, including target, actor, delivery thread, display, and idempotency key. - Verify `merge-gate` Step 7.6 is actionable and fail-closed. ## Open Questions @@ -53,7 +57,7 @@ Reviewer checks: ### 技术 OQ - Is the trusted built-in canonical `merge-gate` Step 7.6 boundary tight enough to prevent workflow-mandated from becoming a blanket bypass? -- Is global uniqueness on stable workflow `idempotencyKey` sufficient, given keys are explicitly namespaced by workflow + repo + PR? +- For cross-cat takeover of the same merge-gate reminder, this implementation chooses fail-closed `409` when actor/provenance differs. Please verify that is the right contract. ### 价值 OQ @@ -61,7 +65,7 @@ Reviewer checks: ## Next Action -Please review PR #92 against its current head. Focus on the trusted workflow boundary, merge-gate hotfix reminder flow, idempotent registration replay, and whether the regression tests protect the right surfaces. +Please review PR #92 against its current head. Focus on the trusted workflow boundary, merge-gate hotfix reminder flow, idempotent registration replay/conflict behavior, preview/register parity, and whether the regression tests protect the right surfaces. ## Review Sandbox @@ -82,37 +86,40 @@ PATH="/opt/homebrew/opt/node@24/bin:$PATH" pnpm install --frozen-lockfile - 巡检范围：hotfix merge-gate reminder tail, schedule MCP descriptions, `schedule-tasks`, and `merge-gate` Step 7.6. - 根因：schedule confirmation wording and merge-gate mandated registration were inconsistent. -- 修复：preview remains mandatory; confirmation remains mandatory for user-requested schedules; only built-in canonical `merge-gate` Step 7.6 hotfix reminder can register after verified preview with no extra confirmation; register retries are deduped by stable `idempotencyKey`. +- 修复：preview remains mandatory; confirmation remains mandatory for user-requested schedules; only built-in canonical `merge-gate` Step 7.6 hotfix reminder can register after verified preview with no extra confirmation; exact register retries are deduped by stable `idempotencyKey`, while same-key semantic mismatches return `409`. - Dogfood scope: exempt. This is an internal SOP/tool-description fix with no user-facing runtime path. - Artifact hygiene: root media/design artifact checks returned no matches. ### 测试结果 ```bash -PATH="/opt/homebrew/opt/node@24/bin:$PATH" pnpm --filter @cat-cafe/mcp-server test -- --test-name-pattern "workflow-mandated|cat_cafe_register_scheduled_task" -# passed: 385 tests, 0 failed +git diff --check +# passed -node --test packages/api/test/scheduler/dynamic-task-store.test.js --test-name-pattern "idempotency|columns" -# passed: 13 tests, 0 failed +PATH="/opt/homebrew/opt/node@24/bin:$PATH" pnpm check +# passed; existing advisory warnings only + +PATH="/opt/homebrew/opt/node@24/bin:$PATH" pnpm --filter @cat-cafe/api build +# passed -node --test packages/api/test/schedule-route.test.js --test-name-pattern "idempotent workflow registration" -# passed: 37 tests, 0 failed +PATH="/opt/homebrew/opt/node@24/bin:$PATH" pnpm --filter @cat-cafe/mcp-server build +# passed -PATH="/opt/homebrew/opt/node@24/bin:$PATH" pnpm check:skills:manifest -# passed: 48 skills validated; existing advisory warnings only +PATH="/opt/homebrew/opt/node@24/bin:$PATH" pnpm --filter @cat-cafe/mcp-server test -- --test-name-pattern "workflow-mandated|idempotency key|workflow fields" +# passed: 387 tests, 0 failed -PATH="/opt/homebrew/opt/node@24/bin:$PATH" pnpm check:skills:surfaces -# passed: 11 tests, no raw first-party Hub/API curl main paths +node --test packages/api/test/scheduler/dynamic-task-store.test.js --test-name-pattern "idempotency|columns" +# passed: 13 tests, 0 failed -PATH="/opt/homebrew/opt/node@24/bin:$PATH" pnpm check -# passed +node --test packages/api/test/schedule-route.test.js --test-name-pattern "idempotencyKey|workflow audit" +# passed: 40 tests, 0 failed -git diff --check -# passed +node --test packages/api/test/memory/schema-v17.test.js packages/api/test/memory/schema-v19-f200.test.js packages/api/test/memory/schema-v2.test.js packages/api/test/memory/schema-v26-recall-result-count.test.js packages/api/test/memory/world-scope-filter.test.js packages/api/test/pack-knowledge-scope.test.js --test-name-pattern "CURRENT_SCHEMA_VERSION|schema V6 migration|V26 migration" +# passed: 30 tests, 0 failed ``` ### Local Caveat -`pnpm check:skills` is blocked in this local worktree by missing provider skill mounts. `pnpm sync:skills --dry-run` would touch 34 worktrees, so I did not apply that environment repair in this PR. +Quality-gate helper caveats in this local checkout: `node scripts/check-fallback-layers.mjs` is unavailable because the script is not present, and `pnpm check:architecture-ownership` is not registered. I ran the equivalent manual diff scan for architecture-surface changes and the root artifact hygiene checks; no blocker found. -`pnpm --filter @cat-cafe/api test` did not produce a usable local full-suite green: the default Node 22 run violates the package `>=24` engine and failed in unrelated `capabilities-route` tests; the Node 24 rerun still mixed the same unrelated capability assertions with a local `better-sqlite3` native ABI mismatch. Focused schedule/API tests, `pnpm check`, and `@cat-cafe/api build` are green above; GitHub CI should be treated as the full-suite truth source for this PR. +Node 24 local API `node --test` is blocked by the local `better-sqlite3` native module being compiled for Node 22 (`NODE_MODULE_VERSION 127` vs Node 24 `137`). Focused API tests are green under default Node 22; TypeScript builds and repo `pnpm check` are green under Node 24. GitHub CI should be treated as the full-suite truth source for this PR. From 986510b4f9bd0df33836e9f367d016e342865c89 Mon Sep 17 00:00:00 2001 From: "MaineCoon-GPT-5.5" Date: Thu, 23 Jul 2026 01:33:18 +0800 Subject: [PATCH 5/6] fix: stabilize schedule preview replay guards MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Why: hotfix reminder registration relies on preview/register parity and idempotency replay; invalid preview params must fail closed before posting, and request fingerprints must not drift with host locale collation. Verification: MCP schedule focused 14/14 passed; API schedule focused 41/41 passed; dynamic task store focused 13/13 passed; schema/migration focused 69/69 passed; api+mcp lint passed; git diff --check passed. MCP full suite still has unrelated refresh-loop pending/cancelled failures. [砚砚/GPT-5.5🐾] --- packages/api/src/routes/schedule.ts | 8 +++- packages/api/test/schedule-route.test.js | 44 +++++++++++++++++++ .../mcp-server/src/tools/schedule-tools.ts | 40 ++++++++--------- .../mcp-server/test/schedule-tools.test.js | 44 +++++++++++++++++++ 4 files changed, 115 insertions(+), 21 deletions(-) diff --git a/packages/api/src/routes/schedule.ts b/packages/api/src/routes/schedule.ts index 3a79d83815..f05a760801 100644 --- a/packages/api/src/routes/schedule.ts +++ b/packages/api/src/routes/schedule.ts @@ -154,12 +154,18 @@ function dynamicTaskResponse(def: Pick) - .sort(([left], [right]) => left.localeCompare(right)) + // Idempotency fingerprints must be byte-stable across host locales. + .sort(([left], [right]) => compareUtf16(left, right)) .map(([key, entry]) => [key, canonicalizeJson(entry)]), ); } diff --git a/packages/api/test/schedule-route.test.js b/packages/api/test/schedule-route.test.js index 61dd832dfd..c9e07e1ee0 100644 --- a/packages/api/test/schedule-route.test.js +++ b/packages/api/test/schedule-route.test.js @@ -823,6 +823,50 @@ describe('Schedule Routes', () => { assert.equal(store.getAll().length, 1, 'conflict must not persist a second task'); assert.equal(store.getAll()[0].params.triggerUserId, 'user-1'); }); + + it('replays the same non-ASCII request when host locale ordering changes', async () => { + const originalLocaleCompare = String.prototype.localeCompare; + const payload = { + templateId: 'reminder', + trigger: { type: 'interval', ms: 60000 }, + params: { z: 'ascii', 'ä': 'non-ascii', message: 'locale-stable replay' }, + display: { + label: 'Locale stable replay', + category: 'system', + description: 'same request must not depend on runtime locale collation', + }, + deliveryThreadId: 'thread-locale', + idempotencyKey: 'workflow:merge-gate:hotfix-upgrade-review:clowder-labs/clowder-ai#92-locale', + }; + + const first = await appDyn.inject({ + method: 'POST', + url: '/api/schedule/tasks', + payload, + }); + assert.equal(first.statusCode, 200, first.body); + const firstBody = first.json(); + + String.prototype.localeCompare = function reversedLocaleCompare(other) { + if (String(this) === String(other)) return 0; + return originalLocaleCompare.call(this, other) < 0 ? 1 : -1; + }; + + try { + const replay = await appDyn.inject({ + method: 'POST', + url: '/api/schedule/tasks', + payload, + }); + + assert.equal(replay.statusCode, 200, replay.body); + assert.equal(replay.json().idempotent, true); + assert.equal(replay.json().task.id, firstBody.task.id); + assert.equal(store.getAll().length, 1, 'locale collation drift must not persist or conflict'); + } finally { + String.prototype.localeCompare = originalLocaleCompare; + } + }); }); describe('POST /api/schedule/tasks — callback auth infers deliveryThreadId', () => { diff --git a/packages/mcp-server/src/tools/schedule-tools.ts b/packages/mcp-server/src/tools/schedule-tools.ts index 192151c2d2..aa187b7f8e 100644 --- a/packages/mcp-server/src/tools/schedule-tools.ts +++ b/packages/mcp-server/src/tools/schedule-tools.ts @@ -76,6 +76,20 @@ export const registerScheduledTaskInputSchema = { .describe('Stable replay key for trusted workflow automation; retries with the same key return the existing task'), }; +function parseScheduleParams(raw: string | undefined): { ok: true; params: Record } | { ok: false; error: string } { + if (!raw) return { ok: true, params: {} }; + + try { + const parsed: unknown = JSON.parse(raw); + if (typeof parsed !== 'object' || parsed === null || Array.isArray(parsed)) { + return { ok: false, error: 'Invalid params JSON — must be a JSON object (not null, array, or primitive)' }; + } + return { ok: true, params: parsed as Record }; + } catch { + return { ok: false, error: 'Invalid params JSON — must be a valid JSON object' }; + } +} + export async function handleRegisterScheduledTask(input: { templateId: string; trigger: string; @@ -93,18 +107,9 @@ export async function handleRegisterScheduledTask(input: { return errorResult('Invalid trigger JSON — must be a valid JSON object'); } - let params: Record = {}; - if (input.params) { - try { - const parsed: unknown = JSON.parse(input.params); - if (typeof parsed !== 'object' || parsed === null || Array.isArray(parsed)) { - return errorResult('Invalid params JSON — must be a JSON object (not null, array, or primitive)'); - } - params = parsed as Record; - } catch { - return errorResult('Invalid params JSON — must be a valid JSON object'); - } - } + const parsedParams = parseScheduleParams(input.params); + if (!parsedParams.ok) return errorResult(parsedParams.error); + const params = parsedParams.params; // Auto-inject current cat's ID so reminder tasks wake the registering cat, not default opus const currentCatId = process.env['CAT_CAFE_CAT_ID']; @@ -173,14 +178,9 @@ export async function handlePreviewScheduledTask(input: { return errorResult('Invalid trigger JSON'); } - let params: Record = {}; - if (input.params) { - try { - params = JSON.parse(input.params); - } catch { - return errorResult('Invalid params JSON'); - } - } + const parsedParams = parseScheduleParams(input.params); + if (!parsedParams.ok) return errorResult(parsedParams.error); + const params = parsedParams.params; // Preview must match registration semantics so the draft audits the same // target that will be persisted by register_scheduled_task. diff --git a/packages/mcp-server/test/schedule-tools.test.js b/packages/mcp-server/test/schedule-tools.test.js index 33cab85098..5a2bbc62eb 100644 --- a/packages/mcp-server/test/schedule-tools.test.js +++ b/packages/mcp-server/test/schedule-tools.test.js @@ -187,6 +187,50 @@ describe('Schedule MCP Tools — module exports', () => { } }); + test('cat_cafe_preview_scheduled_task rejects non-object params without posting', async () => { + const originalFetch = globalThis.fetch; + const originalApiUrl = process.env.CAT_CAFE_API_URL; + const originalInvocationId = process.env.CAT_CAFE_INVOCATION_ID; + const originalCallbackToken = process.env.CAT_CAFE_CALLBACK_TOKEN; + const originalRetryDelays = process.env.CAT_CAFE_CALLBACK_RETRY_DELAYS_MS; + let fetchCalls = 0; + + process.env.CAT_CAFE_API_URL = 'http://127.0.0.1:3004'; + process.env.CAT_CAFE_INVOCATION_ID = 'inv-schedule-preview-invalid-params-test'; + process.env.CAT_CAFE_CALLBACK_TOKEN = 'tok-schedule-preview-invalid-params-test'; + process.env.CAT_CAFE_CALLBACK_RETRY_DELAYS_MS = '0'; + + globalThis.fetch = async () => { + fetchCalls += 1; + throw new Error('preview should reject invalid params before callbackPost'); + }; + + try { + const { handlePreviewScheduledTask } = await import('../dist/tools/schedule-tools.js'); + for (const params of ['null', '[]', '42']) { + const result = await handlePreviewScheduledTask({ + templateId: 'reminder', + trigger: JSON.stringify({ type: 'once', delayMs: 1209600000 }), + params, + }); + + assert.equal(result.isError, true); + assert.match(result.content[0].text, /Invalid params JSON.*JSON object/); + } + assert.equal(fetchCalls, 0, 'invalid params must fail locally without posting to the API'); + } finally { + globalThis.fetch = originalFetch; + if (originalApiUrl === undefined) delete process.env.CAT_CAFE_API_URL; + else process.env.CAT_CAFE_API_URL = originalApiUrl; + if (originalInvocationId === undefined) delete process.env.CAT_CAFE_INVOCATION_ID; + else process.env.CAT_CAFE_INVOCATION_ID = originalInvocationId; + if (originalCallbackToken === undefined) delete process.env.CAT_CAFE_CALLBACK_TOKEN; + else process.env.CAT_CAFE_CALLBACK_TOKEN = originalCallbackToken; + if (originalRetryDelays === undefined) delete process.env.CAT_CAFE_CALLBACK_RETRY_DELAYS_MS; + else process.env.CAT_CAFE_CALLBACK_RETRY_DELAYS_MS = originalRetryDelays; + } + }); + test('cat_cafe_remove_scheduled_task has taskId required', async () => { const { scheduleTools, removeScheduledTaskInputSchema } = await import('../dist/tools/schedule-tools.js'); const tool = scheduleTools.find((t) => t.name === 'cat_cafe_remove_scheduled_task'); From aa78822f5824ddd2c38d603dc8311f2d8caa5149 Mon Sep 17 00:00:00 2001 From: "MaineCoon-GPT-5.5" Date: Thu, 23 Jul 2026 02:09:29 +0800 Subject: [PATCH 6/6] style(schedule): apply biome formatting MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Why: the merge gate reached pnpm check and failed only on Biome formatting in the schedule route test and schedule MCP tool; this keeps the approved behavior unchanged while making the full gate reproducible. [砚砚/GPT-5.5🐾] --- packages/api/test/schedule-route.test.js | 2 +- packages/mcp-server/src/tools/schedule-tools.ts | 4 +++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/packages/api/test/schedule-route.test.js b/packages/api/test/schedule-route.test.js index c9e07e1ee0..cff5d6b4d4 100644 --- a/packages/api/test/schedule-route.test.js +++ b/packages/api/test/schedule-route.test.js @@ -829,7 +829,7 @@ describe('Schedule Routes', () => { const payload = { templateId: 'reminder', trigger: { type: 'interval', ms: 60000 }, - params: { z: 'ascii', 'ä': 'non-ascii', message: 'locale-stable replay' }, + params: { z: 'ascii', ä: 'non-ascii', message: 'locale-stable replay' }, display: { label: 'Locale stable replay', category: 'system', diff --git a/packages/mcp-server/src/tools/schedule-tools.ts b/packages/mcp-server/src/tools/schedule-tools.ts index aa187b7f8e..feb52b8184 100644 --- a/packages/mcp-server/src/tools/schedule-tools.ts +++ b/packages/mcp-server/src/tools/schedule-tools.ts @@ -76,7 +76,9 @@ export const registerScheduledTaskInputSchema = { .describe('Stable replay key for trusted workflow automation; retries with the same key return the existing task'), }; -function parseScheduleParams(raw: string | undefined): { ok: true; params: Record } | { ok: false; error: string } { +function parseScheduleParams( + raw: string | undefined, +): { ok: true; params: Record } | { ok: false; error: string } { if (!raw) return { ok: true, params: {} }; try {