Skip to content

Issue #1540: Changes from Claude #1792

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
MervinPraison merged 6 commits into from
Jun 2, 2026
Merged

Issue #1540: Changes from Claude #1792

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
281adcf
fix: make session store fcntl import cross-platform compatible (fixes…
praisonai-triage-agent[bot] Apr 24, 2026
09f7f4e
fix: add one-time warning for degraded file locking
praisonai-triage-agent[bot] Apr 24, 2026
a9777bf
fix: improve fcntl import robustness and fix unreachable code
praisonai-triage-agent[bot] Jun 2, 2026
d79481b
fix: harden optional fcntl import and add regression test
Copilot Jun 2, 2026
41a6878
test: cover missing fcntl import path for session store
Copilot Jun 2, 2026
da26269
Merge origin/main and resolve fcntl import conflicts
praisonai-triage-agent[bot] Jun 2, 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
8 changes: 4 additions & 4 deletions src/praisonai-agents/praisonaiagents/session/store.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,10 +15,10 @@
import time

# fcntl is Unix-only; on Windows, use msvcrt for file locking
if sys.platform != 'win32':
try:
import fcntl
_HAS_FCNTL = True
else:
except ImportError:
_HAS_FCNTL = False
Comment thread
greptile-apps[bot] marked this conversation as resolved.
Comment on lines +17 to +22
Comment on lines +17 to +22
from dataclasses import dataclass, field
from datetime import datetime, timezone
Expand Down Expand Up @@ -167,11 +167,11 @@ def acquire(self) -> bool:
if _HAS_FCNTL:
fcntl.flock(self._lock_file.fileno(), fcntl.LOCK_EX | fcntl.LOCK_NB)
else:
# Warn once about degraded locking (should never happen with current platform detection)
# Warn once about degraded locking on non-Windows platforms without fcntl
global _WARNED_NO_FCNTL
if not _WARNED_NO_FCNTL:
logger.warning(
"File locking unavailable on this platform (no fcntl/msvcrt); "
"File locking unavailable on this platform (fcntl not available); "
"concurrent writers may corrupt session files."
)
Comment on lines +173 to +176
_WARNED_NO_FCNTL = True
Comment thread
greptile-apps[bot] marked this conversation as resolved.
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Expand Down
23 changes: 23 additions & 0 deletions src/praisonai-agents/tests/unit/session/test_session_store.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,11 @@
import tempfile
import threading
import time
import builtins
import importlib
import pytest
from concurrent.futures import ThreadPoolExecutor
from unittest.mock import patch

from praisonaiagents.session.store import (
DefaultSessionStore,
Expand Down Expand Up @@ -160,6 +163,26 @@ def worker(worker_id):
if start_idx < other_start < end_idx:
pytest.fail(f"Lock interleaving detected: {results}")

def test_import_without_fcntl(self):
"""Test module import succeeds when fcntl is unavailable."""
import praisonaiagents.session.store as store_module
import importlib.util
original_import = builtins.__import__

def import_without_fcntl(name, *args, **kwargs):
if name == "fcntl":
raise ImportError("fcntl unavailable")
return original_import(name, *args, **kwargs)

try:
with patch("builtins.__import__", side_effect=import_without_fcntl):
reloaded_module = importlib.reload(store_module)
assert reloaded_module._HAS_FCNTL is False
finally:
restored_module = importlib.reload(store_module)

assert restored_module._HAS_FCNTL is (importlib.util.find_spec("fcntl") is not None)
Comment on lines +166 to +184
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot Jun 2, 2026

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

Guarantee module restoration with try/finally to prevent cross-test contamination.

The restoring reload at Line 180 sits outside the with patch block and is unguarded. If the assertion at Line 178 fails (or the reload inside the with raises), Line 180 never executes, leaving store_module._HAS_FCNTL stuck at False for the rest of the session. Any subsequent test relying on real fcntl locking (e.g. test_concurrent_locks, test_concurrent_writes) would then silently run in degraded-locking mode, producing confusing failures unrelated to their actual cause.

🛡️ Proposed fix to always restore the module 
     def test_import_without_fcntl(self):
         """Test module import succeeds when fcntl is unavailable."""
         import praisonaiagents.session.store as store_module
+        import importlib.util
         original_import = builtins.__import__
 
         def import_without_fcntl(name, *args, **kwargs):
             if name == "fcntl":
                 raise ImportError("fcntl unavailable")
             return original_import(name, *args, **kwargs)
 
-        with patch("builtins.__import__", side_effect=import_without_fcntl):
-            reloaded_module = importlib.reload(store_module)
-            assert reloaded_module._HAS_FCNTL is False
-
-        restored_module = importlib.reload(store_module)
-        assert restored_module._HAS_FCNTL is (importlib.util.find_spec("fcntl") is not None)
+        try:
+            with patch("builtins.__import__", side_effect=import_without_fcntl):
+                reloaded_module = importlib.reload(store_module)
+                assert reloaded_module._HAS_FCNTL is False
+        finally:
+            restored_module = importlib.reload(store_module)
+        assert restored_module._HAS_FCNTL is (importlib.util.find_spec("fcntl") is not None)
🤖 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-agents/tests/unit/session/test_session_store.py` around lines
166 - 181, The test_import_without_fcntl leaves the module state changed if an
assertion or reload fails; wrap the patched reload and assertion in a
try/finally so the original module is always restored: inside
test_import_without_fcntl, keep the with patch("builtins.__import__",
side_effect=import_without_fcntl) block but ensure you call
importlib.reload(store_module) in a try and then in finally always call
importlib.reload(store_module) to reset store_module._HAS_FCNTL to the real
value; reference the test name (test_import_without_fcntl), the import hook
function (import_without_fcntl), and the module symbol (_HAS_FCNTL) when making
this change.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot Jun 2, 2026

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

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Confirm how store.py detects fcntl and sets _HAS_FCNTL
fd -t f 'store.py' -p 'session' --exec rg -n -C3 '_HAS_FCNTL|import fcntl|find_spec' {}

Repository: MervinPraison/PraisonAI

Length of output: 162

🏁 Script executed:

#!/bin/bash
set -euo pipefail

# 1) Locate the test file and print the relevant section around the assertion.
rg -n "importlib(\.util)?\.find_spec|_HAS_FCNTL" src/praisonai-agents/tests/unit/session/test_session_store.py
echo "---- test file context (around assertion) ----"
python3 - <<'PY'
from pathlib import Path
p = Path("src/praisonai-agents/tests/unit/session/test_session_store.py")
lines = p.read_text().splitlines()
# Print around line 181 (1-based). Clamp safely.
start = max(1, 181-15)
end = min(len(lines), 181+5)
for i in range(start, end+1):
    print(f"{i:4d}: {lines[i-1]}")
PY

# 2) Find the relevant store.py and inspect how _HAS_FCNTL is set.
echo "---- locating store.py and _HAS_FCNTL ----"
# Find any file that defines _HAS_FCNTL
fd --type f "store.py" src/praisonai-agents -a || true
rg -n "_HAS_FCNTL" src/praisonai-agents -S

# Also try to locate a 'session store' implementation file for context.
rg -n "class .*Session|session.*store|Session.*Store|store.*session" src/praisonai-agents -S --glob='*.py' | head -n 50

# Print the relevant part of the file(s) defining _HAS_FCNTL
python3 - <<'PY'
import subprocess, json, os, re
from pathlib import Path

# Get files containing _HAS_FCNTL
import sys
out = subprocess.check_output(["bash","-lc","rg -l \"_HAS_FCNTL\" src/praisonai-agents -S --glob='*.py'"], text=True).splitlines()
for f in out:
    p = Path(f)
    txt = p.read_text().splitlines()
    print(f"\n---- {f} ----")
    for i,line in enumerate(txt, start=1):
        if "_HAS_FCNTL" in line:
            start=max(1,i-10); end=min(len(txt), i+15)
            for j in range(start,end+1):
                print(f"{j:4d}: {txt[j-1]}")
            break
PY

Repository: MervinPraison/PraisonAI

Length of output: 12284

🏁 Script executed:

#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
p = Path("src/praisonai-agents/tests/unit/session/test_session_store.py")
for i, line in enumerate(p.read_text().splitlines()[:60], start=1):
    print(f"{i:3d}: {line}")
PY

Repository: MervinPraison/PraisonAI

Length of output: 1992

🏁 Script executed:

python3 - <<'PY'
import importlib, sys
print("Python", sys.version)
try:
    print("importlib.util exists:", hasattr(importlib, "util"))
    print("importlib.util:", importlib.util)
except Exception as e:
    print("Error accessing importlib.util:", type(e).__name__, e)

# also try calling find_spec if possible
try:
    print("find_spec fcntl:", importlib.util.find_spec("fcntl") is not None)
except Exception as e:
    print("Error calling find_spec:", type(e).__name__, e)
PY

Repository: MervinPraison/PraisonAI

Length of output: 326

🏁 Script executed:

#!/bin/bash
python3 - <<'PY'
import importlib, sys
print("Python", sys.version)
try:
    print("hasattr(importlib, 'util'):", hasattr(importlib, "util"))
    print("importlib.util:", importlib.util)
    print("find_spec('fcntl') is not None:", importlib.util.find_spec("fcntl") is not None)
except Exception as e:
    print("Error accessing importlib.util:", type(e).__name__, e)
PY

Repository: MervinPraison/PraisonAI

Length of output: 248

Fix test determinism: explicitly import importlib.util before using importlib.util.find_spec.

test_session_store.py only imports import importlib, and in Python importlib.util is not available as an attribute unless import importlib.util is imported; the assertion would raise AttributeError. _HAS_FCNTL in praisonaiagents/session/store.py is set via try: import fcntl / except ImportError, so the builtins.__import__ mock drives the expected _HAS_FCNTL value—the remaining issue is just the missing importlib.util import.

🤖 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-agents/tests/unit/session/test_session_store.py` at line 181,
The test fails deterministically because importlib.util is not imported before
calling importlib.util.find_spec in test_session_store (the assertion uses
restored_module._HAS_FCNTL vs importlib.util.find_spec("fcntl")); fix by adding
an explicit import for importlib.util at the top of the test (or immediately
before the assertion) so importlib.util.find_spec is available, leaving the
existing restored_module and _HAS_FCNTL checks unchanged.



class TestDefaultSessionStore:
"""Tests for DefaultSessionStore."""
Expand Down
Loading