From 52018b9b178c3fe97fd4439ae39172895a41dcee Mon Sep 17 00:00:00 2001 From: Jonathan Flatt Date: Mon, 26 May 2025 01:05:35 +0000 Subject: [PATCH 1/2] Fix Docker environment variable passing for long commands MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Remove temp file approach that used invalid @file syntax with Docker - Pass long commands directly as environment variables - Update test to verify long command handling without temp files - Remove unused fsSync import The previous implementation attempted to use Docker's non-existent @file syntax for reading environment variables from files, which caused the COMMAND variable to be empty in the container. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude --- src/services/claudeService.js | 76 ++++-------------------- test/unit/services/claudeService.test.js | 38 +++++++----- 2 files changed, 32 insertions(+), 82 deletions(-) diff --git a/src/services/claudeService.js b/src/services/claudeService.js index 5341937..2d68ff5 100644 --- a/src/services/claudeService.js +++ b/src/services/claudeService.js @@ -1,6 +1,4 @@ const { execFileSync } = require('child_process'); -// Use sync methods for file operations that need to be synchronous -const fsSync = require('fs'); const path = require('path'); // const os = require('os'); const { createLogger } = require('../utils/logger'); @@ -146,25 +144,8 @@ Please complete this task fully and autonomously.`; ANTHROPIC_API_KEY: secureCredentials.get('ANTHROPIC_API_KEY') }; - // Build docker run command - properly escape values for shell - Object.entries(envVars) - .filter(([_, value]) => value !== undefined && value !== '') - .map(([key, value]) => { - // Convert to string and escape shell special characters in the value - const stringValue = String(value); - // Write complex values to files for safer handling - if (key === 'COMMAND' && stringValue.length > 500) { - const crypto = require('crypto'); - const randomSuffix = crypto.randomBytes(16).toString('hex'); - const tmpFile = `/tmp/claude-command-${Date.now()}-${randomSuffix}.txt`; - fsSync.writeFileSync(tmpFile, stringValue, { mode: 0o600 }); // Secure file permissions - return `-e ${key}="$(cat ${tmpFile})"`; - } - // Escape for shell with double quotes (more reliable than single quotes) - const escapedValue = stringValue.replace(/["\\$`!]/g, '\\$&'); - return `-e ${key}="${escapedValue}"`; - }) - .join(' '); + // Note: Environment variables will be added as separate arguments to docker command + // This is safer than building a shell command string // Run the container logger.info( @@ -228,13 +209,12 @@ Please complete this task fully and autonomously.`; Object.entries(envVars) .filter(([_, value]) => value !== undefined && value !== '') .forEach(([key, value]) => { - // Write complex values to files for safer handling + // For long commands, we need to pass them differently + // Docker doesn't support reading env values from files with @ syntax if (key === 'COMMAND' && String(value).length > 500) { - const crypto = require('crypto'); - const randomSuffix = crypto.randomBytes(16).toString('hex'); - const tmpFile = `/tmp/claude-command-${Date.now()}-${randomSuffix}.txt`; - fsSync.writeFileSync(tmpFile, String(value), { mode: 0o600 }); // Secure file permissions - dockerArgs.push('-e', `${key}=@${tmpFile}`); + // We'll pass the command via stdin or mount it as a volume + // For now, let's just pass it directly but properly escaped + dockerArgs.push('-e', `${key}=${String(value)}`); } else { dockerArgs.push('-e', `${key}=${String(value)}`); } @@ -272,26 +252,7 @@ Please complete this task fully and autonomously.`; try { logger.info({ dockerArgs: sanitizedArgs }, 'Executing Docker command'); - // Clear any temporary command files after execution - const cleanupTempFiles = () => { - try { - const tempFiles = execFileSync('find', ['/tmp', '-name', 'claude-command-*.txt', '-type', 'f']) - .toString() - .split('\n'); - tempFiles - .filter(f => f) - .forEach(file => { - try { - fsSync.unlinkSync(file); - logger.info(`Removed temp file: ${file}`); - } catch { - logger.warn(`Failed to remove temp file: ${file}`); - } - }); - } catch { - logger.warn('Failed to clean up temp files'); - } - }; + // No longer using temp files for commands // Get container lifetime from environment variable or use default (2 hours) const containerLifetimeMs = parseInt(process.env.CONTAINER_LIFETIME_MS, 10) || 7200000; // 2 hours in milliseconds @@ -306,8 +267,7 @@ Please complete this task fully and autonomously.`; timeout: containerLifetimeMs // Container lifetime in milliseconds }); - // Clean up temporary files used for command passing - cleanupTempFiles(); + // No cleanup needed anymore let responseText = result.stdout.trim(); @@ -357,23 +317,7 @@ Please complete this task fully and autonomously.`; return responseText; } catch (error) { - // Clean up temporary files even when there's an error - try { - const tempFiles = execFileSync('find', ['/tmp', '-name', 'claude-command-*.txt', '-type', 'f']) - .toString() - .split('\n'); - tempFiles - .filter(f => f) - .forEach(file => { - try { - fsSync.unlinkSync(file); - } catch { - // Ignore cleanup errors - } - }); - } catch { - // Ignore cleanup errors - } + // No cleanup needed - we're not using temp files anymore // Sanitize stderr and stdout to remove any potential credentials const sanitizeOutput = output => { diff --git a/test/unit/services/claudeService.test.js b/test/unit/services/claudeService.test.js index b044ade..ff86c4e 100644 --- a/test/unit/services/claudeService.test.js +++ b/test/unit/services/claudeService.test.js @@ -181,11 +181,11 @@ describe('Claude Service', () => { } }); - test('processCommand should write long commands to temp files', async () => { + test('processCommand should handle long commands properly', async () => { // Save original function for restoration const originalProcessCommand = claudeService.processCommand; - // Create a testing implementation that checks for file writing + // Create a testing implementation that checks for long command handling claudeService.processCommand = async (options) => { // Set up test environment const originalNodeEnv = process.env.NODE_ENV; @@ -197,40 +197,46 @@ describe('Claude Service', () => { return 'mocked output'; }); - // Capture file write calls - writeFileSync.mockImplementation((_path, _content, _options) => { - // File write is mocked + // Configure execFileAsync mock + const execFileAsync = promisify(require('child_process').execFile); + execFileAsync.mockResolvedValue({ + stdout: 'Claude response for long command', + stderr: '' }); // Call the original implementation - try { - await originalProcessCommand(options); - } catch (_e) { - // Ignore errors, we just want to check if writeFileSync was called - } + const result = await originalProcessCommand(options); - // Verify temp file creation occurred - expect(writeFileSync).toHaveBeenCalled(); + // Verify Docker was called with the command as an environment variable + expect(execFileAsync).toHaveBeenCalled(); + const dockerArgs = execFileAsync.mock.calls[0][1]; + + // Check that COMMAND env var is present in the docker args + // The format is ['-e', 'COMMAND=value'] + const commandEnvIndex = dockerArgs.findIndex((arg) => + typeof arg === 'string' && arg.startsWith('COMMAND=') + ); + expect(commandEnvIndex).toBeGreaterThan(-1); // Restore environment process.env.NODE_ENV = originalNodeEnv; - return 'test response'; + return result; }; try { // Call the function with a long command const longCommand = 'A'.repeat(1000); - await claudeService.processCommand({ + const result = await claudeService.processCommand({ repoFullName: 'test/repo', issueNumber: 123, command: longCommand, isPullRequest: false }); - // Verification happens in the override function - expect(writeFileSync).toHaveBeenCalled(); + // Verify we got a response + expect(result).toBe('Claude response for long command'); } finally { // Restore original function claudeService.processCommand = originalProcessCommand; From 4e318199b7f408a64d5c83fc3c0084c9e8267c82 Mon Sep 17 00:00:00 2001 From: Jonathan Flatt Date: Mon, 26 May 2025 01:09:27 +0000 Subject: [PATCH 2/2] Fix linting error: remove unused writeFileSync import --- test/unit/services/claudeService.test.js | 1 - 1 file changed, 1 deletion(-) diff --git a/test/unit/services/claudeService.test.js b/test/unit/services/claudeService.test.js index ff86c4e..8d2cccc 100644 --- a/test/unit/services/claudeService.test.js +++ b/test/unit/services/claudeService.test.js @@ -50,7 +50,6 @@ jest.mock('../../../src/utils/secureCredentials', () => ({ // Now require the module under test const { execFileSync } = require('child_process'); -const { writeFileSync } = require('fs'); const { promisify } = require('util'); const { sanitizeBotMentions } = require('../../../src/utils/sanitize'); const claudeService = require('../../../src/services/claudeService');