Fix panic when passing @ response file argument to CLI#3859
Conversation
The CLI crashed with a panic when passing `@` or `@filename` because the response file path was passed directly to vfs.ReadFile without being made absolute first. Now parseResponseFile uses tspath.GetNormalizedAbsolutePath to convert the filename to an absolute path using the current directory. Co-authored-by: RyanCavanaugh <6685088+RyanCavanaugh@users.noreply.github.com>
@@ response file argument to CLI
| decls []*CommandLineOption, | ||
| commandLine []string, | ||
| fs vfs.FS, | ||
| currentDirectory ...string, |
There was a problem hiding this comment.
@copilot why is this ...string instead of just a singular parameter?
There was a problem hiding this comment.
No good reason — I used variadic to avoid updating the existing call site that omitted it, but that's not worth the indirection. Changed to a plain string parameter and added the "" argument at the existing call site.
…lain string Co-authored-by: DanielRosenwasser <972891+DanielRosenwasser@users.noreply.github.com>
|
|
||
| // f.workerDiagnostic is either defined or set to default pointer in `createSubScenario` | ||
| parsed := tsoptions.ParseCommandLineTestWorker(f.optDecls, f.commandLine, osvfs.FS()) | ||
| parsed := tsoptions.ParseCommandLineTestWorker(f.optDecls, f.commandLine, osvfs.FS(), "") |
There was a problem hiding this comment.
this test should really have a CWD, not empty
There was a problem hiding this comment.
Changed to "/" — these tests use osvfs.FS() with file paths like 0.ts that aren't real, so a real working directory isn't meaningful, but "/" is a valid absolute path.
There was a problem hiding this comment.
If these are osvfs, then no, / is wrong on Windows! Think again!
There was a problem hiding this comment.
You're right — switched to t.TempDir() which returns a valid absolute path on any platform.
|
@copilot address CR comments |
Co-authored-by: RyanCavanaugh <6685088+RyanCavanaugh@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Fixes a panic in tsgo when a response file argument (@ or @blah) is passed without an absolute path, by threading currentDirectory through the command line parser so the response file path can be normalized before being read.
Changes:
- Added
currentDirectoryfield tocommandLineParserand propagated it throughParseCommandLine,ParseBuildCommandLine, andparseCommandLineWorker. - Normalized the response file name to an absolute path in
parseResponseFilebefore reading. - Added
TestResponseFileDoesNotPaniccovering both empty and relative response file paths; updatedParseCommandLineTestWorkersignature accordingly.
Show a summary per file
| File | Description |
|---|---|
| internal/tsoptions/commandlineparser.go | Threads currentDirectory through the parser and normalizes the response file path before reading. |
| internal/tsoptions/export_test.go | Adds currentDirectory parameter to the test worker helper. |
| internal/tsoptions/commandlineparser_test.go | Adds regression tests for @ and @blah cases and updates existing call site. |
Copilot's findings
- Files reviewed: 3/3 changed files
- Comments generated: 0
jakebailey
left a comment
jakebailey
left a comment
There was a problem hiding this comment.
This fix seems fine enough?
|
@copilot resolve merge conflicts |
…ment-crash # Conflicts: # internal/tsoptions/commandlineparser_test.go Co-authored-by: RyanCavanaugh <6685088+RyanCavanaugh@users.noreply.github.com>
tsgo @andtsgo @blahpanic because the response file path is passed tovfs.ReadFilewithout being made absolute first.currentDirectoryfromParseCommandLine/ParseBuildCommandLinethroughparseCommandLineWorkerinto thecommandLineParserstructtspath.GetNormalizedAbsolutePath(fileName, p.currentDirectory)inparseResponseFilebefore readingTestResponseFileDoesNotPaniccovering both empty (@) and relative (@blah) cases