Skip to content

chore(spike): configure ts-binary-wrapper to validate linuxstatic binaries#6783

Open
j-luong wants to merge 1 commit into
mainfrom
spike/cli-1445_tsbinarywrapper_linuxstatic
Open

chore(spike): configure ts-binary-wrapper to validate linuxstatic binaries#6783
j-luong wants to merge 1 commit into
mainfrom
spike/cli-1445_tsbinarywrapper_linuxstatic

Conversation

@j-luong
Copy link
Copy Markdown
Contributor

@j-luong j-luong commented May 7, 2026

Pull Request Submission Checklist

  • Follows CONTRIBUTING guidelines
  • Commit messages
    are release-note ready, emphasizing
    what was changed, not how.
  • Includes detailed description of changes
  • Contains risk assessment (Low | Medium | High)
  • Highlights breaking API changes (if applicable)
  • Links to automated tests covering new functionality
  • Includes manual testing instructions (if necessary)
  • Updates relevant GitBook documentation (PR link: ___)
  • Includes product update to be announced in the next stable release notes

What does this PR do?

Where should the reviewer start?

How should this be manually tested?

What's the product update that needs to be communicated to CLI users?

@snyk-io
Copy link
Copy Markdown

snyk-io Bot commented May 7, 2026

Snyk checks have passed. No issues have been found so far.

Status Scan Engine Critical High Medium Low Total (0)
Open Source Security 0 0 0 0 0 issues
Licenses 0 0 0 0 0 issues
Code Security 0 0 0 0 0 issues

💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 7, 2026

Warnings
⚠️

Since the CLI is unifying on a standard and improved tooling, we're starting to migrate old-style imports and exports to ES6 ones.
A file you've modified is using either module.exports or require(). If you can, please update them to ES6 import syntax and export syntax.
Files found:

  • ts-binary-wrapper/src/common.ts
⚠️

"chore(spike): configure ts-binary-wrapper to validate linuxstatic binaries" is too long. Keep the first line of your commit message under 72 characters.

Generated by 🚫 dangerJS against 19dc4be

@j-luong j-luong force-pushed the spike/cli-1445_tsbinarywrapper_linuxstatic branch from 07f616c to 51d8979 Compare May 7, 2026 13:34
@j-luong j-luong changed the title spike: configure ts-binary-wrapper to validate linuxstatic binaries chore(spike): configure ts-binary-wrapper to validate linuxstatic binaries May 7, 2026
@j-luong j-luong force-pushed the spike/cli-1445_tsbinarywrapper_linuxstatic branch from 51d8979 to 5a46a9e Compare May 11, 2026 12:32
@j-luong j-luong force-pushed the spike/cli-1445_tsbinarywrapper_linuxstatic branch from 5a46a9e to 19dc4be Compare May 11, 2026 12:33
@j-luong j-luong marked this pull request as ready for review May 11, 2026 13:21
@j-luong j-luong requested review from a team as code owners May 11, 2026 13:21
@snyk-pr-review-bot
Copy link
Copy Markdown

PR Reviewer Guide 🔍

🧪 PR contains tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Broken Build Dependency 🟠 [major]

The build-binary-wrapper target now explicitly depends on experimental linux binaries (experimental/snyk-linux and experimental/snyk-linux-arm64). However, there are no rules in the Makefile to produce these files, and the local build script scripts/build-ts-binary-wrapper-locally.go only creates the .sha256 metadata files, not the actual binaries. This will cause make build-binary-wrapper to fail with a 'No rule to make target' error unless those files are manually placed in the output folder.

build-binary-wrapper: pre-build-binary-wrapper $(BINARY_WRAPPER_DIR)/src/generated/version $(BINARY_WRAPPER_DIR)/src/generated/sha256sums.txt $(BINARY_RELEASES_FOLDER_TS_CLI)/experimental/snyk-linux $(BINARY_RELEASES_FOLDER_TS_CLI)/experimental/snyk-linux-arm64
Logic Duplication 🟡 [minor]

In getCurrentConfiguration, the logic for mapping x64 to amd64 is manually implemented. This duplicates existing logic found in determineBinaryName (line 92 in context). Future changes to architecture support (e.g., adding armv7) would require updating both locations, increasing the risk of synchronization bugs.

os.arch() === 'x64' || os.arch() === 'amd64' ? 'amd64' : os.arch();
Misleading API Name 🟡 [minor]

The method getShasumFile() was changed to return an array of strings (string[]) representing the expected hashes, rather than a file path. The name of the method remains getShasumFile, which is now semantically incorrect and conflicts with the module-level constant shasumFile. It should be renamed to something like getExpectedShasums().

public getShasumFile(): string[] {
  return this.expectedSha256sums;
}
📚 Repository Context Analyzed

This review considered 19 relevant code sections from 13 files (average relevance: 0.85)

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant