mirror of
https://github.com/anthropics/claude-code-action.git
synced 2026-01-22 22:44:13 +08:00
refactor: remove CLI path, use Agent SDK exclusively (#849)
* refactor: remove CLI path, use Agent SDK exclusively - Remove CLI-based Claude execution in favor of Agent SDK - Delete prepareRunConfig, parseAndSetSessionId, parseAndSetStructuredOutputs functions - Remove named pipe IPC and sanitizeJsonOutput helper - Remove test-agent-sdk job from test-base-action workflow (SDK is now default) - Delete run-claude.test.ts and structured-output.test.ts (testing removed CLI code) - Update CLAUDE.md to remove named pipe references Co-Authored-By: Claude <noreply@anthropic.com> Claude-Generated-By: Claude Code (cli/claude-opus-4-5=100%) Claude-Steers: 2 Claude-Permission-Prompts: 1 Claude-Escapes: 0 Claude-Plan: <claude-plan> # Plan: Remove Non-Agent SDK Code Path ## Overview Since `use_agent_sdk` defaults to `true`, remove the legacy CLI code path entirely from `base-action/src/run-claude.ts`. ## Files to Modify ### 1. `base-action/src/run-claude.ts` - Main Cleanup **Remove imports:** - `exec` from `child_process` - `promisify` from `util` - `unlink`, `writeFile`, `stat` from `fs/promises` (keep `readFile` - check if needed) - `createWriteStream` from `fs` - `spawn` from `child_process` - `parseShellArgs` from `shell-quote` (still used in `parse-sdk-options.ts`, keep package) **Remove constants:** - `execAsync` - `PIPE_PATH` - `EXECUTION_FILE` (defined in both files, keep in SDK file) - `BASE_ARGS` **Remove types:** - `PreparedConfig` type (lines 85-89) - only used by `prepareRunConfig()` **Remove functions:** - `sanitizeJsonOutput()` (lines 21-68) - `prepareRunConfig()` (lines 91-125) - also remove export - `parseAndSetSessionId()` (lines 131-155) - also remove export - `parseAndSetStructuredOutputs()` (lines 162-197) - also remove export **Simplify `runClaude()`:** - Remove `useAgentSdk` flag check and logging (lines 200-204) - Remove the `if (useAgentSdk)` block, make SDK call direct - Remove entire CLI path (lines 211-438) - Resulting function becomes just: ```typescript export async function runClaude(promptPath: string, options: ClaudeOptions) { const parsedOptions = parseSdkOptions(options); return runClaudeWithSdk(promptPath, parsedOptions); } ``` ### 2. Delete Test Files **`base-action/test/run-claude.test.ts`:** - Delete entire file (only tests `prepareRunConfig()`) **`base-action/test/structured-output.test.ts`:** - Delete entire file (only tests `parseAndSetStructuredOutputs()` and `parseAndSetSessionId()`) ### 3. Workflow Update **`.github/workflows/test-base-action.yml`:** - Remove `test-agent-sdk` job (lines 120-176) - redundant now ### 4. Documentation Update **`base-action/CLAUDE.md`:** - Line 30: Remove "- Named pipes for IPC between prompt input and Claude process" - Line 57: Remove "- Uses `mkfifo` to create named pipes for prompt input" ## Verification 1. Run `bun run typecheck` to ensure no type errors 2. Run `bun test` to ensure remaining tests pass 3. Run `bun run format` to fix any formatting issues </claude-plan> * fix: address PR review comments - Add session_id output handling in run-claude-sdk.ts (critical) - Remove unused claudeEnv parameter from ClaudeOptions and index.ts - Update stale CLI path comment in parse-sdk-options.ts Claude-Generated-By: Claude Code (cli/claude-opus-4-5=100%) Claude-Steers: 0 Claude-Permission-Prompts: 0 Claude-Escapes: 0 Claude-Plan: <claude-plan> # Plan: Remove Non-Agent SDK Code Path ## Overview Since `use_agent_sdk` defaults to `true`, remove the legacy CLI code path entirely from `base-action/src/run-claude.ts`. ## Files to Modify ### 1. `base-action/src/run-claude.ts` - Main Cleanup **Remove imports:** - `exec` from `child_process` - `promisify` from `util` - `unlink`, `writeFile`, `stat` from `fs/promises` (keep `readFile` - check if needed) - `createWriteStream` from `fs` - `spawn` from `child_process` - `parseShellArgs` from `shell-quote` (still used in `parse-sdk-options.ts`, keep package) **Remove constants:** - `execAsync` - `PIPE_PATH` - `EXECUTION_FILE` (defined in both files, keep in SDK file) - `BASE_ARGS` **Remove types:** - `PreparedConfig` type (lines 85-89) - only used by `prepareRunConfig()` **Remove functions:** - `sanitizeJsonOutput()` (lines 21-68) - `prepareRunConfig()` (lines 91-125) - also remove export - `parseAndSetSessionId()` (lines 131-155) - also remove export - `parseAndSetStructuredOutputs()` (lines 162-197) - also remove export **Simplify `runClaude()`:** - Remove `useAgentSdk` flag check and logging (lines 200-204) - Remove the `if (useAgentSdk)` block, make SDK call direct - Remove entire CLI path (lines 211-438) - Resulting function becomes just: ```typescript export async function runClaude(promptPath: string, options: ClaudeOptions) { const parsedOptions = parseSdkOptions(options); return runClaudeWithSdk(promptPath, parsedOptions); } ``` ### 2. Delete Test Files **`base-action/test/run-claude.test.ts`:** - Delete entire file (only tests `prepareRunConfig()`) **`base-action/test/structured-output.test.ts`:** - Delete entire file (only tests `parseAndSetStructuredOutputs()` and `parseAndSetSessionId()`) ### 3. Workflow Update **`.github/workflows/test-base-action.yml`:** - Remove `test-agent-sdk` job (lines 120-176) - redundant now ### 4. Documentation Update **`base-action/CLAUDE.md`:** - Line 30: Remove "- Named pipes for IPC between prompt input and Claude process" - Line 57: Remove "- Uses `mkfifo` to create named pipes for prompt input" ## Verification 1. Run `bun run typecheck` to ensure no type errors 2. Run `bun test` to ensure remaining tests pass 3. Run `bun run format` to fix any formatting issues </claude-plan>
This commit is contained in:
Ashwin Bhat
@@ -1,96 +0,0 @@
|
||||
#!/usr/bin/env bun
|
||||
|
||||
import { describe, test, expect } from "bun:test";
|
||||
import { prepareRunConfig, type ClaudeOptions } from "../src/run-claude";
|
||||
|
||||
describe("prepareRunConfig", () => {
|
||||
test("should prepare config with basic arguments", () => {
|
||||
const options: ClaudeOptions = {};
|
||||
const prepared = prepareRunConfig("/tmp/test-prompt.txt", options);
|
||||
|
||||
expect(prepared.claudeArgs).toEqual([
|
||||
"-p",
|
||||
"--verbose",
|
||||
"--output-format",
|
||||
"stream-json",
|
||||
]);
|
||||
});
|
||||
|
||||
test("should include promptPath", () => {
|
||||
const options: ClaudeOptions = {};
|
||||
const prepared = prepareRunConfig("/tmp/test-prompt.txt", options);
|
||||
|
||||
expect(prepared.promptPath).toBe("/tmp/test-prompt.txt");
|
||||
});
|
||||
|
||||
test("should use provided prompt path", () => {
|
||||
const options: ClaudeOptions = {};
|
||||
const prepared = prepareRunConfig("/custom/prompt/path.txt", options);
|
||||
|
||||
expect(prepared.promptPath).toBe("/custom/prompt/path.txt");
|
||||
});
|
||||
|
||||
describe("claudeArgs handling", () => {
|
||||
test("should parse and include custom claude arguments", () => {
|
||||
const options: ClaudeOptions = {
|
||||
claudeArgs: "--max-turns 10 --model claude-3-opus-20240229",
|
||||
};
|
||||
const prepared = prepareRunConfig("/tmp/test-prompt.txt", options);
|
||||
|
||||
expect(prepared.claudeArgs).toEqual([
|
||||
"-p",
|
||||
"--max-turns",
|
||||
"10",
|
||||
"--model",
|
||||
"claude-3-opus-20240229",
|
||||
"--verbose",
|
||||
"--output-format",
|
||||
"stream-json",
|
||||
]);
|
||||
});
|
||||
|
||||
test("should handle empty claudeArgs", () => {
|
||||
const options: ClaudeOptions = {
|
||||
claudeArgs: "",
|
||||
};
|
||||
const prepared = prepareRunConfig("/tmp/test-prompt.txt", options);
|
||||
|
||||
expect(prepared.claudeArgs).toEqual([
|
||||
"-p",
|
||||
"--verbose",
|
||||
"--output-format",
|
||||
"stream-json",
|
||||
]);
|
||||
});
|
||||
|
||||
test("should handle claudeArgs with quoted strings", () => {
|
||||
const options: ClaudeOptions = {
|
||||
claudeArgs: '--system-prompt "You are a helpful assistant"',
|
||||
};
|
||||
const prepared = prepareRunConfig("/tmp/test-prompt.txt", options);
|
||||
|
||||
expect(prepared.claudeArgs).toEqual([
|
||||
"-p",
|
||||
"--system-prompt",
|
||||
"You are a helpful assistant",
|
||||
"--verbose",
|
||||
"--output-format",
|
||||
"stream-json",
|
||||
]);
|
||||
});
|
||||
|
||||
test("should include json-schema flag when provided", () => {
|
||||
const options: ClaudeOptions = {
|
||||
claudeArgs:
|
||||
'--json-schema \'{"type":"object","properties":{"result":{"type":"boolean"}}}\'',
|
||||
};
|
||||
|
||||
const prepared = prepareRunConfig("/tmp/test-prompt.txt", options);
|
||||
|
||||
expect(prepared.claudeArgs).toContain("--json-schema");
|
||||
expect(prepared.claudeArgs).toContain(
|
||||
'{"type":"object","properties":{"result":{"type":"boolean"}}}',
|
||||
);
|
||||
});
|
||||
});
|
||||
});
|
||||
@@ -1,227 +0,0 @@
|
||||
#!/usr/bin/env bun
|
||||
|
||||
import { describe, test, expect, afterEach, beforeEach, spyOn } from "bun:test";
|
||||
import { writeFile, unlink } from "fs/promises";
|
||||
import { tmpdir } from "os";
|
||||
import { join } from "path";
|
||||
import {
|
||||
parseAndSetStructuredOutputs,
|
||||
parseAndSetSessionId,
|
||||
} from "../src/run-claude";
|
||||
import * as core from "@actions/core";
|
||||
|
||||
// Mock execution file path
|
||||
const TEST_EXECUTION_FILE = join(tmpdir(), "test-execution-output.json");
|
||||
|
||||
// Helper to create mock execution file with structured output
|
||||
async function createMockExecutionFile(
|
||||
structuredOutput?: Record<string, unknown>,
|
||||
includeResult: boolean = true,
|
||||
): Promise<void> {
|
||||
const messages: any[] = [
|
||||
{ type: "system", subtype: "init" },
|
||||
{ type: "turn", content: "test" },
|
||||
];
|
||||
|
||||
if (includeResult) {
|
||||
messages.push({
|
||||
type: "result",
|
||||
cost_usd: 0.01,
|
||||
duration_ms: 1000,
|
||||
structured_output: structuredOutput,
|
||||
});
|
||||
}
|
||||
|
||||
await writeFile(TEST_EXECUTION_FILE, JSON.stringify(messages));
|
||||
}
|
||||
|
||||
// Spy on core functions
|
||||
let setOutputSpy: any;
|
||||
let infoSpy: any;
|
||||
let warningSpy: any;
|
||||
|
||||
beforeEach(() => {
|
||||
setOutputSpy = spyOn(core, "setOutput").mockImplementation(() => {});
|
||||
infoSpy = spyOn(core, "info").mockImplementation(() => {});
|
||||
warningSpy = spyOn(core, "warning").mockImplementation(() => {});
|
||||
});
|
||||
|
||||
describe("parseAndSetStructuredOutputs", () => {
|
||||
afterEach(async () => {
|
||||
setOutputSpy?.mockRestore();
|
||||
infoSpy?.mockRestore();
|
||||
warningSpy?.mockRestore();
|
||||
try {
|
||||
await unlink(TEST_EXECUTION_FILE);
|
||||
} catch {
|
||||
// Ignore if file doesn't exist
|
||||
}
|
||||
});
|
||||
|
||||
test("should set structured_output with valid data", async () => {
|
||||
await createMockExecutionFile({
|
||||
is_flaky: true,
|
||||
confidence: 0.85,
|
||||
summary: "Test looks flaky",
|
||||
});
|
||||
|
||||
await parseAndSetStructuredOutputs(TEST_EXECUTION_FILE);
|
||||
|
||||
expect(setOutputSpy).toHaveBeenCalledWith(
|
||||
"structured_output",
|
||||
'{"is_flaky":true,"confidence":0.85,"summary":"Test looks flaky"}',
|
||||
);
|
||||
expect(infoSpy).toHaveBeenCalledWith(
|
||||
"Set structured_output with 3 field(s)",
|
||||
);
|
||||
});
|
||||
|
||||
test("should handle arrays and nested objects", async () => {
|
||||
await createMockExecutionFile({
|
||||
items: ["a", "b", "c"],
|
||||
config: { key: "value", nested: { deep: true } },
|
||||
});
|
||||
|
||||
await parseAndSetStructuredOutputs(TEST_EXECUTION_FILE);
|
||||
|
||||
const callArgs = setOutputSpy.mock.calls[0];
|
||||
expect(callArgs[0]).toBe("structured_output");
|
||||
const parsed = JSON.parse(callArgs[1]);
|
||||
expect(parsed).toEqual({
|
||||
items: ["a", "b", "c"],
|
||||
config: { key: "value", nested: { deep: true } },
|
||||
});
|
||||
});
|
||||
|
||||
test("should handle special characters in field names", async () => {
|
||||
await createMockExecutionFile({
|
||||
"test-result": "passed",
|
||||
"item.count": 10,
|
||||
"user@email": "test",
|
||||
});
|
||||
|
||||
await parseAndSetStructuredOutputs(TEST_EXECUTION_FILE);
|
||||
|
||||
const callArgs = setOutputSpy.mock.calls[0];
|
||||
const parsed = JSON.parse(callArgs[1]);
|
||||
expect(parsed["test-result"]).toBe("passed");
|
||||
expect(parsed["item.count"]).toBe(10);
|
||||
expect(parsed["user@email"]).toBe("test");
|
||||
});
|
||||
|
||||
test("should throw error when result exists but structured_output is undefined", async () => {
|
||||
const messages = [
|
||||
{ type: "system", subtype: "init" },
|
||||
{ type: "result", cost_usd: 0.01, duration_ms: 1000 },
|
||||
];
|
||||
await writeFile(TEST_EXECUTION_FILE, JSON.stringify(messages));
|
||||
|
||||
await expect(
|
||||
parseAndSetStructuredOutputs(TEST_EXECUTION_FILE),
|
||||
).rejects.toThrow(
|
||||
"--json-schema was provided but Claude did not return structured_output",
|
||||
);
|
||||
});
|
||||
|
||||
test("should throw error when no result message exists", async () => {
|
||||
const messages = [
|
||||
{ type: "system", subtype: "init" },
|
||||
{ type: "turn", content: "test" },
|
||||
];
|
||||
await writeFile(TEST_EXECUTION_FILE, JSON.stringify(messages));
|
||||
|
||||
await expect(
|
||||
parseAndSetStructuredOutputs(TEST_EXECUTION_FILE),
|
||||
).rejects.toThrow(
|
||||
"--json-schema was provided but Claude did not return structured_output",
|
||||
);
|
||||
});
|
||||
|
||||
test("should throw error with malformed JSON", async () => {
|
||||
await writeFile(TEST_EXECUTION_FILE, "{ invalid json");
|
||||
|
||||
await expect(
|
||||
parseAndSetStructuredOutputs(TEST_EXECUTION_FILE),
|
||||
).rejects.toThrow();
|
||||
});
|
||||
|
||||
test("should throw error when file does not exist", async () => {
|
||||
await expect(
|
||||
parseAndSetStructuredOutputs("/nonexistent/file.json"),
|
||||
).rejects.toThrow();
|
||||
});
|
||||
|
||||
test("should handle empty structured_output object", async () => {
|
||||
await createMockExecutionFile({});
|
||||
|
||||
await parseAndSetStructuredOutputs(TEST_EXECUTION_FILE);
|
||||
|
||||
expect(setOutputSpy).toHaveBeenCalledWith("structured_output", "{}");
|
||||
expect(infoSpy).toHaveBeenCalledWith(
|
||||
"Set structured_output with 0 field(s)",
|
||||
);
|
||||
});
|
||||
});
|
||||
|
||||
describe("parseAndSetSessionId", () => {
|
||||
afterEach(async () => {
|
||||
setOutputSpy?.mockRestore();
|
||||
infoSpy?.mockRestore();
|
||||
warningSpy?.mockRestore();
|
||||
try {
|
||||
await unlink(TEST_EXECUTION_FILE);
|
||||
} catch {
|
||||
// Ignore if file doesn't exist
|
||||
}
|
||||
});
|
||||
|
||||
test("should extract session_id from system.init message", async () => {
|
||||
const messages = [
|
||||
{ type: "system", subtype: "init", session_id: "test-session-123" },
|
||||
{ type: "result", cost_usd: 0.01 },
|
||||
];
|
||||
await writeFile(TEST_EXECUTION_FILE, JSON.stringify(messages));
|
||||
|
||||
await parseAndSetSessionId(TEST_EXECUTION_FILE);
|
||||
|
||||
expect(setOutputSpy).toHaveBeenCalledWith("session_id", "test-session-123");
|
||||
expect(infoSpy).toHaveBeenCalledWith("Set session_id: test-session-123");
|
||||
});
|
||||
|
||||
test("should handle missing session_id gracefully", async () => {
|
||||
const messages = [
|
||||
{ type: "system", subtype: "init" },
|
||||
{ type: "result", cost_usd: 0.01 },
|
||||
];
|
||||
await writeFile(TEST_EXECUTION_FILE, JSON.stringify(messages));
|
||||
|
||||
await parseAndSetSessionId(TEST_EXECUTION_FILE);
|
||||
|
||||
expect(setOutputSpy).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
test("should handle missing system.init message gracefully", async () => {
|
||||
const messages = [{ type: "result", cost_usd: 0.01 }];
|
||||
await writeFile(TEST_EXECUTION_FILE, JSON.stringify(messages));
|
||||
|
||||
await parseAndSetSessionId(TEST_EXECUTION_FILE);
|
||||
|
||||
expect(setOutputSpy).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
test("should handle malformed JSON gracefully with warning", async () => {
|
||||
await writeFile(TEST_EXECUTION_FILE, "{ invalid json");
|
||||
|
||||
await parseAndSetSessionId(TEST_EXECUTION_FILE);
|
||||
|
||||
expect(setOutputSpy).not.toHaveBeenCalled();
|
||||
expect(warningSpy).toHaveBeenCalled();
|
||||
});
|
||||
|
||||
test("should handle non-existent file gracefully with warning", async () => {
|
||||
await parseAndSetSessionId("/nonexistent/file.json");
|
||||
|
||||
expect(setOutputSpy).not.toHaveBeenCalled();
|
||||
expect(warningSpy).toHaveBeenCalled();
|
||||
});
|
||||
});
|
||||
Reference in New Issue
Block a user