Reliability follow-ups: referenced lifecycle timers, shared-signal token exchange#891
Merged
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
📝 WalkthroughWalkthroughChangesRuntime diagnostics and reliability
Desktop runtime and updates
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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: 2
🧹 Nitpick comments (6)
apps/account-directory/wrangler.jsonc (1)
8-8: 🚀 Performance & Scalability | 🔵 TrivialConfirm 100% trace sampling is intentional.
head_sampling_rate: 1traces every request/invocation (including the once-a-minute cron). This gives full observability but increases ingested trace volume/cost proportionally to traffic. Worth a deliberate call now rather than an accidental cost surprise later.🤖 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/account-directory/wrangler.jsonc` at line 8, Confirm that head_sampling_rate in the observability configuration is intentionally set to 1 for 100% trace sampling; if full sampling is not explicitly required, replace it with the approved lower sampling rate while preserving observability for scheduled and request-driven invocations.scripts/posthog/dashboard-spec.mjs (1)
510-525: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider whether
UPDATE_AUTO_APPLY_CANCELLEDbelongs in this incident chart.The new "reliability-incidents" insight tracks install-aborted, quit-escalated, and auto-applied outcomes, but omits
UPDATE_AUTO_APPLY_CANCELLED. If cancellations are a signal worth watching alongside aborts/escalations, consider adding it; if intentionally excluded (user-initiated, not a failure), no change needed.🤖 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 `@scripts/posthog/dashboard-spec.mjs` around lines 510 - 525, Decide whether UPDATE_AUTO_APPLY_CANCELLED represents a reliability incident alongside the existing update outcome events in the reliability-incidents insight. If it should be tracked, add eventNode(UPDATE_AUTO_APPLY_CANCELLED, ...) to the series; otherwise leave the chart unchanged and preserve the current intentional exclusion.apps/desktop/src/shared/types/productAnalytics.ts (1)
13-18: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffEvent catalog is duplicated across three independent sources with no shared source of truth. All three files currently agree on the six new
ade_update_*/ade_brain_recovered/ade_publish_failingevent names, but each maintains its own hand-typed copy, so a future rename/typo in any one of them would silently desync the desktop allowlist from the PostHog dashboard spec.
apps/desktop/src/shared/types/productAnalytics.ts#L13-L18: canonical TS event-name union — the natural anchor if a shared source were introduced.apps/desktop/src/main/services/analytics/productAnalyticsPolicy.ts#L16-L21: could import event names fromproductAnalytics.tsinstead of re-listing them.scripts/posthog/dashboard-spec.mjs#L14-L19: a standalone JS script (different language/build target) so full unification may require a generated/shared JSON manifest rather than a direct import.🤖 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/shared/types/productAnalytics.ts` around lines 13 - 18, Introduce one shared event-name manifest as the source of truth for the catalog, using productAnalytics.ts as the canonical type anchor. Update productAnalyticsPolicy.ts to derive or import its allowlist from that manifest, and update scripts/posthog/dashboard-spec.mjs to consume the same manifest through a compatible generated or shared JSON format; remove the duplicated hand-typed event lists at all three sites.apps/ade-cli/src/commands/doctor.ts (1)
540-559: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
DEFAULT_SYNC_HOST_PORTinstead of the literal8787.The rest of the file (including the
portDiagnosesgating at Line 707) keys offDEFAULT_SYNC_HOST_PORT, but this row hardcodes8787in both theokcondition and the detail strings. If the default ever changes,syncPortRowwould diverge from the diagnosis logic and mislabel the bound port.♻️ Proposed change
- if (input.syncPort === 8787) { + if (input.syncPort === DEFAULT_SYNC_HOST_PORT) { return { key: "sync_port", label: "Sync port", status: "ok", - detail: "bound on 8787", + detail: `bound on ${DEFAULT_SYNC_HOST_PORT}`, }; } const holders = input.portDiagnoses.flatMap((diagnosis) => diagnosis.holders.map((holder) => `${diagnosis.port}: pid ${holder.pid}${holder.command ? ` ${holder.command}` : ""}`, ), ); return { key: "sync_port", label: "Sync port", status: "warn", - detail: `bound on ${input.syncPort} instead of 8787${ + detail: `bound on ${input.syncPort} instead of ${DEFAULT_SYNC_HOST_PORT}${🤖 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/doctor.ts` around lines 540 - 559, Update the syncPortRow logic to use DEFAULT_SYNC_HOST_PORT instead of the hardcoded 8787 in the equality check and all related detail strings, keeping the existing status and holder-reporting behavior unchanged.apps/ade-cli/src/multiProjectRpcServer.ts (1)
1060-1065: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated
runtimeInfoextras block across two handlers.The
minCompatibleProtocol/protocolVersion/multiProject/pid/uptimeMsblock is duplicated verbatim in theade/initializeandruntime/info/machineInfo.gethandlers. Since both already callresolveRuntimeStatus()andresolveRuntimeEnvInfo(), consider factoring this into a small shared helper (e.g.buildRuntimeInfoExtras()) to avoid the two sites drifting out of sync on a future change (e.g. ifprotocolVersionsemantics oruptimeMsrounding is later revisited).♻️ Proposed refactor
+ const buildRuntimeInfoExtras = () => ({ + ...resolveRuntimeStatus(), + minCompatibleProtocol: RUNTIME_COMPAT_LEVEL, + protocolVersion: RUNTIME_COMPAT_LEVEL, + multiProject: true, + pid: process.pid, + uptimeMs: Math.max(0, Math.round(process.uptime() * 1_000)), + }); + // ade/initialize: runtimeInfo: { name: "ade-rpc", version: options.serverVersion, ...resolveRuntimeEnvInfo(), - ...resolveRuntimeStatus(), - minCompatibleProtocol: RUNTIME_COMPAT_LEVEL, - protocolVersion: RUNTIME_COMPAT_LEVEL, - multiProject: true, - pid: process.pid, - uptimeMs: Math.max(0, Math.round(process.uptime() * 1_000)), + ...buildRuntimeInfoExtras(), }, // runtime/info / machineInfo.get: runtimeInfo: { name: "ade-rpc", version: options.serverVersion, ...envInfo, - ...resolveRuntimeStatus(), - minCompatibleProtocol: RUNTIME_COMPAT_LEVEL, - protocolVersion: RUNTIME_COMPAT_LEVEL, - multiProject: true, - pid: process.pid, - uptimeMs: Math.max(0, Math.round(process.uptime() * 1_000)), + ...buildRuntimeInfoExtras(), },Also applies to: 1116-1121
🤖 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/multiProjectRpcServer.ts` around lines 1060 - 1065, Factor the duplicated runtime metadata object out of the ade/initialize and runtime/info/machineInfo.get handlers into a shared helper such as buildRuntimeInfoExtras(). Have both handlers spread the helper result alongside resolveRuntimeStatus() and resolveRuntimeEnvInfo(), preserving the existing minCompatibleProtocol, protocolVersion, multiProject, pid, and uptimeMs values and uptime rounding.apps/desktop/src/main/services/localRuntime/localRuntimeConnectionPool.ts (1)
1325-1327: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winBound
activitySummary()with an explicit RPC timeout.
activitySummary()is used bycreateAutoUpdateServicethroughlocalRuntimePool.activitySummary(), and that call path has no Promise timeout, timebox, orPromise.racearoundgetRuntimeActivitySummary. SincecallSync()currently maps directly toentry.client.call(method, params)without options, rely on the RPC client default rather than a short activity-check bound; add an explicit short timeout, or at least use/define one inautoUpdateService.tsfor this idle poll.🤖 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.ts` around lines 1325 - 1327, Update localRuntimeConnectionPool.activitySummary() to enforce an explicit short RPC timeout when calling runtime.activitySummary, using an existing timeout constant or a clearly defined idle-poll timeout from autoUpdateService.ts. Ensure the timeout is passed through the callSync RPC options while preserving the RuntimeActivitySummary return 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 `@apps/desktop/src/main/services/github/adeReleaseFeed.ts`:
- Around line 24-50: Update fetchAdeLatestRelease so its default global fetch
path uses a bounded abort signal, matching the timeout pattern in
githubService.ts. Apply the signal only when no custom fetchImpl is supplied,
preserving custom fetch behavior and the existing response/error handling.
In `@apps/desktop/src/main/services/updates/autoUpdateService.ts`:
- Around line 1039-1080: Update abortInstall to seed autoApplySuppressedUntil
when the parked abort reason represents a refresh, handoff, preflight, or
timeout/setup failure, preventing idle auto-apply from restarting during the
suppression window. Preserve normal ready-state cleanup for other aborts, and
explicitly clear the suppression timestamp for user-initiated cancellation.
---
Nitpick comments:
In `@apps/account-directory/wrangler.jsonc`:
- Line 8: Confirm that head_sampling_rate in the observability configuration is
intentionally set to 1 for 100% trace sampling; if full sampling is not
explicitly required, replace it with the approved lower sampling rate while
preserving observability for scheduled and request-driven invocations.
In `@apps/ade-cli/src/commands/doctor.ts`:
- Around line 540-559: Update the syncPortRow logic to use
DEFAULT_SYNC_HOST_PORT instead of the hardcoded 8787 in the equality check and
all related detail strings, keeping the existing status and holder-reporting
behavior unchanged.
In `@apps/ade-cli/src/multiProjectRpcServer.ts`:
- Around line 1060-1065: Factor the duplicated runtime metadata object out of
the ade/initialize and runtime/info/machineInfo.get handlers into a shared
helper such as buildRuntimeInfoExtras(). Have both handlers spread the helper
result alongside resolveRuntimeStatus() and resolveRuntimeEnvInfo(), preserving
the existing minCompatibleProtocol, protocolVersion, multiProject, pid, and
uptimeMs values and uptime rounding.
In `@apps/desktop/src/main/services/localRuntime/localRuntimeConnectionPool.ts`:
- Around line 1325-1327: Update localRuntimeConnectionPool.activitySummary() to
enforce an explicit short RPC timeout when calling runtime.activitySummary,
using an existing timeout constant or a clearly defined idle-poll timeout from
autoUpdateService.ts. Ensure the timeout is passed through the callSync RPC
options while preserving the RuntimeActivitySummary return behavior.
In `@apps/desktop/src/shared/types/productAnalytics.ts`:
- Around line 13-18: Introduce one shared event-name manifest as the source of
truth for the catalog, using productAnalytics.ts as the canonical type anchor.
Update productAnalyticsPolicy.ts to derive or import its allowlist from that
manifest, and update scripts/posthog/dashboard-spec.mjs to consume the same
manifest through a compatible generated or shared JSON format; remove the
duplicated hand-typed event lists at all three sites.
In `@scripts/posthog/dashboard-spec.mjs`:
- Around line 510-525: Decide whether UPDATE_AUTO_APPLY_CANCELLED represents a
reliability incident alongside the existing update outcome events in the
reliability-incidents insight. If it should be tracked, add
eventNode(UPDATE_AUTO_APPLY_CANCELLED, ...) to the series; otherwise leave the
chart unchanged and preserve the current intentional exclusion.
🪄 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 Plus
Run ID: fb1b6f34-db7a-4ef7-ab93-2edc47a3e213
⛔ Files ignored due to path filters (7)
docs/ARCHITECTURE.mdis excluded by!docs/**docs/features/onboarding-and-settings/README.mdis excluded by!docs/**docs/features/onboarding-and-settings/desktop-auto-update.mdis excluded by!docs/**docs/features/remote-runtime/README.mdis excluded by!docs/**docs/features/storage-and-recovery/README.mdis excluded by!docs/**docs/features/sync-and-multi-device/README.mdis excluded by!docs/**docs/logging.mdis excluded by!docs/**
📒 Files selected for processing (81)
apps/account-directory/wrangler.jsoncapps/ade-cli/README.mdapps/ade-cli/src/adeRpcServer.tsapps/ade-cli/src/bootstrap.tsapps/ade-cli/src/cli.test.tsapps/ade-cli/src/cli.tsapps/ade-cli/src/commands/brainUpdate.tsapps/ade-cli/src/commands/doctor.test.tsapps/ade-cli/src/commands/doctor.tsapps/ade-cli/src/multiProjectRpcServer.test.tsapps/ade-cli/src/multiProjectRpcServer.tsapps/ade-cli/src/serviceManager/common.test.tsapps/ade-cli/src/serviceManager/common.tsapps/ade-cli/src/serviceManager/index.tsapps/ade-cli/src/serviceManager/installLaunchd.tsapps/ade-cli/src/services/account/accountAuthService.test.tsapps/ade-cli/src/services/account/accountAuthService.tsapps/ade-cli/src/services/account/accountMachinePublisherService.test.tsapps/ade-cli/src/services/account/accountMachinePublisherService.tsapps/ade-cli/src/services/personalChats/personalChatScope.tsapps/ade-cli/src/services/projects/projectScope.tsapps/ade-cli/src/services/runtime/brainFreshnessMonitor.test.tsapps/ade-cli/src/services/runtime/brainFreshnessMonitor.tsapps/ade-cli/src/services/runtime/brainLogger.test.tsapps/ade-cli/src/services/runtime/brainLogger.tsapps/ade-cli/src/services/runtime/brainLoopWatchdog.test.tsapps/ade-cli/src/services/runtime/brainLoopWatchdog.tsapps/ade-cli/src/services/runtime/runtimeBuildIdentity.tsapps/ade-cli/src/services/sync/abortSignal.tsapps/ade-cli/src/services/sync/sharedSyncListener.tsapps/ade-cli/src/services/sync/syncHostService.test.tsapps/ade-cli/src/services/sync/syncHostService.tsapps/ade-cli/src/services/sync/syncHostSingleton.tsapps/ade-cli/src/services/sync/syncLoopbackCollision.test.tsapps/ade-cli/src/services/sync/syncPairingConnectInfo.tsapps/ade-cli/src/services/sync/syncProtocol.tsapps/ade-cli/src/services/sync/syncRemoteCommandService.test.tsapps/ade-cli/src/services/sync/syncRemoteCommandService.tsapps/ade-cli/src/services/sync/syncService.tsapps/desktop/src/main/main.tsapps/desktop/src/main/services/analytics/productAnalyticsPolicy.tsapps/desktop/src/main/services/analytics/productAnalyticsService.test.tsapps/desktop/src/main/services/github/adeReleaseFeed.tsapps/desktop/src/main/services/github/githubService.test.tsapps/desktop/src/main/services/github/githubService.tsapps/desktop/src/main/services/ipc/registerIpc.tsapps/desktop/src/main/services/localRuntime/localRuntimeConnectionPool.test.tsapps/desktop/src/main/services/localRuntime/localRuntimeConnectionPool.tsapps/desktop/src/main/services/logging/logger.tsapps/desktop/src/main/services/remoteRuntime/syncPairedMachineStore.test.tsapps/desktop/src/main/services/remoteRuntime/syncPairedMachineStore.tsapps/desktop/src/main/services/updates/autoUpdateService.test.tsapps/desktop/src/main/services/updates/autoUpdateService.tsapps/desktop/src/main/services/updates/autoUpdateVersions.tsapps/desktop/src/preload/global.d.tsapps/desktop/src/preload/preload.tsapps/desktop/src/renderer/browserMock.tsapps/desktop/src/renderer/components/app/AppShell.tsxapps/desktop/src/renderer/components/app/AutoUpdateBanner.test.tsxapps/desktop/src/renderer/components/app/AutoUpdateBanner.tsxapps/desktop/src/renderer/components/app/AutoUpdateControl.test.tsxapps/desktop/src/renderer/components/app/AutoUpdateControl.tsxapps/desktop/src/renderer/components/app/BrainRecoveryNotice.test.tsapps/desktop/src/renderer/components/app/BrainRecoveryNotice.tsxapps/desktop/src/renderer/components/app/useAutoUpdateSnapshot.tsapps/desktop/src/renderer/components/remoteTargets/RemoteTargetList.tsxapps/desktop/src/renderer/components/remoteTargets/remoteMachineModel.test.tsapps/desktop/src/renderer/components/remoteTargets/remoteMachineModel.tsapps/desktop/src/renderer/components/settings/AboutSection.test.tsapps/desktop/src/renderer/components/settings/AboutSection.tsxapps/desktop/src/renderer/webclient/adapter/app.tsapps/desktop/src/renderer/webclient/adapter/index.tsapps/desktop/src/shared/adeRuntimeProtocol.tsapps/desktop/src/shared/ipc.tsapps/desktop/src/shared/sync/adoptChannelCrypto.test.tsapps/desktop/src/shared/sync/adoptChannelCrypto.tsapps/desktop/src/shared/types/core.tsapps/desktop/src/shared/types/productAnalytics.tsapps/desktop/src/shared/types/sync.tsscripts/posthog/dashboard-spec.mjsscripts/posthog/provision.test.mjs
Comment on lines
+1039
to
+1080
| async function abortInstall( | ||
| reason: AutoUpdateInstallAbortReason, | ||
| readySnapshot: AutoUpdateSnapshot, | ||
| ): Promise<false> { | ||
| clearQuitDeadline(); | ||
| installQuitArmed = false; | ||
| clearPendingInstallUpdate(); | ||
| resetAutoApplyTracking(true); | ||
| try { | ||
| await rollbackQuitAndInstall?.(reason); | ||
| } catch (error) { | ||
| logger.error("autoUpdate.install_rollback_failed", { | ||
| reason, | ||
| message: formatErrorMessage(error), | ||
| }); | ||
| } | ||
| logger.warn("autoUpdate.install_aborted", { | ||
| reason, | ||
| version: readySnapshot.version, | ||
| }); | ||
| productAnalyticsService?.captureInternal({ | ||
| event: "ade_update_install_aborted", | ||
| surface: "desktop", | ||
| properties: { reason }, | ||
| }); | ||
| patchSnapshot({ | ||
| ...readySnapshot, | ||
| status: "ready", | ||
| latestKnownVersion: snapshot.latestKnownVersion ?? readySnapshot.latestKnownVersion, | ||
| progressPercent: 100, | ||
| bytesPerSecond: null, | ||
| transferredBytes: null, | ||
| totalBytes: null, | ||
| error: null, | ||
| errorDetails: null, | ||
| parked: { reason, at: nowMs() }, | ||
| autoApplyPending: null, | ||
| }); | ||
| installReadySnapshot = null; | ||
| scheduleActivityCheck(0); | ||
| return false; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate and inspect the relevant service and surrounding idle/suppression logic.
WD="$(pwd)"
echo "PWD=$WD"
fd -a 'autoUpdateService\.ts$' . || true
FILE="$(fd 'autoUpdateService\.ts$' . | head -n 1)"
if [ -n "${FILE:-}" ]; then
echo "FILE=$FILE"
wc -l "$FILE"
sed -n '1,120p' "$FILE"
echo "-------------- lines 960-1120 --------------"
sed -n '960,1120p' "$FILE"
echo "-------------- scheduleActivityCheck/autoApplySuppressedUntil/refresh/handoff usages --------------"
rg -n 'scheduleActivityCheck|autoApplySuppressedUntil|autoApplyIdleMs|autoApply.*Suppressed|refresh_failed|handoff_failed|roll|InstallAbortReason|pause|cancel|abortInstall|parked|quitAndInstall' "$FILE"
fiRepository: arul28/ADE
Length of output: 12981
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="./apps/desktop/src/main/services/updates/autoUpdateService.ts"
echo "-------------- lines 1030-1225 --------------"
sed -n '1030,1225p' "$FILE"
echo "-------------- focused occurrences with context --------------"
python3 - <<'PY'
from pathlib import Path
p = Path('./apps/desktop/src/main/services/updates/autoUpdateService.ts')
lines = p.read_text().splitlines()
targets = [
'function resetAutoApplyTracking',
'function clearPendingAutoApply',
'function scheduleActivityCheck',
'async function checkAutoApplyActivity',
'async function abortInstall',
'function cancelAutoApply',
'"refresh_failed"',
'"handoff_failed"',
'"install_preflight_failed"',
'"prepare_timeout"',
'"prepare_failed"',
'"cancelled"',
]
for t in targets:
print(f"## {t}")
for i,l in enumerate(lines, start=1):
if t in l:
start=max(1,i-8); end=min(len(lines),i+8)
for n in range(start,end+1):
print(f"{n}:{lines[n-1]}")
print()
PYRepository: arul28/ADE
Length of output: 15879
Suppress idle auto-apply after parked install aborts.
When the install abort is due to a failed refresh/handoff/preflight/timeout, abortInstall leaves the update as ready with parked but resets auto-apply tracking and schedules the next activity check immediately. Repeated failures will re-enter the idle countdown every autoApplyIdleMs and retry quitAndInstall until the native quit succeeds. Seed autoApplySuppressedUntil on parked aborts for refresh/handoff/setup failures, and clear it on explicit user cancel instead of leaving suppressions to that path alone.
🤖 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/updates/autoUpdateService.ts` around lines
1039 - 1080, Update abortInstall to seed autoApplySuppressedUntil when the
parked abort reason represents a refresh, handoff, preflight, or timeout/setup
failure, preventing idle auto-apply from restarting during the suppression
window. Preserve normal ready-state cleanup for other aborts, and explicitly
clear the suppression timestamp for user-initiated cancellation.
…change, durable env single-flight Follow-ups to #889 from the final review round: awaited termination/handover timers stay referenced so a standalone CLI can't exit mid-repair; the session token exchange runs on the service-owned shared signal as intended; the env refresh single-flight is released by the shared exchange settling, not by an aborting caller. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
arul28
force-pushed
the
ade/start-skill-both-macbook-mac-01881577
branch
from
July 24, 2026 03:30
8036cc1 to
1ea6038
Compare
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.
Three verified P1s from PR #889's final review round, fixed post-merge before the release:
ade serve --install-serviceexit before SIGKILL escalation andlaunchctl loadwhile repairing a wedged brain.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
ade doctorhealth checks with readable status tables, JSON output, and optional online version checks.Bug Fixes
Greptile Summary
This PR updates service lifecycle waits and account token refresh cancellation. The main changes are:
Confidence Score: 4/5
One contained account refresh bug should be fixed before merging.
The lifecycle timer changes are narrow and safe. The account refresh changes mostly preserve shared cancellation, but one env-token fallback can still let an aborted caller fail shared work.
apps/ade-cli/src/services/account/accountAuthService.ts
What T-Rex did
Important Files Changed
Sequence Diagram
Comments Outside Diff (1)
apps/ade-cli/src/services/account/accountAuthService.ts, line 1803-1806 (link)This fallback re-enters
getAccessTokenWithSignalwith the initiating caller'ssignal. When the env credential changes during the shared refresh after that caller has aborted, the sharedrefreshPromiserejects with that caller's abort instead of consuming the newer credential for other waiters. Use the shared signal for this internal retry so only the shared timeout controls it.Prompt To Fix With AI
Prompt To Fix All With AI
Reviews (2): Last reviewed commit: "fix(reliability): referenced lifecycle t..." | Re-trigger Greptile