fix: remove undeclared dotenv dependency from KG rebuild - #637
Conversation
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_ceeab423-fcf5-4f55-8ac9-5237035867a2) |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
📝 WalkthroughWalkthroughThe KG rebuild script no longer loads ChangesKG rebuild import behavior
Estimated code review effort: 2 (Simple) | ~10 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 955e57e966
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| load_dotenv(Path(__file__).resolve().parent.parent / ".env") | ||
|
|
||
| from brainlayer.paths import get_db_path |
There was a problem hiding this comment.
When a developer keeps BRAINLAYER_DB in the repo .env, removing this load makes main() silently fall back to the canonical database via get_db_path(). Running either rebuild tier then writes entities and relations to the live canonical DB instead of the configured sandbox/custom DB; replace the dotenv dependency with dependency-free loading that preserves the prior override, or require an explicit DB argument rather than silently changing targets.
AGENTS.md reference: AGENTS.md:L5-L8
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with 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.
Inline comments:
In `@tests/test_kg_rebuild.py`:
- Around line 327-329: Update the import-check test around the direct
sys.modules.pop and importlib.import_module calls to save the existing
scripts.kg_rebuild module and parent-package attribute, then restore both in a
finally block even when the import fails. Preserve the import-hook validation
while preventing this test from leaving process-wide module state altered.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 82e8ccce-f3e8-40ad-8f42-4947e0c72327
📒 Files selected for processing (2)
scripts/kg_rebuild.pytests/test_kg_rebuild.py
💤 Files with no reviewable changes (1)
- scripts/kg_rebuild.py
📜 Review details
⏰ Context from checks skipped due to timeout. (3)
- GitHub Check: test (3.12)
- GitHub Check: test (3.11)
- GitHub Check: test (3.13)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Flag risky DB or concurrency changes explicitly and do not hand-wave lock behavior
Enforce one-write-at-a-time concurrency constraint; reads are safe but brain_digest is write-heavy and must not run in parallel with other MCP work
Run pytest before claiming behavior changed safely; current test suite has 929 tests
Files:
tests/test_kg_rebuild.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Tests must not refresh the production backup heartbeat log; use
BRAINLAYER_BACKUP_LOG_PATHand set provenance topytest.
Files:
tests/test_kg_rebuild.py
🔇 Additional comments (1)
tests/test_kg_rebuild.py (1)
314-329: 🗄️ Data Integrity & IntegrationNo change needed.
scripts.kg_rebuildimports do not trigger the production backup heartbeat log, so settingBRAINLAYER_BACKUP_LOG_PATHor provenance before this import is not needed.> Likely an incorrect or invalid review comment.
| sys.modules.pop("scripts.kg_rebuild", None) | ||
|
|
||
| module = importlib.import_module("scripts.kg_rebuild") |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files =="
git ls-files | rg '(^|/)tests/test_kg_rebuild\.py$|(^|/)scripts/kg_rebuild\.py$|(^|/)pytest|\.ini$|pyproject\.toml' || true
echo "== relevant test section =="
if [ -f tests/test_kg_rebuild.py ]; then
nl -ba tests/test_kg_rebuild.py | sed -n '280,360p'
fi
echo "== references to monkeypatch/sys.modules/import hook and scripts.kg_rebuild =="
rg -n "builtins\.__import__|sys\.modules\['scripts\.kg_rebuild'\]|sys\.modules\.pop|importlib\.import_module|scripts\.kg_rebuild|BRAINLAYER_BACKUP_LOG_PATH|pytest" tests/test_kg_rebuild.py | sed -n '1,220p'Repository: EtanHey/brainlayer
Length of output: 298
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
import os, re
for path in ["tests/test_kg_rebuild.py", "scripts/kg_rebuild.py"]:
print(f"== {path} size/exists ==")
print(f"exists={os.path.exists(path)}")
if os.path.exists(path):
with open(path, encoding="utf-8") as f:
lines = f.read().splitlines()
print(f"lines={len(lines)}")
start=max(1,280) if path=="tests/test_kg_rebuild.py" else 1
end=min(len(lines),360) if path=="tests/test_kg_rebuild.py" else 170
for i in range(start,min(end+1,len(lines))+1):
print(f"{i:04d}\t{lines[i-1]}")
PY
echo "== focused references =="
rg -n "builtins\.__import__|sys\.modules\['scripts\.kg_rebuild'\]|sys\.modules\.pop|importlib\.import_module|scripts\.kg_rebuild|BRAINLAYER_BACKUP_LOG_PATH|pytest|monkeypatch" tests/test_kg_rebuild.py | head -n 220 || true
echo "== pytest config =="
for f in pyproject.toml pytest.ini; do
if [ -f "$f" ]; then
echo "--- $f ---"
sed -n '1,220p' "$f"
fi
doneRepository: EtanHey/brainlayer
Length of output: 14148
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
import ast
from pathlib import Path
path = Path("tests/test_kg_rebuild.py")
src = path.read_text(encoding="utf-8")
tree = ast.parse(src)
for node in ast.walk(tree):
if isinstance(node, ast.ClassDef) or isinstance(node, ast.FunctionDef) or isinstance(node, ast.AsyncFunctionDef):
if "test_" in str(node.name):
print(f"Function {node.name} at lines {node.lineno}-{getattr(node, 'end_lineno', '?')}")
test = next(n for n in tree.body if isinstance(n, ast.FunctionDef) and n.name == "test_kg_rebuild_module_import_does_not_require_python_dotenv")
print("\nBody nodes:")
for n in test.body:
print(type(n).__name__, getattr(n, "lineno", "?"), getattr(n, "end_lineno", "?"), ast.unparse(n)[:260])
print("\nsys.modules.pop occurrences:")
for n in ast.walk(test):
if isinstance(n, ast.Call):
func = n.func
if isinstance(getattr(func, "value", None), ast.Attribute) and getattr(func, "attr", None) == "pop":
print(ast.unparse(n))
print("\nsys.modules.pop with cleanup occurrences:")
for n in ast.walk(test):
if isinstance(n, ast.Try):
for stmt in n.body:
print("try body first:", ast.unparse(stmt)[:240])
cleanup = []
for h in n.handlers:
for stmt in h.body:
cleanup.append(ast.unparse(stmt)[:240])
for stmt in n.finalbody:
cleanup.append(ast.unparse(stmt)[:240])
if any("sys.modules" in s or "builtins" in s for s in cleanup):
print("cleanup:")
for s in cleanup:
print(" " + s)
PYRepository: EtanHey/brainlayer
Length of output: 2051
Restore scripts.kg_rebuild after checking the import hook.
monkeypatch restores builtins.__import__, but it does not restore the direct sys.modules.pop("scripts.kg_rebuild", None). If another test loaded scripts.kg_rebuild first, this test replaces it process-wide; an import failure then leaves it removed. Restore the previous module entry and parent package attribute in a finally block, or run this import check in a separate process.
🤖 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 `@tests/test_kg_rebuild.py` around lines 327 - 329, Update the import-check
test around the direct sys.modules.pop and importlib.import_module calls to save
the existing scripts.kg_rebuild module and parent-package attribute, then
restore both in a finally block even when the import fails. Preserve the
import-hook validation while preventing this test from leaving process-wide
module state altered.
1 participant
Summary\n- remove the import-time python-dotenv dependency from scripts/kg_rebuild.py\n- add a regression test proving the KG rebuild helper imports when python-dotenv is unavailable\n\n## Root causes\n- #631 test (3.11): scripts/kg_rebuild.py imported python-dotenv at module load even though it is not a declared dependency\n- #634 test (3.11): the same import-time python-dotenv failure\n- #634 lint: Ruff 0.16.1 wants the branch-only t3_health_path assignment in src/brainlayer/health_check.py formatted on one line; this does not exist on main and will be fixed on that branch after this PR lands\n- #635 test (3.13): the same import-time python-dotenv failure, not a separate Python 3.13 incompatibility\n\nThis chooses option (b): the unit test should not require dotenv. The Groq extraction path already reads GROQ_API_KEY from the process environment and has its own dependency-free repo .env fallback, so loading .env at module import was redundant and made pure helper imports depend on an undeclared package.\n\n## Verification\n- Python 3.11: tests/test_kg_rebuild.py — 20 passed\n- Python 3.13: tests/test_kg_rebuild.py — 20 passed\n- Python 3.12 repository pre-push gate:\n - unit suite — 3641 passed, 9 skipped, 61 deselected, 1 xfailed\n - MCP registration — 3 passed\n - isolated eval/hook routing — 40 passed\n - Bun stale-index test — passed\n - FTS5 determinism shell regression — passed\n- Ruff 0.16.1 check and format check — passed\n\nReview routing is intentionally left to @brainlayer.
Note
Low Risk
Small dependency/import cleanup with a regression test; no change to KG extraction logic or Groq env loading at runtime.
Overview
Removes import-time
python-dotenvloading fromscripts/kg_rebuild.pyso importing the module (including pure helpers likeextracted_entity_from_groq_payloadin tests) no longer depends on an undeclared package.Groq Tier 2 behavior is unchanged:
call_groq_nerstill readsGROQ_API_KEYfrom the process environment and can fall back to parsing the repo.envwithoutdotenv.Adds
test_kg_rebuild_module_import_does_not_require_python_dotenv, which blocksdotenvimports and assertsscripts.kg_rebuildstill loads successfully.Reviewed by Cursor Bugbot for commit 955e57e. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Remove undeclared
python-dotenvdependency fromscripts/kg_rebuild.pyRemoves the
dotenv.load_dotenvimport and the.envfile load from kg_rebuild.py, which was failing for users withoutpython-dotenvinstalled. A regression test in test_kg_rebuild.py verifies the module imports cleanly whendotenvis unavailable.Macroscope summarized 955e57e.
Summary by CodeRabbit
Bug Fixes
.envloading.Tests