diff --git a/packages/server/src/server/agent/providers/claude-agent.test.ts b/packages/server/src/server/agent/providers/claude-agent.test.ts index 911466377..f0d950017 100644 --- a/packages/server/src/server/agent/providers/claude-agent.test.ts +++ b/packages/server/src/server/agent/providers/claude-agent.test.ts @@ -2,7 +2,6 @@ import { afterAll, beforeAll, describe, expect, test, vi } from "vitest"; import { createServer } from "http"; import { randomUUID } from "node:crypto"; import { - copyFileSync, existsSync, mkdtempSync, readFileSync, @@ -17,6 +16,7 @@ import { StreamableHTTPServerTransport } from "@modelcontextprotocol/sdk/server/ import { isInitializeRequest } from "@modelcontextprotocol/sdk/types.js"; import { ClaudeAgentClient, convertClaudeHistoryEntry } from "./claude-agent.js"; +import { useTempClaudeConfigDir } from "../../test-utils/claude-config.js"; import { hydrateStreamState, type StreamItem, @@ -46,46 +46,6 @@ function tmpCwd(): string { } } -function copyClaudeCredentials(sourceDir: string, targetDir: string): void { - const sourceCredentials = path.join(sourceDir, ".credentials.json"); - if (!existsSync(sourceCredentials)) { - return; - } - copyFileSync(sourceCredentials, path.join(targetDir, ".credentials.json")); -} - -function useTempClaudeConfigDir(): () => void { - const previousConfigDir = process.env.CLAUDE_CONFIG_DIR; - const sourceConfigDir = - previousConfigDir ?? path.join(os.homedir(), ".claude"); - const configDir = mkdtempSync(path.join(os.tmpdir(), "claude-config-")); - const settings = { - permissions: { - allow: [], - deny: [], - ask: ["Bash(rm:*)"], - additionalDirectories: [], - }, - sandbox: { - enabled: true, - autoAllowBashIfSandboxed: false, - }, - }; - const settingsText = `${JSON.stringify(settings, null, 2)}\n`; - writeFileSync(path.join(configDir, "settings.json"), settingsText, "utf8"); - writeFileSync(path.join(configDir, "settings.local.json"), settingsText, "utf8"); - copyClaudeCredentials(sourceConfigDir, configDir); - process.env.CLAUDE_CONFIG_DIR = configDir; - return () => { - if (previousConfigDir === undefined) { - delete process.env.CLAUDE_CONFIG_DIR; - } else { - process.env.CLAUDE_CONFIG_DIR = previousConfigDir; - } - rmSync(configDir, { recursive: true, force: true }); - }; -} - async function autoApprove(session: Awaited>, event: AgentStreamEvent) { if (event.type === "permission_requested") { await session.respondToPermission(event.request.id, { behavior: "allow" }); diff --git a/packages/server/src/server/daemon.e2e.test.ts b/packages/server/src/server/daemon.e2e.test.ts index aaf0ab67e..2db880a89 100644 --- a/packages/server/src/server/daemon.e2e.test.ts +++ b/packages/server/src/server/daemon.e2e.test.ts @@ -1,10 +1,11 @@ -import { describe, test, expect, beforeEach, afterEach } from "vitest"; +import { describe, test, expect, beforeEach, afterEach, beforeAll, afterAll } from "vitest"; import { mkdtempSync, writeFileSync, existsSync, rmSync } from "fs"; import { tmpdir } from "os"; import path from "path"; import { createDaemonTestContext, type DaemonTestContext, + useTempClaudeConfigDir, } from "./test-utils/index.js"; import type { AgentTimelineItem } from "./agent/agent-sdk-types.js"; import type { AgentSnapshotPayload } from "./messages.js"; @@ -900,14 +901,19 @@ describe("daemon E2E", () => { ); }); - // Claude permission tests are skipped due to SDK behavior: - // - The sandbox config IS passed correctly to Claude SDK - // - Claude executes tool calls without requesting permission - // - This appears to be related to user/project settings or SDK behavior - // - The direct claude-agent.test.ts permission tests pass - // - Codex permission tests through the daemon work correctly - // TODO: Investigate Claude SDK permission behavior in daemon context - describe.skip("permission flow: Claude", () => { + describe("permission flow: Claude", () => { + // Use isolated Claude config to ensure permission prompts are triggered + // (user's real config may have allow rules that auto-approve commands) + let restoreClaudeConfig: () => void; + + beforeAll(() => { + restoreClaudeConfig = useTempClaudeConfigDir(); + }); + + afterAll(() => { + restoreClaudeConfig(); + }); + test( "approves permission and executes command", async () => { diff --git a/packages/server/src/server/test-utils/claude-config.ts b/packages/server/src/server/test-utils/claude-config.ts new file mode 100644 index 000000000..3925ee829 --- /dev/null +++ b/packages/server/src/server/test-utils/claude-config.ts @@ -0,0 +1,53 @@ +import { mkdtempSync, writeFileSync, copyFileSync, existsSync, rmSync } from "fs"; +import { tmpdir, homedir } from "os"; +import path from "path"; + +function copyClaudeCredentials(sourceDir: string, targetDir: string): void { + const sourceCredentials = path.join(sourceDir, ".credentials.json"); + if (!existsSync(sourceCredentials)) { + return; + } + copyFileSync(sourceCredentials, path.join(targetDir, ".credentials.json")); +} + +/** + * Sets up an isolated Claude config directory for testing. + * Creates a temp directory with: + * - settings.json with ask: ["Bash(rm:*)"] to trigger permission prompts + * - settings.local.json with the same settings + * - .credentials.json copied from user's real config + * + * Sets CLAUDE_CONFIG_DIR env var to point to the temp directory. + * Returns a cleanup function that restores the original env and removes the temp dir. + */ +export function useTempClaudeConfigDir(): () => void { + const previousConfigDir = process.env.CLAUDE_CONFIG_DIR; + const sourceConfigDir = + previousConfigDir ?? path.join(homedir(), ".claude"); + const configDir = mkdtempSync(path.join(tmpdir(), "claude-config-")); + const settings = { + permissions: { + allow: [], + deny: [], + ask: ["Bash(rm:*)"], + additionalDirectories: [], + }, + sandbox: { + enabled: true, + autoAllowBashIfSandboxed: false, + }, + }; + const settingsText = `${JSON.stringify(settings, null, 2)}\n`; + writeFileSync(path.join(configDir, "settings.json"), settingsText, "utf8"); + writeFileSync(path.join(configDir, "settings.local.json"), settingsText, "utf8"); + copyClaudeCredentials(sourceConfigDir, configDir); + process.env.CLAUDE_CONFIG_DIR = configDir; + return () => { + if (previousConfigDir === undefined) { + delete process.env.CLAUDE_CONFIG_DIR; + } else { + process.env.CLAUDE_CONFIG_DIR = previousConfigDir; + } + rmSync(configDir, { recursive: true, force: true }); + }; +} diff --git a/packages/server/src/server/test-utils/index.ts b/packages/server/src/server/test-utils/index.ts index b59530eb3..eeff30370 100644 --- a/packages/server/src/server/test-utils/index.ts +++ b/packages/server/src/server/test-utils/index.ts @@ -11,3 +11,4 @@ export { createDaemonTestContext, type DaemonTestContext, } from "./daemon-test-context.js"; +export { useTempClaudeConfigDir } from "./claude-config.js"; diff --git a/plan.md b/plan.md index 3ae1391e7..0c0a0cde3 100644 --- a/plan.md +++ b/plan.md @@ -1153,3 +1153,87 @@ Build a new Codex MCP provider side‑by‑side with the existing Codex SDK prov - Claude permission tests pass (or clear explanation of why they can't) - **Done (2025-12-25 19:12)**: WHAT: Analyzed permission handling differences between `claude-agent.test.ts` and `daemon.e2e.test.ts`. RESULT: Root cause identified - daemon E2E tests read the user's real `~/.claude/settings.json` which has `allow: ["Bash(rm:*)"]`, causing `rm` commands to auto-execute without permission prompts. In contrast, direct tests use `useTempClaudeConfigDir()` (`claude-agent.test.ts:57-87`) which creates a temp config dir with `ask: ["Bash(rm:*)"]` and sets `CLAUDE_CONFIG_DIR` env var. EVIDENCE: User's `~/.claude/settings.json` contains `"Bash(rm:*)"` in allow list. SDK uses `settingSources: ["user", "project"]` (`claude-agent.ts:665`) to read settings from disk. FIX: Add temp config setup to daemon tests (same pattern as direct tests) or use `settingSources: []` for SDK isolation mode. Full report: `REPORT-claude-permission-tests.md`. + +## DaemonClient Surface Expansion (App Parity) + +Goal: Expand DaemonClient to cover all daemon WebSocket capabilities so the app can eventually use it instead of raw WebSocket code. + +- [x] **Fix**: Add temp Claude config isolation to daemon E2E tests and unskip Claude permission tests. + + **Context**: Investigation found Claude permission tests fail because they read user's real `~/.claude/settings.json` which auto-allows `rm`. Direct tests use `useTempClaudeConfigDir()` for isolation. + + **Implementation**: + 1. Export `useTempClaudeConfigDir()` helper from `claude-agent.test.ts` to `test-utils/` + 2. Update daemon test context to optionally use temp Claude config + 3. Update Claude permission tests to use temp config + 4. Unskip tests and verify they pass + + **Acceptance criteria**: + - Claude permission approve/deny tests pass (not skipped) + - Same isolation pattern as direct claude-agent tests + + - **Done (2025-12-25 18:06)**: WHAT: Created shared `useTempClaudeConfigDir()` helper in `test-utils/claude-config.ts:22-49`, exported from `test-utils/index.ts:14`. Updated `daemon.e2e.test.ts:904-915` to add beforeAll/afterAll hooks using this helper and removed `.skip` from `describe("permission flow: Claude")`. Refactored `claude-agent.test.ts:19` to import shared helper and removed duplicate local implementation. RESULT: Both daemon Claude permission tests now pass. EVIDENCE: `npx vitest run --testNamePattern="permission flow: Claude"` shows: ✓ approves permission and executes command (8424ms), ✓ denies permission and prevents execution (7945ms). Direct `claude-agent.test.ts` permission tests also pass (4 tests). Typecheck passes. + +- [ ] **Implement**: Add `getGitDiff()` to DaemonClient + E2E test. + + **Context**: App calls `git_diff_request` to get file diffs. DaemonClient doesn't support this yet. + + **Implementation**: + 1. Add `getGitDiff(agentId: string, filepath?: string)` method to DaemonClient + 2. Send `git_diff_request` message, wait for `git_diff_response` + 3. Add E2E test that creates agent in a git repo, modifies a file, calls getGitDiff + + **Acceptance criteria**: + - Method returns diff content + - E2E test passes + +- [ ] **Implement**: Add `getGitRepoInfo()` to DaemonClient + E2E test. + + **Context**: App calls `git_repo_info_request` to get repo info (branch, status, etc). + + **Implementation**: + 1. Add `getGitRepoInfo(agentId: string)` method to DaemonClient + 2. Send `git_repo_info_request` message, wait for `git_repo_info_response` + 3. Add E2E test in a git repo + + **Acceptance criteria**: + - Method returns repo info (branch, status, remotes) + - E2E test passes + +- [ ] **Implement**: Add `exploreFileSystem()` to DaemonClient + E2E test. + + **Context**: App calls `file_explorer_request` to browse filesystem. + + **Implementation**: + 1. Add `exploreFileSystem(agentId: string, path: string)` method to DaemonClient + 2. Send `file_explorer_request` message, wait for `file_explorer_response` + 3. Add E2E test that lists a directory + + **Acceptance criteria**: + - Method returns file/directory listing + - E2E test passes + +- [ ] **Implement**: Add `listProviderModels()` to DaemonClient + E2E test. + + **Context**: App calls `list_provider_models_request` to get available models. + + **Implementation**: + 1. Add `listProviderModels(provider: AgentProvider)` method to DaemonClient + 2. Send `list_provider_models_request` message, wait for `list_provider_models_response` + 3. Add E2E test for Codex and Claude providers + + **Acceptance criteria**: + - Method returns model list + - E2E test passes for at least one provider + +- [ ] **Implement**: Add `sendImages()` support to DaemonClient + E2E test. + + **Context**: App can send messages with image attachments. DaemonClient `sendMessage()` has `images` option but it's not tested. + + **Implementation**: + 1. Add E2E test that sends a message with an image attachment + 2. Verify agent receives and processes the image + + **Acceptance criteria**: + - E2E test passes with image attachment + - Claude provider correctly receives image (multimodal support)