Add ADE brain self-update foundation#726
Merged
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
Contributor
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
Owner
Author
|
@copilot review but do not make fixes |
Copilot stopped work on behalf of
arul28 due to an error
July 7, 2026 22:40
📝 WalkthroughWalkthroughAdds mobile host compatibility signaling and enforcement, a brain runtime self-update command with checksum-verified staging, release packaging changes for standalone runtime artifacts, and desktop runtime version-skew tracking with related UI updates. ChangesMobile Host Compatibility Contract
Brain Runtime Self-Update and Release Packaging
Desktop Runtime Version Skew
Estimated code review effort: 4 (Complex) | ~75 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 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 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.agents/skills/release/SKILL.md (1)
549-586: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winStale "14-asset" count contradicts the 15-item list above.
Line 551 states the expected set is 15 assets and enumerates 15 items (552-566), but line 584 still refers to "the 14-asset set is incomplete." This looks like a leftover from before this PR's asset-count change and will confuse whoever runs this release checklist.
📝 Fix the stale count
-If a referenced file is missing, or the 14-asset set is incomplete → the build +If a referenced file is missing, or the 15-asset set is incomplete → the build or upload broke; re-inspect the `build-mac-release` matrix jobs and the runtime artifact download/publish step.🤖 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 @.agents/skills/release/SKILL.md around lines 549 - 586, Update the release checklist in SKILL.md to remove the stale “14-asset” wording and make the count consistent with the current per-arch matrix. The section around the asset verification guidance should match the 15-item list already described, so change the incomplete-set check to refer to the 15-asset set and keep the rest of the validation logic aligned with the latest-mac.yml and release asset names.
🧹 Nitpick comments (1)
apps/ade-cli/scripts/install-runtime.sh (1)
139-150: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winNon-atomic runtime extraction can leave a broken install on interruption.
rm -rf "$runtime_dir/node_modules"followed directly bytar -xzf ... -C "$runtime_dir"removes the old native deps before the new archive is fully extracted. If extraction is interrupted (disk full, killed process), the runtime is left in a broken state requiring a full re-run.brainUpdate.ts's staged-update flow uses atomic promotion for the same class of operation; consider extracting to a temp directory and renaming into place here too for consistency and resilience.♻️ Suggested atomic extraction pattern
-rm -rf "$runtime_dir/node_modules" -tar -xzf "$tmp_dir/native.tar.gz" -C "$runtime_dir" +staged_runtime_dir="$runtime_dir.staging.$$" +rm -rf "$staged_runtime_dir" +mkdir -p "$staged_runtime_dir" +tar -xzf "$tmp_dir/native.tar.gz" -C "$staged_runtime_dir" +rm -rf "$runtime_dir/node_modules" +mv "$staged_runtime_dir/node_modules" "$runtime_dir/node_modules" +rm -rf "$staged_runtime_dir"🤖 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 139 - 150, The runtime extraction in install-runtime.sh is non-atomic because it deletes "$runtime_dir/node_modules" before unpacking the new archive, so an interrupted install can leave a broken runtime. Update the install flow around the native tar extraction to stage the new node_modules in a temporary directory, verify it there, and then atomically replace the old directory by renaming/promotion into "$runtime_dir/node_modules", following the same staged-update approach used by brainUpdate.ts.
🤖 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/README.md`:
- Around line 329-330: Remove the duplicate `ade sync web` entries from the CLI
surface list in the README so each command appears only once; keep a single
canonical pair of lines for `ade sync web --text` and `ade sync web --open`, and
delete the redundant copy to avoid conflicting descriptions.
In `@apps/ade-cli/src/commands/brainUpdate.ts`:
- Around line 302-306: The update staging directories created by
defaultUpdateStagingDir are left behind after apply completes, so clean them up
in applyStagedBrainUpdate once the staged files are no longer needed. Make sure
the success path and the no-restart-needed path both remove the temporary
directory after promoteStagedNativeRuntime finishes, and preserve cleanup even
if promotion succeeds but the process exits early. Use the existing staging-dir
flow in brainUpdate.ts, especially defaultUpdateStagingDir,
promoteStagedNativeRuntime, and applyStagedBrainUpdate, to add a reliable final
cleanup step.
- Around line 314-348: The defaultDownloadFile request can hang forever because
it only handles "error" and never times out. Update defaultDownloadFile to apply
a timeout to the request returned by resolveDownload(url).get, and make sure the
timeout aborts/rejects the promise with a clear error so staging doesn’t block.
Keep the existing redirect and cleanup flow intact, and ensure the same timeout
behavior covers all callers using defaultDownloadFile.
- Around line 314-343: The redirect handling in defaultDownloadFile currently
accepts any 3xx Location returned by resolveDownload().get, which can downgrade
an HTTPS download to HTTP and weaken update integrity. In the redirect branch,
check the current URL scheme against the redirected URL produced from new
URL(location, url), and reject any redirect that changes from https: to http:.
Keep the fix localized to defaultDownloadFile so both the artifact and
SHA256SUMS fetch paths inherit the same HTTPS-only redirect policy.
In `@apps/desktop/src/main/services/localRuntime/localRuntimeConnectionPool.ts`:
- Around line 1698-1707: The compatibility error mapping in
LocalRuntimeConnectionPool currently treats a compareRuntimeVersionStrings()
result of 0 as runtime_older, which is incorrect for semver-equal versions with
different string forms. Update the LocalRuntimeCompatibilityError construction
in localRuntimeConnectionPool so the status is "unknown" when comparison === 0,
keeping "runtime_newer" and "runtime_older" only for positive and negative
comparisons respectively.
In `@apps/ios/ADE/Views/Settings/SettingsConnectionHeader.swift`:
- Around line 154-167: The compatibilityMessage logic in
SettingsConnectionHeader is producing a singular noun with a plural verb when
missingCount is 1. Update the String returned from compatibilityMessage so the
singular case uses correct verb agreement, while keeping the plural form
unchanged, and verify the branch that handles
hostCompatibilityMissingActions.count still reads naturally for both 1 and many.
---
Outside diff comments:
In @.agents/skills/release/SKILL.md:
- Around line 549-586: Update the release checklist in SKILL.md to remove the
stale “14-asset” wording and make the count consistent with the current per-arch
matrix. The section around the asset verification guidance should match the
15-item list already described, so change the incomplete-set check to refer to
the 15-asset set and keep the rest of the validation logic aligned with the
latest-mac.yml and release asset names.
---
Nitpick comments:
In `@apps/ade-cli/scripts/install-runtime.sh`:
- Around line 139-150: The runtime extraction in install-runtime.sh is
non-atomic because it deletes "$runtime_dir/node_modules" before unpacking the
new archive, so an interrupted install can leave a broken runtime. Update the
install flow around the native tar extraction to stage the new node_modules in a
temporary directory, verify it there, and then atomically replace the old
directory by renaming/promotion into "$runtime_dir/node_modules", following the
same staged-update approach used by brainUpdate.ts.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3f305feb-6fa2-43e2-acaa-eb665abcf604
⛔ Files ignored due to path filters (5)
docs/ARCHITECTURE.mdis excluded by!docs/**docs/features/remote-runtime/README.mdis excluded by!docs/**docs/features/sync-and-multi-device/README.mdis excluded by!docs/**docs/features/sync-and-multi-device/ios-companion.mdis excluded by!docs/**docs/features/sync-and-multi-device/remote-commands.mdis excluded by!docs/**
📒 Files selected for processing (29)
.agents/skills/quality/SKILL.md.agents/skills/quality/references/ade-review-rules.md.agents/skills/release/SKILL.md.agents/skills/test/SKILL.md.github/workflows/release-core.ymlapps/ade-cli/README.mdapps/ade-cli/scripts/install-runtime.shapps/ade-cli/src/cli.tsapps/ade-cli/src/commands/brainUpdate.test.tsapps/ade-cli/src/commands/brainUpdate.tsapps/ade-cli/src/serviceManager/common.test.tsapps/ade-cli/src/serviceManager/common.tsapps/ade-cli/src/services/sync/syncHostService.test.tsapps/ade-cli/src/services/sync/syncHostService.tsapps/desktop/src/main/services/localRuntime/localRuntimeConnectionPool.test.tsapps/desktop/src/main/services/localRuntime/localRuntimeConnectionPool.tsapps/desktop/src/main/services/sync/syncRemoteCommandService.test.tsapps/desktop/src/renderer/components/app/AutoUpdateControl.test.tsxapps/desktop/src/renderer/components/app/AutoUpdateControl.tsxapps/desktop/src/renderer/components/settings/AboutSection.tsxapps/desktop/src/renderer/webclient/adapter/app.tsapps/desktop/src/shared/syncMobileCompatibility.test.tsapps/desktop/src/shared/syncMobileCompatibility.tsapps/desktop/src/shared/types/core.tsapps/desktop/src/shared/types/sync.tsapps/ios/ADE/Services/SyncService.swiftapps/ios/ADE/Views/Settings/ConnectionSettingsView.swiftapps/ios/ADE/Views/Settings/SettingsConnectionHeader.swiftapps/ios/ADETests/ADETests.swift
Owner
Author
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0aa2127478
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Owner
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d8ec0fcde6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Contributor
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
Owner
Author
|
@codex review |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/desktop/src/main/services/localRuntime/localRuntimeConnectionPool.test.ts (1)
1574-1576: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove duplicate
tryConnectproperty declaration.The
internalstype assertion declarestryConnecttwice — this is a copy-paste artifact. While TypeScript allows duplicate property names in type literals (the last wins), it's confusing and should be a single declaration.♻️ Proposed fix
const internals = pool as unknown as { tryConnect: (socketPath: string) => Promise<{ socketPath: string } | null>; - tryConnect: (socketPath: string) => Promise<{ socketPath: string } | null>; };🤖 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/desktop/src/main/services/localRuntime/localRuntimeConnectionPool.test.ts` around lines 1574 - 1576, The `internals` type assertion in `localRuntimeConnectionPool.test.ts` has a duplicate `tryConnect` property in the inline type literal, which should be cleaned up. Update the `pool as unknown as { ... }` cast to keep only one `tryConnect` declaration so the `internals` shape is clear and unambiguous while preserving the existing `tryConnect` usage in the test.
🤖 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.
Nitpick comments:
In
`@apps/desktop/src/main/services/localRuntime/localRuntimeConnectionPool.test.ts`:
- Around line 1574-1576: The `internals` type assertion in
`localRuntimeConnectionPool.test.ts` has a duplicate `tryConnect` property in
the inline type literal, which should be cleaned up. Update the `pool as unknown
as { ... }` cast to keep only one `tryConnect` declaration so the `internals`
shape is clear and unambiguous while preserving the existing `tryConnect` usage
in the test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 371ad7f2-2009-4098-b747-3a693a5d3f05
📒 Files selected for processing (8)
.agents/skills/release/SKILL.mdapps/ade-cli/README.mdapps/ade-cli/scripts/install-runtime.shapps/ade-cli/src/commands/brainUpdate.test.tsapps/ade-cli/src/commands/brainUpdate.tsapps/desktop/src/main/services/localRuntime/localRuntimeConnectionPool.test.tsapps/desktop/src/main/services/localRuntime/localRuntimeConnectionPool.tsapps/ios/ADE/Views/Settings/SettingsConnectionHeader.swift
💤 Files with no reviewable changes (1)
- apps/ade-cli/README.md
🚧 Files skipped from review as they are similar to previous changes (5)
- apps/ios/ADE/Views/Settings/SettingsConnectionHeader.swift
- .agents/skills/release/SKILL.md
- apps/ade-cli/scripts/install-runtime.sh
- apps/desktop/src/main/services/localRuntime/localRuntimeConnectionPool.ts
- apps/ade-cli/src/commands/brainUpdate.ts
|
Codex Review: Didn't find any major issues. Keep them coming! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
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.
Summary
ade brain updateflow that downloads checksummed runtime assets, stages native deps, applies with rollback, records update status, and restarts the brain service.Validation
npm --prefix apps/ade-cli run typechecknpm --prefix apps/ade-cli run testnpm --prefix apps/desktop run typechecknpm run test:desktop:shardednpm --prefix apps/desktop run lint(warnings only, existing)npm --prefix apps/desktop run buildnode scripts/validate-docs.mjssh -n apps/ade-cli/scripts/install-runtime.shdarwin-arm64testSyncServiceKeepsLegacyHelloConnectedInLimitedCompatibilityMode,testSyncServiceReadsExplicitFullMobileCompatibilityFromHello,testFireAndForgetRemoteCommandDropsLocallyWhenHostDoesNotAdvertiseActionSummary by CodeRabbit
Greptile Summary
This PR adds the foundation for headless ADE brain updates and mobile host compatibility. The main changes are:
ade brain updatestaging, checksum verification, apply, rollback, status, and service restart handling.Confidence Score: 5/5
This looks safe to merge.
No blocking issues were found in the changed code. The brain update restart path carries the self-mutation override, and restart failure rolls back promoted assets while recording the failed update state.
No files need attention.
What T-Rex did
Important Files Changed
Comments Outside Diff (1)
General comment
resolveAdeServeCommand > uses node plus the CLI script when argv points at a real script. The test expectedargsto include/home/user/repo/src/cli.ts, but the resolver returned only["serve"], causing the focused CLI validation subset to fail despite typecheck and installer syntax passing.process.argv[1] = path.resolve("src/cli.ts")while Vitest is running from/home/user/repo, resolving to/home/user/repo/src/cli.ts. That path does not exist in this repository layout, soresolveAdeServeCommand()treats the entry as a nonexistent/synthetic script and falls back tocommand: process.execPath, args: ["serve"].apps/ade-cli/src/cli.ts, or create a real temporary script fixture before asserting the node-script branch. If the resolver is intended to locate the package CLI from repository-root invocations, updateresolveAdeServeCommand()accordingly and add a regression test for that path.Reviews (4): Last reviewed commit: "ship: address runtime promotion review f..." | Re-trigger Greptile