Skip to content

[api] Move requestFileSystem into project - #64285

Closed
Andrew Branch (andrewbranch) wants to merge 11 commits into
microsoft:mainfrom
andrewbranch:api-layered-vfs-2
Closed

Andrew Branch (andrewbranch) wants to merge 11 commits into
microsoft:mainfrom
andrewbranch:api-layered-vfs-2

Conversation

@andrewbranch

@andrewbranch Andrew Branch (andrewbranch) commented Sep 15, 2026

Copy link
Copy Markdown
Member

This is an even smaller, modulo moves, and less abstract, version of #64269

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

Symlink traversal and invalidation defects can hang snapshot updates or leave programs using stale source files.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Moves request filesystem layering into the project snapshot system so API-provided files correctly override editor overlays.

Changes:

  • Integrates request layers with snapshot, directory, and auto-import file access.
  • Moves request filesystem types and change tracking into project.
  • Expands API and filesystem-layer tests.
File summaries
File Description
tsc/internal/vfs/vfsmatch/vfsmatch.go Generalizes directory traversal behind an interface.
tsc/internal/project/snapshothost.go Removes separate filesystem parameters from snapshot cloning.
tsc/internal/project/snapshotfs.go Implements layered snapshot filesystem access and invalidation.
tsc/internal/project/snapshotfs_test.go Updates builder construction in existing tests.
tsc/internal/project/snapshot.go Stores request layers as snapshot state.
tsc/internal/project/snapshot_test.go Updates temporary snapshot invocation.
tsc/internal/project/requestfilesystem.go Moves and adapts request filesystem behavior.
tsc/internal/project/requestfilesystem_test.go Migrates request filesystem tests to project snapshots.
tsc/internal/project/requestfilesystem_path.go Adds stable file handles to request entries.
tsc/internal/project/requestfilesystem_path_test.go Updates path-tree tests for the new abstraction.
tsc/internal/project/requestfilesystem_changes.go Adds layer-aware file-change generation.
tsc/internal/project/requestfilesystem_changes_test.go Tests layer change summaries.
tsc/internal/project/refcountcache_test.go Updates snapshot clone calls.
tsc/internal/project/projectcollectionbuilder.go Uses the layered filesystem abstraction.
tsc/internal/project/project.go Uses layered case-sensitivity access.
tsc/internal/project/overlayfs.go Exposes immutable file-handle creation.
tsc/internal/project/autoimport.go Routes auto-import reads through request layers.
tsc/internal/project/autoimport_test.go Tests layered auto-import reads.
tsc/internal/project/api.go Passes filesystem requests directly into snapshots.
tsc/internal/api/session.go Removes separately tracked API filesystem state.
tsc/internal/api/session_requestfilesystem_test.go Adds extensive API layer integration coverage.
tsc/internal/api/requestfilesystem/filechanges.go Removes obsolete API-layer change tracking.
tsc/internal/api/requestfilesystem/filechanges_test.go Removes superseded tests.
tsc/internal/api/proto.go References project-owned filesystem request types.
packages/typescript/src/api/proto.generated.ts Updates generated directory-listing documentation.
packages/typescript/src/api/fs.ts Clarifies request filesystem listing semantics.
Review details
  • Files reviewed: 26/26 changed files
  • Comments generated: 4
  • Review effort level: Balanced

💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.

Comment thread tsc/internal/project/requestfilesystem_changes.go Outdated
Comment thread tsc/internal/project/snapshotfs.go
Comment thread tsc/internal/project/snapshotfs.go Outdated
Comment thread tsc/internal/api/proto.go

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

Two critical invalidation issues and three moderate cache-order issues remain unresolved.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (3)

tsc/internal/project/autoimport.go:52

  • The auto-import implementation also reads the host before checking the shared request-handle cache, which causes every repeated lookup through a host symlink to hit the underlying filesystem. Check requestFileHandles first so this path has the same caching behavior as the other request-layer lookups.
		if lookup.host {
			if content, ok := layer.base.ReadFile(lookup.path); ok {
				return a.snapshotFSBuilder.requestFileHandle(originalFileName, originalPath, content)
			}

tsc/internal/project/snapshotfs.go:152

  • The host-symlink path is cached in requestFileHandles, but this branch reads layer.base before consulting that cache. Every repeated compiler lookup through an explicit host symlink therefore performs another host ReadFile even though the newly read content is discarded when the cached handle is returned; check requestFileHandles before the host read so the snapshot cache actually avoids repeated I/O.
		if lookup.host {
			if content, ok := layer.base.ReadFile(lookup.path); ok {
				return s.requestFileHandle(originalFileName, originalPath, content)
			}

tsc/internal/project/snapshotfs.go:408

  • This duplicate host-symlink branch bypasses the requestFileHandles cache until after layer.base.ReadFile has already run, so repeated auto/project lookups reread the host and then discard the result. Load the cached handle before calling the host filesystem, as in the lower-layer cache paths.
		if lookup.host {
			if content, ok := layer.base.ReadFile(lookup.path); ok {
				return s.requestFileHandle(originalFileName, originalPath, content)
			}
  • Files reviewed: 26/26 changed files
  • Comments generated: 2
  • Review effort level: Lite (auto)

Note

Copilot is running an experiment and ran this review at Lite.

Comment thread tsc/internal/project/requestfilesystem_changes.go
Comment thread tsc/internal/project/snapshot.go Outdated

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔵 Needs a closer look

Three unresolved moderate findings remain in request filesystem path resolution and enumeration.

Review details

Suppressed comments (3)

tsc/internal/project/requestfilesystem.go:447

  • For a full request filesystem, an absent path has lookup.fallback == false, but this branch still delegates to below.Realpath. That lets the host canonicalize a path that the full filesystem deliberately does not expose (for example, returning a host-cased path while FileExists/ReadFile are false). Return lookup.path when fallback is false and only consult below for layer fallthrough.
    tsc/internal/project/requestfilesystem.go:442
  • On a case-insensitive host, lookup.info can resolve a request file or directory from a differently cased query (including after following a request symlink), but lookup.path retains the query/target spelling. Returning it here violates vfs.FS.Realpath's casing contract and can give different realpaths for the same request entry. Return the stored requestFile.fileName or requestDirectory.directoryName instead, and cover both direct and symlinked lookups.
    tsc/internal/project/requestfilesystem.go:496
  • When directoryName is a request symlink (for example, /alias -> /target), this filters lower entries using /alias/name, but tombstones are stored under /target/name. GetAccessibleEntries("/alias") therefore still advertises a target path listed in RemovedPaths, even though GetFile("/alias/name") correctly returns nil. Filter using lookup.path so removals are applied after symlink resolution, and add a regression test for enumeration through an alias.
  • Files reviewed: 26/26 changed files
  • Comments generated: 0 new
  • Review effort level: Lite (auto)

Note

Copilot is running an experiment and ran this review at Lite.

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

Labels

Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants