Skip to content

Codebase health check: update SDK, fix reliability issues, improve performance #407

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
nikblanchet merged 2 commits into from
Mar 28, 2026
Merged

Codebase health check: update SDK, fix reliability issues, improve performance #407

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a39d9fe
Codebase health check: update SDK, fix reliability issues, improve pe…
claude Mar 28, 2026
a277efb
Fix CI: add missing JSDoc @returns and fix prettier formatting
claude Mar 28, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 8 additions & 8 deletions cli/src/__tests__/python-bridge/PythonBridge.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -483,10 +483,10 @@ describe('PythonBridge Timeout Handling', () => {
const mockProcess = new EventEmitter() as any;
mockProcess.stdout = new EventEmitter();
mockProcess.stderr = new EventEmitter();
mockProcess.stdin = {
write: jest.fn(),
end: jest.fn(),
};
const stdinEmitter = new EventEmitter() as any;
stdinEmitter.write = jest.fn();
stdinEmitter.end = jest.fn();
mockProcess.stdin = stdinEmitter;
mockProcess.kill = jest.fn();
mockProcess.exitCode = null;
mockSpawn.mockReturnValue(mockProcess);
Expand All @@ -503,10 +503,10 @@ describe('PythonBridge Timeout Handling', () => {
const mockProcess = new EventEmitter() as any;
mockProcess.stdout = new EventEmitter();
mockProcess.stderr = new EventEmitter();
mockProcess.stdin = {
write: jest.fn(),
end: jest.fn(),
};
const stdinEmitter2 = new EventEmitter() as any;
stdinEmitter2.write = jest.fn();
stdinEmitter2.end = jest.fn();
mockProcess.stdin = stdinEmitter2;
mockProcess.kill = jest.fn();
mockProcess.exitCode = null;

Expand Down
232 changes: 92 additions & 140 deletions cli/src/python-bridge/python-bridge.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,7 +146,13 @@ function detectPythonExecutable(): string {
// 3. Check for explicit path from environment (for CI)
const environmentPath = process.env.DOCIMP_PYTHON_PATH;
if (environmentPath) {
return environmentPath;
if (existsSync(environmentPath)) {
return environmentPath;
}
throw new Error(
`DOCIMP_PYTHON_PATH is set to "${environmentPath}" but executable does not exist.\n` +
`Please check the path or unset the environment variable.`
);
}

// 4. Try common Python executables
Expand Down Expand Up @@ -396,79 +402,7 @@ export class PythonBridge implements IPythonBridge {
arguments_.push('--audit-file', absoluteAuditFile);
}

// Handle uv wrapper: spawn('uv', ['run', 'python', ...args]) instead of spawn('python', args)
let executable: string;
let spawnArguments: string[];

if (this.pythonPath === 'uv') {
executable = 'uv';
// Use --project flag to point to project root (parent of analyzer/ directory)
// This ensures uv finds pyproject.toml even when cwd is analyzer/
spawnArguments = ['run', '--project', '..', 'python', ...arguments_];
} else {
executable = this.pythonPath;
spawnArguments = arguments_;
}

// Clean up environment for uv run: remove VIRTUAL_ENV to avoid conflicts
const environment = { ...process.env };
if (this.pythonPath === 'uv') {
delete environment.VIRTUAL_ENV;
}

const childProcess = spawn(executable, spawnArguments, {
cwd: this.analyzerModule,
env: environment,
});

// Setup timeout handling
const { cleanup, timeoutPromise } = this.setupProcessTimeout(
childProcess,
this.defaultTimeout,
'apply-audit'
);

// Create promise for normal process completion
const processPromise = new Promise<void>((resolve, reject) => {
let stderr = '';

// Send ratings as JSON via stdin
childProcess.stdin.write(JSON.stringify(ratings));
childProcess.stdin.end();

childProcess.stderr.on('data', (data: Buffer) => {
stderr += data.toString();
});

childProcess.on('error', (error: Error) => {
reject(
new Error(
`Failed to spawn Python process: ${error.message}\n` +
`Make sure Python is installed and the analyzer module is available.`
)
);
});

childProcess.on('close', (code: number) => {
if (code !== 0) {
reject(
new Error(
`Python analyzer exited with code ${code}\n` + `stderr: ${stderr}`
)
);
return;
}

resolve();
});
});

// Race between timeout and normal completion, cleanup in finally
try {
return await Promise.race([processPromise, timeoutPromise]);
} finally {
cleanup();
}
return this.executePythonStdin(arguments_, ratings, 'apply-audit');
}

/**
Expand Down Expand Up @@ -563,22 +497,92 @@ export class PythonBridge implements IPythonBridge {
*/
async apply(data: ApplyData): Promise<void> {
const arguments_ = ['-m', 'src.main', 'apply'];
return this.executePythonStdin(arguments_, data, 'apply');
}

// Handle uv wrapper: spawn('uv', ['run', 'python', ...args]) instead of spawn('python', args)
/**
* Setup timeout handling for a child process.
*
* Implements graceful shutdown: SIGTERM -> wait -> SIGKILL
*
* @param childProcess - The child process to monitor
* @param timeoutMs - Timeout in milliseconds
* @param commandName - Name of command for error messages
* @returns Object with cleanup function and timeout promise
*/
private setupProcessTimeout(
childProcess: ChildProcess,
timeoutMs: number,
commandName: string
): { cleanup: () => void; timeoutPromise: Promise<never> } {
let timeoutId: NodeJS.Timeout | null = null;
let killTimeoutId: NodeJS.Timeout | null = null;

const cleanup = () => {
if (timeoutId) {
clearTimeout(timeoutId);
timeoutId = null;
}
if (killTimeoutId) {
clearTimeout(killTimeoutId);
killTimeoutId = null;
}
};

const timeoutPromise = new Promise<never>((_resolve, reject) => {
timeoutId = setTimeout(() => {
// Try graceful shutdown first (SIGTERM)
childProcess.kill('SIGTERM');

// If process doesn't exit within configured delay, force kill (SIGKILL)
killTimeoutId = setTimeout(() => {
if (childProcess.exitCode === null) {
childProcess.kill('SIGKILL');
}
}, this.killEscalationDelay);

reject(
new Error(
`Python ${commandName} command timed out after ${timeoutMs}ms.\n` +
`The Python process may be frozen or the operation is taking too long.\n` +
`Consider increasing the timeout in your docimp.config.js file.`
)
);
}, timeoutMs);
});

return { cleanup, timeoutPromise };
}

/**
* Execute a Python command that receives JSON data via stdin.
*
* Unlike executePython which reads JSON from stdout, this method
* writes JSON data to the process's stdin and waits for exit.
*
* @param arguments_ - Python module arguments
* @param stdinData - Data to serialize as JSON and send via stdin
* @param commandName - Name of command for error messages
* @param timeoutMs - Optional timeout override (default: this.defaultTimeout)
* @returns Promise resolving when the command completes successfully
*/
private async executePythonStdin(
arguments_: string[],
stdinData: unknown,
commandName: string,
timeoutMs?: number
): Promise<void> {
let executable: string;
let spawnArguments: string[];

if (this.pythonPath === 'uv') {
executable = 'uv';
// Use --project flag to point to project root (parent of analyzer/ directory)
// This ensures uv finds pyproject.toml even when cwd is analyzer/
spawnArguments = ['run', '--project', '..', 'python', ...arguments_];
} else {
executable = this.pythonPath;
spawnArguments = arguments_;
}

// Clean up environment for uv run: remove VIRTUAL_ENV to avoid conflicts
const environment = { ...process.env };
if (this.pythonPath === 'uv') {
delete environment.VIRTUAL_ENV;
Expand All @@ -589,19 +593,23 @@ export class PythonBridge implements IPythonBridge {
env: environment,
});

// Setup timeout handling
const timeout = timeoutMs ?? this.defaultTimeout;
const { cleanup, timeoutPromise } = this.setupProcessTimeout(
childProcess,
this.defaultTimeout,
'apply'
timeout,
commandName
);

// Create promise for normal process completion
const processPromise = new Promise<void>((resolve, reject) => {
let stderr = '';

// Send apply data as JSON via stdin
childProcess.stdin.write(JSON.stringify(data));
childProcess.stdin.on('error', (error: Error) => {
reject(
new Error(`Failed to write to Python process stdin: ${error.message}`)
);
});

childProcess.stdin.write(JSON.stringify(stdinData));
childProcess.stdin.end();

childProcess.stderr.on('data', (data: Buffer) => {
Expand All @@ -626,73 +634,17 @@ export class PythonBridge implements IPythonBridge {
);
return;
}

resolve();
});
});

// Race between timeout and normal completion, cleanup in finally
try {
return await Promise.race([processPromise, timeoutPromise]);
} finally {
cleanup();
}
}

/**
* Setup timeout handling for a child process.
*
* Implements graceful shutdown: SIGTERM -> wait -> SIGKILL
*
* @param childProcess - The child process to monitor
* @param timeoutMs - Timeout in milliseconds
* @param commandName - Name of command for error messages
* @returns Object with cleanup function and timeout promise
*/
private setupProcessTimeout(
childProcess: ChildProcess,
timeoutMs: number,
commandName: string
): { cleanup: () => void; timeoutPromise: Promise<never> } {
let timeoutId: NodeJS.Timeout | null = null;
let killTimeoutId: NodeJS.Timeout | null = null;

const cleanup = () => {
if (timeoutId) {
clearTimeout(timeoutId);
timeoutId = null;
}
if (killTimeoutId) {
clearTimeout(killTimeoutId);
killTimeoutId = null;
}
};

const timeoutPromise = new Promise<never>((_resolve, reject) => {
timeoutId = setTimeout(() => {
// Try graceful shutdown first (SIGTERM)
childProcess.kill('SIGTERM');

// If process doesn't exit within configured delay, force kill (SIGKILL)
killTimeoutId = setTimeout(() => {
if (childProcess.exitCode === null) {
childProcess.kill('SIGKILL');
}
}, this.killEscalationDelay);

reject(
new Error(
`Python ${commandName} command timed out after ${timeoutMs}ms.\n` +
`The Python process may be frozen or the operation is taking too long.\n` +
`Consider increasing the timeout in your docimp.config.js file.`
)
);
}, timeoutMs);
});

return { cleanup, timeoutPromise };
}

/**
* Execute Python subprocess and return text output (not JSON).
*
Expand Down
Loading
Loading