refactor(state): normalize full_state unwrapping in RemoteState once

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();
+  });
+});

src/conversation/remote-state.ts

@@ -21,7 +21,7 @@
 export interface ConversationStateUpdateEvent extends Event {
   kind: 'ConversationStateUpdateEvent';
   key: string;
   value: any;
 }
 
 export class RemoteState {
@@ -43,28 +43,32 @@
 
   private async getConversationInfo(): Promise<ConversationInfo> {
     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<any>(`/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();
       return conversationInfo;
     });
   }
 
+  /**
+   * 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.
    */
@@ -86,7 +90,7 @@
         if (this.cachedState === null) {
           this.cachedState = {} as ConversationInfo;
         }
         (this.cachedState as any)[event.key] = event.value;
       }
       this.cachedAt = Date.now();
     });
@@ -116,23 +120,14 @@
     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<ConversationExecutionStatus> {
     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 @@
 
   async getConfirmationPolicy(): Promise<ConfirmationPolicyBase> {
     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,24 +159,21 @@
 
   async getActivatedKnowledgeSkills(): Promise<string[]> {
     const info = await this.getConversationInfo();
-    const unwrappedInfo = this.unwrapState(info);
-    return unwrappedInfo.activated_knowledge_skills || [];
+    return info.activated_knowledge_skills || [];
   }
 
   async getAgent(): Promise<AgentBase> {
     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)}`);
     }
     return agentData;
   }
 
   async getWorkspace(): Promise<any> {
     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 @@
 
   async getPersistenceDir(): Promise<string> {
     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 @@
 
   async modelDump(): Promise<Record<string, any>> {
     const info = await this.getConversationInfo();
-    const unwrappedInfo = this.unwrapState(info);
-    return unwrappedInfo as Record<string, any>;
+    return info as Record<string, any>;
   }
 
   async modelDumpJson(): Promise<string> {