From 79532b5a4e05251194b3d8f3ded8df98be14c2d4 Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Tue, 25 Aug 2026 16:03:55 -0700 Subject: [PATCH] refactor(plugins): remove obsolete compaction registry facade (#129565) * refactor(plugins): remove obsolete compaction registry facade Amp-Thread-ID: https://ampcode.com/threads/T-01a037b7-827c-72ea-b58b-6fef778d08fe * test(plugins): reset runtime after compaction cases Amp-Thread-ID: https://ampcode.com/threads/T-01a03a1a-d1fb-75ef-a904-7083040e37f1 --------- Co-authored-by: Amp --- config/knip.config.ts | 1 - .../agent-hooks/compaction-safeguard.test.ts | 29 +-- src/plugins/compaction-provider.test.ts | 197 ++++++------------ src/plugins/compaction-provider.ts | 54 +---- 4 files changed, 83 insertions(+), 198 deletions(-) diff --git a/config/knip.config.ts b/config/knip.config.ts index 7265d6842828..a8f7eb488858 100644 --- a/config/knip.config.ts +++ b/config/knip.config.ts @@ -413,7 +413,6 @@ const config = { // Runtime reason values are exported now so protocol schemas can derive from one tuple later. "src/agents/failover/signal.ts": ["exports"], "src/context-engine/registry.ts": ["exports", "types"], - "src/plugins/compaction-provider.ts": ["exports"], "src/plugins/interactive-registry.ts": ["exports"], "src/plugins/memory-state.ts": ["exports", "types"], "src/plugins/session-discussion-registry.ts": ["exports"], diff --git a/src/agents/agent-hooks/compaction-safeguard.test.ts b/src/agents/agent-hooks/compaction-safeguard.test.ts index ffc2f2a2996e..2a0ccd892720 100644 --- a/src/agents/agent-hooks/compaction-safeguard.test.ts +++ b/src/agents/agent-hooks/compaction-safeguard.test.ts @@ -8,10 +8,11 @@ import { createAssistantMessageEventStream, type Model } from "openclaw/plugin-s import { createRequireRecord } from "openclaw/plugin-sdk/test-fixtures"; import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; import type { OpenClawConfig } from "../../config/config.js"; +import type { CompactionProvider } from "../../plugins/compaction-provider.js"; import { - clearCompactionProviders, - registerCompactionProvider, -} from "../../plugins/compaction-provider.js"; + requireActivePluginRegistry, + resetPluginRuntimeStateForTest, +} from "../../plugins/runtime.js"; import * as compactionModule from "../compaction.js"; import { buildEmbeddedExtensionFactories } from "../embedded-agent-runner/extensions.js"; import { castAgentMessage } from "../test-helpers/agent-message-fixtures.js"; @@ -136,9 +137,13 @@ beforeEach(() => { afterEach(() => { testing.setSummarizeInStagesForTest(); - clearCompactionProviders(); + resetPluginRuntimeStateForTest(); }); +function installCompactionProviderForTest(provider: CompactionProvider): void { + requireActivePluginRegistry().compactionProviders.push({ provider }); +} + function stubSessionManager(): ExtensionContext["sessionManager"] { const stub: ExtensionContext["sessionManager"] = { getCwd: () => "/stub", @@ -2897,7 +2902,7 @@ describe("compaction-safeguard recent-turn preservation", () => { name: "AbortError", }); const failingProviderSummarize = vi.fn().mockRejectedValue(providerAbortErr); - registerCompactionProvider({ + installCompactionProviderForTest({ id: "disconnecting-provider", label: "Disconnecting Provider", summarize: failingProviderSummarize, @@ -2944,7 +2949,7 @@ describe("compaction-safeguard recent-turn preservation", () => { name: "AbortError", }); const failingProviderSummarize = vi.fn().mockRejectedValue(providerAbortErr); - registerCompactionProvider({ + installCompactionProviderForTest({ id: "aborted-provider", label: "Aborted Provider", summarize: failingProviderSummarize, @@ -2988,7 +2993,7 @@ describe("compaction-safeguard recent-turn preservation", () => { it("passes compaction instructions to providers and preserves suffix context", async () => { mockSummarizeInStages.mockReset(); const providerSummarize = vi.fn().mockResolvedValue("provider summary body"); - registerCompactionProvider({ + installCompactionProviderForTest({ id: "test-provider", label: "Test Provider", summarize: providerSummarize, @@ -3063,7 +3068,7 @@ describe("compaction-safeguard recent-turn preservation", () => { it("preserves an above-half provider body byte-for-byte when the joined artifact fits", async () => { const providerBody = `BODY-START${"b".repeat(4_480)}BODY-MIDDLE${"b".repeat(4_480)}BODY-END`; const providerSummarize = vi.fn().mockResolvedValue(providerBody); - registerCompactionProvider({ + installCompactionProviderForTest({ id: "within-budget-provider", label: "Within Budget Provider", summarize: providerSummarize, @@ -3102,7 +3107,7 @@ describe("compaction-safeguard recent-turn preservation", () => { it("emits one redacted provider warning when the preserved-turn producer truncates", async () => { const sensitiveSentinel = "preserved-secret-never-log"; const providerSummarize = vi.fn().mockResolvedValue("provider summary body"); - registerCompactionProvider({ + installCompactionProviderForTest({ id: "preserved-overflow-provider", label: "Preserved Overflow Provider", summarize: providerSummarize, @@ -3147,7 +3152,7 @@ describe("compaction-safeguard recent-turn preservation", () => { const sensitiveSentinel = "credential-sentinel-never-log"; const providerBody = `BODY-START${"b".repeat(3_400)}BODY-MIDDLE${"b".repeat(3_400)}BODY-END`; const providerSummarize = vi.fn().mockResolvedValue(providerBody); - registerCompactionProvider({ + installCompactionProviderForTest({ id: "overflow-provider", label: "Overflow Provider", summarize: providerSummarize, @@ -3201,7 +3206,7 @@ describe("compaction-safeguard recent-turn preservation", () => { it("starts a finally trimmed raw split-turn suffix at a complete message boundary", async () => { const providerBody = `BODY-START${"b".repeat(6_760)}BODY-END`; const providerSummarize = vi.fn().mockResolvedValue(providerBody); - registerCompactionProvider({ + installCompactionProviderForTest({ id: "boundary-provider", label: "Boundary Provider", summarize: providerSummarize, @@ -3248,7 +3253,7 @@ describe("compaction-safeguard recent-turn preservation", () => { it("finally trims a raw tool interaction only at its atomic boundary", async () => { const providerSummarize = vi.fn().mockResolvedValue(`BODY-START${"b".repeat(9_000)}BODY-END`); - registerCompactionProvider({ + installCompactionProviderForTest({ id: "tool-boundary-provider", label: "Tool Boundary Provider", summarize: providerSummarize, diff --git a/src/plugins/compaction-provider.test.ts b/src/plugins/compaction-provider.test.ts index 81f6978518bf..cce782d26048 100644 --- a/src/plugins/compaction-provider.test.ts +++ b/src/plugins/compaction-provider.test.ts @@ -1,20 +1,38 @@ -/** Covers plugin compaction provider registration and lookup behavior. */ +/** Covers canonical plugin compaction provider registration and runtime lookup. */ import { afterEach, describe, expect, it } from "vitest"; -import { - clearCompactionProviders, - getCompactionProvider, - getRegisteredCompactionProvider, - listRegisteredCompactionProviders, - registerCompactionProvider, - type CompactionProvider, -} from "./compaction-provider.js"; -import { createEmptyPluginRegistry } from "./registry-empty.js"; -import { setActivePluginRegistry, withPluginRegistrationContext } from "./runtime.js"; +import { getCompactionProvider, type CompactionProvider } from "./compaction-provider.js"; +import { createPluginRecord } from "./loader-records.js"; +import { createPluginRegistry } from "./registry.js"; +import { resetPluginRuntimeStateForTest, setActivePluginRegistry } from "./runtime.js"; +import type { PluginRuntime } from "./runtime/types.js"; afterEach(() => { - clearCompactionProviders(); + resetPluginRuntimeStateForTest(); }); +function createTestRegistry() { + return createPluginRegistry({ + logger: { + info() {}, + warn() {}, + error() {}, + debug() {}, + }, + runtime: {} as PluginRuntime, + activateGlobalSideEffects: false, + }); +} + +function createRecord(id: string) { + return createPluginRecord({ + id, + source: `/plugins/${id}/index.ts`, + origin: "global", + enabled: true, + configSchema: false, + }); +} + function makeProvider(id: string, label?: string): CompactionProvider { return { id, @@ -25,134 +43,43 @@ function makeProvider(id: string, label?: string): CompactionProvider { }; } -function requireCompactionProvider(id: string): CompactionProvider { - const provider = getCompactionProvider(id); - if (!provider) { - throw new Error(`Expected compaction provider ${id}`); - } - return provider; -} - -function listCompactionProviderIdsForTest(): string[] { - return listRegisteredCompactionProviders().map((entry) => entry.provider.id); -} - describe("compaction provider registry", () => { - it("starts empty", () => { - expect(listCompactionProviderIdsForTest()).toStrictEqual([]); - expect(listRegisteredCompactionProviders()).toStrictEqual([]); - }); + it("reads providers registered through the plugin API from the active registry", async () => { + const pluginRegistry = createTestRegistry(); + const provider = makeProvider("owned"); + pluginRegistry + .createApi(createRecord("owner"), { config: {} }) + .registerCompactionProvider(provider); + setActivePluginRegistry(pluginRegistry.registry); - it("returns undefined for an unknown id", () => { - expect(getCompactionProvider("nonexistent")).toBeUndefined(); - expect(getRegisteredCompactionProvider("nonexistent")).toBeUndefined(); - }); - - it("registers and retrieves a provider", () => { - const p = makeProvider("test-compactor"); - registerCompactionProvider(p); - - expect(getCompactionProvider("test-compactor")).toBe(p); - }); - - it("tracks ownerPluginId", () => { - const p = makeProvider("owned"); - registerCompactionProvider(p, { ownerPluginId: "my-plugin" }); - - const entry = getRegisteredCompactionProvider("owned"); - expect(entry?.provider).toBe(p); - expect(entry?.ownerPluginId).toBe("my-plugin"); - }); - - it("writes direct registration helpers into the synchronous builder context", () => { - const active = createEmptyPluginRegistry(); - const building = createEmptyPluginRegistry(); - setActivePluginRegistry(active); - const provider = makeProvider("builder-owned"); - - withPluginRegistrationContext(building, "builder-plugin", () => { - registerCompactionProvider(provider); - }); - - expect(active.compactionProviders).toStrictEqual([]); - expect(building.compactionProviders).toEqual([{ provider, ownerPluginId: "builder-plugin" }]); - }); - - it("does not let a registering plugin displace another owner's provider", () => { - const building = createEmptyPluginRegistry(); - const original = makeProvider("shared", "original"); - building.compactionProviders.push({ provider: original, ownerPluginId: "first-plugin" }); - - expect(() => - withPluginRegistrationContext(building, "failing-plugin", () => { - registerCompactionProvider(makeProvider("shared", "replacement")); - }), - ).toThrow("compaction provider shared already registered by first-plugin"); - expect(building.compactionProviders).toEqual([ - { provider: original, ownerPluginId: "first-plugin" }, + expect(pluginRegistry.registry.compactionProviders).toEqual([ + { provider, ownerPluginId: "owner" }, ]); + await expect(getCompactionProvider("owned")?.summarize({ messages: [] })).resolves.toBe( + "summary-from-owned", + ); }); - it("lists registered provider ids", () => { - registerCompactionProvider(makeProvider("alpha")); - registerCompactionProvider(makeProvider("beta")); + it("keeps the first provider when another plugin registers the same id", () => { + const pluginRegistry = createTestRegistry(); + const first = makeProvider("shared", "first"); + const second = makeProvider("shared", "second"); + pluginRegistry + .createApi(createRecord("first-owner"), { config: {} }) + .registerCompactionProvider(first); + pluginRegistry + .createApi(createRecord("second-owner"), { config: {} }) + .registerCompactionProvider(second); - expect(listCompactionProviderIdsForTest()).toEqual(["alpha", "beta"]); - }); - - it("lists registered entries with owner metadata", () => { - registerCompactionProvider(makeProvider("a"), { ownerPluginId: "plugin-a" }); - registerCompactionProvider(makeProvider("b")); - - const entries = listRegisteredCompactionProviders(); - expect(entries).toHaveLength(2); - expect(entries[0]?.provider.id).toBe("a"); - expect(entries[0]?.ownerPluginId).toBe("plugin-a"); - expect(entries[1]?.provider.id).toBe("b"); - expect(entries[1]?.ownerPluginId).toBeUndefined(); - }); - - it("supports multiple providers", () => { - registerCompactionProvider(makeProvider("a")); - registerCompactionProvider(makeProvider("b")); - registerCompactionProvider(makeProvider("c")); - - expect(getCompactionProvider("a")?.id).toBe("a"); - expect(getCompactionProvider("b")?.id).toBe("b"); - expect(getCompactionProvider("c")?.id).toBe("c"); - expect(listCompactionProviderIdsForTest()).toHaveLength(3); - }); - - it("calls summarize and returns expected result", async () => { - registerCompactionProvider(makeProvider("my-compactor")); - - const provider = requireCompactionProvider("my-compactor"); - const result = await provider.summarize({ messages: [] }); - - expect(result).toBe("summary-from-my-compactor"); - }); - - it("overwrites when re-registering the same id", () => { - const first = makeProvider("dup", "first-label"); - const second = makeProvider("dup", "second-label"); - - registerCompactionProvider(first); - registerCompactionProvider(second); - - expect(getCompactionProvider("dup")).toBe(second); - expect(getCompactionProvider("dup")?.label).toBe("second-label"); - expect(listCompactionProviderIdsForTest()).toEqual(["dup"]); - }); - - describe("lifecycle", () => { - it("clear removes all providers", () => { - registerCompactionProvider(makeProvider("a")); - registerCompactionProvider(makeProvider("b")); - expect(listCompactionProviderIdsForTest()).toHaveLength(2); - - clearCompactionProviders(); - expect(listCompactionProviderIdsForTest()).toStrictEqual([]); - expect(getCompactionProvider("a")).toBeUndefined(); - }); + expect(pluginRegistry.registry.compactionProviders).toEqual([ + { provider: first, ownerPluginId: "first-owner" }, + ]); + expect(pluginRegistry.registry.diagnostics).toContainEqual( + expect.objectContaining({ + level: "error", + pluginId: "second-owner", + message: "compaction provider already registered: shared (owner: first-owner)", + }), + ); }); }); diff --git a/src/plugins/compaction-provider.ts b/src/plugins/compaction-provider.ts index ee6e228033f6..c7d5b0efc94c 100644 --- a/src/plugins/compaction-provider.ts +++ b/src/plugins/compaction-provider.ts @@ -1,55 +1,9 @@ -import type { - CompactionProvider, - RegisteredCompactionProvider, -} from "./registry-contribution-types.js"; -import { - assertDirectPluginRegistrationReplacement, - requireActivePluginRegistry, - resolveDirectPluginRegistrationOwner, -} from "./runtime.js"; +import type { CompactionProvider } from "./registry-contribution-types.js"; +import { requireActivePluginRegistry } from "./runtime.js"; export type { CompactionProvider } from "./registry-contribution-types.js"; -const getProviders = () => requireActivePluginRegistry().compactionProviders; - -export function registerCompactionProvider( - provider: CompactionProvider, - options?: { ownerPluginId?: string }, -): void { - const providers = getProviders(); - const ownerPluginId = resolveDirectPluginRegistrationOwner(options?.ownerPluginId); - const entry = { - provider, - ownerPluginId, - }; - const index = providers.findIndex((registered) => registered.provider.id === provider.id); - if (index !== -1) { - assertDirectPluginRegistrationReplacement( - providers[index]?.ownerPluginId, - `compaction provider ${provider.id}`, - ); - } - if (index === -1) { - providers.push(entry); - } else { - providers.splice(index, 1, entry); - } -} - export function getCompactionProvider(id: string): CompactionProvider | undefined { - return getProviders().find((entry) => entry.provider.id === id)?.provider; -} - -export function getRegisteredCompactionProvider( - id: string, -): RegisteredCompactionProvider | undefined { - return getProviders().find((entry) => entry.provider.id === id); -} - -export function listRegisteredCompactionProviders(): RegisteredCompactionProvider[] { - return [...getProviders()]; -} - -export function clearCompactionProviders(): void { - getProviders().length = 0; + return requireActivePluginRegistry().compactionProviders.find((entry) => entry.provider.id === id) + ?.provider; }