Skip to content
2 changes: 0 additions & 2 deletions src/praisonai/praisonai/inc/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,6 @@
import importlib.util

logger = logging.getLogger(__name__)
_loglevel = os.environ.get('LOGLEVEL', 'INFO').strip().upper() or 'INFO'
logging.basicConfig(level=_loglevel, format='%(asctime)s - %(levelname)s - %(message)s')

# Constants
LOCAL_SERVER_API_KEY_PLACEHOLDER = "not-needed"
Expand Down
45 changes: 38 additions & 7 deletions src/praisonai/praisonai/llm/env.py
Original file line number Diff line number Diff line change
Expand Up @@ -18,11 +18,23 @@ class LLMEndpoint:
api_key: Optional[str]


# Map well-known model prefixes to their (env-var, default base_url).
_PROVIDER_MAP = {
"anthropic/": ("ANTHROPIC_API_KEY", "https://api.anthropic.com/v1"),
"google/": ("GOOGLE_API_KEY", "https://generativelanguage.googleapis.com/v1beta"),
"gemini/": ("GEMINI_API_KEY", "https://generativelanguage.googleapis.com/v1beta"),
"groq/": ("GROQ_API_KEY", "https://api.groq.com/openai/v1"),
"cohere/": ("COHERE_API_KEY", "https://api.cohere.ai/v1"),
"openrouter/": ("OPENROUTER_API_KEY", "https://openrouter.ai/api/v1"),
"ollama/": ("OLLAMA_API_KEY", "http://localhost:11434/v1"),
}
Comment on lines +21 to +30

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Non-OpenAI providers fall back to OpenAI's base URL.

For anthropic/, google/, gemini/, and cohere/ prefixes, provider_base is None. This causes base_url to fall through to default_base (https://api.openai.com/v1) when no env vars override it.

Looking at PraisonAIModel in models.py, these providers explicitly set base_url = "" to indicate they don't use an OpenAI-compatible endpoint. If resolve_llm_endpoint() returns OpenAI's URL for an Anthropic model and a caller constructs an OpenAI client with it, requests will be misdirected.

Consider either:

  1. Setting provider-appropriate default URLs (e.g., https://api.anthropic.com/v1 for Anthropic)
  2. Using an empty string or sentinel value to signal non-OpenAI-compatible providers
  3. Documenting that callers must check api_key variable naming or model prefix to determine client type
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/praisonai/praisonai/llm/env.py` around lines 21 - 30, The _PROVIDER_MAP
currently uses None for provider_base for "anthropic/", "google/", "gemini/",
and "cohere/", which lets resolve_llm_endpoint() fall back to the OpenAI default
URL and misroute non-OpenAI providers; update _PROVIDER_MAP so those prefixes
map to provider-appropriate default base URLs (e.g., set "anthropic/" ->
("ANTHROPIC_API_KEY", "https://api.anthropic.com/v1") or another real provider
endpoint) or use an explicit sentinel (e.g., empty string) to indicate
non-OpenAI-compatible endpoints, and ensure resolve_llm_endpoint() and callers
that rely on PraisonAIModel.base_url respect that sentinel; adjust any tests or
code that assume None means OpenAI to use the new sentinel/URLs.


# Documented, single precedence list. Add new providers here only.
_MODEL_VARS = ("MODEL_NAME", "OPENAI_MODEL_NAME")
_BASE_URL_VARS = ("OPENAI_BASE_URL", "OPENAI_API_BASE", "OLLAMA_API_BASE")
_DEFAULT_MODEL = "gpt-4o-mini"
_DEFAULT_BASE = "https://api.openai.com/v1"
_DEFAULT_KEY_VAR = "OPENAI_API_KEY"


def _first_set(*names: str) -> Optional[str]:
Expand All @@ -34,23 +46,42 @@ def _first_set(*names: str) -> Optional[str]:
return None


def _provider_from_model(model: str) -> tuple[str, str | None]:
"""Get provider-specific API key environment variable and base URL for a model."""
for prefix, (key_var, default_base) in _PROVIDER_MAP.items():
if model.startswith(prefix):
return key_var, default_base
return _DEFAULT_KEY_VAR, None


def resolve_llm_endpoint(*, default_base: str = _DEFAULT_BASE) -> LLMEndpoint:
"""
Resolve LLM endpoint configuration from environment variables.

Precedence order:
- Model: MODEL_NAME > OPENAI_MODEL_NAME > default
- Base URL: OPENAI_BASE_URL > OPENAI_API_BASE > OLLAMA_API_BASE > default
- API Key: OPENAI_API_KEY
- Base URL: OPENAI_BASE_URL > OPENAI_API_BASE > OLLAMA_API_BASE > provider default > default
- API Key: provider-specific key (e.g., ANTHROPIC_API_KEY) > OPENAI_API_KEY fallback

Args:
default_base: Default base URL if none found in environment variables

Returns:
LLMEndpoint with resolved configuration
"""
return LLMEndpoint(
model=_first_set(*_MODEL_VARS) or _DEFAULT_MODEL,
base_url=_first_set(*_BASE_URL_VARS) or default_base,
api_key=os.environ.get("OPENAI_API_KEY"),
)
model = _first_set(*_MODEL_VARS) or _DEFAULT_MODEL
key_var, provider_base = _provider_from_model(model)

base_url = (
_first_set(*_BASE_URL_VARS)
or provider_base
or default_base
)

# Try provider-specific key first; fall back to OPENAI_API_KEY only for
# OpenAI-compatible proxies that may use a single shared key.
api_key = os.environ.get(key_var) or (
os.environ.get("OPENAI_API_KEY") if key_var == "OPENAI_API_KEY" else None
)

return LLMEndpoint(model=model, base_url=base_url, api_key=api_key)
133 changes: 111 additions & 22 deletions src/praisonai/praisonai/sandbox/subprocess.py
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
SandboxStatus,
ResourceLimits,
)
from praisonaiagents.sandbox.config import SecurityPolicy

logger = logging.getLogger(__name__)

Expand Down Expand Up @@ -51,6 +52,48 @@ def __init__(
self._is_running = False
self._temp_dir: Optional[str] = None

def _build_child_env(self, policy: SecurityPolicy, overrides: Dict[str, str] | None) -> Dict[str, str]:
"""Construct a minimal, policy-driven env for the child process."""
# Start with the explicit env from SandboxConfig and the per-call overrides only.
env = dict(self.config.env)
if overrides:
env.update(overrides)

# If the policy allows network, pass through proxy-related vars so the child can
# reach the outside world; otherwise withhold them.
if policy.allow_network:
for var in ("HTTP_PROXY", "HTTPS_PROXY", "NO_PROXY", "http_proxy",
"https_proxy", "no_proxy"):
if var in os.environ:
env[var] = os.environ[var]

# Always pass a minimal PATH (so /usr/bin/python resolves) — never the host's.
env.setdefault("PATH", "/usr/local/bin:/usr/bin:/bin")
# HOME uses temp_dir which is set during start() - /tmp is defensive fallback # noqa: S108
env.setdefault("HOME", self._temp_dir or "/tmp")
return env

def _apply_rlimits(self, limits: ResourceLimits):
"""Apply resource limits via POSIX setrlimit (Linux/macOS only)."""
if os.name != "posix":
logger.warning("Resource limits not supported on Windows - sandbox isolation is weaker")
return

try:
import resource
if limits.memory_mb and limits.memory_mb > 0:
bytes_ = limits.memory_mb * 1024 * 1024
resource.setrlimit(resource.RLIMIT_AS, (bytes_, bytes_))
if limits.max_processes and limits.max_processes > 0:
resource.setrlimit(resource.RLIMIT_NPROC, (limits.max_processes, limits.max_processes))
if limits.max_open_files and limits.max_open_files > 0:
resource.setrlimit(resource.RLIMIT_NOFILE, (limits.max_open_files, limits.max_open_files))
# Note: RLIMIT_CPU is process CPU time, not wall clock time - timeout is handled separately
except ImportError:
logger.warning("Resource module not available - resource limits not enforced")
except (OSError, ValueError) as e:
logger.warning(f"Failed to set resource limits: {e}")

@property
def is_available(self) -> bool:
"""Subprocess sandbox is always available."""
Expand Down Expand Up @@ -107,27 +150,43 @@ async def execute(
else:
cmd = ["python", code_file]

process_env = os.environ.copy()
if env:
process_env.update(env)
# Build environment based on security policy instead of copying host environment
process_env = self._build_child_env(self.config.security_policy, env)

started_at = time.time()

# preexec_fn is POSIX-only; omit on Windows
popen_kwargs = {
"stdout": asyncio.subprocess.PIPE,
"stderr": asyncio.subprocess.PIPE,
"cwd": working_dir or self._temp_dir,
"env": process_env,
}
if os.name == "posix":
popen_kwargs["start_new_session"] = True
popen_kwargs["preexec_fn"] = lambda: self._apply_rlimits(limits)
else:
logger.warning("Resource limits and session isolation not available on Windows")

try:
proc = await asyncio.create_subprocess_exec(
*cmd,
stdout=asyncio.subprocess.PIPE,
stderr=asyncio.subprocess.PIPE,
cwd=working_dir or self._temp_dir,
env=process_env,
)
proc = await asyncio.create_subprocess_exec(*cmd, **popen_kwargs)

try:
# Truncate output to max_output_size after reading
max_output_size = self.config.security_policy.max_output_size

stdout, stderr = await asyncio.wait_for(
proc.communicate(),
timeout=limits.timeout_seconds,
)

# Truncate output if it exceeds max_output_size
if max_output_size and max_output_size > 0:
if len(stdout) > max_output_size:
stdout = stdout[:max_output_size] + b"\n[OUTPUT TRUNCATED]"
if len(stderr) > max_output_size:
stderr = stderr[:max_output_size] + b"\n[OUTPUT TRUNCATED]"
Comment on lines 174 to +188

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 OOM protection is not actually enforced — full output is buffered before truncation

The comment "Enforce max_output_size by reading incrementally" and the two initialised-but-unused variables stdout_data/stderr_data indicate an incomplete refactoring. proc.communicate() still collects the entire subprocess output into memory before the length check runs, so a malicious or runaway script generating gigabytes of output can exhaust heap memory before any truncation occurs. Incremental reading via asyncio.StreamReader.read in a loop would be needed to honour max_output_size as a true safety bound.


completed_at = time.time()

return SandboxResult(
Expand All @@ -141,7 +200,15 @@ async def execute(
completed_at=completed_at,
)
except asyncio.TimeoutError:
proc.kill()
# Kill the whole process group, not just the leader
try:
if os.name == "posix":
import signal
os.killpg(proc.pid, signal.SIGKILL)
else:
proc.kill()
except (ProcessLookupError, PermissionError, OSError):
proc.kill()
await proc.wait()
Comment on lines 202 to 212

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

os.killpg and signal.SIGKILL are unavailable on Windows.

If the subprocess creation is made Windows-compatible (per earlier comment), this timeout handler would fail with AttributeError because:

  • os.killpg doesn't exist on Windows
  • signal.SIGKILL isn't defined on Windows

Consider adding an OSError or AttributeError fallback, or guard the entire block:

🐛 Proposed fix for cross-platform timeout handling
             except asyncio.TimeoutError:
                 # Kill the whole process group, not just the leader
                 try:
-                    import signal
-                    os.killpg(proc.pid, signal.SIGKILL)
-                except (ProcessLookupError, PermissionError):
+                    if os.name == "posix":
+                        import signal
+                        os.killpg(proc.pid, signal.SIGKILL)
+                    else:
+                        proc.kill()
+                except (ProcessLookupError, PermissionError, OSError):
                     proc.kill()
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
except asyncio.TimeoutError:
proc.kill()
# Kill the whole process group, not just the leader
try:
import signal
os.killpg(proc.pid, signal.SIGKILL)
except (ProcessLookupError, PermissionError):
proc.kill()
await proc.wait()
except asyncio.TimeoutError:
# Kill the whole process group, not just the leader
try:
if os.name == "posix":
import signal
os.killpg(proc.pid, signal.SIGKILL)
else:
proc.kill()
except (ProcessLookupError, PermissionError, OSError):
proc.kill()
await proc.wait()
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/praisonai/praisonai/sandbox/subprocess.py` around lines 198 - 205, The
asyncio.TimeoutError handler currently calls os.killpg(proc.pid, signal.SIGKILL)
which will raise on Windows because os.killpg and signal.SIGKILL may not exist;
update the timeout block in the subprocess timeout handler to
guard/feature-detect before using os.killpg and signal.SIGKILL (e.g., check
hasattr(os, "killpg") and hasattr(signal, "SIGKILL")) and otherwise fall back to
proc.kill(), and also catch AttributeError/OSError around the killpg call so the
code always proceeds to await proc.wait() without raising on Windows; reference
the existing proc, os.killpg, signal.SIGKILL, proc.kill(), and await proc.wait()
in your patch.


return SandboxResult(
Expand Down Expand Up @@ -211,27 +278,41 @@ async def run_command(
from ._shell import build_argv
cmd = build_argv(command, shell=shell)

process_env = os.environ.copy()
if env:
process_env.update(env)
# Build environment based on security policy instead of copying host environment
process_env = self._build_child_env(self.config.security_policy, env)

started_at = time.time()

# preexec_fn is POSIX-only; omit on Windows
popen_kwargs = {
"stdout": asyncio.subprocess.PIPE,
"stderr": asyncio.subprocess.PIPE,
"cwd": working_dir or self._temp_dir,
"env": process_env,
}
if os.name == "posix":
popen_kwargs["start_new_session"] = True
popen_kwargs["preexec_fn"] = lambda: self._apply_rlimits(limits)
else:
logger.warning("Resource limits and session isolation not available on Windows")

try:
proc = await asyncio.create_subprocess_exec(
*cmd,
stdout=asyncio.subprocess.PIPE,
stderr=asyncio.subprocess.PIPE,
cwd=working_dir or self._temp_dir,
env=process_env,
)
proc = await asyncio.create_subprocess_exec(*cmd, **popen_kwargs)

try:
stdout, stderr = await asyncio.wait_for(
proc.communicate(),
timeout=limits.timeout_seconds,
)

# Truncate output if it exceeds max_output_size
max_output_size = self.config.security_policy.max_output_size
if max_output_size and max_output_size > 0:
if len(stdout) > max_output_size:
stdout = stdout[:max_output_size] + b"\n[OUTPUT TRUNCATED]"
if len(stderr) > max_output_size:
stderr = stderr[:max_output_size] + b"\n[OUTPUT TRUNCATED]"

completed_at = time.time()

return SandboxResult(
Expand All @@ -245,7 +326,15 @@ async def run_command(
completed_at=completed_at,
)
except asyncio.TimeoutError:
proc.kill()
# Kill the whole process group, not just the leader
try:
if os.name == "posix":
import signal
os.killpg(proc.pid, signal.SIGKILL)
else:
proc.kill()
except (ProcessLookupError, PermissionError, OSError):
proc.kill()
await proc.wait()

return SandboxResult(
Expand Down
Loading
Loading