Run tar from its resolved path when extracting zstd bundles - #4281
henrymercer merged 2 commits into
Conversation
Sorry, something went wrong.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
The tests still lack coverage proving that a distinct configured tar executable is spawned, so they would not catch a reversion to the hard-coded command.
1 open issue 1 file not reviewed- lib/entry-points.js: Excluded by your content exclusion settings
This PR makes zstd bundle extraction use the same tar executable whose version was checked, avoiding a different executable being selected on Windows.
Changes:
- Records the resolved executable path and uses it for extraction, command logging, and errors.
- Corrects stdout/stderr ordering so extraction errors report tar’s stderr.
- Updates test fixtures with the new executable-path field.
| File | Description |
|---|---|
| src/tools-download.test.ts | Adds executable paths to download-test fixtures. |
| src/tar.ts | Uses the resolved tar path and corrects error-output handling. |
| src/tar.test.ts | Adds an executable path to the stream-error test fixture. |
Review effort: Balanced (auto)
Sorry, something went wrong.
|
|
||
| await new Promise<void>((resolve, reject) => { | ||
| const tarProcess = spawn("tar", args, { stdio: "pipe" }); | ||
| const tarProcess = spawn(tarVersion.path, args, { stdio: "pipe" }); |
There was a problem hiding this comment.
I'll make a follow up improvement to the binary planting custom CodeQL query to cover this.
Sorry, something went wrong.
mbg
left a comment
There was a problem hiding this comment.
LGTM. Just one comment with a question that's not directly related to the change here.
Sorry, something went wrong.
| process.stdout.write(`[command]tar ${args.join(" ")}\n`); | ||
| process.stdout.write(`[command]${tarVersion.path} ${args.join(" ")}\n`); | ||
|
|
||
| await new Promise<void>((resolve, reject) => { | ||
| const tarProcess = spawn("tar", args, { stdio: "pipe" }); | ||
| const tarProcess = spawn(tarVersion.path, args, { stdio: "pipe" }); |
There was a problem hiding this comment.
Not a change in this PR, but is there a reason here why this doesn't use e.g. ToolRunner?
Sorry, something went wrong.
There was a problem hiding this comment.
Afaict, we can't pipe in a HTTP stream with ToolRunner.
Sorry, something went wrong.
extractTarZst ran tar by name. On Windows, child_process.spawn by default looks for a bare command name in the current directory before searching PATH, so this could run a different tar from the one that getTarVersion checked. In practice, this only affects Windows runners with a tools input that points to a .tar.zst bundle, since the Action otherwise uses gzip bundles on Windows.
This PR records the path that getTarVersion resolves with io.which, which only searches PATH, and runs that path. The [command] log line and error messages now show this resolved path. On Linux and macOS, where zstd bundles are the default, this should otherwise run the same tar as before.
It also fixes stdout and stderr being passed to CommandInvocationError the wrong way round, so when tar exits with an error, the error message now includes the last line of tar's stderr rather than its stdout.
Risk assessment
For internal use only. Please select the risk level of this change:
Which use cases does this change impact?
Workflow types:
Products:
Environments:
How did/will you validate this change?
If something goes wrong after this change is released, what are the mitigation and rollback strategies?
How will you know if something goes wrong after this change is released?
Are there any special considerations for merging or releasing this change?
Merge / deployment checklist