[api] Move requestFileSystem into project - #64285
Andrew Branch (andrewbranch) wants to merge 11 commits into
Conversation
There was a problem hiding this comment.
🟡 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.
There was a problem hiding this comment.
🟡 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
requestFileHandlesfirst 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 readslayer.basebefore consulting that cache. Every repeated compiler lookup through an explicit host symlink therefore performs another hostReadFileeven though the newly read content is discarded when the cached handle is returned; checkrequestFileHandlesbefore 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
requestFileHandlescache until afterlayer.base.ReadFilehas 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.
There was a problem hiding this comment.
🔵 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 tobelow.Realpath. That lets the host canonicalize a path that the full filesystem deliberately does not expose (for example, returning a host-cased path whileFileExists/ReadFileare false). Returnlookup.pathwhen fallback is false and only consultbelowfor layer fallthrough.
tsc/internal/project/requestfilesystem.go:442 - On a case-insensitive host,
lookup.infocan resolve a request file or directory from a differently cased query (including after following a request symlink), butlookup.pathretains the query/target spelling. Returning it here violatesvfs.FS.Realpath's casing contract and can give different realpaths for the same request entry. Return the storedrequestFile.fileNameorrequestDirectory.directoryNameinstead, and cover both direct and symlinked lookups.
tsc/internal/project/requestfilesystem.go:496 - When
directoryNameis 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 inRemovedPaths, even thoughGetFile("/alias/name")correctly returns nil. Filter usinglookup.pathso 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.
This is an even smaller, modulo moves, and less abstract, version of #64269