From 59633e45b4c96c5bd21b96d1575c8563b5da6ca7 Mon Sep 17 00:00:00 2001 From: deepak Date: Sun, 8 Mar 2026 10:42:45 +0000 Subject: [PATCH] fix(agent-claude-code): detect cd-prefixed gh/git commands and use relative hook path --- .../src/hook-script.integration.test.ts | 306 ++++++++++++++++++ .../agent-claude-code/src/index.test.ts | 216 ++++++++++++- .../plugins/agent-claude-code/src/index.ts | 39 ++- 3 files changed, 537 insertions(+), 24 deletions(-) create mode 100644 packages/plugins/agent-claude-code/src/hook-script.integration.test.ts diff --git a/packages/plugins/agent-claude-code/src/hook-script.integration.test.ts b/packages/plugins/agent-claude-code/src/hook-script.integration.test.ts new file mode 100644 index 000000000..fdce14024 --- /dev/null +++ b/packages/plugins/agent-claude-code/src/hook-script.integration.test.ts @@ -0,0 +1,306 @@ +import { describe, it, expect, beforeAll, afterAll } from "vitest"; +import { execSync } from "node:child_process"; +import { mkdtempSync, writeFileSync, readFileSync, rmSync, mkdirSync } from "node:fs"; +import { join } from "node:path"; +import { tmpdir } from "node:os"; +import { METADATA_UPDATER_SCRIPT } from "./index.js"; + +// --------------------------------------------------------------------------- +// Integration tests for the metadata-updater.sh hook script. +// These execute the actual bash script with various inputs and verify that +// session metadata files are updated correctly. +// --------------------------------------------------------------------------- + +let testDir: string; +let hookScriptPath: string; + +beforeAll(() => { + testDir = mkdtempSync(join(tmpdir(), "ao-hook-test-")); + hookScriptPath = join(testDir, "metadata-updater.sh"); + writeFileSync(hookScriptPath, METADATA_UPDATER_SCRIPT, { mode: 0o755 }); +}); + +afterAll(() => { + rmSync(testDir, { recursive: true, force: true }); +}); + +/** + * Run the hook script with given parameters and return the script's + * stdout plus the updated metadata file contents. + */ +function runHook(opts: { + command: string; + toolName?: string; + output?: string; + exitCode?: number; + metadataContent?: string; +}): { stdout: string; metadata: string } { + const sessionId = `test-session-${Date.now()}-${Math.random().toString(36).slice(2, 8)}`; + const sessionsDir = join(testDir, "sessions"); + mkdirSync(sessionsDir, { recursive: true }); + const metadataFile = join(sessionsDir, sessionId); + writeFileSync(metadataFile, opts.metadataContent ?? "status=spawning\n"); + + const input = JSON.stringify({ + tool_name: opts.toolName ?? "Bash", + tool_input: { command: opts.command }, + tool_response: opts.output ?? "", + exit_code: opts.exitCode ?? 0, + }); + + let stdout: string; + try { + stdout = execSync(`bash "${hookScriptPath}"`, { + input, + env: { + ...process.env, + AO_SESSION: sessionId, + AO_DATA_DIR: sessionsDir, + HOME: testDir, + }, + encoding: "utf-8", + timeout: 5000, + }); + } catch (err: unknown) { + const e = err as { stdout?: string }; + stdout = e.stdout ?? ""; + } + + let metadata: string; + try { + metadata = readFileSync(metadataFile, "utf-8"); + } catch { + metadata = ""; + } + + return { stdout, metadata }; +} + +// ========================================================================= +// gh pr create detection +// ========================================================================= +describe("hook script: gh pr create", () => { + const prUrl = "https://github.com/owner/repo/pull/42"; + + it("detects plain gh pr create", () => { + const { metadata } = runHook({ + command: 'gh pr create --title "fix" --body "test" --base master', + output: `Creating pull request...\n${prUrl}\n`, + }); + expect(metadata).toContain(`pr=${prUrl}`); + expect(metadata).toContain("status=pr_open"); + }); + + it("detects gh pr create with cd && prefix", () => { + const { metadata } = runHook({ + command: `cd ~/.worktrees/mercury/cleanup && gh pr create --title "fix" --base master`, + output: `${prUrl}`, + }); + expect(metadata).toContain(`pr=${prUrl}`); + expect(metadata).toContain("status=pr_open"); + }); + + it("detects gh pr create with cd ; prefix", () => { + const { metadata } = runHook({ + command: `cd /some/path ; gh pr create --title "test" --body "body"`, + output: prUrl, + }); + expect(metadata).toContain(`pr=${prUrl}`); + expect(metadata).toContain("status=pr_open"); + }); + + it("detects gh pr create with multiple chained cd prefixes", () => { + const { metadata } = runHook({ + command: `cd /tmp && cd ~/.worktrees/mercury && gh pr create --title "fix" --base master`, + output: prUrl, + }); + expect(metadata).toContain(`pr=${prUrl}`); + expect(metadata).toContain("status=pr_open"); + }); + + it("does NOT update metadata when PR URL is missing from output", () => { + const { metadata } = runHook({ + command: 'gh pr create --title "fix"', + output: "Error: something went wrong", + }); + expect(metadata).not.toContain("pr="); + expect(metadata).toContain("status=spawning"); + }); +}); + +// ========================================================================= +// git checkout -b / git switch -c detection +// ========================================================================= +describe("hook script: git checkout -b / git switch -c", () => { + it("detects plain git checkout -b", () => { + const { metadata } = runHook({ + command: "git checkout -b feat/my-feature", + }); + expect(metadata).toContain("branch=feat/my-feature"); + }); + + it("detects plain git switch -c", () => { + const { metadata } = runHook({ + command: "git switch -c fix/bug-123", + }); + expect(metadata).toContain("branch=fix/bug-123"); + }); + + it("detects git checkout -b with cd && prefix", () => { + const { metadata } = runHook({ + command: "cd /some/project && git checkout -b feat/new-feature", + }); + expect(metadata).toContain("branch=feat/new-feature"); + }); + + it("detects git switch -c with cd && prefix", () => { + const { metadata } = runHook({ + command: "cd ~/.worktrees/project && git switch -c fix/issue-456", + }); + expect(metadata).toContain("branch=fix/issue-456"); + }); + + it("detects git checkout -b with cd ; prefix", () => { + const { metadata } = runHook({ + command: "cd /project ; git checkout -b feat/semicolon-test", + }); + expect(metadata).toContain("branch=feat/semicolon-test"); + }); + + it("detects git checkout -b with multiple cd prefixes", () => { + const { metadata } = runHook({ + command: "cd /tmp && cd /project && git checkout -b feat/chained", + }); + expect(metadata).toContain("branch=feat/chained"); + }); +}); + +// ========================================================================= +// gh pr merge detection +// ========================================================================= +describe("hook script: gh pr merge", () => { + it("detects plain gh pr merge", () => { + const { metadata } = runHook({ + command: "gh pr merge 123 --squash", + }); + expect(metadata).toContain("status=merged"); + }); + + it("detects gh pr merge with cd && prefix", () => { + const { metadata } = runHook({ + command: "cd ~/.worktrees/project && gh pr merge 42 --squash", + }); + expect(metadata).toContain("status=merged"); + }); + + it("detects gh pr merge with cd ; prefix", () => { + const { metadata } = runHook({ + command: "cd /project ; gh pr merge --rebase", + }); + expect(metadata).toContain("status=merged"); + }); +}); + +// ========================================================================= +// git checkout detection (feature branches) +// ========================================================================= +describe("hook script: git checkout existing branch", () => { + it("detects plain git checkout of a feature branch", () => { + const { metadata } = runHook({ + command: "git checkout feat/existing-branch", + }); + expect(metadata).toContain("branch=feat/existing-branch"); + }); + + it("detects git checkout of a feature branch with cd prefix", () => { + const { metadata } = runHook({ + command: "cd /project && git checkout feat/existing-branch", + }); + expect(metadata).toContain("branch=feat/existing-branch"); + }); +}); + +// ========================================================================= +// Non-matching commands (should NOT modify metadata) +// ========================================================================= +describe("hook script: non-matching commands", () => { + it("ignores plain ls command", () => { + const { metadata } = runHook({ + command: "ls -la", + }); + expect(metadata).toBe("status=spawning\n"); + }); + + it("ignores non-Bash tool calls", () => { + const { metadata } = runHook({ + command: 'gh pr create --title "test"', + toolName: "Read", + output: "https://github.com/owner/repo/pull/1", + }); + expect(metadata).toBe("status=spawning\n"); + }); + + it("ignores commands with non-zero exit code", () => { + const { metadata } = runHook({ + command: 'gh pr create --title "test"', + exitCode: 1, + output: "https://github.com/owner/repo/pull/1", + }); + expect(metadata).toBe("status=spawning\n"); + }); + + it("ignores cd-only commands (no chained git/gh)", () => { + const { metadata } = runHook({ + command: "cd /some/directory", + }); + expect(metadata).toBe("status=spawning\n"); + }); + + it("ignores git status", () => { + const { metadata } = runHook({ + command: "cd /project && git status", + }); + expect(metadata).toBe("status=spawning\n"); + }); +}); + +// ========================================================================= +// Metadata file update mechanics +// ========================================================================= +describe("hook script: metadata file updates", () => { + it("updates an existing key in the metadata file", () => { + const { metadata } = runHook({ + command: "gh pr merge 10 --squash", + metadataContent: "status=pr_open\nbranch=feat/test\n", + }); + expect(metadata).toContain("status=merged"); + expect(metadata).toContain("branch=feat/test"); + expect(metadata).not.toContain("status=pr_open"); + }); + + it("appends a new key to the metadata file", () => { + const { metadata } = runHook({ + command: 'gh pr create --title "test"', + output: "https://github.com/owner/repo/pull/99", + metadataContent: "branch=feat/test\n", + }); + expect(metadata).toContain("branch=feat/test"); + expect(metadata).toContain("pr=https://github.com/owner/repo/pull/99"); + expect(metadata).toContain("status=pr_open"); + }); + + it("returns systemMessage JSON on successful detection", () => { + const { stdout } = runHook({ + command: "gh pr merge 1 --squash", + }); + expect(stdout).toContain("systemMessage"); + expect(stdout).toContain("merged"); + }); + + it("returns empty JSON for non-matching commands", () => { + const { stdout } = runHook({ + command: "echo hello", + }); + expect(stdout.trim()).toBe("{}"); + }); +}); diff --git a/packages/plugins/agent-claude-code/src/index.test.ts b/packages/plugins/agent-claude-code/src/index.test.ts index 27b689e6d..5b3d8df34 100644 --- a/packages/plugins/agent-claude-code/src/index.test.ts +++ b/packages/plugins/agent-claude-code/src/index.test.ts @@ -1,17 +1,30 @@ import { describe, it, expect, vi, beforeEach } from "vitest"; -import type { Session, RuntimeHandle, AgentLaunchConfig } from "@composio/ao-core"; +import type { Session, RuntimeHandle, AgentLaunchConfig, WorkspaceHooksConfig } from "@composio/ao-core"; // --------------------------------------------------------------------------- // Hoisted mocks — available inside vi.mock factories // --------------------------------------------------------------------------- -const { mockExecFileAsync, mockReaddir, mockReadFile, mockStat, mockHomedir } = - vi.hoisted(() => ({ - mockExecFileAsync: vi.fn(), - mockReaddir: vi.fn(), - mockReadFile: vi.fn(), - mockStat: vi.fn(), - mockHomedir: vi.fn(() => "/mock/home"), - })); +const { + mockExecFileAsync, + mockReaddir, + mockReadFile, + mockStat, + mockHomedir, + mockWriteFile, + mockMkdir, + mockChmod, + mockExistsSync, +} = vi.hoisted(() => ({ + mockExecFileAsync: vi.fn(), + mockReaddir: vi.fn(), + mockReadFile: vi.fn(), + mockStat: vi.fn(), + mockHomedir: vi.fn(() => "/mock/home"), + mockWriteFile: vi.fn().mockResolvedValue(undefined), + mockMkdir: vi.fn().mockResolvedValue(undefined), + mockChmod: vi.fn().mockResolvedValue(undefined), + mockExistsSync: vi.fn().mockReturnValue(false), +})); vi.mock("node:child_process", () => { const fn = Object.assign((..._args: unknown[]) => {}, { @@ -24,13 +37,20 @@ vi.mock("node:fs/promises", () => ({ readdir: mockReaddir, readFile: mockReadFile, stat: mockStat, + writeFile: mockWriteFile, + mkdir: mockMkdir, + chmod: mockChmod, +})); + +vi.mock("node:fs", () => ({ + existsSync: mockExistsSync, })); vi.mock("node:os", () => ({ homedir: mockHomedir, })); -import { create, manifest, default as defaultExport, resetPsCache } from "./index.js"; +import { create, manifest, default as defaultExport, resetPsCache, METADATA_UPDATER_SCRIPT } from "./index.js"; // --------------------------------------------------------------------------- // Test helpers @@ -670,3 +690,179 @@ describe("getSessionInfo", () => { }); }); }); + +// ========================================================================= +// METADATA_UPDATER_SCRIPT — content verification (unit tests) +// ========================================================================= +describe("METADATA_UPDATER_SCRIPT content", () => { + it("contains clean_command stripping logic for cd prefixes", () => { + expect(METADATA_UPDATER_SCRIPT).toContain('clean_command="$command"'); + expect(METADATA_UPDATER_SCRIPT).toMatch(/while.*clean_command.*cd/); + }); + + it("uses $clean_command (not $command) for all regex-based command detection", () => { + const lines = METADATA_UPDATER_SCRIPT.split("\n"); + for (const line of lines) { + // Skip comment lines, the initial assignment, and the stripping logic itself + if (line.trim().startsWith("#")) continue; + if (line.includes('clean_command="$command"')) continue; + if (line.includes("while") && line.includes("clean_command")) continue; + + // Any regex match line (=~) should use $clean_command, NOT $command + if (line.includes("=~") && line.includes("command")) { + expect(line).toContain("clean_command"); + expect(line).not.toMatch(/"\$command"/); + } + } + }); + + it("does NOT use ^-anchored regexes directly on $command for gh/git detection", () => { + // The old buggy patterns matched $command with ^ anchor. + // After the fix, ^ is still used but on $clean_command (which has cd stripped). + expect(METADATA_UPDATER_SCRIPT).not.toMatch( + /"\$command"\s*=~\s*\^gh/, + ); + expect(METADATA_UPDATER_SCRIPT).not.toMatch( + /"\$command"\s*=~\s*\^git/, + ); + }); + + it("strips cd prefixes with both && and ; delimiters", () => { + expect(METADATA_UPDATER_SCRIPT).toMatch(/&&\|;/); + }); + + it("handles multiple chained cd commands via while loop", () => { + expect(METADATA_UPDATER_SCRIPT).toMatch(/while.*clean_command/); + }); + + it("detects gh pr create on clean_command", () => { + expect(METADATA_UPDATER_SCRIPT).toMatch( + /"\$clean_command"\s*=~\s*\^gh\[/, + ); + }); + + it("detects git checkout -b on clean_command", () => { + expect(METADATA_UPDATER_SCRIPT).toMatch( + /"\$clean_command"\s*=~\s*\^git\[.*checkout/, + ); + }); + + it("detects gh pr merge on clean_command", () => { + expect(METADATA_UPDATER_SCRIPT).toMatch( + /"\$clean_command"\s*=~\s*\^gh\[.*merge/, + ); + }); +}); + +// ========================================================================= +// setupWorkspaceHooks / postLaunchSetup — hook path (symlink safety) +// ========================================================================= +describe("hook setup — relative path (symlink-safe)", () => { + const agent = create(); + + /** Extract the hook command from the settings.json that was written */ + function getWrittenHookCommand(): string { + const settingsWrite = mockWriteFile.mock.calls.find( + ([path]: unknown[]) => typeof path === "string" && path.endsWith("settings.json"), + ); + expect(settingsWrite).toBeDefined(); + const parsed = JSON.parse(settingsWrite![1] as string); + return parsed.hooks.PostToolUse[0].hooks[0].command; + } + + it("setupWorkspaceHooks writes a relative hook command (not absolute)", async () => { + await agent.setupWorkspaceHooks!( + "/Users/equinox/.worktrees/integrator/integrator-5", + {} as WorkspaceHooksConfig, + ); + + const hookCommand = getWrittenHookCommand(); + expect(hookCommand).toBe(".claude/metadata-updater.sh"); + expect(hookCommand).not.toMatch(/^\//); + }); + + it("postLaunchSetup writes a relative hook command (not absolute)", async () => { + await agent.postLaunchSetup!( + makeSession({ workspacePath: "/Users/equinox/.worktrees/integrator/integrator-10" }), + ); + + const hookCommand = getWrittenHookCommand(); + expect(hookCommand).toBe(".claude/metadata-updater.sh"); + expect(hookCommand).not.toMatch(/^\//); + }); + + it("different worktree paths produce identical settings.json content", async () => { + await agent.setupWorkspaceHooks!( + "/Users/equinox/.worktrees/integrator/integrator-5", + {} as WorkspaceHooksConfig, + ); + const settingsWrite1 = mockWriteFile.mock.calls.find( + ([path]: unknown[]) => typeof path === "string" && path.endsWith("settings.json"), + ); + const content1 = settingsWrite1![1] as string; + + mockWriteFile.mockClear(); + + await agent.setupWorkspaceHooks!( + "/Users/equinox/.worktrees/integrator/integrator-10", + {} as WorkspaceHooksConfig, + ); + const settingsWrite2 = mockWriteFile.mock.calls.find( + ([path]: unknown[]) => typeof path === "string" && path.endsWith("settings.json"), + ); + const content2 = settingsWrite2![1] as string; + + expect(content1).toBe(content2); + }); + + it("updates an existing absolute hook path to relative", async () => { + mockExistsSync.mockReturnValue(true); + mockReadFile.mockResolvedValue( + JSON.stringify({ + hooks: { + PostToolUse: [ + { + matcher: "Bash", + hooks: [ + { + type: "command", + command: + "/Users/equinox/.worktrees/integrator/integrator-5/.claude/metadata-updater.sh", + timeout: 5000, + }, + ], + }, + ], + }, + }), + ); + + await agent.setupWorkspaceHooks!( + "/Users/equinox/.worktrees/integrator/integrator-10", + {} as WorkspaceHooksConfig, + ); + + const hookCommand = getWrittenHookCommand(); + expect(hookCommand).toBe(".claude/metadata-updater.sh"); + }); + + it("still writes the script file to the correct absolute filesystem path", async () => { + await agent.setupWorkspaceHooks!( + "/Users/equinox/.worktrees/integrator/integrator-5", + {} as WorkspaceHooksConfig, + ); + + const scriptWrite = mockWriteFile.mock.calls.find( + ([path]: unknown[]) => typeof path === "string" && path.endsWith("metadata-updater.sh"), + ); + expect(scriptWrite).toBeDefined(); + expect(scriptWrite![0]).toBe( + "/Users/equinox/.worktrees/integrator/integrator-5/.claude/metadata-updater.sh", + ); + }); + + it("skips postLaunchSetup when workspacePath is null", async () => { + await agent.postLaunchSetup!(makeSession({ workspacePath: null })); + expect(mockWriteFile).not.toHaveBeenCalled(); + }); +}); diff --git a/packages/plugins/agent-claude-code/src/index.ts b/packages/plugins/agent-claude-code/src/index.ts index 7b463277f..65b6a17b8 100644 --- a/packages/plugins/agent-claude-code/src/index.ts +++ b/packages/plugins/agent-claude-code/src/index.ts @@ -36,8 +36,9 @@ function normalizePermissionMode(mode: string | undefined): "permissionless" | " // Metadata Updater Hook Script // ============================================================================= -/** Hook script content that updates session metadata on git/gh commands */ -const METADATA_UPDATER_SCRIPT = `#!/usr/bin/env bash +/** Hook script content that updates session metadata on git/gh commands. + * Exported for integration testing. */ +export const METADATA_UPDATER_SCRIPT = `#!/usr/bin/env bash # Metadata Updater Hook for Agent Orchestrator # # This PostToolUse hook automatically updates session metadata when: @@ -124,8 +125,20 @@ update_metadata_key() { # Command Detection and Parsing # ============================================================================ +# Strip leading directory-change prefixes so that commands like +# cd ~/.worktrees/project && gh pr create ... +# are correctly detected. Agents frequently cd into a worktree first. +clean_command="$command" +while [[ "$clean_command" =~ ^[[:space:]]*cd[[:space:]] ]]; do + if [[ "$clean_command" =~ ^[[:space:]]*cd[[:space:]]+[^'&;']*(&&|;)[[:space:]]*(.*) ]]; then + clean_command="\${BASH_REMATCH[2]}" + else + break + fi +done + # Detect: gh pr create -if [[ "$command" =~ ^gh[[:space:]]+pr[[:space:]]+create ]]; then +if [[ "$clean_command" =~ ^gh[[:space:]]+pr[[:space:]]+create ]]; then # Extract PR URL from output pr_url=$(echo "$output" | grep -Eo 'https://github[.]com/[^/]+/[^/]+/pull/[0-9]+' | head -1) @@ -138,8 +151,8 @@ if [[ "$command" =~ ^gh[[:space:]]+pr[[:space:]]+create ]]; then fi # Detect: git checkout -b or git switch -c -if [[ "$command" =~ ^git[[:space:]]+checkout[[:space:]]+-b[[:space:]]+([^[:space:]]+) ]] || \\ - [[ "$command" =~ ^git[[:space:]]+switch[[:space:]]+-c[[:space:]]+([^[:space:]]+) ]]; then +if [[ "$clean_command" =~ ^git[[:space:]]+checkout[[:space:]]+-b[[:space:]]+([^[:space:]]+) ]] || \\ + [[ "$clean_command" =~ ^git[[:space:]]+switch[[:space:]]+-c[[:space:]]+([^[:space:]]+) ]]; then branch="\${BASH_REMATCH[1]}" if [[ -n "$branch" ]]; then @@ -151,8 +164,8 @@ fi # Detect: git checkout (without -b) or git switch (without -c) # Only update if the branch name looks like a feature branch (contains / or -) -if [[ "$command" =~ ^git[[:space:]]+checkout[[:space:]]+([^[:space:]-]+[/-][^[:space:]]+) ]] || \\ - [[ "$command" =~ ^git[[:space:]]+switch[[:space:]]+([^[:space:]-]+[/-][^[:space:]]+) ]]; then +if [[ "$clean_command" =~ ^git[[:space:]]+checkout[[:space:]]+([^[:space:]-]+[/-][^[:space:]]+) ]] || \\ + [[ "$clean_command" =~ ^git[[:space:]]+switch[[:space:]]+([^[:space:]-]+[/-][^[:space:]]+) ]]; then branch="\${BASH_REMATCH[1]}" # Avoid updating for checkout of commits/tags @@ -164,7 +177,7 @@ if [[ "$command" =~ ^git[[:space:]]+checkout[[:space:]]+([^[:space:]-]+[/-][^[:s fi # Detect: gh pr merge -if [[ "$command" =~ ^gh[[:space:]]+pr[[:space:]]+merge ]]; then +if [[ "$clean_command" =~ ^gh[[:space:]]+pr[[:space:]]+merge ]]; then update_metadata_key "status" "merged" echo '{"systemMessage": "Updated metadata: status = merged"}' exit 0 @@ -817,17 +830,15 @@ function createClaudeCodeAgent(): Agent { }, async setupWorkspaceHooks(workspacePath: string, _config: WorkspaceHooksConfig): Promise { - // Use absolute path for hook command (specific to this workspace) - const hookScriptPath = join(workspacePath, ".claude", "metadata-updater.sh"); - await setupHookInWorkspace(workspacePath, hookScriptPath); + // Relative path so that symlinked .claude/ dirs across worktrees + // all produce the same settings.json (last writer doesn't clobber). + await setupHookInWorkspace(workspacePath, ".claude/metadata-updater.sh"); }, async postLaunchSetup(session: Session): Promise { if (!session.workspacePath) return; - // Use absolute path for hook command (specific to this workspace) - const hookScriptPath = join(session.workspacePath, ".claude", "metadata-updater.sh"); - await setupHookInWorkspace(session.workspacePath, hookScriptPath); + await setupHookInWorkspace(session.workspacePath, ".claude/metadata-updater.sh"); }, }; }