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..f9af3710a --- /dev/null +++ b/src/services/schedule-split-migration.ts @@ -0,0 +1,160 @@ +/** + * 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 (per task): + * - 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. 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 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. + * + * 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 { assertSafeAppId } from '../adapters/cli/read-isolation.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; + let owner: string; + 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 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 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 { + 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]); + } + + 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/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/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/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/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 new file mode 100644 index 000000000..406609450 --- /dev/null +++ b/test/schedule-split-migration.test.ts @@ -0,0 +1,216 @@ +/** + * 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, readdirSync, 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('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, stays ownerless + d: legacyTask('d', UNCONFIGURED), // safe but not configured → its OWN store (not primary) + })); + + const { migration } = await freshImport(); + migration.migrateSharedSchedulesAtStartup([PRIMARY, OTHER], PRIMARY); + + // 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.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(); + 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(() => { 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',