test: assert build-db PYTHONPATH uses package root#47
Conversation
The test was asserting that PYTHONPATH contains "archaeology" or "DEV-ARCH" — hardcoded local directory names. In CI the repo is cloned as "devarch-framework", so neither substring matched. Replace with an assertion that PYTHONPATH is an absolute path (which is the actual invariant the CLI code guarantees via Path(__file__).parent.parent). Fixes #39
📝 WalkthroughWalkthroughCLI commands that spawn internal Python modules now build PYTHONPATH using os.pathsep for cross-platform correctness, and the test test_build_db_propagates_pythonpath imports archaeology.cli to derive and assert the expected PYTHONPATH root from cli.file. ChangesCLI PYTHONPATH fixes and test update
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
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_cli_coverage.py`:
- Around line 401-405: The CLI builds PYTHONPATH by concatenating _pkg_root with
a hardcoded ":" which breaks on Windows; in the places where _env["PYTHONPATH"]
is set (assignments referencing _pkg_root and _env.get("PYTHONPATH") in the CLI
logic—the lines that currently use ":"), replace the literal ":" with os.pathsep
and handle empty existing PYTHONPATH safely (e.g. set _env["PYTHONPATH"] =
_pkg_root + (os.pathsep + _env["PYTHONPATH"] if _env.get("PYTHONPATH") else "")
), ensuring you import os where needed and update all occurrences (the
assignments that construct PYTHONPATH using _pkg_root and _env).
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: ead38582-c56d-43ea-bb92-8b27256e1b6a
📒 Files selected for processing (1)
tests/test_cli_coverage.py
…ssertion # Conflicts: # tests/test_cli_coverage.py
There was a problem hiding this comment.
🧹 Nitpick comments (1)
archaeology/cli.py (1)
86-86: ⚡ Quick winExtract PYTHONPATH assembly into a helper to avoid drift.
The same PYTHONPATH prepend logic is repeated in four places; a small helper would keep behavior consistent and reduce future mismatch risk.
Proposed refactor
+def _with_pkg_root_pythonpath(env: dict[str, str]) -> dict[str, str]: + out = env.copy() + pkg_root = str(Path(__file__).parent.parent) + existing = out.get("PYTHONPATH") + out["PYTHONPATH"] = pkg_root if not existing else f"{pkg_root}{os.pathsep}{existing}" + return out + @@ - _env = os.environ.copy() - _pkg_root = str(Path(__file__).parent.parent) - _env["PYTHONPATH"] = _pkg_root + ((os.pathsep + _env["PYTHONPATH"]) if _env.get("PYTHONPATH") else "") + _env = _with_pkg_root_pythonpath(os.environ)Also applies to: 146-146, 661-661, 996-996
🤖 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 `@archaeology/cli.py` at line 86, Extract the repeated PYTHONPATH assembly into a single helper (e.g., prepend_pkg_root_to_pythonpath(env, pkg_root)) that takes an env dict and a pkg_root string and sets env["PYTHONPATH"] = pkg_root + (os.pathsep + env["PYTHONPATH"] if env.get("PYTHONPATH") else ""); replace the four occurrences that currently inline this logic with calls to that helper so behavior is identical and centralized (use the same helper name where functions like the code around _env and _pkg_root currently run).
🤖 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.
Nitpick comments:
In `@archaeology/cli.py`:
- Line 86: Extract the repeated PYTHONPATH assembly into a single helper (e.g.,
prepend_pkg_root_to_pythonpath(env, pkg_root)) that takes an env dict and a
pkg_root string and sets env["PYTHONPATH"] = pkg_root + (os.pathsep +
env["PYTHONPATH"] if env.get("PYTHONPATH") else ""); replace the four
occurrences that currently inline this logic with calls to that helper so
behavior is identical and centralized (use the same helper name where functions
like the code around _env and _pkg_root currently run).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 96608072-21fd-4008-9635-233f25b81196
📒 Files selected for processing (1)
archaeology/cli.py
Problem
Follow-up from focused review: the previous PR fixed CI by making the PYTHONPATH assertion repo-name agnostic, but it only checked that the first PYTHONPATH entry was absolute. That would still allow regressions where build-db sets PYTHONPATH to an unrelated absolute path like /tmp.
Fix
Assert that the first PYTHONPATH entry exactly matches the package root derived from archaeology.cli.file, while still using os.pathsep for cross-platform path splitting.
Validation
93 tests passed locally.
Need help on this PR? Tag
@codesmithwith what you need. Autofix is disabled.Summary by CodeRabbit
Bug Fixes
Tests