[Refactor] Extract managed-binary installation infrastructure - #1059
[Refactor] Extract managed-binary installation infrastructure#1059navedmerchant wants to merge 2 commits into
Conversation
📝 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 issues
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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
left a comment
edelauna
left a comment
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
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
| it("kills a process that exceeds its timeout", async () => { | ||
| vi.useFakeTimers() | ||
| const child = createChild() | ||
| mockSpawn.mockReturnValue(child as unknown as ReturnType<typeof spawn>) | ||
| const processResult = runProcess("tool", [], 100) | ||
| const assertion = expect(processResult).rejects.toThrow("tool timed out") | ||
|
|
||
| await vi.advanceTimersByTimeAsync(100) | ||
| await assertion | ||
| expect(child.kill).toHaveBeenCalledWith("SIGKILL") | ||
| vi.useRealTimers() |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check for an existing global fake-timer cleanup hook in this spec file.
rg -n "afterEach|useRealTimers|useFakeTimers" src/services/managed-binary/__tests__/archive.spec.tsRepository: Zoo-Code-Org/Zoo-Code
Length of output: 207
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "File size:"
wc -l src/services/managed-binary/__tests__/archive.spec.ts
echo
echo "Top 70 lines:"
sed -n '1,70p' src/services/managed-binary/__tests__/archive.spec.ts
echo
echo "All hook/timer references:"
rg -n "beforeEach|beforeAll|afterEach|afterAll|useRealTimers|useFakeTimers" src/services/managed-binary/__tests__/archive.spec.ts || true
echo
echo "Diff stat/name-status context:"
git diff --stat || true
git diff -- src/services/managed-binary/__tests__/archive.spec.ts | sed -n '1,220p' || trueRepository: Zoo-Code-Org/Zoo-Code
Length of output: 2639
Restore timers in a try/finally block.
vi.useRealTimers() is not part of a teardown hook, so an assertion failure here leaves fake timers active for later tests.
🔧 Proposed fix
it("kills a process that exceeds its timeout", async () => {
vi.useFakeTimers()
const child = createChild()
mockSpawn.mockReturnValue(child as unknown as ReturnType<typeof spawn>)
const processResult = runProcess("tool", [], 100)
const assertion = expect(processResult).rejects.toThrow("tool timed out")
- await vi.advanceTimersByTimeAsync(100)
- await assertion
- expect(child.kill).toHaveBeenCalledWith("SIGKILL")
- vi.useRealTimers()
+ try {
+ await vi.advanceTimersByTimeAsync(100)
+ await assertion
+ expect(child.kill).toHaveBeenCalledWith("SIGKILL")
+ } finally {
+ vi.useRealTimers()
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| it("kills a process that exceeds its timeout", async () => { | |
| vi.useFakeTimers() | |
| const child = createChild() | |
| mockSpawn.mockReturnValue(child as unknown as ReturnType<typeof spawn>) | |
| const processResult = runProcess("tool", [], 100) | |
| const assertion = expect(processResult).rejects.toThrow("tool timed out") | |
| await vi.advanceTimersByTimeAsync(100) | |
| await assertion | |
| expect(child.kill).toHaveBeenCalledWith("SIGKILL") | |
| vi.useRealTimers() | |
| it("kills a process that exceeds its timeout", async () => { | |
| vi.useFakeTimers() | |
| const child = createChild() | |
| mockSpawn.mockReturnValue(child as unknown as ReturnType<typeof spawn>) | |
| const processResult = runProcess("tool", [], 100) | |
| const assertion = expect(processResult).rejects.toThrow("tool timed out") | |
| try { | |
| await vi.advanceTimersByTimeAsync(100) | |
| await assertion | |
| expect(child.kill).toHaveBeenCalledWith("SIGKILL") | |
| } finally { | |
| vi.useRealTimers() | |
| } |
🤖 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/__tests__/archive.spec.ts` around lines 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.
| .map((entry) => entry.trim()) | ||
| .filter(Boolean) | ||
| const archiveEntry = entries[0] | ||
| if (entries.length !== 1 || archiveEntry.replace(/^\.\//, "") !== expectedFile) { | ||
| throw new Error(`${archiveName} archive has an unexpected layout`) | ||
| } | ||
|
|
||
| await runProcess("tar", ["-xJf", archivePath, "-C", destination, archiveEntry]) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Reject non-regular TAR entries before extraction.
Line 106 lists only member names. A one-entry archive with a symbolic link or hard link named expectedFile passes this check. Line 116 then extracts the link into staging.
Inspect the TAR entry type and accept only one regular file. Apply the ownership and directory-overwrite protections used by extractTarGzArchive to this extraction path.
🧰 Tools
🪛 ast-grep (0.45.0)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { spawn } from "child_process"
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
🤖 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 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.
edelauna
left a comment
edelauna
left a comment
There was a problem hiding this comment.
Had some questions about the implementation.
| } | ||
| }) | ||
| response.on("error", reject) | ||
| response.pipe(output) |
There was a problem hiding this comment.
Would it be safer to also destroy the streams here? If the response emits an error, output and request stay open — compare the size-limit handler at lines 133–138, which calls output.destroy(), response.destroy(), and request.destroy() before rejecting. Should this handler follow the same pattern to avoid socket leaks?
| resolve() | ||
| }) | ||
| output.on("error", reject) | ||
| }) |
There was a problem hiding this comment.
Same question: if output errors after piping has started, should we also destroy response and request to release the underlying socket and in-flight HTTP connection?
| input.emit("data", Buffer.from("archive")) | ||
| input.emit("end") | ||
| await expect(verification).rejects.toThrow("checksum mismatch: actual-checksum") | ||
| }) |
There was a problem hiding this comment.
Is there a test for the happy path here — where the computed digest actually equals expected and verifyFileChecksum resolves without throwing? The suite currently only exercises the mismatch branch.
| mockSpawn.mockReturnValueOnce(extraction as unknown as ReturnType<typeof spawn>) | ||
| const result = extractSingleFileTarXzArchive("/tmp/archive.tar.xz", "/tmp/output", "binary", "Tool") | ||
| listing.stdout.write("./binary\n") | ||
| listing.emit("close", 0) |
There was a problem hiding this comment.
This feeds the happy path (single ./binary entry). Are there tests for the error branches — e.g. writing './binary\n./other\n' to listing.stdout (multi-entry) or a filename that doesn't match info.binary (wrong-filename error at archive.ts:112)?
| update: 30_000, | ||
| retries: { retries: 10, factor: 1.5, minTimeout: 100, maxTimeout: 1_000 }, | ||
| }) | ||
| try { |
There was a problem hiding this comment.
Is there a reason not to pass an onCompromised callback here? safeWriteJson.ts uses one so that stale-lock detection failures surface loudly rather than silently continuing — should the same policy apply to the binary install lock?
| } | ||
|
|
||
| export async function extractSingleFileZipArchive( | ||
| archivePath: string, |
There was a problem hiding this comment.
Should this have a process.platform === 'win32' guard, or at least a JSDoc note that it only works on Windows? PowerShell is invoked unconditionally, so calling this function on Linux/macOS will produce a confusing error.
| await vi.waitFor(() => expect(download).toHaveBeenCalledOnce()) | ||
| finishDownload?.() | ||
| await Promise.all([first, second]) | ||
| expect(download).toHaveBeenCalledOnce() |
There was a problem hiding this comment.
Are the resolved values from both calls asserted? A mutation that returns the wrong path would still pass if only the settlement of Promise.all is tested, not the actual paths resolved by first and second.
| name: "Semble", | ||
| trustedDomains: TRUSTED_DOWNLOAD_DOMAINS, | ||
| timeoutMs: 120_000, | ||
| }), |
There was a problem hiding this comment.
The PR description highlights bounded downloads as a key property of downloadBinaryFile — does omitting maxBytes here leave the Semble download unbounded? Should info.size (or a conservative upper limit) be threaded through?
| if (info.archive.endsWith(".tar.gz")) { | ||
| await extractTarGzArchive(archivePath, stagingDir) | ||
| } else if (info.archive.endsWith(".zip")) { | ||
| await extractZipArchive(archivePath, stagingDir) |
There was a problem hiding this comment.
If info.archive is neither .tar.gz nor .zip, this falls through silently — the archive is downloaded but never extracted and binaryPath won't exist. Should an unrecognised format throw instead of no-op?
2 participants
Related GitHub Issue
Closes #1055
Part of #1049. Split from #1050.
Description
Extracts the binary lifecycle code previously embedded in the Semble downloader into reusable managed-binary services, then migrates Semble to those services without changing its behavior.
This PR is intentionally independent of DCG. It provides:
Stack
1 / 4 — base:
mainMerge this PR first. The DCG service PR is stacked on top of this branch.
Test Procedure
pnpm --dir src exec vitest run services/managed-binary/__tests__/archive.spec.ts services/managed-binary/__tests__/download.spec.ts services/managed-binary/__tests__/install.spec.ts services/code-index/semble/__tests__/semble-downloader.spec.ts pnpm --dir src check-typesResult: 4 test files passed, 46 tests passed; type-check passed. Repository pre-push type-check also passed.
Checklist
Summary by CodeRabbit
New Features
Bug Fixes