*: download instrumented buildkitd

[adityamaru]

Important

Adds downloadBuildkitd() to download and set up buildkitd binary, updates startBuildkitd() to use it, and removes unused getDiskSize() function.

• Behavior:
  • Adds downloadBuildkitd() in setup_builder.ts to download buildkitd binary from a URL and make it executable.
  • Updates startBuildkitd() to use the downloaded buildkitd binary.
• Removals:
  • Removes unused getDiskSize() function from setup_builder.ts.

^{This description was created by for 0d308ab. It will automatically update as commits are pushed.}

[ellipsis-dev]

👍 Looks good to me! Reviewed everything up to a444843 in 1 minute and 23 seconds

More details

• Looked at 85 lines of code in 1 files
• Skipped 2 files when reviewing.
• Skipped posting 4 drafted comments based on config settings.

1. src/setup_builder.ts:36

• Draft comment:
  Consider handling errors in fs.unlink callback to avoid unhandled exceptions if the file does not exist or cannot be deleted.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable:
  The unlink is in an error handler path that's already rejecting the promise. If the unlink fails, we're already in an error state and the promise is being rejected anyway. Adding error handling here would just complicate the code without providing value - if we can't delete the file, it doesn't really matter since we're already failing. The empty callback is actually the right approach here.
  Maybe file cleanup is important for security or disk space reasons, and we should log unlink failures? Maybe the unlink error could provide useful debugging information?
  The unlink is just best-effort cleanup of a temporary failed download. A failed cleanup doesn't affect functionality and wouldn't provide meaningful debug info beyond the original download error that we're already handling.
  Delete this comment. The empty callback is appropriate here since we're already handling the main error and the unlink is just best-effort cleanup.

2. src/setup_builder.ts:28

• Draft comment:
  Ensure that any user input passed to execAsync is properly sanitized to prevent command injection vulnerabilities. This applies to all instances where execAsync is used with dynamic input.
• Reason this comment was not posted:
  Comment did not seem useful.

3. src/setup_builder.ts:133

• Draft comment:
  Ensure that any user input passed to execAsync is properly sanitized to prevent command injection vulnerabilities. This applies to all instances where execAsync is used with dynamic input.
• Reason this comment was not posted:
  Marked as duplicate.

4. src/setup_builder.ts:134

• Draft comment:
  Ensure that any user input passed to execAsync is properly sanitized to prevent command injection vulnerabilities. This applies to all instances where execAsync is used with dynamic input.
• Reason this comment was not posted:
  Marked as duplicate.

Workflow ID: wflow_vG8wAZkvytsSokIW

---

You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

[ellipsis-dev]

👍 Looks good to me! Incremental review on 1329ee3 in 43 seconds

More details

• Looked at 89 lines of code in 1 files
• Skipped 0 files when reviewing.
• Skipped posting 3 drafted comments based on config settings.

1. src/setup_builder.ts:17

• Draft comment:
  Consider adding error handling for the file stream to catch issues like disk space or write permissions during the download process.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable:
  The code already has error handling for the HTTPS request and file operations through the .on('error') handler and try/catch blocks. While adding a specific error handler for the file stream could catch additional edge cases, the existing error handling likely covers most failure scenarios. The comment is suggesting a minor improvement that may not provide significant value.
  I might be underestimating the importance of specific file stream error handling. File operations can fail in various ways that might not be caught by the current error handlers.
  While file stream errors are important, the current error handling is reasonably comprehensive. The file.on('error') handler would be a minor improvement that doesn't justify a PR comment.
  The comment should be removed as it suggests a minor improvement to already reasonable error handling, and doesn't meet the bar for "clearly a code change required".

2. src/setup_builder.ts:27

• Draft comment:
  Consider adding error handling for the file stream to catch issues like disk space or write permissions during the download process.
• Reason this comment was not posted:
  Marked as duplicate.

3. src/setup_builder.ts:38

• Draft comment:
  Consider adding error handling for the file stream to catch issues like disk space or write permissions during the download process.
• Reason this comment was not posted:
  Marked as duplicate.

Workflow ID: wflow_C3Ce0dOdPSRPRejr

---

You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

[ellipsis-dev]

👍 Looks good to me! Incremental review on 2a4ae36 in 47 seconds

More details

• Looked at 98 lines of code in 1 files
• Skipped 0 files when reviewing.
• Skipped posting 4 drafted comments based on config settings.

1. src/setup_builder.ts:18

• Draft comment:
  Using sudo chmod 666 on the buildkitd binary is a security risk. Consider using a more restrictive permission, such as 755, to prevent unauthorized modifications.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable:
  The comment has a valid security concern in principle, but misses important context. The 666 permission is temporary during download, and the file is immediately made executable afterward. The binary is downloaded from a trusted source (R2_BUCKET_URL) and the whole operation happens in a controlled environment. The brief window of 666 permissions is likely an intentional design choice to handle the download process.
  I could be underestimating the security risk of having world-writable permissions even briefly. There could be a race condition vulnerability.
  While there is a theoretical risk, this is happening in a controlled CI environment during setup, and the window of vulnerability is extremely brief. The practical risk is minimal.
  The comment should be deleted as it raises a theoretical security concern without understanding the temporary nature of the permissions and the controlled environment context.

2. src/setup_builder.ts:46

• Draft comment:
  Ensure the file stream is closed in case of an error during the https.get request to prevent resource leaks.
• Reason this comment was not posted:
  Comment did not seem useful.

3. src/setup_builder.ts:48

• Draft comment:
  Handle potential errors in the fs.unlink callback to avoid suppressing errors during file deletion.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable:
  The comment is about changed code and points out a real issue - ignoring errors in file deletion could mask problems. However, in this case, the unlink is happening in an error handler that's already logging the main error and rejecting the promise. The file deletion is just cleanup, and failing to delete wouldn't change the overall error handling flow.
  I may be underestimating the importance of proper error handling even in cleanup code. Failed cleanup could leave the system in an inconsistent state.
  While proper error handling is important, in this case the file deletion is best-effort cleanup during error handling. The main error is already being logged and propagated correctly.
  The comment should be deleted as it suggests handling errors for a best-effort cleanup operation where the main error path is already properly handled.

4. src/setup_builder.ts:140

• Draft comment:
  Consider adding logging within the retry loop to indicate progress or retries when checking for the buildkitd process.
• Reason this comment was not posted:
  Confidence changes required: 50%
  The startBuildkitd function has a retry loop for checking the buildkitd process, but it doesn't log any progress or retries, which could be useful for debugging.

Workflow ID: wflow_c1RxzXiaLx4AxGLB

---

You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

[ellipsis-dev]

❌ Changes requested. Incremental review on 0d308ab in 40 seconds

More details

• Looked at 102 lines of code in 1 files
• Skipped 0 files when reviewing.
• Skipped posting 2 drafted comments based on config settings.

1. src/setup_builder.ts:52

• Draft comment:
  Handle the callback of fs.unlink to ensure any errors during file deletion are managed.
• Reason this comment was not posted:
  Comment did not seem useful.

2. src/setup_builder.ts:41

• Draft comment:
  Ensure that inputs to execAsync with sudo are properly validated and sanitized to prevent security risks.
• Reason this comment was not posted:
  Comment did not seem useful.

Workflow ID: wflow_ZeNHegF90nEwFGS0

---

Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

[ellipsis-dev]

Using setTimeout to wait for the file to close is unreliable. Instead, use the close event to ensure the file is fully closed before proceeding.

[ellipsis-dev]

Consider using execAsync instead of exec for consistency and to avoid callback hell.