wylab/claude-hub
1
0
Fork 0
forked from claude-did-this/claude-hub
Code Issues 1 Pull Requests Activity

Merge pull request #59 from intelligence-assist/fix/docker-env-long-commands

Browse Source 
Fix Docker environment variable passing for long commands
This commit is contained in:
Cheffromspace
2025-05-25 20:12:31 -05:00
committed by GitHub
parent 3aeb53f2cc 4e318199b7
commit b0e5d01f6e
2 changed files with 32 additions and 83 deletions
Download Patch File Download Diff File Expand all files Collapse all files

76
src/services/claudeService.js
View File

@@ -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 => {

39
test/unit/services/claudeService.test.js
View File

@@ -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');
@@ -181,11 +180,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 +196,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;