test(init): resolve sys.path entries against cwd so the suite is green from a worktree (#454) - #482
Merged
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (5)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe test now normalizes ChangesWorktree-safe test fix
Priority: ➖ Normal Estimated code review effort: 2 (Simple) | ~10 minutes Change: Other · Severity of issue fixed: Medium Suggested reviewers: Merge Risk: ⚪ Minimal · up to The worktree-specific false failure is addressed without changing shipped behavior. No current merge-blocking risk remains. 🚥 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 |
`test_init_filters_sys_path_from_leaked_pythonpath` failed all five sentinel params in any linked worktree, so every lane tonight ran the suite with it deselected and paid a false red before working that out. The behaviour under test was fine throughout. The assertion was the problem: it iterated `for p in sys.path if p`, which deliberately excludes the empty string — and the empty string is the cwd marker `python -c` puts on the path, which is what actually makes the child's import resolve. Measured in both trees with PYTHONPATH set to the sentinel, as the test sets it: main tree RESOLVED_TO <main>/mempalace/__init__.py PARENT_ON_SYS_PATH True worktree RESOLVED_TO <wt>/mempalace/__init__.py PARENT_ON_SYS_PATH False both EMPTY_IN_PATH True The child imports the tree it was launched from, in both cases, via that cwd entry. The absolute-entry match succeeded in the main tree for an incidental reason: the shared venv's editable install adds exactly `/home/jp/Projects/memorypalace` to sys.path, so the comparison found it there. From a worktree the editable entry points at the main tree, the absolute match fails, and the test reports an over-strip that never happened. Fix: normalise each entry with `os.path.abspath(p or os.curdir)` before comparing, so the cwd marker counts as the entry it is. That is faithful to the assertion's stated purpose — "the mempalace package itself must remain importable, so its parent directory must survive on sys.path" — because the cwd entry is how it remained importable. It keeps its teeth for the real failure: the assertion can now only fail if the filter removed both the cwd marker and any absolute entry providing the package, which is exactly an over-strip. A child that cannot import mempalace at all still trips the earlier `returncode == 0` assertion. `MEMPALACE_IMPORTED_FROM` is printed for diagnosis and deliberately not asserted on: which tree the child resolves depends on its cwd, and pinning that would re-introduce the coupling this fixes. Verified both directions: 8/8 from a linked worktree, and 8/8 with cwd set to the main tree. The full suite now runs green from a worktree with no manual deselect — 7026 passed, 82 skipped. Fixes #454 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
One new file under docs/fork-changes/ per #480's per-entry format, with `commit: HEAD` and `fork_pr: 482` so the merge step resolves the squash sha rather than the lane recording a branch sha that goes unreachable. `seq` 139 from `scripts/fork_changes.py --next-seq`. Renders FORK_CHANGELOG.md, the README table and website/public/llms-full.txt. No test-count literal: #480 removed it and check-docs derives the count. Part of #454 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
jphein
force-pushed
the
fix/454-init-test-worktree
branch
from
September 11, 2026 06:03
8b4509b to
012bb1c
Compare
This was referenced Sep 11, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
tests/test_init.py::test_init_filters_sys_path_from_leaked_pythonpathfailed all five sentinel params in any linked worktree. Every lane in tonight's drain wave ran the suite with it deselected and paid a false red before working that out.Why
The behaviour under test was correct the whole time. The assertion was wrong, and it was wrong in a way that only shows up outside the main tree.
It iterated
for p in sys.path if p, which deliberately excludes the empty string — and the empty string is the cwd markerpython -cputs onsys.path, which is what actually resolves the child's import. Measured in both trees withPYTHONPATHset to the sentinel, exactly as the test sets it:The child imports the tree it was launched from in both cases, via that cwd entry. The absolute-entry match succeeded in the main tree for an incidental reason: the shared venv's editable install adds exactly
/home/jp/Projects/memorypalacetosys.path, so the comparison found it there. From a linked worktree the editable entry points at the main tree, the absolute match fails, and the test reports an over-strip that never happened.So the green in the main tree was luck, not coverage — the assertion was measuring "is this directory the editable install's target?", not "did the filter over-strip?".
How
Normalise each entry with
os.path.abspath(p or os.curdir)before comparing, so the cwd marker counts as the entry it actually is.That is faithful to the assertion's own stated purpose — "the mempalace package itself must remain importable, so its parent directory must survive on sys.path" — because the cwd entry is precisely how it remained importable.
It keeps its teeth. The assertion can now only fail if the filter removed both the cwd marker and any absolute entry providing the package, which is exactly an over-strip. And a child that cannot import mempalace at all still trips the earlier
returncode == 0assertion, so the "unimportable" case is covered twice.MEMPALACE_IMPORTED_FROMis printed for diagnosis and deliberately not asserted on: which tree the child resolves depends on its cwd, and pinning that would re-introduce the exact coupling this PR removes. I wrote that assertion first and took it back out for that reason.Positive control, both directions
As requested — a fix that only passes where it was already passing would be worthless:
And the payoff: the full suite now runs green from a worktree with no manual deselect —
7026 passed, 82 skipped, 115 deselected(the 115 are the benchmark/stress markers, not a hand-written exclusion).No behaviour change to shipped code;
tests/test_init.pyis the only source file touched. Test count unchanged at 7108, since this repairs five existing params rather than adding any.One note for whoever merges
The changelog entry cites the PR-branch commit and will need re-pointing to the squash sha until #480's
commit: HEAD+fork_pr:format lands. Noted in the docs commit message too.Fixes #454
Summary by CodeRabbit
Bug Fixes
Documentation