Repository navigation
feat(acp): a managed runtime installs only when asked, with progress and cancel - #29023
Conversation
cf54393 to
0f0c56e
Compare
e50c315 to
b63c823
Compare
0f0c56e to
c5eb8b2
Compare
b63c823 to
fe9ee28
Compare
c5eb8b2 to
244b415
Compare
0e53d3b to
c40df19
Compare
fe9ee28 to
2bf27e3
Compare
c40df19 to
d2292f6
Compare
2bf27e3 to
ad5633d
Compare
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to
|
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Repository UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
09f80d20-f016-49a3-a804-a809eec180e4
📒 Files selected for processing: 16
src/main/acp/acp-launch-specs.tssrc/main/acp/acp-private-temp.tssrc/main/acp/acp-structured-acquire.tssrc/main/acp/acp-structured-adapter.test-support.tssrc/main/acp/acp-structured-private-temp.test.tssrc/main/acp/acp-structured-session-adapter-deps.tssrc/main/node-runtime/managed-runtime-installation.test.tssrc/main/node-runtime/managed-runtime-installation.tssrc/main/node-runtime/managed-runtime-release.test.tssrc/main/node-runtime/managed-runtime-release.tssrc/main/node-runtime/pinned-runtime-materializer.test.tssrc/main/node-runtime/pinned-runtime-materializer.tssrc/main/node-runtime/runtime-archive-download.tssrc/main/runtime/revisioned-state.tssrc/main/runtime/structured-agent-runtime-registrations.tssrc/shared/managed-runtime-install.ts
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 4 remain after this review.
d2292f6 to
0d08ee8
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to GitHub limitations.
🟡 Minor · Require a valid receipt before committing an existing… · managed-runtime-installation.ts:167-181
src/main/node-runtime/managed-runtime-installation.ts:167-181
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winRequire a valid receipt before committing an existing release.
verifyMemberschecks only the pinned files and permissions. A missing or invalid.install-complete.jsontherefore reachesrun.committed = truewithout repair. The resolver classifies that folder as broken, so status does not report it as current and ACP launch resolution does not select it. Check the receipt before returning; otherwise continue through the repair path.Suggested fix
const whole = await verifyMembers(folder, pin, label, signal).then( - () => true, + () => resolveInstalledRuntimeRelease(this.root, pin).release?.current ?? false, (error: unknown) => {
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Repository UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
9ed8685d-d658-4bcf-96e4-0f72d92850ce
📒 Files selected for processing: 1
src/main/node-runtime/managed-runtime-release.ts
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 2 remain after this review.
…and cancel One installation per runtime owns every change to its releases: it reclaims old ones once, first, then installs one at a time. An install checks free space, downloads with byte progress, extracts, hashes each member (abortable), writes the receipt, then publishes by rename, the commit point: a cancel after it reports success. Reinstall rehashes the pinned release without downloading. A status read waits, bounded, for a newer revision and rereads the installed release from disk.
…ing before downloading, and reads a failed download plainly - Install over a damaged pinned copy downloads and verifies a fresh one, then swaps the damaged folder into staging, so there is no dead end; status reports the damaged fact. - A host that cannot unpack the archive (no unzip) reports it in status and refuses Install without downloading. - A network failure while downloading reads "Couldn't download …"; the raw error goes to the log. - Reclamation skips the whole pass when a release folder could not be read.
… instead of saying not installed, and extraction uses the extractor the preflight found
…, and says so in the log
0d08ee8 to
739531b
Compare
…ead of blaming the network
ad5e7b4 to
9509cb5
Compare
…fore publishing, and Cancel answers with the outcome - The managed install unzips in-process (fflate, bundled into main), so a host without unzip can install; the unzip preflight and the missingProgram status field are gone. The Node runtime download keeps its own extractor. - A runtime may provide validate(): Install starts the downloaded copy once before publishing it, and an existing copy before reusing it; a failure reads "<agent> couldn't start on this host". - No 'cancelling' phase: Cancel resolves once the work has unwound and returns cancelled, or succeeded when it had already committed. - Byte progress every 250 ms; status rereads disk and PATH only on a first read or a phase change. - The declared download size is checked only for an uncompressed body. - An existing copy whose files hash correctly but whose receipt is missing gets its receipt rewritten instead of reading as damaged forever.
… the installer The installer class now holds only its lifecycle: the shared operation state, the one-at-a-time queue, install, cancel, status, reclaim and progress publishing. Reusing a whole copy, the free space check, the download, unpacking, checking each file, the start check, the receipt, the publishing rename and the failure sentences move unchanged into managed-runtime-release-install.ts, which gets the pin, folder, label, signal and two callbacks (progress, and the commit point right after the rename) instead of the class. No behavior change; this keeps the installer well under the file-length limit once Remove lands in it.
…mismatch, so a refused proxy reads as a network problem
…the space and nothing is stored twice
ELI5
Antigravity's chat needs a separate program, its chat server, which Orca downloads for it. When chats fetched that program on their own, the first chat sat on "Working for Ns" for about 18 seconds with no progress and no way to cancel, and opening several chats started several downloads. This stack changes that: nothing downloads until the person clicks Install, with progress and Cancel in Settings and in the chat.
#28460 stopped chat starts from downloading. This PR adds the installer the Install button will drive: one per agent per host, it downloads, checks and puts the program in place only when asked, reports progress, and can be cancelled. Nothing calls it yet (#29024 lets clients reach it), so nothing changes for anyone when this merges alone.
What Changed
This revision (moving the installer to the common pattern; each item is problem, then what changes):
The download was stored twice. The app's network stack also kept the 111 MB archive in its HTTP cache for up to a day, so Remove didn't free that space and a reinstall could finish with no network. Runtime downloads now skip the HTTP cache (
cache: 'no-store'), so the verified copy is the only one.A refused proxy was blamed on the release. QA ran Install behind a proxy that refused connections, and the row said "The Antigravity download didn't match the release Orca expects…". The wording picked that sentence for any error the retry logic doesn't treat as temporary. Now only a proven mismatch (a missing file, an oversized download or a different checksum) gives that sentence; every other failure says "Couldn't download Antigravity. Check your network connection or proxy, then try again.", as the common pattern keeps its mismatch wording to the size and checksum check.
A Linux host without
unzipcould not install at all. Minimal servers and containers often lack it, and the row could only say "needs unzip". Install now unpacks the zip itself, in-process, withfflate(a small zip library with no dependencies of its own, now bundled into the app's main process). Only the pinned files are written, each at most its pinned size and exactly that size at the end; any other entry fails the install. Theunzipcheck before downloading and themissingProgramstatus field are gone. The Node runtime download keeps its existing extractor."Installed" could show for a copy this host can't run (an older system library, a data folder that forbids running programs, the wrong processor type). The file checksums prove the bytes, not that the host can run them. An agent can now provide a start check: Install starts the unpacked server once and publishes it only if it answers as the pinned release. An existing copy is checked the same way before Install reuses it. A failure reads "{agent} couldn't start on this host, so it wasn't installed. This host may not support it." and nothing is published. Antigravity's check comes in feat(acp): Antigravity in the Chat UI over its ACP server #28595.
Cancel published a "cancelling" state that every client flashed for about 50 ms. Cancel now answers with the outcome once the work has unwound:
cancelled, orsucceededwhen the release was already in place. The host publishes no "cancelling" phase; the client that clicked shows "Cancelling…" itself while its call is out (feat(acp): one install row shows and drives a managed runtime install, for Settings and chat #29049).A compressed download could be refused for its size. A compressed body declares its compressed size, which can exceed the pinned size. The declared size is now compared only for an uncompressed body; the unpacked bytes are still capped at the pinned size and hashed.
A copy whose files were whole but whose completion receipt was missing stayed "damaged" forever. Install hashed the files, reported success and changed nothing. It now rewrites the receipt.
Byte progress is published every 250 ms instead of once a second. Disk and PATH are re-read on a first status read and when the operation's phase changes, not for each progress update.
The steps that put one release on disk (reuse a whole copy, free space, download, unpack, check, start check, receipt, rename) moved unchanged into their own module,
managed-runtime-release-install.ts, so the installer class keeps only its lifecycle and stays under the file-length limit.Before: with chat starts no longer downloading (feat(acp): chat agents can declare a managed runtime that a start runs but never installs #28460), there was no way to get a program Orca provides onto a host.
After:
ManagedRuntimeInstallation(one per managed agent per host) owns every change to that agent's release folder:damaged: true; Install downloads and checks a fresh copy, moves the damaged folder into the download's temporary folder (deleted afterwards), then puts the new copy in place.source: 'external').RevisionedState: a small shared value-plus-revision primitive. It holds no lifecycle policy, so the chat sign-in flow can adopt it with its own replace-on-start rule.Download and hashing gain what the installer needs: an optional per-archive size limit (set to the pinned size), a byte-progress callback, the compressed-body size rule above, and abortable hashing.
The shared state type already names a
removeoperation and aremovingphase, used by feat(runtime): remove an agent's downloaded runtime only once nothing on the host can still run it #29139 (removes the downloaded program).New dependency:
fflate0.8.3, used only by the managed install.Merged alone: nothing calls the installer, so nothing changes for anyone.
Why
Making install a separate, person-started operation removes the silent download, the duplicate downloads and the missing progress and cancel together, because there is now exactly one owner of the release folder. Running old-version cleanup inside that same owner, instead of at chat setup, is what makes it unable to delete a version an Update is publishing. Unpacking in-process and starting the server once before publishing follow the common pattern: both move a failure the person would otherwise meet at the first chat to the Install click, where it can be reported plainly.
Linked Issue
N/A (maintainer; part of the stack that adds Antigravity to chat).
Visual Proof
N/A: no caller yet; the calls come in #29024 and the button after that.
Testing
I manually tested these changes locally
Automated tests added/updated, or explained why not below
Whole flow on a fake agent with a fake zip download: progress appears, a second Install joins, Cancel answers
cancelledonce the run has unwound and leaves no temporary folder, Retry installs, and a fresh installer reads "installed, current, Orca's copy" from disk alone. Deleting the receipt then readsdamaged, and Install rewrites it without downloading.Cleanup vs Update: with cleanup paused, an Install does not start downloading; once cleanup runs, the older kept version and the new version both remain and the oldest is gone.
Repair: a truncated file reads as
damaged: true, and Install puts back a whole copy.Checked in QA before this revision: a fast connection finishes a 111 MB download in a few seconds. This revision's changes (in-process unpack, start check, Cancel outcome) have not been exercised in the app yet.
Lint and typecheck: PR CI.
Review
Agent skill upstream boundary
QA, round 2 (M4Air, 10-16, build of
2efa3308959before this revision's last fixes; Grok drove the hidden window over the Chrome DevTools Protocol only)QA, round 2b (M4Air, 10-16, build of
d871611d303, the current stack top; Grok over the Chrome DevTools Protocol only)Notes
Differences from the common pattern
Checklist
N/Awith reasonpnpm lint,pnpm typecheck,pnpm test, andpnpm buildpass (or CI will cover; local preferred)