Repository navigation
Run tar from its resolved path when extracting zstd bundles - #4281
Conversation
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
What changed in this PR
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)
|
|
||
| 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.
mbg
left a comment
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.
| 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?
There was a problem hiding this comment.
Afaict, we can't pipe in a HTTP stream with ToolRunner.
extractTarZstrantarby name. On Windows,child_process.spawnby default looks for a bare command name in the current directory before searchingPATH, so this could run a differenttarfrom the one thatgetTarVersionchecked. In practice, this only affects Windows runners with atoolsinput that points to a.tar.zstbundle, since the Action otherwise uses gzip bundles on Windows.This PR records the path that
getTarVersionresolves withio.which, which only searchesPATH, 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 sametaras before.It also fixes stdout and stderr being passed to
CommandInvocationErrorthe 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:
dynamicworkflows (Default Setup, Code Quality, ...).Products:
analysis-kinds: code-scanning.analysis-kinds: code-quality.upload-sarifaction.Environments:
github.comand/or GitHub Enterprise Cloud with Data Residency.How did/will you validate this change?
pr-checks).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