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
6 changes: 5 additions & 1 deletion AGENTS.md
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,11 @@ 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`.

Executable packages own a `bin` rather than an `apps/*` host shell. They boot the same packages a host does, without a UI:

- `packages/cli`: headless CLI (`@posthog/code-cli`, bin `posthog-code-cli`) for one-shot agent runs over the in-process ACP connection.
- `packages/harness`: `@posthog/harness` (bin `harness`, `hog`), which spawns the pi.dev coding agent against the PostHog LLM gateway.

## Rules

Expand Down
5 changes: 2 additions & 3 deletions knip.json
Original file line number Diff line number Diff line change
Expand Up @@ -29,9 +29,8 @@
"@vitest/coverage-v8"
]
},
"apps/cli": {
"entry": ["src/cli.ts"],
"project": ["src/**/*.ts", "bin/**/*.ts"],
"packages/cli": {
"project": ["src/**/*.ts"],
"includeEntryExports": true
},
"packages/agent": {
Expand Down
4 changes: 4 additions & 0 deletions packages/agent/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,10 @@
"types": "./dist/execution-mode.d.ts",
"import": "./dist/execution-mode.js"
},
"./unattended-permission-policy": {
"types": "./dist/unattended-permission-policy.d.ts",
"import": "./dist/unattended-permission-policy.js"
},
"./resume": {
"types": "./dist/resume.d.ts",
"import": "./dist/resume.js"
Expand Down
73 changes: 73 additions & 0 deletions packages/agent/src/adapters/acp-connection.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,73 @@
import { beforeEach, describe, expect, it, vi } from "vitest";

const claudeAgentConstructor = vi.fn();

vi.mock("./claude/claude-agent", () => ({
ClaudeAcpAgent: class {
constructor(...args: unknown[]) {
claudeAgentConstructor(...args);
}
// Called by the connection's cleanup.
async closeSession(): Promise<void> {}
},
}));

const { createAcpConnection } = await import("./acp-connection");
const { Logger } = await import("../utils/logger");

/**
* The Claude adapter's own diagnostics include whole payloads: the expanded body
* of a slash command, raw tool inputs, the subprocess's stderr. A host logger
* usually carries an `onLog` that persists or transmits what it receives (on
* desktop, electron-log's file and OTLP transports; on cloud, the run log and
* the user's task feed), so forwarding is opt-in.
*/
describe("createAcpConnection adapter log forwarding", () => {
beforeEach(() => {
claudeAgentConstructor.mockClear();
});

function adapterOptions(): { logger?: unknown } {
// AgentSideConnection builds the agent eagerly, so one call is recorded by
// the time createAcpConnection returns.
expect(claudeAgentConstructor).toHaveBeenCalledTimes(1);
return claudeAgentConstructor.mock.calls[0][1] as { logger?: unknown };
}

it("withholds the host logger from the adapter by default", async () => {
const connection = createAcpConnection({
adapter: "claude",
logger: new Logger({ debug: true, onLog: () => {} }),
});
try {
expect(adapterOptions().logger).toBeUndefined();
} finally {
await connection.cleanup();
}
});

it("passes a scoped child logger when the host opts in", async () => {
const connection = createAcpConnection({
adapter: "claude",
logger: new Logger({ debug: true, onLog: () => {} }),
forwardAdapterLogs: true,
});
try {
expect(adapterOptions().logger).toBeInstanceOf(Logger);
} finally {
await connection.cleanup();
}
});

it("passes no logger when the host opts in without supplying one", async () => {
const connection = createAcpConnection({
adapter: "claude",
forwardAdapterLogs: true,
});
try {
expect(adapterOptions().logger).toBeUndefined();
} finally {
await connection.cleanup();
}
});
});
12 changes: 12 additions & 0 deletions packages/agent/src/adapters/acp-connection.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,15 @@ export type AcpConnectionConfig = {
/** Deployment environment - "local" for desktop, "cloud" for cloud sandbox */
deviceType?: "local" | "cloud";
logger?: Logger;
/**
* Route the Claude adapter's own diagnostics through `logger` instead of its
* console fallback. Off by default: adapter-internal logging includes whole
* payloads (expanded slash-command output, tool inputs, subprocess stderr),
* and a host `logger` typically carries an `onLog` that persists or transmits
* what it receives. Only a host whose sink is the operator's own terminal
* should turn this on.
*/
forwardAdapterLogs?: boolean;
processCallbacks?: ProcessSpawnedCallback;
codexOptions?: CodexOptions;
codexModels?: ReadonlyArray<ModelInfo>;
Expand Down Expand Up @@ -119,6 +128,9 @@ function createClaudeConnection(config: AcpConnectionConfig): AcpConnection {
onStructuredOutput: config.onStructuredOutput,
posthogApiConfig: resolveEnricherApiConfig(config),
gatewayEnv: config.claudeGatewayEnv,
logger: config.forwardAdapterLogs
? config.logger?.child("ClaudeAcpAgent")
: undefined,
});
return agent;
}, agentStream);
Expand Down
17 changes: 16 additions & 1 deletion packages/agent/src/adapters/claude/claude-agent.ts
Original file line number Diff line number Diff line change
Expand Up @@ -256,6 +256,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 @@ -279,7 +281,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 +284 to +286

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 Expand Up @@ -2278,6 +2282,17 @@ export class ClaudeAcpAgent extends BaseAcpAgent {
settingsManager.getSettings().model,
]);
modelOptions.currentModelId = resolvedModelId;
// A requested id that isn't available falls back silently, which reads as
// "the model I asked for" to a caller that can't see the allowed set (a
// headless CLI run, a scripted session). Say so.
const requestedModelId = meta?.model?.trim();
if (requestedModelId && requestedModelId !== resolvedModelId) {
this.logger.warn("Requested model is unavailable; using another", {
requested: requestedModelId,
resolved: resolvedModelId,
available: modelOptions.options.map((opt) => opt.value),
});
}
session.modelId = resolvedModelId;
session.lastContextWindowSize =
meta?.contextWindow === "200k"
Expand Down
3 changes: 3 additions & 0 deletions packages/agent/src/agent.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ export class Agent {
private sessionLogWriter?: SessionLogWriter;
private posthogApiConfig?: AgentConfig["posthog"];
private enricherEnabled: boolean;
private forwardAdapterLogs: boolean;

constructor(config: AgentConfig) {
this.logger = new Logger({
Expand All @@ -37,6 +38,7 @@ export class Agent {
this.posthogApiConfig = config.posthog;
}
this.enricherEnabled = config.enricher?.enabled !== false;
this.forwardAdapterLogs = config.forwardAdapterLogs === true;

if (config.posthog && !config.skipLogPersistence) {
this.sessionLogWriter = new SessionLogWriter({
Expand Down Expand Up @@ -131,6 +133,7 @@ export class Agent {
taskId,
deviceType: "local",
logger: this.logger,
forwardAdapterLogs: this.forwardAdapterLogs,
processCallbacks: options.processCallbacks,
onStructuredOutput: options.onStructuredOutput,
codexModels,
Expand Down
7 changes: 7 additions & 0 deletions packages/agent/src/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,13 @@ export interface AgentConfig {
enricher?: { enabled?: boolean };
debug?: boolean;
onLog?: OnLogCallback;
/**
* Send the adapter's own diagnostics to `onLog` too. Off by default because
* those lines carry whole payloads (expanded slash-command output, tool
* inputs, subprocess stderr) and most hosts persist or transmit what `onLog`
* receives. Turn it on only when the sink is the operator's own terminal.
*/
forwardAdapterLogs?: boolean;
}

// Device info for tracking where work happens
Expand Down
Loading
Loading