From 9e6ce98deac8f3baf61e6e9eb9e368e2ceeda043 Mon Sep 17 00:00:00 2001 From: VascoSch92 Date: Mon, 29 Jun 2026 13:49:45 +0200 Subject: [PATCH] refactor(state): normalize full_state unwrapping in RemoteState once MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Seven accessors (getExecutionStatus, getConfirmationPolicy, getAgent, getWorkspace, getPersistenceDir, getActivatedKnowledgeSkills, modelDump) each re-ran a per-accessor unwrap of the API's `full_state` wrapper. Make getConversationInfo() the single unwrap point and drop the per-accessor unwrapping. Crucially, getConversationInfo() previously normalized only the fresh REST fetch, not the cache-hit return — and the cache can be populated by state-update events (createStateUpdateCallback -> updateStateFromEvent) that still carry the `full_state` wrapper. So the normalization now runs on the cache-hit path too; otherwise accessors would throw "execution_status missing" against an event-populated cache. Purely internal (the helper is private); public behavior is unchanged. Adds RemoteState unit coverage for flat and full_state-wrapped REST payloads plus a regression test for the event-populated wrapped cache (the case the integration tests exercise). Closes #109 --- src/__tests__/remote-state.test.ts | 92 ++++++++++++++++++++++++++++++ src/conversation/remote-state.ts | 55 +++++++----------- 2 files changed, 114 insertions(+), 33 deletions(-) create mode 100644 src/__tests__/remote-state.test.ts diff --git a/src/__tests__/remote-state.test.ts b/src/__tests__/remote-state.test.ts new file mode 100644 index 00000000..681e949d --- /dev/null +++ b/src/__tests__/remote-state.test.ts @@ -0,0 +1,92 @@ +import { HttpClient } from '../client/http-client'; +import { RemoteState } from '../conversation/remote-state'; + +const originalFetch = global.fetch; + +function jsonResponse(body: unknown, status = 200): Response { + return new Response(JSON.stringify(body), { + status, + headers: { 'content-type': 'application/json' }, + }); +} + +const CONVERSATION_INFO = { + execution_status: 'running', + confirmation_policy: { kind: 'NeverConfirm' }, + activated_knowledge_skills: ['skill-a', 'skill-b'], + agent: { kind: 'Agent', name: 'test-agent' }, + workspace: { kind: 'LocalWorkspace' }, + persistence_dir: '/data/conversations/abc', +}; + +function makeState(payload: unknown): { state: RemoteState; fetchMock: jest.Mock } { + const fetchMock = jest.fn().mockResolvedValue(jsonResponse(payload)) as jest.Mock; + global.fetch = fetchMock as typeof fetch; + const client = new HttpClient({ baseUrl: 'http://example.com' }); + return { state: new RemoteState(client, 'abc'), fetchMock }; +} + +describe('RemoteState full_state normalization', () => { + afterEach(() => { + global.fetch = originalFetch; + jest.restoreAllMocks(); + }); + + it('reads accessor fields from a flat conversation-info payload', async () => { + const { state } = makeState(CONVERSATION_INFO); + + await expect(state.getExecutionStatus()).resolves.toBe('running'); + await expect(state.getConfirmationPolicy()).resolves.toEqual({ kind: 'NeverConfirm' }); + await expect(state.getActivatedKnowledgeSkills()).resolves.toEqual(['skill-a', 'skill-b']); + await expect(state.getAgent()).resolves.toEqual({ kind: 'Agent', name: 'test-agent' }); + await expect(state.getWorkspace()).resolves.toEqual({ kind: 'LocalWorkspace' }); + await expect(state.getPersistenceDir()).resolves.toBe('/data/conversations/abc'); + await expect(state.modelDump()).resolves.toMatchObject(CONVERSATION_INFO); + }); + + it('unwraps a full_state-wrapped payload exactly once for every accessor', async () => { + // The server may wrap the info in `{ full_state: {...} }`. Normalization now + // happens once in getConversationInfo(); accessors must still read through. + const { state } = makeState({ full_state: CONVERSATION_INFO }); + + await expect(state.getExecutionStatus()).resolves.toBe('running'); + await expect(state.getConfirmationPolicy()).resolves.toEqual({ kind: 'NeverConfirm' }); + await expect(state.getActivatedKnowledgeSkills()).resolves.toEqual(['skill-a', 'skill-b']); + await expect(state.getAgent()).resolves.toEqual({ kind: 'Agent', name: 'test-agent' }); + await expect(state.getWorkspace()).resolves.toEqual({ kind: 'LocalWorkspace' }); + await expect(state.getPersistenceDir()).resolves.toBe('/data/conversations/abc'); + }); + + it('falls back to the legacy agent_status field when execution_status is absent', async () => { + const { state } = makeState({ agent_status: 'idle' }); + await expect(state.getExecutionStatus()).resolves.toBe('idle'); + }); + + it('throws a descriptive error when a required field is missing', async () => { + const { state } = makeState({ execution_status: 'running' }); + await expect(state.getPersistenceDir()).rejects.toThrow(/persistence_dir missing/); + }); + + it('normalizes a cache that was populated with a full_state wrapper via events', async () => { + // Regression: state-update events can leave the cached state wrapped in a + // `full_state` key. The cache-hit path of getConversationInfo() must unwrap + // it too, otherwise accessors throw "execution_status missing". + const fetchMock = jest + .fn() + .mockRejectedValue(new Error('network should not be called')) as jest.Mock; + global.fetch = fetchMock as typeof fetch; + const state = new RemoteState(new HttpClient({ baseUrl: 'http://example.com' }), 'abc'); + + await state.updateStateFromEvent({ + id: 'evt-1', + kind: 'ConversationStateUpdateEvent', + timestamp: '2024-01-01T00:00:00Z', + key: 'full_state', + value: CONVERSATION_INFO, + }); + + await expect(state.getExecutionStatus()).resolves.toBe('running'); + await expect(state.getPersistenceDir()).resolves.toBe('/data/conversations/abc'); + expect(fetchMock).not.toHaveBeenCalled(); + }); +}); diff --git a/src/conversation/remote-state.ts b/src/conversation/remote-state.ts index edd70630..efe06931 100644 --- a/src/conversation/remote-state.ts +++ b/src/conversation/remote-state.ts @@ -43,21 +43,16 @@ export class RemoteState { private async getConversationInfo(): Promise { return await this.lock.acquire(async () => { - // Return cached state if available and fresh + // Return cached state if available and fresh. The cache can be populated + // by state-update events that still carry the `full_state` wrapper, so + // normalize on the way out here too — this is the single unwrap point. if (this.cachedState !== null && Date.now() - this.cachedAt < RemoteState.CACHE_TTL_MS) { - return this.cachedState; + return this.normalizeFullState(this.cachedState); } - // Fetch from REST API + // Fetch from REST API and normalize the `full_state` wrapper once. const response = await this.client.get(`/api/conversations/${this.conversationId}`); - - // Handle the case where the API returns a full_state wrapper - let conversationInfo: ConversationInfo; - if (response.data.full_state) { - conversationInfo = response.data.full_state as ConversationInfo; - } else { - conversationInfo = response.data as ConversationInfo; - } + const conversationInfo = this.normalizeFullState(response.data); this.cachedState = conversationInfo; this.cachedAt = Date.now(); @@ -65,6 +60,15 @@ export class RemoteState { }); } + /** + * Unwrap the API's `full_state` wrapper so every accessor receives a flat + * ConversationInfo. Centralizing this here means accessors never have to + * unwrap themselves. + */ + private normalizeFullState(info: ConversationInfo): ConversationInfo { + return ((info as any).full_state ?? info) as ConversationInfo; + } + /** * Force a fresh fetch from the server, ignoring the cache. */ @@ -116,23 +120,14 @@ export class RemoteState { return this.conversationId; } - /** - * Helper to unwrap full_state if present - */ - private unwrapState(info: ConversationInfo): ConversationInfo { - return (info as any).full_state ?? info; - } - /** * Get the current execution status of the conversation. * This method handles both the new `execution_status` field and the legacy `agent_status` field. */ async getExecutionStatus(): Promise { const info = await this.getConversationInfo(); - // Handle case where info might still be wrapped in full_state - const unwrappedInfo = this.unwrapState(info); // Try new field first, fall back to legacy field - const statusStr = unwrappedInfo.execution_status ?? unwrappedInfo.agent_status; + const statusStr = info.execution_status ?? info.agent_status; if (statusStr === undefined || statusStr === null) { throw new Error(`execution_status missing in conversation info: ${JSON.stringify(info)}`); } @@ -155,8 +150,7 @@ export class RemoteState { async getConfirmationPolicy(): Promise { const info = await this.getConversationInfo(); - const unwrappedInfo = this.unwrapState(info); - const policyData = unwrappedInfo.confirmation_policy; + const policyData = info.confirmation_policy; if (policyData === undefined || policyData === null) { throw new Error(`confirmation_policy missing in conversation info: ${JSON.stringify(info)}`); } @@ -165,14 +159,12 @@ export class RemoteState { async getActivatedKnowledgeSkills(): Promise { const info = await this.getConversationInfo(); - const unwrappedInfo = this.unwrapState(info); - return unwrappedInfo.activated_knowledge_skills || []; + return info.activated_knowledge_skills || []; } async getAgent(): Promise { const info = await this.getConversationInfo(); - const unwrappedInfo = this.unwrapState(info); - const agentData = unwrappedInfo.agent; + const agentData = info.agent; if (agentData === undefined || agentData === null) { throw new Error(`agent missing in conversation info: ${JSON.stringify(info)}`); } @@ -181,8 +173,7 @@ export class RemoteState { async getWorkspace(): Promise { const info = await this.getConversationInfo(); - const unwrappedInfo = this.unwrapState(info); - const workspace = unwrappedInfo.workspace; + const workspace = info.workspace; if (workspace === undefined || workspace === null) { throw new Error(`workspace missing in conversation info: ${JSON.stringify(info)}`); } @@ -191,8 +182,7 @@ export class RemoteState { async getPersistenceDir(): Promise { const info = await this.getConversationInfo(); - const unwrappedInfo = this.unwrapState(info); - const persistenceDir = unwrappedInfo.persistence_dir; + const persistenceDir = info.persistence_dir; if (persistenceDir === undefined || persistenceDir === null) { throw new Error(`persistence_dir missing in conversation info: ${JSON.stringify(info)}`); } @@ -201,8 +191,7 @@ export class RemoteState { async modelDump(): Promise> { const info = await this.getConversationInfo(); - const unwrappedInfo = this.unwrapState(info); - return unwrappedInfo as Record; + return info as Record; } async modelDumpJson(): Promise {