fix(install): register the brain at cto and share one setup UI across platforms - #1040
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
|
Warning Review limit reached
Next review available in: 25 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (30)
📝 WalkthroughWalkthroughChangesShared setup flow
SQLite warning filtering
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested labels: 🚥 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 |
arul28
changed the title
arul28
force-pushed
the
ade/ade-windows-connect-setup
branch
from
August 5, 2026 19:26
4362dd8 to
7b7fe7e
Compare
`ade connect` failed on every clean install, on Windows and macOS alike. Both installers registered the machine brain with `ade serve --install-service`, which inherits `ADE_DEFAULT_ROLE`; on a fresh machine that is unset, so the brain came up at role `agent`. `ade connect` runs at `cto`, and an `agent` brain can never serve a `cto` caller. Every other call site already knew this: the desktop app spawns its runtime at `cto` and refuses to attach to a service that is not (localRuntimeConnectionPool.ts:330,2121), and `ade brain start` pins `cto` internally (cli.ts:15816). Both installers now register through `brain start`, including the PowerShell rollback path that was restoring the previous service at `agent` too. The installer was also silent and dishonest: ~30s with no output, raw node:sqlite ExperimentalWarnings as the only proof of life, no progress on a 118 MB runtime or a 1 GB app download, and a cheerful next step printed after sign-in had already failed. The shell scripts now own only what must happen before the `ade` binary exists. Everything after is `ade setup`: one TypeScript implementation both platforms hand off to, so the drift that left macOS with a download progress bar and Windows without one cannot recur. It runs the agent CLIs, account, and desktop app; verifies the install end to end; and prints a summary where a failed step names the command that fixes it. - account: confirms an existing link (keep/switch/skip) instead of re-prompting blind - desktop: skips the ~1 GB download when that version is installed, resumes a partial download via Range, verifies base64 SHA-512 - desktop launch is detached, so a Windows GUI child no longer inherits the console and sprays Electron logs over the user's prompt - node:sqlite ExperimentalWarning filtered at CLI entry; every other warning class still prints - rendering degrades to plain appended lines on legacy conhost, pipes, and CI Reuses the existing step/summary model from commands/connect.ts, byte progress from commands/tools.ts, readInstalledDesktopVersion from commands/doctor.ts, and releaseAssetUrl from lib/releaseAssets.ts. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The verification step added in this branch checked "brain running + account signed in" and called that ready. A clean Windows install reproduced exactly that state while never reaching the account directory, so the installer would have printed "ADE is ready" over a machine that is absent from the user's account. Sign-in is not the outcome; publication is. Verification now reads the brain's own account-directory publisher health and fails when the machine is not published. It also translates the publisher's internal state into an action. The publisher's only snapshot source is the active project's sync host (cli.ts:17011-17016), so a machine with no project registered emits `no_active_sync_scope` and publishes nothing. The user-facing copy now says to open a project rather than repeating "No active sync scope is available." — the raw diagnostic the Connections pane surfaces today. Absent health is treated as pass, not fail: the publisher runs on a 30s heartbeat and a fresh install can outrun its first attempt. `describeUnpublishedMachine` lives in commands/setup.ts rather than cli.ts so it is testable without importing the whole dispatcher. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
arul28
force-pushed
the
ade/ade-windows-connect-setup
branch
from
August 5, 2026 19:56
7b7fe7e to
1f73ddf
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (3)
apps/ade-cli/scripts/install-runtime.ps1 (1)
133-133: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRename
$input; it shadows a PowerShell automatic variable.
$inputis the pipeline enumerator inside a function. Assigning to it is flagged by PSScriptAnalyzer and breaks ifDownload-Assetever accepts pipeline input. Rename it and the matchingDisposecall at line 152.♻️ Proposed fix
- $input = $response.Content.ReadAsStreamAsync().GetAwaiter().GetResult() + $sourceStream = $response.Content.ReadAsStreamAsync().GetAwaiter().GetResult() $output = [IO.File]::Create($Destination) try { @@ - while (($read = $input.Read($buffer, 0, $buffer.Length)) -gt 0) { + while (($read = $sourceStream.Read($buffer, 0, $buffer.Length)) -gt 0) { @@ $output.Dispose() - $input.Dispose() + $sourceStream.Dispose()🤖 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 `@apps/ade-cli/scripts/install-runtime.ps1` at line 133, Rename the stream variable assigned in Download-Asset from $input to a non-reserved name, and update the matching Dispose call to use the new variable consistently.Source: Linters/SAST tools
apps/ade-cli/src/commands/setupDesktop.ts (1)
94-98: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
sha512Base64reads the whole artifact into memory.The desktop artifact is about 1 GB.
fs.readFileSyncallocates that as a single Buffer before hashing, on top of the memory the installer already holds. Stream the file into the hash instead.♻️ Proposed refactor
-export function sha512Base64(filePath: string): string { - const hash = createHash("sha512"); - hash.update(fs.readFileSync(filePath)); - return hash.digest("base64"); -} +export function sha512Base64(filePath: string): string { + const hash = createHash("sha512"); + const buffer = Buffer.allocUnsafe(1024 * 1024); + const handle = fs.openSync(filePath, "r"); + try { + let read = 0; + while ((read = fs.readSync(handle, buffer, 0, buffer.length, null)) > 0) { + hash.update(buffer.subarray(0, read)); + } + } finally { + fs.closeSync(handle); + } + return hash.digest("base64"); +}This keeps the synchronous signature, so
setup.tsline 544 and the existing test need no change.🤖 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 `@apps/ade-cli/src/commands/setupDesktop.ts` around lines 94 - 98, Update sha512Base64 to hash the artifact incrementally with a synchronous file stream instead of loading it via fs.readFileSync. Preserve the existing synchronous string-returning API and Base64 digest behavior so its callers and tests remain unchanged.apps/ade-cli/src/commands/setup.test.ts (1)
35-41: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a color-enabled capability fixture and a regression test for it.
Every rendering test uses
plainwithcolor: false, so the ANSI-colored branch offormatActiveLineandstateSymbolis never exercised. That branch is where the width truncation atsetupRender.tsline 203 miscounts escape bytes. Add acoloredfixture withcolor: trueand assert that a long colored active line keeps its reset sequence intact and stays within the visible column budget.As per coding guidelines: "Record a named regression test or exact alternate verification for every accepted correctness finding."
🤖 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 `@apps/ade-cli/src/commands/setup.test.ts` around lines 35 - 41, Add a color-enabled TerminalCapabilities fixture alongside plain in setup.test.ts, then add a named regression test covering a long colored active line through formatActiveLine/stateSymbol. Assert the rendered output preserves the ANSI reset sequence and its visible width remains within the configured column budget.Source: Coding guidelines
🤖 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 `@apps/ade-cli/scripts/install-runtime.ps1`:
- Around line 120-125: Update the HTTP download setup around Add-Type and
HttpClient so it detects when System.Net.Http or [Net.Http.HttpClient] is
unavailable and falls back to Invoke-WebRequest for the download. Keep the
existing HttpClient path when the type loads successfully, and ensure the
unavailable-type case is handled before constructing the client so it does not
enter the main install rollback path.
In `@apps/ade-cli/scripts/install-runtime.sh`:
- Around line 534-537: Initialize path_profile_updated to 0 before setup_path
can take any early-return branch, then preserve setting it to 1 only when the
profile is successfully appended. Keep the final conditional in the install flow
safe under set -u so it does not evaluate an unset variable.
In `@apps/ade-cli/src/commands/setup.ts`:
- Around line 522-523: Update the artifact staging logic in downloadWithResume
to use a stable cache directory scoped to the artifact/version instead of
creating a fresh mkdtempSync directory per invocation. Preserve partial files
across interrupted runs by removing or conditioning the finally cleanup so the
cache is deleted only after successful completion, while retaining the existing
resume behavior within the download flow.
In `@apps/ade-cli/src/commands/setupDesktop.ts`:
- Around line 114-186: Update downloadWithResume in
apps/ade-cli/src/commands/setupDesktop.ts (lines 114-186) to create an
idle-timeout AbortSignal that resets whenever a response chunk is received, pass
it to fetchImpl, and allow that timeout to enter the retry loop while still
rethrowing the caller-provided abort. Update the manifest fetchImpl call in
apps/ade-cli/src/commands/setup.ts (lines 456-462) to pass an
AbortSignal.timeout(...) so stalled manifest requests fail the desktop step
instead of hanging.
In `@apps/ade-cli/src/commands/setupRender.ts`:
- Around line 197-203: Update the truncation logic in the setup render flow
around the line construction and return so ANSI escape sequences from
stateSymbol/paint do not count toward caps.columns or get split. Measure visible
terminal width and truncate safely at a display-width boundary, preserving
complete color reset sequences; add coverage for colored output while retaining
the existing plain-text behavior.
- Around line 256-258: Update the summary heading construction near the `failed`
count to select the separator based on `caps.unicode`, using the existing ASCII
fallback when Unicode support is unavailable and retaining the em dash for
Unicode-capable terminals.
In `@apps/ade-cli/src/lib/nodeWarnings.ts`:
- Around line 80-82: Update apps/ade-cli/src/lib/nodeWarnings.ts lines 80-82 and
the installation logic to save the original process.emitWarning before wrapping
it; make resetNodeWarningFilterForTests restore that emitter, clear the saved
reference, and reset installed. In apps/ade-cli/src/lib/nodeWarnings.test.ts
lines 4-44, add the named regression test restores process.emitWarning after
resetNodeWarningFilterForTests that resets, captures the emitter, installs the
filter, resets again, and asserts the captured emitter is restored.
---
Nitpick comments:
In `@apps/ade-cli/scripts/install-runtime.ps1`:
- Line 133: Rename the stream variable assigned in Download-Asset from $input to
a non-reserved name, and update the matching Dispose call to use the new
variable consistently.
In `@apps/ade-cli/src/commands/setup.test.ts`:
- Around line 35-41: Add a color-enabled TerminalCapabilities fixture alongside
plain in setup.test.ts, then add a named regression test covering a long colored
active line through formatActiveLine/stateSymbol. Assert the rendered output
preserves the ANSI reset sequence and its visible width remains within the
configured column budget.
In `@apps/ade-cli/src/commands/setupDesktop.ts`:
- Around line 94-98: Update sha512Base64 to hash the artifact incrementally with
a synchronous file stream instead of loading it via fs.readFileSync. Preserve
the existing synchronous string-returning API and Base64 digest behavior so its
callers and tests remain unchanged.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: bf46e19f-abac-4fb5-aaf5-6f676e53064e
⛔ Files ignored due to path filters (1)
docs/features/onboarding-and-settings/README.mdis excluded by!docs/**
📒 Files selected for processing (10)
apps/ade-cli/README.mdapps/ade-cli/scripts/install-runtime.ps1apps/ade-cli/scripts/install-runtime.shapps/ade-cli/src/cli.tsapps/ade-cli/src/commands/setup.test.tsapps/ade-cli/src/commands/setup.tsapps/ade-cli/src/commands/setupDesktop.tsapps/ade-cli/src/commands/setupRender.tsapps/ade-cli/src/lib/nodeWarnings.test.tsapps/ade-cli/src/lib/nodeWarnings.ts
Comment on lines
534
to
537
| if [ "$path_profile_updated" -eq 1 ]; then | ||
| printf 'New terminals will find `ade` on PATH. To use it in this one, run: %s\n' "$(env_file_ref)" | ||
| printf '\n ade is on your PATH in new terminals. To use it in this one, run:\n %s\n' \ | ||
| "$(env_file_ref)" | ||
| fi |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Guard path_profile_updated against being unset.
setup_path sets path_profile_updated=1 only on the successful profile-append path. Several earlier return 0 branches leave it at whatever it was. If it is never initialized, [ "$path_profile_updated" -eq 1 ] prints an integer-expression error at the end of an otherwise successful install.
🛡️ Proposed fix
-if [ "$path_profile_updated" -eq 1 ]; then
+if [ "${path_profile_updated:-0}" -eq 1 ]; then#!/bin/bash
# Check whether path_profile_updated is initialized before use.
set -euo pipefail
rg -n 'path_profile_updated' apps/ade-cli/scripts/install-runtime.sh🤖 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 `@apps/ade-cli/scripts/install-runtime.sh` around lines 534 - 537, Initialize
path_profile_updated to 0 before setup_path can take any early-return branch,
then preserve setting it to 1 only when the profile is successfully appended.
Keep the final conditional in the install flow safe under set -u so it does not
evaluate an unset variable.
…relay A signed-in machine with no registered project could never appear in its owner's ADE account. The account-directory publisher's only snapshot source was a project-scoped sync host (cli.ts getSnapshot -> resolveActiveSyncHost), so on a clean install it returned null, the publisher bailed with `no_active_sync_scope` before any network call, and the machine stayed invisible. Reproduced on a clean Windows box; the code path has no platform branches, so macOS and Linux behaved identically. This was never intended. runServe already treats projectless serving as a supported hosting state -- it takes the machine-wide sync-host lease and binds the shared listener -- and its own comment says such a brain would "bind the port, publish itself, and dial the relay". It bound the port and did neither of the other two, because getMachineOnlySyncStatus returned a hardcoded all-down literal that was a lie in exactly that case. - new projectlessSyncSnapshot builds an honest snapshot when the lease is held and the listener is bound: real port, real pairing connect info, host role. The publisher falls back to it only while genuinely hosting, so `no_active_sync_scope` stays an honest diagnosis otherwise. - new machineRelayTunnel dials the relay on that path, reusing the shared tunnel-client cache key so a project scope booting later adopts the client instead of re-registering and evicting itself. - a headless Linux box with an empty projects.json now publishes and is reachable off-LAN, which is the whole point of the one-liner install. Pairing stays unset by design: account membership is the auth path and the pairing code is a nearby-device fallback. The published endpoints still enforce pairing-store auth, DPoP binding, and account attestation. Also folds in the review findings this surfaced: - one shared per-state advice table replaces hand-mirrored copies in the CLI and the Connections pane that had already drifted; the pane no longer renders the publisher's internal skipReason at users - "open a project" is gone from every surface and doc: this change makes that case publish, so the advice could no longer help anyone - syncRouteHealth extracts route-health derivation shared by syncService and the projectless builder, flattening a six-level nested ternary - desktop install: streamed SHA-512 instead of buffering ~1 GB, a lock over the shared download cache, a working retry on Windows cleanup, and a macOS rollback that can no longer destroy a working app - installers no longer fail a whole `curl | sh` because an optional post-install step flaked Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
CodeRabbit flagged this twice and both review passes refuted it: nothing reaching `formatActiveLine` is coloured today, because `stateSymbol` only paints the `ok` and `failed` symbols and that line passes a literal "active". Measuring with `String.length` is therefore correct -- by coincidence, not by construction. That is a poor invariant to ship. Colour one more component of the line later and it silently truncates early, and a slice through a reset sequence leaves the colour applied to everything printed afterwards. Truncation now counts columns actually occupied and copies escape sequences whole. Writing the test caught a bug in the first version of this: breaking out of the loop once the visible budget was spent dropped the trailing reset, which is precisely the bleed it was meant to prevent. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This was referenced Aug 6, 2026
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
1 participant
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.
The bug
ade connectfailed on every clean install, on Windows and macOS alike.Both installers registered the machine brain with
ade serve --install-service, which inheritsADE_DEFAULT_ROLE. On a fresh machine that is unset, so the brain came up at roleagent— andade connectruns atcto, which anagentbrain can never serve.Every other call site already held this invariant: the desktop app spawns its runtime at
ctoand refuses to attach to a service that isn't (localRuntimeConnectionPool.ts:330,2121), andade brain startpinsctointernally (cli.ts:15816). The installer was the one place that didn't. Both scripts now register throughbrain start— including the PowerShell rollback path, which was restoring the previous service atagenttoo.The experience
The install was also silent and dishonest: ~30 s with no output, raw
node:sqliteExperimentalWarnings as the only proof of life, no progress on a 118 MB runtime or a 1 GB app download, and a cheerful next step printed after sign-in had already failed.The shell scripts now own only what must happen before the
adebinary exists. Everything after isade setup— one TypeScript implementation both platforms hand off to, so the drift that left macOS with a download progress bar and Windows without one cannot recur.open)node:sqlitenotice is filtered at CLI entry; every other warning class still printsReuse
Extends rather than duplicates: the step/summary model from
commands/connect.ts, byte progress fromcommands/tools.ts,readInstalledDesktopVersionfromcommands/doctor.ts, andreleaseAssetUrlfromlib/releaseAssets.ts.Verification
setup.test.ts,nodeWarnings.test.ts), typecheck and build cleanExperimentalWarning, new binary 0tools/manifestfiles this PR never touches — confirmed identical on the clean baselineNot covered: a clean-host install run. It requires deleting
~/.adeon the dev machine, which would kill the ADE session that produced this PR.Known limitations (recorded, not fixed here)
serve --install-servicestill defaults toagentfor direct callersisPackagedElectronCliRuntime()gate). Thectofix means we never reach the state that needs it.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
ade setupcommand to guide installation, account linking, desktop setup, verification, and recovery.ade brain start.Documentation
Bug Fixes