Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 21 additions & 0 deletions packages/core/src/agents/forkedAgent.cache.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import {
getCacheSafeParams,
getCacheSafeParamsSessionId,
clearCacheSafeParams,
createForkedChat,
runForkedAgent,
} from './forkedAgent.js';
import type { Content, GenerateContentConfig } from '@google/genai';
Expand Down Expand Up @@ -228,6 +229,26 @@ describe('CacheSafeParams', () => {
});
});

describe('createForkedChat', () => {
beforeEach(() => {
clearCacheSafeParams();
vi.mocked(GeminiChat).mockReset();
});

it('marks the fork so its history rewrites skip parent skill tracking', () => {
const forked = {} as unknown as GeminiChat;
vi.mocked(GeminiChat).mockImplementation(() => forked);

saveCacheSafeParams({ systemInstruction: 'si' }, [], 'test-model');
const chat = createForkedChat(
{} as unknown as Config,
getCacheSafeParams()!,
);

expect(chat.isForkedChat).toBe(true);
});
});

describe('runForkedAgent (cache path)', () => {
beforeEach(() => {
clearCacheSafeParams();
Expand Down
6 changes: 5 additions & 1 deletion packages/core/src/agents/forkedAgent.ts
Original file line number Diff line number Diff line change
Expand Up @@ -223,7 +223,7 @@ export function createForkedChat(
? params.history.slice(-maxHistoryEntries)
: params.history;

return new GeminiChat(
const forkedChat = new GeminiChat(
config,
{
...params.generationConfig,
Expand All @@ -235,6 +235,10 @@ export function createForkedChat(
undefined, // no chatRecordingService
undefined, // no telemetryService
);
// The fork shares the parent's ToolRegistry; its history rewrites must
// not touch the parent's loaded-skill tracking.
forkedChat.isForkedChat = true;
return forkedChat;
}

interface ForkedModelRuntime {
Expand Down
3 changes: 3 additions & 0 deletions packages/core/src/core/client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2470,6 +2470,7 @@ export class GeminiClient {
const m = mcResult.meta;
const changed = m.tokensSaved > 0;
if (changed) {
// setHistory conservatively clears loaded-skill tracking.
this.getChat().setHistory(mcResult.history);
await this.disarmFileReadCacheAfterEviction(m, 'microcompaction');
Comment thread
ZijianZhang989 marked this conversation as resolved.
}
Expand Down Expand Up @@ -2768,6 +2769,8 @@ export class GeminiClient {
this.getChat().addHistory(entry);
}
}
// Loaded-skill tracking was conservatively cleared by the strip
// above; restored bodies simply re-inject on their next invoke.
strippedRetryEntries = [];
};

Expand Down
107 changes: 106 additions & 1 deletion packages/core/src/core/geminiChat.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -303,6 +303,107 @@ describe('GeminiChat', async () => {
} as unknown as GenerateContentResponse;
}

describe('history-rewrite loaded-skill tracking', () => {
// Destructive rewrites (compaction, truncation, orphan stripping)
// conservatively clear the SkillTool's loaded-skill tracking so an
// evicted body can never stay stuck behind the dedup guard. The
// trade-off is at most one duplicate body on the next invoke.
const wireSkillTracker = () => {
const skillTool = { clearLoadedSkills: vi.fn() };
vi.mocked(mockConfig.getToolRegistry).mockReturnValue({
getTool: vi.fn().mockReturnValue(skillTool),
} as unknown as ReturnType<Config['getToolRegistry']>);
return skillTool;
};

it('setHistory clears tracking on wholesale replacement', () => {
const skillTool = wireSkillTracker();
chat.setHistory([{ role: 'user', parts: [{ text: 'hi' }] }]);
expect(skillTool.clearLoadedSkills).toHaveBeenCalled();
});

it('tryCompress clears tracking through its setHistory', async () => {
const skillTool = wireSkillTracker();
vi.spyOn(
ChatCompressionService.prototype,
'compress',
).mockResolvedValueOnce({
newHistory: [{ role: 'user', parts: [{ text: 'summary' }] }],
info: {
originalTokenCount: 100_000,
newTokenCount: 30_000,
compressionStatus: CompressionStatus.COMPRESSED,
},
});

await chat.tryCompress('prompt-skill-clear', true);

expect(skillTool.clearLoadedSkills).toHaveBeenCalled();
});

it('tryCompress leaves tracking untouched on NOOP', async () => {
const skillTool = wireSkillTracker();
vi.spyOn(
ChatCompressionService.prototype,
'compress',
).mockResolvedValueOnce({
newHistory: null,
info: {
originalTokenCount: 1_000,
newTokenCount: 1_000,
compressionStatus: CompressionStatus.NOOP,
},
});

await chat.tryCompress('prompt-skill-noop', true);

expect(skillTool.clearLoadedSkills).not.toHaveBeenCalled();
});

it('truncateHistory clears tracking when entries were dropped', () => {
const skillTool = wireSkillTracker();
chat.addHistory({ role: 'user', parts: [{ text: 'a' }] });
chat.addHistory({ role: 'model', parts: [{ text: 'b' }] });
chat.truncateHistory(1);
expect(skillTool.clearLoadedSkills).toHaveBeenCalled();
});

it('truncateHistory leaves tracking when nothing was dropped', () => {
const skillTool = wireSkillTracker();
chat.addHistory({ role: 'user', parts: [{ text: 'a' }] });
chat.truncateHistory(5);
expect(skillTool.clearLoadedSkills).not.toHaveBeenCalled();
});

it('stripOrphanedUserEntriesFromHistory clears tracking when it strips', () => {
const skillTool = wireSkillTracker();
chat.addHistory({ role: 'model', parts: [{ text: 'ack' }] });
chat.addHistory({ role: 'user', parts: [{ text: 'orphan' }] });
chat.stripOrphanedUserEntriesFromHistory();
expect(skillTool.clearLoadedSkills).toHaveBeenCalled();
});

it('stripOrphanedUserEntriesFromHistory leaves tracking when nothing is stripped', () => {
const skillTool = wireSkillTracker();
chat.addHistory({ role: 'model', parts: [{ text: 'ack' }] });
chat.stripOrphanedUserEntriesFromHistory();
expect(skillTool.clearLoadedSkills).not.toHaveBeenCalled();
});

it('forked chats never touch the shared parent tracker', () => {
const skillTool = wireSkillTracker();
chat.isForkedChat = true;

chat.setHistory([{ role: 'model', parts: [{ text: 'ack' }] }]);
chat.addHistory({ role: 'user', parts: [{ text: 'orphan' }] });
chat.stripOrphanedUserEntriesFromHistory();
chat.addHistory({ role: 'model', parts: [{ text: 'ack2' }] });
chat.truncateHistory(1);

expect(skillTool.clearLoadedSkills).not.toHaveBeenCalled();
});
});

describe('system instruction helpers', () => {
it('replaces prior session-start context instead of appending indefinitely', () => {
const isolatedChat = new GeminiChat(
Expand Down Expand Up @@ -16400,7 +16501,10 @@ describe('GeminiChat', async () => {
mockFileSystem.set(planFile, PLAN);
try {
const chat = new GeminiChat(
{ getPlanFilePath: () => planFile } as unknown as Config,
{
getPlanFilePath: () => planFile,
getToolRegistry: () => undefined,
} as unknown as Config,
{},
[],
);
Expand Down Expand Up @@ -16432,6 +16536,7 @@ describe('GeminiChat', async () => {
const chat = new GeminiChat(
{
getPlanFilePath: () => '/plans/never-written.md',
getToolRegistry: () => undefined,
} as unknown as Config,
{},
[],
Expand Down
41 changes: 41 additions & 0 deletions packages/core/src/core/geminiChat.ts
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@ import {
} from './tokenLimits.js';
import { hasCycleInSchema } from '../tools/tools.js';
import { ToolNames, canonicalToolName } from '../tools/tool-names.js';
import { clearLoadedSkillTracking } from '../tools/skill-utils.js';
import * as fs from 'node:fs';
import { PLAN_EXIT_APPROVED_LLM_CONTENT_PREFIXES } from '../tools/exitPlanMode.js';
import { isManagedMemoryPath } from '../memory/paths.js';
Expand Down Expand Up @@ -1973,6 +1974,15 @@ export class GeminiChat {
private userContentPushCount = 0;
private manualPlanExitNoticesEnabled = false;

/**
* True for forked/speculative chats built by `createForkedChat` on the
* parent's Config. They share the parent's ToolRegistry (and the single
* SkillTool tracking instance) while rewriting only a copy of a parent
* history slice, so their rewrites must not touch loaded-skill tracking —
* only the chat owning the authoritative session may.
*/
isForkedChat = false;

/**
* Reset both partial-push markers in lockstep. Every history-mutation
* site uses this — single-field resets are a bug because the fields
Expand Down Expand Up @@ -2395,6 +2405,8 @@ export class GeminiChat {
// the session-token-limit gate blocks a prompt that fits the
// compressed history (#9506).
this.tokenCountsByRouteKey.clear();
// Loaded-skill tracking was conservatively cleared by the setHistory
// above — no second sync here.
this.setLastPromptTokenCount(
info.newTokenCount,
info.newTokenCountIsEstimated,
Expand Down Expand Up @@ -2873,6 +2885,8 @@ export class GeminiChat {
// state. The JSONL compression checkpoint is intentionally not
// written because the send is about to be rejected.
this.setHistory(historyBeforeHardRescue);
// setHistory conservatively cleared loaded-skill tracking; the
// restored bodies re-arm it on their next invoke.
this.lastPromptTokenCount = lastPromptTokenCountBeforeHardRescue;
this.lastPromptTokenCountIsEstimated =
lastPromptTokenCountWasEstimatedBeforeHardRescue;
Expand Down Expand Up @@ -4884,16 +4898,33 @@ export class GeminiChat {
// stash too: its referent (the model turn at the old index) is gone.
this.clearPendingPartialState();
this.redactApprovedPlansFromLoadedHistory();
// Wholesale replacement can drop resident skill bodies (compression,
// /restore, session-manager load_history, ACP restoreSessionHistory
// all land here). Conservatively clear the tracking so an evicted
// skill never stays stuck behind the dedup guard; a still-resident
// body costs at most one duplicate injection on the next invoke.
if (!this.isForkedChat) {
clearLoadedSkillTracking(this.config.getToolRegistry(), 'setHistory');
}
}

truncateHistory(keepCount: number): void {
const prevLen = this.history.length;
this.history = this.history.slice(0, keepCount);
// Truncation can drop the entry the partial-push marker points at,
// or leave it valid but shift the meaning of nearby indices. Reset
// both fields rather than try to fix them up — they're per-send and
// ephemeral, so losing them across a truncate is safe (the
// sendMessageStream that pushed them has already finished or will
// start fresh on the next call).
if (this.history.length < prevLen && !this.isForkedChat) {
// Truncation may have dropped a skill body; conservative clear
// re-arms reload (see setHistory for the trade-off).
clearLoadedSkillTracking(
this.config.getToolRegistry(),
'truncateHistory',
);
}
this.clearPendingPartialState();
}

Expand Down Expand Up @@ -4950,6 +4981,16 @@ export class GeminiChat {
// `sendMessageStream` would otherwise leave a stale marker that
// happens to line up with whatever model entry is at that index
// in the meanwhile.
if (strippedEntries.length > 0 && !this.isForkedChat) {
// The stripped entries may have carried a skill body; conservative
// clear re-arms reload (see setHistory for the trade-off). A forked
// chat shares the parent's tracker while holding only a tail slice,
// so only the authoritative session's chat may clear.
clearLoadedSkillTracking(
this.config.getToolRegistry(),
'stripOrphanedUserEntries',
);
}
this.clearPendingPartialState();
return strippedEntries;
}
Expand Down
Loading
Loading