mirror of
https://github.com/thedotmack/claude-mem.git
synced 2026-09-20 04:23:02 +08:00
fix(observer): don't store observations parsed from the init reply (#3877)
startSession sends the briefing prompt as a standalone query and hands its reply straight to the storage path, so a model that answers the brief with a well-formed <observation> gets it stored as memory. The brief cannot legitimately contain one: buildInitPrompt shows the output format alongside an <observed_from_primary_session> block holding only <user_request> and <requested_at>, with no tool call and nothing observed. What lands is a paraphrase of the user's prompt, asserting work that has not happened, and it then feeds cross-session context injection like any real observation. Measured on a non-Anthropic provider: 2 of 12 sessions. `files_modified: []` is the fingerprint, since sanitizeObservationFiles strips file claims no claimed message supports and at init there are none. handleInitResponse now keeps the reply's token accounting and appends it as the assistant turn itself rather than routing it through processAgentResponse. Role alternation is unchanged; only the storage call goes away. session.lastUsage is deliberately no longer set here — ResponseProcessor reads and nulls it, so with no consumer on this path it leaked the init call's usage into the next observation's telemetry. Not fixed here: ClaudeProvider is exposed the same way, but the SDK streams, so a fix there turns on the one-reply-per-turn invariant rather than a return value and deserves its own change. The two gemini_provider tests that stored through the init query are re-targeted at a tool call; they were exercising the store pipeline, not asserting that a brief reply is memory.
This commit is contained in:
@@ -136,7 +136,6 @@ export abstract class OpenAICompatibleProvider<TConfig extends { apiKey: string;
|
||||
const initPrompt = session.lastPromptNumber === 1
|
||||
? buildInitPrompt(session.project, session.contentSessionId, session.userPrompt, mode, priorContext)
|
||||
: buildContinuationPrompt(session.userPrompt, session.lastPromptNumber, session.contentSessionId, mode, priorContext);
|
||||
const initContext = snapshotResponseContext(session);
|
||||
|
||||
session.conversationHistory.push({ role: 'user', content: initPrompt });
|
||||
|
||||
@@ -144,7 +143,7 @@ export abstract class OpenAICompatibleProvider<TConfig extends { apiKey: string;
|
||||
session.lastPromptSentAt = Date.now();
|
||||
session.lastGeneratorSource = 'init';
|
||||
const initResponse = await this.query(session.conversationHistory, config);
|
||||
await this.handleInitResponse(initResponse, session, worker, model, initContext);
|
||||
this.handleInitResponse(initResponse, session, model);
|
||||
} catch (error: unknown) {
|
||||
// Classified errors are logged once, at SessionRoutes' `Observer failed`
|
||||
// line; here they're debug-level so one failure isn't five error lines.
|
||||
@@ -204,28 +203,27 @@ export abstract class OpenAICompatibleProvider<TConfig extends { apiKey: string;
|
||||
}
|
||||
}
|
||||
|
||||
private async handleInitResponse(
|
||||
private handleInitResponse(
|
||||
initResponse: ProviderQueryResult,
|
||||
session: ActiveSession,
|
||||
worker: WorkerRef | undefined,
|
||||
model: string,
|
||||
responseContext: ReturnType<typeof snapshotResponseContext>
|
||||
): Promise<void> {
|
||||
if (initResponse.content) {
|
||||
// Appended once, by processAgentResponse below — see processObservationMessage.
|
||||
const tokensUsed = initResponse.tokensUsed || 0;
|
||||
session.cumulativeInputTokens += Math.floor(tokensUsed * 0.7);
|
||||
session.cumulativeOutputTokens += Math.floor(tokensUsed * 0.3);
|
||||
session.lastUsage = this.buildLastUsage(initResponse);
|
||||
await processAgentResponse(
|
||||
initResponse.content, session, this.dbManager, this.sessionManager,
|
||||
worker, tokensUsed, null, this.providerName, undefined, initResponse.servedModel ?? model, responseContext
|
||||
);
|
||||
} else {
|
||||
model: string
|
||||
): void {
|
||||
if (!initResponse.content) {
|
||||
logger.error('SDK', `Empty ${this.providerName} init response - session may lack context`, {
|
||||
sessionId: session.sessionDbId, model
|
||||
});
|
||||
return;
|
||||
}
|
||||
|
||||
const tokensUsed = initResponse.tokensUsed || 0;
|
||||
session.cumulativeInputTokens += Math.floor(tokensUsed * 0.7);
|
||||
session.cumulativeOutputTokens += Math.floor(tokensUsed * 0.3);
|
||||
// The init prompt carries the user's request and no tool call, so nothing in
|
||||
// its reply can be an observation of this session — an <observation> here was
|
||||
// invented from <user_request> alone and would be stored as memory for work
|
||||
// that never happened. Keep the turn so role alternation holds, but never
|
||||
// hand it to the storage path.
|
||||
session.conversationHistory.push({ role: 'assistant', content: initResponse.content });
|
||||
}
|
||||
|
||||
private async processObservationMessage(
|
||||
|
||||
@@ -9,6 +9,15 @@ import { ModeManager } from '../src/services/domain/ModeManager';
|
||||
import { SettingsDefaultsManager } from '../src/shared/SettingsDefaultsManager';
|
||||
|
||||
let rateLimitingEnabled = 'false';
|
||||
let queuedMessages: Array<Record<string, unknown>> = [];
|
||||
|
||||
const toolObservationMessage = {
|
||||
type: 'observation',
|
||||
tool_name: 'Read',
|
||||
tool_input: { file_path: 'src/main.ts' },
|
||||
tool_response: 'file contents',
|
||||
prompt_number: 1,
|
||||
};
|
||||
|
||||
const mockMode = {
|
||||
name: 'code',
|
||||
@@ -91,6 +100,7 @@ describe('GeminiProvider', () => {
|
||||
|
||||
beforeEach(() => {
|
||||
rateLimitingEnabled = 'false';
|
||||
queuedMessages = [];
|
||||
|
||||
modeManagerSpy = spyOn(ModeManager, 'getInstance').mockImplementation(() => ({
|
||||
getActiveMode: () => mockMode,
|
||||
@@ -156,7 +166,7 @@ describe('GeminiProvider', () => {
|
||||
};
|
||||
|
||||
mockSessionManager = {
|
||||
getMessageIterator: async function* () { yield* []; },
|
||||
getMessageIterator: async function* () { yield* queuedMessages; },
|
||||
getClaimedMessages: mock(() => []),
|
||||
confirmClaimedMessages: mock(() => Promise.resolve(0)),
|
||||
resetProcessingToPending: mock(() => Promise.resolve(0)),
|
||||
@@ -318,6 +328,7 @@ describe('GeminiProvider', () => {
|
||||
</observation>
|
||||
`;
|
||||
|
||||
queuedMessages = [toolObservationMessage];
|
||||
global.fetch = mock(() => Promise.resolve(new Response(JSON.stringify({
|
||||
candidates: [{ content: { parts: [{ text: observationXml }] } }],
|
||||
usageMetadata: { totalTokenCount: 50 }
|
||||
@@ -325,12 +336,12 @@ describe('GeminiProvider', () => {
|
||||
|
||||
await agent.startSession(session);
|
||||
|
||||
expect(mockStoreObservations).toHaveBeenCalled();
|
||||
expect(mockStoreObservations).toHaveBeenCalledTimes(1);
|
||||
expect(mockSyncObservation).toHaveBeenCalled();
|
||||
expect(session.cumulativeInputTokens).toBeGreaterThan(0);
|
||||
});
|
||||
|
||||
it('stores a deferred init response under the original prompt project after the live session advances', async () => {
|
||||
it('stores a deferred observation response under the original prompt project after the live session advances', async () => {
|
||||
const session = makeSession({
|
||||
project: 'repo-a',
|
||||
userPrompt: 'prompt 1',
|
||||
@@ -339,7 +350,7 @@ describe('GeminiProvider', () => {
|
||||
const observationXml = `
|
||||
<observation>
|
||||
<type>discovery</type>
|
||||
<title>Late init response</title>
|
||||
<title>Late observation response</title>
|
||||
<narrative>Should stay on the original prompt project.</narrative>
|
||||
<facts></facts>
|
||||
<concepts></concepts>
|
||||
@@ -348,10 +359,23 @@ describe('GeminiProvider', () => {
|
||||
</observation>
|
||||
`;
|
||||
|
||||
queuedMessages = [toolObservationMessage];
|
||||
let resolveFetch!: (response: Response) => void;
|
||||
global.fetch = mock(() => new Promise<Response>(resolve => {
|
||||
resolveFetch = resolve;
|
||||
}));
|
||||
let sends = 0;
|
||||
global.fetch = mock(() => {
|
||||
sends++;
|
||||
// Only the observation query is held open; the init query has to complete
|
||||
// for the message loop to reach it.
|
||||
if (sends === 1) {
|
||||
return Promise.resolve(new Response(JSON.stringify({
|
||||
candidates: [{ content: { parts: [{ text: 'Ready.' }] } }],
|
||||
usageMetadata: { totalTokenCount: 10 }
|
||||
})));
|
||||
}
|
||||
return new Promise<Response>(resolve => {
|
||||
resolveFetch = resolve;
|
||||
});
|
||||
});
|
||||
|
||||
const pending = agent.startSession(session);
|
||||
// Wait for the request to actually be in flight rather than assuming it
|
||||
|
||||
@@ -0,0 +1,140 @@
|
||||
import { afterEach, beforeEach, describe, expect, it, mock, spyOn } from 'bun:test';
|
||||
import { ModeManager } from '../../src/services/domain/ModeManager.js';
|
||||
import { OpenAICompatibleProvider, type ProviderQueryResult } from '../../src/services/worker/OpenAICompatibleProvider.js';
|
||||
import type { DatabaseManager } from '../../src/services/worker/DatabaseManager.js';
|
||||
import type { SessionManager } from '../../src/services/worker/SessionManager.js';
|
||||
import type { ActiveSession, ConversationMessage } from '../../src/services/worker-types.js';
|
||||
|
||||
const mockMode = {
|
||||
name: 'code',
|
||||
prompts: {
|
||||
init: 'init prompt',
|
||||
observation: 'obs prompt',
|
||||
summary: 'summary prompt',
|
||||
},
|
||||
observation_types: [{ id: 'discovery' }],
|
||||
observation_concepts: [],
|
||||
};
|
||||
|
||||
const observationXml = `
|
||||
<observation>
|
||||
<type>discovery</type>
|
||||
<title>Invented from the user request</title>
|
||||
<narrative>No tool call had been observed when this was produced.</narrative>
|
||||
<facts></facts>
|
||||
<concepts></concepts>
|
||||
<files_read></files_read>
|
||||
<files_modified></files_modified>
|
||||
</observation>
|
||||
`;
|
||||
|
||||
function makeSession(overrides: Partial<ActiveSession> = {}): ActiveSession {
|
||||
return {
|
||||
sessionDbId: 1,
|
||||
contentSessionId: 'test-session',
|
||||
memorySessionId: 'mem-session-123',
|
||||
project: 'test-project',
|
||||
platformSource: 'claude',
|
||||
userPrompt: 'test prompt',
|
||||
abortController: new AbortController(),
|
||||
generatorPromise: null,
|
||||
lastPromptNumber: 1,
|
||||
startTime: Date.now(),
|
||||
cumulativeInputTokens: 0,
|
||||
cumulativeOutputTokens: 0,
|
||||
earliestPendingTimestamp: null,
|
||||
claimedMessageIds: [],
|
||||
conversationHistory: [],
|
||||
currentProvider: null,
|
||||
consecutiveRestarts: 0,
|
||||
consecutiveInvalidOutputs: 0,
|
||||
lastGeneratorActivity: Date.now(),
|
||||
...overrides,
|
||||
};
|
||||
}
|
||||
|
||||
/** Answers every prompt — the init prompt included — with a valid observation. */
|
||||
class TestProvider extends OpenAICompatibleProvider<{ apiKey: string; model: string }> {
|
||||
protected readonly providerName = 'TestProvider';
|
||||
protected readonly syntheticIdPrefix = 'test';
|
||||
protected readonly forwardEmptyMessageResponse = false;
|
||||
queries = 0;
|
||||
|
||||
protected getConfig() {
|
||||
return { apiKey: 'test-api-key', model: 'session-model' };
|
||||
}
|
||||
|
||||
protected missingApiKeyError(): Error {
|
||||
return new Error('missing key');
|
||||
}
|
||||
|
||||
protected async query(_history: ConversationMessage[], _config: { apiKey: string; model: string }): Promise<ProviderQueryResult> {
|
||||
this.queries++;
|
||||
return { content: observationXml, tokensUsed: 100 };
|
||||
}
|
||||
|
||||
protected estimateTokens(): number {
|
||||
return 0;
|
||||
}
|
||||
|
||||
protected buildLastUsage(): ActiveSession['lastUsage'] {
|
||||
return null;
|
||||
}
|
||||
}
|
||||
|
||||
describe('OpenAICompatibleProvider init response', () => {
|
||||
let modeManagerSpy: ReturnType<typeof spyOn>;
|
||||
let storeObservations: ReturnType<typeof mock>;
|
||||
let dbManager: DatabaseManager;
|
||||
let sessionManager: SessionManager;
|
||||
|
||||
beforeEach(() => {
|
||||
modeManagerSpy = spyOn(ModeManager, 'getInstance').mockImplementation(() => ({
|
||||
getActiveMode: () => mockMode,
|
||||
loadMode: () => {},
|
||||
} as unknown as ModeManager));
|
||||
|
||||
storeObservations = mock(() => ({ observationIds: [1], summaryId: null, createdAtEpoch: Date.now() }));
|
||||
|
||||
dbManager = {
|
||||
getSessionStore: () => ({
|
||||
storeObservations,
|
||||
ensureMemorySessionIdRegistered: mock(() => {}),
|
||||
updateMemorySessionId: mock(() => {}),
|
||||
}),
|
||||
getChromaSync: () => ({
|
||||
syncObservation: mock(() => Promise.resolve()),
|
||||
syncSummary: mock(() => Promise.resolve()),
|
||||
}),
|
||||
getCloudSync: () => null,
|
||||
} as unknown as DatabaseManager;
|
||||
|
||||
sessionManager = {
|
||||
getMessageIterator: async function* () {
|
||||
yield { type: 'observation', tool_name: 'Read', tool_input: { file_path: 'src/main.ts' }, tool_response: 'file contents', prompt_number: 1 };
|
||||
},
|
||||
getClaimedMessages: mock(() => []),
|
||||
confirmClaimedMessages: mock(() => Promise.resolve(0)),
|
||||
resetProcessingToPending: mock(() => Promise.resolve(0)),
|
||||
} as unknown as SessionManager;
|
||||
});
|
||||
|
||||
afterEach(() => {
|
||||
modeManagerSpy.mockRestore();
|
||||
mock.restore();
|
||||
});
|
||||
|
||||
it('does not store observations parsed out of the init response', async () => {
|
||||
const provider = new TestProvider(dbManager, sessionManager);
|
||||
const session = makeSession();
|
||||
|
||||
await provider.startSession(session);
|
||||
|
||||
// Both queries were answered with an observation, but only the reply to a
|
||||
// real tool call is an observation of this session.
|
||||
expect(provider.queries).toBe(2);
|
||||
expect(storeObservations).toHaveBeenCalledTimes(1);
|
||||
// The init reply still occupies its assistant turn, so roles alternate.
|
||||
expect(session.conversationHistory.map(message => message.role)).toEqual(['user', 'assistant', 'user', 'assistant']);
|
||||
});
|
||||
});
|
||||
Reference in New Issue
Block a user