Skip to content

JS-769 Restrict nested node_modules lookups and remove ruling node_modules cleanup#6636

Open
zglicz wants to merge 6 commits intomasterfrom
codex/js-769-compilerhost-base-dir
Open

JS-769 Restrict nested node_modules lookups and remove ruling node_modules cleanup#6636
zglicz wants to merge 6 commits intomasterfrom
codex/js-769-compilerhost-base-dir

Conversation

@zglicz
Copy link
Contributor

@zglicz zglicz commented Mar 19, 2026

Summary

  • normalize compiler-host paths with normalizeToAbsolutePath(..., baseDir)
  • gate only readDirectory for node_modules paths outside the analyzed baseDir
  • keep node_modules lookups under baseDir allowed
  • keep TypeScript default library location lookups allowed in the guard
  • remove cleanRootNodeModules() from ruling IT setup (RulingTest)
  • simplify compiler-host tests to assert skip behavior in readDirectory only

Validation

  • npx tsx --tsconfig packages/tsconfig.test.json --test packages/jsts/tests/program/compilerHost.test.ts
  • npx tsx --tsconfig packages/tsconfig.test.json --test packages/jsts/tests/program/*.test.ts

@zglicz zglicz requested a review from a team March 19, 2026 09:40
@hashicorp-vault-sonar-prod
Copy link

hashicorp-vault-sonar-prod bot commented Mar 19, 2026

JS-769

@sonar-review-alpha
Copy link

sonar-review-alpha bot commented Mar 19, 2026

Summary

Replace global node_modules cleanup with surgical compiler-host filtering. Instead of deleting the project's root node_modules in test setup (which is a CI workaround), the compiler host now intelligently guards lookups: it skips node_modules reads outside the analyzed baseDir, while allowing them inside baseDir and for TypeScript's default libraries. This eliminates the crude CI cleanup and makes module resolution behavior explicit and testable.

What reviewers should know

Start with the shouldSkipNodeModulesOutsideBaseDir() method in compilerHost.ts — it's the core logic: allows only paths under baseDir or paths starting with the default lib location. Then review the readDirectory() changes to see how it applies this guard. The tests in compilerHost.test.ts provide clear scenarios: external node_modules is skipped (empty result), internal node_modules queries hit disk. Finally, verify that removing cleanRootNodeModules() from RulingTest.java doesn't break CI (the compiler host filtering should now prevent any stray typings from being picked up).


  • Generate Walkthrough
  • Generate Diagram

🗣️ Give feedback

@github-actions
Copy link
Contributor

github-actions bot commented Mar 19, 2026

Ruling Report

No changes to ruling expected issues in this PR

Copy link

@sonar-review-alpha sonar-review-alpha bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Conclusion: The core approach is sound and the ruling test cleanup is welcome. Two issues need addressing before merge: a self-inconsistency in the skip guard and a gap in test coverage for the new behaviour.

🗣️ Give feedback

expect(cache.get(normalizeToAbsolutePath('/project/src/index.ts'))).toBe(content);
});

it('should not skip node_modules reads nested under baseDir', () => {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The skip behaviour introduced by this PR is only verified positively for fileExists (the "should skip node_modules lookups outside baseDir" test at line 234). readFile and getSourceFile each have a "should not skip" test but no complementary "should skip" test. If the shouldSkipNodeModulesOutsideBaseDir guard were accidentally removed from either method, nothing here would catch it. Add one test per method asserting that calling readFile/getSourceFile with /external/node_modules/pkg/index.d.ts triggers the skip op and returns undefined.

  • Mark as noise

@sonarqube-next
Copy link

Copy link

@sonar-review-alpha sonar-review-alpha bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! ✅

🗣️ Give feedback

Copy link
Contributor

@vdiez vdiez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we get rid of the symlink creation for jsts ruling?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants