From 531aeff15fffdebf8691303de75e76f8251b9432 Mon Sep 17 00:00:00 2001 From: roboomp Date: Tue, 14 Jul 2026 17:02:08 +0000 Subject: [PATCH 1/2] fix(session): fall back to LLM compaction on manual /compact for text-only models Manual /compact with the snapcompact strategy hard-threw when the active model lacked image input, unlike the auto-compaction path which downgrades to LLM-backed compaction. Clear snapcompactReady instead of throwing so the flow falls through to #compactWithFallbackModel (active text-only model tried first for text->text summarization). Fixes #5064 --- packages/coding-agent/CHANGELOG.md | 1 + .../coding-agent/src/session/agent-session.ts | 11 +- ...ession-manual-snapcompact-fallback.test.ts | 114 ++++++++++++++++++ 3 files changed, 121 insertions(+), 5 deletions(-) create mode 100644 packages/coding-agent/test/agent-session-manual-snapcompact-fallback.test.ts diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index 658e3a904..3507388c1 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -26,6 +26,7 @@ - Fixed subagent `yield` tool calls being discarded when the soft request budget hard-aborted the same assistant turn before the yield result event landed. ([#5006](https://github.com/can1357/oh-my-pi/issues/5006)) - Fixed `--tools` filtering in interactive sessions disabling deferred MCP tools; MCP tools discovered from configured servers now stay active when the flag limits only built-in tools. ([#5013](https://github.com/can1357/oh-my-pi/issues/5013)) - Fixed kept-alive task subagents entering a repeated provider-call loop after an IRC wake and terminal `yield`. ([#4963](https://github.com/can1357/oh-my-pi/issues/4963)) +- Fixed manual `/compact` with the snapcompact strategy hard-failing on text-only active models; it now warns and falls back to LLM compaction (mirroring the auto-compaction path) instead of throwing. ([#5064](https://github.com/can1357/oh-my-pi/issues/5064)) ## [16.3.15] - 2026-07-09 diff --git a/packages/coding-agent/src/session/agent-session.ts b/packages/coding-agent/src/session/agent-session.ts index a98f48521..efe15a03d 100644 --- a/packages/coding-agent/src/session/agent-session.ts +++ b/packages/coding-agent/src/session/agent-session.ts @@ -9799,23 +9799,24 @@ export class AgentSession { // Strategy honored on manual /compact too. Custom instructions (public // user focus OR internal plan-mode guidance) imply a directed LLM // summary; a text-only model cannot read snapcompact frames. When - // snapcompact itself was requested, fail locally instead of silently - // converting the "no LLM call" path into a provider-backed summary. + // snapcompact was requested but the active model is text-only, fall + // back to LLM-backed compaction (mirroring the auto-compaction path) + // instead of hard-failing the manual /compact (issue #5064). const wantsSnapcompact = compactionPrep.kind !== "fromHook" && effectiveSettings.strategy === "snapcompact" && !customInstructions && !options?.internalGuidance; - const snapcompactReady = wantsSnapcompact; + let snapcompactReady = wantsSnapcompact; const snapcompactShapeSetting = this.settings.get("snapcompact.shape"); let snapcompactShape: snapcompact.Shape | undefined; if (wantsSnapcompact && !this.model.input.includes("image")) { this.emitNotice( "warning", - `snapcompact needs a vision-capable model (${this.model.id} is text-only)`, + `snapcompact needs a vision-capable model (${this.model.id} is text-only); falling back to LLM compaction`, "compaction", ); - throw new Error(`snapcompact cannot run locally: ${this.model.id} is text-only.`); + snapcompactReady = false; } else if (snapcompactReady) { const text = snapcompact.serializeConversation( convertToLlm(preparation.messagesToSummarize.concat(preparation.turnPrefixMessages)), diff --git a/packages/coding-agent/test/agent-session-manual-snapcompact-fallback.test.ts b/packages/coding-agent/test/agent-session-manual-snapcompact-fallback.test.ts new file mode 100644 index 000000000..2355fe81d --- /dev/null +++ b/packages/coding-agent/test/agent-session-manual-snapcompact-fallback.test.ts @@ -0,0 +1,114 @@ +import { afterEach, describe, expect, it, vi } from "bun:test"; +import * as path from "node:path"; +import { Agent } from "@oh-my-pi/pi-agent-core"; +import * as compactionModule from "@oh-my-pi/pi-agent-core/compaction"; +import type { Message } from "@oh-my-pi/pi-ai"; +import { getBundledModel } from "@oh-my-pi/pi-catalog/models"; +import { ModelRegistry } from "@oh-my-pi/pi-coding-agent/config/model-registry"; +import { Settings } from "@oh-my-pi/pi-coding-agent/config/settings"; +import { AgentSession } from "@oh-my-pi/pi-coding-agent/session/agent-session"; +import { AuthStorage } from "@oh-my-pi/pi-coding-agent/session/auth-storage"; +import { SessionManager } from "@oh-my-pi/pi-coding-agent/session/session-manager"; +import { TempDir } from "@oh-my-pi/pi-utils"; + +/** + * Regression for issue #5064. + * + * Manual `/compact` with the default snapcompact strategy hard-threw + * ("snapcompact cannot run locally: is text-only") when the active model + * lacked image input, even though the auto-compaction path already downgraded + * to LLM-backed compaction in the same situation. The manual path MUST mirror + * that behavior: warn, then summarize via the LLM fallback candidate chain + * (which tries the active text→text model first). + */ +describe("AgentSession manual snapcompact text-only fallback", () => { + let session: AgentSession | undefined; + let authStorage: AuthStorage | undefined; + let tempDir: TempDir | undefined; + + afterEach(async () => { + try { + await session?.dispose(); + } finally { + authStorage?.close(); + await tempDir?.remove(); + vi.restoreAllMocks(); + session = undefined; + authStorage = undefined; + tempDir = undefined; + } + }); + + it("falls back to LLM compaction instead of throwing on a text-only active model", async () => { + const activeModel = getBundledModel("aimlapi", "alibaba/qwen3-coder-480b-a35b-instruct"); + if (!activeModel) throw new Error("Expected bundled text-only model"); + expect(activeModel.input).not.toContain("image"); + + tempDir = TempDir.createSync("@pi-manual-snapcompact-text-only-"); + authStorage = await AuthStorage.create(path.join(tempDir.path(), "auth.db")); + authStorage.setRuntimeApiKey("aimlapi", "test-key"); + const modelRegistry = new ModelRegistry(authStorage); + + const agent = new Agent({ + initialState: { model: activeModel, systemPrompt: ["Test"], tools: [], messages: [] }, + }); + const sessionManager = SessionManager.create(tempDir.path(), tempDir.path()); + const seed: Message[] = [ + { role: "user", content: "first question", timestamp: Date.now() }, + { + role: "assistant", + content: [{ type: "text", text: "first answer" }], + api: activeModel.api, + provider: activeModel.provider, + model: activeModel.id, + stopReason: "stop", + usage: { + input: 10, + output: 10, + cacheRead: 0, + cacheWrite: 0, + totalTokens: 20, + cost: { input: 0, output: 0, cacheRead: 0, cacheWrite: 0, total: 0 }, + }, + timestamp: Date.now(), + }, + { role: "user", content: "second question", timestamp: Date.now() }, + ]; + for (const message of seed) sessionManager.appendMessage(message); + const firstKeptEntryId = sessionManager.getBranch()[0]?.id; + if (!firstKeptEntryId) throw new Error("Expected seeded branch entry"); + + const settings = Settings.isolated({ + "compaction.strategy": "snapcompact", + "compaction.keepRecentTokens": 1, + }); + session = new AgentSession({ agent, sessionManager, settings, modelRegistry }); + const notices: string[] = []; + session.subscribe(event => { + if (event.type === "notice" && event.source === "compaction") notices.push(event.message); + }); + + const compactSpy = vi.spyOn(compactionModule, "compact").mockImplementation(async (preparation, model) => ({ + summary: "llm summary", + shortSummary: "llm", + firstKeptEntryId: preparation.firstKeptEntryId, + tokensBefore: 42, + details: { provider: model.provider, model: model.id }, + })); + + const result = await session.compact(); + + expect(result.summary).toBe("llm summary"); + // LLM fallback ran; the active text-only model is tried first. + expect(compactSpy).toHaveBeenCalled(); + const [, firstCandidate] = compactSpy.mock.calls[0]!; + expect(`${firstCandidate.provider}/${firstCandidate.id}`).toBe(`${activeModel.provider}/${activeModel.id}`); + expect(notices).toContain( + `snapcompact needs a vision-capable model (${activeModel.id} is text-only); falling back to LLM compaction`, + ); + expect(sessionManager.getBranch().find(entry => entry.type === "compaction")).toMatchObject({ + type: "compaction", + summary: "llm summary", + }); + }); +}); From c9032e57b5ca12f5d622e59d7310e58f2599416d Mon Sep 17 00:00:00 2001 From: roboomp Date: Tue, 14 Jul 2026 20:12:24 +0000 Subject: [PATCH 2/2] fix(session): keep explicit /compact snapcompact failing on text-only models wantsSnapcompact is true for both the default strategy and an explicit /compact snapcompact mode override. The prior fix downgraded both to LLM compaction, which silently shipped the transcript to a provider for users who deliberately requested the local-only no-LLM archive path. Only the default-configured strategy now falls back; explicit snapcompact keeps failing locally. Added a regression test for the explicit path. Fixes #5064 --- .../coding-agent/src/session/agent-session.ts | 20 ++++++-- ...ession-manual-snapcompact-fallback.test.ts | 51 +++++++++++++++---- 2 files changed, 58 insertions(+), 13 deletions(-) diff --git a/packages/coding-agent/src/session/agent-session.ts b/packages/coding-agent/src/session/agent-session.ts index efe15a03d..e28173f77 100644 --- a/packages/coding-agent/src/session/agent-session.ts +++ b/packages/coding-agent/src/session/agent-session.ts @@ -9798,19 +9798,31 @@ export class AgentSession { // Strategy honored on manual /compact too. Custom instructions (public // user focus OR internal plan-mode guidance) imply a directed LLM - // summary; a text-only model cannot read snapcompact frames. When - // snapcompact was requested but the active model is text-only, fall - // back to LLM-backed compaction (mirroring the auto-compaction path) - // instead of hard-failing the manual /compact (issue #5064). + // summary; a text-only model cannot read snapcompact frames. const wantsSnapcompact = compactionPrep.kind !== "fromHook" && effectiveSettings.strategy === "snapcompact" && !customInstructions && !options?.internalGuidance; + // `/compact snapcompact` is an explicit no-LLM archive request: honor + // its contract by failing locally rather than silently shipping the + // transcript to a provider. The default-configured snapcompact + // strategy, in contrast, falls back to LLM compaction (mirroring the + // auto-compaction path) so a routine /compact still completes on a + // text-only model (issue #5064). + const explicitSnapcompact = compactMode?.name === "snapcompact"; let snapcompactReady = wantsSnapcompact; const snapcompactShapeSetting = this.settings.get("snapcompact.shape"); let snapcompactShape: snapcompact.Shape | undefined; if (wantsSnapcompact && !this.model.input.includes("image")) { + if (explicitSnapcompact) { + this.emitNotice( + "warning", + `snapcompact needs a vision-capable model (${this.model.id} is text-only)`, + "compaction", + ); + throw new Error(`snapcompact cannot run locally: ${this.model.id} is text-only.`); + } this.emitNotice( "warning", `snapcompact needs a vision-capable model (${this.model.id} is text-only); falling back to LLM compaction`, diff --git a/packages/coding-agent/test/agent-session-manual-snapcompact-fallback.test.ts b/packages/coding-agent/test/agent-session-manual-snapcompact-fallback.test.ts index 2355fe81d..720640562 100644 --- a/packages/coding-agent/test/agent-session-manual-snapcompact-fallback.test.ts +++ b/packages/coding-agent/test/agent-session-manual-snapcompact-fallback.test.ts @@ -2,7 +2,7 @@ import { afterEach, describe, expect, it, vi } from "bun:test"; import * as path from "node:path"; import { Agent } from "@oh-my-pi/pi-agent-core"; import * as compactionModule from "@oh-my-pi/pi-agent-core/compaction"; -import type { Message } from "@oh-my-pi/pi-ai"; +import type { Message, Model } from "@oh-my-pi/pi-ai"; import { getBundledModel } from "@oh-my-pi/pi-catalog/models"; import { ModelRegistry } from "@oh-my-pi/pi-coding-agent/config/model-registry"; import { Settings } from "@oh-my-pi/pi-coding-agent/config/settings"; @@ -20,6 +20,10 @@ import { TempDir } from "@oh-my-pi/pi-utils"; * to LLM-backed compaction in the same situation. The manual path MUST mirror * that behavior: warn, then summarize via the LLM fallback candidate chain * (which tries the active text→text model first). + * + * An *explicit* `/compact snapcompact` (mode override) is a deliberate no-LLM + * archive request, so it MUST keep failing locally instead of silently + * shipping the transcript to a provider. */ describe("AgentSession manual snapcompact text-only fallback", () => { let session: AgentSession | undefined; @@ -39,7 +43,12 @@ describe("AgentSession manual snapcompact text-only fallback", () => { } }); - it("falls back to LLM compaction instead of throwing on a text-only active model", async () => { + async function createHarness(): Promise<{ + session: AgentSession; + sessionManager: SessionManager; + activeModel: Model; + notices: string[]; + }> { const activeModel = getBundledModel("aimlapi", "alibaba/qwen3-coder-480b-a35b-instruct"); if (!activeModel) throw new Error("Expected bundled text-only model"); expect(activeModel.input).not.toContain("image"); @@ -75,8 +84,7 @@ describe("AgentSession manual snapcompact text-only fallback", () => { { role: "user", content: "second question", timestamp: Date.now() }, ]; for (const message of seed) sessionManager.appendMessage(message); - const firstKeptEntryId = sessionManager.getBranch()[0]?.id; - if (!firstKeptEntryId) throw new Error("Expected seeded branch entry"); + if (!sessionManager.getBranch()[0]?.id) throw new Error("Expected seeded branch entry"); const settings = Settings.isolated({ "compaction.strategy": "snapcompact", @@ -88,6 +96,12 @@ describe("AgentSession manual snapcompact text-only fallback", () => { if (event.type === "notice" && event.source === "compaction") notices.push(event.message); }); + return { session, sessionManager, activeModel, notices }; + } + + it("falls back to LLM compaction instead of throwing on a text-only active model", async () => { + const harness = await createHarness(); + const compactSpy = vi.spyOn(compactionModule, "compact").mockImplementation(async (preparation, model) => ({ summary: "llm summary", shortSummary: "llm", @@ -96,19 +110,38 @@ describe("AgentSession manual snapcompact text-only fallback", () => { details: { provider: model.provider, model: model.id }, })); - const result = await session.compact(); + const result = await harness.session.compact(); expect(result.summary).toBe("llm summary"); // LLM fallback ran; the active text-only model is tried first. expect(compactSpy).toHaveBeenCalled(); const [, firstCandidate] = compactSpy.mock.calls[0]!; - expect(`${firstCandidate.provider}/${firstCandidate.id}`).toBe(`${activeModel.provider}/${activeModel.id}`); - expect(notices).toContain( - `snapcompact needs a vision-capable model (${activeModel.id} is text-only); falling back to LLM compaction`, + expect(`${firstCandidate.provider}/${firstCandidate.id}`).toBe( + `${harness.activeModel.provider}/${harness.activeModel.id}`, ); - expect(sessionManager.getBranch().find(entry => entry.type === "compaction")).toMatchObject({ + expect(harness.notices).toContain( + `snapcompact needs a vision-capable model (${harness.activeModel.id} is text-only); falling back to LLM compaction`, + ); + expect(harness.sessionManager.getBranch().find(entry => entry.type === "compaction")).toMatchObject({ type: "compaction", summary: "llm summary", }); }); + + it("still fails locally for explicit /compact snapcompact on a text-only model (no-LLM contract)", async () => { + const harness = await createHarness(); + + const compactSpy = vi.spyOn(compactionModule, "compact"); + + await expect(harness.session.compact(undefined, { mode: "snapcompact" })).rejects.toThrow( + `snapcompact cannot run locally: ${harness.activeModel.id} is text-only.`, + ); + + // Explicit no-LLM request must never reach the provider-backed summarizer. + expect(compactSpy).not.toHaveBeenCalled(); + expect(harness.notices).toContain( + `snapcompact needs a vision-capable model (${harness.activeModel.id} is text-only)`, + ); + expect(harness.sessionManager.getBranch().find(entry => entry.type === "compaction")).toBeUndefined(); + }); });