node modules symlink to avoid reinstallation of npm package

[Saga4]

No description provided.

[claude]

Claude finished @Saga4's task in 3m 39s —— View job

---

PR Review Summary

• Triage PR (LARGE: 75 lines of production code)
• Lint and typecheck
• Resolve stale threads (none)
• Review code
• Duplicate detection
• Post summary

---

Prek Checks

uv run prek run --from-ref origin/main — passed (ruff check, ruff format).

mypy reports errors in test_runner.py and vitest_runner.py, but these are pre-existing on main and not introduced by this PR.

---

Code Review

Design: --dir block duplicated within vitest_runner.py

The common-ancestor / --dir logic is copy-pasted verbatim into both _build_vitest_behavioral_command and _build_vitest_benchmarking_command (vitest_runner.py:306-316 and vitest_runner.py:366-374). Since both functions already share the same signature and the block is identical, it should be extracted into a helper:

def _maybe_add_dir_flag(cmd: list[str], project_root: Path | None, test_files: list[Path]) -> None:
    if project_root and test_files:
        resolved_root = project_root.resolve()
        test_dirs = {f.resolve().parent for f in test_files}
        if any(not d.is_relative_to(resolved_root) for d in test_dirs):
            all_paths = [str(resolved_root)] + [str(d) for d in test_dirs]
            common_ancestor = Path(os.path.commonpath(all_paths))
            cmd.append(f"--dir={common_ancestor}")
            logger.debug(f"Test files outside project root, using --dir={common_ancestor}")

Fix this →

Design: _ensure_runtime_files triplicated across runner files

mocha_runner.py:62, test_runner.py:695, and vitest_runner.py:92 each define an identical _ensure_runtime_files. This was already the case before this PR, and the fix is correctly propagated to all three. But consider consolidating these into a single shared utility (e.g., in a js_utils.py module) to avoid this class of drift in future.

Correctness: support.py monorepo symlink logic

The new block (support.py:1956-1960) correctly handles the case where original_node_modules (package-level) doesn't exist in the original repo. The outer condition if not worktree_node_modules.exists() acts as a reliable guard: if the first symlink succeeded, the root-level one is skipped; if not (monorepo case), we fall through and symlink original_js_root/node_modules into current_worktree/node_modules. Logic is sound.

Correctness: --dir flag with absolute test file paths

Vitest accepts explicit file paths as positional args (str(f.resolve())), so they should run regardless. The --dir flag is still needed in some vitest versions to widen internal root resolution — the existing comment explains this well. No issue here.

No bugs or security issues found.

---

Duplicate Detection

Finding	Location	Confidence
_ensure_runtime_files identical in 3 files	mocha_runner.py:62, test_runner.py:695, vitest_runner.py:92	HIGH (pre-existing)
--dir common-ancestor block duplicated	vitest_runner.py:306-316 vs vitest_runner.py:366-374	HIGH (introduced by this PR)

---

Test Coverage

No tests added for the monorepo walking logic or the --dir flag path. These code paths are difficult to unit-test without a real filesystem layout, but a simple parametrized test using tmp_path fixtures to verify the upward walk and the symlink creation would improve confidence.

---

Last updated: 2026-03-20T22:58:00Z