From 77028ae957611c9af42e95ff1592f9b5a5e56177 Mon Sep 17 00:00:00 2001 From: jwmacd Date: Sat, 1 Aug 2026 19:12:39 -0400 Subject: [PATCH 1/2] fix(compaction): stop shake from re-eliding artifact recovery reads A recovered artifact:// read is an ordinary read result, so the next shake pass elides it into a new artifact, indefinitely. Extends the 5f9558d17a spill exemption to shake; the dead-end rescue gets a dedicated config instead of falling through to AGGRESSIVE. Manual /shake unchanged. --- packages/agent/src/compaction/shake.ts | 11 +++- .../agent/src/compaction/tool-protection.ts | 10 ++++ .../src/session/session-maintenance.ts | 3 +- ...session-snapcompact-frame-dead-end.test.ts | 12 ++-- packages/coding-agent/test/shake.test.ts | 58 ++++++++++++++++++- 5 files changed, 85 insertions(+), 9 deletions(-) diff --git a/packages/agent/src/compaction/shake.ts b/packages/agent/src/compaction/shake.ts index db95486de..9ce9f36af 100644 --- a/packages/agent/src/compaction/shake.ts +++ b/packages/agent/src/compaction/shake.ts @@ -18,6 +18,7 @@ import type { CustomMessageEntry, SessionEntry, SessionMessageEntry } from "./en import { invalidateMessageCache } from "./message-cache"; import { collectToolCallsById, + isArtifactRecoveryToolResult, isProtectedToolResult, isSkillReadToolResult, type ProtectedToolMatcher, @@ -46,7 +47,7 @@ export interface ShakeConfig { export const DEFAULT_SHAKE_CONFIG: ShakeConfig = { protectTokens: 16_000, minSavings: 4_000, - protectedTools: ["skill", isSkillReadToolResult], + protectedTools: ["skill", isSkillReadToolResult, isArtifactRecoveryToolResult], fenceMinTokens: 400, }; @@ -58,6 +59,14 @@ export const AGGRESSIVE_SHAKE_CONFIG: ShakeConfig = { fenceMinTokens: 400, }; +/** Compaction dead-end rescue: aggressive reach, but artifact recovery reads stay protected. */ +export const RESCUE_SHAKE_CONFIG: ShakeConfig = { + protectTokens: 0, + minSavings: 0, + protectedTools: ["skill", isSkillReadToolResult, isArtifactRecoveryToolResult], + fenceMinTokens: 400, +}; + /** Rough token cost of a placeholder line; used only for the savings gate. */ const PLACEHOLDER_TOKEN_ESTIMATE = 16; diff --git a/packages/agent/src/compaction/tool-protection.ts b/packages/agent/src/compaction/tool-protection.ts index 3ac5ca74e..259fbf95b 100644 --- a/packages/agent/src/compaction/tool-protection.ts +++ b/packages/agent/src/compaction/tool-protection.ts @@ -39,6 +39,16 @@ export function isSkillReadToolResult(context: ProtectedToolContext): boolean { return getReadToolPath(context)?.startsWith(SKILL_INTERNAL_URL_PREFIX) ?? false; } +const ARTIFACT_INTERNAL_URL_PREFIX = "artifact://"; + +/** Recovery reads of session artifacts — eliding one only mints another artifact and can repeat indefinitely. */ +export function isArtifactRecoveryToolResult(context: ProtectedToolContext): boolean { + if (getReadToolPath(context)?.startsWith(ARTIFACT_INTERNAL_URL_PREFIX)) return true; + const meta = (context.toolResult.details as { meta?: { source?: { type?: string; value?: string } } } | undefined) + ?.meta; + return meta?.source?.type === "internal" && (meta.source.value?.startsWith(ARTIFACT_INTERNAL_URL_PREFIX) ?? false); +} + export function isProtectedToolResult( toolResult: ToolResultMessage, toolCall: AgentToolCall | undefined, diff --git a/packages/coding-agent/src/session/session-maintenance.ts b/packages/coding-agent/src/session/session-maintenance.ts index e1ccc43a5..bb424ab07 100644 --- a/packages/coding-agent/src/session/session-maintenance.ts +++ b/packages/coding-agent/src/session/session-maintenance.ts @@ -28,6 +28,7 @@ import { estimateTokens, NativeCompactionError, prepareCompaction, + RESCUE_SHAKE_CONFIG, resolveBudgetReserveTokens, resolveThresholdTokens, type ShakeConfig, @@ -1859,7 +1860,7 @@ export class SessionMaintenance { let elideSink = "placeholders"; if (!options.skipElide) { try { - const result = await this.#host.shake("elide", { signal }); + const result = await this.#host.shake("elide", { config: RESCUE_SHAKE_CONFIG, signal }); elided = result.toolResultsDropped + result.blocksDropped; elidedTokens = result.tokensFreed; if (result.artifactId) elideSink = "an artifact"; diff --git a/packages/coding-agent/test/agent-session-snapcompact-frame-dead-end.test.ts b/packages/coding-agent/test/agent-session-snapcompact-frame-dead-end.test.ts index d07bd7c2e..ef1f63730 100644 --- a/packages/coding-agent/test/agent-session-snapcompact-frame-dead-end.test.ts +++ b/packages/coding-agent/test/agent-session-snapcompact-frame-dead-end.test.ts @@ -1,7 +1,7 @@ import { afterEach, describe, expect, it, vi } from "bun:test"; import * as fs from "node:fs"; import * as path from "node:path"; -import { Agent } from "@oh-my-pi/pi-agent-core"; +import { Agent, RESCUE_SHAKE_CONFIG } from "@oh-my-pi/pi-agent-core"; import * as compactionModule from "@oh-my-pi/pi-agent-core/compaction"; import { getBundledModel } from "@oh-my-pi/pi-catalog/models"; import { ModelRegistry } from "@oh-my-pi/pi-coding-agent/config/model-registry"; @@ -389,7 +389,7 @@ describe("AgentSession snapcompact frame dead-end rescue", () => { const notices = collectNotices(); await triggerMaintenance(); - expect(shakeSpy).toHaveBeenCalledWith("elide", expect.anything()); + expect(shakeSpy).toHaveBeenCalledWith("elide", expect.objectContaining({ config: RESCUE_SHAKE_CONFIG })); const noProgress = notices.filter(n => n.source === NOTICE_SOURCE && n.message.includes(NO_PROGRESS_FRAGMENT)); expect(noProgress.length).toBe(1); expect(noProgress[0].level).toBe("warning"); @@ -431,7 +431,7 @@ describe("AgentSession snapcompact frame dead-end rescue", () => { await triggerMaintenance(); expect(compactSpy).not.toHaveBeenCalled(); - expect(shakeSpy).toHaveBeenCalledWith("elide", expect.anything()); + expect(shakeSpy).toHaveBeenCalledWith("elide", expect.objectContaining({ config: RESCUE_SHAKE_CONFIG })); expect(sessionManager.getBranch().at(-1)?.type).not.toBe("compaction"); }); @@ -455,7 +455,7 @@ describe("AgentSession snapcompact frame dead-end rescue", () => { await triggerMaintenance(); expect(compactSpy).not.toHaveBeenCalled(); - expect(shakeSpy).toHaveBeenCalledWith("elide", expect.anything()); + expect(shakeSpy).toHaveBeenCalledWith("elide", expect.objectContaining({ config: RESCUE_SHAKE_CONFIG })); }); it("leaves an oversized non-archive tail to the elide tiers instead of rescuing the archive", async () => { @@ -492,7 +492,7 @@ describe("AgentSession snapcompact frame dead-end rescue", () => { // The archive was NOT rebuilt; the elide tier got its shot at the tail. expect(compactSpy).not.toHaveBeenCalled(); - expect(shakeSpy).toHaveBeenCalledWith("elide", expect.anything()); + expect(shakeSpy).toHaveBeenCalledWith("elide", expect.objectContaining({ config: RESCUE_SHAKE_CONFIG })); const lastEntry = sessionManager.getBranch().at(-1); expect(lastEntry?.type).not.toBe("compaction"); const noProgress = notices.filter(n => n.source === NOTICE_SOURCE && n.message.includes(NO_PROGRESS_FRAGMENT)); @@ -539,7 +539,7 @@ describe("AgentSession snapcompact frame dead-end rescue", () => { // Text-only model: no frame re-render; existing tiers still run and the // existing dead-end warning is preserved. expect(compactSpy).not.toHaveBeenCalled(); - expect(shakeSpy).toHaveBeenCalledWith("elide", expect.anything()); + expect(shakeSpy).toHaveBeenCalledWith("elide", expect.objectContaining({ config: RESCUE_SHAKE_CONFIG })); const noProgress = notices.filter(n => n.source === NOTICE_SOURCE && n.message.includes(NO_PROGRESS_FRAGMENT)); expect(noProgress.length).toBe(1); }); diff --git a/packages/coding-agent/test/shake.test.ts b/packages/coding-agent/test/shake.test.ts index 69047423c..975ddadf8 100644 --- a/packages/coding-agent/test/shake.test.ts +++ b/packages/coding-agent/test/shake.test.ts @@ -1,7 +1,7 @@ import { afterEach, beforeEach, describe, expect, it, vi } from "bun:test"; import * as path from "node:path"; import { scheduler } from "node:timers/promises"; -import { Agent, type AgentMessage } from "@oh-my-pi/pi-agent-core"; +import { Agent, type AgentMessage, RESCUE_SHAKE_CONFIG } from "@oh-my-pi/pi-agent-core"; import * as compactionModule from "@oh-my-pi/pi-agent-core/compaction"; import type { AssistantMessage, ImageContent, ToolResultMessage } from "@oh-my-pi/pi-ai"; import { getBundledModel } from "@oh-my-pi/pi-catalog/models"; @@ -151,6 +151,62 @@ describe("AgentSession shake", () => { const result = await session.shake("elide"); expect(result.toolResultsDropped).toBe(0); }); + + /** Seed a user → assistant(read toolCall) → toolResult turn recovering an artifact. */ + function seedArtifactRecoveryResult(text: string, args: Record, details?: unknown): void { + const toolCallId = `call_read_${Math.random().toString(36).slice(2)}`; + sessionManager.appendMessage({ + role: "user", + content: [{ type: "text", text: "recover it" }], + timestamp: Date.now() - 3, + }); + sessionManager.appendMessage({ + role: "assistant", + content: [ + { type: "text", text: "recovering" }, + { type: "toolCall", id: toolCallId, name: "read", arguments: args }, + ], + ...apiInfo, + stopReason: "toolUse", + usage, + timestamp: Date.now() - 2, + }); + sessionManager.appendMessage({ + role: "toolResult", + toolCallId, + toolName: "read", + content: [{ type: "text", text }], + ...(details === undefined ? {} : { details }), + isError: false, + timestamp: Date.now() - 1, + }); + } + + it("rescue config never re-elides artifact recovery reads, by path or by source meta", async () => { + seedArtifactRecoveryResult("R".repeat(4000), { path: "artifact://0" }); + seedArtifactRecoveryResult( + "F".repeat(4000), + { path: "/tmp/artifacts/3.shake.log" }, + { + meta: { source: { type: "internal", value: "artifact://3" } }, + }, + ); + const result = await session.shake("elide", { config: RESCUE_SHAKE_CONFIG }); + expect(result.toolResultsDropped).toBe(0); + const texts = branchToolResults().map(m => (m.content[0] as { text: string }).text); + expect(texts.some(t => t.startsWith("R"))).toBe(true); + expect(texts.some(t => t.startsWith("F"))).toBe(true); + }); + + it("rescue config still elides ordinary oversized results", async () => { + seedHeavyToolResult("B".repeat(4000)); + seedArtifactRecoveryResult("R".repeat(4000), { path: "artifact://0" }); + const result = await session.shake("elide", { config: RESCUE_SHAKE_CONFIG }); + expect(result.toolResultsDropped).toBe(1); + const texts = branchToolResults().map(m => (m.content[0] as { text: string }).text); + expect(texts.some(t => t.startsWith("B"))).toBe(false); + expect(texts.some(t => t.startsWith("R"))).toBe(true); + }); }); describe("auto-shake strategy", () => { From 7428146556603eba0b7ec9e078c190bd7be7bf55 Mon Sep 17 00:00:00 2001 From: jwmacd Date: Sun, 2 Aug 2026 03:41:55 -0400 Subject: [PATCH 2/2] review: changelog entries; note manual /shake intent; derive RESCUE from AGGRESSIVE --- packages/agent/CHANGELOG.md | 4 ++++ packages/agent/src/compaction/shake.ts | 11 ++++++----- packages/coding-agent/CHANGELOG.md | 4 ++++ 3 files changed, 14 insertions(+), 5 deletions(-) diff --git a/packages/agent/CHANGELOG.md b/packages/agent/CHANGELOG.md index 8e3fb6a28..eec80294d 100644 --- a/packages/agent/CHANGELOG.md +++ b/packages/agent/CHANGELOG.md @@ -2,6 +2,10 @@ ## [Unreleased] +### Fixed + +- Shake no longer elides artifact recovery reads; the compaction dead-end rescue uses a dedicated `RESCUE_SHAKE_CONFIG`. + ## [17.2.4] - 2026-08-01 ### Fixed diff --git a/packages/agent/src/compaction/shake.ts b/packages/agent/src/compaction/shake.ts index 9ce9f36af..3d5289b7d 100644 --- a/packages/agent/src/compaction/shake.ts +++ b/packages/agent/src/compaction/shake.ts @@ -51,7 +51,10 @@ export const DEFAULT_SHAKE_CONFIG: ShakeConfig = { fenceMinTokens: 400, }; -/** Manual `/shake`: aggressive — drops every eligible region across history. */ +/** + * Manual `/shake`: aggressive — drops every eligible region across history, + * artifact recovery reads included (the user's full escape hatch). + */ export const AGGRESSIVE_SHAKE_CONFIG: ShakeConfig = { protectTokens: 0, minSavings: 0, @@ -61,10 +64,8 @@ export const AGGRESSIVE_SHAKE_CONFIG: ShakeConfig = { /** Compaction dead-end rescue: aggressive reach, but artifact recovery reads stay protected. */ export const RESCUE_SHAKE_CONFIG: ShakeConfig = { - protectTokens: 0, - minSavings: 0, - protectedTools: ["skill", isSkillReadToolResult, isArtifactRecoveryToolResult], - fenceMinTokens: 400, + ...AGGRESSIVE_SHAKE_CONFIG, + protectedTools: [...AGGRESSIVE_SHAKE_CONFIG.protectedTools, isArtifactRecoveryToolResult], }; /** Rough token cost of a placeholder line; used only for the savings gate. */ diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index 11d4830cc..2e9763154 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -2,6 +2,10 @@ ## [Unreleased] +### Fixed + +- Fixed shake re-eliding artifact recovery reads into a new artifact indefinitely. + ## [17.2.4] - 2026-08-01 ### Added