From f0021fc4a3599a3a764b05f14b0b2192fd1bf723 Mon Sep 17 00:00:00 2001 From: Karl Clement Date: Wed, 15 Oct 2025 16:31:05 -0400 Subject: [PATCH 1/2] Now using the Amp SDK --- package.json | 1 + pnpm-lock.yaml | 148 +++++++++++++++++++++++++++++++++++++++++ src/amp.ts | 135 ------------------------------------- src/review/reviewer.ts | 85 ++++++++++++++--------- 4 files changed, 203 insertions(+), 166 deletions(-) delete mode 100644 src/amp.ts diff --git a/package.json b/package.json index d2a049f..60a1b28 100644 --- a/package.json +++ b/package.json @@ -24,6 +24,7 @@ ], "dependencies": { "@hono/node-server": "^1.13.7", + "@sourcegraph/amp-sdk": "latest", "dotenv": "^16.4.7", "hono": "^4.6.10", "js-yaml": "^4.1.0", diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index e5967f9..d92ac09 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -11,6 +11,9 @@ importers: '@hono/node-server': specifier: ^1.13.7 version: 1.19.0(hono@4.9.4) + '@sourcegraph/amp-sdk': + specifier: latest + version: 0.1.0-20251014160341-g7c5de51 dotenv: specifier: ^16.4.7 version: 16.6.1 @@ -438,6 +441,82 @@ packages: '@jridgewell/sourcemap-codec@1.5.5': resolution: {integrity: sha512-cYQ9310grqxueWbl+WuIUIaiUaDcj7WOq5fVhEljNVgRfOUhY9fy2zTvfoqWsnebh8Sl70VScFbICvJnLKB0Og==} + '@napi-rs/keyring-darwin-arm64@1.1.9': + resolution: {integrity: sha512-/lVnrSFrut+8pQC6IcqlfHKzcEmf2XvQDOZPB5X4vI23GrNXBd56EuBlFPdTBtx46A8Bn+Aqi6pS8cnprHtcCw==} + engines: {node: '>= 10'} + cpu: [arm64] + os: [darwin] + + '@napi-rs/keyring-darwin-x64@1.1.9': + resolution: {integrity: sha512-G3PiFZTAFTzUnpSB31A/UaPjl48/3sDTLmLxaAZBEk7HcOyBnL31gA1YqhDCO7F2y5sD5TWiFiuID9MyqYOcjw==} + engines: {node: '>= 10'} + cpu: [x64] + os: [darwin] + + '@napi-rs/keyring-freebsd-x64@1.1.9': + resolution: {integrity: sha512-R4XbvRhEzQyOy4yM+SMDgk8BgkLPkIzXGwR6QR0wJ2YrPeBx3F2TrgdHfsIGSn/X5Axg/2UlrCiZVciZ5BmusA==} + engines: {node: '>= 10'} + cpu: [x64] + os: [freebsd] + + '@napi-rs/keyring-linux-arm-gnueabihf@1.1.9': + resolution: {integrity: sha512-UrKy110I+zQyBtw4HLVUqZ1jDq11K3PmQIYgWAJNwB5VQOj4IQ63zLxk4V01Jx4bNOJmGNlvHDJUAyh/lC5Yww==} + engines: {node: '>= 10'} + cpu: [arm] + os: [linux] + + '@napi-rs/keyring-linux-arm64-gnu@1.1.9': + resolution: {integrity: sha512-yOrhVpNGexDYzybe3dhmHQRPBDjlZPtJDE+eGSi1JwEqYlWDB+4IWjRsetxnO63DhnMFRLeMTdwWghsYrA7VwA==} + engines: {node: '>= 10'} + cpu: [arm64] + os: [linux] + + '@napi-rs/keyring-linux-arm64-musl@1.1.9': + resolution: {integrity: sha512-82EcuzoV/+Dxwi1HHhrEEprN5Ou7OsRKyTJSaRqiVuGvLaQDUhZX/4zXTTh4Pz24m22Q4aoJogafS31w8iKGGw==} + engines: {node: '>= 10'} + cpu: [arm64] + os: [linux] + + '@napi-rs/keyring-linux-riscv64-gnu@1.1.9': + resolution: {integrity: sha512-Q1ar7DszC1X8FW6w7Ql7b72GFeAUxkTiOuxXChCFBy7eWCQSDrr52ZLroIowp82RmkQLZebnK+IwSssD2Ntoag==} + engines: {node: '>= 10'} + cpu: [riscv64] + os: [linux] + + '@napi-rs/keyring-linux-x64-gnu@1.1.9': + resolution: {integrity: sha512-LMvrYt1ho3pEDECssA7ATbcMDgayEUwwSD+UfrC7Hj1+C6dlvipwt5njwUDCno2OeXbjjisCo4CR9fDmXa4sZA==} + engines: {node: '>= 10'} + cpu: [x64] + os: [linux] + + '@napi-rs/keyring-linux-x64-musl@1.1.9': + resolution: {integrity: sha512-x2i/TgS2/fM+6LRj1MrtVC580sepz5GcxbSCXpttx2H58uZKBF0vVM9HDPHoKP2w5++fyrA17eltJNYN3Ob46A==} + engines: {node: '>= 10'} + cpu: [x64] + os: [linux] + + '@napi-rs/keyring-win32-arm64-msvc@1.1.9': + resolution: {integrity: sha512-14t6p8CTBNfGzLO5LXqurT+pAOf/ocGjOM/qiG/LW+jPkhyJYBNI9e3HKq3QX+ObbnxVpt4fAY02b4XLt7EWig==} + engines: {node: '>= 10'} + cpu: [arm64] + os: [win32] + + '@napi-rs/keyring-win32-ia32-msvc@1.1.9': + resolution: {integrity: sha512-7+7aXz5op6PtOnWYcK1GYXWQlk2zfpdPt9taLqmCCVpk1g4m3Gw1wyKyQxjrg9clHWdNhdWxhFEA0osDxG8/Eg==} + engines: {node: '>= 10'} + cpu: [ia32] + os: [win32] + + '@napi-rs/keyring-win32-x64-msvc@1.1.9': + resolution: {integrity: sha512-P1wsSrSqDqvcXLL7yiH2RsO3De65wuEQj1ZjV9s1MHfEP5dIdriNYZfFsRBlOsl32GoK3qFzsuH5DTVviGEHSw==} + engines: {node: '>= 10'} + cpu: [x64] + os: [win32] + + '@napi-rs/keyring@1.1.9': + resolution: {integrity: sha512-qjg04yaJ/gFqgG7wDqLlWBvZpsjvYDtwL+xOr2vSM2JrhojuIKsw7pH013U7xJOradTVGeQqhwqgZtt2IblgOw==} + engines: {node: '>= 10'} + '@nodelib/fs.scandir@2.1.5': resolution: {integrity: sha512-vq24Bq3ym5HEQm2NKCr3yXDwjc7vTsEThRDnkp2DK9p1uqLR+DHurm/NOTo0KG7HYHU7eppKZj3MyqYuMBf62g==} engines: {node: '>= 8'} @@ -550,6 +629,15 @@ packages: cpu: [x64] os: [win32] + '@sourcegraph/amp-sdk@0.1.0-20251014160341-g7c5de51': + resolution: {integrity: sha512-rcAVxm0x5EDEtVTVoIWZqvaIogMPI0fJCMENB57ICplwUubhmalFq05v0v8YNzH8L7/QBqY+IrugdplpWJ2rDQ==} + engines: {node: '>=18'} + + '@sourcegraph/amp@0.0.1760457121-ge7f0f2': + resolution: {integrity: sha512-tDoG72jmHCCJiRsj6Ma+lLl6goLeQO5joKQMsoG2Rlcy5NCOsIRGZHEqWKyD81l7BUxSXhrDNi9WqL6HhVlxAQ==} + engines: {node: '>=20'} + hasBin: true + '@types/estree@1.0.8': resolution: {integrity: sha512-dWHzHa2WqEXI/O1E9OjrocMTKJl2mSrEolh1Iomrv6U+JuNwaHXsXx9bLu5gG7BUWFIN0skIQJQ/L1rIex4X6w==} @@ -1499,6 +1587,57 @@ snapshots: '@jridgewell/sourcemap-codec@1.5.5': {} + '@napi-rs/keyring-darwin-arm64@1.1.9': + optional: true + + '@napi-rs/keyring-darwin-x64@1.1.9': + optional: true + + '@napi-rs/keyring-freebsd-x64@1.1.9': + optional: true + + '@napi-rs/keyring-linux-arm-gnueabihf@1.1.9': + optional: true + + '@napi-rs/keyring-linux-arm64-gnu@1.1.9': + optional: true + + '@napi-rs/keyring-linux-arm64-musl@1.1.9': + optional: true + + '@napi-rs/keyring-linux-riscv64-gnu@1.1.9': + optional: true + + '@napi-rs/keyring-linux-x64-gnu@1.1.9': + optional: true + + '@napi-rs/keyring-linux-x64-musl@1.1.9': + optional: true + + '@napi-rs/keyring-win32-arm64-msvc@1.1.9': + optional: true + + '@napi-rs/keyring-win32-ia32-msvc@1.1.9': + optional: true + + '@napi-rs/keyring-win32-x64-msvc@1.1.9': + optional: true + + '@napi-rs/keyring@1.1.9': + optionalDependencies: + '@napi-rs/keyring-darwin-arm64': 1.1.9 + '@napi-rs/keyring-darwin-x64': 1.1.9 + '@napi-rs/keyring-freebsd-x64': 1.1.9 + '@napi-rs/keyring-linux-arm-gnueabihf': 1.1.9 + '@napi-rs/keyring-linux-arm64-gnu': 1.1.9 + '@napi-rs/keyring-linux-arm64-musl': 1.1.9 + '@napi-rs/keyring-linux-riscv64-gnu': 1.1.9 + '@napi-rs/keyring-linux-x64-gnu': 1.1.9 + '@napi-rs/keyring-linux-x64-musl': 1.1.9 + '@napi-rs/keyring-win32-arm64-msvc': 1.1.9 + '@napi-rs/keyring-win32-ia32-msvc': 1.1.9 + '@napi-rs/keyring-win32-x64-msvc': 1.1.9 + '@nodelib/fs.scandir@2.1.5': dependencies: '@nodelib/fs.stat': 2.0.5 @@ -1571,6 +1710,15 @@ snapshots: '@rollup/rollup-win32-x64-msvc@4.48.1': optional: true + '@sourcegraph/amp-sdk@0.1.0-20251014160341-g7c5de51': + dependencies: + '@sourcegraph/amp': 0.0.1760457121-ge7f0f2 + zod: 3.25.76 + + '@sourcegraph/amp@0.0.1760457121-ge7f0f2': + dependencies: + '@napi-rs/keyring': 1.1.9 + '@types/estree@1.0.8': {} '@types/js-yaml@4.0.9': {} diff --git a/src/amp.ts b/src/amp.ts deleted file mode 100644 index 99aa862..0000000 --- a/src/amp.ts +++ /dev/null @@ -1,135 +0,0 @@ -import { exec } from "node:child_process"; -import { promisify } from "node:util"; -import { tmpdir } from "node:os"; -import { join } from "node:path"; -import { getConfig, Config } from "./config.js"; - -const execAsync = promisify(exec); - -export interface AmpResult { - stdout: string; - stderr: string; - exitCode: number; -} - -export interface ExecuteCommandOptions { - prompt?: string; - folderPath?: string; - threadId?: string; - promptFilePath?: string; - settingsFilePath?: string; - resultFilePath?: string; - debug?: boolean; - logging?: boolean; - env?: Record; -} - -export async function newThread(folderPath: string = tmpdir()): Promise { - if (!folderPath) { - throw new Error("Folder path not set"); - } - - const config: Config = getConfig(); - - try { - const command = `${config.amp.command} threads new`; - const { stdout } = await execAsync(command, { cwd: folderPath }); - - // Extract thread ID from stdout (assuming it's returned as the thread ID) - const threadId = stdout.trim(); - - return threadId; - } catch (error) { - throw new Error(`Failed to start thread: ${error instanceof Error ? error.message : String(error)}`); - } -} - -export async function execute(options: ExecuteCommandOptions = {}): Promise { - const { - prompt, - promptFilePath, - settingsFilePath, - resultFilePath, - folderPath = tmpdir(), - debug = false, - logging = false, - env: customEnv = {} - } = options; - - let { threadId } = options; - - try { - const config: Config = getConfig(); - - if (!threadId) { - threadId = await newThread(folderPath); - } - - const includePrompt = prompt ? `echo "${prompt.replace(/\n/g, "\\n")}" | ` : ''; - const includePromptFile = promptFilePath ? `cat ${promptFilePath} | ` : ''; - const includeThread = threadId ? ` threads continue ${threadId}` : ''; - const includeDebug = debug ? ` --log-level debug ` : ''; - const includeSettings = settingsFilePath ? ` --settings-file ${settingsFilePath} ` : ''; - const includeResult = resultFilePath ? ` > ${resultFilePath}` : ''; - - // Build the command string - const command = `${prompt ? includePrompt : includePromptFile}${config.amp.command}${includeThread}${includeDebug}${includeSettings}${includeResult}`; - - // Set up toolbox environment variables - const toolboxPath = join(process.cwd(), 'dist/toolbox'); - const env = { - ...process.env, - AMP_TOOLBOX: toolboxPath, - ...customEnv - }; - - // Execute command - const { stdout, stderr } = await execAsync(command, { cwd: folderPath, env }); - - if (logging) { - console.log(`[AMP] Command completed. stdout length: ${stdout.length}, stderr length: ${stderr.length}`); - if (stderr) { - console.log(`[AMP] stderr: ${stderr}`); - } - } - - return stdout; - } catch (error) { - // Extract more details from the exec error - const execError = error as { code: number, stderr: string }; - const exitCode = execError.code || 'unknown'; - const stderr = execError.stderr || ''; - - if (logging) { - console.log(`[AMP] Command exited with code: ${exitCode}`); - if (stderr) { - console.log(`[AMP] stderr: ${stderr}`); - } - } - - // Handle Amp CLI terminal cleanup behavior: - // Amp returns exit code 1 when it runs terminal cleanup (writes cursor control codes to stderr) - // even though the actual command execution succeeded. This happens in containerized environments - // where Amp detects terminal-like behavior and attempts cleanup on exit. - // Rather than trying to prevent this (fragile), we check if the expected output files exist, - // which is the semantic definition of success for our use case. - if (resultFilePath) { - const fs = await import('fs'); - - if (fs.existsSync(resultFilePath)) { - if (logging) { - console.log(`[AMP] Result file exists despite exit code ${exitCode}, treating as success`); - } - try { - return fs.readFileSync(resultFilePath, 'utf8'); - } catch (readError) { - console.error(`[AMP] Failed to read result file: ${readError}`); - } - } - } - - throw new Error(`Failed to execute command (exit code ${exitCode}): ${error instanceof Error ? error.message : String(error)}${stderr ? `\nstderr: ${stderr}` : ''}`); - } -} - - diff --git a/src/review/reviewer.ts b/src/review/reviewer.ts index 91ed941..80b76e8 100644 --- a/src/review/reviewer.ts +++ b/src/review/reviewer.ts @@ -1,9 +1,9 @@ -import { writeFileSync } from 'fs'; import { tmpdir } from 'os'; import { join } from 'path'; +import { mkdirSync } from 'fs'; import { v4 as uuidv4 } from 'uuid'; +import { execute } from '@sourcegraph/amp-sdk'; import { Config, getConfig } from "../config.js"; -import { newThread, execute } from "../amp.js"; import { PRContext } from "../github/types.js"; @@ -16,51 +16,74 @@ export const reviewDiff = async ( // Get config const config: Config = getConfig(); - // Prepare temp files - const tempDir = tmpdir(); - const promptFilePath = join(tempDir, `amp-prompt-${uuidv4()}.txt`); - const resultFilePath = join(tempDir, `amp-result-${uuidv4()}.txt`); + // Prepare temp directory + const tempDir = join(tmpdir(), '.amp', uuidv4()); + mkdirSync(tempDir, { recursive: true }); try { // Create prompt content const ampConfig = config.amp; // Format PR context for prompt - const prDetailsContent = `Repository: ${prContext.repository_full_name}\nPR Number: ${prContext.pr_number}\nCommit SHA: ${prContext.commit_sha}\nPR URL: ${prContext.pr_url}`; + const prDetailsContent = ` + Repository: ${prContext.repository_full_name}\n + PR Number: ${prContext.pr_number}\n + Commit SHA: ${prContext.commit_sha}\n + PR URL: ${prContext.pr_url}\n + `; const promptContent = ampConfig.prompt_template .replace(/__PR_DETAILS_CONTENT__/g, prDetailsContent) .replace(/__DIFF_CONTENT__/g, diffContent); - // Tools are now auto-discovered by Amp via toolbox - no manual injection needed - - // Write prompt to file - writeFileSync(promptFilePath, promptContent, 'utf8'); - // Generate unique filename for comment collection - const commentsFileName = `comments-${installationId}-${uuidv4()}.jsonl`; + const commentsFileName = `comments-${installationId}.jsonl`; const commentsFilePath = join(tempDir, commentsFileName); + // Set up environment variables for toolbox + const toolboxPath = join(process.cwd(), 'dist/toolbox'); + const toolboxEnv = { + COMMENTS_FILE: commentsFilePath, + + GITHUB_INSTALLATION_ID: installationId.toString(), + GITHUB_OWNER: prContext.owner, + GITHUB_REPO: prContext.repo, + GITHUB_PR_NUMBER: prContext.pr_number.toString(), + GITHUB_APP_ID: process.env.GITHUB_APP_ID || '', + GITHUB_APP_PRIVATE_KEY_PATH: process.env.GITHUB_APP_PRIVATE_KEY_PATH || '', + GITHUB_APP_CWD: process.env.GITHUB_APP_CWD || '', + }; - const threadId = await newThread(tempDir); - const result = await execute({ - promptFilePath, - resultFilePath, - folderPath: tempDir, - debug: true, - logging: true, - threadId, - env: { - GITHUB_INSTALLATION_ID: installationId.toString(), - COMMENTS_FILE: commentsFilePath, - GITHUB_OWNER: prContext.owner, - GITHUB_REPO: prContext.repo, - GITHUB_PR_NUMBER: prContext.pr_number.toString(), - GITHUB_APP_ID: process.env.GITHUB_APP_ID || '', - GITHUB_APP_PRIVATE_KEY_PATH: process.env.GITHUB_APP_PRIVATE_KEY_PATH || '', - GITHUB_APP_CWD: process.env.GITHUB_APP_CWD || '', + // Execute Amp review using SDK + let threadId: string | undefined; + let result: string | undefined; + + console.log('[Amp] Starting thread...'); + for await (const message of execute({ + prompt: promptContent, + options: { + cwd: tempDir, + dangerouslyAllowAll: true, + env: { ...toolboxEnv }, + toolbox: toolboxPath, + } + })) { + if (message.type === 'system' && message.subtype === 'init') { + // Capture session ID (thread ID) from system message + threadId = message.session_id; + console.log(`[Amp] Started thread: ${threadId}`); + } else if (message.type === 'result') { + if (message.is_error) { + throw new Error(`[Amp] Review failed: ${message.error}`); + } + result = message.result; + console.log(`[Amp] Review completed successfully`); } - }); + } + console.log('[Amp] Iterator completed'); + + // Check if threadId and result are defined + if (!threadId || !result) throw new Error('Amp review failed'); return { success: true, threadId, result, commentsFilePath }; } catch (error) { From cc60e381d32a8674d85d5d40e65dacc7442180f9 Mon Sep 17 00:00:00 2001 From: Karl Clement Date: Wed, 15 Oct 2025 16:33:00 -0400 Subject: [PATCH 2/2] Fix prDetailsContent --- src/review/reviewer.ts | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/src/review/reviewer.ts b/src/review/reviewer.ts index 80b76e8..0b99911 100644 --- a/src/review/reviewer.ts +++ b/src/review/reviewer.ts @@ -25,12 +25,7 @@ export const reviewDiff = async ( const ampConfig = config.amp; // Format PR context for prompt - const prDetailsContent = ` - Repository: ${prContext.repository_full_name}\n - PR Number: ${prContext.pr_number}\n - Commit SHA: ${prContext.commit_sha}\n - PR URL: ${prContext.pr_url}\n - `; + const prDetailsContent = `Repository: ${prContext.repository_full_name}\nPR Number: ${prContext.pr_number}\nCommit SHA: ${prContext.commit_sha}\nPR URL: ${prContext.pr_url}`; const promptContent = ampConfig.prompt_template .replace(/__PR_DETAILS_CONTENT__/g, prDetailsContent)