Skip to content

Commit 52ed8a6

Browse files
fix: address all critical reviewer issues
- Fix API contract violation: maintain sync get_available_integrations() API - Add comprehensive error handling to TypeScript streamCommand - Improve AiderAgent stream error handling - Enhance registry availability checks with constructor inspection - Remove unused imports and improve type safety with discriminated unions Addresses feedback from Gemini, CodeRabbit, Qodo, and Copilot reviewers. Co-authored-by: Mervin Praison <MervinPraison@users.noreply.github.com>
1 parent d522247 commit 52ed8a6

File tree

2 files changed

+59
-12
lines changed

2 files changed

+59
-12
lines changed

src/praisonai-ts/src/cli/features/external-agents.ts

Lines changed: 26 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -19,11 +19,10 @@ export interface ExternalAgentResult {
1919
duration: number;
2020
}
2121

22-
export interface StreamEvent {
23-
type: string;
24-
content?: string;
25-
data?: any;
26-
}
22+
export type StreamEvent =
23+
| { type: 'text'; content: string }
24+
| { type: 'json'; data: unknown }
25+
| { type: 'error'; error: string };
2726

2827
/**
2928
* Base class for external agent integrations
@@ -117,13 +116,24 @@ export abstract class BaseExternalAgent {
117116
const proc = spawn(this.config.command, args, {
118117
cwd: this.config.cwd || process.cwd(),
119118
env: { ...process.env, ...this.config.env },
119+
timeout: this.config.timeout,
120120
stdio: ['pipe', 'pipe', 'pipe']
121121
});
122122

123123
if (!proc.stdout) {
124124
throw new Error('Failed to create stdout stream');
125125
}
126126

127+
let stderr = '';
128+
proc.stderr?.on('data', (chunk) => {
129+
stderr += chunk.toString();
130+
});
131+
132+
const exit = new Promise<number | null>((resolve, reject) => {
133+
proc.once('error', reject);
134+
proc.once('close', resolve);
135+
});
136+
127137
const readline = await import('readline');
128138
const rl = readline.createInterface({
129139
input: proc.stdout,
@@ -143,9 +153,16 @@ export abstract class BaseExternalAgent {
143153
}
144154
}
145155
}
156+
157+
const exitCode = await exit;
158+
if (exitCode !== 0) {
159+
throw new Error(stderr || `${this.config.command} exited with code ${exitCode}`);
160+
}
146161
} finally {
147162
rl.close();
148-
proc.kill();
163+
if (!proc.killed) {
164+
proc.kill();
165+
}
149166
}
150167
}
151168

@@ -273,6 +290,9 @@ export class AiderAgent extends BaseExternalAgent {
273290
async *stream(prompt: string): AsyncGenerator<StreamEvent, void, unknown> {
274291
// Aider doesn't support JSON streaming, so just yield text events
275292
const result = await this.execute(prompt);
293+
if (!result.success) {
294+
throw new Error(result.error || 'Aider execution failed');
295+
}
276296
for (const line of result.output.split('\n')) {
277297
if (line.trim()) {
278298
yield { type: 'text', content: line };

src/praisonai/praisonai/integrations/registry.py

Lines changed: 33 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,6 @@
2727
"""
2828

2929
from typing import Dict, Type, Optional, Any, List
30-
from abc import ABC
3130

3231
from .base import BaseCLIIntegration
3332

@@ -154,15 +153,27 @@ async def get_available(self) -> Dict[str, bool]:
154153
Returns:
155154
Dict[str, bool]: Mapping of integration name to availability status
156155
"""
156+
import inspect
157157
availability = {}
158158

159159
for name, integration_class in self._integrations.items():
160160
try:
161+
# Check if constructor requires parameters beyond self
162+
sig = inspect.signature(integration_class.__init__)
163+
params = [p for p_name, p in sig.parameters.items() if p_name != 'self' and p.default is inspect.Parameter.empty]
164+
165+
if params:
166+
# Constructor requires arguments, can't instantiate for availability check
167+
# Skip this integration rather than marking it unavailable
168+
continue
169+
161170
# Create a temporary instance to check availability
162171
instance = integration_class()
163172
availability[name] = instance.is_available
164-
except Exception:
165-
# If instantiation fails, mark as unavailable
173+
except Exception as e:
174+
# Log real exceptions rather than hiding them
175+
import logging
176+
logging.warning(f"Failed to check availability for {name}: {e}")
166177
availability[name] = False
167178

168179
return availability
@@ -216,14 +227,30 @@ def create_integration(name: str, **kwargs: Any) -> Optional[BaseCLIIntegration]
216227
return registry.create(name, **kwargs)
217228

218229

219-
async def get_available_integrations() -> Dict[str, bool]:
230+
def get_available_integrations() -> Dict[str, bool]:
220231
"""
221232
Get availability status of all registered integrations.
222233
223-
Backward compatibility wrapper for the original function.
234+
Backward compatibility wrapper for the original synchronous function.
224235
225236
Returns:
226237
Dict[str, bool]: Mapping of integration name to availability status
227238
"""
239+
import asyncio
228240
registry = get_registry()
229-
return await registry.get_available()
241+
242+
try:
243+
# Try to get existing event loop
244+
loop = asyncio.get_event_loop()
245+
if loop.is_running():
246+
# We're in an async context, use create_task via thread pool
247+
import concurrent.futures
248+
with concurrent.futures.ThreadPoolExecutor() as executor:
249+
future = executor.submit(asyncio.run, registry.get_available())
250+
return future.result()
251+
else:
252+
# No running loop, safe to use asyncio.run
253+
return asyncio.run(registry.get_available())
254+
except RuntimeError:
255+
# No event loop, safe to use asyncio.run
256+
return asyncio.run(registry.get_available())

0 commit comments

Comments
 (0)