From efe1052a497a28e099f36e59c7b1b9575d210bae Mon Sep 17 00:00:00 2001 From: Austin Date: Mon, 27 Jul 2026 01:24:34 +0700 Subject: [PATCH 1/4] =?UTF-8?q?refactor(schedule):=20=E5=AE=9A=E6=97=B6?= =?UTF-8?q?=E4=BB=BB=E5=8A=A1=E5=AD=98=E5=82=A8=E6=8C=89=20bot=20=E6=8B=86?= =?UTF-8?q?=E5=88=86=E5=88=B0=20BOT=5FHOME=EF=BC=8C=E4=BF=AE=E5=A4=8D?= =?UTF-8?q?=E6=B2=99=E7=9B=92=E5=86=85=E9=94=81=E6=96=87=E4=BB=B6=20EPERM?= =?UTF-8?q?=20=E5=B9=B6=E6=B6=88=E9=99=A4=E8=B7=A8=20bot=20=E4=BB=BB?= =?UTF-8?q?=E5=8A=A1=E6=B3=84=E6=BC=8F?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 存储从共享 data/schedules.json 迁至 /bots//schedules.json： - 沙盒策略零新增规则：own BOT_HOME 本就 readWrite、兄弟构造性 deny——RMW 兄弟锁（schedules.json.lock）随文件进入 rw 目录，两平台的沙盒内 schedule 增删改同时修复（旧单文件放行盖不住兄弟锁路径，EPERM） - 消除旧共享文件被迫接受的泄漏面：任务 prompt/routing 不再对全体沙盒 bot 可读 - fs-policy 删除共享文件特殊放行；worker 删除预建 store 增加 per-bot scope（daemon 绑自己、CLI 绑会话 bot/--lark-app-id）与 per-file 状态机；createTask 按 params.larkAppId 路由到属主文件；新增 listTasksForBots/findTaskAcrossBots（非沙盒聚合/跨库寻址）与 importTasks。 daemon 启动时把 legacy 共享文件按 task.larkAppId 幂等拆分（无归属→bot-0， 同 owner-filter 旧语义；并发 boot 由 legacy 文件锁串行化；原文件保留为 .bak-split-v1，降级手工改名即回）。 codex 两处 P1 已修：v3 reconciler 从冻结 input 取 larkAppId 寻址（非 daemon 运行无全局 scope）；CLI add 的 owner 回退链对齐 scope 解析（flag→marker→env）， 防止沙盒会话产出 ownerless 任务永不执行。 真机验证：sandbox-probe 新增锁文件断言全绿；真实 Seatbelt profile 内 schedule add/list/rm 端到端通过，落点 per-bot 文件。 --- scripts/sandbox-probe.mjs | 21 ++- src/adapters/cli/fs-policy.ts | 12 +- src/cli.ts | 123 +++++++++--- src/daemon.ts | 7 +- src/services/schedule-split-migration.ts | 89 +++++++++ src/services/schedule-store.ts | 178 +++++++++++++++--- src/worker.ts | 6 +- .../hostExecutors/botmux-schedule.ts | 11 +- test/schedule-split-migration.test.ts | 152 +++++++++++++++ test/schedule-store-idempotency.test.ts | 26 ++- test/schedule-store.test.ts | 49 +++-- test/scheduler-cli-scope.test.ts | 2 +- test/v3-host-schedule-runtime.test.ts | 5 +- 13 files changed, 583 insertions(+), 98 deletions(-) create mode 100644 src/services/schedule-split-migration.ts create mode 100644 test/schedule-split-migration.test.ts diff --git a/scripts/sandbox-probe.mjs b/scripts/sandbox-probe.mjs index 20f4b57a8..c7001e653 100755 --- a/scripts/sandbox-probe.mjs +++ b/scripts/sandbox-probe.mjs @@ -139,7 +139,26 @@ check('读 allow-list: .dashboard-port', 'ALLOWED', ['/bin/cat', join(botmuxHome check('读 config.json (voice 凭证, codex#1)', 'DENIED', ['/bin/cat', join(botmuxHome, 'config.json')]); check('读 .env (daemon 配置, codex#1)', 'DENIED', ['/bin/cat', join(botmuxHome, '.env')]); check('读 data/webhook-master.key (AES 主密钥, codex#1)', 'DENIED', ['/bin/cat', join(sessionDataDir, 'webhook-master.key')]); -check('读写 data/schedules.json (RMW 定时任务, owner 接受泄漏)', 'ALLOWED', ['/bin/cat', join(sessionDataDir, 'schedules.json')]); +// Schedules are per-bot now: own BOT_HOME store fully mutable (file + RMW +// sibling lock), sibling stores invisible. The lock-file write is the exact +// operation the old shared-file grant could NOT cover (schedules.json.lock is +// a SIBLING path of a single-file rule) — probe it explicitly. +const ownSchedules = join(botHome, 'schedules.json'); +check('写自己 BOT_HOME 的 schedules.json (per-bot 存储)', 'ALLOWED', + ['/bin/sh', '-c', `[ -f '${ownSchedules}' ] || printf '{}' > '${ownSchedules}'; /bin/cat '${ownSchedules}' > /dev/null`]); +check('建自己 schedules.json.lock (RMW 兄弟锁, 旧共享模型的盲区)', 'ALLOWED', + ['/bin/sh', '-c', `/usr/bin/touch '${ownSchedules}.lock' && /bin/rm -f '${ownSchedules}.lock'`]); +// Only probe a sibling store that really exists — a missing file also fails +// `cat`, and "absent" must never masquerade as "denied". +const siblingSchedules = (() => { + try { + return readdirSync(join(botmuxHome, 'bots')) + .filter((n) => n.startsWith('cli_') && n !== APP) + .map((n) => join(botmuxHome, 'bots', n, 'schedules.json')) + .find((p) => existsSync(p)); + } catch { return undefined; } +})(); +if (siblingSchedules) check('读兄弟 bot 的 schedules.json (跨-bot 任务 prompt)', 'DENIED', ['/bin/cat', siblingSchedules]); check('读 ~/.botmux/bots.json (敏感)', 'DENIED', ['/bin/cat', join(botmuxHome, 'bots.json')]); check('读 ~/.botmux/logs (跨-bot)', 'DENIED', ['/bin/ls', join(botmuxHome, 'logs')]); // End-to-end: actually run the botmux CLI inside the sandbox (loads cli.js + reads diff --git a/src/adapters/cli/fs-policy.ts b/src/adapters/cli/fs-policy.ts index 737fe1504..88ce24530 100644 --- a/src/adapters/cli/fs-policy.ts +++ b/src/adapters/cli/fs-policy.ts @@ -348,13 +348,11 @@ export function buildFsPolicy(ctx: FsPolicyContext): FsPolicy { // worker PRE-CREATES this file before spawn so it survives the existence // filter and bwrap can bind it (bwrap cannot bind a nonexistent source). if (ctx.sessionId) push([`${sd}/turn-sends/${ctx.sessionId}.jsonl`], 'readWrite', 'internal'); - // schedules.json: `botmux schedule` is a READ-MODIFY-WRITE store shared by all - // bots (one file). It must be readWrite — a read-deny makes a sandboxed - // `botmux schedule` load an empty map and overwrite, wiping EVERY bot's tasks. - // This DOES expose other bots' task prompts+routing; accepted by the owner - // (王旭 2026-07-16) as the cost of the schedule feature — same call the old - // read-isolation made (schedules.json deliberately never denied). - push([`${sd}/schedules.json`], 'readWrite', 'internal'); + // (schedules: stored PER BOT inside each BOT_HOME — the owner's dir is + // already readWrite above and siblings' stores are denied by construction, + // so the old shared data/schedules.json grant (and the cross-bot task-prompt + // exposure it had to accept) is gone. The RMW sibling lock lives in the same + // rw dir, so sandboxed `botmux schedule` mutations work on both platforms.) // macOS lark-cli key store carve-out. The baseline DENIES the whole // `~/Library/Application Support/lark-cli` dir (it holds EVERY bot's appsecret // ciphertext + the master key — the pre-refactor cross-bot leak). But this bot diff --git a/src/cli.ts b/src/cli.ts index dd17ecd3b..384c91b18 100644 --- a/src/cli.ts +++ b/src/cli.ts @@ -5281,8 +5281,37 @@ async function cmdSchedule(sub: string, rest: string[]): Promise { const scheduler = await import('./core/scheduler.js'); const scheduleStore = await import('./services/schedule-store.js'); + // Per-bot stores: bind this invocation to one bot's store — explicit + // --lark-app-id wins, else the surrounding session's bot (env/marker-derived; + // always present inside sandboxed sessions). A bare terminal without either + // binds to the primary bot (bots.json[0]) so mutations keep the legacy + // "ownerless task runs on bot-0" semantics; `list` without a bound bot + // aggregates every readable store instead. + const cliScopeAppId = argValue(rest, '--lark-app-id') + ?? detectCurrentSession()?.larkAppId + ?? process.env.BOTMUX_LARK_APP_ID; + // LAZY + sandbox-safe bots.json read: sandboxed sessions always carry a + // scope (env-injected appId) and must never touch bots.json — it is denied + // and `loadBotsJson()` process.exit(1)s on the read error. Only the bare + // unsandboxed terminal (aggregate list / cross-store id lookup) reads it, + // and even then failure degrades to "no other stores visible". + const allBotAppIds = (): string[] => { + try { + return parseBotConfigsJson(readFileSync(BOTS_JSON_FILE, 'utf-8'), BOTS_JSON_FILE) + .map((b: { larkAppId?: unknown }) => b?.larkAppId) + .filter((x: unknown): x is string => typeof x === 'string'); + } catch { return []; } // absent OR sandbox-denied → own scope only + }; + if (cliScopeAppId) scheduleStore.setScheduleScope(cliScopeAppId); + else { + const first = allBotAppIds()[0]; + if (first) scheduleStore.setScheduleScope(first); + } + if (!sub || sub === 'list' || sub === 'ls') { - const tasks = scheduleStore.listTasks(); + const tasks = cliScopeAppId + ? scheduleStore.listTasks() + : scheduleStore.listTasksForBots(allBotAppIds()); if (tasks.length === 0) { console.log('暂无定时任务。\n\n用法:\n botmux schedule add "每日17:50" "帮我看AI新闻"\n botmux schedule add "every 2h" "检查构建"\n botmux schedule add "0 9 * * *" "每天早安"'); return; @@ -5325,7 +5354,12 @@ async function cmdSchedule(sub: string, rest: string[]): Promise { const explicitRootMessageId = argValue(rest, '--root-msg-id'); const rootMessageId = explicitRootMessageId ?? (chatId && chatId === cur?.chatId ? cur.rootMessageId : undefined); - const larkAppId = argValue(rest, '--lark-app-id') ?? cur?.larkAppId; + // Owner resolution mirrors the store-scope resolution above (flag → + // session marker → daemon-injected env): a sandboxed session without a + // readable marker must still stamp its own bot as owner, or the task + // would land in this bot's store as OWNERLESS — which only the primary + // daemon executes — and never fire (codex review P1). + const larkAppId = cliScopeAppId; const workingDir = argValue(rest, '--workdir') ?? cur?.workingDir ?? process.cwd(); const name = argValue(rest, '--name') ?? (promptArg.length > 20 ? promptArg.slice(0, 20) + '…' : promptArg); const legacyDeliver = argValue(rest, '--deliver') as 'origin' | 'local' | 'new-topic' | undefined; @@ -5374,25 +5408,36 @@ async function cmdSchedule(sub: string, rest: string[]): Promise { process.exit(1); } - const task = scheduler.addTask({ - name, - schedule: rawSchedule, - parsed, - prompt: promptArg, - workingDir, - chatId, - rootMessageId, - larkAppId, - creatorChatId: cur?.chatId, - creatorRootMessageId: cur?.rootMessageId, - creatorLarkAppId: cur?.larkAppId, - chatType: cur?.chatType === 'p2p' ? 'p2p' : 'topic_group', - scope, - executionPosition, - topicTitle, - deliver, - silent, - }); + let task; + try { + task = scheduler.addTask({ + name, + schedule: rawSchedule, + parsed, + prompt: promptArg, + workingDir, + chatId, + rootMessageId, + larkAppId, + creatorChatId: cur?.chatId, + creatorRootMessageId: cur?.rootMessageId, + creatorLarkAppId: cur?.larkAppId, + chatType: cur?.chatType === 'p2p' ? 'p2p' : 'topic_group', + scope, + executionPosition, + topicTitle, + deliver, + silent, + }); + } catch (err) { + // Sandboxed sessions can only write their OWN bot's store — a cross-bot + // `--lark-app-id` (or a scope pointing at another bot) fails closed here. + if (/EPERM|EACCES|not permitted/i.test(String(err))) { + console.error(`无法写入目标 bot 的定时任务存储（${larkAppId ?? '未指定'}）：沙盒会话只能管理自己 bot 的任务。`); + process.exit(1); + } + throw err; + } const next = task.nextRunAt ? new Date(task.nextRunAt).toLocaleString('zh-CN', { timeZone: scheduleTimeZone() }) : '—'; console.log(`✅ 已创建定时任务 [${task.id}] ${task.name}`); @@ -5411,29 +5456,51 @@ async function cmdSchedule(sub: string, rest: string[]): Promise { process.exit(1); } + // Id-addressed op missed the bound store: locate the id across every + // READABLE bot store (bare-terminal admin usage) and rebind the scope to the + // owning bot for this one-shot process. Sandboxed callers cannot read + // sibling stores, so they stay confined to their own tasks by construction. + const retargetIfElsewhere = (): boolean => { + const hit = scheduleStore.findTaskAcrossBots(id, allBotAppIds()); + if (!hit || hit.appId === scheduleStore.getScheduleScope()) return false; + scheduleStore.setScheduleScope(hit.appId); + console.log(`（任务属于 bot ${hit.appId} 的存储）`); + return true; + }; + switch (sub) { case 'remove': case 'rm': case 'delete': - case 'del': - if (scheduler.removeTask(id)) console.log(`已删除任务 ${id}`); + case 'del': { + let ok = scheduler.removeTask(id); + if (!ok && retargetIfElsewhere()) ok = scheduler.removeTask(id); + if (ok) console.log(`已删除任务 ${id}`); else { console.error(`未找到任务 ${id}`); process.exit(1); } break; + } case 'pause': - case 'disable': - if (scheduler.disableTask(id)) console.log(`已暂停任务 ${id}`); + case 'disable': { + let ok = scheduler.disableTask(id); + if (!ok && retargetIfElsewhere()) ok = scheduler.disableTask(id); + if (ok) console.log(`已暂停任务 ${id}`); else { console.error(`未找到任务 ${id}`); process.exit(1); } break; + } case 'resume': - case 'enable': - if (scheduler.enableTask(id)) console.log(`已恢复任务 ${id}`); + case 'enable': { + let ok = scheduler.enableTask(id); + if (!ok && retargetIfElsewhere()) ok = scheduler.enableTask(id); + if (ok) console.log(`已恢复任务 ${id}`); else { console.error(`未找到任务 ${id}`); process.exit(1); } break; + } case 'run': // Running requires the daemon (executeCallback is daemon-side). // CLI can only mark a task to run ASAP; daemon's next tick picks it up. { - const task = scheduleStore.getTask(id); + let task = scheduleStore.getTask(id); + if (!task && retargetIfElsewhere()) task = scheduleStore.getTask(id); if (!task) { console.error(`未找到任务 ${id}`); process.exit(1); } scheduleStore.updateTask(id, { nextRunAt: new Date().toISOString() }); console.log(`已标记任务 ${id} 下次 tick 立即执行（< 30s）`); diff --git a/src/daemon.ts b/src/daemon.ts index 65825701d..a410625da 100644 --- a/src/daemon.ts +++ b/src/daemon.ts @@ -60,6 +60,7 @@ import * as sessionStore from './services/session-store.js'; import * as chatFirstSeenStore from './services/chat-first-seen-store.js'; import { ensureDefaultOncallBound } from './services/oncall-store.js'; import * as scheduleStore from './services/schedule-store.js'; +import { migrateSharedSchedulesAtStartup } from './services/schedule-split-migration.js'; import * as messageQueue from './services/message-queue.js'; import { emitHookEvent, emitHookEventLocal, HOOK_EVENTS, type HookEvent } from './services/hook-runner.js'; import { setSessionLifecycleShutdown } from './services/session-lifecycle-hooks.js'; @@ -17177,8 +17178,12 @@ export async function startDaemon(botIndex?: number): Promise { } }, VC_MEETING_DELIVERY_LEASE_SCAN_MS); vcMeetingDeliveryLeaseTimer.unref?.(); - // Watch schedules.json for external writes (e.g. `botmux schedule add` + // Bind the schedule store to this daemon's bot (per-bot stores live in each + // BOT_HOME), split a legacy shared data/schedules.json if one still exists, + // then watch our own store for external writes (e.g. `botmux schedule add` // running in a separate node process) so dashboard event bus stays in sync. + scheduleStore.setScheduleScope(cfg.larkAppId); + migrateSharedSchedulesAtStartup(botConfigs.map(b => b.larkAppId), botConfigs[0]?.larkAppId ?? cfg.larkAppId); scheduleStore.startExternalWriteWatcher(); logger.info(`Bot ${idx}/${botConfigs.length}: ${cfg.larkAppId} (cli: ${cfg.cliId})`) setAskCardDispatcher(createLarkAskCardDispatcher()); diff --git a/src/services/schedule-split-migration.ts b/src/services/schedule-split-migration.ts new file mode 100644 index 000000000..d45d503e9 --- /dev/null +++ b/src/services/schedule-split-migration.ts @@ -0,0 +1,89 @@ +/** + * One-time split of the legacy shared `data/schedules.json` into per-bot + * stores (`/bots//schedules.json`). + * + * Why: the shared file needed a sandbox policy special-case (single-file + * readWrite grant) that could not cover the RMW sibling lock + * (`schedules.json.lock`) — a sandboxed `botmux schedule add` failed EPERM — + * and it exposed every bot's task prompts/routing to every other sandboxed + * bot. Per-bot stores live inside each bot's BOT_HOME, which the sandbox + * already grants readWrite to the owner and denies to siblings by + * construction. + * + * Runs at daemon startup, BEFORE the scheduler and the external-write watcher + * touch the store. Multiple per-bot daemons boot concurrently against the same + * legacy file: the whole split runs under the legacy file's cross-process lock + * and re-checks existence inside it, so exactly one daemon performs the split + * and the rest see the file already gone (renamed to `schedules.json.bak-split-v1`). + * + * Routing: each task goes to its `larkAppId` owner's store; tasks with no + * `larkAppId` (or an appId not in bots.json) go to the PRIMARY bot (index 0) — + * the same fallback the scheduler's owner filter has always used for legacy + * ownerless tasks. Id conflicts with an existing per-bot entry keep the + * existing entry (the per-bot store is newer by definition) and are logged. + * + * Downgrade: the pre-split file survives verbatim as `*.bak-split-v1`; an + * older build can be restored by renaming it back (documented in the PR). + * Never throws — a failed/partial migration must not brick daemon startup; + * the legacy file stays in place and the next boot retries. + */ +import { existsSync, readFileSync, renameSync } from 'node:fs'; +import { join } from 'node:path'; +import { config } from '../config.js'; +import { logger } from '../utils/logger.js'; +import { withFileLockSync } from '../utils/file-lock.js'; +import * as scheduleStore from './schedule-store.js'; +import type { ScheduledTask } from '../types.js'; + +function legacyFilePath(): string { + return join(config.session.dataDir, 'schedules.json'); +} + +export function migrateSharedSchedulesAtStartup( + knownAppIds: readonly string[], + primaryAppId: string, +): void { + const legacyFp = legacyFilePath(); + if (!existsSync(legacyFp)) return; // already split (or fresh install) — no-op + try { + withFileLockSync(legacyFp, () => { + // Another daemon may have completed the split while we waited on the lock. + if (!existsSync(legacyFp)) return; + + let raw: unknown; + try { + raw = JSON.parse(readFileSync(legacyFp, 'utf-8')); + } catch (err) { + // Malformed legacy file: leave it for a human — do NOT rename (that + // would silently discard whatever tasks it held) and do not brick boot. + logger.error(`[schedule-split] legacy schedules.json unreadable, split skipped: ${err}`); + return; + } + if (!raw || typeof raw !== 'object' || Array.isArray(raw)) { + logger.error('[schedule-split] legacy schedules.json root is not an object, split skipped'); + return; + } + + const known = new Set(knownAppIds); + const byBot = new Map>(); + for (const [id, task] of Object.entries(raw as Record)) { + if (!task || typeof task !== 'object') continue; + const owner = task.larkAppId && known.has(task.larkAppId) ? task.larkAppId : primaryAppId; + let bucket = byBot.get(owner); + if (!bucket) { bucket = []; byBot.set(owner, bucket); } + bucket.push([id, task]); + } + + for (const [appId, entries] of byBot) { + scheduleStore.importTasks(appId, entries); + } + + const bak = `${legacyFp}.bak-split-v1`; + renameSync(legacyFp, bak); + const counts = [...byBot.entries()].map(([a, e]) => `${a}:${e.length}`).join(', '); + logger.info(`[schedule-split] split legacy schedules.json into per-bot stores (${counts || 'empty'}); backup: ${bak}`); + }); + } catch (err) { + logger.warn(`[schedule-split] skipped (${err instanceof Error ? err.message : String(err)})`); + } +} diff --git a/src/services/schedule-store.ts b/src/services/schedule-store.ts index 8900a6365..d0ab1a8fa 100644 --- a/src/services/schedule-store.ts +++ b/src/services/schedule-store.ts @@ -20,6 +20,7 @@ import { dashboardEventBus } from '../core/dashboard-events.js'; import { computeInputHash } from '../utils/canonical-input-hash.js'; import { withFileLockSync } from '../utils/file-lock.js'; import { fsyncDirectorySyncPortable } from '../utils/fs-durability.js'; +import { botHomePath } from '../adapters/cli/read-isolation.js'; import type { ScheduledTask, ParsedSchedule, ScheduleExecutionPosition } from '../types.js'; // ─── Idempotency types (events doc v0.1.2 §2.2) ───────────────────────────── @@ -123,12 +124,66 @@ export function canonicalScheduleInput(t: { }; } -let tasks: Map = new Map(); -let loaded = false; -let cachedFileVersion = 'missing'; +// ─── Per-bot store scope ───────────────────────────────────────────────────── +// +// Schedules are stored PER BOT inside each bot's BOT_HOME +// (`/bots//schedules.json`) instead of one shared +// `data/schedules.json`. Why: the bot's own BOT_HOME is already readWrite +// inside the file sandbox while sibling BOT_HOMEs are denied by construction — +// so a sandboxed `botmux schedule add` can take the RMW sibling lock +// (`schedules.json.lock`) on BOTH platforms without any policy special-case, +// and one bot's task prompts/routing are no longer readable by every other +// sandboxed bot (the leak the old shared-file grant had to accept). +// +// Callers bind the store to one bot before use: the daemon binds its own bot +// at startup, the CLI binds the session's bot (or an explicit --lark-app-id). +// Cross-bot reads/writes stay possible for UNsandboxed callers via the +// explicit-appId variants below; inside a sandbox they fail closed (EPERM). + +interface FileState { + tasks: Map; + loaded: boolean; + version: string; +} + +const fileStates = new Map(); +let scopeAppId: string | null = null; + +/** Bind the store's default file to one bot. Daemon: own bot at startup. + * CLI: the session's bot / explicit --lark-app-id before any store call. */ +export function setScheduleScope(appId: string): void { + scopeAppId = appId; +} + +export function getScheduleScope(): string | null { + return scopeAppId; +} + +function requireScope(): string { + if (!scopeAppId) { + throw new Error( + '[schedule-store] no bot scope bound — call setScheduleScope() before using the schedule store', + ); + } + return scopeAppId; +} -function getFilePath(): string { - return join(config.session.dataDir, 'schedules.json'); +/** The per-bot schedules file: `/bots//schedules.json`. */ +export function scheduleFilePathFor(appId: string): string { + return join(botHomePath(dirname(config.session.dataDir), appId), 'schedules.json'); +} + +function getFilePath(appId?: string): string { + return scheduleFilePathFor(appId ?? requireScope()); +} + +function stateFor(fp: string): FileState { + let s = fileStates.get(fp); + if (!s) { + s = { tasks: new Map(), loaded: false, version: 'missing' }; + fileStates.set(fp, s); + } + return s; } function getOutputDir(): string { @@ -304,9 +359,10 @@ function persistDiskSnapshot(fp: string, map: ReadonlyMap } function installSnapshot(map: Map, fp: string): void { - tasks = map; - cachedFileVersion = fileVersion(fp); - loaded = true; + const s = stateFor(fp); + s.tasks = map; + s.version = fileVersion(fp); + s.loaded = true; } interface MutationResult { @@ -322,8 +378,9 @@ interface MutationResult { */ function mutateTasks( mutate: (working: Map) => MutationResult, + appId?: string, ): T { - const fp = getFilePath(); + const fp = getFilePath(appId); ensureDir(dirname(fp)); return withFileLockSync(fp, () => { const working = readDiskSnapshot(fp, true).map; @@ -336,14 +393,15 @@ function mutateTasks( }); } -function load(): void { - ensureDir(dirname(getFilePath())); - const fp = getFilePath(); +function load(appId?: string): void { + const fp = getFilePath(appId); + ensureDir(dirname(fp)); + const state = stateFor(fp); const currentVersion = fileVersion(fp); // Reload if the file has been atomically replaced externally (e.g. by // `botmux schedule add`) or on first load. - if (loaded && currentVersion === cachedFileVersion) return; + if (state.loaded && currentVersion === state.version) return; const snapshot = readDiskSnapshot(fp, false); let nextMap = snapshot.map; @@ -368,7 +426,7 @@ function load(): void { } } - if (!loaded) { + if (!state.loaded) { logger.info( `Loaded ${nextMap.size} scheduled tasks from ${fp}` + `${snapshot.migratedCount ? ` (migrated ${snapshot.migratedCount} legacy)` : ''}`, @@ -417,6 +475,10 @@ export function createTask(params: { deliver?: 'origin' | 'local' | 'new-topic'; silent?: boolean; }): ScheduledTask { + // Route to the OWNING bot's file: a task explicitly created for another bot + // (`--lark-app-id` / dashboard admin flows) must land in that bot's store so + // its daemon (the only one that executes it) can see it. Sandboxed callers + // can only reach their own BOT_HOME — a cross-bot write fails closed (EPERM). return mutateTasks(working => { if (params.id) { const existing = working.get(params.id); @@ -472,19 +534,19 @@ export function createTask(params: { }; working.set(task.id, task); return { result: task, changed: true }; - }); + }, params.larkAppId); } -export function getTask(id: string): ScheduledTask | undefined { - load(); - return tasks.get(id); +export function getTask(id: string, appId?: string): ScheduledTask | undefined { + load(appId); + return stateFor(getFilePath(appId)).tasks.get(id); } -export function removeTask(id: string): boolean { +export function removeTask(id: string, appId?: string): boolean { const existed = mutateTasks(working => { const removed = working.delete(id); return { result: removed, changed: removed }; - }); + }, appId); if (existed) logger.info(`[schedule-store] Removed task ${id}`); return existed; } @@ -494,6 +556,7 @@ export function updateTask( updates: Partial>, + appId?: string, ): void { mutateTasks(working => { const task = working.get(id); @@ -503,7 +566,7 @@ export function updateTask( updates.deliver === 'new-topic' ? { ...updates, deliver: 'origin' as const } : updates, ); return { result: undefined, changed: true }; - }); + }, appId); } /** @@ -543,9 +606,60 @@ export function markRun(id: string, success: boolean, error?: string, deliveryEr } } -export function listTasks(): ScheduledTask[] { - load(); - return [...tasks.values()]; +export function listTasks(appId?: string): ScheduledTask[] { + load(appId); + return [...stateFor(getFilePath(appId)).tasks.values()]; +} + +/** Aggregate view across several bots' stores (unsandboxed admin CLI). Bots + * whose store cannot be read (sandbox deny / missing BOT_HOME) are skipped — + * callers inside a sandbox naturally collapse to their own bot. */ +export function listTasksForBots(appIds: readonly string[]): Array { + const out: Array = []; + for (const appId of appIds) { + try { + for (const t of listTasks(appId)) out.push({ ...t, _storeAppId: appId }); + } catch { /* unreadable (sandboxed sibling / bad appId) → skip */ } + } + return out; +} + +/** Bulk-insert raw task entries into one bot's store (startup split + * migration). Runs the same in-file legacy normalization as a disk read; + * an id already present in the destination wins (the per-bot store is newer + * by definition) and the collision is logged. */ +export function importTasks(appId: string, entries: ReadonlyArray<[string, unknown]>): void { + if (entries.length === 0) return; + mutateTasks(working => { + let changed = false; + for (const [id, raw] of entries) { + const task = migrate(raw); + if (!task) continue; + if (working.has(id)) { + logger.warn(`[schedule-store] import: id ${id} already exists in ${appId}'s store — keeping existing entry`); + continue; + } + working.set(id, task); + changed = true; + } + return { result: undefined, changed }; + }, appId); +} + +/** Locate a task id across several bots' stores. First hit wins (ids are + * UUID-derived; a cross-store collision is negligible and would only make an + * id-addressed command pick the first store). */ +export function findTaskAcrossBots( + id: string, + appIds: readonly string[], +): { task: ScheduledTask; appId: string } | undefined { + for (const appId of appIds) { + try { + const task = getTask(id, appId); + if (task) return { task, appId }; + } catch { /* unreadable → skip */ } + } + return undefined; } /** Ensure per-task output dir exists and return path to today's run log. */ @@ -574,10 +688,11 @@ export function startExternalWriteWatcher(): void { if (watcherStarted) return; watcherStarted = true; - // Make sure the data dir + file exist before we try to watch — fs.watch on - // a non-existent path throws ENOENT. - ensureDir(dirname(getFilePath())); + // Watch this daemon's OWN bot store (the only one it executes/serves). Make + // sure the BOT_HOME + file exist before we try to watch — fs.watch on a + // non-existent path throws ENOENT. const fp = getFilePath(); + ensureDir(dirname(fp)); if (!existsSync(fp)) { try { mutateTasks(working => ({ result: undefined, changed: !existsSync(fp) && working.size === 0 })); @@ -585,6 +700,7 @@ export function startExternalWriteWatcher(): void { } // Prime the cached file identity so the first watcher fire is comparable. load(); + const state = stateFor(fp); try { // Watch the directory, not the file inode: every commit atomically replaces @@ -594,16 +710,16 @@ export function startExternalWriteWatcher(): void { try { if (filename && filename.toString() !== basename(fp)) return; if (!existsSync(fp)) return; - if (fileVersion(fp) === cachedFileVersion) return; + if (fileVersion(fp) === state.version) return; // Snapshot in-memory state, then let load() refresh from disk. // load() compares file identity internally and updates the cache. const before = new Map(); - for (const [k, v] of tasks) before.set(k, v); + for (const [k, v] of state.tasks) before.set(k, v); load(); // Diff and publish. - for (const [id, t] of tasks) { + for (const [id, t] of state.tasks) { const prev = before.get(id); if (!prev) { dashboardEventBus.publish({ type: 'schedule.created', body: { schedule: t } }); @@ -612,7 +728,7 @@ export function startExternalWriteWatcher(): void { } } for (const id of before.keys()) { - if (!tasks.has(id)) { + if (!state.tasks.has(id)) { dashboardEventBus.publish({ type: 'schedule.deleted', body: { id } }); } } diff --git a/src/worker.ts b/src/worker.ts index a5c41548a..214edffbc 100644 --- a/src/worker.ts +++ b/src/worker.ts @@ -7212,10 +7212,8 @@ async function spawnCli( if (!existsSync(tsFile)) writeFileSync(tsFile, ''); } catch { /* */ } try { mkdirSync(join(dataDir, 'attachments', cfg.larkAppId), { recursive: true }); } catch { /* */ } - // schedules.json is a shared RMW store (`botmux schedule`); pre-create as an - // empty map if absent (same content schedule-store itself writes) so a fresh - // install's first in-sandbox `schedule add` can bind+write it on bwrap too. - try { const sf = join(dataDir, 'schedules.json'); if (!existsSync(sf)) writeFileSync(sf, '{}'); } catch { /* */ } + // (Schedules moved into each bot's BOT_HOME — the whole dir is already + // bound readWrite for the owner, so no per-file pre-create is needed.) const mandatoryDenyPaths: string[] = []; const mandatoryDenyRegexes: string[] = []; diff --git a/src/workflows/hostExecutors/botmux-schedule.ts b/src/workflows/hostExecutors/botmux-schedule.ts index 9b335a09f..6c414d2a6 100644 --- a/src/workflows/hostExecutors/botmux-schedule.ts +++ b/src/workflows/hostExecutors/botmux-schedule.ts @@ -203,7 +203,16 @@ export const botmuxScheduleReconciler: ProviderReconciler = { }, async readOnlyLookup(idempotencyKey, input) { - const task = getTask(idempotencyKey); + // Per-bot stores: address the OWNING bot's file from the frozen input's + // larkAppId (same routing createTask used) — a zero-arg getTask would + // depend on a process-global scope that non-daemon v3 runs (cli-run / + // goal-cli resume & reconciliation) never bind. A malformed input falls + // through to the existing validation path below instead of failing here. + let inputAppId: string | undefined; + if (input !== undefined) { + try { inputAppId = parseScheduleInput(input).larkAppId; } catch { /* validated below */ } + } + const task = getTask(idempotencyKey, inputAppId); if (!task) { return { found: false, diff --git a/test/schedule-split-migration.test.ts b/test/schedule-split-migration.test.ts new file mode 100644 index 000000000..3882b2251 --- /dev/null +++ b/test/schedule-split-migration.test.ts @@ -0,0 +1,152 @@ +/** + * Startup split of the legacy shared data/schedules.json into per-bot stores + * (services/schedule-split-migration.ts). Real fs in a temp botmux-home tree, + * mocked config/logger — same scaffolding as schedule-store.test.ts. + */ +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import { existsSync, mkdirSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from 'node:fs'; +import { dirname, join } from 'node:path'; +import { tmpdir } from 'node:os'; + +let tempDir: string; // botmux home root; dataDir = /data + +vi.mock('../src/config.js', () => ({ + config: { + session: { + get dataDir() { + return join(tempDir, 'data'); + }, + }, + }, +})); + +vi.mock('../src/utils/logger.js', () => ({ + logger: { info: vi.fn(), warn: vi.fn(), error: vi.fn(), debug: vi.fn() }, +})); + +const PRIMARY = 'cli_primary000000001'; +const OTHER = 'cli_other0000000002'; + +function legacyFp(): string { + return join(tempDir, 'data', 'schedules.json'); +} +function storeFp(appId: string): string { + return join(tempDir, 'bots', appId, 'schedules.json'); +} +function readStore(appId: string): Record { + return JSON.parse(readFileSync(storeFp(appId), 'utf-8')); +} + +function legacyTask(id: string, larkAppId?: string, extra: Record = {}) { + return { + id, + name: `task ${id}`, + schedule: '0 9 * * *', + parsed: { kind: 'cron', expr: '0 9 * * *', display: '0 9 * * *' }, + prompt: `prompt ${id}`, + workingDir: '/w', + chatId: 'oc_x', + enabled: true, + createdAt: '2026-01-01T00:00:00.000Z', + ...(larkAppId ? { larkAppId } : {}), + ...extra, + }; +} + +async function freshImport() { + vi.resetModules(); + const store = await import('../src/services/schedule-store.js'); + const migration = await import('../src/services/schedule-split-migration.js'); + return { store, migration }; +} + +beforeEach(() => { + tempDir = mkdtempSync(join(tmpdir(), 'schedule-split-')); + mkdirSync(join(tempDir, 'data'), { recursive: true }); +}); +afterEach(() => { + rmSync(tempDir, { recursive: true, force: true }); +}); + +describe('migrateSharedSchedulesAtStartup', () => { + it('splits tasks into each owner bot store; ownerless and unknown owners go to primary', async () => { + writeFileSync(legacyFp(), JSON.stringify({ + a: legacyTask('a', PRIMARY), + b: legacyTask('b', OTHER), + c: legacyTask('c'), // ownerless → primary + d: legacyTask('d', 'cli_unknown0000009'), // not in bots.json → primary + })); + + const { migration } = await freshImport(); + migration.migrateSharedSchedulesAtStartup([PRIMARY, OTHER], PRIMARY); + + expect(Object.keys(readStore(PRIMARY)).sort()).toEqual(['a', 'c', 'd']); + expect(Object.keys(readStore(OTHER))).toEqual(['b']); + // Legacy file renamed to backup, verbatim. + expect(existsSync(legacyFp())).toBe(false); + const bak = JSON.parse(readFileSync(`${legacyFp()}.bak-split-v1`, 'utf-8')); + expect(Object.keys(bak).sort()).toEqual(['a', 'b', 'c', 'd']); + }); + + it('is idempotent — second run is a no-op', async () => { + writeFileSync(legacyFp(), JSON.stringify({ a: legacyTask('a', PRIMARY) })); + const { migration } = await freshImport(); + migration.migrateSharedSchedulesAtStartup([PRIMARY], PRIMARY); + const first = readFileSync(storeFp(PRIMARY), 'utf-8'); + migration.migrateSharedSchedulesAtStartup([PRIMARY], PRIMARY); + expect(readFileSync(storeFp(PRIMARY), 'utf-8')).toBe(first); + expect(existsSync(legacyFp())).toBe(false); + }); + + it('keeps an existing per-bot entry on id conflict (per-bot store is newer)', async () => { + writeFileSync(legacyFp(), JSON.stringify({ a: legacyTask('a', PRIMARY, { prompt: 'stale legacy' }) })); + mkdirSync(dirname(storeFp(PRIMARY)), { recursive: true }); + writeFileSync(storeFp(PRIMARY), JSON.stringify({ a: legacyTask('a', PRIMARY, { prompt: 'newer per-bot' }) })); + + const { migration } = await freshImport(); + migration.migrateSharedSchedulesAtStartup([PRIMARY], PRIMARY); + + expect(readStore(PRIMARY).a.prompt).toBe('newer per-bot'); + expect(existsSync(`${legacyFp()}.bak-split-v1`)).toBe(true); + }); + + it('leaves a malformed legacy file in place (no rename, no brick)', async () => { + writeFileSync(legacyFp(), '<<>>'); + const { migration } = await freshImport(); + migration.migrateSharedSchedulesAtStartup([PRIMARY], PRIMARY); + expect(existsSync(legacyFp())).toBe(true); + expect(existsSync(`${legacyFp()}.bak-split-v1`)).toBe(false); + }); + + it('no-ops when there is no legacy file', async () => { + const { migration } = await freshImport(); + migration.migrateSharedSchedulesAtStartup([PRIMARY], PRIMARY); + expect(existsSync(storeFp(PRIMARY))).toBe(false); + }); + + it('normalizes pre-parsed legacy rows through the store migration on import', async () => { + writeFileSync(legacyFp(), JSON.stringify({ + old: { id: 'old', name: 'legacy', type: 'cron', schedule: '0 8 * * *', prompt: 'p', workingDir: '/w', chatId: 'oc', larkAppId: PRIMARY }, + })); + const { migration, store } = await freshImport(); + migration.migrateSharedSchedulesAtStartup([PRIMARY], PRIMARY); + store.setScheduleScope(PRIMARY); + const t = store.getTask('old'); + expect(t?.parsed).toMatchObject({ kind: 'cron', expr: '0 8 * * *' }); + }); + + it('per-bot stores stay independent after split (mutating one leaves the other untouched)', async () => { + writeFileSync(legacyFp(), JSON.stringify({ + a: legacyTask('a', PRIMARY), + b: legacyTask('b', OTHER), + })); + const { migration, store } = await freshImport(); + migration.migrateSharedSchedulesAtStartup([PRIMARY, OTHER], PRIMARY); + + store.setScheduleScope(PRIMARY); + expect(store.removeTask('b')).toBe(false); // not in primary's store + expect(store.removeTask('a')).toBe(true); + store.setScheduleScope(OTHER); + expect(store.getTask('b')).toBeDefined(); // untouched + }); +}); diff --git a/test/schedule-store-idempotency.test.ts b/test/schedule-store-idempotency.test.ts index b0f44f277..883c3d6a2 100644 --- a/test/schedule-store-idempotency.test.ts +++ b/test/schedule-store-idempotency.test.ts @@ -18,7 +18,7 @@ vi.mock('../src/config.js', () => ({ config: { session: { get dataDir() { - return tempDir; + return join(tempDir, 'data'); }, }, }, @@ -42,9 +42,13 @@ const BASE_PARAMS = { chatId: 'oc_test_chat', }; +const TEST_APP = 'cli_testapp0000000001'; + async function freshImport() { vi.resetModules(); - return import('../src/services/schedule-store.js'); + const mod = await import('../src/services/schedule-store.js'); + mod.setScheduleScope(TEST_APP); + return mod; } beforeEach(() => { @@ -173,7 +177,6 @@ describe('createTask — id provided, task exists with DIFFERENT canonical input ['chatId', { chatId: 'oc_other' }], ['rootMessageId', { rootMessageId: 'om_x' }], ['scope', { scope: 'chat' as const }], - ['larkAppId', { larkAppId: 'cli_other' }], ['deliver', { deliver: 'local' as const }], ])('throws when %s differs', async (_field, diff) => { const { createTask, IdempotencyConflictError } = await freshImport(); @@ -184,6 +187,23 @@ describe('createTask — id provided, task exists with DIFFERENT canonical input ); }); + it('routes a differing larkAppId to that bot\'s own store (no same-store conflict)', async () => { + // Per-bot stores: larkAppId selects WHICH file the task lands in, so the + // same wf id addressed at another bot creates independently in that bot's + // store instead of raising a same-store IdempotencyConflictError. Within + // one bot's store the conflict contract above is unchanged. (Workflow + // attempt-immutability for larkAppId is enforced upstream by the frozen + // input sidecar, not by the store.) + const { createTask, getTask } = await freshImport(); + const id = 'wf_cross_bot'; + createTask({ ...BASE_PARAMS, id }); + const other = createTask({ ...BASE_PARAMS, id, larkAppId: 'cli_other' }); + expect(other.larkAppId).toBe('cli_other'); + expect(getTask(id)).toBeDefined(); // bound scope's store + expect(getTask(id, 'cli_other')).toBeDefined(); // sibling store + expect(getTask(id)!.larkAppId).toBeUndefined(); + }); + it('throws when repeat.times differs', async () => { const { createTask, IdempotencyConflictError } = await freshImport(); const id = 'wf_conflict_repeat'; diff --git a/test/schedule-store.test.ts b/test/schedule-store.test.ts index b76e46cc5..e1f6d36fd 100644 --- a/test/schedule-store.test.ts +++ b/test/schedule-store.test.ts @@ -8,7 +8,7 @@ * Run: pnpm vitest run test/schedule-store.test.ts */ import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; -import { +import { mkdirSync, existsSync, mkdtempSync, readFileSync, @@ -16,7 +16,7 @@ import { rmSync, writeFileSync, } from 'node:fs'; -import { join } from 'node:path'; +import { dirname, join } from 'node:path'; import { tmpdir } from 'node:os'; // ─── Shared state ──────────────────────────────────────────────────────────── @@ -25,18 +25,26 @@ let tempDir: string; // ─── Mocks ─────────────────────────────────────────────────────────────────── -// Mock config so dataDir points to our temp directory. +// Mock config so dataDir points to our temp directory. `tempDir` acts as the +// botmux home root: dataDir = /data, so the per-bot store lands at +// /bots//schedules.json (inside the cleaned-up temp tree). // We update tempDir in beforeEach; the getter ensures the latest value is used. vi.mock('../src/config.js', () => ({ config: { session: { get dataDir() { - return tempDir; + return join(tempDir, 'data'); }, }, }, })); +const TEST_APP = 'cli_testapp0000000001'; +/** The per-bot store file for the bound test bot. */ +function storeFp(appId: string = TEST_APP): string { + return join(tempDir, 'bots', appId, 'schedules.json'); +} + // Suppress log output during tests. vi.mock('../src/utils/logger.js', () => ({ logger: { @@ -65,7 +73,9 @@ const TASK_PARAMS = { */ async function freshImport() { vi.resetModules(); - return import('../src/services/schedule-store.js'); + const mod = await import('../src/services/schedule-store.js'); + mod.setScheduleScope(TEST_APP); + return mod; } // ─── Lifecycle ─────────────────────────────────────────────────────────────── @@ -107,7 +117,7 @@ describe('schedule-store', () => { const { createTask } = await freshImport(); createTask(TASK_PARAMS); - const fp = join(tempDir, 'schedules.json'); + const fp = storeFp(); expect(existsSync(fp)).toBe(true); const data = JSON.parse(readFileSync(fp, 'utf-8')); @@ -152,7 +162,7 @@ describe('schedule-store', () => { expect(loudTask.silent).toBeUndefined(); expect(legacyTask.silent).toBeUndefined(); - const data = JSON.parse(readFileSync(join(tempDir, 'schedules.json'), 'utf-8')); + const data = JSON.parse(readFileSync(storeFp(), 'utf-8')); expect(data[silentTask.id].silent).toBe(true); expect('silent' in data[loudTask.id]).toBe(false); }); @@ -212,7 +222,7 @@ describe('schedule-store', () => { const task = createTask(TASK_PARAMS); removeTask(task.id); - const fp = join(tempDir, 'schedules.json'); + const fp = storeFp(); const data = JSON.parse(readFileSync(fp, 'utf-8')); expect(Object.keys(data)).toHaveLength(0); }); @@ -270,7 +280,8 @@ describe('schedule-store', () => { }); it('migrates a legacy new-topic row to explicit fresh-topic execution', async () => { - const fp = join(tempDir, 'schedules.json'); + const fp = storeFp(); + mkdirSync(dirname(fp), { recursive: true }); writeFileSync(fp, JSON.stringify({ legacy: { ...TASK_PARAMS, @@ -293,7 +304,7 @@ describe('schedule-store', () => { const task = createTask(TASK_PARAMS); updateTask(task.id, { enabled: false }); - const fp = join(tempDir, 'schedules.json'); + const fp = storeFp(); const data = JSON.parse(readFileSync(fp, 'utf-8')); expect(data[task.id].enabled).toBe(false); }); @@ -377,7 +388,7 @@ describe('schedule-store', () => { const modern = store1.createTask({ ...TASK_PARAMS, id: 'modern-scope', scope: 'thread' }); expect(modern.scope).toBe('thread'); - const fp = join(tempDir, 'schedules.json'); + const fp = storeFp(); const onDisk = JSON.parse(readFileSync(fp, 'utf-8')); onDisk['legacy-scope'] = { id: 'legacy-scope', @@ -412,7 +423,7 @@ describe('schedule-store', () => { it('rolls back memory and disk when persistence fails before rename', async () => { const store = await freshImport(); const original = store.createTask({ ...TASK_PARAMS, id: 'durable-original' }); - const fp = join(tempDir, 'schedules.json'); + const fp = storeFp(); const before = readFileSync(fp, 'utf-8'); store.__setScheduleStoreBeforeRenameTestHook(() => { @@ -444,7 +455,7 @@ describe('schedule-store', () => { store1.createTask({ ...TASK_PARAMS, id: 'from-store-1-b', name: 'one-b' }); store2.createTask({ ...TASK_PARAMS, id: 'from-store-2', name: 'two' }); - const persisted = JSON.parse(readFileSync(join(tempDir, 'schedules.json'), 'utf-8')); + const persisted = JSON.parse(readFileSync(storeFp(), 'utf-8')); expect(Object.keys(persisted).sort()).toEqual([ 'from-store-1-a', 'from-store-1-b', @@ -493,14 +504,14 @@ describe('schedule-store', () => { const { createTask } = await freshImport(); createTask(TASK_PARAMS); - expect(existsSync(join(nestedDir, 'schedules.json'))).toBe(true); + expect(existsSync(join(nestedDir, 'bots', TEST_APP, 'schedules.json'))).toBe(true); }); it('should handle an empty JSON file gracefully on reload', async () => { // Write an empty (but valid) JSON object const { writeFileSync, mkdirSync } = await import('node:fs'); - mkdirSync(tempDir, { recursive: true }); - writeFileSync(join(tempDir, 'schedules.json'), '{}', 'utf-8'); + mkdirSync(dirname(storeFp()), { recursive: true }); + writeFileSync(storeFp(), '{}', 'utf-8'); const { listTasks } = await freshImport(); expect(listTasks()).toEqual([]); @@ -508,8 +519,8 @@ describe('schedule-store', () => { it('should handle a corrupted JSON file gracefully', async () => { const { writeFileSync, mkdirSync } = await import('node:fs'); - mkdirSync(tempDir, { recursive: true }); - writeFileSync(join(tempDir, 'schedules.json'), '<<>>', 'utf-8'); + mkdirSync(dirname(storeFp()), { recursive: true }); + writeFileSync(storeFp(), '<<>>', 'utf-8'); const { listTasks } = await freshImport(); // Should recover with an empty store instead of throwing @@ -521,7 +532,7 @@ describe('schedule-store', () => { createTask(TASK_PARAMS); expect(existsSync(join(tempDir, 'schedules.json.tmp'))).toBe(false); - expect(existsSync(join(tempDir, 'schedules.json'))).toBe(true); + expect(existsSync(storeFp())).toBe(true); }); }); }); diff --git a/test/scheduler-cli-scope.test.ts b/test/scheduler-cli-scope.test.ts index 880715b95..efd76dd07 100644 --- a/test/scheduler-cli-scope.test.ts +++ b/test/scheduler-cli-scope.test.ts @@ -9,7 +9,7 @@ describe('schedule CLI session scope propagation', () => { expect(cliSource).toMatch(/function detectCurrentSession[\s\S]*?scope: s\.scope,/); expect(cliSource).toMatch(/const executionPosition: 'top-level' \| 'topic' \| 'new-topic' =[\s\S]*?cur\?\.scope/); expect(cliSource).toMatch(/const scope: 'thread' \| 'chat' = executionPosition === 'topic'/); - expect(cliSource).toMatch(/const task = scheduler\.addTask\(\{[\s\S]*?\bscope,[\s\S]*?\bexecutionPosition,[\s\S]*?\btopicTitle,[\s\S]*?\}\);/); + expect(cliSource).toMatch(/task = scheduler\.addTask\(\{[\s\S]*?\bscope,[\s\S]*?\bexecutionPosition,[\s\S]*?\btopicTitle,[\s\S]*?\}\);/); expect(cliSource).not.toContain('--new-topic 与 --silent 不能同时使用'); expect(cliSource).toMatch(/const silent = rest\.includes\('--silent'\)[\s\S]*?executionPosition[\s\S]*?scheduler\.addTask/); }); diff --git a/test/v3-host-schedule-runtime.test.ts b/test/v3-host-schedule-runtime.test.ts index da41fe737..096d84d4d 100644 --- a/test/v3-host-schedule-runtime.test.ts +++ b/test/v3-host-schedule-runtime.test.ts @@ -7,7 +7,7 @@ let tempDataDir = ''; vi.mock('../src/config.js', () => ({ config: { - session: { get dataDir() { return tempDataDir; } }, + session: { get dataDir() { return join(tempDataDir, 'data'); } }, }, })); vi.mock('../src/utils/logger.js', () => ({ @@ -15,7 +15,7 @@ vi.mock('../src/utils/logger.js', () => ({ })); import { createDefaultHostExecutorRegistry } from '../src/workflows/hostExecutors/registry.js'; -import { getTask, listTasks } from '../src/services/schedule-store.js'; +import { getTask, listTasks, setScheduleScope } from '../src/services/schedule-store.js'; import { validateDag } from '../src/workflows/v3/dag.js'; import { prepareV3HostInputArtifact } from '../src/workflows/v3/host-execution.js'; import { readJournal } from '../src/workflows/v3/journal.js'; @@ -35,6 +35,7 @@ const validateManifest: V3RuntimeDeps['validateManifest'] = async (manifestPath, beforeEach(() => { tempDataDir = mkdtempSync(join(tmpdir(), 'v3-host-schedule-store-')); + setScheduleScope('cli_test'); }); afterEach(() => { From f32c937e093b01f4fba0c954ad99cf43e555f8ea Mon Sep 17 00:00:00 2001 From: Austin Date: Mon, 27 Jul 2026 01:53:36 +0700 Subject: [PATCH 2/4] =?UTF-8?q?test(schedule):=20=E9=80=82=E9=85=8D=20per-?= =?UTF-8?q?bot=20=E5=AD=98=E5=82=A8=E2=80=94=E2=80=94fs-policy=20=E6=96=AD?= =?UTF-8?q?=E8=A8=80=E5=85=B1=E4=BA=AB=E8=B7=AF=E5=BE=84=E4=B8=8D=E5=86=8D?= =?UTF-8?q?=E6=94=BE=E8=A1=8C=E3=80=81host-executor/dashboard-ipc=20?= =?UTF-8?q?=E7=BB=91=E5=AE=9A=E6=B5=8B=E8=AF=95=20scope?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- test/dashboard-ipc.test.ts | 5 +++++ test/fs-policy.test.ts | 5 ++++- test/workflow-host-executors.test.ts | 7 ++++++- 3 files changed, 15 insertions(+), 2 deletions(-) diff --git a/test/dashboard-ipc.test.ts b/test/dashboard-ipc.test.ts index f18b107a1..02cdbe049 100644 --- a/test/dashboard-ipc.test.ts +++ b/test/dashboard-ipc.test.ts @@ -8,6 +8,11 @@ import { ipcRoute, startIpcServer, setLarkAppId, setIpcAuthSecret, setBotRenamer import { cliAuthBind, signCliAuth } from '../src/dashboard/auth.js'; import { dashboardEventBus } from '../src/core/dashboard-events.js'; import * as groupsStore from '../src/services/groups-store.js'; +import { setScheduleScope } from '../src/services/schedule-store.js'; + +// Per-bot schedule stores: the daemon binds the store to its own bot before +// serving IPC; the schedule endpoints under test assume that binding exists. +setScheduleScope('cli_ipc_test_bot001'); import * as larkClient from '../src/im/lark/client.js'; import * as oncallStore from '../src/services/oncall-store.js'; import * as sessionStore from '../src/services/session-store.js'; diff --git a/test/fs-policy.test.ts b/test/fs-policy.test.ts index 8eaa57d92..a33d76406 100644 --- a/test/fs-policy.test.ts +++ b/test/fs-policy.test.ts @@ -153,7 +153,10 @@ describe('buildFsPolicy', () => { expect(accessForPath(p.rules, '/Users/u/.botmux/data/webhook-master.key').access).toBe('none'); expect(accessForPath(p.rules, '/Users/u/.botmux/data/webhook-secrets.json').access).toBe('none'); // cross-bot content/routing (codex high finding) - expect(accessForPath(p.rules, '/Users/u/.botmux/data/schedules.json').access).toBe('readWrite'); // RMW schedule store — owner-accepted cross-bot exposure + // schedules moved into per-bot BOT_HOMEs: the legacy shared path is no + // longer granted (own store rides the BOT_HOME rw; sibling stores denied). + expect(accessForPath(p.rules, '/Users/u/.botmux/data/schedules.json').access).toBe('none'); + expect(accessForPath(p.rules, '/Users/u/.botmux/bots/cli_other/schedules.json').access).toBe('none'); // sibling store expect(accessForPath(p.rules, '/Users/u/.botmux/data/sessions-cli_other.json').access).toBe('none'); expect(accessForPath(p.rules, '/Users/u/.botmux/data/bot-openids-cli_other.json').access).toBe('none'); // sibling expect(accessForPath(p.rules, '/Users/u/.botmux/bots.json').access).toBe('none'); diff --git a/test/workflow-host-executors.test.ts b/test/workflow-host-executors.test.ts index 962740a95..f71ed49ba 100644 --- a/test/workflow-host-executors.test.ts +++ b/test/workflow-host-executors.test.ts @@ -374,7 +374,8 @@ describe('botmuxScheduleExecutor invoke()', () => { const { botmuxScheduleExecutor } = await import( '../src/workflows/hostExecutors/botmux-schedule.js' ); - const { getTask } = await import('../src/services/schedule-store.js'); + const { getTask, setScheduleScope } = await import('../src/services/schedule-store.js'); + setScheduleScope('cli_test'); // per-bot stores: ownerless inputs land in the bound scope const idemKey = 'wf_test_schedule_idem'; const result = await botmuxScheduleExecutor.invoke( @@ -407,6 +408,8 @@ describe('botmuxScheduleExecutor invoke()', () => { const { botmuxScheduleExecutor } = await import( '../src/workflows/hostExecutors/botmux-schedule.js' ); + const { setScheduleScope } = await import('../src/services/schedule-store.js'); + setScheduleScope('cli_test'); // per-bot stores: ownerless inputs land in the bound scope const idemKey = 'wf_test_schedule_rerun'; const input = { @@ -459,6 +462,8 @@ describe('botmuxScheduleExecutor invoke()', () => { const { botmuxScheduleExecutor, botmuxScheduleReconciler } = await import( '../src/workflows/hostExecutors/botmux-schedule.js' ); + const { setScheduleScope } = await import('../src/services/schedule-store.js'); + setScheduleScope('cli_test'); // per-bot stores: ownerless inputs land in the bound scope const idemKey = 'wf_test_schedule_lookup'; const input = { name: 'Lookup', From 01e7ff5879d9ef40288d4a699109d2782d23bbd5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E7=94=B3=E6=99=97?= Date: Sun, 26 Jul 2026 19:13:59 -0700 Subject: [PATCH 3/4] =?UTF-8?q?fix(schedule):=20=E9=87=87=E7=BA=B3=20codex?= =?UTF-8?q?=20#611=20=E5=A4=8D=E5=AE=A1=E2=80=94=E2=80=94=E4=BF=AE?= =?UTF-8?q?=E8=BF=81=E7=A7=BB=E6=9C=AA=E9=85=8D=E7=BD=AE=20owner=20?= =?UTF-8?q?=E6=90=81=E7=BD=AE=20+=20e2e=20=E6=B8=85=E7=90=86=E8=B7=A8=20st?= =?UTF-8?q?ore?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit codex 独立复审抓出 2 个阻塞缺口，均已修复并补回归测试（变异测试证对实现敏感）： 1. 迁移「未知/未配置 owner」搁置（finding 1，已 runtime 复现） - 病因：schedule-split-migration 把 larkAppId 不在 bots.json 的任务写进 primary 的 store 却保留原 larkAppId；primary 的 owner filter 因 appId 不匹配拒绝执行， 而该 owner 自己的 store 又是空的 → 任务永不触发。 - 修法（按 codex 建议区分三种归属，而非一律塞 primary）： · 真 ownerless（无 larkAppId）→ primary 的 store，保持 ownerless（primary daemon 本就执行 ownerless，等同拆分前行为；不 stamp primary appId） · owner 在 bots.json → 自己的 BOT_HOME store（不变） · owner 合法但当前未配置（bot 被删/配置漂移）→ 落自己的 dormant store（非 primary）， 保留 larkAppId，bot 将来恢复即原样可见（塞 primary 要么搁置、要么以错误 bot 身份执行） · larkAppId 不安全（无法做路径段）→ fail-safe：import 前中止整次拆分、保留 legacy 文件交人工，绝不静默吞行 2. 浏览器 E2E 清理器全部失效（finding 2） - 病因：test/e2e-browser/schedule-cleanup.ts 仍零参调用 removeTask/listTasks， per-bot store 固定抛 no bot scope bound；UI 清理失败后真实测试任务无法兜底删除， orphan sweep 也空跑。 - 修法：候选 ID 删除、按 label fallback、orphan sweep 三条路径全部枚举 /bots/ 下的 store 并显式传 appId（findTaskAcrossBots / listTasksForBots / removeTask(id, appId)），不再依赖任何 bound scope。 测试： - 新增 test/schedule-cleanup-per-bot.test.ts（4 用例：跨 store 删候选 id / label fallback / orphan sweep / 空 store no-op） - 更新 schedule-split-migration.test.ts：未配置-但安全 owner → 自己 store 保留 appId、 ownerless → primary 保持 ownerless、不安全 appId → 中止且保留 legacy - 变异测试：还原旧 buggy 路由 → 迁移 2 用例红；还原零参调用 → 清理 2 用例抛 no bot scope - pnpm build 绿；PR 相关 9 套件 213/213；全量 unit 10815 passed / 10 failed， 10 个失败全环境基线（TZ 非 +8 / model-runner 需网络 / root DAC_OVERRIDE 读穿真 bwrap）， 全非本次改动文件，零 no-bot-scope 泄漏 Co-Authored-By: Riff --- src/services/schedule-split-migration.ts | 55 ++++++++++-- test/e2e-browser/schedule-cleanup.ts | 64 +++++++++---- test/schedule-cleanup-per-bot.test.ts | 109 +++++++++++++++++++++++ test/schedule-split-migration.test.ts | 36 +++++++- 4 files changed, 239 insertions(+), 25 deletions(-) create mode 100644 test/schedule-cleanup-per-bot.test.ts diff --git a/src/services/schedule-split-migration.ts b/src/services/schedule-split-migration.ts index d45d503e9..123edd3a9 100644 --- a/src/services/schedule-split-migration.ts +++ b/src/services/schedule-split-migration.ts @@ -16,11 +16,21 @@ * and re-checks existence inside it, so exactly one daemon performs the split * and the rest see the file already gone (renamed to `schedules.json.bak-split-v1`). * - * Routing: each task goes to its `larkAppId` owner's store; tasks with no - * `larkAppId` (or an appId not in bots.json) go to the PRIMARY bot (index 0) — - * the same fallback the scheduler's owner filter has always used for legacy - * ownerless tasks. Id conflicts with an existing per-bot entry keep the - * existing entry (the per-bot store is newer by definition) and are logged. + * Routing (per task): + * - OWNERLESS (no `larkAppId`) → PRIMARY bot's store, kept ownerless. + * The scheduler runs ownerless tasks on the primary daemon (bot-0) — the + * exact pre-split behaviour. The appId is NOT stamped on. + * - owner IS in bots.json → that owner's own BOT_HOME store. + * - owner well-formed but NOT in → that owner's own (dormant) store, NOT + * bots.json (removed / config drift) primary. Folding it into primary would + * either strand it (primary's owner filter rejects a foreign appId) or, if + * stripped, run it under the wrong bot identity; its own store keeps it + * verbatim so it reappears intact if the bot is re-added (codex #611 f1). + * - owner is an UNSAFE appId (cannot → fail-safe: abort the split before any + * be a path segment) import, leave the legacy file for a + * human. Never silently drop the row. + * Id conflicts with an existing per-bot entry keep the existing entry (the + * per-bot store is newer by definition) and are logged. * * Downgrade: the pre-split file survives verbatim as `*.bak-split-v1`; an * older build can be restored by renaming it back (documented in the PR). @@ -32,6 +42,7 @@ import { join } from 'node:path'; import { config } from '../config.js'; import { logger } from '../utils/logger.js'; import { withFileLockSync } from '../utils/file-lock.js'; +import { assertSafeAppId } from '../adapters/cli/read-isolation.js'; import * as scheduleStore from './schedule-store.js'; import type { ScheduledTask } from '../types.js'; @@ -68,7 +79,39 @@ export function migrateSharedSchedulesAtStartup( const byBot = new Map>(); for (const [id, task] of Object.entries(raw as Record)) { if (!task || typeof task !== 'object') continue; - const owner = task.larkAppId && known.has(task.larkAppId) ? task.larkAppId : primaryAppId; + let owner: string; + if (!task.larkAppId) { + // Truly OWNERLESS legacy task → primary's store, kept ownerless. The + // scheduler runs an ownerless task on the primary daemon (bot-0), which + // is exactly the pre-split behaviour. Do NOT stamp primary's appId — that + // would pin it away from the legacy ownerless semantics. + owner = primaryAppId; + } else if (known.has(task.larkAppId)) { + // Configured owner → its own BOT_HOME store. + owner = task.larkAppId; + } else { + // Well-formed appId that is NOT currently in bots.json (bot removed, or + // config drift): route to ITS OWN store, not primary. Folding it into + // primary either strands it (primary's owner filter rejects a foreign + // appId — codex #611 finding 1) or, if we stripped the appId, would run + // it under the WRONG bot identity. Its own dormant store preserves the + // task verbatim so it reappears intact if that bot is re-added later. + // + // An unsafe appId cannot be a path segment (`scheduleFilePathFor` → + // `assertSafeAppId` would throw). Fail-safe: abort the whole split with + // NO import performed yet (imports happen after this loop), leaving the + // legacy file untouched for a human — never silently drop the row. + try { + assertSafeAppId(task.larkAppId); + } catch { + logger.error( + `[schedule-split] task ${id} has an unsafe larkAppId ${JSON.stringify(task.larkAppId)}; ` + + 'split aborted, legacy schedules.json preserved for manual resolution', + ); + return; + } + owner = task.larkAppId; + } let bucket = byBot.get(owner); if (!bucket) { bucket = []; byBot.set(owner, bucket); } bucket.push([id, task]); diff --git a/test/e2e-browser/schedule-cleanup.ts b/test/e2e-browser/schedule-cleanup.ts index be56eac1b..a45079997 100644 --- a/test/e2e-browser/schedule-cleanup.ts +++ b/test/e2e-browser/schedule-cleanup.ts @@ -5,12 +5,17 @@ * cleanup (typing `/schedule remove` into the chat) is fragile because it depends * on Midscene `aiAct` finding the right input box. These helpers provide a * programmatic fallback that imports `schedule-store` directly and writes through - * the same `schedules.json` the daemon reads — so even if the UI flow fails, - * tasks created by the run are wiped before the test process exits. + * the same per-bot `schedules.json` files the daemons read — so even if the UI + * flow fails, tasks created by the run are wiped before the test process exits. + * + * Schedules are stored PER BOT (`/bots//schedules.json`), so + * every store call MUST carry an explicit appId — the store throws + * `no bot scope bound` on a zero-arg call. This unsandboxed admin helper simply + * enumerates every `bots/` store dir and addresses each one explicitly. */ import { existsSync, readFileSync, readdirSync } from 'node:fs'; import { homedir } from 'node:os'; -import { join } from 'node:path'; +import { dirname, join } from 'node:path'; const CONFIG_DIR = join(homedir(), '.botmux'); const DEFAULT_DATA_DIR = join(CONFIG_DIR, 'data'); @@ -34,6 +39,28 @@ function resolveDataDir(): string { return DEFAULT_DATA_DIR; } +/** + * Enumerate every per-bot store appId under `/bots/`. Only dirs that + * actually hold a `schedules.json` are returned, so callers never address an + * empty/absent store. `botmuxHome` = parent of the daemon data dir. + */ +function allStoreAppIds(): string[] { + const botsDir = join(dirname(resolveDataDir()), 'bots'); + let entries: string[]; + try { + entries = readdirSync(botsDir); + } catch { + return []; // no bots dir yet → nothing to clean + } + return entries.filter(appId => { + try { + return existsSync(join(botsDir, appId, 'schedules.json')); + } catch { + return false; + } + }); +} + /** * Lazily import schedule-store with SESSION_DATA_DIR pointed at the daemon's * dataDir. We set the env var before the dynamic import so config.ts picks it up. @@ -45,8 +72,9 @@ async function loadStore() { /** * Remove botmux schedule tasks created by a single test run. Tries each - * candidate id first; if none match, falls back to scanning by `name === label`. - * Never throws — returns warnings the caller can log. + * candidate id across every per-bot store first; if none match, falls back to + * scanning by `name === label` across all stores. Never throws — returns + * warnings the caller can log. */ export async function cleanupTasksByLabel( label: string, @@ -62,10 +90,16 @@ export async function cleanupTasksByLabel( return { removed, warnings }; } + const appIds = allStoreAppIds(); + for (const id of candidateIds) { if (!id) continue; + // Locate the id across every readable store, then delete it from the owning + // store by explicit appId (a zero-arg removeTask throws `no bot scope`). + const hit = store.findTaskAcrossBots(id, appIds); + if (!hit) continue; try { - if (store.removeTask(id)) removed.push(id); + if (store.removeTask(id, hit.appId)) removed.push(id); } catch (err) { warnings.push(`removeTask(${id}) threw: ${(err as Error).message}`); } @@ -73,11 +107,11 @@ export async function cleanupTasksByLabel( if (removed.length === 0 && label) { try { - for (const task of store.listTasks()) { - if (task.name === label && store.removeTask(task.id)) removed.push(task.id); + for (const task of store.listTasksForBots(appIds)) { + if (task.name === label && store.removeTask(task.id, task._storeAppId)) removed.push(task.id); } } catch (err) { - warnings.push(`listTasks() threw: ${(err as Error).message}`); + warnings.push(`listTasksForBots() threw: ${(err as Error).message}`); } } @@ -85,10 +119,10 @@ export async function cleanupTasksByLabel( } /** - * Sweep orphan tasks left over from previous e2e runs. Targets tasks whose name - * matches `sched-` (the exact pattern feishu-schedule.e2e.ts uses) and - * whose `createdAt` is older than `maxAgeDays` days. The narrow regex prevents - * collateral damage to real user-created schedules. + * Sweep orphan tasks left over from previous e2e runs, across ALL per-bot + * stores. Targets tasks whose name matches `sched-` (the exact pattern + * feishu-schedule.e2e.ts uses) and whose `createdAt` is older than `maxAgeDays` + * days. The narrow regex prevents collateral damage to real user schedules. */ export async function sweepOrphanSchedTasks(maxAgeDays = 1): Promise { const removed: string[] = []; @@ -101,11 +135,11 @@ export async function sweepOrphanSchedTasks(maxAgeDays = 1): Promise { } const cutoff = Date.now() - maxAgeDays * 86_400_000; try { - for (const task of store.listTasks()) { + for (const task of store.listTasksForBots(allStoreAppIds())) { if (!/^sched-\d{10,}$/.test(task.name)) continue; const createdMs = task.createdAt ? Date.parse(task.createdAt) : 0; if (!createdMs || createdMs > cutoff) continue; - if (store.removeTask(task.id)) removed.push(task.id); + if (store.removeTask(task.id, task._storeAppId)) removed.push(task.id); } } catch (err) { console.warn(`[e2e:sweep] sweepOrphanSchedTasks failed: ${(err as Error).message}`); diff --git a/test/schedule-cleanup-per-bot.test.ts b/test/schedule-cleanup-per-bot.test.ts new file mode 100644 index 000000000..7ecf6eca8 --- /dev/null +++ b/test/schedule-cleanup-per-bot.test.ts @@ -0,0 +1,109 @@ +/** + * Regression for the e2e schedule cleanup helper under per-bot stores + * (test/e2e-browser/schedule-cleanup.ts). Schedules moved from one shared + * data/schedules.json to per-bot /bots//schedules.json, so + * the helper's old zero-arg removeTask/listTasks calls would throw + * `no bot scope bound`. It must enumerate every per-bot store and address each + * one explicitly (codex #611 finding 2). Real fs in a temp botmux-home tree. + */ +import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; +import { mkdirSync, mkdtempSync, rmSync, writeFileSync, readFileSync, existsSync } from 'node:fs'; +import { dirname, join } from 'node:path'; +import { tmpdir } from 'node:os'; + +let tempDir: string; // botmux home root; dataDir = /data + +// The helper resolves its data dir from SESSION_DATA_DIR, and schedule-store +// reads config.session.dataDir — point both at our temp tree. The store's +// per-bot path is dirname(dataDir)/bots//schedules.json. +vi.mock('../src/config.js', () => ({ + config: { session: { get dataDir() { return join(tempDir, 'data'); } } }, +})); +vi.mock('../src/utils/logger.js', () => ({ + logger: { info: vi.fn(), warn: vi.fn(), error: vi.fn(), debug: vi.fn() }, +})); + +const BOT_A = 'cli_bota00000000001'; +const BOT_B = 'cli_botb00000000002'; + +function storeFp(appId: string): string { + return join(tempDir, 'bots', appId, 'schedules.json'); +} +function seedTask(appId: string, id: string, name: string, extra: Record = {}) { + const fp = storeFp(appId); + mkdirSync(dirname(fp), { recursive: true }); + const existing = existsSync(fp) ? JSON.parse(readFileSync(fp, 'utf-8')) : {}; + existing[id] = { + id, name, schedule: '0 9 * * *', + parsed: { kind: 'cron', expr: '0 9 * * *', display: '0 9 * * *' }, + prompt: `p ${id}`, workingDir: '/w', chatId: 'oc_x', enabled: true, + createdAt: '2026-01-01T00:00:00.000Z', larkAppId: appId, ...extra, + }; + writeFileSync(fp, JSON.stringify(existing)); +} + +async function freshHelper() { + vi.resetModules(); + process.env.SESSION_DATA_DIR = join(tempDir, 'data'); + return import('./e2e-browser/schedule-cleanup.js'); +} + +beforeEach(() => { + tempDir = mkdtempSync(join(tmpdir(), 'sched-cleanup-')); + mkdirSync(join(tempDir, 'data'), { recursive: true }); + process.env.SESSION_DATA_DIR = join(tempDir, 'data'); +}); +afterEach(() => { + delete process.env.SESSION_DATA_DIR; + rmSync(tempDir, { recursive: true, force: true }); +}); + +describe('e2e schedule-cleanup helper across per-bot stores', () => { + it('cleanupTasksByLabel deletes a candidate id from whichever bot store holds it', async () => { + seedTask(BOT_A, 'ta', 'sched-1111111111'); + seedTask(BOT_B, 'tb', 'sched-2222222222'); + const helper = await freshHelper(); + + const { removed, warnings } = await helper.cleanupTasksByLabel('sched-2222222222', ['tb']); + expect(warnings).toEqual([]); + expect(removed).toEqual(['tb']); // deleted from BOT_B's store + expect(JSON.parse(readFileSync(storeFp(BOT_B), 'utf-8'))).toEqual({}); + // BOT_A untouched. + expect(Object.keys(JSON.parse(readFileSync(storeFp(BOT_A), 'utf-8')))).toEqual(['ta']); + }); + + it('cleanupTasksByLabel falls back to name match across ALL stores when no candidate id hits', async () => { + seedTask(BOT_A, 'ta', 'shared-label'); + seedTask(BOT_B, 'tb', 'shared-label'); + const helper = await freshHelper(); + + const { removed, warnings } = await helper.cleanupTasksByLabel('shared-label', []); + expect(warnings).toEqual([]); + expect(removed.sort()).toEqual(['ta', 'tb']); // both stores swept by label + }); + + it('sweepOrphanSchedTasks removes aged sched- tasks from every store, sparing fresh + non-matching', async () => { + const old = '2020-01-01T00:00:00.000Z'; + seedTask(BOT_A, 'old-a', 'sched-1000000000', { createdAt: old }); + seedTask(BOT_B, 'old-b', 'sched-2000000000', { createdAt: old }); + seedTask(BOT_A, 'fresh', 'sched-3000000000', { createdAt: new Date().toISOString() }); + seedTask(BOT_B, 'user', 'my real task', { createdAt: old }); // name doesn't match pattern + const helper = await freshHelper(); + + const removed = await helper.sweepOrphanSchedTasks(1); + expect(removed.sort()).toEqual(['old-a', 'old-b']); + // Fresh + user tasks survive. + const a = JSON.parse(readFileSync(storeFp(BOT_A), 'utf-8')); + const b = JSON.parse(readFileSync(storeFp(BOT_B), 'utf-8')); + expect(Object.keys(a)).toEqual(['fresh']); + expect(Object.keys(b)).toEqual(['user']); + }); + + it('no bot stores → helpers degrade to no-op without throwing', async () => { + const helper = await freshHelper(); + const { removed, warnings } = await helper.cleanupTasksByLabel('sched-x', ['nope']); + expect(removed).toEqual([]); + expect(warnings).toEqual([]); + expect(await helper.sweepOrphanSchedTasks(1)).toEqual([]); + }); +}); diff --git a/test/schedule-split-migration.test.ts b/test/schedule-split-migration.test.ts index 3882b2251..83c77aca1 100644 --- a/test/schedule-split-migration.test.ts +++ b/test/schedule-split-migration.test.ts @@ -69,25 +69,53 @@ afterEach(() => { }); describe('migrateSharedSchedulesAtStartup', () => { - it('splits tasks into each owner bot store; ownerless and unknown owners go to primary', async () => { + it('routes by owner: ownerless→primary (kept ownerless), configured→own store, unconfigured-but-safe→own dormant store', async () => { + const UNCONFIGURED = 'cli_unconfigured009'; // safe appId, NOT in bots.json writeFileSync(legacyFp(), JSON.stringify({ a: legacyTask('a', PRIMARY), b: legacyTask('b', OTHER), - c: legacyTask('c'), // ownerless → primary - d: legacyTask('d', 'cli_unknown0000009'), // not in bots.json → primary + c: legacyTask('c'), // ownerless → primary, stays ownerless + d: legacyTask('d', UNCONFIGURED), // safe but not configured → its OWN store (not primary) })); const { migration } = await freshImport(); migration.migrateSharedSchedulesAtStartup([PRIMARY, OTHER], PRIMARY); - expect(Object.keys(readStore(PRIMARY)).sort()).toEqual(['a', 'c', 'd']); + // Ownerless 'c' lands in primary and STAYS ownerless (so the primary daemon's + // owner filter runs it — stamping primary's appId would break that). + expect(Object.keys(readStore(PRIMARY)).sort()).toEqual(['a', 'c']); + expect(readStore(PRIMARY).c.larkAppId).toBeUndefined(); + expect(readStore(PRIMARY).a.larkAppId).toBe(PRIMARY); expect(Object.keys(readStore(OTHER))).toEqual(['b']); + // 'd' goes to its OWN store keeping its appId — NOT folded into primary. This + // is codex #611 finding 1: folding it into primary either strands it (foreign + // appId fails primary's filter) or runs it under the wrong identity. Its own + // dormant store keeps it intact until that bot is (re-)configured. + expect(Object.keys(readStore(UNCONFIGURED))).toEqual(['d']); + expect(readStore(UNCONFIGURED).d.larkAppId).toBe(UNCONFIGURED); // Legacy file renamed to backup, verbatim. expect(existsSync(legacyFp())).toBe(false); const bak = JSON.parse(readFileSync(`${legacyFp()}.bak-split-v1`, 'utf-8')); expect(Object.keys(bak).sort()).toEqual(['a', 'b', 'c', 'd']); }); + it('fail-safe on an unsafe larkAppId: aborts the split with no import, legacy file preserved', async () => { + // A path-traversal appId cannot be a store path segment. The split must abort + // BEFORE any import (imports happen after the routing loop) rather than throw + // mid-way or silently drop the row — leave everything for a human. + writeFileSync(legacyFp(), JSON.stringify({ + a: legacyTask('a', PRIMARY), + evil: legacyTask('evil', '../../etc'), + })); + const { migration } = await freshImport(); + migration.migrateSharedSchedulesAtStartup([PRIMARY], PRIMARY); + + // Legacy left in place, no backup, no partial per-bot store written. + expect(existsSync(legacyFp())).toBe(true); + expect(existsSync(`${legacyFp()}.bak-split-v1`)).toBe(false); + expect(existsSync(storeFp(PRIMARY))).toBe(false); + }); + it('is idempotent — second run is a no-op', async () => { writeFileSync(legacyFp(), JSON.stringify({ a: legacyTask('a', PRIMARY) })); const { migration } = await freshImport(); From 5114a79d9a716650d937522ff6bf6132d407858c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E7=94=B3=E6=99=97?= Date: Sun, 26 Jul 2026 19:33:12 -0700 Subject: [PATCH 4/4] =?UTF-8?q?fix(schedule):=20=E9=87=87=E7=BA=B3=20codex?= =?UTF-8?q?=20#611=20=E4=BA=8C=E8=BD=AE=E5=A4=8D=E5=AE=A1=E2=80=94?= =?UTF-8?q?=E2=80=94=E8=BF=81=E7=A7=BB=20owner=20=E5=88=A4=E5=AE=9A?= =?UTF-8?q?=E6=94=B6=E7=B4=A7=E5=88=B0=20undefined-only=20+=20=E9=9D=9E?= =?UTF-8?q?=E5=AD=97=E7=AC=A6=E4=B8=B2=E4=B8=80=E5=BE=8B=20fail-safe?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit codex 二轮复审在迁移 fail-safe 边界又抓出两个真实缺口（均已 runtime 复现）： - finding 3：truthy 非字符串 larkAppId（最小例 boolean `true`）——`assertSafeAppId` 的 RegExp.test 把它隐式转成 "true" 通过，任务被迁进 bots/true/、legacy 被删， 而恢复 appId="true" 的 bot 时 owner filter 严格比较 true !== "true" 仍不执行。 - finding 4：falsy 非字符串（`false`/`0`/`null`）——原 `if (!task.larkAppId)` 把它 当 ownerless 塞进 primary，且 primaryWouldExecute=true，会用**错误 bot 身份执行**， 比 finding 3 的搁置更危险。 根因：owner 判定既没区分「字段缺失(真 ownerless)」与「字段存在但类型错」，又在 路径安全校验（assertSafeAppId）之外漏了类型校验。 修法（按 codex 建议一次收紧，覆盖全部 falsy/truthy 非字符串 + 空串）： - 仅 `larkAppId === undefined` 才算 ownerless → primary 保持 ownerless - 其余先要求 `typeof === 'string'`，非字符串（bool/number/null/object，truthy 或 falsy）一律 fail-safe：中止整次拆分、保留 legacy、import 前零写入 - 再对字符串跑 assertSafeAppId（空串/路径穿越同样 fail-safe） - 配置内→自己 store，合法未配置→自己 dormant store（保 appId，不变） 测试： - schedule-split-migration.test.ts 的 fail-safe 用例表扩到 true/false/123/0/null/''/object 七种，断言中止+保留 legacy+无任何 coerced 名 store 目录（bots/ 下零 sibling） - 变异测试：把判定还原成 `!task.larkAppId` → false/0/null/'' 四例转红， 证明 undefined-only 判定正是 finding 4 的修复点 - pnpm build 绿；PR 相关 9 套件 220/220；全量 unit 10822 passed / 10 failed， 10 个失败全环境基线（TZ 非 +8 / model-runner 需网络 / root DAC_OVERRIDE 读穿真 bwrap），全非本次改动文件，零 no-bot-scope 泄漏 Co-Authored-By: Riff --- src/services/schedule-split-migration.ts | 66 +++++++++++++++++------- test/schedule-split-migration.test.ts | 38 +++++++++++++- 2 files changed, 84 insertions(+), 20 deletions(-) diff --git a/src/services/schedule-split-migration.ts b/src/services/schedule-split-migration.ts index 123edd3a9..f9af3710a 100644 --- a/src/services/schedule-split-migration.ts +++ b/src/services/schedule-split-migration.ts @@ -17,17 +17,24 @@ * and the rest see the file already gone (renamed to `schedules.json.bak-split-v1`). * * Routing (per task): - * - OWNERLESS (no `larkAppId`) → PRIMARY bot's store, kept ownerless. + * - OWNERLESS (`larkAppId` undefined) → PRIMARY bot's store, kept ownerless. * The scheduler runs ownerless tasks on the primary daemon (bot-0) — the - * exact pre-split behaviour. The appId is NOT stamped on. + * exact pre-split behaviour. The appId is NOT stamped on. ONLY `undefined` + * is ownerless; a falsy non-string is corrupt data (next branch). + * - owner present but NOT a string → fail-safe: abort the split. Covers + * (bool/number/null/object, truthy `true`/`false`/`0`/`null`/objects — + * OR falsy) none matches a string appId, and a + * falsy one would otherwise slip into primary and run under primary's + * identity (codex #611 f3+f4). `assertSafeAppId`'s RegExp.test would also + * silently coerce a truthy one (`true`→"true"), so guard the TYPE first. * - owner IS in bots.json → that owner's own BOT_HOME store. * - owner well-formed but NOT in → that owner's own (dormant) store, NOT * bots.json (removed / config drift) primary. Folding it into primary would * either strand it (primary's owner filter rejects a foreign appId) or, if * stripped, run it under the wrong bot identity; its own store keeps it * verbatim so it reappears intact if the bot is re-added (codex #611 f1). - * - owner is an UNSAFE appId (cannot → fail-safe: abort the split before any - * be a path segment) import, leave the legacy file for a + * - owner is an UNSAFE string appId → fail-safe: abort the split before any + * (cannot be a path segment) import, leave the legacy file for a * human. Never silently drop the row. * Id conflicts with an existing per-bot entry keep the existing entry (the * per-bot store is newer by definition) and are logged. @@ -80,27 +87,48 @@ export function migrateSharedSchedulesAtStartup( for (const [id, task] of Object.entries(raw as Record)) { if (!task || typeof task !== 'object') continue; let owner: string; - if (!task.larkAppId) { - // Truly OWNERLESS legacy task → primary's store, kept ownerless. The - // scheduler runs an ownerless task on the primary daemon (bot-0), which - // is exactly the pre-split behaviour. Do NOT stamp primary's appId — that - // would pin it away from the legacy ownerless semantics. + if (task.larkAppId === undefined) { + // Truly OWNERLESS legacy task (field absent) → primary's store, kept + // ownerless. The scheduler runs an ownerless task on the primary daemon + // (bot-0), which is exactly the pre-split behaviour. Do NOT stamp + // primary's appId — that would pin it away from the legacy ownerless + // semantics. ONLY `undefined` counts as ownerless: a falsy non-string + // (`false`/`0`/`null`) is corrupt data, not "no owner", and must not be + // silently absorbed into primary where it would run under primary's + // identity (codex #611 finding 4). owner = primaryAppId; + } else if (typeof task.larkAppId !== 'string') { + // Present but NOT a string (boolean/number/null/object, truthy OR falsy): + // corrupt owner data. It can NEVER match a real bot — the scheduler's + // owner filter compares `task.larkAppId === ownerAppId` against a STRING + // appId — and `assertSafeAppId`'s `RegExp.test` would silently coerce it + // (`true`→"true", `false`→"false") and pass, migrating it into + // `bots//` where no daemon ever runs it, OR (for falsy values) + // slip past a truthiness check into primary and run under the WRONG + // identity (codex #611 findings 3 & 4). Guard the TYPE, then fail-safe: + // abort the whole split with no import performed, leaving legacy intact. + logger.error( + `[schedule-split] task ${id} has a non-string larkAppId ` + + `(${task.larkAppId === null ? 'null' : typeof task.larkAppId}); ` + + 'split aborted, legacy schedules.json preserved', + ); + return; } else if (known.has(task.larkAppId)) { // Configured owner → its own BOT_HOME store. owner = task.larkAppId; } else { - // Well-formed appId that is NOT currently in bots.json (bot removed, or - // config drift): route to ITS OWN store, not primary. Folding it into - // primary either strands it (primary's owner filter rejects a foreign - // appId — codex #611 finding 1) or, if we stripped the appId, would run - // it under the WRONG bot identity. Its own dormant store preserves the - // task verbatim so it reappears intact if that bot is re-added later. + // Well-formed STRING appId that is NOT currently in bots.json (bot + // removed, or config drift): route to ITS OWN store, not primary. Folding + // it into primary either strands it (primary's owner filter rejects a + // foreign appId — codex #611 finding 1) or, if we stripped the appId, + // would run it under the WRONG bot identity. Its own dormant store + // preserves the task verbatim so it reappears intact if the bot is + // re-added later. // - // An unsafe appId cannot be a path segment (`scheduleFilePathFor` → - // `assertSafeAppId` would throw). Fail-safe: abort the whole split with - // NO import performed yet (imports happen after this loop), leaving the - // legacy file untouched for a human — never silently drop the row. + // An unsafe string appId (path-traversal, or empty string) cannot be a + // path segment (`scheduleFilePathFor` → `assertSafeAppId` throws). + // Fail-safe: abort the whole split with NO import performed yet (imports + // happen after this loop), leaving legacy untouched — never drop the row. try { assertSafeAppId(task.larkAppId); } catch { diff --git a/test/schedule-split-migration.test.ts b/test/schedule-split-migration.test.ts index 83c77aca1..406609450 100644 --- a/test/schedule-split-migration.test.ts +++ b/test/schedule-split-migration.test.ts @@ -4,7 +4,7 @@ * mocked config/logger — same scaffolding as schedule-store.test.ts. */ import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; -import { existsSync, mkdirSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from 'node:fs'; +import { existsSync, mkdirSync, mkdtempSync, readdirSync, readFileSync, rmSync, writeFileSync } from 'node:fs'; import { dirname, join } from 'node:path'; import { tmpdir } from 'node:os'; @@ -116,6 +116,42 @@ describe('migrateSharedSchedulesAtStartup', () => { expect(existsSync(storeFp(PRIMARY))).toBe(false); }); + it.each([ + ['boolean true', true], + ['boolean false', false], + ['number', 123], + ['zero', 0], + ['null', null], + ['empty string', ''], + ['object', { evil: 1 }], + ])('fail-safe on a non-string / empty larkAppId (%s): abort split, legacy preserved, no coerced store', async (_label, badOwner) => { + // codex #611 findings 3+4: a present-but-non-string larkAppId must NOT be + // treated as ownerless nor coerced into a path. + // - truthy non-string (`true`) slips past assertSafeAppId's RegExp.test + // (coerced to "true") → lands in bots/true/, unreachable (owner filter: + // true !== "true"). + // - falsy non-string (`false`/`0`/`null`) would slip past a `!larkAppId` + // ownerless check into primary and run under primary's WRONG identity. + // - `''` is a string but not a legal appId (assertSafeAppId rejects it). + // All must fail-safe: abort the whole split, keep legacy, import nothing. + writeFileSync(legacyFp(), JSON.stringify({ + a: legacyTask('a', PRIMARY), + bad: { ...legacyTask('bad'), larkAppId: badOwner }, + })); + const { migration } = await freshImport(); + migration.migrateSharedSchedulesAtStartup([PRIMARY], PRIMARY); + + // Split aborted before any import: legacy preserved, no backup, and NO store + // written — neither primary's nor a coerced-name one (bots/true/, bots/0/, …). + expect(existsSync(legacyFp())).toBe(true); + expect(existsSync(`${legacyFp()}.bak-split-v1`)).toBe(false); + expect(existsSync(storeFp(PRIMARY))).toBe(false); + // No sibling per-bot store dir was created for a coerced owner name. + const botsDir = join(tempDir, 'bots'); + const siblingDirs = existsSync(botsDir) ? readdirSync(botsDir) : []; + expect(siblingDirs).toEqual([]); + }); + it('is idempotent — second run is a no-op', async () => { writeFileSync(legacyFp(), JSON.stringify({ a: legacyTask('a', PRIMARY) })); const { migration } = await freshImport();