Skip to content

refactor(agent-chat): extract saga to core AgentChatService + review fixes + comment trim #2781

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
benjackwhite wants to merge 2 commits into
base: posthog-code/m3a-final
Choose a base branch
from
+848 −763
Open

refactor(agent-chat): extract saga to core AgentChatService + review fixes + comment trim #2781

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
bf05129
refactor(agent-chat): extract useAgentChat saga into core AgentChatSe…
benjackwhite Jun 19, 2026
f5edcd8
style(agent-platform): trim redundant/verbose comments in PR-added files
benjackwhite Jun 19, 2026
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
2 changes: 2 additions & 0 deletions apps/code/src/renderer/desktop-contributions.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import { agentChatCoreModule } from "@posthog/core/agent-chat/agentChat.module";
import { billingCoreModule } from "@posthog/core/billing/billing.module";
import { inboxCoreModule } from "@posthog/core/inbox/inbox.module";
import { githubConnectModule } from "@posthog/core/integrations/githubConnect.module";
Expand Down Expand Up @@ -25,6 +26,7 @@ import { container } from "@renderer/di/container";

export function registerDesktopContributions(): void {
for (const module of [
agentChatCoreModule,
agentUiModule,
authUiModule,
billingUiModule,
Expand Down
15 changes: 9 additions & 6 deletions packages/api-client/src/agent-analytics.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,12 +31,15 @@ describe("buildAgentAnalyticsQueries", () => {
});

it("narrows to a single application id when given", () => {
const q = buildAgentAnalyticsQueries("app-uuid-123");
expect(q.kpi).toContain(
"properties.$agent_application_id = 'app-uuid-123'",
);
expect(q.byModel).toContain(
"properties.$agent_application_id = 'app-uuid-123'",
const id = "11111111-2222-3333-4444-555566667777";
const q = buildAgentAnalyticsQueries(id);
expect(q.kpi).toContain(`properties.$agent_application_id = '${id}'`);
expect(q.byModel).toContain(`properties.$agent_application_id = '${id}'`);
});

it("rejects a non-uuid application id", () => {
expect(() => buildAgentAnalyticsQueries("app-uuid-123")).toThrow(
/must be a UUID/,
);
});
Comment on lines +40 to 44

@greptile-apps greptile-apps Bot Jun 19, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 The new UUID rejection test uses a single hard-coded invalid input. Per the team's preference for parameterised tests, this should cover multiple invalid patterns — partial UUIDs, empty strings, and injection-shaped strings are all worth asserting against to ensure the guard is robust.

Suggested change
it("rejects a non-uuid application id", () => {
expect(() => buildAgentAnalyticsQueries("app-uuid-123")).toThrow(
/must be a UUID/,
);
});
it.each([
["non-uuid slug", "app-uuid-123"],
["empty string", ""],
["partial uuid", "11111111-2222-3333-4444"],
["injection attempt", "1'; DROP TABLE--"],
])("rejects %s as application id", (_, id) => {
expect(() => buildAgentAnalyticsQueries(id)).toThrow(/must be a UUID/);
});
Prompt To Fix With AI 
This is a comment left during a code review.
Path: packages/api-client/src/agent-analytics.test.ts
Line: 40-44

Comment:
The new UUID rejection test uses a single hard-coded invalid input. Per the team's preference for parameterised tests, this should cover multiple invalid patterns — partial UUIDs, empty strings, and injection-shaped strings are all worth asserting against to ensure the guard is robust.

```suggestion
  it.each([
    ["non-uuid slug", "app-uuid-123"],
    ["empty string", ""],
    ["partial uuid", "11111111-2222-3333-4444"],
    ["injection attempt", "1'; DROP TABLE--"],
  ])("rejects %s as application id", (_, id) => {
    expect(() => buildAgentAnalyticsQueries(id)).toThrow(/must be a UUID/);
  });
```

How can I resolve this? If you propose a fix, please make it concise.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

});
Expand Down
11 changes: 9 additions & 2 deletions packages/api-client/src/agent-analytics.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,11 +36,18 @@ export interface AgentAnalyticsRaw {
/** Only the agents' own traffic — not the team's other LLM events. */
const AGENT_ORIGIN = "properties.$ai_origin = 'agent_platform_runner'";

const UUID_RE =
/^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/i;

/**
* Shared WHERE scope. `applicationId` (a trusted UUID from the agent record)
* narrows the board to a single agent for the per-agent Observability tab.
* Shared WHERE scope narrowing the board to a single agent. `applicationId` is
* a trusted server UUID, but reject anything non-UUID before interpolating it
* into HogQL rather than rely on that.
*/
function scope(applicationId?: string): string {
if (applicationId && !UUID_RE.test(applicationId)) {
throw new Error("agent analytics: applicationId must be a UUID");
}
const agent = applicationId
? ` AND properties.$agent_application_id = '${applicationId}'`
: "";
Expand Down
8 changes: 8 additions & 0 deletions packages/core/src/agent-chat/agentChat.module.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
import { ContainerModule } from "inversify";
import { AgentChatService } from "./agentChatService";
import { AGENT_CHAT_SERVICE } from "./identifiers";

export const agentChatCoreModule = new ContainerModule(({ bind }) => {
bind(AgentChatService).toSelf().inSingletonScope();
bind(AGENT_CHAT_SERVICE).toService(AgentChatService);
});
83 changes: 36 additions & 47 deletions ...-applications/hooks/useAgentChat.test.tsx → ...e/src/agent-chat/agentChatService.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,13 @@
import { agentChatStore } from "@posthog/core/agent-chat/agentChatStore";
import type { PostHogAPIClient } from "@posthog/api-client/posthog-client";
import type { AgentSessionEvent } from "@posthog/shared/agent-platform-types";
import { act, renderHook } from "@testing-library/react";
import { afterEach, beforeEach, describe, expect, it, vi } from "vitest";
import { AgentChatService } from "./agentChatService";
import { agentChatStore } from "./agentChatStore";
import type { AgentChatSession } from "./identifiers";

const INGRESS = "https://ingress.example";
const SLUG = "my-agent";
const SESSION = "sess-1";

const mockClient = vi.hoisted(() => ({
runAgentSession: vi.fn(),
Expand All @@ -13,18 +19,24 @@ const mockClient = vi.hoisted(() => ({
sendAgentInteractiveToolResult: vi.fn(),
mintAgentPreviewToken: vi.fn(),
}));
vi.mock("@posthog/ui/features/auth/authClient", () => ({
useAuthenticatedClient: () => mockClient,
}));
vi.mock("@posthog/ui/primitives/toast", () => ({
toast: { error: vi.fn(), warning: vi.fn(), info: vi.fn() },
}));
const client = mockClient as unknown as PostHogAPIClient;

import { useAgentChat } from "./useAgentChat";

const INGRESS = "https://ingress.example";
const SLUG = "my-agent";
const SESSION = "sess-1";
function session(chatId: string): AgentChatSession {
return {
chatId,
agentSlug: SLUG,
ingressBaseUrl: INGRESS,
revisionId: null,
createMapper: () => ({
seedUserMessage: () => [],
setPromptIdBase: () => {},
apply: () => [],
}),
resolveClientTool: async () => null,
buildWireText: (text) => text,
mapConversation: () => [],
};
}

function ev(kind: AgentSessionEvent["kind"], data: unknown): AgentSessionEvent {
return {
Expand All @@ -46,16 +58,13 @@ async function* streamThenDrop(...events: AgentSessionEvent[]) {
throw new Error("network reset");
}

function render(chatId: string) {
return renderHook(() =>
useAgentChat({ chatId, agentSlug: SLUG, ingressBaseUrl: INGRESS }),
);
}
describe("AgentChatService /listen reconnect", () => {
let service: AgentChatService;

describe("useAgentChat /listen reconnect", () => {
beforeEach(() => {
vi.clearAllMocks();
vi.useFakeTimers();
service = new AgentChatService();
mockClient.runAgentSession.mockResolvedValue({ session_id: SESSION });
});
afterEach(() => {
Expand All @@ -70,15 +79,10 @@ describe("useAgentChat /listen reconnect", () => {
)
.mockImplementationOnce(() => streamOf(ev("completed", {})));

const { result } = render(chatId);
await act(async () => {
void result.current.send("go");
});
void service.send(client, session(chatId), "go");
// Let start() + the first pump (delta then drop) settle, then clear the
// reconnect backoff so the second attach runs.
await act(async () => {
await vi.advanceTimersByTimeAsync(1000);
});
await vi.advanceTimersByTimeAsync(1000);

expect(mockClient.streamAgentSession).toHaveBeenCalledTimes(2);
// A live drop never asks the api whether the session ended — the re-attach
Expand All @@ -100,13 +104,8 @@ describe("useAgentChat /listen reconnect", () => {
conversation: [],
});

const { result } = render(chatId);
await act(async () => {
void result.current.send("go");
});
await act(async () => {
await vi.advanceTimersByTimeAsync(0);
});
void service.send(client, session(chatId), "go");
await vi.advanceTimersByTimeAsync(0);

// Silent re-attach → we ask the api, see it's terminal, and stop. No retry,
// no error.
Expand Down Expand Up @@ -134,13 +133,8 @@ describe("useAgentChat /listen reconnect", () => {
},
);

const { result } = render(chatId);
await act(async () => {
void result.current.send("go");
});
await act(async () => {
await vi.advanceTimersByTimeAsync(0);
});
void service.send(client, session(chatId), "go");
await vi.advanceTimersByTimeAsync(0);

const chat = agentChatStore.getState().chats[chatId];
expect(chat?.status).toBe("completed");
Expand All @@ -162,14 +156,9 @@ describe("useAgentChat /listen reconnect", () => {
conversation: [],
});

const { result } = render(chatId);
await act(async () => {
void result.current.send("go");
});
void service.send(client, session(chatId), "go");
// Drain the full capped-exponential backoff schedule (≈23.5s).
await act(async () => {
await vi.advanceTimersByTimeAsync(30_000);
});
await vi.advanceTimersByTimeAsync(30_000);

// Initial attach + MAX_LISTEN_RECONNECTS (6) re-attaches.
expect(mockClient.streamAgentSession).toHaveBeenCalledTimes(7);
Expand Down
Loading
Loading