[Improve] Strengthen managed binary installation - #1059
Open
navedmerchant wants to merge 6 commits into
Open
Conversation
navedmerchant
requested review from
JamesRobert20,
edelauna,
hannesrudolph and
taltas
as code owners
July 30, 2026 04:21
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughShared managed-binary utilities now handle secure downloads, checksum verification, archive extraction, versioned atomic installation, cleanup, and concurrent installation control. The Semble downloader delegates to these utilities, and related tests and ESLint configuration were updated. ChangesManaged binary infrastructure
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Semble as downloadSemble
participant Installer as ensureManagedBinaryInstalled
participant Downloader as downloadBinaryFile
participant Checksum as verifySha256Checksum
participant Extractor as extractTarGzArchive
Semble->>Installer: provide version and lifecycle callbacks
Installer->>Downloader: download trusted archive
Installer->>Checksum: verify archive checksum
Installer->>Extractor: extract into staging directory
Installer-->>Semble: return installed binary path
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
9 tasks
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
src/services/managed-binary/archive.ts (1)
55-87: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAvoid constructing PowerShell scripts with string interpolation.
These extraction paths still interpolate user-controlled paths and archive names into PowerShell commands before shell escaping. Use PowerShell parameterized execution instead: pass values via
-ArgumentListwith a-Filescript, or build the command as script text and run it via-EncodedCommandso the values are not quoted into the command literal.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/services/managed-binary/archive.ts` around lines 55 - 87, Update extractZipArchive and extractSingleFileZipArchive to stop interpolating archive paths, destinations, expectedFile, or archiveName into PowerShell script text. Pass these values as PowerShell parameters via -ArgumentList with a -File script or an equivalent EncodedCommand approach, while preserving the existing extraction and archive-layout validation behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/services/code-index/semble/__tests__/semble-downloader.spec.ts`:
- Around line 707-723: Replace the stale fs.unlink assertion in the upgrade
cleanup test with an assertion against fs.rm, verifying the unrelated file path
is not removed while preserving the existing force-option expectations for
intended archives.
In `@src/services/managed-binary/__tests__/archive.spec.ts`:
- Around line 61-79: Update the test “validates a single-file tar.xz layout
before extraction” to assert that extraction passes the exact archive member
name returned by the listing, including the “./” prefix. Adjust the mocked
listing or expected tar arguments only as needed to verify the implementation
preserves the stored member name rather than stripping it.
In `@src/services/managed-binary/archive.ts`:
- Around line 89-105: Preserve the raw tar member name in
extractSingleFileTarXzArchive for the extraction command, while normalizing a
separate value by removing ./ for the single-entry layout comparison. Update the
archive.spec.ts test expectation so a ./binary listing results in extraction
targeting ./binary; apply the change in both specified files.
In `@src/services/managed-binary/download.ts`:
- Around line 120-144: Update the maxBytes error path in the response data
handler to explicitly destroy or close the output stream before rejecting,
alongside response.destroy() and request.destroy(). Preserve the existing
successful output.on("finish") flow and ensure the stream’s file descriptor is
released when assertSizeWithinLimit throws.
In `@src/services/managed-binary/install.ts`:
- Line 4: Add cross-process locking to ensureManagedBinaryInstalled in addition
to the in-memory installationPromises map, using an exclusive lock file under
storageDir to serialize attempts targeting the deterministic staging path.
Ensure lock acquisition, release, and stale/error cleanup are handled safely,
and keep each owner’s archive and staging writes isolated until successful
promotion.
---
Nitpick comments:
In `@src/services/managed-binary/archive.ts`:
- Around line 55-87: Update extractZipArchive and extractSingleFileZipArchive to
stop interpolating archive paths, destinations, expectedFile, or archiveName
into PowerShell script text. Pass these values as PowerShell parameters via
-ArgumentList with a -File script or an equivalent EncodedCommand approach,
while preserving the existing extraction and archive-layout validation behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 1dfa2c7c-2e9f-416c-ba33-4eec94aae3ba
📒 Files selected for processing (9)
src/eslint-suppressions.jsonsrc/services/code-index/semble/__tests__/semble-downloader.spec.tssrc/services/code-index/semble/semble-downloader.tssrc/services/managed-binary/__tests__/archive.spec.tssrc/services/managed-binary/__tests__/download.spec.tssrc/services/managed-binary/__tests__/install.spec.tssrc/services/managed-binary/archive.tssrc/services/managed-binary/download.tssrc/services/managed-binary/install.ts
💤 Files with no reviewable changes (1)
- src/eslint-suppressions.json
github-actions
Bot
added
the
awaiting-review
edelauna
requested changes
Jul 31, 2026
edelauna
left a comment
edelauna
left a comment
Contributor
There was a problem hiding this comment.
Nice! Had a couple implementation comments.
github-actions
Bot
added
awaiting-author
github-actions
Bot
removed
the
awaiting-author
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/services/managed-binary/__tests__/archive.spec.ts`:
- Around line 45-55: Wrap the fake-timer setup and test assertions in a
try/finally block within the “kills a process that exceeds its timeout” test,
and call vi.useRealTimers() in the finally block so timers are restored even
when an assertion fails.
In `@src/services/managed-binary/archive.ts`:
- Around line 109-116: Update the archive extraction flow around the TAR entry
validation and runProcess call to inspect entry types, accepting exactly one
regular file named expectedFile and rejecting symbolic links, hard links,
directories, and other non-regular entries. Apply the same ownership and
directory-overwrite protections already implemented by extractTarGzArchive
before extracting.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: dfecb8ff-1b98-44ef-af6f-4a38bc8f3e09
📒 Files selected for processing (8)
src/services/code-index/semble/__tests__/semble-downloader.spec.tssrc/services/code-index/semble/semble-downloader.tssrc/services/managed-binary/__tests__/archive.spec.tssrc/services/managed-binary/__tests__/download.spec.tssrc/services/managed-binary/__tests__/install.spec.tssrc/services/managed-binary/archive.tssrc/services/managed-binary/download.tssrc/services/managed-binary/install.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/services/code-index/semble/tests/semble-downloader.spec.ts
github-actions
Bot
added
the
awaiting-author
edelauna
requested changes
Aug 2, 2026
edelauna
left a comment
edelauna
left a comment
Contributor
There was a problem hiding this comment.
Had some questions about the implementation.
github-actions
Bot
removed
the
awaiting-author
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/services/managed-binary/archive.ts (2)
1-33: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winBound captured process output.
runProcessstores allstdoutandstderrin memory.extractSingleFileTarXzArchiveuses it to collect the completetar -tvJflisting before checking the entry count. A highly compressed archive with many entries can exhaust memory before validation completes.Add a byte limit and terminate the child when the limit is exceeded, or stream only enough output to detect more than one entry.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/services/managed-binary/archive.ts` around lines 1 - 33, Update runProcess to bound captured stdout and stderr while preserving its ProcessResult contract: track accumulated output bytes, terminate the child and reject when the configured limit is exceeded, and clear the timeout during cleanup. Ensure extractSingleFileTarXzArchive’s tar listing cannot accumulate unbounded output before entry-count validation.
35-49: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winEnforce one common archive-entry policy before generic extraction.
extractTarGzArchiveandextractTarXzArchivepass TAR entries directly totar, whileextractZipArchiveusesunzip/Expand-Archive. The current flags only affect ownership or directory replacement behavior; they do not validate member paths/types or prevent unsafe symlinks or traversal entries. Use a single safe-entry policy that validates every archive member before extraction for both generic extractors.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/services/managed-binary/archive.ts` around lines 35 - 49, Update extractTarGzArchive and extractTarXzArchive to apply the same safe-entry validation policy used by extractZipArchive before generic extraction. Validate every member’s path and type, rejecting traversal entries and unsafe symlinks, then extract only validated entries while preserving the existing TAR options and platform-specific flags.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/services/managed-binary/archive.ts`:
- Around line 112-123: In the archive extraction logic around archiveEntry and
entryName, separate the entries.length validation from parsing and validate
archiveEntry and entryName before calling startsWith, replace, or appending
entryName to args. Ensure the checks narrow both values to strings before the
existing layout error and runProcess("tar", args) flow.
In `@src/services/managed-binary/download.ts`:
- Around line 147-150: Update the download completion handling around the output
stream’s finish/close listeners so the promise resolves only from the close
event after output.close() completes; adjust the success test to emit close
after finish and verify completion remains gated on close.
---
Outside diff comments:
In `@src/services/managed-binary/archive.ts`:
- Around line 1-33: Update runProcess to bound captured stdout and stderr while
preserving its ProcessResult contract: track accumulated output bytes, terminate
the child and reject when the configured limit is exceeded, and clear the
timeout during cleanup. Ensure extractSingleFileTarXzArchive’s tar listing
cannot accumulate unbounded output before entry-count validation.
- Around line 35-49: Update extractTarGzArchive and extractTarXzArchive to apply
the same safe-entry validation policy used by extractZipArchive before generic
extraction. Validate every member’s path and type, rejecting traversal entries
and unsafe symlinks, then extract only validated entries while preserving the
existing TAR options and platform-specific flags.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: fba919fe-d390-4c80-8ef6-3b8d9ec50684
📒 Files selected for processing (7)
src/services/code-index/semble/semble-downloader.tssrc/services/managed-binary/__tests__/archive.spec.tssrc/services/managed-binary/__tests__/download.spec.tssrc/services/managed-binary/__tests__/install.spec.tssrc/services/managed-binary/archive.tssrc/services/managed-binary/download.tssrc/services/managed-binary/install.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/services/managed-binary/tests/download.spec.ts
- src/services/managed-binary/tests/archive.spec.ts
- src/services/managed-binary/install.ts
zoomote
Bot
changed the title
This was referenced Aug 2, 2026
Contributor
github-actions
Bot
added
the
awaiting-author
edelauna
approved these changes
Aug 3, 2026
github-actions
Bot
added
awaiting-review
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changed
Adds tighter managed-binary safeguards: bounded Semble downloads, stricter archive layout validation, stream cleanup on download failures, and regression coverage for successful verification and installation cleanup.
Why this change was made
The shared installer underpins the DCG rollout and must reject malformed archives or unbounded downloads without leaving partial files or open connections. Closes #1055.
Impact
Binary-backed services install more safely and consistently without changing Semble's user-facing workflow.
Related PRs