Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
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: 1 addition & 1 deletion AGENTS.md
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ Hosts:
- `apps/code`: Electron desktop host.
- `apps/web`: web host and portability smoke test.
- `apps/mobile`: React Native host.
- `apps/cli`: thin shell over `@posthog/cli`.
- `packages/cli`: headless CLI (`@posthog/code-cli`, bin `posthog-code-cli`) — one-shot agent runs over the in-process ACP connection.

## Rules

Expand Down
4 changes: 4 additions & 0 deletions knip.json
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,10 @@
"project": ["src/**/*.ts", "bin/**/*.ts"],
"includeEntryExports": true
},
"packages/cli": {
"project": ["src/**/*.ts"],
"includeEntryExports": true
},
"packages/agent": {
"project": ["src/**/*.ts"],
"ignore": ["src/templates/**"],
Expand Down
1 change: 1 addition & 0 deletions packages/agent/src/adapters/acp-connection.ts
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,7 @@ function createClaudeConnection(config: AcpConnectionConfig): AcpConnection {
onStructuredOutput: config.onStructuredOutput,
posthogApiConfig: resolveEnricherApiConfig(config),
gatewayEnv: config.claudeGatewayEnv,
logger: config.logger?.child("ClaudeAcpAgent"),
});
return agent;
}, agentStream);
Expand Down
6 changes: 5 additions & 1 deletion packages/agent/src/adapters/claude/claude-agent.ts
Original file line number Diff line number Diff line change
Expand Up @@ -251,6 +251,8 @@ export interface ClaudeAcpAgentOptions {
posthogApiConfig?: PostHogAPIConfig;
/** Explicit gateway config — avoids global process.env mutation across concurrent sessions. */
gatewayEnv?: GatewayEnv;
/** Injected logger; defaults to a console logger with debug enabled. */
logger?: Logger;
}

export class ClaudeAcpAgent extends BaseAcpAgent {
Expand All @@ -274,7 +276,9 @@ export class ClaudeAcpAgent extends BaseAcpAgent {
this.toolUseCache = {};
this.emittedToolCalls = new Set();
this.toolUseStreamCache = new Map();
this.logger = new Logger({ debug: true, prefix: "[ClaudeAcpAgent]" });
this.logger =
options?.logger ??
new Logger({ debug: true, prefix: "[ClaudeAcpAgent]" });
Comment on lines +279 to +281

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.

Injecting the caller's logger reroutes ClaudeAcpAgent's internal debug logging into Desktop's persisted log sink and Cloud's SSE/log-writer stream, contradicting the PR's "no behavior change" claim

consider code_quality

Why we think it's a valid issue
  • Checked: the full logger chain — logger.ts:38-42 (emitLog skips the level/debugEnabled gate whenever onLog is set) and 72-79 (child() copies onLog); the PR diff (git show 988523a0) confirming acp-connection.ts:122 and claude-agent.ts:279-281 are both new; the Desktop caller chain (agent.ts:29-33 builds the logger with onLog:config.onLog, agent.ts:133 forwards it, workspace-server agent.ts:842-850 always sets onLog:this.onAgentLog, makeOnAgentLog:235-244 forwards unconditionally); the Cloud chain (agent-server.ts:1615 passes this.logger, 1822-1828 reassigns it with onLog:emitConsoleLog after the createAcpConnection call, cleanupSession:4452-4521 never resets this.logger, emitConsoleLog:455-482 broadcasts via SSE + logWriter.appendRawLine with no level filter).
  • Found: The mechanism is genuine and the PR's "Desktop and cloud behavior is unchanged since they don't pass one" is factually false — both hosts pass a logger carrying an onLog, and ClaudeAcpAgent's internal logs (previously console-only, discarded) now route to those sinks. Cloud is confirmed: on any session re-init on the same instance, the child logger inherits emitConsoleLog, so all-level chatter (including debug) is rebroadcast over SSE and persisted.
  • Impact: Consequence is diagnostic log verbosity, not a functional defect — no wrong results, data loss, contract break, secret leak, or leaked resource. Two facts cut the severity below the reviewer's should_fix: (1) the Desktop file transport level is isDev ? "debug" : "info" (apps/code/src/main/utils/logger.ts:63-64), so in production the 8 debug sites — including the per-loop "Idle without an active turn" at claude-agent.ts:991 the finding relies on — are filtered; only ~4 info + warn/error persist at per-session frequency into a rotated 10MB×3 log, making "regardless of isDevBuild()" and "every consumer-loop iteration" overstated. (2) The AcpConnection-level child logger already routed info logs to these same sinks pre-PR, so the change is incremental. Cloud SSE/log noise is real but bounded and only on the resume path.
  • Priority: Kept (real, reproducible, unintended, contradicts an explicit PR claim, untested) but down-ranked should_fix → consider: it is an observability/verbosity regression, not a correctness/security/data/contract/perf-at-scale/reliability defect, and its worst-case Desktop framing is rebutted by production-level log filtering.
Issue description

ClaudeAcpAgent now uses options?.logger ?? new Logger({ debug: true, prefix: "[ClaudeAcpAgent]" }), and createClaudeConnection in acp-connection.ts (line 122) passes config.logger?.child("ClaudeAcpAgent") whenever the caller supplied a logger. The PR description asserts "Desktop and cloud behavior is unchanged since they don't pass one" — but both existing hosts DO pass a logger: Agent.run() (packages/agent/src/agent.ts:133) forwards logger: this.logger, and AgentServer._doInitializeSession (packages/agent/src/server/agent-server.ts:1614) forwards logger: this.logger too. Logger.emitLog (utils/logger.ts) forwards unconditionally to onLog when one is set, bypassing the debugEnabled check entirely, and Logger.child() copies the parent's onLog. Concretely: on Desktop, Agent is always constructed with onLog: this.onAgentLog (packages/workspace-server/src/services/agent/agent.ts:850), which forwards every call straight into RootLogger — so every one of ClaudeAcpAgent's ~30 internal debug/info call sites (fired on essentially every consumer-loop iteration: idle detection, compaction, MCP reconnects, turn activation, etc.) now gets persisted to the on-disk desktop log file on every single task run, regardless of isDevBuild(), where previously they only reached console.* and were discarded. On the cloud sandbox, AgentServer's own this.logger only gains an onLog of emitConsoleLog partway through _doInitializeSession (agent-server.ts:1822-1828, AFTER the createAcpConnection call at line 1609) — but that mutated field is never reset in cleanupSession(), so on any session re-init on the same instance (the code explicitly supports "warm reconnects... sandbox restart with snapshot resume"), the next createAcpConnection call reuses the already-mutated logger, and ClaudeAcpAgent's full internal debug chatter then gets broadcast as _posthog/console SSE notifications to the client AND persisted via session.logWriter.appendRawLine for every session after the first on that instance.

Suggested fix

Verify and, if unintended, avoid coupling ClaudeAcpAgent's own internal logger to the caller-supplied onLog sink. Either keep ClaudeAcpAgent's console-only isolation by default and have the CLI opt in with its own dedicated logger construction (not by threading the host's general-purpose logger through AcpConnectionConfig.logger), or explicitly measure/bound the log volume impact on Desktop (disk usage per run) and Cloud (SSE bandwidth + logWriter storage per session-resume) before merging, since neither is covered by the tests described in the PR ("39 unit tests... Full @posthog/agent suite" — none of which exercise Desktop's onAgentLog wiring or Cloud's session-resume createAcpConnection reuse path).

Prompt to fix with AI (copy-paste)
## Context
@packages/agent/src/adapters/claude/claude-agent.ts#L279-281

<issue_description>
`ClaudeAcpAgent` now uses `options?.logger ?? new Logger({ debug: true, prefix: "[ClaudeAcpAgent]" })`, and `createClaudeConnection` in acp-connection.ts (line 122) passes `config.logger?.child("ClaudeAcpAgent")` whenever the caller supplied a logger. The PR description asserts "Desktop and cloud behavior is unchanged since they don't pass one" — but both existing hosts DO pass a logger: `Agent.run()` (packages/agent/src/agent.ts:133) forwards `logger: this.logger`, and `AgentServer._doInitializeSession` (packages/agent/src/server/agent-server.ts:1614) forwards `logger: this.logger` too. `Logger.emitLog` (utils/logger.ts) forwards unconditionally to `onLog` when one is set, bypassing the `debugEnabled` check entirely, and `Logger.child()` copies the parent's `onLog`. Concretely: on Desktop, `Agent` is always constructed with `onLog: this.onAgentLog` (packages/workspace-server/src/services/agent/agent.ts:850), which forwards every call straight into RootLogger — so every one of ClaudeAcpAgent's ~30 internal debug/info call sites (fired on essentially every consumer-loop iteration: idle detection, compaction, MCP reconnects, turn activation, etc.) now gets persisted to the on-disk desktop log file on every single task run, regardless of `isDevBuild()`, where previously they only reached `console.*` and were discarded. On the cloud sandbox, `AgentServer`'s own `this.logger` only gains an `onLog` of `emitConsoleLog` partway through `_doInitializeSession` (agent-server.ts:1822-1828, AFTER the `createAcpConnection` call at line 1609) — but that mutated field is never reset in `cleanupSession()`, so on any session re-init on the same instance (the code explicitly supports "warm reconnects... sandbox restart with snapshot resume"), the *next* `createAcpConnection` call reuses the already-mutated logger, and ClaudeAcpAgent's full internal debug chatter then gets broadcast as `_posthog/console` SSE notifications to the client AND persisted via `session.logWriter.appendRawLine` for every session after the first on that instance.
</issue_description>

<issue_validation>
- **Checked:** the full logger chain — `logger.ts:38-42` (`emitLog` skips the level/`debugEnabled` gate whenever `onLog` is set) and `72-79` (`child()` copies `onLog`); the PR diff (`git show 988523a0`) confirming acp-connection.ts:122 and claude-agent.ts:279-281 are both new; the Desktop caller chain (agent.ts:29-33 builds the logger with `onLog:config.onLog`, agent.ts:133 forwards it, workspace-server agent.ts:842-850 always sets `onLog:this.onAgentLog`, makeOnAgentLog:235-244 forwards unconditionally); the Cloud chain (agent-server.ts:1615 passes `this.logger`, 1822-1828 reassigns it with `onLog:emitConsoleLog` *after* the createAcpConnection call, cleanupSession:4452-4521 never resets `this.logger`, emitConsoleLog:455-482 broadcasts via SSE + `logWriter.appendRawLine` with no level filter).
- **Found:** The mechanism is genuine and the PR's "Desktop and cloud behavior is unchanged since they don't pass one" is factually false — both hosts pass a logger carrying an `onLog`, and ClaudeAcpAgent's internal logs (previously console-only, discarded) now route to those sinks. Cloud is confirmed: on any session *re-init* on the same instance, the child logger inherits `emitConsoleLog`, so all-level chatter (including debug) is rebroadcast over SSE and persisted.
- **Impact:** Consequence is diagnostic log *verbosity*, not a functional defect — no wrong results, data loss, contract break, secret leak, or leaked resource. Two facts cut the severity below the reviewer's `should_fix`: (1) the Desktop file transport level is `isDev ? "debug" : "info"` (apps/code/src/main/utils/logger.ts:63-64), so in production the 8 `debug` sites — including the per-loop "Idle without an active turn" at claude-agent.ts:991 the finding relies on — are filtered; only ~4 `info` + warn/error persist at per-session frequency into a rotated 10MB×3 log, making "regardless of isDevBuild()" and "every consumer-loop iteration" overstated. (2) The AcpConnection-level child logger already routed info logs to these same sinks pre-PR, so the change is incremental. Cloud SSE/log noise is real but bounded and only on the resume path.
- **Priority:** Kept (real, reproducible, unintended, contradicts an explicit PR claim, untested) but down-ranked should_fix → consider: it is an observability/verbosity regression, not a correctness/security/data/contract/perf-at-scale/reliability defect, and its worst-case Desktop framing is rebutted by production-level log filtering.
</issue_validation>

## Task
Investigate the issue and solve it

<potential_solution>
Verify and, if unintended, avoid coupling ClaudeAcpAgent's own internal logger to the caller-supplied `onLog` sink. Either keep ClaudeAcpAgent's console-only isolation by default and have the CLI opt in with its own dedicated logger construction (not by threading the host's general-purpose logger through `AcpConnectionConfig.logger`), or explicitly measure/bound the log volume impact on Desktop (disk usage per run) and Cloud (SSE bandwidth + logWriter storage per session-resume) before merging, since neither is covered by the tests described in the PR ("39 unit tests... Full @posthog/agent suite" — none of which exercise Desktop's `onAgentLog` wiring or Cloud's session-resume `createAcpConnection` reuse path).
</potential_solution>

this.enrichment = createEnrichment(options?.posthogApiConfig, this.logger);
}

Expand Down
82 changes: 82 additions & 0 deletions packages/cli/e2e/cli.e2e.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,82 @@
import { execFile, execFileSync } from "node:child_process";
import {
existsSync,
mkdtempSync,
realpathSync,
rmSync,
writeFileSync,
} from "node:fs";
import { tmpdir } from "node:os";
import { join, resolve } from "node:path";
import { promisify } from "node:util";
import { describe, expect, it } from "vitest";

const execFileAsync = promisify(execFile);

// Same env contract as packages/agent/e2e/config.ts: a local llm-gateway plus
// a personal API key. Without the token the suite self-skips (never silent).
const GATEWAY_URL =
process.env.POSTHOG_CODE_E2E_GATEWAY_URL || "http://localhost:3308/ci";
const TOKEN = process.env.POSTHOG_CODE_E2E_GATEWAY_PERSONAL_API_KEY ?? "";
const MODEL = process.env.POSTHOG_CODE_E2E_CLAUDE_MODEL || "claude-haiku-4-5";

const CLI_PATH = resolve(__dirname, "../dist/cli.js");

function setupRepo(): string {
// realpath: on macOS os.tmpdir() is a symlink and the SDK keys its session
// store by resolved path.
const repo = realpathSync(mkdtempSync(join(tmpdir(), "code-cli-e2e-")));
writeFileSync(join(repo, "hello.txt"), "hello\n");
execFileSync("git", ["init", "-q"], { cwd: repo });
execFileSync("git", ["add", "-A"], { cwd: repo });
execFileSync(
"git",
[
"-c",
"commit.gpgsign=false",
"-c",
"user.email=e2e@posthog.dev",
"-c",
"user.name=e2e",
"commit",
"-qm",
"init",
],
{ cwd: repo },
);
return repo;
}

describe.skipIf(!TOKEN)("posthog-code-cli", () => {
it("runs one turn and prints the answer", async () => {
expect(
existsSync(CLI_PATH),
`CLI not built at ${CLI_PATH} — run pnpm --filter @posthog/code-cli build first`,
).toBe(true);
const repo = setupRepo();
try {
const { stdout } = await execFileAsync(
process.execPath,
[
CLI_PATH,
"Reply with exactly the word OK and nothing else.",
"--cwd",
repo,
"--model",
MODEL,
],
{
env: {
...process.env,
ANTHROPIC_BASE_URL: GATEWAY_URL,
ANTHROPIC_AUTH_TOKEN: TOKEN,
},
timeout: 240_000,
},
);
expect(stdout).toContain("OK");
} finally {
rmSync(repo, { recursive: true, force: true });
}
});
});
33 changes: 33 additions & 0 deletions packages/cli/package.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
{
"name": "@posthog/code-cli",
"version": "0.0.0-dev",
"private": true,
"description": "Headless CLI for PostHog Code — one-shot agent runs over the in-process ACP connection",
"type": "module",
"bin": {
"posthog-code-cli": "./dist/cli.js"
},
"scripts": {
"build": "node ../../scripts/rimraf.mjs dist && tsup",
"typecheck": "pnpm exec tsc --noEmit",
"test": "vitest run",
"test:watch": "vitest",
"test:e2e": "vitest run --config vitest.e2e.config.ts",
"clean": "node ../../scripts/rimraf.mjs dist .turbo"
},
"engines": {
"node": ">=20.0.0"
},
"dependencies": {
"@agentclientprotocol/sdk": "1.1.0",
"@posthog/agent": "workspace:*",
"@posthog/shared": "workspace:*",
"commander": "^14.0.2"
},
"devDependencies": {
"@types/node": "catalog:",
"tsup": "catalog:",
"typescript": "catalog:",
"vitest": "^4.1.8"
}
}
145 changes: 145 additions & 0 deletions packages/cli/src/args.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,145 @@
import { mkdtempSync, realpathSync } from "node:fs";
import { tmpdir } from "node:os";
import { join } from "node:path";
import { describe, expect, it } from "vitest";
import { parseCliArgs } from "./args";

// argv passed to parseCliArgs mirrors process.argv: [node, script, ...args].
function argv(...args: string[]): string[] {
return ["/usr/bin/node", "/path/to/posthog-code-cli", ...args];
}

describe("parseCliArgs", () => {
describe("defaults", () => {
it("returns default options when no flags or prompt are given", () => {
const result = parseCliArgs(argv());

expect("error" in result).toBe(false);
if ("error" in result) return;
expect(result.prompt).toBeUndefined();
expect(result.cwd).toBe(realpathSync(process.cwd()));
expect(result.permissionMode).toBe("auto");
expect(result.output).toBe("text");
expect(result.debug).toBe(false);
});
});

it("parses the prompt positional", () => {
const result = parseCliArgs(argv("Fix the failing test"));

expect("error" in result).toBe(false);
if ("error" in result) return;
expect(result.prompt).toBe("Fix the failing test");
});

describe("individual flags", () => {
it("parses --cwd to the realpath of a valid directory", () => {
const tmp = mkdtempSync(join(tmpdir(), "cli-args-test-"));
const result = parseCliArgs(argv("--cwd", tmp));

expect("error" in result).toBe(false);
if ("error" in result) return;
expect(result.cwd).toBe(realpathSync(tmp));
});

it("parses --permission-mode bypassPermissions", () => {
const result = parseCliArgs(
argv("--permission-mode", "bypassPermissions"),
);

expect("error" in result).toBe(false);
if ("error" in result) return;
expect(result.permissionMode).toBe("bypassPermissions");
});

it("parses a valid --model id", () => {
const result = parseCliArgs(argv("--model", "claude-opus-4"));

expect("error" in result).toBe(false);
if ("error" in result) return;
expect(result.model).toBe("claude-opus-4");
});

it("parses --system-prompt as-is", () => {
const result = parseCliArgs(
argv("--system-prompt", "Respond only in haiku"),
);

expect("error" in result).toBe(false);
if ("error" in result) return;
expect(result.systemPrompt).toBe("Respond only in haiku");
});

it("parses --output json", () => {
const result = parseCliArgs(argv("--output", "json"));

expect("error" in result).toBe(false);
if ("error" in result) return;
expect(result.output).toBe("json");
});

it("parses --debug as true when present", () => {
const result = parseCliArgs(argv("--debug"));

expect("error" in result).toBe(false);
if ("error" in result) return;
expect(result.debug).toBe(true);
});
});

describe("invalid --permission-mode values", () => {
it.each(["default", "plan", "acceptEdits", "yolo"])(
"rejects %j with an error result",
(mode) => {
const result = parseCliArgs(argv("--permission-mode", mode));

expect("error" in result).toBe(true);
if (!("error" in result)) return;
expect(typeof result.error).toBe("string");
expect(result.error.length).toBeGreaterThan(0);
},
);
});

describe("invalid --model prefixes", () => {
it.each(["gpt-4", "llama-3"])(
"rejects %j since it does not start with claude-",
(model) => {
const result = parseCliArgs(argv("--model", model));

expect("error" in result).toBe(true);
if (!("error" in result)) return;
expect(typeof result.error).toBe("string");
expect(result.error.length).toBeGreaterThan(0);
},
);
});

it("rejects an invalid --output value", () => {
const result = parseCliArgs(argv("--output", "yaml"));

expect("error" in result).toBe(true);
if (!("error" in result)) return;
expect(typeof result.error).toBe("string");
expect(result.error.length).toBeGreaterThan(0);
});

it("rejects a --cwd path that does not exist", () => {
const bogusPath = join(tmpdir(), "definitely-does-not-exist-cli-args-test");
const result = parseCliArgs(argv("--cwd", bogusPath));

expect("error" in result).toBe(true);
if (!("error" in result)) return;
expect(typeof result.error).toBe("string");
expect(result.error.length).toBeGreaterThan(0);
});

it("accepts a --cwd path pointing to a real directory", () => {
const tmp = mkdtempSync(join(tmpdir(), "cli-args-test-"));
const result = parseCliArgs(argv("--cwd", tmp));

expect("error" in result).toBe(false);
if ("error" in result) return;
expect(result.cwd).toBe(realpathSync(tmp));
});
});
Loading
Loading