From 262e5c57c8ff3d1424667f7d72af18cee6d0d953 Mon Sep 17 00:00:00 2001 From: Tak Hoffman <781889+Takhoffman@users.noreply.github.com> Date: Fri, 27 Mar 2026 19:44:15 -0500 Subject: [PATCH] fix(ci): stabilize module-bound exact regressions (#56085) * Adjust compaction identifier test for summary args * Harden exec completion after child exit * Handle SDK compaction and skill shape drift * Stabilize Synology Chat module-bound tests * Restore skill source compatibility shims * Restore self-hosted provider discovery mocks --- .../synology-chat/src/channel.test-mocks.ts | 1 + extensions/synology-chat/src/channel.test.ts | 7 +-- .../synology-chat/src/webhook-handler.ts | 8 +-- ...compaction.identifier-preservation.test.ts | 26 +++++++- src/agents/compaction.retry.test.ts | 13 +++- src/agents/compaction.ts | 63 ++++++++++++++++++- src/agents/skills-install.download.test.ts | 24 ++++++- src/agents/skills-status.test.ts | 24 ++++++- .../skills.buildworkspaceskillstatus.test.ts | 24 ++++++- .../skills.resolveskillspromptforrun.test.ts | 24 ++++++- src/agents/skills/compact-format.test.ts | 4 +- src/agents/skills/source.ts | 10 ++- src/cli/skills-cli.formatting.test.ts | 24 ++++++- src/process/exec.ts | 37 +++++++++-- .../extensions/provider-discovery-contract.ts | 14 +++++ 15 files changed, 267 insertions(+), 36 deletions(-) diff --git a/extensions/synology-chat/src/channel.test-mocks.ts b/extensions/synology-chat/src/channel.test-mocks.ts index 3e86c36c5b1..3fe52e1a699 100644 --- a/extensions/synology-chat/src/channel.test-mocks.ts +++ b/extensions/synology-chat/src/channel.test-mocks.ts @@ -92,6 +92,7 @@ vi.mock("./runtime.js", () => ({ }, }, })), + setSynologyRuntime: vi.fn(), })); export function makeSecurityAccount( diff --git a/extensions/synology-chat/src/channel.test.ts b/extensions/synology-chat/src/channel.test.ts index 03ddb7254f6..37af16afc82 100644 --- a/extensions/synology-chat/src/channel.test.ts +++ b/extensions/synology-chat/src/channel.test.ts @@ -34,7 +34,8 @@ vi.mock("./webhook-handler.js", () => ({ createWebhookHandler: vi.fn(() => vi.fn()), })); -const { createSynologyChatPlugin } = await import("./channel.js"); +const freshChannelModulePath = "./channel.js?channel-test"; +const { createSynologyChatPlugin } = await import(freshChannelModulePath); describe("createSynologyChatPlugin", () => { beforeEach(() => { @@ -548,19 +549,15 @@ describe("createSynologyChatPlugin", () => { abortSignal: abortCtrl.signal, }); - // Start first account (returns a pending promise) const firstPromise = plugin.gateway.startAccount(makeCtx(abortFirst)); - // Start second account on same path — should deregister the first route const secondPromise = plugin.gateway.startAccount(makeCtx(abortSecond)); - // Give microtasks time to settle await new Promise((r) => setTimeout(r, 10)); expect(registerMock).toHaveBeenCalledTimes(2); expect(unregisterFirst).not.toHaveBeenCalled(); expect(unregisterSecond).not.toHaveBeenCalled(); - // Clean up: abort both to resolve promises and prevent test leak abortFirst.abort(); abortSecond.abort(); await Promise.allSettled([firstPromise, secondPromise]); diff --git a/extensions/synology-chat/src/webhook-handler.ts b/extensions/synology-chat/src/webhook-handler.ts index 5409c02b214..2e4a9a50989 100644 --- a/extensions/synology-chat/src/webhook-handler.ts +++ b/extensions/synology-chat/src/webhook-handler.ts @@ -10,7 +10,7 @@ import { readRequestBodyWithLimit, requestBodyErrorToText, } from "openclaw/plugin-sdk/webhook-ingress"; -import { sendMessage, resolveLegacyWebhookNameToChatUserId } from "./client.js"; +import * as synologyClient from "./client.js"; import { validateToken, authorizeUserForDm, sanitizeInput, RateLimiter } from "./security.js"; import type { SynologyWebhookPayload, ResolvedSynologyChatAccount } from "./types.js"; @@ -481,7 +481,7 @@ async function resolveSynologyReplyDeliveryUserId(params: { return params.payload.user_id; } - const resolvedChatApiUserId = await resolveLegacyWebhookNameToChatUserId({ + const resolvedChatApiUserId = await synologyClient.resolveLegacyWebhookNameToChatUserId({ incomingUrl: params.account.incomingUrl, mutableWebhookUsername: params.payload.username, allowInsecureSsl: params.account.allowInsecureSsl, @@ -529,7 +529,7 @@ async function processAuthorizedSynologyWebhook(params: { return; } - await sendMessage( + await synologyClient.sendMessage( params.account.incomingUrl, reply, deliveryUserId, @@ -544,7 +544,7 @@ async function processAuthorizedSynologyWebhook(params: { params.log?.error?.( `Failed to process message from ${params.message.payload.username}: ${errMsg}`, ); - await sendMessage( + await synologyClient.sendMessage( params.account.incomingUrl, "Sorry, an error occurred while processing your message.", deliveryUserId, diff --git a/src/agents/compaction.identifier-preservation.test.ts b/src/agents/compaction.identifier-preservation.test.ts index 67ffdbc7037..075899de858 100644 --- a/src/agents/compaction.identifier-preservation.test.ts +++ b/src/agents/compaction.identifier-preservation.test.ts @@ -65,7 +65,7 @@ describe("compaction identifier-preservation instructions", () => { } function firstSummaryInstructions() { - return mockGenerateSummary.mock.calls[0]?.[6]; + return extractSummaryInstructions(mockGenerateSummary.mock.calls[0]); } it("injects identifier-preservation guidance even without custom instructions", async () => { @@ -101,7 +101,9 @@ describe("compaction identifier-preservation instructions", () => { expect(mockGenerateSummary.mock.calls.length).toBeGreaterThan(1); for (const call of mockGenerateSummary.mock.calls) { - expect(call[6]).toContain("Preserve all opaque identifiers exactly as written"); + expect(extractSummaryInstructions(call)).toContain( + "Preserve all opaque identifiers exactly as written", + ); } }); @@ -114,13 +116,31 @@ describe("compaction identifier-preservation instructions", () => { }); const mergedCall = mockGenerateSummary.mock.calls.at(-1); - const instructions = mergedCall?.[6] ?? ""; + const instructions = extractSummaryInstructions(mergedCall); expect(instructions).toContain("Merge these partial summaries into a single cohesive summary."); expect(instructions).toContain("Prioritize customer-visible regressions."); expect((instructions.match(/Additional focus:/g) ?? []).length).toBe(1); }); }); +function extractSummaryInstructions(call: unknown[] | undefined): string { + if (!call) { + return ""; + } + for (let index = call.length - 1; index >= 4; index -= 1) { + const arg = call[index]; + if ( + typeof arg === "string" && + (arg.includes("Preserve all opaque identifiers exactly as written") || + arg.includes("Merge these partial summaries into a single cohesive summary.") || + arg.includes("Additional focus:")) + ) { + return arg; + } + } + return ""; +} + describe("buildCompactionSummarizationInstructions", () => { it("returns base instructions when no custom text is provided", () => { const result = buildCompactionSummarizationInstructions(); diff --git a/src/agents/compaction.retry.test.ts b/src/agents/compaction.retry.test.ts index 31404e2e9b2..4234bd236da 100644 --- a/src/agents/compaction.retry.test.ts +++ b/src/agents/compaction.retry.test.ts @@ -15,6 +15,17 @@ vi.mock("@mariozechner/pi-coding-agent", async (importOriginal) => { }); const mockGenerateSummary = vi.mocked(piCodingAgent.generateSummary); +type MockGenerateSummaryCompat = ( + currentMessages: AgentMessage[], + model: NonNullable, + reserveTokens: number, + apiKey: string, + headers: Record | undefined, + signal?: AbortSignal, + customInstructions?: string, + previousSummary?: string, +) => Promise; +const mockGenerateSummaryCompat = mockGenerateSummary as unknown as MockGenerateSummaryCompat; describe("compaction retry integration", () => { beforeEach(() => { @@ -56,7 +67,7 @@ describe("compaction retry integration", () => { } as unknown as NonNullable; const invokeGenerateSummary = (signal = new AbortController().signal) => - mockGenerateSummary(testMessages, testModel, 1000, "test-api-key", signal); + mockGenerateSummaryCompat(testMessages, testModel, 1000, "test-api-key", undefined, signal); const runSummaryRetry = (options: Parameters[1]) => retryAsync(() => invokeGenerateSummary(), options); diff --git a/src/agents/compaction.ts b/src/agents/compaction.ts index faa5baf4a9e..12d269f67ec 100644 --- a/src/agents/compaction.ts +++ b/src/agents/compaction.ts @@ -1,6 +1,9 @@ import type { AgentMessage } from "@mariozechner/pi-agent-core"; import type { ExtensionContext } from "@mariozechner/pi-coding-agent"; -import { estimateTokens, generateSummary } from "@mariozechner/pi-coding-agent"; +import { + estimateTokens, + generateSummary as piGenerateSummary, +} from "@mariozechner/pi-coding-agent"; import type { AgentCompactionIdentifierPolicy } from "../config/types.agent-defaults.js"; import { retryAsync } from "../infra/retry.js"; import { createSubsystemLogger } from "../logging/subsystem.js"; @@ -37,6 +40,30 @@ export type CompactionSummarizationInstructions = { identifierInstructions?: string; }; +type GenerateSummaryCompat = { + ( + currentMessages: AgentMessage[], + model: NonNullable, + reserveTokens: number, + apiKey: string, + signal?: AbortSignal, + customInstructions?: string, + previousSummary?: string, + ): Promise; + ( + currentMessages: AgentMessage[], + model: NonNullable, + reserveTokens: number, + apiKey: string, + headers: Record | undefined, + signal?: AbortSignal, + customInstructions?: string, + previousSummary?: string, + ): Promise; +}; + +const generateSummaryCompat = piGenerateSummary as unknown as GenerateSummaryCompat; + function resolveIdentifierPreservationInstructions( instructions?: CompactionSummarizationInstructions, ): string | undefined { @@ -240,6 +267,7 @@ async function summarizeChunks(params: { params.model, params.reserveTokens, params.apiKey, + undefined, params.signal, effectiveInstructions, summary, @@ -258,6 +286,39 @@ async function summarizeChunks(params: { return summary ?? DEFAULT_SUMMARY_FALLBACK; } +function generateSummary( + currentMessages: AgentMessage[], + model: NonNullable, + reserveTokens: number, + apiKey: string, + headers: Record | undefined, + signal: AbortSignal, + customInstructions?: string, + previousSummary?: string, +): Promise { + if (piGenerateSummary.length >= 8) { + return generateSummaryCompat( + currentMessages, + model, + reserveTokens, + apiKey, + headers, + signal, + customInstructions, + previousSummary, + ); + } + return generateSummaryCompat( + currentMessages, + model, + reserveTokens, + apiKey, + signal, + customInstructions, + previousSummary, + ); +} + /** * Summarize with progressive fallback for handling oversized messages. * If full summarization fails, tries partial summarization excluding oversized messages. diff --git a/src/agents/skills-install.download.test.ts b/src/agents/skills-install.download.test.ts index bd8559b60fe..481978a9e2d 100644 --- a/src/agents/skills-install.download.test.ts +++ b/src/agents/skills-install.download.test.ts @@ -55,18 +55,36 @@ const TAR_GZ_TRAVERSAL_BUFFER = Buffer.from( function buildEntry(name: string): SkillEntry { const skillDir = path.join(workspaceDir, "skills", name); return { - skill: { + skill: createFixtureSkill({ name, description: `${name} test skill`, filePath: path.join(skillDir, "SKILL.md"), baseDir: skillDir, source: "openclaw-workspace", - disableModelInvocation: false, - }, + }), frontmatter: {}, }; } +function createFixtureSkill(params: { + name: string; + description: string; + filePath: string; + baseDir: string; + source: string; +}): SkillEntry["skill"] { + const skill = { + name: params.name, + description: params.description, + filePath: params.filePath, + baseDir: params.baseDir, + source: params.source, + sourceInfo: { source: params.source }, + disableModelInvocation: false, + }; + return skill as unknown as SkillEntry["skill"]; +} + function buildDownloadSpec(params: { url: string; archive: "tar.gz" | "tar.bz2" | "zip"; diff --git a/src/agents/skills-status.test.ts b/src/agents/skills-status.test.ts index 3e0b2ab6783..d42ba3fbf10 100644 --- a/src/agents/skills-status.test.ts +++ b/src/agents/skills-status.test.ts @@ -12,14 +12,13 @@ describe("buildWorkspaceSkillStatus", () => { const mismatchedOs = process.platform === "darwin" ? "linux" : "darwin"; const entry: SkillEntry = { - skill: { + skill: createFixtureSkill({ name: "os-scoped", description: "test", filePath: "/tmp/os-scoped", baseDir: "/tmp", source: "test", - disableModelInvocation: false, - }, + }), frontmatter: {}, metadata: { os: [mismatchedOs], @@ -41,3 +40,22 @@ describe("buildWorkspaceSkillStatus", () => { expect(report.skills[0]?.install).toEqual([]); }); }); + +function createFixtureSkill(params: { + name: string; + description: string; + filePath: string; + baseDir: string; + source: string; +}): SkillEntry["skill"] { + const skill = { + name: params.name, + description: params.description, + filePath: params.filePath, + baseDir: params.baseDir, + source: params.source, + sourceInfo: { source: params.source }, + disableModelInvocation: false, + }; + return skill as unknown as SkillEntry["skill"]; +} diff --git a/src/agents/skills.buildworkspaceskillstatus.test.ts b/src/agents/skills.buildworkspaceskillstatus.test.ts index 4b3cca8808f..ee32ff07cf7 100644 --- a/src/agents/skills.buildworkspaceskillstatus.test.ts +++ b/src/agents/skills.buildworkspaceskillstatus.test.ts @@ -19,14 +19,13 @@ function makeEntry(params: { }>; }): SkillEntry { return { - skill: { + skill: createFixtureSkill({ name: params.name, description: `desc:${params.name}`, filePath: `/tmp/${params.name}/SKILL.md`, baseDir: `/tmp/${params.name}`, source: params.source ?? "openclaw-workspace", - disableModelInvocation: false, - }, + }), frontmatter: {}, metadata: { ...(params.os ? { os: params.os } : {}), @@ -37,6 +36,25 @@ function makeEntry(params: { }; } +function createFixtureSkill(params: { + name: string; + description: string; + filePath: string; + baseDir: string; + source: string; +}): SkillEntry["skill"] { + const skill = { + name: params.name, + description: params.description, + filePath: params.filePath, + baseDir: params.baseDir, + source: params.source, + sourceInfo: { source: params.source }, + disableModelInvocation: false, + }; + return skill as unknown as SkillEntry["skill"]; +} + describe("buildWorkspaceSkillStatus", () => { it("reports missing requirements and install options", async () => { const entry = makeEntry({ diff --git a/src/agents/skills.resolveskillspromptforrun.test.ts b/src/agents/skills.resolveskillspromptforrun.test.ts index 305e11f2f4e..446757bedec 100644 --- a/src/agents/skills.resolveskillspromptforrun.test.ts +++ b/src/agents/skills.resolveskillspromptforrun.test.ts @@ -12,14 +12,13 @@ describe("resolveSkillsPromptForRun", () => { }); it("builds prompt from entries when snapshot is missing", () => { const entry: SkillEntry = { - skill: { + skill: createFixtureSkill({ name: "demo-skill", description: "Demo", filePath: "/app/skills/demo-skill/SKILL.md", baseDir: "/app/skills/demo-skill", source: "openclaw-bundled", - disableModelInvocation: false, - }, + }), frontmatter: {}, }; const prompt = resolveSkillsPromptForRun({ @@ -30,3 +29,22 @@ describe("resolveSkillsPromptForRun", () => { expect(prompt).toContain("/app/skills/demo-skill/SKILL.md"); }); }); + +function createFixtureSkill(params: { + name: string; + description: string; + filePath: string; + baseDir: string; + source: string; +}): SkillEntry["skill"] { + const skill = { + name: params.name, + description: params.description, + filePath: params.filePath, + baseDir: params.baseDir, + source: params.source, + sourceInfo: { source: params.source }, + disableModelInvocation: false, + }; + return skill as unknown as SkillEntry["skill"]; +} diff --git a/src/agents/skills/compact-format.test.ts b/src/agents/skills/compact-format.test.ts index cd9d6f42f15..8f44c0f6224 100644 --- a/src/agents/skills/compact-format.test.ts +++ b/src/agents/skills/compact-format.test.ts @@ -10,14 +10,16 @@ import { } from "./workspace.js"; function makeSkill(name: string, desc = "A skill", filePath = `/skills/${name}/SKILL.md`): Skill { - return { + const skill = { name, description: desc, filePath, baseDir: `/skills/${name}`, source: "workspace", + sourceInfo: { source: "workspace" }, disableModelInvocation: false, }; + return skill as unknown as Skill; } function makeEntry(skill: Skill): SkillEntry { diff --git a/src/agents/skills/source.ts b/src/agents/skills/source.ts index 076c10bb8ec..8ff52620d10 100644 --- a/src/agents/skills/source.ts +++ b/src/agents/skills/source.ts @@ -1,5 +1,13 @@ import type { Skill } from "@mariozechner/pi-coding-agent"; +type SkillSourceShapeCompat = Skill & { + source?: string; + sourceInfo?: { + source?: string; + }; +}; + export function resolveSkillSource(skill: Skill): string { - return skill.source; + const compatSkill = skill as SkillSourceShapeCompat; + return compatSkill.source ?? compatSkill.sourceInfo?.source ?? "unknown"; } diff --git a/src/cli/skills-cli.formatting.test.ts b/src/cli/skills-cli.formatting.test.ts index 7002bdc4aec..e7672c7962f 100644 --- a/src/cli/skills-cli.formatting.test.ts +++ b/src/cli/skills-cli.formatting.test.ts @@ -33,14 +33,13 @@ describe("skills-cli (e2e)", () => { const baseDir = path.join(tempWorkspaceDir, "peekaboo"); return [ { - skill: { + skill: createFixtureSkill({ name: "peekaboo", description: "Capture UI screenshots", filePath: path.join(baseDir, "SKILL.md"), baseDir, source: "openclaw-bundled", - disableModelInvocation: false, - }, + }), frontmatter: {}, metadata: { emoji: "📸" }, }, @@ -84,3 +83,22 @@ describe("skills-cli (e2e)", () => { expect(output).toContain("Details:"); }); }); + +function createFixtureSkill(params: { + name: string; + description: string; + filePath: string; + baseDir: string; + source: string; +}): SkillEntry["skill"] { + const skill = { + name: params.name, + description: params.description, + filePath: params.filePath, + baseDir: params.baseDir, + source: params.source, + sourceInfo: { source: params.source }, + disableModelInvocation: false, + }; + return skill as unknown as SkillEntry["skill"]; +} diff --git a/src/process/exec.ts b/src/process/exec.ts index 140582d541a..9af3c909a8b 100644 --- a/src/process/exec.ts +++ b/src/process/exec.ts @@ -246,6 +246,8 @@ export async function runCommandWithTimeout( let settled = false; let timedOut = false; let noOutputTimedOut = false; + let childExitState: { code: number | null; signal: NodeJS.Signals | null } | null = null; + let closeFallbackTimer: NodeJS.Timeout | null = null; let noOutputTimer: NodeJS.Timeout | null = null; const shouldTrackOutputTimeout = typeof noOutputTimeoutMs === "number" && @@ -260,6 +262,14 @@ export async function runCommandWithTimeout( noOutputTimer = null; }; + const clearCloseFallbackTimer = () => { + if (!closeFallbackTimer) { + return; + } + clearTimeout(closeFallbackTimer); + closeFallbackTimer = null; + }; + const armNoOutputTimer = () => { if (!shouldTrackOutputTimeout || settled) { return; @@ -304,8 +314,22 @@ export async function runCommandWithTimeout( settled = true; clearTimeout(timer); clearNoOutputTimer(); + clearCloseFallbackTimer(); reject(err); }); + child.on("exit", (code, signal) => { + childExitState = { code, signal }; + if (settled || closeFallbackTimer) { + return; + } + closeFallbackTimer = setTimeout(() => { + if (settled) { + return; + } + child.stdout?.destroy(); + child.stderr?.destroy(); + }, 250); + }); child.on("close", (code, signal) => { if (settled) { return; @@ -313,25 +337,28 @@ export async function runCommandWithTimeout( settled = true; clearTimeout(timer); clearNoOutputTimer(); + clearCloseFallbackTimer(); + const resolvedCode = childExitState?.code ?? code; + const resolvedSignal = childExitState?.signal ?? signal; const termination = noOutputTimedOut ? "no-output-timeout" : timedOut ? "timeout" - : signal != null + : resolvedSignal != null ? "signal" : "exit"; const normalizedCode = termination === "timeout" || termination === "no-output-timeout" - ? code === 0 + ? resolvedCode === 0 ? 124 - : code - : code; + : resolvedCode + : resolvedCode; resolve({ pid: child.pid ?? undefined, stdout, stderr, code: normalizedCode, - signal, + signal: resolvedSignal, killed: child.killed, termination, noOutputTimedOut, diff --git a/test/helpers/extensions/provider-discovery-contract.ts b/test/helpers/extensions/provider-discovery-contract.ts index 38522d7cee6..48cd854ebe6 100644 --- a/test/helpers/extensions/provider-discovery-contract.ts +++ b/test/helpers/extensions/provider-discovery-contract.ts @@ -149,6 +149,20 @@ function installDiscoveryHooks(state: DiscoveryState) { buildSglangProvider: (...args: unknown[]) => buildSglangProviderMock(...args), }; }); + vi.doMock("../../../extensions/vllm/api.js", async () => { + const actual = await vi.importActual("../../../extensions/vllm/api.js"); + return { + ...actual, + buildVllmProvider: (...args: unknown[]) => buildVllmProviderMock(...args), + }; + }); + vi.doMock("../../../extensions/sglang/api.js", async () => { + const actual = await vi.importActual("../../../extensions/sglang/api.js"); + return { + ...actual, + buildSglangProvider: (...args: unknown[]) => buildSglangProviderMock(...args), + }; + }); ({ runProviderCatalog: state.runProviderCatalog } = await import("../../../src/plugins/provider-discovery.js")); const [