diff --git a/src/channels/transport/stall-watchdog.test.ts b/src/channels/transport/stall-watchdog.test.ts index 1dfbb6d8d50..c5b9601493e 100644 --- a/src/channels/transport/stall-watchdog.test.ts +++ b/src/channels/transport/stall-watchdog.test.ts @@ -1,17 +1,23 @@ import { describe, expect, it, vi } from "vitest"; import { createArmableStallWatchdog } from "./stall-watchdog.js"; +function createTestWatchdog( + onTimeout: Parameters[0]["onTimeout"], +) { + return createArmableStallWatchdog({ + label: "test-watchdog", + timeoutMs: 1_000, + checkIntervalMs: 100, + onTimeout, + }); +} + describe("createArmableStallWatchdog", () => { it("fires onTimeout once when armed and idle exceeds timeout", async () => { vi.useFakeTimers(); try { const onTimeout = vi.fn(); - const watchdog = createArmableStallWatchdog({ - label: "test-watchdog", - timeoutMs: 1_000, - checkIntervalMs: 100, - onTimeout, - }); + const watchdog = createTestWatchdog(onTimeout); watchdog.arm(); await vi.advanceTimersByTimeAsync(1_500); @@ -28,12 +34,7 @@ describe("createArmableStallWatchdog", () => { vi.useFakeTimers(); try { const onTimeout = vi.fn(); - const watchdog = createArmableStallWatchdog({ - label: "test-watchdog", - timeoutMs: 1_000, - checkIntervalMs: 100, - onTimeout, - }); + const watchdog = createTestWatchdog(onTimeout); watchdog.arm(); await vi.advanceTimersByTimeAsync(500); @@ -51,12 +52,7 @@ describe("createArmableStallWatchdog", () => { vi.useFakeTimers(); try { const onTimeout = vi.fn(); - const watchdog = createArmableStallWatchdog({ - label: "test-watchdog", - timeoutMs: 1_000, - checkIntervalMs: 100, - onTimeout, - }); + const watchdog = createTestWatchdog(onTimeout); watchdog.arm(); await vi.advanceTimersByTimeAsync(700); diff --git a/src/commands/agents.commands.bind.ts b/src/commands/agents.commands.bind.ts index 37862f4d00e..5e1bcce3c50 100644 --- a/src/commands/agents.commands.bind.ts +++ b/src/commands/agents.commands.bind.ts @@ -128,6 +128,28 @@ function emitJsonPayload(params: { return true; } +async function resolveConfigAndTargetAgentIdOrExit(params: { + runtime: RuntimeEnv; + agentInput: string | undefined; +}): Promise<{ + cfg: NonNullable>>; + agentId: string; +} | null> { + const cfg = await requireValidConfig(params.runtime); + if (!cfg) { + return null; + } + const agentId = resolveTargetAgentIdOrExit({ + cfg, + runtime: params.runtime, + agentInput: params.agentInput, + }); + if (!agentId) { + return null; + } + return { cfg, agentId }; +} + export async function agentsBindingsCommand( opts: AgentsBindingsListOptions, runtime: RuntimeEnv = defaultRuntime, @@ -186,15 +208,14 @@ export async function agentsBindCommand( opts: AgentsBindOptions, runtime: RuntimeEnv = defaultRuntime, ) { - const cfg = await requireValidConfig(runtime); - if (!cfg) { - return; - } - - const agentId = resolveTargetAgentIdOrExit({ cfg, runtime, agentInput: opts.agent }); - if (!agentId) { + const resolved = await resolveConfigAndTargetAgentIdOrExit({ + runtime, + agentInput: opts.agent, + }); + if (!resolved) { return; } + const { cfg, agentId } = resolved; const parsed = resolveParsedBindingsOrExit({ runtime, @@ -264,15 +285,14 @@ export async function agentsUnbindCommand( opts: AgentsUnbindOptions, runtime: RuntimeEnv = defaultRuntime, ) { - const cfg = await requireValidConfig(runtime); - if (!cfg) { - return; - } - - const agentId = resolveTargetAgentIdOrExit({ cfg, runtime, agentInput: opts.agent }); - if (!agentId) { + const resolved = await resolveConfigAndTargetAgentIdOrExit({ + runtime, + agentInput: opts.agent, + }); + if (!resolved) { return; } + const { cfg, agentId } = resolved; if (opts.all && (opts.bind?.length ?? 0) > 0) { runtime.error("Use either --all or --bind, not both."); runtime.exit(1); diff --git a/src/commands/auth-choice.apply-helpers.test.ts b/src/commands/auth-choice.apply-helpers.test.ts index 471123621e1..dac5d257921 100644 --- a/src/commands/auth-choice.apply-helpers.test.ts +++ b/src/commands/auth-choice.apply-helpers.test.ts @@ -44,6 +44,26 @@ function createPromptSpies(params?: { confirmResult?: boolean; textResult?: stri return { confirm, note, text }; } +async function ensureMinimaxApiKey(params: { + confirm: WizardPrompter["confirm"]; + text: WizardPrompter["text"]; + setCredential: Parameters[0]["setCredential"]; + config?: Parameters[0]["config"]; + secretInputMode?: Parameters[0]["secretInputMode"]; +}) { + return await ensureApiKeyFromEnvOrPrompt({ + config: params.config ?? {}, + provider: "minimax", + envLabel: "MINIMAX_API_KEY", + promptMessage: "Enter key", + normalize: (value) => value.trim(), + validate: () => undefined, + prompter: createPrompter({ confirm: params.confirm, text: params.text }), + secretInputMode: params.secretInputMode, + setCredential: params.setCredential, + }); +} + async function runEnsureMinimaxApiKeyFlow(params: { confirmResult: boolean; textResult: string }) { process.env.MINIMAX_API_KEY = "env-key"; delete process.env.MINIMAX_OAUTH_TOKEN; @@ -53,15 +73,9 @@ async function runEnsureMinimaxApiKeyFlow(params: { confirmResult: boolean; text textResult: params.textResult, }); const setCredential = vi.fn(async () => undefined); - - const result = await ensureApiKeyFromEnvOrPrompt({ - config: {}, - provider: "minimax", - envLabel: "MINIMAX_API_KEY", - promptMessage: "Enter key", - normalize: (value) => value.trim(), - validate: () => undefined, - prompter: createPrompter({ confirm, text }), + const result = await ensureMinimaxApiKey({ + confirm, + text, setCredential, }); @@ -164,14 +178,9 @@ describe("ensureApiKeyFromEnvOrPrompt", () => { }); const setCredential = vi.fn(async () => undefined); - const result = await ensureApiKeyFromEnvOrPrompt({ - config: {}, - provider: "minimax", - envLabel: "MINIMAX_API_KEY", - promptMessage: "Enter key", - normalize: (value) => value.trim(), - validate: () => undefined, - prompter: createPrompter({ confirm, text }), + const result = await ensureMinimaxApiKey({ + confirm, + text, secretInputMode: "ref", setCredential, }); @@ -195,14 +204,9 @@ describe("ensureApiKeyFromEnvOrPrompt", () => { const setCredential = vi.fn(async () => undefined); await expect( - ensureApiKeyFromEnvOrPrompt({ - config: {}, - provider: "minimax", - envLabel: "MINIMAX_API_KEY", - promptMessage: "Enter key", - normalize: (value) => value.trim(), - validate: () => undefined, - prompter: createPrompter({ confirm, text }), + ensureMinimaxApiKey({ + confirm, + text, secretInputMode: "ref", setCredential, }), diff --git a/src/commands/auth-choice.apply.huggingface.test.ts b/src/commands/auth-choice.apply.huggingface.test.ts index 9cc77fceb43..5b55252067f 100644 --- a/src/commands/auth-choice.apply.huggingface.test.ts +++ b/src/commands/auth-choice.apply.huggingface.test.ts @@ -29,6 +29,19 @@ function createHuggingfacePrompter(params: { return createWizardPrompter(overrides, { defaultSelect: "" }); } +type ApplyHuggingfaceParams = Parameters[0]; + +async function runHuggingfaceApply( + params: Omit & + Partial>, +) { + return await applyAuthChoiceHuggingface({ + authChoice: "huggingface-api-key", + setDefaultModel: params.setDefaultModel ?? true, + ...params, + }); +} + describe("applyAuthChoiceHuggingface", () => { const lifecycle = createAuthTestLifecycle([ "OPENCLAW_STATE_DIR", @@ -75,12 +88,10 @@ describe("applyAuthChoiceHuggingface", () => { const prompter = createHuggingfacePrompter({ text, select }); const runtime = createExitThrowingRuntime(); - const result = await applyAuthChoiceHuggingface({ - authChoice: "huggingface-api-key", + const result = await runHuggingfaceApply({ config: {}, prompter, runtime, - setDefaultModel: true, }); expect(result).not.toBeNull(); @@ -132,12 +143,10 @@ describe("applyAuthChoiceHuggingface", () => { const prompter = createHuggingfacePrompter({ text, select, confirm }); const runtime = createExitThrowingRuntime(); - const result = await applyAuthChoiceHuggingface({ - authChoice: "huggingface-api-key", + const result = await runHuggingfaceApply({ config: {}, prompter, runtime, - setDefaultModel: true, opts: { tokenProvider, token, @@ -167,12 +176,10 @@ describe("applyAuthChoiceHuggingface", () => { const prompter = createHuggingfacePrompter({ text, select, note }); const runtime = createExitThrowingRuntime(); - const result = await applyAuthChoiceHuggingface({ - authChoice: "huggingface-api-key", + const result = await runHuggingfaceApply({ config: {}, prompter, runtime, - setDefaultModel: true, }); expect(result).not.toBeNull(); diff --git a/src/commands/doctor-config-flow.include-warning.test.ts b/src/commands/doctor-config-flow.include-warning.test.ts index 79ed3148406..bea208f4022 100644 --- a/src/commands/doctor-config-flow.include-warning.test.ts +++ b/src/commands/doctor-config-flow.include-warning.test.ts @@ -1,16 +1,15 @@ import { describe, expect, it, vi } from "vitest"; import { withTempHomeConfig } from "../config/test-helpers.js"; - -const { noteSpy } = vi.hoisted(() => ({ - noteSpy: vi.fn(), -})); +import { note } from "../terminal/note.js"; vi.mock("../terminal/note.js", () => ({ - note: noteSpy, + note: vi.fn(), })); import { loadAndMaybeMigrateDoctorConfig } from "./doctor-config-flow.js"; +const noteSpy = vi.mocked(note); + describe("doctor include warning", () => { it("surfaces include confinement hint for escaped include paths", async () => { await withTempHomeConfig({ $include: "/etc/passwd" }, async () => { diff --git a/src/commands/doctor-config-flow.missing-default-account-bindings.integration.test.ts b/src/commands/doctor-config-flow.missing-default-account-bindings.integration.test.ts index dae204ede43..ee5ac2e13c6 100644 --- a/src/commands/doctor-config-flow.missing-default-account-bindings.integration.test.ts +++ b/src/commands/doctor-config-flow.missing-default-account-bindings.integration.test.ts @@ -1,13 +1,10 @@ import { describe, expect, it, vi } from "vitest"; +import { note } from "../terminal/note.js"; import { withEnvAsync } from "../test-utils/env.js"; import { runDoctorConfigWithInput } from "./doctor-config-flow.test-utils.js"; -const { noteSpy } = vi.hoisted(() => ({ - noteSpy: vi.fn(), -})); - vi.mock("../terminal/note.js", () => ({ - note: noteSpy, + note: vi.fn(), })); vi.mock("./doctor-legacy-config.js", async (importOriginal) => { @@ -23,6 +20,8 @@ vi.mock("./doctor-legacy-config.js", async (importOriginal) => { import { loadAndMaybeMigrateDoctorConfig } from "./doctor-config-flow.js"; +const noteSpy = vi.mocked(note); + describe("doctor missing default account binding warning", () => { it("emits a doctor warning when named accounts have no valid account-scoped bindings", async () => { await withEnvAsync( diff --git a/src/commands/doctor-config-flow.safe-bins.test.ts b/src/commands/doctor-config-flow.safe-bins.test.ts index 802cfeb8d96..c20f69cf4b5 100644 --- a/src/commands/doctor-config-flow.safe-bins.test.ts +++ b/src/commands/doctor-config-flow.safe-bins.test.ts @@ -2,20 +2,19 @@ import fs from "node:fs/promises"; import os from "node:os"; import path from "node:path"; import { beforeEach, describe, expect, it, vi } from "vitest"; +import { note } from "../terminal/note.js"; import { withEnvAsync } from "../test-utils/env.js"; import { runDoctorConfigWithInput } from "./doctor-config-flow.test-utils.js"; -const { noteSpy } = vi.hoisted(() => ({ - noteSpy: vi.fn(), -})); - vi.mock("../terminal/note.js", () => ({ - note: noteSpy, + note: vi.fn(), })); import { loadAndMaybeMigrateDoctorConfig } from "./doctor-config-flow.js"; describe("doctor config flow safe bins", () => { + const noteSpy = vi.mocked(note); + beforeEach(() => { noteSpy.mockClear(); }); diff --git a/src/commands/doctor-state-integrity.test.ts b/src/commands/doctor-state-integrity.test.ts index dd33786c32d..f2d0d5ec1fc 100644 --- a/src/commands/doctor-state-integrity.test.ts +++ b/src/commands/doctor-state-integrity.test.ts @@ -65,6 +65,20 @@ async function runStateIntegrity(cfg: OpenClawConfig) { return confirmSkipInNonInteractive; } +function writeSessionStore( + cfg: OpenClawConfig, + sessions: Record, +) { + setupSessionState(cfg, process.env, process.env.HOME ?? ""); + const storePath = resolveStorePath(cfg.session?.store, { agentId: "main" }); + fs.writeFileSync(storePath, JSON.stringify(sessions, null, 2)); +} + +async function runStateIntegrityText(cfg: OpenClawConfig): Promise { + await noteStateIntegrity(cfg, { confirmSkipInNonInteractive: vi.fn(async () => false) }); + return stateIntegrityText(); +} + describe("doctor state integrity oauth dir checks", () => { let envSnapshot: EnvSnapshot; let tempHome = ""; @@ -146,25 +160,13 @@ describe("doctor state integrity oauth dir checks", () => { it("prints openclaw-only verification hints when recent sessions are missing transcripts", async () => { const cfg: OpenClawConfig = {}; - setupSessionState(cfg, process.env, process.env.HOME ?? ""); - const storePath = resolveStorePath(cfg.session?.store, { agentId: "main" }); - fs.writeFileSync( - storePath, - JSON.stringify( - { - "agent:main:main": { - sessionId: "missing-transcript", - updatedAt: Date.now(), - }, - }, - null, - 2, - ), - ); - - await noteStateIntegrity(cfg, { confirmSkipInNonInteractive: vi.fn(async () => false) }); - - const text = stateIntegrityText(); + writeSessionStore(cfg, { + "agent:main:main": { + sessionId: "missing-transcript", + updatedAt: Date.now(), + }, + }); + const text = await runStateIntegrityText(cfg); expect(text).toContain("recent sessions are missing transcripts"); expect(text).toMatch(/openclaw sessions --store ".*sessions\.json"/); expect(text).toMatch(/openclaw sessions cleanup --store ".*sessions\.json" --dry-run/); @@ -177,25 +179,13 @@ describe("doctor state integrity oauth dir checks", () => { it("ignores slash-routing sessions for recent missing transcript warnings", async () => { const cfg: OpenClawConfig = {}; - setupSessionState(cfg, process.env, process.env.HOME ?? ""); - const storePath = resolveStorePath(cfg.session?.store, { agentId: "main" }); - fs.writeFileSync( - storePath, - JSON.stringify( - { - "agent:main:telegram:slash:6790081233": { - sessionId: "missing-slash-transcript", - updatedAt: Date.now(), - }, - }, - null, - 2, - ), - ); - - await noteStateIntegrity(cfg, { confirmSkipInNonInteractive: vi.fn(async () => false) }); - - const text = stateIntegrityText(); + writeSessionStore(cfg, { + "agent:main:telegram:slash:6790081233": { + sessionId: "missing-slash-transcript", + updatedAt: Date.now(), + }, + }); + const text = await runStateIntegrityText(cfg); expect(text).not.toContain("recent sessions are missing transcripts"); }); }); diff --git a/src/cron/isolated-agent.uses-last-non-empty-agent-text-as.test.ts b/src/cron/isolated-agent.uses-last-non-empty-agent-text-as.test.ts index 90e6de7e3ac..cb5d03d6a2c 100644 --- a/src/cron/isolated-agent.uses-last-non-empty-agent-text-as.test.ts +++ b/src/cron/isolated-agent.uses-last-non-empty-agent-text-as.test.ts @@ -152,6 +152,19 @@ async function runTurnWithStoredModelOverride( }); } +async function runStoredOverrideAndExpectModel(params: { + home: string; + deterministicCatalog: Array<{ id: string; name: string; provider: string }>; + jobPayload: CronJob["payload"]; + expected: { provider: string; model: string }; +}) { + vi.mocked(runEmbeddedPiAgent).mockClear(); + vi.mocked(loadModelCatalog).mockResolvedValue(params.deterministicCatalog); + const res = (await runTurnWithStoredModelOverride(params.home, params.jobPayload)).res; + expect(res.status).toBe("ok"); + expectEmbeddedProviderModel(params.expected); +} + describe("runCronIsolatedAgentTurn", () => { beforeEach(() => { vi.mocked(runEmbeddedPiAgent).mockClear(); @@ -352,30 +365,28 @@ describe("runCronIsolatedAgentTurn", () => { expect(res.status).toBe("ok"); expectEmbeddedProviderModel({ provider: "openai", model: "gpt-4.1-mini" }); - vi.mocked(runEmbeddedPiAgent).mockClear(); - vi.mocked(loadModelCatalog).mockResolvedValue(deterministicCatalog); - res = ( - await runTurnWithStoredModelOverride(home, { + await runStoredOverrideAndExpectModel({ + home, + deterministicCatalog, + jobPayload: { kind: "agentTurn", message: DEFAULT_MESSAGE, deliver: false, - }) - ).res; - expect(res.status).toBe("ok"); - expectEmbeddedProviderModel({ provider: "openai", model: "gpt-4.1-mini" }); + }, + expected: { provider: "openai", model: "gpt-4.1-mini" }, + }); - vi.mocked(runEmbeddedPiAgent).mockClear(); - vi.mocked(loadModelCatalog).mockResolvedValue(deterministicCatalog); - res = ( - await runTurnWithStoredModelOverride(home, { + await runStoredOverrideAndExpectModel({ + home, + deterministicCatalog, + jobPayload: { kind: "agentTurn", message: DEFAULT_MESSAGE, model: "anthropic/claude-opus-4-5", deliver: false, - }) - ).res; - expect(res.status).toBe("ok"); - expectEmbeddedProviderModel({ provider: "anthropic", model: "claude-opus-4-5" }); + }, + expected: { provider: "anthropic", model: "claude-opus-4-5" }, + }); }); }); diff --git a/src/cron/isolated-agent/delivery-target.test.ts b/src/cron/isolated-agent/delivery-target.test.ts index b28239adda8..14d02f0b380 100644 --- a/src/cron/isolated-agent/delivery-target.test.ts +++ b/src/cron/isolated-agent/delivery-target.test.ts @@ -35,6 +35,17 @@ function makeCfg(overrides?: Partial): OpenClawConfig { } as OpenClawConfig; } +function makeTelegramBoundCfg(accountId = "account-b"): OpenClawConfig { + return makeCfg({ + bindings: [ + { + agentId: AGENT_ID, + match: { channel: "telegram", accountId }, + }, + ], + }); +} + const AGENT_ID = "agent-b"; const DEFAULT_TARGET = { channel: "telegram" as const, @@ -109,16 +120,7 @@ describe("resolveDeliveryTarget", () => { it("falls back to bound accountId when session has no lastAccountId", async () => { setMainSessionEntry(undefined); - - const cfg = makeCfg({ - bindings: [ - { - agentId: "agent-b", - match: { channel: "telegram", accountId: "account-b" }, - }, - ], - }); - + const cfg = makeTelegramBoundCfg(); const result = await resolveForAgent({ cfg }); expect(result.accountId).toBe("account-b"); @@ -133,15 +135,7 @@ describe("resolveDeliveryTarget", () => { lastAccountId: "session-account", }); - const cfg = makeCfg({ - bindings: [ - { - agentId: "agent-b", - match: { channel: "telegram", accountId: "account-b" }, - }, - ], - }); - + const cfg = makeTelegramBoundCfg(); const result = await resolveForAgent({ cfg }); // Session-derived accountId should take precedence over binding diff --git a/src/cron/service.failure-alert.test.ts b/src/cron/service.failure-alert.test.ts index 6cfa9780074..0967274548a 100644 --- a/src/cron/service.failure-alert.test.ts +++ b/src/cron/service.failure-alert.test.ts @@ -4,6 +4,8 @@ import path from "node:path"; import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; import { CronService } from "./service.js"; +type CronServiceParams = ConstructorParameters[0]; + const noopLogger = { debug: vi.fn(), info: vi.fn(), @@ -21,6 +23,24 @@ async function makeStorePath() { }; } +function createFailureAlertCron(params: { + storePath: string; + cronConfig?: CronServiceParams["cronConfig"]; + runIsolatedAgentJob: NonNullable; + sendCronFailureAlert: NonNullable; +}) { + return new CronService({ + storePath: params.storePath, + cronEnabled: true, + cronConfig: params.cronConfig, + log: noopLogger, + enqueueSystemEvent: vi.fn(), + requestHeartbeatNow: vi.fn(), + runIsolatedAgentJob: params.runIsolatedAgentJob, + sendCronFailureAlert: params.sendCronFailureAlert, + }); +} + describe("CronService failure alerts", () => { beforeEach(() => { vi.useFakeTimers(); @@ -43,9 +63,8 @@ describe("CronService failure alerts", () => { error: "wrong model id", })); - const cron = new CronService({ + const cron = createFailureAlertCron({ storePath: store.storePath, - cronEnabled: true, cronConfig: { failureAlert: { enabled: true, @@ -53,9 +72,6 @@ describe("CronService failure alerts", () => { cooldownMs: 60_000, }, }, - log: noopLogger, - enqueueSystemEvent: vi.fn(), - requestHeartbeatNow: vi.fn(), runIsolatedAgentJob, sendCronFailureAlert, }); @@ -109,17 +125,13 @@ describe("CronService failure alerts", () => { error: "timeout", })); - const cron = new CronService({ + const cron = createFailureAlertCron({ storePath: store.storePath, - cronEnabled: true, cronConfig: { failureAlert: { enabled: false, }, }, - log: noopLogger, - enqueueSystemEvent: vi.fn(), - requestHeartbeatNow: vi.fn(), runIsolatedAgentJob, sendCronFailureAlert, }); @@ -161,18 +173,14 @@ describe("CronService failure alerts", () => { error: "auth error", })); - const cron = new CronService({ + const cron = createFailureAlertCron({ storePath: store.storePath, - cronEnabled: true, cronConfig: { failureAlert: { enabled: true, after: 1, }, }, - log: noopLogger, - enqueueSystemEvent: vi.fn(), - requestHeartbeatNow: vi.fn(), runIsolatedAgentJob, sendCronFailureAlert, }); @@ -204,9 +212,8 @@ describe("CronService failure alerts", () => { error: "temporary upstream error", })); - const cron = new CronService({ + const cron = createFailureAlertCron({ storePath: store.storePath, - cronEnabled: true, cronConfig: { failureAlert: { enabled: true, @@ -215,9 +222,6 @@ describe("CronService failure alerts", () => { accountId: "global-account", }, }, - log: noopLogger, - enqueueSystemEvent: vi.fn(), - requestHeartbeatNow: vi.fn(), runIsolatedAgentJob, sendCronFailureAlert, }); diff --git a/src/cron/service.heartbeat-ok-summary-suppressed.test.ts b/src/cron/service.heartbeat-ok-summary-suppressed.test.ts index 7f0cdef19a7..3ae9fc7c758 100644 --- a/src/cron/service.heartbeat-ok-summary-suppressed.test.ts +++ b/src/cron/service.heartbeat-ok-summary-suppressed.test.ts @@ -6,52 +6,80 @@ import type { CronJob } from "./types.js"; const { logger, makeStorePath } = setupCronServiceSuite({ prefix: "cron-heartbeat-ok-suppressed", }); +type CronServiceParams = ConstructorParameters[0]; + +function createDueIsolatedAnnounceJob(params: { + id: string; + message: string; + now: number; +}): CronJob { + return { + id: params.id, + name: params.id, + enabled: true, + createdAtMs: params.now - 10_000, + updatedAtMs: params.now - 10_000, + schedule: { kind: "every", everyMs: 60_000 }, + sessionTarget: "isolated", + wakeMode: "now", + payload: { kind: "agentTurn", message: params.message }, + delivery: { mode: "announce" }, + state: { nextRunAtMs: params.now - 1 }, + }; +} + +function createCronServiceForSummary(params: { + storePath: string; + summary: string; + enqueueSystemEvent: CronServiceParams["enqueueSystemEvent"]; + requestHeartbeatNow: CronServiceParams["requestHeartbeatNow"]; +}) { + return new CronService({ + storePath: params.storePath, + cronEnabled: true, + log: logger, + enqueueSystemEvent: params.enqueueSystemEvent, + requestHeartbeatNow: params.requestHeartbeatNow, + runHeartbeatOnce: vi.fn(), + runIsolatedAgentJob: vi.fn(async () => ({ + status: "ok" as const, + summary: params.summary, + delivered: false, + deliveryAttempted: false, + })), + }); +} + +async function runScheduledCron(cron: CronService): Promise { + await cron.start(); + await vi.advanceTimersByTimeAsync(2_000); + await vi.advanceTimersByTimeAsync(1_000); + cron.stop(); +} describe("cron isolated job HEARTBEAT_OK summary suppression (#32013)", () => { it("does not enqueue HEARTBEAT_OK as a system event to the main session", async () => { const { storePath } = await makeStorePath(); const now = Date.now(); - const job: CronJob = { + const job = createDueIsolatedAnnounceJob({ id: "heartbeat-only-job", - name: "heartbeat-only-job", - enabled: true, - createdAtMs: now - 10_000, - updatedAtMs: now - 10_000, - schedule: { kind: "every", everyMs: 60_000 }, - sessionTarget: "isolated", - wakeMode: "now", - payload: { kind: "agentTurn", message: "Check if anything is new" }, - delivery: { mode: "announce" }, - state: { nextRunAtMs: now - 1 }, - }; + message: "Check if anything is new", + now, + }); await writeCronStoreSnapshot({ storePath, jobs: [job] }); const enqueueSystemEvent = vi.fn(); const requestHeartbeatNow = vi.fn(); - - const cron = new CronService({ + const cron = createCronServiceForSummary({ storePath, - cronEnabled: true, - log: logger, + summary: "HEARTBEAT_OK", enqueueSystemEvent, requestHeartbeatNow, - runHeartbeatOnce: vi.fn(), - // Simulate the isolated agent returning HEARTBEAT_OK — nothing to - // announce. The delivery was intentionally skipped. - runIsolatedAgentJob: vi.fn(async () => ({ - status: "ok" as const, - summary: "HEARTBEAT_OK", - delivered: false, - deliveryAttempted: false, - })), }); - await cron.start(); - await vi.advanceTimersByTimeAsync(2_000); - await vi.advanceTimersByTimeAsync(1_000); - cron.stop(); + await runScheduledCron(cron); // HEARTBEAT_OK should NOT leak into the main session as a system event. expect(enqueueSystemEvent).not.toHaveBeenCalled(); @@ -62,45 +90,24 @@ describe("cron isolated job HEARTBEAT_OK summary suppression (#32013)", () => { const { storePath } = await makeStorePath(); const now = Date.now(); - const job: CronJob = { + const job = createDueIsolatedAnnounceJob({ id: "real-summary-job", - name: "real-summary-job", - enabled: true, - createdAtMs: now - 10_000, - updatedAtMs: now - 10_000, - schedule: { kind: "every", everyMs: 60_000 }, - sessionTarget: "isolated", - wakeMode: "now", - payload: { kind: "agentTurn", message: "Check weather" }, - delivery: { mode: "announce" }, - state: { nextRunAtMs: now - 1 }, - }; + message: "Check weather", + now, + }); await writeCronStoreSnapshot({ storePath, jobs: [job] }); const enqueueSystemEvent = vi.fn(); const requestHeartbeatNow = vi.fn(); - - const cron = new CronService({ + const cron = createCronServiceForSummary({ storePath, - cronEnabled: true, - log: logger, + summary: "Weather update: sunny, 72°F", enqueueSystemEvent, requestHeartbeatNow, - runHeartbeatOnce: vi.fn(), - // Simulate real content that should be forwarded. - runIsolatedAgentJob: vi.fn(async () => ({ - status: "ok" as const, - summary: "Weather update: sunny, 72°F", - delivered: false, - deliveryAttempted: false, - })), }); - await cron.start(); - await vi.advanceTimersByTimeAsync(2_000); - await vi.advanceTimersByTimeAsync(1_000); - cron.stop(); + await runScheduledCron(cron); // Real summaries SHOULD be enqueued. expect(enqueueSystemEvent).toHaveBeenCalledWith( diff --git a/src/cron/service.store-migration.test.ts b/src/cron/service.store-migration.test.ts index e25a0cd7cb2..6322e11b284 100644 --- a/src/cron/service.store-migration.test.ts +++ b/src/cron/service.store-migration.test.ts @@ -27,6 +27,11 @@ function createStartedCron(storePath: string) { }; } +async function listJobById(cron: CronService, jobId: string) { + const jobs = await cron.list({ includeDisabled: true }); + return jobs.find((entry) => entry.id === jobId); +} + describe("CronService store migrations", () => { it("migrates legacy top-level agentTurn fields and initializes missing state", async () => { const store = await makeStorePath(); @@ -69,8 +74,7 @@ describe("CronService store migrations", () => { const status = await cron.status(); expect(status.enabled).toBe(true); - const jobs = await cron.list({ includeDisabled: true }); - const job = jobs.find((entry) => entry.id === "legacy-agentturn-job"); + const job = await listJobById(cron, "legacy-agentturn-job"); expect(job).toBeDefined(); expect(job?.state).toBeDefined(); expect(job?.sessionTarget).toBe("isolated"); @@ -137,8 +141,7 @@ describe("CronService store migrations", () => { const cron = await createStartedCron(store.storePath).start(); - const jobs = await cron.list({ includeDisabled: true }); - const job = jobs.find((entry) => entry.id === "legacy-agentturn-no-timeout"); + const job = await listJobById(cron, "legacy-agentturn-no-timeout"); expect(job).toBeDefined(); expect(job?.payload.kind).toBe("agentTurn"); if (job?.payload.kind === "agentTurn") { @@ -177,8 +180,7 @@ describe("CronService store migrations", () => { ); const cron = await createStartedCron(store.storePath).start(); - const jobs = await cron.list({ includeDisabled: true }); - const job = jobs.find((entry) => entry.id === "legacy-cron-field-job"); + const job = await listJobById(cron, "legacy-cron-field-job"); expect(job).toBeDefined(); expect(job?.wakeMode).toBe("now"); expect(job?.schedule.kind).toBe("cron"); diff --git a/src/cron/types.ts b/src/cron/types.ts index 7e88e76ecb3..ef5de924b02 100644 --- a/src/cron/types.ts +++ b/src/cron/types.ts @@ -77,43 +77,34 @@ export type CronFailureAlert = { accountId?: string; }; -export type CronPayload = - | { kind: "systemEvent"; text: string } - | { - kind: "agentTurn"; - message: string; - /** Optional model override (provider/model or alias). */ - model?: string; - /** Optional per-job fallback models; overrides agent/global fallbacks when defined. */ - fallbacks?: string[]; - thinking?: string; - timeoutSeconds?: number; - allowUnsafeExternalContent?: boolean; - /** If true, run with lightweight bootstrap context. */ - lightContext?: boolean; - deliver?: boolean; - channel?: CronMessageChannel; - to?: string; - bestEffortDeliver?: boolean; - }; +export type CronPayload = { kind: "systemEvent"; text: string } | CronAgentTurnPayload; -export type CronPayloadPatch = - | { kind: "systemEvent"; text?: string } - | { - kind: "agentTurn"; - message?: string; - model?: string; - fallbacks?: string[]; - thinking?: string; - timeoutSeconds?: number; - allowUnsafeExternalContent?: boolean; - /** If true, run with lightweight bootstrap context. */ - lightContext?: boolean; - deliver?: boolean; - channel?: CronMessageChannel; - to?: string; - bestEffortDeliver?: boolean; - }; +export type CronPayloadPatch = { kind: "systemEvent"; text?: string } | CronAgentTurnPayloadPatch; + +type CronAgentTurnPayloadFields = { + message: string; + /** Optional model override (provider/model or alias). */ + model?: string; + /** Optional per-job fallback models; overrides agent/global fallbacks when defined. */ + fallbacks?: string[]; + thinking?: string; + timeoutSeconds?: number; + allowUnsafeExternalContent?: boolean; + /** If true, run with lightweight bootstrap context. */ + lightContext?: boolean; + deliver?: boolean; + channel?: CronMessageChannel; + to?: string; + bestEffortDeliver?: boolean; +}; + +type CronAgentTurnPayload = { + kind: "agentTurn"; +} & CronAgentTurnPayloadFields; + +type CronAgentTurnPayloadPatch = { + kind: "agentTurn"; +} & Partial; export type CronJobState = { nextRunAtMs?: number; diff --git a/src/hooks/install.ts b/src/hooks/install.ts index a41651e4338..38992d33c73 100644 --- a/src/hooks/install.ts +++ b/src/hooks/install.ts @@ -3,11 +3,15 @@ import path from "node:path"; import { MANIFEST_KEY } from "../compat/legacy-names.js"; import { fileExists, readJsonFile, resolveArchiveKind } from "../infra/archive.js"; import { resolveExistingInstallPath, withExtractedArchiveRoot } from "../infra/install-flow.js"; +import { installFromValidatedNpmSpecArchive } from "../infra/install-from-npm-spec.js"; import { resolveInstallModeOptions, resolveTimedInstallModeOptions, } from "../infra/install-mode-options.js"; -import { installPackageDir } from "../infra/install-package-dir.js"; +import { + installPackageDir, + installPackageDirWithManifestDeps, +} from "../infra/install-package-dir.js"; import { resolveSafeInstallDir, unscopedPackageName } from "../infra/install-safe-path.js"; import { type NpmIntegrityDrift, @@ -18,11 +22,6 @@ import { ensureInstallTargetAvailable, resolveCanonicalInstallTarget, } from "../infra/install-target.js"; -import { - finalizeNpmSpecArchiveInstall, - installFromNpmSpecArchiveWithInstaller, -} from "../infra/npm-pack-install.js"; -import { validateRegistryNpmSpec } from "../infra/npm-registry-spec.js"; import { isPathInside, isPathInsideWithRealpath } from "../security/scan-paths.js"; import { CONFIG_DIR, resolveUserPath } from "../utils.js"; import { parseFrontmatter } from "./frontmatter.js"; @@ -231,17 +230,15 @@ async function installHookPackageFromDir(params: { }; } - const deps = manifest.dependencies ?? {}; - const hasDeps = Object.keys(deps).length > 0; - const installRes = await installPackageDir({ + const installRes = await installPackageDirWithManifestDeps({ sourceDir: params.packageDir, targetDir, mode, timeoutMs, logger, copyErrorPrefix: "failed to copy hook pack", - hasDeps, depsLogMessage: "Installing hook pack dependencies…", + manifestDependencies: manifest.dependencies, }); if (!installRes.ok) { return installRes; @@ -376,14 +373,10 @@ export async function installHooksFromNpmSpec(params: { }): Promise { const { logger, timeoutMs, mode, dryRun } = resolveTimedInstallModeOptions(params, defaultLogger); const expectedHookPackId = params.expectedHookPackId; - const spec = params.spec.trim(); - const specError = validateRegistryNpmSpec(spec); - if (specError) { - return { ok: false, error: specError }; - } + const spec = params.spec; - logger.info?.(`Downloading ${spec}…`); - const flowResult = await installFromNpmSpecArchiveWithInstaller({ + logger.info?.(`Downloading ${spec.trim()}…`); + return await installFromValidatedNpmSpecArchive({ tempDirPrefix: "openclaw-hook-pack-", spec, timeoutMs, @@ -402,7 +395,6 @@ export async function installHooksFromNpmSpec(params: { expectedHookPackId, }, }); - return finalizeNpmSpecArchiveInstall(flowResult); } export async function installHooksFromPath(params: { diff --git a/src/hooks/workspace.test.ts b/src/hooks/workspace.test.ts index dc3de2acd9f..7d89919d5f2 100644 --- a/src/hooks/workspace.test.ts +++ b/src/hooks/workspace.test.ts @@ -5,6 +5,22 @@ import { describe, expect, it } from "vitest"; import { MANIFEST_KEY } from "../compat/legacy-names.js"; import { loadHookEntriesFromDir } from "./workspace.js"; +function writeHookPackageManifest(pkgDir: string, hooks: string[]): void { + fs.writeFileSync( + path.join(pkgDir, "package.json"), + JSON.stringify( + { + name: "pkg", + [MANIFEST_KEY]: { + hooks, + }, + }, + null, + 2, + ), + ); +} + describe("hooks workspace", () => { it("ignores package.json hook paths that traverse outside package directory", () => { const root = fs.mkdtempSync(path.join(os.tmpdir(), "openclaw-hooks-workspace-")); @@ -19,19 +35,7 @@ describe("hooks workspace", () => { fs.writeFileSync(path.join(outsideHookDir, "HOOK.md"), "---\nname: outside\n---\n"); fs.writeFileSync(path.join(outsideHookDir, "handler.js"), "export default async () => {};\n"); - fs.writeFileSync( - path.join(pkgDir, "package.json"), - JSON.stringify( - { - name: "pkg", - [MANIFEST_KEY]: { - hooks: ["../outside"], - }, - }, - null, - 2, - ), - ); + writeHookPackageManifest(pkgDir, ["../outside"]); const entries = loadHookEntriesFromDir({ dir: hooksRoot, source: "openclaw-workspace" }); expect(entries.some((e) => e.hook.name === "outside")).toBe(false); @@ -49,19 +53,7 @@ describe("hooks workspace", () => { fs.writeFileSync(path.join(nested, "HOOK.md"), "---\nname: nested\n---\n"); fs.writeFileSync(path.join(nested, "handler.js"), "export default async () => {};\n"); - fs.writeFileSync( - path.join(pkgDir, "package.json"), - JSON.stringify( - { - name: "pkg", - [MANIFEST_KEY]: { - hooks: ["./nested"], - }, - }, - null, - 2, - ), - ); + writeHookPackageManifest(pkgDir, ["./nested"]); const entries = loadHookEntriesFromDir({ dir: hooksRoot, source: "openclaw-workspace" }); expect(entries.some((e) => e.hook.name === "nested")).toBe(true); @@ -85,19 +77,7 @@ describe("hooks workspace", () => { return; } - fs.writeFileSync( - path.join(pkgDir, "package.json"), - JSON.stringify( - { - name: "pkg", - [MANIFEST_KEY]: { - hooks: ["./linked"], - }, - }, - null, - 2, - ), - ); + writeHookPackageManifest(pkgDir, ["./linked"]); const entries = loadHookEntriesFromDir({ dir: hooksRoot, source: "openclaw-workspace" }); expect(entries.some((e) => e.hook.name === "outside")).toBe(false);