From 0dfa2f04bd4c8c81c63afd92565e6ebf7854e270 Mon Sep 17 00:00:00 2001 From: Mohamed Boudra Date: Sat, 29 Nov 2025 23:24:25 +0000 Subject: [PATCH] refactor: remove status from recordConfig, add design proposal MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - recordConfig no longer writes lastStatus (handled by applySnapshot only) - Add warning if recordConfig called before snapshot exists - Update tests to reflect new separation of concerns - Add AGENT_REFACTOR_PROPOSAL.md with comprehensive design plan 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- packages/server/AGENT_REFACTOR_PROPOSAL.md | 143 ++++++++++++++++++ .../src/server/agent/agent-registry.test.ts | 26 ++-- .../server/src/server/agent/agent-registry.ts | 25 ++- 3 files changed, 170 insertions(+), 24 deletions(-) create mode 100644 packages/server/AGENT_REFACTOR_PROPOSAL.md diff --git a/packages/server/AGENT_REFACTOR_PROPOSAL.md b/packages/server/AGENT_REFACTOR_PROPOSAL.md new file mode 100644 index 000000000..fbb02ac8e --- /dev/null +++ b/packages/server/AGENT_REFACTOR_PROPOSAL.md @@ -0,0 +1,143 @@ +# Agent Architecture Refactor Proposal + +## Current State Analysis + +### AgentSnapshot Usage + +| Location | Purpose | +| --- | --- | +| `src/server/agent/agent-manager.ts:231-247` | `getAgent` / `listAgents` expose `AgentSnapshot` copies generated via `toSnapshot`. | +| `src/server/session.ts:534-577, 1130-1145` | Session broadcasts snapshots to clients (`forwardAgentState`, `buildAgentPayload`). | +| `src/server/agent/agent-registry.ts:138-185` | `applySnapshot` is driven by snapshots (via `attachAgentRegistryPersistence`). | +| `src/server/persistence-hooks.ts:23-35` | Persistence hook subscribes to `agent_state` and forwards snapshots. | +| `src/server/agent/mcp-server.ts:12-452` | MCP endpoints serialize snapshots for diagnostics/listing. | +| `src/server/messages.ts:19-338` | `AgentSnapshotPayload` and `serializeAgentSnapshot` expect snapshots. | +| Tests (`src/server/persistence-hooks.test.ts`, `src/server/agent/agent-registry.test.ts`) | Fabricate snapshots as fixtures. | + +### Impacted Areas + +- `AgentManager` snapshot creation & event emission. +- Session serialization (`buildAgentPayload`, `forwardAgentState`). +- Registry persistence (`applySnapshot`, tests). +- MCP server responses. +- Messaging schema & helper utilities. + +### Why AgentSnapshot Existed + +It attempted to provide a JSON-friendly view stripped of internal handles (e.g., `AgentSession`, `pendingRun`). In practice it duplicates the data model, omits config (requiring `recordConfig`), and forces redundant copies. All downstream consumers just serialize the snapshot immediately, so it adds complexity without real protection. + +## Proposed Design + +### Single Source of Truth + +Introduce a discriminated union for `ManagedAgent` that encodes lifecycle-specific invariants: + +```ts +type ManagedAgent = + | { lifecycle: "initializing"; pendingRun: null; /* ... */ } + | { lifecycle: "running"; pendingRun: AsyncGenerator; /* ... */ } + | { lifecycle: "idle"; pendingRun: null; /* ... */ } + | { lifecycle: "error"; pendingRun: null; lastError: string; /* ... */ } + | { lifecycle: "closed"; session: null; pendingRun: null; /* ... */ }; +``` + +Fields include the live `AgentSession`, normalized config (`AgentSessionConfig`), timelines, permissions map, timestamps, persistence handles, etc. Impossible states (e.g., `pendingRun` non-null while lifecycle `"idle"`) become unrepresentable. + +### Pure Transformation Functions + +1. **Persistence Projection** + ```ts + function toStoredAgentRecord(agent: ManagedAgent, options?: { title?: string | null }): StoredAgentRecord + ``` + Copies provider, cwd, ISO timestamps, lifecycle status, `lastModeId`, complete config (`modeId`, `model`, `extra`), persistence handle, title metadata. + +2. **Client Payload Projection** + ```ts + function toAgentPayload(agent: ManagedAgent, options?: { title?: string | null }): AgentSnapshotPayload + ``` + Converts dates to ISO strings, normalizes pending permissions map to an array, includes safe config/model fields for UI, hides `AgentSession` reference. + +3. **Additional helpers** + Reuse the same projections for MCP responses, diagnostics, etc. The functions are deterministic and easy to test. + +### Unified Persistence Flow + +- Remove `AgentSnapshot` and `recordConfig`. +- `AgentManager.subscribe` emits `ManagedAgent` references (or read-only copies) on `agent_state`. +- `attachAgentRegistryPersistence` now calls `toStoredAgentRecord` and writes the result. This single path handles both lifecycle and config data atomically. +- Tests rely on the pure projection functions instead of crafting ad-hoc snapshots. + +### Lazy Initialization Strategy + +- `restorePersistedAgents` still reads `StoredAgentRecord` entries with full config populated by the projection. +- Lazy `ensureAgentLoaded` uses the stored record to resume the agent on demand; no special-case status bootstrapping required. +- When an agent is resumed/created, `AgentManager` emits `agent_state` events with the live `ManagedAgent`, ensuring persistence is updated immediately before any other code touches the registry. + +### Client Communication + +- Session and MCP server call `toAgentPayload` before emitting events. +- `AgentSnapshotPayload` remains the wire-format schema, but it’s derived directly from `ManagedAgent`. +- Clients continue to receive the same data shape (with potential additions, e.g., config info) without intermediate snapshot objects. + +## Risk Assessment + +| Risk | Mitigation | +| --- | --- | +| All consumer APIs expect `AgentSnapshot`. | Update type signatures and provide pure projection helpers; TypeScript will flag missing updates. | +| Accidentally exposing mutable internal state. | Projections must deep-clone arrays/maps; optionally expose read-only `ManagedAgentView` wrappers. | +| Persistence logic mistakes. | Add dedicated unit tests for `toStoredAgentRecord`; compare outputs with existing fixtures. | +| Client payload regressions. | Snapshot serialization tests (`serializeAgentSnapshot`, session tests) must be updated to use the new helper. | +| Integration behavior (lazy init/status updates). | Manual QA and e2e tests verifying “open agent after restart” scenarios. | + +### Test Coverage + +- Existing registry + persistence-hook tests already exercise serialization; they must be ported to the new helpers. +- Need new tests for `toStoredAgentRecord` and `toAgentPayload` to validate field-level correctness. +- Re-run session/MCP/e2e tests to ensure agent cards, timelines, and lazy loading still function. + +### Migration Strategy + +- Breaking change is acceptable (server + client change together). `agents.json` schema stays identical; only the producer path changes. +- Local migrations not required; new code overwrites entries with full config automatically. + +## Implementation Plan + +1. **Introduce transformation helpers & tests** + - Implement `toStoredAgentRecord` and `toAgentPayload`. + - Add unit tests covering all lifecycle variants and edge cases (pending permissions map, optional fields). + +2. **Refactor AgentManager emissions** + - Remove `AgentSnapshot` type and `toSnapshot`. + - Update `subscribe`, `getAgent`, `listAgents`, and event dispatchers to pass `ManagedAgent`. + - Ensure consumers cannot mutate returned references (clone or freeze if necessary). + +3. **Update consumers** + - **Registry:** `applySnapshot` now accepts `ManagedAgent` and uses `toStoredAgentRecord`. Delete `recordConfig`. + - **Session/MCP:** call `toAgentPayload` when broadcasting to clients; update serialized types. + - **Messages schema:** keep `AgentSnapshotPayload` but note it’s derived from `ManagedAgent`. + +4. **Cleanup** + - Remove `AgentSnapshot` definitions, serialization helpers, and obsolete code paths/tests. + - Ensure `AgentRegistry` no longer imports `AgentSnapshot`. + +5. **Validation** + - Run `npm run test agent-registry`, persistence hook tests, session tests. + - Manual QA: create agent → verify `agents.json` includes status & config; restart server → open agent → ensure UI shows “idle” immediately. + +## Validation Plan + +- **Unit Tests** + - `toStoredAgentRecord` and `toAgentPayload` coverage (all lifecycle states, optional fields, config propagation). + - Update registry/persistence tests to use `ManagedAgent` fixtures. + +- **Integration Tests** + - Session → client agent list (`SessionContext` expectations) to ensure payload format. + - Lazy init/resume scenario (open agent after restart without manual refresh). + - MCP “list agents” command returning full info. + +- **Manual Verification** + - Create agents with various configs/modes; inspect `agents.json`. + - Restart server; verify UI cards show accurate status and accept prompts immediately. + - High-load scenario: multiple agents running concurrently to ensure no races in persistence. + +This refactor removes redundant abstractions, consolidates persistence into deterministic pure functions, and keeps `ManagedAgent` as the authoritative state—from which every other representation (disk, UI, MCP) is derived.*** diff --git a/packages/server/src/server/agent/agent-registry.test.ts b/packages/server/src/server/agent/agent-registry.test.ts index 4785ff7b7..ad2fbed73 100644 --- a/packages/server/src/server/agent/agent-registry.test.ts +++ b/packages/server/src/server/agent/agent-registry.test.ts @@ -1,4 +1,4 @@ -import { describe, expect, test, beforeEach, afterEach } from "vitest"; +import { describe, expect, test, beforeEach, afterEach, vi } from "vitest"; import os from "node:os"; import path from "node:path"; import { mkdtempSync, rmSync, writeFileSync, readFileSync } from "node:fs"; @@ -50,6 +50,15 @@ describe("AgentRegistry", () => { }); test("persists configs and snapshot metadata", async () => { + await registry.applySnapshot( + createSnapshot({ + id: "agent-1", + cwd: "/tmp/project", + currentModeId: "coding", + status: "idle", + }) + ); + await registry.recordConfig( "agent-1", "claude", @@ -61,14 +70,6 @@ describe("AgentRegistry", () => { } ); - await registry.applySnapshot( - createSnapshot({ - id: "agent-1", - currentModeId: "coding", - status: "idle", - }) - ); - const records = await registry.list(); expect(records).toHaveLength(1); const [record] = records; @@ -102,7 +103,8 @@ describe("AgentRegistry", () => { expect(persisted?.title).toBe("Fix Login Bug"); }); - test("recordConfig seeds lastModeId before snapshots", async () => { + test("recordConfig warns if no snapshot exists", async () => { + const warnSpy = vi.spyOn(console, "warn").mockImplementation(() => {}); await registry.recordConfig( "agent-3", "claude", @@ -113,7 +115,9 @@ describe("AgentRegistry", () => { ); const record = await registry.get("agent-3"); - expect(record?.lastModeId).toBe("plan"); + expect(record).toBeNull(); + expect(warnSpy).toHaveBeenCalled(); + warnSpy.mockRestore(); }); test("recovers from trailing garbage in agents.json", async () => { diff --git a/packages/server/src/server/agent/agent-registry.ts b/packages/server/src/server/agent/agent-registry.ts index 1110623a2..031db7757 100644 --- a/packages/server/src/server/agent/agent-registry.ts +++ b/packages/server/src/server/agent/agent-registry.ts @@ -110,27 +110,26 @@ export class AgentRegistry { config?: SerializableAgentConfig ): Promise { await this.load(); - const now = new Date().toISOString(); const existing = this.cache.get(agentId); - const sanitizedConfig = config ? sanitizeConfig(config) : existing?.config; + if (!existing) { + console.warn( + `[AgentRegistry] Cannot record config for ${agentId} because no snapshot has been persisted yet` + ); + return; + } + + const now = new Date().toISOString(); + const sanitizedConfig = config ? sanitizeConfig(config) : existing.config; const nextModeId = - config?.modeId ?? - existing?.lastModeId ?? - sanitizedConfig?.modeId ?? - null; + config?.modeId ?? existing.lastModeId ?? sanitizedConfig?.modeId ?? null; + const updated: StoredAgentRecord = { - id: agentId, + ...existing, provider, cwd, - createdAt: existing?.createdAt ?? now, updatedAt: now, - lastActivityAt: existing?.lastActivityAt ?? existing?.updatedAt ?? now, - lastUserMessageAt: existing?.lastUserMessageAt ?? null, - title: existing?.title ?? null, - lastStatus: existing?.lastStatus ?? "closed", lastModeId: nextModeId, config: sanitizedConfig, - persistence: existing?.persistence ?? null, }; this.cache.set(agentId, updated); await this.flush();